[PATCH/RFC] libata-dev: make the returned err_mask more accurate

[PATCH/RFC] libata-dev: make the returned err_mask more accurate

[Albert Lee]

make the returned err_mask more accurate
Changes:
- add ac_err_drq() and ac_err_idle() to map err_mask from drv_status for data xfer and idle states.
- fix ata_hsm_move() to use the functions for err_mask mapping.

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

The current err_mask returned by HSM doesn't look right.
e.g. When we found DRQ=0 when data transfer is expected, AC_ERR_HSM is returned.
However, DRQ=0 maybe caused by device error. So, during data transfer,
if we see DRQ=0 and ERR=1, returning AC_ERR_DEV should be more accurate.

e.g. In HSM_ST_LAST state, AC_ERR_OTHER is returned for BSY=0 DRQ=1 currently.
AC_ERR_HSM shoule be more accurate.

Patch against irq-pio (79fa1b677be3a985cc66b9218a4dd09818f1051b).
For your review, thanks.

--- irq-pio0/drivers/scsi/libata-core.c	2006-04-06 10:08:44.000000000 +0800
+++ irq-pio1/drivers/scsi/libata-core.c	2006-04-06 11:20:15.000000000 +0800
@@ -3906,7 +3906,7 @@ fsm_start:
 		/* check device status */
 		if (unlikely((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)) {
 			/* Wrong status. Let EH handle this */
-			qc->err_mask |= AC_ERR_HSM;
+			qc->err_mask |= ac_err_drq(status);
 			ap->hsm_task_state = HSM_ST_ERR;
 			goto fsm_start;
 		}
@@ -3920,7 +3920,7 @@ fsm_start:
 		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;
+			qc->err_mask |= AC_ERR_HSM;
 			ap->hsm_task_state = HSM_ST_ERR;
 			goto fsm_start;
 		}
@@ -3976,7 +3976,7 @@ fsm_start:
 			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;
+				qc->err_mask |= AC_ERR_HSM;
 				ap->hsm_task_state = HSM_ST_ERR;
 				goto fsm_start;
 			}
@@ -3991,7 +3991,7 @@ fsm_start:
 			/* ATA PIO protocol */
 			if (unlikely((status & ATA_DRQ) == 0)) {
 				/* handle BSY=0, DRQ=0 as error */
-				qc->err_mask |= AC_ERR_HSM;
+				qc->err_mask |= ac_err_drq(status);
 				ap->hsm_task_state = HSM_ST_ERR;
 				goto fsm_start;
 			}
@@ -4007,13 +4007,16 @@ fsm_start:
 			 * transferred to the device.
 			 */
 			if (unlikely(status & (ATA_ERR | ATA_DF))) {
-				/* data might be corrputed */
-				qc->err_mask |= AC_ERR_DEV;
 
-				if (!(qc->tf.flags & ATA_TFLAG_WRITE)) {
+				if (qc->tf.flags & ATA_TFLAG_WRITE)
+					qc->err_mask |= AC_ERR_HSM;
+				else {
+					/* data might be corrputed */
+					qc->err_mask |= AC_ERR_DEV;
 					ata_pio_sectors(qc);
 					ata_altstatus(ap);
 					status = ata_wait_idle(ap);
+					qc->err_mask |= ac_err_idle(status);
 				}
 
 				/* ata_pio_sectors() might change the
@@ -4041,7 +4044,7 @@ fsm_start:
 
 	case HSM_ST_LAST:
 		if (unlikely(!ata_ok(status))) {
-			qc->err_mask |= __ac_err_mask(status);
+			qc->err_mask |= ac_err_idle(status);
 			ap->hsm_task_state = HSM_ST_ERR;
 			goto fsm_start;
 		}
--- irq-pio0/include/linux/libata.h	2006-04-06 10:08:48.000000000 +0800
+++ irq-pio1/include/linux/libata.h	2006-04-06 13:44:25.000000000 +0800
@@ -942,6 +942,46 @@ static inline int ata_try_flush_cache(co
 	       ata_id_has_flush_ext(dev->id);
 }
 
+static inline unsigned int ac_err_drq(u8 status)
+{
+	/* BSY = 1*/
+	if (status & ATA_BUSY)
+		return AC_ERR_HSM;
+
+	/* BSY = 0, DRQ = 1 */
+	if (status & ATA_DRQ)
+		return 0;
+
+	/* BSY = 0, DRQ = 0, device err */
+	if (status & (ATA_ERR | ATA_DF))
+		return AC_ERR_DEV;
+
+	/* BSY = 0, DRQ = 0, no device err
+	 * when we are expecting data transfer.
+	 */
+	return AC_ERR_HSM;
+}
+
+static inline unsigned int ac_err_idle(u8 status)
+{
+	/* BSY = 1 or DRQ = 1 */
+	if (status & (ATA_BUSY | ATA_DRQ))
+		return AC_ERR_HSM;
+
+	/* BSY = 0, DRQ = 0, device err */
+	if (status & (ATA_ERR | ATA_DF))
+		return AC_ERR_DEV;
+
+	/* BSY = 0, DRQ = 0, no device err. DRDY = 1 */
+	if (status & ATA_DRDY)
+		return 0;
+
+	/* thing looks good.
+	 * don't know why DRDY not set.
+	 */
+	return AC_ERR_OTHER
+}
+
 static inline unsigned int ac_err_mask(u8 status)
 {
 	if (status & ATA_BUSY)

Re: [PATCH/RFC] libata-dev: make the returned err_mask more accurate

[Tejun Heo]

Hello, Albert.

On Thu, Apr 06, 2006 at 03:02:36PM +0800, Albert Lee wrote:
[-- snip --]
> --- irq-pio0/include/linux/libata.h	2006-04-06 10:08:48.000000000 +0800
> +++ irq-pio1/include/linux/libata.h	2006-04-06 13:44:25.000000000 +0800
> @@ -942,6 +942,46 @@ static inline int ata_try_flush_cache(co
>  	       ata_id_has_flush_ext(dev->id);
>  }
>  
> +static inline unsigned int ac_err_drq(u8 status)
> +{
> +	/* BSY = 1*/
> +	if (status & ATA_BUSY)
> +		return AC_ERR_HSM;
> +
> +	/* BSY = 0, DRQ = 1 */
> +	if (status & ATA_DRQ)
> +		return 0;
> +
> +	/* BSY = 0, DRQ = 0, device err */
> +	if (status & (ATA_ERR | ATA_DF))
> +		return AC_ERR_DEV;
> +
> +	/* BSY = 0, DRQ = 0, no device err
> +	 * when we are expecting data transfer.
> +	 */
> +	return AC_ERR_HSM;
> +}
> +
> +static inline unsigned int ac_err_idle(u8 status)
> +{
> +	/* BSY = 1 or DRQ = 1 */
> +	if (status & (ATA_BUSY | ATA_DRQ))
> +		return AC_ERR_HSM;
> +
> +	/* BSY = 0, DRQ = 0, device err */
> +	if (status & (ATA_ERR | ATA_DF))
> +		return AC_ERR_DEV;
> +
> +	/* BSY = 0, DRQ = 0, no device err. DRDY = 1 */
> +	if (status & ATA_DRDY)
> +		return 0;
> +
> +	/* thing looks good.
> +	 * don't know why DRDY not set.
> +	 */
> +	return AC_ERR_OTHER
> +}
> +

I don't think putting these two functions into libata.h as inlines is
a good idea.  As currently there are no users, it would be better to
put them where they are used as static functions and export it later
as necessary.

-- 
tejun

Re: [PATCH/RFC] libata-dev: make the returned err_mask more accurate

[Albert Lee]

Tejun Heo wrote:
 > 
> I don't think putting these two functions into libata.h as inlines is
> a good idea.  As currently there are no users, it would be better to
> put them where they are used as static functions and export it later
> as necessary.
> 

You are right. Will revise it.

[PATCH/RFC] libata-dev: make the returned err_mask more accurate (revised)

[Albert Lee]

make the returned err_mask more accurate
Changes:
- add ac_err_drq() and ac_err_idle() to map err_mask from drv_status for data xfer and idle states.
- fix ata_hsm_move() to use the functions for err_mask mapping.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
(revised to not inline per Tejun's comments.)

The current err_mask returned by HSM doesn't look right.
e.g. When we found DRQ=0 when data transfer is expected, AC_ERR_HSM is returned.
However, DRQ=0 maybe caused by device error. So, during data transfer,
if we see DRQ=0 and ERR=1, returning AC_ERR_DEV should be more accurate.

e.g. In HSM_ST_LAST state, AC_ERR_OTHER is returned for BSY=0 DRQ=1 currently.
AC_ERR_HSM shoule be more accurate.

Patch against irq-pio (79fa1b677be3a985cc66b9218a4dd09818f1051b).
For your review, thanks.


--- irq-pio0/drivers/scsi/libata-core.c	2006-04-06 10:08:44.000000000 +0800
+++ irq-pio1/drivers/scsi/libata-core.c	2006-04-06 15:29:47.000000000 +0800
@@ -3838,6 +3838,62 @@ err_out:
 }
 
 /**
+ *	ac_err_drq - check status when PIO data transfer expected.
+ *	@status: device status
+ *
+ *	RETURNS:
+ *	0 if status looks good, err_mask otherwise.
+ */
+
+static unsigned int ac_err_drq(u8 status)
+{
+	/* BSY = 1*/
+	if (status & ATA_BUSY)
+		return AC_ERR_HSM;
+
+	/* BSY = 0, DRQ = 1 */
+	if (status & ATA_DRQ)
+		return 0;
+
+	/* BSY = 0, DRQ = 0, device err */
+	if (status & (ATA_ERR | ATA_DF))
+		return AC_ERR_DEV;
+
+	/* BSY = 0, DRQ = 0, no device err.
+	 * but we are expecting data transfer...
+	 */
+	return AC_ERR_HSM;
+}
+
+/**
+ *	ac_err_idle - check status when the transaction is finished.
+ *	@status: device status
+ *
+ *	RETURNS:
+ *	0 if status looks good, err_mask otherwise.
+ */
+
+static unsigned int ac_err_idle(u8 status)
+{
+	/* BSY = 1 or DRQ = 1 */
+	if (status & (ATA_BUSY | ATA_DRQ))
+		return AC_ERR_HSM;
+
+	/* BSY = 0, DRQ = 0, device err */
+	if (status & (ATA_ERR | ATA_DF))
+		return AC_ERR_DEV;
+
+	/* BSY = 0, DRQ = 0, no device err. DRDY = 1 */
+	if (status & ATA_DRDY)
+		return 0;
+
+	/* thing looks good.
+	 * don't know why DRDY not set.
+	 */
+	return AC_ERR_OTHER;
+}
+
+/**
  *	ata_hsm_ok_in_wq - Check if the qc can be handled in the workqueue.
  *	@ap: the target ata_port
  *	@qc: qc on going
@@ -3906,7 +3962,7 @@ fsm_start:
 		/* check device status */
 		if (unlikely((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)) {
 			/* Wrong status. Let EH handle this */
-			qc->err_mask |= AC_ERR_HSM;
+			qc->err_mask |= ac_err_drq(status);
 			ap->hsm_task_state = HSM_ST_ERR;
 			goto fsm_start;
 		}
@@ -3920,7 +3976,7 @@ fsm_start:
 		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;
+			qc->err_mask |= AC_ERR_HSM;
 			ap->hsm_task_state = HSM_ST_ERR;
 			goto fsm_start;
 		}
@@ -3976,7 +4032,7 @@ fsm_start:
 			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;
+				qc->err_mask |= AC_ERR_HSM;
 				ap->hsm_task_state = HSM_ST_ERR;
 				goto fsm_start;
 			}
@@ -3991,7 +4047,7 @@ fsm_start:
 			/* ATA PIO protocol */
 			if (unlikely((status & ATA_DRQ) == 0)) {
 				/* handle BSY=0, DRQ=0 as error */
-				qc->err_mask |= AC_ERR_HSM;
+				qc->err_mask |= ac_err_drq(status);
 				ap->hsm_task_state = HSM_ST_ERR;
 				goto fsm_start;
 			}
@@ -4007,13 +4063,16 @@ fsm_start:
 			 * transferred to the device.
 			 */
 			if (unlikely(status & (ATA_ERR | ATA_DF))) {
-				/* data might be corrputed */
-				qc->err_mask |= AC_ERR_DEV;
 
-				if (!(qc->tf.flags & ATA_TFLAG_WRITE)) {
+				if (qc->tf.flags & ATA_TFLAG_WRITE)
+					qc->err_mask |= AC_ERR_HSM;
+				else {
+					/* data might be corrputed */
+					qc->err_mask |= AC_ERR_DEV;
 					ata_pio_sectors(qc);
 					ata_altstatus(ap);
 					status = ata_wait_idle(ap);
+					qc->err_mask |= ac_err_idle(status);
 				}
 
 				/* ata_pio_sectors() might change the
@@ -4041,7 +4100,7 @@ fsm_start:
 
 	case HSM_ST_LAST:
 		if (unlikely(!ata_ok(status))) {
-			qc->err_mask |= __ac_err_mask(status);
+			qc->err_mask |= ac_err_idle(status);
 			ap->hsm_task_state = HSM_ST_ERR;
 			goto fsm_start;
 		}