Skip to contentSkip to conversation

fix broken retval test in sr_block_ioctl

PATCH In review Mark Lord 3 participants 11 messages Jan 2, 2007 – Jan 3, 2007

Subsystems (2)
SCSI libata
lore
Share & tools
lore

Mark Lord

author
#0 lore raw

PATCH 1/1 fix broken retval test in sr_block_ioctl

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 <[email protected]>

---

drivers/scsi/sr.c +1 -1
@@ -468,7 +468,7 @@
 	}
 
 	ret = cdrom_ioctl(file, &cd->cdi, inode, cmd, arg);
-	if (ret != ENOSYS)
+	if (ret != -ENOSYS)
 		return ret;
 
 	/*
author
#1 lore raw

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 <[email protected]>

---
Allow ATA_12 / ATA_16 passthru commands to be issued for ATAPI devices

drivers/ata/libata-scsi.c +12 -15
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
@@ -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;
 }
author
#2 lore raw

PATCH 1/1 Add ATA_12/ATA_16 support for libata ATAPI

Resending with fixed Subject line:

Quoted 7 lines · 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 <[email protected]>

---
Allow ATA_12 / ATA_16 passthru commands to be issued for ATAPI devices

drivers/ata/libata-scsi.c +12 -15
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
@@ -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;
 }
author
#3 lore raw

PATCH 1/1 libata use ATA_12 in HDIO_* ioctls for ATAPI compatibility

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 <[email protected]>

---

drivers/ata/libata-scsi.c renamed from drivers/ata/libata-scsi.c.1 +16 -17
@@ -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... */
author
#4 lore raw

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

drivers/ata/libata-scsi.c

Quoted 11 lines · 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 <[email protected]>

---

drivers/ata/libata-scsi.c renamed from drivers/ata/libata-scsi.c.1 +0 -0

..

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

#5 lore raw
In reply to Mark Lord
Quoted 11 lines · 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 <[email protected]>

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

author
#6 lore raw
Quoted 1 line · Jeff Garzik

Mark Lord wrote:

..

Quoted 9 lines
Quoted 1 line

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

author
#7 lore raw
Quoted 4 lines · Mark Lord
Quoted 1 line · Jeff Garzik

Mark Lord wrote:

..

Quoted 1 line
Quoted 1 line

Allow ATA_12 / ATA_16 passthru commands to be issued for ATAPI devices

..

Quoted 1 line

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.

???

#8 lore raw
Quoted 10 lines · Mark Lord
Quoted 4 lines · Mark Lord
Quoted 1 line · Jeff Garzik

Mark Lord wrote:

..

Quoted 1 line
Quoted 1 line

Allow ATA_12 / ATA_16 passthru commands to be issued for ATAPI devices

..

Quoted 1 line

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

Quoted 4 lines

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

#9 lore raw
In reply to Mark Lord
Quoted 12 lines · 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 <[email protected]>

---

drivers/scsi/sr.c line 471

Quoted 5 lines
drivers/scsi/sr.c +1 -1
@@ -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

author
#10 lore raw
Quoted 1 line · Jeff Garzik

Mark Lord wrote:

...

Quoted 5 lines

drivers/scsi/sr.c line 471

Quoted 3 lines
     ret = cdrom_ioctl(file, &cd->cdi, inode, cmd, arg);
drivers/scsi/sr.c +1 -1
-    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!