Possible bug in IRQ handling in pata_of_platform / pata_platform

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.