[PATCH 0/4] UBI: improve empty flash handling

[PATCH 0/4] UBI: improve empty flash handling

[Artem Bityutskiy]

Hi Sebastian,

I've removed your 'UBI: recognize empty flash with errors as empty'
patch from the ubi-2.6.git tree, because it solves one problem, but
introduces another, and needs more work, as you yourself pointed.

This is a series of patches which is intended to fix your problem.
Basically, the idea is that during scanning, we just remember the
information about which eraseblocks we found. Then at the end, we
look at them and decide what to do.

We do the following:

1. If there are more than 8 corrupted eraseblocks, we print a warning.
   This functionality was there before. This is just a warning, nothing
   else.
2. If more than 5% or eraseblocks are corrupted, we refuse. This logic
   was not there, I've just added it.
3. If now UBI-formatted eraseblocks were found, then we start checking
   whether this is empty device and we should format it. Otherwise, we
   go forward and try to attach the device.

   3.1 If there are not corrupted eraseblocks -> empty, format
   3.2 If there are few corrupted eraseblocks, all of them had read error
       -> empty, format. This is the place where we handle the "empty
       flash with few bad eraseblocks not marked yet as such"
   3.3 If there is a corrupted eraseblock, and there was not read error,
       or if there are many corrupted eraseblocks (>= 8), then we refuse.

Please, review and test the following series.

P.S. I gave it only a very quick try

Artem.

[PATCH 1/4] UBI: simplify IO error codes - part 1

[Artem Bityutskiy]

We do not really need 2 separate error codes for indicating bad VID
and bad EC headers (UBI_IO_BAD_EC_HDR, UBI_IO_BAD_VID_HDR), it is
enough to have only one UBI_IO_BAD_HDR return code.

This patch does not introduce any functional change, only some
code simplification.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 drivers/mtd/ubi/eba.c  |    4 ++--
 drivers/mtd/ubi/io.c   |   14 +++++++-------
 drivers/mtd/ubi/scan.c |    4 ++--
 drivers/mtd/ubi/ubi.h  |    7 ++-----
 4 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
index 9f87c99..8e82267 100644
--- a/drivers/mtd/ubi/eba.c
+++ b/drivers/mtd/ubi/eba.c
@@ -418,7 +418,7 @@ retry:
 				 * may try to recover data. FIXME: but this is
 				 * not implemented.
 				 */
-				if (err == UBI_IO_BAD_VID_HDR) {
+				if (err == UBI_IO_BAD_HDR) {
 					ubi_warn("corrupted VID header at PEB "
 						 "%d, LEB %d:%d", pnum, vol_id,
 						 lnum);
@@ -961,7 +961,7 @@ write_error:
  */
 static int is_error_sane(int err)
 {
-	if (err == -EIO || err == -ENOMEM || err == UBI_IO_BAD_VID_HDR ||
+	if (err == -EIO || err == -ENOMEM || err == UBI_IO_BAD_HDR ||
 	    err == -ETIMEDOUT)
 		return 0;
 	return 1;
diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
index 533b1a4..6f5b087 100644
--- a/drivers/mtd/ubi/io.c
+++ b/drivers/mtd/ubi/io.c
@@ -515,7 +515,7 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum)
 	 * In this case we probably anyway have garbage in this PEB.
 	 */
 	err1 = ubi_io_read_vid_hdr(ubi, pnum, &vid_hdr, 0);
-	if (err1 == UBI_IO_BAD_VID_HDR)
+	if (err1 == UBI_IO_BAD_HDR)
 		/*
 		 * The VID header is corrupted, so we can safely erase this
 		 * PEB and not afraid that it will be treated as a valid PEB in
@@ -709,7 +709,7 @@ bad:
  * o %UBI_IO_BITFLIPS if the CRC is correct, but bit-flips were detected
  *   and corrected by the flash driver; this is harmless but may indicate that
  *   this eraseblock may become bad soon (but may be not);
- * o %UBI_IO_BAD_EC_HDR if the erase counter header is corrupted (a CRC error);
+ * o %UBI_IO_BAD_HDR if the erase counter header is corrupted (a CRC error);
  * o %UBI_IO_PEB_EMPTY if the physical eraseblock is empty;
  * o a negative error code in case of failure.
  */
@@ -774,7 +774,7 @@ int ubi_io_read_ec_hdr(struct ubi_device *ubi, int pnum,
 		} else if (UBI_IO_DEBUG)
 			dbg_msg("bad magic number at PEB %d: %08x instead of "
 				"%08x", pnum, magic, UBI_EC_HDR_MAGIC);
-		return UBI_IO_BAD_EC_HDR;
+		return UBI_IO_BAD_HDR;
 	}
 
 	crc = crc32(UBI_CRC32_INIT, ec_hdr, UBI_EC_HDR_SIZE_CRC);
@@ -788,7 +788,7 @@ int ubi_io_read_ec_hdr(struct ubi_device *ubi, int pnum,
 		} else if (UBI_IO_DEBUG)
 			dbg_msg("bad EC header CRC at PEB %d, calculated "
 				"%#08x, read %#08x", pnum, crc, hdr_crc);
-		return UBI_IO_BAD_EC_HDR;
+		return UBI_IO_BAD_HDR;
 	}
 
 	/* And of course validate what has just been read from the media */
@@ -977,7 +977,7 @@ bad:
  * o %UBI_IO_BITFLIPS if the CRC is correct, but bit-flips were detected
  *   and corrected by the flash driver; this is harmless but may indicate that
  *   this eraseblock may become bad soon;
- * o %UBI_IO_BAD_VID_HDR if the volume identifier header is corrupted (a CRC
+ * o %UBI_IO_BAD_HDR if the volume identifier header is corrupted (a CRC
  *   error detected);
  * o %UBI_IO_PEB_FREE if the physical eraseblock is free (i.e., there is no VID
  *   header there);
@@ -1045,7 +1045,7 @@ int ubi_io_read_vid_hdr(struct ubi_device *ubi, int pnum,
 		} else if (UBI_IO_DEBUG)
 			dbg_msg("bad magic number at PEB %d: %08x instead of "
 				"%08x", pnum, magic, UBI_VID_HDR_MAGIC);
-		return UBI_IO_BAD_VID_HDR;
+		return UBI_IO_BAD_HDR;
 	}
 
 	crc = crc32(UBI_CRC32_INIT, vid_hdr, UBI_VID_HDR_SIZE_CRC);
@@ -1059,7 +1059,7 @@ int ubi_io_read_vid_hdr(struct ubi_device *ubi, int pnum,
 		} else if (UBI_IO_DEBUG)
 			dbg_msg("bad CRC at PEB %d, calculated %#08x, "
 				"read %#08x", pnum, crc, hdr_crc);
-		return UBI_IO_BAD_VID_HDR;
+		return UBI_IO_BAD_HDR;
 	}
 
 	/* Validate the VID header that we have just read */
diff --git a/drivers/mtd/ubi/scan.c b/drivers/mtd/ubi/scan.c
index dc5f688..f52adca 100644
--- a/drivers/mtd/ubi/scan.c
+++ b/drivers/mtd/ubi/scan.c
@@ -745,7 +745,7 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
 		bitflips = 1;
 	else if (err == UBI_IO_PEB_EMPTY)
 		return add_to_list(si, pnum, UBI_SCAN_UNKNOWN_EC, &si->erase);
-	else if (err == UBI_IO_BAD_EC_HDR) {
+	else if (err == UBI_IO_BAD_HDR) {
 		/*
 		 * We have to also look at the VID header, possibly it is not
 		 * corrupted. Set %bitflips flag in order to make this PEB be
@@ -813,7 +813,7 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
 		return err;
 	else if (err == UBI_IO_BITFLIPS)
 		bitflips = 1;
-	else if (err == UBI_IO_BAD_VID_HDR ||
+	else if (err == UBI_IO_BAD_HDR ||
 		 (err == UBI_IO_PEB_FREE && ec_corr)) {
 		/* VID header is corrupted */
 		err = add_to_list(si, pnum, ec, &si->corr);
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index a637f02..539b3f6 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -89,16 +89,13 @@
  *                   %0xFF bytes
  * UBI_IO_PEB_FREE: the physical eraseblock is free, i.e. it contains only a
  *                  valid erase counter header, and the rest are %0xFF bytes
- * UBI_IO_BAD_EC_HDR: the erase counter header is corrupted (bad magic or CRC)
- * UBI_IO_BAD_VID_HDR: the volume identifier header is corrupted (bad magic or
- *                     CRC)
+ * UBI_IO_BAD_HDR: the EC or VID header is corrupted (bad magic or CRC)
  * UBI_IO_BITFLIPS: bit-flips were detected and corrected
  */
 enum {
 	UBI_IO_PEB_EMPTY = 1,
 	UBI_IO_PEB_FREE,
-	UBI_IO_BAD_EC_HDR,
-	UBI_IO_BAD_VID_HDR,
+	UBI_IO_BAD_HDR,
 	UBI_IO_BITFLIPS
 };
 
-- 
1.6.6.1

[PATCH 2/4] UBI: introduce a new IO return code

[Artem Bityutskiy]

This patch introduces the %UBI_IO_BAD_HDR_READ return code for
the I/O level function. We will use this code in order to distinguish
between "corrupted header possibly because this is non-ubi data" and
"corrupted header possibly because of real data corruption and ECC error".

So far this patch does not introduce any functional change, just a
preparation.

This patch is pased on a patch from
Sebastian Andrzej Siewior <sebastian@breakpoint.cc>

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 drivers/mtd/ubi/eba.c  |    5 +++--
 drivers/mtd/ubi/io.c   |   42 +++++++++++++++++++++++-------------------
 drivers/mtd/ubi/scan.c |    4 ++--
 drivers/mtd/ubi/ubi.h  |    3 +++
 4 files changed, 31 insertions(+), 23 deletions(-)

diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
index 8e82267..b582671 100644
--- a/drivers/mtd/ubi/eba.c
+++ b/drivers/mtd/ubi/eba.c
@@ -418,7 +418,8 @@ retry:
 				 * may try to recover data. FIXME: but this is
 				 * not implemented.
 				 */
-				if (err == UBI_IO_BAD_HDR) {
+				if (err == UBI_IO_BAD_HDR_READ ||
+				    err == UBI_IO_BAD_HDR) {
 					ubi_warn("corrupted VID header at PEB "
 						 "%d, LEB %d:%d", pnum, vol_id,
 						 lnum);
@@ -962,7 +963,7 @@ write_error:
 static int is_error_sane(int err)
 {
 	if (err == -EIO || err == -ENOMEM || err == UBI_IO_BAD_HDR ||
-	    err == -ETIMEDOUT)
+	    err == UBI_IO_BAD_HDR_READ || err == -ETIMEDOUT)
 		return 0;
 	return 1;
 }
diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
index 6f5b087..abae9af 100644
--- a/drivers/mtd/ubi/io.c
+++ b/drivers/mtd/ubi/io.c
@@ -515,7 +515,7 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum)
 	 * In this case we probably anyway have garbage in this PEB.
 	 */
 	err1 = ubi_io_read_vid_hdr(ubi, pnum, &vid_hdr, 0);
-	if (err1 == UBI_IO_BAD_HDR)
+	if (err1 == UBI_IO_BAD_HDR_READ || err1 == UBI_IO_BAD_HDR)
 		/*
 		 * The VID header is corrupted, so we can safely erase this
 		 * PEB and not afraid that it will be treated as a valid PEB in
@@ -736,23 +736,21 @@ int ubi_io_read_ec_hdr(struct ubi_device *ubi, int pnum,
 		 * header is still OK, we just report this as there was a
 		 * bit-flip.
 		 */
-		read_err = err;
+		if (err == -EBADMSG)
+			read_err = UBI_IO_BAD_HDR_READ;
 	}
 
 	magic = be32_to_cpu(ec_hdr->magic);
 	if (magic != UBI_EC_HDR_MAGIC) {
+		if (read_err)
+			return read_err;
+
 		/*
 		 * The magic field is wrong. Let's check if we have read all
 		 * 0xFF. If yes, this physical eraseblock is assumed to be
 		 * empty.
-		 *
-		 * But if there was a read error, we do not test it for all
-		 * 0xFFs. Even if it does contain all 0xFFs, this error
-		 * indicates that something is still wrong with this physical
-		 * eraseblock and we anyway cannot treat it as empty.
 		 */
-		if (read_err != -EBADMSG &&
-		    check_pattern(ec_hdr, 0xFF, UBI_EC_HDR_SIZE)) {
+		if (check_pattern(ec_hdr, 0xFF, UBI_EC_HDR_SIZE)) {
 			/* The physical eraseblock is supposedly empty */
 			if (verbose)
 				ubi_warn("no EC header found at PEB %d, "
@@ -788,7 +786,7 @@ int ubi_io_read_ec_hdr(struct ubi_device *ubi, int pnum,
 		} else if (UBI_IO_DEBUG)
 			dbg_msg("bad EC header CRC at PEB %d, calculated "
 				"%#08x, read %#08x", pnum, crc, hdr_crc);
-		return UBI_IO_BAD_HDR;
+		return read_err ?: UBI_IO_BAD_HDR;
 	}
 
 	/* And of course validate what has just been read from the media */
@@ -798,6 +796,10 @@ int ubi_io_read_ec_hdr(struct ubi_device *ubi, int pnum,
 		return -EINVAL;
 	}
 
+	/*
+	 * If there was %-EBADMSG, but the header CRC is still OK, report about
+	 * a bit-flip to force scrubbing on this PEB.
+	 */
 	return read_err ? UBI_IO_BITFLIPS : 0;
 }
 
@@ -1008,22 +1010,20 @@ int ubi_io_read_vid_hdr(struct ubi_device *ubi, int pnum,
 		 * CRC check-sum and we will identify this. If the VID header is
 		 * still OK, we just report this as there was a bit-flip.
 		 */
-		read_err = err;
+		if (err == -EBADMSG)
+			read_err = UBI_IO_BAD_HDR_READ;
 	}
 
 	magic = be32_to_cpu(vid_hdr->magic);
 	if (magic != UBI_VID_HDR_MAGIC) {
+		if (read_err)
+			return read_err;
+
 		/*
 		 * If we have read all 0xFF bytes, the VID header probably does
 		 * not exist and the physical eraseblock is assumed to be free.
-		 *
-		 * But if there was a read error, we do not test the data for
-		 * 0xFFs. Even if it does contain all 0xFFs, this error
-		 * indicates that something is still wrong with this physical
-		 * eraseblock and it cannot be regarded as free.
 		 */
-		if (read_err != -EBADMSG &&
-		    check_pattern(vid_hdr, 0xFF, UBI_VID_HDR_SIZE)) {
+		if (check_pattern(vid_hdr, 0xFF, UBI_VID_HDR_SIZE)) {
 			/* The physical eraseblock is supposedly free */
 			if (verbose)
 				ubi_warn("no VID header found at PEB %d, "
@@ -1059,7 +1059,7 @@ int ubi_io_read_vid_hdr(struct ubi_device *ubi, int pnum,
 		} else if (UBI_IO_DEBUG)
 			dbg_msg("bad CRC at PEB %d, calculated %#08x, "
 				"read %#08x", pnum, crc, hdr_crc);
-		return UBI_IO_BAD_HDR;
+		return read_err ?: UBI_IO_BAD_HDR;
 	}
 
 	/* Validate the VID header that we have just read */
@@ -1069,6 +1069,10 @@ int ubi_io_read_vid_hdr(struct ubi_device *ubi, int pnum,
 		return -EINVAL;
 	}
 
+	/*
+	 * If there was a read error (%-EBADMSG), but the header CRC is still
+	 * OK, report about a bit-flip to force scrubbing on this PEB.
+	 */
 	return read_err ? UBI_IO_BITFLIPS : 0;
 }
 
diff --git a/drivers/mtd/ubi/scan.c b/drivers/mtd/ubi/scan.c
index f52adca..65d03b5 100644
--- a/drivers/mtd/ubi/scan.c
+++ b/drivers/mtd/ubi/scan.c
@@ -745,7 +745,7 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
 		bitflips = 1;
 	else if (err == UBI_IO_PEB_EMPTY)
 		return add_to_list(si, pnum, UBI_SCAN_UNKNOWN_EC, &si->erase);
-	else if (err == UBI_IO_BAD_HDR) {
+	else if (err = UBI_IO_BAD_HDR_READ || err == UBI_IO_BAD_HDR) {
 		/*
 		 * We have to also look at the VID header, possibly it is not
 		 * corrupted. Set %bitflips flag in order to make this PEB be
@@ -813,7 +813,7 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
 		return err;
 	else if (err == UBI_IO_BITFLIPS)
 		bitflips = 1;
-	else if (err == UBI_IO_BAD_HDR ||
+	else if (err == UBI_IO_BAD_HDR_READ || err == UBI_IO_BAD_HDR ||
 		 (err == UBI_IO_PEB_FREE && ec_corr)) {
 		/* VID header is corrupted */
 		err = add_to_list(si, pnum, ec, &si->corr);
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 539b3f6..0359e0c 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -90,12 +90,15 @@
  * UBI_IO_PEB_FREE: the physical eraseblock is free, i.e. it contains only a
  *                  valid erase counter header, and the rest are %0xFF bytes
  * UBI_IO_BAD_HDR: the EC or VID header is corrupted (bad magic or CRC)
+ * UBI_IO_BAD_HDR_READ: the same as %UBI_IO_BAD_HDR, but also there was a read
+ * 			error reported by the flash driver
  * UBI_IO_BITFLIPS: bit-flips were detected and corrected
  */
 enum {
 	UBI_IO_PEB_EMPTY = 1,
 	UBI_IO_PEB_FREE,
 	UBI_IO_BAD_HDR,
+	UBI_IO_BAD_HDR_READ,
 	UBI_IO_BITFLIPS
 };
 
-- 
1.6.6.1

Re: [PATCH 2/4] UBI: introduce a new IO return code

[Sebastian Andrzej Siewior]

* Artem Bityutskiy | 2010-05-03 13:13:00 [+0300]:

>diff --git a/drivers/mtd/ubi/scan.c b/drivers/mtd/ubi/scan.c
>index f52adca..65d03b5 100644
>--- a/drivers/mtd/ubi/scan.c
>+++ b/drivers/mtd/ubi/scan.c
>@@ -745,7 +745,7 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
> 		bitflips = 1;
> 	else if (err == UBI_IO_PEB_EMPTY)
> 		return add_to_list(si, pnum, UBI_SCAN_UNKNOWN_EC, &si->erase);
>-	else if (err == UBI_IO_BAD_HDR) {
>+	else if (err = UBI_IO_BAD_HDR_READ || err == UBI_IO_BAD_HDR) {

That part should be == and not = but you fix this in the next patch so I
guess it is okay.

> 		/*
> 		 * We have to also look at the VID header, possibly it is not
> 		 * corrupted. Set %bitflips flag in order to make this PEB be

Sebastian

Re: [PATCH 2/4] UBI: introduce a new IO return code

[Artem Bityutskiy]

On Thu, 2010-05-06 at 11:34 +0200, Sebastian Andrzej Siewior wrote:
> * Artem Bityutskiy | 2010-05-03 13:13:00 [+0300]:
> 
> >diff --git a/drivers/mtd/ubi/scan.c b/drivers/mtd/ubi/scan.c
> >index f52adca..65d03b5 100644
> >--- a/drivers/mtd/ubi/scan.c
> >+++ b/drivers/mtd/ubi/scan.c
> >@@ -745,7 +745,7 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
> > 		bitflips = 1;
> > 	else if (err == UBI_IO_PEB_EMPTY)
> > 		return add_to_list(si, pnum, UBI_SCAN_UNKNOWN_EC, &si->erase);
> >-	else if (err == UBI_IO_BAD_HDR) {
> >+	else if (err = UBI_IO_BAD_HDR_READ || err == UBI_IO_BAD_HDR) {
> 
> That part should be == and not = but you fix this in the next patch so I
> guess it is okay.

Fixed this in the wrong patch, I'll amend them and make sure this typo
goes away, thanks.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

[PATCH 3/4] UBI: introduce eraseblock counter variables

[Artem Bityutskiy]

This is just a preparation patch which introduces several
'struct ubi_scan_info' fields which count eraseblocks of different
types. This will be used later on to decide whether it is safe to
format the flash or not. No functional changes so far.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 drivers/mtd/ubi/scan.c |   29 ++++++++++++++++++-----------
 drivers/mtd/ubi/scan.h |   19 ++++++++++++++-----
 2 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/ubi/scan.c b/drivers/mtd/ubi/scan.c
index 65d03b5..dc5e061 100644
--- a/drivers/mtd/ubi/scan.c
+++ b/drivers/mtd/ubi/scan.c
@@ -72,16 +72,19 @@ static int add_to_list(struct ubi_scan_info *si, int pnum, int ec,
 {
 	struct ubi_scan_leb *seb;
 
-	if (list == &si->free)
+	if (list == &si->free) {
 		dbg_bld("add to free: PEB %d, EC %d", pnum, ec);
-	else if (list == &si->erase)
+		si->free_peb_count += 1;
+	} else if (list == &si->erase) {
 		dbg_bld("add to erase: PEB %d, EC %d", pnum, ec);
-	else if (list == &si->corr) {
+		si->erase_peb_count += 1;
+	} else if (list == &si->corr) {
 		dbg_bld("add to corrupted: PEB %d, EC %d", pnum, ec);
-		si->corr_count += 1;
-	} else if (list == &si->alien)
+		si->corr_peb_count += 1;
+	} else if (list == &si->alien) {
 		dbg_bld("add to alien: PEB %d, EC %d", pnum, ec);
-	else
+		si->alien_peb_count += 1;
+	} else
 		BUG();
 
 	seb = kmalloc(sizeof(struct ubi_scan_leb), GFP_KERNEL);
@@ -517,6 +520,7 @@ int ubi_scan_add_used(struct ubi_device *ubi, struct ubi_scan_info *si,
 	sv->leb_count += 1;
 	rb_link_node(&seb->u.rb, parent, p);
 	rb_insert_color(&seb->u.rb, &sv->root);
+	si->used_peb_count += 1;
 	return 0;
 }
 
@@ -745,13 +749,13 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
 		bitflips = 1;
 	else if (err == UBI_IO_PEB_EMPTY)
 		return add_to_list(si, pnum, UBI_SCAN_UNKNOWN_EC, &si->erase);
-	else if (err = UBI_IO_BAD_HDR_READ || err == UBI_IO_BAD_HDR) {
+	else if (err == UBI_IO_BAD_HDR_READ || err == UBI_IO_BAD_HDR) {
 		/*
 		 * We have to also look at the VID header, possibly it is not
 		 * corrupted. Set %bitflips flag in order to make this PEB be
 		 * moved and EC be re-created.
 		 */
-		ec_corr = 1;
+		ec_corr = err;
 		ec = UBI_SCAN_UNKNOWN_EC;
 		bitflips = 1;
 	}
@@ -816,6 +820,9 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
 	else if (err == UBI_IO_BAD_HDR_READ || err == UBI_IO_BAD_HDR ||
 		 (err == UBI_IO_PEB_FREE && ec_corr)) {
 		/* VID header is corrupted */
+		if (err == UBI_IO_BAD_HDR_READ ||
+		    ec_corr == UBI_IO_BAD_HDR_READ)
+			si->read_err_count += 1;
 		err = add_to_list(si, pnum, ec, &si->corr);
 		if (err)
 			return err;
@@ -855,7 +862,6 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
 			err = add_to_list(si, pnum, ec, &si->alien);
 			if (err)
 				return err;
-			si->alien_peb_count += 1;
 			return 0;
 
 		case UBI_COMPAT_REJECT:
@@ -943,8 +949,9 @@ struct ubi_scan_info *ubi_scan(struct ubi_device *ubi)
 	 * unclean reboots. However, many of them may indicate some problems
 	 * with the flash HW or driver. Print a warning in this case.
 	 */
-	if (si->corr_count >= 8 || si->corr_count >= ubi->peb_count / 4) {
-		ubi_warn("%d PEBs are corrupted", si->corr_count);
+	if (si->corr_peb_count >= 8 ||
+	    si->corr_peb_count >= ubi->peb_count / 4) {
+		ubi_warn("%d PEBs are corrupted", si->corr_peb_count);
 		printk(KERN_WARNING "corrupted PEBs are:");
 		list_for_each_entry(seb, &si->corr, u.list)
 			printk(KERN_CONT " %d", seb->pnum);
diff --git a/drivers/mtd/ubi/scan.h b/drivers/mtd/ubi/scan.h
index ff179ad..2576a8d 100644
--- a/drivers/mtd/ubi/scan.h
+++ b/drivers/mtd/ubi/scan.h
@@ -91,10 +91,16 @@ struct ubi_scan_volume {
  * @erase: list of physical eraseblocks which have to be erased
  * @alien: list of physical eraseblocks which should not be used by UBI (e.g.,
  *         those belonging to "preserve"-compatible internal volumes)
+ * @used_peb_count: count of used PEBs
+ * @corr_peb_count: count of PEBs in the @corr list
+ * @read_err_count: count of PEBs read with error (%UBI_IO_BAD_HDR_READ was
+ *                  returned)
+ * @free_peb_count: count of PEBs in the @free list
+ * @erase_peb_count: count of PEBs in the @erase list
+ * @alien_peb_count: count of PEBs in the @alien list
  * @bad_peb_count: count of bad physical eraseblocks
  * @vols_found: number of volumes found during scanning
  * @highest_vol_id: highest volume ID
- * @alien_peb_count: count of physical eraseblocks in the @alien list
  * @is_empty: flag indicating whether the MTD device is empty or not
  * @min_ec: lowest erase counter value
  * @max_ec: highest erase counter value
@@ -102,7 +108,6 @@ struct ubi_scan_volume {
  * @mean_ec: mean erase counter value
  * @ec_sum: a temporary variable used when calculating @mean_ec
  * @ec_count: a temporary variable used when calculating @mean_ec
- * @corr_count: count of corrupted PEBs
  *
  * This data structure contains the result of scanning and may be used by other
  * UBI sub-systems to build final UBI data structures, further error-recovery
@@ -114,10 +119,15 @@ struct ubi_scan_info {
 	struct list_head free;
 	struct list_head erase;
 	struct list_head alien;
+	int used_peb_count;
+	int corr_peb_count;
+	int read_err_count;
+	int free_peb_count;
+	int erase_peb_count;
+	int alien_peb_count;
 	int bad_peb_count;
 	int vols_found;
 	int highest_vol_id;
-	int alien_peb_count;
 	int is_empty;
 	int min_ec;
 	int max_ec;
@@ -125,7 +135,6 @@ struct ubi_scan_info {
 	int mean_ec;
 	uint64_t ec_sum;
 	int ec_count;
-	int corr_count;
 };
 
 struct ubi_device;
@@ -135,7 +144,7 @@ struct ubi_vid_hdr;
  * ubi_scan_move_to_list - move a PEB from the volume tree to a list.
  *
  * @sv: volume scanning information
- * @seb: scanning eraseblock infprmation
+ * @seb: scanning eraseblock information
  * @list: the list to move to
  */
 static inline void ubi_scan_move_to_list(struct ubi_scan_volume *sv,
-- 
1.6.6.1

[PATCH 4/4] UBI: improve corrupted flash handling

[Artem Bityutskiy]

This patch improves the way UBI handles corrupted flash, or flash
containing garbage or non-UBI data, which is the same from UBI POW.
Namely, we do the following:

* if 5% or more PEBs are corrupted, refuse the flash
* if less than 5% PEBs are corrupted, do not refuse the flash
  and format these PEBs
* if less than 8 PEBs are corrupted, format them silently, otherwise
  print a warning message.

Reported-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 drivers/mtd/ubi/scan.c |  103 +++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 84 insertions(+), 19 deletions(-)

diff --git a/drivers/mtd/ubi/scan.c b/drivers/mtd/ubi/scan.c
index dc5e061..c326f5b 100644
--- a/drivers/mtd/ubi/scan.c
+++ b/drivers/mtd/ubi/scan.c
@@ -760,8 +760,6 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
 		bitflips = 1;
 	}
 
-	si->is_empty = 0;
-
 	if (!ec_corr) {
 		int image_seq;
 
@@ -892,6 +890,87 @@ adjust_mean_ec:
 }
 
 /**
+ * check_what_we_have - check what PEB were found by scanning.
+ * @ubi: UBI device description object
+ * @si: scanning information
+ *
+ * This is a helper function which takes a look what PEBs were found by
+ * scanning, and decides whether the flash is empty and should be formatted and
+ * whether there are too many corrupted PEBs and we should not attach this
+ * MTD device. Returns zero if we should proceed with attaching the MTD device,
+ * and %-EINVAL if we should not.
+ */
+static int check_what_we_have(const struct ubi_device *ubi,
+			      struct ubi_scan_info *si)
+{
+	struct ubi_scan_leb *seb;
+	int max_corr;
+
+	max_corr = ubi->peb_count - si->bad_peb_count - si->alien_peb_count;
+	max_corr = max_corr / 20 ?: 8;
+
+	/*
+	 * Few corrupted PEBs are not a problem and may be just a result of
+	 * unclean reboots. However, many of them may indicate some problems
+	 * with the flash HW or driver.
+	 */
+	if (si->corr_peb_count >= 8) {
+		int cnt;
+
+		ubi_warn("%d PEBs are corrupted", si->corr_peb_count);
+		printk(KERN_WARNING "corrupted PEBs are:");
+		list_for_each_entry(seb, &si->corr, u.list)
+			printk(KERN_CONT " %d", seb->pnum);
+		printk(KERN_CONT "\n");
+
+		/*
+		 * If too many PEBs are corrupted, we refuse attaching,
+		 * otherwise, only print a warning.
+		 */
+		if (si->corr_peb_count >= max_corr) {
+			ubi_err("too many corrupted PEBs, refusing this device");
+			return -EINVAL;
+		}
+	}
+
+	if (si->free_peb_count + si->used_peb_count +
+	    si->alien_peb_count == 0) {
+		/* No UBI-formatted eraseblocks were found */
+		if (si->corr_peb_count == si->read_err_count &&
+		    si->corr_peb_count < 8) {
+			/* No or just few corrupted PEBs, and all of them had a
+			 * read error. We assume that those are bad PEBs, which
+			 * were just not marked as bad so far.
+			 *
+			 * This piece of code basically tries to distinguish
+			 * between the following 2 situations:
+			 *
+			 * 1. Flash is empty, but there are few bad PEBs, which
+			 *    are not marked as bad so far, and which were read
+			 *    with error. We want to go ahead and format this
+			 *    flash. While formating, the faulty PEBs will
+			 *    probably be marked as bad.
+			 *
+			 * 2. Flash contains probably contain non-UBI data
+			 *    and we do not want to format it and destroy
+			 *    possibly needed data (e.g., the bootloader MTD
+			 *    partition was accidentally fed to UBI).
+			 */
+			si->is_empty = 1;
+			ubi_msg("empty MTD device detected");
+		} else {
+			ubi_err("MTD device possibly contains non-UBI data, "
+				"refusing it");
+			return -EINVAL;
+		}
+	}
+
+	if (si->corr_peb_count >= 0)
+		ubi_msg("corrupted PEBs will be formatted");
+	return 0;
+}
+
+/**
  * ubi_scan - scan an MTD device.
  * @ubi: UBI device description object
  *
@@ -915,7 +994,6 @@ struct ubi_scan_info *ubi_scan(struct ubi_device *ubi)
 	INIT_LIST_HEAD(&si->erase);
 	INIT_LIST_HEAD(&si->alien);
 	si->volumes = RB_ROOT;
-	si->is_empty = 1;
 
 	err = -ENOMEM;
 	ech = kzalloc(ubi->ec_hdr_alsize, GFP_KERNEL);
@@ -941,22 +1019,9 @@ struct ubi_scan_info *ubi_scan(struct ubi_device *ubi)
 	if (si->ec_count)
 		si->mean_ec = div_u64(si->ec_sum, si->ec_count);
 
-	if (si->is_empty)
-		ubi_msg("empty MTD device detected");
-
-	/*
-	 * Few corrupted PEBs are not a problem and may be just a result of
-	 * unclean reboots. However, many of them may indicate some problems
-	 * with the flash HW or driver. Print a warning in this case.
-	 */
-	if (si->corr_peb_count >= 8 ||
-	    si->corr_peb_count >= ubi->peb_count / 4) {
-		ubi_warn("%d PEBs are corrupted", si->corr_peb_count);
-		printk(KERN_WARNING "corrupted PEBs are:");
-		list_for_each_entry(seb, &si->corr, u.list)
-			printk(KERN_CONT " %d", seb->pnum);
-		printk(KERN_CONT "\n");
-	}
+	err = check_what_we_have(ubi, si);
+	if (err)
+		goto out_vidh;
 
 	/*
 	 * In case of unknown erase counter we use the mean erase counter
-- 
1.6.6.1

Re: [PATCH 0/4] UBI: improve empty flash handling

[Sebastian Andrzej Siewior]

* Artem Bityutskiy | 2010-05-03 13:12:58 [+0300]:

>Hi Sebastian,
Hi Artem,

>Please, review and test the following series.

The code looks fine. I wiped the flash and the test resulted in:

| UBI: attaching mtd9 to ubi0
| UBI: physical eraseblock size:   131072 bytes (128 KiB)
| UBI: logical eraseblock size:    129024 bytes
| UBI: smallest flash I/O unit:    2048
| UBI: sub-page size:              512
| UBI: VID header offset:          512 (aligned 512)
| UBI: data offset:                2048
| UBI error: ubi_io_read: error -74 while reading 64 bytes from PEB 3399:0, read 64 bytes
| Call Trace:
| [cfbd7c70] [c0008558] show_stack+0x48/0x19c (unreliable)
| [cfbd7cb0] [c01ab6e0] ubi_io_read+0x1a4/0x2ac
| [cfbd7cf0] [c01abb4c] ubi_io_read_ec_hdr+0x48/0x21c
| [cfbd7d10] [c01b0060] ubi_scan+0x194/0x11b8
| [cfbd7d80] [c01a5e48] ubi_attach_mtd_dev+0x67c/0xe30
| [cfbd7e80] [c01a6804] ctrl_cdev_ioctl+0x140/0x1d8
| [cfbd7ec0] [c008bf1c] do_ioctl+0x3c/0xc4
| [cfbd7ee0] [c008c024] vfs_ioctl+0x80/0x448
| [cfbd7f10] [c008c42c] sys_ioctl+0x40/0x88
| [cfbd7f40] [c001008c] ret_from_syscall+0x0/0x38
| --- Exception: c01 at 0xff59908
|     LR = 0xff5986c
| UBI: empty MTD device detected
| UBI: corrupted PEBs will be formatted
| UBI: create volume table (copy #1)
| UBI: create volume table (copy #2)
| UBI: attached mtd9 to ubi0
| UBI: MTD device name:            "ubi"
| UBI: MTD device size:            512 MiB
| UBI: number of good PEBs:        4096
| UBI: number of bad PEBs:         0
| UBI: max. allowed volumes:       128
| UBI: wear-leveling threshold:    4096
| UBI: number of internal volumes: 1
| UBI: number of user volumes:     0
| UBI: available PEBs:             4052
| UBI: total number of reserved PEBs: 44
| UBI: number of PEBs reserved for bad PEB handling: 40
| UBI: max/mean erase counter: 0/0
| UBI: image sequence number: 0
| UBI: background thread "ubi_bgt0d" started, PID 2003
| UBI error: ubi_io_read: error -74 while reading 64 bytes from PEB 3399:0, read 64 bytes
| Call Trace:
| [cfa7bea0] [c0008558] show_stack+0x48/0x19c (unreliable)
| [cfa7bee0] [c01ab6e0] ubi_io_read+0x1a4/0x2ac
| [cfa7bf20] [c01abb4c] ubi_io_read_ec_hdr+0x48/0x21c
| [cfa7bf40] [c01ad628] erase_worker+0x5c/0x670
| [cfa7bf70] [c01ae794] do_work+0xb0/0x16c
| [cfa7bf90] [c01ae990] ubi_thread+0x140/0x1bc
| [cfa7bfd0] [c0037bec] kthread+0x50/0x88
| [cfa7bff0] [c000fdf8] kernel_thread+0x44/0x60
| UBI error: do_sync_erase: cannot erase PEB 3399, error -5
| Call Trace:
| [cfa7be00] [c0008558] show_stack+0x48/0x19c (unreliable)
| [cfa7be40] [c01ac494] do_sync_erase+0x208/0x220
| [cfa7bec0] [c01ac6bc] ubi_io_sync_erase+0x210/0x400
| [cfa7bf40] [c01ad668] erase_worker+0x9c/0x670
| [cfa7bf70] [c01ae794] do_work+0xb0/0x16c
| [cfa7bf90] [c01ae990] ubi_thread+0x140/0x1bc
| [cfa7bfd0] [c0037bec] kthread+0x50/0x88
| [cfa7bff0] [c000fdf8] kernel_thread+0x44/0x60
| UBI error: erase_worker: failed to erase PEB 3399, error -5
| UBI: reserve more 1 PEBs
| UBI: mark PEB 3399 as bad
| UBI: 40 PEBs left in the reserve

So it looks fine. Attaching partitions with non-ubi material failed.

>P.S. I gave it only a very quick try
Works good for that :)

>
>Artem.

Sebastian

Re: [PATCH 0/4] UBI: improve empty flash handling

[Artem Bityutskiy]

On Thu, 2010-05-06 at 11:52 +0200, Sebastian Andrzej Siewior wrote:
> * Artem Bityutskiy | 2010-05-03 13:12:58 [+0300]:
> 
> >Hi Sebastian,
> Hi Artem,
> 
> >Please, review and test the following series.
> 
> The code looks fine. I wiped the flash and the test resulted in:

Thanks, I'll push this to the ubi-2.6.git tree shortly. Will add

Reviewed-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Tested-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>

If you object, please, let me know and I'll amend the tags.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 0/4] UBI: improve empty flash handling

[Artem Bityutskiy]

On Fri, 2010-05-07 at 08:39 +0300, Artem Bityutskiy wrote:
> On Thu, 2010-05-06 at 11:52 +0200, Sebastian Andrzej Siewior wrote:
> > * Artem Bityutskiy | 2010-05-03 13:12:58 [+0300]:
> > 
> > >Hi Sebastian,
> > Hi Artem,
> > 
> > >Please, review and test the following series.
> > 
> > The code looks fine. I wiped the flash and the test resulted in:
> 
> Thanks, I'll push this to the ubi-2.6.git tree shortly. Will add
> 
> Reviewed-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> Tested-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> 
> If you object, please, let me know and I'll amend the tags.

FYI, because these patches did not have enough exposure in linux-next, I
did not merge them so far. But I thing it is fair to consider them as
fixes, so I can merge them in the middle of the 2.6.35 cycle. I'll try
not to forget about this, but you can always remind :-)

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 0/4] UBI: improve empty flash handling

[Artem Bityutskiy]

On Thu, 2010-05-06 at 11:52 +0200, Sebastian Andrzej Siewior wrote:
> * Artem Bityutskiy | 2010-05-03 13:12:58 [+0300]:
> 
> >Hi Sebastian,
> Hi Artem,
> 
> >Please, review and test the following series.
> 
> The code looks fine. I wiped the flash and the test resulted in:

Pushed the patches to the master branch so far. If everything is
allright, I'll push them to linux-next in few days. This is because I
can amend patches which are in the master branch (i.e., rebase the git
tree), but when they go to linux-next, I try hard to avoid any
re-basing.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)