netdev - Re: [PATCH net-next v5 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]

Date: Wed, 29 May 2024 09:29:33 +0300
From: Shay Drori <shayd@...dia.com>
To: Greg KH <gregkh@...uxfoundation.org>
CC: <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 v5 1/2] driver core: auxiliary bus: show
 auxiliary device IRQs



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.
2) second xarray is global that tracks if irq share between multiple aux
devices or exclusive to aux device.


> 
>> +/* Auxiliary devices can share IRQs. Expose to user whether the provided IRQ is
>> + * shared or exclusive.
> 
> Why are you using networking comment style here?  :)

correct, will fix in next version

> 
>> diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
>> index de21d9d24a95..760fadb26620 100644
>> --- a/include/linux/auxiliary_bus.h
>> +++ b/include/linux/auxiliary_bus.h
>> @@ -58,6 +58,7 @@
>>    *       in
>>    * @name: Match name found by the auxiliary device driver,
>>    * @id: unique identitier if multiple devices of the same name are exported,
>> + * @irqs: irqs xarray contains irq indices which are used by the device,
>>    *
>>    * An auxiliary_device represents a part of its parent device's functionality.
>>    * It is given a name that, combined with the registering drivers
>> @@ -138,6 +139,7 @@
>>   struct auxiliary_device {
>>        struct device dev;
>>        const char *name;
>> +     struct xarray irqs;
> 
> wait, why is an xarray added here too?  That feels wrong, or odd, or
> something as you seem to have multiple xarrays here when it feels like
> you need none.
> 

please look the answer above

> confused,
> 
> greg k-h