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

[Zhihao Cheng]

在 2024/12/7 16:06, 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?

Maybe the 'unlikely' is not needed, I'm not sure whether the user will 
call this ioctl frequently. I'm also fine without the 'unlikely'.
> 
>>> +		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.

Make sense. please keep using access_ok() once and then invoking 
__put_user() in loop.
> 
>>
>> 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;
>> }