Hi,
usb-storage.c oopses in fill_inquiry_response if I send an INQUIRY
to device which is currently disconnected from the USB bus.
This happens because fill_inquiry_response is called outside a
check for us->pusb_dev. Moving the special case into the if()
block, the oops is fixed.
(For reference, the oops is below the patch)
Jan
--- linux-2.4.12-ac3/drivers/usb/storage/usb.c.orig Mon Oct 1 12:15:29 2001
+++ linux-2.4.12-ac3/drivers/usb/storage/usb.c Wed Oct 17 00:33:22 2001
@@ -389,24 +389,6 @@
break;
}
- /* Handle those devices which need us to fake their
- * inquiry data */
- if ((us->srb->cmnd[0] == INQUIRY) &&
- (us->flags & US_FL_FIX_INQUIRY)) {
- unsigned char data_ptr[36] = {
- 0x00, 0x80, 0x02, 0x02,
- 0x1F, 0x00, 0x00, 0x00};
-
- US_DEBUGP("Faking INQUIRY command\n");
- fill_inquiry_response(us, data_ptr, 36);
- us->srb->result = GOOD << 1;
-
- set_current_state(TASK_INTERRUPTIBLE);
- us->srb->scsi_done(us->srb);
- us->srb = NULL;
- break;
- }
-
/* lock the device pointers */
down(&(us->dev_semaphore));
@@ -423,15 +405,30 @@
sizeof(usb_stor_sense_notready));
us->srb->result = GOOD << 1;
} else {
+ memset(us->srb->request_buffer, 0, us->srb->request_bufflen);
memcpy(us->srb->sense_buffer,
usb_stor_sense_notready,
sizeof(usb_stor_sense_notready));
us->srb->result = CHECK_CONDITION << 1;
}
} else { /* !us->pusb_dev */
- /* we've got a command, let's do it! */
- US_DEBUG(usb_stor_show_command(us->srb));
- us->proto_handler(us->srb, us);
+
+ /* Handle those devices which need us to fake
+ * their inquiry data */
+ if ((us->srb->cmnd[0] == INQUIRY) &&
+ (us->flags & US_FL_FIX_INQUIRY)) {
+ unsigned char data_ptr[36] = {
+ 0x00, 0x80, 0x02, 0x02,
+ 0x1F, 0x00, 0x00, 0x00};
+
+ US_DEBUGP("Faking INQUIRY command\n");
+ fill_inquiry_response(us, data_ptr, 36);
+ us->srb->result = GOOD << 1;
+ } else {
+ /* we've got a command, let's do it! */
+ US_DEBUG(usb_stor_show_command(us->srb));
+ us->proto_handler(us->srb, us);
+ }
}
/* unlock the device pointers */
Oct 16 21:07:28 sirith kernel: Oops: 0000
Oct 16 21:07:28 sirith kernel: CPU: 0
Oct 16 21:07:28 sirith kernel: EIP: 0010:[<e4951766>] Tainted: P
Oct 16 21:07:28 sirith kernel: EFLAGS: 00010246
Oct 16 21:07:28 sirith kernel: eax: 00000000 ebx: dc636600 ecx: 00000000 edx: 00000010
Oct 16 21:07:28 sirith kernel: esi: e495d460 edi: d9f09fcc ebp: e495d450 esp: d9f09f7c
Oct 16 21:07:28 sirith kernel: ds: 0018 es: 0018 ss: 0018
Oct 16 21:07:28 sirith kernel: Process usb-storage-0 (pid: 766, stackpage=d9f09000)
Oct 16 21:07:28 sirith kernel: Stack: d9f08000 e495da91 d9f09ff0 dc636600 c0116373 c02955a7 00000005 c01162c4
Oct 16 21:07:28 sirith kernel: d9f08000 e4951b44 dc636600 d9f09fcc 00000024 e495daa0 00000100 da003dcc
Oct 16 21:07:28 sirith kernel: dc636600 dc636600 dc636604 00000001 02028000 0000001f 69736143 0000006f
Oct 16 21:07:28 sirith kernel: Call Trace: [<e495da91>] [release_console_sem+115/128] [printk+260/272] [<e4951b44>] [<e495daa0>]
Oct 16 21:07:28 sirith kernel: Code: 0f b7 80 cc 00 00 00 66 c1 e8 0c 0c 30 88 47 20 8b 43 18 8a
>>EIP; e4951766 <[usb-storage]fill_inquiry_response+116/2f0> <=====
Trace; e495da90 <[usb-storage]__module_usb_device_size+670/81be>
Code; e4951766 <[usb-storage]fill_inquiry_response+116/2f0>
00000000 <_EIP>:
Code; e4951766 <[usb-storage]fill_inquiry_response+116/2f0> <=====
0: 0f b7 80 cc 00 00 00 movzwl 0xcc(%eax),%eax <=====
Code; e495176c <[usb-storage]fill_inquiry_response+11c/2f0>
7: 66 c1 e8 0c shr $0xc,%ax
Code; e4951770 <[usb-storage]fill_inquiry_response+120/2f0>
b: 0c 30 or $0x30,%al
Code; e4951772 <[usb-storage]fill_inquiry_response+122/2f0>
d: 88 47 20 mov %al,0x20(%edi)
Code; e4951776 <[usb-storage]fill_inquiry_response+126/2f0>
10: 8b 43 18 mov 0x18(%ebx),%eax
Code; e4951778 <[usb-storage]fill_inquiry_response+128/2f0>
13: 8a 00 mov (%eax),%al
Actually, this is a side-effect of another problem, which is that INQUIRY
is legal for a device at any time (at least, to SCSI). What we really need
to do is fake an INQURIY response for detached devices, separate from also
those devices which need a faked-inquiry all the time.
Matt
On Wed, Oct 17, 2001 at 12:58:22AM +0200, Jan Niehusmann wrote:
> Hi,
>
> usb-storage.c oopses in fill_inquiry_response if I send an INQUIRY
> to device which is currently disconnected from the USB bus.
>
> This happens because fill_inquiry_response is called outside a
> check for us->pusb_dev. Moving the special case into the if()
> block, the oops is fixed.
>
> (For reference, the oops is below the patch)
>
> Jan
>
> --- linux-2.4.12-ac3/drivers/usb/storage/usb.c.orig Mon Oct 1 12:15:29 2001
> +++ linux-2.4.12-ac3/drivers/usb/storage/usb.c Wed Oct 17 00:33:22 2001
> @@ -389,24 +389,6 @@
> break;
> }
>
> - /* Handle those devices which need us to fake their
> - * inquiry data */
> - if ((us->srb->cmnd[0] == INQUIRY) &&
> - (us->flags & US_FL_FIX_INQUIRY)) {
> - unsigned char data_ptr[36] = {
> - 0x00, 0x80, 0x02, 0x02,
> - 0x1F, 0x00, 0x00, 0x00};
> -
> - US_DEBUGP("Faking INQUIRY command\n");
> - fill_inquiry_response(us, data_ptr, 36);
> - us->srb->result = GOOD << 1;
> -
> - set_current_state(TASK_INTERRUPTIBLE);
> - us->srb->scsi_done(us->srb);
> - us->srb = NULL;
> - break;
> - }
> -
> /* lock the device pointers */
> down(&(us->dev_semaphore));
>
> @@ -423,15 +405,30 @@
> sizeof(usb_stor_sense_notready));
> us->srb->result = GOOD << 1;
> } else {
> + memset(us->srb->request_buffer, 0, us->srb->request_bufflen);
> memcpy(us->srb->sense_buffer,
> usb_stor_sense_notready,
> sizeof(usb_stor_sense_notready));
> us->srb->result = CHECK_CONDITION << 1;
> }
> } else { /* !us->pusb_dev */
> - /* we've got a command, let's do it! */
> - US_DEBUG(usb_stor_show_command(us->srb));
> - us->proto_handler(us->srb, us);
> +
> + /* Handle those devices which need us to fake
> + * their inquiry data */
> + if ((us->srb->cmnd[0] == INQUIRY) &&
> + (us->flags & US_FL_FIX_INQUIRY)) {
> + unsigned char data_ptr[36] = {
> + 0x00, 0x80, 0x02, 0x02,
> + 0x1F, 0x00, 0x00, 0x00};
> +
> + US_DEBUGP("Faking INQUIRY command\n");
> + fill_inquiry_response(us, data_ptr, 36);
> + us->srb->result = GOOD << 1;
> + } else {
> + /* we've got a command, let's do it! */
> + US_DEBUG(usb_stor_show_command(us->srb));
> + us->proto_handler(us->srb, us);
> + }
> }
>
> /* unlock the device pointers */
>
>
>
>
> Oct 16 21:07:28 sirith kernel: Oops: 0000
> Oct 16 21:07:28 sirith kernel: CPU: 0
> Oct 16 21:07:28 sirith kernel: EIP: 0010:[<e4951766>] Tainted: P
> Oct 16 21:07:28 sirith kernel: EFLAGS: 00010246
> Oct 16 21:07:28 sirith kernel: eax: 00000000 ebx: dc636600 ecx: 00000000 edx: 00000010
> Oct 16 21:07:28 sirith kernel: esi: e495d460 edi: d9f09fcc ebp: e495d450 esp: d9f09f7c
> Oct 16 21:07:28 sirith kernel: ds: 0018 es: 0018 ss: 0018
> Oct 16 21:07:28 sirith kernel: Process usb-storage-0 (pid: 766, stackpage=d9f09000)
> Oct 16 21:07:28 sirith kernel: Stack: d9f08000 e495da91 d9f09ff0 dc636600 c0116373 c02955a7 00000005 c01162c4
> Oct 16 21:07:28 sirith kernel: d9f08000 e4951b44 dc636600 d9f09fcc 00000024 e495daa0 00000100 da003dcc
> Oct 16 21:07:28 sirith kernel: dc636600 dc636600 dc636604 00000001 02028000 0000001f 69736143 0000006f
> Oct 16 21:07:28 sirith kernel: Call Trace: [<e495da91>] [release_console_sem+115/128] [printk+260/272] [<e4951b44>] [<e495daa0>]
> Oct 16 21:07:28 sirith kernel: Code: 0f b7 80 cc 00 00 00 66 c1 e8 0c 0c 30 88 47 20 8b 43 18 8a
>
> >>EIP; e4951766 <[usb-storage]fill_inquiry_response+116/2f0> <=====
> Trace; e495da90 <[usb-storage]__module_usb_device_size+670/81be>
> Code; e4951766 <[usb-storage]fill_inquiry_response+116/2f0>
> 00000000 <_EIP>:
> Code; e4951766 <[usb-storage]fill_inquiry_response+116/2f0> <=====
> 0: 0f b7 80 cc 00 00 00 movzwl 0xcc(%eax),%eax <=====
> Code; e495176c <[usb-storage]fill_inquiry_response+11c/2f0>
> 7: 66 c1 e8 0c shr $0xc,%ax
> Code; e4951770 <[usb-storage]fill_inquiry_response+120/2f0>
> b: 0c 30 or $0x30,%al
> Code; e4951772 <[usb-storage]fill_inquiry_response+122/2f0>
> d: 88 47 20 mov %al,0x20(%edi)
> Code; e4951776 <[usb-storage]fill_inquiry_response+126/2f0>
> 10: 8b 43 18 mov 0x18(%ebx),%eax
> Code; e4951778 <[usb-storage]fill_inquiry_response+128/2f0>
> 13: 8a 00 mov (%eax),%al
--
Matthew Dharm Home: [email protected]
Maintainer, Linux USB Mass Storage Driver
Sir, for the hundreth time, we do NOT carry 600-round boxes of belt-fed
suction darts!
-- Salesperson to Greg
User Friendly, 12/30/1997
On Tue, Oct 16, 2001 at 05:56:40PM -0700, Matthew Dharm wrote:
> Actually, this is a side-effect of another problem, which is that INQUIRY
> is legal for a device at any time (at least, to SCSI). What we really need
> to do is fake an INQURIY response for detached devices, separate from also
> those devices which need a faked-inquiry all the time.
Ok, then the fix could look like the following, I think. The INQUIRY
response in the disconnected case is a little bit different, as the
information from pusb_dev is not available, but the INQUIRY works and
the oops is fixed.
Jan
--- linux-2.4.12-ac3/drivers/usb/storage/usb.c.orig Mon Oct 1 12:15:29 2001
+++ linux-2.4.12-ac3/drivers/usb/storage/usb.c Wed Oct 17 03:04:32 2001
@@ -268,10 +268,12 @@
memcpy(data+16, us->unusual_dev->productName,
strlen(us->unusual_dev->productName) > 16 ? 16 :
strlen(us->unusual_dev->productName));
- data[32] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>12) & 0x0F);
- data[33] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>8) & 0x0F);
- data[34] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>4) & 0x0F);
- data[35] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice) & 0x0F);
+ if(us->pusb_dev) {
+ data[32] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>12) & 0x0F);
+ data[33] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>8) & 0x0F);
+ data[34] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>4) & 0x0F);
+ data[35] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice) & 0x0F);
+ }
if (us->srb->use_sg) {
sg = (struct scatterlist *)us->srb->request_buffer;
No, I think something different is best.
Your first change to push the US_FL_FIX_INQUIRY code down after the test
for pusb_dev is good.. but, in the case where that pointer is bad, we need
to cook up something totally fake, like INQUIRY data that says
"DISCONNECTED" "USB-STORAGE DEVICE" or somesuch.....
Matt
On Wed, Oct 17, 2001 at 03:11:13AM +0200, Jan Niehusmann wrote:
> On Tue, Oct 16, 2001 at 05:56:40PM -0700, Matthew Dharm wrote:
> > Actually, this is a side-effect of another problem, which is that INQUIRY
> > is legal for a device at any time (at least, to SCSI). What we really need
> > to do is fake an INQURIY response for detached devices, separate from also
> > those devices which need a faked-inquiry all the time.
>
> Ok, then the fix could look like the following, I think. The INQUIRY
> response in the disconnected case is a little bit different, as the
> information from pusb_dev is not available, but the INQUIRY works and
> the oops is fixed.
>
> Jan
>
>
> --- linux-2.4.12-ac3/drivers/usb/storage/usb.c.orig Mon Oct 1 12:15:29 2001
> +++ linux-2.4.12-ac3/drivers/usb/storage/usb.c Wed Oct 17 03:04:32 2001
> @@ -268,10 +268,12 @@
> memcpy(data+16, us->unusual_dev->productName,
> strlen(us->unusual_dev->productName) > 16 ? 16 :
> strlen(us->unusual_dev->productName));
> - data[32] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>12) & 0x0F);
> - data[33] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>8) & 0x0F);
> - data[34] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>4) & 0x0F);
> - data[35] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice) & 0x0F);
> + if(us->pusb_dev) {
> + data[32] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>12) & 0x0F);
> + data[33] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>8) & 0x0F);
> + data[34] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>4) & 0x0F);
> + data[35] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice) & 0x0F);
> + }
>
> if (us->srb->use_sg) {
> sg = (struct scatterlist *)us->srb->request_buffer;
>
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Matthew Dharm Home: [email protected]
Maintainer, Linux USB Mass Storage Driver
It's not that hard. No matter what the problem is, tell the customer
to reinstall Windows.
-- Nurse
User Friendly, 3/22/1998
Ahh... you've never used cdrecord -scanbus
Technically, as long as the system believes that the target exists (i.e.
you haven't unloaded your SCSI driver module), the target is required to
respond to an INQUIRY command. So, if you boot with the scanner on, and
then turn it off, you've got a problem.
What you say about behavior is correct, but it's just another symptom. If
you run cdrecord -scanbus with a disconnected USB device, it just gives you
garbage, which is bad also.
Matt
On Wed, Oct 17, 2001 at 03:44:10AM +0200, Jan Niehusmann wrote:
> On Tue, Oct 16, 2001 at 06:32:43PM -0700, Matthew Dharm wrote:
> > No, I think something different is best.
> >
> > Your first change to push the US_FL_FIX_INQUIRY code down after the test
> > for pusb_dev is good.. but, in the case where that pointer is bad, we need
> > to cook up something totally fake, like INQUIRY data that says
> > "DISCONNECTED" "USB-STORAGE DEVICE" or somesuch.....
>
> Then I don't understand why we have to answer the INQUIRY at all.
> This could as well be a disconnected / turned off external scsi
> device. I often turn off my external scsi scanner while the system
> is running, without a problem up to now. The scanner surely doesn't
> answer to INQUIRYs when it's turned off...
>
> By the way - a device not needing the US_FL_FIX_INQUIRY code wouldn't
> answer to INQUIRYs either, when it's disconnected. So the patch I sent
> first gives the same behaviour for devices with and without
> US_FL_FIX_INQUIRY set.
>
> Jan
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Matthew Dharm Home: [email protected]
Maintainer, Linux USB Mass Storage Driver
E: You run this ship with Windows?! YOU IDIOT!
L: Give me a break, it came bundled with the computer!
-- ESR and Lan Solaris
User Friendly, 12/8/1998
On Tue, Oct 16, 2001 at 11:24:52PM -0700, Matthew Dharm wrote:
> Technically, as long as the system believes that the target exists (i.e.
> you haven't unloaded your SCSI driver module), the target is required to
> respond to an INQUIRY command. So, if you boot with the scanner on, and
> then turn it off, you've got a problem.
Ok. I looked at the SCSI-2 standard and found a possibly sensible answer
for such an INQUIRY to a disconnected device.
First, there is a special peripheral qualifier for disconnected
physical devices:
001b The target is capable of supporting the specified peripheral
device type on this logical unit; however, the physical device
is not currently connected to this logical unit.
Second, the devices is not required to give a complete answer:
NOTES
65 The INQUIRY command is typically used by the initiator after a reset
or power-up condition to determine the device types for system
configuration. To minimize delays after a reset or power-up condition,
the standard INQUIRY data should be available without incurring any
media access delays. If the target does store some of the INQUIRY data
on the device, it may return zeros or ASCII spaces (20h) in those fields
until the data is available from the device.
While this permission to set some fields to zero is included in the
standard for other purposes, it makes clear that you must expect to
get incomplete answers from an INQUIRY command.
So, what about the following patch?
Jan
--- linux-2.4.12-ac3/drivers/usb/storage/usb.c.orig Mon Oct 1 12:15:29 2001
+++ linux-2.4.12-ac3/drivers/usb/storage/usb.c Wed Oct 17 12:33:40 2001
@@ -262,16 +262,28 @@
if (data_len<36) // You lose.
return;
- memcpy(data+8, us->unusual_dev->vendorName,
- strlen(us->unusual_dev->vendorName) > 8 ? 8 :
- strlen(us->unusual_dev->vendorName));
- memcpy(data+16, us->unusual_dev->productName,
- strlen(us->unusual_dev->productName) > 16 ? 16 :
- strlen(us->unusual_dev->productName));
- data[32] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>12) & 0x0F);
- data[33] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>8) & 0x0F);
- data[34] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>4) & 0x0F);
- data[35] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice) & 0x0F);
+ if(data[0]&0x20) { /* USB device currently not connected. Return
+ peripheral qualifier 001b ("...however, the
+ physical device is not currently connected
+ to this logical unit") and leave vendor and
+ product identification empty. ("If the target
+ does store some of the INQUIRY data on the
+ device, it may return zeros or ASCII spaces
+ (20h) in those fields until the data is
+ available from the device."). */
+ memset(data+8,0,28);
+ } else {
+ memcpy(data+8, us->unusual_dev->vendorName,
+ strlen(us->unusual_dev->vendorName) > 8 ? 8 :
+ strlen(us->unusual_dev->vendorName));
+ memcpy(data+16, us->unusual_dev->productName,
+ strlen(us->unusual_dev->productName) > 16 ? 16 :
+ strlen(us->unusual_dev->productName));
+ data[32] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>12) & 0x0F);
+ data[33] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>8) & 0x0F);
+ data[34] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>4) & 0x0F);
+ data[35] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice) & 0x0F);
+ }
if (us->srb->use_sg) {
sg = (struct scatterlist *)us->srb->request_buffer;
@@ -389,24 +401,6 @@
break;
}
- /* Handle those devices which need us to fake their
- * inquiry data */
- if ((us->srb->cmnd[0] == INQUIRY) &&
- (us->flags & US_FL_FIX_INQUIRY)) {
- unsigned char data_ptr[36] = {
- 0x00, 0x80, 0x02, 0x02,
- 0x1F, 0x00, 0x00, 0x00};
-
- US_DEBUGP("Faking INQUIRY command\n");
- fill_inquiry_response(us, data_ptr, 36);
- us->srb->result = GOOD << 1;
-
- set_current_state(TASK_INTERRUPTIBLE);
- us->srb->scsi_done(us->srb);
- us->srb = NULL;
- break;
- }
-
/* lock the device pointers */
down(&(us->dev_semaphore));
@@ -422,16 +416,38 @@
usb_stor_sense_notready,
sizeof(usb_stor_sense_notready));
us->srb->result = GOOD << 1;
+ } else if(us->srb->cmnd[0] == INQUIRY) {
+ unsigned char data_ptr[36] = {
+ 0x20, 0x80, 0x02, 0x02,
+ 0x1F, 0x00, 0x00, 0x00};
+ US_DEBUGP("Faking INQUIRY command for disconnected device\n");
+ fill_inquiry_response(us, data_ptr, 36);
+ us->srb->result = GOOD << 1;
} else {
+ memset(us->srb->request_buffer, 0, us->srb->request_bufflen);
memcpy(us->srb->sense_buffer,
usb_stor_sense_notready,
sizeof(usb_stor_sense_notready));
us->srb->result = CHECK_CONDITION << 1;
}
} else { /* !us->pusb_dev */
- /* we've got a command, let's do it! */
- US_DEBUG(usb_stor_show_command(us->srb));
- us->proto_handler(us->srb, us);
+
+ /* Handle those devices which need us to fake
+ * their inquiry data */
+ if ((us->srb->cmnd[0] == INQUIRY) &&
+ (us->flags & US_FL_FIX_INQUIRY)) {
+ unsigned char data_ptr[36] = {
+ 0x00, 0x80, 0x02, 0x02,
+ 0x1F, 0x00, 0x00, 0x00};
+
+ US_DEBUGP("Faking INQUIRY command\n");
+ fill_inquiry_response(us, data_ptr, 36);
+ us->srb->result = GOOD << 1;
+ } else {
+ /* we've got a command, let's do it! */
+ US_DEBUG(usb_stor_show_command(us->srb));
+ us->proto_handler(us->srb, us);
+ }
}
/* unlock the device pointers */
This looks better... but you have a dangerous memset() in an else clause --
the request buffer pointer could be pointing to a scatter-gather list, so
you can't just memset on it.
Matt
On Wed, Oct 17, 2001 at 12:42:49PM +0200, Jan Niehusmann wrote:
> On Tue, Oct 16, 2001 at 11:24:52PM -0700, Matthew Dharm wrote:
> > Technically, as long as the system believes that the target exists (i.e.
> > you haven't unloaded your SCSI driver module), the target is required to
> > respond to an INQUIRY command. So, if you boot with the scanner on, and
> > then turn it off, you've got a problem.
>
> Ok. I looked at the SCSI-2 standard and found a possibly sensible answer
> for such an INQUIRY to a disconnected device.
> First, there is a special peripheral qualifier for disconnected
> physical devices:
>
> 001b The target is capable of supporting the specified peripheral
> device type on this logical unit; however, the physical device
> is not currently connected to this logical unit.
>
> Second, the devices is not required to give a complete answer:
>
> NOTES
> 65 The INQUIRY command is typically used by the initiator after a reset
> or power-up condition to determine the device types for system
> configuration. To minimize delays after a reset or power-up condition,
> the standard INQUIRY data should be available without incurring any
> media access delays. If the target does store some of the INQUIRY data
> on the device, it may return zeros or ASCII spaces (20h) in those fields
> until the data is available from the device.
>
> While this permission to set some fields to zero is included in the
> standard for other purposes, it makes clear that you must expect to
> get incomplete answers from an INQUIRY command.
>
> So, what about the following patch?
>
> Jan
>
>
>
> --- linux-2.4.12-ac3/drivers/usb/storage/usb.c.orig Mon Oct 1 12:15:29 2001
> +++ linux-2.4.12-ac3/drivers/usb/storage/usb.c Wed Oct 17 12:33:40 2001
> @@ -262,16 +262,28 @@
> if (data_len<36) // You lose.
> return;
>
> - memcpy(data+8, us->unusual_dev->vendorName,
> - strlen(us->unusual_dev->vendorName) > 8 ? 8 :
> - strlen(us->unusual_dev->vendorName));
> - memcpy(data+16, us->unusual_dev->productName,
> - strlen(us->unusual_dev->productName) > 16 ? 16 :
> - strlen(us->unusual_dev->productName));
> - data[32] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>12) & 0x0F);
> - data[33] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>8) & 0x0F);
> - data[34] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>4) & 0x0F);
> - data[35] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice) & 0x0F);
> + if(data[0]&0x20) { /* USB device currently not connected. Return
> + peripheral qualifier 001b ("...however, the
> + physical device is not currently connected
> + to this logical unit") and leave vendor and
> + product identification empty. ("If the target
> + does store some of the INQUIRY data on the
> + device, it may return zeros or ASCII spaces
> + (20h) in those fields until the data is
> + available from the device."). */
> + memset(data+8,0,28);
> + } else {
> + memcpy(data+8, us->unusual_dev->vendorName,
> + strlen(us->unusual_dev->vendorName) > 8 ? 8 :
> + strlen(us->unusual_dev->vendorName));
> + memcpy(data+16, us->unusual_dev->productName,
> + strlen(us->unusual_dev->productName) > 16 ? 16 :
> + strlen(us->unusual_dev->productName));
> + data[32] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>12) & 0x0F);
> + data[33] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>8) & 0x0F);
> + data[34] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>4) & 0x0F);
> + data[35] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice) & 0x0F);
> + }
>
> if (us->srb->use_sg) {
> sg = (struct scatterlist *)us->srb->request_buffer;
> @@ -389,24 +401,6 @@
> break;
> }
>
> - /* Handle those devices which need us to fake their
> - * inquiry data */
> - if ((us->srb->cmnd[0] == INQUIRY) &&
> - (us->flags & US_FL_FIX_INQUIRY)) {
> - unsigned char data_ptr[36] = {
> - 0x00, 0x80, 0x02, 0x02,
> - 0x1F, 0x00, 0x00, 0x00};
> -
> - US_DEBUGP("Faking INQUIRY command\n");
> - fill_inquiry_response(us, data_ptr, 36);
> - us->srb->result = GOOD << 1;
> -
> - set_current_state(TASK_INTERRUPTIBLE);
> - us->srb->scsi_done(us->srb);
> - us->srb = NULL;
> - break;
> - }
> -
> /* lock the device pointers */
> down(&(us->dev_semaphore));
>
> @@ -422,16 +416,38 @@
> usb_stor_sense_notready,
> sizeof(usb_stor_sense_notready));
> us->srb->result = GOOD << 1;
> + } else if(us->srb->cmnd[0] == INQUIRY) {
> + unsigned char data_ptr[36] = {
> + 0x20, 0x80, 0x02, 0x02,
> + 0x1F, 0x00, 0x00, 0x00};
> + US_DEBUGP("Faking INQUIRY command for disconnected device\n");
> + fill_inquiry_response(us, data_ptr, 36);
> + us->srb->result = GOOD << 1;
> } else {
> + memset(us->srb->request_buffer, 0, us->srb->request_bufflen);
> memcpy(us->srb->sense_buffer,
> usb_stor_sense_notready,
> sizeof(usb_stor_sense_notready));
> us->srb->result = CHECK_CONDITION << 1;
> }
> } else { /* !us->pusb_dev */
> - /* we've got a command, let's do it! */
> - US_DEBUG(usb_stor_show_command(us->srb));
> - us->proto_handler(us->srb, us);
> +
> + /* Handle those devices which need us to fake
> + * their inquiry data */
> + if ((us->srb->cmnd[0] == INQUIRY) &&
> + (us->flags & US_FL_FIX_INQUIRY)) {
> + unsigned char data_ptr[36] = {
> + 0x00, 0x80, 0x02, 0x02,
> + 0x1F, 0x00, 0x00, 0x00};
> +
> + US_DEBUGP("Faking INQUIRY command\n");
> + fill_inquiry_response(us, data_ptr, 36);
> + us->srb->result = GOOD << 1;
> + } else {
> + /* we've got a command, let's do it! */
> + US_DEBUG(usb_stor_show_command(us->srb));
> + us->proto_handler(us->srb, us);
> + }
> }
>
> /* unlock the device pointers */
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Matthew Dharm Home: [email protected]
Maintainer, Linux USB Mass Storage Driver
Dudes! May the Open Source be with you.
-- Eric S. Raymond
User Friendly, 12/3/1998
On Wed, Oct 17, 2001 at 12:15:40PM -0700, Matthew Dharm wrote:
> This looks better... but you have a dangerous memset() in an else clause --
> the request buffer pointer could be pointing to a scatter-gather list, so
> you can't just memset on it.
Oh yes, I see - this memset is probably useless with the latest patch.
With an older version, it prevented the return of garbage from INQUIRY
in some cases, but with the later version INQUIRY is handeld specially
anyways.
Jan
On Wed, Oct 17, 2001 at 12:15:40PM -0700, Matthew Dharm wrote:
> This looks better... but you have a dangerous memset() in an else clause --
> the request buffer pointer could be pointing to a scatter-gather list, so
> you can't just memset on it.
Ok here is an updated patch.
--- linux-2.4.12-ac3/drivers/usb/storage/usb.c.orig Thu Oct 18 20:47:58 2001
+++ linux-2.4.12-ac3/drivers/usb/storage/usb.c Thu Oct 18 01:06:32 2001
@@ -262,16 +262,28 @@
if (data_len<36) // You lose.
return;
- memcpy(data+8, us->unusual_dev->vendorName,
- strlen(us->unusual_dev->vendorName) > 8 ? 8 :
- strlen(us->unusual_dev->vendorName));
- memcpy(data+16, us->unusual_dev->productName,
- strlen(us->unusual_dev->productName) > 16 ? 16 :
- strlen(us->unusual_dev->productName));
- data[32] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>12) & 0x0F);
- data[33] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>8) & 0x0F);
- data[34] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>4) & 0x0F);
- data[35] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice) & 0x0F);
+ if(data[0]&0x20) { /* USB device currently not connected. Return
+ peripheral qualifier 001b ("...however, the
+ physical device is not currently connected
+ to this logical unit") and leave vendor and
+ product identification empty. ("If the target
+ does store some of the INQUIRY data on the
+ device, it may return zeros or ASCII spaces
+ (20h) in those fields until the data is
+ available from the device."). */
+ memset(data+8,0,28);
+ } else {
+ memcpy(data+8, us->unusual_dev->vendorName,
+ strlen(us->unusual_dev->vendorName) > 8 ? 8 :
+ strlen(us->unusual_dev->vendorName));
+ memcpy(data+16, us->unusual_dev->productName,
+ strlen(us->unusual_dev->productName) > 16 ? 16 :
+ strlen(us->unusual_dev->productName));
+ data[32] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>12) & 0x0F);
+ data[33] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>8) & 0x0F);
+ data[34] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>4) & 0x0F);
+ data[35] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice) & 0x0F);
+ }
if (us->srb->use_sg) {
sg = (struct scatterlist *)us->srb->request_buffer;
@@ -389,24 +401,6 @@
break;
}
- /* Handle those devices which need us to fake their
- * inquiry data */
- if ((us->srb->cmnd[0] == INQUIRY) &&
- (us->flags & US_FL_FIX_INQUIRY)) {
- unsigned char data_ptr[36] = {
- 0x00, 0x80, 0x02, 0x02,
- 0x1F, 0x00, 0x00, 0x00};
-
- US_DEBUGP("Faking INQUIRY command\n");
- fill_inquiry_response(us, data_ptr, 36);
- us->srb->result = GOOD << 1;
-
- set_current_state(TASK_INTERRUPTIBLE);
- us->srb->scsi_done(us->srb);
- us->srb = NULL;
- break;
- }
-
/* lock the device pointers */
down(&(us->dev_semaphore));
@@ -422,6 +416,13 @@
usb_stor_sense_notready,
sizeof(usb_stor_sense_notready));
us->srb->result = GOOD << 1;
+ } else if(us->srb->cmnd[0] == INQUIRY) {
+ unsigned char data_ptr[36] = {
+ 0x20, 0x80, 0x02, 0x02,
+ 0x1F, 0x00, 0x00, 0x00};
+ US_DEBUGP("Faking INQUIRY command for disconnected device\n");
+ fill_inquiry_response(us, data_ptr, 36);
+ us->srb->result = GOOD << 1;
} else {
memcpy(us->srb->sense_buffer,
usb_stor_sense_notready,
@@ -429,9 +430,23 @@
us->srb->result = CHECK_CONDITION << 1;
}
} else { /* !us->pusb_dev */
- /* we've got a command, let's do it! */
- US_DEBUG(usb_stor_show_command(us->srb));
- us->proto_handler(us->srb, us);
+
+ /* Handle those devices which need us to fake
+ * their inquiry data */
+ if ((us->srb->cmnd[0] == INQUIRY) &&
+ (us->flags & US_FL_FIX_INQUIRY)) {
+ unsigned char data_ptr[36] = {
+ 0x00, 0x80, 0x02, 0x02,
+ 0x1F, 0x00, 0x00, 0x00};
+
+ US_DEBUGP("Faking INQUIRY command\n");
+ fill_inquiry_response(us, data_ptr, 36);
+ us->srb->result = GOOD << 1;
+ } else {
+ /* we've got a command, let's do it! */
+ US_DEBUG(usb_stor_show_command(us->srb));
+ us->proto_handler(us->srb, us);
+ }
}
/* unlock the device pointers */