Tejun Heo wrote:
> Hello, Albert.
>
> Albert Lee wrote:
>
>>Tejun Heo wrote:
>>
>>>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.
>
>
> Oh I see.
>
>
>>>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).
>
>
> Good to know it works. With or without nIEN, I think this change is a
> good thing to have. IRQ handler shouldn't interfere with polling as
> both acquire lock during operation.
This reminds me of a possible issue with the patch:
Previously the polling code assumes that the interrupt handler won't interfere
with it and the polling code runs without holding ap->lock.
However, with the above patch, the interrupt handler might read the
Status register when the polling code is transfering data, etc. from the port.
Would this cause trouble to the ATA/ATAPI devices?
Should we have something like ATA_PFLAG_HSM_BUSY below, such that the interrupt
handler won't read the Status register when HSM is busy accessing the port?
--
albert
(Revised patch: Don't read the Status register when HSM is busy accessing the port)
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 19:17:58.000000000 +0800
@@ -4389,6 +4389,14 @@ int ata_hsm_move(struct ata_port *ap, st
*/
WARN_ON(in_wq != ata_hsm_ok_in_wq(ap, qc));
+ if (in_wq) {
+ spin_lock_irqsave(ap->lock, flags);
+ ap->pflags |= ATA_PFLAG_HSM_BUSY;
+ spin_unlock_irqrestore(ap->lock, flags);
+ } else {
+ ap->pflags |= ATA_PFLAG_HSM_BUSY;
+ }
+
fsm_start:
DPRINTK("ata%u: protocol %d task_state %d (dev_stat 0x%X)\n",
ap->print_id, qc->tf.protocol, ap->hsm_task_state, status);
@@ -4600,6 +4608,14 @@ fsm_start:
BUG();
}
+ if (in_wq) {
+ spin_lock_irqsave(ap->lock, flags);
+ ap->pflags &= ~ATA_PFLAG_HSM_BUSY;
+ spin_unlock_irqrestore(ap->lock, flags);
+ } else {
+ ap->pflags &= ~ATA_PFLAG_HSM_BUSY;
+ }
+
return poll_next;
}
@@ -5224,9 +5240,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)) {
+ handled |= ata_host_intr(ap, qc);
+ } else if (!(ap->pflags & ATA_PFLAG_HSM_BUSY)) {
+ ata_chk_status(ap);
+ handled = 1;
+ }
+ }
}
}
diff -Nrup linux-2.6.21.1-ori/include/linux/libata.h linux-2.6.21.1-mod3/include/linux/libata.h
--- linux-2.6.21.1-ori/include/linux/libata.h 2007-04-28 05:49:26.000000000 +0800
+++ linux-2.6.21.1-mod3/include/linux/libata.h 2007-05-07 18:41:01.000000000 +0800
@@ -195,6 +195,7 @@ enum {
ATA_PFLAG_FLUSH_PORT_TASK = (1 << 16), /* flush port task */
ATA_PFLAG_SUSPENDED = (1 << 17), /* port is suspended (power) */
ATA_PFLAG_PM_PENDING = (1 << 18), /* PM operation pending */
+ ATA_PFLAG_HSM_BUSY = (1 << 19), /* HSM accessing the port */
/* struct ata_queued_cmd flags */
ATA_QCFLAG_ACTIVE = (1 << 0), /* cmd not yet ack'd to scsi lyer */
-
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
