All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Cc: "linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>,
	Brian Norris <computersforpeace@gmail.com>,
	David Woodhouse <dwmw2@infradead.org>,
	"Gupta, Pekon" <pekon@ti.com>,
	Linux Kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/4] mtd: Add sysfs attr to expose ECC stats
Date: Tue, 13 May 2014 13:15:24 +0200	[thread overview]
Message-ID: <20140513111524.GA29552@kroah.com> (raw)
In-Reply-To: <20140513022614.GA1447@arch.cereza>

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

WARNING: multiple messages have this Message-ID (diff)
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Cc: Brian Norris <computersforpeace@gmail.com>,
	"Gupta, Pekon" <pekon@ti.com>,
	David Woodhouse <dwmw2@infradead.org>,
	"linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>,
	Linux Kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/4] mtd: Add sysfs attr to expose ECC stats
Date: Tue, 13 May 2014 13:15:24 +0200	[thread overview]
Message-ID: <20140513111524.GA29552@kroah.com> (raw)
In-Reply-To: <20140513022614.GA1447@arch.cereza>

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

  reply	other threads:[~2014-05-13 11:15 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-03-21 11:57 [PATCH 0/4] mtd: Fix wrong bad block account in ECC stats Ezequiel Garcia
2014-03-21 11:57 ` [PATCH 1/4] mtd: Add sysfs attr to expose " Ezequiel Garcia
2014-03-27 11:56   ` Gupta, Pekon
2014-04-01 11:13     ` Ezequiel Garcia
2014-04-15 11:13       ` Gupta, Pekon
2014-05-13  0:50         ` Brian Norris
2014-05-13  0:50           ` Brian Norris
2014-05-13  2:26           ` Ezequiel Garcia
2014-05-13  2:26             ` Ezequiel Garcia
2014-05-13 11:15             ` Greg Kroah-Hartman [this message]
2014-05-13 11:15               ` Greg Kroah-Hartman
2014-05-13 13:41               ` Ezequiel Garcia
2014-05-13 13:41                 ` Ezequiel Garcia
2014-05-19  3:43             ` Ezequiel Garcia
2014-05-19  3:43               ` Ezequiel Garcia
2014-05-20  8:11               ` Brian Norris
2014-05-20 16:06                 ` Ezequiel Garcia
2014-03-21 11:57 ` [PATCH 2/4] mtd: nand: Account the blocks used by the BBT in the ecc_stats Ezequiel Garcia
2014-05-13  2:27   ` Ezequiel Garcia
2014-05-13  2:36     ` Brian Norris
2014-05-13 13:44       ` Ezequiel Garcia
2014-03-21 11:57 ` [PATCH 3/4] mtd: Introduce mtd_block_isreserved() Ezequiel Garcia
2014-05-13  1:31   ` Brian Norris
2014-05-14 23:37     ` Ezequiel Garcia
2014-05-14 23:57       ` Brian Norris
2014-05-15 20:15         ` Ezequiel Garcia
2014-05-16  5:47           ` Brian Norris
2014-03-21 11:57 ` [PATCH 4/4] mtd: Account for BBT blocks when a partition is being allocated Ezequiel Garcia
2014-05-13  2:28   ` Brian Norris
2014-04-12 14:40 ` [PATCH 0/4] mtd: Fix wrong bad block account in ECC stats Ezequiel Garcia

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140513111524.GA29552@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=computersforpeace@gmail.com \
    --cc=dwmw2@infradead.org \
    --cc=ezequiel.garcia@free-electrons.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=pekon@ti.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.