| Message ID | 20240912091401.4101-1-m@ttsande.rs (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: Fix WARN_ON() always called from devm_hwmon_device_unregister() | expand |
On 9/12/24 02:14, Matthew Sanders wrote: > devm_hwmon_device_unregister() only takes parent of a devres-managed > hwmon device as an argument. This always fails, as devres can't find > the hwmon device it needs to unregister with the parent device alone. > Without this patch, the WARN_ON() in devm_hwmon_device_unregister() will > always be displayed unconditionally: > > [ 7.969746] WARNING: CPU: 1 PID: 224 at drivers/hwmon/hwmon.c:1205 devm_hwmon_device_unregister+0x28/0x30 > > This patch adds an extra argument to devm_hwmon_device_unregister(), a > pointer to a hwmon device which was previously registered to the > parent using devres. > > There aren't any drivers which currently make use of this function, so > any existing users of devm_hwmon_* shouldn't require any changes as a > result of this patch. If there are no users, there is no need to keep the function. We should drop it instead. Also, your patch is not signed and therefore can not be applied. Guenter > --- > drivers/hwmon/hwmon.c | 6 ++++-- > include/linux/hwmon.h | 2 +- > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c > index a362080d41fa..84945a276320 100644 > --- a/drivers/hwmon/hwmon.c > +++ b/drivers/hwmon/hwmon.c > @@ -1199,10 +1199,12 @@ static int devm_hwmon_match(struct device *dev, void *res, void *data) > * devm_hwmon_device_unregister - removes a previously registered hwmon device > * > * @dev: the parent device of the device to unregister > + * @hwdev: the hwmon device to unregister > */ > -void devm_hwmon_device_unregister(struct device *dev) > +void devm_hwmon_device_unregister(struct device *dev, struct device *hwdev) > { > - WARN_ON(devres_release(dev, devm_hwmon_release, devm_hwmon_match, dev)); > + WARN_ON(devres_release(dev, devm_hwmon_release, devm_hwmon_match, > + hwdev)); > } > EXPORT_SYMBOL_GPL(devm_hwmon_device_unregister); > > diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h > index e94314760aab..2434c6fc17a7 100644 > --- a/include/linux/hwmon.h > +++ b/include/linux/hwmon.h > @@ -481,7 +481,7 @@ devm_hwmon_device_register_with_info(struct device *dev, > const struct attribute_group **extra_groups); > > void hwmon_device_unregister(struct device *dev); > -void devm_hwmon_device_unregister(struct device *dev); > +void devm_hwmon_device_unregister(struct device *dev, struct device *hwdev); > > int hwmon_notify_event(struct device *dev, enum hwmon_sensor_types type, > u32 attr, int channel);
On Thu, 2024-09-12 at 07:13 -0700, Guenter Roeck wrote: > On 9/12/24 02:14, Matthew Sanders wrote: > > devm_hwmon_device_unregister() only takes parent of a devres- > > managed > > hwmon device as an argument. This always fails, as devres can't > > find > > the hwmon device it needs to unregister with the parent device > > alone. > > Without this patch, the WARN_ON() in devm_hwmon_device_unregister() > > will > > always be displayed unconditionally: > > > > [ 7.969746] WARNING: CPU: 1 PID: 224 at > > drivers/hwmon/hwmon.c:1205 devm_hwmon_device_unregister+0x28/0x30 > > > > This patch adds an extra argument to > > devm_hwmon_device_unregister(), a > > pointer to a hwmon device which was previously registered to the > > parent using devres. > > > > There aren't any drivers which currently make use of this function, > > so > > any existing users of devm_hwmon_* shouldn't require any changes as > > a > > result of this patch. > > If there are no users, there is no need to keep the function. We > should drop > it instead. There aren't any direct in-tree users of the function. But some out- of-tree drivers can find it useful to use, hence Matt hitting this issue. If there's a desire to just remove it, that's fine. But it would remove a handy hook for out-of-tree stuff. -PJ > > Also, your patch is not signed and therefore can not be applied. > > Guenter > > > --- > > drivers/hwmon/hwmon.c | 6 ++++-- > > include/linux/hwmon.h | 2 +- > > 2 files changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c > > index a362080d41fa..84945a276320 100644 > > --- a/drivers/hwmon/hwmon.c > > +++ b/drivers/hwmon/hwmon.c > > @@ -1199,10 +1199,12 @@ static int devm_hwmon_match(struct device > > *dev, void *res, void *data) > > * devm_hwmon_device_unregister - removes a previously registered > > hwmon device > > * > > * @dev: the parent device of the device to unregister > > + * @hwdev: the hwmon device to unregister > > */ > > -void devm_hwmon_device_unregister(struct device *dev) > > +void devm_hwmon_device_unregister(struct device *dev, struct > > device *hwdev) > > { > > - WARN_ON(devres_release(dev, devm_hwmon_release, > > devm_hwmon_match, dev)); > > + WARN_ON(devres_release(dev, devm_hwmon_release, > > devm_hwmon_match, > > + hwdev)); > > } > > EXPORT_SYMBOL_GPL(devm_hwmon_device_unregister); > > > > diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h > > index e94314760aab..2434c6fc17a7 100644 > > --- a/include/linux/hwmon.h > > +++ b/include/linux/hwmon.h > > @@ -481,7 +481,7 @@ devm_hwmon_device_register_with_info(struct > > device *dev, > > const struct attribute_group > > **extra_groups); > > > > void hwmon_device_unregister(struct device *dev); > > -void devm_hwmon_device_unregister(struct device *dev); > > +void devm_hwmon_device_unregister(struct device *dev, struct > > device *hwdev); > > > > int hwmon_notify_event(struct device *dev, enum > > hwmon_sensor_types type, > > u32 attr, int channel); >
On 9/12/24 23:40, PJ Waskiewicz wrote: > On Thu, 2024-09-12 at 07:13 -0700, Guenter Roeck wrote: >> On 9/12/24 02:14, Matthew Sanders wrote: >>> devm_hwmon_device_unregister() only takes parent of a devres- >>> managed >>> hwmon device as an argument. This always fails, as devres can't >>> find >>> the hwmon device it needs to unregister with the parent device >>> alone. >>> Without this patch, the WARN_ON() in devm_hwmon_device_unregister() >>> will >>> always be displayed unconditionally: >>> >>> [ 7.969746] WARNING: CPU: 1 PID: 224 at >>> drivers/hwmon/hwmon.c:1205 devm_hwmon_device_unregister+0x28/0x30 >>> >>> This patch adds an extra argument to >>> devm_hwmon_device_unregister(), a >>> pointer to a hwmon device which was previously registered to the >>> parent using devres. >>> >>> There aren't any drivers which currently make use of this function, >>> so >>> any existing users of devm_hwmon_* shouldn't require any changes as >>> a >>> result of this patch. >> >> If there are no users, there is no need to keep the function. We >> should drop >> it instead. > > There aren't any direct in-tree users of the function. But some out- > of-tree drivers can find it useful to use, hence Matt hitting this > issue. > > If there's a desire to just remove it, that's fine. But it would > remove a handy hook for out-of-tree stuff. > Any use of this function is most likely wrong. Out-of-tree code is completely irrelevant for the upstream kernel. On the contrary - it is another reason for removing this function. I'll do that myself. Guenter
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c index a362080d41fa..84945a276320 100644 --- a/drivers/hwmon/hwmon.c +++ b/drivers/hwmon/hwmon.c @@ -1199,10 +1199,12 @@ static int devm_hwmon_match(struct device *dev, void *res, void *data) * devm_hwmon_device_unregister - removes a previously registered hwmon device * * @dev: the parent device of the device to unregister + * @hwdev: the hwmon device to unregister */ -void devm_hwmon_device_unregister(struct device *dev) +void devm_hwmon_device_unregister(struct device *dev, struct device *hwdev) { - WARN_ON(devres_release(dev, devm_hwmon_release, devm_hwmon_match, dev)); + WARN_ON(devres_release(dev, devm_hwmon_release, devm_hwmon_match, + hwdev)); } EXPORT_SYMBOL_GPL(devm_hwmon_device_unregister); diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h index e94314760aab..2434c6fc17a7 100644 --- a/include/linux/hwmon.h +++ b/include/linux/hwmon.h @@ -481,7 +481,7 @@ devm_hwmon_device_register_with_info(struct device *dev, const struct attribute_group **extra_groups); void hwmon_device_unregister(struct device *dev); -void devm_hwmon_device_unregister(struct device *dev); +void devm_hwmon_device_unregister(struct device *dev, struct device *hwdev); int hwmon_notify_event(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel);