Re: [PATCH 1/2] UBI: replace MTD_UBI_BEB_LIMIT with module parameter

[Richard Genoud]

2012/8/17 Artem Bityutskiy <dedekind1@xxxxxxxxx>:
> Richard, would you please split this series differently:
>
> 1. Separate out the calculations to the get_bad_peb_limit() func.
> 2. Invent
> 2. Add the module parameter
> 3. Extends the ioctl
> 4. Removes the Kconfig option
>
> This will be much easier to review.
ok, will do that.

>
> See also some comments below.
>
>> --- a/drivers/mtd/ubi/build.c
>> +++ b/drivers/mtd/ubi/build.c
>> @@ -46,6 +46,8 @@
>>  /* Maximum length of the 'mtd=' parameter */
>>  #define MTD_PARAM_LEN_MAX 64
>>
>> +#define MTD_PARAM_NB_MAX 3
>
> Please, make it to be
>
> /* Maximum number of comma-separated items in ht 'mtd=' parameter */
> #define MTD_PARAM_MAX_COUNT 3
>
> instead.
done.
>
>> @@ -57,10 +59,12 @@
>>   * @name: MTD character device node path, MTD device name, or MTD device number
>>   *        string
>>   * @vid_hdr_offs: VID header offset
>> + * @max_beb_per1024: maximum expected number of bad blocks per 1024 erase blocks
>>   */
>>  struct mtd_dev_param {
>>       char name[MTD_PARAM_LEN_MAX];
>>       int vid_hdr_offs;
>> +     unsigned int max_beb_per1024;
>>  };
>
> Please, make it to be just 'int' here and everywhere, just for
> consistency with other parameters, which are 'int' (no other stronger
> reason).
ok, I put it back to an int.

>>  MODULE_PARM_DESC(mtd, "MTD devices to attach. Parameter format: "
>> -                   "mtd=<name|num|path>[,<vid_hdr_offs>].\n"
>> +                   "mtd=<name|num|path>[,<vid_hdr_offs>[,max_beb_per1024]].\n"
>>                     "Multiple \"mtd\" parameters may be specified.\n"
>>                     "MTD devices may be specified by their number, name, or "
>>                     "path to the MTD character device node.\n"
>>                     "Optional \"vid_hdr_offs\" parameter specifies UBI VID "
>>                     "header position to be used by UBI.\n"
>
> This comment needs improvement. Consider a situation when I do not want
> to specify vid_hdr_offs (want to use the default), but want to specify
> max_beb_per1024. I think I can put 0 here which means "default". Would
> you please verify this and add a comment about this in this help output.
you're right, if vid_hdr_offs is 0, the default value is taken.
I'll improve the comments accordingly.
> Also, please, verify that the output looks OK using 'modinfo ubi'.
ok.
>> +                   "Optional \"max_beb_per1024\" parameter specifies the "
>> +                   "maximum expected bad eraseblock per 1024 eraseblocks."
>> +                   "(default " __stringify(MTD_UBI_DEFAULT_BEB_LIMIT)
>> +                   " if 0 or not set)\n"
>
> Yeah, something like "if 0 or not set" is needed for 'vid_hdr_offs' as
> well.
>
>>                     "Example 1: mtd=/dev/mtd0 - attach MTD device "
>>                     "/dev/mtd0.\n"
>>                     "Example 2: mtd=content,1984 mtd=4 - attach MTD device "
>
> Could you also modify one of the examples and add "max_beb_per1024"
> there?
yes, of course.

> --
> Best Regards,
> Artem Bityutskiy



-- 
for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/