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@xxxxxxxxxx> >>--- >>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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
