linux-kernel - RE: [PATCH v3 1/3] mtd: nand: omap2: Update nerrors using ecc.strength

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <518397C60809E147AF5323E0420B992E3EA1139B@DBDE01.ent.ti.com>
Date:	Wed, 5 Dec 2012 12:43:59 +0000
From:	"Philip, Avinash" <avinashphilip@...com>
To:	"Nori, Sekhar" <nsekhar@...com>
CC:	"dwmw2@...radead.org" <dwmw2@...radead.org>,
	"artem.bityutskiy@...ux.intel.com" <artem.bityutskiy@...ux.intel.com>,
	"Mohammed, Afzal" <afzal@...com>,
	"tony@...mide.com" <tony@...mide.com>,
	"broonie@...nsource.wolfsonmicro.com" 
	<broonie@...nsource.wolfsonmicro.com>,
	"rmk+kernel@....linux.org.uk" <rmk+kernel@....linux.org.uk>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree-discuss@...ts.ozlabs.org" 
	<devicetree-discuss@...ts.ozlabs.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"Hebbar, Gururaja" <gururaja.hebbar@...com>,
	"ivan.djelic@...rot.com" <ivan.djelic@...rot.com>
Subject: RE: [PATCH v3 1/3] mtd: nand: omap2: Update nerrors using
 ecc.strength

On Wed, Dec 05, 2012 at 17:33:37, Nori, Sekhar wrote:
> Hi Avinash,
> 
> On 11/29/2012 5:16 PM, Philip, Avinash wrote:
> > Update number of errors using nand ecc strength.
> > Also add macro definitions BCH8_ERROR_MAX & BCH4_ERROR_MAX
> 
> Can you please describe why the original method of setting nerrors was
> incorrect? Was it causing any issues in any particular configuration?

It affects the reusability of the code. For example BCH8 with AM335x RBL 
compatibility requires  14 bytes instead of 13 byte. So setting nerrors
with
	nerrors = (info->nand.ecc.bytes == 13) ? 8 : 4;
will break am335x RBL compatibility.

> Mentioning this will help maintainers assign priority to your patch. If
> this is a real bug affecting existing platforms, then there is a chance
> this patch can get into v3.7 (or at least into v3.8-rc1).
> 
> Like Peter who commented on this before, I am not a fan of using macros
> for self-describing constants - especially when you end up using the
> same numbers inside the macro name itself. No need to resend any thing
> just for this, you can wait to see if the maintainers are okay with it.

Yes I agree. But the same constants are used in multiple places here.
So usage of macro helps in readability & code debugging.

Thanks
Avinash
 
> 
> Thanks,
> Sekhar
> 
> > 
> > Signed-off-by: Philip, Avinash <avinashphilip@...com>
> > ---
> > :100644 100644 359293e... 7e61dac... M	drivers/mtd/nand/omap2.c
> >  drivers/mtd/nand/omap2.c |   12 ++++++++----
> >  1 files changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> > index 359293e..7e61dac 100644
> > --- a/drivers/mtd/nand/omap2.c
> > +++ b/drivers/mtd/nand/omap2.c
> > @@ -118,6 +118,9 @@
> >  
> >  #define OMAP24XX_DMA_GPMC		4
> >  
> > +#define BCH8_MAX_ERROR		8	/* upto 8 bit correctable */
> > +#define BCH4_MAX_ERROR		4	/* upto 4 bit correctable */
> > +
> >  /* oob info generated runtime depending on ecc algorithm and layout selected */
> >  static struct nand_ecclayout omap_oobinfo;
> >  /* Define some generic bad / good block scan pattern which are used
> > @@ -1042,7 +1045,7 @@ static void omap3_enable_hwecc_bch(struct mtd_info *mtd, int mode)
> >  	struct nand_chip *chip = mtd->priv;
> >  	u32 val;
> >  
> > -	nerrors = (info->nand.ecc.bytes == 13) ? 8 : 4;
> > +	nerrors = info->nand.ecc.strength;
> >  	dev_width = (chip->options & NAND_BUSWIDTH_16) ? 1 : 0;
> >  	nsectors = 1;
> >  	/*
> > @@ -1219,13 +1222,14 @@ static int omap3_init_bch(struct mtd_info *mtd, int ecc_opt)
> >  	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
> >  						   mtd);
> >  #ifdef CONFIG_MTD_NAND_OMAP_BCH8
> > -	const int hw_errors = 8;
> > +	const int hw_errors = BCH8_MAX_ERROR;
> >  #else
> > -	const int hw_errors = 4;
> > +	const int hw_errors = BCH4_MAX_ERROR;
> >  #endif
> >  	info->bch = NULL;
> >  
> > -	max_errors = (ecc_opt == OMAP_ECC_BCH8_CODE_HW) ? 8 : 4;
> > +	max_errors = (ecc_opt == OMAP_ECC_BCH8_CODE_HW) ?
> > +		BCH8_MAX_ERROR : BCH4_MAX_ERROR;
> >  	if (max_errors != hw_errors) {
> >  		pr_err("cannot configure %d-bit BCH ecc, only %d-bit supported",
> >  		       max_errors, hw_errors);
> > 
>