[PATCH 1/1] MTD: UBI: try to avoid program data to NOR flash after erasure interrupted

[PATCH 1/1] MTD: UBI: try to avoid program data to NOR flash after erasure interrupted

[Qi Wang 王起 (qiwang)]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3802 bytes --]

On Sa, 2013-10-26 at 05:19 +0000, Artem Bityutskiy wrote:
>On Thu, 2013-10-10 at 08:28 +0000, Qi Wang 王起 (qiwang) wrote:
>> But I want to say the potential risk is if low level driver program data to 
>> this block, it will get “timeout error”. And the timeout period could be very 
>> long(almost several minutes), during this period, any operation on NOR flash 
>> cannot be accept. so program data to a erasure interrupted block isn’t a 
>> sensible action. in order to avoid program a erasure interrupted block, 
>> I suggest UBIFS can read this block before program data. the code changing as below:
>
>Yes, reading first sounds like a good idea. Would you please send a
>patch implementing it?

From: Qi Wang <qiwang@micron.com>

nor_erase_prepare() will be called before erase a NOR flash, it will program '0'
into a block to mark this block. But program data into a erasure interrupted block
can cause program timtout(several minutes at most) error, could impact other 
operation on NOR flash. So UBIFS can read this block first to avoid unneeded 
program operation. 

This patch try to put read operation at at head of write operation in 
nor_erase_prepare(), read out the data. 
If the data is already corrupt, then no need to program any data into this block, 
just go to erase this block.

Signed-off-by: Qi Wang <qiwang@qiwang@micron.com>
---
diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
index bf79def..be6ab56 100644
--- a/drivers/mtd/ubi/io.c
+++ b/drivers/mtd/ubi/io.c
@@ -509,26 +509,10 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum)
 	struct ubi_vid_hdr vid_hdr;
 
 	/*
-	 * It is important to first invalidate the EC header, and then the VID
-	 * header. Otherwise a power cut may lead to valid EC header and
-	 * invalid VID header, in which case UBI will treat this PEB as
-	 * corrupted and will try to preserve it, and print scary warnings.
-	 */
-	addr = (loff_t)pnum * ubi->peb_size;
-	err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
-	if (!err) {
-		addr += ubi->vid_hdr_aloffset;
-		err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
-		if (!err)
-			return 0;
-	}
-
-	/*
-	 * We failed to write to the media. This was observed with Spansion
-	 * S29GL512N NOR flash. Most probably the previously eraseblock erasure
+	 * Most probably the previously eraseblock erasure
 	 * was interrupted at a very inappropriate moment, so it became
-	 * unwritable. In this case we probably anyway have garbage in this
-	 * PEB.
+	 * unwritable. In order to avoid program data into a unwritable block,
+	 * read this block first, and check if do need to program it.  
 	 */
 	err1 = ubi_io_read_vid_hdr(ubi, pnum, &vid_hdr, 0);
 	if (err1 == UBI_IO_BAD_HDR_EBADMSG || err1 == UBI_IO_BAD_HDR ||
@@ -547,6 +531,22 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum)
 	}
 
 	/*
+	 * VID or EC is valid, need to corrupt these header before erase operation.  
+	 * It is important to first invalidate the EC header, and then the VID
+	 * header. Otherwise a power cut may lead to valid EC header and
+	 * invalid VID header, in which case UBI will treat this PEB as
+	 * corrupted and will try to preserve it, and print scary warnings.
+	 */
+	addr = (loff_t)pnum * ubi->peb_size;
+	err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
+	if (!err) {
+		addr += ubi->vid_hdr_aloffset;
+		err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
+		if (!err)
+			return 0;
+	}
+
+	/*
 	 * The PEB contains a valid VID header, but we cannot invalidate it.
 	 * Supposedly the flash media or the driver is screwed up, so return an
 	 * error.
---

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH 1/1] MTD: UBI: try to avoid program data to NOR flash after erasure interrupted

[Qi Wang 王起 (qiwang)]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3802 bytes --]

On Sa, 2013-10-26 at 05:19 +0000, Artem Bityutskiy wrote:
>On Thu, 2013-10-10 at 08:28 +0000, Qi Wang 王起 (qiwang) wrote:
>> But I want to say the potential risk is if low level driver program data to 
>> this block, it will get “timeout error”. And the timeout period could be very 
>> long(almost several minutes), during this period, any operation on NOR flash 
>> cannot be accept. so program data to a erasure interrupted block isn’t a 
>> sensible action. in order to avoid program a erasure interrupted block, 
>> I suggest UBIFS can read this block before program data. the code changing as below:
>
>Yes, reading first sounds like a good idea. Would you please send a
>patch implementing it?

From: Qi Wang <qiwang@micron.com>

nor_erase_prepare() will be called before erase a NOR flash, it will program '0'
into a block to mark this block. But program data into a erasure interrupted block
can cause program timtout(several minutes at most) error, could impact other 
operation on NOR flash. So UBIFS can read this block first to avoid unneeded 
program operation. 

This patch try to put read operation at at head of write operation in 
nor_erase_prepare(), read out the data. 
If the data is already corrupt, then no need to program any data into this block, 
just go to erase this block.

Signed-off-by: Qi Wang <qiwang@qiwang@micron.com>
---
diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
index bf79def..be6ab56 100644
--- a/drivers/mtd/ubi/io.c
+++ b/drivers/mtd/ubi/io.c
@@ -509,26 +509,10 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum)
 	struct ubi_vid_hdr vid_hdr;
 
 	/*
-	 * It is important to first invalidate the EC header, and then the VID
-	 * header. Otherwise a power cut may lead to valid EC header and
-	 * invalid VID header, in which case UBI will treat this PEB as
-	 * corrupted and will try to preserve it, and print scary warnings.
-	 */
-	addr = (loff_t)pnum * ubi->peb_size;
-	err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
-	if (!err) {
-		addr += ubi->vid_hdr_aloffset;
-		err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
-		if (!err)
-			return 0;
-	}
-
-	/*
-	 * We failed to write to the media. This was observed with Spansion
-	 * S29GL512N NOR flash. Most probably the previously eraseblock erasure
+	 * Most probably the previously eraseblock erasure
 	 * was interrupted at a very inappropriate moment, so it became
-	 * unwritable. In this case we probably anyway have garbage in this
-	 * PEB.
+	 * unwritable. In order to avoid program data into a unwritable block,
+	 * read this block first, and check if do need to program it.  
 	 */
 	err1 = ubi_io_read_vid_hdr(ubi, pnum, &vid_hdr, 0);
 	if (err1 == UBI_IO_BAD_HDR_EBADMSG || err1 == UBI_IO_BAD_HDR ||
@@ -547,6 +531,22 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum)
 	}
 
 	/*
+	 * VID or EC is valid, need to corrupt these header before erase operation.  
+	 * It is important to first invalidate the EC header, and then the VID
+	 * header. Otherwise a power cut may lead to valid EC header and
+	 * invalid VID header, in which case UBI will treat this PEB as
+	 * corrupted and will try to preserve it, and print scary warnings.
+	 */
+	addr = (loff_t)pnum * ubi->peb_size;
+	err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
+	if (!err) {
+		addr += ubi->vid_hdr_aloffset;
+		err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
+		if (!err)
+			return 0;
+	}
+
+	/*
 	 * The PEB contains a valid VID header, but we cannot invalidate it.
 	 * Supposedly the flash media or the driver is screwed up, so return an
 	 * error.
---

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/1] MTD: UBI: try to avoid program data to NOR flash after erasure interrupted

[Artem Bityutskiy]

On Mon, 2013-10-28 at 04:54 +0000, Qi Wang 王起 (qiwang) wrote:
> On Sa, 2013-10-26 at 05:19 +0000, Artem Bityutskiy wrote:
> >On Thu, 2013-10-10 at 08:28 +0000, Qi Wang 王起 (qiwang) wrote:
> >> But I want to say the potential risk is if low level driver program data to 
> >> this block, it will get “timeout error”. And the timeout period could be very 
> >> long(almost several minutes), during this period, any operation on NOR flash 
> >> cannot be accept. so program data to a erasure interrupted block isn’t a 
> >> sensible action. in order to avoid program a erasure interrupted block, 
> >> I suggest UBIFS can read this block before program data. the code changing as below:
> >
> >Yes, reading first sounds like a good idea. Would you please send a
> >patch implementing it?
> 
> From: Qi Wang <qiwang@micron.com>
> 
> nor_erase_prepare() will be called before erase a NOR flash, it will program '0'
> into a block to mark this block. But program data into a erasure interrupted block
> can cause program timtout(several minutes at most) error, could impact other 
> operation on NOR flash. So UBIFS can read this block first to avoid unneeded 
> program operation. 
> 
> This patch try to put read operation at at head of write operation in 
> nor_erase_prepare(), read out the data. 
> If the data is already corrupt, then no need to program any data into this block, 
> just go to erase this block.
> 
> Signed-off-by: Qi Wang <qiwang@qiwang@micron.com>
> ---
> diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
> index bf79def..be6ab56 100644
> --- a/drivers/mtd/ubi/io.c
> +++ b/drivers/mtd/ubi/io.c
> @@ -509,26 +509,10 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum)
>  	struct ubi_vid_hdr vid_hdr;
>  
>  	/*
> -	 * It is important to first invalidate the EC header, and then the VID
> -	 * header. Otherwise a power cut may lead to valid EC header and
> -	 * invalid VID header, in which case UBI will treat this PEB as
> -	 * corrupted and will try to preserve it, and print scary warnings.
> -	 */
> -	addr = (loff_t)pnum * ubi->peb_size;
> -	err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
> -	if (!err) {
> -		addr += ubi->vid_hdr_aloffset;
> -		err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
> -		if (!err)
> -			return 0;
> -	}

How about structuring the code this way:

if (EC header is good)
     invalidate EC header()
if (VID header is good)
     invalidate VID header()

Then you'll handle the case when only one of the headers is already
corrupted.

-- 
Best Regards,
Artem Bityutskiy

RE: [PATCH 1/1] MTD: UBI: try to avoid program data to NOR flash after erasure interrupted

[Qi Wang 王起 (qiwang)]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 7955 bytes --]

On Tue, 2013-10-29 at 11:19 +0000, Artem Bityutskiy wrote:
> On Mon, 2013-10-28 at 04:54 +0000, Qi Wang 王起 (qiwang) wrote:
> > On Sa, 2013-10-26 at 05:19 +0000, Artem Bityutskiy wrote:
> > > On Thu, 2013-10-10 at 08:28 +0000, Qi Wang 王起 (qiwang) wrote:
> > > > But I want to say the potential risk is if low level driver program data to 
> > > > this block, it will get “timeout error”. And the timeout period could be very 
> > > > long(almost several minutes), during this period, any operation on NOR flash 
> > > > cannot be accept. so program data to a erasure interrupted block isn’t a 
> > > > sensible action. in order to avoid program a erasure interrupted block, 
> > > > I suggest UBIFS can read this block before program data. the code changing as below:
> > > 
> > > Yes, reading first sounds like a good idea. Would you please send a
> > > patch implementing it?
> > 
> > From: Qi Wang <qiwang@micron.com>
> > 
> > nor_erase_prepare() will be called before erase a NOR flash, it will program '0'
> > into a block to mark this block. But program data into a erasure interrupted block
> > can cause program timtout(several minutes at most) error, could impact other 
> > operation on NOR flash. So UBIFS can read this block first to avoid unneeded 
> > program operation. 
> > 
> > This patch try to put read operation at at head of write operation in 
> > nor_erase_prepare(), read out the data. 
> > If the data is already corrupt, then no need to program any data into this block, 
> > just go to erase this block.
> > 
> > Signed-off-by: Qi Wang <qiwang@qiwang@micron.com>
> > ---
> > diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
> > index bf79def..be6ab56 100644
> > --- a/drivers/mtd/ubi/io.c
> > +++ b/drivers/mtd/ubi/io.c
> > @@ -509,26 +509,10 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum)
> >  	struct ubi_vid_hdr vid_hdr;
> >  
> >  	/*
> > -	 * It is important to first invalidate the EC header, and then the VID
> > -	 * header. Otherwise a power cut may lead to valid EC header and
> > -	 * invalid VID header, in which case UBI will treat this PEB as
> > -	 * corrupted and will try to preserve it, and print scary warnings.
> > -	 */
> > -	addr = (loff_t)pnum * ubi->peb_size;
> > -	err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
> > -	if (!err) {
> > -		addr += ubi->vid_hdr_aloffset;
> > -		err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
> > -		if (!err)
> > -			return 0;
> > -	}
> 
> How about structuring the code this way:
> 
> if (EC header is good)
>      invalidate EC header()
> if (VID header is good)
>      invalidate VID header()
> 
> Then you'll handle the case when only one of the headers is already
> corrupted.
>
> -- 
> Best Regards,
> Artem Bityutskiy


Hi Artem:
Please check below patch. 

From: Qi Wang <qiwang@micron.com>

nor_erase_prepare() will be called before erase a NOR flash, it will program '0'
into a block to mark this block. But program data into a erasure interrupted block
can cause program timtout(several minutes at most) error, could impact other 
operation on NOR flash. So UBIFS can read this block first to avoid unneeded 
program operation. 
 
This patch try to put read operation at at head of write operation in 
nor_erase_prepare(), read out the data. 
If the data is already corrupt, then no need to program any data into this block, 
just go to erase this block.

Signed-off-by: Qi Wang <qiwang@qiwang@micron.com>
---
diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
index bf79def..0fdaca9 100644
--- a/drivers/mtd/ubi/io.c
+++ b/drivers/mtd/ubi/io.c
@@ -499,59 +499,44 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum)
 	size_t written;
 	loff_t addr;
 	uint32_t data = 0;
-	/*
-	 * Note, we cannot generally define VID header buffers on stack,
-	 * because of the way we deal with these buffers (see the header
-	 * comment in this file). But we know this is a NOR-specific piece of
-	 * code, so we can do this. But yes, this is error-prone and we should
-	 * (pre-)allocate VID header buffer instead.
-	 */
 	struct ubi_vid_hdr vid_hdr;
+	struct ubi_ec_hdr ec_hdr;
+
+	addr = (loff_t)pnum * ubi->peb_size;
 
 	/*
+	 * If VID or EC is valid, need to corrupt it before erase operation.  
 	 * It is important to first invalidate the EC header, and then the VID
 	 * header. Otherwise a power cut may lead to valid EC header and
 	 * invalid VID header, in which case UBI will treat this PEB as
 	 * corrupted and will try to preserve it, and print scary warnings.
 	 */
-	addr = (loff_t)pnum * ubi->peb_size;
-	err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
-	if (!err) {
-		addr += ubi->vid_hdr_aloffset;
-		err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
-		if (!err)
-			return 0;
+	err = ubi_io_read_ec_hdr(ubi, pnum, &ec_hdr, 0);
+	if (err != UBI_IO_BAD_HDR_EBADMSG && err != UBI_IO_BAD_HDR &&
+	    err != UBI_IO_FF){
+		err1 = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
+		if(err1)
+			goto error;
 	}
 
-	/*
-	 * We failed to write to the media. This was observed with Spansion
-	 * S29GL512N NOR flash. Most probably the previously eraseblock erasure
-	 * was interrupted at a very inappropriate moment, so it became
-	 * unwritable. 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_EBADMSG || err1 == UBI_IO_BAD_HDR ||
-	    err1 == UBI_IO_FF) {
-		struct ubi_ec_hdr ec_hdr;
-
-		err1 = ubi_io_read_ec_hdr(ubi, pnum, &ec_hdr, 0);
-		if (err1 == UBI_IO_BAD_HDR_EBADMSG || err1 == UBI_IO_BAD_HDR ||
-		    err1 == UBI_IO_FF)
-			/*
-			 * Both VID and EC headers are corrupted, so we can
-			 * safely erase this PEB and not afraid that it will be
-			 * treated as a valid PEB in case of an unclean reboot.
-			 */
-			return 0;
+	err = ubi_io_read_vid_hdr(ubi, pnum, &vid_hdr, 0);
+	if (err != UBI_IO_BAD_HDR_EBADMSG && err != UBI_IO_BAD_HDR &&
+	    err != UBI_IO_FF){
+	    	addr += ubi->vid_hdr_aloffset;
+		err1 = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
+		if (err1)
+			goto error;	
 	}
 
+	return 0;
+	
+error:
 	/*
-	 * The PEB contains a valid VID header, but we cannot invalidate it.
+	 * The PEB contains a valid VID or EC header, but we cannot invalidate it.
 	 * Supposedly the flash media or the driver is screwed up, so return an
 	 * error.
 	 */
-	ubi_err("cannot invalidate PEB %d, write returned %d read returned %d",
+	ubi_err("cannot invalidate PEB %d, read returned %d write returned %d ",
 		pnum, err, err1);
 	ubi_dump_flash(ubi, pnum, 0, ubi->peb_size);
 	return -EIO;
---

My Comments for above changing:
1. 
	-	/*
	-	 * Note, we cannot generally define VID header buffers on stack,
	-	 * because of the way we deal with these buffers (see the header
	-	 * comment in this file). But we know this is a NOR-specific piece of
	-	 * code, so we can do this. But yes, this is error-prone and we should
	-	 * (pre-)allocate VID header buffer instead.
	-	 */
	I remove above comment, because I pre-allocate VID header and EC header together. 
	So I think no need to emphasize VID header buffers cannot be on stack.
	(Maybe my understanding about this comment is error, if so, please correct me)

2.
	why use
	"if (err != UBI_IO_BAD_HDR_EBADMSG && err != UBI_IO_BAD_HDR && err != UBI_IO_FF)"
	but not 
	"if (!err)" 
	to judge if need to program '0' to invalid this block.

	In case err == UBI_IO_FF_BITFLIPS, err == UBI_IO_BITFLIPS or unexpected value return
	from read function, I think UBI still need to invalid this block for above mentioned 
	condition. So I use
	"if (err != UBI_IO_BAD_HDR_EBADMSG && err != UBI_IO_BAD_HDR && err != UBI_IO_FF)"
	to judge.  


Please make me known if my understanding isn't right. 
Thanks



ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/1] MTD: UBI: try to avoid program data to NOR flash after erasure interrupted

[Artem Bityutskiy]

Hi,

could you please re-send your patch separately, without quoting any
parts of this conversation, so that I could use 'git am'.

Your patch also contains trailing white-spaces, please, get rid of them
in the next submission.

Also, could you please clearly state whether you have tested this patch
on a real NOR flash or not. If yes, then could you share the chip
vendor/type information?

On Thu, 2013-10-31 at 04:07 +0000, Qi Wang 王起 (qiwang) wrote:

> --- a/drivers/mtd/ubi/io.c
> +++ b/drivers/mtd/ubi/io.c
> @@ -499,59 +499,44 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum)
>  	size_t written;
>  	loff_t addr;
>  	uint32_t data = 0;
> -	/*
> -	 * Note, we cannot generally define VID header buffers on stack,
> -	 * because of the way we deal with these buffers (see the header
> -	 * comment in this file). But we know this is a NOR-specific piece of
> -	 * code, so we can do this. But yes, this is error-prone and we should
> -	 * (pre-)allocate VID header buffer instead.
> -	 */

Please, do not remove this comment.

>  	struct ubi_vid_hdr vid_hdr;
> +	struct ubi_ec_hdr ec_hdr;

To make it obvious what the above big comment talks about, could you
please define 'struct ubi_ec_hdr ec_hdr' above that big comment.

Otherwise looks good to me, thank you!


> My Comments for above changing:
> 1. 
> 	-	/*
> 	-	 * Note, we cannot generally define VID header buffers on stack,
> 	-	 * because of the way we deal with these buffers (see the header
> 	-	 * comment in this file). But we know this is a NOR-specific piece of
> 	-	 * code, so we can do this. But yes, this is error-prone and we should
> 	-	 * (pre-)allocate VID header buffer instead.
> 	-	 */
> 	I remove above comment, because I pre-allocate VID header and EC header together. 
> 	So I think no need to emphasize VID header buffers cannot be on stack.
> 	(Maybe my understanding about this comment is error, if so, please correct me)

The problem is that some functions in io.c can read or write _beyond_
sizeof(struct ubi_vid_hdr), but this is only relevant to NAND, not for
NOR, and the code you change is NOR-only. This is why that comment is
there, and I'd like to keep it.

> 2.
> 	why use
> 	"if (err != UBI_IO_BAD_HDR_EBADMSG && err != UBI_IO_BAD_HDR && err != UBI_IO_FF)"
> 	but not 
> 	"if (!err)" 
> 	to judge if need to program '0' to invalid this block.
> 
> 	In case err == UBI_IO_FF_BITFLIPS, err == UBI_IO_BITFLIPS or unexpected value return
> 	from read function, I think UBI still need to invalid this block for above mentioned 
> 	condition. So I use
> 	"if (err != UBI_IO_BAD_HDR_EBADMSG && err != UBI_IO_BAD_HDR && err != UBI_IO_FF)"
> 	to judge. 

In case of UBI_IO_FF (all FFs) UBI will erase the eraseblock before
using it anyway, so invalidation is not necessary.

Thanks!

-- 
Best Regards,
Artem Bityutskiy

RE: [PATCH 1/1] MTD: UBI: try to avoid program data to NOR flash after erasure interrupted

[Qi Wang 王起 (qiwang)]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3624 bytes --]

Hi Artem:
Sorry to interrupt your busy life. 
As you said in previous mail, I send my patch separately without quoting this e-mail. And I have send to you, but I never get  your reply. I am very confuse, no sure if is there anything wrong at the patch I send to you.
Can you help explain to me?
Thanks

-----Original Message-----
From: Artem Bityutskiy [mailto:dedekind1@gmail.com] 
Sent: Friday, November 01, 2013 4:58 PM
To: Qi Wang 王起 (qiwang)
Cc: linux-mtd@lists.infradead.org; Adrian Hunter; linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] MTD: UBI: try to avoid program data to NOR flash after erasure interrupted

Hi,

could you please re-send your patch separately, without quoting any
parts of this conversation, so that I could use 'git am'.

Your patch also contains trailing white-spaces, please, get rid of them
in the next submission.

Also, could you please clearly state whether you have tested this patch
on a real NOR flash or not. If yes, then could you share the chip
vendor/type information?

On Thu, 2013-10-31 at 04:07 +0000, Qi Wang 王起 (qiwang) wrote:

> --- a/drivers/mtd/ubi/io.c
> +++ b/drivers/mtd/ubi/io.c
> @@ -499,59 +499,44 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum)
>  	size_t written;
>  	loff_t addr;
>  	uint32_t data = 0;
> -	/*
> -	 * Note, we cannot generally define VID header buffers on stack,
> -	 * because of the way we deal with these buffers (see the header
> -	 * comment in this file). But we know this is a NOR-specific piece of
> -	 * code, so we can do this. But yes, this is error-prone and we should
> -	 * (pre-)allocate VID header buffer instead.
> -	 */

Please, do not remove this comment.

>  	struct ubi_vid_hdr vid_hdr;
> +	struct ubi_ec_hdr ec_hdr;

To make it obvious what the above big comment talks about, could you
please define 'struct ubi_ec_hdr ec_hdr' above that big comment.

Otherwise looks good to me, thank you!


> My Comments for above changing:
> 1. 
> 	-	/*
> 	-	 * Note, we cannot generally define VID header buffers on stack,
> 	-	 * because of the way we deal with these buffers (see the header
> 	-	 * comment in this file). But we know this is a NOR-specific piece of
> 	-	 * code, so we can do this. But yes, this is error-prone and we should
> 	-	 * (pre-)allocate VID header buffer instead.
> 	-	 */
> 	I remove above comment, because I pre-allocate VID header and EC header together. 
> 	So I think no need to emphasize VID header buffers cannot be on stack.
> 	(Maybe my understanding about this comment is error, if so, please correct me)

The problem is that some functions in io.c can read or write _beyond_
sizeof(struct ubi_vid_hdr), but this is only relevant to NAND, not for
NOR, and the code you change is NOR-only. This is why that comment is
there, and I'd like to keep it.

> 2.
> 	why use
> 	"if (err != UBI_IO_BAD_HDR_EBADMSG && err != UBI_IO_BAD_HDR && err != UBI_IO_FF)"
> 	but not 
> 	"if (!err)" 
> 	to judge if need to program '0' to invalid this block.
> 
> 	In case err == UBI_IO_FF_BITFLIPS, err == UBI_IO_BITFLIPS or unexpected value return
> 	from read function, I think UBI still need to invalid this block for above mentioned 
> 	condition. So I use
> 	"if (err != UBI_IO_BAD_HDR_EBADMSG && err != UBI_IO_BAD_HDR && err != UBI_IO_FF)"
> 	to judge. 

In case of UBI_IO_FF (all FFs) UBI will erase the eraseblock before
using it anyway, so invalidation is not necessary.

Thanks!

-- 
Best Regards,
Artem Bityutskiy

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/1] MTD: UBI: try to avoid program data to NOR flash after erasure interrupted

[Brian Norris]

On Mon, Dec 23, 2013 at 02:03:16AM +0000, Qi Wang 王起 (qiwang) wrote:
> Sorry to interrupt your busy life. 

Yes, unfortunately Artem hasn't been too responsive lately.

> As you said in previous mail, I send my patch separately without
> quoting this e-mail. And I have send to you, but I never get  your
> reply. I am very confuse, no sure if is there anything wrong at the
> patch I send to you.
> Can you help explain to me?

I'm not really a UBI expert, but I doubt that there is a real problem
with your patch. Artem just hasn't had time to look at it again.

Although you have been waiting a while, I suppose the best option is
simply to wait some more. Artem tends to revisit patches eventually,
sometimes after several weeks/months.

Brian

Re: [PATCH 1/1] MTD: UBI: try to avoid program data to NOR flash after erasure interrupted

[Qi Wang 王起 (qiwang)]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 4194 bytes --]

Hi:
As we talked in mail before, please check my patch as below:

From: Qi Wang <qiwang@micron.com>

nor_erase_prepare() will be called before erase a NOR flash, it will program '0'
into a block to mark this block. But program data into a erasure interrupted block
can cause program timtout(several minutes at most) error, could impact other 
operation on NOR flash. So UBIFS can read this block first to avoid unneeded 
program operation. 

This patch try to put read operation at head of write operation in 
nor_erase_prepare(), read out the data. 
If the data is already corrupt, then no need to program any data into this block, 
just go to erase this block.

Signed-off-by: Qi Wang <qiwang@qiwang@micron.com>
---
diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
index bf79def..0a6343d 100644
--- a/drivers/mtd/ubi/io.c
+++ b/drivers/mtd/ubi/io.c
@@ -499,6 +499,7 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum)
 	size_t written;
 	loff_t addr;
 	uint32_t data = 0;
+	struct ubi_ec_hdr ec_hdr;
 	/*
 	 * Note, we cannot generally define VID header buffers on stack,
 	 * because of the way we deal with these buffers (see the header
@@ -509,49 +510,38 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum)
 	struct ubi_vid_hdr vid_hdr;
 
 	/*
+	 * If VID or EC is valid, need to corrupt it before erase operation.  
 	 * It is important to first invalidate the EC header, and then the VID
 	 * header. Otherwise a power cut may lead to valid EC header and
 	 * invalid VID header, in which case UBI will treat this PEB as
 	 * corrupted and will try to preserve it, and print scary warnings.
 	 */
 	addr = (loff_t)pnum * ubi->peb_size;
-	err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
-	if (!err) {
-		addr += ubi->vid_hdr_aloffset;
-		err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
-		if (!err)
-			return 0;
+	err = ubi_io_read_ec_hdr(ubi, pnum, &ec_hdr, 0);
+	if (err != UBI_IO_BAD_HDR_EBADMSG && err != UBI_IO_BAD_HDR &&
+	    err != UBI_IO_FF){
+		err1 = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
+		if(err1)
+			goto error;
 	}
 
-	/*
-	 * We failed to write to the media. This was observed with Spansion
-	 * S29GL512N NOR flash. Most probably the previously eraseblock erasure
-	 * was interrupted at a very inappropriate moment, so it became
-	 * unwritable. 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_EBADMSG || err1 == UBI_IO_BAD_HDR ||
-	    err1 == UBI_IO_FF) {
-		struct ubi_ec_hdr ec_hdr;
-
-		err1 = ubi_io_read_ec_hdr(ubi, pnum, &ec_hdr, 0);
-		if (err1 == UBI_IO_BAD_HDR_EBADMSG || err1 == UBI_IO_BAD_HDR ||
-		    err1 == UBI_IO_FF)
-			/*
-			 * Both VID and EC headers are corrupted, so we can
-			 * safely erase this PEB and not afraid that it will be
-			 * treated as a valid PEB in case of an unclean reboot.
-			 */
-			return 0;
+	err = ubi_io_read_vid_hdr(ubi, pnum, &vid_hdr, 0);
+	if (err != UBI_IO_BAD_HDR_EBADMSG && err != UBI_IO_BAD_HDR &&
+	    err != UBI_IO_FF){
+	    	addr += ubi->vid_hdr_aloffset;
+		err1 = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
+		if (err1)
+			goto error;	
 	}
+	return 0;
 
+error:
 	/*
-	 * The PEB contains a valid VID header, but we cannot invalidate it.
+	 * The PEB contains a valid VID or EC header, but we cannot invalidate it.
 	 * Supposedly the flash media or the driver is screwed up, so return an
 	 * error.
 	 */
-	ubi_err("cannot invalidate PEB %d, write returned %d read returned %d",
+	ubi_err("cannot invalidate PEB %d, read returned %d write returned %d",
 		pnum, err, err1);
 	ubi_dump_flash(ubi, pnum, 0, ubi->peb_size);
 	return -EIO;
---

I have tested this patch on Micron NOR flash, part number is£ºJS28F512M29EWHA.
If have any questions, please let me know. 
Thank you

Best Regards, 
Qi Wang ÍõÆð
ESG APAC AE 
Tel: 86-021-38997158
Mobile: 86-15201958202
Email: qiwang@micron.com
Address: No 601 Fasai Rd, Pudong, Shanghai, China, 200131
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/1] MTD: UBI: try to avoid program data to NOR flash after erasure interrupted

[Richard Weinberger]

On Thu, Nov 7, 2013 at 9:18 AM, Qi Wang 王起 (qiwang) <qiwang@micron.com> wrote:
> Hi:
> As we talked in mail before, please check my patch as below:
>
> From: Qi Wang <qiwang@micron.com>
>
> nor_erase_prepare() will be called before erase a NOR flash, it will program '0'
> into a block to mark this block. But program data into a erasure interrupted block
> can cause program timtout(several minutes at most) error, could impact other
> operation on NOR flash. So UBIFS can read this block first to avoid unneeded
> program operation.
>
> This patch try to put read operation at head of write operation in
> nor_erase_prepare(), read out the data.
> If the data is already corrupt, then no need to program any data into this block,
> just go to erase this block.
>
> Signed-off-by: Qi Wang <qiwang@qiwang@micron.com>

This email address is malformed.

> ---
> diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
> index bf79def..0a6343d 100644
> --- a/drivers/mtd/ubi/io.c
> +++ b/drivers/mtd/ubi/io.c
> @@ -499,6 +499,7 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum)
>         size_t written;
>         loff_t addr;
>         uint32_t data = 0;
> +       struct ubi_ec_hdr ec_hdr;
>         /*
>          * Note, we cannot generally define VID header buffers on stack,
>          * because of the way we deal with these buffers (see the header
> @@ -509,49 +510,38 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum)
>         struct ubi_vid_hdr vid_hdr;
>
>         /*
> +        * If VID or EC is valid, need to corrupt it before erase operation.
>          * It is important to first invalidate the EC header, and then the VID
>          * header. Otherwise a power cut may lead to valid EC header and
>          * invalid VID header, in which case UBI will treat this PEB as
>          * corrupted and will try to preserve it, and print scary warnings.
>          */
>         addr = (loff_t)pnum * ubi->peb_size;
> -       err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
> -       if (!err) {
> -               addr += ubi->vid_hdr_aloffset;
> -               err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
> -               if (!err)
> -                       return 0;
> +       err = ubi_io_read_ec_hdr(ubi, pnum, &ec_hdr, 0);
> +       if (err != UBI_IO_BAD_HDR_EBADMSG && err != UBI_IO_BAD_HDR &&
> +           err != UBI_IO_FF){
> +               err1 = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
> +               if(err1)
> +                       goto error;
>         }
>
> -       /*
> -        * We failed to write to the media. This was observed with Spansion
> -        * S29GL512N NOR flash. Most probably the previously eraseblock erasure
> -        * was interrupted at a very inappropriate moment, so it became
> -        * unwritable. 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_EBADMSG || err1 == UBI_IO_BAD_HDR ||
> -           err1 == UBI_IO_FF) {
> -               struct ubi_ec_hdr ec_hdr;
> -
> -               err1 = ubi_io_read_ec_hdr(ubi, pnum, &ec_hdr, 0);
> -               if (err1 == UBI_IO_BAD_HDR_EBADMSG || err1 == UBI_IO_BAD_HDR ||
> -                   err1 == UBI_IO_FF)
> -                       /*
> -                        * Both VID and EC headers are corrupted, so we can
> -                        * safely erase this PEB and not afraid that it will be
> -                        * treated as a valid PEB in case of an unclean reboot.
> -                        */
> -                       return 0;
> +       err = ubi_io_read_vid_hdr(ubi, pnum, &vid_hdr, 0);
> +       if (err != UBI_IO_BAD_HDR_EBADMSG && err != UBI_IO_BAD_HDR &&
> +           err != UBI_IO_FF){
> +               addr += ubi->vid_hdr_aloffset;
> +               err1 = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
> +               if (err1)
> +                       goto error;
>         }
> +       return 0;
>
> +error:
>         /*
> -        * The PEB contains a valid VID header, but we cannot invalidate it.
> +        * The PEB contains a valid VID or EC header, but we cannot invalidate it.
>          * Supposedly the flash media or the driver is screwed up, so return an
>          * error.
>          */
> -       ubi_err("cannot invalidate PEB %d, write returned %d read returned %d",
> +       ubi_err("cannot invalidate PEB %d, read returned %d write returned %d",
>                 pnum, err, err1);
>         ubi_dump_flash(ubi, pnum, 0, ubi->peb_size);
>         return -EIO;
> ---
>
> I have tested this patch on Micron NOR flash, part number is：JS28F512M29EWHA.

Please add this information to the commit message.

> If have any questions, please let me know.

>From UBIs point of view the patch seems OK to me, but it is malformed,
I was unable to apply it using git-am.
Did you use MS Outlook? :)
Please send it using git send-mail or any other sane mailer.

> Thank you
>
> Best Regards,
> Qi Wang 王起
> ESG APAC AE
> Tel: 86-021-38997158
> Mobile: 86-15201958202
> Email: qiwang@micron.com
> Address: No 601 Fasai Rd, Pudong, Shanghai, China, 200131



-- 
Thanks,
//richard

RE: [PATCH 1/1] MTD: UBI: try to avoid program data to NOR flash after erasure interrupted

[Qi Wang 王起 (qiwang)]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 5891 bytes --]

Hi Richard:
ok, I can correct my email address and re-send you again. Please check it. 
Thanks for your great help.


-----Original Message-----
From: Richard Weinberger [mailto:richard.weinberger@gmail.com] 
Sent: Tuesday, December 31, 2013 8:15 PM
To: Qi Wang ÍõÆð (qiwang)
Cc: dedekind1@gmail.com; linux-mtd@lists.infradead.org; Adrian Hunter; linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] MTD: UBI: try to avoid program data to NOR flash after erasure interrupted

On Thu, Nov 7, 2013 at 9:18 AM, Qi Wang ÍõÆð (qiwang) <qiwang@micron.com> wrote:
> Hi:
> As we talked in mail before, please check my patch as below:
>
> From: Qi Wang <qiwang@micron.com>
>
> nor_erase_prepare() will be called before erase a NOR flash, it will program '0'
> into a block to mark this block. But program data into a erasure interrupted block
> can cause program timtout(several minutes at most) error, could impact other
> operation on NOR flash. So UBIFS can read this block first to avoid unneeded
> program operation.
>
> This patch try to put read operation at head of write operation in
> nor_erase_prepare(), read out the data.
> If the data is already corrupt, then no need to program any data into this block,
> just go to erase this block.
>
> Signed-off-by: Qi Wang <qiwang@qiwang@micron.com>

This email address is malformed.

> ---
> diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
> index bf79def..0a6343d 100644
> --- a/drivers/mtd/ubi/io.c
> +++ b/drivers/mtd/ubi/io.c
> @@ -499,6 +499,7 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum)
>         size_t written;
>         loff_t addr;
>         uint32_t data = 0;
> +       struct ubi_ec_hdr ec_hdr;
>         /*
>          * Note, we cannot generally define VID header buffers on stack,
>          * because of the way we deal with these buffers (see the header
> @@ -509,49 +510,38 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum)
>         struct ubi_vid_hdr vid_hdr;
>
>         /*
> +        * If VID or EC is valid, need to corrupt it before erase operation.
>          * It is important to first invalidate the EC header, and then the VID
>          * header. Otherwise a power cut may lead to valid EC header and
>          * invalid VID header, in which case UBI will treat this PEB as
>          * corrupted and will try to preserve it, and print scary warnings.
>          */
>         addr = (loff_t)pnum * ubi->peb_size;
> -       err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
> -       if (!err) {
> -               addr += ubi->vid_hdr_aloffset;
> -               err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
> -               if (!err)
> -                       return 0;
> +       err = ubi_io_read_ec_hdr(ubi, pnum, &ec_hdr, 0);
> +       if (err != UBI_IO_BAD_HDR_EBADMSG && err != UBI_IO_BAD_HDR &&
> +           err != UBI_IO_FF){
> +               err1 = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
> +               if(err1)
> +                       goto error;
>         }
>
> -       /*
> -        * We failed to write to the media. This was observed with Spansion
> -        * S29GL512N NOR flash. Most probably the previously eraseblock erasure
> -        * was interrupted at a very inappropriate moment, so it became
> -        * unwritable. 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_EBADMSG || err1 == UBI_IO_BAD_HDR ||
> -           err1 == UBI_IO_FF) {
> -               struct ubi_ec_hdr ec_hdr;
> -
> -               err1 = ubi_io_read_ec_hdr(ubi, pnum, &ec_hdr, 0);
> -               if (err1 == UBI_IO_BAD_HDR_EBADMSG || err1 == UBI_IO_BAD_HDR ||
> -                   err1 == UBI_IO_FF)
> -                       /*
> -                        * Both VID and EC headers are corrupted, so we can
> -                        * safely erase this PEB and not afraid that it will be
> -                        * treated as a valid PEB in case of an unclean reboot.
> -                        */
> -                       return 0;
> +       err = ubi_io_read_vid_hdr(ubi, pnum, &vid_hdr, 0);
> +       if (err != UBI_IO_BAD_HDR_EBADMSG && err != UBI_IO_BAD_HDR &&
> +           err != UBI_IO_FF){
> +               addr += ubi->vid_hdr_aloffset;
> +               err1 = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data);
> +               if (err1)
> +                       goto error;
>         }
> +       return 0;
>
> +error:
>         /*
> -        * The PEB contains a valid VID header, but we cannot invalidate it.
> +        * The PEB contains a valid VID or EC header, but we cannot invalidate it.
>          * Supposedly the flash media or the driver is screwed up, so return an
>          * error.
>          */
> -       ubi_err("cannot invalidate PEB %d, write returned %d read returned %d",
> +       ubi_err("cannot invalidate PEB %d, read returned %d write returned %d",
>                 pnum, err, err1);
>         ubi_dump_flash(ubi, pnum, 0, ubi->peb_size);
>         return -EIO;
> ---
>
> I have tested this patch on Micron NOR flash, part number is£ºJS28F512M29EWHA.

Please add this information to the commit message.

> If have any questions, please let me know.

>From UBIs point of view the patch seems OK to me, but it is malformed,
I was unable to apply it using git-am.
Did you use MS Outlook? :)
Please send it using git send-mail or any other sane mailer.

> Thank you
>
> Best Regards,
> Qi Wang ÍõÆð
> ESG APAC AE
> Tel: 86-021-38997158
> Mobile: 86-15201958202
> Email: qiwang@micron.com
> Address: No 601 Fasai Rd, Pudong, Shanghai, China, 200131



-- 
Thanks,
//richard
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥