[PATCH/RFC 0/5] libata-dev: integrate polling pio with irq-pio

[PATCH/RFC 0/5] libata-dev: integrate polling pio with irq-pio

[Albert Lee]

This patch set integrates polling pio with irq-pio.

Changes:
1/5: Move out the HSM code from ata_host_intr() to the new ata_hsm_move() function.
2/5: Minor fix for ata_hsm_move() to work with ata_host_intr()
3/5: Let ata_hsm_move() work with both irq-pio and polling pio
4/5: Convert ata_pio_task() to use the new ata_hsm_move()
5/5: Cleanup unused enums/functions.

Patch against irq-pio branch (c2956a3b0d1c17b38da369811a6ce93eb7a01a04). 
Minimally tested ok on x86. 

For your review, thanks,

Albert

[PATCH/RFC 1/5] libata-dev: Move out the HSM code from ata_host_intr()

[Albert Lee]

Move out the irq-pio HSM code from ata_host_intr() to the new ata_hsm_move() function verbatim.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

---

--- irq-pio/drivers/scsi/libata-core.c	2006-03-08 14:24:54.000000000 +0800
+++ 01_move_out/drivers/scsi/libata-core.c	2006-03-08 18:10:21.000000000 +0800
@@ -3628,6 +3628,111 @@ static void ata_pio_error(struct ata_por
 	ata_poll_qc_complete(qc);
 }
 
+static void ata_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
+			u8 status)
+{
+	/* check error */
+	if (unlikely(status & (ATA_ERR | ATA_DF))) {
+		qc->err_mask |= AC_ERR_DEV;
+		ap->hsm_task_state = HSM_ST_ERR;
+	}
+
+fsm_start:
+	switch (ap->hsm_task_state) {
+	case HSM_ST_FIRST:
+		/* Some pre-ATAPI-4 devices assert INTRQ 
+		 * at this state when ready to receive CDB.
+		 */
+
+		/* check device status */
+		if (unlikely((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)) {
+			/* Wrong status. Let EH handle this */
+			qc->err_mask |= AC_ERR_HSM;
+			ap->hsm_task_state = HSM_ST_ERR;
+			goto fsm_start;
+		}
+
+		atapi_send_cdb(ap, qc);
+
+		break;
+
+	case HSM_ST:
+		/* complete command or read/write the data register */
+		if (qc->tf.protocol == ATA_PROT_ATAPI) {
+			/* ATAPI PIO protocol */
+			if ((status & ATA_DRQ) == 0) {
+				/* no more data to transfer */
+				ap->hsm_task_state = HSM_ST_LAST;
+				goto fsm_start;
+			}
+			
+			atapi_pio_bytes(qc);
+
+			if (unlikely(ap->hsm_task_state == HSM_ST_ERR))
+				/* bad ireason reported by device */
+				goto fsm_start;
+
+		} else {
+			/* ATA PIO protocol */
+			if (unlikely((status & ATA_DRQ) == 0)) {
+				/* handle BSY=0, DRQ=0 as error */
+				qc->err_mask |= AC_ERR_HSM;
+				ap->hsm_task_state = HSM_ST_ERR;
+				goto fsm_start;
+			}
+
+			ata_pio_sectors(qc);
+
+			if (ap->hsm_task_state == HSM_ST_LAST &&
+			    (!(qc->tf.flags & ATA_TFLAG_WRITE))) {
+				/* all data read */
+				ata_altstatus(ap);
+				status = ata_chk_status(ap);
+				goto fsm_start;
+			}
+		}
+
+		ata_altstatus(ap); /* flush */
+		break;
+
+	case HSM_ST_LAST:
+		if (unlikely(status & ATA_DRQ)) {
+			/* handle DRQ=1 as error */
+			qc->err_mask |= AC_ERR_HSM;
+			ap->hsm_task_state = HSM_ST_ERR;
+			goto fsm_start;
+		}
+
+		/* no more data to transfer */
+		DPRINTK("ata%u: command complete, drv_stat 0x%x\n",
+			ap->id, status);
+
+		ap->hsm_task_state = HSM_ST_IDLE;
+
+		/* complete taskfile transaction */
+		qc->err_mask |= ac_err_mask(status);
+		ata_qc_complete(qc);
+		break;
+
+	case HSM_ST_ERR:
+		if (qc->tf.command != ATA_CMD_PACKET)
+			printk(KERN_ERR "ata%u: command error, drv_stat 0x%x host_stat 0x%x\n",
+			       ap->id, status, host_stat);
+
+		/* make sure qc->err_mask is available to 
+		 * know what's wrong and recover
+		 */
+		WARN_ON(qc->err_mask == 0);
+
+		ap->hsm_task_state = HSM_ST_IDLE;
+		ata_qc_complete(qc);
+		break;
+	default:
+		goto idle_irq;
+	}
+
+}
+
 static void ata_pio_task(void *_data)
 {
 	struct ata_port *ap = _data;
@@ -4372,106 +4477,7 @@ inline unsigned int ata_host_intr (struc
 	/* ack bmdma irq events */
 	ap->ops->irq_clear(ap);
 
-	/* check error */
-	if (unlikely(status & (ATA_ERR | ATA_DF))) {
-		qc->err_mask |= AC_ERR_DEV;
-		ap->hsm_task_state = HSM_ST_ERR;
-	}
-
-fsm_start:
-	switch (ap->hsm_task_state) {
-	case HSM_ST_FIRST:
-		/* Some pre-ATAPI-4 devices assert INTRQ 
-		 * at this state when ready to receive CDB.
-		 */
-
-		/* check device status */
-		if (unlikely((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)) {
-			/* Wrong status. Let EH handle this */
-			qc->err_mask |= AC_ERR_HSM;
-			ap->hsm_task_state = HSM_ST_ERR;
-			goto fsm_start;
-		}
-
-		atapi_send_cdb(ap, qc);
-
-		break;
-
-	case HSM_ST:
-		/* complete command or read/write the data register */
-		if (qc->tf.protocol == ATA_PROT_ATAPI) {
-			/* ATAPI PIO protocol */
-			if ((status & ATA_DRQ) == 0) {
-				/* no more data to transfer */
-				ap->hsm_task_state = HSM_ST_LAST;
-				goto fsm_start;
-			}
-			
-			atapi_pio_bytes(qc);
-
-			if (unlikely(ap->hsm_task_state == HSM_ST_ERR))
-				/* bad ireason reported by device */
-				goto fsm_start;
-
-		} else {
-			/* ATA PIO protocol */
-			if (unlikely((status & ATA_DRQ) == 0)) {
-				/* handle BSY=0, DRQ=0 as error */
-				qc->err_mask |= AC_ERR_HSM;
-				ap->hsm_task_state = HSM_ST_ERR;
-				goto fsm_start;
-			}
-
-			ata_pio_sectors(qc);
-
-			if (ap->hsm_task_state == HSM_ST_LAST &&
-			    (!(qc->tf.flags & ATA_TFLAG_WRITE))) {
-				/* all data read */
-				ata_altstatus(ap);
-				status = ata_chk_status(ap);
-				goto fsm_start;
-			}
-		}
-
-		ata_altstatus(ap); /* flush */
-		break;
-
-	case HSM_ST_LAST:
-		if (unlikely(status & ATA_DRQ)) {
-			/* handle DRQ=1 as error */
-			qc->err_mask |= AC_ERR_HSM;
-			ap->hsm_task_state = HSM_ST_ERR;
-			goto fsm_start;
-		}
-
-		/* no more data to transfer */
-		DPRINTK("ata%u: command complete, drv_stat 0x%x\n",
-			ap->id, status);
-
-		ap->hsm_task_state = HSM_ST_IDLE;
-
-		/* complete taskfile transaction */
-		qc->err_mask |= ac_err_mask(status);
-		ata_qc_complete(qc);
-		break;
-
-	case HSM_ST_ERR:
-		if (qc->tf.command != ATA_CMD_PACKET)
-			printk(KERN_ERR "ata%u: command error, drv_stat 0x%x host_stat 0x%x\n",
-			       ap->id, status, host_stat);
-
-		/* make sure qc->err_mask is available to 
-		 * know what's wrong and recover
-		 */
-		WARN_ON(qc->err_mask == 0);
-
-		ap->hsm_task_state = HSM_ST_IDLE;
-		ata_qc_complete(qc);
-		break;
-	default:
-		goto idle_irq;
-	}
-
+	ata_hsm_move(ap, qc, status);
 	return 1;	/* irq handled */
 
 idle_irq:

Re: [PATCH/RFC 1/5] libata-dev: Move out the HSM code from ata_host_intr()

[Tejun Heo]

The patch contains three trailing whitespaces which are probably just
carried over from the original code.  Please kill the whitespaces.

On Thu, Mar 09, 2006 at 04:45:00PM +0800, Albert Lee wrote:
> Move out the irq-pio HSM code from ata_host_intr() to the new ata_hsm_move() function verbatim.
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> 
> ---
> 
> --- irq-pio/drivers/scsi/libata-core.c	2006-03-08 14:24:54.000000000 +0800
> +++ 01_move_out/drivers/scsi/libata-core.c	2006-03-08 18:10:21.000000000 +0800
> @@ -3628,6 +3628,111 @@ static void ata_pio_error(struct ata_por
>  	ata_poll_qc_complete(qc);
>  }
>  
> +static void ata_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
> +			u8 status)
> +{
> +	/* check error */
> +	if (unlikely(status & (ATA_ERR | ATA_DF))) {
> +		qc->err_mask |= AC_ERR_DEV;
> +		ap->hsm_task_state = HSM_ST_ERR;
> +	}
> +
> +fsm_start:
> +	switch (ap->hsm_task_state) {
> +	case HSM_ST_FIRST:
> +		/* Some pre-ATAPI-4 devices assert INTRQ 

Here.

> +		 * at this state when ready to receive CDB.
> +		 */
> +
> +		/* check device status */
> +		if (unlikely((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)) {
> +			/* Wrong status. Let EH handle this */
> +			qc->err_mask |= AC_ERR_HSM;
> +			ap->hsm_task_state = HSM_ST_ERR;
> +			goto fsm_start;
> +		}
> +
> +		atapi_send_cdb(ap, qc);
> +
> +		break;
> +
> +	case HSM_ST:
> +		/* complete command or read/write the data register */
> +		if (qc->tf.protocol == ATA_PROT_ATAPI) {
> +			/* ATAPI PIO protocol */
> +			if ((status & ATA_DRQ) == 0) {
> +				/* no more data to transfer */
> +				ap->hsm_task_state = HSM_ST_LAST;
> +				goto fsm_start;
> +			}
> +			

Here.

> +			atapi_pio_bytes(qc);
> +
> +			if (unlikely(ap->hsm_task_state == HSM_ST_ERR))
> +				/* bad ireason reported by device */
> +				goto fsm_start;
> +
> +		} else {
> +			/* ATA PIO protocol */
> +			if (unlikely((status & ATA_DRQ) == 0)) {
> +				/* handle BSY=0, DRQ=0 as error */
> +				qc->err_mask |= AC_ERR_HSM;
> +				ap->hsm_task_state = HSM_ST_ERR;
> +				goto fsm_start;
> +			}
> +
> +			ata_pio_sectors(qc);
> +
> +			if (ap->hsm_task_state == HSM_ST_LAST &&
> +			    (!(qc->tf.flags & ATA_TFLAG_WRITE))) {
> +				/* all data read */
> +				ata_altstatus(ap);
> +				status = ata_chk_status(ap);
> +				goto fsm_start;
> +			}
> +		}
> +
> +		ata_altstatus(ap); /* flush */
> +		break;
> +
> +	case HSM_ST_LAST:
> +		if (unlikely(status & ATA_DRQ)) {
> +			/* handle DRQ=1 as error */
> +			qc->err_mask |= AC_ERR_HSM;
> +			ap->hsm_task_state = HSM_ST_ERR;
> +			goto fsm_start;
> +		}
> +
> +		/* no more data to transfer */
> +		DPRINTK("ata%u: command complete, drv_stat 0x%x\n",
> +			ap->id, status);
> +
> +		ap->hsm_task_state = HSM_ST_IDLE;
> +
> +		/* complete taskfile transaction */
> +		qc->err_mask |= ac_err_mask(status);
> +		ata_qc_complete(qc);
> +		break;
> +
> +	case HSM_ST_ERR:
> +		if (qc->tf.command != ATA_CMD_PACKET)
> +			printk(KERN_ERR "ata%u: command error, drv_stat 0x%x host_stat 0x%x\n",
> +			       ap->id, status, host_stat);
> +
> +		/* make sure qc->err_mask is available to 

And here.

-- 
tejun

[PATCH/RFC 2/5] libata-dev: Minor fix for ata_hsm_move() to work with ata_host_intr()

[Albert Lee]

Minor fix for ata_hsm_move() to work with ata_host_intr().

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---

Changes:
- WARN_ON() and comment fix
- Make the HSM_ST_LAST device status checking more rigid. 
- Treate unknown HSM state as BUG().

--- 01_move_out/drivers/scsi/libata-core.c	2006-03-08 18:10:21.000000000 +0800
+++ 02_minor_fix/drivers/scsi/libata-core.c	2006-03-08 18:58:16.000000000 +0800
@@ -3631,6 +3631,9 @@ static void ata_pio_error(struct ata_por
 static void ata_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
 			u8 status)
 {
+	WARN_ON(qc == NULL);
+	WARN_ON((qc->flags & ATA_QCFLAG_ACTIVE) == 0);
+
 	/* check error */
 	if (unlikely(status & (ATA_ERR | ATA_DF))) {
 		qc->err_mask |= AC_ERR_DEV;
@@ -3640,10 +3643,6 @@ static void ata_hsm_move(struct ata_port
 fsm_start:
 	switch (ap->hsm_task_state) {
 	case HSM_ST_FIRST:
-		/* Some pre-ATAPI-4 devices assert INTRQ 
-		 * at this state when ready to receive CDB.
-		 */
-
 		/* check device status */
 		if (unlikely((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)) {
 			/* Wrong status. Let EH handle this */
@@ -3696,9 +3695,8 @@ fsm_start:
 		break;
 
 	case HSM_ST_LAST:
-		if (unlikely(status & ATA_DRQ)) {
-			/* handle DRQ=1 as error */
-			qc->err_mask |= AC_ERR_HSM;
+		if (unlikely(!ata_ok(status))) {
+			qc->err_mask |= __ac_err_mask(status);
 			ap->hsm_task_state = HSM_ST_ERR;
 			goto fsm_start;
 		}
@@ -3707,17 +3705,18 @@ fsm_start:
 		DPRINTK("ata%u: command complete, drv_stat 0x%x\n",
 			ap->id, status);
 
+		WARN_ON(qc->err_mask);
+
 		ap->hsm_task_state = HSM_ST_IDLE;
 
 		/* complete taskfile transaction */
-		qc->err_mask |= ac_err_mask(status);
 		ata_qc_complete(qc);
 		break;
 
 	case HSM_ST_ERR:
 		if (qc->tf.command != ATA_CMD_PACKET)
-			printk(KERN_ERR "ata%u: command error, drv_stat 0x%x host_stat 0x%x\n",
-			       ap->id, status, host_stat);
+			printk(KERN_ERR "ata%u: command error, drv_stat 0x%x\n",
+			       ap->id, status);
 
 		/* make sure qc->err_mask is available to 
 		 * know what's wrong and recover
@@ -3728,7 +3727,7 @@ fsm_start:
 		ata_qc_complete(qc);
 		break;
 	default:
-		goto idle_irq;
+		BUG();
 	}
 
 }
@@ -4427,6 +4426,10 @@ inline unsigned int ata_host_intr (struc
 	/* Check whether we are expecting interrupt in this state */
 	switch (ap->hsm_task_state) {
 	case HSM_ST_FIRST:
+		/* Some pre-ATAPI-4 devices assert INTRQ 
+		 * at this state when ready to receive CDB.
+		 */
+
 		/* Check the ATA_DFLAG_CDB_INTR flag is enough here.
 		 * The flag was turned on only for atapi devices.
 		 * No need to check is_atapi_taskfile(&qc->tf) again.

Re: [PATCH/RFC 2/5] libata-dev: Minor fix for ata_hsm_move() to work with ata_host_intr()

[Tejun Heo]

Please kill trailing whitespace.

On Thu, Mar 09, 2006 at 04:49:32PM +0800, Albert Lee wrote:
[--- snip ---]
> @@ -4427,6 +4426,10 @@ inline unsigned int ata_host_intr (struc
>  	/* Check whether we are expecting interrupt in this state */
>  	switch (ap->hsm_task_state) {
>  	case HSM_ST_FIRST:
> +		/* Some pre-ATAPI-4 devices assert INTRQ 

HERE!!!

> +		 * at this state when ready to receive CDB.
> +		 */
> +
>  		/* Check the ATA_DFLAG_CDB_INTR flag is enough here.
>  		 * The flag was turned on only for atapi devices.
>  		 * No need to check is_atapi_taskfile(&qc->tf) again.
> 
> 

[PATCH/RFC 3/5] libata-dev: Let ata_hsm_move() work with both irq-pio and polling pio

[Albert Lee]

Let ata_hsm_move() work with both irq-pio and polling pio codepath.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
Changes:
- add a new parameter "in_wq" for polling pio
- add return value "poll_next" to telling polling pio task whether the HSM is finished
- merge code from ata_pio_first_block() to the HSM_ST_FIRST state.
- call ata_poll_qc_complete() if called from the workqueue

--- 02_minor_fix/drivers/scsi/libata-core.c	2006-03-08 18:58:16.000000000 +0800
+++ 03_support_wq/drivers/scsi/libata-core.c	2006-03-09 15:14:00.000000000 +0800
@@ -3628,12 +3628,33 @@ static void ata_pio_error(struct ata_por
 	ata_poll_qc_complete(qc);
 }
 
-static void ata_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
-			u8 status)
+/**
+ *	ata_hsm_move - move the HSM to the next state.
+ *	@ap: the target ata_port
+ *	@qc: qc on going
+ *	@status: current device status
+ *	@in_wq: 1 if called from workqueue, 0 otherwise
+ *
+ *	RETURNS:
+ *	1 when poll next status needed, 0 otherwise.
+ */
+
+static 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 == NULL);
 	WARN_ON((qc->flags & ATA_QCFLAG_ACTIVE) == 0);
 
+	WARN_ON(in_wq != ((qc->tf.flags & ATA_TFLAG_POLLING) ||
+			  (ap->hsm_task_state == HSM_ST_FIRST &&
+			   ((qc->tf.protocol == ATA_PROT_PIO &&
+			     (qc->tf.flags & ATA_TFLAG_WRITE)) ||
+			    (is_atapi_taskfile(&qc->tf) &&
+			     !(qc->dev->flags & ATA_DFLAG_CDB_INTR))))));
+
 	/* check error */
 	if (unlikely(status & (ATA_ERR | ATA_DF))) {
 		qc->err_mask |= AC_ERR_DEV;
@@ -3643,6 +3664,13 @@ static void ata_hsm_move(struct ata_port
 fsm_start:
 	switch (ap->hsm_task_state) {
 	case HSM_ST_FIRST:
+		/* Send first data block or PACKET CDB */
+
+		/* if polling, we will stay in the work queue after sending the data.
+		 * otherwise, interrupt handler takes over after sending the data.
+		 */
+		poll_next = (qc->tf.flags & ATA_TFLAG_POLLING);
+
 		/* check device status */
 		if (unlikely((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)) {
 			/* Wrong status. Let EH handle this */
@@ -3651,8 +3679,35 @@ fsm_start:
 			goto fsm_start;
 		}
 
-		atapi_send_cdb(ap, qc);
+		/* Send the CDB (atapi) or the first data block (ata pio out).
+		 * During the state transition, interrupt handler shouldn't
+		 * 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->host_set->lock, flags);
+
+		if (qc->tf.protocol == ATA_PROT_PIO) {
+			/* PIO data out protocol.
+			 * send first data block.
+			 */
+
+			/* ata_pio_sectors() might change the state to HSM_ST_LAST.
+			 * so, the state is changed here before ata_pio_sectors().
+			 */
+			ap->hsm_task_state = HSM_ST;
+			ata_pio_sectors(qc);
+			ata_altstatus(ap); /* flush */
+		} else
+			/* send CDB */
+			atapi_send_cdb(ap, qc);
 
+		if (in_wq)
+			spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+		/* if polling, ata_pio_task() handles the rest.
+		 * otherwise, interrupt handler takes over from here.
+		 */
 		break;
 
 	case HSM_ST:
@@ -3692,6 +3747,7 @@ fsm_start:
 		}
 
 		ata_altstatus(ap); /* flush */
+		poll_next = 1;
 		break;
 
 	case HSM_ST_LAST:
@@ -3710,7 +3766,12 @@ fsm_start:
 		ap->hsm_task_state = HSM_ST_IDLE;
 
 		/* complete taskfile transaction */
-		ata_qc_complete(qc);
+		if (in_wq)
+			ata_poll_qc_complete(qc);
+		else
+			ata_qc_complete(qc);
+
+		poll_next = 0;
 		break;
 
 	case HSM_ST_ERR:
@@ -3724,12 +3785,20 @@ fsm_start:
 		WARN_ON(qc->err_mask == 0);
 
 		ap->hsm_task_state = HSM_ST_IDLE;
-		ata_qc_complete(qc);
+
+		if (in_wq)
+			ata_poll_qc_complete(qc);
+		else
+			ata_qc_complete(qc);
+
+		poll_next = 0;
 		break;
 	default:
+		poll_next = 0;
 		BUG();
 	}
 
+	return poll_next;
 }
 
 static void ata_pio_task(void *_data)
@@ -4480,7 +4549,7 @@ inline unsigned int ata_host_intr (struc
 	/* ack bmdma irq events */
 	ap->ops->irq_clear(ap);
 
-	ata_hsm_move(ap, qc, status);
+	ata_hsm_move(ap, qc, status, 0);
 	return 1;	/* irq handled */
 
 idle_irq:

Re: [PATCH/RFC 3/5] libata-dev: Let ata_hsm_move() work with both irq-pio and polling pio

[Tejun Heo]

On Thu, Mar 09, 2006 at 04:51:36PM +0800, Albert Lee wrote:
[--- snip ---]
> +
> +static int ata_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
> +			u8 status, int in_wq)

IMHO, polling would be a slightly better name than in_wq.

>  {
> +	unsigned long flags = 0;
> +	int poll_next;
> +
>  	WARN_ON(qc == NULL);
>  	WARN_ON((qc->flags & ATA_QCFLAG_ACTIVE) == 0);
>  
> +	WARN_ON(in_wq != ((qc->tf.flags & ATA_TFLAG_POLLING) ||
> +			  (ap->hsm_task_state == HSM_ST_FIRST &&
> +			   ((qc->tf.protocol == ATA_PROT_PIO &&
> +			     (qc->tf.flags & ATA_TFLAG_WRITE)) ||
> +			    (is_atapi_taskfile(&qc->tf) &&
> +			     !(qc->dev->flags & ATA_DFLAG_CDB_INTR))))));
> +

I think this is a little bit excessive.  Can't we get away with
WARN_ON(in_wq != !in_irq()) or just trust what the caller says?  It's
not like this function has a lot of callers.

>  	/* check error */
>  	if (unlikely(status & (ATA_ERR | ATA_DF))) {
>  		qc->err_mask |= AC_ERR_DEV;
> @@ -3643,6 +3664,13 @@ static void ata_hsm_move(struct ata_port
>  fsm_start:
>  	switch (ap->hsm_task_state) {
>  	case HSM_ST_FIRST:
> +		/* Send first data block or PACKET CDB */
> +
> +		/* if polling, we will stay in the work queue after sending the data.
> +		 * otherwise, interrupt handler takes over after sending the data.
> +		 */
> +		poll_next = (qc->tf.flags & ATA_TFLAG_POLLING);
> +
>  		/* check device status */
>  		if (unlikely((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)) {
>  			/* Wrong status. Let EH handle this */
> @@ -3651,8 +3679,35 @@ fsm_start:
>  			goto fsm_start;
>  		}
>  
> -		atapi_send_cdb(ap, qc);
> +		/* Send the CDB (atapi) or the first data block (ata pio out).
> +		 * During the state transition, interrupt handler shouldn't
> +		 * 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->host_set->lock, flags);
> +
> +		if (qc->tf.protocol == ATA_PROT_PIO) {
> +			/* PIO data out protocol.
> +			 * send first data block.
> +			 */
> +
> +			/* ata_pio_sectors() might change the state to HSM_ST_LAST.
> +			 * so, the state is changed here before ata_pio_sectors().
> +			 */
> +			ap->hsm_task_state = HSM_ST;
> +			ata_pio_sectors(qc);
> +			ata_altstatus(ap); /* flush */
> +		} else
> +			/* send CDB */
> +			atapi_send_cdb(ap, qc);
>  
> +		if (in_wq)
> +			spin_unlock_irqrestore(&ap->host_set->lock, flags);
> +
> +		/* if polling, ata_pio_task() handles the rest.
> +		 * otherwise, interrupt handler takes over from here.
> +		 */
>  		break;
>  

For ATA PIO write transfers, the first transfer and n'th transfers
aren't really different.  The code would be simpler if it handles the
first ATA PIO write in HSM_ST.  And if we do that, HSM_ST_FIRST can be
renamed to HSM_ST_CDB.  Hmmm.. Maybe this should be done in separate
series of patches.

-- 
tejun

Re: [PATCH/RFC 3/5] libata-dev: Let ata_hsm_move() work with both irq-pio and polling pio

[Albert Lee]

Tejun Heo wrote:
> On Thu, Mar 09, 2006 at 04:51:36PM +0800, Albert Lee wrote:
> [--- snip ---]
> 
>>+
>>+static int ata_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
>>+			u8 status, int in_wq)
> 
> 
> IMHO, polling would be a slightly better name than in_wq.
> 
> 

The "in_wq" name helps to differ it from (qc->tf.flags & ATA_TFLAG_POLLING).

>> {
>>+	unsigned long flags = 0;
>>+	int poll_next;
>>+
>> 	WARN_ON(qc == NULL);
>> 	WARN_ON((qc->flags & ATA_QCFLAG_ACTIVE) == 0);
>> 
>>+	WARN_ON(in_wq != ((qc->tf.flags & ATA_TFLAG_POLLING) ||
>>+			  (ap->hsm_task_state == HSM_ST_FIRST &&
>>+			   ((qc->tf.protocol == ATA_PROT_PIO &&
>>+			     (qc->tf.flags & ATA_TFLAG_WRITE)) ||
>>+			    (is_atapi_taskfile(&qc->tf) &&
>>+			     !(qc->dev->flags & ATA_DFLAG_CDB_INTR))))));
>>+
> 
> 
> I think this is a little bit excessive.  Can't we get away with
> WARN_ON(in_wq != !in_irq()) or just trust what the caller says?  It's
> not like this function has a lot of callers.
> 
> 

The check prevents me from doing something stupid to ata_qc_issue_prot(), say,
insert a qc with DMA protocol into the workqueue, etc.
Also it helps to show how in_wq compares with (qc->tf.flags & ATA_TFLAG_POLLING).
(I was once confused when wrote the code.) Will add a comment to make it clear next time.

>> 	/* check error */
>> 	if (unlikely(status & (ATA_ERR | ATA_DF))) {
>> 		qc->err_mask |= AC_ERR_DEV;
>>@@ -3643,6 +3664,13 @@ static void ata_hsm_move(struct ata_port
>> fsm_start:
>> 	switch (ap->hsm_task_state) {
>> 	case HSM_ST_FIRST:
>>+		/* Send first data block or PACKET CDB */
>>+
>>+		/* if polling, we will stay in the work queue after sending the data.
>>+		 * otherwise, interrupt handler takes over after sending the data.
>>+		 */
>>+		poll_next = (qc->tf.flags & ATA_TFLAG_POLLING);
>>+
>> 		/* check device status */
>> 		if (unlikely((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)) {
>> 			/* Wrong status. Let EH handle this */
>>@@ -3651,8 +3679,35 @@ fsm_start:
>> 			goto fsm_start;
>> 		}
>> 
>>-		atapi_send_cdb(ap, qc);
>>+		/* Send the CDB (atapi) or the first data block (ata pio out).
>>+		 * During the state transition, interrupt handler shouldn't
>>+		 * 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->host_set->lock, flags);
>>+
>>+		if (qc->tf.protocol == ATA_PROT_PIO) {
>>+			/* PIO data out protocol.
>>+			 * send first data block.
>>+			 */
>>+
>>+			/* ata_pio_sectors() might change the state to HSM_ST_LAST.
>>+			 * so, the state is changed here before ata_pio_sectors().
>>+			 */
>>+			ap->hsm_task_state = HSM_ST;
>>+			ata_pio_sectors(qc);
>>+			ata_altstatus(ap); /* flush */
>>+		} else
>>+			/* send CDB */
>>+			atapi_send_cdb(ap, qc);
>> 
>>+		if (in_wq)
>>+			spin_unlock_irqrestore(&ap->host_set->lock, flags);
>>+
>>+		/* if polling, ata_pio_task() handles the rest.
>>+		 * otherwise, interrupt handler takes over from here.
>>+		 */
>> 		break;
>> 
> 
> 
> For ATA PIO write transfers, the first transfer and n'th transfers
> aren't really different.  The code would be simpler if it handles the
> first ATA PIO write in HSM_ST.  And if we do that, HSM_ST_FIRST can be
> renamed to HSM_ST_CDB.  Hmmm.. Maybe this should be done in separate
> series of patches.
> 

For ATA PIO write transfer, the first transfer is different:
It is always done by polling, even if irq is turned on.

If we treat the first PIO write transfer as HSM_ST, we need to add some
additional logic to HSM_ST and check whether it is first transfer or not. If it is,
and irq is on, the transition from polling to interrupt-driven must be protected
by spinlock, similar to what's done in HSM_ST_FIRST.

HSM_ST is more frequent than HSM_ST_FIRST, so treat the first PIO
write transfer as HSM_ST generates more if() execution than
treat the first PIO write transfer as HSM_ST_FIRST and do the if() there.

--
Thanks for the review,

Albert

Re: [PATCH/RFC 3/5] libata-dev: Let ata_hsm_move() work with both irq-pio and polling pio

[Tejun Heo]

Albert Lee wrote:
> Tejun Heo wrote:
>>
>>For ATA PIO write transfers, the first transfer and n'th transfers
>>aren't really different.  The code would be simpler if it handles the
>>first ATA PIO write in HSM_ST.  And if we do that, HSM_ST_FIRST can be
>>renamed to HSM_ST_CDB.  Hmmm.. Maybe this should be done in separate
>>series of patches.
>>
> 
> 
> For ATA PIO write transfer, the first transfer is different:
> It is always done by polling, even if irq is turned on.
> 
> If we treat the first PIO write transfer as HSM_ST, we need to add some
> additional logic to HSM_ST and check whether it is first transfer or not. If it is,
> and irq is on, the transition from polling to interrupt-driven must be protected
> by spinlock, similar to what's done in HSM_ST_FIRST.

Oh, you're right. I wasn't thinking of the spinlock. I implemented HSM 
for sata_sil vdma (still slightly broken) and made its HSM function 
always called under the host spinlock, so I could merge ATA HSM_ST_FIRST 
into HSM_ST for ATA WRITE's and got confused about your HSM.  :-)

Hmmm.. I'm not sure but I recall to read about IDE controllers reacting 
badly when disturbed during PIO thus requiring irq-off during PIO. Does 
anyone know better about this?

-- 
tejun

[PATCH/RFC 4/5] libata-dev: Convert ata_pio_task() to use the new ata_hsm_move()

[Albert Lee]

Convert ata_pio_task() to use the new ata_hsm_move().

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
Changes:
- initialize ap->pio_task_timeout() in ata_queue_pio_task()
- refactor ata_pio_task() to poll device status register and call the new ata_hsm_move()
  when device indicates it is not BSY.

--- 03_support_wq/drivers/scsi/libata-core.c	2006-03-09 15:14:00.000000000 +0800
+++ 04_pio_task/drivers/scsi/libata-core.c	2006-03-09 15:14:41.000000000 +0800
@@ -725,6 +725,8 @@ static unsigned int ata_pio_modes(const 
 static inline void
 ata_queue_pio_task(struct ata_port *ap)
 {
+	ap->pio_task_timeout = jiffies + ATA_TMOUT_PIO;
+
 	if (!(ap->flags & ATA_FLAG_FLUSH_PIO_TASK))
 		queue_work(ata_wq, &ap->pio_task);
 }
@@ -3804,44 +3806,55 @@ fsm_start:
 static void ata_pio_task(void *_data)
 {
 	struct ata_port *ap = _data;
-	unsigned long timeout;
-	int has_next;
+	struct ata_queued_cmd *qc;
+	u8 status;
+	int poll_next;
 
 fsm_start:
-	timeout = 0;
-	has_next = 1;
+	WARN_ON(ap->hsm_task_state == HSM_ST_IDLE);
 
-	switch (ap->hsm_task_state) {
-	case HSM_ST_FIRST:
-		has_next = ata_pio_first_block(ap);
-		break;
+	qc = ata_qc_from_tag(ap, ap->active_tag);
+	WARN_ON(qc == NULL);
 
-	case HSM_ST:
-		ata_pio_block(ap);
-		break;
+	/*
+	 * This is purely heuristic.  This is a fast path.
+	 * Sometimes when we enter, BSY will be cleared in
+	 * a chk-status or two.  If not, the drive is probably seeking
+	 * or something.  Snooze for a couple msecs, then
+	 * chk-status again.  If still busy, timeout or queue delayed work.
+	 */
+	status = ata_busy_wait(ap, ATA_BUSY, 5);
+	if (status & ATA_BUSY) {
+		msleep(2);
+		status = ata_busy_wait(ap, ATA_BUSY, 10);
+		if (status & ATA_BUSY) {
+			if (time_before(jiffies, ap->pio_task_timeout)) {
+				ata_queue_delayed_pio_task(ap, ATA_SHORT_PAUSE);
+				return;
+			}
 
-	case HSM_ST_LAST:
-		has_next = ata_pio_complete(ap);
-		break;
+			/* timeout. Let EH handle it later. 
+			 * FIXME: should we remove ap->pio_task_timeout?
+			 */
+			printk(KERN_ERR "ata%u: polling time out\n", ap->id);
+			qc->err_mask |= AC_ERR_TIMEOUT;
+			ap->hsm_task_state = HSM_ST_TMOUT;
+			return; 
+		}
+	}
 
-	case HSM_ST_POLL:
-	case HSM_ST_LAST_POLL:
-		timeout = ata_pio_poll(ap);
-		break;
+	/* FIXME: check if EH is active */
 
-	case HSM_ST_TMOUT:
-	case HSM_ST_ERR:
-		ata_pio_error(ap);
-		return;
+	/* reset the pio task timeout */
+	ap->pio_task_timeout = jiffies + ATA_TMOUT_PIO;
 
-	default:
-		BUG();
-		return;
-	}
+	/* move the HSM */
+	poll_next = ata_hsm_move(ap, qc, status, 1);
 
-	if (timeout)
-		ata_queue_delayed_pio_task(ap, timeout);
-	else if (has_next)
+	/* another command or interrupt handler
+	 * may be running at this point.
+	 */
+	if (poll_next)
 		goto fsm_start;
 }
 

Re: [PATCH/RFC 4/5] libata-dev: Convert ata_pio_task() to use the new ata_hsm_move()

[Tejun Heo]

On Thu, Mar 09, 2006 at 04:54:52PM +0800, Albert Lee wrote:
> --- 03_support_wq/drivers/scsi/libata-core.c	2006-03-09 15:14:00.000000000 +0800
> +++ 04_pio_task/drivers/scsi/libata-core.c	2006-03-09 15:14:41.000000000 +0800
> @@ -725,6 +725,8 @@ static unsigned int ata_pio_modes(const 
>  static inline void
>  ata_queue_pio_task(struct ata_port *ap)
>  {
> +	ap->pio_task_timeout = jiffies + ATA_TMOUT_PIO;
> +
>  	if (!(ap->flags & ATA_FLAG_FLUSH_PIO_TASK))
>  		queue_work(ata_wq, &ap->pio_task);
>  }

I don't think we really need pio_task_timeout.  Generic EH now handles
polling timeouts correctly and I can't see any advantage of
pio_task_timeout.

> @@ -3804,44 +3806,55 @@ fsm_start:
>  static void ata_pio_task(void *_data)
>  {
>  	struct ata_port *ap = _data;
> -	unsigned long timeout;
> -	int has_next;
> +	struct ata_queued_cmd *qc;
> +	u8 status;
> +	int poll_next;
>  
>  fsm_start:
> -	timeout = 0;
> -	has_next = 1;
> +	WARN_ON(ap->hsm_task_state == HSM_ST_IDLE);
>  
> -	switch (ap->hsm_task_state) {
> -	case HSM_ST_FIRST:
> -		has_next = ata_pio_first_block(ap);
> -		break;
> +	qc = ata_qc_from_tag(ap, ap->active_tag);
> +	WARN_ON(qc == NULL);
>  
> -	case HSM_ST:
> -		ata_pio_block(ap);
> -		break;
> +	/*
> +	 * This is purely heuristic.  This is a fast path.
> +	 * Sometimes when we enter, BSY will be cleared in
> +	 * a chk-status or two.  If not, the drive is probably seeking
> +	 * or something.  Snooze for a couple msecs, then
> +	 * chk-status again.  If still busy, timeout or queue delayed work.
> +	 */
> +	status = ata_busy_wait(ap, ATA_BUSY, 5);
> +	if (status & ATA_BUSY) {
> +		msleep(2);
> +		status = ata_busy_wait(ap, ATA_BUSY, 10);
> +		if (status & ATA_BUSY) {
> +			if (time_before(jiffies, ap->pio_task_timeout)) {
> +				ata_queue_delayed_pio_task(ap, ATA_SHORT_PAUSE);
> +				return;
> +			}
>  
> -	case HSM_ST_LAST:
> -		has_next = ata_pio_complete(ap);
> -		break;
> +			/* timeout. Let EH handle it later. 
> +			 * FIXME: should we remove ap->pio_task_timeout?
> +			 */
> +			printk(KERN_ERR "ata%u: polling time out\n", ap->id);
> +			qc->err_mask |= AC_ERR_TIMEOUT;
> +			ap->hsm_task_state = HSM_ST_TMOUT;
> +			return; 
> +		}
> +	}
>  
> -	case HSM_ST_POLL:
> -	case HSM_ST_LAST_POLL:
> -		timeout = ata_pio_poll(ap);
> -		break;
> +	/* FIXME: check if EH is active */

No, you don't need to.  Generic EH already does the right thing.  No
need to worry about EH in polling task.

-- 
tejun

Re: [PATCH/RFC 4/5] libata-dev: Convert ata_pio_task() to use the new ata_hsm_move()

[Albert Lee]

Tejun Heo wrote:
> On Thu, Mar 09, 2006 at 04:54:52PM +0800, Albert Lee wrote:
> 
>>--- 03_support_wq/drivers/scsi/libata-core.c	2006-03-09 15:14:00.000000000 +0800
>>+++ 04_pio_task/drivers/scsi/libata-core.c	2006-03-09 15:14:41.000000000 +0800
>>@@ -725,6 +725,8 @@ static unsigned int ata_pio_modes(const 
>> static inline void
>> ata_queue_pio_task(struct ata_port *ap)
>> {
>>+	ap->pio_task_timeout = jiffies + ATA_TMOUT_PIO;
>>+
>> 	if (!(ap->flags & ATA_FLAG_FLUSH_PIO_TASK))
>> 		queue_work(ata_wq, &ap->pio_task);
>> }
> 
> 
> I don't think we really need pio_task_timeout.  Generic EH now handles
> polling timeouts correctly and I can't see any advantage of
> pio_task_timeout.
> 

Ok, will remove pio_task_timeout and the HSM_ST_TMOUT state in the
follow-up patch if Jeff also agree.

> 
>>@@ -3804,44 +3806,55 @@ fsm_start:
>> static void ata_pio_task(void *_data)
>> {
>> 	struct ata_port *ap = _data;
>>-	unsigned long timeout;
>>-	int has_next;
>>+	struct ata_queued_cmd *qc;
>>+	u8 status;
>>+	int poll_next;
>> 
>> fsm_start:
>>-	timeout = 0;
>>-	has_next = 1;
>>+	WARN_ON(ap->hsm_task_state == HSM_ST_IDLE);
>> 
>>-	switch (ap->hsm_task_state) {
>>-	case HSM_ST_FIRST:
>>-		has_next = ata_pio_first_block(ap);
>>-		break;
>>+	qc = ata_qc_from_tag(ap, ap->active_tag);
>>+	WARN_ON(qc == NULL);
>> 
>>-	case HSM_ST:
>>-		ata_pio_block(ap);
>>-		break;
>>+	/*
>>+	 * This is purely heuristic.  This is a fast path.
>>+	 * Sometimes when we enter, BSY will be cleared in
>>+	 * a chk-status or two.  If not, the drive is probably seeking
>>+	 * or something.  Snooze for a couple msecs, then
>>+	 * chk-status again.  If still busy, timeout or queue delayed work.
>>+	 */
>>+	status = ata_busy_wait(ap, ATA_BUSY, 5);
>>+	if (status & ATA_BUSY) {
>>+		msleep(2);
>>+		status = ata_busy_wait(ap, ATA_BUSY, 10);
>>+		if (status & ATA_BUSY) {
>>+			if (time_before(jiffies, ap->pio_task_timeout)) {
>>+				ata_queue_delayed_pio_task(ap, ATA_SHORT_PAUSE);
>>+				return;
>>+			}
>> 
>>-	case HSM_ST_LAST:
>>-		has_next = ata_pio_complete(ap);
>>-		break;
>>+			/* timeout. Let EH handle it later. 
>>+			 * FIXME: should we remove ap->pio_task_timeout?
>>+			 */
>>+			printk(KERN_ERR "ata%u: polling time out\n", ap->id);
>>+			qc->err_mask |= AC_ERR_TIMEOUT;
>>+			ap->hsm_task_state = HSM_ST_TMOUT;
>>+			return; 
>>+		}
>>+	}
>> 
>>-	case HSM_ST_POLL:
>>-	case HSM_ST_LAST_POLL:
>>-		timeout = ata_pio_poll(ap);
>>-		break;
>>+	/* FIXME: check if EH is active */
> 
> 
> No, you don't need to.  Generic EH already does the right thing.  No
> need to worry about EH in polling task.
> 

Ok, will remove the comment in the follow-up patch.

--
Albert

[PATCH/RFC 5/5] libata-dev: Cleanup unused enums/functions

[Albert Lee]

Cleanup unused enums/functions.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
Remove the following unused functions:
- ata_pio_poll()
- ata_pio_complete()
- ata_pio_first_block()
- ata_pio_block()
- ata_pio_error()
and other enums.

--- 04_pio_task/include/linux/libata.h	2006-03-09 09:58:18.000000000 +0800
+++ 05_cleanup/include/linux/libata.h	2006-03-09 10:54:40.000000000 +0800
@@ -166,10 +166,6 @@ enum {
 	ATA_TMOUT_PIO		= 30 * HZ,
 	ATA_TMOUT_BOOT		= 30 * HZ,	/* heuristic */
 	ATA_TMOUT_BOOT_QUICK	= 7 * HZ,	/* heuristic */
-	ATA_TMOUT_DATAOUT	= 30 * HZ,
-	ATA_TMOUT_DATAOUT_QUICK	= 5 * HZ,
-	ATA_TMOUT_CDB		= 30 * HZ,
-	ATA_TMOUT_CDB_QUICK	= 5 * HZ,
 	ATA_TMOUT_INTERNAL	= 30 * HZ,
 	ATA_TMOUT_INTERNAL_QUICK = 5 * HZ,
 
@@ -209,11 +205,9 @@ enum {
 enum hsm_task_states {
 	HSM_ST_UNKNOWN,		/* state unknown */
 	HSM_ST_IDLE,		/* no command on going */
-	HSM_ST_POLL,		/* same as HSM_ST, waits longer */
 	HSM_ST_TMOUT,		/* timeout */
 	HSM_ST,			/* (waiting the device to) transfer data */
 	HSM_ST_LAST,		/* (waiting the device to) complete command */
-	HSM_ST_LAST_POLL,	/* same as HSM_ST_LAST, waits longer */
 	HSM_ST_ERR,		/* error */
 	HSM_ST_FIRST,		/* (waiting the device to)
 				   write CDB or first data block */
--- 04_pio_task/drivers/scsi/libata-core.c	2006-03-09 15:14:41.000000000 +0800
+++ 05_cleanup/drivers/scsi/libata-core.c	2006-03-09 15:15:17.000000000 +0800
@@ -70,7 +70,6 @@ static int fgb(u32 bitmap);
 static int ata_choose_xfer_mode(const struct ata_port *ap,
 				u8 *xfer_mode_out,
 				unsigned int *xfer_shift_out);
-static void ata_pio_error(struct ata_port *ap);
 
 static unsigned int ata_unique_id = 1;
 static struct workqueue_struct *ata_wq;
@@ -2957,114 +2956,6 @@ void ata_poll_qc_complete(struct ata_que
 }
 
 /**
- *	ata_pio_poll - poll using PIO, depending on current state
- *	@ap: the target ata_port
- *
- *	LOCKING:
- *	None.  (executing in kernel thread context)
- *
- *	RETURNS:
- *	timeout value to use
- */
-
-static unsigned long ata_pio_poll(struct ata_port *ap)
-{
-	struct ata_queued_cmd *qc;
-	u8 status;
-	unsigned int poll_state = HSM_ST_UNKNOWN;
-	unsigned int reg_state = HSM_ST_UNKNOWN;
-
-	qc = ata_qc_from_tag(ap, ap->active_tag);
-	WARN_ON(qc == NULL);
-
-	switch (ap->hsm_task_state) {
-	case HSM_ST:
-	case HSM_ST_POLL:
-		poll_state = HSM_ST_POLL;
-		reg_state = HSM_ST;
-		break;
-	case HSM_ST_LAST:
-	case HSM_ST_LAST_POLL:
-		poll_state = HSM_ST_LAST_POLL;
-		reg_state = HSM_ST_LAST;
-		break;
-	default:
-		BUG();
-		break;
-	}
-
-	status = ata_chk_status(ap);
-	if (status & ATA_BUSY) {
-		if (time_after(jiffies, ap->pio_task_timeout)) {
-			qc->err_mask |= AC_ERR_TIMEOUT;
-			ap->hsm_task_state = HSM_ST_TMOUT;
-			return 0;
-		}
-		ap->hsm_task_state = poll_state;
-		return ATA_SHORT_PAUSE;
-	}
-
-	ap->hsm_task_state = reg_state;
-	return 0;
-}
-
-/**
- *	ata_pio_complete - check if drive is busy or idle
- *	@ap: the target ata_port
- *
- *	LOCKING:
- *	None.  (executing in kernel thread context)
- *
- *	RETURNS:
- *	Zero if qc completed.
- *	Non-zero if has next.
- */
-
-static int ata_pio_complete (struct ata_port *ap)
-{
-	struct ata_queued_cmd *qc;
-	u8 drv_stat;
-
-	/*
-	 * This is purely heuristic.  This is a fast path.  Sometimes when
-	 * we enter, BSY will be cleared in a chk-status or two.  If not,
-	 * the drive is probably seeking or something.  Snooze for a couple
-	 * msecs, then chk-status again.  If still busy, fall back to
-	 * HSM_ST_LAST_POLL state.
-	 */
-	drv_stat = ata_busy_wait(ap, ATA_BUSY, 10);
-	if (drv_stat & ATA_BUSY) {
-		msleep(2);
-		drv_stat = ata_busy_wait(ap, ATA_BUSY, 10);
-		if (drv_stat & ATA_BUSY) {
-			ap->hsm_task_state = HSM_ST_LAST_POLL;
-			ap->pio_task_timeout = jiffies + ATA_TMOUT_PIO;
-			return 1;
-		}
-	}
-
-	qc = ata_qc_from_tag(ap, ap->active_tag);
-	WARN_ON(qc == NULL);
-
-	drv_stat = ata_wait_idle(ap);
-	if (!ata_ok(drv_stat)) {
-		qc->err_mask |= __ac_err_mask(drv_stat);
-		ap->hsm_task_state = HSM_ST_ERR;
-		return 1;
-	}
-
-	ap->hsm_task_state = HSM_ST_IDLE;
-
-	WARN_ON(qc->err_mask);
-	ata_poll_qc_complete(qc);
-
-	/* another command may start at this point */
-
-	return 0;
-}
-
-
-/**
  *	swap_buf_le16 - swap halves of 16-bit words in place
  *	@buf:  Buffer to swap
  *	@buf_words:  Number of 16-bit words in buffer.
@@ -3322,91 +3213,6 @@ static void atapi_send_cdb(struct ata_po
 }
 
 /**
- *	ata_pio_first_block - Write first data block to hardware
- *	@ap: Port to which ATA/ATAPI device is attached.
- *
- *	When device has indicated its readiness to accept
- *	the data, this function sends out the CDB or 
- *	the first data block by PIO.
- *	After this, 
- *	  - If polling, ata_pio_task() handles the rest.
- *	  - Otherwise, interrupt handler takes over.
- *
- *	LOCKING:
- *	Kernel thread context (may sleep)
- *
- *	RETURNS:
- *	Zero if irq handler takes over
- *	Non-zero if has next (polling).
- */
-
-static int ata_pio_first_block(struct ata_port *ap)
-{
-	struct ata_queued_cmd *qc;
-	u8 status;
-	unsigned long flags;
-	int has_next;
-
-	qc = ata_qc_from_tag(ap, ap->active_tag);
-	WARN_ON(qc == NULL);
-	WARN_ON((qc->flags & ATA_QCFLAG_ACTIVE) == 0);
-
-	/* if polling, we will stay in the work queue after sending the data.
-	 * otherwise, interrupt handler takes over after sending the data.
-	 */
-	has_next = (qc->tf.flags & ATA_TFLAG_POLLING);
-
-	/* sleep-wait for BSY to clear */
-	DPRINTK("busy wait\n");
-	if (ata_busy_sleep(ap, ATA_TMOUT_DATAOUT_QUICK, ATA_TMOUT_DATAOUT)) {
-		qc->err_mask |= AC_ERR_TIMEOUT;
-		ap->hsm_task_state = HSM_ST_TMOUT;
-		goto err_out;
-	}
-
-	/* make sure DRQ is set */
-	status = ata_chk_status(ap);
-	if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ) {
-		/* device status error */
-		qc->err_mask |= AC_ERR_HSM;
-		ap->hsm_task_state = HSM_ST_ERR;
-		goto err_out;
-	}
-
-	/* Send the CDB (atapi) or the first data block (ata pio out).
-	 * During the state transition, interrupt handler shouldn't
-	 * be invoked before the data transfer is complete and
-	 * hsm_task_state is changed. Hence, the following locking.
-	 */
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-
-	if (qc->tf.protocol == ATA_PROT_PIO) {
-		/* PIO data out protocol.
-		 * send first data block.
-		 */
-
-		/* ata_pio_sectors() might change the state to HSM_ST_LAST.
-		 * so, the state is changed here before ata_pio_sectors().
-		 */
-		ap->hsm_task_state = HSM_ST;
-		ata_pio_sectors(qc);
-		ata_altstatus(ap); /* flush */
-	} else
-		/* send CDB */
-		atapi_send_cdb(ap, qc);
-
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
-
-	/* if polling, ata_pio_task() handles the rest.
-	 * otherwise, interrupt handler takes over from here.
-	 */
-	return has_next;
-
-err_out:
-	return 1; /* has next */
-}
-
-/**
  *	__atapi_pio_bytes - Transfer data from/to the ATAPI device.
  *	@qc: Command on going
  *	@bytes: number of bytes
@@ -3546,91 +3352,6 @@ err_out:
 }
 
 /**
- *	ata_pio_block - start PIO on a block
- *	@ap: the target ata_port
- *
- *	LOCKING:
- *	None.  (executing in kernel thread context)
- */
-
-static void ata_pio_block(struct ata_port *ap)
-{
-	struct ata_queued_cmd *qc;
-	u8 status;
-
-	/*
-	 * This is purely heuristic.  This is a fast path.
-	 * Sometimes when we enter, BSY will be cleared in
-	 * a chk-status or two.  If not, the drive is probably seeking
-	 * or something.  Snooze for a couple msecs, then
-	 * chk-status again.  If still busy, fall back to
-	 * HSM_ST_POLL state.
-	 */
-	status = ata_busy_wait(ap, ATA_BUSY, 5);
-	if (status & ATA_BUSY) {
-		msleep(2);
-		status = ata_busy_wait(ap, ATA_BUSY, 10);
-		if (status & ATA_BUSY) {
-			ap->hsm_task_state = HSM_ST_POLL;
-			ap->pio_task_timeout = jiffies + ATA_TMOUT_PIO;
-			return;
-		}
-	}
-
-	qc = ata_qc_from_tag(ap, ap->active_tag);
-	WARN_ON(qc == NULL);
-
-	/* check error */
-	if (status & (ATA_ERR | ATA_DF)) {
-		qc->err_mask |= AC_ERR_DEV;
-		ap->hsm_task_state = HSM_ST_ERR;
-		return;
-	}
-
-	/* transfer data if any */
-	if (is_atapi_taskfile(&qc->tf)) {
-		/* DRQ=0 means no more data to transfer */
-		if ((status & ATA_DRQ) == 0) {
-			ap->hsm_task_state = HSM_ST_LAST;
-			return;
-		}
-
-		atapi_pio_bytes(qc);
-	} else {
-		/* handle BSY=0, DRQ=0 as error */
-		if ((status & ATA_DRQ) == 0) {
-			qc->err_mask |= AC_ERR_HSM;
-			ap->hsm_task_state = HSM_ST_ERR;
-			return;
-		}
-
-		ata_pio_sectors(qc);
-	}
-
-	ata_altstatus(ap); /* flush */
-}
-
-static void ata_pio_error(struct ata_port *ap)
-{
-	struct ata_queued_cmd *qc;
-
-	qc = ata_qc_from_tag(ap, ap->active_tag);
-	WARN_ON(qc == NULL);
-
-	if (qc->tf.command != ATA_CMD_PACKET)
-		printk(KERN_WARNING "ata%u: PIO error\n", ap->id);
-
-	/* make sure qc->err_mask is available to 
-	 * know what's wrong and recover
-	 */
-	WARN_ON(qc->err_mask == 0);
-
-	ap->hsm_task_state = HSM_ST_IDLE;
-
-	ata_poll_qc_complete(qc);
-}
-
-/**
  *	ata_hsm_move - move the HSM to the next state.
  *	@ap: the target ata_port
  *	@qc: qc on going

Re: [PATCH/RFC 0/5] libata-dev: integrate polling pio with irq-pio

[Tejun Heo]

On Thu, Mar 09, 2006 at 04:41:47PM +0800, Albert Lee wrote:
> This patch set integrates polling pio with irq-pio.
> 
> Changes:
> 1/5: Move out the HSM code from ata_host_intr() to the new ata_hsm_move() function.
> 2/5: Minor fix for ata_hsm_move() to work with ata_host_intr()
> 3/5: Let ata_hsm_move() work with both irq-pio and polling pio
> 4/5: Convert ata_pio_task() to use the new ata_hsm_move()
> 5/5: Cleanup unused enums/functions.
> 
> Patch against irq-pio branch (c2956a3b0d1c17b38da369811a6ce93eb7a01a04). 
> Minimally tested ok on x86. 
> 

Nice changes.  I hope this makes #irq-pio one step closer to
#upstream.  Thanks for the good work.  :-)

-- 
tejun

Re: [PATCH/RFC 0/5] libata-dev: integrate polling pio with irq-pio

[Jeff Garzik]

Albert Lee wrote:
> This patch set integrates polling pio with irq-pio.
> 
> Changes:
> 1/5: Move out the HSM code from ata_host_intr() to the new ata_hsm_move() function.
> 2/5: Minor fix for ata_hsm_move() to work with ata_host_intr()
> 3/5: Let ata_hsm_move() work with both irq-pio and polling pio
> 4/5: Convert ata_pio_task() to use the new ata_hsm_move()
> 5/5: Cleanup unused enums/functions.
> 
> Patch against irq-pio branch (c2956a3b0d1c17b38da369811a6ce93eb7a01a04). 
> Minimally tested ok on x86. 

What can I say?  :)

I agree with this patchset, once its revised with (a) Tejun's comments 
and (b) the newly merged-with-upstream irq-pio branch.

So I'll wait for your resend of any irq-pio patches.

	Jeff

[PATCH 0/5] libata-dev: integrate polling pio with irq-pio (respin)

[Albert Lee]

This patch set integrates polling pio with irq-pio.
(Revised per Tejun's advice to remove ap->pio_task_timeout, etc.)

Changes:
1/5: Move out the HSM code from ata_host_intr() to the new ata_hsm_move() function.
2/5: Minor fix for ata_hsm_move() to work with ata_host_intr()
3/5: Let ata_hsm_move() work with both irq-pio and polling pio
4/5: Convert ata_pio_task() to use the new ata_hsm_move()
5/5: Cleanup unused enums/functions.

Patch against irq-pio branch (54da9a3968448681d0ddf193ec90f2480c5cba1c) +
the irq-pio minor fixes (respin) patchset.

For your review, thanks,

Albert

[PATCH 1/5] libata-dev: Move out the HSM code from ata_host_intr()

[Albert Lee]

Move out the irq-pio HSM code from ata_host_intr() to the new ata_hsm_move() function verbatim.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

---
(trailing whitespaces removed.)

--- 00.5_removeatapi/drivers/scsi/libata-core.c	2006-03-13 11:38:06.000000000 +0800
+++ 01_move_out/drivers/scsi/libata-core.c	2006-03-13 11:41:07.000000000 +0800
@@ -3770,6 +3770,111 @@ static void ata_pio_error(struct ata_por
 	ata_poll_qc_complete(qc);
 }
 
+static void ata_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
+			 u8 status)
+{
+	/* check error */
+	if (unlikely(status & (ATA_ERR | ATA_DF))) {
+		qc->err_mask |= AC_ERR_DEV;
+		ap->hsm_task_state = HSM_ST_ERR;
+	}
+
+fsm_start:
+	switch (ap->hsm_task_state) {
+	case HSM_ST_FIRST:
+		/* Some pre-ATAPI-4 devices assert INTRQ
+		 * at this state when ready to receive CDB.
+		 */
+
+		/* check device status */
+		if (unlikely((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)) {
+			/* Wrong status. Let EH handle this */
+			qc->err_mask |= AC_ERR_HSM;
+			ap->hsm_task_state = HSM_ST_ERR;
+			goto fsm_start;
+		}
+
+		atapi_send_cdb(ap, qc);
+
+		break;
+
+	case HSM_ST:
+		/* complete command or read/write the data register */
+		if (qc->tf.protocol == ATA_PROT_ATAPI) {
+			/* ATAPI PIO protocol */
+			if ((status & ATA_DRQ) == 0) {
+				/* no more data to transfer */
+				ap->hsm_task_state = HSM_ST_LAST;
+				goto fsm_start;
+			}
+
+			atapi_pio_bytes(qc);
+
+			if (unlikely(ap->hsm_task_state == HSM_ST_ERR))
+				/* bad ireason reported by device */
+				goto fsm_start;
+
+		} else {
+			/* ATA PIO protocol */
+			if (unlikely((status & ATA_DRQ) == 0)) {
+				/* handle BSY=0, DRQ=0 as error */
+				qc->err_mask |= AC_ERR_HSM;
+				ap->hsm_task_state = HSM_ST_ERR;
+				goto fsm_start;
+			}
+
+			ata_pio_sectors(qc);
+
+			if (ap->hsm_task_state == HSM_ST_LAST &&
+			    (!(qc->tf.flags & ATA_TFLAG_WRITE))) {
+				/* all data read */
+				ata_altstatus(ap);
+				status = ata_chk_status(ap);
+				goto fsm_start;
+			}
+		}
+
+		ata_altstatus(ap); /* flush */
+		break;
+
+	case HSM_ST_LAST:
+		if (unlikely(status & ATA_DRQ)) {
+			/* handle DRQ=1 as error */
+			qc->err_mask |= AC_ERR_HSM;
+			ap->hsm_task_state = HSM_ST_ERR;
+			goto fsm_start;
+		}
+
+		/* no more data to transfer */
+		DPRINTK("ata%u: command complete, drv_stat 0x%x\n",
+			ap->id, status);
+
+		ap->hsm_task_state = HSM_ST_IDLE;
+
+		/* complete taskfile transaction */
+		qc->err_mask |= ac_err_mask(status);
+		ata_qc_complete(qc);
+		break;
+
+	case HSM_ST_ERR:
+		if (qc->tf.command != ATA_CMD_PACKET)
+			printk(KERN_ERR "ata%u: command error, drv_stat 0x%x host_stat 0x%x\n",
+			       ap->id, status, host_stat);
+
+		/* make sure qc->err_mask is available to
+		 * know what's wrong and recover
+		 */
+		WARN_ON(qc->err_mask == 0);
+
+		ap->hsm_task_state = HSM_ST_IDLE;
+		ata_qc_complete(qc);
+		break;
+	default:
+		goto idle_irq;
+	}
+
+}
+
 static void ata_pio_task(void *_data)
 {
 	struct ata_port *ap = _data;
@@ -4513,106 +4618,7 @@ inline unsigned int ata_host_intr (struc
 	/* ack bmdma irq events */
 	ap->ops->irq_clear(ap);
 
-	/* check error */
-	if (unlikely(status & (ATA_ERR | ATA_DF))) {
-		qc->err_mask |= AC_ERR_DEV;
-		ap->hsm_task_state = HSM_ST_ERR;
-	}
-
-fsm_start:
-	switch (ap->hsm_task_state) {
-	case HSM_ST_FIRST:
-		/* Some pre-ATAPI-4 devices assert INTRQ
-		 * at this state when ready to receive CDB.
-		 */
-
-		/* check device status */
-		if (unlikely((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)) {
-			/* Wrong status. Let EH handle this */
-			qc->err_mask |= AC_ERR_HSM;
-			ap->hsm_task_state = HSM_ST_ERR;
-			goto fsm_start;
-		}
-
-		atapi_send_cdb(ap, qc);
-
-		break;
-
-	case HSM_ST:
-		/* complete command or read/write the data register */
-		if (qc->tf.protocol == ATA_PROT_ATAPI) {
-			/* ATAPI PIO protocol */
-			if ((status & ATA_DRQ) == 0) {
-				/* no more data to transfer */
-				ap->hsm_task_state = HSM_ST_LAST;
-				goto fsm_start;
-			}
-
-			atapi_pio_bytes(qc);
-
-			if (unlikely(ap->hsm_task_state == HSM_ST_ERR))
-				/* bad ireason reported by device */
-				goto fsm_start;
-
-		} else {
-			/* ATA PIO protocol */
-			if (unlikely((status & ATA_DRQ) == 0)) {
-				/* handle BSY=0, DRQ=0 as error */
-				qc->err_mask |= AC_ERR_HSM;
-				ap->hsm_task_state = HSM_ST_ERR;
-				goto fsm_start;
-			}
-
-			ata_pio_sectors(qc);
-
-			if (ap->hsm_task_state == HSM_ST_LAST &&
-			    (!(qc->tf.flags & ATA_TFLAG_WRITE))) {
-				/* all data read */
-				ata_altstatus(ap);
-				status = ata_chk_status(ap);
-				goto fsm_start;
-			}
-		}
-
-		ata_altstatus(ap); /* flush */
-		break;
-
-	case HSM_ST_LAST:
-		if (unlikely(status & ATA_DRQ)) {
-			/* handle DRQ=1 as error */
-			qc->err_mask |= AC_ERR_HSM;
-			ap->hsm_task_state = HSM_ST_ERR;
-			goto fsm_start;
-		}
-
-		/* no more data to transfer */
-		DPRINTK("ata%u: command complete, drv_stat 0x%x\n",
-			ap->id, status);
-
-		ap->hsm_task_state = HSM_ST_IDLE;
-
-		/* complete taskfile transaction */
-		qc->err_mask |= ac_err_mask(status);
-		ata_qc_complete(qc);
-		break;
-
-	case HSM_ST_ERR:
-		if (qc->tf.command != ATA_CMD_PACKET)
-			printk(KERN_ERR "ata%u: command error, drv_stat 0x%x host_stat 0x%x\n",
-			       ap->id, status, host_stat);
-
-		/* make sure qc->err_mask is available to
-		 * know what's wrong and recover
-		 */
-		WARN_ON(qc->err_mask == 0);
-
-		ap->hsm_task_state = HSM_ST_IDLE;
-		ata_qc_complete(qc);
-		break;
-	default:
-		goto idle_irq;
-	}
-
+	ata_hsm_move(ap, qc, status);
 	return 1;	/* irq handled */
 
 idle_irq:

[PATCH 2/5] libata-dev: Minor fix for ata_hsm_move() to work with ata_host_intr()

[Albert Lee]

Minor fix for ata_hsm_move() to work with ata_host_intr().

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---

Changes:
- WARN_ON() and comment fix
- Make the HSM_ST_LAST device status checking more rigid. 
- Treat unknown HSM state as BUG().

--- 01_move_out/drivers/scsi/libata-core.c	2006-03-13 11:41:07.000000000 +0800
+++ 02_minor_fix/drivers/scsi/libata-core.c	2006-03-13 12:47:13.000000000 +0800
@@ -3773,6 +3773,8 @@ static void ata_pio_error(struct ata_por
 static void ata_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
 			 u8 status)
 {
+	WARN_ON((qc->flags & ATA_QCFLAG_ACTIVE) == 0);
+
 	/* check error */
 	if (unlikely(status & (ATA_ERR | ATA_DF))) {
 		qc->err_mask |= AC_ERR_DEV;
@@ -3782,10 +3784,6 @@ static void ata_hsm_move(struct ata_port
 fsm_start:
 	switch (ap->hsm_task_state) {
 	case HSM_ST_FIRST:
-		/* Some pre-ATAPI-4 devices assert INTRQ
-		 * at this state when ready to receive CDB.
-		 */
-
 		/* check device status */
 		if (unlikely((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)) {
 			/* Wrong status. Let EH handle this */
@@ -3838,9 +3836,8 @@ fsm_start:
 		break;
 
 	case HSM_ST_LAST:
-		if (unlikely(status & ATA_DRQ)) {
-			/* handle DRQ=1 as error */
-			qc->err_mask |= AC_ERR_HSM;
+		if (unlikely(!ata_ok(status))) {
+			qc->err_mask |= __ac_err_mask(status);
 			ap->hsm_task_state = HSM_ST_ERR;
 			goto fsm_start;
 		}
@@ -3849,17 +3846,18 @@ fsm_start:
 		DPRINTK("ata%u: command complete, drv_stat 0x%x\n",
 			ap->id, status);
 
+		WARN_ON(qc->err_mask);
+
 		ap->hsm_task_state = HSM_ST_IDLE;
 
 		/* complete taskfile transaction */
-		qc->err_mask |= ac_err_mask(status);
 		ata_qc_complete(qc);
 		break;
 
 	case HSM_ST_ERR:
 		if (qc->tf.command != ATA_CMD_PACKET)
-			printk(KERN_ERR "ata%u: command error, drv_stat 0x%x host_stat 0x%x\n",
-			       ap->id, status, host_stat);
+			printk(KERN_ERR "ata%u: command error, drv_stat 0x%x\n",
+			       ap->id, status);
 
 		/* make sure qc->err_mask is available to
 		 * know what's wrong and recover
@@ -3870,7 +3868,7 @@ fsm_start:
 		ata_qc_complete(qc);
 		break;
 	default:
-		goto idle_irq;
+		BUG();
 	}
 
 }
@@ -4568,6 +4566,10 @@ inline unsigned int ata_host_intr (struc
 	/* Check whether we are expecting interrupt in this state */
 	switch (ap->hsm_task_state) {
 	case HSM_ST_FIRST:
+		/* Some pre-ATAPI-4 devices assert INTRQ
+		 * at this state when ready to receive CDB.
+		 */
+
 		/* Check the ATA_DFLAG_CDB_INTR flag is enough here.
 		 * The flag was turned on only for atapi devices.
 		 * No need to check is_atapi_taskfile(&qc->tf) again.

[PATCH 3/5] libata-dev: Let ata_hsm_move() work with both irq-pio and polling pio

[Albert Lee]

Let ata_hsm_move() work with both irq-pio and polling pio codepath.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
Changes:
- add a new parameter "in_wq" for polling pio
- add return value "poll_next" to tell polling pio task whether the HSM is finished
- merge code from ata_pio_first_block() to the HSM_ST_FIRST state.
- call ata_poll_qc_complete() if called from the workqueue

--- 02_minor_fix/drivers/scsi/libata-core.c	2006-03-13 12:47:13.000000000 +0800
+++ 03_support_wq/drivers/scsi/libata-core.c	2006-03-13 13:28:05.000000000 +0800
@@ -3770,11 +3770,36 @@ static void ata_pio_error(struct ata_por
 	ata_poll_qc_complete(qc);
 }
 
-static void ata_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
-			 u8 status)
+/**
+ *	ata_hsm_move - move the HSM to the next state.
+ *	@ap: the target ata_port
+ *	@qc: qc on going
+ *	@status: current device status
+ *	@in_wq: 1 if called from workqueue, 0 otherwise
+ *
+ *	RETURNS:
+ *	1 when poll next status needed, 0 otherwise.
+ */
+
+static 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);
 
+	/* Make sure ata_qc_issue_prot() does not throw things
+	 * like DMA polling into the workqueue. Notice that
+	 * in_wq is not equivalent to (qc->tf.flags & ATA_TFLAG_POLLING).
+	 */
+	WARN_ON(in_wq != ((qc->tf.flags & ATA_TFLAG_POLLING) ||
+			  (ap->hsm_task_state == HSM_ST_FIRST &&
+			   ((qc->tf.protocol == ATA_PROT_PIO &&
+			     (qc->tf.flags & ATA_TFLAG_WRITE)) ||
+			    (is_atapi_taskfile(&qc->tf) &&
+			     !(qc->dev->flags & ATA_DFLAG_CDB_INTR))))));
+
 	/* check error */
 	if (unlikely(status & (ATA_ERR | ATA_DF))) {
 		qc->err_mask |= AC_ERR_DEV;
@@ -3784,6 +3809,14 @@ static void ata_hsm_move(struct ata_port
 fsm_start:
 	switch (ap->hsm_task_state) {
 	case HSM_ST_FIRST:
+		/* Send first data block or PACKET CDB */
+
+		/* If polling, we will stay in the work queue after
+		 * sending the data. Otherwise, interrupt handler
+		 * takes over after sending the data.
+		 */
+		poll_next = (qc->tf.flags & ATA_TFLAG_POLLING);
+
 		/* check device status */
 		if (unlikely((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)) {
 			/* Wrong status. Let EH handle this */
@@ -3792,8 +3825,36 @@ fsm_start:
 			goto fsm_start;
 		}
 
-		atapi_send_cdb(ap, qc);
+		/* Send the CDB (atapi) or the first data block (ata pio out).
+		 * During the state transition, interrupt handler shouldn't
+		 * 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->host_set->lock, flags);
 
+		if (qc->tf.protocol == ATA_PROT_PIO) {
+			/* PIO data out protocol.
+			 * send first data block.
+			 */
+
+			/* ata_pio_sectors() might change the state
+			 * to HSM_ST_LAST. so, the state is changed here
+			 * before ata_pio_sectors().
+			 */
+			ap->hsm_task_state = HSM_ST;
+			ata_pio_sectors(qc);
+			ata_altstatus(ap); /* flush */
+		} else
+			/* send CDB */
+			atapi_send_cdb(ap, qc);
+
+		if (in_wq)
+			spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+		/* if polling, ata_pio_task() handles the rest.
+		 * otherwise, interrupt handler takes over from here.
+		 */
 		break;
 
 	case HSM_ST:
@@ -3833,6 +3894,7 @@ fsm_start:
 		}
 
 		ata_altstatus(ap); /* flush */
+		poll_next = 1;
 		break;
 
 	case HSM_ST_LAST:
@@ -3851,7 +3913,12 @@ fsm_start:
 		ap->hsm_task_state = HSM_ST_IDLE;
 
 		/* complete taskfile transaction */
-		ata_qc_complete(qc);
+		if (in_wq)
+			ata_poll_qc_complete(qc);
+		else
+			ata_qc_complete(qc);
+
+		poll_next = 0;
 		break;
 
 	case HSM_ST_ERR:
@@ -3865,12 +3932,20 @@ fsm_start:
 		WARN_ON(qc->err_mask == 0);
 
 		ap->hsm_task_state = HSM_ST_IDLE;
-		ata_qc_complete(qc);
+
+		if (in_wq)
+			ata_poll_qc_complete(qc);
+		else
+			ata_qc_complete(qc);
+
+		poll_next = 0;
 		break;
 	default:
+		poll_next = 0;
 		BUG();
 	}
 
+	return poll_next;
 }
 
 static void ata_pio_task(void *_data)
@@ -4620,7 +4695,7 @@ inline unsigned int ata_host_intr (struc
 	/* ack bmdma irq events */
 	ap->ops->irq_clear(ap);
 
-	ata_hsm_move(ap, qc, status);
+	ata_hsm_move(ap, qc, status, 0);
 	return 1;	/* irq handled */
 
 idle_irq:

Re: [PATCH 3/5] libata-dev: Let ata_hsm_move() work with both irq-pio and polling pio

[Jeff Garzik]

Albert Lee wrote:
> +static 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);
>  
> +	/* Make sure ata_qc_issue_prot() does not throw things
> +	 * like DMA polling into the workqueue. Notice that
> +	 * in_wq is not equivalent to (qc->tf.flags & ATA_TFLAG_POLLING).
> +	 */
> +	WARN_ON(in_wq != ((qc->tf.flags & ATA_TFLAG_POLLING) ||
> +			  (ap->hsm_task_state == HSM_ST_FIRST &&
> +			   ((qc->tf.protocol == ATA_PROT_PIO &&
> +			     (qc->tf.flags & ATA_TFLAG_WRITE)) ||
> +			    (is_atapi_taskfile(&qc->tf) &&
> +			     !(qc->dev->flags & ATA_DFLAG_CDB_INTR))))));


applied patches 1-5 to the irq-pio branch.

Please provide a follow-up patch which makes the above test more 
readable.  For example:

static inline int things_ok_in_wq(input...)
{
	if (test 1)
		return 0;
	if (test 2)
		return 0;
	return 1;	
}

[PATCH 1/1] libata-dev: Make the the in_wq check as an inline function

[Albert Lee]

Make the the in_wq check easier to read as an inline function.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
irq-pio follow-up per Jeff's advice.

--- 06_check_atapi_dma/drivers/scsi/libata-core.c	2006-03-13 13:29:25.000000000 +0800
+++ 07_followup/drivers/scsi/libata-core.c	2006-03-13 16:25:52.000000000 +0800
@@ -3501,6 +3501,33 @@ err_out:
 }
 
 /**
+ *	ata_hsm_ok_in_wq - Check if the qc can be handled in the workqueue.
+ *	@ap: the target ata_port
+ *	@qc: qc on going
+ *
+ *	RETURNS:
+ *	1 if ok in workqueue, 0 otherwise.
+ */
+
+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;
+	}
+
+	return 0;
+}
+
+/**
  *	ata_hsm_move - move the HSM to the next state.
  *	@ap: the target ata_port
  *	@qc: qc on going
@@ -3523,12 +3550,7 @@ static int ata_hsm_move(struct ata_port 
 	 * like DMA polling into the workqueue. Notice that
 	 * in_wq is not equivalent to (qc->tf.flags & ATA_TFLAG_POLLING).
 	 */
-	WARN_ON(in_wq != ((qc->tf.flags & ATA_TFLAG_POLLING) ||
-			  (ap->hsm_task_state == HSM_ST_FIRST &&
-			   ((qc->tf.protocol == ATA_PROT_PIO &&
-			     (qc->tf.flags & ATA_TFLAG_WRITE)) ||
-			    (is_atapi_taskfile(&qc->tf) &&
-			     !(qc->dev->flags & ATA_DFLAG_CDB_INTR))))));
+	WARN_ON(in_wq != ata_hsm_ok_in_wq(ap, qc));
 
 	/* check error */
 	if (unlikely(status & (ATA_ERR | ATA_DF))) {

Re: [PATCH 1/1] libata-dev: Make the the in_wq check as an inline function

[Jeff Garzik]

Albert Lee wrote:
> Make the the in_wq check easier to read as an inline function.
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

applied

[PATCH 4/5] libata-dev: Convert ata_pio_task() to use the new ata_hsm_move()

[Albert Lee]

Convert ata_pio_task() to use the new ata_hsm_move().

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
Changes:

- refactor ata_pio_task() to poll device status register and
- call the new ata_hsm_move() when device indicates it is not BSY.

--- 03_support_wq/drivers/scsi/libata-core.c	2006-03-13 13:28:05.000000000 +0800
+++ 04_pio_task/drivers/scsi/libata-core.c	2006-03-13 13:28:42.000000000 +0800
@@ -3951,44 +3951,40 @@ fsm_start:
 static void ata_pio_task(void *_data)
 {
 	struct ata_port *ap = _data;
-	unsigned long timeout;
-	int has_next;
+	struct ata_queued_cmd *qc;
+	u8 status;
+	int poll_next;
 
 fsm_start:
-	timeout = 0;
-	has_next = 1;
-
-	switch (ap->hsm_task_state) {
-	case HSM_ST_FIRST:
-		has_next = ata_pio_first_block(ap);
-		break;
-
-	case HSM_ST:
-		ata_pio_block(ap);
-		break;
-
-	case HSM_ST_LAST:
-		has_next = ata_pio_complete(ap);
-		break;
+	WARN_ON(ap->hsm_task_state == HSM_ST_IDLE);
 
-	case HSM_ST_POLL:
-	case HSM_ST_LAST_POLL:
-		timeout = ata_pio_poll(ap);
-		break;
+	qc = ata_qc_from_tag(ap, ap->active_tag);
+	WARN_ON(qc == NULL);
 
-	case HSM_ST_TMOUT:
-	case HSM_ST_ERR:
-		ata_pio_error(ap);
-		return;
-
-	default:
-		BUG();
-		return;
+	/*
+	 * This is purely heuristic.  This is a fast path.
+	 * Sometimes when we enter, BSY will be cleared in
+	 * a chk-status or two.  If not, the drive is probably seeking
+	 * or something.  Snooze for a couple msecs, then
+	 * chk-status again.  If still busy, queue delayed work.
+	 */
+	status = ata_busy_wait(ap, ATA_BUSY, 5);
+	if (status & ATA_BUSY) {
+		msleep(2);
+		status = ata_busy_wait(ap, ATA_BUSY, 10);
+		if (status & ATA_BUSY) {
+			ata_port_queue_task(ap, ata_pio_task, ap, ATA_SHORT_PAUSE);
+			return;
+		}
 	}
 
-	if (timeout)
-		ata_port_queue_task(ap, ata_pio_task, ap, timeout);
-	else if (has_next)
+	/* move the HSM */
+	poll_next = ata_hsm_move(ap, qc, status, 1);
+
+	/* another command or interrupt handler
+	 * may be running at this point.
+	 */
+	if (poll_next)
 		goto fsm_start;
 }
 

[PATCH 5/5] libata-dev: Cleanup unused enums/functions

[Albert Lee]

Cleanup unused enums/functions.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
Remove the following unused functions:
- ata_pio_poll()
- ata_pio_complete()
- ata_pio_first_block()
- ata_pio_block()
- ata_pio_error()
ap->pio_task_timeout and other enums.

--- 04_pio_task/include/linux/libata.h	2006-03-13 10:02:55.000000000 +0800
+++ 05_cleanup/include/linux/libata.h	2006-03-13 11:39:30.000000000 +0800
@@ -163,13 +163,8 @@ enum {
 
 	/* various lengths of time */
 	ATA_TMOUT_EDD		= 5 * HZ,	/* heuristic */
-	ATA_TMOUT_PIO		= 30 * HZ,
 	ATA_TMOUT_BOOT		= 30 * HZ,	/* heuristic */
 	ATA_TMOUT_BOOT_QUICK	= 7 * HZ,	/* heuristic */
-	ATA_TMOUT_DATAOUT	= 30 * HZ,
-	ATA_TMOUT_DATAOUT_QUICK	= 5 * HZ,
-	ATA_TMOUT_CDB		= 30 * HZ,
-	ATA_TMOUT_CDB_QUICK	= 5 * HZ,
 	ATA_TMOUT_INTERNAL	= 30 * HZ,
 	ATA_TMOUT_INTERNAL_QUICK = 5 * HZ,
 
@@ -217,11 +212,8 @@ enum {
 enum hsm_task_states {
 	HSM_ST_UNKNOWN,		/* state unknown */
 	HSM_ST_IDLE,		/* no command on going */
-	HSM_ST_POLL,		/* same as HSM_ST, waits longer */
-	HSM_ST_TMOUT,		/* timeout */
 	HSM_ST,			/* (waiting the device to) transfer data */
 	HSM_ST_LAST,		/* (waiting the device to) complete command */
-	HSM_ST_LAST_POLL,	/* same as HSM_ST_LAST, waits longer */
 	HSM_ST_ERR,		/* error */
 	HSM_ST_FIRST,		/* (waiting the device to)
 				   write CDB or first data block */
@@ -404,7 +396,6 @@ struct ata_port {
 	struct work_struct	port_task;
 
 	unsigned int		hsm_task_state;
-	unsigned long		pio_task_timeout;
 
 	u32			msg_enable;
 	struct list_head	eh_done_q;
--- 04_pio_task/drivers/scsi/libata-core.c	2006-03-13 13:28:42.000000000 +0800
+++ 05_cleanup/drivers/scsi/libata-core.c	2006-03-13 13:29:02.000000000 +0800
@@ -65,7 +65,6 @@ static unsigned int ata_dev_init_params(
 					struct ata_device *dev);
 static void ata_set_mode(struct ata_port *ap);
 static void ata_dev_set_xfermode(struct ata_port *ap, struct ata_device *dev);
-static void ata_pio_error(struct ata_port *ap);
 static unsigned int ata_dev_xfermask(struct ata_port *ap,
 				     struct ata_device *dev);
 
@@ -3097,114 +3096,6 @@ void ata_poll_qc_complete(struct ata_que
 }
 
 /**
- *	ata_pio_poll - poll using PIO, depending on current state
- *	@ap: the target ata_port
- *
- *	LOCKING:
- *	None.  (executing in kernel thread context)
- *
- *	RETURNS:
- *	timeout value to use
- */
-
-static unsigned long ata_pio_poll(struct ata_port *ap)
-{
-	struct ata_queued_cmd *qc;
-	u8 status;
-	unsigned int poll_state = HSM_ST_UNKNOWN;
-	unsigned int reg_state = HSM_ST_UNKNOWN;
-
-	qc = ata_qc_from_tag(ap, ap->active_tag);
-	WARN_ON(qc == NULL);
-
-	switch (ap->hsm_task_state) {
-	case HSM_ST:
-	case HSM_ST_POLL:
-		poll_state = HSM_ST_POLL;
-		reg_state = HSM_ST;
-		break;
-	case HSM_ST_LAST:
-	case HSM_ST_LAST_POLL:
-		poll_state = HSM_ST_LAST_POLL;
-		reg_state = HSM_ST_LAST;
-		break;
-	default:
-		BUG();
-		break;
-	}
-
-	status = ata_chk_status(ap);
-	if (status & ATA_BUSY) {
-		if (time_after(jiffies, ap->pio_task_timeout)) {
-			qc->err_mask |= AC_ERR_TIMEOUT;
-			ap->hsm_task_state = HSM_ST_TMOUT;
-			return 0;
-		}
-		ap->hsm_task_state = poll_state;
-		return ATA_SHORT_PAUSE;
-	}
-
-	ap->hsm_task_state = reg_state;
-	return 0;
-}
-
-/**
- *	ata_pio_complete - check if drive is busy or idle
- *	@ap: the target ata_port
- *
- *	LOCKING:
- *	None.  (executing in kernel thread context)
- *
- *	RETURNS:
- *	Zero if qc completed.
- *	Non-zero if has next.
- */
-
-static int ata_pio_complete (struct ata_port *ap)
-{
-	struct ata_queued_cmd *qc;
-	u8 drv_stat;
-
-	/*
-	 * This is purely heuristic.  This is a fast path.  Sometimes when
-	 * we enter, BSY will be cleared in a chk-status or two.  If not,
-	 * the drive is probably seeking or something.  Snooze for a couple
-	 * msecs, then chk-status again.  If still busy, fall back to
-	 * HSM_ST_LAST_POLL state.
-	 */
-	drv_stat = ata_busy_wait(ap, ATA_BUSY, 10);
-	if (drv_stat & ATA_BUSY) {
-		msleep(2);
-		drv_stat = ata_busy_wait(ap, ATA_BUSY, 10);
-		if (drv_stat & ATA_BUSY) {
-			ap->hsm_task_state = HSM_ST_LAST_POLL;
-			ap->pio_task_timeout = jiffies + ATA_TMOUT_PIO;
-			return 1;
-		}
-	}
-
-	qc = ata_qc_from_tag(ap, ap->active_tag);
-	WARN_ON(qc == NULL);
-
-	drv_stat = ata_wait_idle(ap);
-	if (!ata_ok(drv_stat)) {
-		qc->err_mask |= __ac_err_mask(drv_stat);
-		ap->hsm_task_state = HSM_ST_ERR;
-		return 1;
-	}
-
-	ap->hsm_task_state = HSM_ST_IDLE;
-
-	WARN_ON(qc->err_mask);
-	ata_poll_qc_complete(qc);
-
-	/* another command may start at this point */
-
-	return 0;
-}
-
-
-/**
  *	swap_buf_le16 - swap halves of 16-bit words in place
  *	@buf:  Buffer to swap
  *	@buf_words:  Number of 16-bit words in buffer.
@@ -3462,91 +3353,6 @@ static void atapi_send_cdb(struct ata_po
 }
 
 /**
- *	ata_pio_first_block - Write first data block to hardware
- *	@ap: Port to which ATA/ATAPI device is attached.
- *
- *	When device has indicated its readiness to accept
- *	the data, this function sends out the CDB or 
- *	the first data block by PIO.
- *	After this, 
- *	  - If polling, ata_pio_task() handles the rest.
- *	  - Otherwise, interrupt handler takes over.
- *
- *	LOCKING:
- *	Kernel thread context (may sleep)
- *
- *	RETURNS:
- *	Zero if irq handler takes over
- *	Non-zero if has next (polling).
- */
-
-static int ata_pio_first_block(struct ata_port *ap)
-{
-	struct ata_queued_cmd *qc;
-	u8 status;
-	unsigned long flags;
-	int has_next;
-
-	qc = ata_qc_from_tag(ap, ap->active_tag);
-	WARN_ON(qc == NULL);
-	WARN_ON((qc->flags & ATA_QCFLAG_ACTIVE) == 0);
-
-	/* if polling, we will stay in the work queue after sending the data.
-	 * otherwise, interrupt handler takes over after sending the data.
-	 */
-	has_next = (qc->tf.flags & ATA_TFLAG_POLLING);
-
-	/* sleep-wait for BSY to clear */
-	DPRINTK("busy wait\n");
-	if (ata_busy_sleep(ap, ATA_TMOUT_DATAOUT_QUICK, ATA_TMOUT_DATAOUT)) {
-		qc->err_mask |= AC_ERR_TIMEOUT;
-		ap->hsm_task_state = HSM_ST_TMOUT;
-		goto err_out;
-	}
-
-	/* make sure DRQ is set */
-	status = ata_chk_status(ap);
-	if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ) {
-		/* device status error */
-		qc->err_mask |= AC_ERR_HSM;
-		ap->hsm_task_state = HSM_ST_ERR;
-		goto err_out;
-	}
-
-	/* Send the CDB (atapi) or the first data block (ata pio out).
-	 * During the state transition, interrupt handler shouldn't
-	 * be invoked before the data transfer is complete and
-	 * hsm_task_state is changed. Hence, the following locking.
-	 */
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-
-	if (qc->tf.protocol == ATA_PROT_PIO) {
-		/* PIO data out protocol.
-		 * send first data block.
-		 */
-
-		/* ata_pio_sectors() might change the state to HSM_ST_LAST.
-		 * so, the state is changed here before ata_pio_sectors().
-		 */
-		ap->hsm_task_state = HSM_ST;
-		ata_pio_sectors(qc);
-		ata_altstatus(ap); /* flush */
-	} else
-		/* send CDB */
-		atapi_send_cdb(ap, qc);
-
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
-
-	/* if polling, ata_pio_task() handles the rest.
-	 * otherwise, interrupt handler takes over from here.
-	 */
-	return has_next;
-
-err_out:
-	return 1; /* has next */
-}
-
-/**
  *	__atapi_pio_bytes - Transfer data from/to the ATAPI device.
  *	@qc: Command on going
  *	@bytes: number of bytes
@@ -3686,91 +3492,6 @@ err_out:
 }
 
 /**
- *	ata_pio_block - start PIO on a block
- *	@ap: the target ata_port
- *
- *	LOCKING:
- *	None.  (executing in kernel thread context)
- */
-
-static void ata_pio_block(struct ata_port *ap)
-{
-	struct ata_queued_cmd *qc;
-	u8 status;
-
-	/*
-	 * This is purely heuristic.  This is a fast path.
-	 * Sometimes when we enter, BSY will be cleared in
-	 * a chk-status or two.  If not, the drive is probably seeking
-	 * or something.  Snooze for a couple msecs, then
-	 * chk-status again.  If still busy, fall back to
-	 * HSM_ST_POLL state.
-	 */
-	status = ata_busy_wait(ap, ATA_BUSY, 5);
-	if (status & ATA_BUSY) {
-		msleep(2);
-		status = ata_busy_wait(ap, ATA_BUSY, 10);
-		if (status & ATA_BUSY) {
-			ap->hsm_task_state = HSM_ST_POLL;
-			ap->pio_task_timeout = jiffies + ATA_TMOUT_PIO;
-			return;
-		}
-	}
-
-	qc = ata_qc_from_tag(ap, ap->active_tag);
-	WARN_ON(qc == NULL);
-
-	/* check error */
-	if (status & (ATA_ERR | ATA_DF)) {
-		qc->err_mask |= AC_ERR_DEV;
-		ap->hsm_task_state = HSM_ST_ERR;
-		return;
-	}
-
-	/* transfer data if any */
-	if (is_atapi_taskfile(&qc->tf)) {
-		/* DRQ=0 means no more data to transfer */
-		if ((status & ATA_DRQ) == 0) {
-			ap->hsm_task_state = HSM_ST_LAST;
-			return;
-		}
-
-		atapi_pio_bytes(qc);
-	} else {
-		/* handle BSY=0, DRQ=0 as error */
-		if ((status & ATA_DRQ) == 0) {
-			qc->err_mask |= AC_ERR_HSM;
-			ap->hsm_task_state = HSM_ST_ERR;
-			return;
-		}
-
-		ata_pio_sectors(qc);
-	}
-
-	ata_altstatus(ap); /* flush */
-}
-
-static void ata_pio_error(struct ata_port *ap)
-{
-	struct ata_queued_cmd *qc;
-
-	qc = ata_qc_from_tag(ap, ap->active_tag);
-	WARN_ON(qc == NULL);
-
-	if (qc->tf.command != ATA_CMD_PACKET)
-		printk(KERN_WARNING "ata%u: PIO error\n", ap->id);
-
-	/* make sure qc->err_mask is available to 
-	 * know what's wrong and recover
-	 */
-	WARN_ON(qc->err_mask == 0);
-
-	ap->hsm_task_state = HSM_ST_IDLE;
-
-	ata_poll_qc_complete(qc);
-}
-
-/**
  *	ata_hsm_move - move the HSM to the next state.
  *	@ap: the target ata_port
  *	@qc: qc on going