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@xxxxxxxxxx>
> > ---
> > 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
