* PCI/AER sysfs files violate the rules of how sysfs works
@ 2019-06-21 7:29 Greg KH
2019-06-21 14:15 ` Bjorn Helgaas
[not found] ` <CACK8Z6GeAheLfmPcYXNnrTn1Rg7C-rndi_YCxiLsePapGCMmzw@mail.gmail.com>
0 siblings, 2 replies; 6+ messages in thread
From: Greg KH @ 2019-06-21 7:29 UTC (permalink / raw)
To: Rajat Jain; +Cc: Bjorn Helgaas, linux-pci, linux-kernel
Hi,
When working on some documentation scripts to show the
Documentation/ABI/ files in an automated way, I ran across this "gem" of
a sysfs file: Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
In it you describe how the files
/sys/bus/pci/devices/<dev>/aer_dev_correctable and
/sys/bus/pci/devices/<dev>/aer_dev_fatal and
/sys/bus/pci/devices/<dev>/aer_dev_nonfatal
all display a bunch of text on multiple lines.
This violates the "one value per sysfs file" rule, and should never have
been merged as-is :(
Please fix it up to be a lot of individual files if your really need all
of those different values.
Remember, sysfs files should never have to have a parser to read them
other than a simple "what is this single value", and you should NEVER
have fun macros like:
for (i = 0; i < ARRAY_SIZE(strings_array); i++) { \
if (strings_array[i]) \
str += sprintf(str, "%s %llu\n", \
strings_array[i], stats[i]); \
else if (stats[i]) \
str += sprintf(str, #stats_array "_bit[%d] %llu\n",\
i, stats[i]); \
} \
str += sprintf(str, "TOTAL_%s %llu\n", total_string, \
pdev->aer_stats->total_field); \
spit out sysfs information.
Note, I am all for not properly checking the length of the sysfs file
when writing to it, but that is ONLY because you "know" that a single
integer will never overflow anything. Here you are writing a ton of
different values, with no error checking at all. So just when I thought
it couldn't be any worse...
Please fix.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: PCI/AER sysfs files violate the rules of how sysfs works
2019-06-21 7:29 PCI/AER sysfs files violate the rules of how sysfs works Greg KH
@ 2019-06-21 14:15 ` Bjorn Helgaas
2019-06-21 14:44 ` Greg KH
2019-06-28 0:56 ` Rajat Jain
[not found] ` <CACK8Z6GeAheLfmPcYXNnrTn1Rg7C-rndi_YCxiLsePapGCMmzw@mail.gmail.com>
1 sibling, 2 replies; 6+ messages in thread
From: Bjorn Helgaas @ 2019-06-21 14:15 UTC (permalink / raw)
To: Greg KH; +Cc: Rajat Jain, linux-pci, linux-kernel
On Fri, Jun 21, 2019 at 09:29:11AM +0200, Greg KH wrote:
> Hi,
>
> When working on some documentation scripts to show the
> Documentation/ABI/ files in an automated way, I ran across this "gem" of
> a sysfs file: Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
>
> In it you describe how the files
> /sys/bus/pci/devices/<dev>/aer_dev_correctable and
> /sys/bus/pci/devices/<dev>/aer_dev_fatal and
> /sys/bus/pci/devices/<dev>/aer_dev_nonfatal
> all display a bunch of text on multiple lines.
>
> This violates the "one value per sysfs file" rule, and should never have
> been merged as-is :(
>
> Please fix it up to be a lot of individual files if your really need all
> of those different values.
Sorry about that. Do you think we're safe in changing the sysfs ABI
by removing the original files and replacing them with new, better
ones? This is pretty new and hopefully not widely used yet.
Bjorn
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: PCI/AER sysfs files violate the rules of how sysfs works
2019-06-21 14:15 ` Bjorn Helgaas
@ 2019-06-21 14:44 ` Greg KH
2019-06-28 0:56 ` Rajat Jain
1 sibling, 0 replies; 6+ messages in thread
From: Greg KH @ 2019-06-21 14:44 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Rajat Jain, linux-pci, linux-kernel
On Fri, Jun 21, 2019 at 09:15:50AM -0500, Bjorn Helgaas wrote:
> On Fri, Jun 21, 2019 at 09:29:11AM +0200, Greg KH wrote:
> > Hi,
> >
> > When working on some documentation scripts to show the
> > Documentation/ABI/ files in an automated way, I ran across this "gem" of
> > a sysfs file: Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
> >
> > In it you describe how the files
> > /sys/bus/pci/devices/<dev>/aer_dev_correctable and
> > /sys/bus/pci/devices/<dev>/aer_dev_fatal and
> > /sys/bus/pci/devices/<dev>/aer_dev_nonfatal
> > all display a bunch of text on multiple lines.
> >
> > This violates the "one value per sysfs file" rule, and should never have
> > been merged as-is :(
> >
> > Please fix it up to be a lot of individual files if your really need all
> > of those different values.
>
> Sorry about that. Do you think we're safe in changing the sysfs ABI
> by removing the original files and replacing them with new, better
> ones?
I doubt any tool is parsing that monstrosity, so you should be fine :)
> This is pretty new and hopefully not widely used yet.
Only one way to find out...
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PCI/AER sysfs files violate the rules of how sysfs works
2019-06-21 14:15 ` Bjorn Helgaas
2019-06-21 14:44 ` Greg KH
@ 2019-06-28 0:56 ` Rajat Jain
2019-06-28 8:44 ` Greg KH
1 sibling, 1 reply; 6+ messages in thread
From: Rajat Jain @ 2019-06-28 0:56 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Greg KH, linux-pci, Linux Kernel Mailing List, Rajat Jain
On Fri, Jun 21, 2019 at 7:15 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Jun 21, 2019 at 09:29:11AM +0200, Greg KH wrote:
> > Hi,
> >
> > When working on some documentation scripts to show the
> > Documentation/ABI/ files in an automated way, I ran across this "gem" of
> > a sysfs file: Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
> >
> > In it you describe how the files
> > /sys/bus/pci/devices/<dev>/aer_dev_correctable and
> > /sys/bus/pci/devices/<dev>/aer_dev_fatal and
> > /sys/bus/pci/devices/<dev>/aer_dev_nonfatal
> > all display a bunch of text on multiple lines.
> >
> > This violates the "one value per sysfs file" rule, and should never have
> > been merged as-is :(
> >
> > Please fix it up to be a lot of individual files if your really need all
> > of those different values.
>
> Sorry about that. Do you think we're safe in changing the sysfs ABI
> by removing the original files and replacing them with new, better
> ones? This is pretty new and hopefully not widely used yet.
Hi Bjorn / Greg,
I'm thinking of having a named group for AER stats so that all the
individual counter attributes are put under a subdirectory (called
"aer_stats") in the sysfs, instead of cluttering the PCI device
directory. I expect to have the following counters in there:
dev_err_corr_<correctible_error_name> (Total 8 such files)
dev_err_fatal_<fatal_error_name> (Total 17 Such files)
dev_err_nonfatal_<fatal_error_name> (Total 17 Such files)
dev_total_err_corr (1file)
dev_total_err_fatal (1file)
dev_total_err_nonfatal (1file)
rootport_total_err_corr (1file - only for rootports)
rootport_total_err_fatal (1file - only for rootports)
rootport_total_err_nonfatal (1file - only for rootports)
Please let me know if this sounds ok.
Thanks & Best Regards,
Rajat
>
> Bjorn
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PCI/AER sysfs files violate the rules of how sysfs works
2019-06-28 0:56 ` Rajat Jain
@ 2019-06-28 8:44 ` Greg KH
0 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2019-06-28 8:44 UTC (permalink / raw)
To: Rajat Jain
Cc: Bjorn Helgaas, linux-pci, Linux Kernel Mailing List, Rajat Jain
On Thu, Jun 27, 2019 at 05:56:59PM -0700, Rajat Jain wrote:
> On Fri, Jun 21, 2019 at 7:15 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > On Fri, Jun 21, 2019 at 09:29:11AM +0200, Greg KH wrote:
> > > Hi,
> > >
> > > When working on some documentation scripts to show the
> > > Documentation/ABI/ files in an automated way, I ran across this "gem" of
> > > a sysfs file: Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
> > >
> > > In it you describe how the files
> > > /sys/bus/pci/devices/<dev>/aer_dev_correctable and
> > > /sys/bus/pci/devices/<dev>/aer_dev_fatal and
> > > /sys/bus/pci/devices/<dev>/aer_dev_nonfatal
> > > all display a bunch of text on multiple lines.
> > >
> > > This violates the "one value per sysfs file" rule, and should never have
> > > been merged as-is :(
> > >
> > > Please fix it up to be a lot of individual files if your really need all
> > > of those different values.
> >
> > Sorry about that. Do you think we're safe in changing the sysfs ABI
> > by removing the original files and replacing them with new, better
> > ones? This is pretty new and hopefully not widely used yet.
>
> Hi Bjorn / Greg,
>
> I'm thinking of having a named group for AER stats so that all the
> individual counter attributes are put under a subdirectory (called
> "aer_stats") in the sysfs, instead of cluttering the PCI device
> directory. I expect to have the following counters in there:
>
> dev_err_corr_<correctible_error_name> (Total 8 such files)
> dev_err_fatal_<fatal_error_name> (Total 17 Such files)
> dev_err_nonfatal_<fatal_error_name> (Total 17 Such files)
>
> dev_total_err_corr (1file)
> dev_total_err_fatal (1file)
> dev_total_err_nonfatal (1file)
>
> rootport_total_err_corr (1file - only for rootports)
> rootport_total_err_fatal (1file - only for rootports)
> rootport_total_err_nonfatal (1file - only for rootports)
>
> Please let me know if this sounds ok.
Sounds good to me.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <CACK8Z6GeAheLfmPcYXNnrTn1Rg7C-rndi_YCxiLsePapGCMmzw@mail.gmail.com>]
* Re: PCI/AER sysfs files violate the rules of how sysfs works
[not found] ` <CACK8Z6GeAheLfmPcYXNnrTn1Rg7C-rndi_YCxiLsePapGCMmzw@mail.gmail.com>
@ 2019-06-21 14:45 ` Greg Kroah-Hartman
0 siblings, 0 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2019-06-21 14:45 UTC (permalink / raw)
To: Rajat Jain; +Cc: Bjorn Helgaas, linux-pci, Linux Kernel Mailing List
On Fri, Jun 21, 2019 at 07:08:38AM -0700, Rajat Jain wrote:
> On Fri, Jun 21, 2019, 12:29 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> > Hi,
> >
> > When working on some documentation scripts to show the
> > Documentation/ABI/ files in an automated way, I ran across this "gem" of
> > a sysfs file: Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
> >
> > In it you describe how the files
> > /sys/bus/pci/devices/<dev>/aer_dev_correctable and
> > /sys/bus/pci/devices/<dev>/aer_dev_fatal and
> > /sys/bus/pci/devices/<dev>/aer_dev_nonfatal
> > all display a bunch of text on multiple lines.
> >
> > This violates the "one value per sysfs file" rule, and should never have
> > been merged as-is :(
> >
> > Please fix it up to be a lot of individual files if your really need all
> > of those different values.
> >
> > Remember, sysfs files should never have to have a parser to read them
> > other than a simple "what is this single value", and you should NEVER
> > have fun macros like:
> >
> > for (i = 0; i < ARRAY_SIZE(strings_array); i++) { \
> > if (strings_array[i]) \
> > str += sprintf(str, "%s %llu\n", \
> > strings_array[i], stats[i]); \
> > else if (stats[i]) \
> > str += sprintf(str, #stats_array "_bit[%d]
> > %llu\n",\
> > i, stats[i]); \
> > } \
> > str += sprintf(str, "TOTAL_%s %llu\n", total_string, \
> > pdev->aer_stats->total_field); \
> >
> > spit out sysfs information.
> >
> > Note, I am all for not properly checking the length of the sysfs file
> > when writing to it, but that is ONLY because you "know" that a single
> > integer will never overflow anything. Here you are writing a ton of
> > different values, with no error checking at all. So just when I thought
> > it couldn't be any worse...
> >
> > Please fix.
> >
>
> My apologies. I will discuss with Bjorn and fix this.
thank you. I'll be glad to review patches for this.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-28 8:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-21 7:29 PCI/AER sysfs files violate the rules of how sysfs works Greg KH
2019-06-21 14:15 ` Bjorn Helgaas
2019-06-21 14:44 ` Greg KH
2019-06-28 0:56 ` Rajat Jain
2019-06-28 8:44 ` Greg KH
[not found] ` <CACK8Z6GeAheLfmPcYXNnrTn1Rg7C-rndi_YCxiLsePapGCMmzw@mail.gmail.com>
2019-06-21 14:45 ` Greg Kroah-Hartman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).