LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Wan, Jane (Nokia - US/Sunnyvale)" <jane.wan@nokia.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: "dwmw2@infradead.org" <dwmw2@infradead.org>,
	"computersforpeace@gmail.com" <computersforpeace@gmail.com>,
	"Bos, Ties (Nokia - US/Sunnyvale)" <ties.bos@nokia.com>,
	"linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Boris Brezillon <Boris.Brezillon@bootlin.com>,
	"richard@nod.at" <richard@nod.at>,
	"marek.vasut@gmail.com" <marek.vasut@gmail.com>,
	"yamada.masahiro@socionext.com" <yamada.masahiro@socionext.com>,
	"prabhakar.kushwaha@nxp.com" <prabhakar.kushwaha@nxp.com>,
	"miquel.raynal@bootlin.com" <miquel.raynal@bootlin.com>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	"jagdish.gediya@nxp.com" <jagdish.gediya@nxp.com>,
	"shreeya.patel23498@gmail.com" <shreeya.patel23498@gmail.com>
Subject: RE: [PATCH 2/2] Use bit-wise majority to recover the contents of ONFI parameter
Date: Tue, 1 May 2018 05:33:59 +0000	[thread overview]
Message-ID: <VI1PR07MB1615A26D5E38C2568EFADE4C81810@VI1PR07MB1615.eurprd07.prod.outlook.com> (raw)
In-Reply-To: <20180428140638.2e2c04dd@xps13>

[-- 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


  reply	other threads:[~2018-05-01  5:34 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-27  0:19 [PATCH 0/2] Fix fsl_ifc_nand reading ONFI parameters to meet ONFI spec Jane Wan
2018-04-27  0:19 ` [PATCH 1/2] Fix FSL NAND driver to read all ONFI parameter pages Jane Wan
2018-04-28 11:42   ` Miquel Raynal
2018-05-01  5:01     ` Wan, Jane (Nokia - US/Sunnyvale)
2018-05-02  8:10       ` Miquel Raynal
2018-05-02 10:39       ` Boris Brezillon
2018-04-30 10:00   ` Boris Brezillon
2018-04-27  0:19 ` [PATCH 2/2] Use bit-wise majority to recover the contents of ONFI parameter Jane Wan
2018-04-28 12:06   ` Miquel Raynal
2018-05-01  5:33     ` Wan, Jane (Nokia - US/Sunnyvale) [this message]
2018-05-02 10:25   ` Boris Brezillon
2018-05-02 10:31     ` Boris Brezillon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=VI1PR07MB1615A26D5E38C2568EFADE4C81810@VI1PR07MB1615.eurprd07.prod.outlook.com \
    --to=jane.wan@nokia.com \
    --cc=Boris.Brezillon@bootlin.com \
    --cc=computersforpeace@gmail.com \
    --cc=dwmw2@infradead.org \
    --cc=jagdish.gediya@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=marek.vasut@gmail.com \
    --cc=miquel.raynal@bootlin.com \
    --cc=prabhakar.kushwaha@nxp.com \
    --cc=richard@nod.at \
    --cc=shawnguo@kernel.org \
    --cc=shreeya.patel23498@gmail.com \
    --cc=ties.bos@nokia.com \
    --cc=yamada.masahiro@socionext.com \
    --subject='RE: [PATCH 2/2] Use bit-wise majority to recover the contents of ONFI parameter' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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