drivers: Integrate log message printing into devm_thermal_*_register()

[PATCH 01/12] thermal: of: Add error handling in devm_thermal_*_register()

Posted by Xichao Zhao 5 months, 1 week ago

devm_thermal_of_zone_register() does not print any error message
when registering a thermal zone with a device node sensor fails
and allocating device resource data fails.

This forces each driver to implement redundant error logging.
Additionally, when upper-layer functions propagate these errors
without logging, critical debugging information is lost.

Add dev_err_probe() in devm_thermal_of_zone_register() to unify
error reporting.

Signed-off-by: Xichao Zhao <zhao.xichao@vivo.com>
---
 drivers/thermal/thermal_of.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
index 1a51a4d240ff..8fe0ad402579 100644
--- a/drivers/thermal/thermal_of.c
+++ b/drivers/thermal/thermal_of.c
@@ -475,11 +475,15 @@ struct thermal_zone_device *devm_thermal_of_zone_register(struct device *dev, in
 
 	ptr = devres_alloc(devm_thermal_of_zone_release, sizeof(*ptr),
 			   GFP_KERNEL);
-	if (!ptr)
+	if (!ptr) {
+		dev_err(dev, "Failed to allocate device resource data\n");
 		return ERR_PTR(-ENOMEM);
+	}
 
 	tzd = thermal_of_zone_register(dev->of_node, sensor_id, data, ops);
 	if (IS_ERR(tzd)) {
+		dev_err_probe(dev, PTR_ERR(tzd),
+			      "Failed to register thermal zone sensor[%d]\n", sensor_id);
 		devres_free(ptr);
 		return tzd;
 	}
-- 
2.34.1

Re: [PATCH 01/12] thermal: of: Add error handling in devm_thermal_*_register()

Posted by Guenter Roeck 5 months, 1 week ago

On 9/5/25 00:23, Xichao Zhao wrote:
> devm_thermal_of_zone_register() does not print any error message
> when registering a thermal zone with a device node sensor fails
> and allocating device resource data fails.
> 
> This forces each driver to implement redundant error logging.
> Additionally, when upper-layer functions propagate these errors
> without logging, critical debugging information is lost.
> 
> Add dev_err_probe() in devm_thermal_of_zone_register() to unify
> error reporting.
> 
> Signed-off-by: Xichao Zhao <zhao.xichao@vivo.com>
> ---
>   drivers/thermal/thermal_of.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index 1a51a4d240ff..8fe0ad402579 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
> @@ -475,11 +475,15 @@ struct thermal_zone_device *devm_thermal_of_zone_register(struct device *dev, in
>   
>   	ptr = devres_alloc(devm_thermal_of_zone_release, sizeof(*ptr),
>   			   GFP_KERNEL);
> -	if (!ptr)
> +	if (!ptr) {
> +		dev_err(dev, "Failed to allocate device resource data\n");
>   		return ERR_PTR(-ENOMEM);
> +	}
>   
>   	tzd = thermal_of_zone_register(dev->of_node, sensor_id, data, ops);
>   	if (IS_ERR(tzd)) {
> +		dev_err_probe(dev, PTR_ERR(tzd),
> +			      "Failed to register thermal zone sensor[%d]\n", sensor_id);

This will print an error message even if the problem is (for the calling code,
such as hwmon) only informational.

Guenter

Re: [PATCH 01/12] thermal: of: Add error handling in devm_thermal_*_register()

Posted by Andy Shevchenko 5 months, 1 week ago

On Fri, Sep 5, 2025 at 10:25 AM Xichao Zhao <zhao.xichao@vivo.com> wrote:
>
> devm_thermal_of_zone_register() does not print any error message
> when registering a thermal zone with a device node sensor fails
> and allocating device resource data fails.
>
> This forces each driver to implement redundant error logging.
> Additionally, when upper-layer functions propagate these errors
> without logging, critical debugging information is lost.
>
> Add dev_err_probe() in devm_thermal_of_zone_register() to unify
> error reporting.

...

>         ptr = devres_alloc(devm_thermal_of_zone_release, sizeof(*ptr),
>                            GFP_KERNEL);
> -       if (!ptr)
> +       if (!ptr) {

> +               dev_err(dev, "Failed to allocate device resource data\n");

We do not add error messages for ENOMEM.

>                 return ERR_PTR(-ENOMEM);

Even if you want so eagerly to do that, it should be

   return dev_err_probe();

But, it will ignore the ENOMEM error code for printing.

> +       }

So, the bottom line, no need to add this message here.

...

>         tzd = thermal_of_zone_register(dev->of_node, sensor_id, data, ops);
>         if (IS_ERR(tzd)) {
> +               dev_err_probe(dev, PTR_ERR(tzd),
> +                             "Failed to register thermal zone sensor[%d]\n", sensor_id);
>                 devres_free(ptr);
>                 return tzd;

I don't see how ptr is related to the mesasge. Can't we use

  return dev_err_probe(dev, PTR_ERR(...), ...);

instead?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 01/12] thermal: of: Add error handling in devm_thermal_*_register()

Posted by Andy Shevchenko 5 months, 1 week ago

On Fri, Sep 5, 2025 at 12:33 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Sep 5, 2025 at 10:25 AM Xichao Zhao <zhao.xichao@vivo.com> wrote:

...

> >         ptr = devres_alloc(devm_thermal_of_zone_release, sizeof(*ptr),
> >                            GFP_KERNEL);
> > -       if (!ptr)
> > +       if (!ptr) {
>
> > +               dev_err(dev, "Failed to allocate device resource data\n");
>
> We do not add error messages for ENOMEM.
>
> >                 return ERR_PTR(-ENOMEM);
>
> Even if you want so eagerly to do that, it should be
>
>    return dev_err_probe();
>
> But, it will ignore the ENOMEM error code for printing.
>
> > +       }
>
> So, the bottom line, no need to add this message here.

...

> >         tzd = thermal_of_zone_register(dev->of_node, sensor_id, data, ops);
> >         if (IS_ERR(tzd)) {
> > +               dev_err_probe(dev, PTR_ERR(tzd),
> > +                             "Failed to register thermal zone sensor[%d]\n", sensor_id);
> >                 devres_free(ptr);
> >                 return tzd;
>
> I don't see how ptr is related to the message. Can't we use
>
>   return dev_err_probe(dev, PTR_ERR(...), ...);
>
> instead?

On top of that can we actually use devm_add_action_or_reset()?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 01/12] thermal: of: Add error handling in devm_thermal_*_register()

Posted by Niklas Söderlund 5 months, 1 week ago

Hello Xichao,

Thanks for your work.

On 2025-09-05 15:23:53 +0800, Xichao Zhao wrote:
> devm_thermal_of_zone_register() does not print any error message
> when registering a thermal zone with a device node sensor fails
> and allocating device resource data fails.
> 
> This forces each driver to implement redundant error logging.
> Additionally, when upper-layer functions propagate these errors
> without logging, critical debugging information is lost.
> 
> Add dev_err_probe() in devm_thermal_of_zone_register() to unify
> error reporting.
> 
> Signed-off-by: Xichao Zhao <zhao.xichao@vivo.com>
> ---
>  drivers/thermal/thermal_of.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index 1a51a4d240ff..8fe0ad402579 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
> @@ -475,11 +475,15 @@ struct thermal_zone_device *devm_thermal_of_zone_register(struct device *dev, in
>  
>  	ptr = devres_alloc(devm_thermal_of_zone_release, sizeof(*ptr),
>  			   GFP_KERNEL);
> -	if (!ptr)
> +	if (!ptr) {
> +		dev_err(dev, "Failed to allocate device resource data\n");
>  		return ERR_PTR(-ENOMEM);
> +	}
>  
>  	tzd = thermal_of_zone_register(dev->of_node, sensor_id, data, ops);
>  	if (IS_ERR(tzd)) {
> +		dev_err_probe(dev, PTR_ERR(tzd),
> +			      "Failed to register thermal zone sensor[%d]\n", sensor_id);

Don't thermal_of_zone_register() already print an error message for 
failure cases? If not can this print be moved there? That would allow 
the change you make in R-Car drivers to remove the prating completely, 
not just for the devm_* cases.

>  		devres_free(ptr);
>  		return tzd;
>  	}
> -- 
> 2.34.1
> 

-- 
Kind Regards,
Niklas Söderlund