platform_get_irq returns -ENXIO on failure, so !int_irq was probably
always true. Better use (int)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 <[email protected]>
Cc: David Vrabel <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Haavard Skinnemoen <[email protected]>
---
arch/avr32/mach-at32ap/extint.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/avr32/mach-at32ap/extint.c b/arch/avr32/mach-at32ap/extint.c
index 310477b..033f693 100644
--- a/arch/avr32/mach-at32ap/extint.c
+++ b/arch/avr32/mach-at32ap/extint.c
@@ -198,7 +198,7 @@ static int __init eic_probe(struct platform_device *pdev)
regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
int_irq = platform_get_irq(pdev, 0);
- if (!regs || !int_irq) {
+ if (!regs || (int)int_irq <= 0) {
dev_dbg(&pdev->dev, "missing regs and/or irq resource\n");
return -ENXIO;
}
--
1.6.5.2
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 <[email protected]>
Cc: David Vrabel <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Guennadi Liakhovetski <[email protected]>
Cc: Antonio Ospite <[email protected]>
Cc: Paulius Zaleckas <[email protected]>
Cc: [email protected]
---
drivers/media/video/mx1_camera.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/mx1_camera.c b/drivers/media/video/mx1_camera.c
index 7280229..f7a472f 100644
--- a/drivers/media/video/mx1_camera.c
+++ b/drivers/media/video/mx1_camera.c
@@ -650,7 +650,7 @@ static int __init mx1_camera_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) {
err = -ENODEV;
goto exit;
}
--
1.6.5.2
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 <[email protected]>
Cc: David Vrabel <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Guennadi Liakhovetski <[email protected]>
Cc: Magnus Damm <[email protected]>
Cc: Kuninori Morimoto <[email protected]>
Cc: Paul Mundt <[email protected]>
Cc: [email protected]
---
drivers/media/video/sh_mobile_ceu_camera.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/sh_mobile_ceu_camera.c b/drivers/media/video/sh_mobile_ceu_camera.c
index 961e448..f18e674 100644
--- a/drivers/media/video/sh_mobile_ceu_camera.c
+++ b/drivers/media/video/sh_mobile_ceu_camera.c
@@ -1709,7 +1709,7 @@ static int __devinit sh_mobile_ceu_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 CEU platform resources.\n");
err = -ENODEV;
goto exit;
--
1.6.5.2
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 <[email protected]>
Cc: David Vrabel <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Urs Thuermann <[email protected]>
Cc: Oliver Hartkopp <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Wolfgang Grandegger <[email protected]>
Cc: Kurt Van Dijck <[email protected]>
Cc: [email protected]
---
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;
}
--
1.6.5.2
platform_get_irq returns -ENXIO on failure, so !irq was probably
always true. Better use 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 <[email protected]>
Cc: David Vrabel <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>
Cc: [email protected]
---
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 */
--
1.6.5.2
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 <[email protected]>
Cc: David Vrabel <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: David Brownell <[email protected]>
Cc: Grant Likely <[email protected]>
Cc: Kumar Gala <[email protected]>
Cc: Anton Vorontsov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
---
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
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 <[email protected]>
Cc: David Vrabel <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Liam Girdwood <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Jaroslav Kysela <[email protected]>
Cc: Takashi Iwai <[email protected]>
Cc: Kuninori Morimoto <[email protected]>
Cc: Paul Mundt <[email protected]>
Cc: [email protected]
---
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;
--
1.6.5.2
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.
Wolfgang.
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 <[email protected]>
> ---
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: [email protected]
irc://irc.freenode.net/bd2
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:
- 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/ |
> + 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.
Alan
Hello,
[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 <[email protected]>
> > ---
>
> 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/ |
On Wed, Dec 16, 2009 at 06:49:04PM +0100, Uwe Kleine-König wrote:
[...]
> > 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: [email protected]
irc://irc.freenode.net/bd2
Uwe Kleine-K?nig wrote:
> 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
Uwe Kleine-K?nig wrote:
> 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.
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 <[email protected]>
> Cc: David Vrabel <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Urs Thuermann <[email protected]>
> Cc: Oliver Hartkopp <[email protected]>
> Cc: David S. Miller <[email protected]>
> Cc: Wolfgang Grandegger <[email protected]>
> Cc: Kurt Van Dijck <[email protected]>
> Cc: [email protected]
Signed-off-by: Wolfgang Grandegger <[email protected]>
> ---
> 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.
Hi Anton,
On Wed, Dec 16, 2009 at 09:20:34PM +0300, Anton Vorontsov wrote:
> On Wed, Dec 16, 2009 at 06:49:04PM +0100, Uwe Kleine-K?nig wrote:
> [...]
> > > 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.
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/ |
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.
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Wed, Dec 16, 2009 at 10:37:45PM +0300, Anton Vorontsov wrote:
[...]
> 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.
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
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
Alan Cox wrote:
>> + 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.
On Thu, Dec 17, 2009 at 10:42:25AM +0900, Kuninori Morimoto wrote:
>
> 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(...))
This is probably worth fixing.
> 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/ |
Hi Anton,
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/ |
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 <[email protected]>
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...
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 <[email protected]>
> Cc: David Vrabel <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: David Brownell <[email protected]>
> Cc: Grant Likely <[email protected]>
> Cc: Kumar Gala <[email protected]>
> Cc: Anton Vorontsov <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: [email protected]
> ---
> 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: [email protected]
irc://irc.freenode.net/bd2
From: Wolfgang Grandegger <[email protected]>
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 <[email protected]>
>
> Signed-off-by: Wolfgang Grandegger <[email protected]>
Applied, thanks.
platform_get_irq returns -ENXIO on failure, so !irq was probably
always true. Make irq a signed variable and compare 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 <[email protected]>
Cc: David Vrabel <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: David Brownell <[email protected]>
Cc: Grant Likely <[email protected]>
Cc: Kumar Gala <[email protected]>
Cc: Anton Vorontsov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
---
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
On Wed, Dec 16, 2009 at 05:10:09PM +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 <[email protected]>
Applied, thanks.
platform_get_irq returns -ENXIO on failure, so !int_irq was probably
always true. Better use (int)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 <[email protected]>
Cc: David Vrabel <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Haavard Skinnemoen <[email protected]>
---
arch/avr32/mach-at32ap/extint.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/avr32/mach-at32ap/extint.c b/arch/avr32/mach-at32ap/extint.c
index 310477b..033f693 100644
--- a/arch/avr32/mach-at32ap/extint.c
+++ b/arch/avr32/mach-at32ap/extint.c
@@ -198,7 +198,7 @@ static int __init eic_probe(struct platform_device *pdev)
regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
int_irq = platform_get_irq(pdev, 0);
- if (!regs || !int_irq) {
+ if (!regs || (int)int_irq <= 0) {
dev_dbg(&pdev->dev, "missing regs and/or irq resource\n");
return -ENXIO;
}
--
1.6.6
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 <[email protected]>
Cc: David Vrabel <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Guennadi Liakhovetski <[email protected]>
Cc: Antonio Ospite <[email protected]>
Cc: Paulius Zaleckas <[email protected]>
Cc: [email protected]
---
drivers/media/video/mx1_camera.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/mx1_camera.c b/drivers/media/video/mx1_camera.c
index 2ba14fb..c167cc3 100644
--- a/drivers/media/video/mx1_camera.c
+++ b/drivers/media/video/mx1_camera.c
@@ -718,7 +718,7 @@ static int __init mx1_camera_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) {
err = -ENODEV;
goto exit;
}
--
1.6.6
platform_get_irq returns -ENXIO on failure, so !irq was probably
always true. Make irq a signed variable and compare 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 <[email protected]>
Cc: David Vrabel <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: David Brownell <[email protected]>
Cc: Grant Likely <[email protected]>
Cc: Kumar Gala <[email protected]>
Cc: Anton Vorontsov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
---
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.6
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 <[email protected]>
Cc: David Vrabel <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Guennadi Liakhovetski <[email protected]>
Cc: Magnus Damm <[email protected]>
Cc: Kuninori Morimoto <[email protected]>
Cc: Paul Mundt <[email protected]>
Cc: [email protected]
---
drivers/media/video/sh_mobile_ceu_camera.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/sh_mobile_ceu_camera.c b/drivers/media/video/sh_mobile_ceu_camera.c
index d69363f..f09c714 100644
--- a/drivers/media/video/sh_mobile_ceu_camera.c
+++ b/drivers/media/video/sh_mobile_ceu_camera.c
@@ -1827,7 +1827,7 @@ static int __devinit sh_mobile_ceu_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 CEU platform resources.\n");
err = -ENODEV;
goto exit;
--
1.6.6
platform_get_irq returns -ENXIO on failure, so !irq was probably
always true. Better use 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 <[email protected]>
Cc: David Vrabel <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>
Cc: [email protected]
---
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 */
--
1.6.6
On Wed, Jan 13, 2010 at 12:05:46PM +0100, Uwe Kleine-König wrote:
> platform_get_irq returns -ENXIO on failure, so !irq was probably
> always true. Make irq a signed variable and compare 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 <[email protected]>
> Cc: David Vrabel <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: David Brownell <[email protected]>
> Cc: Grant Likely <[email protected]>
> Cc: Kumar Gala <[email protected]>
> Cc: Anton Vorontsov <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: [email protected]
> ---
Acked-by: Anton Vorontsov <[email protected]>
Thanks!
Hi Uwe
Your patches have been included in my pull request of 10.01.2010:
http://www.spinics.net/lists/linux-media/msg14449.html
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
On Tue, 2010-10-19 at 08:45 +0200, Uwe Kleine-König wrote:
> On Wed, Dec 16, 2009 at 05:10:03PM +0100, Uwe Kleine-König wrote:
> > platform_get_irq returns -ENXIO on failure, so !int_irq was probably
> > always true. Better use (int)int_irq <= 0. Note that a return value of
> > zero is still handled as error even though this could mean irq0.
Indeed, but external interrupts are numbered after the internal
interrupt lines, so in practice this does not happen. At least for now
with the AP700X series.
> > This is a followup to 305b3228f9ff4d59f49e6d34a7034d44ee8ce2f0 that
> > changed the return value of platform_get_irq from 0 to -ENXIO on error.
Thanks for this fix.
> > Signed-off-by: Uwe Kleine-König <[email protected]>
> > Cc: David Vrabel <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Cc: Haavard Skinnemoen <[email protected]>
Acked-by: Hans-Christian Egtvedt <[email protected]>
<snipp>
--
Hans-Christian Egtvedt
Hello Hans-Christian,
On Mon, Nov 01, 2010 at 10:58:03AM +0100, Hans-Christian Egtvedt wrote:
> On Tue, 2010-10-19 at 08:45 +0200, Uwe Kleine-K?nig wrote:
> > On Wed, Dec 16, 2009 at 05:10:03PM +0100, Uwe Kleine-K?nig wrote:
> > > platform_get_irq returns -ENXIO on failure, so !int_irq was probably
> > > always true. Better use (int)int_irq <= 0. Note that a return value of
> > > zero is still handled as error even though this could mean irq0.
>
> Indeed, but external interrupts are numbered after the internal
> interrupt lines, so in practice this does not happen. At least for now
> with the AP700X series.
>
> > > This is a followup to 305b3228f9ff4d59f49e6d34a7034d44ee8ce2f0 that
> > > changed the return value of platform_get_irq from 0 to -ENXIO on error.
>
> Thanks for this fix.
>
> > > Signed-off-by: Uwe Kleine-K?nig <[email protected]>
> > > Cc: David Vrabel <[email protected]>
> > > Cc: Greg Kroah-Hartman <[email protected]>
> > > Cc: Haavard Skinnemoen <[email protected]>
>
> Acked-by: Hans-Christian Egtvedt <[email protected]>
I thought this to go via the avr32 tree. You "only" acked, so what tree
do you consider here?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Tue, 2010-11-02 at 10:15 +0100, Uwe Kleine-König wrote:
> Hello Hans-Christian,
>
> On Mon, Nov 01, 2010 at 10:58:03AM +0100, Hans-Christian Egtvedt wrote:
> > On Tue, 2010-10-19 at 08:45 +0200, Uwe Kleine-König wrote:
> > > On Wed, Dec 16, 2009 at 05:10:03PM +0100, Uwe Kleine-König wrote:
> > > > platform_get_irq returns -ENXIO on failure, so !int_irq was probably
> > > > always true. Better use (int)int_irq <= 0. Note that a return value of
> > > > zero is still handled as error even though this could mean irq0.
> >
> > Indeed, but external interrupts are numbered after the internal
> > interrupt lines, so in practice this does not happen. At least for now
> > with the AP700X series.
> >
> > > > This is a followup to 305b3228f9ff4d59f49e6d34a7034d44ee8ce2f0 that
> > > > changed the return value of platform_get_irq from 0 to -ENXIO on error.
> >
> > Thanks for this fix.
> >
> > > > Signed-off-by: Uwe Kleine-König <[email protected]>
> > > > Cc: David Vrabel <[email protected]>
> > > > Cc: Greg Kroah-Hartman <[email protected]>
> > > > Cc: Haavard Skinnemoen <[email protected]>
> >
> > Acked-by: Hans-Christian Egtvedt <[email protected]>
> I thought this to go via the avr32 tree. You "only" acked, so what tree
> do you consider here?
Right now there isn't any AVR32 tree, since I have yet to receive an
answer to my kernel.org account request. Could you push it through Linus
tree?
--
Hans-Christian Egtvedt
platform_get_irq returns -ENXIO on failure, so !int_irq was probably
always true. Better use (int)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.
Acked-by: Hans-Christian Egtvedt <[email protected]>
Signed-off-by: Uwe Kleine-König <[email protected]>
---
Hello,
Hans-Christian Egtvedt asked to push this patch via Linus as there is no avr32
tree.
@Andrew: do you take it?
Best regards
Uwe
arch/avr32/mach-at32ap/extint.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/avr32/mach-at32ap/extint.c b/arch/avr32/mach-at32ap/extint.c
index e9d1205..dae8682 100644
--- a/arch/avr32/mach-at32ap/extint.c
+++ b/arch/avr32/mach-at32ap/extint.c
@@ -199,7 +199,7 @@ static int __init eic_probe(struct platform_device *pdev)
regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
int_irq = platform_get_irq(pdev, 0);
- if (!regs || !int_irq) {
+ if (!regs || (int)int_irq <= 0) {
dev_dbg(&pdev->dev, "missing regs and/or irq resource\n");
return -ENXIO;
}
--
1.7.2.3
On Wed, 9 Feb 2011 11:28:04 +0100
Uwe Kleine-K?nig <[email protected]> wrote:
> platform_get_irq returns -ENXIO on failure, so !int_irq was probably
> always true. Better use (int)int_irq <= 0. Note that a return value of
> zero is still handled as error even though this could mean irq0.
We don't have an IRQ 0 in Linux so that's fine.
On Wed, 2011-02-09 at 11:28 +0100, Uwe Kleine-König wrote:
> platform_get_irq returns -ENXIO on failure, so !int_irq was probably
> always true. Better use (int)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.
>
> Acked-by: Hans-Christian Egtvedt <[email protected]>
> Signed-off-by: Uwe Kleine-König <[email protected]>
> ---
> Hello,
>
> Hans-Christian Egtvedt asked to push this patch via Linus as there is no avr32
> tree.
>
> @Andrew: do you take it?
Actually, now I have a tree for AVR32 on git.kernel.org, but I have no
other updates lined up for 2.6.38. Will you Andrew add it to your
series, or should I push this one through my git tree?
--
Hans-Christian Egtvedt
On Wed, Feb 09, 2011 at 01:28:52PM +0100, Hans-Christian Egtvedt wrote:
> On Wed, 2011-02-09 at 11:28 +0100, Uwe Kleine-K?nig wrote:
> > platform_get_irq returns -ENXIO on failure, so !int_irq was probably
> > always true. Better use (int)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.
> >
> > Acked-by: Hans-Christian Egtvedt <[email protected]>
> > Signed-off-by: Uwe Kleine-K?nig <[email protected]>
> > ---
> > Hello,
> >
> > Hans-Christian Egtvedt asked to push this patch via Linus as there is no avr32
> > tree.
> >
> > @Andrew: do you take it?
>
> Actually, now I have a tree for AVR32 on git.kernel.org, but I have no
> other updates lined up for 2.6.38. Will you Andrew add it to your
> series, or should I push this one through my git tree?
I don't consider it that critical. So for me getting it in the avr32
tree now and then getting it merged for .39 would be OK for me.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Wed, 2011-02-09 at 13:44 +0100, Uwe Kleine-König wrote:
> On Wed, Feb 09, 2011 at 01:28:52PM +0100, Hans-Christian Egtvedt wrote:
> > On Wed, 2011-02-09 at 11:28 +0100, Uwe Kleine-König wrote:
> > > platform_get_irq returns -ENXIO on failure, so !int_irq was probably
> > > always true. Better use (int)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.
> > >
> > > Acked-by: Hans-Christian Egtvedt <[email protected]>
> > > Signed-off-by: Uwe Kleine-König <[email protected]>
> > > ---
> > > Hello,
> > >
> > > Hans-Christian Egtvedt asked to push this patch via Linus as there is no avr32
> > > tree.
> > >
> > > @Andrew: do you take it?
> >
> > Actually, now I have a tree for AVR32 on git.kernel.org, but I have no
> > other updates lined up for 2.6.38. Will you Andrew add it to your
> > series, or should I push this one through my git tree?
> I don't consider it that critical. So for me getting it in the avr32
> tree now and then getting it merged for .39 would be OK for me.
Okay, then I'll schedule it for the next release.
--
Hans-Christian Egtvedt