linux-kernel - Re: [PATCH 1/2] UBI: replace MTD_UBI_BEB

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 17 Aug 2012 11:22:48 +0300
From:	Artem Bityutskiy <dedekind1@...il.com>
To:	Richard Genoud <richard.genoud@...il.com>
Cc:	linux-mtd@...ts.infradead.org,
	Shmulik Ladkani <shmulik.ladkani@...il.com>,
	David Woodhouse <dwmw2@...radead.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] UBI: replace MTD_UBI_BEB_LIMIT with module parameter

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

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)