[U-Boot] [RFC PATCH 0/1] bug writing to NAND 1 byte less than page size

[U-Boot] [RFC PATCH 0/1] bug writing to NAND 1 byte less than page size

[Hector Palacios]

Hi,

I found this bug on v2015.04 but I guess it happens also in v2016.07
as I believe it has been there since 2007.
It is so strange that nobody caught this before that I fear I'm doing
something wrong or not taking something into account on the suggested
fix, so I'd like to know if others can reproduce it and if the fix looks
correct.

I reproduced it on an i.MX6UL platform and on an i.MX28 platform but
it is a bug on the NAND core driver, so it should be reproducible with
any architecture. In fact, it is reproducible with any size, as long
as it is a multiple of the page size, less one byte.

Thanks in advance for your comments.

Hector Palacios (1):
  mtd: nand: fix bug writing 1 byte less than page size

 drivers/mtd/nand/nand_base.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

[U-Boot] [RFC PATCH 1/1] mtd: nand: fix bug writing 1 byte less than page size

[Hector Palacios]

nand_do_write_ops() determines if it is writing a partial page with the
formula:
	part_pagewr = (column || writelen < (mtd->writesize - 1))

When 'writelen' is exactly 1 byte less than the NAND page size the formula
equates to zero, so the code doesn't process it as a partial write, although
it should.
As a consequence the function remains in the while(1) loop with 'writelen'
becoming 0xffffffff and iterating until the watchdog timeout triggers.

To reproduce the issue on a NAND with 2K page (0x800):
	=> nand erase.part <partition>
	=> nand write $loadaddr <partition> 7ff

Signed-off-by: Hector Palacios <hector.palacios@digi.com>
---
 drivers/mtd/nand/nand_base.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 689716753ae6..c8be74849e56 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -877,7 +877,7 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
 
  	u32 timer = (CONFIG_SYS_HZ * timeo) / 1000;
  	u32 time_start;
- 
+
  	time_start = get_timer(0);
  	while (get_timer(time_start) < timer) {
 		if (chip->dev_ready) {
@@ -2411,7 +2411,7 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to,
 		int cached = writelen > bytes && page != blockmask;
 		uint8_t *wbuf = buf;
 		int use_bufpoi;
-		int part_pagewr = (column || writelen < (mtd->writesize - 1));
+		int part_pagewr = (column || writelen < mtd->writesize);
 
 		if (part_pagewr)
 			use_bufpoi = 1;

[U-Boot] [RFC PATCH 1/1] mtd: nand: fix bug writing 1 byte less than page size

[Scott Wood]

On Fri, 2016-07-15 at 09:45 +0200, Hector Palacios wrote:
> nand_do_write_ops() determines if it is writing a partial page with the
> formula:
> 	part_pagewr = (column || writelen < (mtd->writesize - 1))
> 
> When 'writelen' is exactly 1 byte less than the NAND page size the formula
> equates to zero, so the code doesn't process it as a partial write, although
> it should.
> As a consequence the function remains in the while(1) loop with 'writelen'
> becoming 0xffffffff and iterating until the watchdog timeout triggers.
> 
> To reproduce the issue on a NAND with 2K page (0x800):
> 	=> nand erase.part <partition>
> 	=> nand write $loadaddr <partition> 7ff
> 
> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> ---
> ?drivers/mtd/nand/nand_base.c | 4 ++--
> ?1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 689716753ae6..c8be74849e56 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -877,7 +877,7 @@ static int nand_wait(struct mtd_info *mtd, struct
> nand_chip *chip)
> ?
> ??	u32 timer = (CONFIG_SYS_HZ * timeo) / 1000;
> ??	u32 time_start;
> -?
> +
> ??	time_start = get_timer(0);
> ??	while (get_timer(time_start) < timer) {
> ?		if (chip->dev_ready) {

This change is unrelated.

> @@ -2411,7 +2411,7 @@ static int nand_do_write_ops(struct mtd_info *mtd,
> loff_t to,
> ?		int cached = writelen > bytes && page != blockmask;
> ?		uint8_t *wbuf = buf;
> ?		int use_bufpoi;
> -		int part_pagewr = (column || writelen < (mtd->writesize -
> 1));
> +		int part_pagewr = (column || writelen < mtd->writesize);
> ?
> ?		if (part_pagewr)
> ?			use_bufpoi = 1;
> 

Could you send a non-RFC patch? ?This also needs to get fixed in Linux.

-Scott

[U-Boot] [RFC PATCH 1/1] mtd: nand: fix bug writing 1 byte less than page size

[Hector Palacios]

On 07/15/2016 08:49 PM, Scott Wood wrote:
> On Fri, 2016-07-15 at 09:45 +0200, Hector Palacios wrote:
>> nand_do_write_ops() determines if it is writing a partial page with the
>> formula:
>> 	part_pagewr = (column || writelen < (mtd->writesize - 1))
>>
>> When 'writelen' is exactly 1 byte less than the NAND page size the formula
>> equates to zero, so the code doesn't process it as a partial write, although
>> it should.
>> As a consequence the function remains in the while(1) loop with 'writelen'
>> becoming 0xffffffff and iterating until the watchdog timeout triggers.
>>
>> To reproduce the issue on a NAND with 2K page (0x800):
>> 	=> nand erase.part <partition>
>> 	=> nand write $loadaddr <partition> 7ff
>>
>> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
>> ---
>>  drivers/mtd/nand/nand_base.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> index 689716753ae6..c8be74849e56 100644
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -877,7 +877,7 @@ static int nand_wait(struct mtd_info *mtd, struct
>> nand_chip *chip)
>>  
>>   	u32 timer = (CONFIG_SYS_HZ * timeo) / 1000;
>>   	u32 time_start;
>> - 
>> +
>>   	time_start = get_timer(0);
>>   	while (get_timer(time_start) < timer) {
>>  		if (chip->dev_ready) {
> 
> This change is unrelated.

Ok. Removed.

>> @@ -2411,7 +2411,7 @@ static int nand_do_write_ops(struct mtd_info *mtd,
>> loff_t to,
>>  		int cached = writelen > bytes && page != blockmask;
>>  		uint8_t *wbuf = buf;
>>  		int use_bufpoi;
>> -		int part_pagewr = (column || writelen < (mtd->writesize -
>> 1));
>> +		int part_pagewr = (column || writelen < mtd->writesize);
>>  
>>  		if (part_pagewr)
>>  			use_bufpoi = 1;
>>
> 
> Could you send a non-RFC patch?  This also needs to get fixed in Linux.

Done. I will prepare a patch for Linux, but do you know how to reproduce it in Linux?
'nandwrite' checks that the file size is a multiple of the page size or else it will
ask you to force padding, and 'dd' over the mtdblock device is also rounding up the
write size to a page size multiple. Maybe that's the reason why it went undetected for
so long.

--
Hector Palacios