[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 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

[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

[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 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

[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 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 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

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

[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]

> 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

[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

[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

[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

[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

[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

[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;
>  }
>  
>  /**

[RT] MPIC edge sensitive issues with hardirq preemption (was: Re: [PATCH v2 -rt] ide: workaround buggy hardware issues with preemptable hardirqs)

[Anton Vorontsov]

On Mon, Jun 30, 2008 at 09:26:14AM +1000, Benjamin Herrenschmidt wrote:
> 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...
	
Thanks for the idea. With hardirq preempton, fasteoi path does not replay
edge interrupts indeed (at least for MPIC).

Here how I tested this: I have external interrupt connected to the button
on this board, thus I registered irq handler which is doing exactly this
(irq is edge sensitive):

printk("handled\n"); mdelay(2000);

Without hardirq preemption: pressing button twice prints two messages.
With hardirq preemption: pressing button twice prints just one message.

This happens because:
- irq has come;
- fasteoi handler mask()s it, and wakes up the thread;
- thread calls irq handler;
- another IRQ is coming, but MPIC's IRQ is still masked and MPIC does not
  see it.
- thread unmasks the IRQ, second IRQ got lost.

But how this could be a bug in the PIC code? IMO this is a bug in the
kernel/irq code, since it assumes that fasteoi PIC will retrigger masked
edge sources... This isn't true for at least MPIC. To make this work for
all fasteoi PICs, we should mask edge sensitive interrupts very very
carefully.

Also...

I found that I completely messed with MPIC's sense types: my brain was
jammed with the idea that MPIC irq type 0 is low level sensitive, but the
true thing is that it is rising edge sensitive. (Ah, I know where I got
confused, type 0 is active-low for ISA PICs).

So in all my previous emails I was wrong when I was saying "mpic is
programmed to low level sensitive". It was programmed for rising edge
sensitive. An all my further reasonings were flawed because of this.

Re-programming MPIC to high level sensitive also makes IDE work. But
this doesn't mean that IRQ code is correct.

So, in the end, this isn't IDE issue... Much thanks to everyone for
ideas and help... instead of one bug, it seems I've found two. :-)
None in the IDE code.


Following patch fixes MPIC edge sensitive interrupts processing with
hardirqs preemption.

We're still using handle_fasteoi_irq, but passing control to
thread_edge_irq if interrupt is edge sensitive. Otherwise usual fasteoi
path handles the IRQ.

Plys, we're masking edge interrupt _and_ marking it as pending and masked
only if we're already handling one (exactly as we do with !preemptable
hardirqs). And the trick is that thread_edge_irq() will unmask it before
executing handle_IRQ_event.


So here is the patch, how does it look?

(quite weird that I have to pass irq trigger flags to desc->status,
but really, there is no other way. it seems safe though, because
all irqactions should conform only to one trigger type).


diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 9aa6b24..6c9f4ae 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -134,6 +134,10 @@ int set_irq_type(unsigned int irq, unsigned int type)
 	if (desc->chip->set_type) {
 		spin_lock_irqsave(&desc->lock, flags);
 		ret = desc->chip->set_type(irq, type);
+		if (!ret) {
+			desc->status &= ~IRQ_TYPE_SENSE_MASK;
+			desc->status |= type & IRQ_TYPE_SENSE_MASK;
+		}
 		spin_unlock_irqrestore(&desc->lock, flags);
 	}
 	return ret;
@@ -430,7 +434,7 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc)
 	action = desc->action;
 	if (unlikely(!action || (desc->status & (IRQ_INPROGRESS |
 						 IRQ_DISABLED)))) {
-		desc->status |= IRQ_PENDING;
+		desc->status |= IRQ_PENDING | IRQ_MASKED;
 		if (desc->chip->mask)
 			desc->chip->mask(irq);
 		goto out;
@@ -439,9 +443,10 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc)
 	desc->status |= IRQ_INPROGRESS;
 	/*
 	 * In the threaded case we fall back to a mask+eoi sequence:
+	 * Though, we don't mask edge interrupts, thread may lose it then.
 	 */
 	if (redirect_hardirq(desc)) {
-		if (desc->chip->mask)
+		if (!(desc->status & IRQ_TYPE_EDGE_BOTH) && desc->chip->mask)
 			desc->chip->mask(irq);
 		goto out;
 	}
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index c30aa1e..ffeb87f 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -385,10 +385,11 @@ int setup_irq(unsigned int irq, struct irqaction *new)
 
 		/* Setup the type (level, edge polarity) if configured: */
 		if (new->flags & IRQF_TRIGGER_MASK) {
-			if (desc->chip && desc->chip->set_type)
+			if (desc->chip && desc->chip->set_type) {
 				desc->chip->set_type(irq,
 						new->flags & IRQF_TRIGGER_MASK);
-			else
+				desc->status |= new->flags & IRQF_TRIGGER_MASK;
+			} else
 				/*
 				 * IRQF_TRIGGER_* but the PIC does not support
 				 * multiple flow-types?
@@ -772,9 +773,12 @@ static void do_hardirq(struct irq_desc *desc)
 		thread_simple_irq(desc);
 	else if (desc->handle_irq == handle_level_irq)
 		thread_level_irq(desc);
-	else if (desc->handle_irq == handle_fasteoi_irq)
-		thread_fasteoi_irq(desc);
-	else if (desc->handle_irq == handle_edge_irq)
+	else if (desc->handle_irq == handle_fasteoi_irq) {
+		if (desc->status & IRQ_TYPE_EDGE_BOTH)
+			thread_edge_irq(desc);
+		else
+			thread_fasteoi_irq(desc);
+	} else if (desc->handle_irq == handle_edge_irq)
 		thread_edge_irq(desc);
 	else
 		thread_do_irq(desc);

Re: [RT] MPIC edge sensitive issues with hardirq preemption (was: Re: [PATCH v2 -rt] ide: workaround buggy hardware issues with preemptable hardirqs)

[Benjamin Herrenschmidt]

> 	
> Thanks for the idea. With hardirq preempton, fasteoi path does not replay
> edge interrupts indeed (at least for MPIC).
> 
> Here how I tested this: I have external interrupt connected to the button
> on this board, thus I registered irq handler which is doing exactly this
> (irq is edge sensitive):
> 
> printk("handled\n"); mdelay(2000);
> 
> Without hardirq preemption: pressing button twice prints two messages.
> With hardirq preemption: pressing button twice prints just one message.
> 
> This happens because:
> - irq has come;
> - fasteoi handler mask()s it, and wakes up the thread;

Do we eoi first ? We should.

> - thread calls irq handler;
> - another IRQ is coming, but MPIC's IRQ is still masked and MPIC does not
>   see it.
> - thread unmasks the IRQ, second IRQ got lost.
>
> But how this could be a bug in the PIC code? IMO this is a bug in the
> kernel/irq code, since it assumes that fasteoi PIC will retrigger masked
> edge sources... This isn't true for at least MPIC. To make this work for
> all fasteoi PICs, we should mask edge sensitive interrupts very very
> carefully.

Well, retriggering of edge irqs while masked is some new "assumption"
that I deduced from your explanations of how RT works and indeed is not
provided by most PICs which will disable edge detection on masked
interrupts...

I'll double check if the MPIC/OpenPIC spec says anything about this.

I think it's a bug in the RT code to assume that. It should not mask
edge interrupts basically. That does mean that it would have to
differenciate edge and level, and thus can't use the same fasteoi
handler for both I suppose...

> Also...
> 
> I found that I completely messed with MPIC's sense types: my brain was
> jammed with the idea that MPIC irq type 0 is low level sensitive, but the
> true thing is that it is rising edge sensitive. (Ah, I know where I got
> confused, type 0 is active-low for ISA PICs).
> 
> So in all my previous emails I was wrong when I was saying "mpic is
> programmed to low level sensitive". It was programmed for rising edge
> sensitive. An all my further reasonings were flawed because of this.
> 
> Re-programming MPIC to high level sensitive also makes IDE work. But
> this doesn't mean that IRQ code is correct.
> 
> So, in the end, this isn't IDE issue... Much thanks to everyone for
> ideas and help... instead of one bug, it seems I've found two. :-)
> None in the IDE code.
> 
> 
> Following patch fixes MPIC edge sensitive interrupts processing with
> hardirqs preemption.
> 
> We're still using handle_fasteoi_irq, but passing control to
> thread_edge_irq if interrupt is edge sensitive. Otherwise usual fasteoi
> path handles the IRQ.
> 
> Plys, we're masking edge interrupt _and_ marking it as pending and masked
> only if we're already handling one (exactly as we do with !preemptable
> hardirqs). And the trick is that thread_edge_irq() will unmask it before
> executing handle_IRQ_event.
> 
> 
> So here is the patch, how does it look?
> 
> (quite weird that I have to pass irq trigger flags to desc->status,
> but really, there is no other way. it seems safe though, because
> all irqactions should conform only to one trigger type).

I'll let Thomas and Ingo provide feedback here...

Ben.

Re: [RT] MPIC edge sensitive issues with hardirq preemption (was: Re: [PATCH v2 -rt] ide: workaround buggy hardware issues with preemptable hardirqs)

[Anton Vorontsov]

On Tue, Jul 01, 2008 at 07:59:57AM +1000, Benjamin Herrenschmidt wrote:
> > 	
> > Thanks for the idea. With hardirq preempton, fasteoi path does not replay
> > edge interrupts indeed (at least for MPIC).
> > 
> > Here how I tested this: I have external interrupt connected to the button
> > on this board, thus I registered irq handler which is doing exactly this
> > (irq is edge sensitive):
> > 
> > printk("handled\n"); mdelay(2000);
> > 
> > Without hardirq preemption: pressing button twice prints two messages.
> > With hardirq preemption: pressing button twice prints just one message.
> > 
> > This happens because:
> > - irq has come;
> > - fasteoi handler mask()s it, and wakes up the thread;
> 
> Do we eoi first ? We should.

Yup. I just left over this small detail.

Without my patch the code was firstly masking the IRQ, then sending EOI.
Now we don't mask it, but simply send an EOI. If the edge IRQ came again
(while we were processing the same IRQ), non-threaded handler marks IRQ
that it should be replayed, then masks it and sends EOI. When thread
awakes, it unmasks the IRQ before handle_IRQ_event, so that we can catch
next edge IRQ. That is, we handle EOI quite correctly.

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

Re: [RT] MPIC edge sensitive issues with hardirq preemption

[Sergei Shtylyov]

Hello.

Anton Vorontsov wrote:

> But how this could be a bug in the PIC code? IMO this is a bug in the
> kernel/irq code, since it assumes that fasteoi PIC will retrigger masked
> edge sources... This isn't true for at least MPIC. To make this work for
> all fasteoi PICs, we should mask edge sensitive interrupts very very
> carefully.
>   

   I guess it assumed this based on 8259's behavior (not sure about I/O 
APIC).
   Hm, but the 8259 code never used "fasteoi" path for some obscure 
reason...

> jammed with the idea that MPIC irq type 0 is low level sensitive, but the
> true thing is that it is rising edge sensitive. (Ah, I know where I got
> confused, type 0 is active-low for ISA PICs).
>   

    You mean in the device tree?

> So in all my previous emails I was wrong when I was saying "mpic is
> programmed to low level sensitive". It was programmed for rising edge
> sensitive. An all my further reasonings were flawed because of this.
>   

   Gah.  I'm surprised how it could work at all then...

> Re-programming MPIC to high level sensitive also makes IDE work. But
> this doesn't mean that IRQ code is correct.
>   

   I wonder why. :-O
   Your ULi IDE is in native mode, so it should be generating a PCI 
interrupt -- which is *low* level sensitive.

MBR, Sergei

Re: [RT] MPIC edge sensitive issues with hardirq preemption

[Anton Vorontsov]

On Tue, Jul 01, 2008 at 02:41:19PM +0400, Sergei Shtylyov wrote:
> Hello.
>
> Anton Vorontsov wrote:
>
>> But how this could be a bug in the PIC code? IMO this is a bug in the
>> kernel/irq code, since it assumes that fasteoi PIC will retrigger masked
>> edge sources... This isn't true for at least MPIC. To make this work for
>> all fasteoi PICs, we should mask edge sensitive interrupts very very
>> carefully.
>>   
>
>   I guess it assumed this based on 8259's behavior (not sure about I/O  
> APIC).
>   Hm, but the 8259 code never used "fasteoi" path for some obscure  
> reason...
>
>> jammed with the idea that MPIC irq type 0 is low level sensitive, but the
>> true thing is that it is rising edge sensitive. (Ah, I know where I got
>> confused, type 0 is active-low for ISA PICs).
>>   
>
>    You mean in the device tree?

Yes, simply changing device tree.

>> So in all my previous emails I was wrong when I was saying "mpic is
>> programmed to low level sensitive". It was programmed for rising edge
>> sensitive. An all my further reasonings were flawed because of this.
>>   
>
>   Gah.  I'm surprised how it could work at all then...
>
>> Re-programming MPIC to high level sensitive also makes IDE work. But
>> this doesn't mean that IRQ code is correct.
>>   
>
>   I wonder why. :-O
>   Your ULi IDE is in native mode, so it should be generating a PCI  
> interrupt -- which is *low* level sensitive.

Exactly. This is another reason why I was so confused.

-- 
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 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