[PATCH 0/1] Bad block markers here, there and everywhere

[PATCH 0/1] Bad block markers here, there and everywhere

[Ezequiel Garcia]

Hi everyone,

This commit adds a new option called NAND_BBT_DATA_BBM. The change itself
is pretty small and simple to understand: when the badblock_pattern sets the
NAND_BBT_DATA_BBM option, scan_block_fast() reads the data region instead
of the OOB region.

So, let me explain why we need this new feature.

Some NAND controllers provide its own unique view of a device page, where the
page is partitioned into several DATA,OOB regions. In other words, a flash that's
specified by the manufacturer:

|-------------------------|
|         DATA      | OOB |
|-------------------------|

is actually handled by the driver/controller as:

|-------------------------|
| DATA | OOB | DATA | OOB |
|-------------------------|

Now, this has a peculiar side-effect: the bad block marker is located at
position that is regarded as the data region in the controller's view.

Therefore, when the device is scanned for bad blocks (for the first
time) and the bad block table is built using the factory-marked blocks,
these markers are never found.

To address this case, we introduce a new option NAND_BBT_DATA_BBM.
Such option is used to read the data region of a a given page, instead
of the oob region, in the scan_block_fast() function. The page is read
as usual, using the length and offset parameters set in the bad block pattern.

Given drivers can customize the bad block pattern to match its specific
requirements, the new option allows to search for a bad block marker at
*any* position within a page.

This feature would be important and helpful to deal with the Armada 370/XP
(pxa3xx-nand driver) bad block markers factory (initial) detection.
Details about the NAND controller can be found in the last patchset:

  https://lwn.net/Articles/571062/

Also, it seems gpmi-nand would also benefit from this feature. The gpmi-nand
driver seem to go through great pains to deal with this case. So, instead of
doing weird things at a driver level we can just handle this in the core code.

So, how does it look?

Ezequiel Garcia (1):
  mtd: nand: Allow bad block markers to be found in the data region

 drivers/mtd/nand/nand_bbt.c | 11 +++++++++--
 include/linux/mtd/bbm.h     |  4 ++++
 2 files changed, 13 insertions(+), 2 deletions(-)

-- 
1.8.1.5

[PATCH 1/1] mtd: nand: Allow bad block markers to be found in the data region

[Ezequiel Garcia]

Some NAND controllers provide its own unique view of a flash device's page,
where the page is partitioned into several DATA,OOB regions.
In other words, a flash that's specified by the manufacturer as:

|-------------------------|
|         DATA      | OOB |
|-------------------------|

is actually handled by the driver/controller as:

|-------------------------|
| DATA | OOB | DATA | OOB |
|-------------------------|

This has a peculiar side-effect: the bad block marker is located at a position
that is regarded as the data region in the controller's view.

Therefore, when the device is scanned for bad blocks (for the first
time) and the bad block table is built using the factory-marked blocks,
these markers are never found.

To address this case, we introduce a new option NAND_BBT_DATA_BBM.
Such option is used to read the data region of a a given page, instead
of the oob region, in the scan_block_fast() function. The page is read
as usual, using the length and offset parameters set in the bad block pattern.

Given drivers can customize the bad block pattern to match its specific
requirements, the new option allows to search for a bad block marker at
*any* position within a page.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/nand_bbt.c | 11 +++++++++--
 include/linux/mtd/bbm.h     |  4 ++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index c0615d1..3f712cc 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -419,10 +419,17 @@ static int scan_block_fast(struct mtd_info *mtd, struct nand_bbt_descr *bd,
 	struct mtd_oob_ops ops;
 	int j, ret;
 
+	if (bd->options & NAND_BBT_DATA_BBM) {
+		ops.oobbuf = (buf + mtd->writesize);
+		ops.datbuf = buf;
+		ops.len = mtd->writesize;
+	} else {
+		ops.oobbuf = buf;
+		ops.datbuf = NULL;
+		ops.len = 0;
+	}
 	ops.ooblen = mtd->oobsize;
-	ops.oobbuf = buf;
 	ops.ooboffs = 0;
-	ops.datbuf = NULL;
 	ops.mode = MTD_OPS_PLACE_OOB;
 
 	for (j = 0; j < numpages; j++) {
diff --git a/include/linux/mtd/bbm.h b/include/linux/mtd/bbm.h
index 36bb6a5..75d590d 100644
--- a/include/linux/mtd/bbm.h
+++ b/include/linux/mtd/bbm.h
@@ -114,6 +114,10 @@ struct nand_bbt_descr {
  * entire spare area. Must be used with NAND_BBT_USE_FLASH.
  */
 #define NAND_BBT_NO_OOB_BBM	0x00080000
+/*
+ * Search for bad block marker in the data region, instead of the OOB.
+ */
+#define NAND_BBT_DATA_BBM	0x00100000
 
 /*
  * Flag set by nand_create_default_bbt_descr(), marking that the nand_bbt_descr
-- 
1.8.1.5

Re: [PATCH 0/1] Bad block markers here, there and everywhere

[Brian Norris]

On Tue, Nov 05, 2013 at 09:00:20AM -0300, Ezequiel Garcia wrote:
> This commit adds a new option called NAND_BBT_DATA_BBM. The change itself
> is pretty small and simple to understand: when the badblock_pattern sets the
> NAND_BBT_DATA_BBM option, scan_block_fast() reads the data region instead
> of the OOB region.

I think this type of scanning method is better suited to a different
type of solution: implement a custom nand_chip.bad_block() call-back.
Unfortunately, nand_base/nand_bbt are kind of inconsistent, so that some
code paths use nand_chip.bad_block() and some use nand_bbt.c's scanning
code to check for bad block markers, so this is not currently a good
solution.

I've been meaning to follow through with an improved version of this
patch for a while:

  http://lists.infradead.org/pipermail/linux-mtd/2012-June/042571.html

Such a patch provides several benefits, one of them being that drivers
like yours can easily provide a custom BBM location. What do you think?

Brian

Re: [PATCH 0/1] Bad block markers here, there and everywhere

[Ezequiel Garcia]

On Tue, Nov 05, 2013 at 10:07:43AM -0800, Brian Norris wrote:
> On Tue, Nov 05, 2013 at 09:00:20AM -0300, Ezequiel Garcia wrote:
> > This commit adds a new option called NAND_BBT_DATA_BBM. The change itself
> > is pretty small and simple to understand: when the badblock_pattern sets the
> > NAND_BBT_DATA_BBM option, scan_block_fast() reads the data region instead
> > of the OOB region.
> 
> I think this type of scanning method is better suited to a different
> type of solution: implement a custom nand_chip.bad_block() call-back.

Fully agreed (I guess you mean block_bad() right?)

> Unfortunately, nand_base/nand_bbt are kind of inconsistent, so that some
> code paths use nand_chip.bad_block() and some use nand_bbt.c's scanning
> code to check for bad block markers, so this is not currently a good
> solution.
> 

Which is why I couldn't implement a custom block_bad(). My particular
use case (which could match others) needs this customization in the
first scan. After that, once the bad block table is built, the in-flash
bad block markers are never touched.

> I've been meaning to follow through with an improved version of this
> patch for a while:
> 
>   http://lists.infradead.org/pipermail/linux-mtd/2012-June/042571.html
> 
> Such a patch provides several benefits, one of them being that drivers
> like yours can easily provide a custom BBM location. What do you think?
> 

Of course, that sounds much more flexible. Let me pick it where Matthieu left,
I'll read the patch and prepare something to discuss.

On the other side, I'm seeing the above patch was a bit argued :/ I'm
not sure why it got never merged, maybe you can give me some heads up
about potential drawbacks?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 0/1] Bad block markers here, there and everywhere

[Ezequiel Garcia]

On Tue, Nov 05, 2013 at 08:01:26PM -0300, Ezequiel Garcia wrote:
> On Tue, Nov 05, 2013 at 10:07:43AM -0800, Brian Norris wrote:
> > On Tue, Nov 05, 2013 at 09:00:20AM -0300, Ezequiel Garcia wrote:
> > > This commit adds a new option called NAND_BBT_DATA_BBM. The change itself
> > > is pretty small and simple to understand: when the badblock_pattern sets the
> > > NAND_BBT_DATA_BBM option, scan_block_fast() reads the data region instead
> > > of the OOB region.
> > 
> > I think this type of scanning method is better suited to a different
> > type of solution: implement a custom nand_chip.bad_block() call-back.
> 
> Fully agreed (I guess you mean block_bad() right?)
> 
> > Unfortunately, nand_base/nand_bbt are kind of inconsistent, so that some
> > code paths use nand_chip.bad_block() and some use nand_bbt.c's scanning
> > code to check for bad block markers, so this is not currently a good
> > solution.
> > 
> 
> Which is why I couldn't implement a custom block_bad(). My particular
> use case (which could match others) needs this customization in the
> first scan. After that, once the bad block table is built, the in-flash
> bad block markers are never touched.
> 
> > I've been meaning to follow through with an improved version of this
> > patch for a while:
> > 
> >   http://lists.infradead.org/pipermail/linux-mtd/2012-June/042571.html
> > 
> > Such a patch provides several benefits, one of them being that drivers
> > like yours can easily provide a custom BBM location. What do you think?
> > 
> 
> Of course, that sounds much more flexible. Let me pick it where Matthieu left,
> I'll read the patch and prepare something to discuss.
> 
> On the other side, I'm seeing the above patch was a bit argued :/ I'm
> not sure why it got never merged, maybe you can give me some heads up
> about potential drawbacks?

After reading the patch and reading the code, now I'm even more confused :)

The first thing that seems odd is the fact that the scan_block_fast()
path matches a pattern (which can be several bytes long), whereas the
default nand_block_bad() seem to check for just one byte.

This may or may not be an issue, after some thought, but it's not
a trivial change, IMHO.

The second thing, which was already discussed back in June-2012 is the
fact that scan_block_fast() uses mtd_read_oob() to read, but
nand_block_bad() just issues a READOOB command.

So, what do you propose? If you can give me some guidelines I've no problem
in preparing a (first/draft) patch to trigger the discussion.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 0/1] Bad block markers here, there and everywhere

[Ezequiel Garcia]

On Tue, Nov 05, 2013 at 08:15:22PM -0300, Ezequiel Garcia wrote:
> On Tue, Nov 05, 2013 at 08:01:26PM -0300, Ezequiel Garcia wrote:
> > On Tue, Nov 05, 2013 at 10:07:43AM -0800, Brian Norris wrote:
> > > On Tue, Nov 05, 2013 at 09:00:20AM -0300, Ezequiel Garcia wrote:
> > > > This commit adds a new option called NAND_BBT_DATA_BBM. The change itself
> > > > is pretty small and simple to understand: when the badblock_pattern sets the
> > > > NAND_BBT_DATA_BBM option, scan_block_fast() reads the data region instead
> > > > of the OOB region.
> > > 
> > > I think this type of scanning method is better suited to a different
> > > type of solution: implement a custom nand_chip.bad_block() call-back.
> > 
> > Fully agreed (I guess you mean block_bad() right?)
> > 
> > > Unfortunately, nand_base/nand_bbt are kind of inconsistent, so that some
> > > code paths use nand_chip.bad_block() and some use nand_bbt.c's scanning
> > > code to check for bad block markers, so this is not currently a good
> > > solution.
> > > 
> > 
> > Which is why I couldn't implement a custom block_bad(). My particular
> > use case (which could match others) needs this customization in the
> > first scan. After that, once the bad block table is built, the in-flash
> > bad block markers are never touched.
> > 
> > > I've been meaning to follow through with an improved version of this
> > > patch for a while:
> > > 
> > >   http://lists.infradead.org/pipermail/linux-mtd/2012-June/042571.html
> > > 
> > > Such a patch provides several benefits, one of them being that drivers
> > > like yours can easily provide a custom BBM location. What do you think?
> > > 
> > 
> > Of course, that sounds much more flexible. Let me pick it where Matthieu left,
> > I'll read the patch and prepare something to discuss.
> > 
> > On the other side, I'm seeing the above patch was a bit argued :/ I'm
> > not sure why it got never merged, maybe you can give me some heads up
> > about potential drawbacks?
> 
> After reading the patch and reading the code, now I'm even more confused :)
> 
> The first thing that seems odd is the fact that the scan_block_fast()
> path matches a pattern (which can be several bytes long), whereas the
> default nand_block_bad() seem to check for just one byte.
> 
> This may or may not be an issue, after some thought, but it's not
> a trivial change, IMHO.
> 
> The second thing, which was already discussed back in June-2012 is the
> fact that scan_block_fast() uses mtd_read_oob() to read, but
> nand_block_bad() just issues a READOOB command.
> 
> So, what do you propose? If you can give me some guidelines I've no problem
> in preparing a (first/draft) patch to trigger the discussion.

Ping?

This is an important issue for the pxa3xx driver and I'd like to move
forward with it.

However, as I previously said I'd like to discuss some more about your
proposal: currently scan_block_fast() uses mtd_read_oob() to read, and
nand_block_bad() issues a READOOB command, so it's not trivial to
replace the former with the latter.

Ideas?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 0/1] Bad block markers here, there and everywhere

[Brian Norris]

On Thu, Nov 14, 2013 at 03:58:46PM -0300, Ezequiel Garcia wrote:
> On Tue, Nov 05, 2013 at 08:15:22PM -0300, Ezequiel Garcia wrote:
> > On Tue, Nov 05, 2013 at 08:01:26PM -0300, Ezequiel Garcia wrote:
> > > On Tue, Nov 05, 2013 at 10:07:43AM -0800, Brian Norris wrote:
> > > > On Tue, Nov 05, 2013 at 09:00:20AM -0300, Ezequiel Garcia wrote:
> > > > > This commit adds a new option called NAND_BBT_DATA_BBM. The change itself
> > > > > is pretty small and simple to understand: when the badblock_pattern sets the
> > > > > NAND_BBT_DATA_BBM option, scan_block_fast() reads the data region instead
> > > > > of the OOB region.
> > > > 
> > > > I think this type of scanning method is better suited to a different
> > > > type of solution: implement a custom nand_chip.bad_block() call-back.
> > > 
> > > Fully agreed (I guess you mean block_bad() right?)
> > > 
> > > > Unfortunately, nand_base/nand_bbt are kind of inconsistent, so that some
> > > > code paths use nand_chip.bad_block() and some use nand_bbt.c's scanning
> > > > code to check for bad block markers, so this is not currently a good
> > > > solution.
> > > > 
> > > 
> > > Which is why I couldn't implement a custom block_bad(). My particular
> > > use case (which could match others) needs this customization in the
> > > first scan. After that, once the bad block table is built, the in-flash
> > > bad block markers are never touched.
> > > 
> > > > I've been meaning to follow through with an improved version of this
> > > > patch for a while:
> > > > 
> > > >   http://lists.infradead.org/pipermail/linux-mtd/2012-June/042571.html
> > > > 
> > > > Such a patch provides several benefits, one of them being that drivers
> > > > like yours can easily provide a custom BBM location. What do you think?
> > > > 
> > > 
> > > Of course, that sounds much more flexible. Let me pick it where Matthieu left,
> > > I'll read the patch and prepare something to discuss.
> > > 
> > > On the other side, I'm seeing the above patch was a bit argued :/ I'm
> > > not sure why it got never merged, maybe you can give me some heads up
> > > about potential drawbacks?
> > 
> > After reading the patch and reading the code, now I'm even more confused :)
> > 
> > The first thing that seems odd is the fact that the scan_block_fast()
> > path matches a pattern (which can be several bytes long), whereas the
> > default nand_block_bad() seem to check for just one byte.
> > 
> > This may or may not be an issue, after some thought, but it's not
> > a trivial change, IMHO.
> > 
> > The second thing, which was already discussed back in June-2012 is the
> > fact that scan_block_fast() uses mtd_read_oob() to read, but
> > nand_block_bad() just issues a READOOB command.
> > 
> > So, what do you propose? If you can give me some guidelines I've no problem
> > in preparing a (first/draft) patch to trigger the discussion.
> 
> Ping?
> 
> This is an important issue for the pxa3xx driver and I'd like to move
> forward with it.
> 
> However, as I previously said I'd like to discuss some more about your
> proposal: currently scan_block_fast() uses mtd_read_oob() to read, and
> nand_block_bad() issues a READOOB command, so it's not trivial to
> replace the former with the latter.
> 
> Ideas?

I believe my plan was to totally kill of the nand_chip.badblock_pattern
field first, since that's the source of unnecessary complexity (and a
perverted mixture of "BBT" (table) and "BBM" (marker)).

Most drivers already just use the default markers from
nand_base/nand_bbt, and any remaining ones can either just tweak the
nand_chip.badblockpos/nand_chip.bbt_options or (like pxa3xx_nand) can
define their own nand_chip.block_bad() callback, I think. Once there are
no users with a custom badblock_pattern, I think the nand_base
nand_block_bad() implementation should be equivalent to the nand_bbt
scan_block_fast() checks.

I think I started to do all of this a while back (and I have a few
changes queued somewhere), but I stalled on some problem. Or maybe I
just didn't make the time to finish testing it properly. But now that we
have a good reason to do so, I can get back to this issue--either
submitting or reviewing patches, and testing on my setups.

Brian