ATA tape drive STT3401A needs DRQ HSM workaround too

PATCH In review Tejun Heo 5 participants 21 messages Oct 22, 2007 – Nov 15, 2007 libata

Share & tools

Tejun Heo

author

2007-10-22 09:24 UTC #0 lore raw

Hello, all.

There was a bug report involving ATA tape drive STT3401A and the drive also sets DRQ with device error and works correctly if the check is ignored. I saw patch implementing needed horkage. What's the plan here? Blacklist all tape drives or individual ones as they come up?

Thanks.

Signature · 1 line

tejun

Alan Cox

2007-10-22 09:29 UTC #1 lore raw

Quoted 6 lines · On Mon, 22 Oct 2007 18:24:20 +0900 Tejun Heo <[email protected]> wrote:

Hello, all.
There was a bug report involving ATA tape drive STT3401A and the drive also sets DRQ with device error and works correctly if the check is ignored. I saw patch implementing needed horkage. What's the plan here? Blacklist all tape drives or individual ones as they come up?

After discussion with Jeff the horkage patch is going back into the bitbucket and the state machine will be changed to consider the DRQ|ERR case simply a device error. I sent a patch for that to Andrew just before I left on holiday

Alan

Tejun Heo

author

2007-10-22 09:30 UTC #2 lore raw

Quoted 10 lines · Alan Cox

Quoted 6 lines · On Mon, 22 Oct 2007 18:24:20 +0900 Tejun Heo <[email protected]> wrote:
Hello, all.
There was a bug report involving ATA tape drive STT3401A and the drive also sets DRQ with device error and works correctly if the check is ignored. I saw patch implementing needed horkage. What's the plan here? Blacklist all tape drives or individual ones as they come up?

Quoted 4 lines

After discussion with Jeff the horkage patch is going back into the bitbucket and the state machine will be changed to consider the DRQ|ERR case simply a device error. I sent a patch for that to Andrew just before I left on holiday

Alright, thanks.

Signature · 1 line

tejun

Mark Lord

2007-10-22 14:18 UTC #3 lore raw

In reply to Tejun Heo

Quoted 6 lines · Tejun Heo

Hello, all.
There was a bug report involving ATA tape drive STT3401A and the drive also sets DRQ with device error and works correctly if the check is ignored. I saw patch implementing needed horkage. What's the plan here? Blacklist all tape drives or individual ones as they come up?

Speaking of which, I haven't yet tossed my ATAPI tape drive into the rubbish.

Anyone want it? Free, tapes included!

-ml

Jeff Garzik

2007-10-22 18:37 UTC #4 lore raw

In reply to Tejun Heo

Quoted 6 lines · Tejun Heo

Hello, all.
There was a bug report involving ATA tape drive STT3401A and the drive also sets DRQ with device error and works correctly if the check is ignored. I saw patch implementing needed horkage. What's the plan here? Blacklist all tape drives or individual ones as they come up?

I should dig out the SATA (yes, really) tape drive somebody sent me.

Jeff

Mark Lord

2007-10-22 22:36 UTC #5 lore raw

Man.. somebody needs to teach hald the difference between optical drives
and tape drives.. It seems to just sit in a tight loop issuing the same
failed commands over and over and over to the tape unit after boot.

I had to kill it off to gain control of Fedora so I could actually *do* anything on the system.

Weird.

Any, Albert Lee has claimed the drive.

Cheers

Albert Lee

2007-11-13 10:08 UTC #6 lore raw

Quoted 11 lines · Mark Lord

Man.. somebody needs to teach hald the difference between optical drives
and tape drives.. It seems to just sit in a tight loop issuing the same
failed commands over and over and over to the tape unit after boot.
I had to kill it off to gain control of Fedora so I could actually *do* anything on the system.
Weird.
Any, Albert Lee has claimed the drive.

Thanks for the drive.

After playing with the drive and checking the log, the DRQ+ERR problem only happens when the tape drive is doing REQUEST SENSE. Also the DRQ=1 ERR=1 situation only happens when the tape drive wants us to transfer the CDB, not the data.

It looks like the ERR=1 here is the residue of the previous command/error:
After an error and receiving the next PACKET command, the drive actually wants the host to transfer the CDB of the next command (i.e. request sense) by setting DRQ=1, but somehow it forgets to clear ERR to 0...

Maybe we can ignore the ERR bit if the command is REQUEST SENSE. Patch to follow.
--
albert

Albert Lee

2007-11-13 10:30 UTC #7 lore raw

PATCH 1/1 libata: workaround DRQ=1 ERR=1 for ATAPI tape drives

After an error condition, some ATAPI tape drives set DRQ=1 together with ERR=1 when asking the host to transfer the CDB of the next packet command (i.e. request sense). This patch workarounds the problem by ignoring the ERR bit and proceed REQUEST SENSE.

Signed-off-by Albert Lee <[email protected]>

Cc Alan Cox <[email protected]>

Cc Mark Lord <[email protected]>

---
Tested ok with the Seagate STT8000A tape drive. A similar workaround is seen in ide-tape.c:

	if (status.b.check && pc->c[0] == IDETAPE_REQUEST_SENSE_CMD)
		status.b.check = 0;

Although ide-tape only does it when DRQ=0...

drivers/ata/libata-core.c renamed from drivers/ata/libata-core.c.orig +12 -5

@@ -4930,11 +4930,18 @@ fsm_start:
 		 * let the EH abort the command or reset the device.
 		 */
 		if (unlikely(status & (ATA_ERR | ATA_DF))) {
-			ata_port_printk(ap, KERN_WARNING, "DRQ=1 with device "
-					"error, dev_stat 0x%X\n", status);
-			qc->err_mask |= AC_ERR_HSM;
-			ap->hsm_task_state = HSM_ST_ERR;
-			goto fsm_start;
+			/* Some ATAPI tape drives forget to clear the ERR bit
+			 * when doing the next command (i.e. request sense). We
+			 * ignore ERR here to workaround and proceed REQUEST_SENSE.
+			 */
+			if (qc->tf.protocol == ATA_PROT_PIO ||
+			    qc->cdb[0] != REQUEST_SENSE) {
+				ata_port_printk(ap, KERN_WARNING, "DRQ=1 with device "
+						"error, dev_stat 0x%X\n", status);
+				qc->err_mask |= AC_ERR_HSM;
+				ap->hsm_task_state = HSM_ST_ERR;
+				goto fsm_start;
+			}
 		}
 
 		/* Send the CDB (atapi) or the first data block (ata pio out).

Tejun Heo

author

2007-11-13 10:35 UTC #8 lore raw

Re: [PATCH] libata: workaround DRQ=1 ERR=1 for ATAPI tape drives

Quoted 7 lines · Albert Lee

After an error condition, some ATAPI tape drives set DRQ=1 together with ERR=1 when asking the host to transfer the CDB of the next packet command (i.e. request sense). This patch workarounds the problem by ignoring the ERR bit and proceed REQUEST SENSE.
Signed-off-by Albert Lee <[email protected]>
Cc Alan Cox <[email protected]>
Cc Mark Lord <[email protected]>

Looks okay to me. Or anyone thinks the condition should be more strict?

Signature · 1 line

tejun

Mark Lord

2007-11-13 13:49 UTC #9 lore raw

Re: [PATCH] libata: workaround DRQ=1 ERR=1 for ATAPI tape drives

In reply to Albert Lee

Quoted 7 lines · Albert Lee

After an error condition, some ATAPI tape drives set DRQ=1 together with ERR=1 when asking the host to transfer the CDB of the next packet command (i.e. request sense). This patch workarounds the problem by ignoring the ERR bit and proceed REQUEST SENSE.
Signed-off-by Albert Lee <[email protected]>
Cc Alan Cox <[email protected]>
Cc Mark Lord <[email protected]>

drivers/ata/libata-core.c fsm_start: · line 4944

Quoted 8 lines

drivers/ata/libata-core.c renamed from drivers/ata/libata-core.c.orig +8 -0

+			if (qc->tf.protocol == ATA_PROT_PIO ||
+			    qc->cdb[0] != REQUEST_SENSE) {
+				ata_port_printk(ap, KERN_WARNING, "DRQ=1 with device "
+						"error, dev_stat 0x%X\n", status);
+				qc->err_mask |= AC_ERR_HSM;
+				ap->hsm_task_state = HSM_ST_ERR;
+				goto fsm_start;
+			}

NAK. There's no guarantee that the next command after an error
will always be REQUEST_SENSE. Or is there?

I think we should just ignore the ERR bit when DRQ is set for PACKET command. Period. Possibly only for tape drives (since they all seem to do this), and maybe not for non-tape drives.

Cheers

Mark Lord

2007-11-13 14:16 UTC #10 lore raw

Re: [PATCH] libata: workaround DRQ=1 ERR=1 for ATAPI tape drives

Quoted 7 lines · Mark Lord

..
NAK. There's no guarantee that the next command after an error
will always be REQUEST_SENSE. Or is there?
I think we should just ignore the ERR bit when DRQ is set for PACKET command. Period. Possibly only for tape drives (since they all seem to do this), and maybe not for non-tape drives.

Note also that if we already *know* that this ia an ATAPI device, then we *know* that it will accept the PACKET opcode without error, so there is no reason to even look at the ERR bit until after the packet CDB has been transferred.

But I still think this should only apply to tapes for now.

Alan Cox

2007-11-13 15:17 UTC #11 lore raw

Re: [PATCH] libata: workaround DRQ=1 ERR=1 for ATAPI tape drives

In reply to Mark Lord

Quoted 2 lines

NAK. There's no guarantee that the next command after an error
will always be REQUEST_SENSE. Or is there?

For that device over ATAPI I believe the guarantee is there, the same as for SCSI.

Alan

Mark Lord

2007-11-13 17:24 UTC #12 lore raw

Re: [PATCH] libata: workaround DRQ=1 ERR=1 for ATAPI tape drives

Quoted 5 lines · Alan Cox

Quoted 2 lines
NAK. There's no guarantee that the next command after an error
will always be REQUEST_SENSE. Or is there?
For that device over ATAPI I believe the guarantee is there, the same as for SCSI.

Even for SG_IO w/o requesting sense data?

Alan Cox

2007-11-13 18:14 UTC #13 lore raw

Re: [PATCH] libata: workaround DRQ=1 ERR=1 for ATAPI tape drives

Quoted 9 lines · On Tue, 13 Nov 2007 12:24:45 -0500 Mark Lord <[email protected]> wrote:

Quoted 5 lines · Alan Cox
Quoted 2 lines
NAK. There's no guarantee that the next command after an error
will always be REQUEST_SENSE. Or is there?
For that device over ATAPI I believe the guarantee is there, the same as for SCSI.
..
Even for SG_IO w/o requesting sense data?

Thats a very good question. Yes we might need to ignore ERR at that point. Another possibility would be to notice if ERR seems to be stuck after we are finished so we know for next time.

Mark Lord

2007-11-13 18:22 UTC #14 lore raw

Re: [PATCH] libata: workaround DRQ=1 ERR=1 for ATAPI tape drives

Quoted 8 lines · Alan Cox

Quoted 5 lines · On Tue, 13 Nov 2007 12:24:45 -0500 Mark Lord <[email protected]> wrote:
Quoted 4 lines · Alan Cox
Quoted 2 lines
NAK. There's no guarantee that the next command after an error
will always be REQUEST_SENSE. Or is there?
For that device over ATAPI I believe the guarantee is there, the same as for SCSI.

Quoted 7 lines

Quoted 3 lines
..
Even for SG_IO w/o requesting sense data?
Thats a very good question. Yes we might need to ignore ERR at that point. Another possibility would be to notice if ERR seems to be stuck after we are finished so we know for next time.

ERR should stay set until the next command has been written to the device. So we could try and be clever to ignore ERR after PACKET, before CDB, only if it was set by previous command.

Sounds a bit complex.

Just (unconditionally for tape) ignoring it between PACKET and CDB might be simpler.

Cheers

Tejun Heo

author

2007-11-14 02:00 UTC #15 lore raw

Re: [PATCH] libata: workaround DRQ=1 ERR=1 for ATAPI tape drives

Quoted 8 lines · Mark Lord

ERR should stay set until the next command has been written to the device. So we could try and be clever to ignore ERR after PACKET, before CDB, only if it was set by previous command.
Sounds a bit complex.
Just (unconditionally for tape) ignoring it between PACKET and CDB might be simpler.

/me agrees. Albert, can you re-spin the patch? Thanks.

Signature · 1 line

tejun

Albert Lee

2007-11-14 06:13 UTC #16 lore raw

PATCH 1/2 libata: workaround DRQ=1 ERR=1 for ATAPI tape drives

After an error condition, some ATAPI tape drives set DRQ=1 together with ERR=1 when asking the host to transfer the CDB of the next packet command (i.e. request sense). This patch, a revised version of Alan/Mark's previous patch, adds ATA_HORKAGE_STUCK_ERR to workaround the problem by ignoring the ERR bit and proceed sending the CDB.

Signed-off-by Albert Lee <[email protected]>

Cc Alan Cox <[email protected]>

Cc Mark Lord <[email protected]>

---
Revised per Alan, Mark and Tejun's comments. Tested ok with the Seagate STT8000A tape drive. Patch against the libata-dev tree.

drivers/ata/libata-core.c +13 -5

diff -Nrup 00_libata-dev/drivers/ata/libata-core.c 01_ide_tape_stuck_err/drivers/ata/libata-core.c
@@ -5490,11 +5490,19 @@ fsm_start:
 		 * let the EH abort the command or reset the device.
 		 */
 		if (unlikely(status & (ATA_ERR | ATA_DF))) {
-			ata_port_printk(ap, KERN_WARNING, "DRQ=1 with device "
-					"error, dev_stat 0x%X\n", status);
-			qc->err_mask |= AC_ERR_HSM;
-			ap->hsm_task_state = HSM_ST_ERR;
-			goto fsm_start;
+			/* Some ATAPI tape drives forget to clear the ERR bit
+			 * when doing the next command (mostly request sense).
+			 * We ignore ERR here to workaround and proceed sending
+			 * the CDB.
+			 */
+			if (!(qc->dev->horkage & ATA_HORKAGE_STUCK_ERR)) {
+				ata_port_printk(ap, KERN_WARNING,
+						"DRQ=1 with device error, "
+						"dev_stat 0x%X\n", status);
+				qc->err_mask |= AC_ERR_HSM;
+				ap->hsm_task_state = HSM_ST_ERR;
+				goto fsm_start;
+			}
 		}
 
 		/* Send the CDB (atapi) or the first data block (ata pio out).

include/linux/libata.h +1 -0

diff -Nrup 00_libata-dev/include/linux/libata.h 01_ide_tape_stuck_err/include/linux/libata.h
@@ -340,6 +340,7 @@ enum {
 	ATA_HORKAGE_HPA_SIZE	= (1 << 6),	/* native size off by one */
 	ATA_HORKAGE_IPM		= (1 << 7),	/* Link PM problems */
 	ATA_HORKAGE_IVB		= (1 << 8),	/* cbl det validity bit bugs */
+	ATA_HORKAGE_STUCK_ERR	= (1 << 9),	/* stuck ERR on next PACKET */
 
 	 /* DMA mask for user DMA control: User visible values; DO NOT
 	    renumber */

Alan Cox

Acked-by on v1 1/2

2007-11-14 12:22 UTC #17 lore raw

Re: [PATCH 1/2] libata: workaround DRQ=1 ERR=1 for ATAPI tape drives

Quoted 9 lines · On Wed, 14 Nov 2007 14:13:05 +0800 Albert Lee <[email protected]> wrote:

After an error condition, some ATAPI tape drives set DRQ=1 together with ERR=1 when asking the host to transfer the CDB of the next packet command (i.e. request sense). This patch, a revised version of Alan/Mark's previous patch, adds ATA_HORKAGE_STUCK_ERR to workaround the problem by ignoring the ERR bit and proceed sending the CDB.
Signed-off-by Albert Lee <[email protected]>
Cc Alan Cox <[email protected]>
Cc Mark Lord <[email protected]>

Acked-by Alan Cox <[email protected]>

for both 1/2

Tejun Heo

author Applied

2007-11-15 00:32 UTC #18 lore raw

Re: [PATCH 1/2] libata: workaround DRQ=1 ERR=1 for ATAPI tape drives

In reply to Albert Lee

Quoted 9 lines · Albert Lee

After an error condition, some ATAPI tape drives set DRQ=1 together with ERR=1 when asking the host to transfer the CDB of the next packet command (i.e. request sense). This patch, a revised version of Alan/Mark's previous patch, adds ATA_HORKAGE_STUCK_ERR to workaround the problem by ignoring the ERR bit and proceed sending the CDB.
Signed-off-by Albert Lee <[email protected]>
Cc Alan Cox <[email protected]>
Cc Mark Lord <[email protected]>

applied to #tj-upstream-fixes.

Signature · 1 line

tejun

Albert Lee

2007-11-14 06:20 UTC #19 lore raw

PATCH 2/2 libata: use ATA_HORKAGE_STUCK_ERR for ATAPI tape drives

In reply to Tejun Heo

Per Mark's comments, maybe all ATAPI tape drives need ATA_HORKAGE_STUCK_ERR. This patch applys ATA_HORKAGE_STUCK_ERR for all ATAPI tape drives.

Signed-off-by Albert Lee <[email protected]>

Cc Mark Lord <[email protected]>

---

drivers/ata/libata-core.c +3 -1

diff -Nrup 01_ide_tape_stuck_err/drivers/ata/libata-core.c 02_black_ide_tape_drives/drivers/ata/libata-core.c
@@ -2307,8 +2307,10 @@ int ata_dev_configure(struct ata_device 
 	}
 
 	if ((dev->class == ATA_DEV_ATAPI) &&
-	    (atapi_command_packet_set(id) == TYPE_TAPE))
+	    (atapi_command_packet_set(id) == TYPE_TAPE)) {
 		dev->max_sectors = ATA_MAX_SECTORS_TAPE;
+		dev->horkage |= ATA_HORKAGE_STUCK_ERR;
+	}
 
 	if (dev->horkage & ATA_HORKAGE_MAX_SEC_128)
 		dev->max_sectors = min_t(unsigned int, ATA_MAX_SECTORS_128,

Tejun Heo

author Applied

2007-11-15 00:33 UTC #20 lore raw

Re: [PATCH 2/2] libata: use ATA_HORKAGE_STUCK_ERR for ATAPI tape drives

Quoted 5 lines · Albert Lee

Per Mark's comments, maybe all ATAPI tape drives need ATA_HORKAGE_STUCK_ERR. This patch applys ATA_HORKAGE_STUCK_ERR for all ATAPI tape drives.
Signed-off-by Albert Lee <[email protected]>
Cc Mark Lord <[email protected]>

applied to #tj-upstream-fixes.

Signature · 1 line

tejun