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

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

[Prabhakar Kushwaha]

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>
---
 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;
+			}
 			eccstat[i] = ifc_in32(&eccstat_regs[i]);
+		}
 
 		for (i = sector; i <= sector_end; i++) {
 			errors = check_read_ecc(mtd, ctrl, eccstat, i);
-- 
2.14.1

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

[Miquel Raynal]

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.

> ---
>  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;".

>  			eccstat[i] = ifc_in32(&eccstat_regs[i]);
> +		}
>  
>  		for (i = sector; i <= sector_end; i++) {
>  			errors = check_read_ecc(mtd, ctrl, eccstat, i);

Thanks,
Miquèl

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

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

[Boris Brezillon]

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