[PATCH 6/7] spi/mpc8xxx: don't check platform_get_irq's return value against zero
Uwe Kleine-König
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.klein...@pengutronix.de>
Cc: David Vrabel <dvr...@arcom.com>
Cc: Greg Kroah-Hartman <gre...@suse.de>
Cc: David Brownell <dbro...@users.sourceforge.net>
Cc: Grant Likely <grant....@secretlab.ca>
Cc: Kumar Gala <ga...@kernel.crashing.org>
Cc: Anton Vorontsov <avoro...@ru.mvista.com>
Cc: Andrew Morton <ak...@linux-foundation.org>
Cc: spi-deve...@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)
return -EINVAL;
master = mpc8xxx_spi_probe(&pdev->dev, mem, irq);
--
1.6.5.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Uwe Kleine-König
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.klein...@pengutronix.de>
Cc: David Vrabel <dvr...@arcom.com>
Cc: Greg Kroah-Hartman <gre...@suse.de>
Cc: Liam Girdwood <l...@slimlogic.co.uk>
Cc: Mark Brown <bro...@opensource.wolfsonmicro.com>
Cc: Jaroslav Kysela <pe...@perex.cz>
Cc: Takashi Iwai <ti...@suse.de>
Cc: Kuninori Morimoto <morimoto...@renesas.com>
Cc: Paul Mundt <let...@linux-sh.org>
Cc: alsa-...@alsa-project.org
---
sound/soc/sh/fsi.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c
index 9c49c11..42813b8 100644
--- a/sound/soc/sh/fsi.c
+++ b/sound/soc/sh/fsi.c
@@ -876,7 +876,7 @@ static int fsi_probe(struct platform_device *pdev)
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
irq = platform_get_irq(pdev, 0);
- if (!res || !irq) {
+ if (!res || (int)irq <= 0) {
dev_err(&pdev->dev, "Not enough FSI platform resources.\n");
ret = -ENODEV;
goto exit;
Uwe Kleine-König
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.klein...@pengutronix.de>
Cc: David Vrabel <dvr...@arcom.com>
Cc: Greg Kroah-Hartman <gre...@suse.de>
Cc: Rafael J. Wysocki <r...@sisk.pl>
Cc: linux-...@lists.infradead.org
---
drivers/pcmcia/bfin_cf_pcmcia.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/pcmcia/bfin_cf_pcmcia.c b/drivers/pcmcia/bfin_cf_pcmcia.c
index 300b368..2482ce7 100644
--- a/drivers/pcmcia/bfin_cf_pcmcia.c
+++ b/drivers/pcmcia/bfin_cf_pcmcia.c
@@ -205,7 +205,7 @@ static int __devinit bfin_cf_probe(struct platform_device *pdev)
dev_info(&pdev->dev, "Blackfin CompactFlash/PCMCIA Socket Driver\n");
irq = platform_get_irq(pdev, 0);
- if (!irq)
+ if (irq <= 0)
return -EINVAL;
cd_pfx = platform_get_irq(pdev, 1); /*Card Detect GPIO PIN */
Uwe Kleine-König
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.klein...@pengutronix.de>
Cc: David Vrabel <dvr...@arcom.com>
Cc: Greg Kroah-Hartman <gre...@suse.de>
Cc: Urs Thuermann <urs.th...@volkswagen.de>
Cc: Oliver Hartkopp <oliver....@volkswagen.de>
Cc: David S. Miller <da...@davemloft.net>
Cc: Wolfgang Grandegger <w...@grandegger.com>
Cc: Kurt Van Dijck <kurt.va...@eia.be>
Cc: net...@vger.kernel.org
---
drivers/net/can/at91_can.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/can/at91_can.c b/drivers/net/can/at91_can.c
index cbe3fce..631d404 100644
--- a/drivers/net/can/at91_can.c
+++ b/drivers/net/can/at91_can.c
@@ -1037,7 +1037,7 @@ static int __init at91_can_probe(struct platform_device *pdev)
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
irq = platform_get_irq(pdev, 0);
- if (!res || !irq) {
+ if (!res || irq <= 0) {
err = -ENODEV;
goto exit_put;
Wolfgang Grandegger
> 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.
But only on ARM, which is the only platform still using the infamous
NO_IRQ (=-1). As this is a driver for ARM hardware, using irq == NO_IRQ
would make sense, though.
Wolfgang.
Anton Vorontsov
> 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.klein...@pengutronix.de>
Noooooo... :-(
Please revert 305b3228f9ff4d59f49e6d34a7034d44ee8ce2f0 instead,
and fix platforms to remap HWIRQ0 to something that is not VIRQ0.
IRQ0 is invalid for everything that is outside of arch/*.
http://lkml.org/lkml/2005/11/22/159
http://lkml.org/lkml/2005/11/22/213
http://lkml.org/lkml/2005/11/22/227
--
Anton Vorontsov
email: cbouat...@gmail.com
irc://irc.freenode.net/bd2
Uwe Kleine-König
> 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.
>
> But only on ARM, which is the only platform still using the infamous
> NO_IRQ (=-1). As this is a driver for ARM hardware, using irq == NO_IRQ
> would make sense, though.
This has nothing to do with NO_IRQ. You could do:
- if (!res || !irq) {
+ if (!res || irq <= (int)NO_IRQ) {
but this looks too ugly. (IMHO using NO_IRQ is already ugly.)
Still, before my patch platform_get_irq return 0 was an error and if
this should be handled as irq0 this is a separate issue that should be
fixed in a separate patch.
Best regards
Uwe
PS:
linux-2.6$ git grep -E 'define *NO_IRQ\>' arch/*/include
arch/arm/include/asm/irq.h:#define NO_IRQ ((unsigned int)(-1))
arch/microblaze/include/asm/irq.h:#define NO_IRQ (-1)
arch/mn10300/include/asm/irq.h:#define NO_IRQ INT_MAX
arch/parisc/include/asm/irq.h:#define NO_IRQ (-1)
arch/powerpc/include/asm/irq.h:#define NO_IRQ (0)
--
Pengutronix e.K. | Uwe Kleine-K�nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Alan Cox
>
> but this looks too ugly. (IMHO using NO_IRQ is already ugly.)
No IRQ was a private variable for the drivers/ide stack internally and
only present in any form on a few odd platforms where it got "borrowed"
and hasn't yet been eliminated
The absence of an IRQ is zero. A bus IRQ of zero is remapped by the OS.
Alan
Uwe Kleine-König
[Note the email address used for David Vrabel isn't valid any more,
this mail uses his last used address.]
On Wed, Dec 16, 2009 at 07:32:29PM +0300, Anton Vorontsov wrote:
> 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.klein...@pengutronix.de>
> > ---
>
> Noooooo... :-(
>
> Please revert 305b3228f9ff4d59f49e6d34a7034d44ee8ce2f0 instead,
> and fix platforms to remap HWIRQ0 to something that is not VIRQ0.
>
> IRQ0 is invalid for everything that is outside of arch/*.
>
> http://lkml.org/lkml/2005/11/22/159
> http://lkml.org/lkml/2005/11/22/213
> http://lkml.org/lkml/2005/11/22/227
First note that my check is safe with both variants (e.g. it does the
right thing independent of the error being signaled by 0 or
-ESOMETHING.)
Then arch/arm/mach-pxa/devices.c has:
static struct resource pxa27x_resource_ssp3[] = {
...
[1] = {
.start = IRQ_SSP3,
.end = IRQ_SSP3,
.flags = IORESOURCE_IRQ,
},
...
}
with IRQ_SSP3 being zero (sometimes). The driver is implemented in
arch/arm/mach-pxa/ssp.c and uses platform_get_irq. So according to your
definition it's allowed (arch/* only). Still this would break if you
revert 305b3228f9.
Actually I don't care much, but as platform_get_irq returns an int I
think it's fine for it to signal an error using a value < 0 as irqs are
not negative.
My position regarding irq0 is: If a variable holds either a valid irq
or a value indicating "no irq", then feel free to use 0 as "no irq" and
a value > 0 for a valid irq (without offset). If you want irq0 here,
you're out of luck. But if you have a variable holding a valid irq only
(that is, there is no doubt if the value is valid or not) I see no
reason to dogmatically prohibit irq0.
I'm a bit annoyed as this is the third time[1] this month this irq0
discussion pops up for me. I think people see that irq0 is involved
somehow, start wailing and stop seeing the issues being fixed.
Best regards
Uwe
[1] one is:
http://thread.gmane.org/gmane.linux.kernel/924739
the other wasn't on lkml, only mm-commits. Cannot find it on the
net now.
--
Pengutronix e.K. | Uwe Kleine-K�nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
David Vrabel
> Hello,
>
> [Note the email address used for David Vrabel isn't valid any more,
> this mail uses his last used address.]
I've not been involved in PXA27x stuff for years. Please drop me from
the CC, thanks.
David
Anton Vorontsov
[...]
> > Noooooo... :-(
> >
> > Please revert 305b3228f9ff4d59f49e6d34a7034d44ee8ce2f0 instead,
> > and fix platforms to remap HWIRQ0 to something that is not VIRQ0.
> >
> > IRQ0 is invalid for everything that is outside of arch/*.
> >
> > http://lkml.org/lkml/2005/11/22/159
> > http://lkml.org/lkml/2005/11/22/213
> > http://lkml.org/lkml/2005/11/22/227
> First note that my check is safe with both variants (e.g. it does the
> right thing independent of the error being signaled by 0 or
> -ESOMETHING.)
>
> Then arch/arm/mach-pxa/devices.c has:
>
> static struct resource pxa27x_resource_ssp3[] = {
> ...
> [1] = {
> .start = IRQ_SSP3,
> .end = IRQ_SSP3,
> .flags = IORESOURCE_IRQ,
> },
> ...
> }
>
> with IRQ_SSP3 being zero (sometimes). The driver is implemented in
> arch/arm/mach-pxa/ssp.c and uses platform_get_irq.
So fix this *one* driver? Implement arm-specific platform_get_irq() as
a band-aid. Or better, implement virtual irqs <-> hardware irqs mapping
for ARM.
[...]
> I'm a bit annoyed as this is the third time[1] this month this irq0
> discussion pops up for me. I think people see that irq0 is involved
> somehow, start wailing and stop seeing the issues being fixed.
For this particular driver, there is NO issue whatsoever. It is
only used for PowerPC, which has VIRQ0 == invalid IRQ. And note
that there still could be HWIRQ0 on PowerPC, but it is *never*
mapped to VIRQ0.
[...]
> [1] one is:
> http://thread.gmane.org/gmane.linux.kernel/924739
No wonder the discussion popped up. You're adding some ugly
#ifdef stuff that adds some arch-specific knowledge to a generic
code.
Sure, there's a lot of ugly code even in the kernel/ directly, but
you have to prepare for resistance when you add more of it.
So, if you want to fix the root cause of the issue: revert the
305b3228f9ff4d59f49e6d34a7034d44ee8ce2f0, and try to improve the
ARM land, do not band-aid the whole kernel all over the place.
--
Anton Vorontsov
email: cbouat...@gmail.com
irc://irc.freenode.net/bd2
Wolfgang Grandegger
> On Wed, Dec 16, 2009 at 05:27:03PM +0100, Wolfgang Grandegger wrote:
>> 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.
>> But only on ARM, which is the only platform still using the infamous
>> NO_IRQ (=-1). As this is a driver for ARM hardware, using irq == NO_IRQ
>> would make sense, though.
>
> This has nothing to do with NO_IRQ. You could do:
Right, sorry for the noise.
> - if (!res || !irq) {
> + if (!res || irq <= (int)NO_IRQ) {
>
> but this looks too ugly. (IMHO using NO_IRQ is already ugly.)
>
> Still, before my patch platform_get_irq return 0 was an error and if
> this should be handled as irq0 this is a separate issue that should be
> fixed in a separate patch.
"irq <= 0" seems then the best solution. Will add my signed-off-by to
the patch.
Thanks,
Wolfgang.
Wolfgang Grandegger
> 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.
>
> Cc: David Vrabel <dvr...@arcom.com>
> Cc: Greg Kroah-Hartman <gre...@suse.de>
> Cc: Urs Thuermann <urs.th...@volkswagen.de>
> Cc: Oliver Hartkopp <oliver....@volkswagen.de>
> Cc: David S. Miller <da...@davemloft.net>
> Cc: Wolfgang Grandegger <w...@grandegger.com>
> Cc: Kurt Van Dijck <kurt.va...@eia.be>
> Cc: net...@vger.kernel.org
Signed-off-by: Wolfgang Grandegger <w...@grandegger.com>
> ---
> drivers/net/can/at91_can.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/can/at91_can.c b/drivers/net/can/at91_can.c
> index cbe3fce..631d404 100644
> --- a/drivers/net/can/at91_can.c
> +++ b/drivers/net/can/at91_can.c
> @@ -1037,7 +1037,7 @@ static int __init at91_can_probe(struct platform_device *pdev)
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> irq = platform_get_irq(pdev, 0);
> - if (!res || !irq) {
> + if (!res || irq <= 0) {
> err = -ENODEV;
> goto exit_put;
> }
Thanks,
Wolfgang.
Uwe Kleine-König
On Wed, Dec 16, 2009 at 09:20:34PM +0300, Anton Vorontsov 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.
> [...]
> > [1] one is:
> > http://thread.gmane.org/gmane.linux.kernel/924739
>
> No wonder the discussion popped up. You're adding some ugly
> #ifdef stuff that adds some arch-specific knowledge to a generic
> code.
I wouldn't argue if people objected to the arch-specific #ifdef. The
arch-specific code is already in there and I don't object to just
removing it. But answering that irq0 must not be used isn't helpful.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K�nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Anton Vorontsov
[...]
> > > I'm a bit annoyed as this is the third time[1] this month this irq0
> > > discussion pops up for me. I think people see that irq0 is involved
> > > somehow, start wailing and stop seeing the issues being fixed.
> >
> > For this particular driver, there is NO issue whatsoever. It is
> > only used for PowerPC, which has VIRQ0 == invalid IRQ. And note
> > that there still could be HWIRQ0 on PowerPC, but it is *never*
> > mapped to VIRQ0.
> 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.
--
Anton Vorontsov
email: cbouat...@gmail.com
irc://irc.freenode.net/bd2
Anton Vorontsov
[...]
> which is a legacy interface that I'd like to be removed anyway.
...which means I really don't care that much to nack or ack the
patch. Do whatever you feel the best, but you must realize that
what you're doing is just papering over the real problem, and
maybe even spreading the problem further.
Kuninori Morimoto
Dear Uwe
> 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.
(snip)
> - if (!res || !irq) {
> + if (!res || (int)irq <= 0) {
Ohh..
Thank you for checking.
Hmm.. now I tried to check about platform_get_irq in Linux kernel.
In my easy check, I can find a lot of drivers which are...
o doesn't check irq value ex) request_irq(platform_get_irq(...))
o checked irq but it have miss (?) ex) if (irq >= 0) OK
if (irq) OK
if (irq == -Exxx ) NG
o checked irq but don't care zero ex) if (irq < 0) NG
o used above style ex) if (!irq) NG
Should we modify their too ?
How about create new macro/function to check it ?
Below came from 2.6.32-rc6
-- doesn't check irq ---------------
arch/arm/plat-omap/iommu.c :: 945
arch/avr32/mach-at32ap/pio.c :: 392
arch/sh/drivers/push-switch.c :: 55
drivers/ata/pata_bf54x.c :: 1627
drivers/ata/pata_octeon_cf.c :: 905
drivers/ata/sata_mv.c :: 4063
drivers/dma/at_hdmac.c :: 1135
drivers/dma/dw_dmac.c :: 1379
drivers/edac/mv64x60_edac.c :: 177
drivers/edac/mv64x60_edac.c :: 344
drivers/edac/mv64x60_edac.c :: 539
drivers/edac/mv64x60_edac.c :: 784
drivers/i2c/busses/i2c-pca-platform.c :: 141
drivers/i2c/busses/i2c-sh7760.c :: 474
drivers/i2c/busses/i2c-stu300.c :: 917
drivers/input/keyboard/pxa930_rotary.c :: 180
drivers/input/keyboard/sh_keysc.c :: 254
drivers/input/keyboard/sh_keysc.c :: 270
drivers/input/keyboard/sh_keysc.c :: 290
drivers/input/keyboard/twl4030_keypad.c :: 362
drivers/input/misc/twl4030-pwrbutton.c :: 68
drivers/input/misc/twl4030-pwrbutton.c :: 111
drivers/input/misc/wm831x-on.c :: 75
drivers/input/misc/wm831x-on.c :: 128
drivers/input/mouse/pxa930_trkball.c :: 236
drivers/input/touchscreen/w90p910_ts.c :: 279
drivers/mmc/host/at91_mci.c :: 1082
drivers/mmc/host/atmel-mci.c :: 1732
drivers/mmc/host/sdhci-pltfm.c :: 80
drivers/mtd/nand/mxc_nand.c :: 935
drivers/mtd/nand/tmio_nand.c :: 380
drivers/mtd/onenand/generic.c :: 67
drivers/net/dnet.c :: 847
drivers/net/fec.c :: 1890
drivers/net/jazzsonic.c :: 237
drivers/net/macb.c :: 1183
drivers/net/ne.c :: 808
drivers/net/s6gmac.c :: 975
drivers/net/smc911x.c :: 2088
drivers/pcmcia/bfin_cf_pcmcia.c :: 211
drivers/regulator/wm831x-isink.c :: 197
drivers/regulator/wm831x-isink.c :: 223
drivers/rtc/rtc-cmos.c :: 1147
drivers/rtc/rtc-coh901331.c :: 200
drivers/rtc/rtc-stmp3xxx.c :: 201
drivers/rtc/rtc-stmp3xxx.c :: 202
drivers/rtc/rtc-twl4030.c :: 471
drivers/scsi/jazz_esp.c :: 172
drivers/scsi/sni_53c710.c :: 100
drivers/scsi/sun3x_esp.c :: 235
drivers/serial/mpsc.c :: 2058
drivers/serial/sc26xx.c :: 654
drivers/usb/gadget/at91_udc.c :: 1733
drivers/usb/gadget/m66592-udc.c :: 1553
drivers/usb/gadget/pxa25x_udc.c :: 2326
drivers/usb/gadget/r8a66597-udc.c :: 1502
drivers/usb/host/ohci-tmio.c :: 192
drivers/usb/otg/twl4030-usb.c :: 672
drivers/video/pxafb.c :: 2234
drivers/video/s3c2410fb.c :: 1041
drivers/video/tmiofb.c :: 691
drivers/video/tmiofb.c :: 816
drivers/watchdog/coh901327_wdt.c :: 431
sound/drivers/ml403-ac97cr.c :: 1154
sound/drivers/ml403-ac97cr.c :: 1167
---- checked irq but it have miss (?) ---------
drivers/ata/pata_ixp4xx_cf.c :: 168
drivers/block/mg_disk.c :: 926
drivers/crypto/mv_cesa.c :: 517
drivers/dma/at_hdmac.c :: 1110
drivers/dma/txx9dmac.c :: 1256
drivers/gpio/vr41xx_giu.c :: 548
drivers/i2c/busses/i2c-highlander.c :: 385
drivers/i2c/busses/i2c-pmcmsp.c :: 306
drivers/i2c/busses/i2c-pxa.c :: 1006
drivers/input/keyboard/omap-keypad.c :: 399
drivers/mfd/t7l66xb.c :: 313
drivers/mfd/tc6387xb.c :: 123
drivers/mfd/tc6393xb.c :: 588
drivers/misc/atmel_tclib.c :: 114
drivers/misc/atmel_tclib.c :: 139
drivers/misc/atmel_tclib.c :: 142
drivers/mmc/host/s3cmci.c :: 1630
drivers/mmc/host/tmio_mmc.c :: 585
drivers/rtc/rtc-mxc.c :: 446
drivers/rtc/rtc-sh.c :: 631
drivers/rtc/rtc-sh.c :: 632
drivers/serial/imx.c :: 1250
drivers/serial/imx.c :: 1251
drivers/serial/imx.c :: 1252
drivers/serial/imx.c :: 1253
drivers/serial/vr41xx_siu.c :: 721
drivers/usb/musb/musb_core.c :: 2123
drivers/usb/musb/musbhsdma.c :: 358
drivers/video/sh7760fb.c :: 476
---- checked irq but don't care zero ----------
arch/arm/common/locomo.c :: 795
arch/arm/common/sa1111.c :: 950
arch/arm/mach-omap2/mailbox.c :: 308
arch/arm/mach-pxa/ssp.c :: 388
arch/arm/plat-omap/iommu.c :: 897
arch/sh/drivers/push-switch.c :: 103
drivers/ata/pata_at32.c :: 287
drivers/clocksource/sh_cmt.c :: 588
drivers/clocksource/sh_mtu2.c :: 267
drivers/clocksource/sh_tmu.c :: 372
drivers/dma/at_hdmac.c :: 1001
drivers/dma/dw_dmac.c :: 1258
drivers/dma/iop-adma.c :: 1560
drivers/dma/ipu/ipu_idmac.c :: 1758
drivers/dma/ipu/ipu_idmac.c :: 1763
drivers/dma/mv_xor.c :: 1203
drivers/dma/txx9dmac.c :: 1171
drivers/i2c/busses/i2c-bfin-twi.c :: 662
drivers/i2c/busses/i2c-imx.c :: 484
drivers/i2c/busses/i2c-iop3xx.c :: 469
drivers/i2c/busses/i2c-mv64xxx.c :: 523
drivers/i2c/busses/i2c-s6000.c :: 267
drivers/ide/au1xxx-ide.c :: 520
drivers/ide/tx4938ide.c :: 141
drivers/ide/tx4939ide.c :: 546
drivers/input/keyboard/bf54x-keys.c :: 245
drivers/input/keyboard/ep93xx_keypad.c :: 277
drivers/input/keyboard/opencores-kbd.c :: 52
drivers/input/keyboard/pxa27x_keypad.c :: 460
drivers/input/keyboard/pxa930_rotary.c :: 93
drivers/input/keyboard/sh_keysc.c :: 150
drivers/input/keyboard/w90p910_keypad.c :: 137
drivers/input/misc/bfin_rotary.c :: 126
drivers/input/misc/dm355evm_keys.c :: 230
drivers/input/mouse/pxa930_trkball.c :: 153
drivers/input/serio/at32psif.c :: 258
drivers/input/touchscreen/atmel_tsadcc.c :: 200
drivers/input/touchscreen/corgi_ts.c :: 284
drivers/media/video/pxa_camera.c :: 1635
drivers/mfd/asic3.c :: 380
drivers/mfd/asic3.c :: 785
drivers/mfd/sm501.c :: 1417
drivers/misc/atmel_pwm.c :: 308
drivers/mmc/host/atmel-mci.c :: 1612
drivers/mmc/host/imxmmc.c :: 947
drivers/mmc/host/mvsdio.c :: 712
drivers/mmc/host/mxcmmc.c :: 688
drivers/mmc/host/omap.c :: 1413
drivers/mmc/host/omap_hsmmc.c :: 1632
drivers/mmc/host/pxamci.c :: 555
drivers/mmc/host/sdhci-s3c.c :: 231
drivers/mtd/nand/pxa3xx_nand.c :: 1215
drivers/net/arm/am79c961a.c :: 709
drivers/net/arm/w90p910_ether.c :: 1013
drivers/net/arm/w90p910_ether.c :: 1020
drivers/net/fec.c :: 1884
drivers/net/ks8842.c :: 649
drivers/net/ks8851_mll.c :: 1564
drivers/net/s6gmac.c :: 1012
drivers/net/sh_eth.c :: 1410
drivers/pcmcia/omap_cf.c :: 217
drivers/rtc/rtc-pxa.c :: 373
drivers/rtc/rtc-pxa.c :: 378
drivers/rtc/rtc-s3c.c :: 412
drivers/rtc/rtc-s3c.c :: 418
drivers/rtc/rtc-tx4939.c :: 246
drivers/serial/msm_serial.c :: 714
drivers/serial/samsung.c :: 1100
drivers/serial/timbuart.c :: 456
drivers/spi/atmel_spi.c :: 753
drivers/spi/spi_bfin5xx.c :: 1304
drivers/spi/spi_s3c24xx.c :: 370
drivers/spi/spi_stmp.c :: 500
drivers/spi/spi_stmp.c :: 505
drivers/spi/spi_txx9.c :: 393
drivers/uio/uio_smx.c :: 77
drivers/usb/gadget/atmel_usba_udc.c :: 1897
drivers/usb/gadget/imx_udc.c :: 1461
drivers/usb/gadget/pxa25x_udc.c :: 2176
drivers/usb/gadget/pxa27x_udc.c :: 2414
drivers/usb/gadget/s3c-hsotg.c :: 3145
drivers/usb/host/ehci-w90x900.c :: 77
drivers/usb/host/isp1362-hcd.c :: 2727
drivers/usb/host/ohci-omap.c :: 361
drivers/usb/host/ohci-pnx4008.c :: 389
drivers/usb/host/ohci-pxa27x.c :: 297
drivers/usb/host/ohci-sh.c :: 97
drivers/usb/host/ohci-sm501.c :: 95
drivers/video/atmel_lcdfb.c :: 864
drivers/video/bf54x-lq043fb.c :: 657
drivers/video/bfin-t350mcqb-fb.c :: 551
drivers/video/da8xx-fb.c :: 758
drivers/video/msm/mdp.c :: 402
drivers/video/pxa168fb.c :: 629
drivers/video/pxafb.c :: 2129
drivers/video/s3c2410fb.c :: 846
drivers/video/sa1100fb.c :: 1443
drivers/video/sh_mobile_lcdcfb.c :: 904
drivers/video/sm501fb.c :: 1326
drivers/w1/masters/omap_hdq.c :: 630
drivers/watchdog/mpcore_wdt.c :: 348
sound/atmel/abdac.c :: 405
sound/atmel/ac97c.c :: 772
sound/soc/s6000/s6000-i2s.c :: 540
sound/soc/txx9/txx9aclc-ac97.c :: 193
---- it use !irq ------------------------
arch/avr32/mach-at32ap/extint.c :: 200
drivers/media/video/mx1_camera.c :: 651
drivers/media/video/sh_mobile_ceu_camera.c :: 1655
drivers/misc/atmel-ssc.c :: 110
drivers/net/can/at91_can.c :: 1071
drivers/net/sni_82596.c :: 112
drivers/pcmcia/bfin_cf_pcmcia.c :: 207
drivers/spi/spi_imx.c :: 553
drivers/spi/spi_mpc8xxx.c :: 885
drivers/usb/gadget/fsl_udc_core.c :: 2303
Best regards
--
Kuninori Morimoto
Wolfgang Grandegger
>> + if (!res || irq <= (int)NO_IRQ) {
>>
>> but this looks too ugly. (IMHO using NO_IRQ is already ugly.)
>
> No IRQ was a private variable for the drivers/ide stack internally and
> only present in any form on a few odd platforms where it got "borrowed"
> and hasn't yet been eliminated
>
> The absence of an IRQ is zero. A bus IRQ of zero is remapped by the OS.
OK, I was remembering some old issues with NO_IRQ. Next time I will read
the commit message more carefully. Sorry for the noise.
Wolfgang.
Uwe Kleine-König
>
> Dear Uwe
>
> > 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.
> (snip)
> > - if (!res || !irq) {
> > + if (!res || (int)irq <= 0) {
>
> Ohh..
> Thank you for checking.
>
> Hmm.. now I tried to check about platform_get_irq in Linux kernel.
> In my easy check, I can find a lot of drivers which are...
>
> o doesn't check irq value ex) request_irq(platform_get_irq(...))
> o checked irq but it have miss (?) ex) if (irq >= 0) OK
> if (irq) OK
> if (irq == -Exxx ) NG
For some drivers if (irq >= 0) is OK. if (irq) is wrong. if (irq ==
-Exxx ) IMHO isn't carefully enough.
> o checked irq but don't care zero ex) if (irq < 0) NG
Same as if (irq >= 0)
> ---- it use !irq ------------------------
>
> arch/avr32/mach-at32ap/extint.c :: 200
> drivers/media/video/mx1_camera.c :: 651
> drivers/media/video/sh_mobile_ceu_camera.c :: 1655
> drivers/misc/atmel-ssc.c :: 110
> drivers/net/can/at91_can.c :: 1071
> drivers/net/sni_82596.c :: 112
> drivers/pcmcia/bfin_cf_pcmcia.c :: 207
> drivers/spi/spi_imx.c :: 553
> drivers/spi/spi_mpc8xxx.c :: 885
> drivers/usb/gadget/fsl_udc_core.c :: 2303
Most of these are fixed in this series. I seem to have missed
drivers/misc/atmel-ssc.c
drivers/net/sni_82596.c
drivers/usb/gadget/fsl_udc_core.c
and I found sound/soc/sh/fsi.c and drivers/spi/spi_imx.c should already
be fixed in Linus' Tree.
I wait to see the result for the already existing patches. If they are
taken I might look for the others, too. If you want to prepare some
more patches, feel free to do it.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K�nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Uwe Kleine-König
On Wed, Dec 16, 2009 at 10:37:45PM +0300, Anton Vorontsov wrote:
> On Wed, Dec 16, 2009 at 08:18:39PM +0100, Uwe Kleine-K�nig wrote:
> [...]
> > > > I'm a bit annoyed as this is the third time[1] this month this irq0
> > > > discussion pops up for me. I think people see that irq0 is involved
> > > > somehow, start wailing and stop seeing the issues being fixed.
> > >
> > > For this particular driver, there is NO issue whatsoever. It is
> > > only used for PowerPC, which has VIRQ0 == invalid IRQ. And note
> > > that there still could be HWIRQ0 on PowerPC, but it is *never*
> > > mapped to VIRQ0.
> > 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;
is non-sense as !irq always evaluates to 0. If your argue that the
resources are right, then the logical consequence is to strip down
plat_mpc8xxx_spi_probe to just
struct spi_master *master;
master = mpc8xxx_spi_probe(&pdev->dev,
platform_get_resource(pdev, IORESOURCE_MEM, 0)
platform_get_irq(pdev, 0));
if (IS_ERR(master))
return PTR_ERR(master);
return 0;
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K�nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Anton Vorontsov
[...]
> > > 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 <avoro...@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...
Anton Vorontsov
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: cbouat...@gmail.com
irc://irc.freenode.net/bd2
David Miller
Date: Wed, 16 Dec 2009 19:58:38 +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.klein...@pengutronix.de>
>
> Signed-off-by: Wolfgang Grandegger <w...@grandegger.com>
Applied, thanks.
Uwe Kleine-König
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.klein...@pengutronix.de>
Cc: David Vrabel <dvr...@arcom.com>
Cc: Greg Kroah-Hartman <gre...@suse.de>
Cc: David Brownell <dbro...@users.sourceforge.net>
Cc: Grant Likely <grant....@secretlab.ca>
Cc: Kumar Gala <ga...@kernel.crashing.org>
Cc: Anton Vorontsov <avoro...@ru.mvista.com>
Cc: Andrew Morton <ak...@linux-foundation.org>
Cc: spi-deve...@lists.sourceforge.net
---
Hello,
as requested by Anton Vorontsov I made irq signed instead of casting to
int when comparing to zero.
Best regards
Uwe
drivers/spi/spi_mpc8xxx.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi_mpc8xxx.c b/drivers/spi/spi_mpc8xxx.c
index 1fb2a6e..08065fb 100644
--- a/drivers/spi/spi_mpc8xxx.c
+++ b/drivers/spi/spi_mpc8xxx.c
@@ -1328,7 +1328,7 @@ static struct of_platform_driver of_mpc8xxx_spi_driver = {
static int __devinit plat_mpc8xxx_spi_probe(struct platform_device *pdev)
{
struct resource *mem;
- unsigned int irq;
+ int irq;
struct spi_master *master;
if (!pdev->dev.platform_data)
@@ -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 (irq <= 0)
return -EINVAL;
master = mpc8xxx_spi_probe(&pdev->dev, mem, irq);
--
1.6.5.2
--
Mark Brown
> 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.
>
Applied, thanks.