[PATCH] pata_of_platform: fix no irq handling

[PATCH] pata_of_platform: fix no irq handling

[Anton Vorontsov]

When no irq specified the pata_of_platform fills the irq_res with -1,
which is wrong to do for two reasons:

1. By definition, 'no irq' should be IRQ 0, not some negative integer;
2. pata_platform checks for irq_res.start > 0, but since irq_res.start
   is unsigned type, the check will be true for `-1'.

Reported-by: Steven A. Falco <sfalco@harris.com>
Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---

Resending again...

 drivers/ata/pata_of_platform.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
index 408da30..1f18ad9 100644
--- a/drivers/ata/pata_of_platform.c
+++ b/drivers/ata/pata_of_platform.c
@@ -52,7 +52,7 @@ static int __devinit pata_of_platform_probe(struct of_device *ofdev,
 
 	ret = of_irq_to_resource(dn, 0, &irq_res);
 	if (ret == NO_IRQ)
-		irq_res.start = irq_res.end = -1;
+		irq_res.start = irq_res.end = 0;
 	else
 		irq_res.flags = 0;
 
-- 
1.5.6.3

Re: [PATCH] pata_of_platform: fix no irq handling

[Matt Sealey]

There is a simple problem with the patch which is that an "IRQ 0" can and does
actually exist on a bunch of platforms, at least to the best of my knowledge.

Checking for -1 (which means for definite, no irq at all, because it is
totally unambiguous, as a -1 IRQ numbering is "impossible") is more correct.

The problem is the check against an unsigned value for interrupts (is there
any reason why you would need 4 billion interrupts possible instead of just
2 billion?) although I must say, the patch will work, and probably 99.9999999%
of people will never see a problem with it :)

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Anton Vorontsov wrote:
> When no irq specified the pata_of_platform fills the irq_res with -1,
> which is wrong to do for two reasons:
> 
> 1. By definition, 'no irq' should be IRQ 0, not some negative integer;
> 2. pata_platform checks for irq_res.start > 0, but since irq_res.start
>    is unsigned type, the check will be true for `-1'.
> 
> Reported-by: Steven A. Falco <sfalco@harris.com>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> 
> Resending again...
> 
>  drivers/ata/pata_of_platform.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
> index 408da30..1f18ad9 100644
> --- a/drivers/ata/pata_of_platform.c
> +++ b/drivers/ata/pata_of_platform.c
> @@ -52,7 +52,7 @@ static int __devinit pata_of_platform_probe(struct of_device *ofdev,
>  
>  	ret = of_irq_to_resource(dn, 0, &irq_res);
>  	if (ret == NO_IRQ)
> -		irq_res.start = irq_res.end = -1;
> +		irq_res.start = irq_res.end = 0;
>  	else
>  		irq_res.flags = 0;
>  

Re: [PATCH] pata_of_platform: fix no irq handling

[Anton Vorontsov]

On Mon, Oct 06, 2008 at 03:41:19PM -0500, Matt Sealey wrote:
> There is a simple problem with the patch which is that an "IRQ 0" can and does
> actually exist on a bunch of platforms, at least to the best of my knowledge.
> 
> Checking for -1 (which means for definite, no irq at all, because it is
> totally unambiguous, as a -1 IRQ numbering is "impossible") is more correct.

This was discussed years ago.

http://lkml.org/lkml/2005/11/22/159
http://lkml.org/lkml/2005/11/22/227

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] pata_of_platform: fix no irq handling

[Tejun Heo]

Anton Vorontsov wrote:
> On Mon, Oct 06, 2008 at 03:41:19PM -0500, Matt Sealey wrote:
>> There is a simple problem with the patch which is that an "IRQ 0" can and does
>> actually exist on a bunch of platforms, at least to the best of my knowledge.
>>
>> Checking for -1 (which means for definite, no irq at all, because it is
>> totally unambiguous, as a -1 IRQ numbering is "impossible") is more correct.
> 
> This was discussed years ago.
> 
> http://lkml.org/lkml/2005/11/22/159
> http://lkml.org/lkml/2005/11/22/227
> 

Would this break any existing platforms?  If so, can those be fixed
together or does it become a much bigger problem that way?

Thanks.

-- 
tejun

Re: [PATCH] pata_of_platform: fix no irq handling

[Wang Jian]

Tejun Heo wrote:
> Anton Vorontsov wrote:
>> On Mon, Oct 06, 2008 at 03:41:19PM -0500, Matt Sealey wrote:
>>> There is a simple problem with the patch which is that an "IRQ 0" can and does
>>> actually exist on a bunch of platforms, at least to the best of my knowledge.
>>>
>>> Checking for -1 (which means for definite, no irq at all, because it is
>>> totally unambiguous, as a -1 IRQ numbering is "impossible") is more correct.
>> This was discussed years ago.
>>
>> http://lkml.org/lkml/2005/11/22/159
>> http://lkml.org/lkml/2005/11/22/227
>>
> 
> Would this break any existing platforms?  If so, can those be fixed
> together or does it become a much bigger problem that way?
> 

Pata_of_platform stacks upon pata_platform. This patch fixes problem
concerning definition of "no irq" without touch any other place.  So
far I can't see any new problem.

Re: [PATCH] pata_of_platform: fix no irq handling

[Anton Vorontsov]

On Tue, Oct 07, 2008 at 10:30:59AM +0900, Tejun Heo wrote:
> Anton Vorontsov wrote:
> > On Mon, Oct 06, 2008 at 03:41:19PM -0500, Matt Sealey wrote:
> >> There is a simple problem with the patch which is that an "IRQ 0" can and does
> >> actually exist on a bunch of platforms, at least to the best of my knowledge.
> >>
> >> Checking for -1 (which means for definite, no irq at all, because it is
> >> totally unambiguous, as a -1 IRQ numbering is "impossible") is more correct.
> > 
> > This was discussed years ago.
> > 
> > http://lkml.org/lkml/2005/11/22/159
> > http://lkml.org/lkml/2005/11/22/227
> > 
> 
> Would this break any existing platforms?

Nope.

The driver is only available for PPC platforms.

As time goes by one can change `depend on PPC_OF' to just `depends on
OF', so that the driver will be also available for SPARC. And still
it will work, because SPARC also understands VIRQ0 as invalid VIRQ.


Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] pata_of_platform: fix no irq handling

[Benjamin Herrenschmidt]

On Tue, 2008-10-07 at 13:26 +0400, Anton Vorontsov wrote:
> On Tue, Oct 07, 2008 at 10:30:59AM +0900, Tejun Heo wrote:
> > Anton Vorontsov wrote:
> > > On Mon, Oct 06, 2008 at 03:41:19PM -0500, Matt Sealey wrote:
> > >> There is a simple problem with the patch which is that an "IRQ 0" can and does
> > >> actually exist on a bunch of platforms, at least to the best of my knowledge.
> > >>
> > >> Checking for -1 (which means for definite, no irq at all, because it is
> > >> totally unambiguous, as a -1 IRQ numbering is "impossible") is more correct.
> > > 
> > > This was discussed years ago.
> > > 
> > > http://lkml.org/lkml/2005/11/22/159
> > > http://lkml.org/lkml/2005/11/22/227
> > > 
> > 
> > Would this break any existing platforms?
> 
> Nope.
> 
> The driver is only available for PPC platforms.
> 
> As time goes by one can change `depend on PPC_OF' to just `depends on
> OF', so that the driver will be also available for SPARC. And still
> it will work, because SPARC also understands VIRQ0 as invalid VIRQ.
> 

Yup, I agree. I'll pick it up in my next batch.

Cheers,
Ben.

Re: [PATCH] pata_of_platform: fix no irq handling

[Alan Cox]

> > This was discussed years ago.
> > 
> > http://lkml.org/lkml/2005/11/22/159
> > http://lkml.org/lkml/2005/11/22/227
> > 
> 
> Would this break any existing platforms?  If so, can those be fixed
> together or does it become a much bigger problem that way?

Zero means no IRQ. Any platform with bits of code left over exposing IRQ
0 is already not supported by lots of driver code including libata.

As IRQs are unsigned using -1 is asking for trouble

Re: [PATCH] pata_of_platform: fix no irq handling

[David Woodhouse]

On Tue, 2008-10-07 at 10:37 +0100, Alan Cox wrote:
> Zero means no IRQ. Any platform with bits of code left over exposing IRQ
> 0 is already not supported by lots of driver code including libata.

...and must implement some kind of interrupt remapping crap just to work
around this bogus design decision.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] pata_of_platform: fix no irq handling

[Alan Cox]

On Wed, 08 Oct 2008 09:40:54 +0100
David Woodhouse <dwmw2@infradead.org> wrote:

> On Tue, 2008-10-07 at 10:37 +0100, Alan Cox wrote:
> > Zero means no IRQ. Any platform with bits of code left over exposing IRQ
> > 0 is already not supported by lots of driver code including libata.
> 
> ...and must implement some kind of interrupt remapping crap just to work
> around this bogus design decision.

I'll leave you to argue with Linus about that, but since that was the
decision back in 2005 (for good C reasons) we can safely rely on it.

Re: [PATCH] pata_of_platform: fix no irq handling

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1093 bytes --]

On Wed, 8 Oct 2008, Alan Cox wrote:
> On Wed, 08 Oct 2008 09:40:54 +0100
> David Woodhouse <dwmw2@infradead.org> wrote:
> 
> > On Tue, 2008-10-07 at 10:37 +0100, Alan Cox wrote:
> > > Zero means no IRQ. Any platform with bits of code left over exposing IRQ
> > > 0 is already not supported by lots of driver code including libata.
> > 
> > ...and must implement some kind of interrupt remapping crap just to work
> > around this bogus design decision.
> 
> I'll leave you to argue with Linus about that, but since that was the
> decision back in 2005 (for good C reasons) we can safely rely on it.

`git grep NO_IRQ include arch/*/include' is still quite enlightening...

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010

Re: [PATCH] pata_of_platform: fix no irq handling

[Alan Cox]

> > I'll leave you to argue with Linus about that, but since that was the
> > decision back in 2005 (for good C reasons) we can safely rely on it.
> 
> `git grep NO_IRQ include arch/*/include' is still quite enlightening...

Good guide to platform code we should delete really

Re: [PATCH] pata_of_platform: fix no irq handling

[Paul Mundt]

On Wed, Oct 08, 2008 at 11:59:59AM +0200, Geert Uytterhoeven wrote:
> On Wed, 8 Oct 2008, Alan Cox wrote:
> > On Wed, 08 Oct 2008 09:40:54 +0100
> > David Woodhouse <dwmw2@infradead.org> wrote:
> > 
> > > On Tue, 2008-10-07 at 10:37 +0100, Alan Cox wrote:
> > > > Zero means no IRQ. Any platform with bits of code left over exposing IRQ
> > > > 0 is already not supported by lots of driver code including libata.
> > > 
> > > ...and must implement some kind of interrupt remapping crap just to work
> > > around this bogus design decision.
> > 
> > I'll leave you to argue with Linus about that, but since that was the
> > decision back in 2005 (for good C reasons) we can safely rely on it.
> 
> `git grep NO_IRQ include arch/*/include' is still quite enlightening...
> 
In the case of PCI where IRQ is unsigned int, that's certainly bogus. The
problem originates on platforms where a 1:1 mapping between vector and
IRQ exists, where 0 is a valid value. This then needs to be remapped in
to an IRQ cookie that has a non-0 value in order to be assigned to
dev->irq. Despite whether this is bogus or not, there's not much to be
done about it. Those of us with vectored IRQ platforms generally have
enough sources that the use of IRQ-0 doesn't matter, and it's not worth
the headache of setting up the remapping crap.

As an example, on SH, IRQ-0 is mapped to vector 0x200. It is '0' for
symbolic reasons only. It's just as easy to pad out irq_desc and treat it
as an off-by-1 to work around all of code that assumes NO_IRQ == 0. There
is enough code in the kernel today that makes the non-zero IRQ cookie
assumption that platforms that do otherwise are simply broken.

Re: [PATCH] pata_of_platform: fix no irq handling

[Tejun Heo]

Anton Vorontsov wrote:
> When no irq specified the pata_of_platform fills the irq_res with -1,
> which is wrong to do for two reasons:
> 
> 1. By definition, 'no irq' should be IRQ 0, not some negative integer;
> 2. pata_platform checks for irq_res.start > 0, but since irq_res.start
>    is unsigned type, the check will be true for `-1'.
> 
> Reported-by: Steven A. Falco <sfalco@harris.com>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

applied to #tj-upstream (will soon be sent upstream)

-- 
tejun

Re: [PATCH] pata_of_platform: fix no irq handling

[Benjamin Herrenschmidt]

On Mon, 2008-10-13 at 15:56 +0900, Tejun Heo wrote:
> Anton Vorontsov wrote:
> > When no irq specified the pata_of_platform fills the irq_res with -1,
> > which is wrong to do for two reasons:
> > 
> > 1. By definition, 'no irq' should be IRQ 0, not some negative integer;
> > 2. pata_platform checks for irq_res.start > 0, but since irq_res.start
> >    is unsigned type, the check will be true for `-1'.
> > 
> > Reported-by: Steven A. Falco <sfalco@harris.com>
> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> 
> applied to #tj-upstream (will soon be sent upstream)

Hrm... I applied it to powerpc.git too :-) Hopefully the merge will sort
it out !

Cheers,
Ben.

[PATCH] pata_platform struct resource signness fix

[Wang Jian]

Hi,

This patch is to pata_platform.c but at this time, it's powerpc specific
because it can only be triggerred using openfirmware, so I post the patch
here. The patch is against 2.6.26-rc8.

The problem is triggerred when ata device is populated using 
pata_of_platform.c, and no irq is assigned (poll mode, such as CF card).

pata_of_platform_probe() parse device tree and

        if (ret == NO_IRQ)
	                irq_res.start = irq_res.end = -1;

Then irq is 0xffffffff, not NULL. Probe will fail coz irq can't be
requested.


---
(irq_res->start > 0) will be true even when it is (-1). When the device
has no irq, irq_res->start is assigned (-1).

Signed-off-by: Wang Jian <lark@linux.net.cn>
---
 drivers/ata/pata_platform.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index 8f65ad6..b12cd0c 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -123,7 +123,7 @@ int __devinit __pata_platform_probe(struct device *dev,
 	/*
 	 * And the IRQ
 	 */
-	if (irq_res && irq_res->start > 0) {
+	if (irq_res && irq_res->start != -1) {
 		irq = irq_res->start;
 		irq_flags = irq_res->flags;
 	}
-- 
1.5.5.4

Re: [PATCH] pata_platform struct resource signness fix

[Wang Jian]

The alternative fix can be.

diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
index 408da30..1f18ad9 100644
--- a/drivers/ata/pata_of_platform.c
+++ b/drivers/ata/pata_of_platform.c
@@ -52,7 +52,7 @@ static int __devinit pata_of_platform_probe(struct of_device *ofdev,

         ret = of_irq_to_resource(dn, 0, &irq_res);
         if (ret == NO_IRQ)
-               irq_res.start = irq_res.end = -1;
+               irq_res.start = irq_res.end = 0;
         else
                 irq_res.flags = 0;

I just didn't spend much time to see which is better.

Wang Jian wrote:
> Hi,
> 
> This patch is to pata_platform.c but at this time, it's powerpc specific
> because it can only be triggerred using openfirmware, so I post the patch
> here. The patch is against 2.6.26-rc8.
> 
> The problem is triggerred when ata device is populated using 
> pata_of_platform.c, and no irq is assigned (poll mode, such as CF card).
> 
> pata_of_platform_probe() parse device tree and
> 
>         if (ret == NO_IRQ)
> 	                irq_res.start = irq_res.end = -1;
> 
> Then irq is 0xffffffff, not NULL. Probe will fail coz irq can't be
> requested.
> 
> 
> ---
> (irq_res->start > 0) will be true even when it is (-1). When the device
> has no irq, irq_res->start is assigned (-1).
> 
> Signed-off-by: Wang Jian <lark@linux.net.cn>
> ---
>  drivers/ata/pata_platform.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
> index 8f65ad6..b12cd0c 100644
> --- a/drivers/ata/pata_platform.c
> +++ b/drivers/ata/pata_platform.c
> @@ -123,7 +123,7 @@ int __devinit __pata_platform_probe(struct device *dev,
>  	/*
>  	 * And the IRQ
>  	 */
> -	if (irq_res && irq_res->start > 0) {
> +	if (irq_res && irq_res->start != -1) {
>  		irq = irq_res->start;
>  		irq_flags = irq_res->flags;
>  	}

Re: [PATCH] pata_platform struct resource signness fix

[Li Yang]

On Thu, Sep 25, 2008 at 4:54 PM, Wang Jian <lark@linux.net.cn> wrote:
> The alternative fix can be.

This one is better as 0 is defined as 'invalid irq' for all
architectures.  Added linux-ide and Anton to cc.

- Leo

>
> diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
> index 408da30..1f18ad9 100644
> --- a/drivers/ata/pata_of_platform.c
> +++ b/drivers/ata/pata_of_platform.c
> @@ -52,7 +52,7 @@ static int __devinit pata_of_platform_probe(struct
> of_device *ofdev,
>
>        ret = of_irq_to_resource(dn, 0, &irq_res);
>        if (ret == NO_IRQ)
> -               irq_res.start = irq_res.end = -1;
> +               irq_res.start = irq_res.end = 0;
>        else
>                irq_res.flags = 0;
>
> I just didn't spend much time to see which is better.
>
> Wang Jian wrote:
>>
>> Hi,
>>
>> This patch is to pata_platform.c but at this time, it's powerpc specific
>> because it can only be triggerred using openfirmware, so I post the patch
>> here. The patch is against 2.6.26-rc8.
>>
>> The problem is triggerred when ata device is populated using
>> pata_of_platform.c, and no irq is assigned (poll mode, such as CF card).
>>
>> pata_of_platform_probe() parse device tree and
>>
>>        if (ret == NO_IRQ)
>>                        irq_res.start = irq_res.end = -1;
>>
>> Then irq is 0xffffffff, not NULL. Probe will fail coz irq can't be
>> requested.
>>
>>
>> ---
>> (irq_res->start > 0) will be true even when it is (-1). When the device
>> has no irq, irq_res->start is assigned (-1).
>>
>> Signed-off-by: Wang Jian <lark@linux.net.cn>
>> ---
>>  drivers/ata/pata_platform.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
>> index 8f65ad6..b12cd0c 100644
>> --- a/drivers/ata/pata_platform.c
>> +++ b/drivers/ata/pata_platform.c
>> @@ -123,7 +123,7 @@ int __devinit __pata_platform_probe(struct device
>> *dev,
>>        /*
>>         * And the IRQ
>>         */
>> -       if (irq_res && irq_res->start > 0) {
>> +       if (irq_res && irq_res->start != -1) {
>>                irq = irq_res->start;
>>                irq_flags = irq_res->flags;
>>        }
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>

Re: [PATCH] pata_platform struct resource signness fix

[Anton Vorontsov]

On Thu, Sep 25, 2008 at 06:40:59PM +0800, Li Yang wrote:
> On Thu, Sep 25, 2008 at 4:54 PM, Wang Jian <lark@linux.net.cn> wrote:
> > The alternative fix can be.
> 
> This one is better as 0 is defined as 'invalid irq' for all
> architectures.  Added linux-ide and Anton to cc.

Thanks for the correct Cc.

I've sent a patch to fix the issue more than a month ago.

http://www.mail-archive.com/linuxppc-dev@ozlabs.org/msg22851.html

Jeff, could you apply the patch?

Thanks!

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] pata_platform struct resource signness fix

[Jeff Garzik]

Anton Vorontsov wrote:
> On Thu, Sep 25, 2008 at 06:40:59PM +0800, Li Yang wrote:
>> On Thu, Sep 25, 2008 at 4:54 PM, Wang Jian <lark@linux.net.cn> wrote:
>>> The alternative fix can be.
>> This one is better as 0 is defined as 'invalid irq' for all
>> architectures.  Added linux-ide and Anton to cc.
> 
> Thanks for the correct Cc.
> 
> I've sent a patch to fix the issue more than a month ago.
> 
> http://www.mail-archive.com/linuxppc-dev@ozlabs.org/msg22851.html
> 
> Jeff, could you apply the patch?

Can you resend, I don't seem to have it...

[PATCH] pata_of_platform: fix no irq handling

[Anton Vorontsov]

When no irq specified the pata_of_platform fills the irq_res with -1,
which is wrong to do for two reasons:

1. By definition, 'no irq' should be IRQ 0, not some negative integer;
2. pata_platform checks for irq_res.start > 0, but since irq_res.start
   is unsigned type, the check will be true for `-1'.

Reported-by: Steven A. Falco <sfalco@harris.com>
Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---

On Mon, Sep 29, 2008 at 12:19:14AM -0400, Jeff Garzik wrote:
> Anton Vorontsov wrote:
>> On Thu, Sep 25, 2008 at 06:40:59PM +0800, Li Yang wrote:
>>> On Thu, Sep 25, 2008 at 4:54 PM, Wang Jian <lark@linux.net.cn> wrote:
>>>> The alternative fix can be.
>>> This one is better as 0 is defined as 'invalid irq' for all
>>> architectures.  Added linux-ide and Anton to cc.
>>
>> Thanks for the correct Cc.
>>
>> I've sent a patch to fix the issue more than a month ago.
>>
>> http://www.mail-archive.com/linuxppc-dev@ozlabs.org/msg22851.html
>>
>> Jeff, could you apply the patch?
>
> Can you resend, I don't seem to have it...

Here it is.

Thanks,

 drivers/ata/pata_of_platform.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
index 408da30..1f18ad9 100644
--- a/drivers/ata/pata_of_platform.c
+++ b/drivers/ata/pata_of_platform.c
@@ -52,7 +52,7 @@ static int __devinit pata_of_platform_probe(struct of_device *ofdev,
 
 	ret = of_irq_to_resource(dn, 0, &irq_res);
 	if (ret == NO_IRQ)
-		irq_res.start = irq_res.end = -1;
+		irq_res.start = irq_res.end = 0;
 	else
 		irq_res.flags = 0;
 
-- 
1.5.6.3

Possible bug in IRQ handling in pata_of_platform / pata_platform

[Steven A. Falco]

I think there is a bug in the communications between pata_of_platform
and pata_platform.  I will refer to the master branch of the DENX git
tree, which is roughly v2.6.26.1 at this time.  I am using a Sequoia
board with a PPC440EPx.

In pata_of_platform, we have:

    ret = of_irq_to_resource(dn, 0, &irq_res);
    if (ret == NO_IRQ)
        irq_res.start = irq_res.end = -1;

so if there is no interrupt defined, then start and end are -1. 
However, __pata_platform_probe has:

    if (irq_res && irq_res->start > 0) {
        irq = irq_res->start;
        irq_flags = irq_res->flags;
    }

You might think that the (irq_res->start > 0) test will fail, as it
should in this no-irq case.  But, start is a u64, so the -1 actually
looks like a large positive number in the comparison.  So,
__pata_platform_probe attempts to use an interrupt when there isn't one.

I think the fix would be to change __pata_platform_probe to:

    if (irq_res && irq_res->start != -1) {

but that might have other unintended consequences, so I'll defer to
whomever knows more about the intent of this code.

    Steve

[PATCH] pata_of_platform: fix no irq handling

[Anton Vorontsov]

When no irq specified, pata_of_platform fills irq_res with -1,
which is wrong to do for two reasons:

1. By definition, 'no irq' should be IRQ 0, not some negative integer;
2. pata_platform checks for irq_res.start > 0, but since irq_res.start
   is unsigned type, the check will be true for `-1'.

Reported-by: Steven A. Falco <sfalco@harris.com>
Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---

On Mon, Aug 11, 2008 at 10:48:50AM -0400, Steven A. Falco wrote:
> I think there is a bug in the communications between pata_of_platform
> and pata_platform.  I will refer to the master branch of the DENX git
> tree, which is roughly v2.6.26.1 at this time.  I am using a Sequoia
> board with a PPC440EPx.
> 
> In pata_of_platform, we have:
> 
>     ret = of_irq_to_resource(dn, 0, &irq_res);
>     if (ret == NO_IRQ)
>         irq_res.start = irq_res.end = -1;
> 
> so if there is no interrupt defined, then start and end are -1. 
> However, __pata_platform_probe has:
> 
>     if (irq_res && irq_res->start > 0) {
>         irq = irq_res->start;
>         irq_flags = irq_res->flags;
>     }
> 
> You might think that the (irq_res->start > 0) test will fail, as it
> should in this no-irq case.  But, start is a u64, so the -1 actually
> looks like a large positive number in the comparison.  So,
> __pata_platform_probe attempts to use an interrupt when there isn't one.
> 
> I think the fix would be to change __pata_platform_probe to:
> 
>     if (irq_res && irq_res->start != -1) {
> 
> but that might have other unintended consequences, so I'll defer to
> whomever knows more about the intent of this code.

Something like this patch should work. Thanks for noticing!

 drivers/ata/pata_of_platform.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
index 408da30..1f18ad9 100644
--- a/drivers/ata/pata_of_platform.c
+++ b/drivers/ata/pata_of_platform.c
@@ -52,7 +52,7 @@ static int __devinit pata_of_platform_probe(struct of_device *ofdev,
 
 	ret = of_irq_to_resource(dn, 0, &irq_res);
 	if (ret == NO_IRQ)
-		irq_res.start = irq_res.end = -1;
+		irq_res.start = irq_res.end = 0;
 	else
 		irq_res.flags = 0;
 
-- 
1.5.6.3

Re: [PATCH] pata_of_platform: fix no irq handling

[Steven A. Falco]

[-- Attachment #1: Type: text/plain, Size: 2272 bytes --]

Anton Vorontsov wrote:
> When no irq specified, pata_of_platform fills irq_res with -1,
> which is wrong to do for two reasons:
>
> 1. By definition, 'no irq' should be IRQ 0, not some negative integer;
> 2. pata_platform checks for irq_res.start > 0, but since irq_res.start
>    is unsigned type, the check will be true for `-1'.
>
> Reported-by: Steven A. Falco <sfalco@harris.com>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---

Thanks!  Your fix is better - I didn't really like the -1 stuff.

I found this bug because I had to disable the ATA interrupt on my system
in order to get a compact-flash card to work.  I am still trying to find
out why the interrupt doesn't work for me.  Here is part of the console
log with the interrupt enabled:

    Uniform Multi-Platform E-IDE driver
    ide: Assuming 33MHz system bus speed for PIO modes; override with
    idebus=xx
    Driver 'sd' needs updating - please use bus_type methods
    irq: irq_create_mapping(0xc0574900, 0x1b)
    irq: -> using host @c0574900
    irq: -> obtained virq 32
    scsi0 : pata_platform
    ata1: PATA max PIO4 mmio cmd 0x1c1000000 ctl 0x1c1000080 irq 32
    irq 32: nobody cared (try booting with the "irqpoll" option)
    Call Trace:
    [cf83fcc0] [c0005a64] show_stack+0x44/0x1ac (unreliable)
    [cf83fd00] [c00489e4] __report_bad_irq+0x34/0xb8
    [cf83fd20] [c0048cf0] note_interrupt+0x288/0x2d0
    [cf83fd50] [c0049a94] handle_level_irq+0xac/0x114
    [cf83fd60] [c0003df0] do_IRQ+0xa4/0xc8
    [cf83fd70] [c000d60c] ret_from_except+0x0/0x18
    [cf83fe30] [00000020] 0x20
    [cf83fe50] [c0003d48] do_softirq+0x54/0x58
    [cf83fe60] [c00241c0] irq_exit+0x90/0x94
    [cf83fe70] [c0003df4] do_IRQ+0xa8/0xc8
    [cf83fe80] [c000d60c] ret_from_except+0x0/0x18
    [cf83ff40] [c01b2cc0] ata_pio_task+0x48/0x104
    [cf83ff60] [c00307a0] run_workqueue+0xb8/0x148
    [cf83ff90] [c0030d54] worker_thread+0x70/0xd0
    [cf83ffd0] [c0034788] kthread+0x48/0x84
    [cf83fff0] [c000cd6c] kernel_thread+0x44/0x60
    handlers:
    [<c01b2d7c>] (ata_sff_interrupt+0x0/0x234)
    Disabling IRQ #32


So it looks like the ATA handler was attached - not sure yet why I got
the "nobody cared" message.  Also, the system hangs.  If I find
something, I'll post it.

    Steve



[-- Attachment #2: Type: text/html, Size: 3162 bytes --]

Re: [PATCH] pata_of_platform: fix no irq handling

[Anton Vorontsov]

On Mon, Aug 11, 2008 at 12:23:10PM -0400, Steven A. Falco wrote:
> Anton Vorontsov wrote:
> > When no irq specified, pata_of_platform fills irq_res with -1,
> > which is wrong to do for two reasons:
> >
> > 1. By definition, 'no irq' should be IRQ 0, not some negative integer;
> > 2. pata_platform checks for irq_res.start > 0, but since irq_res.start
> >    is unsigned type, the check will be true for `-1'.
> >
> > Reported-by: Steven A. Falco <sfalco@harris.com>
> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > ---
> 
> Thanks!  Your fix is better - I didn't really like the -1 stuff.
> 
> I found this bug because I had to disable the ATA interrupt on my system
> in order to get a compact-flash card to work.  I am still trying to find
> out why the interrupt doesn't work for me.  Here is part of the console
> log with the interrupt enabled:
> 
>     Uniform Multi-Platform E-IDE driver
>     ide: Assuming 33MHz system bus speed for PIO modes; override with
>     idebus=xx
>     Driver 'sd' needs updating - please use bus_type methods
>     irq: irq_create_mapping(0xc0574900, 0x1b)
>     irq: -> using host @c0574900
>     irq: -> obtained virq 32
>     scsi0 : pata_platform
>     ata1: PATA max PIO4 mmio cmd 0x1c1000000 ctl 0x1c1000080 irq 32
>     irq 32: nobody cared (try booting with the "irqpoll" option)
>     Call Trace:
>     [cf83fcc0] [c0005a64] show_stack+0x44/0x1ac (unreliable)
>     [cf83fd00] [c00489e4] __report_bad_irq+0x34/0xb8
>     [cf83fd20] [c0048cf0] note_interrupt+0x288/0x2d0
>     [cf83fd50] [c0049a94] handle_level_irq+0xac/0x114
>     [cf83fd60] [c0003df0] do_IRQ+0xa4/0xc8
>     [cf83fd70] [c000d60c] ret_from_except+0x0/0x18
>     [cf83fe30] [00000020] 0x20
>     [cf83fe50] [c0003d48] do_softirq+0x54/0x58
>     [cf83fe60] [c00241c0] irq_exit+0x90/0x94
>     [cf83fe70] [c0003df4] do_IRQ+0xa8/0xc8
>     [cf83fe80] [c000d60c] ret_from_except+0x0/0x18
>     [cf83ff40] [c01b2cc0] ata_pio_task+0x48/0x104
>     [cf83ff60] [c00307a0] run_workqueue+0xb8/0x148
>     [cf83ff90] [c0030d54] worker_thread+0x70/0xd0
>     [cf83ffd0] [c0034788] kthread+0x48/0x84
>     [cf83fff0] [c000cd6c] kernel_thread+0x44/0x60
>     handlers:
>     [<c01b2d7c>] (ata_sff_interrupt+0x0/0x234)
>     Disabling IRQ #32
> 
> 
> So it looks like the ATA handler was attached - not sure yet why I got
> the "nobody cared" message. 

Nobody cared means that ata_sff_interrupt handler didn't notice
any IDE events, and returned zero value. This could mean that

1. IDE status read does not work. (But am I understand correctly
   that IDE works well if IRQ is unspecified? Then this is hardly
   an issue.)
2. IDE interrupt comes when it should not. I'd recommend to use
   oscilloscope to find out what is happening there, that is, if
   the drive actually deasserts its irq line after status read.
   If so, than this could be a PIC problem.

What is the platform on which you're observing the issue, btw?

Just asking because recently Sam Sparks reported that he is observing
similar issue on MPC8349E-mITX-based boards, which I can't not
reproduce on my board though:

http://www.nabble.com/Compact-Flash-on-8349mITX-td18754330.html
http://www.nabble.com/Compact-flash-on-mpc8349eITX-td18777724.html

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] pata_of_platform: fix no irq handling

[Benjamin Herrenschmidt]

> 1. IDE status read does not work. (But am I understand correctly
>    that IDE works well if IRQ is unspecified? Then this is hardly
>    an issue.)
> 2. IDE interrupt comes when it should not. I'd recommend to use
>    oscilloscope to find out what is happening there, that is, if
>    the drive actually deasserts its irq line after status read.
>    If so, than this could be a PIC problem.
> 
> What is the platform on which you're observing the issue, btw?

Another possibility is that you got the wrong interrupt number
in the device-tree...

Ben.

Re: [PATCH] pata_of_platform: fix no irq handling

[Steven A. Falco]

[-- Attachment #1: Type: text/plain, Size: 2206 bytes --]

Benjamin Herrenschmidt wrote:
>   
>> 1. IDE status read does not work. (But am I understand correctly
>>    that IDE works well if IRQ is unspecified? Then this is hardly
>>    an issue.)
>> 2. IDE interrupt comes when it should not. I'd recommend to use
>>    oscilloscope to find out what is happening there, that is, if
>>    the drive actually deasserts its irq line after status read.
>>    If so, than this could be a PIC problem.
>>
>> What is the platform on which you're observing the issue, btw?
>>     
>
> Another possibility is that you got the wrong interrupt number
> in the device-tree...
>
> Ben.
>   

The platform is the AMCC Sequoia board.  We've built a little adapter to
connect a compact flash card to the processor bus.  I believe the
interrupt selection in the device tree is correct, and I've checked over
the u-boot settings for the IRQ line (active high, level sensitive). 
I've also tried edge-sensitive but it doesn't make a difference.

When u-boot queries the CF card, we see the IRQ pulse as expected, but
when the kernel runs, we see the IRQ go high and stay there, which the
kernel naturally treats as a stuck interrupt.  The other oddity is that
we see a single diagnostic failure on startup:

    ata1.00: Drive reports diagnostics failure. This may indicate a drive
    ata1.00: fault or invalid emulation. Contact drive vendor for
    information.


That is strange, because if we manually do the soft reset from u-boot,
we see the ATA "feature byte" return 0x01, which means success.  When
the kernel does the soft reset, we see a 0x00, which means failure.  You
would think it is timing related, but a logic analyzer trace shows
reasonable timing.  We need to wire up a better test rig, so I don't
want folks on this list to waste any time on it.  I'll report back if I
learn anything of general interest.

With the interrupt disabled in the device tree, and ignoring the
diagnostics failure, the drive actually works.  I'm able to mount a
filesystem, read files from it, etc.  So, the drive is fully functional,
just without using interrupts.  Therefore, I believe most everything is
correct - byte lanes, read/write signaling, timing, etc.  Curious.

    Steve


[-- Attachment #2: Type: text/html, Size: 2699 bytes --]

Re: [PATCH] pata_of_platform: fix no irq handling

[Anton Vorontsov]

On Tue, Aug 12, 2008 at 10:00:40AM -0400, Steven A. Falco wrote:
> Benjamin Herrenschmidt wrote:
> >   
> >> 1. IDE status read does not work. (But am I understand correctly
> >>    that IDE works well if IRQ is unspecified? Then this is hardly
> >>    an issue.)
> >> 2. IDE interrupt comes when it should not. I'd recommend to use
> >>    oscilloscope to find out what is happening there, that is, if
> >>    the drive actually deasserts its irq line after status read.
> >>    If so, than this could be a PIC problem.
> >>
> >> What is the platform on which you're observing the issue, btw?
> >>     
> >
> > Another possibility is that you got the wrong interrupt number
> > in the device-tree...
> >
> > Ben.
> >   
> 
> The platform is the AMCC Sequoia board.  We've built a little adapter to
> connect a compact flash card to the processor bus.  I believe the
> interrupt selection in the device tree is correct, and I've checked over
> the u-boot settings for the IRQ line (active high, level sensitive). 

IDE IRQs are active-low.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] pata_of_platform: fix no irq handling

[Stefan Roese]

On Tuesday 12 August 2008, Anton Vorontsov wrote:
> > > Another possibility is that you got the wrong interrupt number
> > > in the device-tree...
> > >
> > > Ben.
> >
> > The platform is the AMCC Sequoia board.  We've built a little adapter to
> > connect a compact flash card to the processor bus.  I believe the
> > interrupt selection in the device tree is correct, and I've checked over
> > the u-boot settings for the IRQ line (active high, level sensitive).
>
> IDE IRQs are active-low.

IIRC, the CompactFlash interrupt is active-high.

Best regards,
Stefan

Re: [PATCH] pata_of_platform: fix no irq handling

[Sergei Shtylyov]

Anton Vorontsov wrote:

>>>>1. IDE status read does not work. (But am I understand correctly
>>>>   that IDE works well if IRQ is unspecified? Then this is hardly
>>>>   an issue.)
>>>>2. IDE interrupt comes when it should not. I'd recommend to use
>>>>   oscilloscope to find out what is happening there, that is, if
>>>>   the drive actually deasserts its irq line after status read.
>>>>   If so, than this could be a PIC problem.

>>>>What is the platform on which you're observing the issue, btw?

>>>Another possibility is that you got the wrong interrupt number
>>>in the device-tree...

>>>Ben.

>>The platform is the AMCC Sequoia board.  We've built a little adapter to
>>connect a compact flash card to the processor bus.  I believe the
>>interrupt selection in the device tree is correct, and I've checked over
>>the u-boot settings for the IRQ line (active high, level sensitive). 

> IDE IRQs are active-low.

    Only on the PCI and only in the native mode. Natively, the IDE INTRQ 
signal is active-high, rising edge triggering, as on ISA. You seem to have an 
invertor somewhere, if it's not a PCI chip...

WBR, Sergei

Re: [PATCH] pata_of_platform: fix no irq handling

[Anton Vorontsov]

On Tue, Aug 12, 2008 at 06:18:42PM +0400, Sergei Shtylyov wrote:
> Anton Vorontsov wrote:
>
>>>>> 1. IDE status read does not work. (But am I understand correctly
>>>>>   that IDE works well if IRQ is unspecified? Then this is hardly
>>>>>   an issue.)
>>>>> 2. IDE interrupt comes when it should not. I'd recommend to use
>>>>>   oscilloscope to find out what is happening there, that is, if
>>>>>   the drive actually deasserts its irq line after status read.
>>>>>   If so, than this could be a PIC problem.
>
>>>>> What is the platform on which you're observing the issue, btw?
>
>>>> Another possibility is that you got the wrong interrupt number
>>>> in the device-tree...
>
>>>> Ben.
>
>>> The platform is the AMCC Sequoia board.  We've built a little adapter to
>>> connect a compact flash card to the processor bus.  I believe the
>>> interrupt selection in the device tree is correct, and I've checked over
>>> the u-boot settings for the IRQ line (active high, level sensitive). 
>
>> IDE IRQs are active-low.
>
>    Only on the PCI and only in the native mode. Natively, the IDE INTRQ  
> signal is active-high, rising edge triggering, as on ISA. You seem to 
> have an invertor somewhere, if it's not a PCI chip...

Ugh. Right you are, as always. I've just looked into mpc8349emitx
schematics, there is indeed an inverter on the irq line.

CF in True IDE mode is active-high, sorry.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] pata_of_platform: fix no irq handling

[Steven A. Falco]

[-- Attachment #1: Type: text/plain, Size: 1989 bytes --]

Anton Vorontsov wrote:
> On Tue, Aug 12, 2008 at 06:18:42PM +0400, Sergei Shtylyov wrote:
>   
>> Anton Vorontsov wrote:
>>
>>     
>>>>>> 1. IDE status read does not work. (But am I understand correctly
>>>>>>   that IDE works well if IRQ is unspecified? Then this is hardly
>>>>>>   an issue.)
>>>>>> 2. IDE interrupt comes when it should not. I'd recommend to use
>>>>>>   oscilloscope to find out what is happening there, that is, if
>>>>>>   the drive actually deasserts its irq line after status read.
>>>>>>   If so, than this could be a PIC problem.
>>>>>>             
>>>>>> What is the platform on which you're observing the issue, btw?
>>>>>>             
>>>>> Another possibility is that you got the wrong interrupt number
>>>>> in the device-tree...
>>>>>           
>>>>> Ben.
>>>>>           
>>>> The platform is the AMCC Sequoia board.  We've built a little adapter to
>>>> connect a compact flash card to the processor bus.  I believe the
>>>> interrupt selection in the device tree is correct, and I've checked over
>>>> the u-boot settings for the IRQ line (active high, level sensitive). 
>>>>         
>>> IDE IRQs are active-low.
>>>       
>>    Only on the PCI and only in the native mode. Natively, the IDE INTRQ  
>> signal is active-high, rising edge triggering, as on ISA. You seem to 
>> have an invertor somewhere, if it's not a PCI chip...
>>     
>
> Ugh. Right you are, as always. I've just looked into mpc8349emitx
> schematics, there is indeed an inverter on the irq line.
>
> CF in True IDE mode is active-high, sorry.
>   

Ok, just to close this issue, my CF device is now working perfectly. 
Two problems, both my fault.  1) I thought the alternate registers used
the same reg-shift and offset, so the alt_command and alt_status were at
the wrong address, and 2) I was missing a pull-down on the IRQ line -
the sequoia eval board has it in the schematic, but it is marked "not
populated".

    Thanks to all for your comments and help,
    Steve


[-- Attachment #2: Type: text/html, Size: 3345 bytes --]

Re: [PATCH] pata_of_platform: fix no irq handling

[Alan Cox]

On Mon, 11 Aug 2008 19:19:13 +0400
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> When no irq specified, pata_of_platform fills irq_res with -1,
> which is wrong to do for two reasons:
> 
> 1. By definition, 'no irq' should be IRQ 0, not some negative integer;
> 2. pata_platform checks for irq_res.start > 0, but since irq_res.start
>    is unsigned type, the check will be true for `-1'.
> 
> Reported-by: Steven A. Falco <sfalco@harris.com>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
Acked-by: Alan Cox <alan@redhat.com>

Re: [PATCH] pata_of_platform: fix no irq handling

[Ben Dooks]

On Mon, Aug 11, 2008 at 07:19:13PM +0400, Anton Vorontsov wrote:
> When no irq specified, pata_of_platform fills irq_res with -1,
> which is wrong to do for two reasons:
> 
> 1. By definition, 'no irq' should be IRQ 0, not some negative integer;

interesting, IRQ 0 is actually valid on some ARM systems.

> 2. pata_platform checks for irq_res.start > 0, but since irq_res.start
>    is unsigned type, the check will be true for `-1'.
> 
> Reported-by: Steven A. Falco <sfalco@harris.com>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> 
> On Mon, Aug 11, 2008 at 10:48:50AM -0400, Steven A. Falco wrote:
> > I think there is a bug in the communications between pata_of_platform
> > and pata_platform.  I will refer to the master branch of the DENX git
> > tree, which is roughly v2.6.26.1 at this time.  I am using a Sequoia
> > board with a PPC440EPx.
> > 
> > In pata_of_platform, we have:
> > 
> >     ret = of_irq_to_resource(dn, 0, &irq_res);
> >     if (ret == NO_IRQ)
> >         irq_res.start = irq_res.end = -1;
> > 
> > so if there is no interrupt defined, then start and end are -1. 
> > However, __pata_platform_probe has:
> > 
> >     if (irq_res && irq_res->start > 0) {
> >         irq = irq_res->start;
> >         irq_flags = irq_res->flags;
> >     }
> > 
> > You might think that the (irq_res->start > 0) test will fail, as it
> > should in this no-irq case.  But, start is a u64, so the -1 actually
> > looks like a large positive number in the comparison.  So,
> > __pata_platform_probe attempts to use an interrupt when there isn't one.
> > 
> > I think the fix would be to change __pata_platform_probe to:
> > 
> >     if (irq_res && irq_res->start != -1) {
> > 
> > but that might have other unintended consequences, so I'll defer to
> > whomever knows more about the intent of this code.
> 
> Something like this patch should work. Thanks for noticing!
> 
>  drivers/ata/pata_of_platform.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
> index 408da30..1f18ad9 100644
> --- a/drivers/ata/pata_of_platform.c
> +++ b/drivers/ata/pata_of_platform.c
> @@ -52,7 +52,7 @@ static int __devinit pata_of_platform_probe(struct of_device *ofdev,
>  
>  	ret = of_irq_to_resource(dn, 0, &irq_res);
>  	if (ret == NO_IRQ)
> -		irq_res.start = irq_res.end = -1;
> +		irq_res.start = irq_res.end = 0;
>  	else
>  		irq_res.flags = 0;
>  

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: [PATCH] pata_of_platform: fix no irq handling

[Alan Cox]

On Mon, 11 Aug 2008 17:36:48 +0100
Ben Dooks <ben-linux@fluff.org> wrote:

> 
> On Mon, Aug 11, 2008 at 07:19:13PM +0400, Anton Vorontsov wrote:
> > When no irq specified, pata_of_platform fills irq_res with -1,
> > which is wrong to do for two reasons:
> > 
> > 1. By definition, 'no irq' should be IRQ 0, not some negative integer;
> 
> interesting, IRQ 0 is actually valid on some ARM systems.

Not as far as Linux is concerned. It's expected that any IRQ that happens
to be "physical IRQ 0" whatever that means is remapped by the arch code
unless not visible outside the arch.

Re: [PATCH] pata_of_platform: fix no irq handling

[Steven A. Falco]

[-- Attachment #1: Type: text/plain, Size: 2710 bytes --]

Ben Dooks wrote:
> On Mon, Aug 11, 2008 at 07:19:13PM +0400, Anton Vorontsov wrote:
>   
>> When no irq specified, pata_of_platform fills irq_res with -1,
>> which is wrong to do for two reasons:
>>
>> 1. By definition, 'no irq' should be IRQ 0, not some negative integer;
>>     
>
> interesting, IRQ 0 is actually valid on some ARM systems.
>
>   

It is here too, but I believe most of the code uses a virtualized irq
number, so physical IRQ 0 would presumably get mapped to a non-zero
virtual one.

>> 2. pata_platform checks for irq_res.start > 0, but since irq_res.start
>>    is unsigned type, the check will be true for `-1'.
>>
>> Reported-by: Steven A. Falco <sfalco@harris.com>
>> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
>> ---
>>
>> On Mon, Aug 11, 2008 at 10:48:50AM -0400, Steven A. Falco wrote:
>>     
>>> I think there is a bug in the communications between pata_of_platform
>>> and pata_platform.  I will refer to the master branch of the DENX git
>>> tree, which is roughly v2.6.26.1 at this time.  I am using a Sequoia
>>> board with a PPC440EPx.
>>>
>>> In pata_of_platform, we have:
>>>
>>>     ret = of_irq_to_resource(dn, 0, &irq_res);
>>>     if (ret == NO_IRQ)
>>>         irq_res.start = irq_res.end = -1;
>>>
>>> so if there is no interrupt defined, then start and end are -1. 
>>> However, __pata_platform_probe has:
>>>
>>>     if (irq_res && irq_res->start > 0) {
>>>         irq = irq_res->start;
>>>         irq_flags = irq_res->flags;
>>>     }
>>>
>>> You might think that the (irq_res->start > 0) test will fail, as it
>>> should in this no-irq case.  But, start is a u64, so the -1 actually
>>> looks like a large positive number in the comparison.  So,
>>> __pata_platform_probe attempts to use an interrupt when there isn't one.
>>>
>>> I think the fix would be to change __pata_platform_probe to:
>>>
>>>     if (irq_res && irq_res->start != -1) {
>>>
>>> but that might have other unintended consequences, so I'll defer to
>>> whomever knows more about the intent of this code.
>>>       
>> Something like this patch should work. Thanks for noticing!
>>
>>  drivers/ata/pata_of_platform.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
>> index 408da30..1f18ad9 100644
>> --- a/drivers/ata/pata_of_platform.c
>> +++ b/drivers/ata/pata_of_platform.c
>> @@ -52,7 +52,7 @@ static int __devinit pata_of_platform_probe(struct of_device *ofdev,
>>  
>>  	ret = of_irq_to_resource(dn, 0, &irq_res);
>>  	if (ret == NO_IRQ)
>> -		irq_res.start = irq_res.end = -1;
>> +		irq_res.start = irq_res.end = 0;
>>  	else
>>  		irq_res.flags = 0;
>>  
>>     
>
>   


[-- Attachment #2: Type: text/html, Size: 3435 bytes --]

Re: [PATCH] pata_of_platform: fix no irq handling

[Benjamin Herrenschmidt]

On Mon, 2008-08-11 at 17:36 +0100, Ben Dooks wrote:
> On Mon, Aug 11, 2008 at 07:19:13PM +0400, Anton Vorontsov wrote:
> > When no irq specified, pata_of_platform fills irq_res with -1,
> > which is wrong to do for two reasons:
> > 
> > 1. By definition, 'no irq' should be IRQ 0, not some negative integer;

> interesting, IRQ 0 is actually valid on some ARM systems.

It never is on powerpc (anymore). Linus several times said he believed
that was the right thing to do, so when I make the whole IRQ handling
using virtual IRQ numbers on ppc, I made 0 reserved (and 1..15 only ever
assigned to a 8259 if there's one).

ARM, with their collections of cascaded fancy PICs all over the place
should probably look into using a similar remapping scheme :-)
 
Ben.