* [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).