Skip to contentSkip to conversation

libata: use AC_ERR_TIMEOUT err_mask for time out

PATCH In review Albert Lee 3 participants 5 messages Mar 31, 2006 – Apr 1, 2006

Subsystems (2)
lore
Share & tools
lore

Albert Lee

author
#0 lore raw

PATCH 1/1 libata: use AC_ERR_TIMEOUT err_mask for time out

Use AC_ERR_TIMEOUT err_mask for time out,
instead of generating err_mask from drv_stat.

---
This patch was accepted in irq-pio before.

Patch against upstream (8b316a3973f05e572b4edeeda9072987f6bbaa44). For your review, thanks.

Albert

drivers/scsi/libata-core.c +1 -1
@@ -3827,7 +3827,7 @@ static void ata_qc_timeout(struct ata_qu
 		       ap->id, qc->tf.command, drv_stat, host_stat);
 
 		/* complete taskfile transaction */
-		qc->err_mask |= ac_err_mask(drv_stat);
+		qc->err_mask |= AC_ERR_TIMEOUT;
 		break;
 	}
 
#1 lore raw
Quoted 15 lines · Albert Lee

Use AC_ERR_TIMEOUT err_mask for time out,
instead of generating err_mask from drv_stat.

---
This patch was accepted in irq-pio before.

Patch against upstream (8b316a3973f05e572b4edeeda9072987f6bbaa44). For your review, thanks.

Albert

drivers/scsi/libata-core.c static void ata_qc_timeout(struct ata_qu · line 3830

Quoted 5 lines
drivers/scsi/libata-core.c +1 -1
@@ -3827,7 +3827,7 @@ static void ata_qc_timeout(struct ata_qu
 		       ap->id, qc->tf.command, drv_stat, host_stat);
 
 		/* complete taskfile transaction */
-		qc->err_mask |= ac_err_mask(drv_stat);
+		qc->err_mask |= AC_ERR_TIMEOUT;

The intention here is to catch missed interrupts; due to flaky hardware or buggy drivers, we may miss an interrupt, but still be able to complete the qc in the timeout handler. Thus, if the ac_err_mask() indicates a BSY or DRQ or DF is asserted, it will indicate an error, otherwise not.

So, NAK, and we should fix irq-pio for this as well...

Jeff

#2 lore raw
Quoted 20 lines · Jeff Garzik
Quoted 19 lines · Albert Lee

Use AC_ERR_TIMEOUT err_mask for time out,
instead of generating err_mask from drv_stat.

---
This patch was accepted in irq-pio before.

Patch against upstream (8b316a3973f05e572b4edeeda9072987f6bbaa44). For your review, thanks.

Albert

--- upstream0/drivers/scsi/libata-core.c 2006-03-31
13:33:15.000000000 +0800
+++ upstream1/drivers/scsi/libata-core.c 2006-03-31
13:34:59.000000000 +0800

drivers/scsi/libata-core.c +0 -0
@@ -3827,7 +3827,7 @@ static void ata_qc_timeout(struct ata_qu
                ap->id, qc->tf.command, drv_stat, host_stat);
 
Quoted 8 lines

drivers/scsi/libata-core.c static void ata_qc_timeout(struct ata_qu · line 3830

Quoted 3 lines
drivers/scsi/libata-core.c +1 -1
         /* complete taskfile transaction */
-        qc->err_mask |= ac_err_mask(drv_stat);
+        qc->err_mask |= AC_ERR_TIMEOUT;

The intention here is to catch missed interrupts; due to flaky hardware or buggy drivers, we may miss an interrupt, but still be able to complete the qc in the timeout handler. Thus, if the ac_err_mask() indicates a BSY or DRQ or DF is asserted, it will indicate an error, otherwise not.

So, NAK, and we should fix irq-pio for this as well...

Hello, Jeff. Hello, Albert.

I think we must not successfully finish a qc after timeout. If a device hasn't replied in 30 seconds, it's best to assume the device is in an unknown state. ATA TF registers just don't have enough information to tell whether it looks like that because it finished the command or it has simply forgotten about everything.

For example, if a communication error occurs while a qc is being issued and the controller fails to report, the device would have been in the ready state until the qc times out. It does know nothing about the command but ->eng_timeout will complete the command successfully potentially causing data corruption.

Or, if a power fluctation or static discharge breaks the link while a qc is in progress (this happens very easily with SATA, gigahertz serial signals are easy to disrupt), it can result in PHY reset causing the device to forget about the command. Similar result can be obtained with the right combination of device and controller by simply unplugging and replugging the cable while commands are in progress. (I've done a lot of it.)

It just isn't worth to successfully complete a command after 30 seconds at the risk of data corruption. If the device/controller times out on most of commands, the combination is unuseable no matter what we do on timeout (one command every 30secs..). If the combination fails occasionally, retrying doesn't take little time and is much safer.

Thanks.

Signature · 1 line
tejun
#3 lore raw

[--snip--]

Quoted 2 lines

It just isn't worth to successfully complete a command after 30 seconds at the risk of data corruption. If the device/controller times out on most of commands, the combination is unuseable no matter what we do on timeout (one command every 30secs..). If the combination fails occasionally, retrying doesn't take little time and is much safer.

The last sentence should be either 'doesn't take much time' or 'takes little time'. Two sentences got fused in my head while writing. :/

Signature · 1 line
tejun