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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Wed, 21 May 2025 17:59:42 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> On Wed, May 21, 2025 at 11:46:00AM +0100, 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.  
> > ...  
> 
> > 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.  
> 
> > > +#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.  
> 
> Yep, fixed.
> 
> > > +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, we can
> > make sysfs less cluttered for new stuff.
> > 
> > Maybe I'm missing why that doesn't work here though!  
> 
> Is this something we can fix later, or are we locking ourselves into
> user-visible ABI that's hard to change?  I'm kind of against the wall
> relative to the v6.16 merge window and haven't had time to dig into
> this part.

That comes down to Ilpo's question of whether empty directories
are ABI (specifically if anyone notices us removing them).
It seems unlikely anyone will code against requirement for an empty
dir, but you never know.

Given we probably have a bunch of these in PCI anyway that predate
that magic, one more isn't a problem even if we decide we can't
tidy it up later.

So I'm fine with not bothering to hide the dir for now (and maybe for
ever).

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


Jonathan

>