2014-11-19 11:27:06

by SF Markus Elfring

Subject: [PATCH 1/1] PM-wakeup: Deletion of an unnecessary check before the function call "wakeup_source_unregister"

From: Markus Elfring <[email protected]>
Date: Wed, 19 Nov 2014 12:21:24 +0100

The wakeup_source_unregister() function tests whether its argument is NULL
and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <[email protected]>
---
drivers/base/power/wakeup.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index eb1bd2e..87dfc1d 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -267,8 +267,7 @@ int device_wakeup_disable(struct device *dev)
return -EINVAL;

ws = device_wakeup_detach(dev);
- if (ws)
- wakeup_source_unregister(ws);
+ wakeup_source_unregister(ws);

return 0;
}
--
2.1.3

2014-11-19 12:09:50

by Dan Carpenter

Subject: Re: [PATCH 1/1] PM-wakeup: Deletion of an unnecessary check before the function call "wakeup_source_unregister"

On Wed, Nov 19, 2014 at 12:26:45PM +0100, SF Markus Elfring wrote:
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -267,8 +267,7 @@ int device_wakeup_disable(struct device *dev)
> return -EINVAL;
>
> ws = device_wakeup_detach(dev);
> - if (ws)
> - wakeup_source_unregister(ws);
> + wakeup_source_unregister(ws);

In the original code, it's clear that the programmer thought about what
happens when the device_wakeup_detach() returns NULL but in the new code
that's not clear.

I guess the information is still there in the git archive, but why hide
the good code by covering it with confusing code?

regards,
dan carpenter

2014-11-19 13:04:05

by walter harms

Subject: Re: [PATCH 1/1] PM-wakeup: Deletion of an unnecessary check before the function call "wakeup_source_unregister"

Am 19.11.2014 13:09, schrieb Dan Carpenter:
> On Wed, Nov 19, 2014 at 12:26:45PM +0100, SF Markus Elfring wrote:
>> --- a/drivers/base/power/wakeup.c
>> +++ b/drivers/base/power/wakeup.c
>> @@ -267,8 +267,7 @@ int device_wakeup_disable(struct device *dev)
>> return -EINVAL;
>>
>> ws = device_wakeup_detach(dev);
>> - if (ws)
>> - wakeup_source_unregister(ws);
>> + wakeup_source_unregister(ws);
>
> In the original code, it's clear that the programmer thought about what
> happens when the device_wakeup_detach() returns NULL but in the new code
> that's not clear.
>
> I guess the information is still there in the git archive, but why hide
> the good code by covering it with confusing code?
>

hi,
just to add an other point of view ...

device_wakeup_detach returns dev->power.wakeup what is never NULL in this case.
(not visible here but a few line before exactly this is checked)

so this code can be compacted to:
wakeup_source_unregister(device_wakeup_detach(dev));

--readability

let the maintainer decide this byte-saving vs readability

re,
wh

2014-11-19 13:06:43

by Dan Carpenter

Subject: Re: [PATCH 1/1] PM-wakeup: Deletion of an unnecessary check before the function call "wakeup_source_unregister"

On Wed, Nov 19, 2014 at 01:54:49PM +0100, walter harms wrote:
> device_wakeup_detach returns dev->power.wakeup what is never NULL in this case.
> (not visible here but a few line before exactly this is checked)

Huh? I don't see a NULL check.

I think you may be confusing "dev->power.can_wakeup" with
"dev->power.wakeup"?

regards,
dan carpenter

2014-11-19 13:40:10

by walter harms

Subject: Re: [PATCH 1/1] PM-wakeup: Deletion of an unnecessary check before the function call "wakeup_source_unregister"

Am 19.11.2014 14:05, schrieb Dan Carpenter:
> On Wed, Nov 19, 2014 at 01:54:49PM +0100, walter harms wrote:
>> device_wakeup_detach returns dev->power.wakeup what is never NULL in this case.
>> (not visible here but a few line before exactly this is checked)
>
> Huh? I don't see a NULL check.
>
> I think you may be confusing "dev->power.can_wakeup" with
> "dev->power.wakeup"?
>
>

mea culpa,
you are right dev->power.can_wakeup != dev->power.wakeup

therefore device_wakeup_detach(dev) CAN return NULL

re,
wh

2015-06-28 10:22:16

by SF Markus Elfring

Subject: [PATCH] PM-wakeup: Delete unnecessary checks before two function calls

From: Markus Elfring <[email protected]>
Date: Sun, 28 Jun 2015 12:14:43 +0200

The functions dev_pm_disarm_wake_irq() and wakeup_source_unregister() test
whether their argument is NULL and then return immediately.
Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <[email protected]>
---
drivers/base/power/wakeup.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 40f7160..3741bc2 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -341,8 +341,7 @@ void device_wakeup_arm_wake_irqs(void)

rcu_read_lock();
list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
- if (ws->wakeirq)
- dev_pm_arm_wake_irq(ws->wakeirq);
+ dev_pm_arm_wake_irq(ws->wakeirq);
}
rcu_read_unlock();
}
@@ -358,8 +357,7 @@ void device_wakeup_disarm_wake_irqs(void)

rcu_read_lock();
list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
- if (ws->wakeirq)
- dev_pm_disarm_wake_irq(ws->wakeirq);
+ dev_pm_disarm_wake_irq(ws->wakeirq);
}
rcu_read_unlock();
}
@@ -396,9 +394,7 @@ int device_wakeup_disable(struct device *dev)
return -EINVAL;

ws = device_wakeup_detach(dev);
- if (ws)
- wakeup_source_unregister(ws);
-
+ wakeup_source_unregister(ws);
return 0;
}
EXPORT_SYMBOL_GPL(device_wakeup_disable);
--
2.4.4

2015-08-14 06:56:59

by Pavel Machek

Subject: Re: [PATCH] PM-wakeup: Delete unnecessary checks before two function calls

On Sun 2015-06-28 12:21:53, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Sun, 28 Jun 2015 12:14:43 +0200
>
> The functions dev_pm_disarm_wake_irq() and wakeup_source_unregister() test
> whether their argument is NULL and then return immediately.
> Thus the test around the calls is not needed.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <[email protected]>

ACK.


> ---
> drivers/base/power/wakeup.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index 40f7160..3741bc2 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -341,8 +341,7 @@ void device_wakeup_arm_wake_irqs(void)
>
> rcu_read_lock();
> list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
> - if (ws->wakeirq)
> - dev_pm_arm_wake_irq(ws->wakeirq);
> + dev_pm_arm_wake_irq(ws->wakeirq);
> }
> rcu_read_unlock();
> }
> @@ -358,8 +357,7 @@ void device_wakeup_disarm_wake_irqs(void)
>
> rcu_read_lock();
> list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
> - if (ws->wakeirq)
> - dev_pm_disarm_wake_irq(ws->wakeirq);
> + dev_pm_disarm_wake_irq(ws->wakeirq);
> }
> rcu_read_unlock();
> }
> @@ -396,9 +394,7 @@ int device_wakeup_disable(struct device *dev)
> return -EINVAL;
>
> ws = device_wakeup_detach(dev);
> - if (ws)
> - wakeup_source_unregister(ws);
> -
> + wakeup_source_unregister(ws);
> return 0;
> }
> EXPORT_SYMBOL_GPL(device_wakeup_disable);
> --
> 2.4.4

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html