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/[email protected]/msg78030.html
--
1.8.2.1.342.gfa7285d
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 <[email protected]>
Cc: Russell King <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Eduardo Valentin <[email protected]>
---
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
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 <[email protected]>
Cc: Russell King <[email protected]>
Cc: Grant Likely <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Hongbo Zhang <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Eduardo Valentin <[email protected]>
---
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
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:
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!
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 <[email protected]>
...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
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 <[email protected]>
>
> ...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: