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@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?
Will make it one.
>
>> 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...
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
