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

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

[Tariq Toukan]

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

Expose functions auxiliary_device_sysfs_irq_dir_init() and
auxiliary_device_sysfs_irq_dir_destroy(). Move the responsibility for
the IRQs directory creation and destruction to the drivers. This change
corresponds to the general concept according to which the core driver
manages the auxiliary device locking and lifetime. Now the auxiliary
device sysfs related fields, irq_dir_exists and lock, are redundant and
can be removed.

mlx5 SFs, the only users of IRQs sysfs API, must align. Create the
directory before a SF control irq is requested and destroy it upon
its release.

[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: Leon Romanovsky <leonro@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/base/auxiliary.c                      |  1 -
 drivers/base/auxiliary_sysfs.c                | 48 ++++++++++++++-----
 .../net/ethernet/mellanox/mlx5/core/pci_irq.c | 20 ++++++++
 include/linux/auxiliary_bus.h                 | 16 +++++--
 4 files changed, 66 insertions(+), 19 deletions(-)

V2:
- Move the irq directory management from core to drivers: 
  expose auxiliary_device_sysfs_irq_dir_init()
  auxiliary_device_sysfs_irq_dir_destroy().  Drop the attribute group
  visibility approach.
  Remove the mutex from the auxiliary sysfs structure (drivers are
  responsible for concurrency). 
- Revert auxiliary_device_sysfs_irq_remove() to return void.
- Call auxiliary_device_sysfs_irq_dir_(init|destroy) in mlx5 SF device
  create/remove flows.
- Link to V1: https://lore.kernel.org/all/1761200367-922346-1-git-send-email-tariqt@nvidia.com/

diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c
index 04bdbff4dbe5..e1b8c4bc306e 100644
--- a/drivers/base/auxiliary.c
+++ b/drivers/base/auxiliary.c
@@ -294,7 +294,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..598a012ce481 100644
--- a/drivers/base/auxiliary_sysfs.c
+++ b/drivers/base/auxiliary_sysfs.c
@@ -22,22 +22,47 @@ static const struct attribute_group auxiliary_irqs_group = {
 	.attrs = auxiliary_irq_attrs,
 };
 
-static int auxiliary_irq_dir_prepare(struct auxiliary_device *auxdev)
+/**
+ * auxiliary_device_sysfs_irq_dir_init - initialize the IRQ sysfs directory
+ * @auxdev: auxiliary bus device to initialize the sysfs directory.
+ *
+ * This function should be called by drivers to initialize the IRQ directory
+ * before adding any IRQ sysfs entries. The driver is responsible for ensuring
+ * this function is called only once and for handling any concurrency control
+ * if needed.
+ *
+ * Drivers must call auxiliary_device_sysfs_irq_dir_destroy() to clean up when
+ * done.
+ *
+ * Return: zero on success or an error code on failure.
+ */
+int auxiliary_device_sysfs_irq_dir_init(struct auxiliary_device *auxdev)
 {
-	int ret = 0;
-
-	guard(mutex)(&auxdev->sysfs.lock);
-	if (auxdev->sysfs.irq_dir_exists)
-		return 0;
+	int ret;
 
-	ret = devm_device_add_group(&auxdev->dev, &auxiliary_irqs_group);
+	ret = sysfs_create_group(&auxdev->dev.kobj, &auxiliary_irqs_group);
 	if (ret)
 		return ret;
 
-	auxdev->sysfs.irq_dir_exists = true;
 	xa_init(&auxdev->sysfs.irqs);
 	return 0;
 }
+EXPORT_SYMBOL_GPL(auxiliary_device_sysfs_irq_dir_init);
+
+/**
+ * auxiliary_device_sysfs_irq_dir_destroy - destroy the IRQ sysfs directory
+ * @auxdev: auxiliary bus device to destroy the sysfs directory.
+ *
+ * This function should be called by drivers to clean up the IRQ directory
+ * after all IRQ sysfs entries have been removed. The driver is responsible
+ * for ensuring all IRQs are removed before calling this function.
+ */
+void auxiliary_device_sysfs_irq_dir_destroy(struct auxiliary_device *auxdev)
+{
+	xa_destroy(&auxdev->sysfs.irqs);
+	sysfs_remove_group(&auxdev->dev.kobj, &auxiliary_irqs_group);
+}
+EXPORT_SYMBOL_GPL(auxiliary_device_sysfs_irq_dir_destroy);
 
 /**
  * auxiliary_device_sysfs_irq_add - add a sysfs entry for the given IRQ
@@ -45,7 +70,8 @@ static int auxiliary_irq_dir_prepare(struct auxiliary_device *auxdev)
  * @irq: The associated interrupt number.
  *
  * This function should be called after auxiliary device have successfully
- * received the irq.
+ * received the irq. The driver must call auxiliary_device_sysfs_irq_dir_init()
+ * before calling this function for the first time.
  * The driver is responsible to add a unique irq for the auxiliary device. The
  * driver can invoke this function from multiple thread context safely for
  * unique irqs of the auxiliary devices. The driver must not invoke this API
@@ -59,10 +85,6 @@ int auxiliary_device_sysfs_irq_add(struct auxiliary_device *auxdev, int irq)
 	struct device *dev = &auxdev->dev;
 	int ret;
 
-	ret = auxiliary_irq_dir_prepare(auxdev);
-	if (ret)
-		return ret;
-
 	info = kzalloc(sizeof(*info), GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c b/drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c
index aa3b5878e3da..b6e872d7a925 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c
@@ -456,7 +456,15 @@ static void _mlx5_irq_release(struct mlx5_irq *irq)
  */
 void mlx5_ctrl_irq_release(struct mlx5_core_dev *dev, struct mlx5_irq *ctrl_irq)
 {
+	struct mlx5_irq_pool *pool = ctrl_irq_pool_get(dev);
+
 	mlx5_irq_affinity_irq_release(dev, ctrl_irq);
+
+	if (mlx5_irq_pool_is_sf_pool(pool)) {
+		struct auxiliary_device *sadev = mlx5_sf_coredev_to_adev(dev);
+
+		auxiliary_device_sysfs_irq_dir_destroy(sadev);
+	}
 }
 
 /**
@@ -489,9 +497,21 @@ struct mlx5_irq *mlx5_ctrl_irq_request(struct mlx5_core_dev *dev)
 		/* Allocate the IRQ in index 0. The vector was already allocated */
 		irq = irq_pool_request_vector(pool, 0, af_desc, NULL);
 	} else {
+		struct auxiliary_device *sadev = mlx5_sf_coredev_to_adev(dev);
+		int err;
+
+		err = auxiliary_device_sysfs_irq_dir_init(sadev);
+		if (err) {
+			irq = ERR_PTR(err);
+			goto exit;
+		}
+
 		irq = mlx5_irq_affinity_request(dev, pool, af_desc);
+		if (IS_ERR(irq))
+			auxiliary_device_sysfs_irq_dir_destroy(sadev);
 	}
 
+exit:
 	kvfree(af_desc);
 
 	return irq;
diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
index 4086afd0cc6b..d052cb687e6f 100644
--- a/include/linux/auxiliary_bus.h
+++ b/include/linux/auxiliary_bus.h
@@ -60,8 +60,6 @@
  * @id: unique identitier if multiple devices of the same name are exported,
  * @sysfs: embedded struct which hold all sysfs related fields,
  * @sysfs.irqs: irqs xarray contains irq indices which are used by the device,
- * @sysfs.lock: Synchronize irq sysfs creation,
- * @sysfs.irq_dir_exists: whether "irqs" directory exists,
  *
  * An auxiliary_device represents a part of its parent device's functionality.
  * It is given a name that, combined with the registering drivers
@@ -145,8 +143,6 @@ struct auxiliary_device {
 	u32 id;
 	struct {
 		struct xarray irqs;
-		struct mutex lock; /* Synchronize irq sysfs creation */
-		bool irq_dir_exists;
 	} sysfs;
 };
 
@@ -222,10 +218,21 @@ int __auxiliary_device_add(struct auxiliary_device *auxdev, const char *modname)
 #define auxiliary_device_add(auxdev) __auxiliary_device_add(auxdev, KBUILD_MODNAME)
 
 #ifdef CONFIG_SYSFS
+int auxiliary_device_sysfs_irq_dir_init(struct auxiliary_device *auxdev);
+void auxiliary_device_sysfs_irq_dir_destroy(struct auxiliary_device *auxdev);
 int auxiliary_device_sysfs_irq_add(struct auxiliary_device *auxdev, int irq);
 void auxiliary_device_sysfs_irq_remove(struct auxiliary_device *auxdev,
 				       int irq);
 #else /* CONFIG_SYSFS */
+static inline int
+auxiliary_device_sysfs_irq_dir_init(struct auxiliary_device *auxdev)
+{
+	return 0;
+}
+
+static inline void
+auxiliary_device_sysfs_irq_dir_destroy(struct auxiliary_device *auxdev) {}
+
 static inline int
 auxiliary_device_sysfs_irq_add(struct auxiliary_device *auxdev, int irq)
 {
@@ -238,7 +245,6 @@ auxiliary_device_sysfs_irq_remove(struct auxiliary_device *auxdev, int irq) {}
 
 static inline void auxiliary_device_uninit(struct auxiliary_device *auxdev)
 {
-	mutex_destroy(&auxdev->sysfs.lock);
 	put_device(&auxdev->dev);
 }
 

base-commit: 8dfce8991b95d8625d0a1d2896e42f93b9d7f68d
-- 
2.44.0

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

[Jacob Keller]

On 2/19/2026 1:04 PM, 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].
> 
> Expose functions auxiliary_device_sysfs_irq_dir_init() and
> auxiliary_device_sysfs_irq_dir_destroy(). Move the responsibility for
> the IRQs directory creation and destruction to the drivers. This change
> corresponds to the general concept according to which the core driver
> manages the auxiliary device locking and lifetime. Now the auxiliary
> device sysfs related fields, irq_dir_exists and lock, are redundant and
> can be removed.
> 
> mlx5 SFs, the only users of IRQs sysfs API, must align. Create the
> directory before a SF control irq is requested and destroy it upon
> its release.
> 
> [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
> 

This approach looks sensible, and managing the lifetime in the parent 
driver is easier than exposing the extra locks and adding more potential 
issues for mismanaging those locks.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

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

[Danilo Krummrich]

On Thu Feb 19, 2026 at 10:04 PM CET, Tariq Toukan wrote:
> +/**
> + * auxiliary_device_sysfs_irq_dir_init - initialize the IRQ sysfs directory
> + * @auxdev: auxiliary bus device to initialize the sysfs directory.
> + *
> + * This function should be called by drivers to initialize the IRQ directory
> + * before adding any IRQ sysfs entries. The driver is responsible for ensuring
> + * this function is called only once and for handling any concurrency control
> + * if needed.
> + *
> + * Drivers must call auxiliary_device_sysfs_irq_dir_destroy() to clean up when
> + * done.
> + *
> + * Return: zero on success or an error code on failure.
> + */
> +int auxiliary_device_sysfs_irq_dir_init(struct auxiliary_device *auxdev)
>  {
> -	int ret = 0;
> -
> -	guard(mutex)(&auxdev->sysfs.lock);
> -	if (auxdev->sysfs.irq_dir_exists)
> -		return 0;
> +	int ret;
>  
> -	ret = devm_device_add_group(&auxdev->dev, &auxiliary_irqs_group);
> +	ret = sysfs_create_group(&auxdev->dev.kobj, &auxiliary_irqs_group);
>  	if (ret)
>  		return ret;
>  
> -	auxdev->sysfs.irq_dir_exists = true;
>  	xa_init(&auxdev->sysfs.irqs);
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(auxiliary_device_sysfs_irq_dir_init);
> +
> +/**
> + * auxiliary_device_sysfs_irq_dir_destroy - destroy the IRQ sysfs directory
> + * @auxdev: auxiliary bus device to destroy the sysfs directory.
> + *
> + * This function should be called by drivers to clean up the IRQ directory
> + * after all IRQ sysfs entries have been removed. The driver is responsible
> + * for ensuring all IRQs are removed before calling this function.
> + */
> +void auxiliary_device_sysfs_irq_dir_destroy(struct auxiliary_device *auxdev)
> +{
> +	xa_destroy(&auxdev->sysfs.irqs);
> +	sysfs_remove_group(&auxdev->dev.kobj, &auxiliary_irqs_group);
> +}
> +EXPORT_SYMBOL_GPL(auxiliary_device_sysfs_irq_dir_destroy);
>  
>  /**
>   * auxiliary_device_sysfs_irq_add - add a sysfs entry for the given IRQ
> @@ -45,7 +70,8 @@ static int auxiliary_irq_dir_prepare(struct auxiliary_device *auxdev)
>   * @irq: The associated interrupt number.
>   *
>   * This function should be called after auxiliary device have successfully
> - * received the irq.
> + * received the irq. The driver must call auxiliary_device_sysfs_irq_dir_init()
> + * before calling this function for the first time.

I'm not convinced by this approach. This adds two new sources of bugs for
drivers.

  1. Drivers can now forget to call auxiliary_device_sysfs_irq_dir_init()
     *before* auxiliary_device_sysfs_irq_add().

  2. Drivers can forget to call auxiliary_device_sysfs_irq_dir_destroy().

Instead, I suggest to keep the current approach and just replace
devm_device_add_group() with devm_auxiliary_device_add_group(), which in its
devres callback additionally clears auxdev->sysfs.irq_dir_exists.

In terms of the auxdev->sysfs.lock, I think this can still be removed, as it
wasn't needed in the first place.

auxiliary_device_sysfs_irq_add() must only be called from a scope where the
auxiliary device is guaranteed to be bound, so there can't be a concurrent
unbind.

There may only be multiple concurrent calls to auxiliary_device_sysfs_irq_add()
itself, and in this case irq_dir_exists can just be an atomic.

Yes, we're still stuck with an atomic for irq_dir_exists, but the driver API
remains much simpler and less error prone.

- Danilo

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

[Greg Kroah-Hartman]

On Fri, Feb 20, 2026 at 12:59:45AM +0100, Danilo Krummrich wrote:
> On Thu Feb 19, 2026 at 10:04 PM CET, Tariq Toukan wrote:
> > +/**
> > + * auxiliary_device_sysfs_irq_dir_init - initialize the IRQ sysfs directory
> > + * @auxdev: auxiliary bus device to initialize the sysfs directory.
> > + *
> > + * This function should be called by drivers to initialize the IRQ directory
> > + * before adding any IRQ sysfs entries. The driver is responsible for ensuring
> > + * this function is called only once and for handling any concurrency control
> > + * if needed.
> > + *
> > + * Drivers must call auxiliary_device_sysfs_irq_dir_destroy() to clean up when
> > + * done.
> > + *
> > + * Return: zero on success or an error code on failure.
> > + */
> > +int auxiliary_device_sysfs_irq_dir_init(struct auxiliary_device *auxdev)
> >  {
> > -	int ret = 0;
> > -
> > -	guard(mutex)(&auxdev->sysfs.lock);
> > -	if (auxdev->sysfs.irq_dir_exists)
> > -		return 0;
> > +	int ret;
> >  
> > -	ret = devm_device_add_group(&auxdev->dev, &auxiliary_irqs_group);
> > +	ret = sysfs_create_group(&auxdev->dev.kobj, &auxiliary_irqs_group);
> >  	if (ret)
> >  		return ret;
> >  
> > -	auxdev->sysfs.irq_dir_exists = true;
> >  	xa_init(&auxdev->sysfs.irqs);
> >  	return 0;
> >  }
> > +EXPORT_SYMBOL_GPL(auxiliary_device_sysfs_irq_dir_init);
> > +
> > +/**
> > + * auxiliary_device_sysfs_irq_dir_destroy - destroy the IRQ sysfs directory
> > + * @auxdev: auxiliary bus device to destroy the sysfs directory.
> > + *
> > + * This function should be called by drivers to clean up the IRQ directory
> > + * after all IRQ sysfs entries have been removed. The driver is responsible
> > + * for ensuring all IRQs are removed before calling this function.
> > + */
> > +void auxiliary_device_sysfs_irq_dir_destroy(struct auxiliary_device *auxdev)
> > +{
> > +	xa_destroy(&auxdev->sysfs.irqs);
> > +	sysfs_remove_group(&auxdev->dev.kobj, &auxiliary_irqs_group);
> > +}
> > +EXPORT_SYMBOL_GPL(auxiliary_device_sysfs_irq_dir_destroy);
> >  
> >  /**
> >   * auxiliary_device_sysfs_irq_add - add a sysfs entry for the given IRQ
> > @@ -45,7 +70,8 @@ static int auxiliary_irq_dir_prepare(struct auxiliary_device *auxdev)
> >   * @irq: The associated interrupt number.
> >   *
> >   * This function should be called after auxiliary device have successfully
> > - * received the irq.
> > + * received the irq. The driver must call auxiliary_device_sysfs_irq_dir_init()
> > + * before calling this function for the first time.
> 
> I'm not convinced by this approach. This adds two new sources of bugs for
> drivers.
> 
>   1. Drivers can now forget to call auxiliary_device_sysfs_irq_dir_init()
>      *before* auxiliary_device_sysfs_irq_add().
> 
>   2. Drivers can forget to call auxiliary_device_sysfs_irq_dir_destroy().
> 
> Instead, I suggest to keep the current approach and just replace
> devm_device_add_group() with devm_auxiliary_device_add_group(), which in its
> devres callback additionally clears auxdev->sysfs.irq_dir_exists.
> 
> In terms of the auxdev->sysfs.lock, I think this can still be removed, as it
> wasn't needed in the first place.
> 
> auxiliary_device_sysfs_irq_add() must only be called from a scope where the
> auxiliary device is guaranteed to be bound, so there can't be a concurrent
> unbind.
> 
> There may only be multiple concurrent calls to auxiliary_device_sysfs_irq_add()
> itself, and in this case irq_dir_exists can just be an atomic.
> 
> Yes, we're still stuck with an atomic for irq_dir_exists, but the driver API
> remains much simpler and less error prone.

I agree, your recommendations make sense.  And I still hate this irq
addition to auxdevices :(

thanks,

greg k-h

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

[Leon Romanovsky]

On Fri, Feb 20, 2026 at 12:59:45AM +0100, Danilo Krummrich wrote:
> On Thu Feb 19, 2026 at 10:04 PM CET, Tariq Toukan wrote:
> > +/**
> > + * auxiliary_device_sysfs_irq_dir_init - initialize the IRQ sysfs directory
> > + * @auxdev: auxiliary bus device to initialize the sysfs directory.
> > + *
> > + * This function should be called by drivers to initialize the IRQ directory
> > + * before adding any IRQ sysfs entries. The driver is responsible for ensuring
> > + * this function is called only once and for handling any concurrency control
> > + * if needed.
> > + *
> > + * Drivers must call auxiliary_device_sysfs_irq_dir_destroy() to clean up when
> > + * done.
> > + *
> > + * Return: zero on success or an error code on failure.
> > + */
> > +int auxiliary_device_sysfs_irq_dir_init(struct auxiliary_device *auxdev)
> >  {
> > -	int ret = 0;
> > -
> > -	guard(mutex)(&auxdev->sysfs.lock);
> > -	if (auxdev->sysfs.irq_dir_exists)
> > -		return 0;
> > +	int ret;
> >  
> > -	ret = devm_device_add_group(&auxdev->dev, &auxiliary_irqs_group);
> > +	ret = sysfs_create_group(&auxdev->dev.kobj, &auxiliary_irqs_group);
> >  	if (ret)
> >  		return ret;
> >  
> > -	auxdev->sysfs.irq_dir_exists = true;
> >  	xa_init(&auxdev->sysfs.irqs);
> >  	return 0;
> >  }
> > +EXPORT_SYMBOL_GPL(auxiliary_device_sysfs_irq_dir_init);
> > +
> > +/**
> > + * auxiliary_device_sysfs_irq_dir_destroy - destroy the IRQ sysfs directory
> > + * @auxdev: auxiliary bus device to destroy the sysfs directory.
> > + *
> > + * This function should be called by drivers to clean up the IRQ directory
> > + * after all IRQ sysfs entries have been removed. The driver is responsible
> > + * for ensuring all IRQs are removed before calling this function.
> > + */
> > +void auxiliary_device_sysfs_irq_dir_destroy(struct auxiliary_device *auxdev)
> > +{
> > +	xa_destroy(&auxdev->sysfs.irqs);
> > +	sysfs_remove_group(&auxdev->dev.kobj, &auxiliary_irqs_group);
> > +}
> > +EXPORT_SYMBOL_GPL(auxiliary_device_sysfs_irq_dir_destroy);
> >  
> >  /**
> >   * auxiliary_device_sysfs_irq_add - add a sysfs entry for the given IRQ
> > @@ -45,7 +70,8 @@ static int auxiliary_irq_dir_prepare(struct auxiliary_device *auxdev)
> >   * @irq: The associated interrupt number.
> >   *
> >   * This function should be called after auxiliary device have successfully
> > - * received the irq.
> > + * received the irq. The driver must call auxiliary_device_sysfs_irq_dir_init()
> > + * before calling this function for the first time.
> 
> I'm not convinced by this approach. This adds two new sources of bugs for
> drivers.
> 
>   1. Drivers can now forget to call auxiliary_device_sysfs_irq_dir_init()
>      *before* auxiliary_device_sysfs_irq_add().
> 
>   2. Drivers can forget to call auxiliary_device_sysfs_irq_dir_destroy().

This init->add->remove->destroy pattern follows standard Linux kernel practice.
I expect all current review tools to flag any missing function call
among these three.

Drivers already call auxiliary_device_sysfs_irq_add() and
auxiliary_device_sysfs_irq_remove(). It seems unlikely that they would
intentionally omit half of the required callbacks.

<...>

> There may only be multiple concurrent calls to auxiliary_device_sysfs_irq_add()
> itself, and in this case irq_dir_exists can just be an atomic.
> 
> Yes, we're still stuck with an atomic for irq_dir_exists, but the driver API
> remains much simpler and less error prone.

It is not, atomic is not a replacement for locking and this hunk is
going to be racy as hell:

   25 static int auxiliary_irq_dir_prepare(struct auxiliary_device *auxdev)
   26 {
   ...
   30         if (auxdev->sysfs.irq_dir_exists)
   31                 return 0;
   32
   33         ret = devm_device_add_group(&auxdev->dev, &auxiliary_irqs_group);
   34         if (ret)
   35                 return ret;
   36
   37         auxdev->sysfs.irq_dir_exists = true;

In the proposed patch, locking is handled by the driver, which understands the
flow far better than the driver core.

Thanks

> 
> - Danilo

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

[Leon Romanovsky]

On Fri, Feb 20, 2026 at 07:34:02AM +0100, Greg Kroah-Hartman wrote:
> On Fri, Feb 20, 2026 at 12:59:45AM +0100, Danilo Krummrich wrote:
> > On Thu Feb 19, 2026 at 10:04 PM CET, Tariq Toukan wrote:
> > > +/**
> > > + * auxiliary_device_sysfs_irq_dir_init - initialize the IRQ sysfs directory
> > > + * @auxdev: auxiliary bus device to initialize the sysfs directory.
> > > + *
> > > + * This function should be called by drivers to initialize the IRQ directory
> > > + * before adding any IRQ sysfs entries. The driver is responsible for ensuring
> > > + * this function is called only once and for handling any concurrency control
> > > + * if needed.
> > > + *
> > > + * Drivers must call auxiliary_device_sysfs_irq_dir_destroy() to clean up when
> > > + * done.
> > > + *
> > > + * Return: zero on success or an error code on failure.
> > > + */
> > > +int auxiliary_device_sysfs_irq_dir_init(struct auxiliary_device *auxdev)
> > >  {
> > > -	int ret = 0;
> > > -
> > > -	guard(mutex)(&auxdev->sysfs.lock);
> > > -	if (auxdev->sysfs.irq_dir_exists)
> > > -		return 0;
> > > +	int ret;
> > >  
> > > -	ret = devm_device_add_group(&auxdev->dev, &auxiliary_irqs_group);
> > > +	ret = sysfs_create_group(&auxdev->dev.kobj, &auxiliary_irqs_group);
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	auxdev->sysfs.irq_dir_exists = true;
> > >  	xa_init(&auxdev->sysfs.irqs);
> > >  	return 0;
> > >  }
> > > +EXPORT_SYMBOL_GPL(auxiliary_device_sysfs_irq_dir_init);
> > > +
> > > +/**
> > > + * auxiliary_device_sysfs_irq_dir_destroy - destroy the IRQ sysfs directory
> > > + * @auxdev: auxiliary bus device to destroy the sysfs directory.
> > > + *
> > > + * This function should be called by drivers to clean up the IRQ directory
> > > + * after all IRQ sysfs entries have been removed. The driver is responsible
> > > + * for ensuring all IRQs are removed before calling this function.
> > > + */
> > > +void auxiliary_device_sysfs_irq_dir_destroy(struct auxiliary_device *auxdev)
> > > +{
> > > +	xa_destroy(&auxdev->sysfs.irqs);
> > > +	sysfs_remove_group(&auxdev->dev.kobj, &auxiliary_irqs_group);
> > > +}
> > > +EXPORT_SYMBOL_GPL(auxiliary_device_sysfs_irq_dir_destroy);
> > >  
> > >  /**
> > >   * auxiliary_device_sysfs_irq_add - add a sysfs entry for the given IRQ
> > > @@ -45,7 +70,8 @@ static int auxiliary_irq_dir_prepare(struct auxiliary_device *auxdev)
> > >   * @irq: The associated interrupt number.
> > >   *
> > >   * This function should be called after auxiliary device have successfully
> > > - * received the irq.
> > > + * received the irq. The driver must call auxiliary_device_sysfs_irq_dir_init()
> > > + * before calling this function for the first time.
> > 
> > I'm not convinced by this approach. This adds two new sources of bugs for
> > drivers.
> > 
> >   1. Drivers can now forget to call auxiliary_device_sysfs_irq_dir_init()
> >      *before* auxiliary_device_sysfs_irq_add().
> > 
> >   2. Drivers can forget to call auxiliary_device_sysfs_irq_dir_destroy().
> > 
> > Instead, I suggest to keep the current approach and just replace
> > devm_device_add_group() with devm_auxiliary_device_add_group(), which in its
> > devres callback additionally clears auxdev->sysfs.irq_dir_exists.
> > 
> > In terms of the auxdev->sysfs.lock, I think this can still be removed, as it
> > wasn't needed in the first place.
> > 
> > auxiliary_device_sysfs_irq_add() must only be called from a scope where the
> > auxiliary device is guaranteed to be bound, so there can't be a concurrent
> > unbind.
> > 
> > There may only be multiple concurrent calls to auxiliary_device_sysfs_irq_add()
> > itself, and in this case irq_dir_exists can just be an atomic.
> > 
> > Yes, we're still stuck with an atomic for irq_dir_exists, but the driver API
> > remains much simpler and less error prone.
> 
> I agree, your recommendations make sense.

Unfortunately no, https://lore.kernel.org/all/20260220080413.GB10607@unreal/.

Just to summarize:
1. atomic is not a replacement for locking.
2. drivers already handle this irq thing.
3. init->add->remove->destroy is standard Linux kernel coding pattern.

Thanks

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

[Danilo Krummrich]

On Fri Feb 20, 2026 at 9:04 AM CET, Leon Romanovsky wrote:
> This init->add->remove->destroy pattern follows standard Linux kernel practice.
> I expect all current review tools to flag any missing function call
> among these three.

I'm not saying that the flow is not logical, goes against existing patterns,
etc., I'm saying that it is unnecessary to expose a new API to drivers, since
this is already handled internally.

I.e. we can easily fix the bug without increasing the API surface exposing a new
API to drivers.

> It is not, atomic is not a replacement for locking and this hunk is
> going to be racy as hell:

No, of course not, but it is sufficient to ensure that something runs only once.

However, you are still right, since sysfs_create_group() can still fail, we
still need the mutex, because we may need to unwind.

> In the proposed patch, locking is handled by the driver, which understands the
> flow far better than the driver core.

I don't think so, the driver core (or actually the auxiliary bus code) is
perfectly aware of the flow, i.e. create the attribute group once actually
needed by auxiliary_device_sysfs_irq_add(), remove it on driver unbind.

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

[Leon Romanovsky]

On Fri, Feb 20, 2026 at 12:14:12PM +0100, Danilo Krummrich wrote:
> On Fri Feb 20, 2026 at 9:04 AM CET, Leon Romanovsky wrote:
> > This init->add->remove->destroy pattern follows standard Linux kernel practice.
> > I expect all current review tools to flag any missing function call
> > among these three.
> 
> I'm not saying that the flow is not logical, goes against existing patterns,
> etc., I'm saying that it is unnecessary to expose a new API to drivers, since
> this is already handled internally.
> 
> I.e. we can easily fix the bug without increasing the API surface exposing a new
> API to drivers.
> 
> > It is not, atomic is not a replacement for locking and this hunk is
> > going to be racy as hell:
> 
> No, of course not, but it is sufficient to ensure that something runs only once.

No, atomic doesn't ensure that. Atomic makes sure that write/read
variable isn't "interrupted" in the middle.

Multiple simultaneous calls to auxiliary_irq_dir_prepare() without lock can return
that sysfs.irq_dir_exists isn't set yet, will try to call to devm_device_add_group()
which will fail.

> 
> However, you are still right, since sysfs_create_group() can still fail, we
> still need the mutex, because we may need to unwind.

If you decide to keep lock, you won't need atomic_t for irq_dir_exists.

Thanks