ata_piix: detect and clear spurious IRQs
PATCH 1/1 ata_piix: detect and clear spurious IRQs
The DMA_IRQ bit in the bmdma status register is always set when IDEIRQ is asserted allowing spurious IRQ detection. Detect spurious IRQs and clear them. This protects ata_piix against nobody-cared which gets reported not so rarely.
--- drivers/ata/ata_piix.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-)
drivers/ata/ata_piix.c +53 -1
@@ -922,6 +922,58 @@ static int piix_sidpr_scr_read(struct ata_link *link,return 0 ; } +static irqreturn_t piix_interrupt (int irq,void *dev_instance) +{ +struct ata_host *host = dev_instance; +unsigned int i; +unsigned int handled =0 ; +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); + +for (i =0 ; i < host->n_ports; i++) { +struct ata_port *ap = host->ports[i]; +struct ata_queued_cmd *qc; +u8 host_stat; + +if (ata_port_is_dummy (ap)) +continue ; + + qc =ata_qc_from_tag (ap, ap->link.active_tag); +if (qc && !(qc->tf.flags &ATA_TFLAG_POLLING )) { + handled |=ata_sff_host_intr (ap, qc); +continue ; + } + +/* +* Control reaches here if HSM is not expecting IRQ. +* If the controller is actually asserting IRQ line, +* this will lead to nobody cared. Fortuantely, +* DMA_INTR of PIIX is set whenever IDEIRQ is set so +* it can be used to detect spurious IRQs. As the +* driver is not expecting IRQ at all, clearing IRQ +* here won't lead to loss of IRQ event. +*/ +if (unlikely (!ap->ioaddr.bmdma_addr)) +continue ; + + host_stat = ap->ops->bmdma_status (ap); +if (!(host_stat &ATA_DMA_INTR )) +continue ; + +if (printk_ratelimit ()) +ata_port_printk (ap,KERN_INFO , +"clearing spurious IRQ\n" ); + ap->ops->sff_check_status (ap); + ap->ops->sff_irq_clear (ap); + handled |=1 ; + } + +spin_unlock_irqrestore (&host->lock, flags); + +return IRQ_RETVAL (handled); +} +static int piix_sidpr_scr_write (struct ata_link *link,unsigned int reg,u32 val) {
@@ -1470,7 +1522,7 @@ static int __devinit piix_init_one(struct pci_dev *pdev, }pci_set_master (pdev); -return ata_pci_sff_activate_host (host, ata_sff_interrupt, &piix_sht); +return ata_pci_sff_activate_host (host, piix_interrupt, &piix_sht); }static int __init piix_init (void )
Quoted 4 lines · On Fri, 21 Nov 2008 13:13:06 +0900 Tejun Heo <[email protected]> wrote:
The
DMA_IRQbit in the bmdma status register is always set when IDEIRQ is asserted allowing spurious IRQ detection. Detect spurious IRQs and clear them. This protectsata_piixagainst nobody-cared which gets reported not so rarely.
Various controllers have the ability to report the IRQ more reliably in similar fashion, should this not be part of ata_sff_interrupt with an optional ops->irq_pending call ?
Quoted 11 lines · Alan Cox
Quoted 4 lines · On Fri, 21 Nov 2008 13:13:06 +0900 Tejun Heo <[email protected]> wrote:
The
DMA_IRQbit in the bmdma status register is always set when IDEIRQ is asserted allowing spurious IRQ detection. Detect spurious IRQs and clear them. This protectsata_piixagainst nobody-cared which gets reported not so rarely.Various controllers have the ability to report the IRQ more reliably in similar fashion, should this not be part of
ata_sff_interruptwith an optionalops->irq_pendingcall ?
There are? That's a good news. Sure, if it's a common thing, we can definitely make it a callback. It actually goes together with ->sff_irq_clear pretty well.
Thanks.
Signature · 1 line
tejun
Quoted 11 lines · Alan Cox
Quoted 4 lines · On Fri, 21 Nov 2008 13:13:06 +0900 Tejun Heo <[email protected]> wrote:
The
DMA_IRQbit in the bmdma status register is always set when IDEIRQ is asserted allowing spurious IRQ detection. Detect spurious IRQs and clear them. This protectsata_piixagainst nobody-cared which gets reported not so rarely.Various controllers have the ability to report the IRQ more reliably in similar fashion, should this not be part of
ata_sff_interruptwith an optionalops->irq_pendingcall ?
Though I'm in general agreement with this sub-thread of opinions, I do note that, in the past, I have purposefully avoided an ->irq_pending.
I always felt the alternative -- writing a small irq handler function specifically for that controller -- was a much more flexible and powerful method of producing the desired result.
A separate interrupt function can, for example, perform an MMIO read to check irq-pending, outside of a spinlock. ->irq_pending callback is a bit more constraining.
In terms of implementation, we could probably collapse all the non-controller-specific behavior into a single function call or macro, performed inside the custom interrupt handling routine.
Jeff
Hello, Jeff.
Quoted 16 lines · Jeff Garzik
Quoted 11 lines · Alan Cox
Quoted 4 lines · On Fri, 21 Nov 2008 13:13:06 +0900 Tejun Heo <[email protected]> wrote:
The
DMA_IRQbit in the bmdma status register is always set when IDEIRQ is asserted allowing spurious IRQ detection. Detect spurious IRQs and clear them. This protectsata_piixagainst nobody-cared which gets reported not so rarely.Various controllers have the ability to report the IRQ more reliably in similar fashion, should this not be part of
ata_sff_interruptwith an optionalops->irq_pendingcall ?Though I'm in general agreement with this sub-thread of opinions, I do note that, in the past, I have purposefully avoided an ->irq_pending.
Quoted 7 lines
I always felt the alternative -- writing a small irq handler function specifically for that controller -- was a much more flexible and powerful method of producing the desired result.
A separate interrupt function can, for example, perform an MMIO read to check irq-pending, outside of a spinlock. ->irq_pending callback is a bit more constraining.
If we call ->irq_pending() only when no qc is in flight, I don't think there will be any noticeable performance penalty when the IRQ pending register is per-port. If pending status of all ports can be determined by single read, having a separate handler is a good idea. It all depends on how controllers actually implement them.
All BMDMA controllers I know about are sata_sil (already has private irq handler) and ata_piix (this patch). Alan, how do other controllers do it?
Quoted 3 lines
In terms of implementation, we could probably collapse all the non-controller-specific behavior into a single function call or macro, performed inside the custom interrupt handling routine.
->irq_clear() is tightly bound to ->irq_pending(). Drivers which don't support ->irq_pending() probably wouldn't support or need ->irq_clear() either, but still, I can't think of one good location. What's on your mind?
Thanks.
Signature · 1 line
tejun
Quoted 6 lines
Quoted 3 lines
A separate interrupt function can, for example, perform an MMIO read to check irq-pending, outside of a spinlock. ->irq_pending callback is a bit more constraining.
If we call ->irq_pending() only when no qc is in flight, I don't think there will be any noticeable performance penalty when the IRQ pending
Agreed
Quoted 3 lines
All BMDMA controllers I know about are
sata_sil(already has private irq handler) andata_piix(this patch). Alan, how do other controllers do it?
CMD chipsets do a register read .. from PCI config space (gak) SIL680 is similar
SI3112 adds the fact you need to keep at the PHY as well. Promise uses a magic register at dmabase + 0x1D which holds irq bits
All much the same. What I'd like to understand better is why we need any of these ...
Alan
Hello.
Quoted 8 lines · Alan Cox
Quoted 4 lines
All BMDMA controllers I know about are
sata_sil(already has private irq handler) andata_piix(this patch). Alan, how do other controllers do it?CMD chipsets do a register read .. from PCI config space (gak) SIL680 is similar
Similar to the chips driven by sata_sil you mean?
Both 680 and 3112 certainly have the INTRQ status in the MMIO
register (yes, alternatively seen thru the PCI config space). IIRC it's not a latched status, so doesn't require clearing...
Quoted 2 lines
Promise uses a magic register at dmabase +
0x1Dwhich holds irq bits
Yes, if you mean the chips driven by [pata_]pdc202xx_old -- don't
know about SATA.
MBR, Sergei
Hello.
Quoted 7 lines · Alan Cox
Quoted 4 lines
All BMDMA controllers I know about are
sata_sil(already has private irq handler) andata_piix(this patch). Alan, how do other controllers do it?CMD chipsets do a register read .. from PCI config space (gak)
The late chips do have the alternate interrupt latch bits in I/O
space (at BMIDE base + 3).
MBR, Sergei
Quoted 10 lines · Hello, I wrote:
Quoted 6 lines
Quoted 3 lines
irq handler) and
ata_piix(this patch). Alan, how do other controllers do it?CMD chipsets do a register read .. from PCI config space (gak)
All BMDMA controllers I know about are
sata_sil(already has privateThe late chips do have the alternate interrupt latch bits in I/O
space (at BMIDE base + 3).
Just looked into th driver, so to be correct the MRDMODE register is
at BMIDE base + 1.
MBR, Sergei
Quoted 11 lines · Alan Cox
Quoted 3 lines
All BMDMA controllers I know about are
sata_sil(already has private irq handler) andata_piix(this patch). Alan, how do other controllers do it?CMD chipsets do a register read .. from PCI config space (gak) SIL680 is similar
SI3112 adds the fact you need to keep at the PHY as well. Promise uses a magic register at dmabase +0x1Dwhich holds irq bitsWhat I'd like to understand better is why we need any of these ...
Indeed!
Jeff
Quoted 10 lines · Jeff Garzik
Quoted 9 lines · Alan Cox
Quoted 3 lines
All BMDMA controllers I know about are
sata_sil(already has private irq handler) andata_piix(this patch). Alan, how do other controllers do it?CMD chipsets do a register read .. from PCI config space (gak) SIL680 is similar
SI3112 adds the fact you need to keep at the PHY as well. Promise uses a magic register at dmabase +0x1Dwhich holds irq bits
Quoted 4 lines
Quoted 2 lines
What I'd like to understand better is why we need any of these ...
Indeed!
Well, this patch was necessary for a netbook with PATA SSD. The ATA implementation of the SSD was quite flaky and under certain circumstances, during resume SRST, the drive would raise the IRQ line regardless of NIEN and will stay that way for several seconds, thus triggering nobody cared. Other than detecting and clearing the spurious IRQ, there just isn't much driver can do to work around problems like this.
Thanks.
Signature · 1 line
tejun
Quoted 5 lines
circumstances, during resume SRST, the drive would raise the IRQ line regardless of NIEN and will stay that way for several seconds, thus triggering nobody cared. Other than detecting and clearing the spurious IRQ, there just isn't much driver can do to work around problems like this.
There is. It also means your patch isn't sufficient - if that IRQ had been level triggered you'd have hung the box solid.
The old IDE code makes use of disable_irq/enable_irq (and really ought to make use of on chip private IRQ mask bits as first choice but doesn't).
Sounds to me like there are two things we can do to help
Make the nIEN masking via a helper that can on suitable chips also mask on chip.
Make use of disable_irq_nosync() in the case of devices that ignore nIEN in the IRQ handler, setting a flag then re-enable it when the driver path completes the reset and the bit goes clear.
We need to start using enable/disable_irq and/or local chip IRQ masking for PIO in some places anyway.
Alan
Quoted 8 lines · Alan Cox
Quoted 5 lines
circumstances, during resume SRST, the drive would raise the IRQ line regardless of NIEN and will stay that way for several seconds, thus triggering nobody cared. Other than detecting and clearing the spurious IRQ, there just isn't much driver can do to work around problems like this.
There is. It also means your patch isn't sufficient - if that IRQ had been level triggered you'd have hung the box solid.
Right, the attached patch fixed it so it must have been the controller latching the IRQ.
Quoted 8 lines
The old IDE code makes use of disable_irq/enable_irq (and really ought to make use of on chip private IRQ mask bits as first choice but doesn't).
Sounds to me like there are two things we can do to help
Make the nIEN masking via a helper that can on suitable chips also mask on chip.
Quoted 6 lines
Make use of
disable_irq_nosync()in the case of devices that ignore nIEN in the IRQ handler, setting a flag then re-enable it when the driver path completes the reset and the bit goes clear.We need to start using enable/disable_irq and/or local chip IRQ masking for PIO in some places anyway.
Agreed. I think we should use disable/enable_irq for all controller which don't have proper IRQ masking mechanism (NIEN doesn't count) for both reliability and so that we move PIO out of irq handler. Jeff, this has come up quite a few times now, what do you think?
Thanks.
Signature · 1 line
tejun
Quoted 24 lines · Tejun Heo
Quoted 7 lines · Alan Cox
Quoted 5 lines
circumstances, during resume SRST, the drive would raise the IRQ line regardless of NIEN and will stay that way for several seconds, thus triggering nobody cared. Other than detecting and clearing the spurious IRQ, there just isn't much driver can do to work around problems like this.
There is. It also means your patch isn't sufficient - if that IRQ had been level triggered you'd have hung the box solid.
Right, the attached patch fixed it so it must have been the controller latching the IRQ.
Quoted 12 lines
The old IDE code makes use of disable_irq/enable_irq (and really ought to make use of on chip private IRQ mask bits as first choice but doesn't).
Sounds to me like there are two things we can do to help
Make the nIEN masking via a helper that can on suitable chips also mask on chip.
Make use of
disable_irq_nosync()in the case of devices that ignore nIEN in the IRQ handler, setting a flag then re-enable it when the driver path completes the reset and the bit goes clear.
Quoted 7 lines
Quoted 2 lines
We need to start using enable/disable_irq and/or local chip IRQ masking for PIO in some places anyway.
Agreed. I think we should use disable/enable_irq for all controller which don't have proper IRQ masking mechanism (NIEN doesn't count) for both reliability and so that we move PIO out of irq handler. Jeff, this has come up quite a few times now, what do you think?
OSDL bz#10884 seems to suffer similar problem. The patch fixes it.
http://bugzilla.kernel.org/show_bug.cgi?id=10884
We definitely need to do something about it.
Signature · 1 line
tejun
Quoted 1 line · Tejun Heo
Alan Cox wrote:
..
Quoted 7 lines
Quoted 2 lines
We need to start using enable/disable_irq and/or local chip IRQ masking for PIO in some places anyway.
Agreed. I think we should use disable/enable_irq for all controller which don't have proper IRQ masking mechanism (NIEN doesn't count) for both reliability and so that we move PIO out of irq handler. Jeff, this has come up quite a few times now, what do you think?
..
Keep in mind that disable_irq() is nasty for other devices sharing the same PCI interrupt as the IDE interface..
Quoted 2 lines
Keep in mind that
disable_irq()is nasty for other devices sharing the same PCI interrupt as the IDE interface..
Well currently we disable all the interrupts to that CPU which is fairly unfriendly too so it's not perfect but its certainly better and it is what works with old IDE.
If you care about performance you have an AHCI controller ;)
Alan
Hello.
Quoted 4 lines · Tejun Heo
The
DMA_IRQbit in the bmdma status register is always set when IDEIRQ is asserted allowing spurious IRQ detection. Detect spurious IRQs and clear them. This protectsata_piixagainst nobody-cared which gets reported not so rarely.
This should be more generic, as several IDE controllers have the separate
IDE interrupt status bits.
Quoted 12 lines
Signed-off-by Tejun Heo <[email protected]>drivers/ata/ata_piix.c +2
-0@@ -922,6 +922,58 @@ static int piix_sidpr_scr_read(struct ata_link *link,return 0 ; } +static irqreturn_t piix_interrupt (int irq,void *dev_instance) +{
[...]
drivers/ata/ata_piix.c static int piix_sidpr_scr_read(struct ata_link *link, · line 952
Quoted 5 lines
drivers/ata/ata_piix.c +5
-0+/* +* Control reaches here if HSM is not expecting IRQ. +* If the controller is actually asserting IRQ line, +* this will lead to nobody cared. Fortuantely, +* DMA_INTR of PIIX is set whenever IDEIRQ is set so
Frankly speaking, I'm not sure about the PIIX chips themselves... Is that
documented by Intel?
MBR, Sergei
Quoted 11 lines · Sergei Shtylyov
Hello.
Quoted 4 lines · Tejun Heo
The
DMA_IRQbit in the bmdma status register is always set when IDEIRQ is asserted allowing spurious IRQ detection. Detect spurious IRQs and clear them. This protectsata_piixagainst nobody-cared which gets reported not so rarely.This should be more generic, as several IDE controllers have the
separate IDE interrupt status bits.
Well, this was workaround for ata_piix. I'll prep a more generic one with callback.
Quoted 14 lines
Quoted 13 lines
Signed-off-by Tejun Heo <[email protected]>drivers/ata/ata_piix.c +2
-0@@ -922,6 +922,58 @@ static int piix_sidpr_scr_read(struct ata_link *link, return 0; } +static irqreturn_t piix_interrupt (int irq,void *dev_instance) +{[...]
Quoted 8 lines
drivers/ata/ata_piix.c static int piix_sidpr_scr_read(struct ata_link *link, · line 952
Quoted 5 lines
drivers/ata/ata_piix.c +5
-0+/* +* Control reaches here if HSM is not expecting IRQ. +* If the controller is actually asserting IRQ line, +* this will lead to nobody cared. Fortuantely, +* DMA_INTR of PIIX is set whenever IDEIRQ is set so Frankly speaking, I'm not sure about the PIIX chips themselves... Is
that documented by Intel?
Yeap, it is.
Signature · 1 line
tejun
Quoted 9 lines · Tejun Heo
The
DMA_IRQbit in the bmdma status register is always set when IDEIRQ is asserted allowing spurious IRQ detection. Detect spurious IRQs and clear them. This protectsata_piixagainst nobody-cared which gets reported not so rarely.Signed-off-by Tejun Heo <[email protected]>--- drivers/ata/ata_piix.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-)
It's tough to call this #upstream-fixes material, unfortunately... For this late into -rc, I am very nervous about changing the ATA interrupt path for millions of machines.
Jeff
Quoted 6 lines · On Tue, 25 Nov 2008 12:08:12 -0500 Jeff Garzik <[email protected]> wrote:
Quoted 5 lines · Tejun Heo
The
DMA_IRQbit in the bmdma status register is always set when IDEIRQ is asserted allowing spurious IRQ detection. Detect spurious IRQs and clear them. This protectsata_piixagainst nobody-cared which gets reported not so rarely.
Quoted 8 lines
Quoted 4 lines
Signed-off-by Tejun Heo <[email protected]>--- drivers/ata/ata_piix.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-)It's tough to call this #upstream-fixes material, unfortunately... For this late into -rc, I am very nervous about changing the ATA interrupt path for millions of machines.
I would like to go through the PIIX errata first. There are some rules about what registers may not be touched during a transfer on some of the devices and breaking them is *bad*. I don't think this is a problem but it makes me nervous.
Now the DF draining on the other hand ;)
Alan
Quoted 13 lines · Alan Cox
Quoted 10 lines · On Tue, 25 Nov 2008 12:08:12 -0500 Jeff Garzik <[email protected]> wrote:
Quoted 9 lines · Tejun Heo
The
DMA_IRQbit in the bmdma status register is always set when IDEIRQ is asserted allowing spurious IRQ detection. Detect spurious IRQs and clear them. This protectsata_piixagainst nobody-cared which gets reported not so rarely.Signed-off-by Tejun Heo <[email protected]>--- drivers/ata/ata_piix.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-)
Quoted 8 lines
Quoted 3 lines
It's tough to call this #upstream-fixes material, unfortunately... For this late into -rc, I am very nervous about changing the ATA interrupt path for millions of machines.
I would like to go through the PIIX errata first. There are some rules about what registers may not be touched during a transfer on some of the devices and breaking them is *bad*. I don't think this is a problem but it makes me nervous.
Well, the BMDMA status register read and cleared iff no qc is in flight so the patch isn't likely to break any of that. That said, it's true that this is a big change this late in the release cycle. I have no problem with postponing it to the next -rc1. Also, this patch is already in openSUSE11.1 and SLES11 betas, so we'll know a lot more about how this works in the coming weeks. :-)
Thanks.
Signature · 1 line
tejun
Quoted 3 lines
Well, the BMDMA status register read and cleared iff no qc is in flight so the patch isn't likely to break any of that. That said, it's true that this is a big change this late in the release cycle.
Well the kind of thing that bothers me is disk corruption. On the PIIX4 for example a simple read of some of the control registers during a PIO block transfer corrupts the data silently.... (See errata #15).
Alan