Re: [RFC] nand_btt : use nand chip->block_bad

[Matthieu CASTET <matthieu.castet@parrot.com>]

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

Hi,

Shmulik Ladkani a écrit :
> Hi Matthieu,
> 
> On Thu, 28 Jun 2012 17:47:22 +0200 Matthieu CASTET <matthieu.castet@parrot.com> wrote:
>>  	for (i = startblock; i < numblocks;) {
>>  		int ret;
>>  
>>  		BUG_ON(bd->options & NAND_BBT_NO_OOB);
>>  
>> -		if (bd->options & NAND_BBT_SCANALLPAGES)
>> -			ret = scan_block_full(mtd, bd, from, buf, readlen,
>> -					      scanlen, len);
>> -		else
>> -			ret = scan_block_fast(mtd, bd, from, buf, len);
>> -
>> +		ret = this->block_bad(mtd, from, 1);
>>  		if (ret < 0)
>>  			return ret;
>>  
> 
> Hmm, seems elegant, nand_bbt is not supposed to be elegant, what are we
> missing here? ;-))
> 
> - I think nand_chip ops are not supposed to be called directly from
>   outside nand driver (nand_base et al); instead the mtd interfaces
>   should be used.
>   OTOH, one might consider nand_bbt to be part of nand_base driver...
Yes but the current driver already used privare nand_base stuff like
"this->bbt_options". There is duplicate stuff like NAND_BBT_SCAN2NDPAGE that are
in this->bbt_options and bd->options
We can't call mtd interface, because it will calling nand_isbad_bbt and not
chip->block_bad. [1]

> 
> - The new scheme lacks the potential error correction offered by the
>   mtd_read_oob call (invoked from the original scan functions).
>   OTOH, currently, AFAIK, it is only offered by an out-of-tree driver.
Could you explain more here ?
The current scheme doesn't handle bitflip in bad block. We don't care about
error correction : bad block are not protected by ecc !

With the new scheme, we can be robust to bad block corruption : all we need to
do is "chip->badblockbits = 4;"

> 
> - The original scheme allows validating against an arbitrary
>   nand_bbt_descr, whereas 'block_bad' reads the 'badblockpos' byte.
>   Don't know if this is a real issue (need to look at the descriptors
>   used); and probably, 'block_bad' can be augmented to use a given
>   descriptor.
I will be interrested to know why we need to support that.
Are there flash where bad block are not in a fixed position ?

> 
> - To preserve all functionality, we need to augment 'block_bad'
>   implementors to support NAND_BBT_SCANALLPAGES (e.g. nand_block_bad
>   lacks this).
We already support NAND_BBT_SCANLASTPAGE, NAND_BBT_SCAN2NDPAGE.
NAND_BBT_SCANALLPAGES can be added. See the attached patch.


Matthieu


[1]
static int nand_block_checkbad(struct mtd_info *mtd, loff_t ofs, int getchip,
                   int allowbbt)
{
    struct nand_chip *chip = mtd->priv;

    if (!chip->bbt)
        return chip->block_bad(mtd, ofs, getchip);

    /* Return info from the table */
    return nand_isbad_bbt(mtd, ofs, allowbbt);
}

[-- Attachment #2: 0001-add-NAND_BBT_SCANALLPAGES-support-to-block_bad.patch --]
[-- Type: text/x-diff, Size: 1550 bytes --]

>From 03acf40c6bcb5231795d2bd5cf6838cfeaaf1c56 Mon Sep 17 00:00:00 2001
From: Matthieu CASTET <matthieu.castet@parrot.com>
Date: Fri, 29 Jun 2012 10:36:32 +0200
Subject: [PATCH] add NAND_BBT_SCANALLPAGES support to block_bad

---
 drivers/mtd/nand/nand_base.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index a11253a..5ed0a9b 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -335,6 +335,7 @@ static int nand_block_bad(struct mtd_info *mtd, loff_t ofs, int getchip)
 	int page, chipnr, res = 0, i = 0;
 	struct nand_chip *chip = mtd->priv;
 	u16 bad;
+	int max;
 
 	if (chip->bbt_options & NAND_BBT_SCANLASTPAGE)
 		ofs += mtd->erasesize - mtd->writesize;
@@ -350,6 +351,13 @@ static int nand_block_bad(struct mtd_info *mtd, loff_t ofs, int getchip)
 		chip->select_chip(mtd, chipnr);
 	}
 
+	if (chip->bbt_options & NAND_BBT_SCAN2NDPAGE)
+		max = 2;
+	else if (chip->bbt_options & NAND_BBT_SCANALLPAGES)
+		max = 1 << (chip->bbt_erase_shift - chip->page_shift);
+	else
+		max = 1;
+
 	do {
 		if (chip->options & NAND_BUSWIDTH_16) {
 			chip->cmdfunc(mtd, NAND_CMD_READOOB,
@@ -372,7 +380,7 @@ static int nand_block_bad(struct mtd_info *mtd, loff_t ofs, int getchip)
 		ofs += mtd->writesize;
 		page = (int)(ofs >> chip->page_shift) & chip->pagemask;
 		i++;
-	} while (!res && i < 2 && (chip->bbt_options & NAND_BBT_SCAN2NDPAGE));
+	} while (!res && i < max);
 
 	if (getchip)
 		nand_release_device(mtd);
-- 
1.7.10.4