Re: [PATCH][v3] mtd: rawnand: fsl_ifc: Fix eccstat array overflow for IFC ver >= 2.0.0

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

On Tue, 20 Mar 2018 21:37:39 +0530
Jagdish Gediya <jagdish.gediya@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. This is causing eccstat
> array to over flow.
> 
> So, replace eccstat array with u32 variable to make it fail-safe and
> independent of number of ECC status registers or SRAM size.
> 
> Fixes: bccb06c353af ("mtd: nand: ifc: update bufnum mask for ver >= 2.0.0")
> Cc: stable@vger.kernel.org # 3.18+
> Signed-off-by: Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com>
> Signed-off-by: Jagdish Gediya <jagdish.gediya@nxp.com>
> ---
> Changes for v2: Incorporated comments from Miquel Raynal and Boris Brezillon 
> 	- Updated patch subject
> 	- Remove usage of eccstat array
> 	- Added Cc: stable@vger.kernel.org 
> 
> Changes for v3: Incorporated comments from Boris Brezillon
> 	- Added fixes tag
> 
>  drivers/mtd/nand/fsl_ifc_nand.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
> index 4872a7b..9390cbd 100644
> --- a/drivers/mtd/nand/fsl_ifc_nand.c
> +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> @@ -173,14 +173,9 @@ static void set_addr(struct mtd_info *mtd, int column, int page_addr, int oob)
>  
>  /* returns nonzero if entire page is blank */
>  static int check_read_ecc(struct mtd_info *mtd, struct fsl_ifc_ctrl *ctrl,
> -			  u32 *eccstat, unsigned int bufnum)
> +			  u32 eccstat, unsigned int bufnum)
>  {
> -	u32 reg = eccstat[bufnum / 4];
> -	int errors;
> -
> -	errors = (reg >> ((3 - bufnum % 4) * 8)) & 15;
> -
> -	return errors;
> +	return  (eccstat >> ((3 - bufnum % 4) * 8)) & 15;
>  }
>  
>  /*
> @@ -193,7 +188,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;
>  	int i;
>  
>  	/* set the chip select for NAND Transaction */
> @@ -237,10 +232,18 @@ 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++)
> -			eccstat[i] = ifc_in32(&eccstat_regs[i]);
> +		eccstat = 0;

I guess you do that because gcc complains about an uninitialized var.
How about initializing eccstat correctly here:

		eccstat = ifc_in32(&eccstat_regs[sector / 4]);

>  
>  		for (i = sector; i <= sector_end; i++) {
> +

Remove this empty line.

> +			/*
> +			 * sector is not necessarily aligned on 4 and
> +			 * we need to read the eccstat in this case,
> +			 * hence the sector test.
> +			 */
> +			if ((i % 4 == 0) || (sector == sector_end))

I still think the sector == sector_end test is less clear than an i ==
sector (which BTW should be named sector_start to clarify things).

Anyway, if you initialize eccstat as suggested above, here you should
have:

			if (i != sector && !(i % 4))
				eccstat = ifc_in32(&eccstat_regs[i / 4]);

> +				eccstat = ifc_in32(&eccstat_regs[i / 4]);
> +
>  			errors = check_read_ecc(mtd, ctrl, eccstat, i);
>  
>  			if (errors == 15) {



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