[PATCH 2.6.20-rc3] fix broken retval test in sr_block_ioctl

[PATCH 2.6.20-rc3] fix broken retval test in sr_block_ioctl

[Mark Lord]

With recent kernels (and possibly much older ones, too),
ioctl() calls for ATAPI devices never make it to the driver layers.

The reason for this is a borked return code test in drivers/scsi/sr.c.
This patch fixes it.

Signed-off-by:  Mark Lord <mlord@pobox.com>

---
--- old/drivers/scsi/sr.c	2006-11-29 16:57:37.000000000 -0500
+++ linux/drivers/scsi/sr.c	2007-01-02 16:40:33.000000000 -0500
@@ -468,7 +468,7 @@
 	}
 
 	ret = cdrom_ioctl(file, &cd->cdi, inode, cmd, arg);
-	if (ret != ENOSYS)
+	if (ret != -ENOSYS)
 		return ret;
 
 	/*

Re: [PATCH 2.6.20-rc3] fix broken retval test in sr_block_ioctl

[Mark Lord]

This patch adds support for passing ATA_12 and 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.

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

Signed-off-by:  Mark Lord <mlord@pobox.com>
---
Allow ATA_12 / ATA_16 passthru commands to be issued for ATAPI devices


diff -u --recursive --new-file --exclude='.*' --exclude='*.[osa]' --exclude='*.ko' --exclude='*.mod.c' --exclude='*.orig' --exclude=System.map --exclude='*.lds' --exclude='*.symvers' old/drivers/ata/libata-scsi.c linux/drivers/ata/libata-scsi.c
--- old/drivers/ata/libata-scsi.c	2007-01-02 15:12:31.000000000 -0500
+++ linux/drivers/ata/libata-scsi.c	2007-01-02 15:31:56.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_12 || 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:
@@ -2669,10 +2675,6 @@
 	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;
 	}
@@ -2708,6 +2710,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 +2720,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 2.6.20-rc3] fix broken retval test in sr_block_ioctl

[Jeff Garzik]

Mark Lord wrote:
> With recent kernels (and possibly much older ones, too),
> ioctl() calls for ATAPI devices never make it to the driver layers.
> 
> The reason for this is a borked return code test in drivers/scsi/sr.c.
> This patch fixes it.
> 
> Signed-off-by:  Mark Lord <mlord@pobox.com>
> 
> ---
> --- old/drivers/scsi/sr.c	2006-11-29 16:57:37.000000000 -0500
> +++ linux/drivers/scsi/sr.c	2007-01-02 16:40:33.000000000 -0500
> @@ -468,7 +468,7 @@
>  	}
>  
>  	ret = cdrom_ioctl(file, &cd->cdi, inode, cmd, arg);
> -	if (ret != ENOSYS)
> +	if (ret != -ENOSYS)

I think Tejun posted the same patch earlier today.

ACK to either patch, of course.

	Jeff

[PATCH] Add ATA_12/ATA_16 support for libata ATAPI

[Mark Lord]

Resending with fixed Subject line:

On Tuesday 02 January 2007 17:30, Mark Lord wrote:
> This patch adds support for passing ATA_12 and 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.
> 
> This patch is necessary for using SG_IO from userspace,
> and an upcoming hdparm release will be updated to use this interface.

Signed-off-by:  Mark Lord <mlord@pobox.com>
---
Allow ATA_12 / ATA_16 passthru commands to be issued for ATAPI devices


diff -u --recursive --new-file --exclude='.*' --exclude='*.[osa]' --exclude='*.ko' --exclude='*.mod.c' --exclude='*.orig' --exclude=System.map --exclude='*.lds' --exclude='*.symvers' old/drivers/ata/libata-scsi.c linux/drivers/ata/libata-scsi.c
--- old/drivers/ata/libata-scsi.c	2007-01-02 15:12:31.000000000 -0500
+++ linux/drivers/ata/libata-scsi.c	2007-01-02 15:31:56.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_12 || 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:
@@ -2669,10 +2675,6 @@
 	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;
 	}
@@ -2708,6 +2710,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 +2720,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;
 }
 

[PATCH] libata use ATA_12 in HDIO_* ioctls for ATAPI compatibility

[Mark Lord]

Existing ATAPI devices do not support 16-byte commands,
so issuing ATA_16 against them gets rejected by the upper layers.

This patch updates the libata implementations of HDIO_DRIVE_CMD
and HDIO_DRIVE_TASK to use ATA_12 (rather than ATA_16) so that
they can now also be used for ATAPI devices.

Signed-off-by:  Mark Lord <mlord@pobox.com>
---
--- linux/drivers/ata/libata-scsi.c.1	2007-01-02 15:31:56.000000000 -0500
+++ linux/drivers/ata/libata-scsi.c	2007-01-02 15:43:20.000000000 -0500
@@ -199,18 +199,17 @@
 		data_dir = DMA_NONE;
 	}
 
-	scsi_cmd[0] = ATA_16;
-
-	scsi_cmd[4] = args[2];
+	scsi_cmd[0] = ATA_12;
+	scsi_cmd[3] = args[2];
 	if (args[0] == WIN_SMART) { /* hack -- ide driver does this too... */
-		scsi_cmd[6]  = args[3];
-		scsi_cmd[8]  = args[1];
-		scsi_cmd[10] = 0x4f;
-		scsi_cmd[12] = 0xc2;
+		scsi_cmd[4] = args[3];
+		scsi_cmd[5] = args[1];
+		scsi_cmd[6] = 0x4f;
+		scsi_cmd[7] = 0xc2;
 	} else {
-		scsi_cmd[6]  = args[1];
+		scsi_cmd[4] = args[1];
 	}
-	scsi_cmd[14] = args[0];
+	scsi_cmd[9] = args[0];
 
 	/* Good values for timeout and retries?  Values below
 	   from scsi_ioctl_send_command() for default case... */
@@ -283,15 +282,15 @@
 		return -EFAULT;
 
 	memset(scsi_cmd, 0, sizeof(scsi_cmd));
-	scsi_cmd[0]  = ATA_16;
-	scsi_cmd[1]  = (3 << 1); /* Non-data */
+	scsi_cmd[0] = ATA_12;
+	scsi_cmd[1] = (3 << 1); /* Non-data */
 	/* scsi_cmd[2] is already 0 -- no off.line, cc, or data xfer */
-	scsi_cmd[4]  = args[1];
-	scsi_cmd[6]  = args[2];
-	scsi_cmd[8]  = args[3];
-	scsi_cmd[10] = args[4];
-	scsi_cmd[12] = args[5];
-	scsi_cmd[14] = args[0];
+	scsi_cmd[3] = args[1];
+	scsi_cmd[4] = args[2];
+	scsi_cmd[5] = args[3];
+	scsi_cmd[6] = args[4];
+	scsi_cmd[7] = args[5];
+	scsi_cmd[9] = args[0];
 
 	/* Good values for timeout and retries?  Values below
 	   from scsi_ioctl_send_command() for default case... */

Re: [PATCH 2.6.20-rc3] fix broken retval test in sr_block_ioctl

[Jeff Garzik]

Mark Lord wrote:
> This patch adds support for passing ATA_12 and 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.
> 
> This patch is necessary for using SG_IO from userspace,
> and an upcoming hdparm release will be updated to use this interface.
> 
> Signed-off-by:  Mark Lord <mlord@pobox.com>
> ---
> Allow ATA_12 / ATA_16 passthru commands to be issued for ATAPI devices

Douglas Gilbert noticed this a while ago.

The patch's technical content is fine, but there is an open policy 
question:  For some devices, there is an opcode overlap (BLANK?  Doug 
probably remembers the issue better than I).  And from a practical 
standpoint, to handle any vendor weird-isms (nahhh those never happen in 
ATAPI), it would be nice to be able to force the current (pre-mlord 
patch) behavior to ensure that all opcodes are passed to ATAPI.

Absent a better idea, I would simply suggest adding a module parameter 
that restores the "all opcodes are passed to device, guaranteed" mode.

I'm fine with changing the default behavior to that which is presented 
by your patch.

	Jeff

Re: [PATCH 2.6.20-rc3] fix broken retval test in sr_block_ioctl

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
...
>>      ret = cdrom_ioctl(file, &cd->cdi, inode, cmd, arg);
>> -    if (ret != ENOSYS)
>> +    if (ret != -ENOSYS)
> 
> I think Tejun posted the same patch earlier today.

Ahh.. missed it -- could have saved me an hour or two of debugging!

Cheers!

Re: [PATCH 2.6.20-rc3] fix broken retval test in sr_block_ioctl

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
..
>> Allow ATA_12 / ATA_16 passthru commands to be issued for ATAPI devices
> 
> Douglas Gilbert noticed this a while ago.
> 
> The patch's technical content is fine, but there is an open policy 
> question:  For some devices, there is an opcode overlap (BLANK?  Doug 
> probably remembers the issue better than I).  And from a practical 
> standpoint, to handle any vendor weird-isms (nahhh those never happen in 
> ATAPI), it would be nice to be able to force the current (pre-mlord 
> patch) behavior to ensure that all opcodes are passed to ATAPI.
> 
> Absent a better idea, I would simply suggest adding a module parameter 
> that restores the "all opcodes are passed to device, guaranteed" mode.
> 
> I'm fine with changing the default behavior to that which is presented 
> by your patch.

Mmmm.. yes, the "BLANK" opcode is indeed the same as ATA_12.
That looks rather dangerous to me -- isn't BLANK commonly used
with CD-RW discs and the like?  (gotta test that now..)

If so, then perhaps all we can support is ATA_16, except the upper
layers mightn't even pass them to libata without a bit of hackery.

I'll go and see about BLANK now..

Cheers

Mark

Re: [PATCH 2.6.20-rc3] fix broken retval test in sr_block_ioctl

[Mark Lord]

Mark Lord wrote:
> Jeff Garzik wrote:
>> Mark Lord wrote:
> ..
>>> Allow ATA_12 / ATA_16 passthru commands to be issued for ATAPI devices
..
> Mmmm.. yes, the "BLANK" opcode is indeed the same as ATA_12.

Yup, BLANK is commonly used with CD-RW software (cdrecord, k3b, ..),
so we cannot translate ATA_12 for ATAPI CD devices.

How about a hack to the upper layer (scsi, or block??) to allow ATA_16
even when the max_cmd_len is 12 ??  In reality, the command length is
unimportant in that case, so it's more a matter of tastefulness than anything.

???

Re: [PATCH] libata use ATA_12 in HDIO_* ioctls for ATAPI compatibility

[Mark Lord]

Mark Lord wrote:
> Existing ATAPI devices do not support 16-byte commands,
> so issuing ATA_16 against them gets rejected by the upper layers.
> 
> This patch updates the libata implementations of HDIO_DRIVE_CMD
> and HDIO_DRIVE_TASK to use ATA_12 (rather than ATA_16) so that
> they can now also be used for ATAPI devices.
> 
> Signed-off-by:  Mark Lord <mlord@pobox.com>
> ---
> --- linux/drivers/ata/libata-scsi.c.1	2007-01-02 15:31:56.000000000 -0500
> +++ linux/drivers/ata/libata-scsi.c	2007-01-02 15:43:20.000000000 -0500
..

NUKE that one.   Due to the conflict between ATA_12 and "BLANK",
we have to leave it all as ATA_16.

I'll post a new ATAPI/ATA_16 patch shortly.

Mark

Re: [PATCH 2.6.20-rc3] fix broken retval test in sr_block_ioctl

[Douglas Gilbert]

Mark Lord wrote:
> Mark Lord wrote:
>> Jeff Garzik wrote:
>>> Mark Lord wrote:
>> ..
>>>> Allow ATA_12 / ATA_16 passthru commands to be issued for ATAPI devices
> ..
>> Mmmm.. yes, the "BLANK" opcode is indeed the same as ATA_12.
> 
> Yup, BLANK is commonly used with CD-RW software (cdrecord, k3b, ..),
> so we cannot translate ATA_12 for ATAPI CD devices.

I can also confirm that.

I have been around the same loop with Luben Tuikov's
SATL in his aic94xx driver.

There is also a SATL in firmware in the LSI Fusion MPT
SAS HBAs. As I don't have a SATAPI cd/dvd drive I'm
unable to find out what their firmware does in this situation.

I have also pointed out the BLANK/ATA_12 clash to the
editor of the SAT-2 draft (and he wasn't aware of it).

> How about a hack to the upper layer (scsi, or block??) to allow ATA_16
> even when the max_cmd_len is 12 ??  In reality, the command length is
> unimportant in that case, so it's more a matter of tastefulness than
> anything.

Yes, ATA_16 obviously needs to be let through to the SATL
(whether it is in libata or lower) even though the sr driver
might try and cap SCSI cdb length to 12 bytes.


BTW To test this fetch sg3_utils-1.22 and sg_sat_identifier
can be used to send an IDENTIFIER PACKET DEVICE via a ATA_16
(or ATA_12 for that matter).

Doug Gilbert