[PATCH/RFC 0/8] libata: delegate irq driven pio to workqueue (take 2)

[PATCH/RFC 0/8] libata: delegate irq driven pio to workqueue (take 2)

[Albert Lee]

1/8: fix the ata_altstatus() in ata_hsm_qc_complete()
2/8: move ata_altstatus() from ata_hsm_move() to pio data xfer functions
3/8: change the state after "PIO data-in" to HSM_ST_IDLE instead of HSM_ST_LAST
4/8: move and reduce locking to the pio data xfer functions
5/8: ack possibly unsolicited irq when polling
6/8: delegate irq driven pio to workqueue
7/8: ata_port_flush_task() fix for irq pio delegation
8/8: ack more unsolicited irq

---
Revised and reorder the patches per previous comments.
Patch against the libata-dev tree for your review.
(af3b146d26550f0c8e0d77b2117c6f8aec5d8146)

[PATCH 1/8] libata: fix the ata_altstatus() in ata_hsm_qc_complete()

[Albert Lee]

patch 1/8:
In ata_hsm_qc_complete():
Calling ata_altstatus() after the qc completed looks unsafe.
Move it to be before completing the qc.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
Don't know what the ata_altstatus() is doing here?
Anyway, move it to be before ata_qc_complete().

diff -Nrup 00_libata-dev/drivers/ata/libata-core.c 01_flush_fix/drivers/ata/libata-core.c
--- 00_libata-dev/drivers/ata/libata-core.c	2007-05-14 12:18:45.000000000 +0800
+++ 01_flush_fix/drivers/ata/libata-core.c	2007-05-15 10:05:33.000000000 +0800
@@ -4740,6 +4740,8 @@ static void ata_hsm_qc_complete(struct a
 	struct ata_port *ap = qc->ap;
 	unsigned long flags;
 
+	ata_altstatus(ap); /* flush */
+
 	if (ap->ops->error_handler) {
 		if (in_wq) {
 			spin_lock_irqsave(ap->lock, flags);
@@ -4772,8 +4774,6 @@ static void ata_hsm_qc_complete(struct a
 		} else
 			ata_qc_complete(qc);
 	}
-
-	ata_altstatus(ap); /* flush */
 }
 
 /**

[PATCH 2/8] libata: move ata_altstatus() out from ata_hsm_move() to the pio data xfer functions

[Albert Lee]

patch 2/8:
 Move ata_altstatus() out from ata_hsm_move() to the pio data xfer functions,
like ata_pio_sectors() and atapi_pio_bytes() that know better if ata_altstatus() is needed.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
atapi_send_cdb() already did ata_altstatus() in itself.
This patch makes ata_pio_sector(), ata_pio_sectors() and atapi_pio_bytes() do the same.

diff -Nrup 01_flush_fix/drivers/ata/libata-core.c 02_smart_flush/drivers/ata/libata-core.c
--- 01_flush_fix/drivers/ata/libata-core.c	2007-05-15 10:05:33.000000000 +0800
+++ 02_smart_flush/drivers/ata/libata-core.c	2007-05-16 10:37:53.000000000 +0800
@@ -4435,6 +4435,7 @@ void ata_data_xfer_noirq(struct ata_devi
 /**
  *	ata_pio_sector - Transfer a sector of data.
  *	@qc: Command on going
+ *	@drq_last: Last sector of pio DRQ transfer
  *
  *	Transfer qc->sect_size bytes of data from/to the ATA device.
  *
@@ -4442,7 +4443,7 @@ void ata_data_xfer_noirq(struct ata_devi
  *	Inherited from caller.
  */
 
-static void ata_pio_sector(struct ata_queued_cmd *qc)
+static void ata_pio_sector(struct ata_queued_cmd *qc, int drq_last)
 {
 	int do_write = (qc->tf.flags & ATA_TFLAG_WRITE);
 	struct scatterlist *sg = qc->__sg;
@@ -4480,6 +4481,9 @@ static void ata_pio_sector(struct ata_qu
 		ap->ops->data_xfer(qc->dev, buf + offset, qc->sect_size, do_write);
 	}
 
+	if (drq_last)
+		ata_altstatus(ap); /* flush */
+
 	qc->curbytes += qc->sect_size;
 	qc->cursg_ofs += qc->sect_size;
 
@@ -4511,9 +4515,9 @@ static void ata_pio_sectors(struct ata_q
 		nsect = min((qc->nbytes - qc->curbytes) / qc->sect_size,
 			    qc->dev->multi_count);
 		while (nsect--)
-			ata_pio_sector(qc);
+			ata_pio_sector(qc, !nsect);
 	} else
-		ata_pio_sector(qc);
+		ata_pio_sector(qc, 1);
 }
 
 /**
@@ -4596,6 +4600,7 @@ next_sg:
 		for (i = 0; i < words; i++)
 			ap->ops->data_xfer(qc->dev, (unsigned char*)pad_buf, 2, do_write);
 
+		ata_altstatus(ap); /* flush */
 		ap->hsm_task_state = HSM_ST_LAST;
 		return;
 	}
@@ -4645,6 +4650,8 @@ next_sg:
 
 	if (bytes)
 		goto next_sg;
+
+	ata_altstatus(ap); /* flush */
 }
 
 /**
@@ -4861,7 +4868,6 @@ fsm_start:
 			 */
 			ap->hsm_task_state = HSM_ST;
 			ata_pio_sectors(qc);
-			ata_altstatus(ap); /* flush */
 		} else
 			/* send CDB */
 			atapi_send_cdb(ap, qc);
@@ -4942,7 +4948,6 @@ fsm_start:
 
 				if (!(qc->tf.flags & ATA_TFLAG_WRITE)) {
 					ata_pio_sectors(qc);
-					ata_altstatus(ap);
 					status = ata_wait_idle(ap);
 				}
 
@@ -4962,13 +4967,11 @@ fsm_start:
 			if (ap->hsm_task_state == HSM_ST_LAST &&
 			    (!(qc->tf.flags & ATA_TFLAG_WRITE))) {
 				/* all data read */
-				ata_altstatus(ap);
 				status = ata_wait_idle(ap);
 				goto fsm_start;
 			}
 		}
 
-		ata_altstatus(ap); /* flush */
 		poll_next = 1;
 		break;
 

[PATCH 3/8] libata: change the state after "PIO data-in" to HSM_ST_IDLE instead of HSM_ST_LAST

[Albert Lee]

patch 3/8:
  Change the state after "PIO data-in" to HSM_ST_IDLE instead of HSM_ST_LAST.

This can prevent the irq handler from thinking "HSM is waiting for the last irq"
and submitting redundant tasks to the workqueue.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
This patch is needed if irq driven pio to be delegated to workqueue.

diff -Nrup 02_smart_flush/drivers/ata/libata-core.c 03_read_state/drivers/ata/libata-core.c
--- 02_smart_flush/drivers/ata/libata-core.c	2007-05-16 10:37:53.000000000 +0800
+++ 03_read_state/drivers/ata/libata-core.c	2007-05-16 10:37:57.000000000 +0800
@@ -4453,7 +4453,7 @@ static void ata_pio_sector(struct ata_qu
 	unsigned char *buf;
 
 	if (qc->curbytes == qc->nbytes - qc->sect_size)
-		ap->hsm_task_state = HSM_ST_LAST;
+		ap->hsm_task_state = do_write ? HSM_ST_LAST : HSM_ST_IDLE;
 
 	page = sg[qc->cursg].page;
 	offset = sg[qc->cursg].offset + qc->cursg_ofs;
@@ -4807,6 +4807,8 @@ int ata_hsm_move(struct ata_port *ap, st
 	 */
 	WARN_ON(in_wq != ata_hsm_ok_in_wq(ap, qc));
 
+	WARN_ON(ap->hsm_task_state == HSM_ST_IDLE);
+
 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);
@@ -4964,8 +4966,7 @@ fsm_start:
 
 			ata_pio_sectors(qc);
 
-			if (ap->hsm_task_state == HSM_ST_LAST &&
-			    (!(qc->tf.flags & ATA_TFLAG_WRITE))) {
+			if (ap->hsm_task_state == HSM_ST_IDLE) {
 				/* all data read */
 				status = ata_wait_idle(ap);
 				goto fsm_start;
@@ -4976,6 +4977,7 @@ fsm_start:
 		break;
 
 	case HSM_ST_LAST:
+	case HSM_ST_IDLE:
 		if (unlikely(!ata_ok(status))) {
 			qc->err_mask |= __ac_err_mask(status);
 			ap->hsm_task_state = HSM_ST_ERR;

[PATCH 4/8] libata: move and reduce locking to the pio data xfer functions

[Albert Lee]

patch 4/8: 
- Added the ATA_PFLAG_HSM_WQ (i.e. HSM running in workqueue) flag to serialize irq and workqueue access to the port.
- Moved the locking out from the ata_hsm_move() to the data xfer functions like ata_pio_sectors().
- The time holding ap->lock is reduced to only part of last pio transfer and clearing of the ATA_PFLAG_HSM_WQ flag.
- The transfer of "head" is made to be multiple of 8-bytes such that  ->data_xfer() could possibly utilize 32-bit pio/mmio.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Tejun Heo <htejun@gmail.com>
---
The variable name is changed to "irq_handover" per Tejun's review.
sata_sil is also modified per Tejun's advice.
Alan's concern about unsolicited irq will be addressed later in patch 5/8 and 8/8.
The chang to __atapi_pio_bytes() is the hardest part. Hopefully this patch gets it right.

diff -Nrup 03_read_state/drivers/ata/libata-core.c 04_move_narrow_lock/drivers/ata/libata-core.c
--- 03_read_state/drivers/ata/libata-core.c	2007-05-16 10:37:57.000000000 +0800
+++ 04_move_narrow_lock/drivers/ata/libata-core.c	2007-05-16 13:53:16.000000000 +0800
@@ -4436,6 +4436,7 @@ void ata_data_xfer_noirq(struct ata_devi
  *	ata_pio_sector - Transfer a sector of data.
  *	@qc: Command on going
  *	@drq_last: Last sector of pio DRQ transfer
+ *	@irq_handover: workqueue is going to handover to the irq handler
  *
  *	Transfer qc->sect_size bytes of data from/to the ATA device.
  *
@@ -4443,7 +4444,7 @@ void ata_data_xfer_noirq(struct ata_devi
  *	Inherited from caller.
  */
 
-static void ata_pio_sector(struct ata_queued_cmd *qc, int drq_last)
+static void ata_pio_sector(struct ata_queued_cmd *qc, int drq_last, int irq_handover)
 {
 	int do_write = (qc->tf.flags & ATA_TFLAG_WRITE);
 	struct scatterlist *sg = qc->__sg;
@@ -4451,6 +4452,7 @@ static void ata_pio_sector(struct ata_qu
 	struct page *page;
 	unsigned int offset;
 	unsigned char *buf;
+	unsigned long irq_flags = 0;
 
 	if (qc->curbytes == qc->nbytes - qc->sect_size)
 		ap->hsm_task_state = do_write ? HSM_ST_LAST : HSM_ST_IDLE;
@@ -4467,6 +4469,16 @@ static void ata_pio_sector(struct ata_qu
 	if (PageHighMem(page)) {
 		unsigned long flags;
 
+		if (irq_handover)
+			/* Data transfer will trigger INTRQ.
+			 * Acquire ap->lock to 
+			 *  - transfer the last sector of data
+			 *  - read and discard altstatus 
+			 *  - clear ATA_PFLAG_HSM_WQ
+			 * atomically.
+			 */
+			spin_lock_irqsave(ap->lock, irq_flags);
+
 		/* FIXME: use a bounce buffer */
 		local_irq_save(flags);
 		buf = kmap_atomic(page, KM_IRQ0);
@@ -4477,8 +4489,34 @@ static void ata_pio_sector(struct ata_qu
 		kunmap_atomic(buf, KM_IRQ0);
 		local_irq_restore(flags);
 	} else {
+		unsigned int head = 0, tail = qc->sect_size;
+
 		buf = page_address(page);
-		ap->ops->data_xfer(qc->dev, buf + offset, qc->sect_size, do_write);
+
+		if (irq_handover) {
+			tail = 8;
+			head = qc->sect_size - tail;
+
+			/* Data transfer of "head" is done without holding
+			 * ap->lock to improve interrupt latency.
+			 * Hopefully no unsolicited INTRQ at this point,
+			 * otherwise we may have nobody cared irq.
+			 * Make "head" to be multiple of 8 bytes such that
+			 * ->data_xfer() could utilize 32-bit pio/mmio.
+			 */
+			ap->ops->data_xfer(qc->dev, buf + offset, head, do_write);
+
+			/* Data transfer of "tail" will trigger INTRQ.
+			 * Acquire ap->lock to 
+			 *  - transfer the last 8 bytes of data
+			 *  - read and discard altstatus 
+			 *  - clear ATA_PFLAG_HSM_WQ
+			 * atomically.
+			 */
+			spin_lock_irqsave(ap->lock, irq_flags);
+		}
+
+		ap->ops->data_xfer(qc->dev, buf + offset + head, tail, do_write);
 	}
 
 	if (drq_last)
@@ -4491,11 +4529,21 @@ static void ata_pio_sector(struct ata_qu
 		qc->cursg++;
 		qc->cursg_ofs = 0;
 	}
+
+	if (irq_handover) {
+		ap->pflags &= ~ATA_PFLAG_HSM_WQ;
+		spin_unlock_irqrestore(ap->lock, irq_flags);
+	}
+
+	/* irq handler or another command might
+	 *  be running at this point
+	 */
 }
 
 /**
  *	ata_pio_sectors - Transfer one or many sectors.
  *	@qc: Command on going
+ *	@irq_handover: workqueue is going to handover to the irq handler
  *
  *	Transfer one or many sectors of data from/to the
  *	ATA device for the DRQ request.
@@ -4504,7 +4552,7 @@ static void ata_pio_sector(struct ata_qu
  *	Inherited from caller.
  */
 
-static void ata_pio_sectors(struct ata_queued_cmd *qc)
+static void ata_pio_sectors(struct ata_queued_cmd *qc, int irq_handover)
 {
 	if (is_multi_taskfile(&qc->tf)) {
 		/* READ/WRITE MULTIPLE */
@@ -4515,15 +4563,20 @@ static void ata_pio_sectors(struct ata_q
 		nsect = min((qc->nbytes - qc->curbytes) / qc->sect_size,
 			    qc->dev->multi_count);
 		while (nsect--)
-			ata_pio_sector(qc, !nsect);
+			ata_pio_sector(qc, !nsect, irq_handover && !nsect);
 	} else
-		ata_pio_sector(qc, 1);
+		ata_pio_sector(qc, 1, irq_handover);
+
+	/* irq handler or another command might
+	 *  be running at this point
+	 */
 }
 
 /**
  *	atapi_send_cdb - Write CDB bytes to hardware
  *	@ap: Port to which ATAPI device is attached.
  *	@qc: Taskfile currently active
+ *	@irq_handover: workqueue is going to handover to the irq handler
  *
  *	When device has indicated its readiness to accept
  *	a CDB, this function is called.  Send the CDB.
@@ -4532,12 +4585,17 @@ static void ata_pio_sectors(struct ata_q
  *	caller.
  */
 
-static void atapi_send_cdb(struct ata_port *ap, struct ata_queued_cmd *qc)
+static void atapi_send_cdb(struct ata_port *ap, struct ata_queued_cmd *qc, int irq_handover)
 {
+	unsigned long irq_flags = 0;
+
 	/* send SCSI cdb */
 	DPRINTK("send cdb\n");
 	WARN_ON(qc->dev->cdb_len < 12);
 
+	if (irq_handover)
+		spin_lock_irqsave(ap->lock, irq_flags);
+
 	ap->ops->data_xfer(qc->dev, qc->cdb, qc->dev->cdb_len, 1);
 	ata_altstatus(ap); /* flush */
 
@@ -4554,12 +4612,22 @@ static void atapi_send_cdb(struct ata_po
 		ap->ops->bmdma_start(qc);
 		break;
 	}
+
+	if (irq_handover) {
+		ap->pflags &= ~ATA_PFLAG_HSM_WQ;
+		spin_unlock_irqrestore(ap->lock, irq_flags);
+	}
+
+	/* irq handler or another command might
+	 *  be running at this point
+	 */
 }
 
 /**
  *	__atapi_pio_bytes - Transfer data from/to the ATAPI device.
  *	@qc: Command on going
  *	@bytes: number of bytes
+ *	@irq_handover: workqueue is going to handover to the irq handler
  *
  *	Transfer Transfer data from/to the ATAPI device.
  *
@@ -4568,7 +4636,7 @@ static void atapi_send_cdb(struct ata_po
  *
  */
 
-static void __atapi_pio_bytes(struct ata_queued_cmd *qc, unsigned int bytes)
+static void __atapi_pio_bytes(struct ata_queued_cmd *qc, unsigned int bytes, int irq_handover)
 {
 	int do_write = (qc->tf.flags & ATA_TFLAG_WRITE);
 	struct scatterlist *sg = qc->__sg;
@@ -4576,9 +4644,14 @@ static void __atapi_pio_bytes(struct ata
 	struct page *page;
 	unsigned char *buf;
 	unsigned int offset, count;
+	unsigned int trailing_bytes = 0;
+	unsigned long irq_flags = 0;
+	int drq_last = 0;
 
-	if (qc->curbytes + bytes >= qc->nbytes)
+	if (qc->curbytes + bytes >= qc->nbytes) {
+		trailing_bytes = qc->curbytes + bytes - qc->nbytes;
 		ap->hsm_task_state = HSM_ST_LAST;
+	}
 
 next_sg:
 	if (unlikely(qc->cursg >= qc->n_elem)) {
@@ -4593,6 +4666,8 @@ next_sg:
 		unsigned int words = bytes >> 1;
 		unsigned int i;
 
+		WARN_ON(bytes != trailing_bytes);
+
 		if (words) /* warning if bytes > 1 */
 			ata_dev_printk(qc->dev, KERN_WARNING,
 				       "%u bytes trailing data\n", bytes);
@@ -4600,8 +4675,18 @@ next_sg:
 		for (i = 0; i < words; i++)
 			ap->ops->data_xfer(qc->dev, (unsigned char*)pad_buf, 2, do_write);
 
+		WARN_ON(!drq_last);
 		ata_altstatus(ap); /* flush */
 		ap->hsm_task_state = HSM_ST_LAST;
+
+		if (irq_handover) {
+			ap->pflags &= ~ATA_PFLAG_HSM_WQ;
+			spin_unlock_irqrestore(ap->lock, irq_flags);
+		}
+
+		/* irq handler or another command might
+		 *  be running at this point
+		 */
 		return;
 	}
 
@@ -4622,9 +4707,28 @@ next_sg:
 
 	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
 
+	/* check if last transfer of real data */
+	if (bytes - count <= trailing_bytes)
+		drq_last = 1;
+
+	/* odd transfer only permitted at last */
+	WARN_ON((count & 1) && !(ap->hsm_task_state == HSM_ST_LAST &&
+				 drq_last));
+
 	if (PageHighMem(page)) {
 		unsigned long flags;
 
+		if (irq_handover && drq_last)
+			/* Data transfer will trigger INTRQ.
+			 * Acquire ap->lock to 
+			 *  - transfer the last bytes of data
+			 *  - transfer the trailing data, if any
+			 *  - read and discard altstatus 
+			 *  - clear ATA_PFLAG_HSM_WQ
+			 * atomically.
+			 */
+			spin_lock_irqsave(ap->lock, irq_flags);
+
 		/* FIXME: use bounce buffer */
 		local_irq_save(flags);
 		buf = kmap_atomic(page, KM_IRQ0);
@@ -4635,8 +4739,39 @@ next_sg:
 		kunmap_atomic(buf, KM_IRQ0);
 		local_irq_restore(flags);
 	} else {
+		unsigned int head = 0, tail = count;
+
 		buf = page_address(page);
-		ap->ops->data_xfer(qc->dev,  buf + offset, count, do_write);
+
+		if (irq_handover && drq_last) {
+			if (count > 20) {
+				tail = 8 + count % 8;
+				head = count - tail;
+
+				/* Data transfer of "head" is done without
+				 * holding ap->lock to improve interrupt
+				 * latency. Hopefully no unsolicited INTRQ
+				 * at this point, otherwise we may have
+				 * nobody cared irq. Make "head" to be 
+				 * multiple of 8 bytes such that
+				 * ->data_xfer() could utilize 32-bit
+				 * pio/mmio.
+				 */
+				ap->ops->data_xfer(qc->dev,  buf + offset, head, do_write);
+			}
+
+			/* Data transfer of "tail" will trigger INTRQ.
+			 * Acquire ap->lock to 
+			 *  - transfer the last 8(~15) bytes of data
+			 *  - transfer the trailing data, if any
+			 *  - read and discard altstatus 
+			 *  - clear ATA_PFLAG_HSM_WQ
+			 * atomically.
+			 */
+			spin_lock_irqsave(ap->lock, irq_flags);
+		}
+
+		ap->ops->data_xfer(qc->dev,  buf + offset + head, tail, do_write);
 	}
 
 	bytes -= count;
@@ -4651,12 +4786,23 @@ next_sg:
 	if (bytes)
 		goto next_sg;
 
+	WARN_ON(!drq_last);
 	ata_altstatus(ap); /* flush */
+
+	if (irq_handover) {
+		ap->pflags &= ~ATA_PFLAG_HSM_WQ;
+		spin_unlock_irqrestore(ap->lock, irq_flags);
+	}
+
+	/* irq handler or another command might
+	 *  be running at this point
+	 */
 }
 
 /**
  *	atapi_pio_bytes - Transfer data from/to the ATAPI device.
  *	@qc: Command on going
+ *	@irq_handover: workqueue is going to handover to the irq handler
  *
  *	Transfer Transfer data from/to the ATAPI device.
  *
@@ -4664,7 +4810,7 @@ next_sg:
  *	Inherited from caller.
  */
 
-static void atapi_pio_bytes(struct ata_queued_cmd *qc)
+static void atapi_pio_bytes(struct ata_queued_cmd *qc, int irq_handover)
 {
 	struct ata_port *ap = qc->ap;
 	struct ata_device *dev = qc->dev;
@@ -4694,8 +4840,11 @@ static void atapi_pio_bytes(struct ata_q
 
 	VPRINTK("ata%u: xfering %d bytes\n", ap->print_id, bytes);
 
-	__atapi_pio_bytes(qc, bytes);
+	__atapi_pio_bytes(qc, bytes, irq_handover);
 
+	/* irq handler or another command might
+	 *  be running at this point
+	 */
 	return;
 
 err_out:
@@ -4796,7 +4945,6 @@ static void ata_hsm_qc_complete(struct a
 int ata_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
 		 u8 status, int in_wq)
 {
-	unsigned long flags = 0;
 	int poll_next;
 
 	WARN_ON((qc->flags & ATA_QCFLAG_ACTIVE) == 0);
@@ -4856,9 +5004,6 @@ fsm_start:
 		 * be invoked before the data transfer is complete and
 		 * hsm_task_state is changed. Hence, the following locking.
 		 */
-		if (in_wq)
-			spin_lock_irqsave(ap->lock, flags);
-
 		if (qc->tf.protocol == ATA_PROT_PIO) {
 			/* PIO data out protocol.
 			 * send first data block.
@@ -4869,13 +5014,10 @@ fsm_start:
 			 * before ata_pio_sectors().
 			 */
 			ap->hsm_task_state = HSM_ST;
-			ata_pio_sectors(qc);
+			ata_pio_sectors(qc, in_wq);
 		} else
 			/* send CDB */
-			atapi_send_cdb(ap, qc);
-
-		if (in_wq)
-			spin_unlock_irqrestore(ap->lock, flags);
+			atapi_send_cdb(ap, qc, in_wq);
 
 		/* if polling, ata_pio_task() handles the rest.
 		 * otherwise, interrupt handler takes over from here.
@@ -4909,7 +5051,11 @@ fsm_start:
 				goto fsm_start;
 			}
 
-			atapi_pio_bytes(qc);
+			atapi_pio_bytes(qc, in_wq);
+
+			/* irq handler or another command might
+			 *  be running at this point
+			 */
 
 			if (unlikely(ap->hsm_task_state == HSM_ST_ERR))
 				/* bad ireason reported by device */
@@ -4949,7 +5095,10 @@ fsm_start:
 				qc->err_mask |= AC_ERR_DEV;
 
 				if (!(qc->tf.flags & ATA_TFLAG_WRITE)) {
-					ata_pio_sectors(qc);
+					/* set irq_handover = 0 since
+					 * irq is not expected here
+					 */
+					ata_pio_sectors(qc, 0);
 					status = ata_wait_idle(ap);
 				}
 
@@ -4964,7 +5113,11 @@ fsm_start:
 				goto fsm_start;
 			}
 
-			ata_pio_sectors(qc);
+			ata_pio_sectors(qc, in_wq);
+
+			/* irq handler or another command might
+			 *  be running at this point
+			 */
 
 			if (ap->hsm_task_state == HSM_ST_IDLE) {
 				/* all data read */
@@ -5026,6 +5179,7 @@ static void ata_pio_task(struct work_str
 	struct ata_queued_cmd *qc = ap->port_task_data;
 	u8 status;
 	int poll_next;
+	unsigned long irq_flags;
 
 fsm_start:
 	WARN_ON(ap->hsm_task_state == HSM_ST_IDLE);
@@ -5047,6 +5201,11 @@ fsm_start:
 		}
 	}
 
+	/* Let the irq handler know wq is accessing the port */
+	spin_lock_irqsave(ap->lock, irq_flags);
+	ap->pflags |= ATA_PFLAG_HSM_WQ;
+	spin_unlock_irqrestore(ap->lock, irq_flags);
+
 	/* move the HSM */
 	poll_next = ata_hsm_move(ap, qc, status, 1);
 
@@ -5160,6 +5319,7 @@ void __ata_qc_complete(struct ata_queued
 	 */
 	qc->flags &= ~ATA_QCFLAG_ACTIVE;
 	ap->qc_active &= ~(1 << qc->tag);
+	ap->pflags &= ~ATA_PFLAG_HSM_WQ;
 
 	/* call completion callback */
 	qc->complete_fn(qc);
@@ -5530,6 +5690,10 @@ inline unsigned int ata_host_intr (struc
 	VPRINTK("ata%u: protocol %d task_state %d\n",
 		ap->print_id, qc->tf.protocol, ap->hsm_task_state);
 
+	/* HSM accessing the port from the workqueue */
+	if (ap->pflags & ATA_PFLAG_HSM_WQ)
+		goto idle_irq;
+
 	/* Check whether we are expecting interrupt in this state */
 	switch (ap->hsm_task_state) {
 	case HSM_ST_FIRST:
diff -Nrup 03_read_state/drivers/ata/sata_sil.c 04_move_narrow_lock/drivers/ata/sata_sil.c
--- 03_read_state/drivers/ata/sata_sil.c	2007-05-14 12:18:45.000000000 +0800
+++ 04_move_narrow_lock/drivers/ata/sata_sil.c	2007-05-15 12:33:37.000000000 +0800
@@ -396,6 +396,10 @@ static void sil_host_intr(struct ata_por
 	if (unlikely(!qc))
 		goto freeze;
 
+	/* HSM accessing the port from the workqueue */
+	if (ap->pflags & ATA_PFLAG_HSM_WQ)
+		return;
+
 	if (unlikely(qc->tf.flags & ATA_TFLAG_POLLING)) {
 		/* this sometimes happens, just clear IRQ */
 		ata_chk_status(ap);
diff -Nrup 03_read_state/include/linux/libata.h 04_move_narrow_lock/include/linux/libata.h
--- 03_read_state/include/linux/libata.h	2007-05-14 12:19:04.000000000 +0800
+++ 04_move_narrow_lock/include/linux/libata.h	2007-05-14 14:45:28.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_WQ	= (1 << 19), /* hsm accessing the port in wq */
 
 	/* struct ata_queued_cmd flags */
 	ATA_QCFLAG_ACTIVE	= (1 << 0), /* cmd not yet ack'd to scsi lyer */

[PATCH 5/8] libata: ack unsolicited INTRQ when polling

[Albert Lee]

patch 5/8:

- Move the ATA_TFLAG_POLLING check from ata_interrupt to ata_host_intr
- Ack unsolicited INTRQ if polling and before the HSM accessing the port.
  (e.g. some device asserts INTRQ even if polling and nIEN = 1.)

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Tejun Heo <htejun@gmail.com>
---
1.
This fixes the Benq "irq nobody cared" problem.
 (Some device asserts INTRQ even if nIEN = 1 and polling.
http://bugzilla.kernel.org/show_bug.cgi?id=8441)

Other unsolicited irqs that Alan concerned about will be address in patch 8/8.

2. Per Tejun's advice, I have checked the LLDDs.
The following LLDDs checks ATA_TFLAG_POLLING in their interrupt handler:
  pdc_adma, sata_inic162x, sata_mv, sata_nv, sata_promise,
  sata_qstor, sata_sil, sata_sx4 and sata_vsc.
After review, it seems non of them requires change due to this patch.

BTW, sata_sil and some other LLDDs already have such workaround in it.

diff -Nrup 04_move_narrow_lock/drivers/ata/libata-core.c 05_polling_ack/drivers/ata/libata-core.c
--- 04_move_narrow_lock/drivers/ata/libata-core.c	2007-05-16 13:53:16.000000000 +0800
+++ 05_polling_ack/drivers/ata/libata-core.c	2007-05-16 13:53:20.000000000 +0800
@@ -5694,6 +5694,13 @@ inline unsigned int ata_host_intr (struc
 	if (ap->pflags & ATA_PFLAG_HSM_WQ)
 		goto idle_irq;
 
+	/* polling, while HSM not yet active in wq */
+	if (qc->tf.flags & ATA_TFLAG_POLLING) {
+		/* ack unsolicited irq */
+		ata_chk_status(ap);
+		goto idle_irq;
+	}
+
 	/* Check whether we are expecting interrupt in this state */
 	switch (ap->hsm_task_state) {
 	case HSM_ST_FIRST:
@@ -5804,8 +5811,7 @@ 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))
+			if (qc && (qc->flags & ATA_QCFLAG_ACTIVE))
 				handled |= ata_host_intr(ap, qc);
 		}
 	}

[PATCH 6/8] libata: delegate irq driven pio to workqueue

[Albert Lee]

patch 6/8:
 - Delegate irq driven pio to workqueue.
 - HSM_ST_LAST is kept in the interrupt since this is not CPU intensive.
   (Mostly for DMA and non-data.)

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Tejun Heo <htejun@gmail.com>
---
sata_sil is not changed to delegate to workqueue at this moment.
Maybe we can add such feature in the future if needed.


diff -Nrup 05_polling_ack/drivers/ata/libata-core.c 06_irq_wq/drivers/ata/libata-core.c
--- 05_polling_ack/drivers/ata/libata-core.c	2007-05-16 13:53:20.000000000 +0800
+++ 06_irq_wq/drivers/ata/libata-core.c	2007-05-16 13:53:25.000000000 +0800
@@ -4864,20 +4864,15 @@ err_out:
 
 static inline int ata_hsm_ok_in_wq(struct ata_port *ap, struct ata_queued_cmd *qc)
 {
-	if (qc->tf.flags & ATA_TFLAG_POLLING)
-		return 1;
-
-	if (ap->hsm_task_state == HSM_ST_FIRST) {
-		if (qc->tf.protocol == ATA_PROT_PIO &&
-		    (qc->tf.flags & ATA_TFLAG_WRITE))
-		    return 1;
-
-		if (is_atapi_taskfile(&qc->tf) &&
-		    !(qc->dev->flags & ATA_DFLAG_CDB_INTR))
-			return 1;
+	switch (qc->tf.protocol) {
+	case ATA_PROT_DMA:
+		return 0;
+	case ATA_PROT_ATAPI_DMA:
+		if (ap->hsm_task_state != HSM_ST_FIRST)
+			return 0;
 	}
 
-	return 0;
+	return (ap->pflags & ATA_PFLAG_HSM_WQ) ? 1 : 0;
 }
 
 /**
@@ -5217,6 +5212,39 @@ fsm_start:
 }
 
 /**
+ *	ata_irq_task - queue task for irq pio
+ *	@work: associated work_struct
+ *
+ *	LOCKING:
+ *	None.
+ */
+
+static void ata_irq_task(struct work_struct *work)
+{
+	struct ata_port *ap =
+		container_of(work, struct ata_port, port_task.work);
+	struct ata_queued_cmd *qc = ap->port_task_data;
+	u8 status;
+
+	WARN_ON(ap->hsm_task_state == HSM_ST_IDLE);
+
+	/* double check the device is not busy */
+	status = ata_chk_status(ap);
+	if (status & ATA_BUSY) {
+		ata_port_printk(ap, KERN_ERR, "Unexpected device busy "
+				"(Status 0x%x)\n", status);
+		return;
+	}
+
+	/* move the HSM */
+	ata_hsm_move(ap, qc, status, 1);
+
+	/* another command or interrupt handler
+	 * may be running at this point.
+	 */
+}
+
+/**
  *	ata_qc_new - Request an available ATA command, for queueing
  *	@ap: Port associated with device @dev
  *	@dev: Device from whom we request an available command structure
@@ -5756,11 +5784,21 @@ inline unsigned int ata_host_intr (struc
 	/* ack bmdma irq events */
 	ap->ops->irq_clear(ap);
 
-	ata_hsm_move(ap, qc, status, 0);
+	/* move the HSM */
+	switch (ap->hsm_task_state) {
+	case HSM_ST_FIRST:
+	case HSM_ST:
+		/* delegate PIO data transfer to workqueue */
+		ap->pflags |= ATA_PFLAG_HSM_WQ;
+		ata_port_queue_task(ap, ata_irq_task, qc, 0);
+		break;
+	default:
+		ata_hsm_move(ap, qc, status, 0);
 
-	if (unlikely(qc->err_mask) && (qc->tf.protocol == ATA_PROT_DMA ||
-				       qc->tf.protocol == ATA_PROT_ATAPI_DMA))
-		ata_ehi_push_desc(ehi, "BMDMA stat 0x%x", host_stat);
+		if (unlikely(qc->err_mask) && (qc->tf.protocol == ATA_PROT_DMA ||
+					       qc->tf.protocol == ATA_PROT_ATAPI_DMA))
+			ata_ehi_push_desc(ehi, "BMDMA stat 0x%x", host_stat);
+	}
 
 	return 1;	/* irq handled */
 

[PATCH 7/8] libata: fix ata_port_flush_task() for irq pio delegation

[Albert Lee]

7/8:
Submitting tasks from ata_host_intr() breaks ata_port_flush_task().
This patch adds code to disable irq pio delegation to ata_port_flush_task().
Irq pio delegation is re-enabled when next qc is issued.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
Cc: Tejun Heo <htejun@gmail.com>
---
Don't know if this is good enough. Maybe Tejun have better ideas.

diff -Nrup 06_irq_wq/drivers/ata/libata-core.c 07_irq_wq_fix/drivers/ata/libata-core.c
--- 06_irq_wq/drivers/ata/libata-core.c	2007-05-16 13:53:25.000000000 +0800
+++ 07_irq_wq_fix/drivers/ata/libata-core.c	2007-05-16 13:53:37.000000000 +0800
@@ -1321,6 +1321,7 @@ void ata_port_flush_task(struct ata_port
 
 	spin_lock_irqsave(ap->lock, flags);
 	ap->pflags |= ATA_PFLAG_FLUSH_PORT_TASK;
+	ap->pflags &= ~ATA_PFLAG_IRQ_DELEGATE;
 	spin_unlock_irqrestore(ap->lock, flags);
 
 	DPRINTK("flush #1\n");
@@ -5604,6 +5605,9 @@ unsigned int ata_qc_issue_prot(struct at
 	    (ap->flags & ATA_FLAG_SETXFER_POLLING))
 		qc->tf.flags |= ATA_TFLAG_POLLING;
 
+	/* delegate data transfer of irq driven pio to workqueue */
+	ap->pflags |=  ATA_PFLAG_IRQ_DELEGATE;
+
 	/* select the device */
 	ata_dev_select(ap, qc->dev->devno, 1, 0);
 
@@ -5788,9 +5792,12 @@ inline unsigned int ata_host_intr (struc
 	switch (ap->hsm_task_state) {
 	case HSM_ST_FIRST:
 	case HSM_ST:
-		/* delegate PIO data transfer to workqueue */
-		ap->pflags |= ATA_PFLAG_HSM_WQ;
-		ata_port_queue_task(ap, ata_irq_task, qc, 0);
+		if (ap->pflags & ATA_PFLAG_IRQ_DELEGATE) {
+			/* delegate PIO data transfer to workqueue */
+			ap->pflags |= ATA_PFLAG_HSM_WQ;
+			ata_port_queue_task(ap, ata_irq_task, qc, 0);
+		} else
+			ata_hsm_move(ap, qc, status, 0);
 		break;
 	default:
 		ata_hsm_move(ap, qc, status, 0);
diff -Nrup 06_irq_wq/include/linux/libata.h 07_irq_wq_fix/include/linux/libata.h
--- 06_irq_wq/include/linux/libata.h	2007-05-14 14:45:28.000000000 +0800
+++ 07_irq_wq_fix/include/linux/libata.h	2007-05-15 18:14:49.000000000 +0800
@@ -196,6 +196,7 @@ enum {
 	ATA_PFLAG_SUSPENDED	= (1 << 17), /* port is suspended (power) */
 	ATA_PFLAG_PM_PENDING	= (1 << 18), /* PM operation pending */
 	ATA_PFLAG_HSM_WQ	= (1 << 19), /* hsm accessing the port in wq */
+	ATA_PFLAG_IRQ_DELEGATE	= (1 << 20), /* delegate irq pio to wq */
 
 	/* struct ata_queued_cmd flags */
 	ATA_QCFLAG_ACTIVE	= (1 << 0), /* cmd not yet ack'd to scsi lyer */

[PATCH 8/8] libata: ack more unsolicited INTRQ

[Albert Lee]

patch 8/8: ack more unsolicited irq

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Mark Lord <liml@rtr.ca>
Cc: Tejun Heo <htejun@gmail.com>
---
To address Alan's concern about the unsolicited irq (hopefully),
this patch tries to ack irq that happens after HSM accessing the port
but before the data transfer that actually triggers INTRQ.

For a typical transaction:
0. wait for irq or polling for device not busy
1. hsm starts accessing the port (ATA_PFLAG_HSM_WQ set)
2. hsm transfer the "head" part of the data
3. hsm acquires ap->lock
4. hsm transfer the "tail" part of the data (which may trigger INTRQ)
5. hsm clears ATA_PFLAG_HSM_WQ and release ap->lock

This patch allows the irq handler to ack irqs that occur during
1 and 2. (Another patch 5/8 acks irq before 1 and when polling.)

As previously discussed, the possible issue with this patch is:
Some ATA/ATAPI devices might be unhappy if the STATUS register
is read during data transfer (not sure if this is true or not).
(Patch 5/8 doesn't have such issue.) 


diff -Nrup 07_irq_wq_fix/drivers/ata/libata-core.c 08_possible_ack/drivers/ata/libata-core.c
--- 07_irq_wq_fix/drivers/ata/libata-core.c	2007-05-16 13:53:37.000000000 +0800
+++ 08_possible_ack/drivers/ata/libata-core.c	2007-05-16 13:53:45.000000000 +0800
@@ -5723,8 +5723,14 @@ inline unsigned int ata_host_intr (struc
 		ap->print_id, qc->tf.protocol, ap->hsm_task_state);
 
 	/* HSM accessing the port from the workqueue */
-	if (ap->pflags & ATA_PFLAG_HSM_WQ)
+	if (ap->pflags & ATA_PFLAG_HSM_WQ) {
+		/* HSM is not transfering the last piece
+		 * of data that triggers INTRQ yet.
+		 * ack unsolicited irq.
+		 */
+		ata_chk_status(ap);
 		goto idle_irq;
+	}
 
 	/* polling, while HSM not yet active in wq */
 	if (qc->tf.flags & ATA_TFLAG_POLLING) {

Re: [PATCH 8/8] libata: ack more unsolicited INTRQ

[Alan Cox]

> As previously discussed, the possible issue with this patch is:
> Some ATA/ATAPI devices might be unhappy if the STATUS register
> is read during data transfer (not sure if this is true or not).
> (Patch 5/8 doesn't have such issue.) 

Some older intel eats your disk if you do that.

Neither of these approaches quite work. Much as I dislike the "old IDE"
disable/enable irq approach it does look like that is the only safe
answer for some controllers.

Alan
--

Re: [PATCH 8/8] libata: ack more unsolicited INTRQ

[Albert Lee]

Alan Cox wrote:
>>As previously discussed, the possible issue with this patch is:
>>Some ATA/ATAPI devices might be unhappy if the STATUS register
>>is read during data transfer (not sure if this is true or not).
>>(Patch 5/8 doesn't have such issue.) 
> 
> 
> Some older intel eats your disk if you do that.

Ah, too bad. Thanks for the advice.

> 
> Neither of these approaches quite work. Much as I dislike the "old IDE"
> disable/enable irq approach it does look like that is the only safe
> answer for some controllers.
> 

Agreed reading the status register during data transfer looks bad.
Please disregard this patch.

Back to the problem that the patch was trying to solve,
i.e. unsolicited interrupt when HSM doing data transfer in the wq,
is there any experience about how often such situation occurs?

IMHO, it seems not something that happens often. If the cable/devices
are well configured, the intrq would mostly occur when it should.

Even if the unsolicited irq happens, maybe the current code has
good chance to handle it?
e.g. ata_irq_task() already reads the status before data transfer,
thus possibly clearing some of unsolicited irqs.
e.g. maybe the data transfer in the workqueue is quick enough?
If yes, hopefully the ATA_PFLAG_HSM_WQ is soon cleared, and
then the interrupt handler can come in ack the irq. (Much the same
way when the irq handler encounters "early irq" by bad devices.)

--
albert

Re: [PATCH 8/8] libata: ack more unsolicited INTRQ

[Tejun Heo]

Hello, Albert.

Albert Lee wrote:
> Back to the problem that the patch was trying to solve,
> i.e. unsolicited interrupt when HSM doing data transfer in the wq,
> is there any experience about how often such situation occurs?
> 
> IMHO, it seems not something that happens often. If the cable/devices
> are well configured, the intrq would mostly occur when it should.
> 
> Even if the unsolicited irq happens, maybe the current code has
> good chance to handle it?
> e.g. ata_irq_task() already reads the status before data transfer,
> thus possibly clearing some of unsolicited irqs.
> e.g. maybe the data transfer in the workqueue is quick enough?
> If yes, hopefully the ATA_PFLAG_HSM_WQ is soon cleared, and
> then the interrupt handler can come in ack the irq. (Much the same
> way when the irq handler encounters "early irq" by bad devices.)

The problem is that if you have only one cpu and the unsolicited IRQ
happens, you never get to execute any non-IRQ handler code.  IRQ is
raised immediately again when the IRQ handler is complete, so nothing
can be quick enough.

I don't really know how to fix this.  It seems the only options are 1.
disable_irq() as IDE does which sucks if the IRQ is shared 2. doing PIO
transfers with local IRQ disabled and spinlock locked which makes the
system stutter like hell.  Oh crap, all this because we don't have
simple reliable IRQ pending bit.  :-(

Oh, BTW, we do have reliable IRQ pending bit on ata_piix's.  The IRQ bit
in BMDMA status acts as a IRQ pending even if the command isn't a DMA
one.  This is why we clear BMDMA IRQ even after nodata or PIO commands,
so we can probably solve the problem nicely for ata_piix by calling
ata_port_freeze() if IRQ is received while ATA_PFLAG_HSM_WQ is set.
sata_sil also has reliable IRQ pending bit.

Thanks.

-- 
tejun

Re: [PATCH 8/8] libata: ack more unsolicited INTRQ

[Mark Lord]

Tejun Heo wrote:
> 
> I don't really know how to fix this.  It seems the only options are 1.
> disable_irq() as IDE does which sucks if the IRQ is shared 2. doing PIO
> transfers with local IRQ disabled and spinlock locked which makes the
> system stutter like hell.  Oh crap, all this because we don't have
> simple reliable IRQ pending bit.  :-(
> 
> Oh, BTW, we do have reliable IRQ pending bit on ata_piix's.  The IRQ bit
> in BMDMA status acts as a IRQ pending even if the command isn't a DMA
> one.  This is why we clear BMDMA IRQ even after nodata or PIO commands,
> so we can probably solve the problem nicely for ata_piix by calling
> ata_port_freeze() if IRQ is received while ATA_PFLAG_HSM_WQ is set.
> sata_sil also has reliable IRQ pending bit.

I'm not sure if this suggestion is the same as what you just described,
but what we can do here is just set a flag before the PIO block,
and clear it again after PIO completes.

The libata IRQ handler for that channel would then check for this flag
on entry, before touching any chipset registers, and if it sees the flag
then it should self-disable the IRQ and return immediately.

The PIO block would then also have to know to re-enable the IRQ on completion.

This way, we only disable the IRQ when it is actually necessary
for the specific hardware at run-time.

Cheers

Re: [PATCH 8/8] libata: ack more unsolicited INTRQ

[Tejun Heo]

Mark Lord wrote:
> Tejun Heo wrote:
>>
>> I don't really know how to fix this.  It seems the only options are 1.
>> disable_irq() as IDE does which sucks if the IRQ is shared 2. doing PIO
>> transfers with local IRQ disabled and spinlock locked which makes the
>> system stutter like hell.  Oh crap, all this because we don't have
>> simple reliable IRQ pending bit.  :-(
>>
>> Oh, BTW, we do have reliable IRQ pending bit on ata_piix's.  The IRQ bit
>> in BMDMA status acts as a IRQ pending even if the command isn't a DMA
>> one.  This is why we clear BMDMA IRQ even after nodata or PIO commands,
>> so we can probably solve the problem nicely for ata_piix by calling
>> ata_port_freeze() if IRQ is received while ATA_PFLAG_HSM_WQ is set.
>> sata_sil also has reliable IRQ pending bit.
> 
> I'm not sure if this suggestion is the same as what you just described,
> but what we can do here is just set a flag before the PIO block,
> and clear it again after PIO completes.
> 
> The libata IRQ handler for that channel would then check for this flag
> on entry, before touching any chipset registers, and if it sees the flag
> then it should self-disable the IRQ and return immediately.
> 
> The PIO block would then also have to know to re-enable the IRQ on
> completion.
> 
> This way, we only disable the IRQ when it is actually necessary
> for the specific hardware at run-time.

Heh heh, that was the first approach tried and we couldn't solve
unexpected IRQ problem which can be triggered by cable problem or
whatever.  If the work thread doesn't turn off IRQ and the IRQ handler
doesn't clear it when it happens, we end up with nobody-cared.

-- 
tejun

Re: [PATCH 8/8] libata: ack more unsolicited INTRQ

[Alan Cox]

> The libata IRQ handler for that channel would then check for this flag
> on entry, before touching any chipset registers, and if it sees the flag
> then it should self-disable the IRQ and return immediately.

How do you plan to disable the IRQ at this point ?

Alan

Re: [PATCH 8/8] libata: ack more unsolicited INTRQ

[Alan Cox]

> Even if the unsolicited irq happens, maybe the current code has
> good chance to handle it?
> e.g. ata_irq_task() already reads the status before data transfer,
> thus possibly clearing some of unsolicited irqs.
> e.g. maybe the data transfer in the workqueue is quick enough?

Probably not given the PIO cycle time.

> If yes, hopefully the ATA_PFLAG_HSM_WQ is soon cleared, and
> then the interrupt handler can come in ack the irq. (Much the same
> way when the irq handler encounters "early irq" by bad devices.)

We seem to have three classes of controller here

- Those that need IRQs off for transfers anyway (eg ancient VIA)
- Those that we must not touch the status bits for but can otherwise
  behave sanely (Some PIIX, ICH and possibly some others)
- Those that behave sanely

The first lot are really "No change" and not found on SMP boxes anyway
The final ones would I think work with your patch (some testing required)

The best I can see that we could do with the Intel ones would be to use
disable_irq/enable_irq, especially as they are rarely found in native
mode so usually have a private IRQ. The only problem is that we must
disable_irq before we take the other locks otherwise we can deadlock in
disable_irq waiting for the irq handlers to finish while the IRQ handler
is trying to take the lock so never does.