[example PATCH - not for applying] exclude certain commands

[example PATCH - not for applying] exclude certain commands

[Andries.Brouwer]

Yesterday or so we discussed three people's problems with USB devices.
Today a fourth very similar complaint was reported.

I asked

> Could you try this patch of drivers/scsi/sd.c?
>
> --- sd.c~       Sun Apr 20 12:59:32 2003
> +++ sd.c        Wed Apr 23 21:45:10 2003
> @@ -1079,6 +1079,9 @@
>                   int dbd, int modepage, unsigned char *buffer, int len) {
>         unsigned char cmd[8];
>
> +       if (len < 8)
> +               len = 8;
> +
>         memset((void *) &cmd[0], 0, 8);
>         cmd[0] = MODE_SENSE;
>         cmd[1] = dbd;

and got the reply

#
# Wohoo! Patch worked like a charm. Thanks Andries!
#

So, for the time being I assume we know what is causing
these problems. It would be nice if the other three
(afafc@rnl.ist.utl.pt, mike@hingston.demon.co.uk, stelian@popies.net)
could report on their experiences with the above patch.

The next question is whether this is a bug in these devices
or a bug in the usb-storage code.

As Alan remarked, drivers/usb/storage/protocol.c contains
the fragment

        case MODE_SENSE:
                srb->cmnd[8] = 8;

But that is in usb_stor_ufi_command(), and
usb_stor_transparent_scsi_command() doesn't do this.
Nevertheless, MODE_SENSE has minimum length 4, and
MODE_SENSE_10 has minum length 8 for its reply.
So, possibly the latter routine should do something
similar. For example

--- protocol.c~ Fri Nov 22 22:40:13 2002
+++ protocol.c  Thu Apr 24 00:38:31 2003
@@ -366,6 +366,8 @@
                        srb->cmnd[2] = srb->cmnd[2];
                        srb->cmnd[1] = srb->cmnd[1];
                        srb->cmnd[0] = srb->cmnd[0] | 0x40;
+                       if (srb->cmnd[8] < 8)
+                               srb->cmnd[8] = 8;
                        break;
                } /* switch (srb->cmnd[0]) */
        } /* if (us->flags & US_FL_MODE_XLATE) */

(yes, ugly, no guarantee that the buffer is large enough).

Comments?

Andries

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

[Matthew Dharm]

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

On Thu, Apr 24, 2003 at 12:39:28AM +0200, Andries.Brouwer@cwi.nl wrote:
> Comments?

My only comment is that your sample of failed cases primarily seems to
address those devices that support some variant of MODE_SENSE -- there are
lots of devices that don't support it at all, which is why I wanted to
filter it with the (apparently) now-dead command filter concept.

Matt

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

It's not that hard.  No matter what the problem is, tell the customer 
to reinstall Windows.
					-- Nurse
User Friendly, 3/22/1998

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

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

[André Cruz]

It worked for me.
Great! :)
I was able to mount the CF card, list it's contents, etc.
You guys are on the right track. Keep up the good work.
cya

_________________________________________
André Cruz
ICQ#: 9519211
_________________________________________


-----Original Message-----
From: Andries.Brouwer@cwi.nl [mailto:Andries.Brouwer@cwi.nl] 
Sent: quarta-feira, 23 de Abril de 2003 23:39
To: James.Bottomley@steeleye.com
Cc: afafc@rnl.ist.utl.pt; alan@lxorguk.ukuu.org.uk; greg@kroah.com;
linux-scsi@vger.kernel.org; linux-usb-devel@lists.sourceforge.net;
mike@hingston.demon.co.uk; stelian@popies.net; torvalds@transmeta.com
Subject: [example PATCH - not for applying] exclude certain commands


Yesterday or so we discussed three people's problems with USB devices.
Today a fourth very similar complaint was reported.

I asked

> Could you try this patch of drivers/scsi/sd.c?
>
> --- sd.c~       Sun Apr 20 12:59:32 2003
> +++ sd.c        Wed Apr 23 21:45:10 2003
> @@ -1079,6 +1079,9 @@
>                   int dbd, int modepage, unsigned char *buffer, int len) {
>         unsigned char cmd[8];
>
> +       if (len < 8)
> +               len = 8;
> +
>         memset((void *) &cmd[0], 0, 8);
>         cmd[0] = MODE_SENSE;
>         cmd[1] = dbd;

and got the reply

#
# Wohoo! Patch worked like a charm. Thanks Andries!
#

So, for the time being I assume we know what is causing
these problems. It would be nice if the other three
(afafc@rnl.ist.utl.pt, mike@hingston.demon.co.uk, stelian@popies.net)
could report on their experiences with the above patch.

The next question is whether this is a bug in these devices
or a bug in the usb-storage code.

As Alan remarked, drivers/usb/storage/protocol.c contains
the fragment

        case MODE_SENSE:
                srb->cmnd[8] = 8;

But that is in usb_stor_ufi_command(), and
usb_stor_transparent_scsi_command() doesn't do this.
Nevertheless, MODE_SENSE has minimum length 4, and
MODE_SENSE_10 has minum length 8 for its reply.
So, possibly the latter routine should do something
similar. For example

--- protocol.c~ Fri Nov 22 22:40:13 2002
+++ protocol.c  Thu Apr 24 00:38:31 2003
@@ -366,6 +366,8 @@
                        srb->cmnd[2] = srb->cmnd[2];
                        srb->cmnd[1] = srb->cmnd[1];
                        srb->cmnd[0] = srb->cmnd[0] | 0x40;
+                       if (srb->cmnd[8] < 8)
+                               srb->cmnd[8] = 8;
                        break;
                } /* switch (srb->cmnd[0]) */
        } /* if (us->flags & US_FL_MODE_XLATE) */

(yes, ugly, no guarantee that the buffer is large enough).

Comments?

Andries



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

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

[Andries.Brouwer]

    From: Matthew Dharm <mdharm-scsi@one-eyed-alien.net>

    > Comments?

    My only comment is that your sample of failed cases primarily seems to
    address those devices that support some variant of MODE_SENSE -- there are
    lots of devices that don't support it at all, which is why I wanted to
    filter it with the (apparently) now-dead command filter concept.

Well, before we create a new infrastructure it is a good idea
to make sure that it is needed. James mentioned three cases
in Bugzilla and it looks like these are fixed by a small correction
to the MODE_SENSE to MODE_SENSE_10 translation.

You mention lots of devices that don't support MODE_SENSE at all.
Do you have Bugzilla or l-k / l-s / l-u refs?

(Smart Media and similar cards come with a write protect bit.
I would suppose reporting it is supported by most non-broken hardware.
We get it from the header of the MODE_SENSE return value.
So there is some reason to expect that MODE_SENSE[_10] is supported,
at least in the minimum form where no page but only the header
is returned. Closer investigation of other error cases might reveal
more usb-storage flaws.)

Andries

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

[Stelian Pop]

[I'm not subscribed to usb-devel so I may have missed some comments 
on my kernel bugzilla bug report - the one with the 'crusoe'
laptop traces...]

On Thu, Apr 24, 2003 at 12:39:28AM +0200, Andries.Brouwer@cwi.nl wrote:
> Yesterday or so we discussed three people's problems with USB devices.
> Today a fourth very similar complaint was reported.
> 
> I asked
> 
> > Could you try this patch of drivers/scsi/sd.c?
> >
> > --- sd.c~       Sun Apr 20 12:59:32 2003
> > +++ sd.c        Wed Apr 23 21:45:10 2003
> > @@ -1079,6 +1079,9 @@
> >                   int dbd, int modepage, unsigned char *buffer, int len) {
> >         unsigned char cmd[8];
> >
> > +       if (len < 8)
> > +               len = 8;
> > +
> >         memset((void *) &cmd[0], 0, 8);
> >         cmd[0] = MODE_SENSE;
> >         cmd[1] = dbd;
> 
> and got the reply
> 
> #
> # Wohoo! Patch worked like a charm. Thanks Andries!
> #
> 
> So, for the time being I assume we know what is causing
> these problems. It would be nice if the other three
> (afafc@rnl.ist.utl.pt, mike@hingston.demon.co.uk, stelian@popies.net)
> could report on their experiences with the above patch.

I confirm that this patch makes both my memory stick reader
*and* my USB floppy drive work perfectly again.
> 
> The next question is whether this is a bug in these devices
> or a bug in the usb-storage code.

In both devices ? Hard to believe...

> As Alan remarked, drivers/usb/storage/protocol.c contains
> the fragment
> 
>         case MODE_SENSE:
>                 srb->cmnd[8] = 8;
> 
> But that is in usb_stor_ufi_command(), and
> usb_stor_transparent_scsi_command() doesn't do this.
> Nevertheless, MODE_SENSE has minimum length 4, and
> MODE_SENSE_10 has minum length 8 for its reply.
> So, possibly the latter routine should do something
> similar. For example
> 
> --- protocol.c~ Fri Nov 22 22:40:13 2002
> +++ protocol.c  Thu Apr 24 00:38:31 2003
> @@ -366,6 +366,8 @@
>                         srb->cmnd[2] = srb->cmnd[2];
>                         srb->cmnd[1] = srb->cmnd[1];
>                         srb->cmnd[0] = srb->cmnd[0] | 0x40;
> +                       if (srb->cmnd[8] < 8)
> +                               srb->cmnd[8] = 8;
>                         break;
>                 } /* switch (srb->cmnd[0]) */
>         } /* if (us->flags & US_FL_MODE_XLATE) */
> 
> (yes, ugly, no guarantee that the buffer is large enough).
> 
> Comments?

This patch (applied without the first one) does not make any
difference: memory stick initialisation still takes several
minutes to complete, floppy drive initialisation takes about
10 minutes before the block layer aborts the requests with
a read error.

Stelian.
-- 
Stelian Pop <stelian@popies.net>


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

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

[Stelian Pop]

On Thu, Apr 24, 2003 at 11:15:52AM +0200, Stelian Pop wrote:

> I confirm that this patch makes both my memory stick reader
> *and* my USB floppy drive work perfectly again.

Fergot to attach some relevant traces (with the patch):

Apr 24 10:24:04 crusoe kernel: Initializing USB Mass Storage driver...
Apr 24 10:24:04 crusoe kernel: scsi0 : SCSI emulation for USB Mass Storage devices
Apr 24 10:24:04 crusoe /sbin/hotplug: no runnable /etc/hotplug/scsi.agent is installed
Apr 24 10:24:04 crusoe kernel: WARNING: USB Mass Storage data integrity not assured
Apr 24 10:24:04 crusoe kernel: USB Mass Storage device found at 2
Apr 24 10:24:04 crusoe kernel: drivers/usb/core/usb.c: registered new driver usb-storage
Apr 24 10:24:04 crusoe kernel: USB Mass Storage support registered.
Apr 24 10:24:14 crusoe kernel: usb-storage: Had to truncate MODE_SENSE_10 buffer into MODE_SENSE.
Apr 24 10:24:14 crusoe kernel: usb-storage: outputBufferSize is 10 and length is 8.
Apr 24 10:24:14 crusoe kernel: sda: Mode Sense: 06 00 00 00
Apr 24 10:24:14 crusoe kernel: usb-storage: Had to truncate MODE_SENSE_10 buffer into MODE_SENSE.
Apr 24 10:24:14 crusoe kernel: usb-storage: outputBufferSize is 10 and length is 8.
Apr 24 10:24:14 crusoe kernel: usb-storage: Had to truncate MODE_SENSE_10 buffer into MODE_SENSE.
Apr 24 10:24:14 crusoe kernel: usb-storage: outputBufferSize is 10 and length is 8.
Apr 24 10:24:14 crusoe /sbin/hotplug: no runnable /etc/hotplug/block.agent is installed
Apr 24 10:24:14 crusoe kernel:  sda: sda1
Apr 24 10:24:15 crusoe /sbin/hotplug: no runnable /etc/hotplug/block.agent is installed
Apr 24 10:24:48 crusoe kernel: hub 1-0:0: debounce: port 1: delay 100ms stable 4 status 0x101
Apr 24 10:24:48 crusoe kernel: hub 1-0:0: new USB device on port 1, assigned address 3
Apr 24 10:24:48 crusoe /etc/hotplug/usb.agent: Bad USB agent invocation
Apr 24 10:24:49 crusoe kernel: scsi1 : SCSI emulation for USB Mass Storage devices
Apr 24 10:24:49 crusoe /sbin/hotplug: no runnable /etc/hotplug/scsi.agent is installed
Apr 24 10:24:49 crusoe kernel: WARNING: USB Mass Storage data integrity not assured
Apr 24 10:24:49 crusoe kernel: USB Mass Storage device found at 3
Apr 24 10:24:49 crusoe /sbin/hotplug: no runnable /etc/hotplug/block.agent is installed
Apr 24 10:24:52 crusoe /etc/hotplug/usb.agent: Setup usb-storage for USB product 57b/0/312
Apr 24 10:24:52 crusoe modprobe: FATAL: Module usb_storage already in kernel. 
Apr 24 10:24:52 crusoe modprobe: FATAL: Module ide_probe_mod not found. 
Apr 24 10:24:52 crusoe modprobe: FATAL: Module ide_probe not found. 
Apr 24 10:24:52 crusoe modprobe: FATAL: Module ohci1394 already in kernel. 
Apr 24 10:24:57 crusoe kernel: usb-storage: Had to truncate MODE_SENSE_10 buffer into MODE_SENSE.
Apr 24 10:24:57 crusoe kernel: usb-storage: outputBufferSize is 74 and length is 8.
Apr 24 10:24:57 crusoe kernel: sdb: Mode Sense: 46 94 00 00
Apr 24 10:24:57 crusoe kernel: sdb: assuming drive cache: write through
Apr 24 10:24:57 crusoe kernel: usb-storage: Had to truncate MODE_SENSE_10 buffer into MODE_SENSE.
Apr 24 10:24:57 crusoe kernel: usb-storage: outputBufferSize is 74 and length is 8.
Apr 24 10:24:57 crusoe kernel: sdb: Mode Sense: 46 94 00 00
Apr 24 10:24:57 crusoe kernel: sdb: assuming drive cache: write through
Apr 24 10:24:58 crusoe /sbin/hotplug: no runnable /etc/hotplug/block.agent is installed
Apr 24 10:24:58 crusoe kernel:  sdb: sdb1 sdb2 sdb4
Apr 24 10:24:58 crusoe /sbin/hotplug: no runnable /etc/hotplug/block.agent is installed

-- 
Stelian Pop <stelian@popies.net>

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

[Andries.Brouwer]

    From: Stelian Pop <stelian@popies.net>

    [I'm not subscribed to usb-devel so I may have missed some comments 
    on my kernel bugzilla bug report - the one with the 'crusoe'
    laptop traces...]

    >> Could you try this patch of drivers/scsi/sd.c?
    >>
    >> --- sd.c~       Sun Apr 20 12:59:32 2003
    >> +++ sd.c        Wed Apr 23 21:45:10 2003
    >> @@ -1079,6 +1079,9 @@
    >>                   int dbd, int modepage, unsigned char *buffer, int len) {
    >>         unsigned char cmd[8];
    >>
    >> +       if (len < 8)
    >> +               len = 8;
    >> +
    >>         memset((void *) &cmd[0], 0, 8);
    >>         cmd[0] = MODE_SENSE;
    >>         cmd[1] = dbd;

    I confirm that this patch makes both my memory stick reader
    *and* my USB floppy drive work perfectly again.

Very good. That makes three people (and four devices) that are happy.

    > Nevertheless, MODE_SENSE has minimum length 4, and
    > MODE_SENSE_10 has minum length 8 for its reply.
    > So, possibly the latter routine should do something
    > similar. For example
    > 
    > --- protocol.c~ Fri Nov 22 22:40:13 2002
    > +++ protocol.c  Thu Apr 24 00:38:31 2003
    > @@ -366,6 +366,8 @@
    >                         srb->cmnd[2] = srb->cmnd[2];
    >                         srb->cmnd[1] = srb->cmnd[1];
    >                         srb->cmnd[0] = srb->cmnd[0] | 0x40;
    > +                       if (srb->cmnd[8] < 8)
    > +                               srb->cmnd[8] = 8;
    >                         break;
    >                 } /* switch (srb->cmnd[0]) */
    >         } /* if (us->flags & US_FL_MODE_XLATE) */

    This patch (applied without the first one) does not make any
    difference: memory stick initialisation still takes several
    minutes to complete, floppy drive initialisation takes about
    10 minutes before the block layer aborts the requests with
    a read error.

OK. That perhaps means that the mode sense command doesnt
pass through this code, e.g. because US_FL_MODE_XLATE
is not set?

At first sight your reports do not give VendorID, ProductID.
Do these devices occur in the unusual devices list?

Andries


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

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

[Stelian Pop]

On Thu, Apr 24, 2003 at 11:46:10AM +0200, Andries.Brouwer@cwi.nl wrote:

>     This patch (applied without the first one) does not make any
>     difference: memory stick initialisation still takes several
>     minutes to complete, floppy drive initialisation takes about
>     10 minutes before the block layer aborts the requests with
>     a read error.
> 
> OK. That perhaps means that the mode sense command doesnt
> pass through this code, e.g. because US_FL_MODE_XLATE
> is not set?
> 
> At first sight your reports do not give VendorID, ProductID.

Sorry.

Memory stick reader: 
	  idVendor           0x054c Sony Corp.
	  idProduct          0x0032 MemoryStick MSC-U01 Reader
Floppy drive:
	  idVendor           0x057b Y-E Data, Inc.
	  idProduct          0x0000 FlashBuster-U Floppy
    
> Do these devices occur in the unusual devices list?

Yes they are:
	UNUSUAL_DEV(  0x054c, 0x0032, 0x0000, 0x9999,
	                "Sony",
	                "Memorystick MSC-U01N",
	                US_SC_UFI, US_PR_CB, NULL,
	                US_FL_SINGLE_LUN | US_FL_START_STOP ),

	UNUSUAL_DEV(  0x057b, 0x0000, 0x0000, 0x0299,
	                "Y-E Data",
	                "Flashbuster-U",
	                US_SC_UFI,  US_PR_CB, NULL,
	                US_FL_SINGLE_LUN),


Stelian.
-- 
Stelian Pop <stelian@popies.net>

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

[Mike Bursell]

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

On Wed, 2003-04-23 at 23:39, Andries.Brouwer@cwi.nl wrote:
> Yesterday or so we discussed three people's problems with USB devices.
> Today a fourth very similar complaint was reported.
> 
> I asked
> 
> > Could you try this patch of drivers/scsi/sd.c?
> >
> > --- sd.c~       Sun Apr 20 12:59:32 2003
> > +++ sd.c        Wed Apr 23 21:45:10 2003
> > @@ -1079,6 +1079,9 @@
> >                   int dbd, int modepage, unsigned char *buffer, int len) {
> >         unsigned char cmd[8];
> >
> > +       if (len < 8)
> > +               len = 8;
> > +
> >         memset((void *) &cmd[0], 0, 8);
> >         cmd[0] = MODE_SENSE;
> >         cmd[1] = dbd;
> 
> and got the reply
> 
> #
> # Wohoo! Patch worked like a charm. Thanks Andries!
> #
> 
> So, for the time being I assume we know what is causing
> these problems. It would be nice if the other three
> (afafc@rnl.ist.utl.pt, mike@hingston.demon.co.uk, stelian@popies.net)
> could report on their experiences with the above patch.

I can confirm that this patch worked for me under 2.5.68: everything
works absolutely fine.  However, given that I can't open terminals under
Gnome in 2.5.68, I think I'll stick with 2.5.67 (I assume that that the
patch is also OK with this version).

Well done, and thanks.

-Mike.
-- 
07050 144575 mike@hingston.demon.co.uk

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[James Bottomley]

On Wed, 2003-04-23 at 18:39, Andries.Brouwer@cwi.nl wrote:
> --- protocol.c~ Fri Nov 22 22:40:13 2002
> +++ protocol.c  Thu Apr 24 00:38:31 2003
> @@ -366,6 +366,8 @@
>                         srb->cmnd[2] = srb->cmnd[2];
>                         srb->cmnd[1] = srb->cmnd[1];
>                         srb->cmnd[0] = srb->cmnd[0] | 0x40;
> +                       if (srb->cmnd[8] < 8)
> +                               srb->cmnd[8] = 8;
>                         break;
>                 } /* switch (srb->cmnd[0]) */
>         } /* if (us->flags & US_FL_MODE_XLATE) */
> 
> (yes, ugly, no guarantee that the buffer is large enough).
> 
> Comments?

Yes, the buffer size thing would be my major concern too...except that
the usb layer seems to get away with doing this type of thing all over
the place.

I think I can argue that this type of thing is always safe: DMA buffers
cannot be stack allocated, and kmalloc comes in minimum units of 32
bytes, so it looks like it's ugly but should always work.

James

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

[Alan Stern]

On Thu, 24 Apr 2003 Andries.Brouwer@cwi.nl wrote:

>     From: Stelian Pop <stelian@popies.net>
> 
>     > --- protocol.c~ Fri Nov 22 22:40:13 2002
>     > +++ protocol.c  Thu Apr 24 00:38:31 2003
>     > @@ -366,6 +366,8 @@
>     >                         srb->cmnd[2] = srb->cmnd[2];
>     >                         srb->cmnd[1] = srb->cmnd[1];
>     >                         srb->cmnd[0] = srb->cmnd[0] | 0x40;
>     > +                       if (srb->cmnd[8] < 8)
>     > +                               srb->cmnd[8] = 8;
>     >                         break;
>     >                 } /* switch (srb->cmnd[0]) */
>     >         } /* if (us->flags & US_FL_MODE_XLATE) */
> 
>     This patch (applied without the first one) does not make any
>     difference: memory stick initialisation still takes several
>     minutes to complete, floppy drive initialisation takes about
>     10 minutes before the block layer aborts the requests with
>     a read error.
> 
> OK. That perhaps means that the mode sense command doesnt
> pass through this code, e.g. because US_FL_MODE_XLATE
> is not set?
> 
> At first sight your reports do not give VendorID, ProductID.
> Do these devices occur in the unusual devices list?
> 
> Andries

Andries:

You probably already know this, but I'll point it out anyway.  The 
protocol.c file contains 4 different translation routines:

	usb_stor_qic157_command(),
	usb_stor_ATAPI_command(),
	usb_stor_ufi_command(),
	usb_stor_transparent_scsi_command().

The third one already contains the 4->8 modification and your patch adds 
it to the fourth.  But it's still missing from the first two.

I agree with James Bottomley's comment that it's not really safe to be 
doing this at all, even if we do feel sure that sufficient buffer space 
exists.  After all, some other data may be using that space.

Better solutions would be not to issue the 4-byte call at all, 
or to recognize that it failed and re-issue with at least 8 bytes.  That 
way no questionable buffer-size conversion is needed.  There's also the 
advantage of not having to do the same thing in 4 different places within 
usb-storage.

Alan Stern

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

[James Bottomley]

On Thu, 2003-04-24 at 10:05, Alan Stern wrote:
> You probably already know this, but I'll point it out anyway.  The 
> protocol.c file contains 4 different translation routines:
> 
> 	usb_stor_qic157_command(),
> 	usb_stor_ATAPI_command(),
> 	usb_stor_ufi_command(),
> 	usb_stor_transparent_scsi_command().
> 
> The third one already contains the 4->8 modification and your patch adds 
> it to the fourth.  But it's still missing from the first two.
> 
> I agree with James Bottomley's comment that it's not really safe to be 
> doing this at all, even if we do feel sure that sufficient buffer space 
> exists.  After all, some other data may be using that space.
> 
> Better solutions would be not to issue the 4-byte call at all, 
> or to recognize that it failed and re-issue with at least 8 bytes.  That 
> way no questionable buffer-size conversion is needed.  There's also the 
> advantage of not having to do the same thing in 4 different places within 
> usb-storage.

Actually, the better solution might be to enforce a minimum transfer
length for non-scatter gather transfers.  Although we could enforce this
in the mid-layer allocations, we'd be in difficulty for user issued
commands, so the scheme that copes with everything would be a bounce
buffer type of thing (you supply a transfer request too short for the
device, I provide the bigger buffer and then give you back what you
asked for).

Better suggestions?

James

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

[Alan Stern]

On 24 Apr 2003, James Bottomley wrote:

> Actually, the better solution might be to enforce a minimum transfer
> length for non-scatter gather transfers.  Although we could enforce this
> in the mid-layer allocations, we'd be in difficulty for user issued
> commands, so the scheme that copes with everything would be a bounce
> buffer type of thing (you supply a transfer request too short for the
> device, I provide the bigger buffer and then give you back what you
> asked for).
> 
> Better suggestions?

I dislike the idea of avoidable meddling with with user-issued (sg)  
commands.  If the user wants to send a command, we should let him; if it
fails then it's up to the user to fix things.  After all, probably lots of
devices _do_ support these 4-byte MODE-SENSE transfers.

It would probably be enough to make sure that the commands generated by 
sd.c end up working.

Alan Stern

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

[Andries.Brouwer]

# the better solution might be to enforce a minimum transfer length

> You probably already know this, but I'll point it out anyway.  The 
> protocol.c file contains 4 different translation routines:
>
>        usb_stor_qic157_command(),
>        usb_stor_ATAPI_command(),
>        usb_stor_ufi_command(),
>        usb_stor_transparent_scsi_command().
>
> The third one already contains the 4->8 modification and your patch adds 
> it to the fourth.  But it's still missing from the first two.

Yes indeed. I am not yet so far as James - still want to
understand everything before discussing the correct patch.

My theory was: MODE_SENSE returns 4 bytes minimum, MODE_SENSE_10
returns 8 bytes minimum, devices start returning the header and then
look how much more is desired.

Thus, we should never ask for less than 4 bytes with MODE_SENSE,
and never for less than 8 bytes with MODE_SENSE_10.

Since usb-storage translates MODE_SENSE into MODE_SENSE_10
(and does not necessarily change the requested length)
it may be a good idea [for testing] not to ask for less than 8 bytes.

(On the other hand, there are many examples of devices that react badly
when asked for more than is available, so we should not ask for
8 bytes with untranslated MODE_SENSE.)

So far the theory. I asked people to try adding
	if (len < 8)
		len = 8;
in the scsi code, and everybody reported success.

That is good. But there is one point I do not understand.
Stelian Pop reports on two devices, both US_SC_UFI.
But usb_stor_ufi_command() already contained code to
make the length 8. How can it be that my sd.c change
improved things?

Can it be that earlier tests were for 2.5.67 and later tests
for 2.5.68, and Matt's improvements of the latter over the former
are the real reason of improved behaviour?

Andries

[In other words, afafc@rnl.ist.utl.pt, mike@hingston.demon.co.uk,
pwkpete@yahoo.com, stelian@popies.net: did you compare 2.5.68
without and with this "if (len < 8) len = 8" patch?]

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

[James Bottomley]

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

On Thu, 2003-04-24 at 10:46, Alan Stern wrote:
> I dislike the idea of avoidable meddling with with user-issued (sg)  
> commands.  If the user wants to send a command, we should let him; if it
> fails then it's up to the user to fix things.  After all, probably lots of
> devices _do_ support these 4-byte MODE-SENSE transfers.
> 
> It would probably be enough to make sure that the commands generated by 
> sd.c end up working.

OK, if we just want only this, then it's quite easy.  The attached patch
adds a min_xfersize to the host and template.  If it's zero, the
behaviour will be as before.

James


[-- Attachment #2: tmp.diff --]
[-- Type: text/plain, Size: 2719 bytes --]

===== drivers/scsi/hosts.c 1.57 vs edited =====
--- 1.57/drivers/scsi/hosts.c	Tue Apr 15 13:20:37 2003
+++ edited/drivers/scsi/hosts.c	Thu Apr 24 11:00:29 2003
@@ -427,6 +427,7 @@
 
 	shost->max_sectors = shost_tp->max_sectors;
 	shost->use_blk_tcq = shost_tp->use_blk_tcq;
+	shost->min_xfersize = shost_tp->min_xfersize;
 
 	spin_lock(&scsi_host_list_lock);
 	/*
===== drivers/scsi/hosts.h 1.58 vs edited =====
--- 1.58/drivers/scsi/hosts.h	Mon Mar 24 07:14:28 2003
+++ edited/drivers/scsi/hosts.h	Thu Apr 24 11:04:40 2003
@@ -339,6 +339,12 @@
     unsigned use_blk_tcq:1;
 
     /*
+     * Minimum transfer length for the device.  The mid-layer will
+     * not ask for fewer bytes than this (user issued commands may)
+     */
+    unsigned char min_xfersize;
+
+    /*
      * Name of proc directory
      */
     char *proc_name;
@@ -458,6 +464,12 @@
     unsigned use_clustering:1;
     unsigned highmem_io:1;
     unsigned use_blk_tcq:1;
+
+    /* 
+     * Minimum transfer length for the device.  The mid-layer will
+     * not ask for fewer bytes than this (user issued commands may)
+     */
+    unsigned char min_xfersize;
 
     /*
      * Host has requested that no further requests come through for the
===== drivers/scsi/sd.c 1.108 vs edited =====
--- 1.108/drivers/scsi/sd.c	Fri Apr 18 11:58:55 2003
+++ edited/drivers/scsi/sd.c	Thu Apr 24 11:07:25 2003
@@ -1112,7 +1112,8 @@
 	 * We have to start carefully: some devices hang if we ask
 	 * for more than is available.
 	 */
-	res = sd_do_mode_sense6(sdp, SRpnt, 0, 0x3F, buffer, 4);
+	res = sd_do_mode_sense6(sdp, SRpnt, 0, 0x3F, buffer, 
+				max(sdp->host->min_xfersize, 4));
 
 	/*
 	 * Second attempt: ask for page 0
@@ -1120,7 +1121,8 @@
 	 * Sense Key 5: Illegal Request, Sense Code 24: Invalid field in CDB.
 	 */
 	if (res)
-		res = sd_do_mode_sense6(sdp, SRpnt, 0, 0, buffer, 4);
+		res = sd_do_mode_sense6(sdp, SRpnt, 0, 0, buffer,
+					max(sdp->host->min_xfersize, 4));
 
 	/*
 	 * Third attempt: ask 255 bytes, as we did earlier.
@@ -1153,14 +1155,16 @@
 	const int modepage = 0x08; /* current values, cache page */
 
 	/* cautiously ask */
-	res = sd_do_mode_sense6(sdp, SRpnt, dbd, modepage, buffer, 4);
+	res = sd_do_mode_sense6(sdp, SRpnt, dbd, modepage, buffer,
+				max(sdp->host->min_xfersize, 4));
 
 	if (res == 0) {
 		/* that went OK, now ask for the proper length */
 		len = buffer[0] + 1;
 		if (len > 128)
 			len = 128;
-		res = sd_do_mode_sense6(sdp, SRpnt, dbd, modepage, buffer, len);
+		res = sd_do_mode_sense6(sdp, SRpnt, dbd, modepage, buffer,
+					max(sdp->host->min_xfersize, len));
 	}
 
 	if (res == 0 && buffer[3] + 6 < len) {

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

[Pete]

Andries,

> [In other words, afafc@rnl.ist.utl.pt, mike@hingston.demon.co.uk,
> pwkpete@yahoo.com, stelian@popies.net: did you compare 2.5.68
> without and with this "if (len < 8) len = 8" patch?]
> 

The failures I was seeing was with 2.5.68, after adding your patch the error
went away, but never tried the .67 kernel. I still have the entire kernel error
log of the failure case if that is of interest.

Regards,

-Pete

__________________________________________________
Do you Yahoo!?
The New Yahoo! Search - Faster. Easier. Bingo
http://search.yahoo.com

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

[Matthew Dharm]

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

On Thu, Apr 24, 2003 at 11:08:12AM +0200, Andries.Brouwer@cwi.nl wrote:
>     From: Matthew Dharm <mdharm-scsi@one-eyed-alien.net>
> 
>     > Comments?
> 
>     My only comment is that your sample of failed cases primarily seems to
>     address those devices that support some variant of MODE_SENSE -- there are
>     lots of devices that don't support it at all, which is why I wanted to
>     filter it with the (apparently) now-dead command filter concept.
> 
> Well, before we create a new infrastructure it is a good idea
> to make sure that it is needed. James mentioned three cases
> in Bugzilla and it looks like these are fixed by a small correction
> to the MODE_SENSE to MODE_SENSE_10 translation.
> 
> You mention lots of devices that don't support MODE_SENSE at all.
> Do you have Bugzilla or l-k / l-s / l-u refs?

Unfortunately, my data comes from (a) working with this junk for several
years, and (b) non-public conversations with hardware vendors.  While there
linux-usb-devel/users references, you'll have to google for logs to find
them.

Yeah, I know that's a lousy answer.  But try to realize that I've been
dealing with a long, slow, and painful discovery process of what the
'popular OS' uses.

> (Smart Media and similar cards come with a write protect bit.
> I would suppose reporting it is supported by most non-broken hardware.
> We get it from the header of the MODE_SENSE return value.
> So there is some reason to expect that MODE_SENSE[_10] is supported,
> at least in the minimum form where no page but only the header
> is returned. Closer investigation of other error cases might reveal
> more usb-storage flaws.)

This is actually interesting... smart media readers seem to be some of the
worst offenders in terms of not supporting this command (or at least a
variant of it that I've been able to discover).

Hardware vendors tell me that those which build devices based on older
designs (i.e. real SCSI or ATAPI) do support some version of a modesense
because it carried over from their older designs.  Those which are doing
new designs from the ground up do not support it, as apparently neither
Windows nor MacOS check for write-protect this way.  Or so I'm told by the
people who design these things.

Matt

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

Ye gods! I have feet??!
					-- Dust Puppy
User Friendly, 12/4/1997

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

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

[Andries.Brouwer]

    From: Matthew Dharm <mdharm-scsi@one-eyed-alien.net>

    > Well, before we create a new infrastructure it is a good idea
    > to make sure that it is needed. James mentioned three cases
    > in Bugzilla and it looks like these are fixed by a small correction
    > to the MODE_SENSE to MODE_SENSE_10 translation.
    >
    > You mention lots of devices that don't support MODE_SENSE at all.
    > Do you have Bugzilla or l-k / l-s / l-u refs?

    Unfortunately, my data comes from (a) working with this junk for several
    years, and (b) non-public conversations with hardware vendors.

Yes. So what you are saying is that you have no supporting evidence
for the need of a filtering layer.

    Yeah, I know that's a lousy answer.  But try to realize that I've been
    dealing with a long, slow, and painful discovery process of what the
    'popular OS' uses.

Yes, I do not blame you at all. Nevertheless, we must only do
things when we really understand why. If a filter is really
necessary we'll find people with problems that are cured only
that way. Today we do not know any such people, it seems.

Andries

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

[Matthew Dharm]

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

On Thu, Apr 24, 2003 at 08:59:45PM +0200, Andries.Brouwer@cwi.nl wrote:
>     From: Matthew Dharm <mdharm-scsi@one-eyed-alien.net>
>     Yeah, I know that's a lousy answer.  But try to realize that I've been
>     dealing with a long, slow, and painful discovery process of what the
>     'popular OS' uses.
> 
> Yes, I do not blame you at all. Nevertheless, we must only do
> things when we really understand why. If a filter is really
> necessary we'll find people with problems that are cured only
> that way. Today we do not know any such people, it seems.

Well, at this point, I guess I should just be happy that something is being
done.  But, what I'm really afraid of, this coming right back to this
point.

In the past, I modified the write-protect probing in 2.4.x to be more compatible
with what usb-storage wanted.  But then someone changed it, and it broke.
So we changed it again... and it was fine for a while.  Then someone
changed it in 2.5.x, and it broke again.

I guess what's got me frustrated is that this is very fragile, tends to
cause serious problems when it fails, often gets changed, and isn't really
necessary -- after all, that's why the 'assume write-enabled' code path is
there for when the request fails.

I've just been around and around this merry-go-round, and I'm trying to get
off.  We can fix it now, but I am truly afraid that it's going to get
broken again in 6 months.

And, as for the need for filtering, I know we have such a need.  I have
several devices on my desk which choke at INQUIRY EVPD -- so right now I
filter it in the usb-storage driver.  I also have devices that report bogus
INQUIRY data lengths, but (luckily) the sanity checks in the INQUIRY
probing code generally catch those cases and save us.

I've seen devices that choke on START_STOP, unless that START_STOP is an
eject command.  I fixed this by eliminating the only non-eject use of
START_STOP in sd.c -- but how long is it before someone decides they need
to use START_STOP for something?

We may have fixed MODE_SENSE, but what about the other cases?

Matt

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

G:  Let me guess, you started on the 'net with AOL, right?
C:  WOW! d00d! U r leet!
					-- Greg and Customer 
User Friendly, 2/12/1999

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

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

[James Bottomley]

On Thu, 2003-04-24 at 15:14, Matthew Dharm wrote:
> Well, at this point, I guess I should just be happy that something is being
> done.  But, what I'm really afraid of, this coming right back to this
> point.
> 
> In the past, I modified the write-protect probing in 2.4.x to be more compatible
> with what usb-storage wanted.  But then someone changed it, and it broke.
> So we changed it again... and it was fine for a while.  Then someone
> changed it in 2.5.x, and it broke again.
> 
> I guess what's got me frustrated is that this is very fragile, tends to
> cause serious problems when it fails, often gets changed, and isn't really
> necessary -- after all, that's why the 'assume write-enabled' code path is
> there for when the request fails.
> 
> I've just been around and around this merry-go-round, and I'm trying to get
> off.  We can fix it now, but I am truly afraid that it's going to get
> broken again in 6 months.

To some extent, I'm afraid you can't get off:  you're stuck on here with
the rest of us.

I can't guarantee that changes to the SCSI subsystem won't break USB
devices.  However, what I can guarantee is that we'll work to fix the
problems as they arise.

> And, as for the need for filtering, I know we have such a need.  I have
> several devices on my desk which choke at INQUIRY EVPD -- so right now I
> filter it in the usb-storage driver.  I also have devices that report bogus
> INQUIRY data lengths, but (luckily) the sanity checks in the INQUIRY
> probing code generally catch those cases and save us.

I think the evpd code can be thrown out into user land (where the rules
governing how it's used can be much more flexible).  All we really use
it for is to get a unique name.  However, even WWN inquiries don't
always guarantee even that...

> I've seen devices that choke on START_STOP, unless that START_STOP is an
> eject command.  I fixed this by eliminating the only non-eject use of
> START_STOP in sd.c -- but how long is it before someone decides they need
> to use START_STOP for something?

sd.c uses START_STOP to spin up an inactive disc (but only if we get a
check condition/NOT READY). sr_ioctl.c uses it to eject or close a cd
tray.

> We may have fixed MODE_SENSE, but what about the other cases?

I think we just have to identify them and tackle them in the same way.

Do you have any other current bug reports you'd like to share with us?

James

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

[Matthew Dharm]

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

On Thu, Apr 24, 2003 at 04:20:50PM -0400, James Bottomley wrote:
> > And, as for the need for filtering, I know we have such a need.  I have
> > several devices on my desk which choke at INQUIRY EVPD -- so right now I
> > filter it in the usb-storage driver.  I also have devices that report bogus
> > INQUIRY data lengths, but (luckily) the sanity checks in the INQUIRY
> > probing code generally catch those cases and save us.
> 
> I think the evpd code can be thrown out into user land (where the rules
> governing how it's used can be much more flexible).  All we really use
> it for is to get a unique name.  However, even WWN inquiries don't
> always guarantee even that...

Well, I'd love to see a patch to do that.  But I seem to recall someone
being opposed to that idea.... maybe I'm just imagining that.

> > I've seen devices that choke on START_STOP, unless that START_STOP is an
> > eject command.  I fixed this by eliminating the only non-eject use of
> > START_STOP in sd.c -- but how long is it before someone decides they need
> > to use START_STOP for something?
> 
> sd.c uses START_STOP to spin up an inactive disc (but only if we get a
> check condition/NOT READY). sr_ioctl.c uses it to eject or close a cd
> tray.

sd.c used to send START_STOP to check for inactive disk... I changed that
to use TUR instead.

Tho, the point of my comment is really that people keep changing the SCSI
layer, and it keeps having significant and far-reaching effects, and
without command filtering I'm at their mercy.

But it looks like I've lost that battle.

> > We may have fixed MODE_SENSE, but what about the other cases?
> 
> I think we just have to identify them and tackle them in the same way.
> 
> Do you have any other current bug reports you'd like to share with us?

Well, let's see....

(o) If the second INQUIRY during probing fails, the device is dropped on
the floor.
(o) INQUIRY for anything other than 36-bytes crashes some devices (tho they
are few, since the sanity checking code for length is in place -- the
failure mode here is a bit unusual)
(o) You already know about EVPD....
(o) MODE_SENSE for cache-type for TYPE_DISK crashes quite a few devices

I'm also still waiting for the patches to SCSI to support hot-unplugging,
which is being worked on (but the first version was rejected quite
soundly).

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

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

[Stelian Pop]

On Thu, Apr 24, 2003 at 05:21:07PM +0200, Andries.Brouwer@cwi.nl wrote:

> That is good. But there is one point I do not understand.
> Stelian Pop reports on two devices, both US_SC_UFI.
> But usb_stor_ufi_command() already contained code to
> make the length 8. How can it be that my sd.c change
> improved things?

Good question.

Could it be that the unusual_devs treatment is flawed somewhere ?

I'll enable debug traces in usb-storage tomorrow again and try to
trace this a bit more. If there is something more specific I need
to look at just tell me.

> Can it be that earlier tests were for 2.5.67 and later tests
> for 2.5.68, and Matt's improvements of the latter over the former
> are the real reason of improved behaviour?

No. I've done all tests (both without - resulting in failures - and
with the sd.c patch) with the latest 2.5 bk tree as of this morning.

The sd.c patch *is* needed to make it work.

Stelian.
-- 
Stelian Pop <stelian@popies.net>

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

[Patrick Mansfield]

On Thu, Apr 24, 2003 at 01:59:32PM -0700, Matthew Dharm wrote:
> On Thu, Apr 24, 2003 at 04:20:50PM -0400, James Bottomley wrote:

> > I think the evpd code can be thrown out into user land (where the rules
> > governing how it's used can be much more flexible).  All we really use
> > it for is to get a unique name.  However, even WWN inquiries don't
> > always guarantee even that...
> 
> Well, I'd love to see a patch to do that.  But I seem to recall someone
> being opposed to that idea.... maybe I'm just imagining that.

I wanted it to stay for scsi mid-level multi-path usage, but multi-path
isn't in the kernel, and if multi-path wants to use it in-kernel, we can
carry the deleted code as a patch or better yet use some sort of
user-initiated scanning (maybe not full user scsi scanning for now, just
something that allows synchronized user intervention to get/set id's
between the scsi_scan_host and the scsi_attach_device calls.)

And we want some sort of id for udev, but that can be done in user space.

So kill it before it's too late.

-- Patrick Mansfield

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

[Andries.Brouwer]

> The sd.c patch *is* needed to make it work.

Thanks for confirming.

Now that we know the facts, it is easy to answer my question

: But usb_stor_ufi_command() already contained code to
: make the length 8. How can it be that my sd.c change
: improved things?

The answer is that the length occurs twice - as part of
the SCSI command, and as bufflen. The usb-storage code
that fudges the command in the hope of producing something
more agreeable to the device forgets to adapt the bufflen.

This ends, I think, the discussion of what went wrong.
We understand perfectly - I hope.

Now about fixing the code. We must do something in usb-storage
or sd.c or both. I would prefer if sr_bufflen denotes the
actually available length, instead of the length that
will be transferred. If it has that meaning, then the
fudger in usb-storage knows for sure that there is space.
And sd_do_mode_sense6 and family can store the right number.

Are there any objections against viewing sr_bufflen
as the amount of actually available buffer space
(and thus potentially different from the transferred amount)?

Andries


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

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

[Matthew Dharm]

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

On Fri, Apr 25, 2003 at 02:43:25AM +0200, Andries.Brouwer@cwi.nl wrote:
> Are there any objections against viewing sr_bufflen
> as the amount of actually available buffer space
> (and thus potentially different from the transferred amount)?

YES!  I MOST DEFINITELY OBJECT!

If you look through the linux-scsi archives, you'll see I've discussed this
before.  Apparently 'real' SCSI busses don't have this problem, but
usb-storage needs to know (in advance) how much data is going to be
transferred by a given command.

We used to have a tremendous amount of code which analyized the command in
an attempt to 'deduce' the correct answer.  It was right most of the time,
and failed completely on any vendor-specific command.

If you think about it, it's not really possible to figure out how much data
is going to be transfered based on the command bytes alone.  You may need
to know information such a the block size of the device, the device type,
etc.

Originally, I proposed adding a field to indicate how much data is actually
supposed to be transferred.  People objected to adding (including, I think,
Linus), and the general consensus was that sr_bufflen should indicate the
amount of data to be transferred.  The originator of the command was
responsible for making certain that there was actually that much buffer
available.

Short answer: usb-storage needs to know what the expected transfer amount
is.  The agreed-upon solution was to make sr_bufflen the expected transfer
amount instead of the available buffer size.

Matt

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

It was a new hope.
					-- Dust Puppy
User Friendly, 12/25/1998

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

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

[Alan Stern]

On Fri, 25 Apr 2003 Andries.Brouwer@cwi.nl wrote:

> Now about fixing the code. We must do something in usb-storage
> or sd.c or both. I would prefer if sr_bufflen denotes the
> actually available length, instead of the length that
> will be transferred. If it has that meaning, then the
> fudger in usb-storage knows for sure that there is space.
> And sd_do_mode_sense6 and family can store the right number.

There is a pattern emerging here, and I would like to propose a more 
general strategy for this problem.

We have seen that low-level device drivers such as usb-storage need to 
filter/alter the commands they accept.  Such actions include:

	(1) Refusing to issue a command that is known (or very likely) to 
crash the device's firmware.

	(2) Synthesizing an artificial reply to a command which the device 
doesn't handle well but the SCSI code needs.

	(3) Changing one command to another (e.g., READ(6) -> READ(10)).

	(4) Changing the data length for a command to meet the device's 
restrictions.

There may be other cases as well that I haven't thought of.

To begin with, in my opinion the LLDD should do a minimum of filtering and 
altering.  Not only is it frequently difficult to know if such actions are 
valid or will be acceptable to the caller, there is also the principle 
that user-generated requests (via the scsi-generic interface) should be 
left totally alone -- after all, the user may be probing precisely to 
determine whether or not the device will accept a particular command.

(1) is obviously necessary.  In general, refusing to accept a command is 
better than changing it, IMO.

(2) should be minimized, but there are times when it is unavoidable (such 
as INQUIRY for various USB storage devices that don't support it).

(3) should be avoided completely.  If the device doesn't accept some form 
of a command, then that fact should be transmitted back to the caller.

(4) also should be avoided.

This puts the burden of dealing with partially-compliant devices on the 
SCSI midlayer.  The routines there should be prepared to issue multiple 
forms of a some commands, so that if one form doesn't work another form 
can be tried.  Of course, this is already being done for READ and WRITE.  
A similar strategy could be used for things like MODE-SENSE.  However, 
detailed per-device information should be kept small -- little more than 
"this device likes the 6-byte commands rather than the 10-byte commands".

The same idea goes for retrying commands using different data lengths.  
It would be especially nice if there was a good way for the LLDD to report
that the data length specified was too short (or too long) for the device
to accept.

The filtering done by the LLDD should be relatively simple, and removing
the complex translations that are currently being done would help.  
Whether the filtering relies on centralized library routines (such as Matt
Dharm recently proposed) or a more decentralized scheme doesn't need to be
settled now.  In fact, it can fairly easily be changed later, as usage
patterns dictate.

This approach puts the burden of the various tasks where they belong.  
The LLDD is responsible for keeping the device from crashing, which only
it has the knowledge to do.  The SCSI mid-layer is responsible for the
strategies needed to get a device to carry out a particular action, which
is a problem that extends to all sorts of SCSI devices and is not peculiar
to one particular type.

Alan Stern

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

[Oliver Neukum]

> To begin with, in my opinion the LLDD should do a minimum of filtering and
> altering.  Not only is it frequently difficult to know if such actions are
> valid or will be acceptable to the caller, there is also the principle
> that user-generated requests (via the scsi-generic interface) should be
> left totally alone -- after all, the user may be probing precisely to
> determine whether or not the device will accept a particular command.

Seriously, why this principle? It makes user space drivers inferior in
some respects. After all, we usually require a special capability to
allow actions that can crash hardware.

> (1) is obviously necessary.  In general, refusing to accept a command is
> better than changing it, IMO.

Better it would not be issued in the first place.

[..]
> This puts the burden of dealing with partially-compliant devices on the
> SCSI midlayer.  The routines there should be prepared to issue multiple
> forms of a some commands, so that if one form doesn't work another form
> can be tried.  Of course, this is already being done for READ and WRITE.

Wouldn't it be better if drivers had attributes which tell the midlevel
which forms of the several commands are acceptable?

	Regards
		Oliver

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

[Alan Stern]

On Fri, 25 Apr 2003, Oliver Neukum wrote:

> > To begin with, in my opinion the LLDD should do a minimum of filtering and
> > altering.  Not only is it frequently difficult to know if such actions are
> > valid or will be acceptable to the caller, there is also the principle
> > that user-generated requests (via the scsi-generic interface) should be
> > left totally alone -- after all, the user may be probing precisely to
> > determine whether or not the device will accept a particular command.
> 
> Seriously, why this principle? It makes user space drivers inferior in
> some respects. After all, we usually require a special capability to
> allow actions that can crash hardware.

Do you mean it makes user space drivers inferior to kernel drivers?  I 
don't see how.  The LLDD would treat them both the same, doing a minimum 
of modifications.

If you mean that it would make user space drivers inferior to the way they
work now, then I agree.  At least, they might be inferior in terms of
performance, since they would no longer have commands altered
automatically for their benefit.  On the other hand, they would be
superior in terms of receiving a more transparent channel directly to the
device.  For some people this is very important.

I think it won't really be a problem.  A user-space driver _ought_ to be
prepared to cope with the quirks and special requirements of a particular
device.

> > (1) is obviously necessary.  In general, refusing to accept a command is
> > better than changing it, IMO.
> 
> Better it would not be issued in the first place.

True enough.  I'm not so sure that's really feasible at this point.  A 
large part of the debate that started this thread had to do with finding 
an appropriate way to represent the necessary information, and no 
satisfactory resolution was reached.

> [..]
> > This puts the burden of dealing with partially-compliant devices on the
> > SCSI midlayer.  The routines there should be prepared to issue multiple
> > forms of a some commands, so that if one form doesn't work another form
> > can be tried.  Of course, this is already being done for READ and WRITE.
> 
> Wouldn't it be better if drivers had attributes which tell the midlevel
> which forms of the several commands are acceptable?

Yes it would.  I'm not sure that everybody can agree on a good way to do 
this beyond the most rudimentary aspects.

Alan Stern

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

[Oliver Neukum]

> Do you mean it makes user space drivers inferior to kernel drivers?  I
> don't see how.  The LLDD would treat them both the same, doing a minimum
> of modifications.

You got to make up your mind :-)
So the LLDD should filter in both cases?

> If you mean that it would make user space drivers inferior to the way they
> work now, then I agree.  At least, they might be inferior in terms of
> performance, since they would no longer have commands altered
> automatically for their benefit.  On the other hand, they would be
> superior in terms of receiving a more transparent channel directly to the
> device.  For some people this is very important.

For the larger part it's vastly important that exactly that doesn't happen.
Eg. I want the user to have access to a device, but I don't want him to
be able to crash the device. It's a multiuser system and others might
want to use the device afterwards.
We usually require root level to get a transparent channel to hardware.

If filtering is marginal you need to use yet another demon to be checked for
buffer overflows, etc. ... .

> I think it won't really be a problem.  A user-space driver _ought_ to be
> prepared to cope with the quirks and special requirements of a particular
> device.

But can you depend on it? From an administration viewpoint you create a
hole for a DoS attack.

	Regards
		Oliver

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

[Alan Stern]

On Sat, 26 Apr 2003, Oliver Neukum wrote:

> > Do you mean it makes user space drivers inferior to kernel drivers?  I
> > don't see how.  The LLDD would treat them both the same, doing a minimum
> > of modifications.
> 
> You got to make up your mind :-)
> So the LLDD should filter in both cases?

Maybe I wasn't clear enough.  I want the LLDD to do exactly the same thing
regardless of where the request originates from.  I want it to do a
minimum of filtering, really just filtering out commands that are known to
crash the device.  I want it to do essentially no alteration of commands,
just enough to work at all.

In fact, I would argue that usb-storage is already doing too much.  It 
should not be converting between the 6-byte and 10-byte forms for several 
commands.  The caller should be smart enough to handle that.

The changes I'm proposing aren't that large.  There's just a couple of
rules the SCSI code should follow:

	(o) Use the 10-byte command form by default.  If it doesn't work, 
try the 6-byte form, and if that works better then flag the device entry 
as requiring the 6-byte form.  The READ and WRITE commands are already 
handled this way.

The MODE-SENSE problem that Andries was addressing wouldn't exist if this
rule had been followed.  The code tried the 6-byte form first and then did
not fall back to the 10-byte form when that failed.

	(o) Request the smallest-size data transfer that includes the data 
needed.  This is kind of vague, but I mean something like what the INQUIRY 
code now does: start with a minimum-sized 36-byte transfer and from that 
determine how much more is needed and supported.  Maybe try to stick with 
sizes that are commonly used by other popular OS's.

	If these two rules were followed carefully, we could remove a lot
of excess code from usb-storage and things would work more reliably.  So
far as I can see, the commands that would be affected are REQUEST-SENSE,
MODE-SENSE, MODE_SELECT, and maybe INQUIRY.

> > If you mean that it would make user space drivers inferior to the way they
> > work now, then I agree.  At least, they might be inferior in terms of
> > performance, since they would no longer have commands altered
> > automatically for their benefit.  On the other hand, they would be
> > superior in terms of receiving a more transparent channel directly to the
> > device.  For some people this is very important.
> 
> For the larger part it's vastly important that exactly that doesn't happen.
> Eg. I want the user to have access to a device, but I don't want him to
> be able to crash the device. It's a multiuser system and others might
> want to use the device afterwards.
> We usually require root level to get a transparent channel to hardware.

Nothing I propose involves relaxing permissions checking for access to
devices.  If root privileges were required before they will still be
required.  In addition, I am also proposing that the LLDD continue to
filter out commands that are known to crash the device.

If a user program wants convenient access to a device with the kernel
doing all the appropriate conversions to make things work, then the
program should be a SCSI _client_ and use the sd or sr facilities.  A
user-space _driver_ naturally must accept more responsibility for making
things work okay.

> If filtering is marginal you need to use yet another demon to be checked for
> buffer overflows, etc. ... .

No more than any other daemon or device driver requires.

> > I think it won't really be a problem.  A user-space driver _ought_ to be
> > prepared to cope with the quirks and special requirements of a particular
> > device.
> 
> But can you depend on it? From an administration viewpoint you create a
> hole for a DoS attack.

That's true for any user-space driver.  Consider an X11 server, for
example.  Of course a buggy user-space USB-SCSI driver can cause problems,
just as a buggy X11 server can.  The same mechanisms that protect against
one can protect against the other.

Alan Stern

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

[Andries.Brouwer]

[[problem solved, we understand the hardware, it seems,
now wish to find an elegant way of doing what is required
(and only what is required) in the scsi and/or usb storage code]]

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

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

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

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

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:

(a) Matt tells about sr_bufflen.
(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.)

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:

-
-#define USB_STOR_SCSI_SENSE_HDRSZ 4
-#define USB_STOR_SCSI_SENSE_10_HDRSZ 8
-
-struct usb_stor_scsi_sense_hdr
-{
-  __u8* dataLength;
-  __u8* mediumType;
...
-       }
-    }
-
-  /* Set value of length passed in */
-  *length_p = length;
-}
-

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

The next part, in drivers/usb/storage/protocol.c, is almost
as satisfactory:

...
-       /* determine the correct (or minimum) data length for these commands */
-       switch (srb->cmnd[0]) {
-
-               /* change MODE_SENSE/MODE_SELECT from 6 to 10 byte commands */
-       case MODE_SENSE:
-       case MODE_SELECT:
-               /* save the command so we can tell what it was */
-               old_cmnd = srb->cmnd[0];
...
-               /* change READ_6/WRITE_6 to READ_10/WRITE_10, which
-                * are ATAPI commands */
-       case WRITE_6:
-       case READ_6:
-               srb->cmnd[11] = 0;
-               srb->cmnd[10] = 0;
...

(deleted some 200 lines changing READ_6 into READ_10, etc.)

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

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

in the declaration of struct scsi_device.

Now sd_do_mode_sense6() becomes sd_do_mode_sense() and does

+       if (SRpnt->sr_device->use_10_for_ms) {
+               if (len < 8)
+                       len = 8;
+
+               cmd[0] = MODE_SENSE_10;
+               cmd[8] = len;
+       } else {
+               if (len < 4)
+                       len = 4;
+
+               cmd[0] = MODE_SENSE;
+               cmd[4] = len;
+       }

(and its first parameter can go).

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.

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?

Comments?

Andries

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

[Matthew Dharm]

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

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

[James Bottomley]

On Sat, 2003-04-26 at 16:44, Andries.Brouwer@cwi.nl wrote:
> [[problem solved, we understand the hardware, it seems,
> now wish to find an elegant way of doing what is required
> (and only what is required) in the scsi and/or usb storage code]]
> 
> (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.

definitely agreed.

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

This looks fine to me.  Originally, sr_bufflen was exactly that (because
all SCSI LLDs have to be prepared to transfer fewer than the specified
number of bytes anyway, since that's required by the spec).  I believe
only USB needs this to be set to the actual command length (and further
require that we ensure that the device *will* transfer exactly
this...that's to some extent the origin of the INQUIRY problems), so the
change should be small.

[...]

> I dislike the choice of identifier "ten" - it is difficult to grep for,

agreed.

> 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 */
> 
> in the declaration of struct scsi_device.
> 
> Now sd_do_mode_sense6() becomes sd_do_mode_sense() and does

This looks good (probably this should go in scsi_lib.c and sr should
start using it for consistency).

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

Well, this should be done in the USB storage slave_configure...that's
almost exactly what the interface is designed for (altering scsi_device
parameters before the SCSI subsystem starts using it).

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

This sounds to be exactly the right approach (other than the missing
slave_configure).

James

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

[James Bottomley]

On Sat, 2003-04-26 at 17:13, Matthew Dharm wrote:
> > 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....

What's actually wrong with simply parsing the commands?  For all SCSI
Spec commands there's a well known algorithm to get the transfer size
from the command group.  For vendor specific commands, well the
mid-layer doesn't issue them so you'd just have to rely on the user
doing it to pass the correct buffer length (which is no different really
from what happens today).

James

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

[Patrick Mansfield]

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

> Well, this should be done in the USB storage slave_configure...that's
> almost exactly what the interface is designed for (altering scsi_device
> parameters before the SCSI subsystem starts using it).

I prefer Matt's suggestion, that we handle this similar to sd.c use of
use_10_for_rw so the alogrithm works for all devices independent of the
LLDD.

Is MODE SENSE 10 used by default in other OSes and more likely to work
than MODE SENSE 6?

-- Patrick Mansfield

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

[Matthew Dharm]

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

On Sat, Apr 26, 2003 at 05:43:43PM -0500, James Bottomley wrote:
> On Sat, 2003-04-26 at 17:13, Matthew Dharm wrote:
> > > 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....
> 
> What's actually wrong with simply parsing the commands?  For all SCSI
> Spec commands there's a well known algorithm to get the transfer size
> from the command group.  For vendor specific commands, well the
> mid-layer doesn't issue them so you'd just have to rely on the user
> doing it to pass the correct buffer length (which is no different really
> from what happens today).

To parse correctly, I would need to remember what type of device it is by
snooping the INQUIRY data.  I would also have to keep around data such as
block size (for TYPE_DISK).  I haven't even looked at what extra data I
would need to keep around for all the other types.

Why should usb-storage do that when someone else (the command source)
already knows this data?  Whatever is sending the command already knows how
much data to expect -- my reparsing the command to try to figure it out is
guaranteed to be less precise than just using the correct answer from the
source of the command.

Also, user-issued commands can specify buffer size and transfer length
separately, but only one value gets given to the usb-storage driver.  The
other is dropped on the floor somewhere in the mid-layer.

In the end, I guess the short-answer to 'what is wrong with parsing' is
that we tried it for over a year, and it kept biting us on the ass with
weird corner-cases and strange devices.

Matt

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

We can customize our colonels.
					-- Tux
User Friendly, 12/1/1998

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

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

[Matthew Dharm]

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

On Sat, Apr 26, 2003 at 05:29:55PM -0500, James Bottomley wrote:
> This looks fine to me.  Originally, sr_bufflen was exactly that (because
> all SCSI LLDs have to be prepared to transfer fewer than the specified
> number of bytes anyway, since that's required by the spec).  I believe
> only USB needs this to be set to the actual command length (and further
> require that we ensure that the device *will* transfer exactly
> this...that's to some extent the origin of the INQUIRY problems), so the
> change should be small.

This is a common misconception.  usb-storage does not need to know, in
advance, exactly how many bytes will transfer.  It does, however, need to
know exactly how many bytes are being requested for transfer.

There's a subtle difference there.  usb-storage can handle the case of
underrun or overrun properly.  But there are USB<->[SCSI|ATA|whatever]
bridges which can't handle a discrepancy between expected transfer length
and actual transfer length when no error was detected.

I can give examples, but it's confusing and not really relevant unless
someone wants to know.

There are also broken devices which are specifically broken with respect to
the INQUIRY command for length != 36, but that's a separate issue to a
certain degree.  These are really two separate problems.

Matt

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

YOU SEE!!?? It's like being born with only one nipple!
					-- Erwin
User Friendly, 10/19/1998

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

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

[James Bottomley]

On Sat, 2003-04-26 at 20:34, Matthew Dharm wrote:
> To parse correctly, I would need to remember what type of device it is by
> snooping the INQUIRY data.  I would also have to keep around data such as
> block size (for TYPE_DISK).  I haven't even looked at what extra data I
> would need to keep around for all the other types.

OK, I don't understand this.  To extract the transfer length from a
given SCSI command (excluding vendor specific ones) is a well defined
process.  All you need is the command, it doesn't require knowing device
type or anything else.

> Why should usb-storage do that when someone else (the command source)
> already knows this data?  Whatever is sending the command already knows how
> much data to expect -- my reparsing the command to try to figure it out is
> guaranteed to be less precise than just using the correct answer from the
> source of the command.

Because we're exploring options for this "lengthen the transfer size"
behaviour the USB layer exhibits.  Assuming you can transfer more bytes
than the indicated buffer size is a recipe for data corruption problems
down the line.

> Also, user-issued commands can specify buffer size and transfer length
> separately, but only one value gets given to the usb-storage driver.  The
> other is dropped on the floor somewhere in the mid-layer.

I usually use the SCSI_IOCTL_SEND_COMMAND, which only allows the
specification of a buffer size (rather than both length and buffer
size), how do you send SCSI commands?

> In the end, I guess the short-answer to 'what is wrong with parsing' is
> that we tried it for over a year, and it kept biting us on the ass with
> weird corner-cases and strange devices.

Well, the only weird corner cases are vendor specific commands and for
these the rule is that we assume the buffer size and the transfer size
to be the same.  Since they aren't currently issued by the mid-layer
this must be true today (otherwise they wouldn't work today), so I think
we have everything covered.

James

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

[Andries.Brouwer]

Matt asks:

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

Yes, probably that is OK.
I wanted to rewrite code without changing behaviour, and have no
opinion about how non-USB devices might react to 10-byte MODE_SENSE.


James advises:

# Well, this should be done in the USB storage slave_configure

Ah, of course.
All docs say that this routine should call scsi_adjust_queue_depth.
Is that really true? When reading the docs one gets the impression
that our code is
	if (slave_configure)
		slave_configure();
	else
		scsi_adjust_queue_depth(default);
But I read in scsi_scan.c:
        if(sdev->host->hostt->slave_configure)
                sdev->host->hostt->slave_configure(sdev);

Andries

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

[James Bottomley]

On Sat, 2003-04-26 at 21:29, Andries.Brouwer@cwi.nl wrote:
> All docs say that this routine should call scsi_adjust_queue_depth.
> Is that really true? When reading the docs one gets the impression
> that our code is
> 	if (slave_configure)
> 		slave_configure();
> 	else
> 		scsi_adjust_queue_depth(default);
> But I read in scsi_scan.c:
>         if(sdev->host->hostt->slave_configure)
>                 sdev->host->hostt->slave_configure(sdev);
> 


Well, its design purpose was for queue depth adjustment, but it's
ideally placed to modify any parameters about the device we choose,
hence it could set the appropriate mode sense flag for USB storage.

The rework of the SCSI code means that you don't get tag command
queueing without calling adjust_queue_depth from the slave_configure
routine.  The default is not to do it. However, the slab allocation of
commands rather took the teeth out of it for queue depth adjustment.

James

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

[Matthew Dharm]

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

On Sat, Apr 26, 2003 at 09:15:07PM -0500, James Bottomley wrote:
> On Sat, 2003-04-26 at 20:34, Matthew Dharm wrote:
> > To parse correctly, I would need to remember what type of device it is by
> > snooping the INQUIRY data.  I would also have to keep around data such as
> > block size (for TYPE_DISK).  I haven't even looked at what extra data I
> > would need to keep around for all the other types.
> 
> OK, I don't understand this.  To extract the transfer length from a
> given SCSI command (excluding vendor specific ones) is a well defined
> process.  All you need is the command, it doesn't require knowing device
> type or anything else.

Okay.  A READ_10 command for 4 blocks.  How many bytes is that?  The
command bytes in question look something like this:
	0x28 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x4 0x0

I don't see a way to know the byte count without having additional
knowledge about the device.  Maybe I'm wrong -- I certainly don't claim to
be a SCSI expert.  So explain it to me.

> > Why should usb-storage do that when someone else (the command source)
> > already knows this data?  Whatever is sending the command already knows how
> > much data to expect -- my reparsing the command to try to figure it out is
> > guaranteed to be less precise than just using the correct answer from the
> > source of the command.
> 
> Because we're exploring options for this "lengthen the transfer size"
> behaviour the USB layer exhibits.  Assuming you can transfer more bytes
> than the indicated buffer size is a recipe for data corruption problems
> down the line.

My point in asking the question was to emphasize that I do not believe the
transfer length to be calculable (only guessable) -- why guess when some
bit of code has to know?

The problem if transferring more data than the buffer has room for is an
obvious problem -- that I don't debate.

> > Also, user-issued commands can specify buffer size and transfer length
> > separately, but only one value gets given to the usb-storage driver.  The
> > other is dropped on the floor somewhere in the mid-layer.
> 
> I usually use the SCSI_IOCTL_SEND_COMMAND, which only allows the
> specification of a buffer size (rather than both length and buffer
> size), how do you send SCSI commands?

Okay, I'm wrong here.  I would have sworn that the SG3 interface allowed me
to specify the two values separately -- I have quite a bit of code that
uses that interface.

Interestinly enough, the one value it specifies it defines as the length of
the transfer, not the size of the buffer -- at least, according to the sg
HOWTO.

Matt

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

Hey, has anyone seen the Microsoft sales guy? It's his feeding time...
					-- Mike
User Friendly, 4/17/1998

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

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

[James Bottomley]

On Sun, 2003-04-27 at 04:35, Matthew Dharm wrote:
> I don't see a way to know the byte count without having additional
> knowledge about the device.  Maybe I'm wrong -- I certainly don't claim to
> be a SCSI expert.  So explain it to me.

Certainly.  It's documented in the SCSI3 Primary commands (for
reference, I'm using SPC-3 r12), section 4.3.

The SCSI opcode is divided into a group code (bits 7-5) and a Command
Code (bits 4-0), see section 4.3.4.1.

The group code identifies the command length:

Group		Length
0		6 byte
1		10 byte
2		10 byte
3		reserved (except for 0x7f, the variable length commands)
4		16 byte
5		12 byte
6		vendor specific
7		vendor specific

Thus, when you | 0x40 into MODE_SENSE, you change it from group0 to
group2 and it becomes a ten byte command.

For each of the non vendor specific groups, called the fixed length
commands, the location of the transfer count in each command is laid out
in the standard, too, section 4.3.3 (as well as other things like the
block location).

They are

Command	Size	transfer offset		transfer size
6 byte		byte 4			1 byte
10 byte		byte 7			2 bytes
12 byte		byte 6			4 bytes
16 byte		byte 10			4 bytes
32 byte(*)	byte 28			2 bytes

* The 32 byte commands are variable length commands (opcode 0x7f) with
Additional CDB length set to 0x18.  The actual command is specified in
the service action field (two bytes).

So the algorithm is quite simple: you get the command length from the
group (using a table with one exception for the 32 byte commands), and
then extract the transfer length from the command

For the other reserved or vendor specific commands, you just return -1
or some other indicator that we have to believe the buffer length to be
the command length.

James

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

[Kai Makisara]

On Sun, 27 Apr 2003, James Bottomley wrote:

> On Sun, 2003-04-27 at 04:35, Matthew Dharm wrote:
> > I don't see a way to know the byte count without having additional
> > knowledge about the device.  Maybe I'm wrong -- I certainly don't claim to
> > be a SCSI expert.  So explain it to me.
>
> Certainly.  It's documented in the SCSI3 Primary commands (for
> reference, I'm using SPC-3 r12), section 4.3.
>
...
> For each of the non vendor specific groups, called the fixed length
> commands, the location of the transfer count in each command is laid out
> in the standard, too, section 4.3.3 (as well as other things like the
> block location).
>
Quoting from 4.3.1:
---
The fields shown in 4.3.2 and 4.3.3 and described in 4.3.4 are used
consistently by most commands. However, the actual usage of any field
(except OPERATION CODE and CONTROL) is described in the subclause defining
that command.
---

This does not say that _all_ commands behave according to this
description. An example is the READ_6 command for sequential access
devices. The opcode defines a 6 byte command. Transfer length in this
command occupies bytes 2 - 4 (3 bytes instead of 1). This does not
necessarily specify the transfer length in bytes. Bit 0 of byte 1
specifies if the command uses fixed block size or variable block size. If
fixed block size is specified, the transfer length field has to be
multiplied by the block size. This may have been earlier specified using
a MODE SELECT command (in block descriptor). However, the MODE SELECT may
not have been used if the user knows the block size. The mid-level code
should the perform and extra MODE SENSE to get the blocks size. In case
of a very old device, MODE SENSE may not even be supported (optional in
SCSI-1).

I would say the mid-level should not try to determine the transfer
length ;-(

-- 
Kai

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

[Matthew Dharm]

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

On Sun, Apr 27, 2003 at 10:41:45AM -0500, James Bottomley wrote:
> So the algorithm is quite simple: you get the command length from the
> group (using a table with one exception for the 32 byte commands), and
> then extract the transfer length from the command

I'm with you up to about this point.

So tell me how many bytes my command wanted to move?  READ_10 specifies the
transfer length is 'blocks'.  How many bytes are in a block is
device-dependent.

Some commands (i.e. INQUIRY) specify transfer length in bytes.  Others
(i.e. READ_10) specify it in blocks, and the size of a block is not known
based simply on the current command.

Matt

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

Okay, this isn't funny anymore! Let me down!  I'll tell Bill on you!!
					-- Microsoft Salesman
User Friendly, 4/1/1998

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

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

[Luben Tuikov]

Matthew Dharm wrote:
> On Sun, Apr 27, 2003 at 10:41:45AM -0500, James Bottomley wrote:
> 
>>So the algorithm is quite simple: you get the command length from the
>>group (using a table with one exception for the 32 byte commands), and
>>then extract the transfer length from the command
> 
> 
> I'm with you up to about this point.
> 
> So tell me how many bytes my command wanted to move?  READ_10 specifies the
> transfer length is 'blocks'.  How many bytes are in a block is
> device-dependent.
> 
> Some commands (i.e. INQUIRY) specify transfer length in bytes.  Others
> (i.e. READ_10) specify it in blocks, and the size of a block is not known
> based simply on the current command.

Matt,

You cannot really constrict yourself to the CDB only.

As a transport/interconnect (what a LLDD is, as well as USB Storage)
you get an Execute Command remote procedure call, in SCSI Core this
is represented by struct scsi_cmnd.

Execute Command rpc contains all that the transport/ic needs in order
to deliver the command to the device server (who will execute the
CDB) and perform (the interconnect) the data transfer.

The reference you want to take a peek at is SAM-3r6, 5.1 --
this is what USB storage is working with, not just the CDB.

Now, you've been talking about the Data-Out buffer size.
This is either ALLOCATION LENGTH (normally bytes), or
TRANFER LENGTH (READ/WRITE, normally blocks).

The reference here is SPC3r12, 4.3.4.4 Transfer Length and
4.3.4.6 Allocation Length.

Please note that Data-Out buffer size *is* the
Expected (maxumum) Data Tranfer Length, the device
server may tranfer less data, (request_bufflen, bufflen).
This is what the standards specify, and what SCSI Core
sets it to.  The actual memory buffer may be bigger.
The device server may transfer less data, but not more.
The device server will NOT modify the returned data
to reflect the insufficient data-in buffer.

So to repeat what the standards say and what SCSI Core
does: sr_bufflen, request_buflen, buflen *is* the
Expected (maxumum) Data Tranfer Length:
	- when it means Allocation Length it is the
maximum available space (e.g. IQUIRY, Request Sense),
(The transport CANNOT write more data than this!)
	- when it means Transfer Length it is the
Expected Data Transfer Length (READ/WRITE), and I say
``expected'' less any End Of Media or similar errors pop up,
(i.e. number of blocks from CDB * block_size = this value).

I.e. the _actual_ buffer size *is* IRRELEVANT to LLDD and
the Transport/IC -- since it maybe an offset to another yet
bigger buffer and SCSI Core/Appl. Client maybe requesting
part of some data (see SAM-3r6, 5.4.3).

Some transports provide an Overflow and Underflow bits in
the transport protocol to let the Initiator port (LLDD)
know about any residuals -- SCSI Core struct scsi_cmnd supports
only an underflow residual (bytes that were not transferred out
of the number of bytes that were expected to be transferred).

I don't know much about USB Storage, but what do you think
about this: add an unsigned overflow_resid to struct scsi_cmnd,
and set it to an applicable value when a USB storage device
provided more data to be transferred, but the buffer was too small.
(I can see how this will not work, if you need a large enough ptr
to just write the data to...)

-- 
Luben

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

[Luben Tuikov]

Luben Tuikov wrote:
> 
> Matt,
> 
> You cannot really constrict yourself to the CDB only.
> 
> As a transport/interconnect (what a LLDD is, as well as USB Storage)
> you get an Execute Command remote procedure call, in SCSI Core this
> is represented by struct scsi_cmnd.
> 
> Execute Command rpc contains all that the transport/ic needs in order
> to deliver the command to the device server (who will execute the
> CDB) and perform (the interconnect) the data transfer.
> 
> The reference you want to take a peek at is SAM-3r6, 5.1 --
> this is what USB storage is working with, not just the CDB.
> 
> Now, you've been talking about the Data-Out buffer size.

Corr: Data-In

> This is either ALLOCATION LENGTH (normally bytes), or
> TRANFER LENGTH (READ/WRITE, normally blocks).
> 
> The reference here is SPC3r12, 4.3.4.4 Transfer Length and
> 4.3.4.6 Allocation Length.
> 
> Please note that Data-Out buffer size *is* the

Corr: Data-In

> Expected (maxumum) Data Tranfer Length, the device
> server may tranfer less data, (request_bufflen, bufflen).
> This is what the standards specify, and what SCSI Core
> sets it to.  The actual memory buffer may be bigger.
> The device server may transfer less data, but not more.
> The device server will NOT modify the returned data
> to reflect the insufficient data-in buffer.
> 
> So to repeat what the standards say and what SCSI Core
> does: sr_bufflen, request_buflen, buflen *is* the
> Expected (maxumum) Data Tranfer Length:
>     - when it means Allocation Length it is the
> maximum available space (e.g. IQUIRY, Request Sense),
> (The transport CANNOT write more data than this!)
>     - when it means Transfer Length it is the
> Expected Data Transfer Length (READ/WRITE), and I say
> ``expected'' less any End Of Media or similar errors pop up,
> (i.e. number of blocks from CDB * block_size = this value).
> 
> I.e. the _actual_ buffer size *is* IRRELEVANT to LLDD and
> the Transport/IC -- since it maybe an offset to another yet
> bigger buffer and SCSI Core/Appl. Client maybe requesting
> part of some data (see SAM-3r6, 5.4.3).
> 
> Some transports provide an Overflow and Underflow bits in
> the transport protocol to let the Initiator port (LLDD)
> know about any residuals -- SCSI Core struct scsi_cmnd supports
> only an underflow residual (bytes that were not transferred out
> of the number of bytes that were expected to be transferred).
> 
> I don't know much about USB Storage, but what do you think
> about this: add an unsigned overflow_resid to struct scsi_cmnd,
> and set it to an applicable value when a USB storage device
> provided more data to be transferred, but the buffer was too small.
> (I can see how this will not work, if you need a large enough ptr
> to just write the data to...)
> 

-- 
Luben

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

[Matthew Dharm]

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

On Mon, Apr 28, 2003 at 03:05:42PM -0400, Luben Tuikov wrote:
> Now, you've been talking about the Data-Out buffer size.
> This is either ALLOCATION LENGTH (normally bytes), or
> TRANFER LENGTH (READ/WRITE, normally blocks).

Now we're making progess.

> So to repeat what the standards say and what SCSI Core
> does: sr_bufflen, request_buflen, buflen *is* the
> Expected (maxumum) Data Tranfer Length:
> 	- when it means Allocation Length it is the
> maximum available space (e.g. IQUIRY, Request Sense),
> (The transport CANNOT write more data than this!)
> 	- when it means Transfer Length it is the
> Expected Data Transfer Length (READ/WRITE), and I say
> ``expected'' less any End Of Media or similar errors pop up,
> (i.e. number of blocks from CDB * block_size = this value).

So, to paraphrase and make certain I understand:  For a certain class of
commands, buflen is the ALLOCATION LENGTH.  Right now, we guarantee that
this matches the 'expected transfer length', but that's only by convention.

For the other class of commands, it's a TRANSFER_LENGTH, which we guarantee
to be the 'expected transfer length'.

You know, it occurs to me that we're talking in circles.  The only reason
this is an issue is because usb-storage re-writes commands into something
which is more acceptable -- if we eliminate this re-writing with a patch
similar to Andries', the problem goes away.

Matt

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

A:  The most ironic oxymoron wins ...
DP: "Microsoft Works"
A:  Uh, okay, you win.
					-- A.J. & Dust Puppy
User Friendly, 1/18/1998

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

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

[Luben Tuikov]

Matthew Dharm wrote:
> 
>>So to repeat what the standards say and what SCSI Core
>>does: sr_bufflen, request_buflen, buflen *is* the
>>Expected (maxumum) Data Tranfer Length:
>>	- when it means Allocation Length it is the
>>maximum available space (e.g. IQUIRY, Request Sense),
>>(The transport CANNOT write more data than this!)
>>	- when it means Transfer Length it is the
>>Expected Data Transfer Length (READ/WRITE), and I say
>>``expected'' less any End Of Media or similar errors pop up,
>>(i.e. number of blocks from CDB * block_size = this value).
> 
> 
> So, to paraphrase and make certain I understand:  For a certain class of
> commands, buflen is the ALLOCATION LENGTH.  Right now, we guarantee that
> this matches the 'expected transfer length', but that's only by convention.

ALLOCATION LENGTH is for things like INQUIRY and REQUEST SENSE,
and its metric is bytes.

It is *this* value which the device server may send less data
or may _have_ more data than the length of the Data-In buffer.

I.e. it is this value for which we are not certain, this is to say,
the data-in buffer size (request_buflen) is a guess -- the device
may have more or less data to send.

(all directions are always with respect to the Initiator (SCSI Core)).

> For the other class of commands, it's a TRANSFER_LENGTH, which we guarantee
> to be the 'expected transfer length'.

In general -- yes.  When it is TRANSFER LENGTH (e.g. READ/WRITE), you
can be more or less certain that exactly that many blocks will
be transferred.  I.e. you can fully rely on request_buflen (bytes).
The exception is when an error occurs, like End Of Partition
or End of Media.
 
> You know, it occurs to me that we're talking in circles.  The only reason
> this is an issue is because usb-storage re-writes commands into something
> which is more acceptable -- if we eliminate this re-writing with a patch
> similar to Andries', the problem goes away.

As long as any re-writing is *not* arbitrary *and* is reversibe on
the way up.

As Alan Stern mentioned, it may be the case that the command sent is
sent as intended to test the device (or whatever).  If this will
crash the device, USB storage should return the appropriate sense data
or response code. If this will crash the system, SCSI Core
should return the appropriate response/error code.

All, please remember that we're providing mechanism, NOT policy.

So, ok, let us (SCSI Core) send the 10 byte versions of commands, unless
the device server reports CHECK CONDITION status with the sense key set to ILLEGAL REQUEST and an additional sense code of INVALID FIELD IN CDB.
(as per the standard), and then we drop to 6 (rarely).

Inquiry first 36, then more if needed.

As for REQUEST SENSE, the standards specify that in order to guarantee
that all sense data has been retrieved, ALLOCATION LENGTH should be 252.
And if less is specified, then the sense data is lost as REQUEST SENSE
clears the sense data buffer of the device server.

In this case you cannot blame SCSI Core or the application client,
and will have to filter it yourself or provide a settable filter
as per Alan Cox's suggestion for a filter (I think you had the same idea:
the filter framework is universal, but the values are per host/device settable).

(Because SCSI Core/Appl. Client have full right to request 252 bytes
in the ALLOCATION LENGTH field.)

In general, we cannot change SCSI Core much more than what SAM-3 and
SPC-3 specify in order to accomodate broken device servers of
a particular SCSI transport -- the transport is invisible to SCSI Core.

There can and will be a compromise, but only a LLDD knows the particulars
of the transport/ic and the device to do proper job in filtering or
whatever is necessary in order to deliver the command to the device --
but it will have to really be a minimalistic filter.

So overall I see a great improvement in SCSI Core regarding those
issues, beginning with Andries' patch (but buffer extents will have
to be checked!).

-- 
Luben



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel