attaching UBI while getting read errors from NAND driver fails.

attaching UBI while getting read errors from NAND driver fails.

[Sebastian Andrzej Siewior]

Attaching empty nand with a block which contains a RS-Error which can't
be fixed:

| 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:
| [cfbd5c60] [c0008558] show_stack+0x48/0x19c (unreliable)
| [cfbd5ca0] [c01a71e8] ubi_io_read+0x188/0x288
| [cfbd5cf0] [c01a76e8] ubi_io_read_ec_hdr+0x74/0x2a4
| [cfbd5d20] [c01abe9c] ubi_scan+0x178/0x10b4
| [cfbd5d80] [c01a1464] ubi_attach_mtd_dev+0x67c/0xe44
| [cfbd5e80] [c01a1fc8] ctrl_cdev_ioctl+0x178/0x210
| [cfbd5ec0] [c008711c] do_ioctl+0x3c/0xc4
| [cfbd5ee0] [c0087224] vfs_ioctl+0x80/0x448
| [cfbd5f10] [c008762c] sys_ioctl+0x40/0x88
| [cfbd5f40] [c000f960] ret_from_syscall+0x0/0x38
| UBI error: ubi_read_volume_table: the layout volume was not found
| UBI error: ubi_attach_mtd_dev: failed to attach by scanning, error -22

ubi_io_read() calls ubi->mtd->read() which ends in nand_do_read_ops() at
some point. The NAND controller was not able to read the page and
incremented mtd->ecc_stats.failed. The tail of nand_do_read_ops():

|      if (mtd->ecc_stats.failed - stats.failed)
|                return -EBADMSG;

So we return with -EBADMSG as you can see in the error message. This
error is then converted by ubi_io_read() into -EIO:
|                /*
|                 * The driver should never return -EBADMSG if it failed to read
|                 * all the requested data. But some buggy drivers might do
|                 * this, so we change it to -EIO.
|                 */
|                if (read != len && err == -EBADMSG) {
|                        ubi_assert(0);
|                        err = -EIO;
|                }

At this point we leave ubi_scan() and the mtd partition remains
unattached. No really sure how we ended up with -EINVAL in
ubi_attach_mtd_dev().

I guess this is not wanted, is it?

The following patch led to:
| UBI: empty MTD device detected
| 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:        4092
| UBI: number of bad PEBs:         4
| 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:             4048
| 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 1986

ubimkvol gave me:
| UBI error: ubi_io_read: error -74 while reading 64 bytes from PEB 3399:0, read 64 bytes
| nand_erase: Failed erase, page 0x000751c0
| nand_erase: Failed erase, page 0x000751c0
| nand_erase: Failed erase, page 0x000751c0
| nand_erase: Failed erase, page 0x000751c0
| UBI error: do_sync_erase: cannot erase PEB 3399, error -5
| UBI error: erase_worker: failed to erase PEB 3399, error -5
| UBI: mark PEB 3399 as bad
| UBI: 39 PEBs left in the reserve

This looks good so far I guess. Is there much I break with patch? Should
this case be handled at another level?

Sebastian

diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
index 533b1a4..710d069 100644
--- a/drivers/mtd/ubi/io.c
+++ b/drivers/mtd/ubi/io.c
@@ -181,10 +181,12 @@ retry:
 		 * all the requested data. But some buggy drivers might do
 		 * this, so we change it to -EIO.
 		 */
+#if 0
 		if (read != len && err == -EBADMSG) {
 			ubi_assert(0);
 			err = -EIO;
 		}
+#endif
 	} else {
 		ubi_assert(len == read);
 
diff --git a/drivers/mtd/ubi/scan.c b/drivers/mtd/ubi/scan.c
index dc5f688..7529d46 100644
--- a/drivers/mtd/ubi/scan.c
+++ b/drivers/mtd/ubi/scan.c
@@ -756,7 +756,8 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
 		bitflips = 1;
 	}
 
-	si->is_empty = 0;
+	if (err != UBI_IO_BAD_EC_HDR)
+		si->is_empty = 0;
 
 	if (!ec_corr) {
 		int image_seq;
@@ -827,6 +828,7 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
 			return err;
 		goto adjust_mean_ec;
 	}
+	si->is_empty = 0;
 
 	vol_id = be32_to_cpu(vidh->vol_id);
 	if (vol_id > UBI_MAX_VOLUMES && vol_id != UBI_LAYOUT_VOLUME_ID) {

Re: attaching UBI while getting read errors from NAND driver fails.

[Artem Bityutskiy]

On Tue, 2010-04-20 at 20:44 +0200, Sebastian Andrzej Siewior wrote:
> Attaching empty nand with a block which contains a RS-Error which can't
> be fixed:
> 
> | 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:
> | [cfbd5c60] [c0008558] show_stack+0x48/0x19c (unreliable)
> | [cfbd5ca0] [c01a71e8] ubi_io_read+0x188/0x288
> | [cfbd5cf0] [c01a76e8] ubi_io_read_ec_hdr+0x74/0x2a4
> | [cfbd5d20] [c01abe9c] ubi_scan+0x178/0x10b4
> | [cfbd5d80] [c01a1464] ubi_attach_mtd_dev+0x67c/0xe44
> | [cfbd5e80] [c01a1fc8] ctrl_cdev_ioctl+0x178/0x210
> | [cfbd5ec0] [c008711c] do_ioctl+0x3c/0xc4
> | [cfbd5ee0] [c0087224] vfs_ioctl+0x80/0x448
> | [cfbd5f10] [c008762c] sys_ioctl+0x40/0x88
> | [cfbd5f40] [c000f960] ret_from_syscall+0x0/0x38
> | UBI error: ubi_read_volume_table: the layout volume was not found
> | UBI error: ubi_attach_mtd_dev: failed to attach by scanning, error -22
> 
> ubi_io_read() calls ubi->mtd->read() which ends in nand_do_read_ops() at
> some point. The NAND controller was not able to read the page and
> incremented mtd->ecc_stats.failed. The tail of nand_do_read_ops():
> 
> |      if (mtd->ecc_stats.failed - stats.failed)
> |                return -EBADMSG;
> 
> So we return with -EBADMSG as you can see in the error message. This
> error is then converted by ubi_io_read() into -EIO:
> |                /*
> |                 * The driver should never return -EBADMSG if it failed to read
> |                 * all the requested data. But some buggy drivers might do
> |                 * this, so we change it to -EIO.
> |                 */
> |                if (read != len && err == -EBADMSG) {
> |                        ubi_assert(0);
> |                        err = -EIO;
> |                }
> 
> At this point we leave ubi_scan() and the mtd partition remains
> unattached. No really sure how we ended up with -EINVAL in
> ubi_attach_mtd_dev().
> 
> I guess this is not wanted, is it?
> 
> The following patch led to:
> | UBI: empty MTD device detected
> | 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:        4092
> | UBI: number of bad PEBs:         4
> | 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:             4048
> | 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 1986
> 
> ubimkvol gave me:
> | UBI error: ubi_io_read: error -74 while reading 64 bytes from PEB 3399:0, read 64 bytes
> | nand_erase: Failed erase, page 0x000751c0
> | nand_erase: Failed erase, page 0x000751c0
> | nand_erase: Failed erase, page 0x000751c0
> | nand_erase: Failed erase, page 0x000751c0
> | UBI error: do_sync_erase: cannot erase PEB 3399, error -5
> | UBI error: erase_worker: failed to erase PEB 3399, error -5
> | UBI: mark PEB 3399 as bad
> | UBI: 39 PEBs left in the reserve
> 
> This looks good so far I guess. Is there much I break with patch? Should
> this case be handled at another level?

Well, I think the idea was that the MTD layer tries to read all the
requested data despite of a possible unrecoverable ECC error in the
middle. So if it returns -EBADMSG, we know that the read buffer contains
all the data anyway, even though some of the data may be incorrect.

So, I'd suggest changing MTD and making it return read == len in that
case.

> @@ -756,7 +756,8 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
>  		bitflips = 1;
>  	}
>  
> -	si->is_empty = 0;
> +	if (err != UBI_IO_BAD_EC_HDR)
> +		si->is_empty = 0;
>  
>  	if (!ec_corr) {
>  		int image_seq;
> @@ -827,6 +828,7 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
>  			return err;
>  		goto adjust_mean_ec;
>  	}
> +	si->is_empty = 0;
>  
>  	vol_id = be32_to_cpu(vidh->vol_id);
>  	if (vol_id > UBI_MAX_VOLUMES && vol_id != UBI_LAYOUT_VOLUME_ID) {

This part looks right.

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

Re: attaching UBI while getting read errors from NAND driver fails.

[Sebastian Andrzej Siewior]

* Artem Bityutskiy | 2010-04-21 05:50:57 [+0300]:

>On Tue, 2010-04-20 at 20:44 +0200, Sebastian Andrzej Siewior wrote:
>> | UBI error: ubi_io_read: error -74 while reading 64 bytes from PEB 3399:0, read 64 bytes
>> So we return with -EBADMSG as you can see in the error message. This
>> error is then converted by ubi_io_read() into -EIO:
>> |                /*
>> |                 * The driver should never return -EBADMSG if it failed to read
>> |                 * all the requested data. But some buggy drivers might do
>> |                 * this, so we change it to -EIO.
>> |                 */
>> |                if (read != len && err == -EBADMSG) {
>> |                        ubi_assert(0);
>> |                        err = -EIO;
>> |                }
>> 
>Well, I think the idea was that the MTD layer tries to read all the
>requested data despite of a possible unrecoverable ECC error in the
>middle. So if it returns -EBADMSG, we know that the read buffer contains
>all the data anyway, even though some of the data may be incorrect.
>
>So, I'd suggest changing MTD and making it return read == len in that
>case.

Wait. According to the error message, I had read == len (both 64). So I
think it never got changed to -EIO, my mistake.

>> @@ -756,7 +756,8 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
>>  		bitflips = 1;
>>  	}
>>  
>> -	si->is_empty = 0;
>> +	if (err != UBI_IO_BAD_EC_HDR)
>> +		si->is_empty = 0;
>>  
>>  	if (!ec_corr) {
>>  		int image_seq;
>> @@ -827,6 +828,7 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
>>  			return err;
>>  		goto adjust_mean_ec;
>>  	}
>> +	si->is_empty = 0;
>>  
>>  	vol_id = be32_to_cpu(vidh->vol_id);
>>  	if (vol_id > UBI_MAX_VOLUMES && vol_id != UBI_LAYOUT_VOLUME_ID) {
>
>This part looks right.

Do you want me to resend this part as the fix?

Sebastian

[PATCH] mtd/ubi: recognize empty flash with errors as empty

[Sebastian Andrzej Siewior]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Attaching empty nand with a block which contains a RS-Error which can't
be fixed resulted in:

| UBI: attaching mtd9 to ubi0
| UBI error: ubi_io_read: error -74 while reading 64 bytes from PEB 3399:0, read 64 bytes
| Call Trace:
| [cfbd5c60] [c0008558] show_stack+0x48/0x19c (unreliable)
| [cfbd5ca0] [c01a71e8] ubi_io_read+0x188/0x288
| [cfbd5cf0] [c01a76e8] ubi_io_read_ec_hdr+0x74/0x2a4
| [cfbd5d20] [c01abe9c] ubi_scan+0x178/0x10b4
| [cfbd5d80] [c01a1464] ubi_attach_mtd_dev+0x67c/0xe44
| [cfbd5e80] [c01a1fc8] ctrl_cdev_ioctl+0x178/0x210
| [cfbd5ec0] [c008711c] do_ioctl+0x3c/0xc4
| [cfbd5ee0] [c0087224] vfs_ioctl+0x80/0x448
| [cfbd5f10] [c008762c] sys_ioctl+0x40/0x88
| [cfbd5f40] [c000f960] ret_from_syscall+0x0/0x38
| UBI error: ubi_read_volume_table: the layout volume was not found
| UBI error: ubi_attach_mtd_dev: failed to attach by scanning, error -22

Assuming that blocks which can only be read with errors are empty will let
the volume attach. Another access to the block in question resulted here
in:

| UBI error: ubi_io_read: error -74 while reading 64 bytes from PEB 3399:0, read 64 bytes
| nand_erase: Failed erase, page 0x000751c0
| nand_erase: Failed erase, page 0x000751c0
| nand_erase: Failed erase, page 0x000751c0
| nand_erase: Failed erase, page 0x000751c0
| UBI error: do_sync_erase: cannot erase PEB 3399, error -5
| UBI error: erase_worker: failed to erase PEB 3399, error -5
| UBI: mark PEB 3399 as bad
| UBI: 39 PEBs left in the reserve

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/mtd/ubi/scan.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/ubi/scan.c b/drivers/mtd/ubi/scan.c
index dc5f688..7529d46 100644
--- a/drivers/mtd/ubi/scan.c
+++ b/drivers/mtd/ubi/scan.c
@@ -756,7 +756,8 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
 		bitflips = 1;
 	}
 
-	si->is_empty = 0;
+	if (err != UBI_IO_BAD_EC_HDR)
+		si->is_empty = 0;
 
 	if (!ec_corr) {
 		int image_seq;
@@ -827,6 +828,7 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
 			return err;
 		goto adjust_mean_ec;
 	}
+	si->is_empty = 0;
 
 	vol_id = be32_to_cpu(vidh->vol_id);
 	if (vol_id > UBI_MAX_VOLUMES && vol_id != UBI_LAYOUT_VOLUME_ID) {
-- 
1.6.6.1

Re: [PATCH] mtd/ubi: recognize empty flash with errors as empty

[Artem Bityutskiy]

On Fri, 2010-04-23 at 19:28 +0200, Sebastian Andrzej Siewior wrote:
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> Attaching empty nand with a block which contains a RS-Error which can't
> be fixed resulted in:
> 
> | UBI: attaching mtd9 to ubi0
> | UBI error: ubi_io_read: error -74 while reading 64 bytes from PEB 3399:0, read 64 bytes
> | Call Trace:
> | [cfbd5c60] [c0008558] show_stack+0x48/0x19c (unreliable)
> | [cfbd5ca0] [c01a71e8] ubi_io_read+0x188/0x288
> | [cfbd5cf0] [c01a76e8] ubi_io_read_ec_hdr+0x74/0x2a4
> | [cfbd5d20] [c01abe9c] ubi_scan+0x178/0x10b4
> | [cfbd5d80] [c01a1464] ubi_attach_mtd_dev+0x67c/0xe44
> | [cfbd5e80] [c01a1fc8] ctrl_cdev_ioctl+0x178/0x210
> | [cfbd5ec0] [c008711c] do_ioctl+0x3c/0xc4
> | [cfbd5ee0] [c0087224] vfs_ioctl+0x80/0x448
> | [cfbd5f10] [c008762c] sys_ioctl+0x40/0x88
> | [cfbd5f40] [c000f960] ret_from_syscall+0x0/0x38
> | UBI error: ubi_read_volume_table: the layout volume was not found
> | UBI error: ubi_attach_mtd_dev: failed to attach by scanning, error -22
> 
> Assuming that blocks which can only be read with errors are empty will let
> the volume attach. Another access to the block in question resulted here
> in:
> 
> | UBI error: ubi_io_read: error -74 while reading 64 bytes from PEB 3399:0, read 64 bytes
> | nand_erase: Failed erase, page 0x000751c0
> | nand_erase: Failed erase, page 0x000751c0
> | nand_erase: Failed erase, page 0x000751c0
> | nand_erase: Failed erase, page 0x000751c0
> | UBI error: do_sync_erase: cannot erase PEB 3399, error -5
> | UBI error: erase_worker: failed to erase PEB 3399, error -5
> | UBI: mark PEB 3399 as bad
> | UBI: 39 PEBs left in the reserve
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/mtd/ubi/scan.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mtd/ubi/scan.c b/drivers/mtd/ubi/scan.c
> index dc5f688..7529d46 100644
> --- a/drivers/mtd/ubi/scan.c
> +++ b/drivers/mtd/ubi/scan.c
> @@ -756,7 +756,8 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
>  		bitflips = 1;
>  	}
>  
> -	si->is_empty = 0;
> +	if (err != UBI_IO_BAD_EC_HDR)
> +		si->is_empty = 0;
>  
>  	if (!ec_corr) {
>  		int image_seq;
> @@ -827,6 +828,7 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
>  			return err;
>  		goto adjust_mean_ec;
>  	}
> +	si->is_empty = 0;
>  
>  	vol_id = be32_to_cpu(vidh->vol_id);
>  	if (vol_id > UBI_MAX_VOLUMES && vol_id != UBI_LAYOUT_VOLUME_ID) {

Thanks, pushed to ubi-2.6.git / master with the following minor tweak,
please check:

diff --git a/drivers/mtd/ubi/scan.c b/drivers/mtd/ubi/scan.c
index 7529d46..48e570c 100644
--- a/drivers/mtd/ubi/scan.c
+++ b/drivers/mtd/ubi/scan.c
@@ -756,12 +756,12 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
                bitflips = 1;
        }
 
-       if (err != UBI_IO_BAD_EC_HDR)
-               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) {
                        ubi_err("this UBI version is %d, image version is %d",

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

Re: [PATCH] mtd/ubi: recognize empty flash with errors as empty

[Sebastian Andrzej Siewior]

* Artem Bityutskiy | 2010-04-24 14:24:01 [+0300]:

>Thanks, pushed to ubi-2.6.git / master with the following minor tweak,
>please check:
>
>diff --git a/drivers/mtd/ubi/scan.c b/drivers/mtd/ubi/scan.c
>index 7529d46..48e570c 100644
>--- a/drivers/mtd/ubi/scan.c
>+++ b/drivers/mtd/ubi/scan.c
>@@ -756,12 +756,12 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
>                bitflips = 1;
>        }
> 
>-       if (err != UBI_IO_BAD_EC_HDR)
>-               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) {
>                        ubi_err("this UBI version is %d, image version is %d",
>

I guess that's okay. What are the chances that you can't read the EC
header but you can somehow read the VID header. AND if there is a valid
VID header then there is more, and si->is_empty will be set later on,
right?

Sebastian

Re: [PATCH] mtd/ubi: recognize empty flash with errors as empty

[Artem Bityutskiy]

On Sun, 2010-04-25 at 23:09 +0200, Sebastian Andrzej Siewior wrote:
> * Artem Bityutskiy | 2010-04-24 14:24:01 [+0300]:
> 
> >Thanks, pushed to ubi-2.6.git / master with the following minor tweak,
> >please check:
> >
> >diff --git a/drivers/mtd/ubi/scan.c b/drivers/mtd/ubi/scan.c
> >index 7529d46..48e570c 100644
> >--- a/drivers/mtd/ubi/scan.c
> >+++ b/drivers/mtd/ubi/scan.c
> >@@ -756,12 +756,12 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
> >                bitflips = 1;
> >        }
> > 
> >-       if (err != UBI_IO_BAD_EC_HDR)
> >-               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) {
> >                        ubi_err("this UBI version is %d, image version is %d",
> >
> 
> I guess that's okay. What are the chances that you can't read the EC
> header but you can somehow read the VID header. 

When the VID header sits in the next NAND page, there are some changes,
but I never observed such a situation in practice.

> AND if there is a valid
> VID header then there is more, and si->is_empty will be set later on,
> right?

Yes, AFAICS.

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

Re: [PATCH] mtd/ubi: recognize empty flash with errors as empty

[Sebastian Andrzej Siewior]

* Artem Bityutskiy | 2010-04-26 07:59:50 [+0300]:

>On Sun, 2010-04-25 at 23:09 +0200, Sebastian Andrzej Siewior wrote:
>> * Artem Bityutskiy | 2010-04-24 14:24:01 [+0300]:
>> 
>> >Thanks, pushed to ubi-2.6.git / master with the following minor tweak,
>> >please check:
>> >
>> >diff --git a/drivers/mtd/ubi/scan.c b/drivers/mtd/ubi/scan.c
>> >index 7529d46..48e570c 100644
>> >--- a/drivers/mtd/ubi/scan.c
>> >+++ b/drivers/mtd/ubi/scan.c
>> >@@ -756,12 +756,12 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
>> >                bitflips = 1;
>> >        }
>> > 
>> >-       if (err != UBI_IO_BAD_EC_HDR)
>> >-               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) {
>> >                        ubi_err("this UBI version is %d, image version is %d",
>> >
>> 
>> I guess that's okay. What are the chances that you can't read the EC
>> header but you can somehow read the VID header. 
>
>When the VID header sits in the next NAND page, there are some changes,
>but I never observed such a situation in practice.
>
>> AND if there is a valid
>> VID header then there is more, and si->is_empty will be set later on,
>> right?
>
>Yes, AFAICS.

Oh. UBI_IO_BAD_EC_HDR / UBI_IO_BAD_VID_HDR is returned when
- the page page can not be read
- the page contains non-ubi information

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.

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.

Sebastian

Re: [PATCH] mtd/ubi: recognize empty flash with errors as empty

[Artem Bityutskiy]

On Mon, 2010-04-26 at 10:28 +0200, Sebastian Andrzej Siewior wrote:
> * Artem Bityutskiy | 2010-04-26 07:59:50 [+0300]:
> 
> >On Sun, 2010-04-25 at 23:09 +0200, Sebastian Andrzej Siewior wrote:
> >> * Artem Bityutskiy | 2010-04-24 14:24:01 [+0300]:
> >> 
> >> >Thanks, pushed to ubi-2.6.git / master with the following minor tweak,
> >> >please check:
> >> >
> >> >diff --git a/drivers/mtd/ubi/scan.c b/drivers/mtd/ubi/scan.c
> >> >index 7529d46..48e570c 100644
> >> >--- a/drivers/mtd/ubi/scan.c
> >> >+++ b/drivers/mtd/ubi/scan.c
> >> >@@ -756,12 +756,12 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si,
> >> >                bitflips = 1;
> >> >        }
> >> > 
> >> >-       if (err != UBI_IO_BAD_EC_HDR)
> >> >-               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) {
> >> >                        ubi_err("this UBI version is %d, image version is %d",
> >> >
> >> 
> >> I guess that's okay. What are the chances that you can't read the EC
> >> header but you can somehow read the VID header. 
> >
> >When the VID header sits in the next NAND page, there are some changes,
> >but I never observed such a situation in practice.
> >
> >> AND if there is a valid
> >> VID header then there is more, and si->is_empty will be set later on,
> >> right?
> >
> >Yes, AFAICS.
> 
> Oh. UBI_IO_BAD_EC_HDR / UBI_IO_BAD_VID_HDR is returned when
> - the page page can not be read
> - the page contains non-ubi information

Bear in mind that it is difficult to distinguish between non-UBI
information and just very corrupted headers, so ATM, in case of CRC
error, UBI assumes this is a corrupted header, although this could
non-UBI stuff.

> 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?

> 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.

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

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

Re: [PATCH] mtd/ubi: recognize empty flash with errors as empty

[Sebastian Andrzej Siewior]

* 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>

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
 };
 
-- 
1.6.6.1

Re: [PATCH] mtd/ubi: recognize empty flash with errors as empty

[Artem Bityutskiy]

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.

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