RE: [PATCH v6] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter

[Wan, Jane (Nokia - US/Sunnyvale)]

Hi Boris,

I've sent v7 of the patch.

Thanks.
Jane

> -Original Message-
> From: Boris Brezillon [mailto:boris.brezil...@bootlin.com]
> Sent: Saturday, May 12, 2018 1:21 AM
> To: Wan, Jane (Nokia - US/Sunnyvale) <jane@nokia.com>
> Cc: miquel.ray...@bootlin.com; dw...@infradead.org;
> computersforpe...@gmail.com; rich...@nod.at; marek.va...@gmail.com;
> yamada.masah...@socionext.com; prabhakar.kushw...@nxp.com;
> shawn...@kernel.org; jagdish.ged...@nxp.com;
> shreeya.patel23...@gmail.com; linux-...@lists.infradead.org; linux-
> ker...@vger.kernel.org; Bos, Ties (Nokia - US/Sunnyvale) <ties@nokia.com>
> Subject: Re: [PATCH v6] mtd: rawnand: use bit-wise majority to recover the
> contents of ONFI parameter
> 
> On Thu, 10 May 2018 14:28:37 -0700
> Jane Wan <jane@nokia.com> wrote:
> 
> > Per ONFI specification (Rev. 4.0), if all parameter pages have invalid
> > CRC values, the bit-wise majority may be used to recover the contents
> > of the parameter pages from the parameter page copies present.
> >
> > Signed-off-by: Jane Wan <jane@nokia.com>
> > ---
> > v6: support the cases that srcbufs are not contiguous
> > v5: make the bit-wise majority functon generic
> > v4: move the bit-wise majority code in a separate function
> > v3: fix warning message detected by kbuild test robot
> > v2: rebase the changes on top of v4.17-rc1
> >
> >  drivers/mtd/nand/raw/nand_base.c |   52
> ++
> >  1 file changed, 47 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/nand_base.c
> > b/drivers/mtd/nand/raw/nand_base.c
> > index 72f3a89..acf905c 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -5087,6 +5087,35 @@ static int
> > nand_flash_detect_ext_param_page(struct nand_chip *chip,  }
> >
> >  /*
> > + * Recover data with bit-wise majority  */ static void
> > +nand_bit_wise_majority(const void **srcbufs,
> > +  unsigned int nsrcbufs,
> > +  void *dstbuf,
> > +  unsigned int bufsize)
> > +{
> > +   int i, j, k;
> > +
> > +   for (i = 0; i < bufsize; i++) {
> > +   u8 cnt, val;
> > +
> > +   val = 0;
> > +   for (j = 0; j < 8; j++) {
> > +   cnt = 0;
> > +   for (k = 0; k < nsrcbufs; k++) {
> > +   const u8 *srcbuf = srcbufs[k];
> > +
> > +   if (srcbuf[i] & BIT(j))
> > +   cnt++;
> > +   }
> > +   if (cnt > nsrcbufs / 2)
> > +   val |= BIT(j);
> > +   }
> > +   ((u8 *)dstbuf)[i] = val;
> > +   }
> > +}
> > +
> > +/*
> >   * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 
> > otherwise.
> >   */
> >  static int nand_flash_detect_onfi(struct nand_chip *chip) @@ -5102,7
> > +5131,7 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
> > return 0;
> >
> > /* ONFI chip: allocate a buffer to hold its parameter page */
> > -   p = kzalloc(sizeof(*p), GFP_KERNEL);
> > +   p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> > if (!p)
> > return -ENOMEM;
> >
> > @@ -5113,21 +5142,34 @@ static int nand_flash_detect_onfi(struct
> nand_chip *chip)
> > }
> >
> > for (i = 0; i < 3; i++) {
> > -   ret = nand_read_data_op(chip, p, sizeof(*p), true);
> > +   ret = nand_read_data_op(chip, [i], sizeof(*p), true);
> > if (ret) {
> > ret = 0;
> > goto free_onfi_param_page;
> > }
> >
> > -   if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> > +   if (onfi_crc16(ONFI_CRC_BASE, (u8 *)[i], 254) ==
> > le16_to_cpu(p->crc)) {
> > +   if (i)
> > +   memcpy(p, [i], sizeof(*p));
> > break;
> > }
> > }
> >
> > if (i == 3) {
> > -   pr_err("Could not find valid ONFI parameter page; aborting\n");
> > -   goto free_onfi_param_page;
> > +   const void *srcbufs[3] = {p, p + 1, p + 2};
> > +
> > +   pr_err("Could not find valid ONFI parameter page\n");
> 
> 

RE: [PATCH v6] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter

[Wan, Jane (Nokia - US/Sunnyvale)]

Hi Boris,

I've sent v7 of the patch.

Thanks.
Jane

> -Original Message-
> From: Boris Brezillon [mailto:boris.brezil...@bootlin.com]
> Sent: Saturday, May 12, 2018 1:21 AM
> To: Wan, Jane (Nokia - US/Sunnyvale) 
> Cc: miquel.ray...@bootlin.com; dw...@infradead.org;
> computersforpe...@gmail.com; rich...@nod.at; marek.va...@gmail.com;
> yamada.masah...@socionext.com; prabhakar.kushw...@nxp.com;
> shawn...@kernel.org; jagdish.ged...@nxp.com;
> shreeya.patel23...@gmail.com; linux-...@lists.infradead.org; linux-
> ker...@vger.kernel.org; Bos, Ties (Nokia - US/Sunnyvale) 
> Subject: Re: [PATCH v6] mtd: rawnand: use bit-wise majority to recover the
> contents of ONFI parameter
> 
> On Thu, 10 May 2018 14:28:37 -0700
> Jane Wan  wrote:
> 
> > Per ONFI specification (Rev. 4.0), if all parameter pages have invalid
> > CRC values, the bit-wise majority may be used to recover the contents
> > of the parameter pages from the parameter page copies present.
> >
> > Signed-off-by: Jane Wan 
> > ---
> > v6: support the cases that srcbufs are not contiguous
> > v5: make the bit-wise majority functon generic
> > v4: move the bit-wise majority code in a separate function
> > v3: fix warning message detected by kbuild test robot
> > v2: rebase the changes on top of v4.17-rc1
> >
> >  drivers/mtd/nand/raw/nand_base.c |   52
> ++
> >  1 file changed, 47 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/nand_base.c
> > b/drivers/mtd/nand/raw/nand_base.c
> > index 72f3a89..acf905c 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -5087,6 +5087,35 @@ static int
> > nand_flash_detect_ext_param_page(struct nand_chip *chip,  }
> >
> >  /*
> > + * Recover data with bit-wise majority  */ static void
> > +nand_bit_wise_majority(const void **srcbufs,
> > +  unsigned int nsrcbufs,
> > +  void *dstbuf,
> > +  unsigned int bufsize)
> > +{
> > +   int i, j, k;
> > +
> > +   for (i = 0; i < bufsize; i++) {
> > +   u8 cnt, val;
> > +
> > +   val = 0;
> > +   for (j = 0; j < 8; j++) {
> > +   cnt = 0;
> > +   for (k = 0; k < nsrcbufs; k++) {
> > +   const u8 *srcbuf = srcbufs[k];
> > +
> > +   if (srcbuf[i] & BIT(j))
> > +   cnt++;
> > +   }
> > +   if (cnt > nsrcbufs / 2)
> > +   val |= BIT(j);
> > +   }
> > +   ((u8 *)dstbuf)[i] = val;
> > +   }
> > +}
> > +
> > +/*
> >   * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 
> > otherwise.
> >   */
> >  static int nand_flash_detect_onfi(struct nand_chip *chip) @@ -5102,7
> > +5131,7 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
> > return 0;
> >
> > /* ONFI chip: allocate a buffer to hold its parameter page */
> > -   p = kzalloc(sizeof(*p), GFP_KERNEL);
> > +   p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> > if (!p)
> > return -ENOMEM;
> >
> > @@ -5113,21 +5142,34 @@ static int nand_flash_detect_onfi(struct
> nand_chip *chip)
> > }
> >
> > for (i = 0; i < 3; i++) {
> > -   ret = nand_read_data_op(chip, p, sizeof(*p), true);
> > +   ret = nand_read_data_op(chip, [i], sizeof(*p), true);
> > if (ret) {
> > ret = 0;
> > goto free_onfi_param_page;
> > }
> >
> > -   if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> > +   if (onfi_crc16(ONFI_CRC_BASE, (u8 *)[i], 254) ==
> > le16_to_cpu(p->crc)) {
> > +   if (i)
> > +   memcpy(p, [i], sizeof(*p));
> > break;
> > }
> > }
> >
> > if (i == 3) {
> > -   pr_err("Could not find valid ONFI parameter page; aborting\n");
> > -   goto free_onfi_param_page;
> > +   const void *srcbufs[3] = {p, p + 1, p + 2};
> > +
> > +   pr_err("Could not find valid ONFI parameter page\n");
> 
> Maybe pr_warn() here
> 
> > +   pr_info("Recover ONFI params with bit-wise majority\n");
> 
> and maybe you can pack the 2 messages:
> 
>   pr_warn("Could not find a valid ONFI parameter page, trying
> bit-wise majority to recover it");

[Jane] Changed as suggested.

> 
> > +
> > +   nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
> > +  sizeof(*p));
> > +
> > +   if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) !=
> > +   le16_to_cpu(p->crc)) {
> > +   pr_err("ONFI parameter recovery failed, aborting\n");
> > +   goto free_onfi_param_page;
> > +   }
> > }
> >
> > /* Check version */

[PATCH v7] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter

[Wan, Jane (Nokia - US/Sunnyvale)]

Per ONFI specification (Rev. 4.0), if all parameter pages have invalid CRC 
values, the bit-wise majority may be used to recover the contents of the 
parameter pages from the parameter page copies present.

Signed-off-by: Jane Wan 
---
v7: change debug print messages
v6: support the cases that srcbufs are not contiguous
v5: make the bit-wise majority functon generic
v4: move the bit-wise majority code in a separate function
v3: fix warning message detected by kbuild test robot
v2: rebase the changes on top of v4.17-rc1
 
drivers/mtd/nand/raw/nand_base.c |   50 ++
 1 file changed, 45 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 72f3a89..b43b784 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -5087,6 +5087,35 @@ static int nand_flash_detect_ext_param_page(struct 
nand_chip *chip,
 }
 
 /*
+ * Recover data with bit-wise majority
+ */
+static void nand_bit_wise_majority(const void **srcbufs,
+  unsigned int nsrcbufs,
+  void *dstbuf,
+  unsigned int bufsize)
+{
+   int i, j, k;
+
+   for (i = 0; i < bufsize; i++) {
+   u8 cnt, val;
+
+   val = 0;
+   for (j = 0; j < 8; j++) {
+   cnt = 0;
+   for (k = 0; k < nsrcbufs; k++) {
+   const u8 *srcbuf = srcbufs[k];
+
+   if (srcbuf[i] & BIT(j))
+   cnt++;
+   }
+   if (cnt > nsrcbufs / 2)
+   val |= BIT(j);
+   }
+   ((u8 *)dstbuf)[i] = val;
+   }
+}
+
+/*
  * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
  */
 static int nand_flash_detect_onfi(struct nand_chip *chip)
@@ -5102,7 +5131,7 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
return 0;
 
/* ONFI chip: allocate a buffer to hold its parameter page */
-   p = kzalloc(sizeof(*p), GFP_KERNEL);
+   p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
if (!p)
return -ENOMEM;
 
@@ -5113,21 +5142,32 @@ static int nand_flash_detect_onfi(struct nand_chip 
*chip)
}
 
for (i = 0; i < 3; i++) {
-   ret = nand_read_data_op(chip, p, sizeof(*p), true);
+   ret = nand_read_data_op(chip, [i], sizeof(*p), true);
if (ret) {
ret = 0;
goto free_onfi_param_page;
}
 
-   if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
+   if (onfi_crc16(ONFI_CRC_BASE, (u8 *)[i], 254) ==
le16_to_cpu(p->crc)) {
+   if (i)
+   memcpy(p, [i], sizeof(*p));
break;
}
}
 
if (i == 3) {
-   pr_err("Could not find valid ONFI parameter page; aborting\n");
-   goto free_onfi_param_page;
+   const void *srcbufs[3] = {p, p + 1, p + 2};
+
+   pr_warn("Could not find a valid ONFI parameter page, trying 
bit-wise majority to recover it\n");
+   nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
+  sizeof(*p));
+
+   if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) !=
+   le16_to_cpu(p->crc)) {
+   pr_err("ONFI parameter recovery failed, aborting\n");
+   goto free_onfi_param_page;
+   }
}
 
/* Check version */
-- 
1.7.9.5

[PATCH v7] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter

[Wan, Jane (Nokia - US/Sunnyvale)]

Per ONFI specification (Rev. 4.0), if all parameter pages have invalid CRC 
values, the bit-wise majority may be used to recover the contents of the 
parameter pages from the parameter page copies present.

Signed-off-by: Jane Wan 
---
v7: change debug print messages
v6: support the cases that srcbufs are not contiguous
v5: make the bit-wise majority functon generic
v4: move the bit-wise majority code in a separate function
v3: fix warning message detected by kbuild test robot
v2: rebase the changes on top of v4.17-rc1
 
drivers/mtd/nand/raw/nand_base.c |   50 ++
 1 file changed, 45 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 72f3a89..b43b784 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -5087,6 +5087,35 @@ static int nand_flash_detect_ext_param_page(struct 
nand_chip *chip,
 }
 
 /*
+ * Recover data with bit-wise majority
+ */
+static void nand_bit_wise_majority(const void **srcbufs,
+  unsigned int nsrcbufs,
+  void *dstbuf,
+  unsigned int bufsize)
+{
+   int i, j, k;
+
+   for (i = 0; i < bufsize; i++) {
+   u8 cnt, val;
+
+   val = 0;
+   for (j = 0; j < 8; j++) {
+   cnt = 0;
+   for (k = 0; k < nsrcbufs; k++) {
+   const u8 *srcbuf = srcbufs[k];
+
+   if (srcbuf[i] & BIT(j))
+   cnt++;
+   }
+   if (cnt > nsrcbufs / 2)
+   val |= BIT(j);
+   }
+   ((u8 *)dstbuf)[i] = val;
+   }
+}
+
+/*
  * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
  */
 static int nand_flash_detect_onfi(struct nand_chip *chip)
@@ -5102,7 +5131,7 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
return 0;
 
/* ONFI chip: allocate a buffer to hold its parameter page */
-   p = kzalloc(sizeof(*p), GFP_KERNEL);
+   p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
if (!p)
return -ENOMEM;
 
@@ -5113,21 +5142,32 @@ static int nand_flash_detect_onfi(struct nand_chip 
*chip)
}
 
for (i = 0; i < 3; i++) {
-   ret = nand_read_data_op(chip, p, sizeof(*p), true);
+   ret = nand_read_data_op(chip, [i], sizeof(*p), true);
if (ret) {
ret = 0;
goto free_onfi_param_page;
}
 
-   if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
+   if (onfi_crc16(ONFI_CRC_BASE, (u8 *)[i], 254) ==
le16_to_cpu(p->crc)) {
+   if (i)
+   memcpy(p, [i], sizeof(*p));
break;
}
}
 
if (i == 3) {
-   pr_err("Could not find valid ONFI parameter page; aborting\n");
-   goto free_onfi_param_page;
+   const void *srcbufs[3] = {p, p + 1, p + 2};
+
+   pr_warn("Could not find a valid ONFI parameter page, trying 
bit-wise majority to recover it\n");
+   nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
+  sizeof(*p));
+
+   if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) !=
+   le16_to_cpu(p->crc)) {
+   pr_err("ONFI parameter recovery failed, aborting\n");
+   goto free_onfi_param_page;
+   }
}
 
/* Check version */
-- 
1.7.9.5

RE: [PATCH v5 2/2] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter

[Wan, Jane (Nokia - US/Sunnyvale)]

Hi Boris,

I've sent v6 of the patch based on your comments.

Thanks.
Jane

> -Original Message-
> From: Boris Brezillon [mailto:boris.brezil...@bootlin.com]
> Sent: Thursday, May 10, 2018 5:03 AM
> To: Wan, Jane (Nokia - US/Sunnyvale) <jane@nokia.com>
> Cc: miquel.ray...@bootlin.com; dw...@infradead.org;
> computersforpe...@gmail.com; rich...@nod.at; marek.va...@gmail.com;
> yamada.masah...@socionext.com; prabhakar.kushw...@nxp.com;
> shawn...@kernel.org; jagdish.ged...@nxp.com;
> shreeya.patel23...@gmail.com; linux-...@lists.infradead.org; linux-
> ker...@vger.kernel.org; Bos, Ties (Nokia - US/Sunnyvale) <ties@nokia.com>
> Subject: Re: [PATCH v5 2/2] mtd: rawnand: use bit-wise majority to recover the
> contents of ONFI parameter
> 
> Hi Jane,
> 
> Subject prefix should be "[PATCH v5] ...", the 2/2 is no longer valid since 
> you only
> have one patch here.
> 
> On Wed,  9 May 2018 19:46:40 -0700
> Jane Wan <jane@nokia.com> wrote:
> 
> > Per ONFI specification (Rev. 4.0), if all parameter pages have invalid
> > CRC values, the bit-wise majority may be used to recover the contents
> > of the parameter pages from the parameter page copies present.
> >
> > Signed-off-by: Jane Wan <jane@nokia.com>
> > ---
> 
> There should be a changelog here describing what has changed in each version
> of the patch.

[Jane] Added the changelogs in v6.

> 
> >  drivers/mtd/nand/raw/nand_base.c |   46
> +-
> >  1 file changed, 41 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/nand_base.c
> > b/drivers/mtd/nand/raw/nand_base.c
> > index 72f3a89..a7c2507 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -5086,6 +5086,34 @@ static int
> nand_flash_detect_ext_param_page(struct nand_chip *chip,
> > return ret;
> >  }
> >
> > +#define GET_BIT(bit, val)   (((val) >> (bit)) & 0x01)
> 
> Not sure we need that macro, see below.

[Jane] Removed.

> 
> > +
> > +/*
> > + * Recover data with bit-wise majority  */ static void
> > +nand_bit_wise_majority(const void **srcbufs,
> > +  void *dstbuf,
> > +  unsigned int nbufs,
> > +  unsigned int bufsize)
> 
> I'd prefer to have nbufs just after srcbufs and named nsrcbufs:
> 
> static void nand_bit_wise_majority(const void **srcbufs,
>  unsigned int nsrcbufs,
>  void *dstbuf,
>  unsigned int bufsize)

[Jane] changed as above in v6.

> 
> > +{
> > +   int i, j, k;
> > +   u8 v, m;
> > +   u8 *p;
> > +
> > +   p = *(u8 **)srcbufs;
> 
> Nope, I'd like to support the cases where srcbufs are not contiguous, so that
> does not work.

[Jane] Changed as you suggested to support non-contiguous srcbufs.

> 
> > +   for (i = 0; i < bufsize; i++) {
> > +   v = 0;
> 
> You can declare the v variable here, since its scope is limited to the for 
> loop.
> BTW, v, m, can't we pick better names? I guess v is for val, but I'm not even 
> sure
> what m stands for.

[Jane] changed the variables to cnt and val in v6.  The "m" was for majority, 
now changed to cnt (counts for 1s).

> 
> > +   for (j = 0; j < 8; j++) {
> > +   m = 0;
> > +   for (k = 0; k < nbufs; k++)
> > +   m += GET_BIT(j, p[k*bufsize + i]);
> 
>   for (k = 0; k < nbufs; k++) {
>   const u8 *srcbuf = srcbufs[j];
> 
>   if (srcbuf[i] & BIT(k))
>   m++;
>   }
> 
> > +   if (m > nbufs/2)
> 
> Space between operands and operators please
> 
>   if (m > nbufs / 2)

[Jane]  Changed as suggested in v6.  Thanks.

> 
> > +   v |= BIT(j);
> > +   }
> > +   ((u8 *)dstbuf)[i] = v;
> > +   }
> > +}
> > +
> >  /*
> >   * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 
> > otherwise.
> >   */
> > @@ -5102,7 +5130,7 @@ static int nand_flash_detect_onfi(struct nand_chip
> *chip)
> > return 0;
> >
> > /* ONFI chip: allocate a buffer to hold its parameter page */
> > -   p = kzalloc(sizeof(*p), GFP_KERNEL);
> > +   p = kzalloc((sizeof(*p) * 3), GF

RE: [PATCH v5 2/2] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter

[Wan, Jane (Nokia - US/Sunnyvale)]

Hi Boris,

I've sent v6 of the patch based on your comments.

Thanks.
Jane

> -Original Message-
> From: Boris Brezillon [mailto:boris.brezil...@bootlin.com]
> Sent: Thursday, May 10, 2018 5:03 AM
> To: Wan, Jane (Nokia - US/Sunnyvale) 
> Cc: miquel.ray...@bootlin.com; dw...@infradead.org;
> computersforpe...@gmail.com; rich...@nod.at; marek.va...@gmail.com;
> yamada.masah...@socionext.com; prabhakar.kushw...@nxp.com;
> shawn...@kernel.org; jagdish.ged...@nxp.com;
> shreeya.patel23...@gmail.com; linux-...@lists.infradead.org; linux-
> ker...@vger.kernel.org; Bos, Ties (Nokia - US/Sunnyvale) 
> Subject: Re: [PATCH v5 2/2] mtd: rawnand: use bit-wise majority to recover the
> contents of ONFI parameter
> 
> Hi Jane,
> 
> Subject prefix should be "[PATCH v5] ...", the 2/2 is no longer valid since 
> you only
> have one patch here.
> 
> On Wed,  9 May 2018 19:46:40 -0700
> Jane Wan  wrote:
> 
> > Per ONFI specification (Rev. 4.0), if all parameter pages have invalid
> > CRC values, the bit-wise majority may be used to recover the contents
> > of the parameter pages from the parameter page copies present.
> >
> > Signed-off-by: Jane Wan 
> > ---
> 
> There should be a changelog here describing what has changed in each version
> of the patch.

[Jane] Added the changelogs in v6.

> 
> >  drivers/mtd/nand/raw/nand_base.c |   46
> +-
> >  1 file changed, 41 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/nand_base.c
> > b/drivers/mtd/nand/raw/nand_base.c
> > index 72f3a89..a7c2507 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -5086,6 +5086,34 @@ static int
> nand_flash_detect_ext_param_page(struct nand_chip *chip,
> > return ret;
> >  }
> >
> > +#define GET_BIT(bit, val)   (((val) >> (bit)) & 0x01)
> 
> Not sure we need that macro, see below.

[Jane] Removed.

> 
> > +
> > +/*
> > + * Recover data with bit-wise majority  */ static void
> > +nand_bit_wise_majority(const void **srcbufs,
> > +  void *dstbuf,
> > +  unsigned int nbufs,
> > +  unsigned int bufsize)
> 
> I'd prefer to have nbufs just after srcbufs and named nsrcbufs:
> 
> static void nand_bit_wise_majority(const void **srcbufs,
>  unsigned int nsrcbufs,
>  void *dstbuf,
>  unsigned int bufsize)

[Jane] changed as above in v6.

> 
> > +{
> > +   int i, j, k;
> > +   u8 v, m;
> > +   u8 *p;
> > +
> > +   p = *(u8 **)srcbufs;
> 
> Nope, I'd like to support the cases where srcbufs are not contiguous, so that
> does not work.

[Jane] Changed as you suggested to support non-contiguous srcbufs.

> 
> > +   for (i = 0; i < bufsize; i++) {
> > +   v = 0;
> 
> You can declare the v variable here, since its scope is limited to the for 
> loop.
> BTW, v, m, can't we pick better names? I guess v is for val, but I'm not even 
> sure
> what m stands for.

[Jane] changed the variables to cnt and val in v6.  The "m" was for majority, 
now changed to cnt (counts for 1s).

> 
> > +   for (j = 0; j < 8; j++) {
> > +   m = 0;
> > +   for (k = 0; k < nbufs; k++)
> > +   m += GET_BIT(j, p[k*bufsize + i]);
> 
>   for (k = 0; k < nbufs; k++) {
>   const u8 *srcbuf = srcbufs[j];
> 
>   if (srcbuf[i] & BIT(k))
>   m++;
>   }
> 
> > +   if (m > nbufs/2)
> 
> Space between operands and operators please
> 
>   if (m > nbufs / 2)

[Jane]  Changed as suggested in v6.  Thanks.

> 
> > +   v |= BIT(j);
> > +   }
> > +   ((u8 *)dstbuf)[i] = v;
> > +   }
> > +}
> > +
> >  /*
> >   * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 
> > otherwise.
> >   */
> > @@ -5102,7 +5130,7 @@ static int nand_flash_detect_onfi(struct nand_chip
> *chip)
> > return 0;
> >
> > /* ONFI chip: allocate a buffer to hold its parameter page */
> > -   p = kzalloc(sizeof(*p), GFP_KERNEL);
> > +   p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> > if (!p)
> > return -ENOMEM;
> >
> > @@ -5113,21 +5141,29 @

RE: [PATCH v4 2/2] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter

[Wan, Jane (Nokia - US/Sunnyvale)]

Hi Boris,

I've sent v5 of the patch based on your comment.

Thanks.
Jane

> -Original Message-
> From: Boris Brezillon [mailto:boris.brezil...@bootlin.com]
> Sent: Wednesday, May 09, 2018 7:19 AM
> To: Wan, Jane (Nokia - US/Sunnyvale) <jane@nokia.com>
> Cc: miquel.ray...@bootlin.com; dw...@infradead.org;
> computersforpe...@gmail.com; rich...@nod.at; marek.va...@gmail.com;
> yamada.masah...@socionext.com; prabhakar.kushw...@nxp.com;
> shawn...@kernel.org; jagdish.ged...@nxp.com;
> shreeya.patel23...@gmail.com; linux-...@lists.infradead.org; linux-
> ker...@vger.kernel.org; Bos, Ties (Nokia - US/Sunnyvale) <ties@nokia.com>
> Subject: Re: [PATCH v4 2/2] mtd: rawnand: use bit-wise majority to recover the
> contents of ONFI parameter
> 
> On Tue,  8 May 2018 14:19:54 -0700
> Jane Wan <jane@nokia.com> wrote:
> 
> > Per ONFI specification (Rev. 4.0), if all parameter pages have invalid
> > CRC values, the bit-wise majority may be used to recover the contents
> > of the parameter pages from the parameter page copies present.
> >
> > Signed-off-by: Jane Wan <jane@nokia.com>
> > ---
> >  drivers/mtd/nand/raw/nand_base.c |   49
> ++
> >  1 file changed, 44 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/nand_base.c
> > b/drivers/mtd/nand/raw/nand_base.c
> > index 72f3a89..dfc341c 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -5086,6 +5086,38 @@ static int
> nand_flash_detect_ext_param_page(struct nand_chip *chip,
> > return ret;
> >  }
> >
> > +#define GET_BIT(bit, val)   (((val) >> (bit)) & 0x01)
> > +
> > +/*
> > + * Recover NAND parameter page with bit-wise majority  */ static int
> > +onfi_recover_param(struct nand_onfi_params *p, int pages)
> 
> I had something more generic in mind:
> 
> static void nand_bit_wise_majority(const void **srcbufs,
>  void *dstbuf,
>  unsigned int nbufs,
>  unsigned int bufsize)
> {
>   ...
> }
> 
> And then you do the crc check in nand_flash_detect_onfi().
> 
> The reason I'm asking that is because I'm almost sure we'll re-use this 
> functions
> for extended param pages, and also for vendor specific data (we already have a
> byte-wise majority check in the hynix driver, so I wouldn't be surprised if 
> other
> vendors decided to use a bit-wise approach for some of their OTP area).

[Jane] Modified the function as suggested in v5.

> 
> > +{
> > +   int i, j, k;
> > +   u8 v, m;
> > +   u8 *buf;
> > +
> > +   buf = (u8 *)p;
> > +   for (i = 0; i < sizeof(*p); i++) {
> > +   v = 0;
> > +   for (j = 0; j < 8; j++) {
> > +   m = 0;
> > +   for (k = 0; k < pages; k++)
> > +   m += GET_BIT(j, buf[k*sizeof(*p) + i]);
> > +   if (m > pages/2)
> > +   v |= BIT(j);
> > +   }
> > +   ((u8 *)p)[i] = v;
> > +   }
> > +
> > +   if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) ==
> > +   le16_to_cpu(p->crc)) {
> > +   return 1;
> > +   }
> > +
> > +   return 0;
> > +}
> > +
> >  /*
> >   * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 
> > otherwise.
> >   */
> > @@ -5102,7 +5134,7 @@ static int nand_flash_detect_onfi(struct nand_chip
> *chip)
> > return 0;
> >
> > /* ONFI chip: allocate a buffer to hold its parameter page */
> > -   p = kzalloc(sizeof(*p), GFP_KERNEL);
> > +   p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> > if (!p)
> > return -ENOMEM;
> >
> > @@ -5113,21 +5145,28 @@ static int nand_flash_detect_onfi(struct
> nand_chip *chip)
> > }
> >
> > for (i = 0; i < 3; i++) {
> > -   ret = nand_read_data_op(chip, p, sizeof(*p), true);
> > +   ret = nand_read_data_op(chip, [i], sizeof(*p), true);
> > if (ret) {
> > ret = 0;
> > goto free_onfi_param_page;
> > }
> >
> > -   if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> > +   if (onfi_crc16(ONFI_CRC_BASE, (u8 *)[i], 254) ==
> > le16_to_cpu(p->crc)) {
> > +   if (i)
> > +   memcpy(p, [i], sizeof(*p));
> > break;
> > }
> > }
> >
> > if (i == 3) {
> > -   pr_err("Could not find valid ONFI parameter page; aborting\n");
> > -   goto free_onfi_param_page;
> > +   pr_err("Could not find valid ONFI parameter page\n");
> > +   pr_info("Recover ONFI params with bit-wise majority\n");
> > +   ret = onfi_recover_param(p, 3);
> > +   if (ret == 0) {
> > +   pr_err("ONFI parameter recovery failed, aborting\n");
> > +   goto free_onfi_param_page;
> > +   }
> > }
> >
> > /* Check version */

RE: [PATCH v4 2/2] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter

[Wan, Jane (Nokia - US/Sunnyvale)]

Hi Boris,

I've sent v5 of the patch based on your comment.

Thanks.
Jane

> -Original Message-
> From: Boris Brezillon [mailto:boris.brezil...@bootlin.com]
> Sent: Wednesday, May 09, 2018 7:19 AM
> To: Wan, Jane (Nokia - US/Sunnyvale) 
> Cc: miquel.ray...@bootlin.com; dw...@infradead.org;
> computersforpe...@gmail.com; rich...@nod.at; marek.va...@gmail.com;
> yamada.masah...@socionext.com; prabhakar.kushw...@nxp.com;
> shawn...@kernel.org; jagdish.ged...@nxp.com;
> shreeya.patel23...@gmail.com; linux-...@lists.infradead.org; linux-
> ker...@vger.kernel.org; Bos, Ties (Nokia - US/Sunnyvale) 
> Subject: Re: [PATCH v4 2/2] mtd: rawnand: use bit-wise majority to recover the
> contents of ONFI parameter
> 
> On Tue,  8 May 2018 14:19:54 -0700
> Jane Wan  wrote:
> 
> > Per ONFI specification (Rev. 4.0), if all parameter pages have invalid
> > CRC values, the bit-wise majority may be used to recover the contents
> > of the parameter pages from the parameter page copies present.
> >
> > Signed-off-by: Jane Wan 
> > ---
> >  drivers/mtd/nand/raw/nand_base.c |   49
> ++
> >  1 file changed, 44 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/nand_base.c
> > b/drivers/mtd/nand/raw/nand_base.c
> > index 72f3a89..dfc341c 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -5086,6 +5086,38 @@ static int
> nand_flash_detect_ext_param_page(struct nand_chip *chip,
> > return ret;
> >  }
> >
> > +#define GET_BIT(bit, val)   (((val) >> (bit)) & 0x01)
> > +
> > +/*
> > + * Recover NAND parameter page with bit-wise majority  */ static int
> > +onfi_recover_param(struct nand_onfi_params *p, int pages)
> 
> I had something more generic in mind:
> 
> static void nand_bit_wise_majority(const void **srcbufs,
>  void *dstbuf,
>  unsigned int nbufs,
>  unsigned int bufsize)
> {
>   ...
> }
> 
> And then you do the crc check in nand_flash_detect_onfi().
> 
> The reason I'm asking that is because I'm almost sure we'll re-use this 
> functions
> for extended param pages, and also for vendor specific data (we already have a
> byte-wise majority check in the hynix driver, so I wouldn't be surprised if 
> other
> vendors decided to use a bit-wise approach for some of their OTP area).

[Jane] Modified the function as suggested in v5.

> 
> > +{
> > +   int i, j, k;
> > +   u8 v, m;
> > +   u8 *buf;
> > +
> > +   buf = (u8 *)p;
> > +   for (i = 0; i < sizeof(*p); i++) {
> > +   v = 0;
> > +   for (j = 0; j < 8; j++) {
> > +   m = 0;
> > +   for (k = 0; k < pages; k++)
> > +   m += GET_BIT(j, buf[k*sizeof(*p) + i]);
> > +   if (m > pages/2)
> > +   v |= BIT(j);
> > +   }
> > +   ((u8 *)p)[i] = v;
> > +   }
> > +
> > +   if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) ==
> > +   le16_to_cpu(p->crc)) {
> > +   return 1;
> > +   }
> > +
> > +   return 0;
> > +}
> > +
> >  /*
> >   * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 
> > otherwise.
> >   */
> > @@ -5102,7 +5134,7 @@ static int nand_flash_detect_onfi(struct nand_chip
> *chip)
> > return 0;
> >
> > /* ONFI chip: allocate a buffer to hold its parameter page */
> > -   p = kzalloc(sizeof(*p), GFP_KERNEL);
> > +   p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> > if (!p)
> > return -ENOMEM;
> >
> > @@ -5113,21 +5145,28 @@ static int nand_flash_detect_onfi(struct
> nand_chip *chip)
> > }
> >
> > for (i = 0; i < 3; i++) {
> > -   ret = nand_read_data_op(chip, p, sizeof(*p), true);
> > +   ret = nand_read_data_op(chip, [i], sizeof(*p), true);
> > if (ret) {
> > ret = 0;
> > goto free_onfi_param_page;
> > }
> >
> > -   if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> > +   if (onfi_crc16(ONFI_CRC_BASE, (u8 *)[i], 254) ==
> > le16_to_cpu(p->crc)) {
> > +   if (i)
> > +   memcpy(p, [i], sizeof(*p));
> > break;
> > }
> > }
> >
> > if (i == 3) {
> > -   pr_err("Could not find valid ONFI parameter page; aborting\n");
> > -   goto free_onfi_param_page;
> > +   pr_err("Could not find valid ONFI parameter page\n");
> > +   pr_info("Recover ONFI params with bit-wise majority\n");
> > +   ret = onfi_recover_param(p, 3);
> > +   if (ret == 0) {
> > +   pr_err("ONFI parameter recovery failed, aborting\n");
> > +   goto free_onfi_param_page;
> > +   }
> > }
> >
> > /* Check version */

RE: [PATCH v3 2/2] mtd: rawnand: fsl_ifc: use bit-wise majority to recover the contents of ONFI parameter

[Wan, Jane (Nokia - US/Sunnyvale)]

Hi Thomas,

I've sent v4 of the patches based on the comments.

Thank you.
Jane

> -Original Message-
> From: Thomas Petazzoni [mailto:thomas.petazz...@bootlin.com]
> Sent: Tuesday, May 08, 2018 6:13 AM
> To: Wan, Jane (Nokia - US/Sunnyvale) <jane@nokia.com>
> Cc: boris.brezil...@bootlin.com; miquel.ray...@bootlin.com;
> dw...@infradead.org; computersforpe...@gmail.com; rich...@nod.at;
> marek.va...@gmail.com; yamada.masah...@socionext.com;
> prabhakar.kushw...@nxp.com; shawn...@kernel.org;
> jagdish.ged...@nxp.com; shreeya.patel23...@gmail.com; Bos, Ties (Nokia -
> US/Sunnyvale) <ties@nokia.com>; linux-...@lists.infradead.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH v3 2/2] mtd: rawnand: fsl_ifc: use bit-wise majority to
> recover the contents of ONFI parameter
> 
> Hello,
> 
> On Mon,  7 May 2018 09:34:15 -0700, Jane Wan wrote:
> > Per ONFI specification (Rev. 4.0), if all parameter pages have invalid
> > CRC values, the bit-wise majority may be used to recover the contents
> > of the parameter pages from the parameter page copies present.
> >
> > Signed-off-by: Jane Wan <jane@nokia.com>
> 
> Minor nit: the commit title contains "fsl_ifc", but this commit doesn't change
> anything in the fsl_ifc driver, it fixes the NAND core.

[Jane] Removed the "fsl_ifc" from the subject in v4.

> 
> Best regards,
> 
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons) Embedded Linux and
> Kernel engineering https://bootlin.com

RE: [PATCH v3 2/2] mtd: rawnand: fsl_ifc: use bit-wise majority to recover the contents of ONFI parameter

[Wan, Jane (Nokia - US/Sunnyvale)]

Hi Thomas,

I've sent v4 of the patches based on the comments.

Thank you.
Jane

> -Original Message-
> From: Thomas Petazzoni [mailto:thomas.petazz...@bootlin.com]
> Sent: Tuesday, May 08, 2018 6:13 AM
> To: Wan, Jane (Nokia - US/Sunnyvale) 
> Cc: boris.brezil...@bootlin.com; miquel.ray...@bootlin.com;
> dw...@infradead.org; computersforpe...@gmail.com; rich...@nod.at;
> marek.va...@gmail.com; yamada.masah...@socionext.com;
> prabhakar.kushw...@nxp.com; shawn...@kernel.org;
> jagdish.ged...@nxp.com; shreeya.patel23...@gmail.com; Bos, Ties (Nokia -
> US/Sunnyvale) ; linux-...@lists.infradead.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH v3 2/2] mtd: rawnand: fsl_ifc: use bit-wise majority to
> recover the contents of ONFI parameter
> 
> Hello,
> 
> On Mon,  7 May 2018 09:34:15 -0700, Jane Wan wrote:
> > Per ONFI specification (Rev. 4.0), if all parameter pages have invalid
> > CRC values, the bit-wise majority may be used to recover the contents
> > of the parameter pages from the parameter page copies present.
> >
> > Signed-off-by: Jane Wan 
> 
> Minor nit: the commit title contains "fsl_ifc", but this commit doesn't change
> anything in the fsl_ifc driver, it fixes the NAND core.

[Jane] Removed the "fsl_ifc" from the subject in v4.

> 
> Best regards,
> 
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons) Embedded Linux and
> Kernel engineering https://bootlin.com

RE: [PATCH v3 2/2] mtd: rawnand: fsl_ifc: use bit-wise majority to recover the contents of ONFI parameter

[Wan, Jane (Nokia - US/Sunnyvale)]

Hi Boris,

I've sent v4 of the patches based on the comments.

Thanks.
Jane

> -Original Message-
> From: Boris Brezillon [mailto:boris.brezil...@bootlin.com]
> Sent: Tuesday, May 08, 2018 2:09 AM
> To: Wan, Jane (Nokia - US/Sunnyvale) <jane@nokia.com>
> Cc: miquel.ray...@bootlin.com; dw...@infradead.org;
> computersforpe...@gmail.com; rich...@nod.at; marek.va...@gmail.com;
> yamada.masah...@socionext.com; prabhakar.kushw...@nxp.com;
> shawn...@kernel.org; jagdish.ged...@nxp.com;
> shreeya.patel23...@gmail.com; linux-...@lists.infradead.org; linux-
> ker...@vger.kernel.org; Bos, Ties (Nokia - US/Sunnyvale) <ties@nokia.com>
> Subject: Re: [PATCH v3 2/2] mtd: rawnand: fsl_ifc: use bit-wise majority to
> recover the contents of ONFI parameter
> 
> On Mon,  7 May 2018 09:34:15 -0700
> Jane Wan <jane@nokia.com> wrote:
> 
> > Per ONFI specification (Rev. 4.0), if all parameter pages have invalid
> > CRC values, the bit-wise majority may be used to recover the contents
> > of the parameter pages from the parameter page copies present.
> >
> > Signed-off-by: Jane Wan <jane@nokia.com>
> 
> I never received patch 1 of this series. When you fix something in a commit,
> please resend the whole patchset, even if other patches haven't changed.

 [Jane] Thanks for the info.  I've sent both patches on v4.

> 
> > ---
> >  drivers/mtd/nand/raw/nand_base.c |   41
> ++
> >  1 file changed, 33 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/nand_base.c
> > b/drivers/mtd/nand/raw/nand_base.c
> > index 72f3a89..48f2dec 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -5086,15 +5086,18 @@ static int
> nand_flash_detect_ext_param_page(struct nand_chip *chip,
> > return ret;
> >  }
> >
> > +#define GET_BIT(bit, val)   (((val) >> (bit)) & 0x01)
> > +
> >  /*
> >   * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 
> > otherwise.
> >   */
> >  static int nand_flash_detect_onfi(struct nand_chip *chip)  {
> > struct mtd_info *mtd = nand_to_mtd(chip);
> > -   struct nand_onfi_params *p;
> > +   struct nand_onfi_params *p = NULL;
> > char id[4];
> > -   int i, ret, val;
> > +   int i, ret, val, pagesize;
> > +   u8 *buf = NULL;
> >
> > /* Try ONFI for unknown chip or LP */
> > ret = nand_readid_op(chip, 0x20, id, sizeof(id)); @@ -5102,8 +5105,9
> > @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
> > return 0;
> >
> > /* ONFI chip: allocate a buffer to hold its parameter page */
> > -   p = kzalloc(sizeof(*p), GFP_KERNEL);
> > -   if (!p)
> > +   pagesize = sizeof(*p);
> > +   buf = kzalloc((pagesize * 3), GFP_KERNEL);
> 
> Not sure why you have to add a new buf variable here, and pagesize is not
> needed either, just use sizeof(*p) directly.

[Jane] Removed buf in v4.

> 
> > +   if (!buf)
> > return -ENOMEM;
> >
> > ret = nand_read_param_page_op(chip, 0, NULL, 0); @@ -5113,7
> +5117,8
> > @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
> > }
> >
> > for (i = 0; i < 3; i++) {
> > -   ret = nand_read_data_op(chip, p, sizeof(*p), true);
> > +   p = (struct nand_onfi_params *)[i*pagesize];
> > +   ret = nand_read_data_op(chip, p, pagesize, true);
> > if (ret) {
> > ret = 0;
> > goto free_onfi_param_page;
> > @@ -5126,8 +5131,27 @@ static int nand_flash_detect_onfi(struct nand_chip
> *chip)
> > }
> >
> > if (i == 3) {
> > -   pr_err("Could not find valid ONFI parameter page; aborting\n");
> > -   goto free_onfi_param_page;
> > +   int j, k, l;
> > +   u8 v, m;
> > +
> > +   pr_err("Could not find valid ONFI parameter page\n");
> > +   pr_info("Recover ONFI params with bit-wise majority\n");
> > +   for (j = 0; j < pagesize; j++) {
> > +   v = 0;
> > +   for (k = 0; k < 8; k++) {
> > +   m = 0;
> > +   for (l = 0; l < 3; l++)
> > +   m += GET_BIT(k, buf[l*pagesize + j]);
> > +   if (m > 1)
> > +   v |= BIT(k);
> > +   }
> > +   ((u8 *)p)[j] = v

RE: [PATCH v3 2/2] mtd: rawnand: fsl_ifc: use bit-wise majority to recover the contents of ONFI parameter

[Wan, Jane (Nokia - US/Sunnyvale)]

Hi Boris,

I've sent v4 of the patches based on the comments.

Thanks.
Jane

> -Original Message-
> From: Boris Brezillon [mailto:boris.brezil...@bootlin.com]
> Sent: Tuesday, May 08, 2018 2:09 AM
> To: Wan, Jane (Nokia - US/Sunnyvale) 
> Cc: miquel.ray...@bootlin.com; dw...@infradead.org;
> computersforpe...@gmail.com; rich...@nod.at; marek.va...@gmail.com;
> yamada.masah...@socionext.com; prabhakar.kushw...@nxp.com;
> shawn...@kernel.org; jagdish.ged...@nxp.com;
> shreeya.patel23...@gmail.com; linux-...@lists.infradead.org; linux-
> ker...@vger.kernel.org; Bos, Ties (Nokia - US/Sunnyvale) 
> Subject: Re: [PATCH v3 2/2] mtd: rawnand: fsl_ifc: use bit-wise majority to
> recover the contents of ONFI parameter
> 
> On Mon,  7 May 2018 09:34:15 -0700
> Jane Wan  wrote:
> 
> > Per ONFI specification (Rev. 4.0), if all parameter pages have invalid
> > CRC values, the bit-wise majority may be used to recover the contents
> > of the parameter pages from the parameter page copies present.
> >
> > Signed-off-by: Jane Wan 
> 
> I never received patch 1 of this series. When you fix something in a commit,
> please resend the whole patchset, even if other patches haven't changed.

 [Jane] Thanks for the info.  I've sent both patches on v4.

> 
> > ---
> >  drivers/mtd/nand/raw/nand_base.c |   41
> ++
> >  1 file changed, 33 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/nand_base.c
> > b/drivers/mtd/nand/raw/nand_base.c
> > index 72f3a89..48f2dec 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -5086,15 +5086,18 @@ static int
> nand_flash_detect_ext_param_page(struct nand_chip *chip,
> > return ret;
> >  }
> >
> > +#define GET_BIT(bit, val)   (((val) >> (bit)) & 0x01)
> > +
> >  /*
> >   * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 
> > otherwise.
> >   */
> >  static int nand_flash_detect_onfi(struct nand_chip *chip)  {
> > struct mtd_info *mtd = nand_to_mtd(chip);
> > -   struct nand_onfi_params *p;
> > +   struct nand_onfi_params *p = NULL;
> > char id[4];
> > -   int i, ret, val;
> > +   int i, ret, val, pagesize;
> > +   u8 *buf = NULL;
> >
> > /* Try ONFI for unknown chip or LP */
> > ret = nand_readid_op(chip, 0x20, id, sizeof(id)); @@ -5102,8 +5105,9
> > @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
> > return 0;
> >
> > /* ONFI chip: allocate a buffer to hold its parameter page */
> > -   p = kzalloc(sizeof(*p), GFP_KERNEL);
> > -   if (!p)
> > +   pagesize = sizeof(*p);
> > +   buf = kzalloc((pagesize * 3), GFP_KERNEL);
> 
> Not sure why you have to add a new buf variable here, and pagesize is not
> needed either, just use sizeof(*p) directly.

[Jane] Removed buf in v4.

> 
> > +   if (!buf)
> > return -ENOMEM;
> >
> > ret = nand_read_param_page_op(chip, 0, NULL, 0); @@ -5113,7
> +5117,8
> > @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
> > }
> >
> > for (i = 0; i < 3; i++) {
> > -   ret = nand_read_data_op(chip, p, sizeof(*p), true);
> > +   p = (struct nand_onfi_params *)[i*pagesize];
> > +   ret = nand_read_data_op(chip, p, pagesize, true);
> > if (ret) {
> > ret = 0;
> > goto free_onfi_param_page;
> > @@ -5126,8 +5131,27 @@ static int nand_flash_detect_onfi(struct nand_chip
> *chip)
> > }
> >
> > if (i == 3) {
> > -   pr_err("Could not find valid ONFI parameter page; aborting\n");
> > -   goto free_onfi_param_page;
> > +   int j, k, l;
> > +   u8 v, m;
> > +
> > +   pr_err("Could not find valid ONFI parameter page\n");
> > +   pr_info("Recover ONFI params with bit-wise majority\n");
> > +   for (j = 0; j < pagesize; j++) {
> > +   v = 0;
> > +   for (k = 0; k < 8; k++) {
> > +   m = 0;
> > +   for (l = 0; l < 3; l++)
> > +   m += GET_BIT(k, buf[l*pagesize + j]);
> > +   if (m > 1)
> > +   v |= BIT(k);
> > +   }
> > +   ((u8 *)p)[j] = v;
> > +   }
> 
> Can you move the bit-wise majority code in a separate function?

[Jane] Don

RE: [PATCH v2 2/2] mtd: rawnand: fsl_ifc: use bit-wise majority to recover the contents of ONFI parameter

[Wan, Jane (Nokia - US/Sunnyvale)]

Hi,

I've sent v3 of this patch through git send-email.  Please also see the inline 
answer.

Thanks.
Jane

> -Original Message-
> From: kbuild test robot [mailto:l...@intel.com]
> Sent: Saturday, May 05, 2018 1:59 AM
> To: Wan, Jane (Nokia - US/Sunnyvale) <jane@nokia.com>
> Cc: kbuild-...@01.org; boris.brezil...@bootlin.com;
> miquel.ray...@bootlin.com; dw...@infradead.org;
> computersforpe...@gmail.com; rich...@nod.at; marek.va...@gmail.com;
> yamada.masah...@socionext.com; prabhakar.kushw...@nxp.com;
> shawn...@kernel.org; jagdish.ged...@nxp.com;
> shreeya.patel23...@gmail.com; linux-...@lists.infradead.org; linux-
> ker...@vger.kernel.org; Bos, Ties (Nokia - US/Sunnyvale)
> <ties....@nokia.com>; Wan, Jane (Nokia - US/Sunnyvale)
> <jane@nokia.com>
> Subject: Re: [PATCH v2 2/2] mtd: rawnand: fsl_ifc: use bit-wise majority to
> recover the contents of ONFI parameter
> 
> Hi Jane,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on mtd/nand/next] [also build test WARNING on
> v4.17-rc3 next-20180504] [if your patch is applied to the wrong git tree, 
> please
> drop us a note to help improve the system]
> 
> url:https://github.com/0day-ci/linux/commits/Jane-Wan/mtd-rawnand-
> fsl_ifc-fix-FSL-NAND-driver-to-read-all-ONFI-parameter-pages/20180505-
> 163132
> base:   git://git.infradead.org/linux-mtd.git nand/next
> config: x86_64-randconfig-x015-201817 (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64
> 
> Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
> http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
> 
> All warnings (new ones prefixed by >>):
> 
>drivers/mtd/nand/raw/nand_base.c: In function 'nand_scan_ident':
> >> drivers/mtd/nand/raw/nand_base.c:5247:2: warning: 'p' may be used
> >> uninitialized in this function [-Wmaybe-uninitialized]
>  kfree(p);
>  ^~~~
>drivers/mtd/nand/raw/nand_base.c:5097:27: note: 'p' was declared here
>  struct nand_onfi_params *p;
>   ^

 [Jane] the kfree() is fixed in v3 patch.  Thanks.
 
> 
> vim +/p +5247 drivers/mtd/nand/raw/nand_base.c
> 
> 2c5f4f892 drivers/mtd/nand/raw/nand_base.c Jane Wan 2018-05-04  5090
> d1e1f4e42 drivers/mtd/nand/nand_base.c Florian Fainelli 2010-08-30  5091
> /*
> 8b6e50c9e drivers/mtd/nand/nand_base.c Brian Norris 2011-05-25  5092  
>  *
> Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
> 6fb277ba8 drivers/mtd/nand/nand_base.c Florian Fainelli 2010-09-01  5093
> */
> 29a198a15 drivers/mtd/nand/nand_base.c Boris Brezillon  2016-05-24  5094
> static int nand_flash_detect_onfi(struct nand_chip *chip)
> 6fb277ba8 drivers/mtd/nand/nand_base.c Florian Fainelli 2010-09-01  5095  
> {
> cbe435a18 drivers/mtd/nand/nand_base.c Boris Brezillon  2016-05-24  5096
>   struct mtd_info *mtd = nand_to_mtd(chip);
> bd0b64340 drivers/mtd/nand/raw/nand_base.c Miquel Raynal2018-03-19
> 5097  struct nand_onfi_params *p;
> 97d90da8a drivers/mtd/nand/nand_base.c Boris Brezillon  2017-11-30  5098
>   char id[4];
> 2c5f4f892 drivers/mtd/nand/raw/nand_base.c Jane Wan 2018-05-04  5099
>   int i, ret, val, pagesize;
> 2c5f4f892 drivers/mtd/nand/raw/nand_base.c Jane Wan 2018-05-04  5100
>   u8 *buf;
> 6fb277ba8 drivers/mtd/nand/nand_base.c Florian Fainelli 2010-09-01  5101
> 7854d3f74 drivers/mtd/nand/nand_base.c Brian Norris 2011-06-23  5102
>   /* Try ONFI for unknown chip or LP */
> 97d90da8a drivers/mtd/nand/nand_base.c Boris Brezillon  2017-11-30  5103
>   ret = nand_readid_op(chip, 0x20, id, sizeof(id));
> 97d90da8a drivers/mtd/nand/nand_base.c Boris Brezillon  2017-11-30  5104
>   if (ret || strncmp(id, "ONFI", 4))
> 97d90da8a drivers/mtd/nand/nand_base.c Boris Brezillon  2017-11-30  5105
>   return 0;
> 97d90da8a drivers/mtd/nand/nand_base.c Boris Brezillon  2017-11-30  5106
> bd0b64340 drivers/mtd/nand/raw/nand_base.c Miquel Raynal2018-03-19
> 5107  /* ONFI chip: allocate a buffer to hold its parameter page */
> 2c5f4f892 drivers/mtd/nand/raw/nand_base.c Jane Wan 2018-05-04  5108
>   pagesize = sizeof(*p);
> 2c5f4f892 drivers/mtd/nand/raw/nand_base.c Jane Wan 2018-05-04  5109
>   buf = kzalloc((pagesize * 3), GFP_KERNEL);
> 2c5f4f892 drivers/mtd/nand/raw/nand_base.c Jane Wan 2018-05-04  5110
>   if (!buf)
> bd0b64340 drivers/mtd/nand/raw/nand_base.c Miquel Raynal2018-03-19
> 5

RE: [PATCH v2 2/2] mtd: rawnand: fsl_ifc: use bit-wise majority to recover the contents of ONFI parameter

[Wan, Jane (Nokia - US/Sunnyvale)]

Hi,

I've sent v3 of this patch through git send-email.  Please also see the inline 
answer.

Thanks.
Jane

> -Original Message-
> From: kbuild test robot [mailto:l...@intel.com]
> Sent: Saturday, May 05, 2018 1:59 AM
> To: Wan, Jane (Nokia - US/Sunnyvale) 
> Cc: kbuild-...@01.org; boris.brezil...@bootlin.com;
> miquel.ray...@bootlin.com; dw...@infradead.org;
> computersforpe...@gmail.com; rich...@nod.at; marek.va...@gmail.com;
> yamada.masah...@socionext.com; prabhakar.kushw...@nxp.com;
> shawn...@kernel.org; jagdish.ged...@nxp.com;
> shreeya.patel23...@gmail.com; linux-...@lists.infradead.org; linux-
> ker...@vger.kernel.org; Bos, Ties (Nokia - US/Sunnyvale)
> ; Wan, Jane (Nokia - US/Sunnyvale)
> 
> Subject: Re: [PATCH v2 2/2] mtd: rawnand: fsl_ifc: use bit-wise majority to
> recover the contents of ONFI parameter
> 
> Hi Jane,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on mtd/nand/next] [also build test WARNING on
> v4.17-rc3 next-20180504] [if your patch is applied to the wrong git tree, 
> please
> drop us a note to help improve the system]
> 
> url:https://github.com/0day-ci/linux/commits/Jane-Wan/mtd-rawnand-
> fsl_ifc-fix-FSL-NAND-driver-to-read-all-ONFI-parameter-pages/20180505-
> 163132
> base:   git://git.infradead.org/linux-mtd.git nand/next
> config: x86_64-randconfig-x015-201817 (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64
> 
> Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
> http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
> 
> All warnings (new ones prefixed by >>):
> 
>drivers/mtd/nand/raw/nand_base.c: In function 'nand_scan_ident':
> >> drivers/mtd/nand/raw/nand_base.c:5247:2: warning: 'p' may be used
> >> uninitialized in this function [-Wmaybe-uninitialized]
>  kfree(p);
>  ^~~~
>drivers/mtd/nand/raw/nand_base.c:5097:27: note: 'p' was declared here
>  struct nand_onfi_params *p;
>   ^

 [Jane] the kfree() is fixed in v3 patch.  Thanks.
 
> 
> vim +/p +5247 drivers/mtd/nand/raw/nand_base.c
> 
> 2c5f4f892 drivers/mtd/nand/raw/nand_base.c Jane Wan 2018-05-04  5090
> d1e1f4e42 drivers/mtd/nand/nand_base.c Florian Fainelli 2010-08-30  5091
> /*
> 8b6e50c9e drivers/mtd/nand/nand_base.c Brian Norris 2011-05-25  5092  
>  *
> Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
> 6fb277ba8 drivers/mtd/nand/nand_base.c Florian Fainelli 2010-09-01  5093
> */
> 29a198a15 drivers/mtd/nand/nand_base.c Boris Brezillon  2016-05-24  5094
> static int nand_flash_detect_onfi(struct nand_chip *chip)
> 6fb277ba8 drivers/mtd/nand/nand_base.c Florian Fainelli 2010-09-01  5095  
> {
> cbe435a18 drivers/mtd/nand/nand_base.c Boris Brezillon  2016-05-24  5096
>   struct mtd_info *mtd = nand_to_mtd(chip);
> bd0b64340 drivers/mtd/nand/raw/nand_base.c Miquel Raynal2018-03-19
> 5097  struct nand_onfi_params *p;
> 97d90da8a drivers/mtd/nand/nand_base.c Boris Brezillon  2017-11-30  5098
>   char id[4];
> 2c5f4f892 drivers/mtd/nand/raw/nand_base.c Jane Wan 2018-05-04  5099
>   int i, ret, val, pagesize;
> 2c5f4f892 drivers/mtd/nand/raw/nand_base.c Jane Wan 2018-05-04  5100
>   u8 *buf;
> 6fb277ba8 drivers/mtd/nand/nand_base.c Florian Fainelli 2010-09-01  5101
> 7854d3f74 drivers/mtd/nand/nand_base.c Brian Norris 2011-06-23  5102
>   /* Try ONFI for unknown chip or LP */
> 97d90da8a drivers/mtd/nand/nand_base.c Boris Brezillon  2017-11-30  5103
>   ret = nand_readid_op(chip, 0x20, id, sizeof(id));
> 97d90da8a drivers/mtd/nand/nand_base.c Boris Brezillon  2017-11-30  5104
>   if (ret || strncmp(id, "ONFI", 4))
> 97d90da8a drivers/mtd/nand/nand_base.c Boris Brezillon  2017-11-30  5105
>   return 0;
> 97d90da8a drivers/mtd/nand/nand_base.c Boris Brezillon  2017-11-30  5106
> bd0b64340 drivers/mtd/nand/raw/nand_base.c Miquel Raynal2018-03-19
> 5107  /* ONFI chip: allocate a buffer to hold its parameter page */
> 2c5f4f892 drivers/mtd/nand/raw/nand_base.c Jane Wan 2018-05-04  5108
>   pagesize = sizeof(*p);
> 2c5f4f892 drivers/mtd/nand/raw/nand_base.c Jane Wan 2018-05-04  5109
>   buf = kzalloc((pagesize * 3), GFP_KERNEL);
> 2c5f4f892 drivers/mtd/nand/raw/nand_base.c Jane Wan 2018-05-04  5110
>   if (!buf)
> bd0b64340 drivers/mtd/nand/raw/nand_base.c Miquel Raynal2018-03-19
> 5111  return -ENOMEM;
> bd0b64340 drivers/mtd/nand/raw/nand_

[PATCH v2 2/2] mtd: rawnand: fsl_ifc: use bit-wise majority to

[Wan, Jane (Nokia - US/Sunnyvale)]

Hi,

The following is the reposting of patch with v2 version indication based on 
comment on "[PATCH 1/2]" (also in the attachment).

Subject: [PATCH v2 2/2] mtd: rawnand: fsl_ifc: use bit-wise majority to
 recover the contents of ONFI parameter

Per ONFI specification (Rev. 4.0), if all parameter pages have invalid
CRC values, the bit-wise majority may be used to recover the contents of
the parameter pages from the parameter page copies present.

Signed-off-by: Jane Wan <jane@nokia.com>
---
 drivers/mtd/nand/raw/nand_base.c |   36 ++--
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 72f3a89..464c4fb 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -5086,6 +5086,8 @@ static int nand_flash_detect_ext_param_page(struct 
nand_chip *chip,
return ret;
 }
 
+#define GET_BIT(bit, val)   (((val) >> (bit)) & 0x01)
+
 /*
  * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
  */
@@ -5094,7 +5096,8 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
struct mtd_info *mtd = nand_to_mtd(chip);
struct nand_onfi_params *p;
char id[4];
-   int i, ret, val;
+   int i, ret, val, pagesize;
+   u8 *buf;
 
/* Try ONFI for unknown chip or LP */
ret = nand_readid_op(chip, 0x20, id, sizeof(id));
@@ -5102,8 +5105,9 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
return 0;
 
/* ONFI chip: allocate a buffer to hold its parameter page */
-   p = kzalloc(sizeof(*p), GFP_KERNEL);
-   if (!p)
+   pagesize = sizeof(*p);
+   buf = kzalloc((pagesize * 3), GFP_KERNEL);
+   if (!buf)
return -ENOMEM;
 
ret = nand_read_param_page_op(chip, 0, NULL, 0);
@@ -5113,7 +5117,8 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
}
 
for (i = 0; i < 3; i++) {
-   ret = nand_read_data_op(chip, p, sizeof(*p), true);
+   p = (struct nand_onfi_params *)[i*pagesize];
+   ret = nand_read_data_op(chip, p, pagesize, true);
if (ret) {
ret = 0;
goto free_onfi_param_page;
@@ -5126,8 +5131,27 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
}
 
if (i == 3) {
-   pr_err("Could not find valid ONFI parameter page; aborting\n");
-   goto free_onfi_param_page;
+   int j, k, l;
+   u8 v, m;
+
+   pr_err("Could not find valid ONFI parameter page\n");
+   pr_info("Recover ONFI params with bit-wise majority\n");
+   for (j = 0; j < pagesize; j++) {
+   v = 0;
+   for (k = 0; k < 8; k++) {
+   m = 0;
+   for (l = 0; l < 3; l++)
+   m += GET_BIT(k, buf[l*pagesize + j]);
+   if (m > 1)
+   v |= BIT(k);
+   }
+   ((u8 *)p)[j] = v;
+   }
+   if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) !=
+   le16_to_cpu(p->crc)) {
+   pr_err("ONFI parameter recovery failed, aborting\n");
+   goto free_onfi_param_page;
+   }
}
 
/* Check version */
-- 
1.7.9.5

Thanks.
Jane

> -Original Message-
> From: Boris Brezillon [mailto:boris.brezil...@bootlin.com]
> Sent: Wednesday, May 02, 2018 3:32 AM
> To: Wan, Jane (Nokia - US/Sunnyvale) <jane@nokia.com>
> Cc: dw...@infradead.org; computersforpe...@gmail.com; Bos, Ties (Nokia -
> US/Sunnyvale) <ties@nokia.com>; linux-...@lists.infradead.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH 2/2] Use bit-wise majority to recover the contents of ONFI
> parameter
> 
> On Wed, 2 May 2018 12:25:45 +0200
> Boris Brezillon <boris.brezil...@bootlin.com> wrote:
> 
> > Hi Jane,
> >
> > On Thu, 26 Apr 2018 17:19:56 -0700
> > Jane Wan <jane@nokia.com> wrote:
> >
> > > Signed-off-by: Jane Wan <jane@nokia.com>
> > > ---
> > >  drivers/mtd/nand/nand_base.c |   35 +++-
> ---
> > >  1 file changed, 31 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/mtd/nand/nand_base.c
> > > b/drivers/mtd/nand/nand_base.c index c2e1232..161b523 100644
> > > --- a/drivers/mtd/nand/nand_base.c
> > > +++ b/drivers/mtd/nand/nand_base.c
> > > @@ -3153,8 +3153,10 @@ static int nand_flash_detect_onfi(struct
> mtd_info *mtd, st

[PATCH v2 2/2] mtd: rawnand: fsl_ifc: use bit-wise majority to

[Wan, Jane (Nokia - US/Sunnyvale)]

Hi,

The following is the reposting of patch with v2 version indication based on 
comment on "[PATCH 1/2]" (also in the attachment).

Subject: [PATCH v2 2/2] mtd: rawnand: fsl_ifc: use bit-wise majority to
 recover the contents of ONFI parameter

Per ONFI specification (Rev. 4.0), if all parameter pages have invalid
CRC values, the bit-wise majority may be used to recover the contents of
the parameter pages from the parameter page copies present.

Signed-off-by: Jane Wan 
---
 drivers/mtd/nand/raw/nand_base.c |   36 ++--
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 72f3a89..464c4fb 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -5086,6 +5086,8 @@ static int nand_flash_detect_ext_param_page(struct 
nand_chip *chip,
return ret;
 }
 
+#define GET_BIT(bit, val)   (((val) >> (bit)) & 0x01)
+
 /*
  * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
  */
@@ -5094,7 +5096,8 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
struct mtd_info *mtd = nand_to_mtd(chip);
struct nand_onfi_params *p;
char id[4];
-   int i, ret, val;
+   int i, ret, val, pagesize;
+   u8 *buf;
 
/* Try ONFI for unknown chip or LP */
ret = nand_readid_op(chip, 0x20, id, sizeof(id));
@@ -5102,8 +5105,9 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
return 0;
 
/* ONFI chip: allocate a buffer to hold its parameter page */
-   p = kzalloc(sizeof(*p), GFP_KERNEL);
-   if (!p)
+   pagesize = sizeof(*p);
+   buf = kzalloc((pagesize * 3), GFP_KERNEL);
+   if (!buf)
return -ENOMEM;
 
ret = nand_read_param_page_op(chip, 0, NULL, 0);
@@ -5113,7 +5117,8 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
}
 
for (i = 0; i < 3; i++) {
-   ret = nand_read_data_op(chip, p, sizeof(*p), true);
+   p = (struct nand_onfi_params *)[i*pagesize];
+   ret = nand_read_data_op(chip, p, pagesize, true);
if (ret) {
ret = 0;
goto free_onfi_param_page;
@@ -5126,8 +5131,27 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
}
 
if (i == 3) {
-   pr_err("Could not find valid ONFI parameter page; aborting\n");
-   goto free_onfi_param_page;
+   int j, k, l;
+   u8 v, m;
+
+   pr_err("Could not find valid ONFI parameter page\n");
+   pr_info("Recover ONFI params with bit-wise majority\n");
+   for (j = 0; j < pagesize; j++) {
+   v = 0;
+   for (k = 0; k < 8; k++) {
+   m = 0;
+   for (l = 0; l < 3; l++)
+   m += GET_BIT(k, buf[l*pagesize + j]);
+   if (m > 1)
+   v |= BIT(k);
+   }
+   ((u8 *)p)[j] = v;
+   }
+   if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) !=
+   le16_to_cpu(p->crc)) {
+   pr_err("ONFI parameter recovery failed, aborting\n");
+   goto free_onfi_param_page;
+   }
}
 
/* Check version */
-- 
1.7.9.5

Thanks.
Jane

> -Original Message-
> From: Boris Brezillon [mailto:boris.brezil...@bootlin.com]
> Sent: Wednesday, May 02, 2018 3:32 AM
> To: Wan, Jane (Nokia - US/Sunnyvale) 
> Cc: dw...@infradead.org; computersforpe...@gmail.com; Bos, Ties (Nokia -
> US/Sunnyvale) ; linux-...@lists.infradead.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH 2/2] Use bit-wise majority to recover the contents of ONFI
> parameter
> 
> On Wed, 2 May 2018 12:25:45 +0200
> Boris Brezillon  wrote:
> 
> > Hi Jane,
> >
> > On Thu, 26 Apr 2018 17:19:56 -0700
> > Jane Wan  wrote:
> >
> > > Signed-off-by: Jane Wan 
> > > ---
> > >  drivers/mtd/nand/nand_base.c |   35 +++-
> ---
> > >  1 file changed, 31 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/mtd/nand/nand_base.c
> > > b/drivers/mtd/nand/nand_base.c index c2e1232..161b523 100644
> > > --- a/drivers/mtd/nand/nand_base.c
> > > +++ b/drivers/mtd/nand/nand_base.c
> > > @@ -3153,8 +3153,10 @@ static int nand_flash_detect_onfi(struct
> mtd_info *mtd, struct nand_chip *chip,
> > >   int *busw)
> > >  {
> > >   struct nand_onfi_params *p = >onfi_

[PATCH v2 1/2] mtd: rawnand: fsl_ifc: fix FSL NAND driver to read

[Wan, Jane (Nokia - US/Sunnyvale)]

Hi Miquel and Boris,

Thank you for your response.  The following is the reposting the v2 version of 
the patch (also in the attachment).

Subject: [PATCH v2 1/2] mtd: rawnand: fsl_ifc: fix FSL NAND driver to read
 all ONFI parameter pages

Per ONFI specification (Rev. 4.0), if the CRC of the first parameter page
read is not valid, the host should read redundant parameter page copies.
Fix FSL NAND driver to read the two redundant copies which are mandatory
in the specification.

Signed-off-by: Jane Wan <jane@nokia.com>
---
 drivers/mtd/nand/raw/fsl_ifc_nand.c |   17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nand/raw/fsl_ifc_nand.c 
b/drivers/mtd/nand/raw/fsl_ifc_nand.c
index 61aae02..98aac1f 100644
--- a/drivers/mtd/nand/raw/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/raw/fsl_ifc_nand.c
@@ -342,9 +342,16 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned 
int command,
 
case NAND_CMD_READID:
case NAND_CMD_PARAM: {
+   /*
+* For READID, read 8 bytes that are currently used.
+* For PARAM, read all 3 copies of 256-bytes pages.
+*/
+   int len = 8;
int timing = IFC_FIR_OP_RB;
-   if (command == NAND_CMD_PARAM)
+   if (command == NAND_CMD_PARAM) {
timing = IFC_FIR_OP_RBCD;
+   len = 256 * 3;
+   }
 
ifc_out32((IFC_FIR_OP_CW0 << IFC_NAND_FIR0_OP0_SHIFT) |
  (IFC_FIR_OP_UA  << IFC_NAND_FIR0_OP1_SHIFT) |
@@ -354,12 +361,8 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned 
int command,
  >ifc_nand.nand_fcr0);
ifc_out32(column, >ifc_nand.row3);
 
-   /*
-* although currently it's 8 bytes for READID, we always read
-* the maximum 256 bytes(for PARAM)
-*/
-   ifc_out32(256, >ifc_nand.nand_fbcr);
-   ifc_nand_ctrl->read_bytes = 256;
+   ifc_out32(len, >ifc_nand.nand_fbcr);
+   ifc_nand_ctrl->read_bytes = len;
 
set_addr(mtd, 0, 0, 0);
fsl_ifc_run_command(mtd);
-- 
1.7.9.5

Best regards,
Jane

> -Original Message-
> From: Miquel Raynal [mailto:miquel.ray...@bootlin.com]
> Sent: Wednesday, May 02, 2018 1:10 AM
> To: Wan, Jane (Nokia - US/Sunnyvale) <jane@nokia.com>
> Cc: Boris Brezillon <boris.brezil...@bootlin.com>; dw...@infradead.org;
> computersforpe...@gmail.com; Bos, Ties (Nokia - US/Sunnyvale)
> <ties@nokia.com>; linux-...@lists.infradead.org; linux-
> ker...@vger.kernel.org; rich...@nod.at; marek.va...@gmail.com;
> yamada.masah...@socionext.com; prabhakar.kushw...@nxp.com;
> shawn...@kernel.org; jagdish.ged...@nxp.com;
> shreeya.patel23...@gmail.com
> Subject: Re: [PATCH 1/2] Fix FSL NAND driver to read all ONFI parameter pages
> 
> Hi Jane,
> 
> On Tue, 1 May 2018 05:01:23 +, "Wan, Jane (Nokia - US/Sunnyvale)"
> <jane@nokia.com> wrote:
> 
> > Hi Miquèl and Boris,
> >
> > Thank you for your response and feedback.  I've modified the fix based on 
> > your
> comments.
> > Please see the updated patch file at the end of this message (also in
> attachment).
> > My answers to your comments/questions are inline in the previous message.
> 
> Usually we answer inline in each e-mail, then we post a new version with a
> version counter incremented (use the '-v2- of 'git format-patch'
> to add the '[PATCH v2 x/y]' prefix automatically). Reposting is important as
> maintainers use 'patchwork' to follow the evolution of each patch. So in your
> case, nothing shows that you posted a v2.

 [Jane] Thank you for the info.  I'm reposting the v2 version of the patch.

> 
> >
> > Here is the answer to Boris question in another email thread:
> >
> > > What if some NANDs have 4 or more copies of the param page?
> >  [Jane] The ONFI spec defines that the parameter page and its two redundant
> copies are mandatory.
> > The additional redundant pages are optional.  Currently, the FSL NAND
> > driver only reads the first parameter page.  This patch is to fix the 
> > driver to
> meet the mandatory requirement in the spec.
> > We got a batch of particularly bad NAND chips recently and we needed
> > these changes to make them work reliably over temperature.  The patch was
> verified using these bad chips.
> 
> I think Boris' remark was much more general than just this use case.
> The whole logic of tailoring ->cmdfunc() in each driver by guessing the data
> length, while there is absolutely no indication of it in this hook is broken. 
> The
> core is moving to a new interf

[PATCH v2 1/2] mtd: rawnand: fsl_ifc: fix FSL NAND driver to read

[Wan, Jane (Nokia - US/Sunnyvale)]

Hi Miquel and Boris,

Thank you for your response.  The following is the reposting the v2 version of 
the patch (also in the attachment).

Subject: [PATCH v2 1/2] mtd: rawnand: fsl_ifc: fix FSL NAND driver to read
 all ONFI parameter pages

Per ONFI specification (Rev. 4.0), if the CRC of the first parameter page
read is not valid, the host should read redundant parameter page copies.
Fix FSL NAND driver to read the two redundant copies which are mandatory
in the specification.

Signed-off-by: Jane Wan 
---
 drivers/mtd/nand/raw/fsl_ifc_nand.c |   17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nand/raw/fsl_ifc_nand.c 
b/drivers/mtd/nand/raw/fsl_ifc_nand.c
index 61aae02..98aac1f 100644
--- a/drivers/mtd/nand/raw/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/raw/fsl_ifc_nand.c
@@ -342,9 +342,16 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned 
int command,
 
case NAND_CMD_READID:
case NAND_CMD_PARAM: {
+   /*
+* For READID, read 8 bytes that are currently used.
+* For PARAM, read all 3 copies of 256-bytes pages.
+*/
+   int len = 8;
int timing = IFC_FIR_OP_RB;
-   if (command == NAND_CMD_PARAM)
+   if (command == NAND_CMD_PARAM) {
timing = IFC_FIR_OP_RBCD;
+   len = 256 * 3;
+   }
 
ifc_out32((IFC_FIR_OP_CW0 << IFC_NAND_FIR0_OP0_SHIFT) |
  (IFC_FIR_OP_UA  << IFC_NAND_FIR0_OP1_SHIFT) |
@@ -354,12 +361,8 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned 
int command,
  >ifc_nand.nand_fcr0);
ifc_out32(column, >ifc_nand.row3);
 
-   /*
-* although currently it's 8 bytes for READID, we always read
-* the maximum 256 bytes(for PARAM)
-*/
-   ifc_out32(256, >ifc_nand.nand_fbcr);
-   ifc_nand_ctrl->read_bytes = 256;
+   ifc_out32(len, >ifc_nand.nand_fbcr);
+   ifc_nand_ctrl->read_bytes = len;
 
set_addr(mtd, 0, 0, 0);
fsl_ifc_run_command(mtd);
-- 
1.7.9.5

Best regards,
Jane

> -Original Message-
> From: Miquel Raynal [mailto:miquel.ray...@bootlin.com]
> Sent: Wednesday, May 02, 2018 1:10 AM
> To: Wan, Jane (Nokia - US/Sunnyvale) 
> Cc: Boris Brezillon ; dw...@infradead.org;
> computersforpe...@gmail.com; Bos, Ties (Nokia - US/Sunnyvale)
> ; linux-...@lists.infradead.org; linux-
> ker...@vger.kernel.org; rich...@nod.at; marek.va...@gmail.com;
> yamada.masah...@socionext.com; prabhakar.kushw...@nxp.com;
> shawn...@kernel.org; jagdish.ged...@nxp.com;
> shreeya.patel23...@gmail.com
> Subject: Re: [PATCH 1/2] Fix FSL NAND driver to read all ONFI parameter pages
> 
> Hi Jane,
> 
> On Tue, 1 May 2018 05:01:23 +, "Wan, Jane (Nokia - US/Sunnyvale)"
>  wrote:
> 
> > Hi Miquèl and Boris,
> >
> > Thank you for your response and feedback.  I've modified the fix based on 
> > your
> comments.
> > Please see the updated patch file at the end of this message (also in
> attachment).
> > My answers to your comments/questions are inline in the previous message.
> 
> Usually we answer inline in each e-mail, then we post a new version with a
> version counter incremented (use the '-v2- of 'git format-patch'
> to add the '[PATCH v2 x/y]' prefix automatically). Reposting is important as
> maintainers use 'patchwork' to follow the evolution of each patch. So in your
> case, nothing shows that you posted a v2.

 [Jane] Thank you for the info.  I'm reposting the v2 version of the patch.

> 
> >
> > Here is the answer to Boris question in another email thread:
> >
> > > What if some NANDs have 4 or more copies of the param page?
> >  [Jane] The ONFI spec defines that the parameter page and its two redundant
> copies are mandatory.
> > The additional redundant pages are optional.  Currently, the FSL NAND
> > driver only reads the first parameter page.  This patch is to fix the 
> > driver to
> meet the mandatory requirement in the spec.
> > We got a batch of particularly bad NAND chips recently and we needed
> > these changes to make them work reliably over temperature.  The patch was
> verified using these bad chips.
> 
> I think Boris' remark was much more general than just this use case.
> The whole logic of tailoring ->cmdfunc() in each driver by guessing the data
> length, while there is absolutely no indication of it in this hook is broken. 
> The
> core is moving to a new interface called ->exec_op(), and we would welcome a
> migration of this driver to this hook, that would be much much more easy to
> ma

RE: [PATCH 2/2] Use bit-wise majority to recover the contents of ONFI parameter

[Wan, Jane (Nokia - US/Sunnyvale)]

Hi Miquèl,

Thank you for your response and feedback.  I've modified the fix based on your 
comments.  
Please see the updated patch file at the end of this message (also in 
attachment).
My answers to your comments/questions are inline in the previous message.

The new patch is rebased on top of v4.17-rc1.

Best regards,
Jane

Updated patch:
From e14ed7dc08296a52f81d14781dee2f455dd90bbd Mon Sep 17 00:00:00 2001
From: Jane Wan <jane@nokia.com>
Date: Mon, 30 Apr 2018 14:05:40 -0700
Subject: [PATCH 2/2] mtd: rawnand: fsl_ifc: use bit-wise majority to recover
 the contents of ONFI parameter

Per ONFI specification (Rev. 4.0), if all parameter pages have invalid
CRC values, the bit-wise majority may be used to recover the contents of
the parameter pages from the parameter page copies present.

Signed-off-by: Jane Wan <jane@nokia.com>
---
 drivers/mtd/nand/raw/nand_base.c |   36 ++--
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 72f3a89..464c4fb 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -5086,6 +5086,8 @@ static int nand_flash_detect_ext_param_page(struct 
nand_chip *chip,
return ret;
 }
 
+#define GET_BIT(bit, val)   (((val) >> (bit)) & 0x01)
+
 /*
  * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
  */
@@ -5094,7 +5096,8 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
struct mtd_info *mtd = nand_to_mtd(chip);
struct nand_onfi_params *p;
char id[4];
-   int i, ret, val;
+   int i, ret, val, pagesize;
+   u8 *buf;
 
/* Try ONFI for unknown chip or LP */
ret = nand_readid_op(chip, 0x20, id, sizeof(id));
@@ -5102,8 +5105,9 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
return 0;
 
/* ONFI chip: allocate a buffer to hold its parameter page */
-   p = kzalloc(sizeof(*p), GFP_KERNEL);
-   if (!p)
+   pagesize = sizeof(*p);
+   buf = kzalloc((pagesize * 3), GFP_KERNEL);
+   if (!buf)
return -ENOMEM;
 
ret = nand_read_param_page_op(chip, 0, NULL, 0);
@@ -5113,7 +5117,8 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
}
 
for (i = 0; i < 3; i++) {
-   ret = nand_read_data_op(chip, p, sizeof(*p), true);
+   p = (struct nand_onfi_params *)[i*pagesize];
+   ret = nand_read_data_op(chip, p, pagesize, true);
if (ret) {
ret = 0;
goto free_onfi_param_page;
@@ -5126,8 +5131,27 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
}
 
if (i == 3) {
-   pr_err("Could not find valid ONFI parameter page; aborting\n");
-   goto free_onfi_param_page;
+   int j, k, l;
+   u8 v, m;
+
+   pr_err("Could not find valid ONFI parameter page\n");
+   pr_info("Recover ONFI params with bit-wise majority\n");
+   for (j = 0; j < pagesize; j++) {
+   v = 0;
+   for (k = 0; k < 8; k++) {
+   m = 0;
+   for (l = 0; l < 3; l++)
+   m += GET_BIT(k, buf[l*pagesize + j]);
+   if (m > 1)
+   v |= BIT(k);
+   }
+   ((u8 *)p)[j] = v;
+   }
+   if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) !=
+   le16_to_cpu(p->crc)) {
+   pr_err("ONFI parameter recovery failed, aborting\n");
+   goto free_onfi_param_page;
+   }
}
 
/* Check version */
-- 
1.7.9.5

-Original Message-
From: Miquel Raynal [mailto:miquel.ray...@bootlin.com] 
Sent: Saturday, April 28, 2018 5:07 AM
To: Wan, Jane (Nokia - US/Sunnyvale) <jane@nokia.com>
Cc: dw...@infradead.org; computersforpe...@gmail.com; Bos, Ties (Nokia - 
US/Sunnyvale) <ties@nokia.com>; linux-...@lists.infradead.org; 
linux-kernel@vger.kernel.org; Boris Brezillon <boris.brezil...@bootlin.com>
Subject: Re: [PATCH 2/2] Use bit-wise majority to recover the contents of ONFI 
parameter

Hi Jane,

Same comments as before, please: get the right maintainers, add a commit log, 
rebase and fix the title prefix.
[Jane]  Added.  Thanks.

Have you ever needed/tried this algorithm before?
[Jane] Yes, we got a batch of particularly bad NAND chips recently and we 
needed these changes to make them work reliably over temperature.  The patch 
was verified using these bad chips.

On Thu, 26 Apr 2018 17:19:56 -0700, Jane Wan <jane@nokia.com> wrote:

> Signed-off-by: Jane Wan <

RE: [PATCH 2/2] Use bit-wise majority to recover the contents of ONFI parameter

[Wan, Jane (Nokia - US/Sunnyvale)]

Hi Miquèl,

Thank you for your response and feedback.  I've modified the fix based on your 
comments.  
Please see the updated patch file at the end of this message (also in 
attachment).
My answers to your comments/questions are inline in the previous message.

The new patch is rebased on top of v4.17-rc1.

Best regards,
Jane

Updated patch:
From e14ed7dc08296a52f81d14781dee2f455dd90bbd Mon Sep 17 00:00:00 2001
From: Jane Wan 
Date: Mon, 30 Apr 2018 14:05:40 -0700
Subject: [PATCH 2/2] mtd: rawnand: fsl_ifc: use bit-wise majority to recover
 the contents of ONFI parameter

Per ONFI specification (Rev. 4.0), if all parameter pages have invalid
CRC values, the bit-wise majority may be used to recover the contents of
the parameter pages from the parameter page copies present.

Signed-off-by: Jane Wan 
---
 drivers/mtd/nand/raw/nand_base.c |   36 ++--
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 72f3a89..464c4fb 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -5086,6 +5086,8 @@ static int nand_flash_detect_ext_param_page(struct 
nand_chip *chip,
return ret;
 }
 
+#define GET_BIT(bit, val)   (((val) >> (bit)) & 0x01)
+
 /*
  * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
  */
@@ -5094,7 +5096,8 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
struct mtd_info *mtd = nand_to_mtd(chip);
struct nand_onfi_params *p;
char id[4];
-   int i, ret, val;
+   int i, ret, val, pagesize;
+   u8 *buf;
 
/* Try ONFI for unknown chip or LP */
ret = nand_readid_op(chip, 0x20, id, sizeof(id));
@@ -5102,8 +5105,9 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
return 0;
 
/* ONFI chip: allocate a buffer to hold its parameter page */
-   p = kzalloc(sizeof(*p), GFP_KERNEL);
-   if (!p)
+   pagesize = sizeof(*p);
+   buf = kzalloc((pagesize * 3), GFP_KERNEL);
+   if (!buf)
return -ENOMEM;
 
ret = nand_read_param_page_op(chip, 0, NULL, 0);
@@ -5113,7 +5117,8 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
}
 
for (i = 0; i < 3; i++) {
-   ret = nand_read_data_op(chip, p, sizeof(*p), true);
+   p = (struct nand_onfi_params *)[i*pagesize];
+   ret = nand_read_data_op(chip, p, pagesize, true);
if (ret) {
ret = 0;
goto free_onfi_param_page;
@@ -5126,8 +5131,27 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
}
 
if (i == 3) {
-   pr_err("Could not find valid ONFI parameter page; aborting\n");
-   goto free_onfi_param_page;
+   int j, k, l;
+   u8 v, m;
+
+   pr_err("Could not find valid ONFI parameter page\n");
+   pr_info("Recover ONFI params with bit-wise majority\n");
+   for (j = 0; j < pagesize; j++) {
+   v = 0;
+   for (k = 0; k < 8; k++) {
+   m = 0;
+   for (l = 0; l < 3; l++)
+   m += GET_BIT(k, buf[l*pagesize + j]);
+   if (m > 1)
+   v |= BIT(k);
+   }
+   ((u8 *)p)[j] = v;
+   }
+   if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) !=
+   le16_to_cpu(p->crc)) {
+   pr_err("ONFI parameter recovery failed, aborting\n");
+   goto free_onfi_param_page;
+   }
}
 
/* Check version */
-- 
1.7.9.5

-Original Message-
From: Miquel Raynal [mailto:miquel.ray...@bootlin.com] 
Sent: Saturday, April 28, 2018 5:07 AM
To: Wan, Jane (Nokia - US/Sunnyvale) 
Cc: dw...@infradead.org; computersforpe...@gmail.com; Bos, Ties (Nokia - 
US/Sunnyvale) ; linux-...@lists.infradead.org; 
linux-kernel@vger.kernel.org; Boris Brezillon 
Subject: Re: [PATCH 2/2] Use bit-wise majority to recover the contents of ONFI 
parameter

Hi Jane,

Same comments as before, please: get the right maintainers, add a commit log, 
rebase and fix the title prefix.
[Jane]  Added.  Thanks.

Have you ever needed/tried this algorithm before?
[Jane] Yes, we got a batch of particularly bad NAND chips recently and we 
needed these changes to make them work reliably over temperature.  The patch 
was verified using these bad chips.

On Thu, 26 Apr 2018 17:19:56 -0700, Jane Wan  wrote:

> Signed-off-by: Jane Wan 
> ---
>  drivers/mtd/nand/nand_base.c |   35 +++
>  1 file changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a

RE: [PATCH 1/2] Fix FSL NAND driver to read all ONFI parameter pages

[Wan, Jane (Nokia - US/Sunnyvale)]

Hi Miquèl and Boris,

Thank you for your response and feedback.  I've modified the fix based on your 
comments.  
Please see the updated patch file at the end of this message (also in 
attachment).
My answers to your comments/questions are inline in the previous message.

Here is the answer to Boris question in another email thread:

> What if some NANDs have 4 or more copies of the param page?
 [Jane] The ONFI spec defines that the parameter page and its two redundant 
copies are mandatory.  
The additional redundant pages are optional.  Currently, the FSL NAND driver 
only reads the first 
parameter page.  This patch is to fix the driver to meet the mandatory 
requirement in the spec. 
We got a batch of particularly bad NAND chips recently and we needed these 
changes to make them 
work reliably over temperature.  The patch was verified using these bad chips.

Best regards,
Jane

Updated patch:
From 701de4146aa6355c951e97a77476e12d2da56d42 Mon Sep 17 00:00:00 2001
From: Jane Wan <jane@nokia.com>
Date: Mon, 30 Apr 2018 13:30:46 -0700
Subject: [PATCH 1/2] mtd: rawnand: fsl_ifc: fix FSL NAND driver to read all
 ONFI parameter pages

Per ONFI specification (Rev. 4.0), if the CRC of the first parameter page
read is not valid, the host should read redundant parameter page copies.
Fix FSL NAND driver to read the two redundant copies which are mandatory
in the specification.

Signed-off-by: Jane Wan <jane@nokia.com>
---
 drivers/mtd/nand/raw/fsl_ifc_nand.c |   17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nand/raw/fsl_ifc_nand.c 
b/drivers/mtd/nand/raw/fsl_ifc_nand.c
index 61aae02..98aac1f 100644
--- a/drivers/mtd/nand/raw/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/raw/fsl_ifc_nand.c
@@ -342,9 +342,16 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned 
int command,
 
case NAND_CMD_READID:
case NAND_CMD_PARAM: {
+   /*
+* For READID, read 8 bytes that are currently used.
+* For PARAM, read all 3 copies of 256-bytes pages.
+*/
+   int len = 8;
int timing = IFC_FIR_OP_RB;
-   if (command == NAND_CMD_PARAM)
+   if (command == NAND_CMD_PARAM) {
timing = IFC_FIR_OP_RBCD;
+   len = 256 * 3;
+   }
 
ifc_out32((IFC_FIR_OP_CW0 << IFC_NAND_FIR0_OP0_SHIFT) |
  (IFC_FIR_OP_UA  << IFC_NAND_FIR0_OP1_SHIFT) |
@@ -354,12 +361,8 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned 
int command,
  >ifc_nand.nand_fcr0);
ifc_out32(column, >ifc_nand.row3);
 
-   /*
-* although currently it's 8 bytes for READID, we always read
-* the maximum 256 bytes(for PARAM)
-*/
-   ifc_out32(256, >ifc_nand.nand_fbcr);
-   ifc_nand_ctrl->read_bytes = 256;
+   ifc_out32(len, >ifc_nand.nand_fbcr);
+   ifc_nand_ctrl->read_bytes = len;
 
set_addr(mtd, 0, 0, 0);
fsl_ifc_run_command(mtd);
-- 
1.7.9.5


-Original Message-
From: Miquel Raynal [mailto:miquel.ray...@bootlin.com] 
Sent: Saturday, April 28, 2018 4:42 AM
To: Wan, Jane (Nokia - US/Sunnyvale) <jane@nokia.com>
Cc: dw...@infradead.org; computersforpe...@gmail.com; Bos, Ties (Nokia - 
US/Sunnyvale) <ties@nokia.com>; linux-...@lists.infradead.org; 
linux-kernel@vger.kernel.org; Boris Brezillon <boris.brezil...@bootlin.com>
Subject: Re: [PATCH 1/2] Fix FSL NAND driver to read all ONFI parameter pages

Hi Jane,

You forgot to Cc the right maintainers, please use ./scripts/get_maintainer.pl 
for that.
[Jane]  Added through 4.17-rc1 get_maintainer.pl.  I was running the script 
from an older kernel version we're using.

> Signed-off-by: Jane Wan <jane@nokia.com>

Please add a description of what your are doing in the commit message.
The description in the cover letter is good, you can copy the relevant section 
here.
[Jane]  Added.

> ---
>  drivers/mtd/nand/fsl_ifc_nand.c |   10 ++

Also, just for you to know, files have moved in a raw/ subdirectory, so please 
rebase on top of 4.17-rc1 and prefix the commit title with
"mtd: rawnand: fsl_ifc:".
[Jane] Thank you for the info.  I've rebased the change on top of 4.17-rc1 and 
modified the commit title as you suggested.

>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/fsl_ifc_nand.c 
> b/drivers/mtd/nand/fsl_ifc_nand.c index ca36b35..a3cf6ca 100644
> --- a/drivers/mtd/nand/fsl_ifc_nand.c
> +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> @@ -413,6 +413,7 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, 
> unsigned int command,
>   struct fsl_ifc_mtd *priv = chip->priv;
>   struct fsl_ifc_ctrl *ctr

RE: [PATCH 1/2] Fix FSL NAND driver to read all ONFI parameter pages

[Wan, Jane (Nokia - US/Sunnyvale)]

Hi Miquèl and Boris,

Thank you for your response and feedback.  I've modified the fix based on your 
comments.  
Please see the updated patch file at the end of this message (also in 
attachment).
My answers to your comments/questions are inline in the previous message.

Here is the answer to Boris question in another email thread:

> What if some NANDs have 4 or more copies of the param page?
 [Jane] The ONFI spec defines that the parameter page and its two redundant 
copies are mandatory.  
The additional redundant pages are optional.  Currently, the FSL NAND driver 
only reads the first 
parameter page.  This patch is to fix the driver to meet the mandatory 
requirement in the spec. 
We got a batch of particularly bad NAND chips recently and we needed these 
changes to make them 
work reliably over temperature.  The patch was verified using these bad chips.

Best regards,
Jane

Updated patch:
From 701de4146aa6355c951e97a77476e12d2da56d42 Mon Sep 17 00:00:00 2001
From: Jane Wan 
Date: Mon, 30 Apr 2018 13:30:46 -0700
Subject: [PATCH 1/2] mtd: rawnand: fsl_ifc: fix FSL NAND driver to read all
 ONFI parameter pages

Per ONFI specification (Rev. 4.0), if the CRC of the first parameter page
read is not valid, the host should read redundant parameter page copies.
Fix FSL NAND driver to read the two redundant copies which are mandatory
in the specification.

Signed-off-by: Jane Wan 
---
 drivers/mtd/nand/raw/fsl_ifc_nand.c |   17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nand/raw/fsl_ifc_nand.c 
b/drivers/mtd/nand/raw/fsl_ifc_nand.c
index 61aae02..98aac1f 100644
--- a/drivers/mtd/nand/raw/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/raw/fsl_ifc_nand.c
@@ -342,9 +342,16 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned 
int command,
 
case NAND_CMD_READID:
case NAND_CMD_PARAM: {
+   /*
+* For READID, read 8 bytes that are currently used.
+* For PARAM, read all 3 copies of 256-bytes pages.
+*/
+   int len = 8;
int timing = IFC_FIR_OP_RB;
-   if (command == NAND_CMD_PARAM)
+   if (command == NAND_CMD_PARAM) {
timing = IFC_FIR_OP_RBCD;
+   len = 256 * 3;
+   }
 
ifc_out32((IFC_FIR_OP_CW0 << IFC_NAND_FIR0_OP0_SHIFT) |
  (IFC_FIR_OP_UA  << IFC_NAND_FIR0_OP1_SHIFT) |
@@ -354,12 +361,8 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned 
int command,
  >ifc_nand.nand_fcr0);
ifc_out32(column, >ifc_nand.row3);
 
-   /*
-* although currently it's 8 bytes for READID, we always read
-* the maximum 256 bytes(for PARAM)
-*/
-   ifc_out32(256, >ifc_nand.nand_fbcr);
-   ifc_nand_ctrl->read_bytes = 256;
+   ifc_out32(len, >ifc_nand.nand_fbcr);
+   ifc_nand_ctrl->read_bytes = len;
 
set_addr(mtd, 0, 0, 0);
fsl_ifc_run_command(mtd);
-- 
1.7.9.5


-Original Message-
From: Miquel Raynal [mailto:miquel.ray...@bootlin.com] 
Sent: Saturday, April 28, 2018 4:42 AM
To: Wan, Jane (Nokia - US/Sunnyvale) 
Cc: dw...@infradead.org; computersforpe...@gmail.com; Bos, Ties (Nokia - 
US/Sunnyvale) ; linux-...@lists.infradead.org; 
linux-kernel@vger.kernel.org; Boris Brezillon 
Subject: Re: [PATCH 1/2] Fix FSL NAND driver to read all ONFI parameter pages

Hi Jane,

You forgot to Cc the right maintainers, please use ./scripts/get_maintainer.pl 
for that.
[Jane]  Added through 4.17-rc1 get_maintainer.pl.  I was running the script 
from an older kernel version we're using.

> Signed-off-by: Jane Wan 

Please add a description of what your are doing in the commit message.
The description in the cover letter is good, you can copy the relevant section 
here.
[Jane]  Added.

> ---
>  drivers/mtd/nand/fsl_ifc_nand.c |   10 ++

Also, just for you to know, files have moved in a raw/ subdirectory, so please 
rebase on top of 4.17-rc1 and prefix the commit title with
"mtd: rawnand: fsl_ifc:".
[Jane] Thank you for the info.  I've rebased the change on top of 4.17-rc1 and 
modified the commit title as you suggested.

>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/fsl_ifc_nand.c 
> b/drivers/mtd/nand/fsl_ifc_nand.c index ca36b35..a3cf6ca 100644
> --- a/drivers/mtd/nand/fsl_ifc_nand.c
> +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> @@ -413,6 +413,7 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, 
> unsigned int command,
>   struct fsl_ifc_mtd *priv = chip->priv;
>   struct fsl_ifc_ctrl *ctrl = priv->ctrl;
>   struct fsl_ifc_runtime __iomem *ifc = ctrl->rregs;
> + int len;
>  
>   /* clear the read buffer */
>