| Message ID | 20250317103254.573985-1-mbloch@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] driver core: auxiliary bus: Fix sysfs creation on bind | expand |
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
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
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.