Patchwork mtd/ubi: recognize empty flash with errors as empty

login
register
mail settings
Submitter Sebastian Siewior
Date April 29, 2010, 7:23 p.m.
Message ID <20100429192333.GA12067@Chamillionaire.breakpoint.cc>
Download mbox | patch
Permalink /patch/51305/
State New
Headers show

Comments

Sebastian Siewior - April 29, 2010, 7:23 p.m.
* Artem Bityutskiy | 2010-04-29 12:42:29 [+0300]:

>> So I think the latter case is now broken. In fact I just copied some
>> random things into my mtd partition and after attach & mkvol they were
>> gone with no error.
>
>You mean UBI just attached your device? What would you expect it to do
>when it sees that part of eraseblocks contain corrupted headers? ATM, it
>just formats those eraseblocks. What would be your expectation?

Before the patch attaching a bootloader[0] partition would not work. Now
it does.

>> So in case we want to support something other than UBI then we should
>> probably add another error code in order to distinguish between read
>> error and not a vald EC / VID header.
>
>If you feed UBI flash with no valid UBI headers, it will be refused, I
>think.

It does not, not anymore. si->is_empty is only set if we find a valid EC
header. Unknown data results now in UBI_IO_BAD_{EC|VID}_HDR is excluded
from si->is_empty. Duh.

>I actually do not really see what is the use-case or scenario you want
>UBI to handle better.

The first case was:
- empty flash
- a bad block which is not marked as such.

This is now fixed. However, not empty flash with non-UBI data will be
now UBInized. Earlier, it was refused, you had to use flash_eraseall or
ubiformat first.

In my optinion accidently attaching is not a problem in the field. It is
only a problem if you are attaching by hand and you mix up the
partitions.

The patch attached should fix this and flash with something not UBI
will be refused again. It is compiled tested, don't have hw here right
now.

[0] assumed that the bootloader does not start with 0xff within the
    first few bytes.

Sebastian

---

Subject: mtd/ubi: don't ubinize everything

Since commit 9fb9b36c aka (" UBI: recognize empty flash with errors as
empty") it is now possible to attach a non-empty flash partition and it
will be UBInized. This patch brings the old behavior where attaching of
such partitions will be refused again. This is not a revert because
attaching empty flash with CRC errors will still be ubinized.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Artem Bityutskiy - April 30, 2010, 1:37 p.m.
On Thu, 2010-04-29 at 21:23 +0200, Sebastian Andrzej Siewior wrote:
> * Artem Bityutskiy | 2010-04-29 12:42:29 [+0300]:
> 
> >> So I think the latter case is now broken. In fact I just copied some
> >> random things into my mtd partition and after attach & mkvol they were
> >> gone with no error.
> >
> >You mean UBI just attached your device? What would you expect it to do
> >when it sees that part of eraseblocks contain corrupted headers? ATM, it
> >just formats those eraseblocks. What would be your expectation?
> 
> Before the patch attaching a bootloader[0] partition would not work. Now
> it does.
> 
> >> So in case we want to support something other than UBI then we should
> >> probably add another error code in order to distinguish between read
> >> error and not a vald EC / VID header.
> >
> >If you feed UBI flash with no valid UBI headers, it will be refused, I
> >think.
> 
> It does not, not anymore. si->is_empty is only set if we find a valid EC
> header. Unknown data results now in UBI_IO_BAD_{EC|VID}_HDR is excluded
> from si->is_empty. Duh.
> 
> >I actually do not really see what is the use-case or scenario you want
> >UBI to handle better.
> 
> The first case was:
> - empty flash
> - a bad block which is not marked as such.
> 
> This is now fixed. However, not empty flash with non-UBI data will be
> now UBInized. Earlier, it was refused, you had to use flash_eraseall or
> ubiformat first.
> 
> In my optinion accidently attaching is not a problem in the field. It is
> only a problem if you are attaching by hand and you mix up the
> partitions.
> 
> The patch attached should fix this and flash with something not UBI
> will be refused again. It is compiled tested, don't have hw here right
> now.
> 
> [0] assumed that the bootloader does not start with 0xff within the
>     first few bytes.

Yeah, I see. I am not sure I like your patch because the process_eb()
function becomes even more twisted and unreadable. I'll think if we can
do this another way.

Basically, what I am thinking about is to stop playing with si->is_empty
in process_eb. Instead, make process_eb just account which blocks were
found. And at the end, when all blocks are scanned, look at the
situation, what were the blocks, and decide whether to go for formatting
or not.

I'll send you an e-mail later.

Patch

diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
index 533b1a4..7ed5c4f 100644
--- a/drivers/mtd/ubi/io.c
+++ b/drivers/mtd/ubi/io.c
@@ -763,6 +763,9 @@  int ubi_io_read_ec_hdr(struct ubi_device *ubi, int pnum,
 			return UBI_IO_PEB_EMPTY;
 		}
 
+		if (read_err == -EBADMSG)
+			return UBI_IO_BAD_READ;
+
 		/*
 		 * This is not a valid erase counter header, and these are not
 		 * 0xFF bytes. Report that the header is corrupted.
@@ -1034,6 +1037,9 @@  int ubi_io_read_vid_hdr(struct ubi_device *ubi, int pnum,
 			return UBI_IO_PEB_FREE;
 		}
 
+		if (read_err == -EBADMSG)
+			return UBI_IO_BAD_READ;
+
 		/*
 		 * This is not a valid VID header, and these are not 0xFF
 		 * bytes. Report that the header is corrupted.
diff --git a/drivers/mtd/ubi/scan.c b/drivers/mtd/ubi/scan.c
index 48e570c..99ed225 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_EC_HDR || err = UBI_IO_BAD_READ) {
 		/*
 		 * We have to also look at the VID header, possibly it is not
 		 * corrupted. Set %bitflips flag in order to make this PEB be
@@ -756,11 +756,12 @@  static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
 		bitflips = 1;
 	}
 
+	if (err != UBI_IO_BAD_READ)
+		si->is_empty = 0;
+
 	if (!ec_corr) {
 		int image_seq;
 
-		/* There is an EC header, so the flash is not empty */
-		si->is_empty = 0;
 
 		/* Make sure UBI version is OK */
 		if (ech->version != UBI_VERSION) {
@@ -814,7 +815,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_VID_HDR || err == UBI_IO_BAD_READ ||
 		 (err == UBI_IO_PEB_FREE && ec_corr)) {
 		/* VID header is corrupted */
 		err = add_to_list(si, pnum, ec, &si->corr);
@@ -828,7 +829,8 @@  static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
 			return err;
 		goto adjust_mean_ec;
 	}
-	si->is_empty = 0;
+	if (err != UBI_IO_BAD_READ)
+		si->is_empty = 0;
 
 	vol_id = be32_to_cpu(vidh->vol_id);
 	if (vol_id > UBI_MAX_VOLUMES && vol_id != UBI_LAYOUT_VOLUME_ID) {
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index a637f02..dd8467e 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -99,6 +99,7 @@  enum {
 	UBI_IO_PEB_FREE,
 	UBI_IO_BAD_EC_HDR,
 	UBI_IO_BAD_VID_HDR,
+	UBI_IO_BAD_READ,
 	UBI_IO_BITFLIPS
 };