On the subject of DRQ and EH...

On the subject of DRQ and EH...

[Jeff Garzik]

grep for try_to_flush_leftover_data in drivers/ide.

	Jeff

Re: On the subject of DRQ and EH...

[Albert Lee]

Jeff Garzik wrote:
> grep for try_to_flush_leftover_data in drivers/ide.
> 

Thanks for the pointer. Patch to follow.

--
Albert

[PATCH/RFC] libata-dev: handle DRQ=1 ERR=1 (revised)

[Albert Lee]

Handle DRQ=1 ERR=1 situation. Revised according to what IDE try_to_flush_leftover_data() does.

Changes:
 - For ATA PIO writes and ATAPI devices, just stop the HSM and let EH handle it.
 - For ATA PIO reads, read only one block of junk data and then let EH handle it.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
Thanks for the IDE try_to_flush_leftover_data() pointer.
The difference between IDE and irq-pio (before this patch):
 1. IDE only does the data transfer for PIO reads.
    For PIO writes and ATAPI, it just stops and abort/reset.
 2. For PIO reads, only one block of data is transferred. EH then takes over if DRQ is still set after that.
    The behavior of irq-pio before this patch is: let HSM go on, until the device set DRQ=0 or all data read.

Te IDE behavior looks safer. (When the device is in bad status, we really should not
write any data to it.)

Patch against irq-pio branch (08a556db919f67e1e4d33ae8d40f7222da34d994).
For your review, thanks.

--- irq-pio0/drivers/scsi/libata-core.c	2006-04-01 10:34:59.000000000 +0800
+++ irq-pio-drq/drivers/scsi/libata-core.c	2006-04-01 17:09:00.000000000 +0800
@@ -3651,13 +3651,16 @@ fsm_start:
 
 		/* Device should not ask for data transfer (DRQ=1)
 		 * when it finds something wrong.
-		 * Anyway, we respect DRQ here and let HSM go on
-		 * without changing hsm_task_state to HSM_ST_ERR.
+		 * We ignore DRQ here and stop the HSM by
+		 * changing hsm_task_state to HSM_ST_ERR and
+		 * let the EH abort the command or reset the device.
 		 */
 		if (unlikely(status & (ATA_ERR | ATA_DF))) {
 			printk(KERN_WARNING "ata%d: DRQ=1 with device error, dev_stat 0x%X\n",
 			       ap->id, status);
 			qc->err_mask |= AC_ERR_DEV;
+			ap->hsm_task_state = HSM_ST_ERR;
+			goto fsm_start;
 		}
 
 		/* Send the CDB (atapi) or the first data block (ata pio out).
@@ -3704,13 +3707,16 @@ fsm_start:
 
 			/* Device should not ask for data transfer (DRQ=1)
 			 * when it finds something wrong.
-			 * Anyway, we respect DRQ here and let HSM go on
-			 * without changing hsm_task_state to HSM_ST_ERR.
+			 * We ignore DRQ here and stop the HSM by
+			 * changing hsm_task_state to HSM_ST_ERR and
+			 * let the EH abort the command or reset the device.
 			 */
 			if (unlikely(status & (ATA_ERR | ATA_DF))) {
 				printk(KERN_WARNING "ata%d: DRQ=1 with device error, dev_stat 0x%X\n",
 				       ap->id, status);
 				qc->err_mask |= AC_ERR_DEV;
+				ap->hsm_task_state = HSM_ST_ERR;
+				goto fsm_start;
 			}
 
 			atapi_pio_bytes(qc);
@@ -3728,20 +3734,32 @@ fsm_start:
 				goto fsm_start;
 			}
 
-			/* Some devices may ask for data transfer (DRQ=1)
-			 * alone with ERR=1 for PIO reads.
-			 * We respect DRQ here and let HSM go on without
-			 * changing hsm_task_state to HSM_ST_ERR.
+			/* For PIO reads, some devices may ask for
+			 * data transfer (DRQ=1) alone with ERR=1.
+			 * We respect DRQ here and transfer one
+			 * block of junk data before changing the
+			 * hsm_task_state to HSM_ST_ERR.
+			 *
+			 * For PIO writes, ERR=1 DRQ=1 doesn't make
+			 * sense since the data block has been
+			 * transferred to the device.
 			 */
 			if (unlikely(status & (ATA_ERR | ATA_DF))) {
-				/* For writes, ERR=1 DRQ=1 doesn't make
-				 * sense since the data block has been
-				 * transferred to the device.
-				 */
-				WARN_ON(qc->tf.flags & ATA_TFLAG_WRITE);
-
 				/* data might be corrputed */
 				qc->err_mask |= AC_ERR_DEV;
+
+				if (!(qc->tf.flags & ATA_TFLAG_WRITE)) {
+					ata_pio_sectors(qc);
+					ata_altstatus(ap);
+					status = ata_wait_idle(ap);
+				}
+
+				/* ata_pio_sectors() might change the
+				 * state to HSM_ST_LAST. so, the state
+				 * is changed after ata_pio_sectors().
+				 */
+				ap->hsm_task_state = HSM_ST_ERR;
+				goto fsm_start;
 			}
 
 			ata_pio_sectors(qc);

Re: [PATCH/RFC] libata-dev: handle DRQ=1 ERR=1 (revised)

[Jeff Garzik]

Albert Lee wrote:
> Handle DRQ=1 ERR=1 situation. Revised according to what IDE try_to_flush_leftover_data() does.
> 
> Changes:
>  - For ATA PIO writes and ATAPI devices, just stop the HSM and let EH handle it.
>  - For ATA PIO reads, read only one block of junk data and then let EH handle it.
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> ---
> Thanks for the IDE try_to_flush_leftover_data() pointer.
> The difference between IDE and irq-pio (before this patch):
>  1. IDE only does the data transfer for PIO reads.
>     For PIO writes and ATAPI, it just stops and abort/reset.
>  2. For PIO reads, only one block of data is transferred. EH then takes over if DRQ is still set after that.
>     The behavior of irq-pio before this patch is: let HSM go on, until the device set DRQ=0 or all data read.
> 
> Te IDE behavior looks safer. (When the device is in bad status, we really should not
> write any data to it.)
> 
> Patch against irq-pio branch (08a556db919f67e1e4d33ae8d40f7222da34d994).
> For your review, thanks.

applied