----- "Alan Stern" <[email protected]> wrote:
> On Sun, 11 Oct 2009, Benjamin Herrenschmidt wrote:
>
> >
> > > Further REQUEST SENSE commands therefore requested 96 bytes of
> data
> > > instead of the standard 18 bytes. With LUN 0 this worked okay.
> But
> > > with LUN 1 it didn't; the device reported a failure of the REQUEST
>
> > > SENSE. This is what caused usb-storage to issue the device
> reset.
> > >
> > > After the reset usb-storage continued to ask for 96 bytes of
> sense
> > > data, and LUN 1 continued to fail the commands. Hence the
> repeated
> > > resets.
> >
> > Maybe a better approach would be to go back to 18 bytes when it
> fails,
> > what do you think ?
>
> We certainly could do that. But should we turn off the SANE_SENSE
> flag
> at the same time?
No I don't think its a good idea to turn off SANE_SENSE in this situation. Here is a patch similar to Ben Herrenschmidt's but will not turn off SANE_SENSE just because a transport failure.
Retry with short sense when SANE_SENSE fails.
Signed-off-by: Ben Efros <[email protected]>
--- linux-2.6.31.1/drivers/usb/storage/transport.c 2009-09-24 08:45:25.000000000 -0700
+++ linux-2.6.31.1.new/drivers/usb/storage/transport.c 2009-10-11 08:06:26.000000000 -0700
@@ -696,7 +696,7 @@ void usb_stor_invoke_transport(struct sc
/* device supports and needs bigger sense buffer */
if (us->fflags & US_FL_SANE_SENSE)
sense_size = ~0;
-
+Retry_Sense:
US_DEBUGP("Issuing auto-REQUEST_SENSE\n");
scsi_eh_prep_cmnd(srb, &ses, NULL, 0, sense_size);
@@ -723,6 +723,12 @@ void usb_stor_invoke_transport(struct sc
if (temp_result != USB_STOR_TRANSPORT_GOOD) {
US_DEBUGP("-- auto-sense failure\n");
+ if ((us->fflags & US_FL_SANE_SENSE) &&
+ sense_size != US_SENSE_SIZE) {
+ sense_size = US_SENSE_SIZE;
+ US_DEBUGP("-- retry without SANE_SENSE\n");
+ goto Retry_Sense;
+ }
/* we skip the reset if this happens to be a
* multi-target device, since failure of an
* auto-sense is perfectly valid
On Sun, 2009-10-11 at 09:16 -0700, Ben Efros wrote:
> No I don't think its a good idea to turn off SANE_SENSE in this situation.
> Here is a patch similar to Ben Herrenschmidt's but will not turn off SANE_SENSE
> just because a transport failure.
>
> Retry with short sense when SANE_SENSE fails.
Works for me too.
Alan, what do you think ? I definitely don't like Ben E's most recent
patch with a quirk for all devices, it's simply a lot more code for
something that will come back and bite again when somebody does the
same mistake again. I'd rather have the request sense code be more
robust. But this patch is fine, as was my previous one.
So it boils down on clearing SANE_SENSE vs. not clearing it. If we
clear it, we probably want to keep it cleared (via an INSANE_SENSE
flag ?). But on the other hand, I don't think that always going
for a retry when a SANE_SENSE fails is going to hurt and sounds
like the robust thing to do, so I don't mind that simple patch
from Ben. So up to you :-)
Cheers,
Ben.
> Signed-off-by: Ben Efros <[email protected]>
Tested-by: Benjamin Herrenschmidt <[email protected]>
> --- linux-2.6.31.1/drivers/usb/storage/transport.c 2009-09-24 08:45:25.000000000 -0700
> +++ linux-2.6.31.1.new/drivers/usb/storage/transport.c 2009-10-11 08:06:26.000000000 -0700
> @@ -696,7 +696,7 @@ void usb_stor_invoke_transport(struct sc
> /* device supports and needs bigger sense buffer */
> if (us->fflags & US_FL_SANE_SENSE)
> sense_size = ~0;
> -
> +Retry_Sense:
> US_DEBUGP("Issuing auto-REQUEST_SENSE\n");
>
> scsi_eh_prep_cmnd(srb, &ses, NULL, 0, sense_size);
> @@ -723,6 +723,12 @@ void usb_stor_invoke_transport(struct sc
> if (temp_result != USB_STOR_TRANSPORT_GOOD) {
> US_DEBUGP("-- auto-sense failure\n");
>
> + if ((us->fflags & US_FL_SANE_SENSE) &&
> + sense_size != US_SENSE_SIZE) {
> + sense_size = US_SENSE_SIZE;
> + US_DEBUGP("-- retry without SANE_SENSE\n");
> + goto Retry_Sense;
> + }
> /* we skip the reset if this happens to be a
> * multi-target device, since failure of an
> * auto-sense is perfectly valid
>
>
> --
> 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/
On Mon, 12 Oct 2009, Benjamin Herrenschmidt wrote:
> On Sun, 2009-10-11 at 09:16 -0700, Ben Efros wrote:
>
> > No I don't think its a good idea to turn off SANE_SENSE in this situation.
> > Here is a patch similar to Ben Herrenschmidt's but will not turn off SANE_SENSE
> > just because a transport failure.
> >
> > Retry with short sense when SANE_SENSE fails.
>
> Works for me too.
>
> Alan, what do you think ? I definitely don't like Ben E's most recent
> patch with a quirk for all devices, it's simply a lot more code for
> something that will come back and bite again when somebody does the
> same mistake again. I'd rather have the request sense code be more
> robust. But this patch is fine, as was my previous one.
I agree that it seems silly to have a flag _for_ SANE_SENSE and another
flag _against_ SANE_SENSE. Retrying seems easier and more robust.
> So it boils down on clearing SANE_SENSE vs. not clearing it. If we
> clear it, we probably want to keep it cleared (via an INSANE_SENSE
> flag ?). But on the other hand, I don't think that always going
> for a retry when a SANE_SENSE fails is going to hurt and sounds
> like the robust thing to do, so I don't mind that simple patch
> from Ben. So up to you :-)
I agree; it won't hurt much and only if the device is buggy to begin
with.
> Cheers,
> Ben.
>
> > Signed-off-by: Ben Efros <[email protected]>
>
> Tested-by: Benjamin Herrenschmidt <[email protected]>
>
> > --- linux-2.6.31.1/drivers/usb/storage/transport.c 2009-09-24 08:45:25.000000000 -0700
> > +++ linux-2.6.31.1.new/drivers/usb/storage/transport.c 2009-10-11 08:06:26.000000000 -0700
> > @@ -696,7 +696,7 @@ void usb_stor_invoke_transport(struct sc
> > /* device supports and needs bigger sense buffer */
> > if (us->fflags & US_FL_SANE_SENSE)
> > sense_size = ~0;
> > -
> > +Retry_Sense:
> > US_DEBUGP("Issuing auto-REQUEST_SENSE\n");
> >
> > scsi_eh_prep_cmnd(srb, &ses, NULL, 0, sense_size);
> > @@ -723,6 +723,12 @@ void usb_stor_invoke_transport(struct sc
> > if (temp_result != USB_STOR_TRANSPORT_GOOD) {
> > US_DEBUGP("-- auto-sense failure\n");
> >
> > + if ((us->fflags & US_FL_SANE_SENSE) &&
> > + sense_size != US_SENSE_SIZE) {
> > + sense_size = US_SENSE_SIZE;
> > + US_DEBUGP("-- retry without SANE_SENSE\n");
> > + goto Retry_Sense;
> > + }
Except that this is wrong. We can retry if temp_result ==
USB_STOR_TRANSPORT_FAILURE, but we should not retry if temp_result ==
USB_STOR_TRANSPORT_ERROR.
Alan Stern
P.S.: In case you don't already know... TRANSPORT_FAILURE means we
were able to communicate with the device, and it told us that it
couldn't carry out the command. TRANSPORT_ERROR means we weren't able
to communicate with the device.
On Mon, Oct 12, 2009 at 10:58:40AM -0400, Alan Stern wrote:
> On Mon, 12 Oct 2009, Benjamin Herrenschmidt wrote:
> >
> > Alan, what do you think ? I definitely don't like Ben E's most recent
> > patch with a quirk for all devices, it's simply a lot more code for
> > something that will come back and bite again when somebody does the
> > same mistake again. I'd rather have the request sense code be more
> > robust. But this patch is fine, as was my previous one.
>
> I agree that it seems silly to have a flag _for_ SANE_SENSE and another
> flag _against_ SANE_SENSE. Retrying seems easier and more robust.
Dualing flags, where one is auto-set and the other quirked, is almost
guaranteed to get us into a maintance nightmare.
> > So it boils down on clearing SANE_SENSE vs. not clearing it. If we
> > clear it, we probably want to keep it cleared (via an INSANE_SENSE
> > flag ?). But on the other hand, I don't think that always going
> > for a retry when a SANE_SENSE fails is going to hurt and sounds
> > like the robust thing to do, so I don't mind that simple patch
> > from Ben. So up to you :-)
>
> I agree; it won't hurt much and only if the device is buggy to begin
> with.
I agree; the extra retry is more robust, more straightforward, and more
maintainable long-term.
Matt
--
Matthew Dharm Home: [email protected]
Maintainer, Linux USB Mass Storage Driver
My mother not mind to die for stoppink Windows NT! She is rememberink
Stalin!
-- Pitr
User Friendly, 9/6/1998
----- "Alan Stern" <[email protected]> wrote:
> > > Signed-off-by: Ben Efros <[email protected]>
> >
> > Tested-by: Benjamin Herrenschmidt <[email protected]>
> >
> > > --- linux-2.6.31.1/drivers/usb/storage/transport.c
> 2009-09-24 08:45:25.000000000 -0700
> > > +++ linux-2.6.31.1.new/drivers/usb/storage/transport.c
> 2009-10-11 08:06:26.000000000 -0700
> > > @@ -696,7 +696,7 @@ void usb_stor_invoke_transport(struct sc
> > > /* device supports and needs bigger sense buffer
> */
> > > if (us->fflags & US_FL_SANE_SENSE)
> > > sense_size = ~0;
> > > -
> > > +Retry_Sense:
> > > US_DEBUGP("Issuing auto-REQUEST_SENSE\n");
> > >
> > > scsi_eh_prep_cmnd(srb, &ses, NULL, 0,
> sense_size);
> > > @@ -723,6 +723,12 @@ void usb_stor_invoke_transport(struct sc
> > > if (temp_result != USB_STOR_TRANSPORT_GOOD) {
> > > US_DEBUGP("-- auto-sense failure\n");
> > >
> > > + if ((us->fflags & US_FL_SANE_SENSE) &&
> > > + sense_size != US_SENSE_SIZE) {
> > > + sense_size = US_SENSE_SIZE;
> > > + US_DEBUGP("-- retry without
> SANE_SENSE\n");
> > > + goto Retry_Sense;
> > > + }
>
> Except that this is wrong. We can retry if temp_result ==
> USB_STOR_TRANSPORT_FAILURE, but we should not retry if temp_result ==
> USB_STOR_TRANSPORT_ERROR.
>
Agreed.
On Mon, 2009-10-12 at 10:58 -0400, Alan Stern wrote:
> Except that this is wrong. We can retry if temp_result ==
> USB_STOR_TRANSPORT_FAILURE, but we should not retry if temp_result ==
> USB_STOR_TRANSPORT_ERROR.
>
> Alan Stern
>
> P.S.: In case you don't already know... TRANSPORT_FAILURE means we
> were able to communicate with the device, and it told us that it
> couldn't carry out the command. TRANSPORT_ERROR means we weren't able
> to communicate with the device.
Ah good, I was wondering about precisely that (discriminating the errors
more intelligently). No point retrying if the device was yanked or went
totally dead.
Cheers,
Ben.
usb-storage: Workaround devices with bogus sense size
Some devices, such as Huawei E169, advertise more than the standard
amount of sense data, causing us to set US_FL_SANE_SENSE, assuming
they support it. However, they subsequently fail the request sense
with that size.
This works around it generically. When a sense request fails due to
a device returning an error, US_FL_SANE_SENSE was set, and that sense
request used a larger sense size, we retry with a smaller size before
giving up.
Based on an original patch by Ben Efros <[email protected]>
Signed-off-by: Benjamin Herrenschmidt <[email protected]>
---
This should also be applied to 2.6.31 stable
diff -urN linux-source-2.6.31-11-benh/drivers/usb/storage/transport.c linux-source-2.6.31-12-benh/drivers/usb/storage/transport.c
--- linux-source-2.6.31-11-benh/drivers/usb/storage/transport.c 2009-09-10 08:13:59.000000000 +1000
+++ linux-source-2.6.31-12-benh/drivers/usb/storage/transport.c 2009-10-13 15:44:57.117722653 +1100
@@ -696,7 +696,7 @@
/* device supports and needs bigger sense buffer */
if (us->fflags & US_FL_SANE_SENSE)
sense_size = ~0;
-
+Retry_Sense:
US_DEBUGP("Issuing auto-REQUEST_SENSE\n");
scsi_eh_prep_cmnd(srb, &ses, NULL, 0, sense_size);
@@ -720,6 +720,21 @@
srb->result = DID_ABORT << 16;
goto Handle_Errors;
}
+
+ /* Some devices claim to support larger sense but fail when
+ * trying to request it. When a transport failure happens
+ * using US_FS_SANE_SENSE, we always retry with a standard
+ * (small) sense request. This fixes some USB GSM modems
+ */
+ if (temp_result == USB_STOR_TRANSPORT_FAILED &&
+ (us->fflags & US_FL_SANE_SENSE) &&
+ sense_size != US_SENSE_SIZE) {
+ US_DEBUGP("-- auto-sense failure, retry small sense\n");
+ sense_size = US_SENSE_SIZE;
+ goto Retry_Sense;
+ }
+
+ /* Other failures */
if (temp_result != USB_STOR_TRANSPORT_GOOD) {
US_DEBUGP("-- auto-sense failure\n");
On Tue, 13 Oct 2009, Benjamin Herrenschmidt wrote:
> usb-storage: Workaround devices with bogus sense size
>
> Some devices, such as Huawei E169, advertise more than the standard
> amount of sense data, causing us to set US_FL_SANE_SENSE, assuming
> they support it. However, they subsequently fail the request sense
> with that size.
>
> This works around it generically. When a sense request fails due to
> a device returning an error, US_FL_SANE_SENSE was set, and that sense
> request used a larger sense size, we retry with a smaller size before
> giving up.
>
> Based on an original patch by Ben Efros <[email protected]>
>
> Signed-off-by: Benjamin Herrenschmidt <[email protected]>
Acked-by: Alan Stern <[email protected]>
On Tue, 2009-10-13 at 10:03 -0400, Alan Stern wrote:
> On Tue, 13 Oct 2009, Benjamin Herrenschmidt wrote:
>
> > usb-storage: Workaround devices with bogus sense size
> >
> > Some devices, such as Huawei E169, advertise more than the standard
> > amount of sense data, causing us to set US_FL_SANE_SENSE, assuming
> > they support it. However, they subsequently fail the request sense
> > with that size.
> >
> > This works around it generically. When a sense request fails due to
> > a device returning an error, US_FL_SANE_SENSE was set, and that sense
> > request used a larger sense size, we retry with a smaller size before
> > giving up.
> >
> > Based on an original patch by Ben Efros <[email protected]>
> >
> > Signed-off-by: Benjamin Herrenschmidt <[email protected]>
>
> Acked-by: Alan Stern <[email protected]>
Cool. Who's going to commit it ? Greg ? Or should I send it to Linus ?
Cheers,
Ben
On Wed, Oct 14, 2009 at 10:30:06AM +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2009-10-13 at 10:03 -0400, Alan Stern wrote:
> > On Tue, 13 Oct 2009, Benjamin Herrenschmidt wrote:
> >
> > > usb-storage: Workaround devices with bogus sense size
> > >
> > > Some devices, such as Huawei E169, advertise more than the standard
> > > amount of sense data, causing us to set US_FL_SANE_SENSE, assuming
> > > they support it. However, they subsequently fail the request sense
> > > with that size.
> > >
> > > This works around it generically. When a sense request fails due to
> > > a device returning an error, US_FL_SANE_SENSE was set, and that sense
> > > request used a larger sense size, we retry with a smaller size before
> > > giving up.
> > >
> > > Based on an original patch by Ben Efros <[email protected]>
> > >
> > > Signed-off-by: Benjamin Herrenschmidt <[email protected]>
> >
> > Acked-by: Alan Stern <[email protected]>
>
> Cool. Who's going to commit it ? Greg ? Or should I send it to Linus ?
I will, I'll send it off tomorrow.
thanks,
greg k-h