Skip to contentSkip to conversation

libata: add support for ATA_16 commands to ATAPI devices

PATCH In review Mark Lord 3 participants 14 messages Jan 3, 2007 – Jan 26, 2007 libata

lore
Share & tools
lore

Mark Lord

author
#0 lore raw

PATCH 1/1 libata: add support for ATA_16 commands to ATAPI devices

This patch adds support for passing ATA_16 commands
through to ATAPI devices in libata.  In practice, the upper layers will still currently prevent ATA_16 commands, as no (?) ATAPI drives support them yet.  But it will work if such a drive is ever encountered.

Support for ATA_16 is necessary for using SG_IO from userspace, and an upcoming hdparm release will be updated to use this interface.

This version is very similar to the earlier first submission, except that ATA_12 is no longer passed through to ATAPI because of the conflict it has with the SCSI BLANK opcode.

Signed-off-by Mark Lord <[email protected]>
drivers/ata/libata-scsi.c +12 -12
@@ -2650,6 +2650,12 @@
 
 static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device *dev, u8 cmd)
 {
+	if (cmd == ATA_16)
+		return ata_scsi_pass_thru;
+
+	if (dev->class == ATA_DEV_ATAPI)
+		return atapi_xlat;
+
 	switch (cmd) {
 	case READ_6:
 	case READ_10:

@@ -2670,7 +2676,6 @@
 		return ata_scsi_verify_xlat;
 
 	case ATA_12:
-	case ATA_16:
 		return ata_scsi_pass_thru;
 
 	case START_STOP:

@@ -2708,6 +2713,7 @@
 				      void (*done)(struct scsi_cmnd *),
 				      struct ata_device *dev)
 {
+	ata_xlat_func_t xlat_func;
 	int rc = 0;
 
 	if (unlikely(!scmd->cmd_len)) {

@@ -2717,17 +2723,11 @@
 		return 0;
 	}
 
-	if (dev->class == ATA_DEV_ATA) {
-		ata_xlat_func_t xlat_func = ata_get_xlat_func(dev,
-							      scmd->cmnd[0]);
-
-		if (xlat_func)
-			rc = ata_scsi_translate(dev, scmd, done, xlat_func);
-		else
-			ata_scsi_simulate(dev, scmd, done);
-	} else
-		rc = ata_scsi_translate(dev, scmd, done, atapi_xlat);
-
+	xlat_func = ata_get_xlat_func(dev, scmd->cmnd[0]);
+	if (xlat_func)
+		rc = ata_scsi_translate(dev, scmd, done, xlat_func);
+	else
+		ata_scsi_simulate(dev, scmd, done);
 	return rc;
 }
author
#1 lore raw
Quoted 4 lines · Mark Lord

This patch adds support for passing ATA_16 commands
through to ATAPI devices in libata.  In practice, the upper layers will still currently prevent ATA_16 commands, as no (?) ATAPI drives support them yet.  But it will work if such a drive is ever encountered.

The above comment is out of date -- nuke it.
Instead, there is now a separate patch to permit ATA_16 commands for 12-byte libata devices.

The rest of this stuff is still correct no changes below).

* * *
Support for ATA_16 is necessary for using SG_IO from userspace, and an upcoming hdparm release will be updated to use this interface.

This version is very similar to the earlier first submission, except that ATA_12 is no longer passed through to ATAPI because of the conflict it has with the SCSI BLANK opcode.

Signed-off-by Mark Lord <[email protected]>
drivers/ata/libata-scsi.c +12 -12
@@ -2650,6 +2650,12 @@
 
 static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device *dev, u8 cmd)
 {
+	if (cmd == ATA_16)
+		return ata_scsi_pass_thru;
+
+	if (dev->class == ATA_DEV_ATAPI)
+		return atapi_xlat;
+
 	switch (cmd) {
 	case READ_6:
 	case READ_10:

@@ -2670,7 +2676,6 @@
 		return ata_scsi_verify_xlat;
 
 	case ATA_12:
-	case ATA_16:
 		return ata_scsi_pass_thru;
 
 	case START_STOP:

@@ -2708,6 +2713,7 @@
 				      void (*done)(struct scsi_cmnd *),
 				      struct ata_device *dev)
 {
+	ata_xlat_func_t xlat_func;
 	int rc = 0;
 
 	if (unlikely(!scmd->cmd_len)) {

@@ -2717,17 +2723,11 @@
 		return 0;
 	}
 
-	if (dev->class == ATA_DEV_ATA) {
-		ata_xlat_func_t xlat_func = ata_get_xlat_func(dev,
-							      scmd->cmnd[0]);
-
-		if (xlat_func)
-			rc = ata_scsi_translate(dev, scmd, done, xlat_func);
-		else
-			ata_scsi_simulate(dev, scmd, done);
-	} else
-		rc = ata_scsi_translate(dev, scmd, done, atapi_xlat);
-
+	xlat_func = ata_get_xlat_func(dev, scmd->cmnd[0]);
+	if (xlat_func)
+		rc = ata_scsi_translate(dev, scmd, done, xlat_func);
+	else
+		ata_scsi_simulate(dev, scmd, done);
 	return rc;
 }
#2 lore raw
In reply to Mark Lord
Quoted 17 lines · Mark Lord

This patch adds support for passing ATA_16 commands
through to ATAPI devices in libata. In practice, the upper layers will still currently prevent ATA_16 commands, as no (?) ATAPI drives support them yet. But it will work if such a drive is ever encountered.

Support for ATA_16 is necessary for using SG_IO from userspace, and an upcoming hdparm release will be updated to use this interface.

This version is very similar to the earlier first submission, except that ATA_12 is no longer passed through to ATAPI because of the conflict it has with the SCSI BLANK opcode.

Signed-off-by Mark Lord <[email protected]>

drivers/ata/libata-scsi.c line 2658

Quoted 9 lines
drivers/ata/libata-scsi.c +6 -0
@@ -2650,6 +2650,12 @@
 
 static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device *dev, u8 cmd)
 {
+	if (cmd == ATA_16)
+		return ata_scsi_pass_thru;
+
+	if (dev->class == ATA_DEV_ATAPI)
+		return atapi_xlat;
+

As I said, provide some avenue for the user to choose.

I do not wish to completely eliminate the current behavior, which passes through all opcodes to the ATAPI device. You cannot guarantee that ATA_16 is never used for a vendor-reserved opcode, for example.

Your proposed change to the default behavior is ACK'd, as long as the current behavior is not completely eliminated, as you have done above.

Jeff

#3 lore raw
Quoted 3 lines

I do not wish to completely eliminate the current behavior, which passes through all opcodes to the ATAPI device. You cannot guarantee that ATA_16 is never used for a vendor-reserved opcode, for example.

For 16 byte commands via SG_IO you know the command is 16 bytes long as the command length is passed in hdr->cmd_len which becomes rq->cmd_len. Is that not sufficient to avoid this ATA_16 test if you pass it on to the required functions ?

Alan

#4 lore raw
Quoted 6 lines · Alan
Quoted 1 line

I do not wish to completely eliminate the current behavior, which passes through all opcodes to the ATAPI device. You cannot guarantee that ATA_16 is never used for a vendor-reserved opcode, for example.

For 16 byte commands via SG_IO you know the command is 16 bytes long as the command length is passed in hdr->cmd_len which becomes rq->cmd_len. Is that not sufficient to avoid this ATA_16 test if you pass it on to the required functions ?

Well, its a question of whether you are executing the supplied cdb as an ATA command or a SCSI command. If executing as an ATA command, libata needs to intercept it (as Mark's patch does), because few if any ATAPI devices support the ATA_16 command in the firmware.

The current behavior treats everything as SCSI commands, passed to the firmware (if a bit massaged, a la INQUIRY), which means there is no way for userspace to directly address the underlying ATA bus outside of libata-scsi.

Or am I misunderstanding things completely? :)

Jeff

author
#5 lore raw
In reply to Alan
Quoted 6 lines · Alan
Quoted 1 line

I do not wish to completely eliminate the current behavior, which passes through all opcodes to the ATAPI device. You cannot guarantee that ATA_16 is never used for a vendor-reserved opcode, for example.

For 16 byte commands via SG_IO you know the command is 16 bytes long as the command length is passed in hdr->cmd_len which becomes rq->cmd_len. Is that not sufficient to avoid this ATA_16 test if you pass it on to the required functions ?

No. The SCSI code calculates the expected cmd_len based on the opcode it sees, and makes exceptions only for the vendor-specific command blocks, which ATA_16 is *not* part of.

Cheers

#6 lore raw
Quoted 8 lines
Quoted 4 lines

For 16 byte commands via SG_IO you know the command is 16 bytes long as the command length is passed in hdr->cmd_len which becomes rq->cmd_len. Is that not sufficient to avoid this ATA_16 test if you pass it on to the required functions ?

No. The SCSI code calculates the expected cmd_len based on the opcode it sees, and makes exceptions only for the vendor-specific command blocks, which ATA_16 is *not* part of.

I know it currently does this, the more important question for the ATA side is - should it ?

author
#7 lore raw
Quoted 3 lines · Alan

I know it currently does this, the more important question for the ATA side is - should it ?

No. That's why we now have a patch in play to tell it not to. ;)

Cheers!

author
#8 lore raw
In reply to Jeff Garzik
Quoted 12 lines · Jeff Garzik
Quoted 11 lines · Mark Lord

This patch adds support for passing ATA_16 commands
through to ATAPI devices in libata. In practice, the upper layers will still currently prevent ATA_16 commands, as no (?) ATAPI drives support them yet. But it will work if such a drive is ever encountered.

Support for ATA_16 is necessary for using SG_IO from userspace, and an upcoming hdparm release will be updated to use this interface.

This version is very similar to the earlier first submission, except that ATA_12 is no longer passed through to ATAPI because of the conflict it has with the SCSI BLANK opcode.

..

Quoted 1 line

As I said, provide some avenue for the user to choose.

The user does NOT want to choose here. They just want it to work, without needing to alter some mysterious boot flag from app to app. ;)

The only conflict was with ATA_12, and that is no longer a problem here, so no option is necessary for it.

ATA_16 is a heretofore unused opcode, now standardized for the future. It falls completely outside the "vendor specific" opcode blocks for SCSI (there's a standard formula for those, and ATA_16 doesn't fit it).

We can add a useless boot/module flag if you really want one, though. But it's really just bloat in this case.

Cheers

author
#9 lore raw

PATCH 1/1 libata: add atapi_passthru=1 parameter

Quoted 2 lines · Mark Lord
Quoted 1 line · Jeff Garzik

As I said, provide some avenue for the user to choose.

This patch isn't really needed, but it might provide an "out" just in case somebody has some really non-compliant hardware out there someday.

Add a boot/module parameter for libata to force the ATA_16 SCSI opcode to not be interpreted as a standards-compliant ATA passthrough mechanism when used on an ATAPI device
-- instead, it gets passed directly to the ATAPI device.

Signed-off-by Mark Lord <[email protected]>
drivers/ata/libata.h +1 -0
@@ -46,6 +46,7 @@
 
 extern struct workqueue_struct *ata_aux_wq;
 extern int atapi_enabled;
+extern int atapi_passthru;
 extern int atapi_dmadir;
 extern int libata_fua;
 extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev);
drivers/ata/libata-core.c +4 -0
@@ -78,6 +78,10 @@
 module_param(atapi_enabled, int, 0444);
 MODULE_PARM_DESC(atapi_enabled, "Enable discovery of ATAPI devices (0=off, 1=on)");
 
+int atapi_passthru = 0;
+module_param(atapi_passthru, int, 0444);
+MODULE_PARM_DESC(atapi_passthru, "Enable passthru of SCSI opcode 0x85 to ATAPI devices");
+
 int atapi_dmadir = 0;
 module_param(atapi_dmadir, int, 0444);
 MODULE_PARM_DESC(atapi_dmadir, "Enable ATAPI DMADIR bridge support (0=off, 1=on)");
drivers/ata/libata-scsi.c +1 -1
@@ -2650,7 +2650,7 @@
 
 static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device *dev, u8 cmd)
 {
-	if (cmd == ATA_16)
+	if (cmd == ATA_16 && !atapi_passthru)
 		return ata_scsi_pass_thru;
 
 	if (dev->class == ATA_DEV_ATAPI)
author
#10 lore raw

PATCH 1/1 RESPIN: libata: add atapi_passthru=1 parameter

Quoted 9 lines · Mark Lord

..
This patch isn't really needed, but it might provide an "out" just in case somebody has some really non-compliant hardware out there someday.

Add a boot/module parameter for libata to force the ATA_16 SCSI opcode to not be interpreted as a standards-compliant ATA passthrough mechanism when used on an ATAPI device
-- instead, it gets passed directly to the ATAPI device.

This version of the patch leaves ATA_16 enabled for non-ATAPI regardless of the setting of the new module parm. Take your pick.

Signed-off-by Mark Lord <[email protected]>
drivers/ata/libata.h +1 -0
@@ -46,6 +46,7 @@
 
 extern struct workqueue_struct *ata_aux_wq;
 extern int atapi_enabled;
+extern int atapi_passthru;
 extern int atapi_dmadir;
 extern int libata_fua;
 extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev);
drivers/ata/libata-core.c +4 -0
@@ -78,6 +78,10 @@
 module_param(atapi_enabled, int, 0444);
 MODULE_PARM_DESC(atapi_enabled, "Enable discovery of ATAPI devices (0=off, 1=on)");
 
+int atapi_passthru = 0;
+module_param(atapi_passthru, int, 0444);
+MODULE_PARM_DESC(atapi_passthru, "Enable passthru of SCSI opcode 0x85 to ATAPI devices");
+
 int atapi_dmadir = 0;
 module_param(atapi_dmadir, int, 0444);
 MODULE_PARM_DESC(atapi_dmadir, "Enable ATAPI DMADIR bridge support (0=off, 1=on)");
drivers/ata/libata-scsi.c +2 -1
@@ -2650,7 +2650,7 @@
 
 static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device *dev, u8 cmd)
 {
-	if (cmd == ATA_16)
+	if (cmd == ATA_16 && !atapi_passthru)
 		return ata_scsi_pass_thru;
 
 	if (dev->class == ATA_DEV_ATAPI)

@@ -2676,6 +2676,7 @@
 		return ata_scsi_verify_xlat;
 
 	case ATA_12:
+	case ATA_16:
 		return ata_scsi_pass_thru;
 
 	case START_STOP:
author
#11 lore raw

Re: [PATCH] RESPIN: libata: add atapi_passthru=1 parameter

Quoted 1 line · Mark Lord

On Wednesday 03 January 2007 01:01, Mark Lord wrote:

..

Quoted 4 lines
Quoted 4 lines

Add a boot/module parameter for libata to force the ATA_16 SCSI opcode to not be interpreted as a standards-compliant ATA passthrough mechanism when used on an ATAPI device
-- instead, it gets passed directly to the ATAPI device.

..

drivers/ata/libata-core.c line 83

Quoted 3 lines
drivers/ata/libata-core.c +3 -0
+int atapi_passthru = 0;
+module_param(atapi_passthru, int, 0444);
+MODULE_PARM_DESC(atapi_passthru, "Enable passthru of SCSI opcode 0x85 to ATAPI devices");

Oh, and if anyone can come up with a less-sucky name for this variable, then PLEASE do so! I don't like it, but had trouble inventing anything better that didn't harken back to my (long ago) COBOL days.. ;)

Cheers

#12 lore raw

Re: [PATCH] RESPIN: libata: add atapi_passthru=1 parameter

In reply to Mark Lord
Quoted 11 lines · Mark Lord
Quoted 9 lines · Mark Lord

..
This patch isn't really needed, but it might provide an "out" just in case somebody has some really non-compliant hardware out there someday.

Add a boot/module parameter for libata to force the ATA_16 SCSI opcode to not be interpreted as a standards-compliant ATA passthrough mechanism when used on an ATAPI device
-- instead, it gets passed directly to the ATAPI device.

Quoted 4 lines

This version of the patch leaves ATA_16 enabled for non-ATAPI regardless of the setting of the new module parm. Take your pick.

Signed-off-by Mark Lord <[email protected]>

ACK this version of the patch, but it does not apply to #upstream...

Jeff

author
#13 lore raw

Re: [PATCH] RESPIN: libata: add atapi_passthru=1 parameter

Quoted 8 lines · Jeff Garzik

Mark Lord wrote:
..

Quoted 2 lines

I believe Tejun and I were in agreement that proper ATA16 support for ATAPI was the better way to go.

I like both approaches. I think direct support of HDIO_GET_IDENTITY can reduce the number of failure points, while making old tools work a bit better.

Okay. It just seemed like a bit of cruft from my old drivers that we could leave behind at this point. But that's fine, it'll keep more folks off my back regarding hdparm. :)

Jeff: I just did this here from scratch:

git clone rsync://rsync2.kernel.org/pub/scm/linux/kernel/git/jgarzik/libata-dev.git libata-dev cd libata-dev
git-checkout upstream

And then applied my patches for ATAPI-ATA16 support to them. They appear to have applied 100% cleanly as-is.

Do I have the correct "upstream" repo ?

Here is the unified patch.
This adds ATA16 command support to libata for ATAPI drives, permitting issuing of ATA (not ATAPI packet) commands to ATAPI devices (eg. "hdparm -I" and "hdparm -C").

 libata-core.c |    4 ++++
 libata-scsi.c |   23 ++++++++++++-----------
 libata.h      |    1 +
 3 files changed, 17 insertions(+), 11 deletions(-)
Signed-off-by Mark Lord <[email protected]>
drivers/ata/libata-core.c +4 -0
index 5117577..509d04a 100644
@@ -78,6 +78,10 @@ int atapi_enabled = 1;
 module_param(atapi_enabled, int, 0444);
 MODULE_PARM_DESC(atapi_enabled, "Enable discovery of ATAPI devices (0=off, 1=on)");
 
+int atapi_passthru = 0;
+module_param(atapi_passthru, int, 0444);
+MODULE_PARM_DESC(atapi_passthru, "Enable passthru of SCSI opcode 0x85 to ATAPI devices");
+
 int atapi_dmadir = 0;
 module_param(atapi_dmadir, int, 0444);
 MODULE_PARM_DESC(atapi_dmadir, "Enable ATAPI DMADIR bridge support (0=off, 1=on)");
drivers/ata/libata-scsi.c +12 -11
index a39d100..4c60d16 100644
@@ -2690,6 +2690,12 @@ static unsigned int ata_scsi_pass_thru(s
 
 static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device *dev, u8 cmd)
 {
+	if (cmd == ATA_16 && !atapi_passthru)
+		return ata_scsi_pass_thru;
+
+	if (dev->class == ATA_DEV_ATAPI)
+		return atapi_xlat;
+
 	switch (cmd) {
 	case READ_6:
 	case READ_10:

@@ -2748,6 +2754,7 @@ static inline int __ata_scsi_queuecmd(st
 				      void (*done)(struct scsi_cmnd *),
 				      struct ata_device *dev)
 {
+	ata_xlat_func_t xlat_func;
 	int rc = 0;
 
 	if (unlikely(!scmd->cmd_len)) {

@@ -2757,17 +2764,11 @@ static inline int __ata_scsi_queuecmd(st
 		return 0;
 	}
 
-	if (dev->class == ATA_DEV_ATA) {
-		ata_xlat_func_t xlat_func = ata_get_xlat_func(dev,
-							      scmd->cmnd[0]);
-
-		if (xlat_func)
-			rc = ata_scsi_translate(dev, scmd, done, xlat_func);
-		else
-			ata_scsi_simulate(dev, scmd, done);
-	} else
-		rc = ata_scsi_translate(dev, scmd, done, atapi_xlat);
-
+	xlat_func = ata_get_xlat_func(dev, scmd->cmnd[0]);
+	if (xlat_func)
+		rc = ata_scsi_translate(dev, scmd, done, xlat_func);
+	else
+		ata_scsi_simulate(dev, scmd, done);
 	return rc;
 }
drivers/ata/libata.h +1 -0
index 81ae41d..89981b4 100644
@@ -46,6 +46,7 @@ enum {
 
 extern struct workqueue_struct *ata_aux_wq;
 extern int atapi_enabled;
+extern int atapi_passthru;
 extern int atapi_dmadir;
 extern int libata_fua;
 extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev);