Re: [PATCH v7 17/17] PCI/AER: Add sysfs attributes for log ratelimits

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5392 bytes --]

On Wed, 21 May 2025, Jonathan Cameron wrote:

> On Tue, 20 May 2025 16:50:34 -0500
> Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> > From: Jon Pan-Doh <pandoh@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.stolarek@oracle.com>
> > Signed-off-by: Jon Pan-Doh <pandoh@google.com>
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > Tested-by: Krzysztof Wilczyński <kwilczynski@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.