Re: [RESEND PATCH v3 2/5] mtd: nand: return consistent error codes in ecc.correct() implementations

[Boris Brezillon <boris.brezillon@free-electrons.com>]

Hi,

On Mon, 21 Sep 2015 16:10:29 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> + a few others for review/test, since this touches several drivers
> 
> On Thu, Sep 03, 2015 at 06:03:39PM +0200, Boris Brezillon wrote:
> > The error code returned by the ecc.correct() are not consistent over the
> > all implementations.
> > 
> > Document the expected behavior in include/linux/mtd/nand.h and fix
> > offending implementations.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> 
> The patch all looks good to me. A note below:
> 
> > ---
> >  drivers/mtd/nand/atmel_nand.c   |  2 +-
> >  drivers/mtd/nand/bf5xx_nand.c   | 20 ++++++++++++++------
> >  drivers/mtd/nand/davinci_nand.c |  6 +++---
> >  drivers/mtd/nand/jz4740_nand.c  |  4 ++--
> >  drivers/mtd/nand/mxc_nand.c     |  4 ++--
> >  drivers/mtd/nand/nand_bch.c     |  2 +-
> >  drivers/mtd/nand/nand_ecc.c     |  2 +-
> >  drivers/mtd/nand/omap2.c        |  6 +++---
> >  drivers/mtd/nand/r852.c         |  4 ++--
> >  include/linux/mtd/nand.h        |  8 +++++++-
> >  include/linux/mtd/nand_bch.h    |  2 +-
> >  11 files changed, 37 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> > index 46010bd..dc7b399 100644
> > --- a/drivers/mtd/nand/atmel_nand.c
> > +++ b/drivers/mtd/nand/atmel_nand.c
> > @@ -1443,7 +1443,7 @@ static int atmel_nand_correct(struct mtd_info *mtd, u_char *dat,
> >  		 * We can't correct so many errors */
> >  		dev_dbg(host->dev, "atmel_nand : multiple errors detected."
> >  				" Unable to correct.\n");
> > -		return -EIO;
> > +		return -EBADMSG;
> >  	}
> >  
> >  	/* if there's a single bit error : we can correct it */
> > diff --git a/drivers/mtd/nand/bf5xx_nand.c b/drivers/mtd/nand/bf5xx_nand.c
> > index 4d8d4ba..9c39056 100644
> > --- a/drivers/mtd/nand/bf5xx_nand.c
> > +++ b/drivers/mtd/nand/bf5xx_nand.c
> > @@ -252,7 +252,7 @@ static int bf5xx_nand_correct_data_256(struct mtd_info *mtd, u_char *dat,
> >  	 */
> >  	if (hweight32(syndrome[0]) == 1) {
> >  		dev_err(info->device, "ECC data was incorrect!\n");
> > -		return 1;
> > +		return -EBADMSG;
> >  	}
> >  
> >  	syndrome[1] = (calced & 0x7FF) ^ (stored & 0x7FF);
> > @@ -285,7 +285,7 @@ static int bf5xx_nand_correct_data_256(struct mtd_info *mtd, u_char *dat,
> >  		data = data ^ (0x1 << failing_bit);
> >  		*(dat + failing_byte) = data;
> >  
> > -		return 0;
> > +		return 1;
> >  	}
> >  
> >  	/*
> > @@ -298,26 +298,34 @@ static int bf5xx_nand_correct_data_256(struct mtd_info *mtd, u_char *dat,
> >  	dev_err(info->device,
> >  		"Please discard data, mark bad block\n");
> >  
> > -	return 1;
> > +	return -EBADMSG;
> >  }
> 
> The above changes to bf5xx_nand all look correct, but they are actually
> semantic changes that could be considered a bugfix (correctable errors
> were going unreported, and uncorrectable errors were being reported as
> correctable). I'm not sure this really deserves special treatment (e.g.,
> -stable) since apparently no one cared. Perhaps this platform is dead?
> 
> Anyway, I'll probably add a comment to the commit description if I take
> this patch.

No problem, feel free to add extra explanations in the commit message.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com