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

login
register
mail settings
Submitter Matthieu CASTET
Date June 29, 2012, 8:41 a.m.
Message ID <4FED6A2E.9010603@parrot.com>
Download mbox | patch
Permalink /patch/168018/
State New
Headers show

Comments

Matthieu CASTET - June 29, 2012, 8:41 a.m.
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);
}
Shmulik Ladkani - June 30, 2012, 8:02 p.m.
Hi,

On Fri, 29 Jun 2012 10:41:18 +0200 Matthieu CASTET <matthieu.castet@parrot.com> wrote:
> > - 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 !

I was refering to utilizing the ECC when reading the OOB, so we won't
get a false "bad" indication when reading the BBM from the OOB.

See this post (and subsequent ones):
http://lists.infradead.org/pipermail/linux-mtd/2012-June/042203.html

Regards,
Shmulik
Matthieu CASTET - July 2, 2012, 8:29 a.m.
Hi,

Shmulik Ladkani a écrit :
> Hi,
> 
> On Fri, 29 Jun 2012 10:41:18 +0200 Matthieu CASTET <matthieu.castet@parrot.com> wrote:
>>> - 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 !
> 
> I was refering to utilizing the ECC when reading the OOB, so we won't
> get a false "bad" indication when reading the BBM from the OOB.
> 
> See this post (and subsequent ones):
> http://lists.infradead.org/pipermail/linux-mtd/2012-June/042203.html
> 
But oob is not protected by ecc in normal configuration. I believe more than 95%
of current configuration don't protect bad block with ecc.
Manufacturer bad block are also not protected by ecc.

How could this work ?
Do you have example of linux driver that protect bad block with ecc ?

Finally if there is a config with bad block protected by ecc, we can provide
another chip->block_bad callback which use ecc.


Also note that the post speak of "nand_chip.badblockbits" [1] which can be used
with this patch.


Matthieu

[1]
Because when ECC is available on OOB, it is good to utilize it, so
that bitflips don't cause good blocks to be marked bad. Less
importantly, when bad block markers are written by Linux (with ECC),
then these markers can be read back more reliably. (I note that there
is a nand_chip.badblockbits option that could theoretically alleviate
the first issue, but I see it is not actually implemented throughout
nand_bbt.c - only in nand_base.c)
Brian Norris - July 24, 2012, 3:53 a.m.
Hi Matthieu,

Sorry for taking an extraordinarily long amount of time to respond.
This isn't a very easy issue to address, and I'm kind of behind on
email anyway. I'll respond directly to a few of your comments, and
then just include my general comments about this RFC.

On Mon, Jul 2, 2012 at 1:29 AM, Matthieu CASTET
<matthieu.castet@parrot.com> wrote:
> Shmulik Ladkani a écrit :
>> On Fri, 29 Jun 2012 10:41:18 +0200 Matthieu CASTET <matthieu.castet@parrot.com> wrote:
>>>> - 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 !
>>
>> I was refering to utilizing the ECC when reading the OOB, so we won't
>> get a false "bad" indication when reading the BBM from the OOB.
>>
>> See this post (and subsequent ones):
>> http://lists.infradead.org/pipermail/linux-mtd/2012-June/042203.html
>>
> But oob is not protected by ecc in normal configuration. I believe more than 95%
> of current configuration don't protect bad block with ecc.
> Manufacturer bad block are also not protected by ecc.
>
> How could this work ?
> Do you have example of linux driver that protect bad block with ecc ?

I think we've been over this a few times...my out-of-tree driver
protects OOB ECC. This is partly a driver implementation choice and
partly a property of the ASIC. I won't re-explain here unless you're
really curious.

> Finally if there is a config with bad block protected by ecc, we can provide
> another chip->block_bad callback which use ecc.

It seems pointless to re-implement the entire chip->block_bad just to
enable/disable ECC: the rest of the function should be standard and
irreplaceable, I believe. So I would expect that overriding
chip->block_bad should be reserved for systems that need an entirely
new BBM position or pattern that is not standard in nand_base.

> Also note that the post speak of "nand_chip.badblockbits" [1] which can be used
> with this patch.

Right, that is one positive side. But there are a few failings of the
current chip->block_bad implementation, I think. With a little more
work, though, it could be a sensible replacement for some portions of
nand_bbt.c.

General comments:

I think that the duplication of code between nand_bbt.c and
nand_base.c (nand_block_bad) is kind of absurd. And the nand_bbt.c
code is very much spaghetti code, IMO, with a sprinkling of strange
and/or little-understood features. So I agree with Matthieu that we
could unify to using chip->block_bad. I feel like this gives several
benefits. Some of this may be a little redundant:
* get rid of the difficult-to-understand spaghetti code seen in nand_bbt.c
* prevent code duplication in nand_base/nand_bbt
* fix the nand_block_bad implementation, which seems to be
inconsistent with nand_bbt (I'm not sure; see the second disadvantage
below...)
* make consistent use of the chip->badblockbits feature (this
addresses some bitflip issues from other thread(s))
* allow consistent overriding of chip->block_bad and
chip->block_markbad together [a]
* we can properly separate some BBM and BBT code (note the Marker vs.
Table) to nand_base and nand_bbt respectively

Disadvantages:
* need to improve nand_block_bad code [b]
* need to understand some NAND_BBT_* options better (e.g.,
NAND_BBT_SCANALLPAGES and NAND_BBT_SCANEMPTY). This may not really be
a disadvantage, as they should be understood/documented better or else
removed. See some problems we're having with SCANEMPTY:
http://lists.infradead.org/pipermail/linux-mtd/2012-July/042902.html

Now, I have a separate question:
Suppose we replace some nand_bbt code with the nand_block_bad code in
nand_base.c, and we make use of badblockbits to solve the bitflip
problems I brought up. I still don't see a reason we can't read/write
with MTD_OPS_PLACE_OOB instead of MTD_OPS_RAW. There is *no*
difference between these options for most current implementations,
regarding ECC protecting OOB as remarked previously. But it provides
only *benefit* for my driver and allows other systems (e.g., docg3,
docg4) to do the same if desired. So why can't the default
implementation use both badblockbits and MTD_OPS_PLACE_OOB
simultaneously?

Thanks,
Brian

[a] Does it really make sense to override both in the current code
base? It looks like chip->block_bad won't take effect if you have
*any* BBT (on-flash OR in-memory!). So you can't properly redefine new
BBM marking/checking for a non-standard scheme, as nand_bbt gives you
no choice...

[b] e.g., why does nand_block_bad manually run NAND_CMD_READOOB,
whereas nand_default_markbad uses the nand_do_write_oob() wrapper? I
feel like we could use nand_do_read_oob()...
Shmulik Ladkani - July 25, 2012, 11:02 a.m.
Hi Brian,

On Mon, 23 Jul 2012 20:53:56 -0700 Brian Norris <computersforpeace@gmail.com> wrote:
> Now, I have a separate question:
> Suppose we replace some nand_bbt code with the nand_block_bad code in
> nand_base.c, and we make use of badblockbits to solve the bitflip
> problems I brought up. I still don't see a reason we can't read/write
> with MTD_OPS_PLACE_OOB instead of MTD_OPS_RAW. There is *no*
> difference between these options for most current implementations,
> regarding ECC protecting OOB as remarked previously. But it provides
> only *benefit* for my driver and allows other systems (e.g., docg3,
> docg4) to do the same if desired. So why can't the default
> implementation use both badblockbits and MTD_OPS_PLACE_OOB
> simultaneously?

Well, I can't tell for sure.

But as I'm rethinking this, I'm getting more convinced MTD_OPS_RAW
should be used.
(took me a while to understand Matthieu's arguments...)

1. Factory marked bad blocks

For the blocks factory marked bad, the manufacturers simply state
"read position X in OOB, and if not entirely 0xff - consider the
block bad".
I guess they provide no guarantees regarding the page content in
general, and specifically the ECC part (as they usually don't even
know what ECC layout and algorithm is going to be used).
So applying ECC on the read makes no sense.

2. Blocks that go bad during use

Suppose you had a one system software, with an OOB BBM setup, running
and using the nand chip, and then you boot using a new system software
that uses BBT (hence scans and builds the BBT on first boot).

Usually, the manufacturers state "if erase has failed, software must
mark the block bad".
Suppose software adhere to that recommendation.

Now for those blocks marked bad by 1st system, you have NO guarantees
regarding the content of the block, because the last erase operation
(the one that lead the SW to mark the block bad) did not complete
succesfully.

(BTW, in a recent patch of yours, nand_default_block_markbad attempts to
erase the block PRIOR writing the BBM to the OOB; but this is not a
must on SLC, older linux systems lacked this patch, and obviously even
if we attempt the "last erase prior mark" we have no guarantees that the
last erase will indeed succeed this time)

Point is, again, no guarantees on block content (including OOB portion).
So applying ECC on read, by the second system scanning the nand, makes
no sense (would produce bogus results).

(Also, some software systems even mark the block bad if page-program
operation had failed, probably without erasing it first).


To conclude, MTD_OPS_RAW seems more correct IMO. It is simplistic by
nature, not relying on page content, OOB content or ECC.

As I understand, the motivation using MTD_OPS_PLACE_OOB is to overcome a
false positive condition, where a good block is accidentally considered
bad by the software, due to a bitflip (1 to 0) in the badblockpos.

I guess utilizing badblockbits may provide good coverage to that
condition.

Regards,
Shmulik
Brian Norris - Aug. 6, 2012, 10:21 p.m.
Hi Shmulik,

On Wed, Jul 25, 2012 at 4:02 AM, Shmulik Ladkani
<shmulik.ladkani@gmail.com> wrote:
> On Mon, 23 Jul 2012 20:53:56 -0700 Brian Norris <computersforpeace@gmail.com> wrote:
>> Now, I have a separate question:
>> Suppose we replace some nand_bbt code with the nand_block_bad code in
>> nand_base.c, and we make use of badblockbits to solve the bitflip
>> problems I brought up. I still don't see a reason we can't read/write
>> with MTD_OPS_PLACE_OOB instead of MTD_OPS_RAW. There is *no*
>> difference between these options for most current implementations,
>> regarding ECC protecting OOB as remarked previously. But it provides
>> only *benefit* for my driver and allows other systems (e.g., docg3,
>> docg4) to do the same if desired. So why can't the default
>> implementation use both badblockbits and MTD_OPS_PLACE_OOB
>> simultaneously?
>
> Well, I can't tell for sure.
>
> But as I'm rethinking this, I'm getting more convinced MTD_OPS_RAW
> should be used.
> (took me a while to understand Matthieu's arguments...)
>
> 1. Factory marked bad blocks
...
> So applying ECC on the read makes no sense.

Yes, that is understood. But in that case, shouldn't any ECC algorithm
simply return the raw data, with a -EBADMSG? So we're no worse off
than with MTD_OPS_RAW mode.

> 2. Blocks that go bad during use
>
> Suppose you had a one system software, with an OOB BBM setup, running
> and using the nand chip, and then you boot using a new system software
> that uses BBT (hence scans and builds the BBT on first boot).
>
> Usually, the manufacturers state "if erase has failed, software must
> mark the block bad".
> Suppose software adhere to that recommendation.

Just a side, pedantic note: the datasheet specification given doesn't
specify *how* you mark the block bad. Specifically, it really isn't
required to be in the OOB area of the bad block. That area was only
specified for factory-marked bad blocks, and it seems to been extended
as an established convention in MTD/NAND. It could just as well be
*only* a flash-based bad block table: hence, the NAND_BBT_NO_OOB_BBM
option (see include/linux/mtd/bbm.h).

> Now for those blocks marked bad by 1st system, you have NO guarantees
> regarding the content of the block, because the last erase operation
> (the one that lead the SW to mark the block bad) did not complete
> succesfully.

(Another side note:)
Right, we can't guarantee the erase, nor can we even 100% guarantee
that the bad block marker writes properly. Instead, my patches have
made attempts to improve flash-based BBT in conjunction with OOB BBMs.
When writing BBM to a true bad block, I think we can only make a best
effort.

> (BTW, in a recent patch of yours, nand_default_block_markbad attempts to
> erase the block PRIOR writing the BBM to the OOB; but this is not a
> must on SLC, older linux systems lacked this patch, and obviously even
> if we attempt the "last erase prior mark" we have no guarantees that the
> last erase will indeed succeed this time)

I think I understand your point here, but to confirm: you're not
suggesting that it is *bad* to try to erase before writing BBM, are
you? I was attempting this as a best-effort to cleanly write BBM, and
we don't care if the erase actually fails.

> To conclude, MTD_OPS_RAW seems more correct IMO. It is simplistic by
> nature, not relying on page content, OOB content or ECC.
>
> As I understand, the motivation using MTD_OPS_PLACE_OOB is to overcome a
> false positive condition, where a good block is accidentally considered
> bad by the software, due to a bitflip (1 to 0) in the badblockpos.
>
> I guess utilizing badblockbits may provide good coverage to that
> condition.

Yes, I agree that badblockbits is a good solution. I'm still not sure
that any of the above arguments really suggest that we *can't* apply
ECC, but for any important case, badblockbits would be more robust. I
think I will look into fleshing out Matthieu's RFC at some point,
unless he's already working on it.

Brian
Shmulik Ladkani - Aug. 7, 2012, 7:09 a.m.
Hi Brian,

On Mon, 6 Aug 2012 15:21:19 -0700 Brian Norris <computersforpeace@gmail.com> wrote:
> Hi Shmulik,
> 
> On Wed, Jul 25, 2012 at 4:02 AM, Shmulik Ladkani
> <shmulik.ladkani@gmail.com> wrote:
> > But as I'm rethinking this, I'm getting more convinced MTD_OPS_RAW
> > should be used.
> > (took me a while to understand Matthieu's arguments...)
> >
> > 1. Factory marked bad blocks
> ...
> > So applying ECC on the read makes no sense.
> 
> Yes, that is understood. But in that case, shouldn't any ECC algorithm
> simply return the raw data, with a -EBADMSG? So we're no worse off
> than with MTD_OPS_RAW mode.

It should, probably.
But I found out that for particular inputs, there's an algorithm that
might incorrectly report EUCLEAN (correctable error) for multibit
errors.

> > 2. Blocks that go bad during use
> >
> > Suppose you had a one system software, with an OOB BBM setup, running
> > and using the nand chip, and then you boot using a new system software
> > that uses BBT (hence scans and builds the BBT on first boot).
> >
> > Usually, the manufacturers state "if erase has failed, software must
> > mark the block bad".
> > Suppose software adhere to that recommendation.
> 
> Just a side, pedantic note: the datasheet specification given doesn't
> specify *how* you mark the block bad. Specifically, it really isn't
> required to be in the OOB area of the bad block. That area was only
> specified for factory-marked bad blocks, and it seems to been extended
> as an established convention in MTD/NAND. It could just as well be
> *only* a flash-based bad block table: hence, the NAND_BBT_NO_OOB_BBM
> option (see include/linux/mtd/bbm.h).

Agreed, well known.
I'm specifically referring to the case SW uses OOB BBM, just to
emphasize the argument regarding no guarantees of the page/oob/ecc
content (where the BBM needs to be placed).

> > (BTW, in a recent patch of yours, nand_default_block_markbad attempts to
> > erase the block PRIOR writing the BBM to the OOB; but this is not a
> > must on SLC, older linux systems lacked this patch, and obviously even
> > if we attempt the "last erase prior mark" we have no guarantees that the
> > last erase will indeed succeed this time)
> 
> I think I understand your point here, but to confirm: you're not
> suggesting that it is *bad* to try to erase before writing BBM, are
> you? I was attempting this as a best-effort to cleanly write BBM, and
> we don't care if the erase actually fails.

No, I have no problem with the attempt to erase.

Again, just arguing that the page/oob/ecc content is unexpected and
as such, applying ECC is meaningless.

I think my 1st argument, regarding the content of factory marked blocks,
is the strongest and most relevant one; I simply tried to think of cases
where BBM is set by SW, and argue that these cases also does not provide
any guarantees that future ECC read on this page will give any
meaningful results.

Regards,
Shmulik
Brian Norris - Sept. 18, 2012, 1:06 a.m.
Hi Shmulik, Matthieu, and others,

On Mon, Aug 6, 2012 at 3:21 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Wed, Jul 25, 2012 at 4:02 AM, Shmulik Ladkani
>> I guess utilizing badblockbits may provide good coverage to that
>> condition.
>
> Yes, I agree that badblockbits is a good solution. I'm still not sure
> that any of the above arguments really suggest that we *can't* apply
> ECC, but for any important case, badblockbits would be more robust. I
> think I will look into fleshing out Matthieu's RFC at some point,
> unless he's already working on it.

I've done a little work at trying to flesh out this RFC, as I'm
thinking that we should more cleanly separate NAND BBT code and NAND
BBM code - i.e., bad block *table* vs. bad block *marker*. Markers
should be handled in a standard way in nand_base.c
(nand_default_block_markbad and nand_block_bad) and then nand_bbt.c
can be left the generic, focused job of handling the table. Now, that
all sounds nice, but I've run across at least one difficulty... This
approach should lead to the removal of nand_chip.badblock_pattern
entirely, as we would only be supporting the nand_base.c badblock
marker pattern, using nand_chip.badblockpos and a few
nand_chip.bbt_options flags. This leads me to analyzing all the
strange forms of badblock_pattern seen in various drivers. I'm not
sure all of them are even well thought out...

The following files use badblock_pattern to varying degrees. Some are
just duplicating some nand_base stuff, where we can replace the
badblock_pattern with something simple like "chip->badblockpos = 0" or
setting a few "chip->bbt_options |= NAND_BBT_SCAN*" options. But it's
not all so simple:

arch/arm/mach-pxa/corgi.c
arch/arm/mach-pxa/eseries.c
arch/arm/mach-pxa/poodle.c
arch/arm/mach-pxa/spitz.c
arch/arm/mach-pxa/tosa.c
drivers/mtd/nand/bcm_umi_nand.c
drivers/mtd/nand/docg4.c
drivers/mtd/nand/fsl_elbc_nand.c
drivers/mtd/nand/gpmi-nand/gpmi-nand.c
drivers/mtd/nand/nand_base.c
drivers/mtd/nand/omap2.c
drivers/mtd/nand/sh_flctl.c
drivers/mtd/nand/sharpsl.c
drivers/mtd/nand/tmio_nand.c
drivers/mtd/onenand/onenand_bbt.c
include/linux/mfd/tmio.h
include/linux/mtd/sharpsl.h

Any thoughts on how to tackle this? Or is it even worth doing
properly? Is there a policy for dealing with old/unmaintained drivers
here, if I can't get a response from driver authors?

Thanks,
Brian
Brian Norris - Sept. 26, 2012, 10:43 p.m.
On Mon, Sep 17, 2012 at 6:28 PM, Scott Wood <scottwood@freescale.com> wrote:
> On 09/17/2012 08:06:54 PM, Brian Norris wrote:
>> On Mon, Aug 6, 2012 at 3:21 PM, Brian Norris <computersforpeace@gmail.com> wrote:
...
>> The following files use badblock_pattern to varying degrees. Some are
>> just duplicating some nand_base stuff, where we can replace the
>> badblock_pattern with something simple like "chip->badblockpos = 0" or
>> setting a few "chip->bbt_options |= NAND_BBT_SCAN*" options. But it's
>> not all so simple:
>>
>> arch/arm/mach-pxa/corgi.c
>> arch/arm/mach-pxa/eseries.c
>> arch/arm/mach-pxa/poodle.c
>> arch/arm/mach-pxa/spitz.c
>> arch/arm/mach-pxa/tosa.c
>> drivers/mtd/nand/bcm_umi_nand.c
>> drivers/mtd/nand/docg4.c
>> drivers/mtd/nand/fsl_elbc_nand.c
>> drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>> drivers/mtd/nand/nand_base.c
>> drivers/mtd/nand/omap2.c
>> drivers/mtd/nand/sh_flctl.c
>> drivers/mtd/nand/sharpsl.c
>> drivers/mtd/nand/tmio_nand.c
>> drivers/mtd/onenand/onenand_bbt.c
>> include/linux/mfd/tmio.h
>> include/linux/mtd/sharpsl.h
>>
>> Any thoughts on how to tackle this? Or is it even worth doing
>> properly? Is there a policy for dealing with old/unmaintained drivers
>> here, if I can't get a response from driver authors?
>
> fsl_elbc_nand uses this for historical reasons, to retain compatibility with
> the original OOB layout which only reserved one byte for the bad block
> marker and let users write to the second byte.  This controller only
> supports 8-bit chips.

OK. Could this be fixed up by forcing nand_chip.badblockpos=0 in
fsl_elbc_nand? I think nand_base/nand_bbt are configured to read/write
only a single byte for 8-bit chips now.

Also, if done properly, this change could still support other legacy
layouts by allowing them to provide their own nand_chip.block_markbad
and nand_chip.block_bad implementations.

> See commit 97ae023648e764f794ffb9c52da109d6caf09c47

I can't find such a commit in upstream. Perhaps you're referring to a
private git tree?

Brian
Brian Norris - Sept. 26, 2012, 11:15 p.m.
Hi Scott,

On Wed, Sep 26, 2012 at 3:57 PM, Scott Wood <scottwood@freescale.com> wrote:
> On 09/26/2012 05:43:35 PM, Brian Norris wrote:
>>
>> On Mon, Sep 17, 2012 at 6:28 PM, Scott Wood <scottwood@freescale.com>
>> wrote:
>> > On 09/17/2012 08:06:54 PM, Brian Norris wrote:
>> >> On Mon, Aug 6, 2012 at 3:21 PM, Brian Norris
>> >> <computersforpeace@gmail.com> wrote:
>> ...
>> >> The following files use badblock_pattern to varying degrees. Some are
>> >> just duplicating some nand_base stuff, where we can replace the
>> >> badblock_pattern with something simple like "chip->badblockpos = 0" or
>> >> setting a few "chip->bbt_options |= NAND_BBT_SCAN*" options. But it's
>> >> not all so simple:
>> >>
>> >> arch/arm/mach-pxa/corgi.c
>> >> arch/arm/mach-pxa/eseries.c
>> >> arch/arm/mach-pxa/poodle.c
>> >> arch/arm/mach-pxa/spitz.c
>> >> arch/arm/mach-pxa/tosa.c
>> >> drivers/mtd/nand/bcm_umi_nand.c
>> >> drivers/mtd/nand/docg4.c
>> >> drivers/mtd/nand/fsl_elbc_nand.c
>> >> drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>> >> drivers/mtd/nand/nand_base.c
>> >> drivers/mtd/nand/omap2.c
>> >> drivers/mtd/nand/sh_flctl.c
>> >> drivers/mtd/nand/sharpsl.c
>> >> drivers/mtd/nand/tmio_nand.c
>> >> drivers/mtd/onenand/onenand_bbt.c
>> >> include/linux/mfd/tmio.h
>> >> include/linux/mtd/sharpsl.h
>> >>
>> >> Any thoughts on how to tackle this? Or is it even worth doing
>> >> properly? Is there a policy for dealing with old/unmaintained drivers
>> >> here, if I can't get a response from driver authors?
>> >
>> > fsl_elbc_nand uses this for historical reasons, to retain compatibility
>> > with
>> > the original OOB layout which only reserved one byte for the bad block
>> > marker and let users write to the second byte.  This controller only
>> > supports 8-bit chips.
>>
>> OK. Could this be fixed up by forcing nand_chip.badblockpos=0 in
>> fsl_elbc_nand? I think nand_base/nand_bbt are configured to read/write
>> only a single byte for 8-bit chips now.
>
>
> Good, in that case I think we can just drop it.  No need to override
> nand_chip.badblockpos; it's only the size that we were overriding.

Yep. I just figured that out myself.

>> > See commit 97ae023648e764f794ffb9c52da109d6caf09c47
>>
>> I can't find such a commit in upstream. Perhaps you're referring to a
>> private git tree?
>
>
> Oops, that was the U-Boot tree. :-P
>
> The Linux commit is 452db2724351ff3d9416a183a7955e00ab4e6ab4

Found that as well. I should just put a few minute delay on the "send"
button, then I'd have my answers :)

Thanks for the help, I'll send a patch shortly; your testing (if
possible) and Ack would be appreciated.

Brian

Patch

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