2012-08-15 14:52:52

by Artem Bityutskiy

[permalink] [raw]
Subject: Re: [PATCH 4/4] UBI: replace MTD_UBI_BEB_LIMIT with user-space parameter

On Tue, 2012-07-10 at 18:23 +0200, Richard Genoud wrote:
> This patch provides the possibility to adjust the "maximum expected number of
> bad blocks per 1024 blocks" (max_beb_per1024) for each mtd device.
>
> The majority of NAND devices have their max_beb_per1024 equal to 20, but
> sometimes it's more.
> Now, we can adjust that via a kernel parameter:
> ubi.mtd=<name|num|path>[,<vid_hdr_offs>[,max_beb_per1024]]
> and via UBI_IOCATT ioctl which is now:
> struct ubi_attach_req {
> __s32 ubi_num;
> __s32 mtd_num;
> __s32 vid_hdr_offset;
> __u8 max_beb_per1024;
> __s8 padding[11];
> };
>
> Signed-off-by: Richard Genoud <[email protected]>

Would you please split this patch on 2 - first adds the kernel module
parameter, the second changes the ioctl.

--
Best Regards,
Artem Bityutskiy


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2012-08-16 14:52:33

by Richard Genoud

[permalink] [raw]
Subject: [PATCH 0/2] splitting "UBI: replace MTD_UBI_BEB_LIMIT with user-space parameter"

Artem,
Here are the 2 patches you requested in place of
"[PATCH 4/4] UBI: replace MTD_UBI_BEB_LIMIT with user-space parameter"


I made some changes to correct some things that Shmulik pointed out:

* max_beb_per1024 is now an unsigned.
* ubi_mtd_param_parse can fail on kstrtouint error.

Richard Genoud (2):
UBI: replace MTD_UBI_BEB_LIMIT with module parameter
UBI: add ioctl for max_beb_per1024

arch/arm/configs/sam9_l9260_defconfig | 1 -
drivers/mtd/ubi/Kconfig | 26 ---------
drivers/mtd/ubi/build.c | 93 ++++++++++++++++++++++-----------
drivers/mtd/ubi/cdev.c | 10 +++-
drivers/mtd/ubi/ubi.h | 6 ++-
include/mtd/ubi-user.h | 19 ++++++-
6 files changed, 95 insertions(+), 60 deletions(-)

--
1.7.2.5

2012-08-16 14:52:44

by Richard Genoud

[permalink] [raw]
Subject: [PATCH 2/2] UBI: add ioctl for max_beb_per1024

This patch provides the possibility to adjust the "maximum expected number of
bad blocks per 1024 blocks" (max_beb_per1024) for each mtd device from
UBI_IOCATT ioctl.

The majority of NAND devices have their max_beb_per1024 equal to 20, but
sometimes it's more.
We already could adjust that via a kernel parameter, now we can also use
UBI_IOCATT ioctl:
struct ubi_attach_req {
__s32 ubi_num;
__s32 mtd_num;
__s32 vid_hdr_offset;
__u8 max_beb_per1024;
__s8 padding[11];
};

Signed-off-by: Richard Genoud <[email protected]>
---
drivers/mtd/ubi/cdev.c | 9 ++++++++-
include/mtd/ubi-user.h | 19 ++++++++++++++++++-
2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
index e0027e7..d268b42 100644
--- a/drivers/mtd/ubi/cdev.c
+++ b/drivers/mtd/ubi/cdev.c
@@ -1006,12 +1006,19 @@ static long ctrl_cdev_ioctl(struct file *file, unsigned int cmd,
}

/*
+ * For compatibility with older user-space tools,
+ * a zero value should be treated like a default value
+ */
+ if (!req.max_beb_per1024)
+ req.max_beb_per1024 = MTD_UBI_DEFAULT_BEB_LIMIT;
+
+ /*
* Note, further request verification is done by
* 'ubi_attach_mtd_dev()'.
*/
mutex_lock(&ubi_devices_mutex);
err = ubi_attach_mtd_dev(mtd, req.ubi_num, req.vid_hdr_offset,
- MTD_UBI_DEFAULT_BEB_LIMIT);
+ req.max_beb_per1024);
mutex_unlock(&ubi_devices_mutex);
if (err < 0)
put_mtd_device(mtd);
diff --git a/include/mtd/ubi-user.h b/include/mtd/ubi-user.h
index 8787349..77cd0d1 100644
--- a/include/mtd/ubi-user.h
+++ b/include/mtd/ubi-user.h
@@ -222,6 +222,7 @@ enum {
* @ubi_num: UBI device number to create
* @mtd_num: MTD device number to attach
* @vid_hdr_offset: VID header offset (use defaults if %0)
+ * @max_beb_per1024: Maximum expected bad eraseblocks per 1024 eraseblocks
* @padding: reserved for future, not used, has to be zeroed
*
* This data structure is used to specify MTD device UBI has to attach and the
@@ -245,12 +246,28 @@ enum {
* be 2KiB-64 bytes = 1984. Note, that this position is not even 512-bytes
* aligned, which is OK, as UBI is clever enough to realize this is 4th
* sub-page of the first page and add needed padding.
+ *
+ * The @max_beb_per1024 is the maximum bad eraseblocks UBI expects on the ubi
+ * device per 1024 eraseblocks.
+ * This value is often given in an other form in the NAND datasheet (min NVB
+ * i.e. minimal number of valid blocks). The maximum expected bad eraseblocks
+ * per 1024 is then:
+ * 1024 * (1 - MinNVB / MaxNVB)
+ * Which gives 20 for most NAND devices.
+ * This limit is used in order to derive amount of eraseblock UBI reserves for
+ * handling new bad blocks.
+ * If the device has more bad eraseblocks than this limit, UBI does not reserve
+ * any physical eraseblocks for new bad eraseblocks, but attempts to use
+ * available eraseblocks (if any).
+ * The accepted range is 0-255. If 0 is given, the default value
+ * MTD_UBI_DEFAULT_BEB_LIMIT will be used for compatibility.
*/
struct ubi_attach_req {
__s32 ubi_num;
__s32 mtd_num;
__s32 vid_hdr_offset;
- __s8 padding[12];
+ __u8 max_beb_per1024;
+ __s8 padding[11];
};

/**
--
1.7.2.5

2012-08-16 14:53:32

by Richard Genoud

[permalink] [raw]
Subject: [PATCH 1/2] UBI: replace MTD_UBI_BEB_LIMIT with module parameter

This patch provides the possibility to adjust the "maximum expected number of
bad blocks per 1024 blocks" (max_beb_per1024) for each mtd device.

The majority of NAND devices have their max_beb_per1024 equal to 20, but
sometimes it's more.
Now, we can adjust that via a kernel parameter:
ubi.mtd=<name|num|path>[,<vid_hdr_offs>[,max_beb_per1024]]

Signed-off-by: Richard Genoud <[email protected]>
---
arch/arm/configs/sam9_l9260_defconfig | 1 -
drivers/mtd/ubi/Kconfig | 26 ---------
drivers/mtd/ubi/build.c | 93 ++++++++++++++++++++++-----------
drivers/mtd/ubi/cdev.c | 3 +-
drivers/mtd/ubi/ubi.h | 6 ++-
5 files changed, 70 insertions(+), 59 deletions(-)

diff --git a/arch/arm/configs/sam9_l9260_defconfig b/arch/arm/configs/sam9_l9260_defconfig
index d11fea5..47dd71a 100644
--- a/arch/arm/configs/sam9_l9260_defconfig
+++ b/arch/arm/configs/sam9_l9260_defconfig
@@ -39,7 +39,6 @@ CONFIG_MTD_NAND=y
CONFIG_MTD_NAND_ATMEL=y
CONFIG_MTD_NAND_PLATFORM=y
CONFIG_MTD_UBI=y
-CONFIG_MTD_UBI_BEB_LIMIT=30
CONFIG_MTD_UBI_GLUEBI=y
CONFIG_BLK_DEV_LOOP=y
CONFIG_BLK_DEV_RAM=y
diff --git a/drivers/mtd/ubi/Kconfig b/drivers/mtd/ubi/Kconfig
index f326877..7a57cc0 100644
--- a/drivers/mtd/ubi/Kconfig
+++ b/drivers/mtd/ubi/Kconfig
@@ -27,32 +27,6 @@ config MTD_UBI_WL_THRESHOLD
life-cycle less than 10000, the threshold should be lessened (e.g.,
to 128 or 256, although it does not have to be power of 2).

-config MTD_UBI_BEB_LIMIT
- int "Maximum expected bad eraseblock count per 1024 eraseblocks"
- default 20
- range 2 256
- help
- This option specifies the maximum bad physical eraseblocks UBI
- expects on the MTD device (per 1024 eraseblocks). If the underlying
- flash does not admit of bad eraseblocks (e.g. NOR flash), this value
- is ignored.
-
- NAND datasheets often specify the minimum and maximum NVM (Number of
- Valid Blocks) for the flashes' endurance lifetime. The maximum
- expected bad eraseblocks per 1024 eraseblocks then can be calculated
- as "1024 * (1 - MinNVB / MaxNVB)", which gives 20 for most NANDs
- (MaxNVB is basically the total count of eraseblocks on the chip).
-
- To put it differently, if this value is 20, UBI will try to reserve
- about 1.9% of physical eraseblocks for bad blocks handling. And that
- will be 1.9% of eraseblocks on the entire NAND chip, not just the MTD
- partition UBI attaches. This means that if you have, say, a NAND
- flash chip admits maximum 40 bad eraseblocks, and it is split on two
- MTD partitions of the same size, UBI will reserve 40 eraseblocks when
- attaching a partition.
-
- Leave the default value if unsure.
-
config MTD_UBI_GLUEBI
tristate "MTD devices emulation driver (gluebi)"
help
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 738772c..65edbc0 100644
--- 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
+
#ifdef CONFIG_MTD_UBI_MODULE
#define ubi_is_module() 1
#else
@@ -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;
};

/* Numbers of elements set in the @mtd_dev_param array */
@@ -565,9 +569,37 @@ void ubi_free_internal_volumes(struct ubi_device *ubi)
}
}

+static int get_bad_peb_limit(const struct ubi_device *ubi,
+ unsigned int max_beb_per1024)
+{
+ int device_pebs;
+ uint64_t device_size;
+ int limit = 0;
+
+ /*
+ * Here we are using size of the entire flash chip and
+ * not just the MTD partition size because the maximum
+ * number of bad eraseblocks is a percentage of the
+ * whole device and bad eraseblocks are not fairly
+ * distributed over the flash chip. So the worst case
+ * is that all the bad eraseblocks of the chip are in
+ * the MTD partition we are attaching (ubi->mtd).
+ */
+ device_size = mtd_get_device_size(ubi->mtd);
+ device_pebs = mtd_div_by_eb(device_size, ubi->mtd);
+ limit = mult_frac(device_pebs, max_beb_per1024, 1024);
+
+ /* Round it up */
+ if (mult_frac(limit, 1024, max_beb_per1024) < device_pebs)
+ limit += 1;
+
+ return limit;
+}
+
/**
* io_init - initialize I/O sub-system for a given UBI device.
* @ubi: UBI device description object
+ * @max_beb_per1024: maximum expected number of bad PEB per 1024 PEB
*
* If @ubi->vid_hdr_offset or @ubi->leb_start is zero, default offsets are
* assumed:
@@ -580,7 +612,7 @@ void ubi_free_internal_volumes(struct ubi_device *ubi)
* This function returns zero in case of success and a negative error code in
* case of failure.
*/
-static int io_init(struct ubi_device *ubi)
+static int io_init(struct ubi_device *ubi, unsigned int max_beb_per1024)
{
if (ubi->mtd->numeraseregions != 0) {
/*
@@ -610,29 +642,7 @@ static int io_init(struct ubi_device *ubi)

if (mtd_can_have_bb(ubi->mtd)) {
ubi->bad_allowed = 1;
- if (CONFIG_MTD_UBI_BEB_LIMIT > 0) {
- int per1024 = CONFIG_MTD_UBI_BEB_LIMIT;
- int limit, device_pebs;
- uint64_t device_size;
-
- /*
- * Here we are using size of the entire flash chip and
- * not just the MTD partition size because the maximum
- * number of bad eraseblocks is a percentage of the
- * whole device and bad eraseblocks are not fairly
- * distributed over the flash chip. So the worst case
- * is that all the bad eraseblocks of the chip are in
- * the MTD partition we are attaching (ubi->mtd).
- */
- device_size = mtd_get_device_size(ubi->mtd);
- device_pebs = mtd_div_by_eb(device_size, ubi->mtd);
- limit = mult_frac(device_pebs, per1024, 1024);
-
- /* Round it up */
- if (mult_frac(limit, 1024, per1024) < device_pebs)
- limit += 1;
- ubi->bad_peb_limit = limit;
- }
+ ubi->bad_peb_limit = get_bad_peb_limit(ubi, max_beb_per1024);
}

if (ubi->mtd->type == MTD_NORFLASH) {
@@ -825,6 +835,7 @@ static int autoresize(struct ubi_device *ubi, int vol_id)
* @mtd: MTD device description object
* @ubi_num: number to assign to the new UBI device
* @vid_hdr_offset: VID header offset
+ * @max_beb_per1024: maximum number of expected bad blocks per 1024 eraseblocks
*
* This function attaches MTD device @mtd_dev to UBI and assign @ubi_num number
* to the newly created UBI device, unless @ubi_num is %UBI_DEV_NUM_AUTO, in
@@ -835,7 +846,8 @@ static int autoresize(struct ubi_device *ubi, int vol_id)
* Note, the invocations of this function has to be serialized by the
* @ubi_devices_mutex.
*/
-int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, int vid_hdr_offset)
+int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
+ int vid_hdr_offset, unsigned int max_beb_per1024)
{
struct ubi_device *ubi;
int i, err, ref = 0;
@@ -908,7 +920,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, int vid_hdr_offset)
dbg_msg("sizeof(struct ubi_ainf_peb) %zu", sizeof(struct ubi_ainf_peb));
dbg_msg("sizeof(struct ubi_wl_entry) %zu", sizeof(struct ubi_wl_entry));

- err = io_init(ubi);
+ err = io_init(ubi, max_beb_per1024);
if (err)
goto out_free;

@@ -1197,7 +1209,7 @@ static int __init ubi_init(void)

mutex_lock(&ubi_devices_mutex);
err = ubi_attach_mtd_dev(mtd, UBI_DEV_NUM_AUTO,
- p->vid_hdr_offs);
+ p->vid_hdr_offs, p->max_beb_per1024);
mutex_unlock(&ubi_devices_mutex);
if (err < 0) {
ubi_err("cannot attach mtd%d", mtd->index);
@@ -1316,7 +1328,8 @@ static int __init ubi_mtd_param_parse(const char *val, struct kernel_param *kp)
struct mtd_dev_param *p;
char buf[MTD_PARAM_LEN_MAX];
char *pbuf = &buf[0];
- char *tokens[2] = {NULL, NULL};
+ char *tokens[MTD_PARAM_NB_MAX];
+ int err;

if (!val)
return -EINVAL;
@@ -1346,7 +1359,7 @@ static int __init ubi_mtd_param_parse(const char *val, struct kernel_param *kp)
if (buf[len - 1] == '\n')
buf[len - 1] = '\0';

- for (i = 0; i < 2; i++)
+ for (i = 0; i < MTD_PARAM_NB_MAX; i++)
tokens[i] = strsep(&pbuf, ",");

if (pbuf) {
@@ -1364,18 +1377,38 @@ static int __init ubi_mtd_param_parse(const char *val, struct kernel_param *kp)
if (p->vid_hdr_offs < 0)
return p->vid_hdr_offs;

+ if (tokens[2]) {
+ err = kstrtouint(tokens[2], 10, &p->max_beb_per1024);
+ if (err) {
+ pr_err("UBI error: bad value for max_beb_per1024 parameter: %s",
+ tokens[2]);
+ return -EINVAL;
+ }
+ }
+
+ /*
+ * A value of 0 is forced to the default value to keep the same
+ * behavior between ubiattach command and module parameter
+ */
+ if (!p->max_beb_per1024)
+ p->max_beb_per1024 = MTD_UBI_DEFAULT_BEB_LIMIT;
+
mtd_devs += 1;
return 0;
}

module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000);
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"
+ "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"
"Example 1: mtd=/dev/mtd0 - attach MTD device "
"/dev/mtd0.\n"
"Example 2: mtd=content,1984 mtd=4 - attach MTD device "
diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
index fb55678..e0027e7 100644
--- a/drivers/mtd/ubi/cdev.c
+++ b/drivers/mtd/ubi/cdev.c
@@ -1010,7 +1010,8 @@ static long ctrl_cdev_ioctl(struct file *file, unsigned int cmd,
* 'ubi_attach_mtd_dev()'.
*/
mutex_lock(&ubi_devices_mutex);
- err = ubi_attach_mtd_dev(mtd, req.ubi_num, req.vid_hdr_offset);
+ err = ubi_attach_mtd_dev(mtd, req.ubi_num, req.vid_hdr_offset,
+ MTD_UBI_DEFAULT_BEB_LIMIT);
mutex_unlock(&ubi_devices_mutex);
if (err < 0)
put_mtd_device(mtd);
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index c94612e..f926343 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -50,6 +50,9 @@
/* UBI name used for character devices, sysfs, etc */
#define UBI_NAME_STR "ubi"

+/* Default number of maximum expected bad blocks per 1024 eraseblocks */
+#define MTD_UBI_DEFAULT_BEB_LIMIT 20
+
/* Normal UBI messages */
#define ubi_msg(fmt, ...) printk(KERN_NOTICE "UBI: " fmt "\n", ##__VA_ARGS__)
/* UBI warning messages */
@@ -693,7 +696,8 @@ int ubi_io_write_vid_hdr(struct ubi_device *ubi, int pnum,
struct ubi_vid_hdr *vid_hdr);

/* build.c */
-int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, int vid_hdr_offset);
+int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
+ int vid_hdr_offset, unsigned int max_beb_per1024);
int ubi_detach_mtd_dev(int ubi_num, int anyway);
struct ubi_device *ubi_get_device(int ubi_num);
void ubi_put_device(struct ubi_device *ubi);
--
1.7.2.5

2012-08-17 08:18:32

by Artem Bityutskiy

[permalink] [raw]
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


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2012-08-17 08:23:58

by Artem Bityutskiy

[permalink] [raw]
Subject: Re: [PATCH 2/2] UBI: add ioctl for max_beb_per1024

On Thu, 2012-08-16 at 16:52 +0200, Richard Genoud wrote:
> mutex_lock(&ubi_devices_mutex);
> err = ubi_attach_mtd_dev(mtd, req.ubi_num, req.vid_hdr_offset,
> - MTD_UBI_DEFAULT_BEB_LIMIT);
> + req.max_beb_per1024);

Do not forget to explicitely validate 'req.max_beb_per1024' valuse in
'ubi_attach_mtd_dev()', as we do for other parameters.

--
Best Regards,
Artem Bityutskiy


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2012-08-17 10:28:19

by Richard Genoud

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

2012/8/17 Artem Bityutskiy <[email protected]>:
> 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 ?