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
