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

[Ezequiel Garcia <ezequiel.garcia@free-electrons.com>]

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