[PATCH] libata: fix ATA passthrough handling for ATAPI devices

[PATCH] libata: fix ATA passthrough handling for ATAPI devices

[Tejun Heo]

libata used to pass all SCSI commands directly to ATAPI devices.
However, this is incorrect for ATA passthrough commands as they must
be handled by the SAT layer in libata.  Also, regardless of the
attached ATAPI device's supported packet length, SAT says that both
flavors of passthrough commands (ATA12 and ATA16) should work.

This patch makes the following changes to fix ATA passthrough handling
for ATAPI devices.

* implement atapi_get_xlat_func() and make libata handle ATA12 and
  ATA16 in SAT layer instead of passing it directly to the target
  device even if the device is ATAPI.

* Always allow 16byte CDBs for ATAPI devices.  This makes
  ata_set_port_max_cmd_len() unnecessary and dev->cdb_len meaningless
  for ATA devices.  Both are stripped away.  Note that this doesn't
  breach error checking in any substantial way.  shost->max_cmd_len
  has never beena ble to properly protect CDB length violation anyway.

This problem has been spotted by Doublas Gilbert <dougg@torque.net>.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Douglas Gilbert <dougg@torque.net>

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index c127d6f..5c80c73 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1357,20 +1357,6 @@ static void ata_dev_config_ncq(struct at
 		snprintf(desc, desc_sz, "NCQ (depth %d/%d)", hdepth, ddepth);
 }
 
-static void ata_set_port_max_cmd_len(struct ata_port *ap)
-{
-	int i;
-
-	if (ap->scsi_host) {
-		unsigned int len = 0;
-
-		for (i = 0; i < ATA_MAX_DEVICES; i++)
-			len = max(len, ap->device[i].cdb_len);
-
-		ap->scsi_host->max_cmd_len = len;
-	}
-}
-
 /**
  *	ata_dev_configure - Configure the specified ATA/ATAPI device
  *	@dev: Target device to configure
@@ -1500,8 +1486,6 @@ int ata_dev_configure(struct ata_device 
 					"ata%u: dev %u multi count %u\n",
 					ap->id, dev->devno, dev->multi_count);
 		}
-
-		dev->cdb_len = 16;
 	}
 
 	/* ATAPI-specific feature tests */
@@ -1542,8 +1526,6 @@ int ata_dev_configure(struct ata_device 
 		}
 	}
 
-	ata_set_port_max_cmd_len(ap);
-
 	/* limit bridge transfers to udma5, 200 sectors */
 	if (ata_dev_knobble(dev)) {
 		if (ata_msg_drv(ap) && print_info)
@@ -5371,7 +5353,7 @@ static void ata_port_init_shost(struct a
 	shost->max_id = 16;
 	shost->max_lun = 1;
 	shost->max_channel = 1;
-	shost->max_cmd_len = 12;
+	shost->max_cmd_len = 16;
 }
 
 /**
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 7af2a4b..9e27b7f 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2794,6 +2794,23 @@ static inline ata_xlat_func_t ata_get_xl
 }
 
 /**
+ *	atapi_get_xlat_func - determine translation function for ATAPI
+ *	@cmd: SCSI command opcode to consider
+ *
+ *	Look up the SCSI command given, and determine how to translate
+ *	the command.
+ *
+ *	RETURNS:
+ *	Pointer to translation function.
+ */
+static ata_xlat_func_t atapi_get_xlat_func(u8 cmd)
+{
+	if (cmd == ATA_12 || cmd == ATA_16)
+		return ata_scsi_pass_thru;
+	return atapi_xlat;
+}
+
+/**
  *	ata_scsi_dump_cdb - dump SCSI command contents to dmesg
  *	@ap: ATA port to which the command was being sent
  *	@cmd: SCSI command to dump
@@ -2831,8 +2848,11 @@ static inline int __ata_scsi_queuecmd(st
 			rc = ata_scsi_translate(dev, cmd, done, xlat_func);
 		else
 			ata_scsi_simulate(dev, cmd, done);
-	} else
-		rc = ata_scsi_translate(dev, cmd, done, atapi_xlat);
+	} else {
+		ata_xlat_func_t xlat_func = atapi_get_xlat_func(cmd->cmnd[0]);
+
+		rc = ata_scsi_translate(dev, cmd, done, xlat_func);
+	}
 
 	return rc;
 }
diff --git a/include/linux/libata.h b/include/linux/libata.h
index d0a7ad5..a0f95f4 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -480,7 +480,7 @@ struct ata_device {
 	unsigned int		multi_count;	/* sectors count for
 						   READ/WRITE MULTIPLE */
 	unsigned int		max_sectors;	/* per-device max sectors */
-	unsigned int		cdb_len;
+	unsigned int		cdb_len;	/* ATAPI CDB length */
 
 	/* per-dev xfer mask */
 	unsigned int		pio_mask;

Re: [PATCH] libata: fix ATA passthrough handling for ATAPI devices

[Jeff Garzik]

Tejun Heo wrote:
> libata used to pass all SCSI commands directly to ATAPI devices.
> However, this is incorrect for ATA passthrough commands as they must
> be handled by the SAT layer in libata.  Also, regardless of the
> attached ATAPI device's supported packet length, SAT says that both
> flavors of passthrough commands (ATA12 and ATA16) should work.
> 
> This patch makes the following changes to fix ATA passthrough handling
> for ATAPI devices.
> 
> * implement atapi_get_xlat_func() and make libata handle ATA12 and
>   ATA16 in SAT layer instead of passing it directly to the target
>   device even if the device is ATAPI.
> 
> * Always allow 16byte CDBs for ATAPI devices.  This makes

This is definitely wrong.  Some ATAPI devices are limited to 12-byte CDBs.

Also, are we sure that no ATAPI device ever implements these opcodes?

Prior to the SAT spec -- which includes most ATAPI firmwares -- those 
opcodes might have been vendor-reserved space.  Did you or Doug verify 
against the older specifications who might care about these opcodes?

Or maybe there is a flag somewhere we can abuse, that permits support of 
both scenarios -- passing the command to the device, and handling the 
command internally?

	Jeff

Re: [PATCH] libata: fix ATA passthrough handling for ATAPI devices

[Douglas Gilbert]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> libata used to pass all SCSI commands directly to ATAPI devices.
>> However, this is incorrect for ATA passthrough commands as they must
>> be handled by the SAT layer in libata.  Also, regardless of the
>> attached ATAPI device's supported packet length, SAT says that both
>> flavors of passthrough commands (ATA12 and ATA16) should work.
>>
>> This patch makes the following changes to fix ATA passthrough handling
>> for ATAPI devices.
>>
>> * implement atapi_get_xlat_func() and make libata handle ATA12 and
>>   ATA16 in SAT layer instead of passing it directly to the target
>>   device even if the device is ATAPI.
>>
>> * Always allow 16byte CDBs for ATAPI devices.  This makes
> 
> This is definitely wrong.  Some ATAPI devices are limited to 12-byte CDBs.

Jeff,
The 16 byte CDB that Tejun is talking about is the
ATA PASS THROUGH (16) SCSI command. That is valid to
send to SATL (in front of a ATAPI device) for certain
values of EXTEND and T_LENGTH in that CDB (see
sat-r09.pdf section 12.2.4).

> Also, are we sure that no ATAPI device ever implements these opcodes?

The SATL is on the computer side of the ATAPI host
(initiator) so the ATA PASS THROUGH (12+16) SCSI
commands should never be seen by the ATAPI device
(target). Reason: the SATL translates those SCSI
commands into ATA commands prior to them being presented
to the ATAPI host.

> Prior to the SAT spec -- which includes most ATAPI firmwares -- those
> opcodes might have been vendor-reserved space.  Did you or Doug verify
> against the older specifications who might care about these opcodes?

The only clash between MMC and SATL SCSI opcodes is
with opcode 0xA1: BLANK in MMC and ATA PASS THROUGH (12)
in SAT. So, in the future, one way to resolve this is
to optionally tell the SATL not to translate the ATA PASS
THROUGH (12) SCSI command and send it through to the
ATAPI device which would interpret it as BLANK. That would
leave SATL users with the ATA PASS THROUGH (16) SCSI command
for sending ATA commands to the ATAPI device. And that ....
is definitely valid IMO.

> Or maybe there is a flag somewhere we can abuse, that permits support of
> both scenarios -- passing the command to the device, and handling the
> command internally?

This is all about being able to send ATA commands to
ATAPI devices that are valid for PACKET devices, examples:
    IDENTIFY PACKET DEVICE
    SET FEATURES
    IDENTIFY DEVICE (should abort command + set signature)
    DEVICE RESET
and I assume there are others.

Doug Gilbert

Re: [PATCH] libata: fix ATA passthrough handling for ATAPI devices

[Jeff Garzik]

Douglas Gilbert wrote:
> The 16 byte CDB that Tejun is talking about is the
> ATA PASS THROUGH (16) SCSI command. That is valid to

I'm talking about the changes to the implementation, which appear to 
mistakenly allow 16-byte CDBs through to the ATAPI device, even if it 
only supports 12-byte CDBs.

I am quite aware of the ATA passthru SCSI command.  Heck, the spec had 
input from me.  I'm talking about something different.


> The only clash between MMC and SATL SCSI opcodes is
> with opcode 0xA1: BLANK in MMC and ATA PASS THROUGH (12)

Ok, so the answer is, yes there is a clash, and thus this change will 
remove the ability for working-today setups to use BLANK.

In order to avoid breaking working setups, a method must be found which 
tells the SATL to not filter out the ATA passthru commands.


> This is all about being able to send ATA commands to
> ATAPI devices that are valid for PACKET devices, examples:
>     IDENTIFY PACKET DEVICE
>     SET FEATURES
>     IDENTIFY DEVICE (should abort command + set signature)
>     DEVICE RESET
> and I assume there are others.

I am quite aware of the purpose of ATA passthru :)

	Jeff

Re: [PATCH] libata: fix ATA passthrough handling for ATAPI devices

[Douglas Gilbert]

Jeff Garzik wrote:
> Douglas Gilbert wrote:
>> The 16 byte CDB that Tejun is talking about is the
>> ATA PASS THROUGH (16) SCSI command. That is valid to
> 
> I'm talking about the changes to the implementation, which appear to
> mistakenly allow 16-byte CDBs through to the ATAPI device, even if it
> only supports 12-byte CDBs.
> 
> I am quite aware of the ATA passthru SCSI command.  Heck, the spec had
> input from me.  I'm talking about something different.

This is what I saw, with the new ATA subsystem in
lk 2.6.19-rc1:

[root@sas ~]# lsscsi -H
[0]    sata_nv
[1]    sata_nv
[2]    sata_nv
[3]    sata_nv
[4]    pata_amd
[5]    pata_amd
[6]    mptsas
[7]    aic94xx

[root@sas ~]# lsscsi
[1:0:0:0]    disk    ATA      ST380013AS       3.18  /dev/sdb
[4:0:0:0]    disk    ATA      ST380011A        8.01  /dev/sda
[5:0:1:0]    cd/dvd  HL-DT-ST DVD-ROM GDR8163B 0L23  /dev/scd0

[root@sas ~]# sg_sat_identify --packet /dev/scd0 -vv
open /dev/scd0 with flags=0x802
    ATA pass through (16) cdb: 85 08 0e 00 00 00 01 00 00 00 00 00 00 00 a1 00
ATA pass through (16): transport error: Host_status=0x05 [DID_ABORT]
Driver_status=0x00 [DRIVER_OK, SUGGEST_OK]
ATA pass through (16) failed

[root@sas ~]# sg_sat_identify --packet --len=12 /dev/scd0 -vv
open /dev/scd0 with flags=0x802
    ATA pass through (12) cdb: a1 08 0e 00 01 00 00 00 00 a1 00 00
ATA pass through:  Fixed format, current;  Sense key: Illegal Request
 Additional sense: Invalid command operation code



The above were attempts to send IDENTIFY PACKET DEVICE
to the PATAPI cd/dvd drive via the ATA PASS THROUGH
(16 and 12) commands respectively. The first one caught
my attention. Why did it fail with a DID_ABORT?

Whatever the reason both responses are wrong. The second
one is wrong because
  - the cd/dvd drive does support that opcode (the error
    should be either not ready or incompatible format), or
  - it didn't do the ATA PASS THROUGH (12) properly

>> The only clash between MMC and SATL SCSI opcodes is
>> with opcode 0xA1: BLANK in MMC and ATA PASS THROUGH (12)
> 
> Ok, so the answer is, yes there is a clash, and thus this change will
> remove the ability for working-today setups to use BLANK.
> 
> In order to avoid breaking working setups, a method must be found which
> tells the SATL to not filter out the ATA passthru commands.

Or, as a temporary solution, if the device is ATAPI
carrying a SCSI command set with pdt=5 then:
  1) opcode 0xa1 goes straight through (i.e. BLANK)
  2) opcode 0x85 works as defined by sat-r09.pdf
     (implements the ATA PASS THROUGH (16) SCSI command)

>> This is all about being able to send ATA commands to
>> ATAPI devices that are valid for PACKET devices, examples:
>>     IDENTIFY PACKET DEVICE
>>     SET FEATURES
>>     IDENTIFY DEVICE (should abort command + set signature)
>>     DEVICE RESET
>> and I assume there are others.
> 
> I am quite aware of the purpose of ATA passthru :)

... and I do comparative analysis of SAT layers and
send bug reports.

Doug Gilbert

Re: [PATCH] libata: fix ATA passthrough handling for ATAPI devices

[Jeff Garzik]

Douglas Gilbert wrote:
> Jeff Garzik wrote:
>> I'm talking about the changes to the implementation, which appear to
>> mistakenly allow 16-byte CDBs through to the ATAPI device, even if it
>> only supports 12-byte CDBs.
>>
>> I am quite aware of the ATA passthru SCSI command.  Heck, the spec had
>> input from me.  I'm talking about something different.

> The above were attempts to send IDENTIFY PACKET DEVICE
> to the PATAPI cd/dvd drive via the ATA PASS THROUGH
> (16 and 12) commands respectively. The first one caught
> my attention. Why did it fail with a DID_ABORT?
> 
> Whatever the reason both responses are wrong. The second
> one is wrong because
>   - the cd/dvd drive does support that opcode (the error
>     should be either not ready or incompatible format), or
>   - it didn't do the ATA PASS THROUGH (12) properly

The thread has moved on from discussing the acknowledged problem to 
discussing the solution.

To summarize my quoted email above, I'm talking about problems with the 
implementation details.  There are two engineering constraints which a 
solution must not violate:

1) The kernel must prevent 16-byte non-passthru commands from reaching 
12-byte-only ATAPI devices.

2) Existing in-the-field solutions that issue commands such as BLANK or 
a vendor-reserved 0x85 opcode must continue working, without recompile.

	Jeff

Re: [PATCH] libata: fix ATA passthrough handling for ATAPI devices

[Douglas Gilbert]

Jeff Garzik wrote:
> Douglas Gilbert wrote:
>> Jeff Garzik wrote:
>>> I'm talking about the changes to the implementation, which appear to
>>> mistakenly allow 16-byte CDBs through to the ATAPI device, even if it
>>> only supports 12-byte CDBs.
>>>
>>> I am quite aware of the ATA passthru SCSI command.  Heck, the spec had
>>> input from me.  I'm talking about something different.
> 
>> The above were attempts to send IDENTIFY PACKET DEVICE
>> to the PATAPI cd/dvd drive via the ATA PASS THROUGH
>> (16 and 12) commands respectively. The first one caught
>> my attention. Why did it fail with a DID_ABORT?
>>
>> Whatever the reason both responses are wrong. The second
>> one is wrong because
>>   - the cd/dvd drive does support that opcode (the error
>>     should be either not ready or incompatible format), or
>>   - it didn't do the ATA PASS THROUGH (12) properly
> 
> The thread has moved on from discussing the acknowledged problem to
> discussing the solution.
> 
> To summarize my quoted email above, I'm talking about problems with the
> implementation details.  There are two engineering constraints which a
> solution must not violate:

> 1) The kernel must prevent 16-byte non-passthru commands from reaching
> 12-byte-only ATAPI devices.

I would prefer to see the command sent through until
it hits a transport that cannot carry a 16 byte cdb.
That transport may be in an external enclosure, or
the limitation may have been removed.

> 2) Existing in-the-field solutions that issue commands such as BLANK or
> a vendor-reserved 0x85 opcode must continue working, without recompile.

Opcodes 0x85 and 0xa1 are not vendor reserved. They have
been reserved for t10.org use since SCSI-2. Other SCSI
opcode ranges are set aside for vendor use.
The BLANK problems remains.

Doug Gilbert

Re: [PATCH] libata: fix ATA passthrough handling for ATAPI devices

[Tejun Heo]

Douglas Gilbert wrote:
>> 1) The kernel must prevent 16-byte non-passthru commands from reaching
>> 12-byte-only ATAPI devices.
> 
> I would prefer to see the command sent through until
> it hits a transport that cannot carry a 16 byte cdb.
> That transport may be in an external enclosure, or
> the limitation may have been removed.

The host max CDB len has never been a proper protection against illegal 
CDB length.  Think about PATA, ata_piix SLAVE_POSS or PMP cases sharing 
the host w/ ATA devices.  Max CDB len is always 16 in such cases whether 
an ATAPI device supports 12 or 16 byte CDB.  Also, max CDB len doesn't 
protect against smaller CDB (12 bytes) when the device supports 16 byte 
CDBs.

So, for some cases, we had protection and in many others we didn't. 
We've always depended on applications sending correctly sized CDBs and 
ATAPI devices aborting wrong sized CDBs.

I'm not sure whether the protection is necessary, but if it is, we can 
do the protection *properly* by moving cdb restriction to lower level 
such that it's applied per-device not per-host.  This also solves the 
16byte passthrough problem.

IMHO, this issues is separate from SAT passthrough and needs 
improvements regardless.

>> 2) Existing in-the-field solutions that issue commands such as BLANK or
>> a vendor-reserved 0x85 opcode must continue working, without recompile.
> 
> Opcodes 0x85 and 0xa1 are not vendor reserved. They have
> been reserved for t10.org use since SCSI-2. Other SCSI
> opcode ranges are set aside for vendor use.
> The BLANK problems remains.

Ah.. I haven't thought about opcode clash.  I'm all ears on this.  Just 
tell me what to do, I'll go and implement it.

Thanks.

-- 
tejun

Re: [PATCH] libata: fix ATA passthrough handling for ATAPI devices

[Douglas Gilbert]

Tejun Heo wrote:
> Douglas Gilbert wrote:
>>> 1) The kernel must prevent 16-byte non-passthru commands from reaching
>>> 12-byte-only ATAPI devices.
>>
>> I would prefer to see the command sent through until
>> it hits a transport that cannot carry a 16 byte cdb.
>> That transport may be in an external enclosure, or
>> the limitation may have been removed.
> 
> The host max CDB len has never been a proper protection against illegal
> CDB length.  Think about PATA, ata_piix SLAVE_POSS or PMP cases sharing
> the host w/ ATA devices.  Max CDB len is always 16 in such cases whether
> an ATAPI device supports 12 or 16 byte CDB.  Also, max CDB len doesn't
> protect against smaller CDB (12 bytes) when the device supports 16 byte
> CDBs.
> 
> So, for some cases, we had protection and in many others we didn't.
> We've always depended on applications sending correctly sized CDBs and
> ATAPI devices aborting wrong sized CDBs.
> 
> I'm not sure whether the protection is necessary, but if it is, we can
> do the protection *properly* by moving cdb restriction to lower level
> such that it's applied per-device not per-host.  This also solves the
> 16byte passthrough problem.
> 
> IMHO, this issues is separate from SAT passthrough and needs
> improvements regardless.
> 
>>> 2) Existing in-the-field solutions that issue commands such as BLANK or
>>> a vendor-reserved 0x85 opcode must continue working, without recompile.
>>
>> Opcodes 0x85 and 0xa1 are not vendor reserved. They have
>> been reserved for t10.org use since SCSI-2. Other SCSI
>> opcode ranges are set aside for vendor use.
>> The BLANK problems remains.
> 
> Ah.. I haven't thought about opcode clash.  I'm all ears on this.  Just
> tell me what to do, I'll go and implement it.

Tejun,
To summarize:
  - there should never be vendor specific commands at
    SCSI opcodes 0x85 and 0xa1
  - referring the Annex D.3.2 in spc4r07a.pdf (www.t10.org)
    opcode 0x85 (APT (16)) does not clash with anything.
    Opcode 0xa1 (APT (12)) clashes with the BLANK command
    defined for MMC devices

The BLANK command (mmc5r03c.pdf section 6.2) seems to be
mandatory for drives that support writing to CD-RW and DVD-RW
media. That implies opcode 0xa1 should go through the SATL
to an ATAPI CD/DVD drive. Should it go through to an ATAPI
tape drive?

Given the APT (12) will not be available for ATAPI CD/DVD
drives, it is important that APT (16) (i.e. opcode 0x85)
works properly (i.e. implements the SAT ATA PASS THROUGH
logic).

Doug Gilbert

Re: [PATCH] libata: fix ATA passthrough handling for ATAPI devices

[Jeff Garzik]

Tejun Heo wrote:
> libata used to pass all SCSI commands directly to ATAPI devices.
> However, this is incorrect for ATA passthrough commands as they must
> be handled by the SAT layer in libata.  Also, regardless of the
> attached ATAPI device's supported packet length, SAT says that both
> flavors of passthrough commands (ATA12 and ATA16) should work.
> 
> This patch makes the following changes to fix ATA passthrough handling
> for ATAPI devices.
> 
> * implement atapi_get_xlat_func() and make libata handle ATA12 and
>   ATA16 in SAT layer instead of passing it directly to the target
>   device even if the device is ATAPI.
> 
> * Always allow 16byte CDBs for ATAPI devices.  This makes
>   ata_set_port_max_cmd_len() unnecessary and dev->cdb_len meaningless
>   for ATA devices.  Both are stripped away.  Note that this doesn't
>   breach error checking in any substantial way.  shost->max_cmd_len
>   has never beena ble to properly protect CDB length violation anyway.
> 
> This problem has been spotted by Doublas Gilbert <dougg@torque.net>.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: Douglas Gilbert <dougg@torque.net>

Resuming from the thread:

* Regarding CDB length:  DEPENDING ON IMPLEMENTATION DETAILS AND HOT 
PATH IMPACT, I think Douglas's comment about CDB lengths is fair:  we 
could unconditionally set the limit to 16 bytes, and then filter out 
16-byte CDBs at a lower level if the ATAPI device doesn't support 
16-byte CDBs.

* We need a better solution for the duplicate opcode.