[PATCH 4/4] UBI: replace MTD_UBI_BEB_LIMIT with user-space parameter

[Richard Genoud]

2012/8/16 Shmulik Ladkani <shmulik.ladkani at gmail.com>:
> Hi Richard,
>
> Sorry for reviewing this late...
>
> On Tue, 10 Jul 2012 18:23:42 +0200 Richard Genoud <richard.genoud at gmail.com> wrote:
>> -config MTD_UBI_BEB_LIMIT
>> -     int "Maximum expected bad eraseblocks per 1024 eraseblocks"
>> -     default 20
>> -     range 2 256
>
> I see some benefit keeping the config.
>
> For the simplest systems (those having one ubi device) that need a limit
> *other* than the default (20 per 1024), they can simply set the config
> to their chosen value, as they were used to.
>
> With you approach, these system MUST pass the limit parameter via the
> ioctl / module-parameter.
That's right.
We can add a kernel config option to change the max_beb_per1024
default value (actually, this is almost the patch I send first).
But I see something disturbing with that:
It means that an ubi_attach call from userspace, without specifying
max_beb_per1024, won't have the same result, depending of the default
config value the kernel has been compiled with.
(Or maybe this behavior is acceptable).

>> +static int get_bad_peb_limit(const struct ubi_device *ubi, int max_beb_per1024)
>> +{
>> +     int device_peb_count;
>> +     uint64_t device_size;
>> +     int beb_limit = 0;
>> +
>> +     /* this has already been checked in ioctl */
>> +     if (max_beb_per1024 <= 0)
>> +             goto out;
>
> Can you explain how 'max_beb_per1024 <= 0' may happen?
>
> It seems that all of your calls to 'ubi_attach_mtd_dev' pass a positive
> max_beb_per1024 value (the default is always set). See your
> 'ubi_mtd_param_parse' and 'ctrl_cdev_ioctl'. Am I missing something?
Actually it can. But it's because I made a mistake:
p->max_beb_per1024 = bytes_str_to_int(tokens[2]);
=> I didn't check the return value of this. It can fail, and if it
does the return value is >0.
I'm going to change that.

>
> Also, since max_beb_per1024 is always set, how one may specify a zero
> limit?
You can't.
Do you think we need that ?
0 should be kept for "default value", not to break user-space.
But we can use another value for 0, like 1024 or 2048.
(but honestly, I think this will add complexity for an unlikely configuration.)

>
> Regards,
> Shmulik

Regards,
Richard.


-- 
for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?