[PATCH 0/2] Fix fsl_ifc_nand reading ONFI parameters to meet ONFI spec

[PATCH 0/2] Fix fsl_ifc_nand reading ONFI parameters to meet ONFI spec

[Jane Wan]

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
until it finds a valid copy.  If all three 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.

The FSL NAND driver only reads the first page.  The first patch fixes the
driver to read all three parameter pages.  The second patch is the change
for using bit-wise majority to recover the contents of ONFI parameter.

Jane Wan (2):
  Fix FSL NAND driver to read all ONFI parameter pages
  Use bit-wise majority to recover the contents of ONFI parameter

 drivers/mtd/nand/fsl_ifc_nand.c |   10 ++++++----
 drivers/mtd/nand/nand_base.c    |   35 +++++++++++++++++++++++++++++++----
 2 files changed, 37 insertions(+), 8 deletions(-)

-- 
1.7.9.5

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

[Jane Wan]

Signed-off-by: Jane Wan <Jane.Wan@nokia.com>
---
 drivers/mtd/nand/fsl_ifc_nand.c |   10 ++++++----
 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 */
 	ifc_nand_ctrl->read_bytes = 0;
@@ -462,11 +463,12 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command,
 		ifc_out32(column, &ifc->ifc_nand.row3);
 
 		/*
-		 * although currently it's 8 bytes for READID, we always read
-		 * the maximum 256 bytes(for PARAM)
+		 * For READID, read 8 bytes that are currently used.
+		 * For PARAM, read all 3 copies of 256-bytes pages.
 		 */
-		ifc_out32(256, &ifc->ifc_nand.nand_fbcr);
-		ifc_nand_ctrl->read_bytes = 256;
+		len = (command == NAND_CMD_PARAM) ? (3 * 256) : 8;
+		ifc_out32(len, &ifc->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

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

[Jane Wan]

Signed-off-by: Jane Wan <Jane.Wan@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, struct nand_chip *chip,
 					int *busw)
 {
 	struct nand_onfi_params *p = &chip->onfi_params;
-	int i, j;
-	int val;
+	int i, j, k, len, val;
+	uint8_t copy[3][256], v8;
+
+	len = (sizeof(*p) > 256) ? 256 : sizeof(*p);
 
 	/* Try ONFI for unknown chip or LP */
 	chip->cmdfunc(mtd, NAND_CMD_READID, 0x20, -1);
@@ -3170,11 +3172,36 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 				le16_to_cpu(p->crc)) {
 			break;
 		}
+		pr_err("CRC of parameter page %d is not valid\n", i);
+		for (j = 0; j < len; j++)
+			copy[i][j] = ((uint8_t *)p)[j];
 	}
 
 	if (i == 3) {
-		pr_err("Could not find valid ONFI parameter page; aborting\n");
-		return 0;
+		pr_err("Could not find valid ONFI parameter page\n");
+		pr_info("Recover ONFI parameters with bit-wise majority\n");
+		for (j = 0; j < len; j++) {
+			if (copy[0][j] == copy[1][j] ||
+			    copy[0][j] == copy[2][j]) {
+				((uint8_t *)p)[j] = copy[0][j];
+			} else if (copy[1][j] == copy[2][j]) {
+				((uint8_t *)p)[j] = copy[1][j];
+			} else {
+				((uint8_t *)p)[j] = 0;
+				for (k = 0; k < 8; k++) {
+					v8 = (copy[0][j] >> k) & 0x1;
+					v8 += (copy[1][j] >> k) & 0x1;
+					v8 += (copy[2][j] >> k) & 0x1;
+					if (v8 > 1)
+						((uint8_t *)p)[j] |= (1 << k);
+				}
+			}
+		}
+		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) !=
+		    le16_to_cpu(p->crc)) {
+			pr_err("ONFI parameter recovery failed, aborting\n");
+			return 0;
+		}
 	}
 
 	/* Check version */
-- 
1.7.9.5

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

[Miquel Raynal]

Hi Jane,

You forgot to Cc the right maintainers, please
use ./scripts/get_maintainer.pl for that.

> Signed-off-by: Jane Wan <Jane.Wan@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.

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

>  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 */
>  	ifc_nand_ctrl->read_bytes = 0;
> @@ -462,11 +463,12 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command,
>  		ifc_out32(column, &ifc->ifc_nand.row3);
>  
>  		/*
> -		 * although currently it's 8 bytes for READID, we always read
> -		 * the maximum 256 bytes(for PARAM)
> +		 * For READID, read 8 bytes that are currently used.
> +		 * For PARAM, read all 3 copies of 256-bytes pages.
>  		 */
> -		ifc_out32(256, &ifc->ifc_nand.nand_fbcr);
> -		ifc_nand_ctrl->read_bytes = 256;
> +		len = (command == NAND_CMD_PARAM) ? (3 * 256) : 8;

There is already a "command == NAND_CMD_PARAM" condition above, you
might want to use it.

> +		ifc_out32(len, &ifc->ifc_nand.nand_fbcr);
> +		ifc_nand_ctrl->read_bytes = len;
>  
>  		set_addr(mtd, 0, 0, 0);
>  		fsl_ifc_run_command(mtd);

The overall ->cmdfunc() approach of this driver is horrible. However
this fixes its implementation to match the current state of the core,
so I guess it is fine.

Regards,
Miquèl

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

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

[Miquel Raynal]

Hi Jane,

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

Have you ever needed/tried this algorithm before? 

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

> Signed-off-by: Jane Wan <Jane.Wan@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, struct nand_chip *chip,
>  					int *busw)
>  {
>  	struct nand_onfi_params *p = &chip->onfi_params;
> -	int i, j;
> -	int val;
> +	int i, j, k, len, val;
> +	uint8_t copy[3][256], v8;

Please use u8 instead of uint8_t (./scripts/checkpatch.pl --strict will
give you the list of styling issues to fix.

I don't think you should allocate that much space on the stack, please
use dynamic allocation.

> +
> +	len = (sizeof(*p) > 256) ? 256 : sizeof(*p);

This is a maximum derivation, there are helpers for that.

I am not sure this is relevant, won't you read only 256 bytes anyway?

>  
>  	/* Try ONFI for unknown chip or LP */
>  	chip->cmdfunc(mtd, NAND_CMD_READID, 0x20, -1);
> @@ -3170,11 +3172,36 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
>  				le16_to_cpu(p->crc)) {
>  			break;
>  		}

Space.

> +		pr_err("CRC of parameter page %d is not valid\n", i);
> +		for (j = 0; j < len; j++)
> +			copy[i][j] = ((uint8_t *)p)[j];

'copy' is maybe not a meaningful name.

>  	}
>  
>  	if (i == 3) {
> -		pr_err("Could not find valid ONFI parameter page; aborting\n");
> -		return 0;
> +		pr_err("Could not find valid ONFI parameter page\n");
> +		pr_info("Recover ONFI parameters with bit-wise majority\n");
> +		for (j = 0; j < len; j++) {
> +			if (copy[0][j] == copy[1][j] ||
> +			    copy[0][j] == copy[2][j]) {
> +				((uint8_t *)p)[j] = copy[0][j];
> +			} else if (copy[1][j] == copy[2][j]) {
> +				((uint8_t *)p)[j] = copy[1][j];
> +			} else {
> +				((uint8_t *)p)[j] = 0;
> +				for (k = 0; k < 8; k++) {
> +					v8 = (copy[0][j] >> k) & 0x1;

v8 could be declared in the else statement of in the for loop.
The name could also be changed.

> +					v8 += (copy[1][j] >> k) & 0x1;
> +					v8 += (copy[2][j] >> k) & 0x1;
> +					if (v8 > 1)
> +						((uint8_t *)p)[j] |= (1 << k);

Please use the BIT() macro.

> +				}
> +			}
> +		}

Space.

> +		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) !=
> +		    le16_to_cpu(p->crc)) {
> +			pr_err("ONFI parameter recovery failed, aborting\n");
> +			return 0;
> +		}
>  	}
>  
>  	/* Check version */

Thanks,
Miquèl

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

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

[Boris Brezillon]

Hi Jane,

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

> Signed-off-by: Jane Wan <Jane.Wan@nokia.com>
> ---
>  drivers/mtd/nand/fsl_ifc_nand.c |   10 ++++++----
>  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 */
>  	ifc_nand_ctrl->read_bytes = 0;
> @@ -462,11 +463,12 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command,
>  		ifc_out32(column, &ifc->ifc_nand.row3);
>  
>  		/*
> -		 * although currently it's 8 bytes for READID, we always read
> -		 * the maximum 256 bytes(for PARAM)
> +		 * For READID, read 8 bytes that are currently used.
> +		 * For PARAM, read all 3 copies of 256-bytes pages.
>  		 */
> -		ifc_out32(256, &ifc->ifc_nand.nand_fbcr);
> -		ifc_nand_ctrl->read_bytes = 256;
> +		len = (command == NAND_CMD_PARAM) ? (3 * 256) : 8;
> +		ifc_out32(len, &ifc->ifc_nand.nand_fbcr);
> +		ifc_nand_ctrl->read_bytes = len;

This driver really calls for a clean rework to transition to
->exec_op(). Guessing the amount of data to be read from ->cmdfunc() is
broken and your patch series just shows how broken this is. What next?
What if some NANDs have 4 or more copies of the param page?

I'm still undecided whether I want to apply this patch. I guess having
some guarantees that someone will actually work on implementing
->exec_op() in a near future would help me take this decision.

Regards,

Boris

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

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

[-- Attachment #1: Type: text/plain, Size: 5887 bytes --]

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.Wan@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.Wan@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->ifc_nand.nand_fcr0);
 		ifc_out32(column, &ifc->ifc_nand.row3);
 
-		/*
-		 * although currently it's 8 bytes for READID, we always read
-		 * the maximum 256 bytes(for PARAM)
-		 */
-		ifc_out32(256, &ifc->ifc_nand.nand_fbcr);
-		ifc_nand_ctrl->read_bytes = 256;
+		ifc_out32(len, &ifc->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.raynal@bootlin.com] 
Sent: Saturday, April 28, 2018 4:42 AM
To: Wan, Jane (Nokia - US/Sunnyvale) <jane.wan@nokia.com>
Cc: dwmw2@infradead.org; computersforpeace@gmail.com; Bos, Ties (Nokia - US/Sunnyvale) <ties.bos@nokia.com>; linux-mtd@lists.infradead.org; linux-kernel@vger.kernel.org; Boris Brezillon <Boris.Brezillon@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.Wan@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 *ctrl = priv->ctrl;
>  	struct fsl_ifc_runtime __iomem *ifc = ctrl->rregs;
> +	int len;
>  
>  	/* clear the read buffer */
>  	ifc_nand_ctrl->read_bytes = 0;
> @@ -462,11 +463,12 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command,
>  		ifc_out32(column, &ifc->ifc_nand.row3);
>  
>  		/*
> -		 * although currently it's 8 bytes for READID, we always read
> -		 * the maximum 256 bytes(for PARAM)
> +		 * For READID, read 8 bytes that are currently used.
> +		 * For PARAM, read all 3 copies of 256-bytes pages.
>  		 */
> -		ifc_out32(256, &ifc->ifc_nand.nand_fbcr);
> -		ifc_nand_ctrl->read_bytes = 256;
> +		len = (command == NAND_CMD_PARAM) ? (3 * 256) : 8;

There is already a "command == NAND_CMD_PARAM" condition above, you might want to use it.
[Jane] Done as suggested.

> +		ifc_out32(len, &ifc->ifc_nand.nand_fbcr);
> +		ifc_nand_ctrl->read_bytes = len;
>  
>  		set_addr(mtd, 0, 0, 0);
>  		fsl_ifc_run_command(mtd);

The overall ->cmdfunc() approach of this driver is horrible. However this fixes its implementation to match the current state of the core, so I guess it is fine.

Regards,
Miquèl

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

[-- Attachment #2: 0001-mtd-rawnand-fsl_ifc-fix-FSL-NAND-driver-to-read-all-.patch --]
[-- Type: application/octet-stream, Size: 1923 bytes --]

From 701de4146aa6355c951e97a77476e12d2da56d42 Mon Sep 17 00:00:00 2001
From: Jane Wan <Jane.Wan@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.Wan@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->ifc_nand.nand_fcr0);
 		ifc_out32(column, &ifc->ifc_nand.row3);
 
-		/*
-		 * although currently it's 8 bytes for READID, we always read
-		 * the maximum 256 bytes(for PARAM)
-		 */
-		ifc_out32(256, &ifc->ifc_nand.nand_fbcr);
-		ifc_nand_ctrl->read_bytes = 256;
+		ifc_out32(len, &ifc->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

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

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

[-- Attachment #1: Type: text/plain, Size: 7620 bytes --]

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.Wan@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.Wan@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 *)&buf[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.raynal@bootlin.com] 
Sent: Saturday, April 28, 2018 5:07 AM
To: Wan, Jane (Nokia - US/Sunnyvale) <jane.wan@nokia.com>
Cc: dwmw2@infradead.org; computersforpeace@gmail.com; Bos, Ties (Nokia - US/Sunnyvale) <ties.bos@nokia.com>; linux-mtd@lists.infradead.org; linux-kernel@vger.kernel.org; Boris Brezillon <Boris.Brezillon@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.Wan@nokia.com> wrote:

> Signed-off-by: Jane Wan <Jane.Wan@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, struct nand_chip *chip,
>  					int *busw)
>  {
>  	struct nand_onfi_params *p = &chip->onfi_params;
> -	int i, j;
> -	int val;
> +	int i, j, k, len, val;
> +	uint8_t copy[3][256], v8;

Please use u8 instead of uint8_t (./scripts/checkpatch.pl --strict will give you the list of styling issues to fix.
[Jane] Changed.

I don't think you should allocate that much space on the stack, please use dynamic allocation.
[Jane] Please see new patch file.

> +
> +	len = (sizeof(*p) > 256) ? 256 : sizeof(*p);

This is a maximum derivation, there are helpers for that.
[Jane] Not needed after changing to dynamic allocation.

I am not sure this is relevant, won't you read only 256 bytes anyway?
[Jane] I modified the code to allocate a buffer to store all 3 pages.  If in case all 3 pages failed, use the data stored
In the buffer to recover the content through bit-wise majority.

>  
>  	/* Try ONFI for unknown chip or LP */
>  	chip->cmdfunc(mtd, NAND_CMD_READID, 0x20, -1); @@ -3170,11 +3172,36 
> @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
>  				le16_to_cpu(p->crc)) {
>  			break;
>  		}

Space.
[Jane] checked the patch through checkpatch.pl.

> +		pr_err("CRC of parameter page %d is not valid\n", i);
> +		for (j = 0; j < len; j++)
> +			copy[i][j] = ((uint8_t *)p)[j];

'copy' is maybe not a meaningful name.
[Jane] 'copy' is removed.  Please see new patch file.

>  	}
>  
>  	if (i == 3) {
> -		pr_err("Could not find valid ONFI parameter page; aborting\n");
> -		return 0;
> +		pr_err("Could not find valid ONFI parameter page\n");
> +		pr_info("Recover ONFI parameters with bit-wise majority\n");
> +		for (j = 0; j < len; j++) {
> +			if (copy[0][j] == copy[1][j] ||
> +			    copy[0][j] == copy[2][j]) {
> +				((uint8_t *)p)[j] = copy[0][j];
> +			} else if (copy[1][j] == copy[2][j]) {
> +				((uint8_t *)p)[j] = copy[1][j];
> +			} else {
> +				((uint8_t *)p)[j] = 0;
> +				for (k = 0; k < 8; k++) {
> +					v8 = (copy[0][j] >> k) & 0x1;

v8 could be declared in the else statement of in the for loop.
The name could also be changed.
[Jane] Changed.  Please see the new patch file.

> +					v8 += (copy[1][j] >> k) & 0x1;
> +					v8 += (copy[2][j] >> k) & 0x1;
> +					if (v8 > 1)
> +						((uint8_t *)p)[j] |= (1 << k);

Please use the BIT() macro.
[Jane] Changed.  Please see the new patch file.

> +				}
> +			}
> +		}

Space.
[Jane] checked the patch through checkpatch.pl.

> +		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) !=
> +		    le16_to_cpu(p->crc)) {
> +			pr_err("ONFI parameter recovery failed, aborting\n");
> +			return 0;
> +		}
>  	}
>  
>  	/* Check version */

Thanks,
Miquèl

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

[-- Attachment #2: 0002-mtd-rawnand-fsl_ifc-use-bit-wise-majority-to-recover.patch --]
[-- Type: application/octet-stream, Size: 2891 bytes --]

From e14ed7dc08296a52f81d14781dee2f455dd90bbd Mon Sep 17 00:00:00 2001
From: Jane Wan <Jane.Wan@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.Wan@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 *)&buf[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

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

[Miquel Raynal]

Hi Jane,

On Tue, 1 May 2018 05:01:23 +0000, "Wan, Jane (Nokia - US/Sunnyvale)"
<jane.wan@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.

> 
> 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 maintain for everyone.


Thanks,
Miquèl

> 
> Best regards,
> Jane
> 
> Updated patch:
> From 701de4146aa6355c951e97a77476e12d2da56d42 Mon Sep 17 00:00:00 2001
> From: Jane Wan <Jane.Wan@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.Wan@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->ifc_nand.nand_fcr0);
>  		ifc_out32(column, &ifc->ifc_nand.row3);
>  
> -		/*
> -		 * although currently it's 8 bytes for READID, we always read
> -		 * the maximum 256 bytes(for PARAM)
> -		 */
> -		ifc_out32(256, &ifc->ifc_nand.nand_fbcr);
> -		ifc_nand_ctrl->read_bytes = 256;
> +		ifc_out32(len, &ifc->ifc_nand.nand_fbcr);
> +		ifc_nand_ctrl->read_bytes = len;
>  
>  		set_addr(mtd, 0, 0, 0);
>  		fsl_ifc_run_command(mtd);



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

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

[Boris Brezillon]

Hi Jane,

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

> Signed-off-by: Jane Wan <Jane.Wan@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, struct nand_chip *chip,
>  					int *busw)
>  {
>  	struct nand_onfi_params *p = &chip->onfi_params;
> -	int i, j;
> -	int val;
> +	int i, j, k, len, val;
> +	uint8_t copy[3][256], v8;
> +
> +	len = (sizeof(*p) > 256) ? 256 : sizeof(*p);
>  
>  	/* Try ONFI for unknown chip or LP */
>  	chip->cmdfunc(mtd, NAND_CMD_READID, 0x20, -1);
> @@ -3170,11 +3172,36 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
>  				le16_to_cpu(p->crc)) {
>  			break;
>  		}
> +		pr_err("CRC of parameter page %d is not valid\n", i);
> +		for (j = 0; j < len; j++)
> +			copy[i][j] = ((uint8_t *)p)[j];
>  	}
>  
>  	if (i == 3) {
> -		pr_err("Could not find valid ONFI parameter page; aborting\n");
> -		return 0;
> +		pr_err("Could not find valid ONFI parameter page\n");
> +		pr_info("Recover ONFI parameters with bit-wise majority\n");
> +		for (j = 0; j < len; j++) {
> +			if (copy[0][j] == copy[1][j] ||
> +			    copy[0][j] == copy[2][j]) {
> +				((uint8_t *)p)[j] = copy[0][j];
> +			} else if (copy[1][j] == copy[2][j]) {
> +				((uint8_t *)p)[j] = copy[1][j];
> +			} else {
> +				((uint8_t *)p)[j] = 0;
> +				for (k = 0; k < 8; k++) {
> +					v8 = (copy[0][j] >> k) & 0x1;
> +					v8 += (copy[1][j] >> k) & 0x1;
> +					v8 += (copy[2][j] >> k) & 0x1;
> +					if (v8 > 1)
> +						((uint8_t *)p)[j] |= (1 << k);
> +				}
> +			}
> +		}

I'd like this bit-wise majority algorithm to be generic and moved to
nand_base.c, because we might want to do the same in the core and make
it work for any number of repetitions of the PARAM page.

> +		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) !=
> +		    le16_to_cpu(p->crc)) {
> +			pr_err("ONFI parameter recovery failed, aborting\n");
> +			return 0;
> +		}
>  	}
>  
>  	/* Check version */

Thanks,

Boris

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

[Boris Brezillon]

On Wed, 2 May 2018 12:25:45 +0200
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> Hi Jane,
> 
> On Thu, 26 Apr 2018 17:19:56 -0700
> Jane Wan <Jane.Wan@nokia.com> wrote:
> 
> > Signed-off-by: Jane Wan <Jane.Wan@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, struct nand_chip *chip,
> >  					int *busw)
> >  {
> >  	struct nand_onfi_params *p = &chip->onfi_params;
> > -	int i, j;
> > -	int val;
> > +	int i, j, k, len, val;
> > +	uint8_t copy[3][256], v8;
> > +
> > +	len = (sizeof(*p) > 256) ? 256 : sizeof(*p);
> >  
> >  	/* Try ONFI for unknown chip or LP */
> >  	chip->cmdfunc(mtd, NAND_CMD_READID, 0x20, -1);
> > @@ -3170,11 +3172,36 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
> >  				le16_to_cpu(p->crc)) {
> >  			break;
> >  		}
> > +		pr_err("CRC of parameter page %d is not valid\n", i);
> > +		for (j = 0; j < len; j++)
> > +			copy[i][j] = ((uint8_t *)p)[j];
> >  	}
> >  
> >  	if (i == 3) {
> > -		pr_err("Could not find valid ONFI parameter page; aborting\n");
> > -		return 0;
> > +		pr_err("Could not find valid ONFI parameter page\n");
> > +		pr_info("Recover ONFI parameters with bit-wise majority\n");
> > +		for (j = 0; j < len; j++) {
> > +			if (copy[0][j] == copy[1][j] ||
> > +			    copy[0][j] == copy[2][j]) {
> > +				((uint8_t *)p)[j] = copy[0][j];
> > +			} else if (copy[1][j] == copy[2][j]) {
> > +				((uint8_t *)p)[j] = copy[1][j];
> > +			} else {
> > +				((uint8_t *)p)[j] = 0;
> > +				for (k = 0; k < 8; k++) {
> > +					v8 = (copy[0][j] >> k) & 0x1;
> > +					v8 += (copy[1][j] >> k) & 0x1;
> > +					v8 += (copy[2][j] >> k) & 0x1;
> > +					if (v8 > 1)
> > +						((uint8_t *)p)[j] |= (1 << k);
> > +				}
> > +			}
> > +		}  
> 
> I'd like this bit-wise majority algorithm to be generic and moved to
> nand_base.c, because we might want to do the same in the core and make
> it work for any number of repetitions of the PARAM page.

Never mind, I thought you were implementing that in the FSL IFC driver,
but you're actually modifying the core.

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

[Boris Brezillon]

On Tue, 1 May 2018 05:01:23 +0000
"Wan, Jane (Nokia - US/Sunnyvale)" <jane.wan@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.
> 
> 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.

And that proves my point. The core is reading 3 param pages [1], but
since this driver was trying to guess how many bytes to read from
->cmdfunc() and did not guess correctly you ended up with a partially
working implementation (works only if the first PARAM page is valid).
Now, you fix it to read 3 PARAM pages, but what if we decide to read
more to cope with MLC NANDs where even more copy are needed to have one
valid version? You'll have to patch ->cmdfunc() again, just because
you're trying to guess something that the core is supposed to tell you.

[1]https://elixir.bootlin.com/linux/v4.17-rc3/source/drivers/mtd/nand/raw/nand_base.c#L5115