Message ID | 1338909119-5188-2-git-send-email-dedekind1@gmail.com |
---|---|
State | New, archived |
Headers | show |
Am 05.06.2012 17:11, schrieb Artem Bityutskiy: > From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com> > > I've spent 10 minutes looking at the code and added few TODOs. Many of them are > nit-picks, though, but I'd like them to be fixed - should not be difficult. > Some are more than just nit-picks. > > Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com> > --- > TODO | 1 - > drivers/mtd/ubi/attach.c | 21 +++++++++++++++------ > drivers/mtd/ubi/fastmap.c | 26 ++++++++++++++++++++++++++ > 3 files changed, 41 insertions(+), 7 deletions(-) > > diff --git a/TODO b/TODO > index 63a22b9..17e30b6 100644 > --- a/TODO > +++ b/TODO > @@ -9,4 +9,3 @@ to the ubi-utils.git repository, to a separate branch at the beginning > test UBI + fastmap with it. > 3. Test the autoresize feature > 4. Test 'ubi_flush()' > -5. Test > diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c > index acf7db3..2a0c1ba 100644 > --- a/drivers/mtd/ubi/attach.c > +++ b/drivers/mtd/ubi/attach.c > @@ -1228,12 +1228,21 @@ int ubi_attach(struct ubi_device *ubi) > } else if (err < 0) > return err; > > - /* TODO: When you create an image with ubinize - you do not know the > - * amount of PEBs. So you need to initialize this field with '-1' at > - * ubinize time. And here you need to check for -1 and initialize it if > - * needed. Then store it at fastmap. This special value has to be also > - * documented at ubi-media.h. You also have to amend 'nused' etc. > - * Probably this can be done later. */ > + /* TODO: currently the fastmap code assumes that the fastmap data > + * structures are created only by the kernel when the kernel attaches > + * an fastmap-less image. However, this assumption is too limiting and > + * for sure people will want to pre-create UBI images with fastmap > + * using the ubinize tool. Then they wont have to waste a lot of time > + * waiting for full scan and fastmap initializetion during the first > + * boot. This is a very important feature for the factory production > + * line where every additional minute per device costs a lot. > + * > + * When you are attaching an MTD device which contains an image > + * generated by ubinize with a fastmap, you will not know the > + * 'bad_peb_count' value. Most probably it will contain something like > + * -1. The same is true for the per-PEB information in the fastmap - it > + * won't tell which PEBs are bad. So we need to detect this and iterate > + * over all PEBs, find out which are bad, and update 'ai' here. */ I'm confused to find bad PEBs I'd still need a full scan, right? > ubi->bad_peb_count = ai->bad_peb_count; > ubi->good_peb_count = ubi->peb_count - ubi->bad_peb_count; > ubi->corr_peb_count = ai->corr_peb_count; > diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c > index a8143da..09629c2 100644 > --- a/drivers/mtd/ubi/fastmap.c > +++ b/drivers/mtd/ubi/fastmap.c > @@ -718,6 +718,17 @@ out: > * ubi_scan_fastmap - scan the fastmap > * @ubi: UBI device object > * @ai: UBI attach info to be filled > + * > + * TODO: not urgent, but at some point - check the code with kernel doc and fix > + * its complaints. Okay. > + * TODO: not urgent, but for consistency, follow the UBI/UBIFS style and put a > + * dot at the end of the first short description sentence (globally): > + * ubi_scan_fastmap - scan the fastmap. (<-dot). Will fix! > + * TODO: not urgent, but it is desireble to document error codes in the header > + * comments and probably describe what the function does, if there is something > + * to say (globally). > */ > int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai) > { > @@ -744,8 +755,13 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai) > goto out; > } > > + /* TODO: If 'ubi_io_read()' returns you UBI_IO_BITFLIP, this means that > + * the PEB has a bit-flip and has to be scrubbed. How will the > + * superblock be scrubbed or how is the bit-flip guaranteed to be taken > + * care of? */ At this stage it's a bit difficult. But I can add this information to the in-memory fastmap structure such that the PEB will be scrubbed upon it's returned to the WL sub-system. > ret = ubi_io_read(ubi, fmsb, sb_pnum, ubi->leb_start, sizeof(*fmsb)); > if (ret && ret != UBI_IO_BITFLIPS) { > + /* TODO: what are the error codes > 0 ? Why is this check? */ To catch UBI_IO_BAD_HDR, UBI_IO_BAD_HDR_EBADMSG and friends. If we fail to read from a fastmap PEB we have to fail with UBI_BAD_FASTMAP. > if (ret > 0) > ret = UBI_BAD_FASTMAP; > > @@ -754,7 +770,17 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai) > goto out; > } > > + /* TODO: please, use 'be32_to_cpu()' _every_ time you access a __be32 / > + * etc field. Please, look how things are done in io.c. Please, check > + * and fix globally. */ AFAIK ->magic is the only __b32 field where I not use be32_to_cpu() because it's useless here. The magic value will be always the same. (It's fixed) I know UBI does also use be32_to_cpu for the EC and HDR magic. Anyway, I'll change it. :-) > if (fmsb->magic != UBI_FM_SB_MAGIC) { > + /* TODO: not urgent, but examine all the error messages and > + * print more information there. Here you should print what was > + * read and what was expected. See io.c and do similarly or > + * better. > + * Please, change globally. E.g., when you print about bad > + * version - print what was expected and what was actually > + * found. */ Okay, this is a good idea! Thanks, //richard
On Wed, 2012-06-06 at 23:30 +0200, Richard Weinberger wrote: > I'm confused to find bad PEBs I'd still need a full scan, right? If full scan means call mtd_block_isbad() for each PEB, then yes. > > ret = ubi_io_read(ubi, fmsb, sb_pnum, ubi->leb_start, sizeof(*fmsb)); > > if (ret && ret != UBI_IO_BITFLIPS) { > > + /* TODO: what are the error codes > 0 ? Why is this check? */ > > To catch UBI_IO_BAD_HDR, UBI_IO_BAD_HDR_EBADMSG and friends. Which are never returned by ubi_io_read(). > AFAIK ->magic is the only __b32 field where I not use be32_to_cpu() because it's useless here. Why is it useless? if we have the following bytes starting from address 0xC0000000: 7B 11 D6 9F then we do: unsigned int *p = 0xC0000000; printk("%#x\n", *p); we should see: 0x7B11D69F on BE system (e.g., powerpc) and 0x9FD6117B on LE system (e.g., x86).
On Thu, 2012-06-07 at 01:46 +0200, Richard Weinberger wrote: > >> To catch UBI_IO_BAD_HDR, UBI_IO_BAD_HDR_EBADMSG and friends. > > > > Which are never returned by ubi_io_read(). > > Also never ever in future? > If so, I'll drop this check. Well, if you read those constants they suggest that they are specific to functions which read the UBI headers. ubi_io_read() function is just a general read function which is just a simple wrapper over mtd_read() plus re-trying, nice error messaging, debugging prints, some error codes handling, nothing else. So no, it is not going to ever check UBI headers and return those codes. /me goes to sleep.
Am 07.06.2012 01:38, schrieb Artem Bityutskiy: > On Wed, 2012-06-06 at 23:30 +0200, Richard Weinberger wrote: >> I'm confused to find bad PEBs I'd still need a full scan, right? > > If full scan means call mtd_block_isbad() for each PEB, then yes. > >>> ret = ubi_io_read(ubi, fmsb, sb_pnum, ubi->leb_start, sizeof(*fmsb)); >>> if (ret && ret != UBI_IO_BITFLIPS) { >>> + /* TODO: what are the error codes > 0 ? Why is this check? */ >> >> To catch UBI_IO_BAD_HDR, UBI_IO_BAD_HDR_EBADMSG and friends. > > Which are never returned by ubi_io_read(). Also never ever in future? If so, I'll drop this check. >> AFAIK ->magic is the only __b32 field where I not use be32_to_cpu() because it's useless here. > > Why is it useless? > > if we have the following bytes starting from address 0xC0000000: > 7B 11 D6 9F > > then we do: > > unsigned int *p = 0xC0000000; > printk("%#x\n", *p); > > we should see: > > 0x7B11D69F on BE system (e.g., powerpc) > and > 0x9FD6117B on LE system (e.g., x86). > Damit, you are right! Sorry, //richard
diff --git a/TODO b/TODO index 63a22b9..17e30b6 100644 --- a/TODO +++ b/TODO @@ -9,4 +9,3 @@ to the ubi-utils.git repository, to a separate branch at the beginning test UBI + fastmap with it. 3. Test the autoresize feature 4. Test 'ubi_flush()' -5. Test diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c index acf7db3..2a0c1ba 100644 --- a/drivers/mtd/ubi/attach.c +++ b/drivers/mtd/ubi/attach.c @@ -1228,12 +1228,21 @@ int ubi_attach(struct ubi_device *ubi) } else if (err < 0) return err; - /* TODO: When you create an image with ubinize - you do not know the - * amount of PEBs. So you need to initialize this field with '-1' at - * ubinize time. And here you need to check for -1 and initialize it if - * needed. Then store it at fastmap. This special value has to be also - * documented at ubi-media.h. You also have to amend 'nused' etc. - * Probably this can be done later. */ + /* TODO: currently the fastmap code assumes that the fastmap data + * structures are created only by the kernel when the kernel attaches + * an fastmap-less image. However, this assumption is too limiting and + * for sure people will want to pre-create UBI images with fastmap + * using the ubinize tool. Then they wont have to waste a lot of time + * waiting for full scan and fastmap initializetion during the first + * boot. This is a very important feature for the factory production + * line where every additional minute per device costs a lot. + * + * When you are attaching an MTD device which contains an image + * generated by ubinize with a fastmap, you will not know the + * 'bad_peb_count' value. Most probably it will contain something like + * -1. The same is true for the per-PEB information in the fastmap - it + * won't tell which PEBs are bad. So we need to detect this and iterate + * over all PEBs, find out which are bad, and update 'ai' here. */ ubi->bad_peb_count = ai->bad_peb_count; ubi->good_peb_count = ubi->peb_count - ubi->bad_peb_count; ubi->corr_peb_count = ai->corr_peb_count; diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c index a8143da..09629c2 100644 --- a/drivers/mtd/ubi/fastmap.c +++ b/drivers/mtd/ubi/fastmap.c @@ -718,6 +718,17 @@ out: * ubi_scan_fastmap - scan the fastmap * @ubi: UBI device object * @ai: UBI attach info to be filled + * + * TODO: not urgent, but at some point - check the code with kernel doc and fix + * its complaints. + * + * TODO: not urgent, but for consistency, follow the UBI/UBIFS style and put a + * dot at the end of the first short description sentence (globally): + * ubi_scan_fastmap - scan the fastmap. (<-dot). + * + * TODO: not urgent, but it is desireble to document error codes in the header + * comments and probably describe what the function does, if there is something + * to say (globally). */ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai) { @@ -744,8 +755,13 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai) goto out; } + /* TODO: If 'ubi_io_read()' returns you UBI_IO_BITFLIP, this means that + * the PEB has a bit-flip and has to be scrubbed. How will the + * superblock be scrubbed or how is the bit-flip guaranteed to be taken + * care of? */ ret = ubi_io_read(ubi, fmsb, sb_pnum, ubi->leb_start, sizeof(*fmsb)); if (ret && ret != UBI_IO_BITFLIPS) { + /* TODO: what are the error codes > 0 ? Why is this check? */ if (ret > 0) ret = UBI_BAD_FASTMAP; @@ -754,7 +770,17 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai) goto out; } + /* TODO: please, use 'be32_to_cpu()' _every_ time you access a __be32 / + * etc field. Please, look how things are done in io.c. Please, check + * and fix globally. */ if (fmsb->magic != UBI_FM_SB_MAGIC) { + /* TODO: not urgent, but examine all the error messages and + * print more information there. Here you should print what was + * read and what was expected. See io.c and do similarly or + * better. + * Please, change globally. E.g., when you print about bad + * version - print what was expected and what was actually + * found. */ ubi_err("super block magic does not match"); ret = UBI_BAD_FASTMAP; kfree(fmsb);