Skip to contentSkip to conversation

libata-dev: handle ERR=1 DRQ=1

PATCH/RFC In review Albert Lee 3 participants 11 messages Mar 21, 2006 – Mar 29, 2006

Subsystems (2)
lore
Share & tools
lore

Albert Lee

author
#0 lore raw

handle ERR=1 DRQ=1 for PIO protocol

---

Some PIO read commands might set DRQ=1 alone with ERR=1. Under such situation, the data block should be transferred even if err=1.

This patch adds extra logic to HSM_ST_ERR to handle the required data transfer.

BTW, also found the following work in progress:
"Disable Data Transfer after Error Detection"
http://t13.org/docs2006/dt1825Lr3-DDT_TR.pdf
If this feature is turned on, then DRQ will be guaranteed 0 when ERR=1 which would simplify the PIO error handling.

Patch against the irq-pio branch.
Need your review and advice, thanks.

Albert

drivers/scsi/libata-core.c +40 -0
@@ -3684,6 +3684,46 @@ fsm_start:
 			printk(KERN_ERR "ata%u: command error, drv_stat 0x%x\n",
 			       ap->id, status);
 
+		/* handle ERR=1 DRQ=1 */
+		if ((status & ATA_ERR) && (status & ATA_DRQ)) {
+			/* ERR=1 DRQ=1 is only possible for reads (READ PIO
+			 * and READ MULTI, etc). For writes, DRQ=1 doesn't
+			 * make sense since the data block has been
+			 * transferred to the device.
+			 */
+			if ((qc->tf.protocol == ATA_PROT_PIO) &&
+			    !(qc->tf.flags & ATA_TFLAG_WRITE)) {
+				/* Transfer the corrupted data */
+				ata_pio_sectors(qc);
+
+				/* FIXME: contents of taskfile registers
+				 * undefined after the data xfer.
+				 * Maybe we should call ->tf_read()
+				 * and backup the taskfile somewhere?
+				 * Otherwise ata_gen_fixed_sense() won't
+				 * get needed information.
+				 */
+
+				/* FIXME: The spec said if "correctable"
+				 * data error, subsequent blocks will
+				 * be transferred. Isn't the command
+				 * stopped if ERR is set?
+				 */
+
+				ata_altstatus(ap); /* flush */
+				status = ata_chk_status(ap);
+
+				printk(KERN_ERR "ata%u: ERR=1 DRQ=1 error"
+				       " block transferred, drv_stat 0x%x\n",
+				       ap->id, status);
+			} else
+				/* Low possibility for other protocols
+				 * being ERR=1 DRQ=1. Maybe device error?
+				 */
+				printk(KERN_ERR "ata%u: ERR=1 DRQ=1\n",
+				       ap->id);
+		}
+
 		/* make sure qc->err_mask is available to
 		 * know what's wrong and recover
 		 */
#1 lore raw
Quoted 11 lines · Albert Lee

handle ERR=1 DRQ=1 for PIO protocol

---

Some PIO read commands might set DRQ=1 alone with ERR=1. Under such situation, the data block should be transferred even if err=1.

This patch adds extra logic to HSM_ST_ERR to handle the required data transfer.

Quoted 8 lines

BTW, also found the following work in progress:
"Disable Data Transfer after Error Detection"
http://t13.org/docs2006/dt1825Lr3-DDT_TR.pdf
If this feature is turned on, then DRQ will be guaranteed 0 when ERR=1 which would simplify the PIO error handling.

Patch against the irq-pio branch.
Need your review and advice, thanks.

If DRQ==1 then the error register and ATA_ERR should be considered irrelevant. I agree with Eric, that's more of a "don't touch it, other than to SRST it" situation.

Jeff

#2 lore raw
Quoted 5 lines · Jeff Garzik

If DRQ==1 then the error register and ATA_ERR should be considered irrelevant. I agree with Eric, that's more of a "don't touch it, other than to SRST it" situation.

Jeff

Well, some applications want to know the drive's best-guess at the data, as there are some cases where the resulting data resembles the original and might be useful even in its partial-block form.

Of course, many errors result in absolute garbage, so it's probably only useful for a very limited set of applications, that in most cases already know roughly what type of data is in the bad block.

--eric

author
#3 lore raw

PATCH 1/2 libata-dev: irq-pio minor fixes

irq-pio minor fixes for printk() and comments.

---

Patch against previous irq-pio snapshot.
The current irq-pio branch (949ec2c8e6b7b89179b85baf6309c009e1a1b951) looks strange. Maybe it is under refresh.

drivers/scsi/libata-core.c +5 -4
@@ -1283,7 +1283,7 @@ static int ata_dev_configure(struct ata_
 		if (dev->id[59] & 0x100) {
 			dev->multi_count = dev->id[59] & 0xff;
 			DPRINTK("ata%u: dev %u multi count %u\n",
-				ap->id, device, dev->multi_count);
+				ap->id, dev->devno, dev->multi_count);
 		}
 
 		dev->cdb_len = 16;
@@ -3562,6 +3562,9 @@ static int ata_hsm_move(struct ata_port 
 	}
 
 fsm_start:
+	DPRINTK("ata%u: protocol %d task_state %d (dev_stat 0x%X)\n",
+		ap->id, qc->tf.protocol, ap->hsm_task_state, status);
+
 	switch (ap->hsm_task_state) {
 	case HSM_ST_FIRST:
 		/* Send first data block or PACKET CDB */
@@ -3688,6 +3691,7 @@ fsm_start:
 
 		ap->hsm_task_state = HSM_ST_IDLE;
 
+		/* complete taskfile transaction */
 		if (in_wq)
 			ata_poll_qc_complete(qc);
 		else
@@ -4441,9 +4445,6 @@ inline unsigned int ata_host_intr (struc
 	if (unlikely(status & ATA_BUSY))
 		goto idle_irq;
 
-	DPRINTK("ata%u: protocol %d task_state %d (dev_stat 0x%X)\n",
-		ap->id, qc->tf.protocol, ap->hsm_task_state, status);
-
 	/* ack bmdma irq events */
 	ap->ops->irq_clear(ap);
 
#4 lore raw

Re: [PATCH 1/2] libata-dev: irq-pio minor fixes

Quoted 8 lines · Albert Lee

irq-pio minor fixes for printk() and comments.

---

Patch against previous irq-pio snapshot.
The current irq-pio branch (949ec2c8e6b7b89179b85baf6309c009e1a1b951) looks strange. Maybe it is under refresh.

It was rebased, which means you need to do a fresh pull. I apologize for not warning you and linux-ide. Something like

	git reset --hard 949ec2c8e6b7b89179b85baf6309c009e1a1b951

may work. Or simply

git checkout -f irq-pio

Or, as you say, it may be mirroring from master.kernel.org to the two public-facing kernel.org machines.

Jeff

author
#6 lore raw

PATCH 1/3 libata-dev: irq-pio minor fixes (respin)

irq-pio minor fixes for printk() and comments.

---

Patch against the irq-pio branch.

drivers/scsi/libata-core.c +5 -4
@@ -1295,7 +1295,7 @@ static int ata_dev_configure(struct ata_
 		if (dev->id[59] & 0x100) {
 			dev->multi_count = dev->id[59] & 0xff;
 			DPRINTK("ata%u: dev %u multi count %u\n",
-				ap->id, device, dev->multi_count);
+				ap->id, dev->devno, dev->multi_count);
 		}
 
 		dev->cdb_len = 16;
@@ -3594,6 +3594,9 @@ static int ata_hsm_move(struct ata_port 
 	}
 
 fsm_start:
+	DPRINTK("ata%u: protocol %d task_state %d (dev_stat 0x%X)\n",
+		ap->id, qc->tf.protocol, ap->hsm_task_state, status);
+
 	switch (ap->hsm_task_state) {
 	case HSM_ST_FIRST:
 		/* Send first data block or PACKET CDB */
@@ -3720,6 +3723,7 @@ fsm_start:
 
 		ap->hsm_task_state = HSM_ST_IDLE;
 
+		/* complete taskfile transaction */
 		if (in_wq)
 			ata_poll_qc_complete(qc);
 		else
@@ -4241,9 +4245,6 @@ inline unsigned int ata_host_intr (struc
 	if (unlikely(status & ATA_BUSY))
 		goto idle_irq;
 
-	DPRINTK("ata%u: protocol %d task_state %d (dev_stat 0x%X)\n",
-		ap->id, qc->tf.protocol, ap->hsm_task_state, status);
-
 	/* ack bmdma irq events */
 	ap->ops->irq_clear(ap);
 
#7 lore raw

Re: [PATCH 1/3] libata-dev: irq-pio minor fixes (respin)

Quoted 3 lines · Albert Lee

irq-pio minor fixes for printk() and comments.

applied 1-3, and patches 1-10 of previous series, to irq-pio branch

author
#8 lore raw

PATCH 2/3 libata-dev: fix the device err check sequence (respin)

In reply to Jeff Garzik

Current irq-pio checks ERR bit and stops on ERR before it does anything else. This behavior doesn't look right.
The DRQ bit should take higher precedence than the ERR bit.

Changes:
- Let the HSM do the data transfer whenever the device asks for DRQ bit, even if the ERR bit is set.
- For DRQ=1 ERR=1, don't trust the data

---

Revised per previous comments of Jeff and Eric.

The device error check in the entrance is removed.
Given the data returned on ERR=1 DRQ=1 might be corrupted or partial, the device err check is added after DRQ check and before the data transfer. AC_ERR_DEV is set to err_mask and tell the upper layer that the data does not look good.

drivers/scsi/libata-core.c +38 -6
@@ -3587,12 +3587,6 @@ static int ata_hsm_move(struct ata_port 
 	 */
 	WARN_ON(in_wq != ata_hsm_ok_in_wq(ap, qc));
 
-	/* check error */
-	if (unlikely(status & (ATA_ERR | ATA_DF))) {
-		qc->err_mask |= AC_ERR_DEV;
-		ap->hsm_task_state = HSM_ST_ERR;
-	}
-
 fsm_start:
 	DPRINTK("ata%u: protocol %d task_state %d (dev_stat 0x%X)\n",
 		ap->id, qc->tf.protocol, ap->hsm_task_state, status);
@@ -3615,6 +3609,17 @@ fsm_start:
 			goto 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.
+		 */
+		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;
+		}
+
 		/* 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
@@ -3657,6 +3662,17 @@ fsm_start:
 				goto 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.
+			 */
+			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;
+			}
+
 			atapi_pio_bytes(qc);
 
 			if (unlikely(ap->hsm_task_state == HSM_ST_ERR))
@@ -3672,6 +3688,22 @@ 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.
+			 */
+			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;
+			}
+
 			ata_pio_sectors(qc);
 
 			if (ap->hsm_task_state == HSM_ST_LAST &&
author
#9 lore raw

PATCH 3/3 libata-dev: wait idle after reading the last data block

In reply to Jeff Garzik

Some CD-ROM drives are slow to clear DRQ, after the last data block is read by PIO. Use ata_wait_idle() after reading the last data block.

---

drivers/scsi/libata-core.c +1 -1
@@ -3710,7 +3710,7 @@ fsm_start:
 			    (!(qc->tf.flags & ATA_TFLAG_WRITE))) {
 				/* all data read */
 				ata_altstatus(ap);
-				status = ata_chk_status(ap);
+				status = ata_wait_idle(ap);
 				goto fsm_start;
 			}
 		}
author
#10 lore raw

PATCH 2/2 libata-dev: fix the device err check sequence

In reply to Jeff Garzik

Current irq-pio checks ERR bit and stops on ERR before it does anything else. This behavior doesn't look right.
The DRQ bit should take higher precedence than the ERR bit.

Changes:
- Let the HSM do the data transfer whenever the device asks for DRQ bit, even if the ERR bit is set.
- For DRQ=1 ERR=1, don't trust the data

---

Revised per previous comments of Jeff and Eric.

The device error check in the entrance is removed.
Given the data returned on ERR=1 DRQ=1 might be corrupted or partial, the device err check is added after DRQ check and before the data transfer. We set AC_ERR_DEV to err_mask and tell the upper layer that the data does not look good.

For your review, thanks.

drivers/scsi/libata-core.c +38 -6
@@ -3555,12 +3555,6 @@ static int ata_hsm_move(struct ata_port 
 	 */
 	WARN_ON(in_wq != ata_hsm_ok_in_wq(ap, qc));
 
-	/* check error */
-	if (unlikely(status & (ATA_ERR | ATA_DF))) {
-		qc->err_mask |= AC_ERR_DEV;
-		ap->hsm_task_state = HSM_ST_ERR;
-	}
-
 fsm_start:
 	DPRINTK("ata%u: protocol %d task_state %d (dev_stat 0x%X)\n",
 		ap->id, qc->tf.protocol, ap->hsm_task_state, status);
@@ -3583,6 +3577,17 @@ fsm_start:
 			goto 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.
+		 */
+		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;
+		}
+
 		/* 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
@@ -3625,6 +3630,17 @@ fsm_start:
 				goto 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.
+			 */
+			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;
+			}
+
 			atapi_pio_bytes(qc);
 
 			if (unlikely(ap->hsm_task_state == HSM_ST_ERR))
@@ -3640,6 +3656,22 @@ 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.
+			 */
+			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;
+			}
+
 			ata_pio_sectors(qc);
 
 			if (ap->hsm_task_state == HSM_ST_LAST &&