[PATCH 1/2] hwmon: core: add thermal sensors only if dev->of_node is present

[PATCH 1/2] hwmon: core: add thermal sensors only if dev->of_node is present

[Eduardo Valentin]

Drivers may register to hwmon and request for also registering
with the thermal subsystem (HWMON_C_REGISTER_TZ). However,
some of these driver, e.g. marvell phy, may be probed from
Device Tree or being dynamically allocated, and in the later
case, it will not have a dev->of_node entry.

Registering with hwmon without the dev->of_node may result in
different outcomes depending on the device tree, which may
be a bit misleading. If the device tree blob has no 'thermal-zones'
node, the *hwmon_device_register*() family functions are going
to gracefully succeed, because of-thermal,
*thermal_zone_of_sensor_register() return -ENODEV in this case,
and the hwmon error path handles this error code as success to
cover for the case where CONFIG_THERMAL_OF is not set.
However, if the device tree blob has the 'thermal-zones'
entry, the *hwmon_device_register*() will always fail on callers
with no dev->of_node, propagating -EINVAL.

If dev->of_node is not present, calling of-thermal does not
make sense. For this reason, this patch checks first if the
device has a of_node before going over the process of registering
with the thermal subsystem of-thermal interface. And in this case,
when a caller of *hwmon_device_register*() with HWMON_C_REGISTER_TZ
and no dev->of_node will still register with hwmon, but not with
the thermal subsystem. If all the hwmon part bits are in place,
the registration will succeed.

Cc: Jean Delvare <jdelvare@suse.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-hwmon@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <eduval@amazon.com>
---
 drivers/hwmon/hwmon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index fcdbac4a56e3..6b3559f58b67 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -619,7 +619,7 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
 	if (err)
 		goto free_hwmon;
 
-	if (dev && chip && chip->ops->read &&
+	if (dev && dev->of_node && chip && chip->ops->read &&
 	    chip->info[0]->type == hwmon_chip &&
 	    (chip->info[0]->config[0] & HWMON_C_REGISTER_TZ)) {
 		const struct hwmon_channel_info **info = chip->info;
-- 
2.21.0

Re: [PATCH 1/2] hwmon: core: add thermal sensors only if dev->of_node is present

[Guenter Roeck]

Hi Eduardo,

On Fri, May 17, 2019 at 04:13:36PM -0700, Eduardo Valentin wrote:
> Drivers may register to hwmon and request for also registering
> with the thermal subsystem (HWMON_C_REGISTER_TZ). However,
> some of these driver, e.g. marvell phy, may be probed from
> Device Tree or being dynamically allocated, and in the later
> case, it will not have a dev->of_node entry.
> 
> Registering with hwmon without the dev->of_node may result in
> different outcomes depending on the device tree, which may
> be a bit misleading. If the device tree blob has no 'thermal-zones'
> node, the *hwmon_device_register*() family functions are going
> to gracefully succeed, because of-thermal,
> *thermal_zone_of_sensor_register() return -ENODEV in this case,
> and the hwmon error path handles this error code as success to
> cover for the case where CONFIG_THERMAL_OF is not set.
> However, if the device tree blob has the 'thermal-zones'
> entry, the *hwmon_device_register*() will always fail on callers
> with no dev->of_node, propagating -EINVAL.
> 
> If dev->of_node is not present, calling of-thermal does not
> make sense. For this reason, this patch checks first if the
> device has a of_node before going over the process of registering
> with the thermal subsystem of-thermal interface. And in this case,
> when a caller of *hwmon_device_register*() with HWMON_C_REGISTER_TZ
> and no dev->of_node will still register with hwmon, but not with
> the thermal subsystem. If all the hwmon part bits are in place,
> the registration will succeed.
> 
Makes sense. I'd apply it as-is, but it would be better if you resend
it to the list to give others a chance to comment.

Thanks,
Guenter

> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-hwmon@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Eduardo Valentin <eduval@amazon.com>
> ---
>  drivers/hwmon/hwmon.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index fcdbac4a56e3..6b3559f58b67 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -619,7 +619,7 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
>  	if (err)
>  		goto free_hwmon;
>  
> -	if (dev && chip && chip->ops->read &&
> +	if (dev && dev->of_node && chip && chip->ops->read &&
>  	    chip->info[0]->type == hwmon_chip &&
>  	    (chip->info[0]->config[0] & HWMON_C_REGISTER_TZ)) {
>  		const struct hwmon_channel_info **info = chip->info;
> -- 
> 2.21.0
> 

Re: [PATCH 1/2] hwmon: core: add thermal sensors only if dev->of_node is present

[Eduardo Valentin]

On Tue, May 28, 2019 at 08:08:21AM -0700, Guenter Roeck wrote:
> Hi Eduardo,
> 
> On Fri, May 17, 2019 at 04:13:36PM -0700, Eduardo Valentin wrote:
> > Drivers may register to hwmon and request for also registering
> > with the thermal subsystem (HWMON_C_REGISTER_TZ). However,
> > some of these driver, e.g. marvell phy, may be probed from
> > Device Tree or being dynamically allocated, and in the later
> > case, it will not have a dev->of_node entry.
> > 
> > Registering with hwmon without the dev->of_node may result in
> > different outcomes depending on the device tree, which may
> > be a bit misleading. If the device tree blob has no 'thermal-zones'
> > node, the *hwmon_device_register*() family functions are going
> > to gracefully succeed, because of-thermal,
> > *thermal_zone_of_sensor_register() return -ENODEV in this case,
> > and the hwmon error path handles this error code as success to
> > cover for the case where CONFIG_THERMAL_OF is not set.
> > However, if the device tree blob has the 'thermal-zones'
> > entry, the *hwmon_device_register*() will always fail on callers
> > with no dev->of_node, propagating -EINVAL.
> > 
> > If dev->of_node is not present, calling of-thermal does not
> > make sense. For this reason, this patch checks first if the
> > device has a of_node before going over the process of registering
> > with the thermal subsystem of-thermal interface. And in this case,
> > when a caller of *hwmon_device_register*() with HWMON_C_REGISTER_TZ
> > and no dev->of_node will still register with hwmon, but not with
> > the thermal subsystem. If all the hwmon part bits are in place,
> > the registration will succeed.
> > 
> Makes sense. I'd apply it as-is, but it would be better if you resend
> it to the list to give others a chance to comment.

Ok Cool.

Yeah, the patches were copied to the mailing list. Only the cover letter
somehow I forgot to copy the mailing lists while git-sending-email.

I will resend the two patches (and the cover letter) after fixing
the comment on patch 2/2.

> 
> Thanks,
> Guenter
> 
> > Cc: Jean Delvare <jdelvare@suse.com>
> > Cc: Guenter Roeck <linux@roeck-us.net>
> > Cc: linux-hwmon@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Eduardo Valentin <eduval@amazon.com>
> > ---
> >  drivers/hwmon/hwmon.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> > index fcdbac4a56e3..6b3559f58b67 100644
> > --- a/drivers/hwmon/hwmon.c
> > +++ b/drivers/hwmon/hwmon.c
> > @@ -619,7 +619,7 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
> >  	if (err)
> >  		goto free_hwmon;
> >  
> > -	if (dev && chip && chip->ops->read &&
> > +	if (dev && dev->of_node && chip && chip->ops->read &&
> >  	    chip->info[0]->type == hwmon_chip &&
> >  	    (chip->info[0]->config[0] & HWMON_C_REGISTER_TZ)) {
> >  		const struct hwmon_channel_info **info = chip->info;
> > -- 
> > 2.21.0
> > 

-- 
All the best,
Eduardo Valentin

[PATCH 2/2] hwmon: core: fix potential memory leak in *hwmon_device_register*

[Eduardo Valentin]

When registering a hwmon device with HWMON_C_REGISTER_TZ flag
in place, the hwmon subsystem will attempt to register the device
also with the thermal subsystem. When the of-thermal registration
fails, __hwmon_device_register jumps to ida_remove, leaving
the locally allocated hwdev pointer and also the hdev registered.

This patch fixes both issues by jumping to a new label that
will first unregister hdev and the fall into the kfree of hwdev
to finally remove the idas and propagate the error code.

Cc: Jean Delvare <jdelvare@suse.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-hwmon@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <eduval@amazon.com>
---
 drivers/hwmon/hwmon.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 6b3559f58b67..6f1194952189 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -637,7 +637,7 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
 								hwdev, j);
 					if (err) {
 						device_unregister(hdev);
-						goto ida_remove;
+						goto device_unregister;
 					}
 				}
 			}
@@ -646,6 +646,8 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
 
 	return hdev;
 
+device_unregister:
+	device_unregister(hdev);
 free_hwmon:
 	kfree(hwdev);
 ida_remove:
-- 
2.21.0

Re: [PATCH 2/2] hwmon: core: fix potential memory leak in *hwmon_device_register*

[Guenter Roeck]

Hi Eduardo,

On Fri, May 17, 2019 at 04:13:37PM -0700, Eduardo Valentin wrote:
> When registering a hwmon device with HWMON_C_REGISTER_TZ flag
> in place, the hwmon subsystem will attempt to register the device
> also with the thermal subsystem. When the of-thermal registration
> fails, __hwmon_device_register jumps to ida_remove, leaving
> the locally allocated hwdev pointer and also the hdev registered.
> 
> This patch fixes both issues by jumping to a new label that
> will first unregister hdev and the fall into the kfree of hwdev
> to finally remove the idas and propagate the error code.
> 
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-hwmon@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Eduardo Valentin <eduval@amazon.com>
> ---
>  drivers/hwmon/hwmon.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 6b3559f58b67..6f1194952189 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -637,7 +637,7 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
>  								hwdev, j);
>  					if (err) {
>  						device_unregister(hdev);
> -						goto ida_remove;
> +						goto device_unregister;

Good find, but device_unregister() is already called above.
You need to either remove that, or replace the goto to point to free_hwmon.
The new label would probably the cleaner solution since it follows the
coding style.

Thanks
Guenter

>  					}
>  				}
>  			}
> @@ -646,6 +646,8 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
>  
>  	return hdev;
>  
> +device_unregister:
> +	device_unregister(hdev);
>  free_hwmon:
>  	kfree(hwdev);
>  ida_remove:
> -- 
> 2.21.0
> 

Re: [PATCH 2/2] hwmon: core: fix potential memory leak in *hwmon_device_register*

[Eduardo Valentin]

On Tue, May 28, 2019 at 08:06:40AM -0700, Guenter Roeck wrote:
> Hi Eduardo,
> 
> On Fri, May 17, 2019 at 04:13:37PM -0700, Eduardo Valentin wrote:
> > When registering a hwmon device with HWMON_C_REGISTER_TZ flag
> > in place, the hwmon subsystem will attempt to register the device
> > also with the thermal subsystem. When the of-thermal registration
> > fails, __hwmon_device_register jumps to ida_remove, leaving
> > the locally allocated hwdev pointer and also the hdev registered.
> > 
> > This patch fixes both issues by jumping to a new label that
> > will first unregister hdev and the fall into the kfree of hwdev
> > to finally remove the idas and propagate the error code.
> > 
> > Cc: Jean Delvare <jdelvare@suse.com>
> > Cc: Guenter Roeck <linux@roeck-us.net>
> > Cc: linux-hwmon@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Eduardo Valentin <eduval@amazon.com>
> > ---
> >  drivers/hwmon/hwmon.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> > index 6b3559f58b67..6f1194952189 100644
> > --- a/drivers/hwmon/hwmon.c
> > +++ b/drivers/hwmon/hwmon.c
> > @@ -637,7 +637,7 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
> >  								hwdev, j);
> >  					if (err) {
> >  						device_unregister(hdev);
> > -						goto ida_remove;
> > +						goto device_unregister;
> 
> Good find, but device_unregister() is already called above.
> You need to either remove that, or replace the goto to point to free_hwmon.
> The new label would probably the cleaner solution since it follows the
> coding style.

Right, somehow I completely missed that unregister call. In any case, I will
take the route of adding a new label and remove the unregister call above.

> 
> Thanks
> Guenter
> 
> >  					}
> >  				}
> >  			}
> > @@ -646,6 +646,8 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
> >  
> >  	return hdev;
> >  
> > +device_unregister:
> > +	device_unregister(hdev);
> >  free_hwmon:
> >  	kfree(hwdev);
> >  ida_remove:
> > -- 
> > 2.21.0
> > 

-- 
All the best,
Eduardo Valentin