On Wed, 21 May 2025, Jonathan Cameron wrote:
> On Tue, 20 May 2025 16:50:34 -0500
> Bjorn Helgaas <helg...@kernel.org> wrote:
>
> > From: Jon Pan-Doh <pan...@google.com>
> >
> > Allow userspace to read/write log ratelimits per device (including
> > enable/disable). Create aer/ sysfs directory to store them and any
> > future aer configs.
> >
> > Update AER sysfs ABI filename to reflect the broader scope of AER sysfs
> > attributes (e.g. stats and ratelimits).
> >
> > Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats ->
> > sysfs-bus-pci-devices-aer
> >
> > Tested using aer-inject[1]. Configured correctable log ratelimit to 5.
> > Sent 6 AER errors. Observed 5 errors logged while AER stats
> > (cat /sys/bus/pci/devices/<dev>/aer_dev_correctable) shows 6.
> >
> > Disabled ratelimiting and sent 6 more AER errors. Observed all 6 errors
> > logged and accounted in AER stats (12 total errors).
> >
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/gong.chen/aer-inject.git
> >
> > [bhelgaas: note fatal errors are not ratelimited, "aer_report" ->
> > "aer_info"]
> > Signed-off-by: Karolina Stolarek <karolina.stola...@oracle.com>
> > Signed-off-by: Jon Pan-Doh <pan...@google.com>
> > Signed-off-by: Bjorn Helgaas <bhelg...@google.com>
> > Tested-by: Krzysztof Wilczyński <kwilczyn...@kernel.org>
>
> There is some relatively new SYSFS infra that I think will help
> make this slightly nicer by getting rid of the extra directory when
> there is nothing to be done with it.
>
> > ---
> > ...es-aer_stats => sysfs-bus-pci-devices-aer} | 34 +++++++
> > Documentation/PCI/pcieaer-howto.rst | 5 +-
> > drivers/pci/pci-sysfs.c | 1 +
> > drivers/pci/pci.h | 1 +
> > drivers/pci/pcie/aer.c | 99 +++++++++++++++++++
> > 5 files changed, 139 insertions(+), 1 deletion(-)
> > rename Documentation/ABI/testing/{sysfs-bus-pci-devices-aer_stats =>
> > sysfs-bus-pci-devices-aer} (77%)
>
>
> > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> > index f9e684ac7878..9b8dea317a79 100644
> > --- a/drivers/pci/pcie/aer.c
> > +++ b/drivers/pci/pcie/aer.c
> > @@ -627,6 +627,105 @@ const struct attribute_group aer_stats_attr_group = {
> > .is_visible = aer_stats_attrs_are_visible,
> > };
>
> > +#define aer_ratelimit_burst_attr(name, ratelimit) \
> > + static ssize_t \
> > + name##_show(struct device *dev, struct device_attribute *attr, \
> > + char *buf) \
> > +{ \
>
> A little odd looking to indent this less than the line above.
>
> > + struct pci_dev *pdev = to_pci_dev(dev); \
> > + \
> > + return sysfs_emit(buf, "%d\n", \
> > + pdev->aer_info->ratelimit.burst); \
> > +} \
> > + \
> > + static ssize_t \
> > + name##_store(struct device *dev, struct device_attribute *attr, \
> > + const char *buf, size_t count) \
> > +{ \
> > + struct pci_dev *pdev = to_pci_dev(dev); \
> > + int burst; \
> > + \
> > + if (!capable(CAP_SYS_ADMIN)) \
> > + return -EPERM; \
> > + \
> > + if (kstrtoint(buf, 0, &burst) < 0) \
> > + return -EINVAL; \
> > + \
> > + pdev->aer_info->ratelimit.burst = burst; \
> > + \
> > + return count; \
> > +} \
> > +static DEVICE_ATTR_RW(name)
> > +
> > +aer_ratelimit_burst_attr(ratelimit_burst_cor_log, cor_log_ratelimit);
> > +aer_ratelimit_burst_attr(ratelimit_burst_uncor_log, uncor_log_ratelimit);
> > +
> > +static struct attribute *aer_attrs[] = {
> > + &dev_attr_ratelimit_log_enable.attr,
> > + &dev_attr_ratelimit_burst_cor_log.attr,
> > + &dev_attr_ratelimit_burst_uncor_log.attr,
> > + NULL
> > +};
> > +
> > +static umode_t aer_attrs_are_visible(struct kobject *kobj,
> > + struct attribute *a, int n)
> > +{
> > + struct device *dev = kobj_to_dev(kobj);
> > + struct pci_dev *pdev = to_pci_dev(dev);
> > +
> > + if (!pdev->aer_info)
> > + return 0;
> > +
> > + return a->mode;
> > +}
> > +
> > +const struct attribute_group aer_attr_group = {
> > + .name = "aer",
> > + .attrs = aer_attrs,
> > + .is_visible = aer_attrs_are_visible,
> > +};
>
> There are a bunch of macros to simplify cases where
> a whole group is either enabled or not and make the group
> itself go away if there is nothing to be shown.
>
> DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE() combined with
> SYSFS_GROUP_VISIBLE() around the assignment does what we
> want here I think.
>
> Whilst we can't retrofit that stuff onto existing ABI
> as someone may be assuming directory presence,
Are you sure about this? That empty directories are part of ABI as well?
Are any of these directories listed under Documentation/ABI ?
I can see somebody could in theory rely on the existance of empty
directories but it's not like it contains any real substance without
a file with the actual content of interest so it seems somewhat strange
to check for directory and not the file of interest itself.
> we can make sysfs less cluttered for new stuff.
>
> Maybe I'm missing why that doesn't work here though!
>
> J
>
> > +
> > static void pci_dev_aer_stats_incr(struct pci_dev *pdev,
> > struct aer_err_info *info)
> > {
>
--
i.
