Prev: use DECLARE_COMPLETION_ONSTACK for non-constant completion
Next: [PATCH 12/12] sched: Simplify set_task_cpu()
From: Anton Vorontsov on 17 Dec 2009 11:30
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(a)ru.mvista.com>
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(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Anton Vorontsov on 17 Dec 2009 11:50
On Wed, Dec 16, 2009 at 05:10:08PM +0100, Uwe Kleine-König wrote:
> platform_get_irq returns -ENXIO on failure, so !irq was probably
> always true. Better use (int)irq <= 0. Note that a return value of
> zero is still handled as error even though this could mean irq0.
>
> This is a followup to 305b3228f9ff4d59f49e6d34a7034d44ee8ce2f0 that
> changed the return value of platform_get_irq from 0 to -ENXIO on error.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig(a)pengutronix.de>
> Cc: David Vrabel <dvrabel(a)arcom.com>
> Cc: Greg Kroah-Hartman <gregkh(a)suse.de>
> Cc: David Brownell <dbrownell(a)users.sourceforge.net>
> Cc: Grant Likely <grant.likely(a)secretlab.ca>
> Cc: Kumar Gala <galak(a)kernel.crashing.org>
> Cc: Anton Vorontsov <avorontsov(a)ru.mvista.com>
> Cc: Andrew Morton <akpm(a)linux-foundation.org>
> Cc: spi-devel-general(a)lists.sourceforge.net
> ---
> drivers/spi/spi_mpc8xxx.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/spi/spi_mpc8xxx.c b/drivers/spi/spi_mpc8xxx.c
> index e9390d7..b13501a 100644
> --- a/drivers/spi/spi_mpc8xxx.c
> +++ b/drivers/spi/spi_mpc8xxx.c
> @@ -1339,7 +1339,7 @@ static int __devinit plat_mpc8xxx_spi_probe(struct platform_device *pdev)
> return -EINVAL;
>
> irq = platform_get_irq(pdev, 0);
> - if (!irq)
> + if ((int)irq <= 0)

I tend to think that it's really hopeless to fix the
platform_get_irq() in its current form, so can you get rid
of this ugly cast and just make the irq signed?

And I'll be fine with it. :-(

--
Anton Vorontsov
email: cbouatmailru(a)gmail.com
irc://irc.freenode.net/bd2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  | 
Pages: 1 2
Prev: use DECLARE_COMPLETION_ONSTACK for non-constant completion
Next: [PATCH 12/12] sched: Simplify set_task_cpu()