* Re: + spi-imx-correct-check-for-platform_get_irq-failing.patch added to -mm tree
[not found] ` <20091209074533.GB8136-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
@ 2009-12-09 15:08 ` Grant Likely
[not found] ` <fa686aa40912090708g45879802l6cea7b401b1434e3-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 5+ messages in thread
From: Grant Likely @ 2009-12-09 15:08 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f,
mm-commits-u79uwXL29TY76Z2rM5mHXA, daniel-rDUAYElUppE,
spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b
(resend because I forgot to cc the mailing list)
2009/12/9 Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>:
> Hello Grant,
>
> On Tue, Dec 08, 2009 at 05:38:57PM -0700, Grant Likely wrote:
>> > diff -puN drivers/spi/spi_imx.c~spi-imx-correct-check-for-platform_get_irq-failing drivers/spi/spi_imx.c
>> > --- a/drivers/spi/spi_imx.c~spi-imx-correct-check-for-platform_get_irq-failing
>> > +++ a/drivers/spi/spi_imx.c
>> > @@ -554,7 +554,7 @@ static int __init spi_imx_probe(struct p
>> > }
>> >
>> > spi_imx->irq = platform_get_irq(pdev, 0);
>> > - if (!spi_imx->irq) {
>> > + if (spi_imx->irq < 0) {
>>
>> This changes the old behaviour. Is that what you intended? '<= 0' perhaps?
> Yes, the old check was wrong. What if the irq to use is 0? I thought
> the commit log to be understandable. platform_get_irq returns -ENXIO on
> error and an irq number on success. So 0 has to be interpreted as valid
> irq, not an error.
0 is not a valid IRQ
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
------------------------------------------------------------------------------
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: + spi-imx-correct-check-for-platform_get_irq-failing.patch added to -mm tree
[not found] ` <fa686aa40912090708g45879802l6cea7b401b1434e3-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2009-12-09 15:32 ` Uwe Kleine-König
[not found] ` <20091209153241.GB1389-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2009-12-09 17:55 ` Uwe Kleine-König
0 siblings, 2 replies; 5+ messages in thread
From: Uwe Kleine-König @ 2009-12-09 15:32 UTC (permalink / raw)
To: Grant Likely
Cc: dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f,
mm-commits-u79uwXL29TY76Z2rM5mHXA, daniel-rDUAYElUppE,
spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b
On Wed, Dec 09, 2009 at 08:08:19AM -0700, Grant Likely wrote:
> (resend because I forgot to cc the mailing list)
>
> 2009/12/9 Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>:
> > Hello Grant,
> >
> > On Tue, Dec 08, 2009 at 05:38:57PM -0700, Grant Likely wrote:
> >> > diff -puN drivers/spi/spi_imx.c~spi-imx-correct-check-for-platform_get_irq-failing drivers/spi/spi_imx.c
> >> > --- a/drivers/spi/spi_imx.c~spi-imx-correct-check-for-platform_get_irq-failing
> >> > +++ a/drivers/spi/spi_imx.c
> >> > @@ -554,7 +554,7 @@ static int __init spi_imx_probe(struct p
> >> > }
> >> >
> >> > spi_imx->irq = platform_get_irq(pdev, 0);
> >> > - if (!spi_imx->irq) {
> >> > + if (spi_imx->irq < 0) {
> >>
> >> This changes the old behaviour. Is that what you intended? '<= 0' perhaps?
> > Yes, the old check was wrong. What if the irq to use is 0? I thought
> > the commit log to be understandable. platform_get_irq returns -ENXIO on
> > error and an irq number on success. So 0 has to be interpreted as valid
> > irq, not an error.
>
> 0 is not a valid IRQ
Hmm, on my x86 I have:
$ grep '\<0:' /proc/interrupts
0: 24330 IO-APIC-edge timer
arm/davinci starts at 0, too. As does arm/ns9xxx. arm/pxa seems to
start at 1. realview starts at 1, too. So four out of five make are
wrong? Seems like a big area for cleanup.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
------------------------------------------------------------------------------
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: + spi-imx-correct-check-for-platform_get_irq-failing.patch added to -mm tree
[not found] ` <20091209153241.GB1389-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
@ 2009-12-09 17:23 ` Grant Likely
0 siblings, 0 replies; 5+ messages in thread
From: Grant Likely @ 2009-12-09 17:23 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f,
mm-commits-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman,
daniel-rDUAYElUppE,
spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b, David Vrabel
2009/12/9 Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>:
> On Wed, Dec 09, 2009 at 08:08:19AM -0700, Grant Likely wrote:
>> >> This changes the old behaviour. Is that what you intended? '<= 0' perhaps?
>> > Yes, the old check was wrong. What if the irq to use is 0? I thought
>> > the commit log to be understandable. platform_get_irq returns -ENXIO on
>> > error and an irq number on success. So 0 has to be interpreted as valid
>> > irq, not an error.
>>
>> 0 is not a valid IRQ
> Hmm, on my x86 I have:
>
> $ grep '\<0:' /proc/interrupts
> 0: 24330 IO-APIC-edge timer
>
> arm/davinci starts at 0, too. As does arm/ns9xxx. arm/pxa seems to
> start at 1. realview starts at 1, too. So four out of five make are
> wrong? Seems like a big area for cleanup.
As discussed on IRC, regardless of some of the core & arch use cases.
As far as drivers are concerned, 0 is not a valid IRQ. Linus'
comments here were referenced: http://lkml.org/lkml/2005/11/21/221
The return of -ENXIO was added in commit 305b3228, and it is
definitely in the workaround category. Damn. And a quick grep show a
lot of broken drivers because of it. (variations on the common 'if
(!irq)' pattern abound as well as a large number of the variant 'if
(irq < 0)').
Well. Though I don't like it, I will take the patch as testing for <=
0 instead of < 0. At least that doesn't create new situations of 0
being used as a valid IRQ.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
------------------------------------------------------------------------------
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: + spi-imx-correct-check-for-platform_get_irq-failing.patch added to -mm tree
2009-12-09 15:32 ` Uwe Kleine-König
[not found] ` <20091209153241.GB1389-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
@ 2009-12-09 17:55 ` Uwe Kleine-König
2009-12-09 18:00 ` Grant Likely
1 sibling, 1 reply; 5+ messages in thread
From: Uwe Kleine-König @ 2009-12-09 17:55 UTC (permalink / raw)
To: Grant Likely
Cc: akpm, mm-commits, daniel, dbrownell, s.hauer, spi-devel-general,
linux-kernel
On Wed, Dec 09, 2009 at 04:32:41PM +0100, Uwe Kleine-König wrote:
> On Wed, Dec 09, 2009 at 08:08:19AM -0700, Grant Likely wrote:
> > (resend because I forgot to cc the mailing list)
> >
> > 2009/12/9 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > > Hello Grant,
> > >
> > > On Tue, Dec 08, 2009 at 05:38:57PM -0700, Grant Likely wrote:
> > >> > diff -puN drivers/spi/spi_imx.c~spi-imx-correct-check-for-platform_get_irq-failing drivers/spi/spi_imx.c
> > >> > --- a/drivers/spi/spi_imx.c~spi-imx-correct-check-for-platform_get_irq-failing
> > >> > +++ a/drivers/spi/spi_imx.c
> > >> > @@ -554,7 +554,7 @@ static int __init spi_imx_probe(struct p
> > >> > }
> > >> >
> > >> > spi_imx->irq = platform_get_irq(pdev, 0);
> > >> > - if (!spi_imx->irq) {
> > >> > + if (spi_imx->irq < 0) {
> > >>
> > >> This changes the old behaviour. Is that what you intended? '<= 0' perhaps?
> > > Yes, the old check was wrong. What if the irq to use is 0? I thought
> > > the commit log to be understandable. platform_get_irq returns -ENXIO on
> > > error and an irq number on success. So 0 has to be interpreted as valid
> > > irq, not an error.
> >
> > 0 is not a valid IRQ
> Hmm, on my x86 I have:
>
> $ grep '\<0:' /proc/interrupts
> 0: 24330 IO-APIC-edge timer
>
> arm/davinci starts at 0, too. As does arm/ns9xxx. arm/pxa seems to
> start at 1. realview starts at 1, too. So four out of five make are
> wrong? Seems like a big area for cleanup.
I've read a bit and I think the best for a driver writer (i.e. the role
I have when changing drivers/spi/spi_imx.c) is to accept what
platform_get_irq returns to me. If the platform specified
struct resource mydevicesresources[] = {
...
{
.start = 0,
.end = 0,
.flags = IORESOURCE_IRQ,
},
...
};
then the best thing to do is to take irq0, isn't it. So as
platform_get_irq is implemented as
int platform_get_irq(struct platform_device *dev, unsigned int num)
{
struct resource *r = platform_get_resource(dev, IORESOURCE_IRQ, num);
return r ? r->start : -ENXIO;
}
testing for <0 seems right to me.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: + spi-imx-correct-check-for-platform_get_irq-failing.patch added to -mm tree
2009-12-09 17:55 ` Uwe Kleine-König
@ 2009-12-09 18:00 ` Grant Likely
0 siblings, 0 replies; 5+ messages in thread
From: Grant Likely @ 2009-12-09 18:00 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: akpm, mm-commits, daniel, dbrownell, s.hauer, spi-devel-general,
linux-kernel
2009/12/9 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> On Wed, Dec 09, 2009 at 04:32:41PM +0100, Uwe Kleine-König wrote:
>> On Wed, Dec 09, 2009 at 08:08:19AM -0700, Grant Likely wrote:
>> > (resend because I forgot to cc the mailing list)
>> >
>> > 2009/12/9 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> > > Hello Grant,
>> > >
>> > > On Tue, Dec 08, 2009 at 05:38:57PM -0700, Grant Likely wrote:
>> > >> > diff -puN drivers/spi/spi_imx.c~spi-imx-correct-check-for-platform_get_irq-failing drivers/spi/spi_imx.c
>> > >> > --- a/drivers/spi/spi_imx.c~spi-imx-correct-check-for-platform_get_irq-failing
>> > >> > +++ a/drivers/spi/spi_imx.c
>> > >> > @@ -554,7 +554,7 @@ static int __init spi_imx_probe(struct p
>> > >> > }
>> > >> >
>> > >> > spi_imx->irq = platform_get_irq(pdev, 0);
>> > >> > - if (!spi_imx->irq) {
>> > >> > + if (spi_imx->irq < 0) {
>> > >>
>> > >> This changes the old behaviour. Is that what you intended? '<= 0' perhaps?
>> > > Yes, the old check was wrong. What if the irq to use is 0? I thought
>> > > the commit log to be understandable. platform_get_irq returns -ENXIO on
>> > > error and an irq number on success. So 0 has to be interpreted as valid
>> > > irq, not an error.
>> >
>> > 0 is not a valid IRQ
>> Hmm, on my x86 I have:
>>
>> $ grep '\<0:' /proc/interrupts
>> 0: 24330 IO-APIC-edge timer
>>
>> arm/davinci starts at 0, too. As does arm/ns9xxx. arm/pxa seems to
>> start at 1. realview starts at 1, too. So four out of five make are
>> wrong? Seems like a big area for cleanup.
> I've read a bit and I think the best for a driver writer (i.e. the role
> I have when changing drivers/spi/spi_imx.c) is to accept what
> platform_get_irq returns to me. If the platform specified
>
> struct resource mydevicesresources[] = {
> ...
> {
> .start = 0,
> .end = 0,
> .flags = IORESOURCE_IRQ,
> },
> ...
> };
>
> then the best thing to do is to take irq0, isn't it. So as
> platform_get_irq is implemented as
>
> int platform_get_irq(struct platform_device *dev, unsigned int num)
> {
> struct resource *r = platform_get_resource(dev, IORESOURCE_IRQ, num);
>
> return r ? r->start : -ENXIO;
> }
>
> testing for <0 seems right to me.
Regardless. I won't accept that change for a theoretical use case.
In the general case I'll maintain the pattern that irq 0 is invalid
unless it is the only way to get around a real problem.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-12-09 18:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <200912082330.nB8NU4IO016215@imap1.linux-foundation.org>
[not found] ` <fa686aa40912081638m734e97c3r2c3f412898d293b@mail.gmail.com>
[not found] ` <20091209074533.GB8136@pengutronix.de>
[not found] ` <20091209074533.GB8136-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2009-12-09 15:08 ` + spi-imx-correct-check-for-platform_get_irq-failing.patch added to -mm tree Grant Likely
[not found] ` <fa686aa40912090708g45879802l6cea7b401b1434e3-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2009-12-09 15:32 ` Uwe Kleine-König
[not found] ` <20091209153241.GB1389-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2009-12-09 17:23 ` Grant Likely
2009-12-09 17:55 ` Uwe Kleine-König
2009-12-09 18:00 ` Grant Likely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).