On Wed, May 29, 2024 at 09:29:33AM +0300, Shay Drori wrote:
>
>
> On 28/05/2024 21:00, Greg KH wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Tue, May 28, 2024 at 12:11:43PM +0300, Shay Drory wrote:
> > > +#ifdef CONFIG_SYSFS
> > > +/* Xarray of irqs to determine if irq is exclusive or shared. */
> > > +static DEFINE_XARRAY(irqs);
> > > +/* Protects insertions into the irqs xarray. */
> > > +static DEFINE_MUTEX(irqs_lock);
> >
> > You access the irq xarray without grabbing the lock in places :(
> >
> > But again, I fail to see why the xarray is needed at all, why isn't the
> > needed information here:
> >
> > > +struct auxiliary_irq_info {
> > > + struct device_attribute sysfs_attr;
> > > + int irq;
> > > +};
> >
> > Right there^ should contain everything you need, NOT a global array and
> > lock at all.
>
>
> 1) one xarray is per aux device that indicates which IRQs irqs are used
> by this device. this xarray is holding the info above.
Ok, please document that better, it's not obvious.
> 2) second xarray is global that tracks if irq share between multiple aux
> devices or exclusive to aux device.
That should not be a "global" thing, as now you are getting into what
the msi irq core should be handling, NOT the aux device.
Userspace should be able to determine, just by the number, if it is
"shared" or not by looking at them all, so why need to add complex logic
here to attempt to also mirror this information?
Doesn't the irq layer track this sufficiently? And it wouldn't even be
correct if an irq was "shared" by a device that was NOT controlled by an
aux device so it could be incorrect.
thanks,
greg k-h
