[PATCH 00/14] ubi: Fastmap updates

[PATCH 00/14] ubi: Fastmap updates

[Richard Weinberger]

1. Support for preseeded fastmaps.
A preseeded fastmap is a fastmap created by tools such as ubinize.
That way fastmap deployment is less painful and fast attach is
available upon first boot.

2. Refine various checks.
Detect misconfigured setups better.

3. Allow a forcing a full scan.
Forcing a full scan is useful mostly for debugging and making sure that
it works.

4. Remove the experimental stigma.

Richard Weinberger (14):
  ubi: fastmap: Read PEB numbers more carefully
  ubi: Fix assert in ubi_wl_init
  ubi: fastmap: Add support for flags
  ubi: fastmap: Add UBI_FM_SB_PRESEEDED_FLG flag
  ubi: fastmap: Implement PEB translation
  ubi: fastmap: Handle bad block count for preseeded fastmap case
  ubi: fastmap: Fixup pool sizes for preseeded fastmaps
  ubi: fastmap: Scan empty space if fastmap is preseeded
  ubi: fastmap: Relax size check
  ubi: fastmap: Change a WARN_ON() to ubi_err()
  ubi: Add module parameter to force a full scan
  ubi: uapi: Add mode selector to attach request
  ubi: Wire up attach mode selector
  ubi: Remove experimental stigma from Fastmap

 drivers/mtd/ubi/Kconfig     |   7 +-
 drivers/mtd/ubi/attach.c    |  14 +-
 drivers/mtd/ubi/build.c     |  18 ++-
 drivers/mtd/ubi/cdev.c      |   7 +-
 drivers/mtd/ubi/fastmap.c   | 326 +++++++++++++++++++++++++++++++++++++++-----
 drivers/mtd/ubi/ubi-media.h |  15 +-
 drivers/mtd/ubi/ubi.h       |   7 +-
 drivers/mtd/ubi/wl.c        |  12 +-
 include/uapi/mtd/ubi-user.h |  14 +-
 9 files changed, 369 insertions(+), 51 deletions(-)

-- 
2.13.6

[PATCH 01/14] ubi: fastmap: Read PEB numbers more carefully

[Richard Weinberger]

PEB numbers can be used as indices, make sure that they
are within bounds.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c | 71 ++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 58 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 462526a10537..768fa8a76867 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -101,6 +101,29 @@ size_t ubi_calc_fm_size(struct ubi_device *ubi)
 	return roundup(size, ubi->leb_size);
 }
 
+static bool read_pnum(struct ubi_device *ubi, struct ubi_attach_info *ai,
+		     __be32 pnum, int *out_pnum)
+{
+	int ret = true;
+	int max_pnum = ubi->peb_count;
+
+	pnum = be32_to_cpu(pnum);
+	if (pnum == UBI_UNKNOWN) {
+		*out_pnum = pnum;
+		goto out;
+	}
+
+	if (pnum < 0 || pnum >= max_pnum) {
+		ubi_err(ubi, "fastmap references PEB out of range: %i", pnum);
+		ret = false;
+		goto out;
+	} else {
+		*out_pnum = pnum;
+	}
+
+out:
+	return ret;
+}
 
 /**
  * new_fm_vhdr - allocate a new volume header for fastmap usage.
@@ -438,7 +461,10 @@ static int scan_pool(struct ubi_device *ubi, struct ubi_attach_info *ai,
 		int scrub = 0;
 		int image_seq;
 
-		pnum = be32_to_cpu(pebs[i]);
+		if (!read_pnum(ubi, ai, pebs[i], &pnum)) {
+			ret = UBI_BAD_FASTMAP;
+			goto out;
+		}
 
 		if (ubi_io_is_bad(ubi, pnum)) {
 			ubi_err(ubi, "bad PEB in fastmap pool!");
@@ -565,7 +591,7 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 	struct ubi_fm_ec *fmec;
 	struct ubi_fm_volhdr *fmvhdr;
 	struct ubi_fm_eba *fm_eba;
-	int ret, i, j, pool_size, wl_pool_size;
+	int ret, i, j, pool_size, wl_pool_size, pnum;
 	size_t fm_pos = 0, fm_size = ubi->fm_size;
 	unsigned long long max_sqnum = 0;
 	void *fm_raw = ubi->fm_buf;
@@ -647,8 +673,10 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 		if (fm_pos >= fm_size)
 			goto fail_bad;
 
-		add_aeb(ai, &ai->free, be32_to_cpu(fmec->pnum),
-			be32_to_cpu(fmec->ec), 0);
+		if (!read_pnum(ubi, ai, fmec->pnum, &pnum))
+			goto fail_bad;
+
+		add_aeb(ai, &ai->free, pnum, be32_to_cpu(fmec->ec), 0);
 	}
 
 	/* read EC values from used list */
@@ -658,8 +686,10 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 		if (fm_pos >= fm_size)
 			goto fail_bad;
 
-		add_aeb(ai, &used, be32_to_cpu(fmec->pnum),
-			be32_to_cpu(fmec->ec), 0);
+		if (!read_pnum(ubi, ai, fmec->pnum, &pnum))
+			goto fail_bad;
+
+		add_aeb(ai, &used, pnum, be32_to_cpu(fmec->ec), 0);
 	}
 
 	/* read EC values from scrub list */
@@ -669,8 +699,10 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 		if (fm_pos >= fm_size)
 			goto fail_bad;
 
-		add_aeb(ai, &used, be32_to_cpu(fmec->pnum),
-			be32_to_cpu(fmec->ec), 1);
+		if (!read_pnum(ubi, ai, fmec->pnum, &pnum))
+			goto fail_bad;
+
+		add_aeb(ai, &used, pnum, be32_to_cpu(fmec->ec), 1);
 	}
 
 	/* read EC values from erase list */
@@ -680,8 +712,10 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 		if (fm_pos >= fm_size)
 			goto fail_bad;
 
-		add_aeb(ai, &ai->erase, be32_to_cpu(fmec->pnum),
-			be32_to_cpu(fmec->ec), 1);
+		if (!read_pnum(ubi, ai, fmec->pnum, &pnum))
+			goto fail_bad;
+
+		add_aeb(ai, &ai->erase, pnum, be32_to_cpu(fmec->ec), 1);
 	}
 
 	ai->mean_ec = div_u64(ai->ec_sum, ai->ec_count);
@@ -731,7 +765,8 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 		}
 
 		for (j = 0; j < be32_to_cpu(fm_eba->reserved_pebs); j++) {
-			int pnum = be32_to_cpu(fm_eba->pnum[j]);
+			if (!read_pnum(ubi, ai, fm_eba->pnum[j], &pnum))
+				goto fail_bad;
 
 			if (pnum < 0)
 				continue;
@@ -954,7 +989,10 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
 	for (i = 0; i < used_blocks; i++) {
 		int image_seq;
 
-		pnum = be32_to_cpu(fmsb->block_loc[i]);
+		if (!read_pnum(ubi, ai, fmsb->block_loc[i], &pnum)) {
+			ret = UBI_BAD_FASTMAP;
+			goto free_hdr;
+		}
 
 		if (ubi_io_is_bad(ubi, pnum)) {
 			ret = UBI_BAD_FASTMAP;
@@ -1068,7 +1106,14 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
 			goto free_hdr;
 		}
 
-		e->pnum = be32_to_cpu(fmsb2->block_loc[i]);
+		if (!read_pnum(ubi, ai, fmsb2->block_loc[i], &e->pnum)) {
+			while (i--)
+				kmem_cache_free(ubi_wl_entry_slab, fm->e[i]);
+
+			ret = -ENOMEM;
+			goto free_hdr;
+		}
+
 		e->ec = be32_to_cpu(fmsb2->block_ec[i]);
 		fm->e[i] = e;
 	}
-- 
2.13.6

[PATCH 02/14] ubi: Fix assert in ubi_wl_init

[Richard Weinberger]

When multiple PEBs are used for a fastmap, found_pebs
can be wrong and the assert triggers.

The current approach is broken in two ways:
1. The "continue" in list_for_each_entry() over all fastmap PEBs
   misses the counter at all.
2. When the fastmap changes in size, growing due to a preseeded fastmap
   or shrinking due to new bad blocks, iterating over the current
   fastmap will give wrong numbers. We have to exclude the fastmap size
   at all from the calculation to be able to compare the numbers.
   At this stage we simply have no longer the information whether the
   fastmap changed in size.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/wl.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index f66b3b22f328..6bbb968fe9da 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -1695,11 +1695,19 @@ int ubi_wl_init(struct ubi_device *ubi, struct ubi_attach_info *ai)
 			err = erase_aeb(ubi, aeb, sync);
 			if (err)
 				goto out_free;
-		}
 
-		found_pebs++;
+			/*
+			 * If no fastmap is used, all fastmap PEBs will get be
+			 * erased and are member of ai->fastmap.
+			 */
+			if (!ubi->fm)
+				found_pebs++;
+		}
 	}
 
+	if (ubi->fm)
+		found_pebs += ubi->fm->used_blocks;
+
 	dbg_wl("found %i PEBs", found_pebs);
 
 	ubi_assert(ubi->good_peb_count == found_pebs);
-- 
2.13.6

[PATCH 03/14] ubi: fastmap: Add support for flags

[Richard Weinberger]

With version 2 of fastmap, flags are supported.
We fall back to scanning mode if unsupported flags are found.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c   | 24 ++++++++++++++++++++----
 drivers/mtd/ubi/ubi-media.h | 11 +++++++++--
 drivers/mtd/ubi/ubi.h       |  2 ++
 3 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 768fa8a76867..1ebb5d15ab1a 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -949,9 +949,24 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
 		goto free_fm_sb;
 	}
 
-	if (fmsb->version != UBI_FM_FMT_VERSION) {
-		ubi_err(ubi, "bad fastmap version: %i, expected: %i",
-			fmsb->version, UBI_FM_FMT_VERSION);
+	fm->flags = be32_to_cpu(fmsb->flags);
+
+	if (fmsb->version == 1) {
+		if (fm->flags != 0) {
+			ubi_err(ubi, "fastmap flags are non-zero: %#x",
+				fm->flags);
+			ret = UBI_BAD_FASTMAP;
+			goto free_fm_sb;
+		}
+	} else if (fmsb->version == 2) {
+		if ((fm->flags & UBI_FM_SB_FLG_MASK) != UBI_FM_SB_FLG_MASK) {
+			ubi_err(ubi, "unsupported fastmap flags present: %#x",
+				fm->flags);
+			ret = UBI_BAD_FASTMAP;
+			goto free_fm_sb;
+		}
+	} else {
+		ubi_err(ubi, "bad fastmap version: %i", fmsb->version);
 		ret = UBI_BAD_FASTMAP;
 		goto free_fm_sb;
 	}
@@ -1229,10 +1244,11 @@ static int ubi_write_fastmap(struct ubi_device *ubi,
 	ubi_assert(fm_pos <= ubi->fm_size);
 
 	fmsb->magic = cpu_to_be32(UBI_FM_SB_MAGIC);
-	fmsb->version = UBI_FM_FMT_VERSION;
+	fmsb->version = UBI_FM_FMT_WRITE_VERSION;
 	fmsb->used_blocks = cpu_to_be32(new_fm->used_blocks);
 	/* the max sqnum will be filled in while *reading* the fastmap */
 	fmsb->sqnum = 0;
+	fmsb->flags = 0;
 
 	fmh->magic = cpu_to_be32(UBI_FM_HDR_MAGIC);
 	free_peb_count = 0;
diff --git a/drivers/mtd/ubi/ubi-media.h b/drivers/mtd/ubi/ubi-media.h
index 195ff8ca8211..6136a97f4844 100644
--- a/drivers/mtd/ubi/ubi-media.h
+++ b/drivers/mtd/ubi/ubi-media.h
@@ -365,7 +365,10 @@ struct ubi_vtbl_record {
 #define UBI_FM_DATA_VOLUME_ID	(UBI_LAYOUT_VOLUME_ID + 2)
 
 /* fastmap on-flash data structure format version */
-#define UBI_FM_FMT_VERSION	1
+#define UBI_FM_FMT_VERSION		2
+
+/* this implementation writes always version 1 */
+#define UBI_FM_FMT_WRITE_VERSION	1
 
 #define UBI_FM_SB_MAGIC		0x7B11D69F
 #define UBI_FM_HDR_MAGIC	0xD4B82EF7
@@ -387,6 +390,8 @@ struct ubi_vtbl_record {
 #define UBI_FM_MIN_POOL_SIZE	8
 #define UBI_FM_MAX_POOL_SIZE	256
 
+#define UBI_FM_SB_FLG_MASK	0
+
 /**
  * struct ubi_fm_sb - UBI fastmap super block
  * @magic: fastmap super block magic number (%UBI_FM_SB_MAGIC)
@@ -396,6 +401,7 @@ struct ubi_vtbl_record {
  * @block_loc: an array containing the location of all PEBs of the fastmap
  * @block_ec: the erase counter of each used PEB
  * @sqnum: highest sequence number value at the time while taking the fastmap
+ * @flags: fastmap specific flags, only used with @version > 1, zero otherwise
  *
  */
 struct ubi_fm_sb {
@@ -407,7 +413,8 @@ struct ubi_fm_sb {
 	__be32 block_loc[UBI_FM_MAX_BLOCKS];
 	__be32 block_ec[UBI_FM_MAX_BLOCKS];
 	__be64 sqnum;
-	__u8 padding2[32];
+	__be32 flags;
+	__u8 padding2[28];
 } __packed;
 
 /**
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index f5ba97c46160..4fab3790733b 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -249,6 +249,7 @@ struct ubi_volume_desc;
  * @used_blocks: number of used PEBs
  * @max_pool_size: maximal size of the user pool
  * @max_wl_pool_size: maximal size of the pool used by the WL sub-system
+ * @flags: fastmap flags
  */
 struct ubi_fastmap_layout {
 	struct ubi_wl_entry *e[UBI_FM_MAX_BLOCKS];
@@ -256,6 +257,7 @@ struct ubi_fastmap_layout {
 	int used_blocks;
 	int max_pool_size;
 	int max_wl_pool_size;
+	int flags;
 };
 
 /**
-- 
2.13.6

[PATCH 04/14] ubi: fastmap: Add UBI_FM_SB_PRESEEDED_FLG flag

[Richard Weinberger]

This flag indicates that the fastmap was preseeded, which means
it was created offline by a tool such as ubinize which cannot know
the whole MTD state such as real size and bad blocks.
As consequence UBI has to take special care to use that fastmap.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/ubi-media.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/ubi-media.h b/drivers/mtd/ubi/ubi-media.h
index 6136a97f4844..be339fb924af 100644
--- a/drivers/mtd/ubi/ubi-media.h
+++ b/drivers/mtd/ubi/ubi-media.h
@@ -390,7 +390,11 @@ struct ubi_vtbl_record {
 #define UBI_FM_MIN_POOL_SIZE	8
 #define UBI_FM_MAX_POOL_SIZE	256
 
-#define UBI_FM_SB_FLG_MASK	0
+enum {
+	UBI_FM_SB_PRESEEDED_FLG	= 0x1,
+};
+
+#define UBI_FM_SB_FLG_MASK (UBI_FM_SB_PRESEEDED_FLG)
 
 /**
  * struct ubi_fm_sb - UBI fastmap super block
-- 
2.13.6

[PATCH 05/14] ubi: fastmap: Implement PEB translation

[Richard Weinberger]

When a fastmap is preseeded we have to translate PEB numbers because
during the creation of the fastmap the creation tool cannot know
which blocks are bad on the target(s).

Therefore fastmap has to learn all bad blocks during attach and
changes PEB numbers accordingly.
This feature assumes that bad blocks are skipped while the image was
flashed, what nandwrite does by default.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/attach.c  |  1 +
 drivers/mtd/ubi/fastmap.c | 74 +++++++++++++++++++++++++++++++++++++++++++++--
 drivers/mtd/ubi/ubi.h     |  2 ++
 3 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 93ceea4f27d5..9a8072cf458c 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -1370,6 +1370,7 @@ static void destroy_ai(struct ubi_attach_info *ai)
 	}
 
 	kmem_cache_destroy(ai->aeb_slab_cache);
+	kfree(ai->bb_trans);
 	kfree(ai);
 }
 
diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 1ebb5d15ab1a..279d02874297 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -107,6 +107,9 @@ static bool read_pnum(struct ubi_device *ubi, struct ubi_attach_info *ai,
 	int ret = true;
 	int max_pnum = ubi->peb_count;
 
+	if (ai->bb_trans)
+		max_pnum -= ai->bad_peb_count;
+
 	pnum = be32_to_cpu(pnum);
 	if (pnum == UBI_UNKNOWN) {
 		*out_pnum = pnum;
@@ -117,10 +120,13 @@ static bool read_pnum(struct ubi_device *ubi, struct ubi_attach_info *ai,
 		ubi_err(ubi, "fastmap references PEB out of range: %i", pnum);
 		ret = false;
 		goto out;
-	} else {
-		*out_pnum = pnum;
 	}
 
+	if (!ai->bb_trans)
+		*out_pnum = pnum;
+	else
+		*out_pnum = ai->bb_trans[pnum];
+
 out:
 	return ret;
 }
@@ -880,6 +886,61 @@ static struct ubi_ainf_peb *clone_aeb(struct ubi_attach_info *ai,
 	return new;
 }
 
+/*
+ * build_bb_trans_table - create a translation table to fix PEB numbers.
+ * @ubi: UBI device object
+ * @ai: UBI attach info object
+ *
+ * A preseeded Fastmap has no knowledge of bad blocks. During first attach
+ * UBI has to update PEB numbers to leave out existing bad blocks.
+ */
+static int build_bb_trans_table(struct ubi_device *ubi,
+				struct ubi_attach_info *ai)
+{
+	int pnum, new_pnum, ret;
+	unsigned long *claimed_blocks;
+
+	ret = -ENOMEM;
+	claimed_blocks = kcalloc(BITS_TO_LONGS(ubi->peb_count),
+			       sizeof(unsigned long), GFP_KERNEL);
+	if (!claimed_blocks)
+		goto out;
+
+	/* ai->bb_trans will get free'ed via destroy_ai() */
+	ai->bb_trans = kcalloc(ubi->peb_count, sizeof(int), GFP_KERNEL);
+	if (!ai->bb_trans)
+		goto out;
+
+	/* Find all bad blocks and mark them as claimed */
+	for (pnum = 0; pnum < ubi->peb_count; pnum++) {
+		ret = ubi_io_is_bad(ubi, pnum);
+		if (ret < 0)
+			goto out;
+
+		if (ret == 1) {
+			set_bit(pnum, claimed_blocks);
+			ai->bad_peb_count++;
+		}
+	}
+
+	/*
+	 * Start with PEB 0 and try to place each PEB around all bad blocks
+	 * to create the translation table.
+	 */
+	for (pnum = 0; pnum < ubi->peb_count - ai->bad_peb_count; pnum++) {
+		ubi_assert(!bitmap_full(claimed_blocks, ubi->peb_count));
+
+		new_pnum = find_first_zero_bit(claimed_blocks, ubi->peb_count);
+		ai->bb_trans[pnum] = new_pnum;
+		set_bit(new_pnum, claimed_blocks);
+	}
+
+out:
+	kfree(claimed_blocks);
+
+	return ret;
+}
+
 /**
  * ubi_scan_fastmap - scan the fastmap.
  * @ubi: UBI device object
@@ -987,6 +1048,15 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
 		goto free_fm_sb;
 	}
 
+	if (fm->flags & UBI_FM_SB_PRESEEDED_FLG) {
+		ubi_msg(ubi, "preseeded fastmap found");
+		ret = build_bb_trans_table(ubi, ai);
+		if (ret) {
+			ubi_err(ubi, "failed to construct bb translation table");
+			goto free_fm_sb;
+		}
+	}
+
 	ech = kzalloc(ubi->ec_hdr_alsize, GFP_KERNEL);
 	if (!ech) {
 		ret = -ENOMEM;
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 4fab3790733b..28af5115d180 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -760,6 +760,7 @@ struct ubi_ainf_volume {
  * @aeb_slab_cache: slab cache for &struct ubi_ainf_peb objects
  * @ech: temporary EC header. Only available during scan
  * @vidh: temporary VID buffer. Only available during scan
+ * @bb_trans: bad block translation table, used by fastmap, NULL otherwise
  *
  * This data structure contains the result of attaching an MTD device and may
  * be used by other UBI sub-systems to build final UBI data structures, further
@@ -790,6 +791,7 @@ struct ubi_attach_info {
 	struct kmem_cache *aeb_slab_cache;
 	struct ubi_ec_hdr *ech;
 	struct ubi_vid_io_buf *vidb;
+	int *bb_trans;
 };
 
 /**
-- 
2.13.6

[PATCH 06/14] ubi: fastmap: Handle bad block count for preseeded fastmap case

[Richard Weinberger]

If the fastmap is preseeded the bad block count is created while
scanning for bad blocks in the PEB fixup code.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 279d02874297..69b855b3cf2a 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -725,7 +725,16 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 	}
 
 	ai->mean_ec = div_u64(ai->ec_sum, ai->ec_count);
-	ai->bad_peb_count = be32_to_cpu(fmhdr->bad_peb_count);
+
+	if (fm->flags & UBI_FM_SB_PRESEEDED_FLG) {
+		/* When we have a preseeded Fastmap we cannot use the provided bad block number */
+		if (be32_to_cpu(fmhdr->bad_peb_count) != 0) {
+			ubi_err(ubi, "Bad block count in preseeded Fastmap is non-zero");
+			goto fail_bad;
+		}
+	} else {
+		ai->bad_peb_count = be32_to_cpu(fmhdr->bad_peb_count);
+	}
 
 	/* Iterate over all volumes and read their EBA table */
 	for (i = 0; i < be32_to_cpu(fmhdr->vol_count); i++) {
-- 
2.13.6

[PATCH 07/14] ubi: fastmap: Fixup pool sizes for preseeded fastmaps

[Richard Weinberger]

A preseeded fastmap has, by definition, no dirty PEBs and therefore all
pools are empty. The creation tool can also not calculate the maximal
pool sizes. This means we have to set them during attach.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 69b855b3cf2a..976d371d7cef 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -645,8 +645,15 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 
 	pool_size = be16_to_cpu(fmpl->size);
 	wl_pool_size = be16_to_cpu(fmpl_wl->size);
-	fm->max_pool_size = be16_to_cpu(fmpl->max_size);
-	fm->max_wl_pool_size = be16_to_cpu(fmpl_wl->max_size);
+
+	if (fm->flags & UBI_FM_SB_PRESEEDED_FLG) {
+		fm->max_pool_size = ubi->fm_pool.max_size;
+		fm->max_wl_pool_size = ubi->fm_wl_pool.max_size;
+
+	} else {
+		fm->max_pool_size = be16_to_cpu(fmpl->max_size);
+		fm->max_wl_pool_size = be16_to_cpu(fmpl_wl->max_size);
+	}
 
 	if (pool_size > UBI_FM_MAX_POOL_SIZE || pool_size < 0) {
 		ubi_err(ubi, "bad pool size: %i", pool_size);
-- 
2.13.6

[PATCH 08/14] ubi: fastmap: Scan empty space if fastmap is preseeded

[Richard Weinberger]

The creation tool does not know the real size of the MTD, therefore the
preseeded fastmap references only used PEBs.
Free PEBs need to be discovered during the initial attach of the
preseeded fastmap.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c | 123 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 117 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 976d371d7cef..09a9d3a0ccf5 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -422,6 +422,96 @@ static void unmap_peb(struct ubi_attach_info *ai, int pnum)
 		}
 	}
 }
+/**
+ * scan_empty_space - scan empty space.
+ *
+ * @ubi: UBI device object
+ * @ai: attach info object
+ * @start: PEB number where empty space is expected to start
+ *
+ * Is is the fastmap preseeded, it references only used PEBs, the creation
+ * does not know the real MTD size. Therefore many PEBs are 0xff and unknown
+ * to the fastmap. Scan for this PEBs during attach and make them known.
+ * These PEBs are only allowed to be 0xff or have a valid EC header.
+ * EC headers are allowed because the initial scan+erase operation could be
+ * interrupted by a power cycle.
+ */
+static int scan_empty_space(struct ubi_device *ubi, struct ubi_attach_info *ai,
+			    int start)
+{
+	int pnum, err, scrub, empty, image_seq;
+	unsigned long long ec;
+	struct ubi_ec_hdr *ech = NULL;
+	struct ubi_vid_io_buf *vb = NULL;
+	struct ubi_vid_hdr *vh;
+
+	err = -ENOMEM;
+
+	ech = kzalloc(ubi->ec_hdr_alsize, GFP_KERNEL);
+	if (!ech)
+		goto out;
+
+	vb = ubi_alloc_vid_buf(ubi, GFP_KERNEL);
+	if (!vb)
+		goto out;
+
+	vh = ubi_get_vid_hdr(vb);
+
+	ubi_msg(ubi, "scanning %i additional PEBs", ubi->peb_count - start);
+
+	for (pnum = start; pnum < ubi->peb_count; pnum++) {
+		if (ubi_io_is_bad(ubi, pnum))
+			continue;
+
+		scrub = empty = ec = 0;
+		err = ubi_io_read_ec_hdr(ubi, pnum, ech, 0);
+		switch (err) {
+			case UBI_IO_FF_BITFLIPS:
+				scrub = 1;
+				/* fall through */
+			case UBI_IO_FF:
+				empty = 1;
+				break;
+			case UBI_IO_BITFLIPS:
+				scrub = 1;
+				/* fall through */
+			case 0:
+				ec = be64_to_cpu(ech->ec);
+				image_seq = be32_to_cpu(ech->image_seq);
+
+				if (image_seq && (image_seq != ubi->image_seq)) {
+					ubi_err(ubi, "bad image seq: 0x%x, expected: 0x%x",
+						image_seq, ubi->image_seq);
+					err = UBI_BAD_FASTMAP;
+					goto out;
+				}
+				break;
+			default:
+				ubi_err(ubi, "Unable to read EC header in empty space: %i",
+					err);
+				err = UBI_BAD_FASTMAP;
+				goto out;
+		}
+
+		err = ubi_io_read_vid_hdr(ubi, pnum, vb, 0);
+		if (err != UBI_IO_FF && err != UBI_IO_FF_BITFLIPS) {
+			ubi_err(ubi, "Unexpected data in empty space: %i", err);
+			err = UBI_BAD_FASTMAP;
+			goto out;
+		}
+
+		if (empty)
+			add_aeb(ai, &ai->erase, pnum, ec, scrub);
+		else
+			add_aeb(ai, &ai->free, pnum, ec, scrub);
+	}
+
+	err = 0;
+out:
+	kfree(ech);
+	ubi_free_vid_buf(vb);
+	return err;
+}
 
 /**
  * scan_pool - scans a pool for changed (no longer empty PEBs).
@@ -818,13 +908,34 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 		}
 	}
 
-	ret = scan_pool(ubi, ai, fmpl->pebs, pool_size, &max_sqnum, &free);
-	if (ret)
-		goto fail;
+	if (!(fm->flags & UBI_FM_SB_PRESEEDED_FLG)) {
+		ret = scan_pool(ubi, ai, fmpl->pebs, pool_size, &max_sqnum, &free);
+		if (ret)
+			goto fail;
 
-	ret = scan_pool(ubi, ai, fmpl_wl->pebs, wl_pool_size, &max_sqnum, &free);
-	if (ret)
-		goto fail;
+		ret = scan_pool(ubi, ai, fmpl_wl->pebs, wl_pool_size, &max_sqnum, &free);
+		if (ret)
+			goto fail;
+	}
+
+	if (fm->flags & UBI_FM_SB_PRESEEDED_FLG) {
+		int empty_start = be32_to_cpu(fmhdr->used_peb_count) + \
+				  be32_to_cpu(fmhdr->erase_peb_count) + fm->used_blocks;
+
+		if (empty_start + ai->bad_peb_count > ubi->peb_count) {
+			ubi_err(ubi, "fastmap points beyond end of device!");
+			goto fail_bad;
+		} else if (empty_start + ai->bad_peb_count == ubi->peb_count) {
+			ubi_msg(ubi, "no need to scan empty space");
+		} else {
+			if (!read_pnum(ubi, ai, cpu_to_be32(empty_start), &empty_start))
+				goto fail_bad;
+
+			ret = scan_empty_space(ubi, ai, empty_start);
+			if (ret)
+				goto fail;
+		}
+	}
 
 	if (max_sqnum > ai->max_sqnum)
 		ai->max_sqnum = max_sqnum;
-- 
2.13.6

[PATCH 09/14] ubi: fastmap: Relax size check

[Richard Weinberger]

While attaching via fastmap we verify whether the found fastmap
is as large as we have computed.
With preseeded Fastmaps this assumtion can fail since ubinize cannot
know the total size of the MTD and uses the number if used PEBs for
the calculation.
Therefore the found fastmap might be smaller than the kernel expects.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 09a9d3a0ccf5..dabeb01af24a 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -1168,7 +1168,7 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
 	}
 
 	fm_size = ubi->leb_size * used_blocks;
-	if (fm_size != ubi->fm_size) {
+	if (fm_size > ubi->fm_size) {
 		ubi_err(ubi, "bad fastmap size: %zi, expected: %zi",
 			fm_size, ubi->fm_size);
 		ret = UBI_BAD_FASTMAP;
-- 
2.13.6

[PATCH 10/14] ubi: fastmap: Change a WARN_ON() to ubi_err()

[Richard Weinberger]

This WARN_ON() was used while development of fastmap, now
it can be a regular ubi_err().

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index dabeb01af24a..d5506152c9f7 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -949,14 +949,16 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 	ubi_assert(list_empty(&free));
 
 	/*
-	 * If fastmap is leaking PEBs (must not happen), raise a
-	 * fat warning and fall back to scanning mode.
-	 * We do this here because in ubi_wl_init() it's too late
-	 * and we cannot fall back to scanning.
+	 * The orginal purpose of this check was detecting fastmap bugs were
+	 * it missed blocks. Now it helps to detect mis-flashed UBIs.
+	 * E.g. when a fastmap enabled UBI is copied to another target with
+	 * different bad blocks.
 	 */
-	if (WARN_ON(count_fastmap_pebs(ai) != ubi->peb_count -
-		    ai->bad_peb_count - fm->used_blocks))
+	if (count_fastmap_pebs(ai) != ubi->peb_count - ai->bad_peb_count -
+		fm->used_blocks) {
+		ubi_err(ubi, "number of PEBs referenced by fastmap does not match MTD!");
 		goto fail_bad;
+	}
 
 	return 0;
 
-- 
2.13.6

[PATCH 11/14] ubi: Add module parameter to force a full scan

[Richard Weinberger]

Using this parameter one can force UBI do to a full scan
instead of using a fastmap.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/attach.c | 13 +++++++++----
 drivers/mtd/ubi/build.c  |  5 ++++-
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 9a8072cf458c..134b15f093c3 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -925,7 +925,7 @@ static int check_corruption(struct ubi_device *ubi, struct ubi_vid_hdr *vid_hdr,
 	return err;
 }
 
-static bool vol_ignored(int vol_id)
+static bool vol_ignored(struct ubi_attach_info *ai, int vol_id)
 {
 	switch (vol_id) {
 		case UBI_LAYOUT_VOLUME_ID:
@@ -933,6 +933,9 @@ static bool vol_ignored(int vol_id)
 	}
 
 #ifdef CONFIG_MTD_UBI_FASTMAP
+	if (ai->force_full_scan)
+		return false;
+
 	return ubi_is_fm_vol(vol_id);
 #else
 	return false;
@@ -1143,7 +1146,7 @@ static int scan_peb(struct ubi_device *ubi, struct ubi_attach_info *ai,
 	}
 
 	vol_id = be32_to_cpu(vidh->vol_id);
-	if (vol_id > UBI_MAX_VOLUMES && !vol_ignored(vol_id)) {
+	if (vol_id > UBI_MAX_VOLUMES && !vol_ignored(ai, vol_id)) {
 		int lnum = be32_to_cpu(vidh->lnum);
 
 		/* Unsupported internal volume */
@@ -1581,9 +1584,11 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
 		force_scan = 1;
 	}
 
-	if (force_scan)
+	if (force_scan) {
+		ubi_msg(ubi, "full scan forced");
+		ai->force_full_scan = 1;
 		err = scan_all(ubi, ai, 0);
-	else {
+	} else {
 		err = scan_fast(ubi, &ai);
 		if (err > 0 || mtd_is_eccerr(err)) {
 			if (err != UBI_NO_FASTMAP) {
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index d2a726654ff1..1e3f75ede985 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -83,6 +83,7 @@ static struct mtd_dev_param mtd_dev_param[UBI_MAX_DEVICES];
 static bool fm_autoconvert;
 static bool fm_debug;
 #endif
+static bool force_scan;
 
 /* Slab cache for wear-leveling entries */
 struct kmem_cache *ubi_wl_entry_slab;
@@ -956,7 +957,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
 	if (!ubi->fm_buf)
 		goto out_free;
 #endif
-	err = ubi_attach(ubi, 0);
+	err = ubi_attach(ubi, force_scan);
 	if (err) {
 		ubi_err(ubi, "failed to attach mtd%d, error %d",
 			mtd->index, err);
@@ -1458,6 +1459,8 @@ module_param(fm_autoconvert, bool, 0644);
 MODULE_PARM_DESC(fm_autoconvert, "Set this parameter to enable fastmap automatically on images without a fastmap.");
 module_param(fm_debug, bool, 0);
 MODULE_PARM_DESC(fm_debug, "Set this parameter to enable fastmap debugging by default. Warning, this will make fastmap slow!");
+module_param(force_scan, bool, 0644);
+MODULE_PARM_DESC(force_scan, "Always do a full scan of the MTD and drop possible fastmap structures from the MTD.");
 #endif
 MODULE_VERSION(__stringify(UBI_VERSION));
 MODULE_DESCRIPTION("UBI - Unsorted Block Images");
-- 
2.13.6

[PATCH 12/14] ubi: uapi: Add mode selector to attach request

[Richard Weinberger]

This allows userspace, ubiattach, to force a full scan.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 include/uapi/mtd/ubi-user.h | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/uapi/mtd/ubi-user.h b/include/uapi/mtd/ubi-user.h
index 5b04a494d139..cc0e54cfa7f9 100644
--- a/include/uapi/mtd/ubi-user.h
+++ b/include/uapi/mtd/ubi-user.h
@@ -235,12 +235,19 @@ enum {
 	UBI_VOL_PROP_DIRECT_WRITE = 1,
 };
 
+enum {
+	UBI_ATTACH_MODE_AUTO = 0x0,
+	UBI_ATTACH_MODE_SCAN = 0x1,
+	UBI_ATTACH_MODE_MAX = UBI_ATTACH_MODE_SCAN,
+};
+
 /**
  * struct ubi_attach_req - attach MTD device request.
  * @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 number of bad PEB per 1024 PEBs
+ * @attach_mode: selects the attach mode
  * @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
@@ -276,13 +283,18 @@ enum {
  * eraseblocks for new bad eraseblocks, but attempts to use available
  * eraseblocks (if any). The accepted range is 0-768. If 0 is given, the
  * default kernel value of %CONFIG_MTD_UBI_BEB_LIMIT will be used.
+ *
+ * @attach_mode is %UBI_ATTACH_MODE_AUTO by default, and let's the UBI
+ * implementation decide how to attach. If %UBI_ATTACH_MODE_SCAN is selected
+ * a full scan is forced.
  */
 struct ubi_attach_req {
 	__s32 ubi_num;
 	__s32 mtd_num;
 	__s32 vid_hdr_offset;
 	__s16 max_beb_per1024;
-	__s8 padding[10];
+	__s8 attach_mode;
+	__s8 padding[9];
 };
 
 /**
-- 
2.13.6

[PATCH 13/14] ubi: Wire up attach mode selector

[Richard Weinberger]

Now userspace can specify how UBI shall attach.
By default UBI decides how to attach, but userspace can
also force a full scan.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/build.c | 15 ++++++++++++---
 drivers/mtd/ubi/cdev.c  |  7 ++++++-
 drivers/mtd/ubi/ubi.h   |  3 ++-
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 1e3f75ede985..7d08b6498b2a 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -805,6 +805,7 @@ static int autoresize(struct ubi_device *ubi, int vol_id)
  * @ubi_num: number to assign to the new UBI device
  * @vid_hdr_offset: VID header offset
  * @max_beb_per1024: maximum expected number of bad PEB per 1024 PEBs
+ * @attach_mode: selects how to attach
  *
  * 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
@@ -816,10 +817,12 @@ static int autoresize(struct ubi_device *ubi, int vol_id)
  * @ubi_devices_mutex.
  */
 int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
-		       int vid_hdr_offset, int max_beb_per1024)
+		       int vid_hdr_offset, int max_beb_per1024,
+		       int attach_mode)
 {
 	struct ubi_device *ubi;
 	int i, err;
+	int do_full_scan = false;
 
 	if (max_beb_per1024 < 0 || max_beb_per1024 > MAX_MTD_UBI_BEB_LIMIT)
 		return -EINVAL;
@@ -957,7 +960,12 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
 	if (!ubi->fm_buf)
 		goto out_free;
 #endif
-	err = ubi_attach(ubi, force_scan);
+	if (force_scan)
+		do_full_scan = 1;
+	else
+		do_full_scan = !!attach_mode;
+
+	err = ubi_attach(ubi, do_full_scan);
 	if (err) {
 		ubi_err(ubi, "failed to attach mtd%d, error %d",
 			mtd->index, err);
@@ -1240,7 +1248,8 @@ static int __init ubi_init(void)
 
 		mutex_lock(&ubi_devices_mutex);
 		err = ubi_attach_mtd_dev(mtd, p->ubi_num,
-					 p->vid_hdr_offs, p->max_beb_per1024);
+					 p->vid_hdr_offs, p->max_beb_per1024,
+					 UBI_ATTACH_MODE_AUTO);
 		mutex_unlock(&ubi_devices_mutex);
 		if (err < 0) {
 			pr_err("UBI error: cannot attach mtd%d\n",
diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
index 45c329694a5e..cf9b2b00e8c5 100644
--- a/drivers/mtd/ubi/cdev.c
+++ b/drivers/mtd/ubi/cdev.c
@@ -1001,6 +1001,11 @@ static long ctrl_cdev_ioctl(struct file *file, unsigned int cmd,
 			break;
 		}
 
+		if (req.attach_mode < 0 || req.attach_mode > UBI_ATTACH_MODE_MAX) {
+			err = -EINVAL;
+			break;
+		}
+
 		mtd = get_mtd_device(NULL, req.mtd_num);
 		if (IS_ERR(mtd)) {
 			err = PTR_ERR(mtd);
@@ -1013,7 +1018,7 @@ static long ctrl_cdev_ioctl(struct file *file, unsigned int cmd,
 		 */
 		mutex_lock(&ubi_devices_mutex);
 		err = ubi_attach_mtd_dev(mtd, req.ubi_num, req.vid_hdr_offset,
-					 req.max_beb_per1024);
+					 req.max_beb_per1024, req.attach_mode);
 		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 28af5115d180..0b2f79f68986 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -949,7 +949,8 @@ int ubi_io_write_vid_hdr(struct ubi_device *ubi, int pnum,
 
 /* build.c */
 int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
-		       int vid_hdr_offset, int max_beb_per1024);
+		       int vid_hdr_offset, int max_beb_per1024,
+		       int attach_mode);
 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);
-- 
2.13.6

[PATCH 14/14] ubi: Remove experimental stigma from Fastmap

[Richard Weinberger]

I've hold up this a way too long. Over the last two years Fastmap gained
many users which made it possible to sort out the last issues.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/Kconfig | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/ubi/Kconfig b/drivers/mtd/ubi/Kconfig
index 43d131f5ae10..4de5e4e8d4ab 100644
--- a/drivers/mtd/ubi/Kconfig
+++ b/drivers/mtd/ubi/Kconfig
@@ -57,12 +57,9 @@ config MTD_UBI_BEB_LIMIT
 	  Leave the default value if unsure.
 
 config MTD_UBI_FASTMAP
-	bool "UBI Fastmap (Experimental feature)"
+	bool "UBI Fastmap"
 	default n
 	help
-	   Important: this feature is experimental so far and the on-flash
-	   format for fastmap may change in the next kernel versions
-
 	   Fastmap is a mechanism which allows attaching an UBI device
 	   in nearly constant time. Instead of scanning the whole MTD device it
 	   only has to locate a checkpoint (called fastmap) on the device.
@@ -74,6 +71,8 @@ config MTD_UBI_FASTMAP
 	   images are still usable with UBI implementations without
 	   fastmap support. On typical flash devices the whole fastmap fits
 	   into one PEB. UBI will reserve PEBs to hold two fastmaps.
+	   While Fastmap is considered correct it weakens UBI wrt. robustness,
+	   a full scan of the MTD allows UBI to react better to bad flashes.
 
 	   If in doubt, say "N".
 
-- 
2.13.6

Re: [PATCH 01/14] ubi: fastmap: Read PEB numbers more carefully

[kbuild test robot]

Hi Richard,

I love your patch! Perhaps something to improve:

[auto build test WARNING on mtd/master]
[also build test WARNING on v4.17 next-20180613]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Richard-Weinberger/ubi-Fastmap-updates/20180614-052830
base:   git://git.infradead.org/linux-mtd.git master
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/mtd/ubi/fastmap.c:110:14: sparse: incorrect type in assignment (different base types) @@    expected restricted __be32 [usertype] pnum @@    got  [usertype] pnum @@
   drivers/mtd/ubi/fastmap.c:110:14:    expected restricted __be32 [usertype] pnum
   drivers/mtd/ubi/fastmap.c:110:14:    got unsigned int
>> drivers/mtd/ubi/fastmap.c:111:13: sparse: restricted __be32 degrades to integer
>> drivers/mtd/ubi/fastmap.c:112:27: sparse: incorrect type in assignment (different base types) @@    expected int [signed] <noident> @@    got restricted __be3int [signed] <noident> @@
   drivers/mtd/ubi/fastmap.c:112:27:    expected int [signed] <noident>
   drivers/mtd/ubi/fastmap.c:112:27:    got restricted __be32 [usertype] pnum
   drivers/mtd/ubi/fastmap.c:116:13: sparse: restricted __be32 degrades to integer
   drivers/mtd/ubi/fastmap.c:116:25: sparse: restricted __be32 degrades to integer
   drivers/mtd/ubi/fastmap.c:121:27: sparse: incorrect type in assignment (different base types) @@    expected int [signed] <noident> @@    got restricted __be3int [signed] <noident> @@
   drivers/mtd/ubi/fastmap.c:121:27:    expected int [signed] <noident>
   drivers/mtd/ubi/fastmap.c:121:27:    got restricted __be32 [usertype] pnum
   drivers/mtd/ubi/fastmap.c:604:23: sparse: incorrect type in assignment (different base types) @@    expected unsigned long long [unsigned] max_sqnum @@    got nsigned long long [unsigned] max_sqnum @@
   drivers/mtd/ubi/fastmap.c:604:23:    expected unsigned long long [unsigned] max_sqnum
   drivers/mtd/ubi/fastmap.c:604:23:    got restricted __be64 [usertype] sqnum
   drivers/mtd/ubi/fastmap.c:1075:17: sparse: incorrect type in assignment (different base types) @@    expected restricted __be32 [usertype] tmp_crc @@    got  [usertype] tmp_crc @@
   drivers/mtd/ubi/fastmap.c:1075:17:    expected restricted __be32 [usertype] tmp_crc
   drivers/mtd/ubi/fastmap.c:1075:17:    got unsigned int
   drivers/mtd/ubi/fastmap.c:1077:13: sparse: incorrect type in assignment (different base types) @@    expected restricted __be32 [usertype] crc @@    got  [usertype] crc @@
   drivers/mtd/ubi/fastmap.c:1077:13:    expected restricted __be32 [usertype] crc
   drivers/mtd/ubi/fastmap.c:1077:13:    got unsigned int
   drivers/mtd/ubi/fastmap.c:1086:22: sparse: incorrect type in assignment (different base types) @@    expected restricted __be64 [usertype] sqnum @@    got unsigned lonrestricted __be64 [usertype] sqnum @@
   drivers/mtd/ubi/fastmap.c:1086:22:    expected restricted __be64 [usertype] sqnum
   drivers/mtd/ubi/fastmap.c:1086:22:    got unsigned long long [unsigned] [assigned] sqnum

vim +110 drivers/mtd/ubi/fastmap.c

   103	
   104	static bool read_pnum(struct ubi_device *ubi, struct ubi_attach_info *ai,
   105			     __be32 pnum, int *out_pnum)
   106	{
   107		int ret = true;
   108		int max_pnum = ubi->peb_count;
   109	
 > 110		pnum = be32_to_cpu(pnum);
 > 111		if (pnum == UBI_UNKNOWN) {
 > 112			*out_pnum = pnum;
   113			goto out;
   114		}
   115	
   116		if (pnum < 0 || pnum >= max_pnum) {
   117			ubi_err(ubi, "fastmap references PEB out of range: %i", pnum);
   118			ret = false;
   119			goto out;
   120		} else {
   121			*out_pnum = pnum;
   122		}
   123	
   124	out:
   125		return ret;
   126	}
   127	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH 01/14] ubi: fastmap: Read PEB numbers more carefully

[Richard Weinberger]

Am Donnerstag, 14. Juni 2018, 03:04:40 CEST schrieb kbuild test robot:
> Hi Richard,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on mtd/master]
> [also build test WARNING on v4.17 next-20180613]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Richard-Weinberger/ubi-Fastmap-updates/20180614-052830
> base:   git://git.infradead.org/linux-mtd.git master
> reproduce:
>         # apt-get install sparse
>         make ARCH=x86_64 allmodconfig
>         make C=1 CF=-D__CHECK_ENDIAN__
> 
> 
> sparse warnings: (new ones prefixed by >>)
> 
> >> drivers/mtd/ubi/fastmap.c:110:14: sparse: incorrect type in assignment (different base types) @@    expected restricted __be32 [usertype] pnum @@    got  [usertype] pnum @@
>    drivers/mtd/ubi/fastmap.c:110:14:    expected restricted __be32 [usertype] pnum
>    drivers/mtd/ubi/fastmap.c:110:14:    got unsigned int
> >> drivers/mtd/ubi/fastmap.c:111:13: sparse: restricted __be32 degrades to integer
> >> drivers/mtd/ubi/fastmap.c:112:27: sparse: incorrect type in assignment (different base types) @@    expected int [signed] <noident> @@    got restricted __be3int [signed] <noident> @@
>    drivers/mtd/ubi/fastmap.c:112:27:    expected int [signed] <noident>
>    drivers/mtd/ubi/fastmap.c:112:27:    got restricted __be32 [usertype] pnum
>    drivers/mtd/ubi/fastmap.c:116:13: sparse: restricted __be32 degrades to integer
>    drivers/mtd/ubi/fastmap.c:116:25: sparse: restricted __be32 degrades to integer
>    drivers/mtd/ubi/fastmap.c:121:27: sparse: incorrect type in assignment (different base types) @@    expected int [signed] <noident> @@    got restricted __be3int [signed] <noident> @@
>    drivers/mtd/ubi/fastmap.c:121:27:    expected int [signed] <noident>
>    drivers/mtd/ubi/fastmap.c:121:27:    got restricted __be32 [usertype] pnum
>    drivers/mtd/ubi/fastmap.c:604:23: sparse: incorrect type in assignment (different base types) @@    expected unsigned long long [unsigned] max_sqnum @@    got nsigned long long [unsigned] max_sqnum @@
>    drivers/mtd/ubi/fastmap.c:604:23:    expected unsigned long long [unsigned] max_sqnum
>    drivers/mtd/ubi/fastmap.c:604:23:    got restricted __be64 [usertype] sqnum
>    drivers/mtd/ubi/fastmap.c:1075:17: sparse: incorrect type in assignment (different base types) @@    expected restricted __be32 [usertype] tmp_crc @@    got  [usertype] tmp_crc @@
>    drivers/mtd/ubi/fastmap.c:1075:17:    expected restricted __be32 [usertype] tmp_crc
>    drivers/mtd/ubi/fastmap.c:1075:17:    got unsigned int
>    drivers/mtd/ubi/fastmap.c:1077:13: sparse: incorrect type in assignment (different base types) @@    expected restricted __be32 [usertype] crc @@    got  [usertype] crc @@
>    drivers/mtd/ubi/fastmap.c:1077:13:    expected restricted __be32 [usertype] crc
>    drivers/mtd/ubi/fastmap.c:1077:13:    got unsigned int
>    drivers/mtd/ubi/fastmap.c:1086:22: sparse: incorrect type in assignment (different base types) @@    expected restricted __be64 [usertype] sqnum @@    got unsigned lonrestricted __be64 [usertype] sqnum @@
>    drivers/mtd/ubi/fastmap.c:1086:22:    expected restricted __be64 [usertype] sqnum
>    drivers/mtd/ubi/fastmap.c:1086:22:    got unsigned long long [unsigned] [assigned] sqnum
> 
> vim +110 drivers/mtd/ubi/fastmap.c
> 
>    103	
>    104	static bool read_pnum(struct ubi_device *ubi, struct ubi_attach_info *ai,
>    105			     __be32 pnum, int *out_pnum)
>    106	{
>    107		int ret = true;
>    108		int max_pnum = ubi->peb_count;
>    109	
>  > 110		pnum = be32_to_cpu(pnum);
>  > 111		if (pnum == UBI_UNKNOWN) {
>  > 112			*out_pnum = pnum;
>    113			goto out;
>    114		}

Okay, let's use a new variable of type int instead of reusing pnum.

Thanks,
//richard

Re: [PATCH 02/14] ubi: Fix assert in ubi_wl_init

[Boris Brezillon]

On Wed, 13 Jun 2018 23:23:32 +0200
Richard Weinberger <richard@nod.at> wrote:

> When multiple PEBs are used for a fastmap, found_pebs
> can be wrong and the assert triggers.
> 
> The current approach is broken in two ways:
> 1. The "continue" in list_for_each_entry() over all fastmap PEBs
>    misses the counter at all.
> 2. When the fastmap changes in size, growing due to a preseeded fastmap
>    or shrinking due to new bad blocks, iterating over the current
>    fastmap will give wrong numbers. We have to exclude the fastmap size
>    at all from the calculation to be able to compare the numbers.
>    At this stage we simply have no longer the information whether the
>    fastmap changed in size.

Should this patch be backported to stable releases? You say the problem
arises when new bad blocks appear, so it's not only a problem you'll
have with the preseeded fastmap changes.

> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/mtd/ubi/wl.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
> index f66b3b22f328..6bbb968fe9da 100644
> --- a/drivers/mtd/ubi/wl.c
> +++ b/drivers/mtd/ubi/wl.c
> @@ -1695,11 +1695,19 @@ int ubi_wl_init(struct ubi_device *ubi, struct ubi_attach_info *ai)
>  			err = erase_aeb(ubi, aeb, sync);
>  			if (err)
>  				goto out_free;
> -		}
>  
> -		found_pebs++;
> +			/*
> +			 * If no fastmap is used, all fastmap PEBs will get be

				remove either "get" or "be" here ^

> +			 * erased and are member of ai->fastmap.
> +			 */
> +			if (!ubi->fm)
> +				found_pebs++;
> +		}
>  	}
>  
> +	if (ubi->fm)
> +		found_pebs += ubi->fm->used_blocks;
> +

Hm, are we sure this is always correct? I mean, what if you had an old
fastmap scheduled for erasure but a power-cut happened before it was
erased. Are you sure we won't have an inconsistent found_pebs number
(found_pebs != ubi->good_peb_count).

I understand that this problem already exists because of

	if (ubi->lookuptbl[aeb->pnum])
		continue;

but I'm not sure your solution fixes that.

>  	dbg_wl("found %i PEBs", found_pebs);
>  
>  	ubi_assert(ubi->good_peb_count == found_pebs);

Re: [PATCH 01/14] ubi: fastmap: Read PEB numbers more carefully

[Boris Brezillon]

On Wed, 13 Jun 2018 23:23:31 +0200
Richard Weinberger <richard@nod.at> wrote:

> PEB numbers can be used as indices, make sure that they
> are within bounds.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/mtd/ubi/fastmap.c | 71 ++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 58 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index 462526a10537..768fa8a76867 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -101,6 +101,29 @@ size_t ubi_calc_fm_size(struct ubi_device *ubi)
>  	return roundup(size, ubi->leb_size);
>  }
>  
> +static bool read_pnum(struct ubi_device *ubi, struct ubi_attach_info *ai,
> +		     __be32 pnum, int *out_pnum)

Why not returning an int here, with negative values meaning that
pnum is invalid and positive values encoding the actual PEB number?
Also, I think check_pnum(), validate_pnum() or decode_pnum() would be
better names than read_pnum().


> +{
> +	int ret = true;
> +	int max_pnum = ubi->peb_count;
> +
> +	pnum = be32_to_cpu(pnum);
> +	if (pnum == UBI_UNKNOWN) {
> +		*out_pnum = pnum;
> +		goto out;
> +	}
> +
> +	if (pnum < 0 || pnum >= max_pnum) {
> +		ubi_err(ubi, "fastmap references PEB out of range: %i", pnum);
> +		ret = false;
> +		goto out;
> +	} else {
> +		*out_pnum = pnum;
> +	}
> +
> +out:
> +	return ret;
> +}
>  
>  /**
>   * new_fm_vhdr - allocate a new volume header for fastmap usage.
> @@ -438,7 +461,10 @@ static int scan_pool(struct ubi_device *ubi, struct ubi_attach_info *ai,
>  		int scrub = 0;
>  		int image_seq;
>  
> -		pnum = be32_to_cpu(pebs[i]);
> +		if (!read_pnum(ubi, ai, pebs[i], &pnum)) {
> +			ret = UBI_BAD_FASTMAP;
> +			goto out;
> +		}
>  
>  		if (ubi_io_is_bad(ubi, pnum)) {
>  			ubi_err(ubi, "bad PEB in fastmap pool!");
> @@ -565,7 +591,7 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
>  	struct ubi_fm_ec *fmec;
>  	struct ubi_fm_volhdr *fmvhdr;
>  	struct ubi_fm_eba *fm_eba;
> -	int ret, i, j, pool_size, wl_pool_size;
> +	int ret, i, j, pool_size, wl_pool_size, pnum;
>  	size_t fm_pos = 0, fm_size = ubi->fm_size;
>  	unsigned long long max_sqnum = 0;
>  	void *fm_raw = ubi->fm_buf;
> @@ -647,8 +673,10 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
>  		if (fm_pos >= fm_size)
>  			goto fail_bad;
>  
> -		add_aeb(ai, &ai->free, be32_to_cpu(fmec->pnum),
> -			be32_to_cpu(fmec->ec), 0);
> +		if (!read_pnum(ubi, ai, fmec->pnum, &pnum))
> +			goto fail_bad;
> +
> +		add_aeb(ai, &ai->free, pnum, be32_to_cpu(fmec->ec), 0);
>  	}
>  
>  	/* read EC values from used list */
> @@ -658,8 +686,10 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
>  		if (fm_pos >= fm_size)
>  			goto fail_bad;
>  
> -		add_aeb(ai, &used, be32_to_cpu(fmec->pnum),
> -			be32_to_cpu(fmec->ec), 0);
> +		if (!read_pnum(ubi, ai, fmec->pnum, &pnum))
> +			goto fail_bad;
> +
> +		add_aeb(ai, &used, pnum, be32_to_cpu(fmec->ec), 0);
>  	}
>  
>  	/* read EC values from scrub list */
> @@ -669,8 +699,10 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
>  		if (fm_pos >= fm_size)
>  			goto fail_bad;
>  
> -		add_aeb(ai, &used, be32_to_cpu(fmec->pnum),
> -			be32_to_cpu(fmec->ec), 1);
> +		if (!read_pnum(ubi, ai, fmec->pnum, &pnum))
> +			goto fail_bad;
> +
> +		add_aeb(ai, &used, pnum, be32_to_cpu(fmec->ec), 1);
>  	}
>  
>  	/* read EC values from erase list */
> @@ -680,8 +712,10 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
>  		if (fm_pos >= fm_size)
>  			goto fail_bad;
>  
> -		add_aeb(ai, &ai->erase, be32_to_cpu(fmec->pnum),
> -			be32_to_cpu(fmec->ec), 1);
> +		if (!read_pnum(ubi, ai, fmec->pnum, &pnum))
> +			goto fail_bad;
> +
> +		add_aeb(ai, &ai->erase, pnum, be32_to_cpu(fmec->ec), 1);
>  	}
>  
>  	ai->mean_ec = div_u64(ai->ec_sum, ai->ec_count);
> @@ -731,7 +765,8 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
>  		}
>  
>  		for (j = 0; j < be32_to_cpu(fm_eba->reserved_pebs); j++) {
> -			int pnum = be32_to_cpu(fm_eba->pnum[j]);
> +			if (!read_pnum(ubi, ai, fm_eba->pnum[j], &pnum))
> +				goto fail_bad;
>  
>  			if (pnum < 0)
>  				continue;
> @@ -954,7 +989,10 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
>  	for (i = 0; i < used_blocks; i++) {
>  		int image_seq;
>  
> -		pnum = be32_to_cpu(fmsb->block_loc[i]);
> +		if (!read_pnum(ubi, ai, fmsb->block_loc[i], &pnum)) {
> +			ret = UBI_BAD_FASTMAP;
> +			goto free_hdr;
> +		}
>  
>  		if (ubi_io_is_bad(ubi, pnum)) {
>  			ret = UBI_BAD_FASTMAP;
> @@ -1068,7 +1106,14 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
>  			goto free_hdr;
>  		}
>  
> -		e->pnum = be32_to_cpu(fmsb2->block_loc[i]);
> +		if (!read_pnum(ubi, ai, fmsb2->block_loc[i], &e->pnum)) {
> +			while (i--)
> +				kmem_cache_free(ubi_wl_entry_slab, fm->e[i]);
> +
> +			ret = -ENOMEM;
> +			goto free_hdr;
> +		}
> +
>  		e->ec = be32_to_cpu(fmsb2->block_ec[i]);
>  		fm->e[i] = e;
>  	}

Re: [PATCH 03/14] ubi: fastmap: Add support for flags

[Boris Brezillon]

On Wed, 13 Jun 2018 23:23:33 +0200
Richard Weinberger <richard@nod.at> wrote:

> With version 2 of fastmap, flags are supported.
> We fall back to scanning mode if unsupported flags are found.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/mtd/ubi/fastmap.c   | 24 ++++++++++++++++++++----
>  drivers/mtd/ubi/ubi-media.h | 11 +++++++++--
>  drivers/mtd/ubi/ubi.h       |  2 ++
>  3 files changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index 768fa8a76867..1ebb5d15ab1a 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -949,9 +949,24 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
>  		goto free_fm_sb;
>  	}
>  
> -	if (fmsb->version != UBI_FM_FMT_VERSION) {
> -		ubi_err(ubi, "bad fastmap version: %i, expected: %i",
> -			fmsb->version, UBI_FM_FMT_VERSION);
> +	fm->flags = be32_to_cpu(fmsb->flags);
> +
> +	if (fmsb->version == 1) {
> +		if (fm->flags != 0) {
> +			ubi_err(ubi, "fastmap flags are non-zero: %#x",
> +				fm->flags);
> +			ret = UBI_BAD_FASTMAP;
> +			goto free_fm_sb;
> +		}
> +	} else if (fmsb->version == 2) {
> +		if ((fm->flags & UBI_FM_SB_FLG_MASK) != UBI_FM_SB_FLG_MASK) {

Do you mean

		if (fm->flags & ~UBI_FM_SB_FLG_MASK) {

?

Because you test will return an error if at least one of all the
possible flags are not set, which I guess is a valid case.

> +			ubi_err(ubi, "unsupported fastmap flags present: %#x",
> +				fm->flags);
> +			ret = UBI_BAD_FASTMAP;
> +			goto free_fm_sb;
> +		}

Is there really a point in supporting FM v2 so early in the patch
series? I mean, the new thing in v2 is flag support, and here you fail
when ->flags != 0. Better add support for at least one flag before
saying you support v2 IMO.

> +	} else {
> +		ubi_err(ubi, "bad fastmap version: %i", fmsb->version);
>  		ret = UBI_BAD_FASTMAP;
>  		goto free_fm_sb;
>  	}
> @@ -1229,10 +1244,11 @@ static int ubi_write_fastmap(struct ubi_device *ubi,
>  	ubi_assert(fm_pos <= ubi->fm_size);
>  
>  	fmsb->magic = cpu_to_be32(UBI_FM_SB_MAGIC);
> -	fmsb->version = UBI_FM_FMT_VERSION;
> +	fmsb->version = UBI_FM_FMT_WRITE_VERSION;
>  	fmsb->used_blocks = cpu_to_be32(new_fm->used_blocks);
>  	/* the max sqnum will be filled in while *reading* the fastmap */
>  	fmsb->sqnum = 0;
> +	fmsb->flags = 0;
>  
>  	fmh->magic = cpu_to_be32(UBI_FM_HDR_MAGIC);
>  	free_peb_count = 0;
> diff --git a/drivers/mtd/ubi/ubi-media.h b/drivers/mtd/ubi/ubi-media.h
> index 195ff8ca8211..6136a97f4844 100644
> --- a/drivers/mtd/ubi/ubi-media.h
> +++ b/drivers/mtd/ubi/ubi-media.h
> @@ -365,7 +365,10 @@ struct ubi_vtbl_record {
>  #define UBI_FM_DATA_VOLUME_ID	(UBI_LAYOUT_VOLUME_ID + 2)
>  
>  /* fastmap on-flash data structure format version */
> -#define UBI_FM_FMT_VERSION	1
> +#define UBI_FM_FMT_VERSION		2

How about renaming that one UBI_FM_MAX_SUPPORTED_VERSION or something
like that.

> +
> +/* this implementation writes always version 1 */
> +#define UBI_FM_FMT_WRITE_VERSION	1

It really feels weird to hardcode that. What if you're reading a
fastmap volume that has version 2 set, and need to update it? Does that
mean you'll downgrade the FM to version 1 or will you prevent updating
the it?

>  
>  #define UBI_FM_SB_MAGIC		0x7B11D69F
>  #define UBI_FM_HDR_MAGIC	0xD4B82EF7
> @@ -387,6 +390,8 @@ struct ubi_vtbl_record {
>  #define UBI_FM_MIN_POOL_SIZE	8
>  #define UBI_FM_MAX_POOL_SIZE	256
>  
> +#define UBI_FM_SB_FLG_MASK	0

How about naming that one UBI_FM_SB_SUPPORTED_FLG_MASK since you seem
to add new flags afterwards without changing the version number.

> +
>  /**
>   * struct ubi_fm_sb - UBI fastmap super block
>   * @magic: fastmap super block magic number (%UBI_FM_SB_MAGIC)
> @@ -396,6 +401,7 @@ struct ubi_vtbl_record {
>   * @block_loc: an array containing the location of all PEBs of the fastmap
>   * @block_ec: the erase counter of each used PEB
>   * @sqnum: highest sequence number value at the time while taking the fastmap
> + * @flags: fastmap specific flags, only used with @version > 1, zero otherwise
>   *
>   */
>  struct ubi_fm_sb {
> @@ -407,7 +413,8 @@ struct ubi_fm_sb {
>  	__be32 block_loc[UBI_FM_MAX_BLOCKS];
>  	__be32 block_ec[UBI_FM_MAX_BLOCKS];
>  	__be64 sqnum;
> -	__u8 padding2[32];
> +	__be32 flags;
> +	__u8 padding2[28];
>  } __packed;
>  
>  /**
> diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
> index f5ba97c46160..4fab3790733b 100644
> --- a/drivers/mtd/ubi/ubi.h
> +++ b/drivers/mtd/ubi/ubi.h
> @@ -249,6 +249,7 @@ struct ubi_volume_desc;
>   * @used_blocks: number of used PEBs
>   * @max_pool_size: maximal size of the user pool
>   * @max_wl_pool_size: maximal size of the pool used by the WL sub-system
> + * @flags: fastmap flags
>   */
>  struct ubi_fastmap_layout {
>  	struct ubi_wl_entry *e[UBI_FM_MAX_BLOCKS];
> @@ -256,6 +257,7 @@ struct ubi_fastmap_layout {
>  	int used_blocks;
>  	int max_pool_size;
>  	int max_wl_pool_size;
> +	int flags;
>  };
>  
>  /**

Re: [PATCH 04/14] ubi: fastmap: Add UBI_FM_SB_PRESEEDED_FLG flag

[Boris Brezillon]

On Wed, 13 Jun 2018 23:23:34 +0200
Richard Weinberger <richard@nod.at> wrote:

> This flag indicates that the fastmap was preseeded, which means
> it was created offline by a tool such as ubinize which cannot know
> the whole MTD state such as real size and bad blocks.
> As consequence UBI has to take special care to use that fastmap.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/mtd/ubi/ubi-media.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/ubi/ubi-media.h b/drivers/mtd/ubi/ubi-media.h
> index 6136a97f4844..be339fb924af 100644
> --- a/drivers/mtd/ubi/ubi-media.h
> +++ b/drivers/mtd/ubi/ubi-media.h
> @@ -390,7 +390,11 @@ struct ubi_vtbl_record {
>  #define UBI_FM_MIN_POOL_SIZE	8
>  #define UBI_FM_MAX_POOL_SIZE	256
>  
> -#define UBI_FM_SB_FLG_MASK	0
> +enum {
> +	UBI_FM_SB_PRESEEDED_FLG	= 0x1,
> +};
> +
> +#define UBI_FM_SB_FLG_MASK (UBI_FM_SB_PRESEEDED_FLG)

You update the def that you use to check when a flag is supported,
still, you do not support this feature yet. Better move this line after
the support is really functional, don't you think?

Re: [PATCH 09/14] ubi: fastmap: Relax size check

[Boris Brezillon]

On Wed, 13 Jun 2018 23:23:39 +0200
Richard Weinberger <richard@nod.at> wrote:

> While attaching via fastmap we verify whether the found fastmap
> is as large as we have computed.
> With preseeded Fastmaps this assumtion can fail since ubinize cannot
> know the total size of the MTD and uses the number if used PEBs for
> the calculation.
> Therefore the found fastmap might be smaller than the kernel expects.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>

Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>

> ---
>  drivers/mtd/ubi/fastmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index 09a9d3a0ccf5..dabeb01af24a 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -1168,7 +1168,7 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
>  	}
>  
>  	fm_size = ubi->leb_size * used_blocks;
> -	if (fm_size != ubi->fm_size) {
> +	if (fm_size > ubi->fm_size) {
>  		ubi_err(ubi, "bad fastmap size: %zi, expected: %zi",
>  			fm_size, ubi->fm_size);
>  		ret = UBI_BAD_FASTMAP;

Re: [PATCH 10/14] ubi: fastmap: Change a WARN_ON() to ubi_err()

[Boris Brezillon]

On Wed, 13 Jun 2018 23:23:40 +0200
Richard Weinberger <richard@nod.at> wrote:

> This WARN_ON() was used while development of fastmap, now
> it can be a regular ubi_err().
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/mtd/ubi/fastmap.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index dabeb01af24a..d5506152c9f7 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -949,14 +949,16 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
>  	ubi_assert(list_empty(&free));
>  
>  	/*
> -	 * If fastmap is leaking PEBs (must not happen), raise a
> -	 * fat warning and fall back to scanning mode.
> -	 * We do this here because in ubi_wl_init() it's too late
> -	 * and we cannot fall back to scanning.
> +	 * The orginal purpose of this check was detecting fastmap bugs were
> +	 * it missed blocks. Now it helps to detect mis-flashed UBIs.
> +	 * E.g. when a fastmap enabled UBI is copied to another target with
> +	 * different bad blocks.
>  	 */
> -	if (WARN_ON(count_fastmap_pebs(ai) != ubi->peb_count -
> -		    ai->bad_peb_count - fm->used_blocks))
> +	if (count_fastmap_pebs(ai) != ubi->peb_count - ai->bad_peb_count -
> +		fm->used_blocks) {

Nitpick, but can you align things like that:

	if (count_fastmap_pebs(ai) != ubi->peb_count - ai->bad_peb_count -
				      fm->used_blocks) {

or

	if (count_fastmap_pebs(ai) !=
	    ubi->peb_count - ai->bad_peb_count - fm->used_blocks) {

or even better, have an avail_peb_count variable:

	avail_peb_count = ubi->peb_count - ai->bad_peb_count - fm->used_blocks;
	if (count_fastmap_pebs(ai) != avail_peb_count)

> +		ubi_err(ubi, "number of PEBs referenced by fastmap does not match MTD!");
>  		goto fail_bad;

Also, I'm not sure I get it. Why is that an error now that we have
preseeded fastmap? Isn't it expected to have a potential mismatch the
first time we boot, or is it already taken into account before we reach
this point?



> +	}
>  
>  	return 0;
>  

Re: [PATCH 11/14] ubi: Add module parameter to force a full scan

[Boris Brezillon]

On Wed, 13 Jun 2018 23:23:41 +0200
Richard Weinberger <richard@nod.at> wrote:

> Using this parameter one can force UBI do to a full scan
> instead of using a fastmap.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/mtd/ubi/attach.c | 13 +++++++++----
>  drivers/mtd/ubi/build.c  |  5 ++++-
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
> index 9a8072cf458c..134b15f093c3 100644
> --- a/drivers/mtd/ubi/attach.c
> +++ b/drivers/mtd/ubi/attach.c
> @@ -925,7 +925,7 @@ static int check_corruption(struct ubi_device *ubi, struct ubi_vid_hdr *vid_hdr,
>  	return err;
>  }
>  
> -static bool vol_ignored(int vol_id)
> +static bool vol_ignored(struct ubi_attach_info *ai, int vol_id)
>  {
>  	switch (vol_id) {
>  		case UBI_LAYOUT_VOLUME_ID:
> @@ -933,6 +933,9 @@ static bool vol_ignored(int vol_id)
>  	}
>  
>  #ifdef CONFIG_MTD_UBI_FASTMAP
> +	if (ai->force_full_scan)
> +		return false;
> +
>  	return ubi_is_fm_vol(vol_id);
>  #else
>  	return false;
> @@ -1143,7 +1146,7 @@ static int scan_peb(struct ubi_device *ubi, struct ubi_attach_info *ai,
>  	}
>  
>  	vol_id = be32_to_cpu(vidh->vol_id);
> -	if (vol_id > UBI_MAX_VOLUMES && !vol_ignored(vol_id)) {
> +	if (vol_id > UBI_MAX_VOLUMES && !vol_ignored(ai, vol_id)) {
>  		int lnum = be32_to_cpu(vidh->lnum);
>  
>  		/* Unsupported internal volume */
> @@ -1581,9 +1584,11 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
>  		force_scan = 1;
>  	}
>  
> -	if (force_scan)
> +	if (force_scan) {
> +		ubi_msg(ubi, "full scan forced");
> +		ai->force_full_scan = 1;
>  		err = scan_all(ubi, ai, 0);
> -	else {
> +	} else {
>  		err = scan_fast(ubi, &ai);
>  		if (err > 0 || mtd_is_eccerr(err)) {
>  			if (err != UBI_NO_FASTMAP) {
> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> index d2a726654ff1..1e3f75ede985 100644
> --- a/drivers/mtd/ubi/build.c
> +++ b/drivers/mtd/ubi/build.c
> @@ -83,6 +83,7 @@ static struct mtd_dev_param mtd_dev_param[UBI_MAX_DEVICES];
>  static bool fm_autoconvert;
>  static bool fm_debug;
>  #endif
> +static bool force_scan;
>  
>  /* Slab cache for wear-leveling entries */
>  struct kmem_cache *ubi_wl_entry_slab;
> @@ -956,7 +957,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
>  	if (!ubi->fm_buf)
>  		goto out_free;
>  #endif
> -	err = ubi_attach(ubi, 0);
> +	err = ubi_attach(ubi, force_scan);
>  	if (err) {
>  		ubi_err(ubi, "failed to attach mtd%d, error %d",
>  			mtd->index, err);
> @@ -1458,6 +1459,8 @@ module_param(fm_autoconvert, bool, 0644);
>  MODULE_PARM_DESC(fm_autoconvert, "Set this parameter to enable fastmap automatically on images without a fastmap.");
>  module_param(fm_debug, bool, 0);
>  MODULE_PARM_DESC(fm_debug, "Set this parameter to enable fastmap debugging by default. Warning, this will make fastmap slow!");
> +module_param(force_scan, bool, 0644);
> +MODULE_PARM_DESC(force_scan, "Always do a full scan of the MTD and drop possible fastmap structures from the MTD.");

Should we do that on a per-UBI device basis? I mean, you might want to
force a full-scan but only on a specific UBI instance, not all of them,
right?

>  #endif
>  MODULE_VERSION(__stringify(UBI_VERSION));
>  MODULE_DESCRIPTION("UBI - Unsorted Block Images");

Re: [PATCH 12/14] ubi: uapi: Add mode selector to attach request

[Boris Brezillon]

On Wed, 13 Jun 2018 23:23:42 +0200
Richard Weinberger <richard@nod.at> wrote:

> This allows userspace, ubiattach, to force a full scan.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  include/uapi/mtd/ubi-user.h | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/mtd/ubi-user.h b/include/uapi/mtd/ubi-user.h
> index 5b04a494d139..cc0e54cfa7f9 100644
> --- a/include/uapi/mtd/ubi-user.h
> +++ b/include/uapi/mtd/ubi-user.h
> @@ -235,12 +235,19 @@ enum {
>  	UBI_VOL_PROP_DIRECT_WRITE = 1,
>  };
>  
> +enum {
> +	UBI_ATTACH_MODE_AUTO = 0x0,
> +	UBI_ATTACH_MODE_SCAN = 0x1,
> +	UBI_ATTACH_MODE_MAX = UBI_ATTACH_MODE_SCAN,
> +};
> +
>  /**
>   * struct ubi_attach_req - attach MTD device request.
>   * @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 number of bad PEB per 1024 PEBs
> + * @attach_mode: selects the attach mode
>   * @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
> @@ -276,13 +283,18 @@ enum {
>   * eraseblocks for new bad eraseblocks, but attempts to use available
>   * eraseblocks (if any). The accepted range is 0-768. If 0 is given, the
>   * default kernel value of %CONFIG_MTD_UBI_BEB_LIMIT will be used.
> + *
> + * @attach_mode is %UBI_ATTACH_MODE_AUTO by default, and let's the UBI
> + * implementation decide how to attach. If %UBI_ATTACH_MODE_SCAN is selected
> + * a full scan is forced.
>   */
>  struct ubi_attach_req {
>  	__s32 ubi_num;
>  	__s32 mtd_num;
>  	__s32 vid_hdr_offset;
>  	__s16 max_beb_per1024;
> -	__s8 padding[10];
> +	__s8 attach_mode;
> +	__s8 padding[9];
>  };

Looks like the force full-scan is just a flag, and we might need other
flags to encode things that are not related to attach mode, so how
about creating an __u16 flags field here and defining

#define UBI_ATTACH_FLG_FORCE_FULL_SCAN		0x1

?