linux-mtd.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH][v4] mtd: rawnand: fsl_ifc: Fix eccstat array overflow for IFC ver >= 2.0.0
@ 2018-03-21  0:21 Jagdish Gediya
  2018-03-21  8:55 ` Boris Brezillon
  0 siblings, 1 reply; 3+ messages in thread
From: Jagdish Gediya @ 2018-03-21  0:21 UTC (permalink / raw)
  To: linux-mtd
  Cc: boris.brezillon, computersforpeace, oss, leoyang.li,
	Jagdish Gediya, stable, 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. 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

Changes for v4: Incorporated comments from Boris Brezillon

 drivers/mtd/nand/fsl_ifc_nand.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
index 4872a7b..9a01309 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 */
@@ -228,8 +223,8 @@ static void fsl_ifc_run_command(struct mtd_info *mtd)
 	if (nctrl->eccread) {
 		int errors;
 		int bufnum = nctrl->page & priv->bufnum_mask;
-		int sector = bufnum * chip->ecc.steps;
-		int sector_end = sector + chip->ecc.steps - 1;
+		int sector_start = bufnum * chip->ecc.steps;
+		int sector_end = sector_start + chip->ecc.steps - 1;
 		__be32 *eccstat_regs;
 
 		if (ctrl->version >= FSL_IFC_VERSION_2_0_0)
@@ -237,10 +232,12 @@ 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 = ifc_in32(&eccstat_regs[sector_start / 4]);
+
+		for (i = sector_start; i <= sector_end; i++) {
+			if (!(i % 4))
+				eccstat = ifc_in32(&eccstat_regs[i / 4]);
 
-		for (i = sector; i <= sector_end; i++) {
 			errors = check_read_ecc(mtd, ctrl, eccstat, i);
 
 			if (errors == 15) {
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH][v4] mtd: rawnand: fsl_ifc: Fix eccstat array overflow for IFC ver >= 2.0.0
  2018-03-21  0:21 [PATCH][v4] mtd: rawnand: fsl_ifc: Fix eccstat array overflow for IFC ver >= 2.0.0 Jagdish Gediya
@ 2018-03-21  8:55 ` Boris Brezillon
  2018-03-23  7:52   ` Boris Brezillon
  0 siblings, 1 reply; 3+ messages in thread
From: Boris Brezillon @ 2018-03-21  8:55 UTC (permalink / raw)
  To: Jagdish Gediya
  Cc: linux-mtd, boris.brezillon, computersforpeace, oss, leoyang.li,
	stable, Prabhakar Kushwaha

On Wed, 21 Mar 2018 05:51:46 +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
> 
> Changes for v4: Incorporated comments from Boris Brezillon
> 
>  drivers/mtd/nand/fsl_ifc_nand.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
> index 4872a7b..9a01309 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 */
> @@ -228,8 +223,8 @@ static void fsl_ifc_run_command(struct mtd_info *mtd)
>  	if (nctrl->eccread) {
>  		int errors;
>  		int bufnum = nctrl->page & priv->bufnum_mask;
> -		int sector = bufnum * chip->ecc.steps;
> -		int sector_end = sector + chip->ecc.steps - 1;
> +		int sector_start = bufnum * chip->ecc.steps;
> +		int sector_end = sector_start + chip->ecc.steps - 1;
>  		__be32 *eccstat_regs;
>  
>  		if (ctrl->version >= FSL_IFC_VERSION_2_0_0)
> @@ -237,10 +232,12 @@ 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 = ifc_in32(&eccstat_regs[sector_start / 4]);
> +
> +		for (i = sector_start; i <= sector_end; i++) {
> +			if (!(i % 4))
> +				eccstat = ifc_in32(&eccstat_regs[i / 4]);

So now you're reading eccstat_regs[sector_start / 4] twice if
sector_start is aligned on 4. Why don't you want the test I proposed
in my last review?

			if (i != sector_start && !(i % 4))

>  
> -		for (i = sector; i <= sector_end; i++) {
>  			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

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH][v4] mtd: rawnand: fsl_ifc: Fix eccstat array overflow for IFC ver >= 2.0.0
  2018-03-21  8:55 ` Boris Brezillon
@ 2018-03-23  7:52   ` Boris Brezillon
  0 siblings, 0 replies; 3+ messages in thread
From: Boris Brezillon @ 2018-03-23  7:52 UTC (permalink / raw)
  To: Jagdish Gediya
  Cc: boris.brezillon, stable, leoyang.li, oss, Prabhakar Kushwaha,
	linux-mtd, computersforpeace

On Wed, 21 Mar 2018 09:55:35 +0100
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> On Wed, 21 Mar 2018 05:51:46 +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
> > 
> > Changes for v4: Incorporated comments from Boris Brezillon
> > 
> >  drivers/mtd/nand/fsl_ifc_nand.c | 23 ++++++++++-------------
> >  1 file changed, 10 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
> > index 4872a7b..9a01309 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 */
> > @@ -228,8 +223,8 @@ static void fsl_ifc_run_command(struct mtd_info *mtd)
> >  	if (nctrl->eccread) {
> >  		int errors;
> >  		int bufnum = nctrl->page & priv->bufnum_mask;
> > -		int sector = bufnum * chip->ecc.steps;
> > -		int sector_end = sector + chip->ecc.steps - 1;
> > +		int sector_start = bufnum * chip->ecc.steps;
> > +		int sector_end = sector_start + chip->ecc.steps - 1;
> >  		__be32 *eccstat_regs;
> >  
> >  		if (ctrl->version >= FSL_IFC_VERSION_2_0_0)
> > @@ -237,10 +232,12 @@ 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 = ifc_in32(&eccstat_regs[sector_start / 4]);
> > +
> > +		for (i = sector_start; i <= sector_end; i++) {
> > +			if (!(i % 4))
> > +				eccstat = ifc_in32(&eccstat_regs[i / 4]);  
> 
> So now you're reading eccstat_regs[sector_start / 4] twice if
> sector_start is aligned on 4. Why don't you want the test I proposed
> in my last review?
> 
> 			if (i != sector_start && !(i % 4))
>

Applied with this adjustment.

Thanks,

Boris
 
> >  
> > -		for (i = sector; i <= sector_end; i++) {
> >  			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

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-03-23  7:52 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-21  0:21 [PATCH][v4] mtd: rawnand: fsl_ifc: Fix eccstat array overflow for IFC ver >= 2.0.0 Jagdish Gediya
2018-03-21  8:55 ` Boris Brezillon
2018-03-23  7:52   ` Boris Brezillon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).