Re: [PATCH net] driver core: auxiliary bus: Fix sysfs creation on bind

[Leon Romanovsky <leon@kernel.org>]

On Thu, Oct 23, 2025 at 09:19:27AM +0300, Tariq Toukan wrote:
> From: Amir Tzin <amirtz@nvidia.com>
> 
> In case an auxiliary device with IRQs directory is unbinded, the
> directory is released, but auxdev->sysfs.irq_dir_exists remains true.
> This leads to a failure recreating the directory on bind [1].
> 
> Using the attributes group visibility interface, expose the IRQs
> attributes group if"f the xarray storing IRQs entries is not empty. Now

if"f -> if

> irq_dir_exists field is redundant and can be removed.
> 
> [1]
> [] mlx5_core.sf mlx5_core.sf.2: mlx5_irq_affinity_request:167:(pid 1939):
>    Failed to create sysfs entry for irq 56, ret = -2
> [] mlx5_core.sf mlx5_core.sf.2: mlx5_eq_table_create:1195:(pid 1939):
>    Failed to create async EQs
> [] mlx5_core.sf mlx5_core.sf.2: mlx5_load:1362:(pid 1939):
>    Failed to create EQs
> 
> Fixes: a808878308a8 ("driver core: auxiliary bus: show auxiliary device IRQs")
> Signed-off-by: Amir Tzin <amirtz@nvidia.com>
> Reviewed-by: Mark Bloch <mbloch@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
> ---
>  drivers/base/auxiliary.c       |  13 +++-
>  drivers/base/auxiliary_sysfs.c | 117 +++++++++++++++++++++++++--------
>  include/linux/auxiliary_bus.h  |  26 ++++++--
>  3 files changed, 118 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c
> index 04bdbff4dbe5..b0fb31279257 100644
> --- a/drivers/base/auxiliary.c
> +++ b/drivers/base/auxiliary.c
> @@ -225,7 +225,16 @@ static int auxiliary_bus_probe(struct device *dev)
>  		return ret;
>  	}
>  
> -	return auxdrv->probe(auxdev, auxiliary_match_id(auxdrv->id_table, auxdev));
> +	ret = auxiliary_bus_irq_dir_res_probe(auxdev);
> +	if  (ret)
> +		return ret;
> +
> +	ret = auxdrv->probe(auxdev,
> +			    auxiliary_match_id(auxdrv->id_table, auxdev));
> +	if (ret)
> +		auxiliary_bus_irq_dir_res_remove(auxdev);
> +
> +	return ret;

return 0;

>  }
>  
>  static void auxiliary_bus_remove(struct device *dev)
> @@ -235,6 +244,7 @@ static void auxiliary_bus_remove(struct device *dev)
>  
>  	if (auxdrv->remove)
>  		auxdrv->remove(auxdev);
> +	auxiliary_bus_irq_dir_res_remove(auxdev);
>  }
>  
>  static void auxiliary_bus_shutdown(struct device *dev)
> @@ -294,7 +304,6 @@ int auxiliary_device_init(struct auxiliary_device *auxdev)
>  
>  	dev->bus = &auxiliary_bus_type;
>  	device_initialize(&auxdev->dev);
> -	mutex_init(&auxdev->sysfs.lock);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(auxiliary_device_init);
> diff --git a/drivers/base/auxiliary_sysfs.c b/drivers/base/auxiliary_sysfs.c
> index 754f21730afd..8ae3ec62b3db 100644
> --- a/drivers/base/auxiliary_sysfs.c
> +++ b/drivers/base/auxiliary_sysfs.c
> @@ -13,30 +13,71 @@ struct auxiliary_irq_info {
>  	char name[AUXILIARY_MAX_IRQ_NAME];
>  };
>  
> +static struct attribute auxiliary_irq_attr = {
> +	.mode = 0,

"static" variable is always initialized to 0, there is no need in .mode = 0 line.

> +	.name = "DUMMY",
> +};
> +
>  static struct attribute *auxiliary_irq_attrs[] = {
> -	NULL
> +	[0] = &auxiliary_irq_attr,
> +	[1] = NULL,

Let's use more common pattern - without [x]

>  };
>  
> +static bool auxiliary_irq_dir_group_visible(struct kobject *kobj)
> +{
> +	struct auxiliary_device *auxdev;
> +	struct device *dev;
> +
> +	dev = container_of(kobj, struct device, kobj);
> +	auxdev = container_of(dev, struct auxiliary_device, dev);
> +
> +	return !xa_empty(&auxdev->sysfs.irqs);
> +}
> +
> +DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(auxiliary_irq_dir);
> +
>  static const struct attribute_group auxiliary_irqs_group = {
>  	.name = "irqs",
>  	.attrs = auxiliary_irq_attrs,
> +	.is_visible = SYSFS_GROUP_VISIBLE(auxiliary_irq_dir),
>  };
>  
> -static int auxiliary_irq_dir_prepare(struct auxiliary_device *auxdev)
> +void auxiliary_bus_irq_dir_res_remove(struct auxiliary_device *auxdev)
>  {
> -	int ret = 0;
> +	struct device *dev = &auxdev->dev;
>  
> -	guard(mutex)(&auxdev->sysfs.lock);
> -	if (auxdev->sysfs.irq_dir_exists)
> -		return 0;
> +	sysfs_remove_group(&dev->kobj, &auxiliary_irqs_group);
> +	xa_destroy(&auxdev->sysfs.irqs);
> +	mutex_destroy(&auxdev->sysfs.lock);

Actually you don't need extra sysfs.lock mutex and everything can be
protected through xarray lock. So instead of two locks, you will have
one with proper lifetime.

> +}

<...>

> @@ -146,7 +145,6 @@ struct auxiliary_device {
>  	struct {
>  		struct xarray irqs;
>  		struct mutex lock; /* Synchronize irq sysfs creation */
> -		bool irq_dir_exists;

"struct mutex lock" can be replaced with xa_lock(&auxdev->sysfs.irqs).

Thanks