Skip to contentSkip to conversation

libata: ATA passthru fixes

PATCH In review Albert Lee 3 participants 11 messages Jun 7, 2007 – Jun 15, 2007 libata

lore
Share & tools
lore

Albert Lee

author
#0 lore raw

cover libata: ATA passthru fixes

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.

author
#1 lore raw

PATCH 1/6 libata: update protocol numbers

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

---

drivers/ata/libata-scsi.c +7 -8
diff -Nrup 00_libata-dev/drivers/ata/libata-scsi.c 01_protocol_update/drivers/ata/libata-scsi.c
@@ -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;
 	}
#2 lore raw

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

Quoted 21 lines · Albert Lee

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

---

diff -Nrup 00_libata-dev/drivers/ata/libata-scsi.c 01_protocol_update/drivers/ata/libata-scsi.c

drivers/ata/libata-scsi.c +0 -6
@@ -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 */

drivers/ata/libata-scsi.c ata_scsi_map_proto(u8 byte1) · line 2524

Quoted 10 lines
drivers/ata/libata-scsi.c +7 -2
-		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

#4 lore raw

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

Quoted 7 lines · Jeff Garzik
Quoted 4 lines · Albert Lee

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

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

#5 lore raw

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

Quoted 5 lines · Mark Lord
Quoted 4 lines · Jeff Garzik
Quoted 3 lines · Albert Lee

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

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

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

author
#6 lore raw

PATCH 2/6 libata: support PIO multi commands

In reply to Albert Lee

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

---

drivers/ata/libata-scsi.c +16 -4
diff -Nrup 01_protocol_update/drivers/ata/libata-scsi.c 02_pio_multi/drivers/ata/libata-scsi.c
@@ -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) {
include/linux/ata.h +1 -1
diff -Nrup 01_protocol_update/include/linux/ata.h 02_pio_multi/include/linux/ata.h
@@ -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*/
author
#7 lore raw

PATCH 3/6 libata: map UDMA protocols

In reply to Albert Lee

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

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

drivers/ata/libata-scsi.c +2 -2
diff -Nrup 02_pio_multi/drivers/ata/libata-scsi.c 03_udma_supp/drivers/ata/libata-scsi.c
@@ -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 */
author
#8 lore raw

PATCH 4/6 libata: always enforce correct DEV bit

In reply to 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.

---

drivers/ata/libata-scsi.c +4 -6
diff -Nrup 03_udma_supp/drivers/ata/libata-scsi.c 04_dev_bit/drivers/ata/libata-scsi.c
@@ -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))
author
#9 lore raw

PATCH 5/6 libata: support ATAPI devices

In reply to Albert Lee

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

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

drivers/ata/libata-scsi.c +7 -6
diff -Nrup 04_dev_bit/drivers/ata/libata-scsi.c 05_supp_atapi/drivers/ata/libata-scsi.c
@@ -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);
author
#10 lore raw

PATCH 6/6 libata: update cached device paramters

In reply to 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.

---

drivers/ata/libata-scsi.c +16 -6
diff -Nrup 05_supp_atapi/drivers/ata/libata-scsi.c 06_sync_param/drivers/ata/libata-scsi.c
@@ -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
include/linux/ata.h +1 -0
diff -Nrup 05_supp_atapi/include/linux/ata.h 06_sync_param/include/linux/ata.h
@@ -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,