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
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
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
On Fri, Jun 21, 2019 at 07:08:38AM -0700, Rajat Jain wrote:
> On Fri, Jun 21, 2019, 12:29 AM Greg KH <[email protected]> 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
On Fri, Jun 21, 2019 at 7:15 AM Bjorn Helgaas <[email protected]> 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
On Thu, Jun 27, 2019 at 05:56:59PM -0700, Rajat Jain wrote:
> On Fri, Jun 21, 2019 at 7:15 AM Bjorn Helgaas <[email protected]> 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