Re: [git patches] libata fixes for .26

[Linus Torvalds <torvalds@linux-foundation.org>]

On Fri, 4 Jul 2008, Jeff Garzik wrote:
> 
> The libata-sff change is longer than I'd like for 2.6.26-rc, but it's
> all printk changes/additions.  No behavior changes, just improved
> diagnostics upon error, something we really need in that area.

Hmm.. 

Looking at the AHCI change, I think it's still potentially buggy.

I think it is potentially buggy for two separate reasons:

 - if the interrupt happens because of some port that we don't handle, we 
   should still ACK it, in order to get rid of it. I don't think Tejun's 
   patch fixed anything at all, since it still did a binary 'and' with 
   hpriv->port_map on the bits, so it would never ACK anything that didn't 
   have a bit set, and the (bogus) interrupt would keep screaming.

 - I also wonder if / suspect that the IRQ ACK should happen _before_ we 
   handle the source of the interrupt, because otherwise if one port ends 
   up having two events in close succession (can this happen? I think so), 
   then we end up perhaps getting the irq for the first one, and handle 
   that first event, but then the second event happens immediately 
   afterwards, and before we do the writel() to ACK it, so now the 
   _hardware_ thinks we have handled both of them, even though we never 
   actually reacted to the second event.

So I think the appended (TOTALLY UNTESTED!) patch - based on top of the 
pull that I already did - might be a good idea.

NOTE! I _really_ didn't test it. I do not know how AHCI works at a low 
level, and maybe there is some reason why the IRQ ACK writel() actually 
has to happen after you've handled the event (to avoid getting a new 
interrupt immediately. But based on other controllers I've worked with, 
this is the correct way to not lose irq events.

[ And yes, the race for the irq ack issue is small. And yes, the 
  likelihood of a bogus interrupt triggering is probably small too. And 
  see above about my lack of specific knowledge about AHCI.

  So I'm sure as heck not going to commit this patch, I'm just sending it 
  out as a "Are you sure you shouldn't do it like this?" RFC patch.. ]

Hmm?

			Linus
---
 drivers/ata/ahci.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 061817a..5cfee74 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1786,12 +1786,17 @@ static irqreturn_t ahci_interrupt(int irq, void *dev_instance)
 
 	/* sigh.  0xffffffff is a valid return from h/w */
 	irq_stat = readl(mmio + HOST_IRQ_STAT);
-	irq_stat &= hpriv->port_map;
 	if (!irq_stat)
 		return IRQ_NONE;
 
 	spin_lock(&host->lock);
 
+	/* Ack _all_ sources of interrupts.. */
+	writel(irq_stat, mmio + HOST_IRQ_STAT);
+
+	/* ..but only care (and report as handled) about the ones we can handle */
+	irq_stat &= hpriv->port_map;
+
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap;
 
@@ -1811,9 +1816,6 @@ static irqreturn_t ahci_interrupt(int irq, void *dev_instance)
 
 		handled = 1;
 	}
-
-	writel(irq_stat, mmio + HOST_IRQ_STAT);
-
 	spin_unlock(&host->lock);
 
 	VPRINTK("EXIT\n");