Re: [usb-storage] Re: 2 PATCHES: fix request_tranferlength

Re: [usb-storage] Re: 2 PATCHES: fix request_tranferlength

[Andries.Brouwer]

    I haven't seen these patches integrated into a kernel yet... have I just
    missed them, or is there some reason they have been rejected?

    > +++ b/drivers/scsi/scsi_scan.c    Sun Jul 21 00:55:37 2002
    > -              256, SCSI_TIMEOUT+4*HZ, 3);
    > +              scsi_cmd[4], SCSI_TIMEOUT+4*HZ, 3);

    > +++ b/drivers/scsi/sd.c    Sun Jul 21 00:55:44 2002
    > -              512, SD_TIMEOUT, MAX_RETRIES);
    > +              255, SD_TIMEOUT, MAX_RETRIES);

There are many more possibilities.
Since the number of hours in a day is very finite, Linus cannot
look at everything, so even if the patch is perfect it can be ignored.

There is not really a SCSI maintainer, but there are people that are
well-known as active in the SCSI area. You may try to submit via
one of them.

But then the question is whether these are good patches.

I booted 2.5.29 earlier this evening and was greeted by
kernel BUG at transport.c: 351 and
kernel BUG at scsiglue.c: 150.
(And the usb-storage module now hangs initializing; rmmod fails,
reboot is necessary.)

The former BUG_ON wants to have len == srb->request_bufflen.
But I don't know whether that is wise. It is less confusing
when bufflen indicates the size of the buffer available,
rather than the number of bytes you expect to move.
It is not unusual to get one byte more than you asked for,
in case you ask for an odd number of bytes.

So, maybe these patches aren't clear improvements.

Andries

Re: [usb-storage] Re: 2 PATCHES: fix request_tranferlength

[Matthew Dharm]

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

ARGH!!

Okay search the linux-scsi archives for May 29-30.  We've had this
discussion before.

At that time, I proposed adding a field to indicate the actual amount of
data being requested, as opposed to the buffer size.  At the time (again,
see the archives), I was told that the request_bufflen field _was_ that
field.... any case where the request_bufflen didn't match up with the
command was a bug and needed to be fixed.

So which is it?  This needs to be fixed one way or another.

Emulated SCSI busses _need_ to know how much data to move.  usb-storage has
a large table to try to figure it out, but it only gets the correct answer
in _most_ cases, not all cases.  Only the originator of the command truly
knows how much data is going to be moved, so the only mechanism that makes
sense is to make the originator of the command indicate that number via
some mechanism.

Matt

On Sun, Jul 28, 2002 at 12:48:03AM +0200, Andries.Brouwer@cwi.nl wrote:
>     I haven't seen these patches integrated into a kernel yet... have I just
>     missed them, or is there some reason they have been rejected?
> 
>     > +++ b/drivers/scsi/scsi_scan.c    Sun Jul 21 00:55:37 2002
>     > -              256, SCSI_TIMEOUT+4*HZ, 3);
>     > +              scsi_cmd[4], SCSI_TIMEOUT+4*HZ, 3);
> 
>     > +++ b/drivers/scsi/sd.c    Sun Jul 21 00:55:44 2002
>     > -              512, SD_TIMEOUT, MAX_RETRIES);
>     > +              255, SD_TIMEOUT, MAX_RETRIES);
> 
> There are many more possibilities.
> Since the number of hours in a day is very finite, Linus cannot
> look at everything, so even if the patch is perfect it can be ignored.
> 
> There is not really a SCSI maintainer, but there are people that are
> well-known as active in the SCSI area. You may try to submit via
> one of them.
> 
> But then the question is whether these are good patches.
> 
> I booted 2.5.29 earlier this evening and was greeted by
> kernel BUG at transport.c: 351 and
> kernel BUG at scsiglue.c: 150.
> (And the usb-storage module now hangs initializing; rmmod fails,
> reboot is necessary.)
> 
> The former BUG_ON wants to have len == srb->request_bufflen.
> But I don't know whether that is wise. It is less confusing
> when bufflen indicates the size of the buffer available,
> rather than the number of bytes you expect to move.
> It is not unusual to get one byte more than you asked for,
> in case you ask for an odd number of bytes.
> 
> So, maybe these patches aren't clear improvements.
> 
> Andries

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

I could always suspend a few hundred accounts and watch what happens.
					-- Tanya
User Friendly, 7/31/1998

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

Re: [usb-storage] Re: 2 PATCHES: fix request_tranferlength

[Douglas Gilbert]

Matt,
I have no objection to adding a field in Scsi_Request
called something like 'dxfer_len' (CAM name) that makes
its intent pretty clear. We probably have to keep
'bufflen' there for the time being. 'bufflen' is pretty 
clear in the non scatter gather case (i.e. number of bytes 
in 'buffer') but is confusing in the scatter gather case 
when 'buffer' contains the scatter gather list.

Doug Gilbert


Matthew Dharm wrote:
> 
> ARGH!!
> 
> Okay search the linux-scsi archives for May 29-30.  We've had this
> discussion before.
> 
> At that time, I proposed adding a field to indicate the actual amount of
> data being requested, as opposed to the buffer size.  At the time (again,
> see the archives), I was told that the request_bufflen field _was_ that
> field.... any case where the request_bufflen didn't match up with the
> command was a bug and needed to be fixed.
> 
> So which is it?  This needs to be fixed one way or another.
> 
> Emulated SCSI busses _need_ to know how much data to move.  usb-storage has
> a large table to try to figure it out, but it only gets the correct answer
> in _most_ cases, not all cases.  Only the originator of the command truly
> knows how much data is going to be moved, so the only mechanism that makes
> sense is to make the originator of the command indicate that number via
> some mechanism.
> 
> Matt
> 
> On Sun, Jul 28, 2002 at 12:48:03AM +0200, Andries.Brouwer@cwi.nl wrote:
> >     I haven't seen these patches integrated into a kernel yet... have I just
> >     missed them, or is there some reason they have been rejected?
> >
> >     > +++ b/drivers/scsi/scsi_scan.c    Sun Jul 21 00:55:37 2002
> >     > -              256, SCSI_TIMEOUT+4*HZ, 3);
> >     > +              scsi_cmd[4], SCSI_TIMEOUT+4*HZ, 3);
> >
> >     > +++ b/drivers/scsi/sd.c    Sun Jul 21 00:55:44 2002
> >     > -              512, SD_TIMEOUT, MAX_RETRIES);
> >     > +              255, SD_TIMEOUT, MAX_RETRIES);
> >
> > There are many more possibilities.
> > Since the number of hours in a day is very finite, Linus cannot
> > look at everything, so even if the patch is perfect it can be ignored.
> >
> > There is not really a SCSI maintainer, but there are people that are
> > well-known as active in the SCSI area. You may try to submit via
> > one of them.
> >
> > But then the question is whether these are good patches.
> >
> > I booted 2.5.29 earlier this evening and was greeted by
> > kernel BUG at transport.c: 351 and
> > kernel BUG at scsiglue.c: 150.
> > (And the usb-storage module now hangs initializing; rmmod fails,
> > reboot is necessary.)
> >
> > The former BUG_ON wants to have len == srb->request_bufflen.
> > But I don't know whether that is wise. It is less confusing
> > when bufflen indicates the size of the buffer available,
> > rather than the number of bytes you expect to move.
> > It is not unusual to get one byte more than you asked for,
> > in case you ask for an odd number of bytes.
> >
> > So, maybe these patches aren't clear improvements.
> >
> > Andries
> 
> --
> Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net
> Maintainer, Linux USB Mass Storage Driver
> 
> I could always suspend a few hundred accounts and watch what happens.
>                                         -- Tanya
> User Friendly, 7/31/1998
> 
>   ------------------------------------------------------------------------------------------
>    Part 1.2Type: application/pgp-signature

Re: [usb-storage] Re: 2 PATCHES: fix request_tranferlength

[Matthew Dharm]

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

Which is, oddly enough, what I proposed in May.  Tho I didn't name the
field.

Let's do this.  I can't get to this for another 2 weeks or so tho (ComicCon
this weekend, road trip next).  If someone wants to make a first cut at
this, I won't object. :)  If not, I'll do it when I get back.

I presume you envision this simply as another field in Scsi_Request,
leaving the call to scsi_wait_req() the same?  That would require
modification to sd, st, sr, osst, scsi_scan, sg... is that about it?  The
low-level drivers should be able to ignore this.... am I missing anything?

Does sc_request in the Scsi_Cmnd structure always point back to the
Scsi_Request structure associated with it?  All queuecommand() gets is the
Scsi_Cmnd... I'm guessing it does, but I don't immediately see the code to
make this all work....

Matt

On Sun, Jul 28, 2002 at 12:54:48AM -0400, Douglas Gilbert wrote:
> Matt,
> I have no objection to adding a field in Scsi_Request
> called something like 'dxfer_len' (CAM name) that makes
> its intent pretty clear. We probably have to keep
> 'bufflen' there for the time being. 'bufflen' is pretty 
> clear in the non scatter gather case (i.e. number of bytes 
> in 'buffer') but is confusing in the scatter gather case 
> when 'buffer' contains the scatter gather list.
> 
> Doug Gilbert
> 
> 
> Matthew Dharm wrote:
> > 
> > ARGH!!
> > 
> > Okay search the linux-scsi archives for May 29-30.  We've had this
> > discussion before.
> > 
> > At that time, I proposed adding a field to indicate the actual amount of
> > data being requested, as opposed to the buffer size.  At the time (again,
> > see the archives), I was told that the request_bufflen field _was_ that
> > field.... any case where the request_bufflen didn't match up with the
> > command was a bug and needed to be fixed.
> > 
> > So which is it?  This needs to be fixed one way or another.
> > 
> > Emulated SCSI busses _need_ to know how much data to move.  usb-storage has
> > a large table to try to figure it out, but it only gets the correct answer
> > in _most_ cases, not all cases.  Only the originator of the command truly
> > knows how much data is going to be moved, so the only mechanism that makes
> > sense is to make the originator of the command indicate that number via
> > some mechanism.
> > 
> > Matt
> > 
> > On Sun, Jul 28, 2002 at 12:48:03AM +0200, Andries.Brouwer@cwi.nl wrote:
> > >     I haven't seen these patches integrated into a kernel yet... have I just
> > >     missed them, or is there some reason they have been rejected?
> > >
> > >     > +++ b/drivers/scsi/scsi_scan.c    Sun Jul 21 00:55:37 2002
> > >     > -              256, SCSI_TIMEOUT+4*HZ, 3);
> > >     > +              scsi_cmd[4], SCSI_TIMEOUT+4*HZ, 3);
> > >
> > >     > +++ b/drivers/scsi/sd.c    Sun Jul 21 00:55:44 2002
> > >     > -              512, SD_TIMEOUT, MAX_RETRIES);
> > >     > +              255, SD_TIMEOUT, MAX_RETRIES);
> > >
> > > There are many more possibilities.
> > > Since the number of hours in a day is very finite, Linus cannot
> > > look at everything, so even if the patch is perfect it can be ignored.
> > >
> > > There is not really a SCSI maintainer, but there are people that are
> > > well-known as active in the SCSI area. You may try to submit via
> > > one of them.
> > >
> > > But then the question is whether these are good patches.
> > >
> > > I booted 2.5.29 earlier this evening and was greeted by
> > > kernel BUG at transport.c: 351 and
> > > kernel BUG at scsiglue.c: 150.
> > > (And the usb-storage module now hangs initializing; rmmod fails,
> > > reboot is necessary.)
> > >
> > > The former BUG_ON wants to have len == srb->request_bufflen.
> > > But I don't know whether that is wise. It is less confusing
> > > when bufflen indicates the size of the buffer available,
> > > rather than the number of bytes you expect to move.
> > > It is not unusual to get one byte more than you asked for,
> > > in case you ask for an odd number of bytes.
> > >
> > > So, maybe these patches aren't clear improvements.
> > >
> > > Andries
> > 
> > --
> > Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net
> > Maintainer, Linux USB Mass Storage Driver
> > 
> > I could always suspend a few hundred accounts and watch what happens.
> >                                         -- Tanya
> > User Friendly, 7/31/1998
> > 
> >   ------------------------------------------------------------------------------------------
> >    Part 1.2Type: application/pgp-signature

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

I think the problem is there's a nut loose on your keyboard.
					-- Greg to Customer
User Friendly, 1/5/1999 

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

Re: [usb-storage] Re: 2 PATCHES: fix request_tranferlength

[Matthew Dharm]

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

Out of curiosity... which value does the sg interface pass through?  It
collects both the buffer length and the requested transfer length, if my
memory serves...

Matt

On Sun, Jul 28, 2002 at 12:54:48AM -0400, Douglas Gilbert wrote:
> Matt,
> I have no objection to adding a field in Scsi_Request
> called something like 'dxfer_len' (CAM name) that makes
> its intent pretty clear. We probably have to keep
> 'bufflen' there for the time being. 'bufflen' is pretty 
> clear in the non scatter gather case (i.e. number of bytes 
> in 'buffer') but is confusing in the scatter gather case 
> when 'buffer' contains the scatter gather list.
> 
> Doug Gilbert
> 
> 
> Matthew Dharm wrote:
> > 
> > ARGH!!
> > 
> > Okay search the linux-scsi archives for May 29-30.  We've had this
> > discussion before.
> > 
> > At that time, I proposed adding a field to indicate the actual amount of
> > data being requested, as opposed to the buffer size.  At the time (again,
> > see the archives), I was told that the request_bufflen field _was_ that
> > field.... any case where the request_bufflen didn't match up with the
> > command was a bug and needed to be fixed.
> > 
> > So which is it?  This needs to be fixed one way or another.
> > 
> > Emulated SCSI busses _need_ to know how much data to move.  usb-storage has
> > a large table to try to figure it out, but it only gets the correct answer
> > in _most_ cases, not all cases.  Only the originator of the command truly
> > knows how much data is going to be moved, so the only mechanism that makes
> > sense is to make the originator of the command indicate that number via
> > some mechanism.
> > 
> > Matt
> > 
> > On Sun, Jul 28, 2002 at 12:48:03AM +0200, Andries.Brouwer@cwi.nl wrote:
> > >     I haven't seen these patches integrated into a kernel yet... have I just
> > >     missed them, or is there some reason they have been rejected?
> > >
> > >     > +++ b/drivers/scsi/scsi_scan.c    Sun Jul 21 00:55:37 2002
> > >     > -              256, SCSI_TIMEOUT+4*HZ, 3);
> > >     > +              scsi_cmd[4], SCSI_TIMEOUT+4*HZ, 3);
> > >
> > >     > +++ b/drivers/scsi/sd.c    Sun Jul 21 00:55:44 2002
> > >     > -              512, SD_TIMEOUT, MAX_RETRIES);
> > >     > +              255, SD_TIMEOUT, MAX_RETRIES);
> > >
> > > There are many more possibilities.
> > > Since the number of hours in a day is very finite, Linus cannot
> > > look at everything, so even if the patch is perfect it can be ignored.
> > >
> > > There is not really a SCSI maintainer, but there are people that are
> > > well-known as active in the SCSI area. You may try to submit via
> > > one of them.
> > >
> > > But then the question is whether these are good patches.
> > >
> > > I booted 2.5.29 earlier this evening and was greeted by
> > > kernel BUG at transport.c: 351 and
> > > kernel BUG at scsiglue.c: 150.
> > > (And the usb-storage module now hangs initializing; rmmod fails,
> > > reboot is necessary.)
> > >
> > > The former BUG_ON wants to have len == srb->request_bufflen.
> > > But I don't know whether that is wise. It is less confusing
> > > when bufflen indicates the size of the buffer available,
> > > rather than the number of bytes you expect to move.
> > > It is not unusual to get one byte more than you asked for,
> > > in case you ask for an odd number of bytes.
> > >
> > > So, maybe these patches aren't clear improvements.
> > >
> > > Andries
> > 
> > --
> > Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net
> > Maintainer, Linux USB Mass Storage Driver
> > 
> > I could always suspend a few hundred accounts and watch what happens.
> >                                         -- Tanya
> > User Friendly, 7/31/1998
> > 
> >   ------------------------------------------------------------------------------------------
> >    Part 1.2Type: application/pgp-signature

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

C:  They kicked your ass, didn't they?
S:  They were cheating!
					-- The Chief and Stef
User Friendly, 11/19/1997

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

Re: [usb-storage] Re: 2 PATCHES: fix request_tranferlength

[Douglas Gilbert]

Matthew Dharm wrote:
> 
> Out of curiosity... which value does the sg interface pass through?  It
> collects both the buffer length and the requested transfer length, if my
> memory serves...

Matt,
The sg driver does not attempt to decode SCSI commands (and,
for example, calculate allocation lengths) but simply passes
them through. In the sg_io_hdr interface what the user gives
as dxfer_len becomes the 4th argument to scsi_do_req() and
hence becomes Scsi_Request::sr_bufflen in the mid and lower
levels. The cumulative length of the elements in scatter gather 
list built by sg (or the single buffer) is greater than or
equal to dxfer_len.

I agree with the others who have stated that the case in which
the mid level sets allocation length at 255 (within a cdb) and 
bufflen=512 is just a bug and should be fixed. Both figures 
should probably be 252 (i.e. maximum 8 bit unsigned value rounded
down to ease alignment problems).

I agree with Kai concerning the use of sr_bufflen but can
see how LLDD maintainers could easily get confused. My idea
about dxfer_len was a duplication intended to help the LLDD
maintainers. Clear documentation would also help.

BTW When sr_buffer points to a scatter gather list sr_sglist_len 
is that list's length in bytes.

Doug Gilbert

> Matt
> 
> On Sun, Jul 28, 2002 at 12:54:48AM -0400, Douglas Gilbert wrote:
> > Matt,
> > I have no objection to adding a field in Scsi_Request
> > called something like 'dxfer_len' (CAM name) that makes
> > its intent pretty clear. We probably have to keep
> > 'bufflen' there for the time being. 'bufflen' is pretty
> > clear in the non scatter gather case (i.e. number of bytes
> > in 'buffer') but is confusing in the scatter gather case
> > when 'buffer' contains the scatter gather list.
> >
> > Doug Gilbert
> >
> >
> > Matthew Dharm wrote:
> > >
> > > ARGH!!
> > >
> > > Okay search the linux-scsi archives for May 29-30.  We've had this
> > > discussion before.
> > >
> > > At that time, I proposed adding a field to indicate the actual amount of
> > > data being requested, as opposed to the buffer size.  At the time (again,
> > > see the archives), I was told that the request_bufflen field _was_ that
> > > field.... any case where the request_bufflen didn't match up with the
> > > command was a bug and needed to be fixed.
> > >
> > > So which is it?  This needs to be fixed one way or another.
> > >
> > > Emulated SCSI busses _need_ to know how much data to move.  usb-storage has
> > > a large table to try to figure it out, but it only gets the correct answer
> > > in _most_ cases, not all cases.  Only the originator of the command truly
> > > knows how much data is going to be moved, so the only mechanism that makes
> > > sense is to make the originator of the command indicate that number via
> > > some mechanism.
> > >
> > > Matt
> > >
> > > On Sun, Jul 28, 2002 at 12:48:03AM +0200, Andries.Brouwer@cwi.nl wrote:
> > > >     I haven't seen these patches integrated into a kernel yet... have I just
> > > >     missed them, or is there some reason they have been rejected?
> > > >
> > > >     > +++ b/drivers/scsi/scsi_scan.c    Sun Jul 21 00:55:37 2002
> > > >     > -              256, SCSI_TIMEOUT+4*HZ, 3);
> > > >     > +              scsi_cmd[4], SCSI_TIMEOUT+4*HZ, 3);
> > > >
> > > >     > +++ b/drivers/scsi/sd.c    Sun Jul 21 00:55:44 2002
> > > >     > -              512, SD_TIMEOUT, MAX_RETRIES);
> > > >     > +              255, SD_TIMEOUT, MAX_RETRIES);
> > > >
> > > > There are many more possibilities.
> > > > Since the number of hours in a day is very finite, Linus cannot
> > > > look at everything, so even if the patch is perfect it can be ignored.
> > > >
> > > > There is not really a SCSI maintainer, but there are people that are
> > > > well-known as active in the SCSI area. You may try to submit via
> > > > one of them.
> > > >
> > > > But then the question is whether these are good patches.
> > > >
> > > > I booted 2.5.29 earlier this evening and was greeted by
> > > > kernel BUG at transport.c: 351 and
> > > > kernel BUG at scsiglue.c: 150.
> > > > (And the usb-storage module now hangs initializing; rmmod fails,
> > > > reboot is necessary.)
> > > >
> > > > The former BUG_ON wants to have len == srb->request_bufflen.
> > > > But I don't know whether that is wise. It is less confusing
> > > > when bufflen indicates the size of the buffer available,
> > > > rather than the number of bytes you expect to move.
> > > > It is not unusual to get one byte more than you asked for,
> > > > in case you ask for an odd number of bytes.
> > > >
> > > > So, maybe these patches aren't clear improvements.
> > > >
> > > > Andries
> > >
> > > --
> > > Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net
> > > Maintainer, Linux USB Mass Storage Driver
> > >
> > > I could always suspend a few hundred accounts and watch what happens.
> > >                                         -- Tanya
> > > User Friendly, 7/31/1998
> > >
> > >   ------------------------------------------------------------------------------------------
> > >    Part 1.2Type: application/pgp-signature
> 
> --
> Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net
> Maintainer, Linux USB Mass Storage Driver
> 
> C:  They kicked your ass, didn't they?
> S:  They were cheating!
>                                         -- The Chief and Stef
> User Friendly, 11/19/1997
> 
>   ------------------------------------------------------------------------------------------
>    Part 1.2Type: application/pgp-signature

Re: [usb-storage] Re: 2 PATCHES: fix request_tranferlength

[Matthew Dharm]

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

On Tue, Jul 30, 2002 at 09:40:12AM -0400, Douglas Gilbert wrote:
> Matthew Dharm wrote:
> > 
> > Out of curiosity... which value does the sg interface pass through?  It
> > collects both the buffer length and the requested transfer length, if my
> > memory serves...
> 
> Matt,
> The sg driver does not attempt to decode SCSI commands (and,
> for example, calculate allocation lengths) but simply passes
> them through. In the sg_io_hdr interface what the user gives
> as dxfer_len becomes the 4th argument to scsi_do_req() and
> hence becomes Scsi_Request::sr_bufflen in the mid and lower
> levels. The cumulative length of the elements in scatter gather 
> list built by sg (or the single buffer) is greater than or
> equal to dxfer_len.

Hrm....

Are you saying then that the sum of the sg segments can be greater than the
requested length?

After some discussions with Linus and others, I was under the impression
that this was a no-no...

Matt

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

Department of Justice agent.  I have come to purify the flock.
					-- DOJ agent
User Friendly, 5/22/1998

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

Re: [usb-storage] Re: 2 PATCHES: fix request_tranferlength

[Doug Ledford]

On Tue, Jul 30, 2002 at 09:29:09PM -0700, Matthew Dharm wrote:
> On Tue, Jul 30, 2002 at 09:40:12AM -0400, Douglas Gilbert wrote:
> > Matthew Dharm wrote:
> > > 
> > > Out of curiosity... which value does the sg interface pass through?  It
> > > collects both the buffer length and the requested transfer length, if my
> > > memory serves...
> > 
> > Matt,
> > The sg driver does not attempt to decode SCSI commands (and,
> > for example, calculate allocation lengths) but simply passes
> > them through. In the sg_io_hdr interface what the user gives
> > as dxfer_len becomes the 4th argument to scsi_do_req() and
> > hence becomes Scsi_Request::sr_bufflen in the mid and lower
> > levels. The cumulative length of the elements in scatter gather 
> > list built by sg (or the single buffer) is greater than or
> > equal to dxfer_len.
> 
> Hrm....
> 
> Are you saying then that the sum of the sg segments can be greater than the
> requested length?
> 
> After some discussions with Linus and others, I was under the impression
> that this was a no-no...

Only if the last element in the list were allowed to be oversized.  
Obviously, if element 2 in a 3 element list has a size that was larger 
than it should be then the sg DMA code would end up dropping the data in 
the wrong place.  All the sizes on the sg segments up to the last one must 
be exact, and then the last one could only be overly long, not short.  
But, in practice, I think the real answer is that the length of all the 
elements in the sg list == dxfer_len in all cases.


-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606
  

Re: [usb-storage] Re: 2 PATCHES: fix request_tranferlength

[Douglas Gilbert]

Matthew Dharm wrote:
> 
> On Tue, Jul 30, 2002 at 09:40:12AM -0400, Douglas Gilbert wrote:
> > Matthew Dharm wrote:
> > >
> > > Out of curiosity... which value does the sg interface pass through?  It
> > > collects both the buffer length and the requested transfer length, if my
> > > memory serves...
> >
> > Matt,
> > The sg driver does not attempt to decode SCSI commands (and,
> > for example, calculate allocation lengths) but simply passes
> > them through. In the sg_io_hdr interface what the user gives
> > as dxfer_len becomes the 4th argument to scsi_do_req() and
> > hence becomes Scsi_Request::sr_bufflen in the mid and lower
> > levels. The cumulative length of the elements in scatter gather
> > list built by sg (or the single buffer) is greater than or
> > equal to dxfer_len.
> 
> Hrm....
> 
> Are you saying then that the sum of the sg segments can be greater than the
> requested length?

Yes.
For indirect IO this shouldn't be a problem as dxfer_len
is used for the copy_to_user(). For direct IO, overrunning
the dxfer_len is a little more suspect.
 
> After some discussions with Linus and others, I was under the impression
> that this was a no-no...

The venerable aha1542 used to panic if it every received
an odd scatter gather element size. Also most modern
PCI SCSI controllers are DMAing from a 16 bit SPI bus and
an odd count seems to cause some of them mild indigestion.


Doug Ledford <dledford@redhat.com> wrote:
> But, in practice, I think the real answer is that the length 
> of all the elements in the sg list == dxfer_len in all cases.

In sd and sr probably. In sg consider this code comment:
/* round request up to next highest SG_SECTOR_SZ byte boundary */

That code was there from the original sg driver (lk 2.0 and
before). I took it out, got burnt, then put it back in.

 
Doug Gilbert

Re: [usb-storage] Re: 2 PATCHES: fix request_tranferlength

[Kai Makisara]

On Sat, 27 Jul 2002, Matthew Dharm wrote:

> ARGH!!
>
...
> Emulated SCSI busses _need_ to know how much data to move.  usb-storage has
> a large table to try to figure it out, but it only gets the correct answer
> in _most_ cases, not all cases.  Only the originator of the command truly
> knows how much data is going to be moved, so the only mechanism that makes
> sense is to make the originator of the command indicate that number via
> some mechanism.
>
Note that even the originator does not always know how much data will be
transferred. One example is reading a tape block in variable block mode.
The originator specifies the maximum length but the target returns less if
the block is shorter.

The current interface is a little messy. The function scsi_do_req has the
following signature:

void scsi_do_req(Scsi_Request * SRpnt, const void *cmnd,
              void *buffer, unsigned bufflen, void (*done) (Scsi_Cmnd *),
                 int timeout, int retries)

Here bufflen is defined "size of data buffer". In order to see what is
really desired to be done, one must additionally look at some fields in
the Scsi_Request: if SRpnt->sr_use_sg is non-zero, buffer points to a
scatter/gather list. Otherwise it points to the continguous buffer.

A common interpretation seems to be that bufflen is the buffer length only
when scatter/gather is used. Otherwise the buffer length is computed from
the scatter/gather elements and bufflen is a parameter with no meaning.

Another interpretation (which I have used) is that the bufflen parameter
is always the length of the buffer _used in this transfer_. Whether the
buffer is contiguous or defined using scatter/gather list is determined by
SRpnt->sr_use_sg. (The lower level drivers don't have to know the actual
length of the buffer as long as it is long enough.)

The second interpretation has the transfer length in exactly one place. In
the first interpretation a complicated logic (or a new structure member)
is needed to get the transfer length. Do we need this or should we just
enforce the second interpretation (maybe change the name bufflen to
maxtransfer or something like that)?

(If the new structure member dxfer_len is useful to lower level drivers, I
don't oppose adding it to Scsi_Request as long as it is set in
scsi_do_req. What I am opposing is to make the callers responsible for
redundant specifications.)

	Kai

Re: [usb-storage] Re: 2 PATCHES: fix request_tranferlength

[Matthew Dharm]

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

On Sun, Jul 28, 2002 at 01:22:40PM +0300, Kai Makisara wrote:
> On Sat, 27 Jul 2002, Matthew Dharm wrote:
> 
> > Emulated SCSI busses _need_ to know how much data to move.  usb-storage has
> > a large table to try to figure it out, but it only gets the correct answer
> > in _most_ cases, not all cases.  Only the originator of the command truly
> > knows how much data is going to be moved, so the only mechanism that makes
> > sense is to make the originator of the command indicate that number via
> > some mechanism.
> >
> Note that even the originator does not always know how much data will be
> transferred. One example is reading a tape block in variable block mode.
> The originator specifies the maximum length but the target returns less if
> the block is shorter.

That's not the problem I speak of.  Tho I recognize it....

The problem I (and sbp2 faces), is this:  Imagine the 'device' is really a
USB/ATA bridge device, with an ATA device directly attached to it.

The case of 'return short data' is a common one (INQUIRY, MODE_SENSE, etc.)
and so is well handled.

However, when speaking to the bridge, we need to provide it with (a) the
command bytes to be sent to the ATA unit, and (b) the amount of data we
intend to move.

The case that is not handled well by the bridge is the 'bad host' case of
the command bytes indicating a length which is different from the explicit
length we give.  Lots of bridges choke on this configuration.

As long as those two values _agree_, there is no problem.  A 'return short
data' case is well handled.  But we need to make those values agree.  A
variable length read isn't a problem.

I won't pretend I understand how ATA or SCSI works... all I know is that
under certain conditions, these bridge chips can't DoTheRightThing.

> The current interface is a little messy. The function scsi_do_req has the
> following signature:
> 
> void scsi_do_req(Scsi_Request * SRpnt, const void *cmnd,
>               void *buffer, unsigned bufflen, void (*done) (Scsi_Cmnd *),
>                  int timeout, int retries)

Agreed.  Why half of these parameters aren't in the Scsi_Request structure
is beyond me.

> Another interpretation (which I have used) is that the bufflen parameter
> is always the length of the buffer _used in this transfer_. Whether the
> buffer is contiguous or defined using scatter/gather list is determined by
> SRpnt->sr_use_sg. (The lower level drivers don't have to know the actual
> length of the buffer as long as it is long enough.)

This is the interpretation I use in usb-storage.  Experimentally, it seems
to be correct.

> The second interpretation has the transfer length in exactly one place. In
> the first interpretation a complicated logic (or a new structure member)
> is needed to get the transfer length. Do we need this or should we just
> enforce the second interpretation (maybe change the name bufflen to
> maxtransfer or something like that)?
> 
> (If the new structure member dxfer_len is useful to lower level drivers, I
> don't oppose adding it to Scsi_Request as long as it is set in
> scsi_do_req. What I am opposing is to make the callers responsible for
> redundant specifications.)

Do you see from my explanation why dxfer_len is not redundant to any other
fields?  One more example to try to make it clear...

One code path in scsi_scan.c sends an INQUIRY with cmd[4] = xff.  This is a
command to transfer (up to) 255 bytes.  The bufflen passed is 512.

I can hear the objection "Why doesn't usb-storage inspect the command to
determine the length?".  This is, in fact, what we do now.  It only works
_most_ of the time, not all of the time.  Vendor-specifc commands are
screwed.  Several opcodes have different meaning depending on what type of
device they are sent to, so we're screwed there.  Commands which specify
length in terms of "number of blocks" also screw us up, because we don't
know what the block size is.

I've been working on this problem for more than a year.  My conclusion is
that we need a separate indication of dxfer_len.  I'm open to alternatives,
but none proposed so far gets me the information I need for usb-storage to
function properly.

Matt

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

Type "format c:"  That should fix everything.
					-- Greg
User Friendly, 12/18/1997

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

Re: [usb-storage] Re: 2 PATCHES: fix request_tranferlength

[Kai Makisara]

On Sun, 28 Jul 2002, Matthew Dharm wrote:

> On Sun, Jul 28, 2002 at 01:22:40PM +0300, Kai Makisara wrote:
...
> The case of 'return short data' is a common one (INQUIRY, MODE_SENSE, etc.)
> and so is well handled.
>
Good. I just get worried when someone talks about the originator knowing
the length of the returned data :-)

...
> > The current interface is a little messy. The function scsi_do_req has the
> > following signature:
> >
> > void scsi_do_req(Scsi_Request * SRpnt, const void *cmnd,
> >               void *buffer, unsigned bufflen, void (*done) (Scsi_Cmnd *),
> >                  int timeout, int retries)
>
> Agreed.  Why half of these parameters aren't in the Scsi_Request structure
> is beyond me.
>
I like using parameters because they explicitly show what the function
needs. Passing the values in a subset of fields in a structure is somewhat
obscure. However, in this case I agree that everything can't be passed as
parameters.

> > Another interpretation (which I have used) is that the bufflen parameter
> > is always the length of the buffer _used in this transfer_. Whether the
> > buffer is contiguous or defined using scatter/gather list is determined by
> > SRpnt->sr_use_sg. (The lower level drivers don't have to know the actual
> > length of the buffer as long as it is long enough.)
>
> This is the interpretation I use in usb-storage.  Experimentally, it seems
> to be correct.
>
The st driver up to 2.5.27 has mostly violated these assumptions when
scatter/gather is used. The newest version sets the segment lengths sum up
to the transfer length. However, this is a byproduct of a future
enhancement.

> > The second interpretation has the transfer length in exactly one place. In
> > the first interpretation a complicated logic (or a new structure member)
> > is needed to get the transfer length. Do we need this or should we just
> > enforce the second interpretation (maybe change the name bufflen to
> > maxtransfer or something like that)?
> >
> > (If the new structure member dxfer_len is useful to lower level drivers, I
> > don't oppose adding it to Scsi_Request as long as it is set in
> > scsi_do_req. What I am opposing is to make the callers responsible for
> > redundant specifications.)
>
> Do you see from my explanation why dxfer_len is not redundant to any other
> fields?  One more example to try to make it clear...
>
> One code path in scsi_scan.c sends an INQUIRY with cmd[4] = xff.  This is a
> command to transfer (up to) 255 bytes.  The bufflen passed is 512.
>
My interpretation is that bufflen in the call should be 255 in this case.
Using 512 is a bug. Is it useful for the lower levels to know how much
excess space the caller has reserved? If it is, I take back my suggestion.

I think we both agree that the information in dxfer_len must be passed.
The difference is that you suggest adding a new field to Scsi_Request
whereas I suggest clarifying the meaning of the existing bufflen
parameter.

Whatever the result will be, I hope someone writes down the rules. (It is
also a good way to see if the definition is simple or complex :-)

> I can hear the objection "Why doesn't usb-storage inspect the command to
> determine the length?".  This is, in fact, what we do now.  It only works
> _most_ of the time, not all of the time.  Vendor-specifc commands are
> screwed.  Several opcodes have different meaning depending on what type of
> device they are sent to, so we're screwed there.  Commands which specify
> length in terms of "number of blocks" also screw us up, because we don't
> know what the block size is.
>
You don't hear the objection from me. I can just add a couple more:
- the new commands the standard defines during time
- why have the complicated parsing when the caller knows the address for
free

	Kai

Re: [usb-storage] Re: 2 PATCHES: fix request_tranferlength

[Doug Ledford]

On Mon, Jul 29, 2002 at 12:01:24AM +0300, Kai Makisara wrote:
> On Sun, 28 Jul 2002, Matthew Dharm wrote:
> 
> > > The second interpretation has the transfer length in exactly one place. In
> > > the first interpretation a complicated logic (or a new structure member)
> > > is needed to get the transfer length. Do we need this or should we just
> > > enforce the second interpretation (maybe change the name bufflen to
> > > maxtransfer or something like that)?
> > >
> > > (If the new structure member dxfer_len is useful to lower level drivers, I
> > > don't oppose adding it to Scsi_Request as long as it is set in
> > > scsi_do_req. What I am opposing is to make the callers responsible for
> > > redundant specifications.)
> >
> > Do you see from my explanation why dxfer_len is not redundant to any other
> > fields?  One more example to try to make it clear...
> >
> > One code path in scsi_scan.c sends an INQUIRY with cmd[4] = xff.  This is a
> > command to transfer (up to) 255 bytes.  The bufflen passed is 512.
> >
> My interpretation is that bufflen in the call should be 255 in this case.
> Using 512 is a bug. Is it useful for the lower levels to know how much
> excess space the caller has reserved? If it is, I take back my suggestion.

No, that just confuses the issue needlessly and down that road lies future 
bugs.

> I think we both agree that the information in dxfer_len must be passed.
> The difference is that you suggest adding a new field to Scsi_Request
> whereas I suggest clarifying the meaning of the existing bufflen
> parameter.

There really isn't a need to clarify the meaning of the request_bufflen 
item.  It's meaning has been set for some time (1.x days).  However, 
documenting the proper use of those variables in a low level driver might 
be usefull.  So, here's a psuedo code example of how to interpret these 
data items:

if(cmd->use_sg) {
	struct scatterlist *sg = (struct scatterlist *)cmd->request_buffer;
	int i, length;

	{ walk sg table, build hardware dependant sg list, add length of
	  each sg segment to length, length will be total transfer length
	  when done walking list }
} else if(cmd->request_bufflen) {
	cmd->request_buffer is pointer to actual contiguous buffer
	cmd->request_bufflen is amount of data to be transferred (max)
	cmd->underflow is minimum amount of data we should transfer
		(your driver should return DID_ERROR if final transfer
		 length was less than cmd->underflow and it should set
		 the cmd->residual field to indicate how much of an
		 underflow we had) (note: I think it's cmd->residual you
		 need to set, I haven't look recently)
} else
	no data transfer expected (things like TEST_UNIT_READY)

> Whatever the result will be, I hope someone writes down the rules. (It is
> also a good way to see if the definition is simple or complex :-)

It's more complex than it should be.  My personal opinion, all transfers 
should use a sg table, period.  Even single segment transfers should 
still be set up as such.  The logic would then become if(cmd->use_sg) {set 
up sg list } else {no data transfer}.  Making this a reality would require 
driver updates though since there are some drivers known to fail on single 
element sg tables.  Getting the above logic correct is paramount to having 
a working PCI DMA API driver BTW.


-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606
  

Re: [usb-storage] Re: 2 PATCHES: fix request_tranferlength

[Kai Makisara]

On Mon, 29 Jul 2002, Doug Ledford wrote:

> On Mon, Jul 29, 2002 at 12:01:24AM +0300, Kai Makisara wrote:
...
> > My interpretation is that bufflen in the call should be 255 in this case.
> > Using 512 is a bug. Is it useful for the lower levels to know how much
> > excess space the caller has reserved? If it is, I take back my suggestion.
>
> No, that just confuses the issue needlessly and down that road lies future
> bugs.
>
It simplifies the issue. However, note that we are looking at the problem
from different sides of the scsi_do_req() interface. I am arguing that for
the _users_ of scsi_do_req() the current situation is confusing and adding
a new member to Scsi_Request only makes it worse. You are looking at the
problem as consumer of Scsi_Cmnds.

We are talking about 2.5. Perhaps the scsi_do_req() interface should be
cleaned. If you don't want to use function arguments, everything
necessary can be stuffed directly into Scsi_Request and the parameters
buff, bufflen, cmnd, done, timeout, retries can be removed.

...
> > I think we both agree that the information in dxfer_len must be passed.
> > The difference is that you suggest adding a new field to Scsi_Request
> > whereas I suggest clarifying the meaning of the existing bufflen
> > parameter.
>
> There really isn't a need to clarify the meaning of the request_bufflen
                                                          ^^^^^^^^^^^^^^^
I agree, but I was talking about the bufflen parameter of the
scsi_do_req() function taking Scsi_Request. request_bufflen is a field in
struct Scsi_Cmnd.

	Kai