On Mon, Mar 17, 2025 at 12:32:54PM +0200, Mark Bloch wrote:
> From: Amir Tzin <amirtz@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Reviewed-by: Moshe Shemesh <moshe@xxxxxxxxxx>
> Signed-off-by: Mark Bloch <mbloch@xxxxxxxxxx>
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
