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);
}