diff mbox series

watchdog: ftwdt010_wdt: fix test for platform_get_irq() failure

Message ID YvTgRk/ABp62/hNA@kili (mailing list archive)
State Accepted
Headers show
Series watchdog: ftwdt010_wdt: fix test for platform_get_irq() failure | expand

Commit Message

Dan Carpenter Aug. 11, 2022, 10:56 a.m. UTC 
This code assumes that platform_get_irq() function returns zero on
failure.  In fact, platform_get_irq() never returns zero.  It returns
negative error codes or positive non-zero values on success.

Fixes: eca10ae6000d ("watchdog: add driver for Cortina Gemini watchdog")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/watchdog/ftwdt010_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Guenter Roeck Aug. 11, 2022, 2:56 p.m. UTC | #1 
On 8/11/22 03:56, Dan Carpenter wrote:
> This code assumes that platform_get_irq() function returns zero on
> failure.  In fact, platform_get_irq() never returns zero.  It returns
> negative error codes or positive non-zero values on success.
> 
> Fixes: eca10ae6000d ("watchdog: add driver for Cortina Gemini watchdog")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/ftwdt010_wdt.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/ftwdt010_wdt.c b/drivers/watchdog/ftwdt010_wdt.c
> index 21dcc7765688..02112fc264bd 100644
> --- a/drivers/watchdog/ftwdt010_wdt.c
> +++ b/drivers/watchdog/ftwdt010_wdt.c
> @@ -156,7 +156,7 @@ static int ftwdt010_wdt_probe(struct platform_device *pdev)
>   	}
>   
>   	irq = platform_get_irq(pdev, 0);
> -	if (irq) {
> +	if (irq > 0) {
>   		ret = devm_request_irq(dev, irq, ftwdt010_wdt_interrupt, 0,
>   				       "watchdog bark", gwdt);
>   		if (ret)
Christophe JAILLET Aug. 11, 2022, 8:52 p.m. UTC | #2 
Le 11/08/2022 à 12:56, Dan Carpenter a écrit :
> This code assumes that platform_get_irq() function returns zero on
> failure.  In fact, platform_get_irq() never returns zero.  It returns
> negative error codes or positive non-zero values on success.
> 
> Fixes: eca10ae6000d ("watchdog: add driver for Cortina Gemini watchdog")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>   drivers/watchdog/ftwdt010_wdt.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/ftwdt010_wdt.c b/drivers/watchdog/ftwdt010_wdt.c
> index 21dcc7765688..02112fc264bd 100644
> --- a/drivers/watchdog/ftwdt010_wdt.c
> +++ b/drivers/watchdog/ftwdt010_wdt.c
> @@ -156,7 +156,7 @@ static int ftwdt010_wdt_probe(struct platform_device *pdev)
>   	}
>   
>   	irq = platform_get_irq(pdev, 0);
> -	if (irq) {
> +	if (irq > 0) {
>   		ret = devm_request_irq(dev, irq, ftwdt010_wdt_interrupt, 0,
>   				       "watchdog bark", gwdt);
>   		if (ret)

Hi,
can't platform_get_irq() return 0?
All the paths in platform_get_irq() look like 0 is a valid value.

The other patches you just sent are "< 0 ==> error", so ">= 0 ==> valid"

Any reason here for >0?

CJ
Guenter Roeck Aug. 11, 2022, 9:49 p.m. UTC | #3 
On Thu, Aug 11, 2022 at 10:52:13PM +0200, Christophe JAILLET wrote:
> Le 11/08/2022 à 12:56, Dan Carpenter a écrit :
> > This code assumes that platform_get_irq() function returns zero on
> > failure.  In fact, platform_get_irq() never returns zero.  It returns
> > negative error codes or positive non-zero values on success.
> > 
> > Fixes: eca10ae6000d ("watchdog: add driver for Cortina Gemini watchdog")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >   drivers/watchdog/ftwdt010_wdt.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/watchdog/ftwdt010_wdt.c b/drivers/watchdog/ftwdt010_wdt.c
> > index 21dcc7765688..02112fc264bd 100644
> > --- a/drivers/watchdog/ftwdt010_wdt.c
> > +++ b/drivers/watchdog/ftwdt010_wdt.c
> > @@ -156,7 +156,7 @@ static int ftwdt010_wdt_probe(struct platform_device *pdev)
> >   	}
> >   	irq = platform_get_irq(pdev, 0);
> > -	if (irq) {
> > +	if (irq > 0) {
> >   		ret = devm_request_irq(dev, irq, ftwdt010_wdt_interrupt, 0,
> >   				       "watchdog bark", gwdt);
> >   		if (ret)
> 
> Hi,
> can't platform_get_irq() return 0?
> All the paths in platform_get_irq() look like 0 is a valid value.
> 
> The other patches you just sent are "< 0 ==> error", so ">= 0 ==> valid"
> 
> Any reason here for >0?

API documentation says:

* Return: non-zero IRQ number on success, negative error number on failure.

Also, 0 is not a valid interrupt number.

Guenter
Dan Carpenter Aug. 12, 2022, 6:07 a.m. UTC | #4 
On Thu, Aug 11, 2022 at 10:52:13PM +0200, Christophe JAILLET wrote:
> Le 11/08/2022 à 12:56, Dan Carpenter a écrit :
> > This code assumes that platform_get_irq() function returns zero on
> > failure.  In fact, platform_get_irq() never returns zero.  It returns
> > negative error codes or positive non-zero values on success.
> > 
> > Fixes: eca10ae6000d ("watchdog: add driver for Cortina Gemini watchdog")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >   drivers/watchdog/ftwdt010_wdt.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/watchdog/ftwdt010_wdt.c b/drivers/watchdog/ftwdt010_wdt.c
> > index 21dcc7765688..02112fc264bd 100644
> > --- a/drivers/watchdog/ftwdt010_wdt.c
> > +++ b/drivers/watchdog/ftwdt010_wdt.c
> > @@ -156,7 +156,7 @@ static int ftwdt010_wdt_probe(struct platform_device *pdev)
> >   	}
> >   	irq = platform_get_irq(pdev, 0);
> > -	if (irq) {
> > +	if (irq > 0) {
> >   		ret = devm_request_irq(dev, irq, ftwdt010_wdt_interrupt, 0,
> >   				       "watchdog bark", gwdt);
> >   		if (ret)
> 
> Hi,
> can't platform_get_irq() return 0?
> All the paths in platform_get_irq() look like 0 is a valid value.
> 
> The other patches you just sent are "< 0 ==> error", so ">= 0 ==> valid"
> 
> Any reason here for >0?

It can't really be zero.  On some of the other patches there was a tests
failure and success.  So if we're testing for < 0 then a test for >= 0
felt more complete.

But here it was like, testing for > 0 won't break anything that isn't
already broken.  It's easier to review.

Somebody has a Coccinelle test for:

	irq = platform_get_irq(pdev, 0);
	if (!irq)
		return -ENODEV;

But I implemented it in Smatch just for fun.  It turns out people had
introduced a couple new bugs recently.  Also the it appears that their
Coccinelle test does not warn about for success tests like this one.
And there are still a bunch of test that do:

	if (irq <= 0)
		return irq ?: -ENODEV;

But testing for zero is dead code so I didn't bother cleaning it up.

regards,
dan carpenter
Christophe JAILLET Aug. 12, 2022, 7:22 a.m. UTC | #5 
Le 12/08/2022 à 08:07, Dan Carpenter a écrit :
> On Thu, Aug 11, 2022 at 10:52:13PM +0200, Christophe JAILLET wrote:
>> Le 11/08/2022 à 12:56, Dan Carpenter a écrit :
>>> This code assumes that platform_get_irq() function returns zero on
>>> failure.  In fact, platform_get_irq() never returns zero.  It returns
>>> negative error codes or positive non-zero values on success.
>>>
>>> Fixes: eca10ae6000d ("watchdog: add driver for Cortina Gemini watchdog")
>>> Signed-off-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>    drivers/watchdog/ftwdt010_wdt.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/watchdog/ftwdt010_wdt.c b/drivers/watchdog/ftwdt010_wdt.c
>>> index 21dcc7765688..02112fc264bd 100644
>>> --- a/drivers/watchdog/ftwdt010_wdt.c
>>> +++ b/drivers/watchdog/ftwdt010_wdt.c
>>> @@ -156,7 +156,7 @@ static int ftwdt010_wdt_probe(struct platform_device *pdev)
>>>    	}
>>>    	irq = platform_get_irq(pdev, 0);
>>> -	if (irq) {
>>> +	if (irq > 0) {
>>>    		ret = devm_request_irq(dev, irq, ftwdt010_wdt_interrupt, 0,
>>>    				       "watchdog bark", gwdt);
>>>    		if (ret)
>>
>> Hi,
>> can't platform_get_irq() return 0?
>> All the paths in platform_get_irq() look like 0 is a valid value.
>>
>> The other patches you just sent are "< 0 ==> error", so ">= 0 ==> valid"
>>
>> Any reason here for >0?
> 
> It can't really be zero.  On some of the other patches there was a tests
> failure and success.  So if we're testing for < 0 then a test for >= 0
> felt more complete.
> 
> But here it was like, testing for > 0 won't break anything that isn't
> already broken.  It's easier to review.
> 
> Somebody has a Coccinelle test for:
> 
> 	irq = platform_get_irq(pdev, 0);
> 	if (!irq)
> 		return -ENODEV;
> 
> But I implemented it in Smatch just for fun.  It turns out people had
> introduced a couple new bugs recently.  Also the it appears that their
> Coccinelle test does not warn about for success tests like this one.
> And there are still a bunch of test that do:
> 
> 	if (irq <= 0)
> 		return irq ?: -ENODEV;
> 
> But testing for zero is dead code so I didn't bother cleaning it up.
> 
> regards,
> dan carpenter
> 

Got it, thanks.

CJ
diff mbox series

Patch

diff --git a/drivers/watchdog/ftwdt010_wdt.c b/drivers/watchdog/ftwdt010_wdt.c
index 21dcc7765688..02112fc264bd 100644
--- a/drivers/watchdog/ftwdt010_wdt.c
+++ b/drivers/watchdog/ftwdt010_wdt.c
@@ -156,7 +156,7 @@  static int ftwdt010_wdt_probe(struct platform_device *pdev)
 	}
 
 	irq = platform_get_irq(pdev, 0);
-	if (irq) {
+	if (irq > 0) {
 		ret = devm_request_irq(dev, irq, ftwdt010_wdt_interrupt, 0,
 				       "watchdog bark", gwdt);
 		if (ret)