[PATCH] hwmon: Fix WARN_ON() always called from devm_hwmon_device_unregister()

[PATCH] hwmon: Fix WARN_ON() always called from devm_hwmon_device_unregister()

[Matthew Sanders]

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.
---
 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);
-- 
2.46.0

Re: [PATCH] hwmon: Fix WARN_ON() always called from devm_hwmon_device_unregister()

[Guenter Roeck]

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

Re: [PATCH] hwmon: Fix WARN_ON() always called from devm_hwmon_device_unregister()

[PJ Waskiewicz]

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

Re: [PATCH] hwmon: Fix WARN_ON() always called from devm_hwmon_device_unregister()

[Guenter Roeck]

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