All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5 2/2] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter
@ 2018-05-10  2:46 Jane Wan
  2018-05-10 12:03 ` Boris Brezillon
  0 siblings, 1 reply; 10+ messages in thread
From: Jane Wan @ 2018-05-10  2:46 UTC (permalink / raw)
  To: Boris.Brezillon, miquel.raynal, dwmw2, computersforpeace,
	richard, marek.vasut, yamada.masahiro, prabhakar.kushwaha,
	shawnguo, jagdish.gediya, shreeya.patel23498
  Cc: linux-mtd, linux-kernel, ties.bos, Jane Wan

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 |   46 +++++++++++++++++++++++++++++++++-----
 1 file changed, 41 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 72f3a89..a7c2507 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -5086,6 +5086,34 @@ static int nand_flash_detect_ext_param_page(struct nand_chip *chip,
 	return ret;
 }
 
+#define GET_BIT(bit, val)   (((val) >> (bit)) & 0x01)
+
+/*
+ * Recover data with bit-wise majority
+ */
+static void nand_bit_wise_majority(const void **srcbufs,
+				   void *dstbuf,
+				   unsigned int nbufs,
+				   unsigned int bufsize)
+{
+	int i, j, k;
+	u8 v, m;
+	u8 *p;
+
+	p = *(u8 **)srcbufs;
+	for (i = 0; i < bufsize; i++) {
+		v = 0;
+		for (j = 0; j < 8; j++) {
+			m = 0;
+			for (k = 0; k < nbufs; k++)
+				m += GET_BIT(j, p[k*bufsize + i]);
+			if (m > nbufs/2)
+				v |= BIT(j);
+		}
+		((u8 *)dstbuf)[i] = v;
+	}
+}
+
 /*
  * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
  */
@@ -5102,7 +5130,7 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
 		return 0;
 
 	/* ONFI chip: allocate a buffer to hold its parameter page */
-	p = kzalloc(sizeof(*p), GFP_KERNEL);
+	p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
 	if (!p)
 		return -ENOMEM;
 
@@ -5113,21 +5141,29 @@ 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;
+		pr_err("Could not find valid ONFI parameter page\n");
+		pr_info("Recover ONFI params with bit-wise majority\n");
+		nand_bit_wise_majority((const void **)&p, p, 3, 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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 2/2] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter
  2018-05-10  2:46 [PATCH v5 2/2] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter Jane Wan
@ 2018-05-10 12:03 ` Boris Brezillon
  2018-05-10 21:37   ` Wan, Jane (Nokia - US/Sunnyvale)
  2018-05-14 17:54   ` Andy Shevchenko
  0 siblings, 2 replies; 10+ messages in thread
From: Boris Brezillon @ 2018-05-10 12:03 UTC (permalink / raw)
  To: Jane Wan
  Cc: miquel.raynal, dwmw2, computersforpeace, richard, marek.vasut,
	yamada.masahiro, prabhakar.kushwaha, shawnguo, jagdish.gediya,
	shreeya.patel23498, linux-mtd, linux-kernel, ties.bos

Hi Jane,

Subject prefix should be "[PATCH v5] ...", the 2/2 is no longer valid
since you only have one patch here.

On Wed,  9 May 2018 19:46:40 -0700
Jane Wan <Jane.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.
> 
> Signed-off-by: Jane Wan <Jane.Wan@nokia.com>
> ---

There should be a changelog here describing what has changed in each
version of the patch.

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

Not sure we need that macro, see below.

> +
> +/*
> + * Recover data with bit-wise majority
> + */
> +static void nand_bit_wise_majority(const void **srcbufs,
> +				   void *dstbuf,
> +				   unsigned int nbufs,
> +				   unsigned int bufsize)

I'd prefer to have nbufs just after srcbufs and named nsrcbufs:

static void nand_bit_wise_majority(const void **srcbufs,
				   unsigned int nsrcbufs,
				   void *dstbuf,
				   unsigned int bufsize)

> +{
> +	int i, j, k;
> +	u8 v, m;
> +	u8 *p;
> +
> +	p = *(u8 **)srcbufs;

Nope, I'd like to support the cases where srcbufs are not contiguous,
so that does not work.

> +	for (i = 0; i < bufsize; i++) {
> +		v = 0;

You can declare the v variable here, since its scope is limited to the
for loop. BTW, v, m, can't we pick better names? I guess v is for val,
but I'm not even sure what m stands for.

> +		for (j = 0; j < 8; j++) {
> +			m = 0;
> +			for (k = 0; k < nbufs; k++)
> +				m += GET_BIT(j, p[k*bufsize + i]);

			for (k = 0; k < nbufs; k++) {
				const u8 *srcbuf = srcbufs[j];

				if (srcbuf[i] & BIT(k))
					m++;
			}

> +			if (m > nbufs/2)

Space between operands and operators please

			if (m > nbufs / 2)

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

		const void *srcbufs[3] = {p, p + 1, p + 2};

> -		pr_err("Could not find valid ONFI parameter page; aborting\n");
> -		goto free_onfi_param_page;
> +		pr_err("Could not find valid ONFI parameter page\n");
> +		pr_info("Recover ONFI params with bit-wise majority\n");
> +		nand_bit_wise_majority((const void **)&p, p, 3, sizeof(*p));

		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 */

Thanks,

Boris

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v5 2/2] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter
  2018-05-10 12:03 ` Boris Brezillon
@ 2018-05-10 21:37   ` Wan, Jane (Nokia - US/Sunnyvale)
  2018-05-14 17:54   ` Andy Shevchenko
  1 sibling, 0 replies; 10+ messages in thread
From: Wan, Jane (Nokia - US/Sunnyvale) @ 2018-05-10 21:37 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: miquel.raynal, dwmw2, computersforpeace, richard, marek.vasut,
	yamada.masahiro, prabhakar.kushwaha, shawnguo, jagdish.gediya,
	shreeya.patel23498, linux-mtd, linux-kernel, Bos,
	Ties (Nokia - US/Sunnyvale)

Hi Boris,

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

Thanks.
Jane

> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezillon@bootlin.com]
> Sent: Thursday, May 10, 2018 5:03 AM
> To: Wan, Jane (Nokia - US/Sunnyvale) <jane.wan@nokia.com>
> Cc: miquel.raynal@bootlin.com; dwmw2@infradead.org;
> computersforpeace@gmail.com; richard@nod.at; marek.vasut@gmail.com;
> yamada.masahiro@socionext.com; prabhakar.kushwaha@nxp.com;
> shawnguo@kernel.org; jagdish.gediya@nxp.com;
> shreeya.patel23498@gmail.com; linux-mtd@lists.infradead.org; linux-
> kernel@vger.kernel.org; Bos, Ties (Nokia - US/Sunnyvale) <ties.bos@nokia.com>
> Subject: Re: [PATCH v5 2/2] mtd: rawnand: use bit-wise majority to recover the
> contents of ONFI parameter
> 
> Hi Jane,
> 
> Subject prefix should be "[PATCH v5] ...", the 2/2 is no longer valid since you only
> have one patch here.
> 
> On Wed,  9 May 2018 19:46:40 -0700
> Jane Wan <Jane.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.
> >
> > Signed-off-by: Jane Wan <Jane.Wan@nokia.com>
> > ---
> 
> There should be a changelog here describing what has changed in each version
> of the patch.

[Jane] Added the changelogs in v6.

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

[Jane] Removed.

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

[Jane] changed as above in v6.

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

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

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

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

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

[Jane]  Changed as suggested in v6.  Thanks.

> 
> > +				v |= BIT(j);
> > +		}
> > +		((u8 *)dstbuf)[i] = v;
> > +	}
> > +}
> > +
> >  /*
> >   * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
> >   */
> > @@ -5102,7 +5130,7 @@ static int nand_flash_detect_onfi(struct nand_chip
> *chip)
> >  		return 0;
> >
> >  	/* ONFI chip: allocate a buffer to hold its parameter page */
> > -	p = kzalloc(sizeof(*p), GFP_KERNEL);
> > +	p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> >  	if (!p)
> >  		return -ENOMEM;
> >
> > @@ -5113,21 +5141,29 @@ 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) {
> 
> 		const void *srcbufs[3] = {p, p + 1, p + 2};
> 
> > -		pr_err("Could not find valid ONFI parameter page; aborting\n");
> > -		goto free_onfi_param_page;
> > +		pr_err("Could not find valid ONFI parameter page\n");
> > +		pr_info("Recover ONFI params with bit-wise majority\n");
> > +		nand_bit_wise_majority((const void **)&p, p, 3, sizeof(*p));
> 
> 		nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
> 				       sizeof(*p))

[Jane]  Changed in v6.  Thanks.

> 
> > +		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 */
> 
> Thanks,
> 
> Boris

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 2/2] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter
  2018-05-10 12:03 ` Boris Brezillon
  2018-05-10 21:37   ` Wan, Jane (Nokia - US/Sunnyvale)
@ 2018-05-14 17:54   ` Andy Shevchenko
  2018-05-15  7:35     ` Boris Brezillon
  1 sibling, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2018-05-14 17:54 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: Jane Wan, Miquel Raynal, David Woodhouse, Brian Norris,
	Richard Weinberger, Marek Vasut, Masahiro Yamada,
	prabhakar.kushwaha, Shawn Guo, jagdish.gediya, Shreeya Patel,
	open list:MEMORY TECHNOLOGY...,
	Linux Kernel Mailing List, ties.bos

On Thu, May 10, 2018 at 3:03 PM, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:

>> +#define GET_BIT(bit, val)   (((val) >> (bit)) & 0x01)
>
> Not sure we need that macro, see below.

+1. We have too many nice helpers for bit manipulations
(for_each_set_bit() as an example).


>                         for (k = 0; k < nbufs; k++) {
>                                 const u8 *srcbuf = srcbufs[j];
>
>                                 if (srcbuf[i] & BIT(k))
>                                         m++;
>                         }

...which is effectively hweightXX().

>> -     p = kzalloc(sizeof(*p), GFP_KERNEL);
>> +     p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
>>       if (!p)
>>               return -ENOMEM;

...which is kcalloc().


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 2/2] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter
  2018-05-14 17:54   ` Andy Shevchenko
@ 2018-05-15  7:35     ` Boris Brezillon
  2018-05-15  7:46       ` Andy Shevchenko
  0 siblings, 1 reply; 10+ messages in thread
From: Boris Brezillon @ 2018-05-15  7:35 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Shreeya Patel, Masahiro Yamada, Jane Wan, Miquel Raynal,
	Linux Kernel Mailing List, Marek Vasut, ties.bos,
	prabhakar.kushwaha, open list:MEMORY TECHNOLOGY...,
	jagdish.gediya, Richard Weinberger, Shawn Guo, Brian Norris,
	David Woodhouse

On Mon, 14 May 2018 20:54:36 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, May 10, 2018 at 3:03 PM, Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> 
> >> +#define GET_BIT(bit, val)   (((val) >> (bit)) & 0x01)  
> >
> > Not sure we need that macro, see below.  
> 
> +1. We have too many nice helpers for bit manipulations
> (for_each_set_bit() as an example).
> 
> 
> >                         for (k = 0; k < nbufs; k++) {
> >                                 const u8 *srcbuf = srcbufs[j];
> >
> >                                 if (srcbuf[i] & BIT(k))
> >                                         m++;
> >                         }  
> 
> ...which is effectively hweightXX().

No it's not.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 2/2] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter
  2018-05-15  7:35     ` Boris Brezillon
@ 2018-05-15  7:46       ` Andy Shevchenko
  2018-05-15  8:03         ` Boris Brezillon
  0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2018-05-15  7:46 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: Shreeya Patel, Masahiro Yamada, Jane Wan, Miquel Raynal,
	Linux Kernel Mailing List, Marek Vasut, ties.bos,
	prabhakar.kushwaha, open list:MEMORY TECHNOLOGY...,
	jagdish.gediya, Richard Weinberger, Shawn Guo, Brian Norris,
	David Woodhouse

On Tue, May 15, 2018 at 10:35 AM, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> On Mon, 14 May 2018 20:54:36 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
>> On Thu, May 10, 2018 at 3:03 PM, Boris Brezillon
>> <boris.brezillon@bootlin.com> wrote:
>>
>> >> +#define GET_BIT(bit, val)   (((val) >> (bit)) & 0x01)
>> >
>> > Not sure we need that macro, see below.
>>
>> +1. We have too many nice helpers for bit manipulations
>> (for_each_set_bit() as an example).
>>
>>
>> >                         for (k = 0; k < nbufs; k++) {
>> >                                 const u8 *srcbuf = srcbufs[j];
>> >
>> >                                 if (srcbuf[i] & BIT(k))
>> >                                         m++;
>> >                         }
>>
>> ...which is effectively hweightXX().
>
> No it's not.

I don't see how "not". In the loop everithing except m and k are
invariants. What did I miss?

The powerness of two of nbufs is another thing of _existing_
prototypes of hweightXX().

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 2/2] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter
  2018-05-15  7:46       ` Andy Shevchenko
@ 2018-05-15  8:03         ` Boris Brezillon
  2018-05-15 20:23           ` Andy Shevchenko
  0 siblings, 1 reply; 10+ messages in thread
From: Boris Brezillon @ 2018-05-15  8:03 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Shreeya Patel, Masahiro Yamada, Jane Wan, Miquel Raynal,
	Linux Kernel Mailing List, Marek Vasut, ties.bos,
	prabhakar.kushwaha, open list:MEMORY TECHNOLOGY...,
	jagdish.gediya, Richard Weinberger, Shawn Guo, Brian Norris,
	David Woodhouse

On Tue, 15 May 2018 10:46:00 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, May 15, 2018 at 10:35 AM, Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> > On Mon, 14 May 2018 20:54:36 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >  
> >> On Thu, May 10, 2018 at 3:03 PM, Boris Brezillon
> >> <boris.brezillon@bootlin.com> wrote:
> >>  
> >> >> +#define GET_BIT(bit, val)   (((val) >> (bit)) & 0x01)  
> >> >
> >> > Not sure we need that macro, see below.  
> >>
> >> +1. We have too many nice helpers for bit manipulations
> >> (for_each_set_bit() as an example).
> >>
> >>  
> >> >                         for (k = 0; k < nbufs; k++) {
> >> >                                 const u8 *srcbuf = srcbufs[j];
> >> >
> >> >                                 if (srcbuf[i] & BIT(k))
> >> >                                         m++;
> >> >                         }  
> >>
> >> ...which is effectively hweightXX().  
> >
> > No it's not.  
> 
> I don't see how "not". In the loop everithing except m and k are
> invariants. What did I miss?

We're not counting the number of bits set in an uXX var, but the number
of set bits at the same position in different buffers.

> 
> The powerness of two of nbufs is another thing of _existing_
> prototypes of hweightXX().
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 2/2] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter
  2018-05-15  8:03         ` Boris Brezillon
@ 2018-05-15 20:23           ` Andy Shevchenko
  2018-05-15 20:35             ` Boris Brezillon
  0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2018-05-15 20:23 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: Shreeya Patel, Masahiro Yamada, Jane Wan, Miquel Raynal,
	Linux Kernel Mailing List, Marek Vasut, ties.bos,
	prabhakar.kushwaha, open list:MEMORY TECHNOLOGY...,
	jagdish.gediya, Richard Weinberger, Shawn Guo, Brian Norris,
	David Woodhouse

On Tue, May 15, 2018 at 11:03 AM, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> On Tue, 15 May 2018 10:46:00 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
>> On Tue, May 15, 2018 at 10:35 AM, Boris Brezillon
>> <boris.brezillon@bootlin.com> wrote:
>> > On Mon, 14 May 2018 20:54:36 +0300
>> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

>> >> >                         for (k = 0; k < nbufs; k++) {
>> >> >                                 const u8 *srcbuf = srcbufs[j];
>> >> >
>> >> >                                 if (srcbuf[i] & BIT(k))
>> >> >                                         m++;
>> >> >                         }
>> >>
>> >> ...which is effectively hweightXX().
>> >
>> > No it's not.
>>
>> I don't see how "not". In the loop everithing except m and k are
>> invariants. What did I miss?
>
> We're not counting the number of bits set in an uXX var, but the number
> of set bits at the same position in different buffers.

...on big picture. The excerpt above is hweight() against srcbuf[i].

Let's rewrite it like this:

                    const u8 *srcbuf = srcbufs[j];

                    for (k = 0; k < nbufs; k++) {
                               if (srcbuf[i] & BIT(k))
                                      m++;
                    }

...and now it looks obvious:

m += hweight...(srcbuf[i])

_If_ nbufs is power of two we may use primitive helper.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 2/2] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter
  2018-05-15 20:23           ` Andy Shevchenko
@ 2018-05-15 20:35             ` Boris Brezillon
  2018-05-15 21:02               ` Andy Shevchenko
  0 siblings, 1 reply; 10+ messages in thread
From: Boris Brezillon @ 2018-05-15 20:35 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: jagdish.gediya, Masahiro Yamada, Jane Wan,
	Linux Kernel Mailing List, Richard Weinberger, Marek Vasut,
	ties.bos, prabhakar.kushwaha, open list:MEMORY TECHNOLOGY...,
	Shreeya Patel, Miquel Raynal, Brian Norris, Shawn Guo,
	David Woodhouse

On Tue, 15 May 2018 23:23:02 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, May 15, 2018 at 11:03 AM, Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> > On Tue, 15 May 2018 10:46:00 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >  
> >> On Tue, May 15, 2018 at 10:35 AM, Boris Brezillon
> >> <boris.brezillon@bootlin.com> wrote:  
> >> > On Mon, 14 May 2018 20:54:36 +0300
> >> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> 
> >> >> >                         for (k = 0; k < nbufs; k++) {
> >> >> >                                 const u8 *srcbuf = srcbufs[j];
> >> >> >
> >> >> >                                 if (srcbuf[i] & BIT(k))
> >> >> >                                         m++;
> >> >> >                         }  
> >> >>
> >> >> ...which is effectively hweightXX().  
> >> >
> >> > No it's not.  
> >>
> >> I don't see how "not". In the loop everithing except m and k are
> >> invariants. What did I miss?  
> >
> > We're not counting the number of bits set in an uXX var, but the number
> > of set bits at the same position in different buffers.  
> 
> ...on big picture. The excerpt above is hweight() against srcbuf[i].
> 
> Let's rewrite it like this:
> 
>                     const u8 *srcbuf = srcbufs[j];
> 
>                     for (k = 0; k < nbufs; k++) {
>                                if (srcbuf[i] & BIT(k))

I made a mistake in my code sample, it's

				if (srcbuf[i] & BIT(j))

If you look at v6, you'll see it's been fixed by Jane.

>                                       m++;
>                     }
> 
> ...and now it looks obvious:
> 
> m += hweight...(srcbuf[i])
> 
> _If_ nbufs is power of two we may use primitive helper.
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 2/2] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter
  2018-05-15 20:35             ` Boris Brezillon
@ 2018-05-15 21:02               ` Andy Shevchenko
  0 siblings, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2018-05-15 21:02 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: jagdish.gediya, Masahiro Yamada, Jane Wan,
	Linux Kernel Mailing List, Richard Weinberger, Marek Vasut,
	ties.bos, prabhakar.kushwaha, open list:MEMORY TECHNOLOGY...,
	Shreeya Patel, Miquel Raynal, Brian Norris, Shawn Guo,
	David Woodhouse

On Tue, May 15, 2018 at 11:35 PM, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> On Tue, 15 May 2018 23:23:02 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> I made a mistake in my code sample, it's
>
>                                 if (srcbuf[i] & BIT(j))
>
> If you look at v6, you'll see it's been fixed by Jane.

In this case, indeed.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-05-15 21:02 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-10  2:46 [PATCH v5 2/2] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter Jane Wan
2018-05-10 12:03 ` Boris Brezillon
2018-05-10 21:37   ` Wan, Jane (Nokia - US/Sunnyvale)
2018-05-14 17:54   ` Andy Shevchenko
2018-05-15  7:35     ` Boris Brezillon
2018-05-15  7:46       ` Andy Shevchenko
2018-05-15  8:03         ` Boris Brezillon
2018-05-15 20:23           ` Andy Shevchenko
2018-05-15 20:35             ` Boris Brezillon
2018-05-15 21:02               ` Andy Shevchenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.