* [PATCH/RFC] libata-dev: make the returned err_mask more accurate
@ 2006-04-06 7:02 Albert Lee
2006-04-06 7:14 ` Tejun Heo
0 siblings, 1 reply; 4+ messages in thread
From: Albert Lee @ 2006-04-06 7:02 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Tejun Heo, IDE Linux, Doug Maxey
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)
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH/RFC] libata-dev: make the returned err_mask more accurate
2006-04-06 7:02 [PATCH/RFC] libata-dev: make the returned err_mask more accurate Albert Lee
@ 2006-04-06 7:14 ` Tejun Heo
2006-04-06 7:34 ` Albert Lee
2006-04-06 7:43 ` [PATCH/RFC] libata-dev: make the returned err_mask more accurate (revised) Albert Lee
0 siblings, 2 replies; 4+ messages in thread
From: Tejun Heo @ 2006-04-06 7:14 UTC (permalink / raw)
To: albertl; +Cc: Jeff Garzik, IDE Linux, Doug Maxey
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH/RFC] libata-dev: make the returned err_mask more accurate
2006-04-06 7:14 ` Tejun Heo
@ 2006-04-06 7:34 ` Albert Lee
2006-04-06 7:43 ` [PATCH/RFC] libata-dev: make the returned err_mask more accurate (revised) Albert Lee
1 sibling, 0 replies; 4+ messages in thread
From: Albert Lee @ 2006-04-06 7:34 UTC (permalink / raw)
To: Tejun Heo; +Cc: Jeff Garzik, IDE Linux, Doug Maxey
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH/RFC] libata-dev: make the returned err_mask more accurate (revised)
2006-04-06 7:14 ` Tejun Heo
2006-04-06 7:34 ` Albert Lee
@ 2006-04-06 7:43 ` Albert Lee
1 sibling, 0 replies; 4+ messages in thread
From: Albert Lee @ 2006-04-06 7:43 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Tejun Heo, IDE Linux, Doug Maxey
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;
}
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2006-04-06 7:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-04-06 7:02 [PATCH/RFC] libata-dev: make the returned err_mask more accurate Albert Lee
2006-04-06 7:14 ` Tejun Heo
2006-04-06 7:34 ` Albert Lee
2006-04-06 7:43 ` [PATCH/RFC] libata-dev: make the returned err_mask more accurate (revised) Albert Lee
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.