[PATCH] libata: add support for ATA_16 commands to ATAPI devices

[PATCH] libata: add support for ATA_16 commands to ATAPI devices

[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 <mlord@pobox.com>

--- old/drivers/ata/libata-scsi.c	2007-01-02 19:20:23.000000000 -0500
+++ new/drivers/ata/libata-scsi.c	2007-01-02 19:20:49.000000000 -0500
@@ -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;
 }
 

re:[PATCH] libata: add support for ATA_16 commands to ATAPI devices

[Mark Lord]

On Tuesday 02 January 2007 19:39, Mark Lord wrote:
> 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 <mlord@pobox.com>

--- old/drivers/ata/libata-scsi.c	2007-01-02 19:20:23.000000000 -0500
+++ new/drivers/ata/libata-scsi.c	2007-01-02 19:20:49.000000000 -0500
@@ -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;
 }
 

Re: [PATCH] libata: add support for ATA_16 commands to ATAPI devices

[Jeff Garzik]

Mark Lord wrote:
> 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 <mlord@pobox.com>
> 
> --- old/drivers/ata/libata-scsi.c	2007-01-02 19:20:23.000000000 -0500
> +++ new/drivers/ata/libata-scsi.c	2007-01-02 19:20:49.000000000 -0500
> @@ -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

Re: [PATCH] libata: add support for ATA_16 commands to ATAPI devices

[Jeff Garzik]

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

Re: [PATCH] libata: add support for ATA_16 commands to ATAPI devices

[Alan]

> 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

Re: [PATCH] libata: add support for ATA_16 commands to ATAPI devices

[Mark Lord]

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

Re: [PATCH] libata: add support for ATA_16 commands to ATAPI devices

[Mark Lord]

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

[PATCH] libata: add atapi_passthru=1 parameter

[Mark Lord]

On Wednesday 03 January 2007 00:47, Mark Lord wrote:
> Jeff Garzik wrote:
> > 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 <mlord@pobox.com>
--- old/drivers/ata/libata.h	2007-01-02 19:06:56.000000000 -0500
+++ linux/drivers/ata/libata.h	2007-01-03 00:55:01.000000000 -0500
@@ -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);
--- old/drivers/ata/libata-core.c	2007-01-02 19:09:06.000000000 -0500
+++ linux/drivers/ata/libata-core.c	2007-01-03 00:52:51.000000000 -0500
@@ -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)");
--- old/drivers/ata/libata-scsi.c	2007-01-02 19:20:49.000000000 -0500
+++ linux/drivers/ata/libata-scsi.c	2007-01-03 00:55:08.000000000 -0500
@@ -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)

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

[Mark Lord]

On Wednesday 03 January 2007 01:01, Mark Lord wrote:
>..
> 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 <mlord@pobox.com>

--- old/drivers/ata/libata.h	2007-01-02 19:06:56.000000000 -0500
+++ linux/drivers/ata/libata.h	2007-01-03 00:55:01.000000000 -0500
@@ -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);
--- old/drivers/ata/libata-core.c	2007-01-02 19:09:06.000000000 -0500
+++ linux/drivers/ata/libata-core.c	2007-01-03 00:52:51.000000000 -0500
@@ -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)");
--- old/drivers/ata/libata-scsi.c	2007-01-02 19:20:49.000000000 -0500
+++ linux/drivers/ata/libata-scsi.c	2007-01-03 01:21:28.000000000 -0500
@@ -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:

Re: [PATCH] libata: add support for ATA_16 commands to ATAPI devices

[Alan]

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

Re: [PATCH] libata: add support for ATA_16 commands to ATAPI devices

[Mark Lord]

Alan wrote:
>
> 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!

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

[Mark Lord]

Mark Lord wrote:
> On Wednesday 03 January 2007 01:01, Mark Lord wrote:
..
>> 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.
..
> +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

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

[Jeff Garzik]

Mark Lord wrote:
> On Wednesday 03 January 2007 01:01, Mark Lord wrote:
>> ..
>> 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 <mlord@pobox.com>

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

	Jeff

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

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
>..
>> 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 <mlord@pobox.com>

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 5117577..509d04a 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -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)");
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index a39d100..4c60d16 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -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;
 }
 
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 81ae41d..89981b4 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -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);