[SCSI] Add support for braindead Cypress USB ATA passthrough CDBs

[SCSI] Add support for braindead Cypress USB ATA passthrough CDBs

[David Caldwell]

This patch does 2 things. It reimplements the SG_FLAG_LUN_INHIBIT flag
in the SG_IO ioctl which stops the scsi subsystem from overwriting the
2nd byte of the CDB with the LUN. It also doesn't guess the CDB length
when sending the SG_IO ioctl to the sg device (the main scsi_ioctl
already did this).

This is for the Cypress CY7C68310 USB to ATA bridge chip (and most
likely other USB to ATA chips from Cypress), which implements an ATA
passthrough command that is 16 bytes long and starts with the bytes
0x24 0x24. (Not vendor unique, weird length for opcode 0x24, and
misuse of the LUN area all at the same time--Lovely).

Signed-off-by: David Caldwell <david@porkrind.org>

---

 block/scsi_ioctl.c         |    3 +++
 drivers/scsi/scsi.c        |    3 ++-
 drivers/scsi/scsi_error.c  |    4 ++--
 drivers/scsi/scsi_lib.c    |    8 ++++----
 drivers/scsi/sg.c          |   15 ++++++++-------
 drivers/scsi/st.c          |    4 ++--
 include/linux/blkdev.h     |    2 ++
 include/scsi/scsi_device.h |    5 +++--
 include/scsi/sg.h          |    2 +-
 9 files changed, 27 insertions(+), 19 deletions(-)

ea88a92a77263617575d957316464c1e6e8162ca
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 4e390df..266b828 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -291,6 +291,9 @@ static int sg_io(struct file *file, requ
 	rq->flags |= REQ_BLOCK_PC;
 	bio = rq->bio;
 
+	if (hdr->flags & SG_FLAG_LUN_INHIBIT)
+	    rq->flags |= REQ_LUN_INHIBIT;
+
 	/*
 	 * bounce this after holding a reference to the original bio, it's
 	 * needed for proper unmapping
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 180676d..cdcf6a5 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -567,7 +567,8 @@ int scsi_dispatch_cmd(struct scsi_cmnd *
 	/* 
 	 * If SCSI-2 or lower, store the LUN value in cmnd.
 	 */
-	if (cmd->device->scsi_level <= SCSI_2) {
+	if (cmd->device->scsi_level <= SCSI_2 &&
+	    !(cmd->request->flags & REQ_LUN_INHIBIT)) {
 		cmd->cmnd[1] = (cmd->cmnd[1] & 0x1f) |
 			       (cmd->device->lun << 5 & 0xe0);
 	}
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index a2333d2..fd2f28c 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1350,8 +1350,8 @@ static void scsi_eh_lock_door(struct scs
 	cmnd[4] = SCSI_REMOVAL_PREVENT;
 	cmnd[5] = 0;
 
-	scsi_execute_async(sdev, cmnd, DMA_NONE, NULL, 0, 0, 10 * HZ,
-			   5, NULL, NULL, GFP_KERNEL);
+	scsi_execute_async(sdev, cmnd, 6, DMA_NONE, NULL, 0, 0, 10 * HZ,
+			   5, 0, NULL, NULL, GFP_KERNEL);
 }
 
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index a7f3f0c..a7504ca 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -445,8 +445,8 @@ free_bios:
  * @flags:	or into request flags
  **/
 int scsi_execute_async(struct scsi_device *sdev, const unsigned char *cmd,
-		       int data_direction, void *buffer, unsigned bufflen,
-		       int use_sg, int timeout, int retries, void *privdata,
+		       unsigned cmd_len, int data_direction, void *buffer, unsigned bufflen,
+		       int use_sg, int timeout, int retries, int flags, void *privdata,
 		       void (*done)(void *, char *, int, int), gfp_t gfp)
 {
 	struct request *req;
@@ -462,7 +462,7 @@ int scsi_execute_async(struct scsi_devic
 	req = blk_get_request(sdev->request_queue, write, gfp);
 	if (!req)
 		goto free_sense;
-	req->flags |= REQ_BLOCK_PC | REQ_QUIET;
+	req->flags |= flags | REQ_BLOCK_PC | REQ_QUIET;
 
 	if (use_sg)
 		err = scsi_req_map_sg(req, buffer, use_sg, bufflen, gfp);
@@ -472,7 +472,7 @@ int scsi_execute_async(struct scsi_devic
 	if (err)
 		goto free_req;
 
-	req->cmd_len = COMMAND_SIZE(cmd[0]);
+	req->cmd_len = cmd_len ? cmd_len : COMMAND_SIZE(cmd[0]);
 	memcpy(req->cmd, cmd, req->cmd_len);
 	req->sense = sioc->sense;
 	req->sense_len = 0;
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 221e96e..7b3c57f 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -183,7 +183,7 @@ static ssize_t sg_new_read(Sg_fd * sfp, 
 static ssize_t sg_new_write(Sg_fd * sfp, const char __user *buf, size_t count,
 			    int blocking, int read_only, Sg_request ** o_srp);
 static int sg_common_write(Sg_fd * sfp, Sg_request * srp,
-			   unsigned char *cmnd, int timeout, int blocking);
+			   unsigned char *cmnd, int timeout, int blocking, int lun_inhibit);
 static int sg_u_iovec(sg_io_hdr_t * hp, int sg_num, int ind,
 		      int wr_xf, int *countp, unsigned char __user **up);
 static int sg_write_xfer(Sg_request * srp);
@@ -614,7 +614,7 @@ sg_write(struct file *filp, const char _
 			       old_hdr.reply_len - (int)SZ_SG_HEADER,
 			       input_size, (unsigned int) cmnd[0],
 			       current->comm);
-	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
+	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking, 0);
 	return (k < 0) ? k : count;
 }
 
@@ -681,7 +681,8 @@ sg_new_write(Sg_fd * sfp, const char __u
 		sg_remove_request(sfp, srp);
 		return -EPERM;
 	}
-	k = sg_common_write(sfp, srp, cmnd, timeout, blocking);
+	k = sg_common_write(sfp, srp, cmnd, timeout, blocking,
+			    hp->flags & SG_FLAG_LUN_INHIBIT);
 	if (k < 0)
 		return k;
 	if (o_srp)
@@ -691,7 +692,7 @@ sg_new_write(Sg_fd * sfp, const char __u
 
 static int
 sg_common_write(Sg_fd * sfp, Sg_request * srp,
-		unsigned char *cmnd, int timeout, int blocking)
+		unsigned char *cmnd, int timeout, int blocking, int lun_inhibit)
 {
 	int k, data_dir;
 	Sg_device *sdp = sfp->parentdp;
@@ -741,10 +742,10 @@ sg_common_write(Sg_fd * sfp, Sg_request 
 	hp->duration = jiffies_to_msecs(jiffies);
 /* Now send everything of to mid-level. The next time we hear about this
    packet is when sg_cmd_done() is called (i.e. a callback). */
-	if (scsi_execute_async(sdp->device, cmnd, data_dir, srp->data.buffer,
+	if (scsi_execute_async(sdp->device, cmnd, hp->cmd_len, data_dir, srp->data.buffer,
 				hp->dxfer_len, srp->data.k_use_sg, timeout,
-				SG_DEFAULT_RETRIES, srp, sg_cmd_done,
-				GFP_ATOMIC)) {
+				SG_DEFAULT_RETRIES, lun_inhibit ? REQ_LUN_INHIBIT : 0,
+				srp, sg_cmd_done, GFP_ATOMIC)) {
 		SCSI_LOG_TIMEOUT(1, printk("sg_write: scsi_execute_async failed\n"));
 		/*
 		 * most likely out of mem, but could also be a bad map
diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index c4aade8..d73cb24 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -509,9 +509,9 @@ st_do_scsi(struct st_request * SRpnt, st
 	STp->buffer->cmdstat.have_sense = 0;
 	STp->buffer->syscall_result = 0;
 
-	if (scsi_execute_async(STp->device, cmd, direction,
+	if (scsi_execute_async(STp->device, cmd, 0, direction,
 			&((STp->buffer)->sg[0]), bytes, (STp->buffer)->sg_segs,
-			       timeout, retries, SRpnt, st_sleep_done, GFP_KERNEL)) {
+			       timeout, retries, 0, SRpnt, st_sleep_done, GFP_KERNEL)) {
 		/* could not allocate the buffer or request was too large */
 		(STp->buffer)->syscall_result = (-EBUSY);
 		(STp->buffer)->last_SRpnt = NULL;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index a18500d..0612a91 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -233,6 +233,7 @@ enum rq_flag_bits {
 	__REQ_BAR_PREFLUSH,	/* barrier pre-flush done */
 	__REQ_BAR_POSTFLUSH,	/* barrier post-flush */
 	__REQ_BAR_FLUSH,	/* rq is the flush request */
+	__REQ_LUN_INHIBIT,	/* leave cmd[1] unmolested */
 	__REQ_NR_BITS,		/* stops here */
 };
 
@@ -263,6 +264,7 @@ enum rq_flag_bits {
 #define REQ_BAR_PREFLUSH	(1 << __REQ_BAR_PREFLUSH)
 #define REQ_BAR_POSTFLUSH	(1 << __REQ_BAR_POSTFLUSH)
 #define REQ_BAR_FLUSH	(1 << __REQ_BAR_FLUSH)
+#define REQ_LUN_INHIBIT	(1 << __REQ_LUN_INHIBIT)
 
 /*
  * State information carried for REQ_PM_SUSPEND and REQ_PM_RESUME
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index e94ca4d..4c53f8c 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -275,9 +275,10 @@ extern int scsi_execute_req(struct scsi_
 			    int data_direction, void *buffer, unsigned bufflen,
 			    struct scsi_sense_hdr *, int timeout, int retries);
 extern int scsi_execute_async(struct scsi_device *sdev,
-			      const unsigned char *cmd, int data_direction,
+			      const unsigned char *cmd, unsigned cmd_len,
+			      int data_direction,
 			      void *buffer, unsigned bufflen, int use_sg,
-			      int timeout, int retries, void *privdata,
+			      int timeout, int retries, int flags, void *privdata,
 			      void (*done)(void *, char *, int, int),
 			      gfp_t gfp);
 
diff --git a/include/scsi/sg.h b/include/scsi/sg.h
index 0a487fe..6151e9c 100644
--- a/include/scsi/sg.h
+++ b/include/scsi/sg.h
@@ -139,7 +139,7 @@ typedef struct sg_io_hdr
 
 /* following flag values can be "or"-ed together */
 #define SG_FLAG_DIRECT_IO 1     /* default is indirect IO */
-#define SG_FLAG_UNUSED_LUN_INHIBIT 2   /* default is overwrite lun in SCSI */
+#define SG_FLAG_LUN_INHIBIT 2   /* default is overwrite lun in SCSI */
 				/* command block (when <= SCSI_2) */
 #define SG_FLAG_MMAP_IO 4       /* request memory mapped IO */
 #define SG_FLAG_NO_DXFER 0x10000 /* no transfer of kernel buffers to/from */
-- 
0.99.9n

Re: [SCSI] Add support for braindead Cypress USB ATA passthrough CDBs

[thomas schorpp]

David Caldwell wrote:
> This patch does 2 things. It reimplements the SG_FLAG_LUN_INHIBIT flag
> in the SG_IO ioctl which stops the scsi subsystem from overwriting the
> 2nd byte of the CDB with the LUN. It also doesn't guess the CDB length
> when sending the SG_IO ioctl to the sg device (the main scsi_ioctl
> already did this).
> 
> This is for the Cypress CY7C68310 USB to ATA bridge chip (and most
> likely other USB to ATA chips from Cypress), which implements an ATA
> passthrough command that is 16 bytes long and starts with the bytes
> 0x24 0x24. (Not vendor unique, weird length for opcode 0x24, and
> misuse of the LUN area all at the same time--Lovely).

thx, hm, that chip is that old that datasheet is available no more...

i test it as soon as i get my 68300A changed with the new -B- type back from lab.

anyway, i see the first ATACB enabled (guaranteed) announced chip should be 
the 683xxB types. maybe they have the correct behaviour (16Bytes, opccode length 
problem), so maybe the last part of above functionality could interfere.

interesting is the lately errata sheet for -B-:

-some atacb functionality not availlable if in udma mode
-high order bits of error sector not returned at atacb

Re: [SCSI] Add support for braindead Cypress USB ATA passthrough CDBs

[David Caldwell]

thomas schorpp <t.schorpp <at> gmx.de> writes:

> 
> David Caldwell wrote:
> > This patch does 2 things. It reimplements the SG_FLAG_LUN_INHIBIT flag
> > in the SG_IO ioctl which stops the scsi subsystem from overwriting the
> > 2nd byte of the CDB with the LUN. It also doesn't guess the CDB length
> > when sending the SG_IO ioctl to the sg device (the main scsi_ioctl
> > already did this).
> > 
> > This is for the Cypress CY7C68310 USB to ATA bridge chip (and most
> > likely other USB to ATA chips from Cypress), which implements an ATA
> > passthrough command that is 16 bytes long and starts with the bytes
> > 0x24 0x24. (Not vendor unique, weird length for opcode 0x24, and
> > misuse of the LUN area all at the same time--Lovely).
> 
> thx, hm, that chip is that old that datasheet is available no more...

Yeah, the chip I tested with was a CY7C68310, but the datasheet I used
was the B rev, I think:
<http://www.cypress.com/portal/server.pt?space=CommunityPage&control=SetCommunity&CommunityID=209&PageID=259&fid=14&rpn=CY7C68301B>

Actually, the chip was labelled CY7C68310-80AC, but I couldn't tell if
those last 4 digits were a version or just a datecode.

> i test it as soon as i get my 68300A changed with the new -B- type
> back from lab.

Cool.

> anyway, i see the first ATACB enabled (guaranteed) announced chip
> should be the 683xxB types. maybe they have the correct behaviour
> (16Bytes, opccode length problem), so maybe the last part of above
> functionality could interfere.

It shoudn't interfere, but it could be redundant. The length mod
requires that the user set the correct CDB length in the SG_IO
ioctl, which is how it was documented anyway. It is also how the sd
and st drivers interpret the ioctl, so now at least that part is
consistent between drivers. It would be better if they shared the
code, though.

I kind of doubt they've fixed their interface since I remember seeing
the "24 24" opcode (as described in the B datasheet) being used on
some Cypress chips at work 2 or 3 years ago, so it makes me think
they've been consistent.

An interesting note: the datasheet mentions that the 1st byte of the
cdb comes from the EEPROM which means that when a board maker programs
the chip they *could* pick a reasonable opcode (though the 2nd byte is
still *required* to be 0x24, so the LUN issue still stands).

-David

Re: [SCSI] Add support for braindead Cypress USB ATA passthrough CDBs

[James Bottomley]

On Wed, 2005-12-21 at 04:24 -0800, David Caldwell wrote:
> This patch does 2 things. It reimplements the SG_FLAG_LUN_INHIBIT flag
> in the SG_IO ioctl which stops the scsi subsystem from overwriting the
> 2nd byte of the CDB with the LUN. It also doesn't guess the CDB length
> when sending the SG_IO ioctl to the sg device (the main scsi_ioctl
> already did this).
> 
> This is for the Cypress CY7C68310 USB to ATA bridge chip (and most
> likely other USB to ATA chips from Cypress), which implements an ATA
> passthrough command that is 16 bytes long and starts with the bytes
> 0x24 0x24. (Not vendor unique, weird length for opcode 0x24, and
> misuse of the LUN area all at the same time--Lovely).

I think this approach is too invasive to the stack.  When this was
discussed in november, there wasn't much enthusiasm for resurrecting the
long dead LUN_INHIBIT flag.  The two suggested mechanisms are

1) Simply don't mangle the LUN for SCSI_UNKNOWN and then have all the
subsystems lying about SCSI_2 compliance instead set their mangled level
to SCSI_UNKNOWN (which seems to be more truthful)

2) If 1) doesn't work, then use a blacklist entry which the subsystems
would also have access to.

James

Re: [SCSI] Add support for braindead Cypress USB ATA passthrough CDBs

[Jeff Garzik]

James Bottomley wrote:
> 1) Simply don't mangle the LUN for SCSI_UNKNOWN and then have all the
> subsystems lying about SCSI_2 compliance instead set their mangled level
> to SCSI_UNKNOWN (which seems to be more truthful)

It is specified in MMC that ATAPI or USB may be SCSI level to zero. 
Thus "lying" is not really accurate at all.  We need to update SCSI to 
handle these devices as specified.

This is why I snoop INQUIRY for ATAPI devices in libata-scsi, and force 
the SCSI version to 0x05, if SCSI version is returned from the device as 
zero.  It is conditional because -- due to the wonderful world of 
hardware -- some ATAPI devices (such as SCSI devices plugged into an 
SPI<->ATA bridge) and some USB devices correctly report SCSI version.

	Jeff

Re: [SCSI] Add support for braindead Cypress USB ATA passthrough CDBs

[James Bottomley]

On Fri, 2005-12-23 at 12:41 -0500, Jeff Garzik wrote:
> James Bottomley wrote:
> > 1) Simply don't mangle the LUN for SCSI_UNKNOWN and then have all the
> > subsystems lying about SCSI_2 compliance instead set their mangled level
> > to SCSI_UNKNOWN (which seems to be more truthful)
> 
> It is specified in MMC that ATAPI or USB may be SCSI level to zero. 
> Thus "lying" is not really accurate at all.  We need to update SCSI to 
> handle these devices as specified.

Isn't that what I proposed?

But it only fixes the devices whose SCSI level is correctly passed on to
the mid-layer.  There's still the problem of USB and other subsystems
mangling the level back to SCSI_2 which causes the LUN to be placed in
cdb[1] as per spec.  

> This is why I snoop INQUIRY for ATAPI devices in libata-scsi, and force 
> the SCSI version to 0x05, if SCSI version is returned from the device as 
> zero.  It is conditional because -- due to the wonderful world of 
> hardware -- some ATAPI devices (such as SCSI devices plugged into an 
> SPI<->ATA bridge) and some USB devices correctly report SCSI version.

Well, that's SCSI3 (SPC-3), which is fine, but does mean we try
REPORT_LUNS scanning.  The problem in other subsystems is that some
devices hang when they see a REPORT_LUNS command.

What problems do you see returning zero (apart from cdb[1] changes)?

James

Re: [SCSI] Add support for braindead Cypress USB ATA passthrough CDBs

[David Caldwell]

On 12/23/05 09:52:40 -0600 James Bottomley wrote:

> On Wed, 2005-12-21 at 04:24 -0800, David Caldwell wrote:
>> This patch does 2 things. It reimplements the SG_FLAG_LUN_INHIBIT flag
>> in the SG_IO ioctl which stops the scsi subsystem from overwriting the
>> 2nd byte of the CDB with the LUN. It also doesn't guess the CDB length
>> when sending the SG_IO ioctl to the sg device (the main scsi_ioctl
>> already did this).
>
> I think this approach is too invasive to the stack.  When this was
> discussed in november, there wasn't much enthusiasm for resurrecting the
> long dead LUN_INHIBIT flag.  The two suggested mechanisms are

Invasive because of the extra flag in the request structure?

> 2) If 1) doesn't work, then use a blacklist entry which the subsystems
> would also have access to.

I think this might not be optimal. The problem is that it's impossible to 
tell that it's a Cypress part from the USB side of things (or the SCSI side 
for that matter), so there would have to be an entry for each of the 50,000 
vendors of USB bridges.

What about the patch's cdb length additions in sg and scsi_lib.c? It seems 
like half the code guesses CDB length and the other half passes it around. 
Clearly, given devices like this, guessing isn't going to work 100% of the 
time. So either eveyone needs to pass around lengths, or there needs to be 
another flag somewhere. The code should at least be consistent though.

-David

Re: [SCSI] Add support for braindead Cypress USB ATA passthrough CDBs

[James Bottomley]

On Fri, 2005-12-23 at 11:12 -0800, David Caldwell wrote:
> > I think this approach is too invasive to the stack.  When this was
> > discussed in november, there wasn't much enthusiasm for resurrecting the
> > long dead LUN_INHIBIT flag.  The two suggested mechanisms are
> 
> Invasive because of the extra flag in the request structure?

Yes. But also having users determine this is wrong when it's a device
feature.

> > 2) If 1) doesn't work, then use a blacklist entry which the subsystems
> > would also have access to.
> 
> I think this might not be optimal. The problem is that it's impossible to 
> tell that it's a Cypress part from the USB side of things (or the SCSI side 
> for that matter), so there would have to be an entry for each of the 50,000 
> vendors of USB bridges.

I meant use it in the way usb uses other blacklist flags: set them from
slave_configure.

> What about the patch's cdb length additions in sg and scsi_lib.c? It seems 
> like half the code guesses CDB length and the other half passes it around. 
> Clearly, given devices like this, guessing isn't going to work 100% of the 
> time. So either eveyone needs to pass around lengths, or there needs to be 
> another flag somewhere. The code should at least be consistent though.

I don't think they're necessary, are they?  Zero in cmnd_len means
mid-layer determines size.  What it prevents is the issuing of vendor
specific commands via the API, which is arguably a good thing.

James

Re: [SCSI] Add support for braindead Cypress USB ATA passthrough CDBs

[David Caldwell]

On 12/23/05 14:34:30 -0600 James Bottomley wrote:

> On Fri, 2005-12-23 at 11:12 -0800, David Caldwell wrote:
>> > I think this approach is too invasive to the stack.  When this was
>> > discussed in november, there wasn't much enthusiasm for resurrecting
>> > the long dead LUN_INHIBIT flag.  The two suggested mechanisms are
>>
>> Invasive because of the extra flag in the request structure?
>
> Yes. But also having users determine this is wrong when it's a device
> feature.

I see. I can agree with that. It certainly would be better if it was 
somehow automatic.

>> > 2) If 1) doesn't work, then use a blacklist entry which the subsystems
>> > would also have access to.
>>
>> I think this might not be optimal. The problem is that it's impossible
>> to  tell that it's a Cypress part from the USB side of things (or the
>> SCSI side  for that matter), so there would have to be an entry for each
>> of the 50,000  vendors of USB bridges.
>
> I meant use it in the way usb uses other blacklist flags: set them from
> slave_configure.

Hmm. I don't understand this yet, I'll have to read more, but a cursory 
glance looks like this is a kernel interface. I'd really like things to 
flexible enough to be able to do this sort of one off random device stuff 
from user space. Or is the idea that the [generic] usb_storage driver would 
set a flag that says "I don't do LUNs"?

>> What about the patch's cdb length additions in sg and scsi_lib.c? It
>> seems  like half the code guesses CDB length and the other half passes
>> it around.  Clearly, given devices like this, guessing isn't going to
>> work 100% of the  time. So either eveyone needs to pass around lengths,
>> or there needs to be  another flag somewhere. The code should at least
>> be consistent though.
>
> I don't think they're necessary, are they?  Zero in cmnd_len means
> mid-layer determines size.

I think it's fine for zero length to mean mid_layer figures out the size, 
but
right now SG is throwing away the user passed in length (since 
scsi_execute_async() doesn't have a cdb_length parameter) and then guessing 
at it later regardless of whether it's zero or non-zero. 
blkdev/scsi_ioctl.c doesn't do this, but sg.c does which is at least 
inconsistent.

> What it prevents is the issuing of vendor specific commands via the API,
> which is arguably a good thing.

Hmmm, isn't this the point of the ioctl API? How else do you get vendor 
unique things to a device?

-David

Re: [SCSI] Add support for braindead Cypress USB ATA passthrough CDBs

[Douglas Gilbert]

David Caldwell wrote:
> On 12/23/05 14:34:30 -0600 James Bottomley wrote:
> 
>> On Fri, 2005-12-23 at 11:12 -0800, David Caldwell wrote:
>>
>>> > I think this approach is too invasive to the stack.  When this was
>>> > discussed in november, there wasn't much enthusiasm for resurrecting
>>> > the long dead LUN_INHIBIT flag.  The two suggested mechanisms are
>>>
>>> Invasive because of the extra flag in the request structure?
>>
>>
>> Yes. But also having users determine this is wrong when it's a device
>> feature.
> 
> 
> I see. I can agree with that. It certainly would be better if it was
> somehow automatic.
> 
>>> > 2) If 1) doesn't work, then use a blacklist entry which the subsystems
>>> > would also have access to.
>>>
>>> I think this might not be optimal. The problem is that it's impossible
>>> to  tell that it's a Cypress part from the USB side of things (or the
>>> SCSI side  for that matter), so there would have to be an entry for each
>>> of the 50,000  vendors of USB bridges.
>>
>>
>> I meant use it in the way usb uses other blacklist flags: set them from
>> slave_configure.
> 
> 
> Hmm. I don't understand this yet, I'll have to read more, but a cursory
> glance looks like this is a kernel interface. I'd really like things to
> flexible enough to be able to do this sort of one off random device
> stuff from user space. Or is the idea that the [generic] usb_storage
> driver would set a flag that says "I don't do LUNs"?
> 
>>> What about the patch's cdb length additions in sg and scsi_lib.c? It
>>> seems  like half the code guesses CDB length and the other half passes
>>> it around.  Clearly, given devices like this, guessing isn't going to
>>> work 100% of the  time. So either eveyone needs to pass around lengths,
>>> or there needs to be  another flag somewhere. The code should at least
>>> be consistent though.
>>
>>
>> I don't think they're necessary, are they?  Zero in cmnd_len means
>> mid-layer determines size.
> 
> 
> I think it's fine for zero length to mean mid_layer figures out the
> size,

David,
I don't think it is a good idea to ever pass zero command
length through a scsi pass through. I'm not even sure the
SCSI-2 (1992) standard asserts that the first command byte
can be used to determine command length. Definitely nothing
since then.

 but
> right now SG is throwing away the user passed in length (since
> scsi_execute_async() doesn't have a cdb_length parameter) and then
> guessing at it later regardless of whether it's zero or non-zero.

I can find no such code. Which kernel version are you
describing? There was a bug in the block layer SG_IO
ioctl code in the early lk 2.6 series that caused
the user supplied length to be ignored. That has now
been fixed. The sg driver from about 1998 has had
mechanisms to explicitly supply the command length.

> blkdev/scsi_ioctl.c doesn't do this, but sg.c does which is at least
> inconsistent.

do what ??

>> What it prevents is the issuing of vendor specific commands via the API,
>> which is arguably a good thing.
> 
> 
> Hmmm, isn't this the point of the ioctl API? How else do you get vendor
> unique things to a device?

I supplied a patch to allow the user to write to scsi_level
in sysfs, thus overriding any assumptions that scsi_lib
made about the device supplied scsi_level:
http://marc.theaimsgroup.com/?l=linux-scsi&m=113343693013725&w=2
That would solve your command overwrite problem in a less
invasive way than the patch you are proposing.

Doug Gilbert

Re: [SCSI] Add support for braindead Cypress USB ATA passthrough CDBs

[David Caldwell]

On 12/24/05 13:28:20 +1000 Douglas Gilbert wrote:

> David Caldwell wrote:
>> On 12/23/05 14:34:30 -0600 James Bottomley wrote:
>>
>>> On Fri, 2005-12-23 at 11:12 -0800, David Caldwell wrote:
>>>
>>>> What about the patch's cdb length additions in sg and scsi_lib.c? It
>>>> seems  like half the code guesses CDB length and the other half passes
>>>> it around.  Clearly, given devices like this, guessing isn't going to
>>>> work 100% of the  time. So either eveyone needs to pass around lengths,
>>>> or there needs to be  another flag somewhere. The code should at least
>>>> be consistent though.
>>>
>>>
>>> I don't think they're necessary, are they?  Zero in cmnd_len means
>>> mid-layer determines size.
>>
>> I think it's fine for zero length to mean mid_layer figures out the
>> size,
>
> David,
> I don't think it is a good idea to ever pass zero command
> length through a scsi pass through. I'm not even sure the
> SCSI-2 (1992) standard asserts that the first command byte
> can be used to determine command length. Definitely nothing
> since then.

I only did this because the ST driver happens to call this 
scsi_execute_async() from a function that itself gets called a bunch of 
times with no cdb length. So I either had to change even more things, or 
build in that "zero == guess" backwards compatibility. It wasn't really 
meant to be used from the SG_IO interface, but it happens to work (unless 
you call it on an sd device which is a completely different code path).

>>  but right now SG is throwing away the user passed in length (since
>>  scsi_execute_async() doesn't have a cdb_length parameter) and then
>>  guessing at it later regardless of whether it's zero or non-zero.
>
> I can find no such code. Which kernel version are you
> describing?

This is in the scsi_misc git tree from kernel.org.

> There was a bug in the block layer SG_IO
> ioctl code in the early lk 2.6 series that caused
> the user supplied length to be ignored. That has now
> been fixed. The sg driver from about 1998 has had
> mechanisms to explicitly supply the command length.
>
>> blkdev/scsi_ioctl.c doesn't do this, but sg.c does which is at least
>> inconsistent.
>
> do what ??

Ignore the user supplied length (from the SG_IO ioctl).

> I supplied a patch to allow the user to write to scsi_level
> in sysfs, thus overriding any assumptions that scsi_lib
> made about the device supplied scsi_level:
> http://marc.theaimsgroup.com/?l=linux-scsi&m=113343693013725&w=2
> That would solve your command overwrite problem in a less
> invasive way than the patch you are proposing.

Sort of. It's kind of an oblique way of stopping it from messing with the 
LUN though. There seems to be checks all around the scsi code for the scsi 
level (I saw it in more than one place). So changing the scsi_level so that 
the LUN wouldn't be overwritten could affect other code paths in unintended 
ways... I think it would be better to be more granular with these kind of 
overrides.

-David