From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tejun Heo Subject: Re: [PATCH #upstraem-fixes] ata_piix: detect and clear spurious IRQs Date: Wed, 26 Nov 2008 11:52:53 +0900 Message-ID: <492CBA05.8000107@kernel.org> References: <49263552.8090602@kernel.org> <20081121102504.63007bf6@lxorguk.ukuu.org.uk> <492C30EE.9080600@garzik.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: Received: from hera.kernel.org ([140.211.167.34]:59872 "EHLO hera.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752284AbYKZCxJ (ORCPT ); Tue, 25 Nov 2008 21:53:09 -0500 In-Reply-To: <492C30EE.9080600@garzik.org> Sender: linux-ide-owner@vger.kernel.org List-Id: linux-ide@vger.kernel.org To: Jeff Garzik Cc: Alan Cox , IDE/ATA development list Hello, Jeff. Jeff Garzik wrote: > Alan Cox wrote: >> On Fri, 21 Nov 2008 13:13:06 +0900 >> 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. >> >> 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