Re: [example PATCH - not for applying] exclude certain commands

[Matthew Dharm <mdharm-scsi@one-eyed-alien.net>]

[-- Attachment #1: Type: text/plain, Size: 4917 bytes --]

On Sat, Apr 26, 2003 at 11:44:10PM +0200, Andries.Brouwer@cwi.nl wrote:
> (1) usb-storage is broken in the sense that it uses sr_bufflen
> for the transfer size instead of the buffer length, then fudges
> commands to make them transfer a different length, but without
> updating sr_bufflen.

I would argue with the term 'broken', especially after having discussed
this publically and having been told that using sr_bufflen this way was not
only apropriate but recommended.

But that's water under the bridge.

> (1A)
> Of course it is bad when variables have a function that differs
> from what is suggested by their name. So, sr_bufflen must be
> the length of the buffer and nothing else. If the length of
> the transfer is needed, there must be a field sr_xferlen or so.

Sounds good.  It also sounds very similar to what I proposed some time ago.
Here's a question from that discussion: How you you set sr_xferlen?  I'm
guessing that you want to add a parameters to scsi_wait_req()?  Or
will you simply remove the bufflen parameter and force callers to set both
fields in the struct scsi_request?

Either way, you'll be touching a great deal of code.  Which isn't
necessarily a bad thing, but it might raise a few eyebrows....

> (1B)
> Of course it is bad when usb-storage fudges commands.

Agreed.  I don't think you'll find anyone who would argue this point.

> (2) Also other parts of the kernel do this translation,
> so there is a lot of duplication; see for example
> the idescsi_transform code in ide-scsi.c.

Yes, there are.  That's bothered me too.... especially when programs like
cdrecord use a special IOCTL to turn that translation on-and-off.

> Lazy as I am I had hoped that I would find the right setup
> in my mailbox. But upon returning I see essentially two replies:

Hey, I've got about a 0% success rate with patches so far, so I'm a bit
hesitant.  Besides, I'm still waiting for the EVPD-removal patch to get
accepted by Linus.

> (b) Alan says that changing must be avoided when possible.
> Of course I agree. (By the way, he says among other things
> "The SCSI midlayer could try multiple forms of MODE_SENSE", but
> that happens already.)

It does?  I know it tries MODE_SENSE for cache-data... but I think Alan was
referring to WP-detect.  Maybe I just missed something.

> So I made a patch, but will only describe it since it is long,
> and I am not quite happy yet, as you will see below.
> 
> The first part, in drivers/usb/storage/scsiglue.c is very
> satisfactory:
>
> (496 lines deleted). That is a good start.
> If we do not fudge commands in usb-storage, then a lot of fudging code
> can go.

Very true.  If you can say that sd.c and SCSI core won't send MODE_SENSE,
all this can go.

> The next part, in drivers/usb/storage/protocol.c, is almost
> as satisfactory:
> 
> (deleted some 200 lines changing READ_6 into READ_10, etc.)

We need to be careful here... there are several different flavors of
comamnd-protocol used by usb-storage, and we need to double-check things
like UFI before we rip everything out.  I think it can all go, but we just
need to make sure (and I'd want to see the actual patch before doing that
check).

> These lines can be deleted because the SCSI layer does not send
> such commands. (What about sg you ask? I don't care. People who
> send commands "by hand" are responsible themselves. Moreover, it
> is really bad when these handcrafted commands are changed by the
> driver - probably they were intended precisely as written.)

Agreed.

> Why doesnt SCSI send READ_6/WRITE_6? Because there is a field "ten"
> that is initially 1 and says that READ_10 must be used. It will be
> cleared only when the device returns ILLEGAL_REQUEST.
> 
> I dislike the choice of identifier "ten" - it is difficult to grep for,
> and I changed it into use_10_for_rw, with the same function as before.
> Added a field use_10_for_ms. Thus, in scsi.h:
> 
> +       unsigned use_10_for_rw:1; /* first try 10-byte read / write */
> +       unsigned use_10_for_ms:1; /* first try 10-byte mode sense/select */

> All this is nice and well. Remains the question how usb-storage
> makes sure that the use_10_for_rw and use_10_for_ms flags are set
> for its devices.

Why not just make us_10_for_ms default to 1, just like use_10_for_rw?  The
same logic (fallback if ILLEGAL_REQUEST) could apply to both.

> Tonight I kludged this by setting this (in protocol.c) at the moment
> the first INQUIRY is done. But that is terribly ugly.
> Did I overlook some obvious means of communication?

Not that I'm aware of.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

What, are you one of those Microsoft-bashing Linux freaks?
					-- Customer to Greg
User Friendly, 2/10/1999

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]