Re: [linux-usb-devel] Re: inquiry in scsi_scan.c

Re: [linux-usb-devel] Re: inquiry in scsi_scan.c

[Alan Stern]

On Sun, 5 Jan 2003 Andries.Brouwer@cwi.nl wrote:

> The SCSI code has no means of knowing the actual length transferred,
> so has no choice but to believe the length byte in the reply.
> But the USB code does the transferring itself, and knows precisely
> how many bytes were transferred. If 36 bytes were transferred and
> the additional length byte is 0, indicating a length of 5, then the
> USB code can fix the response and change the additional length byte
> to 31, indicating a length of 36. That way the SCSI code knows that
> not 5 but 36 bytes are valid, and it gets actual vendor and model strings.

I'm not familiar with the details of the SCSI code you are referring to,
but usb-storage does make available the actual transfer length.  All the
transport routine paths set the resid field of the Scsi_Cmnd structure
properly.  With this information, there should be no difficulty in
determining how many bytes were transferred.  (Maybe the setting doesn't
percolate up to the particular code you mention -- and maybe other host
adapter drivers don't set resid correctly so you cannot rely on its value.
I don't know what other problems might crop up.)

Alan Stern

Re: [linux-usb-devel] Re: inquiry in scsi_scan.c

[Luben Tuikov]

Alan Stern wrote:
> 
>>The SCSI code has no means of knowing the actual length transferred,
>>so has no choice but to believe the length byte in the reply.
>>But the USB code does the transferring itself, and knows precisely
>>how many bytes were transferred. If 36 bytes were transferred and
>>the additional length byte is 0, indicating a length of 5, then the
>>USB code can fix the response and change the additional length byte
>>to 31, indicating a length of 36. That way the SCSI code knows that
>>not 5 but 36 bytes are valid, and it gets actual vendor and model strings.
> 
> 
> I'm not familiar with the details of the SCSI code you are referring to,
> but usb-storage does make available the actual transfer length.  All the
> transport routine paths set the resid field of the Scsi_Cmnd structure
> properly.  With this information, there should be no difficulty in
> determining how many bytes were transferred.  (Maybe the setting doesn't
> percolate up to the particular code you mention -- and maybe other host
> adapter drivers don't set resid correctly so you cannot rely on its value.
> I don't know what other problems might crop up.)

In this most recent case reported, this looks very similar to
overflow residual, but not quite the same. I.e. *more* data is
actually immediately available (in the buffer) than *we requested* or can
find out by other means (i.e. checking the ADDITIONAL LENGTH field).

But if we requested 36 and the transport provided 36 then the
residual should be 0 (set by the transport). Had we requested 5 and the
transport provided 36 then the residual should be 31, but there's no way
of reporting this residual, since it is an *overflow* residual, and from
the comments therein, cmd->resid is *only underflow* residual.

Thus, *if* the cmd->resid field is set properly, then something like this,
in SCSI Core, might suffice:

</Disclaimer: this is NOT actual C code, but should be descriptive enough/>

	int bytes_requested = <whatever>;
	int bytes_got, additional_len;
	...
	</ok, we got response from INQUIRY/>

	additional_len = buffer[4];
	bytes_got = max(bytes_requested - cmd->resid, 0);

	</now use bytes_got exclusively and ignore additional_len/>
	...
	</do assignments and whatever, and just before we're done print
	a nice warning message if bytes_got != additional_len+5 />

-- 
Luben

Re: [linux-usb-devel] Re: inquiry in scsi_scan.c

[Alan Stern]

On Mon, 6 Jan 2003, Luben Tuikov wrote:

> In this most recent case reported, this looks very similar to
> overflow residual, but not quite the same. I.e. *more* data is
> actually immediately available (in the buffer) than *we requested* or can
> find out by other means (i.e. checking the ADDITIONAL LENGTH field).

In the case reported, the problem was not that there was additional data
immediately available.  In fact, there was not any additional data.  The
problem was that the firmware returned a value in the ADDITIONAL LENGTH
field indicating that additional data existed when in fact it did not.
(The host requested 36 bytes, the device returned 36 bytes and indicated
that one more byte was available, the host then requested 37 bytes, and
the device returned a short reply of 36 bytes.)

> But if we requested 36 and the transport provided 36 then the
> residual should be 0 (set by the transport).

As indeed it would be.

> Had we requested 5 and the
> transport provided 36 then the residual should be 31, but there's no way
> of reporting this residual, since it is an *overflow* residual, and from
> the comments therein, cmd->resid is *only underflow* residual.

No.  This can't happen at all with USB; the protocol does not allow the
device to return more data than the host requested.

Alan Stern

Re: Re: inquiry in scsi_scan.c

[Andries.Brouwer]

> In the case reported, the problem was

Ha, Alan - it is possible that the two of you are referring
to different things.

I mentioned two devices, both return 36 bytes when asked for
36 bytes, but the first has 0 in the additional length field
(thus reports length 5), the second has 32 in the additional
length field (thus reports length 37).
This second device, when asked for 37 bytes, still only returns 36.

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: Re: inquiry in scsi_scan.c

[Matthew Dharm]

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

On Mon, Jan 06, 2003 at 08:18:45PM +0100, Andries.Brouwer@cwi.nl wrote:
> > In the case reported, the problem was
> 
> Ha, Alan - it is possible that the two of you are referring
> to different things.

That confusion is definatly happening.

> I mentioned two devices, both return 36 bytes when asked for
> 36 bytes, but the first has 0 in the additional length field
> (thus reports length 5), the second has 32 in the additional
> length field (thus reports length 37).
> This second device, when asked for 37 bytes, still only returns 36.

The first case:  If the additional length indicates < 36 bytes, we should
never issue the second request (which is where this device choked).  This
should be a sanity check in scsi_scan.c, and it works for reasons I've
previously outlined.

The second case:  This is a bad device.  A classic off-by-one error.  But
what can usb-storage do?  We don't know that the device is bad.  But,
focusing on this case, what happens?  Short data is returned... if the
resid field is set to indicate this, then scsi_scan.c should be able to do
something sane here.

Perhaps the "best" fix here is to simply make scsi_scan.c only send 36 byte
inquiry requests if the bus is 'emulated'.  That would solve a world of
problems....

Matt

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

God, root, what is difference?
					-- Pitr
User Friendly, 11/11/1999

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

Re: [linux-usb-devel] Re: inquiry in scsi_scan.c

[Luben Tuikov]

Matthew Dharm wrote:
> 
> Perhaps the "best" fix here is to simply make scsi_scan.c only send 36 byte
> inquiry requests if the bus is 'emulated'.  That would solve a world of
> problems....

When scsi_scan.c does it's own scanning for SCSI Core, maybe it's best to
ignore 36 < INQUIRY_DATA_LEN < 57, since this is just vendor specific
data and SCSI Core is not interested in it.

In descriptive-C this looks like this:

</issue a 36 byte buffer INQUIRY/>
</now dissect:/>

int bytes_got = max(bytes_requested - cmd->resid, 0);

if (31 < buffer[4] && buffer[4] < 52) {
	/* we don't care, do not issue another INQUIRY */	
else if (buffer[4] >= 52) {
	bytes_requested = 5+buffer[4];
	/* issue another INQUIRY to get the additional flags, */
	/* plus any version descriptors if available */
	</insert code here/>
	bytes_got = max(bytes_requested - cmd->resid, 0);
}

if (bytes_got != buffer[4]+5) {
	/* let's rely on the transport to have correctly set */
	/* cmd->resid and report a broken device server */
	</insert code here/>
}

/* Now we rely on bytes_got */

</rest of scanning code/>

-- 
Luben

Re: [linux-usb-devel] Re: inquiry in scsi_scan.c

[James Bottomley]

luben@splentec.com said:
> 	bytes_got = max(bytes_requested - cmd->resid, 0); 
[...]
> 	/* let's rely on the transport to have correctly set */
> 	/* cmd->resid and report a broken device server */
> 	</insert code here/> 

I'm afraid that for a large number of HBAs in the tree SCpnt->resid just isn't 
reliable (nothing ever needed it, and it's overhead to compute, so they didn't 
bother).

James

Re: Re: inquiry in scsi_scan.c

[Matthew Dharm]

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

I'm told this is a bad idea because there are some HBA which snoop the
INQUIRY data.  Since I don't know how that snooping works, I can't comment
further.

Matt

On Mon, Jan 06, 2003 at 03:49:08PM -0500, Luben Tuikov wrote:
> Matthew Dharm wrote:
> > 
> > Perhaps the "best" fix here is to simply make scsi_scan.c only send 36 byte
> > inquiry requests if the bus is 'emulated'.  That would solve a world of
> > problems....
> 
> When scsi_scan.c does it's own scanning for SCSI Core, maybe it's best to
> ignore 36 < INQUIRY_DATA_LEN < 57, since this is just vendor specific
> data and SCSI Core is not interested in it.
> 
> In descriptive-C this looks like this:
> 
> </issue a 36 byte buffer INQUIRY/>
> </now dissect:/>
> 
> int bytes_got = max(bytes_requested - cmd->resid, 0);
> 
> if (31 < buffer[4] && buffer[4] < 52) {
> 	/* we don't care, do not issue another INQUIRY */	
> else if (buffer[4] >= 52) {
> 	bytes_requested = 5+buffer[4];
> 	/* issue another INQUIRY to get the additional flags, */
> 	/* plus any version descriptors if available */
> 	</insert code here/>
> 	bytes_got = max(bytes_requested - cmd->resid, 0);
> }
> 
> if (bytes_got != buffer[4]+5) {
> 	/* let's rely on the transport to have correctly set */
> 	/* cmd->resid and report a broken device server */
> 	</insert code here/>
> }
> 
> /* Now we rely on bytes_got */
> 
> </rest of scanning code/>
> 
> -- 
> Luben
> 

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

Would you mind not using our Web server? We're trying to have a game of 
Quake here.
					-- Greg
User Friendly, 5/11/1998

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

Re: [linux-usb-devel] Re: inquiry in scsi_scan.c

[Luben Tuikov]

Matthew Dharm wrote:
> I'm told this is a bad idea because there are some HBA which snoop the
> INQUIRY data.  Since I don't know how that snooping works, I can't comment
> further.

I do, in my own LLDDs. If anything the additional length might increase.
Others snoop it for their own awareness of the device and personal gains
to possibly result in better service to the actual device.

I cannot see how this could fail.

-- 
Luben

Re: [linux-usb-devel] Re: inquiry in scsi_scan.c

[Doug Ledford]

On Mon, Jan 06, 2003 at 04:16:19PM -0500, Luben Tuikov wrote:
> Matthew Dharm wrote:
> >I'm told this is a bad idea because there are some HBA which snoop the
> >INQUIRY data.  Since I don't know how that snooping works, I can't comment
> >further.
> 
> I do, in my own LLDDs. If anything the additional length might increase.
> Others snoop it for their own awareness of the device and personal gains
> to possibly result in better service to the actual device.
> 
> I cannot see how this could fail.

This can't fail.  Luben's right here.  If the lldd is snooping INQUIRY 
response, then the lldd is responsible for knowing how much it actually 
transferred and the scsi mid layer is not on the hook for telling it 
anything.  We are free to play our "try different INQUIRY lengths" games 
at the mid level and ignore any lldd snooping entirely.

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

Re: [linux-usb-devel] Re: inquiry in scsi_scan.c

[Doug Ledford]

On Mon, Jan 06, 2003 at 03:49:08PM -0500, Luben Tuikov wrote:
> Matthew Dharm wrote:
> >
> >Perhaps the "best" fix here is to simply make scsi_scan.c only send 36 byte
> >inquiry requests if the bus is 'emulated'.  That would solve a world of
> >problems....
> 
> When scsi_scan.c does it's own scanning for SCSI Core, maybe it's best to
> ignore 36 < INQUIRY_DATA_LEN < 57, since this is just vendor specific
> data and SCSI Core is not interested in it.

Yes please, this is the right thing to do.  We don't care about extra data
until you get up to 57 and above (to check for DT flag and therefore find
devices that report SCSI-2 as the scsi level but which are still capable
of PPR negotiation messages and therefore Ultra160 and possibly above
speeds), so the case of returning 37 bytes available after a 36 byte
request is totally non-interesting to the scsi core.

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

Re: [linux-usb-devel] Re: inquiry in scsi_scan.c

[Doug Ledford]

On Mon, Jan 06, 2003 at 11:22:59AM -0800, Matthew Dharm wrote:
> 
> The first case:  If the additional length indicates < 36 bytes, we should
> never issue the second request (which is where this device choked).  This
> should be a sanity check in scsi_scan.c, and it works for reasons I've
> previously outlined.

No, this should be fixed in usb code.  It's a hack, I know, but it's a 
hack to work around the fact that usb devices are broken by and large much 
more so than non-usb scsi devices.  Basically, the usb code has the 
ability to tell that 36 bytes were actually transferred, so the usb code 
should set the length byte in the return to max(actual_transfer - 5, 
current_length_byte);  In actuality, other scsi HBAs that know how many 
bytes were transferred by devices could do this as well, but they don't 
need to because their SCSI devices aren't so braindead....

> The second case:  This is a bad device.  A classic off-by-one error.  But
> what can usb-storage do?  We don't know that the device is bad.  But,
> focusing on this case, what happens?  Short data is returned... if the
> resid field is set to indicate this, then scsi_scan.c should be able to do
> something sane here.

Yep, as my previous email, scsi_scan.c should simply ignore the extra byte 
because we don't care about it in the least.

> Perhaps the "best" fix here is to simply make scsi_scan.c only send 36 byte
> inquiry requests if the bus is 'emulated'.  That would solve a world of
> problems....

Except that if a device *does* transfer 36 bytes and then lies and says it 
only transferred 5 then we are missing information that might actually be 
usefull, hence the reason to set the transfer length up to the real amount 
transferred (and BTW, I would only do this for INQUIRY responses, for 
anything else the device is simply too buggy to live if it lies about the 
transfer length).


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

Re: [linux-usb-devel] Re: inquiry in scsi_scan.c

[Matthew Dharm]

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

On Mon, Jan 06, 2003 at 05:23:22PM -0500, Doug Ledford wrote:
> On Mon, Jan 06, 2003 at 11:22:59AM -0800, Matthew Dharm wrote:
> > 
> > The first case:  If the additional length indicates < 36 bytes, we should
> > never issue the second request (which is where this device choked).  This
> > should be a sanity check in scsi_scan.c, and it works for reasons I've
> > previously outlined.
> 
> No, this should be fixed in usb code.  It's a hack, I know, but it's a 
> hack to work around the fact that usb devices are broken by and large much 
> more so than non-usb scsi devices.  Basically, the usb code has the 
> ability to tell that 36 bytes were actually transferred, so the usb code 
> should set the length byte in the return to max(actual_transfer - 5, 
> current_length_byte);  In actuality, other scsi HBAs that know how many 
> bytes were transferred by devices could do this as well, but they don't 
> need to because their SCSI devices aren't so braindead....

Okay, so what about sbp2, where this sort of thing is also common?  Why not
just fix it at the SCSI layer?

> > The second case:  This is a bad device.  A classic off-by-one error.  But
> > what can usb-storage do?  We don't know that the device is bad.  But,
> > focusing on this case, what happens?  Short data is returned... if the
> > resid field is set to indicate this, then scsi_scan.c should be able to do
> > something sane here.
> 
> Yep, as my previous email, scsi_scan.c should simply ignore the extra byte 
> because we don't care about it in the least.
> 
> > Perhaps the "best" fix here is to simply make scsi_scan.c only send 36 byte
> > inquiry requests if the bus is 'emulated'.  That would solve a world of
> > problems....
> 
> Except that if a device *does* transfer 36 bytes and then lies and says it 
> only transferred 5 then we are missing information that might actually be 
> usefull, hence the reason to set the transfer length up to the real amount 
> transferred (and BTW, I would only do this for INQUIRY responses, for 
> anything else the device is simply too buggy to live if it lies about the 
> transfer length).

First, I think this is a bogus situation.  If we reqest 36, get X, and
indicate a total of 5, then we should look at the X we got.  And that has
to be indicated by resid.

The argument that many HBAs don't set resid just doesn't hold water with
me.  Just because other drivers are broken, we should break more things
instead of fixing the problem?

Matt

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

Da.  Am thinkink of carbonated borscht for lonk nights of coding.
					-- Pitr
User Friendly, 7/24/1998

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

Re: [linux-usb-devel] Re: inquiry in scsi_scan.c

[Doug Ledford]

On Mon, Jan 06, 2003 at 04:46:47PM -0800, Matthew Dharm wrote:
> On Mon, Jan 06, 2003 at 05:23:22PM -0500, Doug Ledford wrote:
> [ snipped my comments ]
>
> Okay, so what about sbp2, where this sort of thing is also common?  Why not
> just fix it at the SCSI layer?

How are you wanting to fix it?  The infrastructure for doing this does not
currently exist in the mid layer code.  It would have to be added.  And it
would have to be added in a way that was reliable.  Ideas?
 
> > Except that if a device *does* transfer 36 bytes and then lies and says it 
> > only transferred 5 then we are missing information that might actually be 
> > usefull, hence the reason to set the transfer length up to the real amount 
> > transferred (and BTW, I would only do this for INQUIRY responses, for 
> > anything else the device is simply too buggy to live if it lies about the 
> > transfer length).
> 
> First, I think this is a bogus situation.  If we reqest 36, get X, and
> indicate a total of 5, then we should look at the X we got.  And that has
> to be indicated by resid.

I disagree.  For a compliant SCSI device, it's legal for it to return all
36 bytes of data, but only have the first 5 contain anything and the rest
all be NULL pad bytes and to put 0 into the extra data field.  We are
*suppossed* to be able to rely upon that extra data field being reliable.  
Just because sbp2 and usb scsi device manufacturers are such half-wit
shops that they hire entry level java programmers that couldn't write to a
standard to save their mother's lives doesn't mean that we should be
wrecking the scsi standards at the core level to compensate.

The only way to fix this up (somewhat) reliably as far as I can tell, at
the mid layer, would involve pre-clearing the INQUIRY return area then
issuing the command.  Upon command completion, check to see if extra data
length byte + 5 == cmd->length - cmd->residual.  If not, then check if
there are non-0 bytes beyond the end of the indicated extra data area.  
If so, assume real data length is cmd->length - cmd->residual and if not
then assume extra data length was correct.  

HOWEVER!  This does require changing all the lldd to set cmd->residual.  
This is currently not done, as cmd->residual is optional.  Low level
device drivers are only required to return an error condition when the
actual transfer is < cmd->underflow, they aren't required to set
cmd->residual ever.

HOWEVER2!  This is also just a heuristic and it could be fooled.

> The argument that many HBAs don't set resid just doesn't hold water with
> me.  Just because other drivers are broken, we should break more things
> instead of fixing the problem?

First off, the other drivers aren't broken.  Cmd->residual was added 
about a year or two ago as an ad hoc change to improve CD burning.  It was 
never officially added to the SCSI core API as a requirement in any 
driver.

Besides, we *aren't* talking about fixing a problem.  We are talking about
ignoring data we are suppossed to be able to rely upon to be accurate.  
Ignoring return data should only be done if you have a reasonable
suspicion that the data is wrong.  In usb, that appears to be the case.  
In real scsi devices, that is not the case.  Hence, the location of the
proper fix is, IMHO, in the usb stack (share it with spb2 via a library
call if you wish, but I don't think the scsi core should be doing it).


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

Re: Re: inquiry in scsi_scan.c

[Alan Stern]

On Mon, 6 Jan 2003, Doug Ledford wrote:

> On Mon, Jan 06, 2003 at 04:46:47PM -0800, Matthew Dharm wrote:
> > On Mon, Jan 06, 2003 at 05:23:22PM -0500, Doug Ledford wrote:
> > > Except that if a device *does* transfer 36 bytes and then lies and says it 
> > > only transferred 5 then we are missing information that might actually be 
> > > usefull, hence the reason to set the transfer length up to the real amount 
> > > transferred (and BTW, I would only do this for INQUIRY responses, for 
> > > anything else the device is simply too buggy to live if it lies about the 
> > > transfer length).
> > 
> > First, I think this is a bogus situation.  If we reqest 36, get X, and
> > indicate a total of 5, then we should look at the X we got.  And that has
> > to be indicated by resid.
> 
> I disagree.  For a compliant SCSI device, it's legal for it to return all
> 36 bytes of data, but only have the first 5 contain anything and the rest
> all be NULL pad bytes and to put 0 into the extra data field.  We are
> *suppossed* to be able to rely upon that extra data field being reliable.  
> Just because sbp2 and usb scsi device manufacturers are such half-wit
> shops that they hire entry level java programmers that couldn't write to a
> standard to save their mother's lives doesn't mean that we should be
> wrecking the scsi standards at the core level to compensate.

Hmm.  Sounds like Doug is trying to have it both ways.  He wants to be
able to handle the case of a device that transfers 36 bytes but only
indicates 5, where the remaining 31 bytes are valid and contain useful
information (a broken device).  He also wants to handle the case of a
device that transfers 36 bytes and only indicates 5, where the remaining
31 bytes only contain padding (legal according to the SCSI spec).  It
seems clear that the driver doesn't have enough information to distinguish
these cases by itself.

How about this as a compromise?  Have the fix_inquiry_data() routine in
storage/protocol.c adjust the additional-length byte if it indicates less
data than actually got transferred (and maybe also < 36 bytes), _unless_
an unusual_devs.h flag bit is set (something like
US_FL_DONT_FIX_INQUIRY_LENGTH).  This flag would only be needed for
devices that transmit useless padding, and presumably these are much rarer
than the brain-dead devices that have an invalid additional-length byte.
In fact, maybe there aren't any examples of any such USB storage devices, 
in which case the flag would be unnecessary.

Alan Stern



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel