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

[Shay Drori <shayd@nvidia.com>]

On 11/05/2024 10:44, Greg KH wrote:
> 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?

If I understand you correctly, you are referring to the xarray of the
auxdevice (not the global one).
If so, instead of xarray what can be used by the auxdevice?

> 
> thanks,
> 
> greg k-h
>