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]

Message-ID: <2024052829-pretended-dad-ac9b@gregkh>
Date: Tue, 28 May 2024 20:00:11 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Shay Drory <shayd@...dia.com>
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 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