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

[Greg KH <gregkh@linuxfoundation.org>]

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.

> +/* Auxiliary devices can share IRQs. Expose to user whether the provided IRQ is
> + * shared or exclusive.

Why are you using networking comment style here?  :)

> 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.

confused,

greg k-h