Skip to contentSkip to conversation

fusion update to current APIs

PATCH In review Christoph Hellwig 8 participants 28 messages May 31, 2004 – Aug 30, 2004

Subsystems (3)
SCSI LSI Logic MPT fusion drivers (FC/SAS/SPI) SCSI SG driver
lore
Share & tools
lore

Christoph Hellwig

author
#0 lore raw

PATCH 1/1 fusion update to current APIs

- kill scsi_to_pci_dma_dir usage, both pci and scsi use the same bits
- kill mptscsih_io_direction and always trust the midlayer
- kill usage of Scsi_Foo typedefs
- use <scsi/*.h> headers

drivers/message/fusion/mptctl.c +2 -4
@@ -84,13 +84,11 @@
 #include <linux/miscdevice.h>
 #include <linux/smp_lock.h>
 
+#include <scsi/scsi_host.h>
+
 #include <asm/io.h>
 #include <asm/uaccess.h>
 
-#include <linux/kdev_t.h>	/* needed for access to Scsi_Host struct */
-#include <linux/blkdev.h>
-#include "../../scsi/scsi.h"
-#include "../../scsi/hosts.h"
 
 #define COPYRIGHT	"Copyright (c) 1999-2004 LSI Logic Corporation"
 #define MODULEAUTHOR	"Steven J. Ralston, Noah Romer, Pamela Delaney"

===== drivers/message/fusion/mptscsih.c 1.41 vs edited =====

drivers/message/fusion/mptscsih.c +59 -155

492 lines not on this page · raw message

#1 lore raw
Quoted 2 lines · Christoph Hellwig

- kill scsi_to_pci_dma_dir usage, both pci and scsi use the same bits
- kill mptscsih_io_direction and always trust the midlayer

Christoph, the midlayer gets it wrong sometimes.

Specifically, when the sg driver can't tell the direction, because the input and output buffer sizes are identical (and an older sg interface is being used), the sg driver assumes that the direction is inbound (SCSI_READ).

There are several apps out there that get that do this, but which work because the host drivers override the direction passed in from sg.

Now if we have to go out and get all these apps fixed, there will be a lot of pain out there :-)

jeremy

#2 lore raw
Quoted 1 line · Jeremy Higdon

Christoph, the midlayer gets it wrong sometimes.

... then the midlayer needs to get fixed rather than have all this code duplicated in a million different drivers with subtly different bugs in each one.

Signature · 6 lines
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain
#3 lore raw
Quoted 6 lines · Matthew Wilcox
Quoted 1 line · Jeremy Higdon

Christoph, the midlayer gets it wrong sometimes.

... then the midlayer needs to get fixed rather than have all this code duplicated in a million different drivers with subtly different bugs in each one.

That makes sense to me. In fact, I think we could just fix it in sg. It's only a problem with the older sg interfaces that don't specify a direction.

To review, the problem is that many sg1 and sg2 apps don't tightly trim the reply length. Thus, even though they're issuing a Write command, they will set the reply length to a value greater than SZ_SG_HEADER.

so in sg_write(), hp->dxfer_direction gets set to SG_DXFER_TO_FROM_DEV:

	if (input_size > 0)
                hp->dxfer_direction = (old_hdr.reply_len > SZ_SG_HEADER) ?
                    SG_DXFER_TO_FROM_DEV : SG_DXFER_TO_DEV;
        else
                hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : SG_DXFER_NONE;

then in sg_common_write, the data direction of the SCSI request is set to SCSI_DATA_READ:

        switch (hp->dxfer_direction) {
        case SG_DXFER_TO_FROM_DEV:
        case SG_DXFER_FROM_DEV:
                SRpnt->sr_data_direction = SCSI_DATA_READ;
                break;
        case SG_DXFER_TO_DEV:
                SRpnt->sr_data_direction = SCSI_DATA_WRITE;
                break;
        case SG_DXFER_UNKNOWN:
                SRpnt->sr_data_direction = SCSI_DATA_UNKNOWN;
                break;
        default:
                SRpnt->sr_data_direction = SCSI_DATA_NONE;
                break;
        }

Then when the host driver sends the Write command to the disk with the data direction set the other way, you usually get a SCSI bus reset.

So we should probably put the direction interpretation in sg_write() near the bottom just before calling sg_common_write().

Something like this, where sg_direction() does what the host drivers are currently doing. I haven't actually tried this yet, but I can code something up and give it a whirl if folks would like (I'm a little short on time tonight :-).

===== drivers/scsi/sg.c 1.90 vs edited =====

drivers/scsi/sg.c +1 -5
@@ -552,11 +552,6 @@
 	hp->cmd_len = (unsigned char) cmd_size;
 	hp->iovec_count = 0;
 	hp->mx_sb_len = 0;
-	if (input_size > 0)
-		hp->dxfer_direction = (old_hdr.reply_len > SZ_SG_HEADER) ?
-		    SG_DXFER_TO_FROM_DEV : SG_DXFER_TO_DEV;
-	else
-		hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : SG_DXFER_NONE;
 	hp->dxfer_len = mxsize;
 	hp->dxferp = (char __user *)buf + cmd_size;
 	hp->sbp = NULL;

@@ -566,6 +561,7 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	hp->dxfer_direction = sg_direction(cmnd);
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }

jeremy

#4 lore raw
Quoted 8 lines · Jeremy Higdon
Quoted 3 lines · Matthew Wilcox

... then the midlayer needs to get fixed rather than have all this code duplicated in a million different drivers with subtly different bugs in each one.

That makes sense to me. In fact, I think we could just fix it in sg. It's only a problem with the older sg interfaces that don't specify a direction.

[...]

Quoted 4 lines

Something like this, where sg_direction() does what the host drivers are currently doing. I haven't actually tried this yet, but I can code something up and give it a whirl if folks would like (I'm a little short on time tonight :-).

This looks like exactly the right approach to take.

Signature · 6 lines
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain
#5 lore raw

PATCH 1/1 sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

Okay, here's my proposed patch to sg.

If it's able to determine the direction based on the input and output buffer sizes, it uses those. If both input and output buffer sizes are non-zero, then it resorts to the command bytes. Folks familiar with the mptscsi driver will recognize the sg_direction function :-)

I've tested this using sg_utils, and it seems to work, but those are probably well-behaved. I will endeavor to test tomorrow with some RAID utilities that have not been well-behaved, in conjunction with removing the direction-override code from the host driver.

If this works, then we can remove the nasty direction code from all the host drivers.

jeremy

===== drivers/scsi/sg.c 1.90 vs edited =====

drivers/scsi/sg.c +67 -5
@@ -480,6 +480,61 @@
 	return (0 == err) ? count : err;
 }
 
+
+static int
+sg_direction(char *cmnd)
+{
+	switch (cmnd[0]) {
+	/*  _DATA_OUT commands	*/
+	case WRITE_6:		case WRITE_10:		case WRITE_12:
+	case WRITE_16:
+	case WRITE_LONG:	case WRITE_SAME:	case WRITE_BUFFER:
+	case WRITE_VERIFY:	case WRITE_VERIFY_12:
+	case COMPARE:		case COPY:		case COPY_VERIFY:
+	case SEARCH_EQUAL:	case SEARCH_HIGH:	case SEARCH_LOW:
+	case SEARCH_EQUAL_12:	case SEARCH_HIGH_12:	case SEARCH_LOW_12:
+	case MODE_SELECT:	case MODE_SELECT_10:	case LOG_SELECT:
+	case SEND_DIAGNOSTIC:	case CHANGE_DEFINITION: case UPDATE_BLOCK:
+	case SET_WINDOW:	case MEDIUM_SCAN:	case SEND_VOLUME_TAG:
+	case REASSIGN_BLOCKS:
+	case PERSISTENT_RESERVE_OUT:
+	case 0xea:
+	case 0xa3:
+		return SG_DXFER_TO_DEV;
+
+	/*  No data transfer commands  */
+	case SEEK_6:		case SEEK_10:
+	case RESERVE:		case RELEASE:
+	case TEST_UNIT_READY:
+	case START_STOP:
+	case ALLOW_MEDIUM_REMOVAL:
+		return SG_DXFER_NONE;
+
+	/*  Conditional data transfer commands	*/
+	case FORMAT_UNIT:
+		if (cmnd[1] & 0x10)	/* FmtData (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case VERIFY:
+		if (cmnd[1] & 0x02)	/* VERIFY:BYTCHK (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case RESERVE_10:
+		if (cmnd[1] & 0x03)  /* RESERVE:{LongID|Extent} (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	/*  Must be data _IN!  */
+	default:
+		return SG_DXFER_FROM_DEV;
+	}
+}
+
 static ssize_t
 sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 {

@@ -552,11 +607,6 @@
 	hp->cmd_len = (unsigned char) cmd_size;
 	hp->iovec_count = 0;
 	hp->mx_sb_len = 0;
-	if (input_size > 0)
-		hp->dxfer_direction = (old_hdr.reply_len > SZ_SG_HEADER) ?
-		    SG_DXFER_TO_FROM_DEV : SG_DXFER_TO_DEV;
-	else
-		hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : SG_DXFER_NONE;
 	hp->dxfer_len = mxsize;
 	hp->dxferp = (char __user *)buf + cmd_size;
 	hp->sbp = NULL;

@@ -566,6 +616,18 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * If data direction is indeterminate because both input and output size
+	 * are greater than 0, use command bytes to determine direction.
+	 */
+	if (input_size == 0 && mxsize > 0)
+		hp->dxfer_direction = SG_DXFER_FROM_DEV;
+	else if (input_size > 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_TO_DEV;
+	else if (input_size == 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_NONE;
+	else
+		hp->dxfer_direction = sg_direction(cmnd);
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }
#6 lore raw

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

Quoted 12 lines · Jeremy Higdon

Okay, here's my proposed patch to sg.

If it's able to determine the direction based on the input and output buffer sizes, it uses those. If both input and output buffer sizes are non-zero, then it resorts to the command bytes. Folks familiar with the mptscsi driver will recognize the sg_direction function :-)

I've tested this using sg_utils, and it seems to work, but those are probably well-behaved. I will endeavor to test tomorrow with some RAID utilities that have not been well-behaved, in conjunction with removing the direction-override code from the host driver.

Quoted 4 lines

If this works, then we can remove the nasty direction code from all the host drivers.

jeremy

So I guess this patch only applies to sg_header usage since the users of sg_io_hdr (including SG_IO ioctl users) must explicitly give the data direction.

Quoted 16 lines

===== drivers/scsi/sg.c 1.90 vs edited =====

drivers/scsi/sg.c +9 -0
@@ -480,6 +480,61 @@
 	return (0 == err) ? count : err;
 }
 
+
+static int
+sg_direction(char *cmnd)
+{
+	switch (cmnd[0]) {
+	/*  _DATA_OUT commands	*/
+	case WRITE_6:		case WRITE_10:		case WRITE_12:
+	case WRITE_16:
+	case WRITE_LONG:	case WRITE_SAME:	case WRITE_BUFFER:

drivers/scsi/sg.c line 501

Quoted 10 lines
drivers/scsi/sg.c +10 -0
+	case WRITE_VERIFY:	case WRITE_VERIFY_12:
+	case COMPARE:		case COPY:		case COPY_VERIFY:
+	case SEARCH_EQUAL:	case SEARCH_HIGH:	case SEARCH_LOW:
+	case SEARCH_EQUAL_12:	case SEARCH_HIGH_12:	case SEARCH_LOW_12:
+	case MODE_SELECT:	case MODE_SELECT_10:	case LOG_SELECT:
+	case SEND_DIAGNOSTIC:	case CHANGE_DEFINITION: case UPDATE_BLOCK:
+	case SET_WINDOW:	case MEDIUM_SCAN:	case SEND_VOLUME_TAG:
+	case REASSIGN_BLOCKS:
+	case PERSISTENT_RESERVE_OUT:
+	case 0xea:

Perhaps you might like to tell us what the 0xea vendor specific command is (for the record)?

drivers/scsi/sg.c line 502

Quoted 1 line
drivers/scsi/sg.c +1 -0
+	case 0xa3:

and 0xa3 is MAINTENANCE IN which would be ..._FROM_DEV . Did you mean 0xa4 (MAINTENANCE OUT)?

Quoted 60 lines
drivers/scsi/sg.c +41 -5
+		return SG_DXFER_TO_DEV;
+
+	/*  No data transfer commands  */
+	case SEEK_6:		case SEEK_10:
+	case RESERVE:		case RELEASE:
+	case TEST_UNIT_READY:
+	case START_STOP:
+	case ALLOW_MEDIUM_REMOVAL:
+		return SG_DXFER_NONE;
+
+	/*  Conditional data transfer commands	*/
+	case FORMAT_UNIT:
+		if (cmnd[1] & 0x10)	/* FmtData (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case VERIFY:
+		if (cmnd[1] & 0x02)	/* VERIFY:BYTCHK (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case RESERVE_10:
+		if (cmnd[1] & 0x03)  /* RESERVE:{LongID|Extent} (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	/*  Must be data _IN!  */
+	default:
+		return SG_DXFER_FROM_DEV;
+	}
+}
+
 static ssize_t
 sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 {

@@ -552,11 +607,6 @@
 	hp->cmd_len = (unsigned char) cmd_size;
 	hp->iovec_count = 0;
 	hp->mx_sb_len = 0;
-	if (input_size > 0)
-		hp->dxfer_direction = (old_hdr.reply_len > SZ_SG_HEADER) ?
-		    SG_DXFER_TO_FROM_DEV : SG_DXFER_TO_DEV;
-	else
-		hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : SG_DXFER_NONE;
 	hp->dxfer_len = mxsize;
 	hp->dxferp = (char __user *)buf + cmd_size;
 	hp->sbp = NULL;

@@ -566,6 +616,18 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * If data direction is indeterminate because both input and output size
+	 * are greater than 0, use command bytes to determine direction.
+	 */
+	if (input_size == 0 && mxsize > 0)
+		hp->dxfer_direction = SG_DXFER_FROM_DEV;

drivers/scsi/sg.c

Quoted 10 lines
drivers/scsi/sg.c +6 -1
+	else if (input_size > 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_TO_DEV;
+	else if (input_size == 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_NONE;
+	else
+		hp->dxfer_direction = sg_direction(cmnd);
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }
-

Doug Gilbert

#7 lore raw

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

Quoted 4 lines · Douglas Gilbert

So I guess this patch only applies to sg_header usage since the users of sg_io_hdr (including SG_IO ioctl users) must explicitly give the data direction.

Correct. Several apps declare a response length greater than the absolute minimum, not realizing that this confuses sg, causing direction to get set incorrectly, and requiring the host drivers to have their own tables.

Quoted 13 lines
Quoted 13 lines
drivers/scsi/sg.c +13 -0
+static int
+sg_direction(char *cmnd)
+{
+	switch (cmnd[0]) {
+	/*  _DATA_OUT commands	*/
+	case WRITE_6:		case WRITE_10:		case WRITE_12:
+	case WRITE_16:
+	case WRITE_LONG:	case WRITE_SAME:	case WRITE_BUFFER:
+	case WRITE_VERIFY:	case WRITE_VERIFY_12:
+	case COMPARE:		case COPY:		case COPY_VERIFY:
+	case SEARCH_EQUAL:	case SEARCH_HIGH:	case SEARCH_LOW:
+	case SEARCH_EQUAL_12:	case SEARCH_HIGH_12:	case SEARCH_LOW_12:
+	case MODE_SELECT:	case MODE_SELECT_10:	case LOG_SELECT:
Quoted 8 lines

drivers/scsi/sg.c line 501

Quoted 5 lines
drivers/scsi/sg.c +5 -0
+	case SEND_DIAGNOSTIC:	case CHANGE_DEFINITION: case UPDATE_BLOCK:
+	case SET_WINDOW:	case MEDIUM_SCAN:	case SEND_VOLUME_TAG:
+	case REASSIGN_BLOCKS:
+	case PERSISTENT_RESERVE_OUT:
+	case 0xea:

Perhaps you might like to tell us what the 0xea vendor specific command is (for the record)?

I would like to, but I don't know any more than you do.

Quoted 4 lines

drivers/scsi/sg.c line 502

Quoted 1 line
drivers/scsi/sg.c +1 -0
+	case 0xa3:

and 0xa3 is MAINTENANCE IN which would be ..._FROM_DEV . Did you mean 0xa4 (MAINTENANCE OUT)?

The table is mostly copied from drivers/message/fusion/mptscsih.c. It was presumably developed through experience, though I have no firsthand (or secondhand) knowledge of that.

jeremy

#8 lore raw

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

Quoted 11 lines · Jeremy Higdon
Quoted 3 lines · Douglas Gilbert

So I guess this patch only applies to sg_header usage since the users of sg_io_hdr (including SG_IO ioctl users) must explicitly give the data direction.

Correct. Several apps declare a response length greater than the absolute minimum, not realizing that this confuses sg, causing direction to get set incorrectly, and requiring the host drivers to have their own tables.

We're still at "hacking the kernel to fix broken apps" :(:(

Jeff

#9 lore raw

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

Quoted 14 lines · Jeff Garzik
Quoted 11 lines · Jeremy Higdon
Quoted 3 lines · Douglas Gilbert

So I guess this patch only applies to sg_header usage since the users of sg_io_hdr (including SG_IO ioctl users) must explicitly give the data direction.

Correct. Several apps declare a response length greater than the absolute minimum, not realizing that this confuses sg, causing direction to get set incorrectly, and requiring the host drivers to have their own tables.

Quoted 3 lines

We're still at "hacking the kernel to fix broken apps" :(:(

Jeff

Okay. How about this then. At least it will fail in a noisy way, so that host adapter driver maintainers will not be pressured to "fix" their drivers (which is how the direction override code ended up in qlogic and mpt in the first place).

===== drivers/scsi/sg.c 1.90 vs edited =====

drivers/scsi/sg.c +13 -3
@@ -552,9 +552,19 @@
 	hp->cmd_len = (unsigned char) cmd_size;
 	hp->iovec_count = 0;
 	hp->mx_sb_len = 0;
-	if (input_size > 0)
-		hp->dxfer_direction = (old_hdr.reply_len > SZ_SG_HEADER) ?
-		    SG_DXFER_TO_FROM_DEV : SG_DXFER_TO_DEV;
+	if (input_size > 0) {	/* data is being sent to device */
+		if (old_hdr.reply_len <= SZ_SG_HEADER)
+			hp->dxfer_direction = SG_DXFER_TO_DEV;
+		else {
+			printk("sg_write: cannot infer direction from count %ld "
+			       "and reply_len %d.  Command 0x%x will probably fail. "
+			       "Please fix the application to use proper counts or "
+			       "the sg version 3 interface\n",
+			       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
+			/* SG_DXFER_TO_FROM_DEV is equivalent to SG_DXFER_FROM_DEV */
+			hp->dxfer_direction = SG_DXFER_TO_FROM_DEV;
+		}
+	}
 	else
 		hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : SG_DXFER_NONE;
 	hp->dxfer_len = mxsize;
#10 lore raw

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

Quoted 4 lines · Jeremy Higdon

Okay. How about this then. At least it will fail in a noisy way, so that host adapter driver maintainers will not be pressured to "fix" their drivers (which is how the direction override code ended up in qlogic and mpt in the first place).

ok with me :)

#11 lore raw

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

In reply to Jeremy Higdon
Quoted 5 lines · Jeremy Higdon

Okay. How about this then. At least it will fail in a noisy way, so that host adapter driver maintainers will not be pressured to "fix" their drivers (which is how the direction override code ended up in qlogic and mpt in the first place).

Sorry -- I sent that one out too quickly -- it uses cmnd[0] before it is set. This patch is smaller and more correct.

Does anyone have objections to this? I don't think we need to ratelimit these printks, because whatever is causing them is likely experiencing I/O errors and will probably give up.

===== drivers/scsi/sg.c 1.90 vs edited =====

drivers/scsi/sg.c +11 -0
@@ -566,6 +566,17 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * SG_DXFER_TO_FROM_DEV is functionally equivalent to SG_DXFER_FROM_DEV,
+	 * but is is possible that the app intended SG_DXFER_TO_DEV, because there
+	 * is a non-zero input_size, so emit a warning.
+	 */
+	if (hp->dxfer_direction == SG_DXFER_TO_FROM_DEV)
+		printk("sg_write: cannot infer direction from count %ld "
+		       "and reply_len %d.  Command 0x%x will probably fail. "
+		       "Please fix the application to use proper counts or "
+		       "the sg version 3 interface\n",
+		       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }
#12 lore raw

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

I still think that this change should go into the sg1/sg2 command issue path, so that a warning is issued when sg has to guess on command direction. This will encourage developers to fix up apps that don't set buffer sizes properly.

Doug, can you comment on this? If you don't like it, I'll drop it :-)

This patch is against 2.6.9-rc1.

thanks,

jeremy

===== drivers/scsi/sg.c 1.95 vs edited =====

drivers/scsi/sg.c +11 -0
@@ -564,6 +564,17 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * SG_DXFER_TO_FROM_DEV is functionally equivalent to SG_DXFER_FROM_DEV,
+	 * but is is possible that the app intended SG_DXFER_TO_DEV, because there
+	 * is a non-zero input_size, so emit a warning.
+	 */
+	if (hp->dxfer_direction == SG_DXFER_TO_FROM_DEV)
+		printk("sg_write: cannot infer direction from count %ld "
+		       "and reply_len %d.  Command 0x%x will probably fail. "
+		       "Please fix the application to use proper counts or "
+		       "the sg version 3 interface\n",
+		       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }
#13 lore raw

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

Quoted 26 lines · Jeremy Higdon

I still think that this change should go into the sg1/sg2 command issue path, so that a warning is issued when sg has to guess on command direction. This will encourage developers to fix up apps that don't set buffer sizes properly.

Doug, can you comment on this? If you don't like it, I'll drop it :-)

This patch is against 2.6.9-rc1.

thanks,

jeremy

===== drivers/scsi/sg.c 1.95 vs edited =====

drivers/scsi/sg.c +4 -0
@@ -564,6 +564,17 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * SG_DXFER_TO_FROM_DEV is functionally equivalent to SG_DXFER_FROM_DEV,
+	 * but is is possible that the app intended SG_DXFER_TO_DEV, because there
+	 * is a non-zero input_size, so emit a warning.

drivers/scsi/sg.c line 580

Quoted 10 lines
drivers/scsi/sg.c +7 -0
+	 */
+	if (hp->dxfer_direction == SG_DXFER_TO_FROM_DEV)
+		printk("sg_write: cannot infer direction from count %ld "
+		       "and reply_len %d.  Command 0x%x will probably fail. "
+		       "Please fix the application to use proper counts or "
+		       "the sg version 3 interface\n",
+		       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }

Jeremy,
Perhaps the first argument of printk() could be prefixed with KERN_WARNING to make it a bit more syslog friendly. Otherwise ok.

Doug Gilbert

#14 lore raw

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

Quoted 6 lines · Douglas Gilbert

Jeremy,
Perhaps the first argument of printk() could be prefixed with KERN_WARNING to make it a bit more syslog friendly. Otherwise ok.

Doug Gilbert

You're right. It's fixed in this patch.
James, if you're satisfied, please apply.

thanks,
jeremy

Signed-off-by [email protected]

===== drivers/scsi/sg.c 1.95 vs edited =====

drivers/scsi/sg.c +12 -0
@@ -564,6 +564,18 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * SG_DXFER_TO_FROM_DEV is functionally equivalent to SG_DXFER_FROM_DEV,
+	 * but is is possible that the app intended SG_DXFER_TO_DEV, because there
+	 * is a non-zero input_size, so emit a warning.
+	 */
+	if (hp->dxfer_direction == SG_DXFER_TO_FROM_DEV)
+		printk(KERN_WARNING
+		       "sg_write: cannot infer direction from count %ld "
+		       "and reply_len %d.  Command 0x%x will probably fail. "
+		       "Please fix the application to use proper counts or "
+		       "the sg version 3 interface\n",
+		       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }
#15 lore raw

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

Quoted 10 lines · Jeremy Higdon
Quoted 6 lines · Douglas Gilbert

Jeremy,
Perhaps the first argument of printk() could be prefixed with KERN_WARNING to make it a bit more syslog friendly. Otherwise ok.

Doug Gilbert

You're right. It's fixed in this patch.
James, if you're satisfied, please apply.

This is one of those days when I seem to be working against myself. It turns out that our RAID vendor modified their agent to trim the "reply_len" field for write commands, but did not trim the "count" field for read commands. So in this case, sg actually guesses correctly when it needs to, and the app works. Only with this patch, /var/log/messages gets filled with this new error message.

Doug, what do you think is the right thing to do?

jeremy

Quoted 27 lines

thanks,
jeremy

Signed-off-by [email protected]

===== drivers/scsi/sg.c 1.95 vs edited =====

drivers/scsi/sg.c +12 -0
@@ -564,6 +564,18 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * SG_DXFER_TO_FROM_DEV is functionally equivalent to SG_DXFER_FROM_DEV,
+	 * but is is possible that the app intended SG_DXFER_TO_DEV, because there
+	 * is a non-zero input_size, so emit a warning.
+	 */
+	if (hp->dxfer_direction == SG_DXFER_TO_FROM_DEV)
+		printk(KERN_WARNING
+		       "sg_write: cannot infer direction from count %ld "
+		       "and reply_len %d.  Command 0x%x will probably fail. "
+		       "Please fix the application to use proper counts or "
+		       "the sg version 3 interface\n",
+		       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }
#16 lore raw

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

Quoted 23 lines · Jeremy Higdon
Quoted 11 lines · Jeremy Higdon
Quoted 6 lines · Douglas Gilbert

Jeremy,
Perhaps the first argument of printk() could be prefixed with KERN_WARNING to make it a bit more syslog friendly. Otherwise ok.

Doug Gilbert

You're right. It's fixed in this patch.
James, if you're satisfied, please apply.

This is one of those days when I seem to be working against myself. It turns out that our RAID vendor modified their agent to trim the "reply_len" field for write commands, but did not trim the "count" field for read commands. So in this case, sg actually guesses correctly when it needs to, and the app works. Only with this patch, /var/log/messages gets filled with this new error message.

Quoted 1 line

Doug, what do you think is the right thing to do?

Jeremy,
You could use a (block scope) static and only print out the warning the first time it is detected.

Doug Gilbert

#17 lore raw

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

Quoted 12 lines · Douglas Gilbert
Quoted 11 lines · Jeremy Higdon

On Thu, Aug 26, 2004 at 06:12:53PM -0700, Jeremy Higdon wrote:

This is one of those days when I seem to be working against myself. It turns out that our RAID vendor modified their agent to trim the "reply_len" field for write commands, but did not trim the "count" field for read commands. So in this case, sg actually guesses correctly when it needs to, and the app works. Only with this patch, /var/log/messages gets filled with this new error message.

Quoted 7 lines
Quoted 1 line

Doug, what do you think is the right thing to do?

Jeremy,
You could use a (block scope) static and only print out the warning the first time it is detected.

Doug Gilbert

Yes, that's a good idea -- I'll add a rate limiter and also print out the process name like James does in his recent deprecated ioctl warning patch. I'll send a new patch out later this weekend. Sorry for the churn.

thanks

jeremy

#18 lore raw

Re: [PATCH] sg.c to warn about ambiguous data direction

Quoted 6 lines · Jeremy Higdon
Quoted 5 lines

Jeremy,
You could use a (block scope) static and only print out the warning the first time it is detected.

Doug Gilbert

Quoted 8 lines

Yes, that's a good idea -- I'll add a rate limiter and also print out the process name like James does in his recent deprecated ioctl warning patch. I'll send a new patch out later this weekend. Sorry for the churn.

thanks

jeremy

Okay, I'm pretty happy with this. I get this kind of output:

sg_write: data in/out 512/512 bytes for SCSI command 0x8--guessing data in;
program java not setting count and/or reply_len properly
printk: 11 messages suppressed.
sg_write: data in/out 512/512 bytes for SCSI command 0x8--guessing data in;
program java not setting count and/or reply_len properly
printk: 4 messages suppressed.

Signed-off-by [email protected]

===== drivers/scsi/sg.c 1.95 vs edited =====

drivers/scsi/sg.c +13 -0
@@ -564,6 +564,19 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * SG_DXFER_TO_FROM_DEV is functionally equivalent to SG_DXFER_FROM_DEV,
+	 * but is is possible that the app intended SG_DXFER_TO_DEV, because there
+	 * is a non-zero input_size, so emit a warning.
+	 */
+	if (hp->dxfer_direction == SG_DXFER_TO_FROM_DEV)
+		if (printk_ratelimit())
+			printk(KERN_WARNING
+			       "sg_write: data in/out %d/%d bytes for SCSI command 0x%x--"
+			       "guessing data in;\n" KERN_WARNING "   "
+			       "program %s not setting count and/or reply_len properly\n",
+			       old_hdr.reply_len - SZ_SG_HEADER, input_size,
+			       (unsigned int) cmnd[0], current->comm);
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }
#19 lore raw

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

In reply to Jeremy Higdon
Quoted 43 lines · Jeremy Higdon
drivers/scsi/sg.c +43 -0
+static int
+sg_direction(char *cmnd)
+{
+	switch (cmnd[0]) {
+	/*  _DATA_OUT commands	*/
+	case WRITE_6:		case WRITE_10:		case WRITE_12:
+	case WRITE_16:
+	case WRITE_LONG:	case WRITE_SAME:	case WRITE_BUFFER:
+	case WRITE_VERIFY:	case WRITE_VERIFY_12:
+	case COMPARE:		case COPY:		case COPY_VERIFY:
+	case SEARCH_EQUAL:	case SEARCH_HIGH:	case SEARCH_LOW:
+	case SEARCH_EQUAL_12:	case SEARCH_HIGH_12:	case SEARCH_LOW_12:
+	case MODE_SELECT:	case MODE_SELECT_10:	case LOG_SELECT:
+	case SEND_DIAGNOSTIC:	case CHANGE_DEFINITION: case UPDATE_BLOCK:
+	case SET_WINDOW:	case MEDIUM_SCAN:	case SEND_VOLUME_TAG:
+	case REASSIGN_BLOCKS:
+	case PERSISTENT_RESERVE_OUT:
+	case 0xea:
+	case 0xa3:
+		return SG_DXFER_TO_DEV;
+
+	/*  No data transfer commands  */
+	case SEEK_6:		case SEEK_10:
+	case RESERVE:		case RELEASE:
+	case TEST_UNIT_READY:
+	case START_STOP:
+	case ALLOW_MEDIUM_REMOVAL:
+		return SG_DXFER_NONE;
+
+	/*  Conditional data transfer commands	*/
+	case FORMAT_UNIT:
+		if (cmnd[1] & 0x10)	/* FmtData (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case VERIFY:
+		if (cmnd[1] & 0x02)	/* VERIFY:BYTCHK (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case RESERVE_10:

drivers/scsi/sg.c line 536

Quoted 10 lines
drivers/scsi/sg.c +10 -0
+		if (cmnd[1] & 0x03)  /* RESERVE:{LongID|Extent} (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	/*  Must be data _IN!  */
+	default:
+		return SG_DXFER_FROM_DEV;
+	}
+}

This is definitely moving backwards from the direction we want to go.

The entity creating the SCSI command needs to specify data direction, _not_ tables hardcoded into the kernel. Bart recently removed such a table from IDE.

These tables are fundamentally broken anyway, due to vendor-reserved commands where the data directions are not specified, but simply "known" by the submittor.

If /dev/sg's userland interface does not permit userland to provide the data direction, then let's just consider it broken and move to Axboe's bsg post-haste.

Jeff

#20 lore raw

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

Quoted 2 lines · Jeff Garzik

This is definitely moving backwards from the direction we want to go.

You really need to look at this in the context of the original thread.

Quoted 3 lines

The entity creating the SCSI command needs to specify data direction, _not_ tables hardcoded into the kernel. Bart recently removed such a table from IDE.

Yes. In this case, sg is the entity. This table causes sg to get it right much more often than it did.

Quoted 3 lines

These tables are fundamentally broken anyway, due to vendor-reserved commands where the data directions are not specified, but simply "known" by the submittor.

If the reply length is trimmed to the minimum, it will still work as well as it does today.

Quoted 3 lines

If /dev/sg's userland interface does not permit userland to provide the data direction, then let's just consider it broken and move to Axboe's bsg post-haste.

If you look at Documentation/scsi/scsi-generic.txt and then refer to the v1 and v2 sg requests, you'll notice that you can't find a way to specify direction. Sg tries to figure it out, but often gets it wrong. Note that in the v3 sg interface, data direction is explicitly specified (as it should have been originally, imho).

For those new to the thread, the idea is to remove the direction overrides from various host drivers and centralize them in sg. If we remove the direction overrides from the host drivers and do not add this code to sg, then several apps that use the sg v1 and v2 interfaces will suddenly break.

jeremy

#21 lore raw

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

Quoted 6 lines

For those new to the thread, the idea is to remove the direction overrides from various host drivers and centralize them in sg. If we remove the direction overrides from the host drivers and do not add this code to sg, then several apps that use the sg v1 and v2 interfaces will suddenly break.

can we add a printk to mark the v1 and v2 interfaces deprecated please ?

#22 lore raw

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

Quoted 2 lines · Arjan van de Ven

can we add a printk to mark the v1 and v2 interfaces deprecated please ?

New patch with a printk:

===== drivers/scsi/sg.c 1.90 vs edited =====

drivers/scsi/sg.c +71 -5
@@ -480,6 +480,61 @@
 	return (0 == err) ? count : err;
 }
 
+
+static int
+sg_direction(char *cmnd)
+{
+	switch (cmnd[0]) {
+	/*  _DATA_OUT commands	*/
+	case WRITE_6:		case WRITE_10:		case WRITE_12:
+	case WRITE_16:
+	case WRITE_LONG:	case WRITE_SAME:	case WRITE_BUFFER:
+	case WRITE_VERIFY:	case WRITE_VERIFY_12:
+	case COMPARE:		case COPY:		case COPY_VERIFY:
+	case SEARCH_EQUAL:	case SEARCH_HIGH:	case SEARCH_LOW:
+	case SEARCH_EQUAL_12:	case SEARCH_HIGH_12:	case SEARCH_LOW_12:
+	case MODE_SELECT:	case MODE_SELECT_10:	case LOG_SELECT:
+	case SEND_DIAGNOSTIC:	case CHANGE_DEFINITION: case UPDATE_BLOCK:
+	case SET_WINDOW:	case MEDIUM_SCAN:	case SEND_VOLUME_TAG:
+	case REASSIGN_BLOCKS:
+	case PERSISTENT_RESERVE_OUT:
+	case 0xea:
+	case 0xa3:
+		return SG_DXFER_TO_DEV;
+
+	/*  No data transfer commands  */
+	case SEEK_6:		case SEEK_10:
+	case RESERVE:		case RELEASE:
+	case TEST_UNIT_READY:
+	case START_STOP:
+	case ALLOW_MEDIUM_REMOVAL:
+		return SG_DXFER_NONE;
+
+	/*  Conditional data transfer commands	*/
+	case FORMAT_UNIT:
+		if (cmnd[1] & 0x10)	/* FmtData (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case VERIFY:
+		if (cmnd[1] & 0x02)	/* VERIFY:BYTCHK (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case RESERVE_10:
+		if (cmnd[1] & 0x03)  /* RESERVE:{LongID|Extent} (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	/*  Must be data _IN!  */
+	default:
+		return SG_DXFER_FROM_DEV;
+	}
+}
+
 static ssize_t
 sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 {

@@ -552,11 +607,6 @@
 	hp->cmd_len = (unsigned char) cmd_size;
 	hp->iovec_count = 0;
 	hp->mx_sb_len = 0;
-	if (input_size > 0)
-		hp->dxfer_direction = (old_hdr.reply_len > SZ_SG_HEADER) ?
-		    SG_DXFER_TO_FROM_DEV : SG_DXFER_TO_DEV;
-	else
-		hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : SG_DXFER_NONE;
 	hp->dxfer_len = mxsize;
 	hp->dxferp = (char __user *)buf + cmd_size;
 	hp->sbp = NULL;

@@ -566,6 +616,22 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * If data direction is indeterminate because both input and output size
+	 * are greater than 0, use command bytes to determine direction.
+	 */
+	if (input_size == 0 && mxsize > 0)
+		hp->dxfer_direction = SG_DXFER_FROM_DEV;
+	else if (input_size > 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_TO_DEV;
+	else if (input_size == 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_NONE;
+	else {
+		hp->dxfer_direction = sg_direction(cmnd);
+		printk("sg_write: cannot infer direction from count"
+		       " %ld and reply_len %d; inferring from command 0x%x\n",
+		       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
+	}
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }
#23 lore raw

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

In reply to Jeremy Higdon

Not too happy about working around broken apps in the kernel. At least remove vendor-specific commands here and add a big printk about the app writer beeing stupid when it detects a mismatch, or even better return EINVAL for that case.

Quoted 97 lines · Jeremy Higdon

===== drivers/scsi/sg.c 1.90 vs edited =====

drivers/scsi/sg.c +67 -6
@@ -480,6 +480,61 @@
 	return (0 == err) ? count : err;
 }
 
+
+static int
+sg_direction(char *cmnd)
+{
+	switch (cmnd[0]) {
+	/*  _DATA_OUT commands	*/
+	case WRITE_6:		case WRITE_10:		case WRITE_12:
+	case WRITE_16:
+	case WRITE_LONG:	case WRITE_SAME:	case WRITE_BUFFER:
+	case WRITE_VERIFY:	case WRITE_VERIFY_12:
+	case COMPARE:		case COPY:		case COPY_VERIFY:
+	case SEARCH_EQUAL:	case SEARCH_HIGH:	case SEARCH_LOW:
+	case SEARCH_EQUAL_12:	case SEARCH_HIGH_12:	case SEARCH_LOW_12:
+	case MODE_SELECT:	case MODE_SELECT_10:	case LOG_SELECT:
+	case SEND_DIAGNOSTIC:	case CHANGE_DEFINITION: case UPDATE_BLOCK:
+	case SET_WINDOW:	case MEDIUM_SCAN:	case SEND_VOLUME_TAG:
+	case REASSIGN_BLOCKS:
+	case PERSISTENT_RESERVE_OUT:
+	case 0xea:
+	case 0xa3:
+		return SG_DXFER_TO_DEV;
+
+	/*  No data transfer commands  */
+	case SEEK_6:		case SEEK_10:
+	case RESERVE:		case RELEASE:
+	case TEST_UNIT_READY:
+	case START_STOP:
+	case ALLOW_MEDIUM_REMOVAL:
+		return SG_DXFER_NONE;
+
+	/*  Conditional data transfer commands	*/
+	case FORMAT_UNIT:
+		if (cmnd[1] & 0x10)	/* FmtData (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case VERIFY:
+		if (cmnd[1] & 0x02)	/* VERIFY:BYTCHK (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case RESERVE_10:
+		if (cmnd[1] & 0x03)  /* RESERVE:{LongID|Extent} (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	/*  Must be data _IN!  */
+	default:
+		return SG_DXFER_FROM_DEV;
+	}
+}
+
 static ssize_t
 sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 {

@@ -552,11 +607,6 @@
 	hp->cmd_len = (unsigned char) cmd_size;
 	hp->iovec_count = 0;
 	hp->mx_sb_len = 0;
-	if (input_size > 0)
-		hp->dxfer_direction = (old_hdr.reply_len > SZ_SG_HEADER) ?
-		    SG_DXFER_TO_FROM_DEV : SG_DXFER_TO_DEV;
-	else
-		hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : SG_DXFER_NONE;
 	hp->dxfer_len = mxsize;
 	hp->dxferp = (char __user *)buf + cmd_size;
 	hp->sbp = NULL;

@@ -566,6 +616,18 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * If data direction is indeterminate because both input and output size
+	 * are greater than 0, use command bytes to determine direction.
+	 */
+	if (input_size == 0 && mxsize > 0)
+		hp->dxfer_direction = SG_DXFER_FROM_DEV;
+	else if (input_size > 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_TO_DEV;
+	else if (input_size == 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_NONE;
+	else
+		hp->dxfer_direction = sg_direction(cmnd);
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }
-
Quoted 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

---end quoted text---

#24 lore raw

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

Christoph wants a more explicit message.

===== drivers/scsi/sg.c 1.90 vs edited =====

drivers/scsi/sg.c +72 -5
@@ -480,6 +480,61 @@
 	return (0 == err) ? count : err;
 }
 
+
+static int
+sg_direction(char *cmnd)
+{
+	switch (cmnd[0]) {
+	/*  _DATA_OUT commands	*/
+	case WRITE_6:		case WRITE_10:		case WRITE_12:
+	case WRITE_16:
+	case WRITE_LONG:	case WRITE_SAME:	case WRITE_BUFFER:
+	case WRITE_VERIFY:	case WRITE_VERIFY_12:
+	case COMPARE:		case COPY:		case COPY_VERIFY:
+	case SEARCH_EQUAL:	case SEARCH_HIGH:	case SEARCH_LOW:
+	case SEARCH_EQUAL_12:	case SEARCH_HIGH_12:	case SEARCH_LOW_12:
+	case MODE_SELECT:	case MODE_SELECT_10:	case LOG_SELECT:
+	case SEND_DIAGNOSTIC:	case CHANGE_DEFINITION: case UPDATE_BLOCK:
+	case SET_WINDOW:	case MEDIUM_SCAN:	case SEND_VOLUME_TAG:
+	case REASSIGN_BLOCKS:
+	case PERSISTENT_RESERVE_OUT:
+	case 0xea:
+	case 0xa3:
+		return SG_DXFER_TO_DEV;
+
+	/*  No data transfer commands  */
+	case SEEK_6:		case SEEK_10:
+	case RESERVE:		case RELEASE:
+	case TEST_UNIT_READY:
+	case START_STOP:
+	case ALLOW_MEDIUM_REMOVAL:
+		return SG_DXFER_NONE;
+
+	/*  Conditional data transfer commands	*/
+	case FORMAT_UNIT:
+		if (cmnd[1] & 0x10)	/* FmtData (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case VERIFY:
+		if (cmnd[1] & 0x02)	/* VERIFY:BYTCHK (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case RESERVE_10:
+		if (cmnd[1] & 0x03)  /* RESERVE:{LongID|Extent} (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	/*  Must be data _IN!  */
+	default:
+		return SG_DXFER_FROM_DEV;
+	}
+}
+
 static ssize_t
 sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 {

@@ -552,11 +607,6 @@
 	hp->cmd_len = (unsigned char) cmd_size;
 	hp->iovec_count = 0;
 	hp->mx_sb_len = 0;
-	if (input_size > 0)
-		hp->dxfer_direction = (old_hdr.reply_len > SZ_SG_HEADER) ?
-		    SG_DXFER_TO_FROM_DEV : SG_DXFER_TO_DEV;
-	else
-		hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : SG_DXFER_NONE;
 	hp->dxfer_len = mxsize;
 	hp->dxferp = (char __user *)buf + cmd_size;
 	hp->sbp = NULL;

@@ -566,6 +616,23 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * If data direction is indeterminate because both input and output size
+	 * are greater than 0, use command bytes to determine direction.
+	 */
+	if (input_size == 0 && mxsize > 0)
+		hp->dxfer_direction = SG_DXFER_FROM_DEV;
+	else if (input_size > 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_TO_DEV;
+	else if (input_size == 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_NONE;
+	else {
+		hp->dxfer_direction = sg_direction(cmnd);
+		printk("sg_write: cannot infer direction from count %ld "
+		       "and reply_len %d; inferring from command 0x%x.  Please "
+		       "fix the application to trim count and reply_len\n",
+		       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
+	}
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }
#25 lore raw

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

One more comment from hch via IRC. When we have to guess direction, print a suggestion to use SG v3 interface.

I also removed the vendor unique commands from the switch.

jeremy

===== drivers/scsi/sg.c 1.90 vs edited =====

drivers/scsi/sg.c +70 -5
@@ -480,6 +480,59 @@
 	return (0 == err) ? count : err;
 }
 
+
+static int
+sg_direction(char *cmnd)
+{
+	switch (cmnd[0]) {
+	/*  _DATA_OUT commands	*/
+	case WRITE_6:		case WRITE_10:		case WRITE_12:
+	case WRITE_16:
+	case WRITE_LONG:	case WRITE_SAME:	case WRITE_BUFFER:
+	case WRITE_VERIFY:	case WRITE_VERIFY_12:
+	case COMPARE:		case COPY:		case COPY_VERIFY:
+	case SEARCH_EQUAL:	case SEARCH_HIGH:	case SEARCH_LOW:
+	case SEARCH_EQUAL_12:	case SEARCH_HIGH_12:	case SEARCH_LOW_12:
+	case MODE_SELECT:	case MODE_SELECT_10:	case LOG_SELECT:
+	case SEND_DIAGNOSTIC:	case CHANGE_DEFINITION: case UPDATE_BLOCK:
+	case SET_WINDOW:	case MEDIUM_SCAN:	case SEND_VOLUME_TAG:
+	case REASSIGN_BLOCKS:
+	case PERSISTENT_RESERVE_OUT:
+		return SG_DXFER_TO_DEV;
+
+	/*  No data transfer commands  */
+	case SEEK_6:		case SEEK_10:
+	case RESERVE:		case RELEASE:
+	case TEST_UNIT_READY:
+	case START_STOP:
+	case ALLOW_MEDIUM_REMOVAL:
+		return SG_DXFER_NONE;
+
+	/*  Conditional data transfer commands	*/
+	case FORMAT_UNIT:
+		if (cmnd[1] & 0x10)	/* FmtData (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case VERIFY:
+		if (cmnd[1] & 0x02)	/* VERIFY:BYTCHK (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case RESERVE_10:
+		if (cmnd[1] & 0x03)  /* RESERVE:{LongID|Extent} (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	/*  Must be data _IN!  */
+	default:
+		return SG_DXFER_FROM_DEV;
+	}
+}
+
 static ssize_t
 sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 {

@@ -552,11 +605,6 @@
 	hp->cmd_len = (unsigned char) cmd_size;
 	hp->iovec_count = 0;
 	hp->mx_sb_len = 0;
-	if (input_size > 0)
-		hp->dxfer_direction = (old_hdr.reply_len > SZ_SG_HEADER) ?
-		    SG_DXFER_TO_FROM_DEV : SG_DXFER_TO_DEV;
-	else
-		hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : SG_DXFER_NONE;
 	hp->dxfer_len = mxsize;
 	hp->dxferp = (char __user *)buf + cmd_size;
 	hp->sbp = NULL;

@@ -566,6 +614,23 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * If data direction is indeterminate because both input and output size
+	 * are greater than 0, use command bytes to determine direction.
+	 */
+	if (input_size == 0 && mxsize > 0)
+		hp->dxfer_direction = SG_DXFER_FROM_DEV;
+	else if (input_size > 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_TO_DEV;
+	else if (input_size == 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_NONE;
+	else {
+		hp->dxfer_direction = sg_direction(cmnd);
+		printk("sg_write: cannot infer direction from count %ld "
+		       "and reply_len %d; inferring from command 0x%x.  Please "
+		       "fix the application to use the sg version 3 interface\n",
+		       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
+	}
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }
#26 lore raw

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

Quoted 4 lines · Jeremy Higdon

One more comment from hch via IRC. When we have to guess direction, print a suggestion to use SG v3 interface.

I also removed the vendor unique commands from the switch.

I had a bug in that last patch.

===== drivers/scsi/sg.c 1.90 vs edited =====

drivers/scsi/sg.c +70 -5
@@ -480,6 +480,59 @@
 	return (0 == err) ? count : err;
 }
 
+
+static int
+sg_direction(char *cmnd)
+{
+	switch (cmnd[0]) {
+	/*  _DATA_OUT commands	*/
+	case WRITE_6:		case WRITE_10:		case WRITE_12:
+	case WRITE_16:
+	case WRITE_LONG:	case WRITE_SAME:	case WRITE_BUFFER:
+	case WRITE_VERIFY:	case WRITE_VERIFY_12:
+	case COMPARE:		case COPY:		case COPY_VERIFY:
+	case SEARCH_EQUAL:	case SEARCH_HIGH:	case SEARCH_LOW:
+	case SEARCH_EQUAL_12:	case SEARCH_HIGH_12:	case SEARCH_LOW_12:
+	case MODE_SELECT:	case MODE_SELECT_10:	case LOG_SELECT:
+	case SEND_DIAGNOSTIC:	case CHANGE_DEFINITION: case UPDATE_BLOCK:
+	case SET_WINDOW:	case MEDIUM_SCAN:	case SEND_VOLUME_TAG:
+	case REASSIGN_BLOCKS:
+	case PERSISTENT_RESERVE_OUT:
+		return SG_DXFER_TO_DEV;
+
+	/*  No data transfer commands  */
+	case SEEK_6:		case SEEK_10:
+	case RESERVE:		case RELEASE:
+	case TEST_UNIT_READY:
+	case START_STOP:
+	case ALLOW_MEDIUM_REMOVAL:
+		return SG_DXFER_NONE;
+
+	/*  Conditional data transfer commands	*/
+	case FORMAT_UNIT:
+		if (cmnd[1] & 0x10)	/* FmtData (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case VERIFY:
+		if (cmnd[1] & 0x02)	/* VERIFY:BYTCHK (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	case RESERVE_10:
+		if (cmnd[1] & 0x03)  /* RESERVE:{LongID|Extent} (data out phase)? */
+			return SG_DXFER_TO_DEV;
+		else
+			return SG_DXFER_NONE;
+
+	/*  Must be data _IN!  */
+	default:
+		return SG_DXFER_FROM_DEV;
+	}
+}
+
 static ssize_t
 sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 {

@@ -552,11 +605,6 @@
 	hp->cmd_len = (unsigned char) cmd_size;
 	hp->iovec_count = 0;
 	hp->mx_sb_len = 0;
-	if (input_size > 0)
-		hp->dxfer_direction = (old_hdr.reply_len > SZ_SG_HEADER) ?
-		    SG_DXFER_TO_FROM_DEV : SG_DXFER_TO_DEV;
-	else
-		hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : SG_DXFER_NONE;
 	hp->dxfer_len = mxsize;
 	hp->dxferp = (char __user *)buf + cmd_size;
 	hp->sbp = NULL;

@@ -566,6 +614,23 @@
 	hp->usr_ptr = NULL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
+	/*
+	 * If data direction is indeterminate because both input and output size
+	 * are greater than 0, use command bytes to determine direction.
+	 */
+	if (input_size == 0 && mxsize > 0)
+		hp->dxfer_direction = SG_DXFER_FROM_DEV;
+	else if (input_size > 0 && old_hdr.reply_len <= SZ_SG_HEADER)
+		hp->dxfer_direction = SG_DXFER_TO_DEV;
+	else if (input_size == 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_NONE;
+	else {
+		hp->dxfer_direction = sg_direction(cmnd);
+		printk("sg_write: cannot infer direction from count %ld "
+		       "and reply_len %d; inferring from command 0x%x.  Please "
+		       "fix the application to use the sg version 3 interface\n",
+		       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
+	}
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }
#27 lore raw

Re: [PATCH] sg.c to set direction more reliably (was Re: [PATCH] fusion update to current APIs)

Quoted 3 lines · Jeremy Higdon
Quoted 2 lines · Jeremy Higdon

One more comment from hch via IRC. When we have to guess direction, print a suggestion to use SG v3 interface.

But print only once, so borked applications that continually poll devices won't generate lots of these messages.

Quoted 20 lines
drivers/scsi/sg.c +17 -0
+	/*
+	 * If data direction is indeterminate because both input and output size
+	 * are greater than 0, use command bytes to determine direction.
+	 */
+	if (input_size == 0 && mxsize > 0)
+		hp->dxfer_direction = SG_DXFER_FROM_DEV;
+	else if (input_size > 0 && old_hdr.reply_len <= SZ_SG_HEADER)
+		hp->dxfer_direction = SG_DXFER_TO_DEV;
+	else if (input_size == 0 && mxsize == 0)
+		hp->dxfer_direction = SG_DXFER_NONE;
+	else {
+		hp->dxfer_direction = sg_direction(cmnd);
+		printk("sg_write: cannot infer direction from count %ld "
+		       "and reply_len %d; inferring from command 0x%x.  Please "
+		       "fix the application to use the sg version 3 interface\n",
+		       count, old_hdr.reply_len, (unsigned int) cmnd[0]);
+	}
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
 	return (k < 0) ? k : count;
 }