[PATCH 1/2] UBI: replace MTD_UBI_BEB_LIMIT with module parameter
Artem Bityutskiy
dedekind1 at gmail.com
Fri Aug 17 04:22:48 EDT 2012
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.
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.
> @@ -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).
> 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.
Also, please, verify that the output looks OK using 'modinfo ubi'.
> + "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?
--
Best Regards,
Artem Bityutskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-mtd/attachments/20120817/124f6ffc/attachment.sig>
More information about the linux-mtd
mailing list