Re: [PATCH] driver: mtd: ifc: increase eccstat array size for ver >= 2.0.0

[Boris Brezillon <boris.brezillon@bootlin.com>]

On Wed, 7 Mar 2018 10:46:34 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Prabhakar,
> 
> On Wed,  7 Mar 2018 14:46:56 +0530, Prabhakar Kushwaha
> <prabhakar.kushwaha@nxp.com> wrote:
> 
> > Number of ECC status registers i.e. (ECCSTATx) has been increased
> > in IFC version 2.0.0 due to increase in SRAM size.
> > 
> > Fix the resulting eccstat array overflow.
> > 
> > Signed-off-by: Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com>  
> 
> Prefix should be "mtd: rawnand: fsl_ifc: ". It used to be "nand"
> instead of "rawnand" but "raw NAND" drivers are moved into a separate
> subfolder to make clear separations between families, and the prefix
> should follow.

And you miss the Fixes and Cc-stable tags.

> 
> > ---
> >  drivers/mtd/nand/fsl_ifc_nand.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
> > index 4872a7ba6503..612f0990fa90 100644
> > --- a/drivers/mtd/nand/fsl_ifc_nand.c
> > +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> > @@ -193,7 +193,7 @@ static void fsl_ifc_run_command(struct mtd_info *mtd)
> >  	struct fsl_ifc_ctrl *ctrl = priv->ctrl;
> >  	struct fsl_ifc_nand_ctrl *nctrl = ifc_nand_ctrl;
> >  	struct fsl_ifc_runtime __iomem *ifc = ctrl->rregs;
> > -	u32 eccstat[4];
> > +	u32 eccstat[8];
> >  	int i;
> >  
> >  	/* set the chip select for NAND Transaction */
> > @@ -237,8 +237,14 @@ static void fsl_ifc_run_command(struct mtd_info *mtd)
> >  		else
> >  			eccstat_regs = ifc->ifc_nand.v1_nand_eccstat;
> >  
> > -		for (i = sector / 4; i <= sector_end / 4; i++)
> > +		for (i = sector / 4; i <= sector_end / 4; i++) {
> > +			if (i >= ARRAY_SIZE(eccstat)) {
> > +				dev_err(priv->dev, "%s: eccstat small for %d\n",
> > +					__func__, i);
> > +				return;
> > +			}  
> 
> I suppose this check is here to prevent possible silent overflows
> due to further enlargements of the number of ECC status registers, but
> I don't think this is the right place to do so.
> 
> Actually, the only function in which this array is used is
> check_read_ecc() and it does nothing else than looking at one register
> in the array at a time. I am pretty sure this function could be
> reworked a bit to get rid of the array and use a single "u32 eccstat;".

I totally agree with that.


-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com