| Message ID | 1345627680-8166-6-git-send-email-richard.genoud@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2012-08-22 at 11:28 +0200, Richard Genoud wrote: > +"-b, --max-beb-per1024 Maximum expected bad block number per 1024 eraseblock.\n" > +" The default value is correct for most NAND devices.\n" > +" (Range 1-768, 0 for default kernel value).\n" If the feature is not supported (the kernel is old), the tool will just return success and the kernel will just ignore max-beb-per1024, which is not very nice. It would be better if ubiattach complained like this instead: ubiattach error: your UBI driver does not allow changing the reserved PEBs count, probably you run old kernel? The support was added in kernel version 3.7.
2012/8/22 Artem Bityutskiy <dedekind1@gmail.com>: > If the feature is not supported (the kernel is old), the tool will just > return success and the kernel will just ignore max-beb-per1024, which is > not very nice. > > It would be better if ubiattach complained like this instead: > > ubiattach error: your UBI driver does not allow changing the reserved > PEBs count, probably you run old kernel? The support > was added in kernel version 3.7. yes, you're right, I'll change that. Thanks, Richard.
On Wed, 2012-08-22 at 13:17 +0200, Richard Genoud wrote: > 2012/8/22 Artem Bityutskiy <dedekind1@gmail.com>: > > If the feature is not supported (the kernel is old), the tool will just > > return success and the kernel will just ignore max-beb-per1024, which is > > not very nice. > > > > It would be better if ubiattach complained like this instead: > > > > ubiattach error: your UBI driver does not allow changing the reserved > > PEBs count, probably you run old kernel? The support > > was added in kernel version 3.7. > yes, you're right, I'll change that. The question is how :-)
On Wed, 2012-08-22 at 13:17 +0200, Richard Genoud wrote: > 2012/8/22 Artem Bityutskiy <dedekind1@gmail.com>: > > If the feature is not supported (the kernel is old), the tool will just > > return success and the kernel will just ignore max-beb-per1024, which is > > not very nice. > > > > It would be better if ubiattach complained like this instead: > > > > ubiattach error: your UBI driver does not allow changing the reserved > > PEBs count, probably you run old kernel? The support > > was added in kernel version 3.7. > yes, you're right, I'll change that. > Thanks, I see 2 ways. 1. Add a 'get device property' ioctl, similar to the 'set volume property' ioctl we have. And use this ioctl to query the current bad PEBs limit. 2. Just call the 'attach' ioctl with an insane max_beb_per1024 value (say, -1) first. If it succeeded, than the driver does not support max_beb_per1024 at all, and you may exit with a warning that you have attached it, but the max_beb_per1024 was ignored because the kernel is old. If it fails with -EINVAL, this means the kernel checked the value and the feature is supported, so you can call it for the second time with the right max_beb_per1024. The first approach is more complex but probably cleaner. The second is easier to implement.
On Wed, 2012-08-22 at 14:30 +0300, Artem Bityutskiy wrote: > 1. Add a 'get device property' ioctl, similar to the 'set volume > property' ioctl we have. And use this ioctl to query the current bad > PEBs limit. > > 2. Just call the 'attach' ioctl with an insane max_beb_per1024 value > (say, -1) first. If it succeeded, than the driver does not support > max_beb_per1024 at all, and you may exit with a warning that you have > attached it, but the max_beb_per1024 was ignored because the kernel is > old. > > If it fails with -EINVAL, this means the kernel checked the value and > the feature is supported, so you can call it for the second time with > the right max_beb_per1024. > > The first approach is more complex but probably cleaner. The second is > easier to implement. I guess I prefer the second option, though. What do you think?
2012/8/22 Artem Bityutskiy <dedekind1@gmail.com>: > I see 2 ways. > > 1. Add a 'get device property' ioctl, similar to the 'set volume > property' ioctl we have. And use this ioctl to query the current bad > PEBs limit. > > 2. Just call the 'attach' ioctl with an insane max_beb_per1024 value > (say, -1) first. If it succeeded, than the driver does not support > max_beb_per1024 at all, and you may exit with a warning that you have > attached it, but the max_beb_per1024 was ignored because the kernel is > old. > > If it fails with -EINVAL, this means the kernel checked the value and > the feature is supported, so you can call it for the second time with > the right max_beb_per1024. > > The first approach is more complex but probably cleaner. The second is > easier to implement. > > -- > Best Regards, > Artem Bityutskiy There's also a 3rd one with an uname system call, checking if kernel >= 3.7, but that's not very elegant.
2012/8/22 Artem Bityutskiy <dedekind1@gmail.com>: > On Wed, 2012-08-22 at 14:30 +0300, Artem Bityutskiy wrote: >> 1. Add a 'get device property' ioctl, similar to the 'set volume >> property' ioctl we have. And use this ioctl to query the current bad >> PEBs limit. >> >> 2. Just call the 'attach' ioctl with an insane max_beb_per1024 value >> (say, -1) first. If it succeeded, than the driver does not support >> max_beb_per1024 at all, and you may exit with a warning that you have >> attached it, but the max_beb_per1024 was ignored because the kernel is >> old. >> >> If it fails with -EINVAL, this means the kernel checked the value and >> the feature is supported, so you can call it for the second time with >> the right max_beb_per1024. >> >> The first approach is more complex but probably cleaner. The second is >> easier to implement. > > I guess I prefer the second option, though. What do you think? Well, for this one, we can go with the 2nd option, and if one day there's another parameter, then we could implement the 'get device property ioctl' for max_beb_per1024 and the new parameter. How's that sound ?
Hi Artem, On Wed, 22 Aug 2012 14:33:17 +0300 Artem Bityutskiy <dedekind1@gmail.com> wrote: > > 2. Just call the 'attach' ioctl with an insane max_beb_per1024 value > > (say, -1) first. If it succeeded, than the driver does not support > > max_beb_per1024 at all, and you may exit with a warning that you have > > attached it, but the max_beb_per1024 was ignored because the kernel is > > old. > > > > If it fails with -EINVAL, this means the kernel checked the value and > > the feature is supported, so you can call it for the second time with > > the right max_beb_per1024. But the ioctl might return -EINVAL due to any of the *other* ubi_attach_req fields being invalid. So you can't really determine whether max_beb_per1024 is supported. > > The first approach is more complex but probably cleaner. The second is > > easier to implement. > > I guess I prefer the second option, though. What do you think? I would go with first, due to my slight tendency to over-design things ;-) It is cleaner, however bloatier too... I'll try to think if there's an easy alternative. Regards, Shmulik
On Wed, 2012-08-22 at 13:38 +0200, Richard Genoud wrote: > > I guess I prefer the second option, though. What do you think? > Well, for this one, we can go with the 2nd option, and if one day > there's another parameter, then we could implement the 'get device > property ioctl' for max_beb_per1024 and the new parameter. > How's that sound ? Good. But please, implement this double 'attach' invocation inside 'ubi_attach_mtd()'. Please, introduce a special return code to indicate the 'attached but max_beb_per1024 was ignored' case (say, 1). Add comments explaining the trick.
On Wed, 2012-08-22 at 14:47 +0300, Shmulik Ladkani wrote: > But the ioctl might return -EINVAL due to any of the *other* > ubi_attach_req fields being invalid. > So you can't really determine whether max_beb_per1024 is supported. We call the attach ioctl 2 times. 1. Call it with bogus max_beb_per1024. a) Success - print the warning and exit b) Failure with non-EINVAL - overall failure. c) Failure with -EINVAL, go to step 2. 2. Call with user-supplied max_beb_per1024. a) Success - everything is fine, max_beb_per1024 is supported, the first failure was because we passed max_beb_per1024=-1 b) Failure with non-EINVAL - overall failure. b) Failure with -EINVAL, return error, the whole operation failed, one of the parameters was incorrect. Where is the flaw?
On Wed, 22 Aug 2012 15:15:15 +0300 Artem Bityutskiy <dedekind1@gmail.com> wrote: > On Wed, 2012-08-22 at 14:47 +0300, Shmulik Ladkani wrote: > > But the ioctl might return -EINVAL due to any of the *other* > > ubi_attach_req fields being invalid. > > So you can't really determine whether max_beb_per1024 is supported. > > We call the attach ioctl 2 times. > > 1. Call it with bogus max_beb_per1024. > a) Success - print the warning and exit > b) Failure with non-EINVAL - overall failure. > c) Failure with -EINVAL, go to step 2. > > > 2. Call with user-supplied max_beb_per1024. > a) Success - everything is fine, max_beb_per1024 is supported, > the first failure was because we passed max_beb_per1024=-1 > b) Failure with non-EINVAL - overall failure. > b) Failure with -EINVAL, return error, the whole operation failed, > one of the parameters was incorrect. > > Where is the flaw? Sorry, this seems right. Should have given this more thought... will go for some cofee. Regards, Shmulik
2012/8/22 Artem Bityutskiy <dedekind1@gmail.com>: > Good. But please, implement this double 'attach' invocation inside > 'ubi_attach_mtd()'. Please, introduce a special return code to indicate > the 'attached but max_beb_per1024 was ignored' case (say, 1). Add > comments explaining the trick. one question: - does the call: ubiattach -m 2 -b 0 should fail on an old kernel ? In this case, I'll have to introduce a flag or something to let ubi_attach_mtd() know that the -b option was given. Honestly, I don't think it's necessary, as far as -b0 means "the default kernel value", so on an old kernel the default that's what will happened. I'll have also to change the ubi_attach() call. My though is to factorize code between ubi_attach_mtd() and ubi_attach(), and then make the change. Best regards, Richard.
On Wed, 2012-08-22 at 15:31 +0200, Richard Genoud wrote: > 2012/8/22 Artem Bityutskiy <dedekind1@gmail.com>: > > Good. But please, implement this double 'attach' invocation inside > > 'ubi_attach_mtd()'. Please, introduce a special return code to indicate > > the 'attached but max_beb_per1024 was ignored' case (say, 1). Add > > comments explaining the trick. > > one question: > - does the call: > ubiattach -m 2 -b 0 > should fail on an old kernel ? No, no need to complicate the thing with handling this case. Let's keep it simple.
diff --git a/tests/fs-tests/integrity/integck.c b/tests/fs-tests/integrity/integck.c index 30322cd..f12dfac 100644 --- a/tests/fs-tests/integrity/integck.c +++ b/tests/fs-tests/integrity/integck.c @@ -3152,6 +3152,7 @@ static int reattach(void) req.mtd_num = args.mtdn; req.vid_hdr_offset = 0; req.mtd_dev_node = NULL; + req.max_beb_per1024 = 0; err = ubi_attach(libubi, "/dev/ubi_ctrl", &req); if (err) diff --git a/ubi-utils/include/libubi.h b/ubi-utils/include/libubi.h index 11a186b..4e9f3c4 100644 --- a/ubi-utils/include/libubi.h +++ b/ubi-utils/include/libubi.h @@ -50,6 +50,7 @@ typedef void * libubi_t; * @mtd_dev_node: path to MTD device node to attach * @vid_hdr_offset: VID header offset (%0 means default offset and this is what * most of the users want) + * @max_beb_per1024: Maximum expected bad eraseblocks per 1024 eraseblocks */ struct ubi_attach_request { @@ -57,6 +58,7 @@ struct ubi_attach_request int mtd_num; const char *mtd_dev_node; int vid_hdr_offset; + int max_beb_per1024; }; /** diff --git a/ubi-utils/libubi.c b/ubi-utils/libubi.c index ae805c8..1db4a19 100644 --- a/ubi-utils/libubi.c +++ b/ubi-utils/libubi.c @@ -719,6 +719,7 @@ int ubi_attach_mtd(libubi_t desc, const char *node, r.ubi_num = req->dev_num; r.mtd_num = req->mtd_num; r.vid_hdr_offset = req->vid_hdr_offset; + r.max_beb_per1024 = req->max_beb_per1024; ret = do_attach(node, &r); if (ret == 0) @@ -780,6 +781,7 @@ int ubi_attach(libubi_t desc, const char *node, struct ubi_attach_request *req) memset(&r, 0, sizeof(struct ubi_attach_req)); r.ubi_num = req->dev_num; r.vid_hdr_offset = req->vid_hdr_offset; + r.max_beb_per1024 = req->max_beb_per1024; /* * User has passed path to device node. Lets find out MTD device number diff --git a/ubi-utils/ubiattach.c b/ubi-utils/ubiattach.c index 27e7c09..4521788 100644 --- a/ubi-utils/ubiattach.c +++ b/ubi-utils/ubiattach.c @@ -42,6 +42,7 @@ struct args { int vidoffs; const char *node; const char *dev; + int max_beb_per1024; }; static struct args args = { @@ -50,6 +51,7 @@ static struct args args = { .vidoffs = 0, .node = NULL, .dev = NULL, + .max_beb_per1024 = 0, }; static const char doc[] = PROGRAM_NAME " version " VERSION @@ -63,6 +65,9 @@ static const char optionsstr[] = " if the character device node does not exist)\n" "-O, --vid-hdr-offset VID header offset (do not specify this unless you really\n" " know what you are doing, the default should be optimal)\n" +"-b, --max-beb-per1024 Maximum expected bad block number per 1024 eraseblock.\n" +" The default value is correct for most NAND devices.\n" +" (Range 1-768, 0 for default kernel value).\n" "-h, --help print help message\n" "-V, --version print program version"; @@ -71,19 +76,25 @@ static const char usage[] = "\t[-m <MTD device number>] [-d <UBI device number>] [-p <path to device>]\n" "\t[--mtdn=<MTD device number>] [--devn=<UBI device number>]\n" "\t[--dev-path=<path to device>]\n" +"\t[--max-beb-per1024=<maximum bad block number per 1024 blocks>]\n" "UBI control device defaults to " DEFAULT_CTRL_DEV " if not supplied.\n" "Example 1: " PROGRAM_NAME " -p /dev/mtd0 - attach /dev/mtd0 to UBI\n" "Example 2: " PROGRAM_NAME " -m 0 - attach MTD device 0 (mtd0) to UBI\n" "Example 3: " PROGRAM_NAME " -m 0 -d 3 - attach MTD device 0 (mtd0) to UBI\n" -" and create UBI device number 3 (ubi3)"; +" and create UBI device number 3 (ubi3)\n" +"Example 4: " PROGRAM_NAME " -m 1 -b 25 - attach /dev/mtd1 to UBI and reserve \n" +" 25*nand_size_in_blocks/1024 erase blocks for bad block handling.\n" +" (e.g. if the NAND *chipset* has 4096 PEB, 100 will be reserved \n" +" for this UBI device)."; static const struct option long_options[] = { - { .name = "devn", .has_arg = 1, .flag = NULL, .val = 'd' }, - { .name = "dev-path", .has_arg = 1, .flag = NULL, .val = 'p' }, - { .name = "mtdn", .has_arg = 1, .flag = NULL, .val = 'm' }, - { .name = "vid-hdr-offset", .has_arg = 1, .flag = NULL, .val = 'O' }, - { .name = "help", .has_arg = 0, .flag = NULL, .val = 'h' }, - { .name = "version", .has_arg = 0, .flag = NULL, .val = 'V' }, + { .name = "devn", .has_arg = 1, .flag = NULL, .val = 'd' }, + { .name = "dev-path", .has_arg = 1, .flag = NULL, .val = 'p' }, + { .name = "mtdn", .has_arg = 1, .flag = NULL, .val = 'm' }, + { .name = "vid-hdr-offset", .has_arg = 1, .flag = NULL, .val = 'O' }, + { .name = "help", .has_arg = 0, .flag = NULL, .val = 'h' }, + { .name = "version", .has_arg = 0, .flag = NULL, .val = 'V' }, + { .name = "max-beb-per1024", .has_arg = 1, .flag = NULL, .val = 'b' }, { NULL, 0, NULL, 0}, }; @@ -92,7 +103,7 @@ static int parse_opt(int argc, char * const argv[]) while (1) { int key, error = 0; - key = getopt_long(argc, argv, "p:m:d:O:hV", long_options, NULL); + key = getopt_long(argc, argv, "p:m:d:O:hVb:", long_options, NULL); if (key == -1) break; @@ -134,6 +145,16 @@ static int parse_opt(int argc, char * const argv[]) case ':': return errmsg("parameter is missing"); + case 'b': + args.max_beb_per1024 = simple_strtoul(optarg, &error); + if (error || args.max_beb_per1024 < 0 || args.max_beb_per1024 > 768) + return errmsg("bad maximum of expected bad blocks (0-768): \"%s\"", optarg); + + if (args.max_beb_per1024 == 0) + warnmsg("default kernel value will be used for maximum expected bad blocks\n"); + + break; + default: fprintf(stderr, "Use -h for help\n"); return -1; @@ -190,6 +211,7 @@ int main(int argc, char * const argv[]) req.mtd_num = args.mtdn; req.vid_hdr_offset = args.vidoffs; req.mtd_dev_node = args.dev; + req.max_beb_per1024 = args.max_beb_per1024; err = ubi_attach(libubi, args.node, &req); if (err) {
The ioctl UBI_IOCATT has been extended with max_beb_per1024 parameter. This parameter is used for adjusting the "maximum expected number of bad blocks per 1024 blocks" for each mtd device. The number of physical erase blocks (PEB) that UBI will reserve for bad block handling is now: whole_flash_chipset__PEB_number * max_beb_per1024 / 1024 This means that for a 4096 PEB NAND device with 3 MTD partitions: mtd0: 512 PEB mtd1: 1536 PEB mtd2: 2048 PEB the commands: ubiattach -m 0 -d 0 -b 20 /dev/ubi_ctrl ubiattach -m 1 -d 1 -b 20 /dev/ubi_ctrl ubiattach -m 2 -d 2 -b 20 /dev/ubi_ctrl will attach mtdx to UBIx and reserve: 80 PEB for bad block handling on UBI0 80 PEB for bad block handling on UBI1 80 PEB for bad block handling on UBI2 => for the whole device, 240 PEB will be reserved for bad block handling. This may seems a waste of space, but as far as the bad blocks can appear every where on a flash device, in the worst case scenario they can all appear in one MTD partition. So the maximum number of expected erase blocks given by the NAND manufacturer should be reserve on each MTD partition. Signed-off-by: Richard Genoud <richard.genoud@gmail.com> --- tests/fs-tests/integrity/integck.c | 1 + ubi-utils/include/libubi.h | 2 + ubi-utils/libubi.c | 2 + ubi-utils/ubiattach.c | 38 ++++++++++++++++++++++++++++------- 4 files changed, 35 insertions(+), 8 deletions(-)