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

[Andries.Brouwer]

Zwane Mwaikambo writes:

> This looks related to something i also bumped into
>
> scsi scan: host 2 channel 0 id 0 lun 0 identifier too long

Sounds familiar. Please try the below (on 2.5.54).

Andries


diff -u --recursive --new-file -X /linux/dontdiff a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
--- a/drivers/scsi/scsi_scan.c	Wed Jan  1 23:54:23 2003
+++ b/drivers/scsi/scsi_scan.c	Sun Jan  5 14:22:21 2003
@@ -544,32 +544,6 @@
 }
 
 /**
- * scsi_check_id_size - check if size fits in the driverfs name
- * @sdev:	Scsi_Device to use for error message
- * @size:	the length of the id we want to store
- *
- * Description:
- *     Use a function for this since the same code is used in various
- *     places, and we only create one string and call to printk.
- *
- * Return:
- *     0 - fits
- *     1 - size too large
- **/
-static int scsi_check_id_size(Scsi_Device *sdev, int size)
-{
-	if (size > DEVICE_NAME_SIZE) {
-		printk(KERN_WARNING "scsi scan: host %d channel %d id %d lun %d"
-		       " identifier too long, length %d, max %d. Device might"
-		       " be improperly identified.\n", sdev->host->host_no,
-		       sdev->channel, sdev->id, sdev->lun, size,
-		       DEVICE_NAME_SIZE);
-		return 1;
-	} else
-		return 0;
-}
-
-/**
  * scsi_get_evpd_page - get a list of supported vpd pages
  * @sdev:	Scsi_Device to send an INQUIRY VPD
  * @sreq:	Scsi_Request associated with @sdev
@@ -715,17 +689,16 @@
  * scsi_check_fill_deviceid - check the id and if OK fill it
  * @sdev:	device to use for error messages
  * @id_page:	id descriptor for INQUIRY VPD DEVICE ID, page 0x83
- * @name:	store the id in name
+ * @name:	store the id in name (of size DEVICE_NAME_SIZE > 26)
  * @id_search:	store if the id_page matches these values
  *
  * Description:
  *     Check if @id_page matches the @id_search, if so store an id (uid)
- *     into name.
+ *     into name, that is all zero on entrance.
  *
  * Return:
  *     0: Success
  *     1: No match
- *     2: Failure due to size constraints
  **/
 static int scsi_check_fill_deviceid(Scsi_Device *sdev, char *id_page,
 	char *name, const struct scsi_id_search_values *id_search)
@@ -755,70 +728,41 @@
 	if ((id_page[0] & 0x0f) != id_search->code_set)
 		return 1;
 
-	name[0]  = hex_str[id_search->id_type];
+	/*
+	 * All OK - store ID
+	 */
+	name[0] = hex_str[id_search->id_type];
+
+	/*
+	 * Prepend the vendor and model before the id, since the id
+	 * might not be unique across all vendors and models.
+	 * The same code is used below, with a different size.
+	 */
+	if (id_search->id_type == SCSI_ID_VENDOR_SPECIFIC) {
+		strncat(name, sdev->vendor, 8);
+		strncat(name, sdev->model, 16);
+	}
+
+	i = 4;
+	j = strlen(name);
 	if ((id_page[0] & 0x0f) == SCSI_ID_ASCII) {
 		/*
 		 * ASCII descriptor.
 		 */
-		if (id_search->id_type == SCSI_ID_VENDOR_SPECIFIC) {
-			/*
-			 * Prepend the vendor and model before the id,
-			 * since the id might not be unique across all
-			 * vendors and models. The same code is used
-			 * below, with a differnt size.
-			 *
-			 * Need 1 byte for the idtype, 1 for trailing
-			 * '\0', 8 for vendor, 16 for model total 26, plus
-			 * the name descriptor length.
-			 */
-			if (scsi_check_id_size(sdev, 26 + id_page[3]))
-				return 2;
-			else {
-				strncat(name, sdev->vendor, 8);
-				strncat(name, sdev->model, 16);
-			}
-		} else if (scsi_check_id_size (sdev, (2 + id_page[3])))
-			/*
-			 * Need 1 byte for the idtype, 1 byte for
-			 * the trailing '\0', plus the descriptor length.
-			 */
-			return 2;
-		memcpy(&name[strlen(name)], &id_page[4], id_page[3]);
-		return 0;
-	} else if ((id_page[0] & 0x0f) == SCSI_ID_BINARY) {
-		if (id_search->id_type == SCSI_ID_VENDOR_SPECIFIC) {
-			/*
-			 * Prepend the vendor and model before the id.
-			 */
-			if (scsi_check_id_size(sdev, 26 + (id_page[3] * 2)))
-				return 2;
-			else {
-				strncat(name, sdev->vendor, 8);
-				strncat(name, sdev->model, 16);
-			}
-		} else if (scsi_check_id_size(sdev, 2 + (id_page[3] * 2)))
-			/*
-			 * Need 1 byte for the idtype, 1 for trailing
-			 * '\0', 8 for vendor, 16 for model total 26, plus
-			 * the name descriptor length.
-			 */
-			return 2;
+		while (i < 4 + id_page[3] && j < DEVICE_NAME_SIZE-1)
+			name[j++] = id_page[i++];
+	} else {
 		/*
 		 * Binary descriptor, convert to ASCII, using two bytes of
-		 * ASCII for each byte in the id_page. Store starting at
-		 * the end of name.
+		 * ASCII for each byte in the id_page.
 		 */
-		for(i = 4, j = strlen(name); i < 4 + id_page[3]; i++) {
+		while (i < 4 + id_page[3] && j < DEVICE_NAME_SIZE-2) {
 			name[j++] = hex_str[(id_page[i] & 0xf0) >> 4];
 			name[j++] = hex_str[id_page[i] & 0x0f];
+			i++;
 		}
-		return 0;
 	}
-	/*
-	 * Code set must have already matched.
-	 */
-	printk(KERN_ERR "scsi scan: scsi_check_fill_deviceid unexpected state.\n");
-	return 1;
+	return 0;
 }
 
 /**
@@ -834,7 +778,7 @@
  *     0: Failure
  *     1: Success
  **/
-int scsi_get_deviceid(Scsi_Device *sdev, Scsi_Request *sreq)
+static int scsi_get_deviceid(Scsi_Device *sdev, Scsi_Request *sreq)
 {
 	unsigned char *id_page;
 	unsigned char scsi_cmd[MAX_COMMAND_SIZE];
@@ -879,14 +823,14 @@
 	}
 
 	/*
-	 * Search for a match in the proiritized id_search_list.
+	 * Search for a match in the prioritized id_search_list.
 	 */
 	for (id_idx = 0; id_idx < ARRAY_SIZE(id_search_list); id_idx++) {
 		/*
 		 * Examine each descriptor returned. There is normally only
 		 * one or a small number of descriptors.
 		 */
-		for(scnt = 4; scnt <= id_page[3] + 3;
+		for (scnt = 4; scnt <= id_page[3] + 3;
 			scnt += id_page[scnt + 3] + 4) {
 			if ((scsi_check_fill_deviceid(sdev, &id_page[scnt],
 			     sdev->sdev_driverfs_dev.name,
@@ -941,12 +885,11 @@
 {
 	unsigned char *serialnumber_page;
 	unsigned char scsi_cmd[MAX_COMMAND_SIZE];
-	int max_lgth = 255;
+	const int max_lgth = 255;
+	int len;
 
- retry:
 	serialnumber_page = kmalloc(max_lgth, GFP_ATOMIC |
-			      (sdev->host->unchecked_isa_dma) ?
-			      GFP_DMA : 0);
+			      (sdev->host->unchecked_isa_dma) ? GFP_DMA : 0);
 	if (!serialnumber_page) {
 		printk(KERN_WARNING "scsi scan: Allocation failure identifying"
 		       " host %d channel %d id %d lun %d, device might be"
@@ -969,26 +912,19 @@
 
 	if (sreq->sr_result)
 		goto leave;
-	/*
-	 * check to see if response was truncated
-	 */
-	if (serialnumber_page[3] > max_lgth) {
-		max_lgth = serialnumber_page[3] + 4;
-		kfree(serialnumber_page);
-		goto retry;
-	}
 
 	/*
-	 * Need 1 byte for SCSI_UID_SER_NUM, 1 for trailing '\0', 8 for
-	 * vendor, 16 for model = 26, plus serial number size.
+	 * a check to see if response was truncated is superfluous,
+	 * since serialnumber_page[3] cannot be larger than 255
 	 */
-	if (scsi_check_id_size (sdev, (26 + serialnumber_page[3])))
-		goto leave;
+
 	sdev->sdev_driverfs_dev.name[0] = SCSI_UID_SER_NUM;
 	strncat(sdev->sdev_driverfs_dev.name, sdev->vendor, 8);
 	strncat(sdev->sdev_driverfs_dev.name, sdev->model, 16);
-	strncat(sdev->sdev_driverfs_dev.name, &serialnumber_page[4],
-		serialnumber_page[3]);
+	len = serialnumber_page[3];
+	if (len > DEVICE_NAME_SIZE-26)
+		len = DEVICE_NAME_SIZE-26;
+	strncat(sdev->sdev_driverfs_dev.name, &serialnumber_page[4], len);
 	kfree(serialnumber_page);
 	return 1;
  leave:
@@ -1002,23 +938,19 @@
  * @sdev:	get a default name for this device
  *
  * Description:
- *     Set the name of @sdev to the concatenation of the vendor, model,
- *     and revision found in @sdev.
+ *     Set the name of @sdev (of size DEVICE_NAME_SIZE > 29) to the
+ *     concatenation of the vendor, model, and revision found in @sdev.
  *
  * Return:
  *     1: Success
  **/
 int scsi_get_default_name(Scsi_Device *sdev)
 {
-	if (scsi_check_id_size(sdev, 29))
-		return 0;
-	else {
-		sdev->sdev_driverfs_dev.name[0] = SCSI_UID_UNKNOWN;
-		strncpy(&sdev->sdev_driverfs_dev.name[1], sdev->vendor, 8);
-		strncat(sdev->sdev_driverfs_dev.name, sdev->model, 16);
-		strncat(sdev->sdev_driverfs_dev.name, sdev->rev, 4);
-		return 1;
-	}
+	sdev->sdev_driverfs_dev.name[0] = SCSI_UID_UNKNOWN;
+	strncpy(&sdev->sdev_driverfs_dev.name[1], sdev->vendor, 8);
+	strncat(sdev->sdev_driverfs_dev.name, sdev->model, 16);
+	strncat(sdev->sdev_driverfs_dev.name, sdev->rev, 4);
+	return 1;
 }
 
 /**

Re: inquiry in scsi_scan.c

[Patrick Mansfield]

On Sun, Jan 05, 2003 at 10:42:46PM +0100, Andries.Brouwer@cwi.nl wrote:
> Zwane Mwaikambo writes:
> 
> > This looks related to something i also bumped into
> >
> > scsi scan: host 2 channel 0 id 0 lun 0 identifier too long
> 
> Sounds familiar. Please try the below (on 2.5.54).
> 
> Andries
> 

Instead of truncating the id, we need a new scsi_uid field allocated
to whatever size required. And, a descriptive string put in the name,
rather than the id, such as:

    scsi disk
    scsi processor
    scsi tape 

Even if the id is truncated, that information has to be available.

-- Patrick Mansfield

Re: inquiry in scsi_scan.c

[Andries.Brouwer]

    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.

    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!


There are at least four replies:

The factual: It seems you are unaware of the present USB storage code.
For many devices the INQUIRY response is entirely fabricated.

The vicious circle: The SCSI blacklist works by attaching quirks
to vendor and model data. This fails when the quirk is precisely
that vendor and model data are not reported.

The theoretical: USB-storage is the SCSI host - it is responsible
for presenting the SCSI layer with a device that complies with the
SCSI standard. If any blacklisting is to be done it must be
blacklisting in the USB storage code, not in the SCSI code.
(And that blacklist exists, of course - it is called unusual_devs.h.)

The practical: USB devices are notoriously bad as far as standard
compliance is concerned. If it works with Windows that is good
enough. That standard, too expensive to implement it all, or,
after implementing, to test it all.
Your philosophy leads to blacklisting almost every USB storage device
(I possess a dozen or so, not a single one without quirks).

Of course that is a possibility: describe for every device on the market
in what ways it fails. But it is counterproductive. When people buy
a new device it would be nice if it worked with Linux immediately,
not first after adding its quirks to some list. Indeed, several times
a week I read someone reporting "add this to unusual_devs.h to make
this device work". No doubt thousands of people just decide that their
device does not work with Linux. In cases where it is possible to
automatically detect and correct faulty data no list of quirks is
required, and more devices will work with Linux out-of-the-box.

Andries

Re: inquiry in scsi_scan.c

[Luben Tuikov]

Andries.Brouwer@cwi.nl wrote:
> 
> There are at least four replies:
> 
> The factual: It seems you are unaware of the present USB storage code.
> For many devices the INQUIRY response is entirely fabricated.

I'm aware of this, but you were complaining of a new device
which you got, so I assumed the INQUIRY response data came
from the device, in your particular situation.
 
> The vicious circle: The SCSI blacklist works by attaching quirks
> to vendor and model data. This fails when the quirk is precisely
> that vendor and model data are not reported.

I agree with this.
 
> The theoretical: USB-storage is the SCSI host - it is responsible
> for presenting the SCSI layer with a device that complies with the
> SCSI standard. If any blacklisting is to be done it must be
> blacklisting in the USB storage code, not in the SCSI code.
> (And that blacklist exists, of course - it is called unusual_devs.h.)

I'm aware of this list.
 
> The practical: USB devices are notoriously bad as far as standard
> compliance is concerned. If it works with Windows that is good
> enough. That standard, too expensive to implement it all, or,
> after implementing, to test it all.
> Your philosophy leads to blacklisting almost every USB storage device
> (I possess a dozen or so, not a single one without quirks).
> 
> Of course that is a possibility: describe for every device on the market
> in what ways it fails. But it is counterproductive. When people buy
> a new device it would be nice if it worked with Linux immediately,
> not first after adding its quirks to some list. Indeed, several times
> a week I read someone reporting "add this to unusual_devs.h to make
> this device work". No doubt thousands of people just decide that their
> device does not work with Linux. In cases where it is possible to
> automatically detect and correct faulty data no list of quirks is
> required, and more devices will work with Linux out-of-the-box.
> 

Yes, I did understand all this from your first email -- the practicallity
of the matter. This is good.

I was just speaking out of principle, to present the other side.
Sometimes it's better to present a fix out of principle rather than
a particuliarity, this abstractizes further up and provides a long
lasting solution.

But yes, this is a pickle of a problem.

-- 
Luben

Re: inquiry in scsi_scan.c

[Andries.Brouwer]

By the way - there are other cases where the INQUIRY length is
reported incorrectly. Another device does:

usb_stor_bulk_transfer_buf(): xfer 37 bytes
 00 80 02 02 20 00 00 00 65 55 53 42 20 20 20 20
 43 6F 6D 70 61 63 74 20 46 6C 61 73 68 20 20 20
 00 00 00 00
usb-storage: Status code 0; transferred 36/37
usb-storage: -- short transfer

In other words, we asked for 36, got 36 but the 0x20
indicated that the full length is 37. So we ask a second time,
but learn that only 36 bytes are available.

An off-by-one, as happens more often.

Fortunately this device does not hang, so is not yet a reason
to introduce additional patch code.

Andries

Re: inquiry in scsi_scan.c

[Andries.Brouwer]

Matthew Dharm writes:

> There should probably be a sanity check to never ask for INQUIRY
> less than 36 bytes.  I thought there used to be such a thing....

As Doug also points out, we ask for 36, but there is no
guarantee that we get what we ask for.

> Actually, 5 isn't minimal... it's sub-minimal.
> That's an error in the INQUIRY data/
> The minimum (by spec) is 36 bytes.

No. Quoting:

"The INQUIRY data (Table 7-9) contains a five byte header,
followed by the vendor unique parameters, if any."
(SCSI-1 standard)

So, as long as we are willing to support SCSI-1 devices,
we must accept that the INQUIRY data can be as short as this.
And in fact all our other code is careful - look at
print_inquiry() how before looking at a byte we check
whether it really there.


On the other hand, my case was not an ancient SCSI-1 device,
it was a brand new USB device. So, I have the SCSI host in hand.
Looking at what happens:

usb-storage: usb_stor_bulk_transfer_buf(): xfer 36 bytes
 00 80 00 00 00 00 00 00 4F 45 49 2D 55 53 42 20
 53 6D 61 72 74 4D 65 64 69 61 20 20 20 20 20 20
 32 2E 30 35
usb-storage: Status code 0; transferred 36/36
usb-storage: Fixing INQUIRY data to show SCSI rev 2 - was 0
usb-storage: Fixing INQUIRY data to show length 36 - was 0

and all is fine.

Instead of the old garbage I now see:
% cat /proc/scsi/scsi
...
Host: scsi2 Channel: 00 Id: 00 Lun: 00
  Vendor: OEI-USB  Model: SmartMedia       Rev: 2.05
  Type:   Direct-Access                    ANSI SCSI revision: 02
...


Conclusion:
(i) scsi_scan.c has to be careful about INQUIRY lengths,
and some patch is required for devices that return a short length.
(ii) usb-storage knows the transport length, and can fix it
in case it is (5+)0. For example, in protocol.c:fix_inquiry_data():

static void fix_inquiry_data(Scsi_Cmnd *srb)
{
        unsigned char *data_ptr;

        /* verify that it's an INQUIRY command */
        if (srb->cmnd[0] != INQUIRY)
                return;

        data_ptr = find_data_location(srb);

        if ((data_ptr[2] & 7) != 2) {
                US_DEBUGP("Fixing INQUIRY data to show SCSI rev 2 - was %d\n",
                          data_ptr[2] & 7);

                /* Change the SCSI revision number */
                data_ptr[2] = (data_ptr[2] & ~7) | 2;
        }

        if (data_ptr[4] == 0) {
                US_DEBUGP("Fixing INQUIRY data to show length 36 - was 0\n");
                data_ptr[4] = 36 - 5;
        }
}

Andries

Re: inquiry in scsi_scan.c

[Matthew Dharm]

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

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

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

Matt

On Sat, Jan 04, 2003 at 04:07:51AM +0100, Andries.Brouwer@cwi.nl wrote:
> Matthew Dharm writes:
> 
> > There should probably be a sanity check to never ask for INQUIRY
> > less than 36 bytes.  I thought there used to be such a thing....
> 
> As Doug also points out, we ask for 36, but there is no
> guarantee that we get what we ask for.
> 
> > Actually, 5 isn't minimal... it's sub-minimal.
> > That's an error in the INQUIRY data/
> > The minimum (by spec) is 36 bytes.
> 
> No. Quoting:
> 
> "The INQUIRY data (Table 7-9) contains a five byte header,
> followed by the vendor unique parameters, if any."
> (SCSI-1 standard)
> 
> So, as long as we are willing to support SCSI-1 devices,
> we must accept that the INQUIRY data can be as short as this.
> And in fact all our other code is careful - look at
> print_inquiry() how before looking at a byte we check
> whether it really there.
> 
> 
> On the other hand, my case was not an ancient SCSI-1 device,
> it was a brand new USB device. So, I have the SCSI host in hand.
> Looking at what happens:
> 
> usb-storage: usb_stor_bulk_transfer_buf(): xfer 36 bytes
>  00 80 00 00 00 00 00 00 4F 45 49 2D 55 53 42 20
>  53 6D 61 72 74 4D 65 64 69 61 20 20 20 20 20 20
>  32 2E 30 35
> usb-storage: Status code 0; transferred 36/36
> usb-storage: Fixing INQUIRY data to show SCSI rev 2 - was 0
> usb-storage: Fixing INQUIRY data to show length 36 - was 0
> 
> and all is fine.
> 
> Instead of the old garbage I now see:
> % cat /proc/scsi/scsi
> ...
> Host: scsi2 Channel: 00 Id: 00 Lun: 00
>   Vendor: OEI-USB  Model: SmartMedia       Rev: 2.05
>   Type:   Direct-Access                    ANSI SCSI revision: 02
> ...
> 
> 
> Conclusion:
> (i) scsi_scan.c has to be careful about INQUIRY lengths,
> and some patch is required for devices that return a short length.
> (ii) usb-storage knows the transport length, and can fix it
> in case it is (5+)0. For example, in protocol.c:fix_inquiry_data():
> 
> static void fix_inquiry_data(Scsi_Cmnd *srb)
> {
>         unsigned char *data_ptr;
> 
>         /* verify that it's an INQUIRY command */
>         if (srb->cmnd[0] != INQUIRY)
>                 return;
> 
>         data_ptr = find_data_location(srb);
> 
>         if ((data_ptr[2] & 7) != 2) {
>                 US_DEBUGP("Fixing INQUIRY data to show SCSI rev 2 - was %d\n",
>                           data_ptr[2] & 7);
> 
>                 /* Change the SCSI revision number */
>                 data_ptr[2] = (data_ptr[2] & ~7) | 2;
>         }
> 
>         if (data_ptr[4] == 0) {
>                 US_DEBUGP("Fixing INQUIRY data to show length 36 - was 0\n");
>                 data_ptr[4] = 36 - 5;
>         }
> }
> 
> Andries

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

inquiry in scsi_scan.c

[Andries.Brouwer]

Got a new USB device and noticed some scsi silliness while playing with it.

A bug in scsi_scan.c is

        sdev->inquiry = kmalloc(sdev->inquiry_len, GFP_ATOMIC);
        memcpy(sdev->inquiry, inq_result, sdev->inquiry_len);
        sdev->vendor = (char *) (sdev->inquiry + 8);
        sdev->model = (char *) (sdev->inquiry + 16);
        sdev->rev = (char *) (sdev->inquiry + 32);

since it happens that inquiry_len is short (in my case it is 5)
and the vendor/model/rev pointers are wild.
Catting /proc/scsi/scsi now yields random garbage.

I made a patch but hesitated between a small patch and a larger one.
Why do we have this malloced inquiry field? As far as I can see
nobody uses it. And the comment in scsi_add_lun() advises us
not to save the inquiry, precisely what we did until recently.
So, should this change from 2.5.11 be reverted?

Andries


[My present scsi_scan.c differes quite a lot from a stock one.
Already fixed the scsi_check_id_size() some time ago.
Below some diff fragments from today.]

+/*
+ * Do an INQUIRY with given length (minimum 5, maximum 255).
+ * Note: many devices react badly when given an unexpected length.
+ */
+static int
+scsi_do_inquiry(Scsi_Request *sreq, char *buffer, int len) {
+       Scsi_Device *sdev = sreq->sr_device;
+       unsigned char cmd[6];
+       int res;
+
+       SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO "scsi scan: INQUIRY (len %d) "
+                                   "to host %d channel %d id %d lun %d\n",
+                                   len, sdev->host->host_no, sdev->channel,
+                                   sdev->id, sdev->lun));
+
+       memset(cmd, 0, 6);
+       cmd[0] = INQUIRY;
+       cmd[4] = len;
+       sreq->sr_cmd_len = 0;
+       sreq->sr_data_direction = SCSI_DATA_READ;
+       memset(buffer, 0, len);
+
+       scsi_wait_req(sreq, (void *) cmd, (void *) buffer, len,
+                     SCSI_TIMEOUT + 4 * HZ, 3);
+
+       res = sreq->sr_result;
+       SCSI_LOG_SCAN_BUS(3, printk(res ? KERN_INFO "scsi scan: "
+                                   "INQUIRY returned code 0x%x\n" :
+                                   KERN_INFO "scsi scan: INQUIRY OK\n", res));
+       return res;
+}

+/*
+ * The inquiry length is  inq_result[4] + 5  unless overridden.
+ */
+static int
+valid_inquiry_lth(int bflags, unsigned char *inq_result) {
+       int len = ((bflags & BLIST_INQUIRY_36) ? 36 :
+                  (bflags & BLIST_INQUIRY_58) ? 58 :
+                  inq_result[4] + 5);
+       if (len > 255)
+               len = 36;       /* sanity */
+       return len;
+}

Text in scsi_probe_lun(), including two reactions to comments:

static void
scsi_probe_lun(Scsi_Request *sreq, char *inq_result, int *bflags)
{
        Scsi_Device *sdev = sreq->sr_device;    /* a bit ugly */
        unsigned char scsi_cmd[MAX_COMMAND_SIZE];
        int res, possible_inq_resp_len;

        /* first issue an inquiry with conservative alloc_length */
        res = scsi_do_inquiry(sreq, inq_result, 36);

        if (res) {
                if ((driver_byte(res) & DRIVER_SENSE) != 0 &&
                    (sreq->sr_sense_buffer[2] & 0xf) == UNIT_ATTENTION &&
                    sreq->sr_sense_buffer[12] == 0x28 &&
                    sreq->sr_sense_buffer[13] == 0) {
                        /* not-ready to ready transition - good */
                        /* dpg: bogus? INQUIRY never returns UNIT_ATTENTION */
                        /* aeb: seen with an USB CF card reader */
                } else
                        /*
                         * assume no peripheral if any other sort of error
                         */
                        return;
        }

        /*
         * Get any flags for this device.
         *
         * Some devices return only 5 bytes for an INQUIRY, but the memset
         * in scsi_do_inquiry makes sure that scsi_get_device_flags() gets
         * well-defined arguments.
         */
        *bflags = scsi_get_device_flags(&inq_result[8], &inq_result[16]);

        possible_inq_resp_len = valid_inquiry_lth(*bflags, inq_result);

        if (possible_inq_resp_len > 36) {       /* do additional INQUIRY */
                res = scsi_do_inquiry(sreq, inq_result, possible_inq_resp_len);
                if (res)
                        return;

                /*
                 * The INQUIRY can change, this means the length can change.
                 */
                possible_inq_resp_len = valid_inquiry_lth(*bflags, inq_result);
        }

        sdev->inquiry_len = possible_inq_resp_len;

        /*
         * Abort if the response length is less than 36?
         * No, some USB devices just produce the minimal 5-byte INQUIRY.
         *
         * ...
         */

Re: inquiry in scsi_scan.c

[Matthew Dharm]

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

Actually, 5 isn't minimal... it's sub-minimal.  That's an error in the
INQUIRY data.  The minimum (by spec) is 36 bytes.

There should probably be a sanity check to never ask for INQUIRY less than
36 bytes.  I thought there used to be such a thing....

Matt

On Sat, Jan 04, 2003 at 01:21:11AM +0100, Andries.Brouwer@cwi.nl wrote:
> Got a new USB device and noticed some scsi silliness while playing with it.
> 
> A bug in scsi_scan.c is
> 
>         sdev->inquiry = kmalloc(sdev->inquiry_len, GFP_ATOMIC);
>         memcpy(sdev->inquiry, inq_result, sdev->inquiry_len);
>         sdev->vendor = (char *) (sdev->inquiry + 8);
>         sdev->model = (char *) (sdev->inquiry + 16);
>         sdev->rev = (char *) (sdev->inquiry + 32);
> 
> since it happens that inquiry_len is short (in my case it is 5)
> and the vendor/model/rev pointers are wild.
> Catting /proc/scsi/scsi now yields random garbage.
> 
> I made a patch but hesitated between a small patch and a larger one.
> Why do we have this malloced inquiry field? As far as I can see
> nobody uses it. And the comment in scsi_add_lun() advises us
> not to save the inquiry, precisely what we did until recently.
> So, should this change from 2.5.11 be reverted?
> 
> Andries
> 
> 
> [My present scsi_scan.c differes quite a lot from a stock one.
> Already fixed the scsi_check_id_size() some time ago.
> Below some diff fragments from today.]
> 
> +/*
> + * Do an INQUIRY with given length (minimum 5, maximum 255).
> + * Note: many devices react badly when given an unexpected length.
> + */
> +static int
> +scsi_do_inquiry(Scsi_Request *sreq, char *buffer, int len) {
> +       Scsi_Device *sdev = sreq->sr_device;
> +       unsigned char cmd[6];
> +       int res;
> +
> +       SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO "scsi scan: INQUIRY (len %d) "
> +                                   "to host %d channel %d id %d lun %d\n",
> +                                   len, sdev->host->host_no, sdev->channel,
> +                                   sdev->id, sdev->lun));
> +
> +       memset(cmd, 0, 6);
> +       cmd[0] = INQUIRY;
> +       cmd[4] = len;
> +       sreq->sr_cmd_len = 0;
> +       sreq->sr_data_direction = SCSI_DATA_READ;
> +       memset(buffer, 0, len);
> +
> +       scsi_wait_req(sreq, (void *) cmd, (void *) buffer, len,
> +                     SCSI_TIMEOUT + 4 * HZ, 3);
> +
> +       res = sreq->sr_result;
> +       SCSI_LOG_SCAN_BUS(3, printk(res ? KERN_INFO "scsi scan: "
> +                                   "INQUIRY returned code 0x%x\n" :
> +                                   KERN_INFO "scsi scan: INQUIRY OK\n", res));
> +       return res;
> +}
> 
> +/*
> + * The inquiry length is  inq_result[4] + 5  unless overridden.
> + */
> +static int
> +valid_inquiry_lth(int bflags, unsigned char *inq_result) {
> +       int len = ((bflags & BLIST_INQUIRY_36) ? 36 :
> +                  (bflags & BLIST_INQUIRY_58) ? 58 :
> +                  inq_result[4] + 5);
> +       if (len > 255)
> +               len = 36;       /* sanity */
> +       return len;
> +}
> 
> Text in scsi_probe_lun(), including two reactions to comments:
> 
> static void
> scsi_probe_lun(Scsi_Request *sreq, char *inq_result, int *bflags)
> {
>         Scsi_Device *sdev = sreq->sr_device;    /* a bit ugly */
>         unsigned char scsi_cmd[MAX_COMMAND_SIZE];
>         int res, possible_inq_resp_len;
> 
>         /* first issue an inquiry with conservative alloc_length */
>         res = scsi_do_inquiry(sreq, inq_result, 36);
> 
>         if (res) {
>                 if ((driver_byte(res) & DRIVER_SENSE) != 0 &&
>                     (sreq->sr_sense_buffer[2] & 0xf) == UNIT_ATTENTION &&
>                     sreq->sr_sense_buffer[12] == 0x28 &&
>                     sreq->sr_sense_buffer[13] == 0) {
>                         /* not-ready to ready transition - good */
>                         /* dpg: bogus? INQUIRY never returns UNIT_ATTENTION */
>                         /* aeb: seen with an USB CF card reader */
>                 } else
>                         /*
>                          * assume no peripheral if any other sort of error
>                          */
>                         return;
>         }
> 
>         /*
>          * Get any flags for this device.
>          *
>          * Some devices return only 5 bytes for an INQUIRY, but the memset
>          * in scsi_do_inquiry makes sure that scsi_get_device_flags() gets
>          * well-defined arguments.
>          */
>         *bflags = scsi_get_device_flags(&inq_result[8], &inq_result[16]);
> 
>         possible_inq_resp_len = valid_inquiry_lth(*bflags, inq_result);
> 
>         if (possible_inq_resp_len > 36) {       /* do additional INQUIRY */
>                 res = scsi_do_inquiry(sreq, inq_result, possible_inq_resp_len);
>                 if (res)
>                         return;
> 
>                 /*
>                  * The INQUIRY can change, this means the length can change.
>                  */
>                 possible_inq_resp_len = valid_inquiry_lth(*bflags, inq_result);
>         }
> 
>         sdev->inquiry_len = possible_inq_resp_len;
> 
>         /*
>          * Abort if the response length is less than 36?
>          * No, some USB devices just produce the minimal 5-byte INQUIRY.
>          *
>          * ...
>          */
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

You were using cheat codes too.  You guys suck.
					-- Greg to General Studebaker
User Friendly, 12/16/1997

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

Re: inquiry in scsi_scan.c

[Douglas Gilbert]

Matthew Dharm wrote:
> Actually, 5 isn't minimal... it's sub-minimal.  That's an error in the
> INQUIRY data.  The minimum (by spec) is 36 bytes.
> 
> There should probably be a sanity check to never ask for INQUIRY less than
> 36 bytes.  I thought there used to be such a thing....

Matt,
The scan code does a 36 byte INQUIRY first and then does
a second INQUIRY if the response indicates more (than 36
bytes) is available. In this case _less_ than 36 bytes
was supplied. So either the scsi scan code or usb-storage **
needs to make some pro forma vendor, model and rev strings.

We found previously that doing a longer INQUIRY (254
or 255 bytes) locked up some [arguably broken] usb devices.
Hence we switched to the twin INQUIRY strategy. Evidentally
FreeBSD does the same thing.

Throwing away such broken devices is not an option I guess.
What does that device look like under Windows?


** Assuming the usb device in question is using usb-storage,
shouldn't it make sure at least 36 (valid) bytes of response
data is supplied by an INQUIRY? Andrew Morton had to do this
(perhaps at a lower level in the usb stack) to make some device
he had play with the scsi subsystem.

Doug Gilbert

> On Sat, Jan 04, 2003 at 01:21:11AM +0100, Andries.Brouwer@cwi.nl wrote:
> 
>>Got a new USB device and noticed some scsi silliness while playing with it.
>>
>>A bug in scsi_scan.c is
>>
>>        sdev->inquiry = kmalloc(sdev->inquiry_len, GFP_ATOMIC);
>>        memcpy(sdev->inquiry, inq_result, sdev->inquiry_len);
>>        sdev->vendor = (char *) (sdev->inquiry + 8);
>>        sdev->model = (char *) (sdev->inquiry + 16);
>>        sdev->rev = (char *) (sdev->inquiry + 32);
>>
>>since it happens that inquiry_len is short (in my case it is 5)
>>and the vendor/model/rev pointers are wild.
>>Catting /proc/scsi/scsi now yields random garbage.
>>
>>I made a patch but hesitated between a small patch and a larger one.
>>Why do we have this malloced inquiry field? As far as I can see
>>nobody uses it. And the comment in scsi_add_lun() advises us
>>not to save the inquiry, precisely what we did until recently.
>>So, should this change from 2.5.11 be reverted?
>>
>>Andries
>>
>>
>>[My present scsi_scan.c differes quite a lot from a stock one.
>>Already fixed the scsi_check_id_size() some time ago.

Re: inquiry in scsi_scan.c

[Patrick Mansfield]

On Fri, Jan 03, 2003 at 05:04:04PM -0800, Matthew Dharm wrote:
> Actually, 5 isn't minimal... it's sub-minimal.  That's an error in the
> INQUIRY data.  The minimum (by spec) is 36 bytes.
> 
> There should probably be a sanity check to never ask for INQUIRY less than
> 36 bytes.  I thought there used to be such a thing....
> 
> Matt

Well we do ask for 36, but in Andries case only got back 5.

The zero fill is a good solution and matches what the device should do if it
can't get all of the INQUIRY data.

There should be a warning output, in case other bad things happen, and so
people know they have a borken device, and maybe a comment like:

        /*
         * If the response length is less than 36 the rest of the INQUIRY
	 * is zero filled.
	 *
	 * etc.
         */

> 
> On Sat, Jan 04, 2003 at 01:21:11AM +0100, Andries.Brouwer@cwi.nl wrote:
> > Got a new USB device and noticed some scsi silliness while playing with it.
> > 
> > A bug in scsi_scan.c is
> > 
> >         sdev->inquiry = kmalloc(sdev->inquiry_len, GFP_ATOMIC);
> >         memcpy(sdev->inquiry, inq_result, sdev->inquiry_len);
> >         sdev->vendor = (char *) (sdev->inquiry + 8);
> >         sdev->model = (char *) (sdev->inquiry + 16);
> >         sdev->rev = (char *) (sdev->inquiry + 32);
> > 
> > since it happens that inquiry_len is short (in my case it is 5)
> > and the vendor/model/rev pointers are wild.
> > Catting /proc/scsi/scsi now yields random garbage.

> > I made a patch but hesitated between a small patch and a larger one.
> > Why do we have this malloced inquiry field? As far as I can see
> > nobody uses it. And the comment in scsi_add_lun() advises us
> > not to save the inquiry, precisely what we did until recently.
> > So, should this change from 2.5.11 be reverted?

The INQUIRY was saved so that users could avoid sending a long INQUIRY to
a borken device and hang it, and just send an ioctl to retrieve it, but no
ioctl was ever added.

Rather than not saving it, we should just get the INQUIRY again (and
implicitly overwrite vendor/model/rev) after spin up or if we get a unit
attention with additional sense code of INQUIRY DATA HAS CHANGED; this
would probably break somehow for some devices (changing the INQUIRY after
a spin up is not out of spec), and is hard to handle with our current
scanning (scsi_scan.c sends the INQUIRY, but sd.c has the spin up code),
as per comments in scsi_probe_lun.

-- Patrick Mansfield

Re: inquiry in scsi_scan.c

[Matthew Dharm]

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

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.

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.

Matt

On Fri, Jan 03, 2003 at 06:44:58PM -0800, Patrick Mansfield wrote:
> On Fri, Jan 03, 2003 at 05:04:04PM -0800, Matthew Dharm wrote:
> > Actually, 5 isn't minimal... it's sub-minimal.  That's an error in the
> > INQUIRY data.  The minimum (by spec) is 36 bytes.
> > 
> > There should probably be a sanity check to never ask for INQUIRY less than
> > 36 bytes.  I thought there used to be such a thing....
> > 
> > Matt
> 
> Well we do ask for 36, but in Andries case only got back 5.
> 
> The zero fill is a good solution and matches what the device should do if it
> can't get all of the INQUIRY data.
> 
> There should be a warning output, in case other bad things happen, and so
> people know they have a borken device, and maybe a comment like:
> 
>         /*
>          * If the response length is less than 36 the rest of the INQUIRY
> 	 * is zero filled.
> 	 *
> 	 * etc.
>          */
> 
> > 
> > On Sat, Jan 04, 2003 at 01:21:11AM +0100, Andries.Brouwer@cwi.nl wrote:
> > > Got a new USB device and noticed some scsi silliness while playing with it.
> > > 
> > > A bug in scsi_scan.c is
> > > 
> > >         sdev->inquiry = kmalloc(sdev->inquiry_len, GFP_ATOMIC);
> > >         memcpy(sdev->inquiry, inq_result, sdev->inquiry_len);
> > >         sdev->vendor = (char *) (sdev->inquiry + 8);
> > >         sdev->model = (char *) (sdev->inquiry + 16);
> > >         sdev->rev = (char *) (sdev->inquiry + 32);
> > > 
> > > since it happens that inquiry_len is short (in my case it is 5)
> > > and the vendor/model/rev pointers are wild.
> > > Catting /proc/scsi/scsi now yields random garbage.
> 
> > > I made a patch but hesitated between a small patch and a larger one.
> > > Why do we have this malloced inquiry field? As far as I can see
> > > nobody uses it. And the comment in scsi_add_lun() advises us
> > > not to save the inquiry, precisely what we did until recently.
> > > So, should this change from 2.5.11 be reverted?
> 
> The INQUIRY was saved so that users could avoid sending a long INQUIRY to
> a borken device and hang it, and just send an ioctl to retrieve it, but no
> ioctl was ever added.
> 
> Rather than not saving it, we should just get the INQUIRY again (and
> implicitly overwrite vendor/model/rev) after spin up or if we get a unit
> attention with additional sense code of INQUIRY DATA HAS CHANGED; this
> would probably break somehow for some devices (changing the INQUIRY after
> a spin up is not out of spec), and is hard to handle with our current
> scanning (scsi_scan.c sends the INQUIRY, but sd.c has the spin up code),
> as per comments in scsi_probe_lun.
> 
> -- Patrick Mansfield
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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