[PATCH 0/6] libata: ATA passthru fixes

[PATCH 0/6] libata: ATA passthru fixes

[Albert Lee]

1/6: update protocol numbers
2/6: support PIO multi commands
3/6: map UDMA protocols
4/6: always enforce correct DEV bit
5/6: support ATAPI devices
6/6: update cached device parameters

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

[PATCH 1/6] libata: update protocol numbers

[Albert Lee]

Patch 1/6:
 Update the ATA passthru protocol numbers according to the new spec.

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

diff -Nrup 00_libata-dev/drivers/ata/libata-scsi.c 01_protocol_update/drivers/ata/libata-scsi.c
--- 00_libata-dev/drivers/ata/libata-scsi.c	2007-06-01 12:08:21.000000000 +0800
+++ 01_protocol_update/drivers/ata/libata-scsi.c	2007-06-07 11:38:50.000000000 +0800
@@ -2512,16 +2512,15 @@ ata_scsi_map_proto(u8 byte1)
 		case 5:		/* PIO Data-out */
 			return ATA_PROT_PIO;
 
-		case 10:	/* Device Reset */
 		case 0:		/* Hard Reset */
 		case 1:		/* SRST */
-		case 2:		/* Bus Idle */
-		case 7:		/* Packet */
-		case 8:		/* DMA Queued */
-		case 9:		/* Device Diagnostic */
-		case 11:	/* UDMA Data-in */
-		case 12:	/* UDMA Data-Out */
-		case 13:	/* FPDMA */
+		case 8:		/* Device Diagnostic */
+		case 9:		/* Device Reset */
+		case 7:		/* DMA Queued */
+		case 10:	/* UDMA Data-in */
+		case 11:	/* UDMA Data-Out */
+		case 12:	/* FPDMA */
+		case 15:	/* Return Response Info */
 		default:	/* Reserved */
 			break;
 	}

[PATCH 2/6] libata: support PIO multi commands

[Albert Lee]

Patch 2/6:
  support the pass through of PIO multi commands.

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

diff -Nrup 01_protocol_update/drivers/ata/libata-scsi.c 02_pio_multi/drivers/ata/libata-scsi.c
--- 01_protocol_update/drivers/ata/libata-scsi.c	2007-06-07 11:38:50.000000000 +0800
+++ 02_pio_multi/drivers/ata/libata-scsi.c	2007-06-07 11:38:53.000000000 +0800
@@ -2551,10 +2551,6 @@ static unsigned int ata_scsi_pass_thru(s
 	if (tf->protocol == ATA_PROT_DMA && dev->dma_mode == 0)
 		goto invalid_fld;
 
-	if (cdb[1] & 0xe0)
-		/* PIO multi not supported yet */
-		goto invalid_fld;
-
 	/*
 	 * 12 and 16 byte CDBs use different offsets to
 	 * provide the various register values.
@@ -2606,6 +2602,22 @@ static unsigned int ata_scsi_pass_thru(s
 		tf->device = qc->dev->devno ?
 			tf->device | ATA_DEV1 : tf->device & ~ATA_DEV1;
 
+	/* sanity check for pio multi commands */
+	if ((cdb[1] & 0xe0) && !is_multi_taskfile(tf))
+		goto invalid_fld;
+
+	if (is_multi_taskfile(tf)) {
+		unsigned int multi_count = 1 << (cdb[1] >> 5);
+
+		/* compare the passed through multi_count
+		 * with the cached multi_count of libata
+		 */
+		if (multi_count != dev->multi_count)
+			ata_dev_printk(dev, KERN_WARNING,
+				       "invalid multi_count %u ignored\n",
+				       multi_count);
+	}	
+
 	/* READ/WRITE LONG use a non-standard sect_size */
 	qc->sect_size = ATA_SECT_SIZE;
 	switch (tf->command) {
diff -Nrup 01_protocol_update/include/linux/ata.h 02_pio_multi/include/linux/ata.h
--- 01_protocol_update/include/linux/ata.h	2007-06-01 12:08:32.000000000 +0800
+++ 02_pio_multi/include/linux/ata.h	2007-06-06 13:34:05.000000000 +0800
@@ -249,7 +249,7 @@ enum ata_tf_protocols {
 	/* ATA taskfile protocols */
 	ATA_PROT_UNKNOWN,	/* unknown/invalid */
 	ATA_PROT_NODATA,	/* no data */
-	ATA_PROT_PIO,		/* PIO single sector */
+	ATA_PROT_PIO,		/* PIO data xfer */
 	ATA_PROT_DMA,		/* DMA */
 	ATA_PROT_NCQ,		/* NCQ */
 	ATA_PROT_ATAPI,		/* packet command, PIO data xfer*/

[PATCH 3/6] libata: map UDMA protocols

[Albert Lee]

Patch 3/6:
 Map the ATA passthru UDMA protocols to ATA_PROT_DMA.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
Don't know why SAT distinguishs "UDMA Data In" and "UDMA Data Out" from "DMA".
These UDMA protocols only matter for on-the-wire-protocol and are mostly transparent
to the software. Anyway, map both of UDMA protocols to "ATA_PROT_DMA" here.

diff -Nrup 02_pio_multi/drivers/ata/libata-scsi.c 03_udma_supp/drivers/ata/libata-scsi.c
--- 02_pio_multi/drivers/ata/libata-scsi.c	2007-06-07 11:38:53.000000000 +0800
+++ 03_udma_supp/drivers/ata/libata-scsi.c	2007-06-07 11:41:30.000000000 +0800
@@ -2506,6 +2506,8 @@ ata_scsi_map_proto(u8 byte1)
 			return ATA_PROT_NODATA;
 
 		case 6:		/* DMA */
+		case 10:	/* UDMA Data-in */
+		case 11:	/* UDMA Data-Out */
 			return ATA_PROT_DMA;
 
 		case 4:		/* PIO Data-in */
@@ -2517,8 +2519,6 @@ ata_scsi_map_proto(u8 byte1)
 		case 8:		/* Device Diagnostic */
 		case 9:		/* Device Reset */
 		case 7:		/* DMA Queued */
-		case 10:	/* UDMA Data-in */
-		case 11:	/* UDMA Data-Out */
 		case 12:	/* FPDMA */
 		case 15:	/* Return Response Info */
 		default:	/* Reserved */

[PATCH 4/6] libata: always enforce correct DEV bit

[Albert Lee]

Patch 4/6:
 Always enforce correct DEV bit since we know which drive the command
is targeted. SAT demands to ignore the DEV bit, too.

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

diff -Nrup 03_udma_supp/drivers/ata/libata-scsi.c 04_dev_bit/drivers/ata/libata-scsi.c
--- 03_udma_supp/drivers/ata/libata-scsi.c	2007-06-07 11:41:30.000000000 +0800
+++ 04_dev_bit/drivers/ata/libata-scsi.c	2007-06-07 14:44:27.000000000 +0800
@@ -2595,12 +2595,10 @@ static unsigned int ata_scsi_pass_thru(s
 		tf->device = cdb[8];
 		tf->command = cdb[9];
 	}
-	/*
-	 * If slave is possible, enforce correct master/slave bit
-	*/
-	if (qc->ap->flags & ATA_FLAG_SLAVE_POSS)
-		tf->device = qc->dev->devno ?
-			tf->device | ATA_DEV1 : tf->device & ~ATA_DEV1;
+
+	/* enforce correct master/slave bit */
+	tf->device = dev->devno ?
+		tf->device | ATA_DEV1 : tf->device & ~ATA_DEV1;
 
 	/* sanity check for pio multi commands */
 	if ((cdb[1] & 0xe0) && !is_multi_taskfile(tf))

[PATCH 5/6] libata: support ATAPI devices

[Albert Lee]

Patch 5/6:
 Support ATA passthru to ATAPI devices.
Revised based on Mark's patch and Jeff's comments.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
Cc: Mark Lord <mlord@pobox.com> 
---
After reading Mark's patch, I guess we can safely by-pass the 
cdb_len check: the ATA_12/ATA_16 are for the libata SATL and
get unwrapped to ATA taskfiles before the devices ever see them.

diff -Nrup 04_dev_bit/drivers/ata/libata-scsi.c 05_supp_atapi/drivers/ata/libata-scsi.c
--- 04_dev_bit/drivers/ata/libata-scsi.c	2007-06-07 14:44:27.000000000 +0800
+++ 05_supp_atapi/drivers/ata/libata-scsi.c	2007-06-07 14:44:30.000000000 +0800
@@ -2701,10 +2701,6 @@ static inline ata_xlat_func_t ata_get_xl
 	case VERIFY_16:
 		return ata_scsi_verify_xlat;
 
-	case ATA_12:
-	case ATA_16:
-		return ata_scsi_pass_thru;
-
 	case START_STOP:
 		return ata_scsi_start_stop_xlat;
 	}
@@ -2740,8 +2736,14 @@ static inline int __ata_scsi_queuecmd(st
 				      void (*done)(struct scsi_cmnd *),
 				      struct ata_device *dev)
 {
+	u8 cmd = scmd->cmnd[0];
 	int rc = 0;
 
+	if (unlikely(cmd == ATA_12 || cmd == ATA_16)) {
+		rc = ata_scsi_translate(dev, scmd, done, ata_scsi_pass_thru);
+		return rc;
+	}
+
 	if (unlikely(!scmd->cmd_len || scmd->cmd_len > dev->cdb_len)) {
 		DPRINTK("bad CDB len=%u, max=%u\n",
 			scmd->cmd_len, dev->cdb_len);
@@ -2751,8 +2753,7 @@ static inline int __ata_scsi_queuecmd(st
 	}
 
 	if (dev->class == ATA_DEV_ATA) {
-		ata_xlat_func_t xlat_func = ata_get_xlat_func(dev,
-							      scmd->cmnd[0]);
+		ata_xlat_func_t xlat_func = ata_get_xlat_func(dev, cmd);
 
 		if (xlat_func)
 			rc = ata_scsi_translate(dev, scmd, done, xlat_func);

[PATCH 6/6] libata: update cached device paramters

[Albert Lee]

Patch 6/6:
INIT_DEV_PARAMS and SET_MULTI_MODE change the device parameters cached by libata.
Re-read IDENTIFY DEVICE info and update the cached device paramters when seeing these commands.

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

diff -Nrup 05_supp_atapi/drivers/ata/libata-scsi.c 06_sync_param/drivers/ata/libata-scsi.c
--- 05_supp_atapi/drivers/ata/libata-scsi.c	2007-06-07 14:44:30.000000000 +0800
+++ 06_sync_param/drivers/ata/libata-scsi.c	2007-06-07 14:44:33.000000000 +0800
@@ -1363,12 +1363,22 @@ static void ata_scsi_qc_complete(struct 
 	 * schedule EH_REVALIDATE operation to update the IDENTIFY DEVICE
 	 * cache
 	 */
-	if (ap->ops->error_handler &&
-	    !need_sense && (qc->tf.command == ATA_CMD_SET_FEATURES) &&
-	    ((qc->tf.feature == SETFEATURES_WC_ON) ||
-	     (qc->tf.feature == SETFEATURES_WC_OFF))) {
-		ap->eh_info.action |= ATA_EH_REVALIDATE;
-		ata_port_schedule_eh(ap);
+	if (ap->ops->error_handler && !need_sense) {
+		switch (qc->tf.command) {
+		case ATA_CMD_SET_FEATURES:
+			if ((qc->tf.feature == SETFEATURES_WC_ON) ||
+			    (qc->tf.feature == SETFEATURES_WC_OFF)) {
+				ap->eh_info.action |= ATA_EH_REVALIDATE;
+				ata_port_schedule_eh(ap);
+			}
+			break;
+
+		case ATA_CMD_INIT_DEV_PARAMS: /* CHS translation changed */
+		case ATA_CMD_SET_MULTI: /* multi_count changed */
+			ap->eh_info.action |= ATA_EH_REVALIDATE;
+			ata_port_schedule_eh(ap);
+			break;
+		}
 	}
 
 	/* For ATA pass thru (SAT) commands, generate a sense block if
diff -Nrup 05_supp_atapi/include/linux/ata.h 06_sync_param/include/linux/ata.h
--- 05_supp_atapi/include/linux/ata.h	2007-06-06 13:34:05.000000000 +0800
+++ 06_sync_param/include/linux/ata.h	2007-06-07 10:50:26.000000000 +0800
@@ -151,6 +151,7 @@ enum {
 	ATA_CMD_WRITE_MULTI_EXT	= 0x39,
 	ATA_CMD_WRITE_MULTI_FUA_EXT = 0xCE,
 	ATA_CMD_SET_FEATURES	= 0xEF,
+	ATA_CMD_SET_MULTI	= 0xC6,
 	ATA_CMD_PACKET		= 0xA0,
 	ATA_CMD_VERIFY		= 0x40,
 	ATA_CMD_VERIFY_EXT	= 0x42,

Re: [PATCH 1/6] libata: update protocol numbers

[Jeff Garzik]

Albert Lee wrote:
> Patch 1/6:
>  Update the ATA passthru protocol numbers according to the new spec.
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> ---
> 
> diff -Nrup 00_libata-dev/drivers/ata/libata-scsi.c 01_protocol_update/drivers/ata/libata-scsi.c
> --- 00_libata-dev/drivers/ata/libata-scsi.c	2007-06-01 12:08:21.000000000 +0800
> +++ 01_protocol_update/drivers/ata/libata-scsi.c	2007-06-07 11:38:50.000000000 +0800
> @@ -2512,16 +2512,15 @@ ata_scsi_map_proto(u8 byte1)
>  		case 5:		/* PIO Data-out */
>  			return ATA_PROT_PIO;
>  
> -		case 10:	/* Device Reset */
>  		case 0:		/* Hard Reset */
>  		case 1:		/* SRST */
> -		case 2:		/* Bus Idle */
> -		case 7:		/* Packet */
> -		case 8:		/* DMA Queued */
> -		case 9:		/* Device Diagnostic */
> -		case 11:	/* UDMA Data-in */
> -		case 12:	/* UDMA Data-Out */
> -		case 13:	/* FPDMA */
> +		case 8:		/* Device Diagnostic */
> +		case 9:		/* Device Reset */
> +		case 7:		/* DMA Queued */
> +		case 10:	/* UDMA Data-in */
> +		case 11:	/* UDMA Data-Out */
> +		case 12:	/* FPDMA */
> +		case 15:	/* Return Response Info */
>  		default:	/* Reserved */

Ugh.  Those idiots.  YOUR patch is fine.  But this is clearly an 
incompatible change in the spec.  If we had actually deployed code based 
the spec, we would be up shit creek :/

	Jeff

Re: [PATCH 1/6] libata: update protocol numbers

[Jeff Garzik]

Albert Lee wrote:
> Patch 1/6:
>  Update the ATA passthru protocol numbers according to the new spec.
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

applied 1-4, 6 to #upstream-fixes

Re: [PATCH 1/6] libata: update protocol numbers

[Mark Lord]

Jeff Garzik wrote:
> Albert Lee wrote:
>> Patch 1/6:
>>  Update the ATA passthru protocol numbers according to the new spec.
>>
>> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> 
> applied 1-4, 6 to #upstream-fixes

What about this one:  [PATCH 5/6] libata: support ATAPI devices  ??

I'm quite happy to let Albert run with that,
as he's busily maintaining the rest of the passthrough stuff.

Cheers

Re: [PATCH 1/6] libata: update protocol numbers

[Jeff Garzik]

Mark Lord wrote:
> Jeff Garzik wrote:
>> Albert Lee wrote:
>>> Patch 1/6:
>>>  Update the ATA passthru protocol numbers according to the new spec.
>>>
>>> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
>>
>> applied 1-4, 6 to #upstream-fixes
> 
> What about this one:  [PATCH 5/6] libata: support ATAPI devices  ??
> 
> I'm quite happy to let Albert run with that,
> as he's busily maintaining the rest of the passthrough stuff.

I answered this a month or so ago, but between your lost mail and my 
apparently-lossy Sent folder, it would probably be worth re-answering:

Rather than sprinkling "is it ata or atapi?" tests throughout the 
libata-scsi hotpaths, I would strongly prefer that
__ata_scsi_queuecmd() change to look more like

	if (device is ATA)
		xlat_func = ata_get_xlat_func()
		if (xlat_func)
			ata_scsi_translate()
		else
			ata_scsi_simulate()
	else
		xlat_func = atapi_get_xlat_func()
		if (xlat_func)
			atapi_scsi_translate()
		else
			atapi_scsi_simulate()

and then make the changes that logically fall from that.

	Jeff