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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2024051114-gladly-feline-4302@gregkh>
Date: Sat, 11 May 2024 08:44:50 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
Cc: Shay Drory <shayd@...dia.com>, netdev@...r.kernel.org,
	pabeni@...hat.com, davem@...emloft.net, kuba@...nel.org,
	edumazet@...gle.com, david.m.ertman@...el.com, rafael@...nel.org,
	ira.weiny@...el.com, linux-rdma@...r.kernel.org, leon@...nel.org,
	tariqt@...dia.com, Parav Pandit <parav@...dia.com>
Subject: Re: [PATCH net-next v4 1/2] driver core: auxiliary bus: show
 auxiliary device IRQs

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