ATA tape drive STT3401A needs DRQ HSM workaround too

ATA tape drive STT3401A needs DRQ HSM workaround too

[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?

Thanks.

-- 
tejun

Re: ATA tape drive STT3401A needs DRQ HSM workaround too

[Alan Cox]

On Mon, 22 Oct 2007 18:24:20 +0900
Tejun Heo <htejun@gmail.com> 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

Re: ATA tape drive STT3401A needs DRQ HSM workaround too

[Tejun Heo]

Alan Cox wrote:
> On Mon, 22 Oct 2007 18:24:20 +0900
> Tejun Heo <htejun@gmail.com> 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

Alright, thanks.

-- 
tejun

Re: ATA tape drive STT3401A needs DRQ HSM workaround too

[Mark Lord]

Tejun Heo 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?

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

Anyone want it?  Free, tapes included!

-ml

Re: ATA tape drive STT3401A needs DRQ HSM workaround too

[Jeff Garzik]

Tejun Heo 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?

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

	Jeff

Re: ATA tape drive STT3401A needs DRQ HSM workaround too

[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.

Cheers

Re: ATA tape drive STT3401A needs DRQ HSM workaround too

[Albert Lee]

Mark Lord wrote:
> 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

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

[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 <albertcc@tw.ibm.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Mark Lord <liml@rtr.ca>
---
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...

--- linux-2.6.23.1/drivers/ata/libata-core.c.orig	2007-10-13 00:43:44.000000000 +0800
+++ linux-2.6.23.1/drivers/ata/libata-core.c	2007-11-13 17:11:03.000000000 +0800
@@ -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).

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

[Tejun Heo]

Albert Lee 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 workarounds the problem by ignoring the ERR bit and proceed REQUEST SENSE.
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Cc: Mark Lord <liml@rtr.ca>

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

-- 
tejun

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

[Mark Lord]

Albert Lee 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 workarounds the problem by ignoring the ERR bit and proceed REQUEST SENSE.
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Cc: Mark Lord <liml@rtr.ca>
..
> +			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

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

[Mark Lord]

Mark Lord wrote:
> ..
> 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.

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

[Alan Cox]

> 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

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

[Mark Lord]

Alan Cox wrote:
>> 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?

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

[Alan Cox]

On Tue, 13 Nov 2007 12:24:45 -0500
Mark Lord <liml@rtr.ca> wrote:

> Alan Cox wrote:
> >> 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.

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

[Mark Lord]

Alan Cox wrote:
> On Tue, 13 Nov 2007 12:24:45 -0500
> Mark Lord <liml@rtr.ca> wrote:
> 
>> Alan Cox wrote:
>>>> 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.
..

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

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

[Tejun Heo]

Mark Lord wrote:
> 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.

-- 
tejun

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

[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 <albertcc@tw.ibm.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Mark Lord <liml@rtr.ca>
---
Revised per Alan, Mark and Tejun's comments. Tested ok with the Seagate STT8000A tape drive.
Patch against the libata-dev tree.

diff -Nrup 00_libata-dev/drivers/ata/libata-core.c 01_ide_tape_stuck_err/drivers/ata/libata-core.c
--- 00_libata-dev/drivers/ata/libata-core.c	2007-11-14 10:08:36.000000000 +0800
+++ 01_ide_tape_stuck_err/drivers/ata/libata-core.c	2007-11-14 11:20:31.000000000 +0800
@@ -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).
diff -Nrup 00_libata-dev/include/linux/libata.h 01_ide_tape_stuck_err/include/linux/libata.h
--- 00_libata-dev/include/linux/libata.h	2007-11-14 10:08:59.000000000 +0800
+++ 01_ide_tape_stuck_err/include/linux/libata.h	2007-11-14 11:19:32.000000000 +0800
@@ -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 */

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

[Alan Cox]

On Wed, 14 Nov 2007 14:13:05 +0800
Albert Lee <albertcc@tw.ibm.com> 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 <albertcc@tw.ibm.com>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Cc: Mark Lord <liml@rtr.ca>

Acked-by: Alan Cox <alan@redhat.com>

for both 1/2

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

[Tejun Heo]

Albert Lee 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 <albertcc@tw.ibm.com>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Cc: Mark Lord <liml@rtr.ca>

applied to #tj-upstream-fixes.

-- 
tejun

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

[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 <albertcc@tw.ibm.com>
Cc: Mark Lord <liml@rtr.ca>
---

diff -Nrup 01_ide_tape_stuck_err/drivers/ata/libata-core.c 02_black_ide_tape_drives/drivers/ata/libata-core.c
--- 01_ide_tape_stuck_err/drivers/ata/libata-core.c	2007-11-14 11:20:31.000000000 +0800
+++ 02_black_ide_tape_drives/drivers/ata/libata-core.c	2007-11-14 11:45:33.000000000 +0800
@@ -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,

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

[Tejun Heo]

Albert Lee wrote:
> 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 <albertcc@tw.ibm.com>
> Cc: Mark Lord <liml@rtr.ca>

applied to #tj-upstream-fixes.

-- 
tejun