[PATCH 0/2] Use badblockbits-like approach in nand_bbt.c

[PATCH 0/2] Use badblockbits-like approach in nand_bbt.c

[rnd4]

Hi all,

I'm currenlty working with an MLC NAND (MT29F32G08CBACAWP) on a iMX6 based
platform (kernel 3.10.17).
The first problem I have is about badblock detection. I experienced a lot of bit
flips on badblock marker (some of them probalbly due misuse, e.g. powercut).

By looking into the MDT code, I've found that inside nand_base.c there's support
for badblockbits feature, which, for short, allow to tolerate 1 or more bitflip
when looking for BB.

In my experience usually the marker is 0x00 for bad blocks and 0xFF for good one
(at least this is true for my current component).

Unfortunately this approach is not used into BBT code (nand_bbt.c) and thus I have
a lot of false positive BB.

With the following patches I try to add badblockbits-like approach to nand_bbt.c
code.

The first patch is optional and configure badblockbits depending on SLC vs MLC NAND.

The second patch implements the badblockbits approach into BBT scan.
Please note that this is not usually enough, because many NAND controller override
nand_bbt_descr and thus requires changes also the the specific controller driver.

The patch has been made in collaboration with Boris Brezillon (thank you for you
suggestions Boris).

I tested it on a 3.10.17 kernel, but the second patch applies fine to
v3.10 stable and latest 4.0-rc1 (the first patch needs some changes to apply to this
one).

Feel free to comment it and tell me if something can be done in a better way

Best Regards,

Andrea Scian
DAVE Embedded Systems

[PATCH 1/2] mtd: nand: use a lower value for badblockbits when working with MLC NAND

[rnd4]

From: Andrea Scian <andrea.scian@dave.eu>

MLC NANDs have more bit flips that SLC. When looking for bad block
marker we have a lot of false positive if we check for the whole byte. To
avoid this tolerate a few (4 here) bit flips for byte.

Signed-off-by: Andrea Scian <andrea.scian@dave.eu>
---
 drivers/mtd/nand/nand_base.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index dfcd0a5..bdca223 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3315,7 +3315,10 @@ ident_done:
 		chip->chip_shift += 32 - 1;
 	}
 
-	chip->badblockbits = 8;
+	if (nand_is_slc(chip))
+		chip->badblockbits = 8;
+	else
+		chip->badblockbits = 4;
 	chip->erase_cmd = single_erase_cmd;
 
 	/* Do not replace user supplied command function! */
-- 
1.7.9.5

[PATCH 2/2] mtd: nand: use nand_chip badblockbits when checking bad block pattern

[rnd4]

From: Andrea Scian <andrea.scian@dave.eu>

Use nand_chip->badblockbits like it's used inside nand_base.c when scanning
bad block markers.
This is particularly useful when using MLC NAND

Signed-off-by: Andrea Scian <andrea.scian@dave.eu>
---
 drivers/mtd/nand/nand_bbt.c |   19 ++++++++++++++++---
 include/linux/mtd/bbm.h     |    3 +++
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index 2672643..dba7f8b 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -127,9 +127,21 @@ static int check_pattern(uint8_t *buf, int len, int paglen, struct nand_bbt_desc
  */
 static int check_short_pattern(uint8_t *buf, struct nand_bbt_descr *td)
 {
-	/* Compare the pattern */
-	if (memcmp(buf + td->offs, td->pattern, td->len))
-		return -1;
+	if (likely(td->toleratedbitdiff == 0)) {
+		/* Compare the pattern */
+		if (memcmp(buf + td->offs, td->pattern, td->len))
+			return -1;
+	} else {
+		int i, nbitdiff = 0;
+		uint8_t tmp;
+
+		for (i = 0; i < td->len; i++) {
+			tmp = buf[td->offs + i] ^ td->pattern[i];
+			nbitdiff += hweight8(tmp);
+			if (nbitdiff > td->toleratedbitdiff)
+				return -1;
+		}
+	}
 	return 0;
 }
 
@@ -1309,6 +1321,7 @@ static int nand_create_badblock_pattern(struct nand_chip *this)
 	bd->len = (this->options & NAND_BUSWIDTH_16) ? 2 : 1;
 	bd->pattern = scan_ff_pattern;
 	bd->options |= NAND_BBT_DYNAMICSTRUCT;
+	bd->toleratedbitdiff = 8-this->badblockbits;
 	this->badblock_pattern = bd;
 	return 0;
 }
diff --git a/include/linux/mtd/bbm.h b/include/linux/mtd/bbm.h
index 211ff67..0714337 100644
--- a/include/linux/mtd/bbm.h
+++ b/include/linux/mtd/bbm.h
@@ -48,6 +48,8 @@
  *              bad) block in the stored bbt
  * @pattern:	pattern to identify bad block table or factory marked good /
  *		bad blocks, can be NULL, if len = 0
+ * @toleratedbitdiff: number of tolerated bit difference (in a byte) between
+ *		pattern and buffer
  *
  * Descriptor for the bad block table marker and the descriptor for the
  * pattern which identifies good and bad blocks. The assumption is made
@@ -64,6 +66,7 @@ struct nand_bbt_descr {
 	int maxblocks;
 	int reserved_block_code;
 	uint8_t *pattern;
+	int toleratedbitdiff;
 };
 
 /* Options for the bad block table descriptors */
-- 
1.7.9.5

Re: [PATCH 1/2] mtd: nand: use a lower value for badblockbits when working with MLC NAND

[Boris Brezillon]

Hi Andrea,

On Fri,  6 Mar 2015 13:12:17 +0100
rnd4@dave-tech.it wrote:

> From: Andrea Scian <andrea.scian@dave.eu>
> 
> MLC NANDs have more bit flips that SLC. When looking for bad block
> marker we have a lot of false positive if we check for the whole byte. To
> avoid this tolerate a few (4 here) bit flips for byte.

I'm not sure sure we want to accept 4 bitflips for all MLC NANDs. IMHO
this value should be chip dependent.
I know there is currently no way to retrieve this information, so here
are two suggestions:

1/ make this value depend on the required NAND ecc strength
(badblockbits = ecc_strength / 10 ?)
2/ let each controller change this value after nand_scan_ident
depending on the detected chip until we find a generic solution to
select this value

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 2/2] mtd: nand: use nand_chip badblockbits when checking bad block pattern

[Boris Brezillon]

Hi Andrea,

On Fri,  6 Mar 2015 13:12:18 +0100
rnd4@dave-tech.it wrote:

> From: Andrea Scian <andrea.scian@dave.eu>
> 
> Use nand_chip->badblockbits like it's used inside nand_base.c when scanning
> bad block markers.
> This is particularly useful when using MLC NAND
> 
> Signed-off-by: Andrea Scian <andrea.scian@dave.eu>
> ---
>  drivers/mtd/nand/nand_bbt.c |   19 ++++++++++++++++---
>  include/linux/mtd/bbm.h     |    3 +++
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
> index 2672643..dba7f8b 100644
> --- a/drivers/mtd/nand/nand_bbt.c
> +++ b/drivers/mtd/nand/nand_bbt.c
> @@ -127,9 +127,21 @@ static int check_pattern(uint8_t *buf, int len, int paglen, struct nand_bbt_desc
>   */
>  static int check_short_pattern(uint8_t *buf, struct nand_bbt_descr *td)
>  {
> -	/* Compare the pattern */
> -	if (memcmp(buf + td->offs, td->pattern, td->len))
> -		return -1;
> +	if (likely(td->toleratedbitdiff == 0)) {

I know I'm the one who suggested the toleratedbitdiff variable name,
but maybe you should follow the naming convention in this file:
tolerated_bit_diff.

> +		/* Compare the pattern */
> +		if (memcmp(buf + td->offs, td->pattern, td->len))
> +			return -1;
> +	} else {
> +		int i, nbitdiff = 0;
> +		uint8_t tmp;
> +
> +		for (i = 0; i < td->len; i++) {
> +			tmp = buf[td->offs + i] ^ td->pattern[i];
> +			nbitdiff += hweight8(tmp);
> +			if (nbitdiff > td->toleratedbitdiff)
> +				return -1;
> +		}
> +	}
>  	return 0;
>  }
>  
> @@ -1309,6 +1321,7 @@ static int nand_create_badblock_pattern(struct nand_chip *this)
>  	bd->len = (this->options & NAND_BUSWIDTH_16) ? 2 : 1;
>  	bd->pattern = scan_ff_pattern;
>  	bd->options |= NAND_BBT_DYNAMICSTRUCT;
> +	bd->toleratedbitdiff = 8-this->badblockbits;
>  	this->badblock_pattern = bd;
>  	return 0;
>  }
> diff --git a/include/linux/mtd/bbm.h b/include/linux/mtd/bbm.h
> index 211ff67..0714337 100644
> --- a/include/linux/mtd/bbm.h
> +++ b/include/linux/mtd/bbm.h
> @@ -48,6 +48,8 @@
>   *              bad) block in the stored bbt
>   * @pattern:	pattern to identify bad block table or factory marked good /
>   *		bad blocks, can be NULL, if len = 0
> + * @toleratedbitdiff: number of tolerated bit difference (in a byte) between
> + *		pattern and buffer

Why do you want to make this value a per byte value. I mean, if we have
a pattern taking more than one byte, it makes sense to specify the
total number of tolerated bitflips for the whole pattern.
BTW, the implementation does not match your description (this value
represent the number of tolerated bitflips for the whole pattern).

>   *
>   * Descriptor for the bad block table marker and the descriptor for the
>   * pattern which identifies good and bad blocks. The assumption is made
> @@ -64,6 +66,7 @@ struct nand_bbt_descr {
>  	int maxblocks;
>  	int reserved_block_code;
>  	uint8_t  *pattern;
> +	int toleratedbitdiff;

Do we really need to add a new field, can't we pass this value when
calling check_short_pattern.

Best Regards,

Boris



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 1/2] mtd: nand: use a lower value for badblockbits when working with MLC NAND

[Andrea Scian]

Sorry for the later feedback, but unfortunately I had to move to other
stuff before coming back to this topic

Il 15/03/2015 10:07, Boris Brezillon ha scritto:
> Hi Andrea,
> 
> On Fri,  6 Mar 2015 13:12:17 +0100
> rnd4@dave-tech.it wrote:
> 
>> From: Andrea Scian <andrea.scian@dave.eu>
>>
>> MLC NANDs have more bit flips that SLC. When looking for bad block
>> marker we have a lot of false positive if we check for the whole byte. To
>> avoid this tolerate a few (4 here) bit flips for byte.
> 
> I'm not sure sure we want to accept 4 bitflips for all MLC NANDs. IMHO
> this value should be chip dependent.

I agree

> I know there is currently no way to retrieve this information,

For this reason I just put a hardcoded value.

> so here are two suggestions:
> 
> 1/ make this value depend on the required NAND ecc strength
> (badblockbits = ecc_strength / 10 ?)
> 2/ let each controller change this value after nand_scan_ident
> depending on the detected chip until we find a generic solution to
> select this value

I'll try to figure out how to solve this
Any suggestion is welcome!

Regards,

-- 

Andrea SCIAN

DAVE Embedded Systems

Re: [PATCH 1/2] mtd: nand: use a lower value for badblockbits when working with MLC NAND

[Andrea Scian]

Dear Bean,

Il 21/07/2015 16:50, Bean Huo 霍斌斌 (beanhuo) ha scritto:
 > Hi,
 > What is status of this patch? I think 4 bits is make sense for all
 > MLC nand,
 > Bit flips on bad block mark should not be regarded as a bad block.
 >

I think that there was something wrong with your email, because I didn't 
see it in MTD ML archives, probably it has been blocked for some reason.

I didn't find a solution to implement Boris suggestions on how to choose 
the bad block bit threshold. In my implementation is still statically 
defined.

However I did some minor changes to the second patch, I'll send it in a 
few minutes.

Any feedback is welcome, of course ;-)

Kind Regards,

Andrea Scian


Il 03/04/2015 14:52, Andrea Scian ha scritto:
>
> Sorry for the later feedback, but unfortunately I had to move to other
> stuff before coming back to this topic
>
> Il 15/03/2015 10:07, Boris Brezillon ha scritto:
>> Hi Andrea,
>>
>> On Fri,  6 Mar 2015 13:12:17 +0100
>> rnd4@dave-tech.it wrote:
>>
>>> From: Andrea Scian <andrea.scian@dave.eu>
>>>
>>> MLC NANDs have more bit flips that SLC. When looking for bad block
>>> marker we have a lot of false positive if we check for the whole byte. To
>>> avoid this tolerate a few (4 here) bit flips for byte.
>>
>> I'm not sure sure we want to accept 4 bitflips for all MLC NANDs. IMHO
>> this value should be chip dependent.
>
> I agree
>
>> I know there is currently no way to retrieve this information,
>
> For this reason I just put a hardcoded value.
>
>> so here are two suggestions:
>>
>> 1/ make this value depend on the required NAND ecc strength
>> (badblockbits = ecc_strength / 10 ?)
>> 2/ let each controller change this value after nand_scan_ident
>> depending on the detected chip until we find a generic solution to
>> select this value
>
> I'll try to figure out how to solve this
> Any suggestion is welcome!
>
> Regards,
>

-- 

Andrea SCIAN

DAVE Embedded Systems

Re: [PATCH 1/2] mtd: nand: use a lower value for badblockbits when working with MLC NAND

[Brian Norris]

On Thu, Jul 23, 2015 at 11:24:54PM +0200, Andrea Scian wrote:
> Il 21/07/2015 16:50, Bean Huo 霍斌斌 (beanhuo) ha scritto:
> > Hi,
> > What is status of this patch? I think 4 bits is make sense for all
> > MLC nand,
> > Bit flips on bad block mark should not be regarded as a bad block.
> >
> 
> I think that there was something wrong with your email, because I
> didn't see it in MTD ML archives, probably it has been blocked for
> some reason.

X-Bad-Reply: 'Re:' in Subject but no References or In-Reply-To headers

I would let it through the moderation filter, but it would screw with
everybody's threading, and the entire contents are there in the quote
anyway.

Bean: please fix your mailer.

Regards,
Brian

Re: [PATCH 0/2] Use badblockbits-like approach in nand_bbt.c

[Boris Brezillon]

Hi Brian,

On Fri,  6 Mar 2015 13:12:16 +0100
rnd4@dave-tech.it wrote:

> 
> Hi all,
> 
> I'm currenlty working with an MLC NAND (MT29F32G08CBACAWP) on a iMX6 based
> platform (kernel 3.10.17).
> The first problem I have is about badblock detection. I experienced a lot of bit
> flips on badblock marker (some of them probalbly due misuse, e.g. powercut).
> 
> By looking into the MDT code, I've found that inside nand_base.c there's support
> for badblockbits feature, which, for short, allow to tolerate 1 or more bitflip
> when looking for BB.
> 
> In my experience usually the marker is 0x00 for bad blocks and 0xFF for good one
> (at least this is true for my current component).
> 
> Unfortunately this approach is not used into BBT code (nand_bbt.c) and thus I have
> a lot of false positive BB.
> 
> With the following patches I try to add badblockbits-like approach to nand_bbt.c
> code.
> 
> The first patch is optional and configure badblockbits depending on SLC vs MLC NAND.
> 
> The second patch implements the badblockbits approach into BBT scan.
> Please note that this is not usually enough, because many NAND controller override
> nand_bbt_descr and thus requires changes also the the specific controller driver.
> 
> The patch has been made in collaboration with Boris Brezillon (thank you for you
> suggestions Boris).
> 
> I tested it on a 3.10.17 kernel, but the second patch applies fine to
> v3.10 stable and latest 4.0-rc1 (the first patch needs some changes to apply to this
> one).
> 
> Feel free to comment it and tell me if something can be done in a better way

We recently discussed the option of retrieving BBM using raw access
mode, which will likely require something similar to what this series is
doing.
I'm still not happy with the selection of ->badblockbits (4 for MLC
NANDs and 8 otherwise), but maybe that's something we can adjust later.

What do you think?

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 0/2] Use badblockbits-like approach in nand_bbt.c

[Brian Norris]

+ Peter Pan

On Tue, Oct 13, 2015 at 12:46:20PM +0200, Boris Brezillon wrote:
> On Fri,  6 Mar 2015 13:12:16 +0100
> rnd4@dave-tech.it wrote:
> 
> > 
> > Hi all,
> > 
> > I'm currenlty working with an MLC NAND (MT29F32G08CBACAWP) on a iMX6 based
> > platform (kernel 3.10.17).
> > The first problem I have is about badblock detection. I experienced a lot of bit
> > flips on badblock marker (some of them probalbly due misuse, e.g. powercut).
> > 
> > By looking into the MDT code, I've found that inside nand_base.c there's support
> > for badblockbits feature, which, for short, allow to tolerate 1 or more bitflip
> > when looking for BB.
> > 
> > In my experience usually the marker is 0x00 for bad blocks and 0xFF for good one
> > (at least this is true for my current component).
> > 
> > Unfortunately this approach is not used into BBT code (nand_bbt.c) and thus I have
> > a lot of false positive BB.
> > 
> > With the following patches I try to add badblockbits-like approach to nand_bbt.c
> > code.
> > 
> > The first patch is optional and configure badblockbits depending on SLC vs MLC NAND.
> > 
> > The second patch implements the badblockbits approach into BBT scan.
> > Please note that this is not usually enough, because many NAND controller override
> > nand_bbt_descr and thus requires changes also the the specific controller driver.
> > 
> > The patch has been made in collaboration with Boris Brezillon (thank you for you
> > suggestions Boris).
> > 
> > I tested it on a 3.10.17 kernel, but the second patch applies fine to
> > v3.10 stable and latest 4.0-rc1 (the first patch needs some changes to apply to this
> > one).
> > 
> > Feel free to comment it and tell me if something can be done in a better way
> 
> We recently discussed the option of retrieving BBM using raw access
> mode, which will likely require something similar to what this series is
> doing.
> I'm still not happy with the selection of ->badblockbits (4 for MLC
> NANDs and 8 otherwise), but maybe that's something we can adjust later.
> 
> What do you think?

This patch set is old and lost in my mailbox, so I'm not looking at the
details. There is also an effort to refactor nand_bbt by Peter [1]. These
should probably be coordinated in some way.

Years ago, I was looking at this problem and considering having nand_bbt
call into the nand_base implementation of checkbad(). Now, it seems like
we should probably keep nand_bbt independent. We could still use a
callback that nand_base can supply to nand_bbt, if desired, but then
that means SPI NAND will have to do the same, if it's going to need
this. I'd like not to have this reimplemented in multiple places, nor do
I want to be configuring badblockbits in too many different places.

Is there a good way to satisfy this? Peter, did you address any of this
sort of stuff in your series, or is there still a scattering of bad
block marker code in both nand_base and nand_bbt?

Brian

[1] http://lists.infradead.org/pipermail/linux-mtd/2015-September/062084.html