[PATCH 0/4] mtd: Fix wrong bad block account in ECC stats

[PATCH 0/4] mtd: Fix wrong bad block account in ECC stats

[Ezequiel Garcia]

This series addresses the wrong bad and bbt block accounting we currently
have in MTD.

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

Currently, a device with eight blocks used for the BBT in the last partition
would show (getting ECC stats from sysfs):

$ cat /sys/class/mtd/mtd2/ecc_stats
       0        0       10        0
       0        0        4        0
       0        0        8        0

The third column is the badblocks field in the ecc_stats structure. The
number of badblocks is obtained for each partition near the end of
allocate_partition(). The partition is scanned for badblocks and this
number is updated. This series fixes this by adding a check for BBT reserved
blocks, showing:

$ cat /sys/class/mtd/mtd2/ecc_stats
       0        0       10        0
       0        0        4        0
       0        0        0        8

The first two patches have already been posted. I'm resending them here for
conveniency.

Ezequiel Garcia (4):
  mtd: Add sysfs attr to expose ECC stats
  mtd: nand: Account the blocks used by the BBT in the ecc_stats
  mtd: Introduce mtd_block_isreserved()
  mtd: Account for BBT blocks when a partition is being allocated

 drivers/mtd/mtdcore.c        | 24 ++++++++++++++++++++++++
 drivers/mtd/mtdpart.c        | 13 ++++++++++++-
 drivers/mtd/nand/nand_base.c |  1 +
 drivers/mtd/nand/nand_bbt.c  | 16 ++++++++++++++++
 include/linux/mtd/mtd.h      |  2 ++
 include/linux/mtd/nand.h     |  1 +
 6 files changed, 56 insertions(+), 1 deletion(-)

-- 
1.9.0

[PATCH 1/4] mtd: Add sysfs attr to expose ECC stats

[Ezequiel Garcia]

This new sysfs device attribute allows to retrieve the ECC stats
by poking a sysfs file, which is much simpler than using the ioctl.

The output format is inspired in block device sysfs 'stat' file.
The ecc_stats struct fields are shown as ordered in the structure,
in a machine parseable format.

Here's a typical output showing 3 ECC-corrected and 8 bad blocks:
       3        0        8        0

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

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index d201fee..c5e9943 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -298,6 +298,19 @@ 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_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, "%8u %8u %8u %8u\n", ecc_stats->corrected,
+						ecc_stats->failed,
+						ecc_stats->badblocks,
+						ecc_stats->bbtblocks);
+}
+static DEVICE_ATTR(ecc_stats, S_IRUGO, mtd_ecc_stats_show, NULL);
+
 static struct attribute *mtd_attrs[] = {
 	&dev_attr_type.attr,
 	&dev_attr_flags.attr,
@@ -310,6 +323,7 @@ static struct attribute *mtd_attrs[] = {
 	&dev_attr_name.attr,
 	&dev_attr_ecc_strength.attr,
 	&dev_attr_ecc_step_size.attr,
+	&dev_attr_ecc_stats.attr,
 	&dev_attr_bitflip_threshold.attr,
 	NULL,
 };
-- 
1.9.0

[PATCH 2/4] mtd: nand: Account the blocks used by the BBT in the ecc_stats

[Ezequiel Garcia]

Strictly speaking we should be updating the ecc_stats in the master
MTD object, with the blocks used by the bad block table.

This is already being done for bad and reserved blocks detected doing
the BBT search, but not for the blocks used by the BBT itself. This commit
adds the latter.

It should be noted that the ecc_stats structure is kept only for userspace
information, accesible through an ioctl. However, since the master MTD object
is not tied to any /dev/mtd{N} device node in the filesystem, there's currently
no way to retrieve this information.

This ecc_stats is used for the MTD partitions typically allocated and
registered by mtd_device_parse_register(). These have a device node, but scan
for bad blocks and updates the ecc_stats in a different code path.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
--
For the reasons exposed above, it's not clear we should remove the ecc_stats
update in the master MTD altogether or simply take account of the BBT blocks
for consistency. I've chosen the latter, for it seemed a safer changer.

I'm open to discussion, though.
---
 drivers/mtd/nand/nand_bbt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index c0615d1..ea9a266 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -991,6 +991,7 @@ static void mark_bbt_region(struct mtd_info *mtd, struct nand_bbt_descr *td)
 			block = td->pages[i] >> (this->bbt_erase_shift - this->page_shift);
 			oldval = bbt_get_entry(this, block);
 			bbt_mark_entry(this, block, BBT_BLOCK_RESERVED);
+			mtd->ecc_stats.bbtblocks++;
 			if ((oldval != BBT_BLOCK_RESERVED) &&
 					td->reserved_block_code)
 				nand_update_bbt(mtd, (loff_t)block <<
@@ -1005,6 +1006,7 @@ static void mark_bbt_region(struct mtd_info *mtd, struct nand_bbt_descr *td)
 		for (j = 0; j < td->maxblocks; j++) {
 			oldval = bbt_get_entry(this, block);
 			bbt_mark_entry(this, block, BBT_BLOCK_RESERVED);
+			mtd->ecc_stats.bbtblocks++;
 			if (oldval != BBT_BLOCK_RESERVED)
 				update = 1;
 			block++;
-- 
1.9.0

[PATCH 3/4] 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().

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

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index c5e9943..d65c5dc 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -1012,6 +1012,16 @@ int mtd_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 }
 EXPORT_SYMBOL_GPL(mtd_is_locked);
 
+int mtd_block_isreserved(struct mtd_info *mtd, loff_t ofs)
+{
+	if (!mtd->_block_isreserved)
+		return 0;
+	if (ofs < 0 || ofs > mtd->size)
+		return -EINVAL;
+	return mtd->_block_isreserved(mtd, ofs);
+}
+EXPORT_SYMBOL_GPL(mtd_block_isreserved);
+
 int mtd_block_isbad(struct mtd_info *mtd, loff_t ofs)
 {
 	if (!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 5826da3..58f5884 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -4044,6 +4044,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 	mtd->_unlock = NULL;
 	mtd->_suspend = nand_suspend;
 	mtd->_resume = nand_resume;
+	mtd->_block_isreserved = nand_isreserved_bbt;
 	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 ea9a266..fd21169 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -1312,6 +1312,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 0747fef..3b78c6b 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -806,6 +806,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.0

[PATCH 4/4] 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'.

For instance, a device with eight blocks used for the BBT in the last
partition would now show (getting ECC stats from sysfs):

$ cat /sys/class/mtd/mtd2/ecc_stats
       0        0       10        0
       0        0        4        0
       0        0        0        8

instead of the currently wrong stats:

$ cat /sys/class/mtd/mtd2/ecc_stats
       0        0       10        0
       0        0        4        0
       0        0        8        0

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.0

RE: [PATCH 1/4] mtd: Add sysfs attr to expose ECC stats

[Gupta, Pekon]

Hi Ezequiel,

>From: Ezequiel Garcia
>
>This new sysfs device attribute allows to retrieve the ECC stats
>by poking a sysfs file, which is much simpler than using the ioctl.
>
>The output format is inspired in block device sysfs 'stat' file.
>The ecc_stats struct fields are shown as ordered in the structure,
>in a machine parseable format.
>
>Here's a typical output showing 3 ECC-corrected and 8 bad blocks:
>       3        0        8        0
>
>Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
>---
> drivers/mtd/mtdcore.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
>diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>index d201fee..c5e9943 100644
>--- a/drivers/mtd/mtdcore.c
>+++ b/drivers/mtd/mtdcore.c
>@@ -298,6 +298,19 @@ 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_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, "%8u %8u %8u %8u\n", ecc_stats->corrected,
>+						ecc_stats->failed,
>+						ecc_stats->badblocks,
>+						ecc_stats->bbtblocks);
Though I would have liked to see each field of ecc_stats as a separate sysfs entry
But, I don't know what it impacts to keep it different from /sys/block/<device>/stat
Do you know any user-space tools which utilize these, and can be re-used on
UBI-block kind of layer, if we keep this compatibility ?

Also, How about printing what these numbers mean ?
I hope this will still keep it machine readable.
snprintf(buf, PAGE_SIZE, "%16s %16s %16s %16s\n",
					"ecc_corrected",
 					"ecc_failed",
					"ecc_badblocks",
					"ecc_bbtblocks");
 return snprintf(buf, PAGE_SIZE, " %16u  %16u  %16u  %16u\n",
					ecc_stats->corrected,
 					ecc_stats->failed,
					ecc_stats->badblocks,
					ecc_stats->bbtblocks);

>+}


>+static DEVICE_ATTR(ecc_stats, S_IRUGO, mtd_ecc_stats_show, NULL);
>+
> static struct attribute *mtd_attrs[] = {
> 	&dev_attr_type.attr,
> 	&dev_attr_flags.attr,
>@@ -310,6 +323,7 @@ static struct attribute *mtd_attrs[] = {
> 	&dev_attr_name.attr,
> 	&dev_attr_ecc_strength.attr,
> 	&dev_attr_ecc_step_size.attr,
>+	&dev_attr_ecc_stats.attr,
> 	&dev_attr_bitflip_threshold.attr,
> 	NULL,
> };
>--
>1.9.0
>

Also please update "Documentation/ABI/testing/sysfs-class-mtd"
with details about 'ecc_stats'

with regards, pekon

Re: [PATCH 1/4] mtd: Add sysfs attr to expose ECC stats

[Ezequiel Garcia]

Hello Pekon,

On Mar 27, Gupta, Pekon wrote:
> >
> >+static ssize_t mtd_ecc_stats_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, "%8u %8u %8u %8u\n", ecc_stats->corrected,
> >+						ecc_stats->failed,
> >+						ecc_stats->badblocks,
> >+						ecc_stats->bbtblocks);
> Though I would have liked to see each field of ecc_stats as a separate sysfs entry

I tried to match the ecc_stats structure exactly in the sysfs entry. Keep
in mind creating/keeping a sysfs file has its cost. Since it's trivial
to get one of the fields with any text parsing tool I didn't think having
a file per stat was worth it.

> But, I don't know what it impacts to keep it different from /sys/block/<device>/stat
> Do you know any user-space tools which utilize these, and can be re-used on
> UBI-block kind of layer, if we keep this compatibility ?
> 

It won't impact as they are two completely different things. The ecc_stats
output is only "vaguely inspired" in block stats; but these are two completely
different stats. It's not about tool reusability, but about consistency.

> Also, How about printing what these numbers mean ?
> I hope this will still keep it machine readable.

Well, this is not a debugfs entry, so I'm not sure we want to add such debug
information. Anyone can take a look at the code and see what ecc_stats mean.

On the other side, I don't have a strong opinion on this.

> 
> Also please update "Documentation/ABI/testing/sysfs-class-mtd"
> with details about 'ecc_stats'
> 

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

Re: [PATCH 0/4] mtd: Fix wrong bad block account in ECC stats

[Ezequiel Garcia]

On Mar 21, Ezequiel Garcia wrote:
> This series addresses the wrong bad and bbt block accounting we currently
> have in MTD.
> 
> Notice that this is a very old issue, probably around since the dawn of time,
> and nobody seem to care much about it. However, with the introduction of
> the sysfs ECC stats access, it's easier to access this stats and easier to
> see the issue.
> 
> Currently, a device with eight blocks used for the BBT in the last partition
> would show (getting ECC stats from sysfs):
> 
> $ cat /sys/class/mtd/mtd2/ecc_stats
>        0        0       10        0
>        0        0        4        0
>        0        0        8        0
> 
> The third column is the badblocks field in the ecc_stats structure. The
> number of badblocks is obtained for each partition near the end of
> allocate_partition(). The partition is scanned for badblocks and this
> number is updated. This series fixes this by adding a check for BBT reserved
> blocks, showing:
> 
> $ cat /sys/class/mtd/mtd2/ecc_stats
>        0        0       10        0
>        0        0        4        0
>        0        0        0        8
> 
> The first two patches have already been posted. I'm resending them here for
> conveniency.
> 
> Ezequiel Garcia (4):
>   mtd: Add sysfs attr to expose ECC stats
>   mtd: nand: Account the blocks used by the BBT in the ecc_stats
>   mtd: Introduce mtd_block_isreserved()
>   mtd: Account for BBT blocks when a partition is being allocated
> 
>  drivers/mtd/mtdcore.c        | 24 ++++++++++++++++++++++++
>  drivers/mtd/mtdpart.c        | 13 ++++++++++++-
>  drivers/mtd/nand/nand_base.c |  1 +
>  drivers/mtd/nand/nand_bbt.c  | 16 ++++++++++++++++
>  include/linux/mtd/mtd.h      |  2 ++
>  include/linux/mtd/nand.h     |  1 +
>  6 files changed, 56 insertions(+), 1 deletion(-)
> 

Any comments?

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

RE: [PATCH 1/4] mtd: Add sysfs attr to expose ECC stats

[Gupta, Pekon]

Hi Ezequiel,

>From: Ezequiel Garcia
>>On Mar 27, Gupta, Pekon wrote:
>> >
>> >+static ssize_t mtd_ecc_stats_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, "%8u %8u %8u %8u\n", ecc_stats->corrected,
>> >+						ecc_stats->failed,
>> >+						ecc_stats->badblocks,
>> >+						ecc_stats->bbtblocks);
>> Though I would have liked to see each field of ecc_stats as a separate sysfs entry
>
>I tried to match the ecc_stats structure exactly in the sysfs entry. Keep
>in mind creating/keeping a sysfs file has its cost. Since it's trivial
>to get one of the fields with any text parsing tool I didn't think having
>a file per stat was worth it.
>
There are some guidelines about attributes in 'Documentation/filesystems/sysfs.txt'
Though it's acceptable to put array of values of the "same type" in single sysfs file,
But I'm still not confident on having all members of 'struct ecc_stats' being
represented by single sysfs file because:

(1) ecc_stat represents variety of information like
  ecc_stats->failed: may be used to debug | analyze a access failure.
  ecc_stats->badblocks: may be used to determine the health of flash device.
  So, keeping these information in separate sysfs files seemed more user-friendly.

(2) Having single sysfs file may constrains future expansion of 'struct ecc_stats'
   as we have to stick to "same type" members.

I'm just raising these doubts because it's difficult to change sysfs attributes
once they are merged, as it might break user-space compatibility.

(feedbacks from other are welcomed)..

>> But, I don't know what it impacts to keep it different from /sys/block/<device>/stat
>> Do you know any user-space tools which utilize these, and can be re-used on
>> UBI-block kind of layer, if we keep this compatibility ?
>>
>
>It won't impact as they are two completely different things. The ecc_stats
>output is only "vaguely inspired" in block stats; but these are two completely
>different stats. It's not about tool reusability, but about consistency.
>
>> Also, How about printing what these numbers mean ?
>> I hope this will still keep it machine readable.
>
>Well, this is not a debugfs entry, so I'm not sure we want to add such debug
>information. Anyone can take a look at the code and see what ecc_stats mean.
>
>On the other side, I don't have a strong opinion on this.
>
>>
>> Also please update "Documentation/ABI/testing/sysfs-class-mtd"
>> with details about 'ecc_stats'
>>
>
>OK, I will.

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

with regards, pekon

Re: [PATCH 1/4] mtd: Add sysfs attr to expose ECC stats

[Brian Norris]

+ Greg KH, LKML

For context:

http://thread.gmane.org/gmane.linux.drivers.mtd/52517/focus=52518

On Tue, Apr 15, 2014 at 11:13:52AM +0000, Pekon Gupta wrote:
> >From: Ezequiel Garcia
> >>On Mar 27, Gupta, Pekon wrote:
> >> >
> >> >+static ssize_t mtd_ecc_stats_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, "%8u %8u %8u %8u\n", ecc_stats->corrected,
> >> >+						ecc_stats->failed,
> >> >+						ecc_stats->badblocks,
> >> >+						ecc_stats->bbtblocks);
> >> Though I would have liked to see each field of ecc_stats as a separate sysfs entry
> >
> >I tried to match the ecc_stats structure exactly in the sysfs entry. Keep
> >in mind creating/keeping a sysfs file has its cost. Since it's trivial
> >to get one of the fields with any text parsing tool I didn't think having
> >a file per stat was worth it.

I'm not sure what kind of cost this has these days. I thought there was
some work to reduce the required kobject memory usage for each new sysfs
file.

> There are some guidelines about attributes in 'Documentation/filesystems/sysfs.txt'
> Though it's acceptable to put array of values of the "same type" in single sysfs file,
> But I'm still not confident on having all members of 'struct ecc_stats' being
> represented by single sysfs file
[...]

I agree, it looks like the sysfs policy would recommend against putting
distinct properties in the same file.

I'm not sure if /sys/block/<disk>/stat is a good example, as it does
violate this policy. It also seems to have some historical baggage.

But there is potentially one good reason for putting this distinct
information in a single file: if the information must be returned
atomically. For disk stats, it might be important to get a consistent
snapshot of the disk stats (or nearly so, with minimal locking
overhead), which might change significantly between file accesses if
we're doing half a dozen file queries instead.

This same reason may not apply to these ECC stats, since none of these
ECC stats are likely to be changing concurrently.

So I personally might lean toward "one file per attribute" here.

> >It won't impact as they are two completely different things. The ecc_stats
> >output is only "vaguely inspired" in block stats; but these are two completely
> >different stats. It's not about tool reusability, but about consistency.
> >
> >> Also, How about printing what these numbers mean ?

This sounds like an argument from the /proc days. Not sure if we want to
repeat that in /sys.

> >> I hope this will still keep it machine readable.
> >Well, this is not a debugfs entry, so I'm not sure we want to add such debug
> >information. Anyone can take a look at the code and see what ecc_stats mean.

The code or (as suggested by Pekon) the documentation
(Documentation/ABI/). Either way -- with a single 'ecc_stats' table, or
with 4 separate files -- they need to be documented.

Brian

Re: [PATCH 3/4] mtd: Introduce mtd_block_isreserved()

[Brian Norris]

On Fri, Mar 21, 2014 at 08:57:43AM -0300, Ezequiel Garcia wrote:
> 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().
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  drivers/mtd/mtdcore.c        | 10 ++++++++++
>  drivers/mtd/mtdpart.c        |  9 +++++++++
>  drivers/mtd/nand/nand_base.c |  1 +
>  drivers/mtd/nand/nand_bbt.c  | 14 ++++++++++++++
>  include/linux/mtd/mtd.h      |  2 ++
>  include/linux/mtd/nand.h     |  1 +
>  6 files changed, 37 insertions(+)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index c5e9943..d65c5dc 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -1012,6 +1012,16 @@ int mtd_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  }
>  EXPORT_SYMBOL_GPL(mtd_is_locked);
>  
> +int mtd_block_isreserved(struct mtd_info *mtd, loff_t ofs)
> +{
> +	if (!mtd->_block_isreserved)
> +		return 0;
> +	if (ofs < 0 || ofs > mtd->size)
> +		return -EINVAL;

At first, I was going to recommend that the out-of-bounds check go
before the !mtd->_block_isreserved check, since it's best to warn users
for invalid input. But then, mtd_block_isbad() has the same ordering, so
it'd be nice to consistent...

Do we flip a coin to decide whether to change both or leave as-is? :)

> +	return mtd->_block_isreserved(mtd, ofs);
> +}
> +EXPORT_SYMBOL_GPL(mtd_block_isreserved);
> +
>  int mtd_block_isbad(struct mtd_info *mtd, loff_t ofs)
>  {
>  	if (!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 5826da3..58f5884 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -4044,6 +4044,7 @@ int nand_scan_tail(struct mtd_info *mtd)
>  	mtd->_unlock = NULL;
>  	mtd->_suspend = nand_suspend;
>  	mtd->_resume = nand_resume;
> +	mtd->_block_isreserved = nand_isreserved_bbt;

I think you want a small wrapper function in case we aren't using a BBT
at all. See nand_block_checkbad(), which checks for !chip->bbt. So we'd
need:

	if (!chip->bbt)
		return 0;

>  	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 ea9a266..fd21169 100644
> --- a/drivers/mtd/nand/nand_bbt.c
> +++ b/drivers/mtd/nand/nand_bbt.c
> @@ -1312,6 +1312,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 0747fef..3b78c6b 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -806,6 +806,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);

Otherwise, looks good.

Brian

Re: [PATCH 1/4] mtd: Add sysfs attr to expose ECC stats

[Ezequiel Garcia]

On 12 May 05:50 PM, Brian Norris wrote:
> > There are some guidelines about attributes in 'Documentation/filesystems/sysfs.txt'
> > Though it's acceptable to put array of values of the "same type" in single sysfs file,
> > But I'm still not confident on having all members of 'struct ecc_stats' being
> > represented by single sysfs file
> [...]
> 
> I agree, it looks like the sysfs policy would recommend against putting
> distinct properties in the same file.
> 

OK...

> I'm not sure if /sys/block/<disk>/stat is a good example, as it does
> violate this policy. It also seems to have some historical baggage.
> 
> But there is potentially one good reason for putting this distinct
> information in a single file: if the information must be returned
> atomically. For disk stats, it might be important to get a consistent
> snapshot of the disk stats (or nearly so, with minimal locking
> overhead), which might change significantly between file accesses if
> we're doing half a dozen file queries instead.
> 
> This same reason may not apply to these ECC stats, since none of these
> ECC stats are likely to be changing concurrently.
> 

Right.

> So I personally might lean toward "one file per attribute" here.
> 

Yup, no problem. Greg, if you can confirm this it'd be great.

> > >> I hope this will still keep it machine readable.
> > >Well, this is not a debugfs entry, so I'm not sure we want to add such debug
> > >information. Anyone can take a look at the code and see what ecc_stats mean.
> 
> The code or (as suggested by Pekon) the documentation
> (Documentation/ABI/). Either way -- with a single 'ecc_stats' table, or
> with 4 separate files -- they need to be documented.
> 

Sure, will document in next round.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 2/4] mtd: nand: Account the blocks used by the BBT in the ecc_stats

[Ezequiel Garcia]

On 21 Mar 08:57 AM, Ezequiel Garcia wrote:
> Strictly speaking we should be updating the ecc_stats in the master
> MTD object, with the blocks used by the bad block table.
> 
> This is already being done for bad and reserved blocks detected doing
> the BBT search, but not for the blocks used by the BBT itself. This commit
> adds the latter.
> 
> It should be noted that the ecc_stats structure is kept only for userspace
> information, accesible through an ioctl. However, since the master MTD object
> is not tied to any /dev/mtd{N} device node in the filesystem, there's currently
> no way to retrieve this information.
> 
> This ecc_stats is used for the MTD partitions typically allocated and
> registered by mtd_device_parse_register(). These have a device node, but scan
> for bad blocks and updates the ecc_stats in a different code path.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> --
> For the reasons exposed above, it's not clear we should remove the ecc_stats
> update in the master MTD altogether or simply take account of the BBT blocks
> for consistency. I've chosen the latter, for it seemed a safer changer.
> 
> I'm open to discussion, though.

Brian,

Can you comment a bit on this one? Should I keep this change in v2?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

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

[Brian Norris]

On Fri, Mar 21, 2014 at 08:57:44AM -0300, Ezequiel Garcia wrote:
> 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'.
> 
> For instance, a device with eight blocks used for the BBT in the last
> partition would now show (getting ECC stats from sysfs):
> 
> $ cat /sys/class/mtd/mtd2/ecc_stats

I assume you're actually using a wildcard instead of '2', since that's
more than one file's contents below.

>        0        0       10        0
>        0        0        4        0
>        0        0        0        8
> 
> instead of the currently wrong stats:
> 
> $ cat /sys/class/mtd/mtd2/ecc_stats
>        0        0       10        0
>        0        0        4        0
>        0        0        8        0
> 
> 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;
>  		}

Looks good. We've had some missing pieces here regarding the
reserved_blocks stat for a while.

Brian

Re: [PATCH 2/4] mtd: nand: Account the blocks used by the BBT in the ecc_stats

[Brian Norris]

On Mon, May 12, 2014 at 11:27:53PM -0300, Ezequiel Garcia wrote:
> On 21 Mar 08:57 AM, Ezequiel Garcia wrote:
> > Strictly speaking we should be updating the ecc_stats in the master
> > MTD object, with the blocks used by the bad block table.
> > 
> > This is already being done for bad and reserved blocks detected doing
> > the BBT search, but not for the blocks used by the BBT itself. This commit
> > adds the latter.
> > 
> > It should be noted that the ecc_stats structure is kept only for userspace
> > information, accesible through an ioctl. However, since the master MTD object
> > is not tied to any /dev/mtd{N} device node in the filesystem, there's currently
> > no way to retrieve this information.
> > 
> > This ecc_stats is used for the MTD partitions typically allocated and
> > registered by mtd_device_parse_register(). These have a device node, but scan
> > for bad blocks and updates the ecc_stats in a different code path.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > --
> > For the reasons exposed above, it's not clear we should remove the ecc_stats
> > update in the master MTD altogether or simply take account of the BBT blocks
> > for consistency. I've chosen the latter, for it seemed a safer changer.
> > 
> > I'm open to discussion, though.
> 
> Brian,
> 
> Can you comment a bit on this one? Should I keep this change in v2?

I'm not really sure. I'm honestly having a hard time tracking all the
potentially-configurable knobs of nand_bbt.c. It looks like only
diskonchip sets a reserved_block_code, so some of the existing code
isn't really even tested widely. And like you mention, the ecc_stats
from the master MTD are not propagated directly to the partition (nor
should they be), so the stat is really unused.

I'm not 100% confident that we won't double-count any 'bbtblocks' in
your current patch. Maybe we should rewrite some of this stuff...

I'll look at this again when my eyes are fresh.

Brian

Re: [PATCH 1/4] mtd: Add sysfs attr to expose ECC stats

[Greg Kroah-Hartman]

On Mon, May 12, 2014 at 11:26:14PM -0300, Ezequiel Garcia wrote:
> On 12 May 05:50 PM, Brian Norris wrote:
> > > There are some guidelines about attributes in 'Documentation/filesystems/sysfs.txt'
> > > Though it's acceptable to put array of values of the "same type" in single sysfs file,
> > > But I'm still not confident on having all members of 'struct ecc_stats' being
> > > represented by single sysfs file
> > [...]
> > 
> > I agree, it looks like the sysfs policy would recommend against putting
> > distinct properties in the same file.
> > 
> 
> OK...
> 
> > I'm not sure if /sys/block/<disk>/stat is a good example, as it does
> > violate this policy. It also seems to have some historical baggage.
> > 
> > But there is potentially one good reason for putting this distinct
> > information in a single file: if the information must be returned
> > atomically. For disk stats, it might be important to get a consistent
> > snapshot of the disk stats (or nearly so, with minimal locking
> > overhead), which might change significantly between file accesses if
> > we're doing half a dozen file queries instead.
> > 
> > This same reason may not apply to these ECC stats, since none of these
> > ECC stats are likely to be changing concurrently.
> > 
> 
> Right.
> 
> > So I personally might lean toward "one file per attribute" here.
> > 
> 
> Yup, no problem. Greg, if you can confirm this it'd be great.

Yes, that is the rule for sysfs, please do not put multiple values in a
single sysfs file for a variety of good reasons.

> > > >> I hope this will still keep it machine readable.
> > > >Well, this is not a debugfs entry, so I'm not sure we want to add such debug
> > > >information. Anyone can take a look at the code and see what ecc_stats mean.
> > 
> > The code or (as suggested by Pekon) the documentation
> > (Documentation/ABI/). Either way -- with a single 'ecc_stats' table, or
> > with 4 separate files -- they need to be documented.
> > 
> 
> Sure, will document in next round.

This documentation is required, thanks.

greg k-h

Re: [PATCH 1/4] mtd: Add sysfs attr to expose ECC stats

[Ezequiel Garcia]

On 13 May 01:15 PM, Greg Kroah-Hartman wrote:
> On Mon, May 12, 2014 at 11:26:14PM -0300, Ezequiel Garcia wrote:
> > On 12 May 05:50 PM, Brian Norris wrote:
> > > > There are some guidelines about attributes in 'Documentation/filesystems/sysfs.txt'
> > > > Though it's acceptable to put array of values of the "same type" in single sysfs file,
> > > > But I'm still not confident on having all members of 'struct ecc_stats' being
> > > > represented by single sysfs file
> > > [...]
> > > 
> > > I agree, it looks like the sysfs policy would recommend against putting
> > > distinct properties in the same file.
> > > 
> > 
> > OK...
> > 
> > > I'm not sure if /sys/block/<disk>/stat is a good example, as it does
> > > violate this policy. It also seems to have some historical baggage.
> > > 
> > > But there is potentially one good reason for putting this distinct
> > > information in a single file: if the information must be returned
> > > atomically. For disk stats, it might be important to get a consistent
> > > snapshot of the disk stats (or nearly so, with minimal locking
> > > overhead), which might change significantly between file accesses if
> > > we're doing half a dozen file queries instead.
> > > 
> > > This same reason may not apply to these ECC stats, since none of these
> > > ECC stats are likely to be changing concurrently.
> > > 
> > 
> > Right.
> > 
> > > So I personally might lean toward "one file per attribute" here.
> > > 
> > 
> > Yup, no problem. Greg, if you can confirm this it'd be great.
> 
> Yes, that is the rule for sysfs, please do not put multiple values in a
> single sysfs file for a variety of good reasons.
> 

Will do!

> > > > >> I hope this will still keep it machine readable.
> > > > >Well, this is not a debugfs entry, so I'm not sure we want to add such debug
> > > > >information. Anyone can take a look at the code and see what ecc_stats mean.
> > > 
> > > The code or (as suggested by Pekon) the documentation
> > > (Documentation/ABI/). Either way -- with a single 'ecc_stats' table, or
> > > with 4 separate files -- they need to be documented.
> > > 
> > 
> > Sure, will document in next round.
> 
> This documentation is required, thanks.
> 

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

Re: [PATCH 2/4] mtd: nand: Account the blocks used by the BBT in the ecc_stats

[Ezequiel Garcia]

On 12 May 07:36 PM, Brian Norris wrote:
> On Mon, May 12, 2014 at 11:27:53PM -0300, Ezequiel Garcia wrote:
> > On 21 Mar 08:57 AM, Ezequiel Garcia wrote:
> > > Strictly speaking we should be updating the ecc_stats in the master
> > > MTD object, with the blocks used by the bad block table.
> > > 
> > > This is already being done for bad and reserved blocks detected doing
> > > the BBT search, but not for the blocks used by the BBT itself. This commit
> > > adds the latter.
> > > 
> > > It should be noted that the ecc_stats structure is kept only for userspace
> > > information, accesible through an ioctl. However, since the master MTD object
> > > is not tied to any /dev/mtd{N} device node in the filesystem, there's currently
> > > no way to retrieve this information.
> > > 
> > > This ecc_stats is used for the MTD partitions typically allocated and
> > > registered by mtd_device_parse_register(). These have a device node, but scan
> > > for bad blocks and updates the ecc_stats in a different code path.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > > --
> > > For the reasons exposed above, it's not clear we should remove the ecc_stats
> > > update in the master MTD altogether or simply take account of the BBT blocks
> > > for consistency. I've chosen the latter, for it seemed a safer changer.
> > > 
> > > I'm open to discussion, though.
> > 
> > Brian,
> > 
> > Can you comment a bit on this one? Should I keep this change in v2?
> 
> I'm not really sure. I'm honestly having a hard time tracking all the
> potentially-configurable knobs of nand_bbt.c. It looks like only
> diskonchip sets a reserved_block_code, so some of the existing code
> isn't really even tested widely. And like you mention, the ecc_stats
> from the master MTD are not propagated directly to the partition (nor
> should they be), so the stat is really unused.
> 
> I'm not 100% confident that we won't double-count any 'bbtblocks' in
> your current patch. Maybe we should rewrite some of this stuff...
> 

Indeed, I wrote this stuff a while ago, but I remember having a really hard
time to follow the bbtblocks count increments.

> I'll look at this again when my eyes are fresh.
> 

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

Re: [PATCH 3/4] mtd: Introduce mtd_block_isreserved()

[Ezequiel Garcia]

On 12 May 06:31 PM, Brian Norris wrote:
> On Fri, Mar 21, 2014 at 08:57:43AM -0300, Ezequiel Garcia wrote:
[..]
> >  
> > +int mtd_block_isreserved(struct mtd_info *mtd, loff_t ofs)
> > +{
> > +	if (!mtd->_block_isreserved)
> > +		return 0;
> > +	if (ofs < 0 || ofs > mtd->size)
> > +		return -EINVAL;
> 
> At first, I was going to recommend that the out-of-bounds check go
> before the !mtd->_block_isreserved check, since it's best to warn users
> for invalid input. But then, mtd_block_isbad() has the same ordering, so
> it'd be nice to consistent...
> 
> Do we flip a coin to decide whether to change both or leave as-is? :)
> 

Actually, I just followed the same convention as all the other functions,
not just mtd_block_isbad().

I'll add a patch changing them all so the parameters checking is done first.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 3/4] mtd: Introduce mtd_block_isreserved()

[Brian Norris]

On Wed, May 14, 2014 at 08:37:21PM -0300, Ezequiel Garcia wrote:
> On 12 May 06:31 PM, Brian Norris wrote:
> > On Fri, Mar 21, 2014 at 08:57:43AM -0300, Ezequiel Garcia wrote:
> [..]
> > >  
> > > +int mtd_block_isreserved(struct mtd_info *mtd, loff_t ofs)
> > > +{
> > > +	if (!mtd->_block_isreserved)
> > > +		return 0;
> > > +	if (ofs < 0 || ofs > mtd->size)
> > > +		return -EINVAL;
> > 
> > At first, I was going to recommend that the out-of-bounds check go
> > before the !mtd->_block_isreserved check, since it's best to warn users
> > for invalid input. But then, mtd_block_isbad() has the same ordering, so
> > it'd be nice to consistent...
> > 
> > Do we flip a coin to decide whether to change both or leave as-is? :)
> > 
> 
> Actually, I just followed the same convention as all the other functions,
> not just mtd_block_isbad().

All? Like what? mtd_read_oob()? mtd_get_fact_prot_info()? These return
-EOPNOTSUPP, which is an informative error code. But that's different
than returning 0 for mtd_block_isbad() or mtd_block_isreserved(), even
if the block doesn't exist.

> I'll add a patch changing them all so the parameters checking is done first.

Can you mention which ones you think are problematic before adding
another patch? I'd be careful on playing with error codes for APIs that
are already have reasonable enough error codes. AFAICT,
mtd_block_isbad() (and your mtd_block_isreserved()) are the only
problems.

(Also, is it just me, or is mtd_writev() missing bounds checking?)

Brian

Re: [PATCH 3/4] mtd: Introduce mtd_block_isreserved()

[Ezequiel Garcia]

On 14 May 04:57 PM, Brian Norris wrote:
> On Wed, May 14, 2014 at 08:37:21PM -0300, Ezequiel Garcia wrote:
> > On 12 May 06:31 PM, Brian Norris wrote:
> > > On Fri, Mar 21, 2014 at 08:57:43AM -0300, Ezequiel Garcia wrote:
> > [..]
> > > >  
> > > > +int mtd_block_isreserved(struct mtd_info *mtd, loff_t ofs)
> > > > +{
> > > > +	if (!mtd->_block_isreserved)
> > > > +		return 0;
> > > > +	if (ofs < 0 || ofs > mtd->size)
> > > > +		return -EINVAL;
> > > 
> > > At first, I was going to recommend that the out-of-bounds check go
> > > before the !mtd->_block_isreserved check, since it's best to warn users
> > > for invalid input. But then, mtd_block_isbad() has the same ordering, so
> > > it'd be nice to consistent...
> > > 
> > > Do we flip a coin to decide whether to change both or leave as-is? :)
> > > 
> > 
> > Actually, I just followed the same convention as all the other functions,
> > not just mtd_block_isbad().
> 
> All? Like what? mtd_read_oob()? mtd_get_fact_prot_info()? These return
> -EOPNOTSUPP, which is an informative error code. But that's different
> than returning 0 for mtd_block_isbad() or mtd_block_isreserved(), even
> if the block doesn't exist.
> 

Ah... I misunderstood your comment. The point is that you don't want to
return 0, when the parameters may be are wrong.

So, I'll fix this patch by changing both, mtd_block_isbad and
mtd_block_isreserved, just as you asked in the first place.

> 
> (Also, is it just me, or is mtd_writev() missing bounds checking?)
> 

Hm... so it seems. I guess the check should be something like:

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index d201fee..21ce0f4 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -1066,11 +1066,22 @@ static int default_mtd_writev(struct mtd_info *mtd, const struct kvec *vecs,
 int mtd_writev(struct mtd_info *mtd, const struct kvec *vecs,
 	       unsigned long count, loff_t to, size_t *retlen)
 {
+	int i;
+
 	*retlen = 0;
 	if (!(mtd->flags & MTD_WRITEABLE))
 		return -EROFS;
 	if (!mtd->_writev)
 		return default_mtd_writev(mtd, vecs, count, to, retlen);
+
+	if (to < 0 || to > mtd->size)
+		return -EINVAL;
+	for (i = 0; i < count; i++) {
+		if (!vecs[i].iov_len)
+			continue;
+		if (vecs[i].iov_len > mtd->size - to)
+			return -EINVAL;
+	}
 	return mtd->_writev(mtd, vecs, count, to, retlen);
 }
 EXPORT_SYMBOL_GPL(mtd_writev);

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

Re: [PATCH 3/4] mtd: Introduce mtd_block_isreserved()

[Brian Norris]

On Thu, May 15, 2014 at 05:15:18PM -0300, Ezequiel Garcia wrote:
> On 14 May 04:57 PM, Brian Norris wrote:
> > All? Like what? mtd_read_oob()? mtd_get_fact_prot_info()? These return
> > -EOPNOTSUPP, which is an informative error code. But that's different
> > than returning 0 for mtd_block_isbad() or mtd_block_isreserved(), even
> > if the block doesn't exist.
> 
> Ah... I misunderstood your comment. The point is that you don't want to
> return 0, when the parameters may be are wrong.

Right. I think it is wrong to return '0' for the out-of-bounds case.

> So, I'll fix this patch by changing both, mtd_block_isbad and
> mtd_block_isreserved, just as you asked in the first place.

Sounds good.

> > (Also, is it just me, or is mtd_writev() missing bounds checking?)
> 
> Hm... so it seems. I guess the check should be something like:
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index d201fee..21ce0f4 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -1066,11 +1066,22 @@ static int default_mtd_writev(struct mtd_info *mtd, const struct kvec *vecs,
>  int mtd_writev(struct mtd_info *mtd, const struct kvec *vecs,
>  	       unsigned long count, loff_t to, size_t *retlen)
>  {
> +	int i;
> +
>  	*retlen = 0;
>  	if (!(mtd->flags & MTD_WRITEABLE))
>  		return -EROFS;
>  	if (!mtd->_writev)
>  		return default_mtd_writev(mtd, vecs, count, to, retlen);
> +
> +	if (to < 0 || to > mtd->size)
> +		return -EINVAL;
> +	for (i = 0; i < count; i++) {
> +		if (!vecs[i].iov_len)
> +			continue;
> +		if (vecs[i].iov_len > mtd->size - to)

This isn't quite the right check. It's OK for the first iteration, but
it's not a good check for vecs[i].iov_len where i > 0. Maybe just check
the sum of all the lengths against `mtd->size - to'.

> +			return -EINVAL;
> +	}
>  	return mtd->_writev(mtd, vecs, count, to, retlen);
>  }
>  EXPORT_SYMBOL_GPL(mtd_writev);
> 
> Right?

Brian

Re: [PATCH 1/4] mtd: Add sysfs attr to expose ECC stats

[Ezequiel Garcia]

On 12 May 11:26 PM, Ezequiel Garcia wrote:
> On 12 May 05:50 PM, Brian Norris wrote:
> > > There are some guidelines about attributes in 'Documentation/filesystems/sysfs.txt'
> > > Though it's acceptable to put array of values of the "same type" in single sysfs file,
> > > But I'm still not confident on having all members of 'struct ecc_stats' being
> > > represented by single sysfs file
> > [...]
> > 
> > I agree, it looks like the sysfs policy would recommend against putting
> > distinct properties in the same file.
> > 
[..]
> > So I personally might lean toward "one file per attribute" here.
> > 

Brian,

Having agreed on doing one file per attribute, I'm now not sure how to
name them. Maybe you can give me a hand?

Let me add some context: these are the per-MTD partition fields of the
mtd_ecc_stats struct, although two of them aren't related to ECC, but to
the bad blocks management. This is the struct:

struct mtd_ecc_stats {
        __u32 corrected;
        __u32 failed;
        __u32 badblocks;
        __u32 bbtblocks;
};

How about the following?

* corrected_bits
* uncorrectable_errors
* badblocks
* bbtblocks
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 1/4] mtd: Add sysfs attr to expose ECC stats

[Brian Norris]

Trimming CC list

On Mon, May 19, 2014 at 12:43:55AM -0300, Ezequiel Garcia wrote:
> How about the following?
> 
> * corrected_bits
> * uncorrectable_errors
> * badblocks
> * bbtblocks

How about we consistently separate words with underscores ('_')? So:

 * corrected_bits
 * uncorrectable_errors
 * bad_blocks
 * bbt_blocks

I'm not too stuck on the naming, so other ideas are welcome.

Brian

Re: [PATCH 1/4] mtd: Add sysfs attr to expose ECC stats

[Ezequiel Garcia]

On 20 May 01:11 AM, Brian Norris wrote:
> On Mon, May 19, 2014 at 12:43:55AM -0300, Ezequiel Garcia wrote:
> > How about the following?
> > 
> > * corrected_bits
> > * uncorrectable_errors
> > * badblocks
> > * bbtblocks
> 
> How about we consistently separate words with underscores ('_')? So:
> 
>  * corrected_bits
>  * uncorrectable_errors
>  * bad_blocks
>  * bbt_blocks
>

Fine by me.
 
> I'm not too stuck on the naming, so other ideas are welcome.
> 

To be honest, me neither :)
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com