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

[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 <Jane.Wan@nokia.com>
---
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, &p[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 *)&p[i], 254) ==
 				le16_to_cpu(p->crc)) {
+			if (i)
+				memcpy(p, &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 v7] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter

[Boris Brezillon]

Applied after fixing a few things (see below).

Changed the subject to "mtd: rawnand: use bit-wise majority to recover
the ONFI param page"

On Sun, 13 May 2018 04:30:02 +0000
"Wan, Jane (Nokia - US/Sunnyvale)" <jane.wan@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.

Wrapped this line.

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

Moved declaration cnt in the for(j) loop and made it an unsigned int.

> +
> +		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, &p[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 *)&p[i], 254) ==
>  				le16_to_cpu(p->crc)) {
> +			if (i)
> +				memcpy(p, &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 */

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

[Chris Moore]

Hi,

Le 13/05/2018 à 06:30, Wan, Jane (Nokia - US/Sunnyvale) a écrit :
> 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>
> ---
> 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, &p[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 *)&p[i], 254) ==
>   				le16_to_cpu(p->crc)) {
> +			if (i)
> +				memcpy(p, &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 */

This version is still hard coded for a three sample bitwise majority vote.
So why not use the method which I suggested previously for v2 and which 
I repeat below?

The three sample bitwise majority can be implemented without bit level 
manipulation using the identity:
majority3(a, b, c) = (a & b) | (a & c) | (b & c)
This can be factorized slightly to (a & (b | c)) | (b & c)
This enables the operation to be performed 8, 16, 32 or even 64 bits at 
a time depending on the hardware.

This method is not only faster and but also more compact.

Cheers,
Chris

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

[Boris Brezillon]

On Tue, 15 May 2018 06:45:51 +0200
Chris Moore <moore@free.fr> wrote:

> Hi,
> 
> Le 13/05/2018 à 06:30, Wan, Jane (Nokia - US/Sunnyvale) a écrit :
> > 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>
> > ---
> > 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, &p[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 *)&p[i], 254) ==
> >   				le16_to_cpu(p->crc)) {
> > +			if (i)
> > +				memcpy(p, &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 */  
> 
> This version is still hard coded for a three sample bitwise majority vote.
> So why not use the method which I suggested previously for v2 and which 
> I repeat below?

Because I want the nand_bit_wise_majority() function to work with
nsrcbufs > 3 (the ONFI spec says there's at least 3 copy of the param
page, but NAND vendor can decide to put more). Also, if the X copies of
the PARAM are corrupted (which is rather unlikely), that means we
already spent quite a lot of time reading the different copies and
calculating the CRC, so I think we don't care about perf optimizations
when doing bit-wise majority.

> 
> The three sample bitwise majority can be implemented without bit level 
> manipulation using the identity:
> majority3(a, b, c) = (a & b) | (a & c) | (b & c)
> This can be factorized slightly to (a & (b | c)) | (b & c)
> This enables the operation to be performed 8, 16, 32 or even 64 bits at 
> a time depending on the hardware.
> 
> This method is not only faster and but also more compact.
> 
> Cheers,
> Chris
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Chris Moore]

Hi,

Le 15/05/2018 à 09:34, Boris Brezillon a écrit :
> On Tue, 15 May 2018 06:45:51 +0200
> Chris Moore <moore@free.fr> wrote:
>
>> Hi,
>>
>> Le 13/05/2018 à 06:30, Wan, Jane (Nokia - US/Sunnyvale) a écrit :
>>> 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>
>>> ---
>>> 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, &p[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 *)&p[i], 254) ==
>>>    				le16_to_cpu(p->crc)) {
>>> +			if (i)
>>> +				memcpy(p, &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 */
>> This version is still hard coded for a three sample bitwise majority vote.
>> So why not use the method which I suggested previously for v2 and which
>> I repeat below?
> Because I want the nand_bit_wise_majority() function to work with
> nsrcbufs > 3 (the ONFI spec says there's at least 3 copy of the param
> page, but NAND vendor can decide to put more). Also, if the X copies of
> the PARAM are corrupted (which is rather unlikely), that means we
> already spent quite a lot of time reading the different copies and
> calculating the CRC, so I think we don't care about perf optimizations
> when doing bit-wise majority.
>
>> The three sample bitwise majority can be implemented without bit level
>> manipulation using the identity:
>> majority3(a, b, c) = (a & b) | (a & c) | (b & c)
>> This can be factorized slightly to (a & (b | c)) | (b & c)
>> This enables the operation to be performed 8, 16, 32 or even 64 bits at
>> a time depending on the hardware.
>>
>> This method is not only faster and but also more compact.
>>

I do understand that the ONFI specifications permit more than 3 copies.
However elsewhere the proposed code shows no intention of handling other 
cases.
The constant 3 is hard coded in the following lines extracted from the 
proposed code:
...
+    p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
...
      for (i = 0; i < 3; i++) {
...
      if (i == 3) {
...
+        const void *srcbufs[3] = {p, p + 1, p + 2};

Moreover the last of these is difficult to generalize.

Cheers,
Chris

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

[Miquel Raynal]

Hi Chris,

> >>> +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, &p[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 *)&p[i], 254) ==
> >>>    				le16_to_cpu(p->crc)) {
> >>> +			if (i)
> >>> +				memcpy(p, &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 */  
> >> This version is still hard coded for a three sample bitwise majority vote.
> >> So why not use the method which I suggested previously for v2 and which
> >> I repeat below?  
> > Because I want the nand_bit_wise_majority() function to work with
> > nsrcbufs > 3 (the ONFI spec says there's at least 3 copy of the param
> > page, but NAND vendor can decide to put more). Also, if the X copies of
> > the PARAM are corrupted (which is rather unlikely), that means we
> > already spent quite a lot of time reading the different copies and
> > calculating the CRC, so I think we don't care about perf optimizations
> > when doing bit-wise majority.
> >  
> >> The three sample bitwise majority can be implemented without bit level
> >> manipulation using the identity:
> >> majority3(a, b, c) = (a & b) | (a & c) | (b & c)
> >> This can be factorized slightly to (a & (b | c)) | (b & c)
> >> This enables the operation to be performed 8, 16, 32 or even 64 bits at
> >> a time depending on the hardware.
> >>
> >> This method is not only faster and but also more compact.
> >>  
> 
> I do understand that the ONFI specifications permit more than 3 copies.
> However elsewhere the proposed code shows no intention of handling other cases.
> The constant 3 is hard coded in the following lines extracted from the proposed code:
> ...
> +    p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> ...
>       for (i = 0; i < 3; i++) {
> ...
>       if (i == 3) {
> ...
> +        const void *srcbufs[3] = {p, p + 1, p + 2};
> 
> Moreover the last of these is difficult to generalize.

Indeed, this is something to improve. I think Boris' request was to
prepare changes like this one, to avoid the situation where the code
does not scale (like this 'p, p + 1, p + 2').

Thanks,
Miquèl

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

[Boris Brezillon]

On Wed, 16 May 2018 09:42:27 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Chris,
> 
> > >>> +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, &p[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 *)&p[i], 254) ==
> > >>>    				le16_to_cpu(p->crc)) {
> > >>> +			if (i)
> > >>> +				memcpy(p, &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 */    
> > >> This version is still hard coded for a three sample bitwise majority vote.
> > >> So why not use the method which I suggested previously for v2 and which
> > >> I repeat below?    
> > > Because I want the nand_bit_wise_majority() function to work with
> > > nsrcbufs > 3 (the ONFI spec says there's at least 3 copy of the param
> > > page, but NAND vendor can decide to put more). Also, if the X copies of
> > > the PARAM are corrupted (which is rather unlikely), that means we
> > > already spent quite a lot of time reading the different copies and
> > > calculating the CRC, so I think we don't care about perf optimizations
> > > when doing bit-wise majority.
> > >    
> > >> The three sample bitwise majority can be implemented without bit level
> > >> manipulation using the identity:
> > >> majority3(a, b, c) = (a & b) | (a & c) | (b & c)
> > >> This can be factorized slightly to (a & (b | c)) | (b & c)
> > >> This enables the operation to be performed 8, 16, 32 or even 64 bits at
> > >> a time depending on the hardware.
> > >>
> > >> This method is not only faster and but also more compact.
> > >>    
> > 
> > I do understand that the ONFI specifications permit more than 3 copies.
> > However elsewhere the proposed code shows no intention of handling other cases.
> > The constant 3 is hard coded in the following lines extracted from the proposed code:
> > ...
> > +    p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> > ...
> >       for (i = 0; i < 3; i++) {
> > ...
> >       if (i == 3) {
> > ...
> > +        const void *srcbufs[3] = {p, p + 1, p + 2};
> > 
> > Moreover the last of these is difficult to generalize.  
> 
> Indeed, this is something to improve. I think Boris' request was to
> prepare changes like this one, to avoid the situation where the code
> does not scale (like this 'p, p + 1, p + 2').

Yep, here is a quick/untested patch [1] making ONFI param page
detection and recovery more robust by reading more than 3 param pages if
there are more. And that's the reason I want a generic bit-wise
majority helper, not something that only works for 3 copies.

[1]http://code.bulix.org/t21eys-335698

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

[Boris Brezillon]

On Wed, 16 May 2018 09:32:57 +0200
Chris Moore <moore@free.fr> wrote:

> Hi,
> 
> Le 15/05/2018 à 09:34, Boris Brezillon a écrit :
> > On Tue, 15 May 2018 06:45:51 +0200
> > Chris Moore <moore@free.fr> wrote:
> >  
> >> Hi,
> >>
> >> Le 13/05/2018 à 06:30, Wan, Jane (Nokia - US/Sunnyvale) a écrit :  
> >>> 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>
> >>> ---
> >>> 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, &p[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 *)&p[i], 254) ==
> >>>    				le16_to_cpu(p->crc)) {
> >>> +			if (i)
> >>> +				memcpy(p, &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 */  
> >> This version is still hard coded for a three sample bitwise majority vote.
> >> So why not use the method which I suggested previously for v2 and which
> >> I repeat below?  
> > Because I want the nand_bit_wise_majority() function to work with
> > nsrcbufs > 3 (the ONFI spec says there's at least 3 copy of the param
> > page, but NAND vendor can decide to put more). Also, if the X copies of
> > the PARAM are corrupted (which is rather unlikely), that means we
> > already spent quite a lot of time reading the different copies and
> > calculating the CRC, so I think we don't care about perf optimizations
> > when doing bit-wise majority.
> >  
> >> The three sample bitwise majority can be implemented without bit level
> >> manipulation using the identity:
> >> majority3(a, b, c) = (a & b) | (a & c) | (b & c)
> >> This can be factorized slightly to (a & (b | c)) | (b & c)
> >> This enables the operation to be performed 8, 16, 32 or even 64 bits at
> >> a time depending on the hardware.
> >>
> >> This method is not only faster and but also more compact.
> >>  
> 
> I do understand that the ONFI specifications permit more than 3 copies.
> However elsewhere the proposed code shows no intention of handling other 
> cases.
> The constant 3 is hard coded in the following lines extracted from the 
> proposed code:
> ...
> +    p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> ...
>       for (i = 0; i < 3; i++) {
> ...
>       if (i == 3) {
> ...
> +        const void *srcbufs[3] = {p, p + 1, p + 2};
> 
> Moreover the last of these is difficult to generalize.

Not that much. We just have to allocate srcbufs dynamically and
krealloc() everytime we want to add a new entry.

May I ask why you care that much about this optimization? As I said, if
we really have to read all the copies to realize none of them is good,
we already lost a lot of time, so having a "suboptimal but generic"
version of the bit-wise majority shouldn't hurt that much.

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

[Chris Moore]

Hi,

Le 16/05/2018 à 09:56, Boris Brezillon a écrit :
> On Wed, 16 May 2018 09:32:57 +0200
> Chris Moore <moore@free.fr> wrote:
>
>> Hi,
>>
>> Le 15/05/2018 à 09:34, Boris Brezillon a écrit :
>>> On Tue, 15 May 2018 06:45:51 +0200
>>> Chris Moore <moore@free.fr> wrote:
>>>   
>>>> Hi,
>>>>
>>>> Le 13/05/2018 à 06:30, Wan, Jane (Nokia - US/Sunnyvale) a écrit :
>>>>> 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>
>>>>> ---
>>>>> 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, &p[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 *)&p[i], 254) ==
>>>>>     				le16_to_cpu(p->crc)) {
>>>>> +			if (i)
>>>>> +				memcpy(p, &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 */
>>>> This version is still hard coded for a three sample bitwise majority vote.
>>>> So why not use the method which I suggested previously for v2 and which
>>>> I repeat below?
>>> Because I want the nand_bit_wise_majority() function to work with
>>> nsrcbufs > 3 (the ONFI spec says there's at least 3 copy of the param
>>> page, but NAND vendor can decide to put more). Also, if the X copies of
>>> the PARAM are corrupted (which is rather unlikely), that means we
>>> already spent quite a lot of time reading the different copies and
>>> calculating the CRC, so I think we don't care about perf optimizations
>>> when doing bit-wise majority.
>>>   
>>>> The three sample bitwise majority can be implemented without bit level
>>>> manipulation using the identity:
>>>> majority3(a, b, c) = (a & b) | (a & c) | (b & c)
>>>> This can be factorized slightly to (a & (b | c)) | (b & c)
>>>> This enables the operation to be performed 8, 16, 32 or even 64 bits at
>>>> a time depending on the hardware.
>>>>
>>>> This method is not only faster and but also more compact.
>>>>   
>> I do understand that the ONFI specifications permit more than 3 copies.
>> However elsewhere the proposed code shows no intention of handling other
>> cases.
>> The constant 3 is hard coded in the following lines extracted from the
>> proposed code:
>> ...
>> +    p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
>> ...
>>        for (i = 0; i < 3; i++) {
>> ...
>>        if (i == 3) {
>> ...
>> +        const void *srcbufs[3] = {p, p + 1, p + 2};
>>
>> Moreover the last of these is difficult to generalize.
> Not that much. We just have to allocate srcbufs dynamically and
> krealloc() everytime we want to add a new entry.
>
> May I ask why you care that much about this optimization? As I said, if
> we really have to read all the copies to realize none of them is good,
> we already lost a lot of time, so having a "suboptimal but generic"
> version of the bit-wise majority shouldn't hurt that much.
>

I just wanted to point out that, as the proposed code only handles the 
case of 3 copies, there is a much more efficient way of performing the 
majority vote than looping over each bit.
My method would also reduce the code size which is an important factor 
in the kernel.
Otherwise I have no objection and I shall accept your choice without 
further remarks on this point.

Cheers,
Chris