[PATCH v2 0/3] mtd: Add sysfs entries and account BBT blocks properly

[PATCH v2 0/3] mtd: Add sysfs entries and account BBT blocks properly

[Ezequiel Garcia]

Here's a new round of the patchset to add sysfs entries for the ECC stats
struct fields. See the discussion around previous version for context:

http://lists.infradead.org/pipermail/linux-mtd/2014-March/052804.html

Changes from v1:

  * As agreed I've added a sysfs file per field to replace the ecc_stats
    sysfs file. The current proposal follows the "one file per value" sysfs
    rule, adding the following device attributes:
    - corrected_bits (ecc_stats.corrected)
    - uncorrectable_errors (ecc_stats.failed)
    - bad_blocks (ecc_stats.badblocks)
    - bbt_blocks (ecc_stats.bbtblocks)

  * Following Brian's request, added small wrapper nand_block_isreserved()
    that checks if the NAND has a bad block table before querying the number
    of reserved blocks.

  * Finally, this new series drops patch: "nand: Account the blocks used by
    the BBT in the ecc_stats". It's not yet clear how to best deal with the
    current inconsistencies in MTD master and partition handling.

The first patch adds the sysfs entries. The rest of the patches fixes the
bbtblock accounting and allows to keep track of the reserved blocks correctly.

This is a very old issue, probably around since the dawn of time,
and nobody seemed to care much about it. However, with the introduction of
the sysfs access, it's easier to access this stats and easier to
see the issue.

Ezequiel Garcia (3):
  mtd: Add sysfs attributes to expose the ECC stats fields
  mtd: Introduce mtd_block_isreserved()
  mtd: Account for BBT blocks when a partition is being allocated

 drivers/mtd/mtdcore.c        | 60 ++++++++++++++++++++++++++++++++++++++++++--
 drivers/mtd/mtdpart.c        | 13 +++++++++-
 drivers/mtd/nand/nand_base.c | 18 +++++++++++++
 drivers/mtd/nand/nand_bbt.c  | 14 +++++++++++
 include/linux/mtd/mtd.h      |  2 ++
 include/linux/mtd/nand.h     |  1 +
 6 files changed, 105 insertions(+), 3 deletions(-)

-- 
1.9.1

[PATCH v2 1/3] mtd: Add sysfs attributes to expose the ECC stats fields

[Ezequiel Garcia]

These new sysfs device attributes allows to retrieve the ECC
and bad block stats by poking a sysfs file, which is often more convenient
than using the ioctl.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/mtdcore.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 21ce0f4..d4dcc45 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -298,6 +298,48 @@ static ssize_t mtd_ecc_step_size_show(struct device *dev,
 }
 static DEVICE_ATTR(ecc_step_size, S_IRUGO, mtd_ecc_step_size_show, NULL);
 
+static ssize_t mtd_ecc_stats_corrected_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct mtd_info *mtd = dev_get_drvdata(dev);
+	struct mtd_ecc_stats *ecc_stats = &mtd->ecc_stats;
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", ecc_stats->corrected);
+}
+static DEVICE_ATTR(corrected_bits, S_IRUGO,
+		   mtd_ecc_stats_corrected_show, NULL);
+
+static ssize_t mtd_ecc_stats_errors_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct mtd_info *mtd = dev_get_drvdata(dev);
+	struct mtd_ecc_stats *ecc_stats = &mtd->ecc_stats;
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", ecc_stats->failed);
+}
+static DEVICE_ATTR(uncorrectable_errors, S_IRUGO,
+		   mtd_ecc_stats_errors_show, NULL);
+
+static ssize_t mtd_badblocks_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct mtd_info *mtd = dev_get_drvdata(dev);
+	struct mtd_ecc_stats *ecc_stats = &mtd->ecc_stats;
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", ecc_stats->badblocks);
+}
+static DEVICE_ATTR(bad_blocks, S_IRUGO, mtd_badblocks_show, NULL);
+
+static ssize_t mtd_bbtblocks_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct mtd_info *mtd = dev_get_drvdata(dev);
+	struct mtd_ecc_stats *ecc_stats = &mtd->ecc_stats;
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", ecc_stats->bbtblocks);
+}
+static DEVICE_ATTR(bbt_blocks, S_IRUGO, mtd_bbtblocks_show, NULL);
+
 static struct attribute *mtd_attrs[] = {
 	&dev_attr_type.attr,
 	&dev_attr_flags.attr,
@@ -310,6 +352,10 @@ static struct attribute *mtd_attrs[] = {
 	&dev_attr_name.attr,
 	&dev_attr_ecc_strength.attr,
 	&dev_attr_ecc_step_size.attr,
+	&dev_attr_corrected_bits.attr,
+	&dev_attr_uncorrectable_errors.attr,
+	&dev_attr_bad_blocks.attr,
+	&dev_attr_bbt_blocks.attr,
 	&dev_attr_bitflip_threshold.attr,
 	NULL,
 };
-- 
1.9.1

[PATCH v2 2/3] mtd: Introduce mtd_block_isreserved()

[Ezequiel Garcia]

In addition to mtd_block_isbad(), which checks if a block is bad or reserved,
it's needed to check if a block is reserved only (but not bad). This commit
adds an MTD interface for it, in a similar fashion to mtd_block_isbad().

While here, fix mtd_block_isbad() so the out-of-bounds checking is done the
callback check.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/mtdcore.c        | 14 ++++++++++++--
 drivers/mtd/mtdpart.c        |  9 +++++++++
 drivers/mtd/nand/nand_base.c | 18 ++++++++++++++++++
 drivers/mtd/nand/nand_bbt.c  | 14 ++++++++++++++
 include/linux/mtd/mtd.h      |  2 ++
 include/linux/mtd/nand.h     |  1 +
 6 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index d4dcc45..0017ef6 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -1044,12 +1044,22 @@ int mtd_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 }
 EXPORT_SYMBOL_GPL(mtd_is_locked);
 
-int mtd_block_isbad(struct mtd_info *mtd, loff_t ofs)
+int mtd_block_isreserved(struct mtd_info *mtd, loff_t ofs)
 {
-	if (!mtd->_block_isbad)
+	if (ofs < 0 || ofs > mtd->size)
+		return -EINVAL;
+	if (!mtd->_block_isreserved)
 		return 0;
+	return mtd->_block_isreserved(mtd, ofs);
+}
+EXPORT_SYMBOL_GPL(mtd_block_isreserved);
+
+int mtd_block_isbad(struct mtd_info *mtd, loff_t ofs)
+{
 	if (ofs < 0 || ofs > mtd->size)
 		return -EINVAL;
+	if (!mtd->_block_isbad)
+		return 0;
 	return mtd->_block_isbad(mtd, ofs);
 }
 EXPORT_SYMBOL_GPL(mtd_block_isbad);
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 1ca9aec..921e8c6 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -290,6 +290,13 @@ static void part_resume(struct mtd_info *mtd)
 	part->master->_resume(part->master);
 }
 
+static int part_block_isreserved(struct mtd_info *mtd, loff_t ofs)
+{
+	struct mtd_part *part = PART(mtd);
+	ofs += part->offset;
+	return part->master->_block_isreserved(part->master, ofs);
+}
+
 static int part_block_isbad(struct mtd_info *mtd, loff_t ofs)
 {
 	struct mtd_part *part = PART(mtd);
@@ -422,6 +429,8 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
 		slave->mtd._unlock = part_unlock;
 	if (master->_is_locked)
 		slave->mtd._is_locked = part_is_locked;
+	if (master->_block_isreserved)
+		slave->mtd._block_isreserved = part_block_isreserved;
 	if (master->_block_isbad)
 		slave->mtd._block_isbad = part_block_isbad;
 	if (master->_block_markbad)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 7853b9b..65f59dd 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -487,6 +487,23 @@ static int nand_check_wp(struct mtd_info *mtd)
  * nand_block_checkbad - [GENERIC] Check if a block is marked bad
  * @mtd: MTD device structure
  * @ofs: offset from device start
+ *
+ * Check if the block is mark as reserved.
+ */
+static int nand_block_isreserved(struct mtd_info *mtd, loff_t ofs)
+{
+	struct nand_chip *chip = mtd->priv;
+
+	if (!chip->bbt)
+		return 0;
+	/* Return info from the table */
+	return nand_isreserved_bbt(mtd, ofs);
+}
+
+/**
+ * nand_block_checkbad - [GENERIC] Check if a block is marked bad
+ * @mtd: MTD device structure
+ * @ofs: offset from device start
  * @getchip: 0, if the chip is already selected
  * @allowbbt: 1, if its allowed to access the bbt area
  *
@@ -4042,6 +4059,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 	mtd->_unlock = NULL;
 	mtd->_suspend = nand_suspend;
 	mtd->_resume = nand_resume;
+	mtd->_block_isreserved = nand_block_isreserved;
 	mtd->_block_isbad = nand_block_isbad;
 	mtd->_block_markbad = nand_block_markbad;
 	mtd->writebufsize = mtd->writesize;
diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index c0615d1..34db1e5 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -1310,6 +1310,20 @@ int nand_default_bbt(struct mtd_info *mtd)
 }
 
 /**
+ * nand_isreserved_bbt - [NAND Interface] Check if a block is reserved
+ * @mtd: MTD device structure
+ * @offs: offset in the device
+ */
+int nand_isreserved_bbt(struct mtd_info *mtd, loff_t offs)
+{
+	struct nand_chip *this = mtd->priv;
+	int block;
+
+	block = (int)(offs >> this->bbt_erase_shift);
+	return bbt_get_entry(this, block) == BBT_BLOCK_RESERVED;
+}
+
+/**
  * nand_isbad_bbt - [NAND Interface] Check if a block is bad
  * @mtd: MTD device structure
  * @offs: offset in the device
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index a1b0b4c..031ff3a 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -222,6 +222,7 @@ struct mtd_info {
 	int (*_lock) (struct mtd_info *mtd, loff_t ofs, uint64_t len);
 	int (*_unlock) (struct mtd_info *mtd, loff_t ofs, uint64_t len);
 	int (*_is_locked) (struct mtd_info *mtd, loff_t ofs, uint64_t len);
+	int (*_block_isreserved) (struct mtd_info *mtd, loff_t ofs);
 	int (*_block_isbad) (struct mtd_info *mtd, loff_t ofs);
 	int (*_block_markbad) (struct mtd_info *mtd, loff_t ofs);
 	int (*_suspend) (struct mtd_info *mtd);
@@ -302,6 +303,7 @@ static inline void mtd_sync(struct mtd_info *mtd)
 int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
 int mtd_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
 int mtd_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len);
+int mtd_block_isreserved(struct mtd_info *mtd, loff_t ofs);
 int mtd_block_isbad(struct mtd_info *mtd, loff_t ofs);
 int mtd_block_markbad(struct mtd_info *mtd, loff_t ofs);
 
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 7a922e6..a2ccc63 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -805,6 +805,7 @@ extern struct nand_manufacturers nand_manuf_ids[];
 extern int nand_scan_bbt(struct mtd_info *mtd, struct nand_bbt_descr *bd);
 extern int nand_default_bbt(struct mtd_info *mtd);
 extern int nand_markbad_bbt(struct mtd_info *mtd, loff_t offs);
+extern int nand_isreserved_bbt(struct mtd_info *mtd, loff_t offs);
 extern int nand_isbad_bbt(struct mtd_info *mtd, loff_t offs, int allowbbt);
 extern int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
 			   int allowbbt);
-- 
1.9.1

[PATCH v2 3/3] mtd: Account for BBT blocks when a partition is being allocated

[Ezequiel Garcia]

With the introduction of mtd_block_isreserved(), it's now possible
to fix the bad and reserved block distribution exposed by ecc_stats,
instead of accounting all the bad or reserved blocks as 'bad'.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/mtdpart.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 921e8c6..a3e3a7d 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -535,7 +535,9 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
 		uint64_t offs = 0;
 
 		while (offs < slave->mtd.size) {
-			if (mtd_block_isbad(master, offs + slave->offset))
+			if (mtd_block_isreserved(master, offs + slave->offset))
+				slave->mtd.ecc_stats.bbtblocks++;
+			else if (mtd_block_isbad(master, offs + slave->offset))
 				slave->mtd.ecc_stats.badblocks++;
 			offs += slave->mtd.erasesize;
 		}
-- 
1.9.1

Re: [PATCH v2 0/3] mtd: Add sysfs entries and account BBT blocks properly

[Ezequiel Garcia]

Hi Brian,

On 21 May 07:06 PM, Ezequiel Garcia wrote:
> Here's a new round of the patchset to add sysfs entries for the ECC stats
> struct fields. See the discussion around previous version for context:
> 
> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052804.html
> 
> Changes from v1:
> 
>   * As agreed I've added a sysfs file per field to replace the ecc_stats
>     sysfs file. The current proposal follows the "one file per value" sysfs
>     rule, adding the following device attributes:
>     - corrected_bits (ecc_stats.corrected)
>     - uncorrectable_errors (ecc_stats.failed)
>     - bad_blocks (ecc_stats.badblocks)
>     - bbt_blocks (ecc_stats.bbtblocks)
> 
>   * Following Brian's request, added small wrapper nand_block_isreserved()
>     that checks if the NAND has a bad block table before querying the number
>     of reserved blocks.
> 
>   * Finally, this new series drops patch: "nand: Account the blocks used by
>     the BBT in the ecc_stats". It's not yet clear how to best deal with the
>     current inconsistencies in MTD master and partition handling.
> 
> The first patch adds the sysfs entries. The rest of the patches fixes the
> bbtblock accounting and allows to keep track of the reserved blocks correctly.
> 
> This is a very old issue, probably around since the dawn of time,
> and nobody seemed to care much about it. However, with the introduction of
> the sysfs access, it's easier to access this stats and easier to
> see the issue.
> 
> Ezequiel Garcia (3):
>   mtd: Add sysfs attributes to expose the ECC stats fields
>   mtd: Introduce mtd_block_isreserved()
>   mtd: Account for BBT blocks when a partition is being allocated
> 
>  drivers/mtd/mtdcore.c        | 60 ++++++++++++++++++++++++++++++++++++++++++--
>  drivers/mtd/mtdpart.c        | 13 +++++++++-
>  drivers/mtd/nand/nand_base.c | 18 +++++++++++++
>  drivers/mtd/nand/nand_bbt.c  | 14 +++++++++++
>  include/linux/mtd/mtd.h      |  2 ++
>  include/linux/mtd/nand.h     |  1 +
>  6 files changed, 105 insertions(+), 3 deletions(-)
> 

What's the status of this one? Do we have pending things to discuss?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v2 0/3] mtd: Add sysfs entries and account BBT blocks properly

[Brian Norris]

Hi Ezequiel,

On Sun, Jun 15, 2014 at 10:21:12PM -0300, Ezequiel Garcia wrote:
> On 21 May 07:06 PM, Ezequiel Garcia wrote:
> > Here's a new round of the patchset to add sysfs entries for the ECC stats
> > struct fields. See the discussion around previous version for context:
[...]
> What's the status of this one? Do we have pending things to discuss?

I wasn't ready to merge this for 3.16, but it looks OK to me. Can you
provide documentation for Documentation/ABI/testing/sysfs-class-mtd?

Brian

Re: [PATCH v2 0/3] mtd: Add sysfs entries and account BBT blocks properly

[Ezequiel Garcia]

On 17 Jun 10:01 AM, Brian Norris wrote:
> Hi Ezequiel,
> 
> On Sun, Jun 15, 2014 at 10:21:12PM -0300, Ezequiel Garcia wrote:
> > On 21 May 07:06 PM, Ezequiel Garcia wrote:
> > > Here's a new round of the patchset to add sysfs entries for the ECC stats
> > > struct fields. See the discussion around previous version for context:
> [...]
> > What's the status of this one? Do we have pending things to discuss?
> 
> I wasn't ready to merge this for 3.16, but it looks OK to me. Can you
> provide documentation for Documentation/ABI/testing/sysfs-class-mtd?
> 

Dammit! I forgot about that requirement from Greg's feedback.

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

RE: [PATCH v2 0/3] mtd: Add sysfs entries and account BBT blocks properly

[Gupta, Pekon]

Hi Ezequiel


>From: Ezequiel Garcia
>
>Here's a new round of the patchset to add sysfs entries for the ECC stats
>struct fields. See the discussion around previous version for context:
>
>http://lists.infradead.org/pipermail/linux-mtd/2014-March/052804.html
>
>Changes from v1:
>
>  * As agreed I've added a sysfs file per field to replace the ecc_stats
>    sysfs file. The current proposal follows the "one file per value" sysfs
>    rule, adding the following device attributes:
>    - corrected_bits (ecc_stats.corrected)
>    - uncorrectable_errors (ecc_stats.failed)

Just a minor nit-pick ..
If you are re-spinning the patch then can you rename "uncorrectable_errors"
to actual binding attribute (something like "ecc_failures"), because the errors
could be due to multiple reasons like:
- uncorrectable bitflips
- timeout waiting for I/O.
- error status from flash controller | device
- ...
These all conditions are accounted in ecc_stat->failed, not alone 
"uncorrectable" conditions.


with regards, pekon

Re: [PATCH v2 0/3] mtd: Add sysfs entries and account BBT blocks properly

[Ezequiel Garcia]

On 18 Jun 05:22 AM, Gupta, Pekon wrote:
> 
> Just a minor nit-pick ..
> If you are re-spinning the patch then can you rename "uncorrectable_errors"
> to actual binding attribute (something like "ecc_failures"), because the errors
> could be due to multiple reasons like:

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