[U-Boot] Possible bug in NAND driver

[U-Boot] Possible bug in NAND driver

[Paulraj, Sandeep]

If we refer to the following code snippet from nand_util.c

rval = nand_read (nand, offset, &read_length, p_buffer);

                if (rval != 0) {

                          printf ("NAND read from offset %llx failed %d\n",

                                  offset, rval);

                         *length -= left_to_read;

                          return rval;

                  }


The above code will return failure even after ECC errors are corrected.

This is because of the following line of code in nand_base.c

return  mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN : 0;

This is in the nand_do_read_ops in nand_bsae.c


I see that changing

if (rval != 0)


to

if (rval != 0 && rval != -EUCLEAN )


solves the problem.

I can submit a patch if required.


Thanks,
Sandeep

[U-Boot] Possible bug in NAND driver

[Valeriy Glushkov]

You are right, this is a bug.

I've already fixed it in our code tree some monthes ago but forgotten to 
send the patch to the list.

Best regards,
Valeriy Glushkov

----- Original Message ----- 
From: "Paulraj, Sandeep" <s-paulraj@ti.com>
To: <u-boot@lists.denx.de>
Sent: 13 ???? 2009 ?. 22:34
Subject: [U-Boot] Possible bug in NAND driver


>
> If we refer to the following code snippet from nand_util.c
>
> rval = nand_read (nand, offset, &read_length, p_buffer);
>
>                if (rval != 0) {
>
>                          printf ("NAND read from offset %llx failed %d\n",
>
>                                  offset, rval);
>
>                         *length -= left_to_read;
>
>                          return rval;
>
>                  }
>
>
> The above code will return failure even after ECC errors are corrected.
>
> This is because of the following line of code in nand_base.c
>
> return  mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN : 0;
>
> This is in the nand_do_read_ops in nand_bsae.c
>
>
> I see that changing
>
> if (rval != 0)
>
>
> to
>
> if (rval != 0 && rval != -EUCLEAN )
>
>
> solves the problem.
>
> I can submit a patch if required.
>
>
> Thanks,
> Sandeep
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot 

[U-Boot] [PATCH] nand: fixed failed reads on corrected ECC errors in nand_util.c

[Valeriy Glushkov]

Signed-off-by: Valeriy Glushkov <gvv@lstec.com>
Signed-off-by: Paulraj, Sandeep <s-paulraj@ti.com>
---
 drivers/mtd/nand/nand_util.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/nand_util.c b/drivers/mtd/nand/nand_util.c
index fc16282..694ead6 100644
--- a/drivers/mtd/nand/nand_util.c
+++ b/drivers/mtd/nand/nand_util.c
@@ -567,10 +567,10 @@ int nand_read_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
 
 	if (len_incl_bad == *length) {
 		rval = nand_read (nand, offset, length, buffer);
-		if (rval != 0)
-			printf ("NAND read from offset %llx failed %d\n",
-				offset, rval);
-
+		if (!rval || rval == -EUCLEAN)
+			return 0;
+		printf ("NAND read from offset %llx failed %d\n",
+			offset, rval);
 		return rval;
 	}
 
@@ -591,7 +591,7 @@ int nand_read_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
 			read_length = nand->erasesize - block_offset;
 
 		rval = nand_read (nand, offset, &read_length, p_buffer);
-		if (rval != 0) {
+		if (rval && rval != -EUCLEAN) {
 			printf ("NAND read from offset %llx failed %d\n",
 				offset, rval);
 			*length -= left_to_read;
-- 
1.5.2.5

[U-Boot] Possible bug in NAND driver

[Scott Wood]

On Mon, Jul 13, 2009 at 02:34:42PM -0500, Paulraj, Sandeep wrote:
> I see that changing
> 
> if (rval != 0)
> 
> 
> to
> 
> if (rval != 0 && rval != -EUCLEAN )
> 
> 
> solves the problem.
> 
> I can submit a patch if required.

Yes, please submit a patch.  Thanks!

-Scott

[U-Boot] [PATCH] nand: fixed failed reads on corrected ECC errors in nand_util.c

[Scott Wood]

On Tue, Jul 14, 2009 at 01:51:10PM +0300, Valeriy Glushkov wrote:
> 
> Signed-off-by: Valeriy Glushkov <gvv@lstec.com>
> Signed-off-by: Paulraj, Sandeep <s-paulraj@ti.com>
> ---

Applied to u-boot-nand-flash.

>  drivers/mtd/nand/nand_util.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_util.c b/drivers/mtd/nand/nand_util.c
> index fc16282..694ead6 100644
> --- a/drivers/mtd/nand/nand_util.c
> +++ b/drivers/mtd/nand/nand_util.c
> @@ -567,10 +567,10 @@ int nand_read_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
>  
>  	if (len_incl_bad == *length) {
>  		rval = nand_read (nand, offset, length, buffer);
> -		if (rval != 0)
> -			printf ("NAND read from offset %llx failed %d\n",
> -				offset, rval);
> -
> +		if (!rval || rval == -EUCLEAN)
> +			return 0;
> +		printf ("NAND read from offset %llx failed %d\n",
> +			offset, rval);

Out of curiosity, why invert the logic from if (error) print; return to if
(!error) return; print; return?

-Scott

[U-Boot] [PATCH] nand: fixed failed reads on corrected ECC errors in nand_util.c

[Valeriy Glushkov]

Hi Scott,
>>  if (len_incl_bad == *length) {
>>  rval = nand_read (nand, offset, length, buffer);
>> - if (rval != 0)
>> - printf ("NAND read from offset %llx failed %d\n",
>> - offset, rval);
>> -
>> + if (!rval || rval == -EUCLEAN)
>> + return 0;
>> + printf ("NAND read from offset %llx failed %d\n",
>> + offset, rval);
> 
> Out of curiosity, why invert the logic from if (error) print; return to if
> (!error) return; print; return?

Because it looks a bit better for me than 2 other versions.
And saves a line. :)
-------
if (!rval || rval == -EUCLEAN)
  return 0;
printf ("NAND read from offset %llx failed %d\n",
  offset, rval);
return rval;
--------
if (rval && rval != -EUCLEAN) {
 printf ("NAND read from offset %llx failed %d\n",
  offset, rval);
 return rval;
}
return 0;
-------

if (rval && rval != -EUCLEAN)
 printf ("NAND read from offset %llx failed %d\n",
  offset, rval);
else
  rval = 0;
return rval;
--------

Best regards,
Valeriy Glushkov