[PATCH 0/4] libata: Workaround/fixes for ATAPI devices

[PATCH 0/4] libata: Workaround/fixes for ATAPI devices

[Albert Lee]

patch 1/4: Reorder HSM_ST_FIRST
patch 2/4: Clear tf before doing request sense
patch 3/4: Limit max sector to 240 for TORiSAN DVD drives
patch 4/4: Limit ATAPI DMA to R/W commands only for TORiSAN DVD drives

[PATCH 1/4] libata: reorder HSM_ST_FIRST for easier decoding

[Albert Lee]

patch 1/4:
  Reorder HSM_ST_FIRST, such that the task state transition is easier decoded with human eyes.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
Patch against libata-dev tree, for your review, thanks.

diff -Nrup 00_libata-dev.ori/include/linux/libata.h 01_hsm_st/include/linux/libata.h
--- 00_libata-dev.ori/include/linux/libata.h	2007-03-23 16:56:24.000000000 +0800
+++ 01_hsm_st/include/linux/libata.h	2007-03-26 12:33:28.000000000 +0800
@@ -315,11 +315,11 @@ enum {
 
 enum hsm_task_states {
 	HSM_ST_IDLE,		/* no command on going */
+	HSM_ST_FIRST,		/* (waiting the device to)
+				   write CDB or first data block */
 	HSM_ST,			/* (waiting the device to) transfer data */
 	HSM_ST_LAST,		/* (waiting the device to) complete command */
 	HSM_ST_ERR,		/* error */
-	HSM_ST_FIRST,		/* (waiting the device to)
-				   write CDB or first data block */
 };
 
 enum ata_completion_errors {

[PATCH 2/4] libata: Clear tf before doing request sense

[Albert Lee]

patch 2/4:
  Clear tf before doing request sense.

This fixes the AOpen 56X/AKH timeout problem.
(http://bugzilla.kernel.org/show_bug.cgi?id=8244)

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---

Patch against libata-dev tree, for your review, thanks.

diff -Nrup 01_hsm_st/drivers/ata/libata-eh.c 02_aopen_rs/drivers/ata/libata-eh.c
--- 01_hsm_st/drivers/ata/libata-eh.c	2007-03-23 16:56:13.000000000 +0800
+++ 02_aopen_rs/drivers/ata/libata-eh.c	2007-03-31 01:11:01.000000000 +0800
@@ -991,18 +991,19 @@ static unsigned int atapi_eh_request_sen
 
 	DPRINTK("ATAPI request sense\n");
 
-	ata_tf_init(dev, &tf);
-
 	/* FIXME: is this needed? */
 	memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);
 
-	/* XXX: why tf_read here? */
+	/* read error register to initialize sense_buf */
 	ap->ops->tf_read(ap, &tf);
 
 	/* fill these in, for the case where they are -not- overwritten */
 	sense_buf[0] = 0x70;
 	sense_buf[2] = tf.feature >> 4;
 
+	/* some devices time out if garbage left in tf */ 
+	ata_tf_init(dev, &tf);
+
 	memset(cdb, 0, ATAPI_CDB_LEN);
 	cdb[0] = REQUEST_SENSE;
 	cdb[4] = SCSI_SENSE_BUFFERSIZE;

[PATCH 3/4] libata: Limit max sector to 240 for TORiSAN DVD drives

[Albert Lee]

patch 3/4:
  The TORiSAN drive locks up when max sector == 256.
  Limit max sector to 240 for TORiSAN DRD-N216 drives.
  (http://bugzilla.kernel.org/show_bug.cgi?id=6710)

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
Patch against libata-dev tree, for your review, thanks.

diff -Nrup 02_aopen_rs/drivers/ata/libata-core.c 03_max_sect/drivers/ata/libata-core.c
--- 02_aopen_rs/drivers/ata/libata-core.c	2007-03-23 16:56:13.000000000 +0800
+++ 03_max_sect/drivers/ata/libata-core.c	2007-03-31 14:11:33.000000000 +0800
@@ -1784,6 +1784,9 @@ int ata_dev_configure(struct ata_device 
 		dev->max_sectors = ATA_MAX_SECTORS;
 	}
 
+	if (ata_device_blacklisted(dev) & ATA_HORKAGE_MAX_SEC_240)
+		dev->max_sectors = min(ATA_MAX_SECTORS_240, dev->max_sectors);
+
 	if (ap->ops->dev_config)
 		ap->ops->dev_config(ap, dev);
 
@@ -3352,6 +3355,9 @@ static const struct ata_blacklist_entry 
 	{ "_NEC DV5800A", 	NULL,		ATA_HORKAGE_NODMA },
 	{ "SAMSUNG CD-ROM SN-124","N001",	ATA_HORKAGE_NODMA },
 
+	/* Weird ATAPI devices */
+	{ "TORiSAN DVD-ROM DRD-N216", NULL,	ATA_HORKAGE_MAX_SEC_240 },
+
 	/* Devices we expect to fail diagnostics */
 
 	/* Devices where NCQ should be avoided */
diff -Nrup 02_aopen_rs/include/linux/ata.h 03_max_sect/include/linux/ata.h
--- 02_aopen_rs/include/linux/ata.h	2007-03-23 16:56:24.000000000 +0800
+++ 03_max_sect/include/linux/ata.h	2007-03-31 10:06:53.000000000 +0800
@@ -40,6 +40,7 @@ enum {
 	ATA_MAX_DEVICES		= 2,	/* per bus/port */
 	ATA_MAX_PRD		= 256,	/* we could make these 256/256 */
 	ATA_SECT_SIZE		= 512,
+	ATA_MAX_SECTORS_240	= 240,
 	ATA_MAX_SECTORS		= 256,
 	ATA_MAX_SECTORS_LBA48	= 65535,/* TODO: 65536? */
 
diff -Nrup 02_aopen_rs/include/linux/libata.h 03_max_sect/include/linux/libata.h
--- 02_aopen_rs/include/linux/libata.h	2007-03-26 12:33:28.000000000 +0800
+++ 03_max_sect/include/linux/libata.h	2007-03-31 14:07:49.000000000 +0800
@@ -311,6 +311,7 @@ enum {
 	ATA_HORKAGE_DIAGNOSTIC	= (1 << 0),	/* Failed boot diag */
 	ATA_HORKAGE_NODMA	= (1 << 1),	/* DMA problems */
 	ATA_HORKAGE_NONCQ	= (1 << 2),	/* Don't use NCQ */
+	ATA_HORKAGE_MAX_SEC_240	= (1 << 3),	/* Limit max sects below 256 */
 };
 
 enum hsm_task_states {

Re: [PATCH 2/4] libata: Clear tf before doing request sense

[Tejun Heo]

Albert Lee wrote:
> diff -Nrup 01_hsm_st/drivers/ata/libata-eh.c 02_aopen_rs/drivers/ata/libata-eh.c
> --- 01_hsm_st/drivers/ata/libata-eh.c	2007-03-23 16:56:13.000000000 +0800
> +++ 02_aopen_rs/drivers/ata/libata-eh.c	2007-03-31 01:11:01.000000000 +0800
> @@ -991,18 +991,19 @@ static unsigned int atapi_eh_request_sen
>  
>  	DPRINTK("ATAPI request sense\n");
>  
> -	ata_tf_init(dev, &tf);
> -
>  	/* FIXME: is this needed? */
>  	memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);
>  
> -	/* XXX: why tf_read here? */
> +	/* read error register to initialize sense_buf */
>  	ap->ops->tf_read(ap, &tf);
>  
>  	/* fill these in, for the case where they are -not- overwritten */
>  	sense_buf[0] = 0x70;
>  	sense_buf[2] = tf.feature >> 4;

Oh, now I see why it's there.  Thanks for spotting this.  We don't need 
tf_read here, you can simply use the value in qc->result_tf.feature for 
this purpose.

-- 
tejun

[PATCH 4/4] libata: Limit ATAPI DMA to R/W commands only for TORiSAN DVD drives

[Albert Lee]

patch 4/4:

  Limit ATAPI DMA to R/W commands only for TORiSAN DRD-N216 DVD-ROM drives
  (http://bugzilla.kernel.org/show_bug.cgi?id=6710)

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
I guess maybe other commands like READ_DVD_STRUCTURE and READ_CD also
work under ATAPI DMA, but not confirmed by Vlad yet. So, allow ATAPI DMA to
only for Read/Write here for safty.

Patch against libata-dev tree, for your review, thanks.

diff -Nrup 03_max_sect/drivers/ata/libata-core.c 04_dma_rw_only/drivers/ata/libata-core.c
--- 03_max_sect/drivers/ata/libata-core.c	2007-03-31 14:11:33.000000000 +0800
+++ 04_dma_rw_only/drivers/ata/libata-core.c	2007-03-31 14:27:47.000000000 +0800
@@ -1787,6 +1787,10 @@ int ata_dev_configure(struct ata_device 
 	if (ata_device_blacklisted(dev) & ATA_HORKAGE_MAX_SEC_240)
 		dev->max_sectors = min(ATA_MAX_SECTORS_240, dev->max_sectors);
 
+	/* limit ATAPI DMA to R/W commands only */
+	if (ata_device_blacklisted(dev) & ATA_HORKAGE_DMA_RW_ONLY)
+		dev->horkage |= ATA_HORKAGE_DMA_RW_ONLY;
+
 	if (ap->ops->dev_config)
 		ap->ops->dev_config(ap, dev);
 
@@ -3356,7 +3360,8 @@ static const struct ata_blacklist_entry 
 	{ "SAMSUNG CD-ROM SN-124","N001",	ATA_HORKAGE_NODMA },
 
 	/* Weird ATAPI devices */
-	{ "TORiSAN DVD-ROM DRD-N216", NULL,	ATA_HORKAGE_MAX_SEC_240 },
+	{ "TORiSAN DVD-ROM DRD-N216", NULL,	ATA_HORKAGE_MAX_SEC_240 |
+						ATA_HORKAGE_DMA_RW_ONLY },
 
 	/* Devices we expect to fail diagnostics */
 
@@ -3676,6 +3681,26 @@ int ata_check_atapi_dma(struct ata_queue
 	struct ata_port *ap = qc->ap;
 	int rc = 0; /* Assume ATAPI DMA is OK by default */
 
+	/* some drives can only do ATAPI DMA on read/write */
+	if (unlikely(qc->dev->horkage & ATA_HORKAGE_DMA_RW_ONLY)) {
+		struct scsi_cmnd *cmd = qc->scsicmd;
+		u8 *scsicmd = cmd->cmnd;
+
+		switch (scsicmd[0]) {
+		case READ_10:
+		case WRITE_10:
+		case READ_12:
+		case WRITE_12:
+		case READ_6:
+		case WRITE_6:
+			/* atapi dma maybe ok */
+			break;
+		default:
+			/* turn off atapi dma */
+			return 1;
+		}
+	}
+
 	if (ap->ops->check_atapi_dma)
 		rc = ap->ops->check_atapi_dma(qc);
 
diff -Nrup 03_max_sect/include/linux/libata.h 04_dma_rw_only/include/linux/libata.h
--- 03_max_sect/include/linux/libata.h	2007-03-31 14:07:49.000000000 +0800
+++ 04_dma_rw_only/include/linux/libata.h	2007-03-31 14:08:31.000000000 +0800
@@ -312,6 +312,7 @@ enum {
 	ATA_HORKAGE_NODMA	= (1 << 1),	/* DMA problems */
 	ATA_HORKAGE_NONCQ	= (1 << 2),	/* Don't use NCQ */
 	ATA_HORKAGE_MAX_SEC_240	= (1 << 3),	/* Limit max sects below 256 */
+	ATA_HORKAGE_DMA_RW_ONLY	= (1 << 4),	/* ATAPI DMA for RW only */
 };
 
 enum hsm_task_states {

Re: [PATCH 1/4] libata: reorder HSM_ST_FIRST for easier decoding

[Tejun Heo]

Albert Lee wrote:
> patch 1/4:
>   Reorder HSM_ST_FIRST, such that the task state transition is easier decoded with human eyes.
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

Acked-by: Tejun Heo <htejun@gmail.com>

-- 
tejun

Re: [PATCH 3/4] libata: Limit max sector to 240 for TORiSAN DVD drives

[Tejun Heo]

Albert Lee wrote:
> patch 3/4:
>   The TORiSAN drive locks up when max sector == 256.
>   Limit max sector to 240 for TORiSAN DRD-N216 drives.
>   (http://bugzilla.kernel.org/show_bug.cgi?id=6710)
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

Acked-by: Tejun Heo <htejun@gmail.com>

-- 
tejun

Re: [PATCH 4/4] libata: Limit ATAPI DMA to R/W commands only for TORiSAN DVD drives

[Tejun Heo]

Albert Lee wrote:
> patch 4/4:
> 
>   Limit ATAPI DMA to R/W commands only for TORiSAN DRD-N216 DVD-ROM drives
>   (http://bugzilla.kernel.org/show_bug.cgi?id=6710)
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

Acked-by: Tejun Heo <htejun@gmail.com>

-- 
tejun

Re: [PATCH 2/4] libata: Clear tf before doing request sense

[Albert Lee]

Tejun Heo wrote:
> Albert Lee wrote:
> 
>> diff -Nrup 01_hsm_st/drivers/ata/libata-eh.c
>> 02_aopen_rs/drivers/ata/libata-eh.c
>> --- 01_hsm_st/drivers/ata/libata-eh.c    2007-03-23 16:56:13.000000000
>> +0800
>> +++ 02_aopen_rs/drivers/ata/libata-eh.c    2007-03-31
>> 01:11:01.000000000 +0800
>> @@ -991,18 +991,19 @@ static unsigned int atapi_eh_request_sen
>>  
>>      DPRINTK("ATAPI request sense\n");
>>  
>> -    ata_tf_init(dev, &tf);
>> -
>>      /* FIXME: is this needed? */
>>      memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);
>>  
>> -    /* XXX: why tf_read here? */
>> +    /* read error register to initialize sense_buf */
>>      ap->ops->tf_read(ap, &tf);
>>  
>>      /* fill these in, for the case where they are -not- overwritten */
>>      sense_buf[0] = 0x70;
>>      sense_buf[2] = tf.feature >> 4;
> 
> 
> Oh, now I see why it's there.  Thanks for spotting this.  We don't need
> tf_read here, you can simply use the value in qc->result_tf.feature for
> this purpose.
> 

Thanks for the advice. Will revise this patch.
--
albert

[PATCH 1/4] libata: reorder HSM_ST_FIRST for easier decoding

[Albert Lee]

patch 1/4:
  Reorder HSM_ST_FIRST, such that the task state transition is easier decoded with human eyes.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
Patch against libata-dev tree, for your review, thanks.

diff -Nrup 00_libata-dev.ori/include/linux/libata.h 01_hsm_st/include/linux/libata.h
--- 00_libata-dev.ori/include/linux/libata.h	2007-03-23 16:56:24.000000000 +0800
+++ 01_hsm_st/include/linux/libata.h	2007-03-26 12:33:28.000000000 +0800
@@ -315,11 +315,11 @@ enum {
 
 enum hsm_task_states {
 	HSM_ST_IDLE,		/* no command on going */
+	HSM_ST_FIRST,		/* (waiting the device to)
+				   write CDB or first data block */
 	HSM_ST,			/* (waiting the device to) transfer data */
 	HSM_ST_LAST,		/* (waiting the device to) complete command */
 	HSM_ST_ERR,		/* error */
-	HSM_ST_FIRST,		/* (waiting the device to)
-				   write CDB or first data block */
 };
 
 enum ata_completion_errors {