[PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Tejun Heo]

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.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ata/ata_piix.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index 8e37be1..b438edc 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -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)

Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Alan Cox]

On Fri, 21 Nov 2008 13:13:06 +0900
Tejun Heo <tj@kernel.org> wrote:

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

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 ?

Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Tejun Heo]

Alan Cox wrote:
> On Fri, 21 Nov 2008 13:13:06 +0900
> Tejun Heo <tj@kernel.org> wrote:
> 
>> 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.
> 
> 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 ?

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.

-- 
tejun

Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Sergei Shtylyov]

Hello.

Tejun Heo wrote:

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

    This should be more generic, as several IDE controllers have the separate 
IDE interrupt status bits.

> Signed-off-by: Tejun Heo <tj@kernel.org>

> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
> index 8e37be1..b438edc 100644
> --- a/drivers/ata/ata_piix.c
> +++ b/drivers/ata/ata_piix.c
> @@ -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)
> +{
[...]
> +		/*
> +		 * 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

Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Tejun Heo]

Sergei Shtylyov wrote:
> Hello.
> 
> Tejun Heo wrote:
> 
>> 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.
> 
>    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.

>> Signed-off-by: Tejun Heo <tj@kernel.org>
> 
>> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
>> index 8e37be1..b438edc 100644
>> --- a/drivers/ata/ata_piix.c
>> +++ b/drivers/ata/ata_piix.c
>> @@ -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)
>> +{
> [...]
>> +        /*
>> +         * 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.

-- 
tejun

Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Jeff Garzik]

Alan Cox wrote:
> On Fri, 21 Nov 2008 13:13:06 +0900
> Tejun Heo <tj@kernel.org> wrote:
> 
>> 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.
> 
> 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 ?

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

Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Jeff Garzik]

Tejun Heo wrote:
> 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.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  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

Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Alan Cox]

On Tue, 25 Nov 2008 12:08:12 -0500
Jeff Garzik <jeff@garzik.org> wrote:

> Tejun Heo wrote:
> > 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.
> > 
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > ---
> >  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

Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Tejun Heo]

Alan Cox wrote:
> On Tue, 25 Nov 2008 12:08:12 -0500
> Jeff Garzik <jeff@garzik.org> wrote:
> 
>> Tejun Heo wrote:
>>> 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.
>>>
>>> Signed-off-by: Tejun Heo <tj@kernel.org>
>>> ---
>>>  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.

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.

-- 
tejun

Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Tejun Heo]

Hello, Jeff.

Jeff Garzik wrote:
> Alan Cox wrote:
>> On Fri, 21 Nov 2008 13:13:06 +0900
>> Tejun Heo <tj@kernel.org> wrote:
>>
>>> 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.
>>
>> 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 ?
> 
> 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.

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?

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

-- 
tejun

Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Alan Cox]

> 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

Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Alan Cox]

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

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

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

Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Sergei Shtylyov]

Hello.

Alan Cox wrote:

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

> Promise uses a magic register at dmabase + 0x1D which holds irq bits
>   

   Yes, if you mean the chips driven by [pata_]pdc202xx_old -- don't 
know about SATA.

MBR, Sergei

Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Sergei Shtylyov]

Hello.

Alan Cox wrote:

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

Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Sergei Shtylyov]

Hello, I wrote:

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

Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Jeff Garzik]

Alan Cox wrote:
>> 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?
> 
> 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
> 
> What I'd like to understand better is why we need any
> of these ...

Indeed!

	Jeff

Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Tejun Heo]

Jeff Garzik wrote:
> Alan Cox wrote:
>>> 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?
>>
>> 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
>>
>> 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.

-- 
tejun

Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Alan Cox]

> 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

Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Tejun Heo]

Alan Cox wrote:
>> 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.

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

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.

-- 
tejun

Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Tejun Heo]

Tejun Heo wrote:
> Alan Cox wrote:
>>> 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.
> 
>> 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.
> 
> 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.

-- 
tejun

Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Mark Lord]

Tejun Heo wrote:
> Alan Cox wrote:
..
>> 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..

Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs

[Alan Cox]

> 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