Re: inquiry in scsi_scan.c

Re: inquiry in scsi_scan.c

[Andries.Brouwer]

Matthew Dharm writes:

> Instead of fixing this in usb-storage, I think I would rather make
> scsi_scan.c just assume a minimum of 36.

No, because (for SCSI-1) the minimum is 5.

> Or, put another way, if the first request indicates less than 36, why
> should we do a second request?  We already have all the data...

We don't do a second request.

> Actually, we ask for 36 and get 36, but the field in the response which is
> supposed to tell us how much there is total is zeroed out, instead of
> having a real value.

Right.

> All we need to do is recognize when that field indicates less than 36
> bytes, and then stop asking for anything else.  Either (a) the device is
> lying, in which case our original INQUIRY is fine, or (b) the device really
> has less than 36 bytes, which means that we already have all the data.

I think you misunderstand the problems. We do not ask for anything else.
There are two problems: a SCSI and a USB problem.

In the SCSI code a length of 5 is legal. Now the code
allocates space for these 5 bytes but subsequently uses
pointers to vendor etc that point past the end of the allocated area.
If ever anything is written via these pointers random memory is corrupted.
And "cat /proc/scsi/scsi" shows randow junk.
A bug that has to be fixed, independently of USB.

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.

Andries


[the code I showed does the right things; will submit actual diffs
sooner or later]

Re: inquiry in scsi_scan.c

[Luben Tuikov]

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

And what if the transport is *not* USB? Or they used
a similar firmware of their device server in another
product which used another transport?

I suggest that this device is blacklisted in that
SCSI Core would know that the ADDITIONAL LENGTH field
in the INQURY response is incorrectly set (to 0).
I.e. leave it to the interpreter.

A transport is *not* supposed to peek and poke in the
data it transfers!

-- 
Luben

Re: inquiry in scsi_scan.c

[Zwane Mwaikambo]

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.

This looks related to something i also bumped into;

scsi scan: host 2 channel 0 id 0 lun 0 identifier too long, length 60, max
50. Device might be improperly identified.

The 'HBA' is idescsi with a memorex cdrw with an inquiry returning a
length value of 34

scsi_check_fill_deviceid:
...
	if (scsi_check_id_size(sdev, 26 + id_page[3]))

I wrote up an ugly hack to truncate the length in idescsi_transform_pc2,
but i don't know SCSI and it doesn't seem right.

> [the code I showed does the right things; will submit actual diffs
> sooner or later]

Thanks,
	Zwane
-- 
function.linuxpower.ca

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