| 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 |
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)
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
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
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
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 --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)
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(-)