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 <[EMAIL PROTECTED]>
>>---
>>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);
+ }
+ }
}
}
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
