Re: [PATCH] libata: disable_irq() during polling IDENTIFY

[Albert Lee <albertcc@tw.ibm.com>]

Tejun Heo wrote:
> [cc'ing Bartlomiej and Mark, hi]
> 
> Hello, Albert.
> 
> Albert Lee wrote:
> 
>>Problem:
>>  Kernel got "irq 5: nobody cared" when using
>>  libata + polling IDENTIFY + Promise 20275 adapter + Benq DW1620 drive.
>>
>>  Detail message available in bug 8441 (http://bugzilla.kernel.org/show_bug.cgi?id=8441).
>>
>>Cause:
>> The Benq DW1620 drive raises INTRQ during polling IDENTIFY PACKET DEVICE,
>> even if nIEN = 1.
> 
> 
> Aieeee...
> 
> 
>>Proposed fix:
>>  disable_irq() during polling IDENTIFY to work around, the same as what IDE subsystem does.
>>
>>Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
>>---
>>Some controller like Intel ICH4 is immune from the problem, with the same kernel
>>and the same Benq DW1620 drive. So, adding the ATA_FLAG_IDENT_IRQ_OFF flag for
>>those adapters that needs the workaround. Patch against 2.6.21.1 for your review, thanks.
> 
> 
> I guess piix is masking the interrupt at the host side.
> 
> Another interesting aspect is that the SATA spec says the device is
> recommended to ignore nIEN while the controller is recommended to not
> set nIEN when it sends FIS to the device.  ie. nIEN should be
> implemented on the host controller.  I bet there are controllers out
> there which doesn't do host-side masking and there will be more and more
> devices which ignore nIEN, so we're likely to see similar problems on
> SATA too.
> 
> 
>>+	/* Disable IRQ since some devices like Benq DW1620 raises INTRQ
>>+	 * when IDENTIFY PACKET DEVICE, even with polling IDENTIFY.
>>+	 */
>>+	if (ap->flags & ATA_FLAG_IDENT_IRQ_OFF) {
>>+		if (host->irq)
>>+			disable_irq(host->irq);
>>+
>>+		if (host->irq2)
>>+			disable_irq(host->irq2);
>>+	}
>>+
>> 	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_FROM_DEVICE,
>> 				     id, sizeof(id[0]) * ATA_ID_WORDS);
>>+
>>+	/* Re-enable IRQ */
>>+	if (ap->flags & ATA_FLAG_IDENT_IRQ_OFF) {
>>+		if (host->irq)
>>+			enable_irq(host->irq);
>>+
>>+		if (host->irq2)
>>+			enable_irq(host->irq2);
>>+	}
>>+
> 
> 
> Yeap, this is how IDE deals with polling commands but I'm not sure how
> it's supposed to work with PCI IRQ sharing.  Bartlomiej, can you
> enlighten me here?
> 
> Also, this is a problem for not only IDENTIFY but all polling commands.

Yes, other command might also assert INTRQ during polling.
However, for the specific BENQ DW1620 drive, only IDENTIFY_PACKET_DEVICE
has such behavior; other commands like READ or REQUEST_SENSE are ok.

> 
> One solution I can think of is to let IRQ handler ack IRQ
> unconditionally during polling commands - ie. just read the TF Status
> register once and tell the IRQ subsystem that the IRQ is handled.  This
> shouldn't affect the operation of polling as the only side effect of
> reading Status is clearing pending IRQ && will give us a nice way to
> deal with the SATA bridge chip which chokes on nIEN.  Considering the
> sorry state of nIEN in SATA, I guess this might be the best way to deal
> with this.
> 
> Albert, can you please test whether this works?  Modifying
> ata_interrupt() such that it reads TF Status if ATA_TFLAG_POLLING should
> do the trick.
> 

Yes, reading the Status register and acking interrupt also fixes the
problem (patch attached below). 

--
albert

diff -Nrup linux-2.6.21.1-ori/drivers/ata/libata-core.c linux-2.6.21.1-mod3/drivers/ata/libata-core.c
--- linux-2.6.21.1-ori/drivers/ata/libata-core.c	2007-05-04 11:22:23.000000000 +0800
+++ linux-2.6.21.1-mod3/drivers/ata/libata-core.c	2007-05-07 17:44:21.000000000 +0800
@@ -5224,9 +5224,14 @@ irqreturn_t ata_interrupt (int irq, void
 			struct ata_queued_cmd *qc;
 
 			qc = ata_qc_from_tag(ap, ap->active_tag);
-			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)) &&
-			    (qc->flags & ATA_QCFLAG_ACTIVE))
-				handled |= ata_host_intr(ap, qc);
+			if (qc && (qc->flags & ATA_QCFLAG_ACTIVE)) {
+				if (qc->tf.flags & ATA_TFLAG_POLLING) {
+					ata_chk_status(ap);
+					handled = 1;
+				} else {
+					handled |= ata_host_intr(ap, qc);
+				}
+			}
 		}
 	}