[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