[PATCH] fix atapi_packet_task vs. intr race (take 2)

Interrupts from devices sharing the same IRQ could cause
ata_host_intr to finish commands being processed by atapi_packet_task
if the commands are using ATA_PROT_ATAPI_NODATA or ATA_PROT_ATAPI_DMA
protocol.  This is because libata interrupt handler is unaware that
interrupts are not expected during that period.  This patch adds
ATA_FLAG_NOINTR flag to tell the interrupt handler that we're not
expecting interrupts.

 Note that once proper HSM is implemented for interrupt-driven PIO,
this should be merged into it and this flag will be removed.

 ahci.c is a different kind of beast, so it's left alone.

* The following drivers use ata_qc_issue_prot and ata_interrupt, so
  changes in libata core will do.

  ata_piix sata_sil sata_svw sata_via sata_sis sata_uli

* The following drivers use ata_qc_issue_prot and custom intr handler.
  They need this change to work correctly.

  sata_nv sata_vsc

* The following drivers use custom issue function and intr handler.
  Currently all custom issue functions don't support ATAPI, so this
  change is irrelevant, updated for consistency and to avoid later
  mistakes.

  sata_promise sata_qstor sata_sx4

Signed-off-by: Tejun Heo <htejun@gmail.com>
Signed-off-by: Jeff Garzik <jgarzik@pobox.com>

diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 9a6aacf467b86807c417dfe210e2b69482d7a0bb..c92439fe5daed2354a97abeef6f50a160ee18678 100644 (file)
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -3350,11 +3350,13 @@ int ata_qc_issue_prot(struct ata_queued_cmd *qc)
                break;
 
        case ATA_PROT_ATAPI_NODATA:
+               ap->flags |= ATA_FLAG_NOINTR;
                ata_tf_to_host_nolock(ap, &qc->tf);
                queue_work(ata_wq, &ap->packet_task);
                break;
 
        case ATA_PROT_ATAPI_DMA:
+               ap->flags |= ATA_FLAG_NOINTR;
                ap->ops->tf_load(ap, &qc->tf);   /* load tf registers */
                ap->ops->bmdma_setup(qc);           /* set up bmdma */
                queue_work(ata_wq, &ap->packet_task);
@@ -3708,7 +3710,8 @@ irqreturn_t ata_interrupt (int irq, void *dev_instance, struct pt_regs *regs)
                struct ata_port *ap;
 
                ap = host_set->ports[i];
-               if (ap && (!(ap->flags & ATA_FLAG_PORT_DISABLED))) {
+               if (ap &&
+                   !(ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR))) {
                        struct ata_queued_cmd *qc;
 
                        qc = ata_qc_from_tag(ap, ap->active_tag);
@@ -3760,19 +3763,27 @@ static void atapi_packet_task(void *_data)
        /* send SCSI cdb */
        DPRINTK("send cdb\n");
        assert(ap->cdb_len >= 12);
-       ata_data_xfer(ap, qc->cdb, ap->cdb_len, 1);
 
-       /* if we are DMA'ing, irq handler takes over from here */
-       if (qc->tf.protocol == ATA_PROT_ATAPI_DMA)
-               ap->ops->bmdma_start(qc);           /* initiate bmdma */
+       if (qc->tf.protocol == ATA_PROT_ATAPI_DMA ||
+           qc->tf.protocol == ATA_PROT_ATAPI_NODATA) {
+               unsigned long flags;
 
-       /* non-data commands are also handled via irq */
-       else if (qc->tf.protocol == ATA_PROT_ATAPI_NODATA) {
-               /* do nothing */
-       }
+               /* Once we're done issuing command and kicking bmdma,
+                * irq handler takes over.  To not lose irq, we need
+                * to clear NOINTR flag before sending cdb, but
+                * interrupt handler shouldn't be invoked before we're
+                * finished.  Hence, the following locking.
+                */
+               spin_lock_irqsave(&ap->host_set->lock, flags);
+               ap->flags &= ~ATA_FLAG_NOINTR;
+               ata_data_xfer(ap, qc->cdb, ap->cdb_len, 1);
+               if (qc->tf.protocol == ATA_PROT_ATAPI_DMA)
+                       ap->ops->bmdma_start(qc);       /* initiate bmdma */
+               spin_unlock_irqrestore(&ap->host_set->lock, flags);
+       } else {
+               ata_data_xfer(ap, qc->cdb, ap->cdb_len, 1);
 
-       /* PIO commands are handled by polling */
-       else {
+               /* PIO commands are handled by polling */
                ap->pio_task_state = PIO_ST;
                queue_work(ata_wq, &ap->pio_task);
        }

diff --git a/drivers/scsi/sata_nv.c b/drivers/scsi/sata_nv.c
index 9b9142790bd616c22c2af8aca3304b773c5f73e4..41a3421b02b4c1958754dede8ed9955c5c67b143 100644 (file)
--- a/drivers/scsi/sata_nv.c
+++ b/drivers/scsi/sata_nv.c
@@ -291,7 +291,8 @@ static irqreturn_t nv_interrupt (int irq, void *dev_instance,
                struct ata_port *ap;
 
                ap = host_set->ports[i];
-               if (ap && (!(ap->flags & ATA_FLAG_PORT_DISABLED))) {
+               if (ap &&
+                   !(ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR))) {
                        struct ata_queued_cmd *qc;
 
                        qc = ata_qc_from_tag(ap, ap->active_tag);

diff --git a/drivers/scsi/sata_promise.c b/drivers/scsi/sata_promise.c
index cc613b3c6ce6b5cc75041ad16136278c8e347a04..6defd79623592858ec4031c7c612c5c18e03ad3f 100644 (file)
--- a/drivers/scsi/sata_promise.c
+++ b/drivers/scsi/sata_promise.c
@@ -445,7 +445,8 @@ static irqreturn_t pdc_interrupt (int irq, void *dev_instance, struct pt_regs *r
                VPRINTK("port %u\n", i);
                ap = host_set->ports[i];
                tmp = mask & (1 << (i + 1));
-               if (tmp && ap && (!(ap->flags & ATA_FLAG_PORT_DISABLED))) {
+               if (tmp && ap &&
+                   !(ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR))) {
                        struct ata_queued_cmd *qc;
 
                        qc = ata_qc_from_tag(ap, ap->active_tag);

diff --git a/drivers/scsi/sata_qstor.c b/drivers/scsi/sata_qstor.c
index dca9ed7ac760d02a9a5645341bca1adc523881a8..08a84042ce09e6afb3fb4fa749e49445ca0afbd9 100644 (file)
--- a/drivers/scsi/sata_qstor.c
+++ b/drivers/scsi/sata_qstor.c
@@ -386,7 +386,8 @@ static inline unsigned int qs_intr_pkt(struct ata_host_set *host_set)
                        DPRINTK("SFF=%08x%08x: sCHAN=%u sHST=%d sDST=%02x\n",
                                        sff1, sff0, port_no, sHST, sDST);
                        handled = 1;
-                       if (ap && (!(ap->flags & ATA_FLAG_PORT_DISABLED))) {
+                       if (ap && !(ap->flags &
+                                   (ATA_FLAG_PORT_DISABLED|ATA_FLAG_NOINTR))) {
                                struct ata_queued_cmd *qc;
                                struct qs_port_priv *pp = ap->private_data;
                                if (!pp || pp->state != qs_state_pkt)
@@ -417,7 +418,8 @@ static inline unsigned int qs_intr_mmio(struct ata_host_set *host_set)
        for (port_no = 0; port_no < host_set->n_ports; ++port_no) {
                struct ata_port *ap;
                ap = host_set->ports[port_no];
-               if (ap && (!(ap->flags & ATA_FLAG_PORT_DISABLED))) {
+               if (ap &&
+                   !(ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR))) {
                        struct ata_queued_cmd *qc;
                        struct qs_port_priv *pp = ap->private_data;
                        if (!pp || pp->state != qs_state_mmio)

diff --git a/drivers/scsi/sata_sx4.c b/drivers/scsi/sata_sx4.c
index 76644ea62d6772c469dd90817784d0d5cca18d55..e2db499f22dd26c3c0b9cec584524c493fd8d6eb 100644 (file)
--- a/drivers/scsi/sata_sx4.c
+++ b/drivers/scsi/sata_sx4.c
@@ -825,7 +825,8 @@ static irqreturn_t pdc20621_interrupt (int irq, void *dev_instance, struct pt_re
                        ap = host_set->ports[port_no];
                tmp = mask & (1 << i);
                VPRINTK("seq %u, port_no %u, ap %p, tmp %x\n", i, port_no, ap, tmp);
-               if (tmp && ap && (!(ap->flags & ATA_FLAG_PORT_DISABLED))) {
+               if (tmp && ap &&
+                   !(ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR))) {
                        struct ata_queued_cmd *qc;
 
                        qc = ata_qc_from_tag(ap, ap->active_tag);

diff --git a/drivers/scsi/sata_vsc.c b/drivers/scsi/sata_vsc.c
index cb3a6d89cf0024d8d7d504e00e0af93cd63879c4..6f2562171be05347342b9ce64c351e9ed1b8ed17 100644 (file)
--- a/drivers/scsi/sata_vsc.c
+++ b/drivers/scsi/sata_vsc.c
@@ -173,7 +173,8 @@ static irqreturn_t vsc_sata_interrupt (int irq, void *dev_instance,
                        struct ata_port *ap;
 
                        ap = host_set->ports[i];
-                       if (ap && (!(ap->flags & ATA_FLAG_PORT_DISABLED))) {
+                       if (ap && !(ap->flags &
+                                   (ATA_FLAG_PORT_DISABLED|ATA_FLAG_NOINTR))) {
                                struct ata_queued_cmd *qc;
 
                                qc = ata_qc_from_tag(ap, ap->active_tag);

diff --git a/include/linux/libata.h b/include/linux/libata.h
index 85b0aaee0ef89a750bc825bde330f3bac719051b..724b7d1c18ea2223da214cbf85c264b29be384fb 100644 (file)
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -113,6 +113,8 @@ enum {
        ATA_FLAG_MMIO           = (1 << 6), /* use MMIO, not PIO */
        ATA_FLAG_SATA_RESET     = (1 << 7), /* use COMRESET */
        ATA_FLAG_PIO_DMA        = (1 << 8), /* PIO cmds via DMA */
+       ATA_FLAG_NOINTR         = (1 << 9), /* FIXME: Remove this once
+                                            * proper HSM is in place. */
 
        ATA_QCFLAG_ACTIVE       = (1 << 1), /* cmd not yet ack'd to scsi lyer */
        ATA_QCFLAG_SG           = (1 << 3), /* have s/g table? */