[PATCH v3 2/2] ubi: Implement ioctl for detailed erase counters

[Zhihao Cheng]

在 2024/12/6 19:51, Rickard Andersson 写道:
> Currently, "max_ec" can be read from sysfs, which provides a limited
> view of the flash device’s wear. In certain cases, such as bugs in
> the wear-leveling algorithm, specific blocks can be worn down more
> than others, resulting in uneven wear distribution. Also some use cases
> can wear the erase blocks of the fastmap area more heavily than other
> parts of flash.
> Providing detailed erase counter values give a better understanding of
> the overall flash wear and is needed to be able to calculate for example
> expected life time.
> There exists more detailed info in debugfs, but this information is
> only available for debug builds.
> 
> Signed-off-by: Rickard Andersson <rickard.andersson at axis.com>
> ---
>   drivers/mtd/ubi/cdev.c | 72 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 72 insertions(+)

Hi Rickard. Some small nits below.
> 
> diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
> index 0d8f04cf03c5..bcdc47374578 100644
> --- a/drivers/mtd/ubi/cdev.c
> +++ b/drivers/mtd/ubi/cdev.c
> @@ -828,6 +828,72 @@ static int rename_volumes(struct ubi_device *ubi,
>   	return err;
>   }
>   
> +static int ubi_get_ec_info(struct ubi_device *ubi, struct ubi_ecinfo_req __user *ureq)
> +{
> +	struct ubi_ecinfo_req req;
> +	struct ubi_wl_entry *wl;
> +	int i;

We can use a obvious name like 'read_cnt' or 'read_length', instead of 'i'.
> +	int peb;
> +	int end_peb;
> +
> +	/* Copy the input arguments */
> +	if (copy_from_user(&req, ureq, sizeof(struct ubi_ecinfo_req)))
> +		return -EFAULT;
> +
> +	/* Check input arguments */
> +	if (req.length <= 0 || req.start < 0)

How about 'if (req.length <= 0 || req.start < 0 || req.start >= 
ubi->peb_count)'? Then 'if (req.start > end_peb)' below can be deleted.
> +		return -EINVAL;
> +
> +	if (check_add_overflow(req.start, req.length, &end_peb))

How about 'if (unlikely(check_add_overflow(req.start, req.length, 
&end_peb)))' ?
> +		return -EINVAL;
> +
> +	if (end_peb > ubi->peb_count)
> +		end_peb = ubi->peb_count;
> +
> +	if (req.start > end_peb)

This check can be removed.
> +		return -EINVAL;
> +
> +	/* Check access rights before filling erase_counters array */
> +	if (!access_ok(ureq->erase_counters, (end_peb-req.start) * sizeof(int32_t)))
> +		return -EFAULT;

We can use 'put_user' below to replace the check 'access_ok'.

for (peb = req.start; peb < end_peb; i++, peb++) {
	int ec = UBI_UNKNOWN;

	if (!ubi_io_is_bad(ubi, peb)) {
		spin_lock(&ubi->wl_lock);
		wl = ubi->lookuptbl[peb];
		if (wl)
			ec = wl->ec;
		else
			ec = UBI_UNKNOWN;
		spin_unlock(&ubi->wl_lock);
	}

	if (unlikely(put_user(ec, ureq->erase_counters+i)))
		return -EFAULT;
}

> +
> +	/* Fill erase counter array */
> +	i = 0;
> +	for (peb = req.start; peb < end_peb; i++, peb++) {
> +		int ec;
> +
> +		if (ubi_io_is_bad(ubi, peb)) {
> +			if (__put_user(UBI_UNKNOWN, ureq->erase_counters+i))
> +				return -EFAULT;
> +
> +			continue;
> +		}
> +
> +		spin_lock(&ubi->wl_lock);
> +
> +		wl = ubi->lookuptbl[peb];
> +		if (wl)
> +			ec = wl->ec;
> +		else
> +			ec = UBI_UNKNOWN;
> +
> +		spin_unlock(&ubi->wl_lock);
> +
> +		if (__put_user(ec, ureq->erase_counters+i))
> +			return -EFAULT;
> +
> +	}


> +
> +	/* Return actual read length */
> +	req.read_length = i;
> +
> +	/* Copy everything except erase counter array */
> +	if (copy_to_user(ureq, &req, sizeof(struct ubi_ecinfo_req)))
> +		return -EFAULT;

if (unlikely(...
> +
> +	return 0;
> +}
> +
>   static long ubi_cdev_ioctl(struct file *file, unsigned int cmd,
>   			   unsigned long arg)
>   {
> @@ -991,6 +1057,12 @@ static long ubi_cdev_ioctl(struct file *file, unsigned int cmd,
>   		break;
>   	}
>   
> +	case UBI_IOCECNFO:
> +	{
> +		err = ubi_get_ec_info(ubi, argp);
> +		break;
> +	}
> +
>   	default:
>   		err = -ENOTTY;
>   		break;
>