[PATCH -rt] ide: fix interrupts processing issue with preempt-able hardirqs

[PATCH -rt] ide: fix interrupts processing issue with preempt-able hardirqs

[Anton Vorontsov]

IDE interrupt handler relies on the fact that, if necessary, hardirqs
will re-trigger on ISR exit. With fully preemtable IRQs this seems to be
not true, since if hardirq thread is currently running, and the same IRQ
raised again, then this IRQ will be simply lost.

This patch fixes following issue:

ALI15X3: IDE controller (0x10b9:0x5229 rev 0xc8) at  PCI slot 0001:03:1f.0
ALI15X3: 100% native mode on irq 18
    ide0: BM-DMA at 0x1120-0x1127, BIOS settings: hda:PIO, hdb:PIO
    ide1: BM-DMA at 0x1128-0x112f, BIOS settings: hdc:PIO, hdd:PIO
hda: Optiarc DVD RW AD-7190A, ATAPI CD/DVD-ROM drive
hda: UDMA/66 mode selected
ide0 at 0x1100-0x1107,0x110a on irq 18
ide-cd: cmd 0x5a timed out
hda: lost interrupt
hda: ATAPI 12X DVD-ROM DVD-R-RAM CD-R/RW drive, 2048kB Cache
Uniform CD-ROM driver Revision: 3.20
ide-cd: cmd 0x3 timed out
hda: lost interrupt
ide-cd: cmd 0x3 timed out
hda: lost interrupt
...

Though, some IDE devices (e.g. SONY DVD RW AW-Q170A) seem to work fine
without this patch, probably because they trigger IRQ after some delay.

This patch should be almost no-op for the !RT kernels, thus no #ifdefs.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/ide/ide-io.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index 6c1b288..a52733e 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -1460,6 +1460,7 @@ static void unexpected_intr (int irq, ide_hwgroup_t *hwgroup)
  
 irqreturn_t ide_intr (int irq, void *dev_id)
 {
+	int ret = IRQ_NONE;
 	unsigned long flags;
 	ide_hwgroup_t *hwgroup = (ide_hwgroup_t *)dev_id;
 	ide_hwif_t *hwif;
@@ -1467,12 +1468,13 @@ irqreturn_t ide_intr (int irq, void *dev_id)
 	ide_handler_t *handler;
 	ide_startstop_t startstop;
 
+again:
 	spin_lock_irqsave(&ide_lock, flags);
 	hwif = hwgroup->hwif;
 
 	if (!ide_ack_intr(hwif)) {
 		spin_unlock_irqrestore(&ide_lock, flags);
-		return IRQ_NONE;
+		return ret;
 	}
 
 	if ((handler = hwgroup->handler) == NULL || hwgroup->polling) {
@@ -1510,7 +1512,7 @@ irqreturn_t ide_intr (int irq, void *dev_id)
 #endif /* CONFIG_BLK_DEV_IDEPCI */
 		}
 		spin_unlock_irqrestore(&ide_lock, flags);
-		return IRQ_NONE;
+		return ret;
 	}
 	drive = hwgroup->drive;
 	if (!drive) {
@@ -1532,7 +1534,7 @@ irqreturn_t ide_intr (int irq, void *dev_id)
 		 * enough advance overhead that the latter isn't a problem.
 		 */
 		spin_unlock_irqrestore(&ide_lock, flags);
-		return IRQ_NONE;
+		return ret;
 	}
 	if (!hwgroup->busy) {
 		hwgroup->busy = 1;	/* paranoia */
@@ -1578,7 +1580,16 @@ irqreturn_t ide_intr (int irq, void *dev_id)
 		}
 	}
 	spin_unlock_irqrestore(&ide_lock, flags);
-	return IRQ_HANDLED;
+	ret = IRQ_HANDLED;
+
+	/*
+	 * Previous handler() may have set things up for another interrupt to
+	 * occur soon... in case we've lost it (e.g. with preemtable hardirqs),
+	 * try again and then return gracefully if no irqs were pending.
+	 */
+	if (startstop != ide_stopped)
+		goto again;
+	return ret;
 }
 
 /**
-- 
1.5.5.4

Re: [PATCH -rt] ide: fix interrupts processing issue with preempt-able hardirqs

[Ingo Molnar]

* Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> IDE interrupt handler relies on the fact that, if necessary, hardirqs 
> will re-trigger on ISR exit. With fully preemtable IRQs this seems to 
> be not true, since if hardirq thread is currently running, and the 
> same IRQ raised again, then this IRQ will be simply lost.

actually no, that should not happen - if -rt loses an IRQ then something 
broke in the threaded IRQ code. It's supposed to be a drop-in, 
compatible IRQ flow with no driver changes needed.

( also, please do not Cc: mainline maintainers to RFC -rt patches, let
  the -rt maintainers sort out the need for any patch propagation - once 
  the patches are sufficiently cooked. Thanks. )

	Ingo

Re: [PATCH -rt] ide: fix interrupts processing issue with preempt-able hardirqs

[Anton Vorontsov]

On Tue, Jun 24, 2008 at 03:40:37AM +0400, Anton Vorontsov wrote:
> IDE interrupt handler relies on the fact that, if necessary, hardirqs
> will re-trigger on ISR exit. With fully preemtable IRQs this seems to be
> not true, since if hardirq thread is currently running, and the same IRQ
> raised again, then this IRQ will be simply lost.

Btw, I don't actually understand why interrupt "simply lost", IMO it
should not.. :-/ But this what I'm observing via bunch of printks in
the code flow. Maybe there is not so obvious race, which I can't see.

So, help needed. Either it is a proper fix, or it is a workaround for
another bug somewhere else.

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

Re: [PATCH -rt] ide: fix interrupts processing issue with preempt-able hardirqs

[Anton Vorontsov]

On Tue, Jun 24, 2008 at 01:51:41AM +0200, Ingo Molnar wrote:
> 
> * Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> 
> > IDE interrupt handler relies on the fact that, if necessary, hardirqs 
> > will re-trigger on ISR exit. With fully preemtable IRQs this seems to 
> > be not true, since if hardirq thread is currently running, and the 
> > same IRQ raised again, then this IRQ will be simply lost.
> 
> actually no, that should not happen - if -rt loses an IRQ then something 
> broke in the threaded IRQ code. It's supposed to be a drop-in, 
> compatible IRQ flow with no driver changes needed.

..just as I thought, the bug somewhere deeper... heh.

> ( also, please do not Cc: mainline maintainers to RFC -rt patches, let
>   the -rt maintainers sort out the need for any patch propagation - once 
>   the patches are sufficiently cooked. Thanks. )

Ok, thanks.

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

Re: [PATCH -rt] ide: fix interrupts processing issue with preempt-able hardirqs

[Alan Cox]

> The bug, as I see it, in the alim15x3 (ULi M5228) hardware: for some
> reason it does not hold IRQ line, but rises it for some short period
> of time (while the drive itself rises and holds it correctly -- I'm
> seeing it via oscilloscope).

Are you sure the kernel isn't touching the status register and so
clearing the IRQ condition at the controller ?

Re: [PATCH -rt] ide: fix interrupts processing issue with preempt-able hardirqs

[Anton Vorontsov]

On Tue, Jun 24, 2008 at 04:00:16AM +0400, Anton Vorontsov wrote:
> On Tue, Jun 24, 2008 at 01:51:41AM +0200, Ingo Molnar wrote:
> > 
> > * Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> > 
> > > IDE interrupt handler relies on the fact that, if necessary, hardirqs 
> > > will re-trigger on ISR exit. With fully preemtable IRQs this seems to 
> > > be not true, since if hardirq thread is currently running, and the 
> > > same IRQ raised again, then this IRQ will be simply lost.
> > 
> > actually no, that should not happen - if -rt loses an IRQ then something 
> > broke in the threaded IRQ code. It's supposed to be a drop-in, 
> > compatible IRQ flow with no driver changes needed.
> 
> ..just as I thought, the bug somewhere deeper... heh.

Ok, a bit more investigation showed that this is indeed not RT specific
per see, but issue emerges only on RT-style IRQ handlers + alim15x3 IDE
controller (for example, PDC20269 works ok).

The difference is that that with RT: low-level (non-threaded) IRQ
handler masks IDE IRQ, then wakes up appropriate IRQ thread, which calls
IDE handler, and then, after IDE handler exits, thread routine unmasks
IDE IRQ.

Without RT: low-level non-threaded IRQ handler does not mask specific
IRQ, but disables local interrupts, and calls IDE handler directly.

The bug, as I see it, in the alim15x3 (ULi M5228) hardware: for some
reason it does not hold IRQ line, but rises it for some short period
of time (while the drive itself rises and holds it correctly -- I'm
seeing it via oscilloscope).

So this scheme does not work:
mask_irq()
...do something that will trigger IDE interrupt...
unmask_irq()

Because at the unmask_irq() time IDE IRQ is gone already, and interrupt
controller could not notice it (interrupts are level sensitive).

I did following test: disable RT + insert mask/unmask sequence in the
IDE IRQ handler, and I got the same behaviour as with RT enabled.

Also, further testing showed that this issue isn't drive-specific, i.e.
with a delay inserted before the unmask_irq(), the bug shows with any
drive I have.

So, in summary: I think that the patch is still correct as a hw bug
workaround (I'll need to correct its comments and description though).

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

Re: [PATCH -rt] ide: fix interrupts processing issue with preempt-able hardirqs

[Sergei Shtylyov]

Hello.

Anton Vorontsov wrote:

>>>>IDE interrupt handler relies on the fact that, if necessary, hardirqs 
>>>>will re-trigger on ISR exit. With fully preemtable IRQs this seems to 
>>>>be not true, since if hardirq thread is currently running, and the 
>>>>same IRQ raised again, then this IRQ will be simply lost.

>>>actually no, that should not happen - if -rt loses an IRQ then something 
>>>broke in the threaded IRQ code. It's supposed to be a drop-in, 
>>>compatible IRQ flow with no driver changes needed.

>>..just as I thought, the bug somewhere deeper... heh.

> Ok, a bit more investigation showed that this is indeed not RT specific
> per see, but issue emerges only on RT-style IRQ handlers + alim15x3 IDE
> controller (for example, PDC20269 works ok).

    Does it happen only with ATAPI devices also or with ATA disks too?

> The difference is that that with RT: low-level (non-threaded) IRQ
> handler masks IDE IRQ, then wakes up appropriate IRQ thread, which calls
> IDE handler, and then, after IDE handler exits, thread routine unmasks
> IDE IRQ.

> Without RT: low-level non-threaded IRQ handler does not mask specific
> IRQ, but disables local interrupts, and calls IDE handler directly.

    Hm, handle_level_irq() (and PCI IRQs are level-triggered) calls 
mask_ack_irq() which should *mask* IRQ and send EOI (at least on i8259). By 
IRQ18 I can assume it's I/O APIC -- this one may be using different methods of 
handling IRQ depending on whether hardirq preemption is on or off (at least it 
was so in 2.6.18-rt time). The default, "fasteoi" path doesn't mask off the 
IRQ indeed (it should be disabled from re-occuring anyway until the code 
issues EOI)...
    So, which machine and PIC you have?

> The bug, as I see it, in the alim15x3 (ULi M5228) hardware: for some
> reason it does not hold IRQ line, but rises it for some short period
> of time (while the drive itself rises and holds it correctly -- I'm
> seeing it via oscilloscope).

    That's surely an invalid behavior for a level triggered interrupt that can 
also result in spurious IRQs...  I'm not even sure how it can reliably work 
without masking since there should be no latching for level triggered interupts...

> So this scheme does not work:
> mask_irq()
> ...do something that will trigger IDE interrupt...
> unmask_irq()

> Also, further testing showed that this issue isn't drive-specific, i.e.
> with a delay inserted before the unmask_irq(), the bug shows with any
> drive I have.

    So, "shit happens" even with the ATA drives?

> So, in summary: I think that the patch is still correct as a hw bug
> workaround (I'll need to correct its comments and description though).

   Well, the patch seemed sane (and the hardware absolutely insane :-)...

WBR, Sergei

Re: [PATCH -rt] ide: fix interrupts processing issue with preempt-able hardirqs

[Anton Vorontsov]

On Wed, Jun 25, 2008 at 01:32:57PM +0100, Alan Cox wrote:
> > The bug, as I see it, in the alim15x3 (ULi M5228) hardware: for some
> > reason it does not hold IRQ line, but rises it for some short period
> > of time (while the drive itself rises and holds it correctly -- I'm
> > seeing it via oscilloscope).
> 
> Are you sure the kernel isn't touching the status register and so
> clearing the IRQ condition at the controller ?

There are lots of places that reads status register, and looking into
the code flow I don't see any problem with the code.

But to be sure I inserted a printk into INB, and I don't see any status
reads when/after the drive asserts its IRQ.

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

Re: [PATCH -rt] ide: fix interrupts processing issue with preempt-able hardirqs

[Anton Vorontsov]

On Wed, Jun 25, 2008 at 05:15:43PM +0400, Sergei Shtylyov wrote:
> Hello.
>
> Anton Vorontsov wrote:
>
>>>>> IDE interrupt handler relies on the fact that, if necessary, 
>>>>> hardirqs will re-trigger on ISR exit. With fully preemtable IRQs 
>>>>> this seems to be not true, since if hardirq thread is currently 
>>>>> running, and the same IRQ raised again, then this IRQ will be 
>>>>> simply lost.
>
>>>> actually no, that should not happen - if -rt loses an IRQ then 
>>>> something broke in the threaded IRQ code. It's supposed to be a 
>>>> drop-in, compatible IRQ flow with no driver changes needed.
>
>>> ..just as I thought, the bug somewhere deeper... heh.
>
>> Ok, a bit more investigation showed that this is indeed not RT specific
>> per see, but issue emerges only on RT-style IRQ handlers + alim15x3 IDE
>> controller (for example, PDC20269 works ok).
>
>    Does it happen only with ATAPI devices also or with ATA disks too?

So far I own two ATAPI devices, IDE disks are quire rare nowadays,
should find one. ;-)

>> The difference is that that with RT: low-level (non-threaded) IRQ
>> handler masks IDE IRQ, then wakes up appropriate IRQ thread, which calls
>> IDE handler, and then, after IDE handler exits, thread routine unmasks
>> IDE IRQ.
>
>> Without RT: low-level non-threaded IRQ handler does not mask specific
>> IRQ, but disables local interrupts, and calls IDE handler directly.
>
>    Hm, handle_level_irq() (and PCI IRQs are level-triggered) calls  
> mask_ack_irq() which should *mask* IRQ and send EOI (at least on i8259). 
> By IRQ18 I can assume it's I/O APIC 

No, it's not Intel I/O APIC. IRQ18 is the virtual "Linux IRQ", no
correlations with the PIC IRQ number.

> -- this one may be using different 
> methods of handling IRQ depending on whether hardirq preemption is on or 
> off (at least it was so in 2.6.18-rt time).

Hardirq preemption is on, of course. The whole problem is when hardirqs
preempted.

> The default, "fasteoi" path 
> doesn't mask off the IRQ indeed (it should be disabled from re-occuring 
> anyway until the code issues EOI)...

What do you mean by doesn't mask off? With hardirqs preemption it does
mask off IDE interrupt, and then sends an EOI. But it doesn't mask
processors' IRQs (i.e. local_irq_disable()), true. And MPIC is using
fasteoi path indeed.

>    So, which machine and PIC you have?

It is PowerPC MPC8610 + ULi "Super South Bridge" connected through
PCI Express. This south bridge contains lots of devices (and lots of
PCI quirks, see arch/powerpc/platforms/86xx/mpc8610_hpcd.c).

PIC is OpenPIC-compatible (MPIC, built-in into MPC8610 SOC).

Note: I don't have any specifications on that ULi bridge, neither I have
any schematics for that board (so far, let's hope). So I can't say
exactly how things are inter-connected or what these PCI quirks are
actually doing (despite few comments in them).

And since it is PCI-E, interrupt things are quite troublesome to
debug without serial logic analyzer. :-)

>> The bug, as I see it, in the alim15x3 (ULi M5228) hardware: for some
>> reason it does not hold IRQ line, but rises it for some short period
>> of time (while the drive itself rises and holds it correctly -- I'm
>> seeing it via oscilloscope).
>
>    That's surely an invalid behavior for a level triggered interrupt that 
> can also result in spurious IRQs...  I'm not even sure how it can 
> reliably work without masking since there should be no latching for level 
> triggered interupts...

Yeah.

>> So this scheme does not work:
>> mask_irq()
>> ...do something that will trigger IDE interrupt...
>> unmask_irq()
>
>> Also, further testing showed that this issue isn't drive-specific, i.e.
>> with a delay inserted before the unmask_irq(), the bug shows with any
>> drive I have.
>
>    So, "shit happens" even with the ATA drives?

Will try as soon as I'll get one.

Thanks,

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

Re: [PATCH -rt] ide: fix interrupts processing issue with preempt-able hardirqs

[Alan Cox]

> Note: I don't have any specifications on that ULi bridge, neither I have
> any schematics for that board (so far, let's hope). So I can't say
> exactly how things are inter-connected or what these PCI quirks are
> actually doing (despite few comments in them).

So you don't for example know if the bridge is correctly configured for
that device to be edge or level triggered ?

Alan

Re: [PATCH -rt] ide: fix interrupts processing issue with preempt-able hardirqs

[Anton Vorontsov]

On Wed, Jun 25, 2008 at 06:22:49PM +0400, Anton Vorontsov wrote:
> On Wed, Jun 25, 2008 at 05:15:43PM +0400, Sergei Shtylyov wrote:
> > Hello.
> >
> > Anton Vorontsov wrote:
> >
> >>>>> IDE interrupt handler relies on the fact that, if necessary, 
> >>>>> hardirqs will re-trigger on ISR exit. With fully preemtable IRQs 
> >>>>> this seems to be not true, since if hardirq thread is currently 
> >>>>> running, and the same IRQ raised again, then this IRQ will be 
> >>>>> simply lost.
> >
> >>>> actually no, that should not happen - if -rt loses an IRQ then 
> >>>> something broke in the threaded IRQ code. It's supposed to be a 
> >>>> drop-in, compatible IRQ flow with no driver changes needed.
> >
> >>> ..just as I thought, the bug somewhere deeper... heh.
> >
> >> Ok, a bit more investigation showed that this is indeed not RT specific
> >> per see, but issue emerges only on RT-style IRQ handlers + alim15x3 IDE
> >> controller (for example, PDC20269 works ok).
> >
> >    Does it happen only with ATAPI devices also or with ATA disks too?
> 
> So far I own two ATAPI devices, IDE disks are quire rare nowadays,
> should find one. ;-)
[...]
> >> Also, further testing showed that this issue isn't drive-specific, i.e.
> >> with a delay inserted before the unmask_irq(), the bug shows with any
> >> drive I have.
> >
> >    So, "shit happens" even with the ATA drives?
> 
> Will try as soon as I'll get one.

Thanks for the drive, and the result is:

hda: irq timeout: status=0x58 { DriveReady SeekComplete DataRequest }
ide: failed opcode was: unknown

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

Re: [PATCH -rt] ide: fix interrupts processing issue with preempt-able hardirqs

[Anton Vorontsov]

On Wed, Jun 25, 2008 at 03:32:12PM +0100, Alan Cox wrote:
> > Note: I don't have any specifications on that ULi bridge, neither I have
> > any schematics for that board (so far, let's hope). So I can't say
> > exactly how things are inter-connected or what these PCI quirks are
> > actually doing (despite few comments in them).
> 
> So you don't for example know if the bridge is correctly configured for
> that device to be edge or level triggered ?

Nope. But I don't think that I can configure it anyway. The thing is
that this particular setup doesn't use ULi's i8259 PIC (it is disabled by
one of PCI quirks), and IDE interrupt is a sideband PCI-E interrupt (also
configured by the PCI quirk). So IDE interrupt is "directly" connected to
OpenPIC interrupt line (through the SOC PCI-E controller, of course).

If that ULi bridge (M1575) provides some other means of configuring, I
could try it... with the specifications.

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

[PATCH v2 -rt] ide: workaround buggy hardware issues with preemptable hardirqs

[Anton Vorontsov]

IDE interrupt handler relies on the fact that, if necessary, hardirqs
will re-trigger on ISR exit. The assumption is valid for level sensitive
interrupts.

But some hardware (namely ULi M5228 in the ULi M1575 "Super South Brige")
behaves in a strange way: it asserts interrupts as edge sensitive. And
because preemptable IRQ handler disables PIC's interrupt, PIC will likely
miss it.

This patch fixes following issue:

ALI15X3: IDE controller (0x10b9:0x5229 rev 0xc8) at  PCI slot 0001:03:1f.0
ALI15X3: 100% native mode on irq 18
ide0: BM-DMA at 0x1120-0x1127, BIOS settings: hda:PIO, hdb:PIO
ide1: BM-DMA at 0x1128-0x112f, BIOS settings: hdc:PIO, hdd:PIO
hda: Optiarc DVD RW AD-7190A, ATAPI CD/DVD-ROM drive
hda: UDMA/66 mode selected
ide0 at 0x1100-0x1107,0x110a on irq 18
ide-cd: cmd 0x5a timed out
hda: lost interrupt
hda: ATAPI 12X DVD-ROM DVD-R-RAM CD-R/RW drive, 2048kB Cache
Uniform CD-ROM driver Revision: 3.20
ide-cd: cmd 0x3 timed out
hda: lost interrupt
ide-cd: cmd 0x3 timed out
hda: lost interrupt
...

It would be great to re-configure the ULi bridge or ULi IDE controller
to behave sanely, but no one knows how or if this is possible at all
(no available specifications).

So.. to workaround the issue IDE interrupt handler should re-check for
any pending IRQs. This isn't bulletproof solution, but it works and this
is the best one we can do.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---

On Wed, Jun 25, 2008 at 04:34:31PM +0400, Anton Vorontsov wrote:
[...]
> The bug, as I see it, in the alim15x3 (ULi M5228) hardware: for some
> reason it does not hold IRQ line, but rises it for some short period
> of time (while the drive itself rises and holds it correctly -- I'm
> seeing it via oscilloscope).
> 
> So this scheme does not work:
> mask_irq()
> ...do something that will trigger IDE interrupt...
> unmask_irq()
> 
> Because at the unmask_irq() time IDE IRQ is gone already, and interrupt
> controller could not notice it (interrupts are level sensitive).
> 
> I did following test: disable RT + insert mask/unmask sequence in the
> IDE IRQ handler, and I got the same behaviour as with RT enabled.
> 
> Also, further testing showed that this issue isn't drive-specific, i.e.
> with a delay inserted before the unmask_irq(), the bug shows with any
> drive I have.
> 
> So, in summary: I think that the patch is still correct as a hw bug
> workaround (I'll need to correct its comments and description though).

Here is updated patch.

 drivers/ide/ide-io.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index 6c1b288..19d36f0 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -1460,6 +1460,7 @@ static void unexpected_intr (int irq, ide_hwgroup_t *hwgroup)
  
 irqreturn_t ide_intr (int irq, void *dev_id)
 {
+	irqreturn_t ret = IRQ_NONE;
 	unsigned long flags;
 	ide_hwgroup_t *hwgroup = (ide_hwgroup_t *)dev_id;
 	ide_hwif_t *hwif;
@@ -1467,12 +1468,13 @@ irqreturn_t ide_intr (int irq, void *dev_id)
 	ide_handler_t *handler;
 	ide_startstop_t startstop;
 
+again:
 	spin_lock_irqsave(&ide_lock, flags);
 	hwif = hwgroup->hwif;
 
 	if (!ide_ack_intr(hwif)) {
 		spin_unlock_irqrestore(&ide_lock, flags);
-		return IRQ_NONE;
+		return ret;
 	}
 
 	if ((handler = hwgroup->handler) == NULL || hwgroup->polling) {
@@ -1510,7 +1512,7 @@ irqreturn_t ide_intr (int irq, void *dev_id)
 #endif /* CONFIG_BLK_DEV_IDEPCI */
 		}
 		spin_unlock_irqrestore(&ide_lock, flags);
-		return IRQ_NONE;
+		return ret;
 	}
 	drive = hwgroup->drive;
 	if (!drive) {
@@ -1532,7 +1534,7 @@ irqreturn_t ide_intr (int irq, void *dev_id)
 		 * enough advance overhead that the latter isn't a problem.
 		 */
 		spin_unlock_irqrestore(&ide_lock, flags);
-		return IRQ_NONE;
+		return ret;
 	}
 	if (!hwgroup->busy) {
 		hwgroup->busy = 1;	/* paranoia */
@@ -1578,7 +1580,17 @@ irqreturn_t ide_intr (int irq, void *dev_id)
 		}
 	}
 	spin_unlock_irqrestore(&ide_lock, flags);
-	return IRQ_HANDLED;
+	ret = IRQ_HANDLED;
+
+	/*
+	 * Previous handler() may have set things up for another interrupt to
+	 * occur soon... with hardirqs preemption we may lose it because of
+	 * buggy hardware that asserts edge-sensitive IRQs, so try again and
+	 * then return gracefully if no IRQs were actually pending.
+	 */
+	if (hardirq_preemption && startstop != ide_stopped)
+		goto again;
+	return ret;
 }
 
 /**
-- 
1.5.5.4

Re: [PATCH v2 -rt] ide: workaround buggy hardware issues with preemptable hardirqs

[Anton Vorontsov]

On Sat, Jun 28, 2008 at 04:54:36AM +0400, Anton Vorontsov wrote:
> IDE interrupt handler relies on the fact that, if necessary, hardirqs
> will re-trigger on ISR exit. The assumption is valid for level sensitive
> interrupts.
> 
> But some hardware (namely ULi M5228 in the ULi M1575 "Super South Brige")
> behaves in a strange way: it asserts interrupts as edge sensitive. And
> because preemptable IRQ handler disables PIC's interrupt, PIC will likely
> miss it.
> 
> This patch fixes following issue:
> 
> ALI15X3: IDE controller (0x10b9:0x5229 rev 0xc8) at  PCI slot 0001:03:1f.0
> ALI15X3: 100% native mode on irq 18
> ide0: BM-DMA at 0x1120-0x1127, BIOS settings: hda:PIO, hdb:PIO
> ide1: BM-DMA at 0x1128-0x112f, BIOS settings: hdc:PIO, hdd:PIO
> hda: Optiarc DVD RW AD-7190A, ATAPI CD/DVD-ROM drive
> hda: UDMA/66 mode selected
> ide0 at 0x1100-0x1107,0x110a on irq 18
> ide-cd: cmd 0x5a timed out
> hda: lost interrupt
> hda: ATAPI 12X DVD-ROM DVD-R-RAM CD-R/RW drive, 2048kB Cache
> Uniform CD-ROM driver Revision: 3.20
> ide-cd: cmd 0x3 timed out
> hda: lost interrupt
> ide-cd: cmd 0x3 timed out
> hda: lost interrupt
> ...
> 
> It would be great to re-configure the ULi bridge or ULi IDE controller
> to behave sanely, but no one knows how or if this is possible at all
> (no available specifications).

Btw, thanks to Jeff Garzik for mentioning this link:

http://gkernel.sourceforge.net/specs/

Though, from a brief glance there is nothing much useful for this ULi
chip. The only thing I gathered is that IDE controller does not provide
any means of configuring its IRQs behaviour, so it is indeed ULi's bridge
problems. It doesn't change anything, just another data point...

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

Re: [PATCH v2 -rt] ide: workaround buggy hardware issues with preemptable hardirqs

[Alan Cox]

> But some hardware (namely ULi M5228 in the ULi M1575 "Super South Brige")
> behaves in a strange way: it asserts interrupts as edge sensitive. And
> because preemptable IRQ handler disables PIC's interrupt, PIC will likely
> miss it.

You mean "I've programmed the hardware wrong"

If your M5228 is in native mode it should be generating a level trigger,
providing you've programmed it in full that way. If you have it in legacy
mode then it honours IDEIRT and you want the relevant PIC/APIC input set
to level.

How to program an IDE controller out of legacy mode is a public open
standard document.

> It would be great to re-configure the ULi bridge or ULi IDE controller
> to behave sanely, but no one knows how or if this is possible at all
> (no available specifications).

You need an NDA with ULi for the documentation or I suspect you can
program the APIC or EISA level registers to match assuming its a PCI like
bridge.

> So.. to workaround the issue IDE interrupt handler should re-check for
> any pending IRQs. This isn't bulletproof solution, but it works and this
> is the best one we can do.

That really does not belong in a mainstream tree.

Alan

Re: [PATCH v2 -rt] ide: workaround buggy hardware issues with preemptable hardirqs

[Alan Cox]

> Though, from a brief glance there is nothing much useful for this ULi
> chip. The only thing I gathered is that IDE controller does not provide
> any means of configuring its IRQs behaviour, so it is indeed ULi's bridge
> problems. It doesn't change anything, just another data point...

Legacy mode - Int line, edge
Native mode - PCI, level

Standard for IDE controllers

Re: [PATCH v2 -rt] ide: workaround buggy hardware issues with preemptable hardirqs

[Sergei Shtylyov]

Hello.

Anton Vorontsov wrote:
> IDE interrupt handler relies on the fact that, if necessary, hardirqs
> will re-trigger on ISR exit. The assumption is valid for level sensitive
> interrupts.
>   

   It's valid for both edge and level triggered interrupts.

> But some hardware (namely ULi M5228 in the ULi M1575 "Super South Brige")
> behaves in a strange way: it asserts interrupts as edge sensitive. And
> because preemptable IRQ handler disables PIC's interrupt, PIC will likely
> miss it.
>   

   Unmasking an IRQ should re-enable an edge detector in a PIC (or that 
detector should even be independent from mask). So, unless an IRQ signal 
is a short pulse (which shouldn't be the case for IDE) it shouldn't be 
missed.

> This patch fixes following issue:
>
> ALI15X3: IDE controller (0x10b9:0x5229 rev 0xc8) at  PCI slot 0001:03:1f.0
> ALI15X3: 100% native mode on irq 18
> ide0: BM-DMA at 0x1120-0x1127, BIOS settings: hda:PIO, hdb:PIO
> ide1: BM-DMA at 0x1128-0x112f, BIOS settings: hdc:PIO, hdd:PIO
> hda: Optiarc DVD RW AD-7190A, ATAPI CD/DVD-ROM drive
> hda: UDMA/66 mode selected
> ide0 at 0x1100-0x1107,0x110a on irq 18
> ide-cd: cmd 0x5a timed out
> hda: lost interrupt
> hda: ATAPI 12X DVD-ROM DVD-R-RAM CD-R/RW drive, 2048kB Cache
> Uniform CD-ROM driver Revision: 3.20
> ide-cd: cmd 0x3 timed out
> hda: lost interrupt
> ide-cd: cmd 0x3 timed out
> hda: lost interrupt
> ...
>
> It would be great to re-configure the ULi bridge or ULi IDE controller
> to behave sanely, but no one knows how or if this is possible at all
> (no available specifications).
>
> So.. to workaround the issue IDE interrupt handler should re-check for
> any pending IRQs. This isn't bulletproof solution, but it works and this
> is the best one we can do

   I remembered a pssible issue with such code: it's possible that BSY 
bit will be cleared before INTRQ assertion (this is a race not spotted 
for a long time by the ATA standards -- and an observed behavior, at 
least of some ATAPI drives), thus a status register read won't clear the 
"interrupt pending" condition within drive.

MBR, Sergei

Re: [PATCH v2 -rt] ide: workaround buggy hardware issues with preemptable hardirqs

[Sergei Shtylyov]

Hello.

Alan Cox wrote:
>> But some hardware (namely ULi M5228 in the ULi M1575 "Super South Brige")
>> behaves in a strange way: it asserts interrupts as edge sensitive. And
>> because preemptable IRQ handler disables PIC's interrupt, PIC will likely
>> miss it.
>>     
>
> You mean "I've programmed the hardware wrong"
>
> If your M5228 is in native mode it should be generating a level trigger,
> providing you've programmed it in full that way. If you have it in legacy
>   

   His boot log shows the native mode.
   And you don't have much control about the interrupt sense at the IDE 
controller side, you can only select legacy/native (which would mean 
edge/level IRQs respectively) mode but the BIOS has a freedom to say 
misprogram ELCR for the PCI interrupt the controller is using.

> mode then it honours IDEIRT and you want the relevant PIC/APIC input set
> to level.
>
>   

   What's IDEIRT, some ISA bridge register? And why should one set 
[A]PIC to level mode for legacy mode IDE? :-O

> How to program an IDE controller out of legacy mode is a public open
> standard document.
>   

   It's *not* in legacy mode, boot log shows "100# native mode".

>> It would be great to re-configure the ULi bridge or ULi IDE controller
>> to behave sanely, but no one knows how or if this is possible at all
>> (no available specifications).
>>     
>
> You need an NDA with ULi for the documentation or I suspect you can
> program the APIC or EISA level registers to match assuming its a PCI like
> bridge.
>   

   He has an OpenPIC, it's PowerPC SoC, so no ELCR either. Anton said to 
me that OpenPIC inputs from PCI IRQs are correctly programmed for level 
trigger.
Nothe that this ULi chip is on PCI Express, so maybe something is wrong 
with how IRQs are delivered over it to the SoC's PCIE controller

MBR, Sergei

Re: [PATCH v2 -rt] ide: workaround buggy hardware issues with preemptable hardirqs

[Anton Vorontsov]

On Sat, Jun 28, 2008 at 02:30:36PM +0400, Sergei Shtylyov wrote:
> Hello.
>
> Anton Vorontsov wrote:
>> IDE interrupt handler relies on the fact that, if necessary, hardirqs
>> will re-trigger on ISR exit. The assumption is valid for level sensitive
>> interrupts.
>>   
>
>   It's valid for both edge and level triggered interrupts.

I think this is depends on PIC, whether it can or can not detect masked
edge interrupts...

>> But some hardware (namely ULi M5228 in the ULi M1575 "Super South Brige")
>> behaves in a strange way: it asserts interrupts as edge sensitive. And
>> because preemptable IRQ handler disables PIC's interrupt, PIC will likely
>> miss it.
>>   
>
>   Unmasking an IRQ should re-enable an edge detector in a PIC (or that  
> detector should even be independent from mask).

Should? Hm.. well, I can easily check it. Will just program the MPIC IRQ
to edge sensitive, and see if it fixes the problem (not sure if I already
tried this, I think I did try).

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

Re: [PATCH v2 -rt] ide: workaround buggy hardware issues with preemptable hardirqs

[Sergei Shtylyov]

Hello.

Anton Vorontsov wrote:
>>> But some hardware (namely ULi M5228 in the ULi M1575 "Super South Brige")
>>> behaves in a strange way: it asserts interrupts as edge sensitive. And
>>> because preemptable IRQ handler disables PIC's interrupt, PIC will likely
>>> miss it.
>>>   
>>>       
>>   Unmasking an IRQ should re-enable an edge detector in a PIC (or that  
>> detector should even be independent from mask).
>>     
> Should? Hm.. well, I can easily check it. Will just program the MPIC IRQ
> to edge sensitive, and see if it fixes the problem (not sure if I already
> tried this, I think I did try).
>   

   That will not help if edge detector is not active for masked out IRQs 
(that's how i8259 behaves, IIRC) *and* you're getting the short IRQ 
"pulses" but should help if IRQ is generated normally -- though in this 
case the level trigger mode (with the same polarity) should work as well...

MBR, Sergei

Re: [PATCH v2 -rt] ide: workaround buggy hardware issues with preemptable hardirqs

[Alan Cox]

>    His boot log shows the native mode.

Then the bridge is misprogrammed

>    What's IDEIRT, some ISA bridge register? And why should one set 
> [A]PIC to level mode for legacy mode IDE? :-O

You need an NDA and then they'll send you the data sheet for the bridges
and other logic. It's all programmable.

"He has an OpenPIC, it's PowerPC SoC, so no ELCR either"

Really - he's using a designed for PC ISA bridge etc.. he might have all
sorts of stuff on it.

Alan

Re: [PATCH v2 -rt] ide: workaround buggy hardware issues with preemptable hardirqs

[Sergei Shtylyov]

Alan Cox wrote:

>>   His boot log shows the native mode.

> Then the bridge is misprogrammed

>>   What's IDEIRT, some ISA bridge register? And why should one set 
>>[A]PIC to level mode for legacy mode IDE? :-O

> You need an NDA and then they'll send you the data sheet for the bridges
> and other logic. It's all programmable.

    That could be a very long story... :-|

> "He has an OpenPIC, it's PowerPC SoC, so no ELCR either"

> Really - he's using a designed for PC ISA bridge etc.. he might have all
> sorts of stuff on it.

   Ah, indeed. I meant to say that 8259 and ELCR are bypassed for PCI IRQs (or 
at least Anton says so :-).

> Alan

MBR, Sergei

Re: [PATCH v2 -rt] ide: workaround buggy hardware issues with preemptable hardirqs

[Anton Vorontsov]

On Sun, Jun 29, 2008 at 05:17:38PM +0400, Sergei Shtylyov wrote:
> Alan Cox wrote:
>
>>>   His boot log shows the native mode.
>
>> Then the bridge is misprogrammed
>
>>>   What's IDEIRT, some ISA bridge register? And why should one set  
>>> [A]PIC to level mode for legacy mode IDE? :-O
>
>> You need an NDA and then they'll send you the data sheet for the bridges
>> and other logic. It's all programmable.
>
>    That could be a very long story... :-|
>
>> "He has an OpenPIC, it's PowerPC SoC, so no ELCR either"
>
>> Really - he's using a designed for PC ISA bridge etc.. he might have all
>> sorts of stuff on it.
>
>   Ah, indeed. I meant to say that 8259 and ELCR are bypassed for PCI IRQs 
> (or at least Anton says so :-).

Yes, we don't use 8259 for the IDE interrupt.

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

Re: [PATCH v2 -rt] ide: workaround buggy hardware issues with preemptable hardirqs

[Benjamin Herrenschmidt]

On Sat, 2008-06-28 at 04:54 +0400, Anton Vorontsov wrote:
> IDE interrupt handler relies on the fact that, if necessary, hardirqs
> will re-trigger on ISR exit. The assumption is valid for level sensitive
> interrupts.
> 
> But some hardware (namely ULi M5228 in the ULi M1575 "Super South Brige")
> behaves in a strange way: it asserts interrupts as edge sensitive. And
> because preemptable IRQ handler disables PIC's interrupt, PIC will likely
> miss it.

Don't we replay edge IRQs that happen while soft-disabled ? Could be a
bug in your PIC code not to do so...

Ben.

> This patch fixes following issue:
> 
> ALI15X3: IDE controller (0x10b9:0x5229 rev 0xc8) at  PCI slot 0001:03:1f.0
> ALI15X3: 100% native mode on irq 18
> ide0: BM-DMA at 0x1120-0x1127, BIOS settings: hda:PIO, hdb:PIO
> ide1: BM-DMA at 0x1128-0x112f, BIOS settings: hdc:PIO, hdd:PIO
> hda: Optiarc DVD RW AD-7190A, ATAPI CD/DVD-ROM drive
> hda: UDMA/66 mode selected
> ide0 at 0x1100-0x1107,0x110a on irq 18
> ide-cd: cmd 0x5a timed out
> hda: lost interrupt
> hda: ATAPI 12X DVD-ROM DVD-R-RAM CD-R/RW drive, 2048kB Cache
> Uniform CD-ROM driver Revision: 3.20
> ide-cd: cmd 0x3 timed out
> hda: lost interrupt
> ide-cd: cmd 0x3 timed out
> hda: lost interrupt
> ...
> 
> It would be great to re-configure the ULi bridge or ULi IDE controller
> to behave sanely, but no one knows how or if this is possible at all
> (no available specifications).
> 
> So.. to workaround the issue IDE interrupt handler should re-check for
> any pending IRQs. This isn't bulletproof solution, but it works and this
> is the best one we can do.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> 
> On Wed, Jun 25, 2008 at 04:34:31PM +0400, Anton Vorontsov wrote:
> [...]
> > The bug, as I see it, in the alim15x3 (ULi M5228) hardware: for some
> > reason it does not hold IRQ line, but rises it for some short period
> > of time (while the drive itself rises and holds it correctly -- I'm
> > seeing it via oscilloscope).
> > 
> > So this scheme does not work:
> > mask_irq()
> > ...do something that will trigger IDE interrupt...
> > unmask_irq()
> > 
> > Because at the unmask_irq() time IDE IRQ is gone already, and interrupt
> > controller could not notice it (interrupts are level sensitive).
> > 
> > I did following test: disable RT + insert mask/unmask sequence in the
> > IDE IRQ handler, and I got the same behaviour as with RT enabled.
> > 
> > Also, further testing showed that this issue isn't drive-specific, i.e.
> > with a delay inserted before the unmask_irq(), the bug shows with any
> > drive I have.
> > 
> > So, in summary: I think that the patch is still correct as a hw bug
> > workaround (I'll need to correct its comments and description though).
> 
> Here is updated patch.
> 
>  drivers/ide/ide-io.c |   20 ++++++++++++++++----
>  1 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
> index 6c1b288..19d36f0 100644
> --- a/drivers/ide/ide-io.c
> +++ b/drivers/ide/ide-io.c
> @@ -1460,6 +1460,7 @@ static void unexpected_intr (int irq, ide_hwgroup_t *hwgroup)
>   
>  irqreturn_t ide_intr (int irq, void *dev_id)
>  {
> +	irqreturn_t ret = IRQ_NONE;
>  	unsigned long flags;
>  	ide_hwgroup_t *hwgroup = (ide_hwgroup_t *)dev_id;
>  	ide_hwif_t *hwif;
> @@ -1467,12 +1468,13 @@ irqreturn_t ide_intr (int irq, void *dev_id)
>  	ide_handler_t *handler;
>  	ide_startstop_t startstop;
>  
> +again:
>  	spin_lock_irqsave(&ide_lock, flags);
>  	hwif = hwgroup->hwif;
>  
>  	if (!ide_ack_intr(hwif)) {
>  		spin_unlock_irqrestore(&ide_lock, flags);
> -		return IRQ_NONE;
> +		return ret;
>  	}
>  
>  	if ((handler = hwgroup->handler) == NULL || hwgroup->polling) {
> @@ -1510,7 +1512,7 @@ irqreturn_t ide_intr (int irq, void *dev_id)
>  #endif /* CONFIG_BLK_DEV_IDEPCI */
>  		}
>  		spin_unlock_irqrestore(&ide_lock, flags);
> -		return IRQ_NONE;
> +		return ret;
>  	}
>  	drive = hwgroup->drive;
>  	if (!drive) {
> @@ -1532,7 +1534,7 @@ irqreturn_t ide_intr (int irq, void *dev_id)
>  		 * enough advance overhead that the latter isn't a problem.
>  		 */
>  		spin_unlock_irqrestore(&ide_lock, flags);
> -		return IRQ_NONE;
> +		return ret;
>  	}
>  	if (!hwgroup->busy) {
>  		hwgroup->busy = 1;	/* paranoia */
> @@ -1578,7 +1580,17 @@ irqreturn_t ide_intr (int irq, void *dev_id)
>  		}
>  	}
>  	spin_unlock_irqrestore(&ide_lock, flags);
> -	return IRQ_HANDLED;
> +	ret = IRQ_HANDLED;
> +
> +	/*
> +	 * Previous handler() may have set things up for another interrupt to
> +	 * occur soon... with hardirqs preemption we may lose it because of
> +	 * buggy hardware that asserts edge-sensitive IRQs, so try again and
> +	 * then return gracefully if no IRQs were actually pending.
> +	 */
> +	if (hardirq_preemption && startstop != ide_stopped)
> +		goto again;
> +	return ret;
>  }
>  
>  /**