Re: [PATCH net-next v4 1/2] driver core: auxiliary bus: show auxiliary device IRQs

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, May 10, 2024 at 04:01:01PM +0200, Przemek Kitszel wrote:
> On 5/10/24 15:07, Greg KH wrote:
> > On Fri, May 10, 2024 at 02:54:49PM +0200, Przemek Kitszel wrote:
> > > > > +static ssize_t auxiliary_irq_mode_show(struct device *dev,
> > > > > +				       struct device_attribute *attr, char *buf)
> > > > > +{
> > > > > +	struct auxiliary_irq_info *info =
> > > > > +		container_of(attr, struct auxiliary_irq_info, sysfs_attr);
> > > > > +
> > > > > +	if (refcount_read(xa_load(&irqs, info->irq)) > 1)
> > > > 
> > > > refcount combined with xa?  That feels wrong, why is refcount used for
> > > > this at all?
> > > 
> > > Not long ago I commented on similar usage for ice driver,
> > > ~"since you are locking anyway this could be a plain counter",
> > > and author replied
> > > ~"additional semantics (like saturation) of refcount make me feel warm
> > > and fuzzy" (sorry if misquoting too much).
> > > That convinced me back then, so I kept quiet about that here.
> > 
> > But why is this being incremented / decremented at all?  What is that
> > for?
> 
> [global]
> This is just a counter, it is used to tell if given IRQ is shared or
> exclusive. Hence there is a global xarray for that.
> And my argument is for this case precisely.
> 
> [other]
> There is also a separate xarray for each auxdev (IIRC) which is used as
> generic dynamic container [that stores sysfs attrs], any other would
> work (with different characteristics), but I see no problems with
> picking xarray here.

Again, why is an xarray needed, why isn't this part of the auxdevice
structure to start with?

thanks,

greg k-h