[PATCH 1/5] UBI: fastmap: add more TODOs

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy at 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 at 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. */
 	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