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

[Tejun Heo <tj@kernel.org>]

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