Re: [PATCH 6/7] spi/mpc8xxx: don't check platform_get_irq's returnvalue against zero

[Anton Vorontsov]

On Thu, Dec 17, 2009 at 02:05:49PM +0100, Uwe Kleine-KÃnig wrote:
[...]
> > > Yes, there is an issue.  If the platform device doesn't have a resource
> > > specifing the irq, platform_get_irq returns -ENXIO.  So in the driver
> > > (unsigned)(-ENXIO) is passed to mpc8xxx_spi_probe as (!(-ENXIO)) is
> > > false and so the error isn't catched.
> > 
> > If you look into how we create the platform device, you'll notice
> > that -ENXIO isn't possible.
> > It's in arch/powerpc/platforms/83xx/mpc832x_rdb.c:of_fsl_spi_probe(),
> > which is a legacy interface that I'd like to be removed anyway.
> > 
> > Though, if you want to fix the inconsistency in the platform device
> > API, then I'd suggest to fix the platform_get_irq(). The driver itself
> > is correct.
> With platform_get_irq as it is today, the check in
> 
> 	irq = platform_get_irq(pdev, 0);
> 	if (!irq)
> 		return -EINVAL;

And I see no problem with this piece of code, really. I see the
problem in platform_get_irq() implementation (not that easy to fix,
see below).

> is non-sense as !irq always evaluates to 0.  If your argue that the
> resources are right,

No, I'm arguing that there is no issue. There *could* be an issue in
the future (if someone break the platform code), but the way you're
trying to fix the *possibility* isn't quite correct.

Believe me, I'd have no problem with this particular patch if the
patch could possibly fix any real world issue with this driver.

For example, you may notice the following commit:

commit 2fac6674ddf3164da42a76d62f8912073d629a30
Author: Anton Vorontsov <avorontsov@xxxxxxxxxxxxx>
Date:   Tue Jan 6 14:42:11 2009 -0800

    rtc: bunch of drivers: fix 'no irq' case handing

    This patch fixes a bunch of irq checking misuses.  Most drivers were
    getting irq via platform_get_irq(), which returns -ENXIO or r->start.

Sounds similar, eh? But you may notice that the unfixed code
was really wrong and didn't work for every sane platform:

        m48t59->irq = platform_get_irq(pdev, 0);
-       if (m48t59->irq < 0)
+       if (m48t59->irq <= 0)

The checks were irq < 0, so the drivers were requesting irq0, and
then were failing to probe.

Unfortunately, I stopped half way, afraid to possibly break current
platforms that use these drivers, so I kept the "<" part. Which is
a shame, because... um, it seems I spread the bad example further.

Anyway, I just did 'grep platform_get_irq -A 2 -r drivers/', and
it looks horrible, most callers check for irq < 0, which means
the code won't work on anything but arches with NO_IRQ == -1
(ARM, parisc, xtensa).

It seems the situation is near to hopeless. Maybe someone needs to
sit down and cleanup this mess once and for ever...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/