From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga09.intel.com ([134.134.136.24]) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1SbvPB-0004Fy-C7 for linux-mtd@lists.infradead.org; Tue, 05 Jun 2012 15:10:46 +0000 From: Artem Bityutskiy To: Richard Weinberger Subject: [PATCH 1/5] UBI: fastmap: add more TODOs Date: Tue, 5 Jun 2012 18:11:55 +0300 Message-Id: <1338909119-5188-2-git-send-email-dedekind1@gmail.com> In-Reply-To: <1338909119-5188-1-git-send-email-dedekind1@gmail.com> References: <1338909119-5188-1-git-send-email-dedekind1@gmail.com> Cc: MTD Maling List , Shmulik Ladkani List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Artem Bityutskiy 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 --- 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. */ 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); -- 1.7.10