[PATCH 0/2] thermal: avoid using IS_ERR_OR_NULL()

[PATCH 0/2] thermal: avoid using IS_ERR_OR_NULL()

[Eduardo Valentin]

Hello Rui,

Here are two important patches I cooked while reading
linux-arm archives. IS_ERR_OR_NULL() can cause serious
problems and some ppl is even considering removing it [1].

For this reason, I am proposing the following cleanup under
drivers/thermal/

Please consider them.

BR,

Eduardo Valentin (2):
  thermal: thermal_core: remove usage of IS_ERR_OR_NULL
  thermal: db8500_cpufreq_cooling: remove usage of IS_ERR_OR_NULL()

 drivers/thermal/db8500_cpufreq_cooling.c | 2 +-
 drivers/thermal/thermal_core.c           | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

[1] - http://www.mail-archive.com/linux-omap@vger.kernel.org/msg78030.html

-- 
1.8.2.1.342.gfa7285d

[PATCH 1/2] thermal: thermal_core: remove usage of IS_ERR_OR_NULL

[Eduardo Valentin]

This patch changes the driver to avoid the usage of IS_ERR_OR_NULL()
macro. This macro can lead to dangerous results, like returning
success (0) during a failure scenario (NULL pointer handling).

The case present in this patch has simply be translated to
normal check for NULL and if the pointer has an error code.
The later case is needed because functions like
thermal_zone_get_zone_by_name() could return an ERR_PTR().

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Russell King <rmk@arm.linux.org.uk>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
---
 drivers/thermal/thermal_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index f36cd44..d755440 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -388,7 +388,7 @@ int thermal_zone_get_temp(struct thermal_zone_device *tz, unsigned long *temp)
 	enum thermal_trip_type type;
 #endif
 
-	if (IS_ERR_OR_NULL(tz))
+	if (!tz || IS_ERR(tz))
 		goto exit;
 
 	mutex_lock(&tz->lock);
-- 
1.8.2.1.342.gfa7285d

[PATCH 2/2] thermal: db8500_cpufreq_cooling: remove usage of IS_ERR_OR_NULL()

[Eduardo Valentin]

This patch changes the driver to avoid the usage of IS_ERR_OR_NULL()
macro. This macro can lead to dangerous results, like returning
success (0) during a failure scenario (NULL pointer handling).

The case present in this driver can be translated to a simple
check for IS_ERR(), as the cpufreq_cooling_register() returns
either a valid pointer or an ERR_PTR().

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Russell King <rmk@arm.linux.org.uk>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Hongbo Zhang <hongbo.zhang@stericsson.com>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: devicetree-discuss@lists.ozlabs.org
Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
---
 drivers/thermal/db8500_cpufreq_cooling.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/db8500_cpufreq_cooling.c b/drivers/thermal/db8500_cpufreq_cooling.c
index 21419851..786d192 100644
--- a/drivers/thermal/db8500_cpufreq_cooling.c
+++ b/drivers/thermal/db8500_cpufreq_cooling.c
@@ -37,7 +37,7 @@ static int db8500_cpufreq_cooling_probe(struct platform_device *pdev)
 	cpumask_set_cpu(0, &mask_val);
 	cdev = cpufreq_cooling_register(&mask_val);
 
-	if (IS_ERR_OR_NULL(cdev)) {
+	if (IS_ERR(cdev)) {
 		dev_err(&pdev->dev, "Failed to register cooling device\n");
 		return PTR_ERR(cdev);
 	}
-- 
1.8.2.1.342.gfa7285d

Re: [PATCH 2/2] thermal: db8500_cpufreq_cooling: remove usage of IS_ERR_OR_NULL()

[Russell King]

On Thu, Apr 25, 2013 at 10:13:34AM -0400, Eduardo Valentin wrote:
> diff --git a/drivers/thermal/db8500_cpufreq_cooling.c b/drivers/thermal/db8500_cpufreq_cooling.c
> index 21419851..786d192 100644
> --- a/drivers/thermal/db8500_cpufreq_cooling.c
> +++ b/drivers/thermal/db8500_cpufreq_cooling.c
> @@ -37,7 +37,7 @@ static int db8500_cpufreq_cooling_probe(struct platform_device *pdev)
>  	cpumask_set_cpu(0, &mask_val);
>  	cdev = cpufreq_cooling_register(&mask_val);
>  
> -	if (IS_ERR_OR_NULL(cdev)) {
> +	if (IS_ERR(cdev)) {
>  		dev_err(&pdev->dev, "Failed to register cooling device\n");
>  		return PTR_ERR(cdev);

Correct.  cpufreq_cooling_register() returns either an error-pointer or
a valid pointer.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 2/2] thermal: db8500_cpufreq_cooling: remove usage of IS_ERR_OR_NULL()

[Eduardo Valentin]

On 25-04-2013 13:46, Russell King wrote:
> On Thu, Apr 25, 2013 at 10:13:34AM -0400, Eduardo Valentin wrote:
>> diff --git a/drivers/thermal/db8500_cpufreq_cooling.c b/drivers/thermal/db8500_cpufreq_cooling.c
>> index 21419851..786d192 100644
>> --- a/drivers/thermal/db8500_cpufreq_cooling.c
>> +++ b/drivers/thermal/db8500_cpufreq_cooling.c
>> @@ -37,7 +37,7 @@ static int db8500_cpufreq_cooling_probe(struct platform_device *pdev)
>>   	cpumask_set_cpu(0, &mask_val);
>>   	cdev = cpufreq_cooling_register(&mask_val);
>>
>> -	if (IS_ERR_OR_NULL(cdev)) {
>> +	if (IS_ERR(cdev)) {
>>   		dev_err(&pdev->dev, "Failed to register cooling device\n");
>>   		return PTR_ERR(cdev);
>
> Correct.  cpufreq_cooling_register() returns either an error-pointer or
> a valid pointer.
>

Thanks for your review!

Re: [PATCH 2/2] thermal: db8500_cpufreq_cooling: remove usage of IS_ERR_OR_NULL()

[Fabio Baltieri]

On Thu, Apr 25, 2013 at 06:46:35PM +0100, Russell King wrote:
> On Thu, Apr 25, 2013 at 10:13:34AM -0400, Eduardo Valentin wrote:
> > diff --git a/drivers/thermal/db8500_cpufreq_cooling.c b/drivers/thermal/db8500_cpufreq_cooling.c
> > index 21419851..786d192 100644
> > --- a/drivers/thermal/db8500_cpufreq_cooling.c
> > +++ b/drivers/thermal/db8500_cpufreq_cooling.c
> > @@ -37,7 +37,7 @@ static int db8500_cpufreq_cooling_probe(struct platform_device *pdev)
> >  	cpumask_set_cpu(0, &mask_val);
> >  	cdev = cpufreq_cooling_register(&mask_val);
> >  
> > -	if (IS_ERR_OR_NULL(cdev)) {
> > +	if (IS_ERR(cdev)) {
> >  		dev_err(&pdev->dev, "Failed to register cooling device\n");
> >  		return PTR_ERR(cdev);
> 
> Correct.  cpufreq_cooling_register() returns either an error-pointer or
> a valid pointer.

Acked-by: Fabio Baltieri <fabio.baltieri@linaro.org>

...but more of these are going to appear forever, was the proposed
removal of IS_ERR_OR_NULL rejected?  Can't find any new message about
that on the list.

Fabio

-- 
Fabio Baltieri

Re: [PATCH 2/2] thermal: db8500_cpufreq_cooling: remove usage of IS_ERR_OR_NULL()

[Russell King]

On Fri, Apr 26, 2013 at 12:19:08PM +0200, Fabio Baltieri wrote:
> On Thu, Apr 25, 2013 at 06:46:35PM +0100, Russell King wrote:
> > On Thu, Apr 25, 2013 at 10:13:34AM -0400, Eduardo Valentin wrote:
> > > diff --git a/drivers/thermal/db8500_cpufreq_cooling.c b/drivers/thermal/db8500_cpufreq_cooling.c
> > > index 21419851..786d192 100644
> > > --- a/drivers/thermal/db8500_cpufreq_cooling.c
> > > +++ b/drivers/thermal/db8500_cpufreq_cooling.c
> > > @@ -37,7 +37,7 @@ static int db8500_cpufreq_cooling_probe(struct platform_device *pdev)
> > >  	cpumask_set_cpu(0, &mask_val);
> > >  	cdev = cpufreq_cooling_register(&mask_val);
> > >  
> > > -	if (IS_ERR_OR_NULL(cdev)) {
> > > +	if (IS_ERR(cdev)) {
> > >  		dev_err(&pdev->dev, "Failed to register cooling device\n");
> > >  		return PTR_ERR(cdev);
> > 
> > Correct.  cpufreq_cooling_register() returns either an error-pointer or
> > a valid pointer.
> 
> Acked-by: Fabio Baltieri <fabio.baltieri@linaro.org>
> 
> ...but more of these are going to appear forever, was the proposed
> removal of IS_ERR_OR_NULL rejected?  Can't find any new message about
> that on the list.

We first need to get rid of the existing users of it - I've got rid of
most of those in arch/arm - but it seems that I never pushed that in the
last merge window.

Really it needs the cooperation of everyone to make it happen across the
tree with everyone removing IS_ERR_OR_NULL() in their subsystem.  There
are currently 366 instances of this macro being used in the entire tree
which is far too many to even mark it as deprecated.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of: