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

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "chengzhihao1" <chengzhihao1 at huawei.com>
> An: "Rickard X Andersson" <rickard.andersson at axis.com>, "richard" <richard at nod.at>, "linux-mtd"
> 在 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.

Please also run checkpatch.pl, there are some coding style issues.
Overall the change looks good to me and can be merged soon!

>> 
>> 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)))' ?

Well, this is not a hot path. What is the benefit of an unlikely() here?

>> +		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'.

Depending on the architecture, access_ok() can bit rather expensive.
That's why using access_ok() once on the whole range and then using
the cheap __put_user() in loops is a common pattern.

> 
> 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;