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

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

[Mark Bloch]

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.

Remove flag auxdev->sysfs.irq_dir_exists, add an API for updating
managed attributes group and use it to create the IRQs attribute group
as it does not fail if the group exists. Move initialization of the
sysfs xarray to auxiliary device probe.

Fixes: a808878308a8 ("driver core: auxiliary bus: show auxiliary device IRQs")
Signed-off-by: Amir Tzin <amirtz@nvidia.com>
Reviewed-by: Moshe Shemesh <moshe@nvidia.com>
Signed-off-by: Mark Bloch <mbloch@nvidia.com>
---
 drivers/base/auxiliary.c       | 20 +++++++++--
 drivers/base/auxiliary_sysfs.c | 13 +------
 drivers/base/core.c            | 65 +++++++++++++++++++++++++++-------
 include/linux/auxiliary_bus.h  |  2 --
 include/linux/device.h         |  2 ++
 5 files changed, 73 insertions(+), 29 deletions(-)

diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c
index afa4df4c5a3f..56a487fce053 100644
--- a/drivers/base/auxiliary.c
+++ b/drivers/base/auxiliary.c
@@ -201,6 +201,18 @@ static const struct dev_pm_ops auxiliary_dev_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(pm_generic_suspend, pm_generic_resume)
 };
 
+static void auxiliary_bus_sysfs_probe(struct auxiliary_device *auxdev)
+{
+	mutex_init(&auxdev->sysfs.lock);
+	xa_init(&auxdev->sysfs.irqs);
+}
+
+static void auxiliary_bus_sysfs_remove(struct auxiliary_device *auxdev)
+{
+	xa_destroy(&auxdev->sysfs.irqs);
+	mutex_destroy(&auxdev->sysfs.lock);
+}
+
 static int auxiliary_bus_probe(struct device *dev)
 {
 	const struct auxiliary_driver *auxdrv = to_auxiliary_drv(dev->driver);
@@ -213,10 +225,12 @@ static int auxiliary_bus_probe(struct device *dev)
 		return ret;
 	}
 
+	auxiliary_bus_sysfs_probe(auxdev);
 	ret = auxdrv->probe(auxdev, auxiliary_match_id(auxdrv->id_table, auxdev));
-	if (ret)
+	if (ret) {
+		auxiliary_bus_sysfs_remove(auxdev);
 		dev_pm_domain_detach(dev, true);
-
+	}
 	return ret;
 }
 
@@ -227,6 +241,7 @@ static void auxiliary_bus_remove(struct device *dev)
 
 	if (auxdrv->remove)
 		auxdrv->remove(auxdev);
+	auxiliary_bus_sysfs_remove(auxdev);
 	dev_pm_domain_detach(dev, true);
 }
 
@@ -287,7 +302,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..fa0eb4009f4d 100644
--- a/drivers/base/auxiliary_sysfs.c
+++ b/drivers/base/auxiliary_sysfs.c
@@ -24,19 +24,8 @@ static const struct attribute_group auxiliary_irqs_group = {
 
 static int auxiliary_irq_dir_prepare(struct auxiliary_device *auxdev)
 {
-	int ret = 0;
-
 	guard(mutex)(&auxdev->sysfs.lock);
-	if (auxdev->sysfs.irq_dir_exists)
-		return 0;
-
-	ret = devm_device_add_group(&auxdev->dev, &auxiliary_irqs_group);
-	if (ret)
-		return ret;
-
-	auxdev->sysfs.irq_dir_exists = true;
-	xa_init(&auxdev->sysfs.irqs);
-	return 0;
+	return devm_device_update_group(&auxdev->dev, &auxiliary_irqs_group);
 }
 
 /**
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 2fde698430df..5cc89528ffd2 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2835,17 +2835,8 @@ static void devm_attr_group_remove(struct device *dev, void *res)
 	sysfs_remove_group(&dev->kobj, group);
 }
 
-/**
- * devm_device_add_group - given a device, create a managed attribute group
- * @dev:	The device to create the group for
- * @grp:	The attribute group to create
- *
- * This function creates a group for the first time.  It will explicitly
- * warn and error if any of the attribute files being created already exist.
- *
- * Returns 0 on success or error code on failure.
- */
-int devm_device_add_group(struct device *dev, const struct attribute_group *grp)
+static int __devm_device_add_group(struct device *dev, const struct attribute_group *grp,
+				   bool sysfs_update)
 {
 	union device_attr_group_devres *devres;
 	int error;
@@ -2855,7 +2846,8 @@ int devm_device_add_group(struct device *dev, const struct attribute_group *grp)
 	if (!devres)
 		return -ENOMEM;
 
-	error = sysfs_create_group(&dev->kobj, grp);
+	error = sysfs_update ? sysfs_update_group(&dev->kobj, grp) :
+			       sysfs_create_group(&dev->kobj, grp);
 	if (error) {
 		devres_free(devres);
 		return error;
@@ -2865,8 +2857,57 @@ int devm_device_add_group(struct device *dev, const struct attribute_group *grp)
 	devres_add(dev, devres);
 	return 0;
 }
+
+/**
+ * devm_device_add_group - given a device, create a managed attribute group
+ * @dev:	The device to create the group for
+ * @grp:	The attribute group to create
+ *
+ * This function creates a group for the first time.  It will explicitly
+ * warn and error if any of the attribute files being created already exist.
+ *
+ * Returns 0 on success or error code on failure.
+ */
+int devm_device_add_group(struct device *dev, const struct attribute_group *grp)
+{
+	return __devm_device_add_group(dev, grp, false);
+}
 EXPORT_SYMBOL_GPL(devm_device_add_group);
 
+static int devm_device_group_match(struct device *dev, void *res, void *grp)
+{
+	union device_attr_group_devres *devres = res;
+
+	return devres->group == grp;
+}
+
+/**
+ * devm_device_update_group - given a device, update managed attribute group
+ * @dev:	The device to update the group for
+ * @grp:	The attribute group to update
+ *
+ * This function updates a managed attribute group, create it if it does not
+ * exist and converts an unmanaged attributes group into a managed attributes
+ * group. Unlike devm_device_add_group it will explicitly not warn or error if
+ * any of the attribute files being created already exist. Furthermore, if the
+ * visibility of the files has changed through the is_visible() callback, it
+ * will update the permissions and add or remove the relevant files. Changing a
+ * group's name (subdirectory name under kobj's directory in sysfs) is not
+ * allowed.
+ *
+ * Returns 0 on success or error code on failure.
+ */
+int devm_device_update_group(struct device *dev, const struct attribute_group *grp)
+{
+	union device_attr_group_devres *devres;
+
+	devres = devres_find(dev, devm_attr_group_remove, devm_device_group_match, (void *)grp);
+
+	return devres ? sysfs_update_group(&dev->kobj, grp) :
+			__devm_device_add_group(dev, grp, true);
+}
+EXPORT_SYMBOL_GPL(devm_device_update_group);
+
 static int device_add_attrs(struct device *dev)
 {
 	const struct class *class = dev->class;
diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
index 65dd7f154374..d8684cbff54e 100644
--- a/include/linux/auxiliary_bus.h
+++ b/include/linux/auxiliary_bus.h
@@ -146,7 +146,6 @@ struct auxiliary_device {
 	struct {
 		struct xarray irqs;
 		struct mutex lock; /* Synchronize irq sysfs creation */
-		bool irq_dir_exists;
 	} sysfs;
 };
 
@@ -238,7 +237,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);
 }
 
diff --git a/include/linux/device.h b/include/linux/device.h
index 80a5b3268986..faec7a3fab68 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1273,6 +1273,8 @@ static inline void device_remove_group(struct device *dev,
 
 int __must_check devm_device_add_group(struct device *dev,
 				       const struct attribute_group *grp);
+int __must_check devm_device_update_group(struct device *dev,
+					  const struct attribute_group *grp);
 
 /*
  * get_device - atomically increment the reference count for the device.
-- 
2.34.1

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

[Greg Kroah-Hartman]

On Mon, Mar 17, 2025 at 12:32:54PM +0200, Mark Bloch 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.
> 
> Remove flag auxdev->sysfs.irq_dir_exists, add an API for updating
> managed attributes group and use it to create the IRQs attribute group
> as it does not fail if the group exists. Move initialization of the
> sysfs xarray to auxiliary device probe.

This feels like a lot of different things all tied up into one patch,
why isn't this a series?

> Fixes: a808878308a8 ("driver core: auxiliary bus: show auxiliary device IRQs")
> Signed-off-by: Amir Tzin <amirtz@nvidia.com>
> Reviewed-by: Moshe Shemesh <moshe@nvidia.com>
> Signed-off-by: Mark Bloch <mbloch@nvidia.com>

Why the [net] on the subject line, this is not for the networking
tree...

> ---
>  drivers/base/auxiliary.c       | 20 +++++++++--
>  drivers/base/auxiliary_sysfs.c | 13 +------
>  drivers/base/core.c            | 65 +++++++++++++++++++++++++++-------
>  include/linux/auxiliary_bus.h  |  2 --
>  include/linux/device.h         |  2 ++
>  5 files changed, 73 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c
> index afa4df4c5a3f..56a487fce053 100644
> --- a/drivers/base/auxiliary.c
> +++ b/drivers/base/auxiliary.c
> @@ -201,6 +201,18 @@ static const struct dev_pm_ops auxiliary_dev_pm_ops = {
>  	SET_SYSTEM_SLEEP_PM_OPS(pm_generic_suspend, pm_generic_resume)
>  };
>  
> +static void auxiliary_bus_sysfs_probe(struct auxiliary_device *auxdev)
> +{
> +	mutex_init(&auxdev->sysfs.lock);
> +	xa_init(&auxdev->sysfs.irqs);

You aren't adding anything to sysfs here, so why is this called
auxiliary_bus_sysfs_probe()?  Naming is hard, I know :(

> +}
> +
> +static void auxiliary_bus_sysfs_remove(struct auxiliary_device *auxdev)
> +{
> +	xa_destroy(&auxdev->sysfs.irqs);
> +	mutex_destroy(&auxdev->sysfs.lock);

Same here, you aren't removing anything from sysfs.

> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2835,17 +2835,8 @@ static void devm_attr_group_remove(struct device *dev, void *res)
>  	sysfs_remove_group(&dev->kobj, group);
>  }
>  
> -/**
> - * devm_device_add_group - given a device, create a managed attribute group
> - * @dev:	The device to create the group for
> - * @grp:	The attribute group to create
> - *
> - * This function creates a group for the first time.  It will explicitly
> - * warn and error if any of the attribute files being created already exist.
> - *
> - * Returns 0 on success or error code on failure.
> - */
> -int devm_device_add_group(struct device *dev, const struct attribute_group *grp)
> +static int __devm_device_add_group(struct device *dev, const struct attribute_group *grp,
> +				   bool sysfs_update)
>  {
>  	union device_attr_group_devres *devres;
>  	int error;
> @@ -2855,7 +2846,8 @@ int devm_device_add_group(struct device *dev, const struct attribute_group *grp)
>  	if (!devres)
>  		return -ENOMEM;
>  
> -	error = sysfs_create_group(&dev->kobj, grp);
> +	error = sysfs_update ? sysfs_update_group(&dev->kobj, grp) :
> +			       sysfs_create_group(&dev->kobj, grp);

Add is really an update?  That feels broken.

>  	if (error) {
>  		devres_free(devres);
>  		return error;
> @@ -2865,8 +2857,57 @@ int devm_device_add_group(struct device *dev, const struct attribute_group *grp)
>  	devres_add(dev, devres);
>  	return 0;
>  }
> +
> +/**
> + * devm_device_add_group - given a device, create a managed attribute group
> + * @dev:	The device to create the group for
> + * @grp:	The attribute group to create
> + *
> + * This function creates a group for the first time.  It will explicitly
> + * warn and error if any of the attribute files being created already exist.
> + *
> + * Returns 0 on success or error code on failure.
> + */
> +int devm_device_add_group(struct device *dev, const struct attribute_group *grp)
> +{
> +	return __devm_device_add_group(dev, grp, false);
> +}
>  EXPORT_SYMBOL_GPL(devm_device_add_group);
>  
> +static int devm_device_group_match(struct device *dev, void *res, void *grp)
> +{
> +	union device_attr_group_devres *devres = res;
> +
> +	return devres->group == grp;
> +}
> +
> +/**
> + * devm_device_update_group - given a device, update managed attribute group
> + * @dev:	The device to update the group for
> + * @grp:	The attribute group to update
> + *
> + * This function updates a managed attribute group, create it if it does not
> + * exist and converts an unmanaged attributes group into a managed attributes
> + * group. Unlike devm_device_add_group it will explicitly not warn or error if
> + * any of the attribute files being created already exist. Furthermore, if the
> + * visibility of the files has changed through the is_visible() callback, it
> + * will update the permissions and add or remove the relevant files. Changing a
> + * group's name (subdirectory name under kobj's directory in sysfs) is not
> + * allowed.
> + *
> + * Returns 0 on success or error code on failure.
> + */
> +int devm_device_update_group(struct device *dev, const struct attribute_group *grp)
> +{
> +	union device_attr_group_devres *devres;
> +
> +	devres = devres_find(dev, devm_attr_group_remove, devm_device_group_match, (void *)grp);
> +
> +	return devres ? sysfs_update_group(&dev->kobj, grp) :
> +			__devm_device_add_group(dev, grp, true);
> +}
> +EXPORT_SYMBOL_GPL(devm_device_update_group);

Who is now using this new function?  I don't see it here in this patch,
so why is it included here?

> +
>  static int device_add_attrs(struct device *dev)
>  {
>  	const struct class *class = dev->class;
> diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
> index 65dd7f154374..d8684cbff54e 100644
> --- a/include/linux/auxiliary_bus.h
> +++ b/include/linux/auxiliary_bus.h
> @@ -146,7 +146,6 @@ struct auxiliary_device {
>  	struct {
>  		struct xarray irqs;
>  		struct mutex lock; /* Synchronize irq sysfs creation */
> -		bool irq_dir_exists;
>  	} sysfs;
>  };
>  
> @@ -238,7 +237,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);
>  }
>  
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 80a5b3268986..faec7a3fab68 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1273,6 +1273,8 @@ static inline void device_remove_group(struct device *dev,
>  
>  int __must_check devm_device_add_group(struct device *dev,
>  				       const struct attribute_group *grp);
> +int __must_check devm_device_update_group(struct device *dev,
> +					  const struct attribute_group *grp);

Oh no, please no.  I hate the devm_device_add_group() to start with (and
still think it is wrong and will break people's real use cases), I don't
want to mess with a update group thing as well.

Please fix this up and make this a patch series to make it more obvious
why all of this is needed, and that the change really is fixing a real
problem.  As it is, I can't take this, sorry.

greg k-h

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

[Mark Bloch]

On 17/03/2025 15:16, Greg Kroah-Hartman wrote:
> On Mon, Mar 17, 2025 at 12:32:54PM +0200, Mark Bloch 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.
>>
>> Remove flag auxdev->sysfs.irq_dir_exists, add an API for updating
>> managed attributes group and use it to create the IRQs attribute group
>> as it does not fail if the group exists. Move initialization of the
>> sysfs xarray to auxiliary device probe.
> 
> This feels like a lot of different things all tied up into one patch,
> why isn't this a series?

Will make it one.

> 
>> Fixes: a808878308a8 ("driver core: auxiliary bus: show auxiliary device IRQs")
>> Signed-off-by: Amir Tzin <amirtz@nvidia.com>
>> Reviewed-by: Moshe Shemesh <moshe@nvidia.com>
>> Signed-off-by: Mark Bloch <mbloch@nvidia.com>
> 
> Why the [net] on the subject line, this is not for the networking
> tree...

We tested it internally over net tree as it affects SFs bind/unbind
on mlx5 driver and because of that my scripts got it wrong, sorry
for the noise.

> 
>> ---
>>  drivers/base/auxiliary.c       | 20 +++++++++--
>>  drivers/base/auxiliary_sysfs.c | 13 +------
>>  drivers/base/core.c            | 65 +++++++++++++++++++++++++++-------
>>  include/linux/auxiliary_bus.h  |  2 --
>>  include/linux/device.h         |  2 ++
>>  5 files changed, 73 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c
>> index afa4df4c5a3f..56a487fce053 100644
>> --- a/drivers/base/auxiliary.c
>> +++ b/drivers/base/auxiliary.c
>> @@ -201,6 +201,18 @@ static const struct dev_pm_ops auxiliary_dev_pm_ops = {
>>  	SET_SYSTEM_SLEEP_PM_OPS(pm_generic_suspend, pm_generic_resume)
>>  };
>>  
>> +static void auxiliary_bus_sysfs_probe(struct auxiliary_device *auxdev)
>> +{
>> +	mutex_init(&auxdev->sysfs.lock);
>> +	xa_init(&auxdev->sysfs.irqs);
> 
> You aren't adding anything to sysfs here, so why is this called
> auxiliary_bus_sysfs_probe()?  Naming is hard, I know :(
> 
>> +}
>> +
>> +static void auxiliary_bus_sysfs_remove(struct auxiliary_device *auxdev)
>> +{
>> +	xa_destroy(&auxdev->sysfs.irqs);
>> +	mutex_destroy(&auxdev->sysfs.lock);
> 
> Same here, you aren't removing anything from sysfs.
> 
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -2835,17 +2835,8 @@ static void devm_attr_group_remove(struct device *dev, void *res)
>>  	sysfs_remove_group(&dev->kobj, group);
>>  }
>>  
>> -/**
>> - * devm_device_add_group - given a device, create a managed attribute group
>> - * @dev:	The device to create the group for
>> - * @grp:	The attribute group to create
>> - *
>> - * This function creates a group for the first time.  It will explicitly
>> - * warn and error if any of the attribute files being created already exist.
>> - *
>> - * Returns 0 on success or error code on failure.
>> - */
>> -int devm_device_add_group(struct device *dev, const struct attribute_group *grp)
>> +static int __devm_device_add_group(struct device *dev, const struct attribute_group *grp,
>> +				   bool sysfs_update)
>>  {
>>  	union device_attr_group_devres *devres;
>>  	int error;
>> @@ -2855,7 +2846,8 @@ int devm_device_add_group(struct device *dev, const struct attribute_group *grp)
>>  	if (!devres)
>>  		return -ENOMEM;
>>  
>> -	error = sysfs_create_group(&dev->kobj, grp);
>> +	error = sysfs_update ? sysfs_update_group(&dev->kobj, grp) :
>> +			       sysfs_create_group(&dev->kobj, grp);
> 
> Add is really an update?  That feels broken.
> 
>>  	if (error) {
>>  		devres_free(devres);
>>  		return error;
>> @@ -2865,8 +2857,57 @@ int devm_device_add_group(struct device *dev, const struct attribute_group *grp)
>>  	devres_add(dev, devres);
>>  	return 0;
>>  }
>> +
>> +/**
>> + * devm_device_add_group - given a device, create a managed attribute group
>> + * @dev:	The device to create the group for
>> + * @grp:	The attribute group to create
>> + *
>> + * This function creates a group for the first time.  It will explicitly
>> + * warn and error if any of the attribute files being created already exist.
>> + *
>> + * Returns 0 on success or error code on failure.
>> + */
>> +int devm_device_add_group(struct device *dev, const struct attribute_group *grp)
>> +{
>> +	return __devm_device_add_group(dev, grp, false);
>> +}
>>  EXPORT_SYMBOL_GPL(devm_device_add_group);
>>  
>> +static int devm_device_group_match(struct device *dev, void *res, void *grp)
>> +{
>> +	union device_attr_group_devres *devres = res;
>> +
>> +	return devres->group == grp;
>> +}
>> +
>> +/**
>> + * devm_device_update_group - given a device, update managed attribute group
>> + * @dev:	The device to update the group for
>> + * @grp:	The attribute group to update
>> + *
>> + * This function updates a managed attribute group, create it if it does not
>> + * exist and converts an unmanaged attributes group into a managed attributes
>> + * group. Unlike devm_device_add_group it will explicitly not warn or error if
>> + * any of the attribute files being created already exist. Furthermore, if the
>> + * visibility of the files has changed through the is_visible() callback, it
>> + * will update the permissions and add or remove the relevant files. Changing a
>> + * group's name (subdirectory name under kobj's directory in sysfs) is not
>> + * allowed.
>> + *
>> + * Returns 0 on success or error code on failure.
>> + */
>> +int devm_device_update_group(struct device *dev, const struct attribute_group *grp)
>> +{
>> +	union device_attr_group_devres *devres;
>> +
>> +	devres = devres_find(dev, devm_attr_group_remove, devm_device_group_match, (void *)grp);
>> +
>> +	return devres ? sysfs_update_group(&dev->kobj, grp) :
>> +			__devm_device_add_group(dev, grp, true);
>> +}
>> +EXPORT_SYMBOL_GPL(devm_device_update_group);
> 
> Who is now using this new function?  I don't see it here in this patch,
> so why is it included here?
> 
>> +
>>  static int device_add_attrs(struct device *dev)
>>  {
>>  	const struct class *class = dev->class;
>> diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
>> index 65dd7f154374..d8684cbff54e 100644
>> --- a/include/linux/auxiliary_bus.h
>> +++ b/include/linux/auxiliary_bus.h
>> @@ -146,7 +146,6 @@ struct auxiliary_device {
>>  	struct {
>>  		struct xarray irqs;
>>  		struct mutex lock; /* Synchronize irq sysfs creation */
>> -		bool irq_dir_exists;
>>  	} sysfs;
>>  };
>>  
>> @@ -238,7 +237,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);
>>  }
>>  
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 80a5b3268986..faec7a3fab68 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -1273,6 +1273,8 @@ static inline void device_remove_group(struct device *dev,
>>  
>>  int __must_check devm_device_add_group(struct device *dev,
>>  				       const struct attribute_group *grp);
>> +int __must_check devm_device_update_group(struct device *dev,
>> +					  const struct attribute_group *grp);
> 
> Oh no, please no.  I hate the devm_device_add_group() to start with (and
> still think it is wrong and will break people's real use cases), I don't
> want to mess with a update group thing as well.
> 
> Please fix this up and make this a patch series to make it more obvious
> why all of this is needed, and that the change really is fixing a real
> problem.  As it is, I can't take this, sorry.

ACK to all comments, I'll take it with the author and will make it a
proper series.

Mark

> 
> greg k-h

[PATCH net] 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].

Using the attributes group visibility interface, expose the IRQs
attributes group if"f the xarray storing IRQs entries is not empty. Now
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;
 }
 
 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,
+	.name = "DUMMY",
+};
+
 static struct attribute *auxiliary_irq_attrs[] = {
-	NULL
+	[0] = &auxiliary_irq_attr,
+	[1] = NULL,
 };
 
+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);
+}
 
-	ret = devm_device_add_group(&auxdev->dev, &auxiliary_irqs_group);
-	if (ret)
-		return ret;
+int auxiliary_bus_irq_dir_res_probe(struct auxiliary_device *auxdev)
+{
+	struct device *dev = &auxdev->dev;
 
-	auxdev->sysfs.irq_dir_exists = true;
+	mutex_init(&auxdev->sysfs.lock);
 	xa_init(&auxdev->sysfs.irqs);
-	return 0;
+	return sysfs_create_group(&dev->kobj, &auxiliary_irqs_group);
+}
+
+static struct auxiliary_irq_info *auxiliary_irq_info_init(int irq)
+{
+	struct auxiliary_irq_info *info;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return NULL;
+
+	sysfs_attr_init(&info->sysfs_attr.attr);
+	snprintf(info->name, AUXILIARY_MAX_IRQ_NAME, "%d", irq);
+	info->sysfs_attr.attr.name = info->name;
+
+	return info;
+}
+
+static void auxiliary_irq_info_destroy(struct auxiliary_irq_info *info)
+{
+	kfree(info);
 }
 
 /**
@@ -55,36 +96,41 @@ static int auxiliary_irq_dir_prepare(struct auxiliary_device *auxdev)
  */
 int auxiliary_device_sysfs_irq_add(struct auxiliary_device *auxdev, int irq)
 {
-	struct auxiliary_irq_info *info __free(kfree) = NULL;
 	struct device *dev = &auxdev->dev;
+	struct auxiliary_irq_info *info;
+	bool sysfs_add_error = false;
 	int ret;
 
-	ret = auxiliary_irq_dir_prepare(auxdev);
-	if (ret)
-		return ret;
-
-	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	info = auxiliary_irq_info_init(irq);
 	if (!info)
 		return -ENOMEM;
 
-	sysfs_attr_init(&info->sysfs_attr.attr);
-	snprintf(info->name, AUXILIARY_MAX_IRQ_NAME, "%d", irq);
-
+	mutex_lock(&auxdev->sysfs.lock);
 	ret = xa_insert(&auxdev->sysfs.irqs, irq, info, GFP_KERNEL);
 	if (ret)
-		return ret;
+		goto unlock;
+
+	ret = sysfs_update_group(&dev->kobj, &auxiliary_irqs_group);
+	if (ret)
+		goto irq_erase;
 
-	info->sysfs_attr.attr.name = info->name;
 	ret = sysfs_add_file_to_group(&dev->kobj, &info->sysfs_attr.attr,
 				      auxiliary_irqs_group.name);
-	if (ret)
-		goto sysfs_add_err;
+	if (ret) {
+		sysfs_add_error = true;
+		goto irq_erase;
+	}
 
-	xa_store(&auxdev->sysfs.irqs, irq, no_free_ptr(info), GFP_KERNEL);
+	mutex_unlock(&auxdev->sysfs.lock);
 	return 0;
 
-sysfs_add_err:
+irq_erase:
 	xa_erase(&auxdev->sysfs.irqs, irq);
+	if (sysfs_add_error)
+		sysfs_update_group(&dev->kobj, &auxiliary_irqs_group);
+unlock:
+	mutex_unlock(&auxdev->sysfs.lock);
+	auxiliary_irq_info_destroy(info);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(auxiliary_device_sysfs_irq_add);
@@ -97,17 +143,30 @@ EXPORT_SYMBOL_GPL(auxiliary_device_sysfs_irq_add);
  * This function should be called to remove an IRQ sysfs entry.
  * The driver must invoke this API when IRQ is released by the device.
  */
-void auxiliary_device_sysfs_irq_remove(struct auxiliary_device *auxdev, int irq)
+int auxiliary_device_sysfs_irq_remove(struct auxiliary_device *auxdev, int irq)
 {
-	struct auxiliary_irq_info *info __free(kfree) = xa_load(&auxdev->sysfs.irqs, irq);
 	struct device *dev = &auxdev->dev;
+	struct auxiliary_irq_info *info;
+	int err;
 
+	mutex_lock(&auxdev->sysfs.lock);
+	info = xa_load(&auxdev->sysfs.irqs, irq);
 	if (!info) {
+		mutex_unlock(&auxdev->sysfs.lock);
 		dev_err(&auxdev->dev, "IRQ %d doesn't exist\n", irq);
-		return;
+		return -ENOMEM;
 	}
+
 	sysfs_remove_file_from_group(&dev->kobj, &info->sysfs_attr.attr,
 				     auxiliary_irqs_group.name);
 	xa_erase(&auxdev->sysfs.irqs, irq);
+	err = sysfs_update_group(&dev->kobj, &auxiliary_irqs_group);
+	if (err)
+		dev_err(&auxdev->dev,
+			"Failed to update IRQs group, irq %d\n", irq);
+
+	mutex_unlock(&auxdev->sysfs.lock);
+	auxiliary_irq_info_destroy(info);
+	return err;
 }
 EXPORT_SYMBOL_GPL(auxiliary_device_sysfs_irq_remove);
diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
index 4086afd0cc6b..06c247b647ab 100644
--- a/include/linux/auxiliary_bus.h
+++ b/include/linux/auxiliary_bus.h
@@ -61,7 +61,6 @@
  * @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
@@ -146,7 +145,6 @@ struct auxiliary_device {
 	struct {
 		struct xarray irqs;
 		struct mutex lock; /* Synchronize irq sysfs creation */
-		bool irq_dir_exists;
 	} sysfs;
 };
 
@@ -222,23 +220,37 @@ 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
+void auxiliary_bus_irq_dir_res_remove(struct auxiliary_device *auxdev);
+int auxiliary_bus_irq_dir_res_probe(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);
+int auxiliary_device_sysfs_irq_remove(struct auxiliary_device *auxdev, int irq);
 #else /* CONFIG_SYSFS */
+static inline void
+auxiliary_bus_irq_dir_res_remove(struct auxiliary_device *auxdev)
+{
+}
+
+static inline int
+auxiliary_bus_irq_dir_res_probe(struct auxiliary_device *auxdev)
+{
+	return 0;
+}
+
 static inline int
 auxiliary_device_sysfs_irq_add(struct auxiliary_device *auxdev, int irq)
 {
 	return 0;
 }
 
-static inline void
-auxiliary_device_sysfs_irq_remove(struct auxiliary_device *auxdev, int irq) {}
+static inline int
+auxiliary_device_sysfs_irq_remove(struct auxiliary_device *auxdev, int irq)
+{
+	return 0;
+}
 #endif
 
 static inline void auxiliary_device_uninit(struct auxiliary_device *auxdev)
 {
-	mutex_destroy(&auxdev->sysfs.lock);
 	put_device(&auxdev->dev);
 }
 

base-commit: c0178eec8884231a5ae0592b9fce827bccb77e86
-- 
2.31.1

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

[Greg Kroah-Hartman]

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

Please always use scripts/checkpatch.pl so that you don't get grumpy
maintainers asking you why you didn't use scripts/checkpatch.pl...

thanks!

greg k-h

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

[Greg Kroah-Hartman]

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

Why would auxbus patches go through the net tree?

confused,

greg k-h

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

[Tariq Toukan]

On 23/10/2025 9:47, Greg Kroah-Hartman wrote:
> 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
>> 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 ++++++--
> 
> Why would auxbus patches go through the net tree?
> 

It shouldn't...
Honest mistake passing the argument to my script.

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

[Tariq Toukan]

On 23/10/2025 9:46, Greg Kroah-Hartman wrote:
> 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
>> 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;
> 
> Please always use scripts/checkpatch.pl so that you don't get grumpy
> maintainers asking you why you didn't use scripts/checkpatch.pl...
> 

Sure. Always running it before submissions.
It passes for me. Anything I miss?


total: 0 errors, 0 warnings, 258 lines checked
patch has no obvious style problems and is ready for submission.

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

[Greg Kroah-Hartman]

On Thu, Oct 23, 2025 at 10:14:29AM +0300, Tariq Toukan wrote:
> 
> 
> On 23/10/2025 9:46, Greg Kroah-Hartman wrote:
> > 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
> > > 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;
> > 
> > Please always use scripts/checkpatch.pl so that you don't get grumpy
> > maintainers asking you why you didn't use scripts/checkpatch.pl...
> > 
> 
> Sure. Always running it before submissions.
> It passes for me. Anything I miss?
> 
> total: 0 errors, 0 warnings, 258 lines checked
> patch has no obvious style problems and is ready for submission.

Odd, why did it not catch that extra ' ' after the "if"?

thanks,

greg k-h

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

[Leon Romanovsky]

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