[PATCH] usb-storage: scsiglue: Changing the command result

[PATCH] usb-storage: scsiglue: Changing the command result

[Vishal Annapurve]

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

Hi,

There was a recent commit in mainline for the scsi devices which do not
respond properly to medium access command:

commit    18a4d0a22ed6c54b67af7718c305cd010f09ddf8

[SCSI] Handle disk devices which can not process medium access commands
We have experienced several devices which fail in a fashion we do not
currently handle gracefully in SCSI. After a failure these devices will
respond to the SCSI primary command set (INQUIRY, TEST UNIT READY, etc.)
but any command accessing the storage medium will time out.

I came across a USB drive which showed similar problem and what I see is
usb storage is still not able to cope with such devices properly.

The control flow downwards is like:
scsi_times_out --> Setting cmd->result as DID_TIME_OUT
scsi_error_handler
scsi_unjam_host
scsi_eh_abort_cmds
command_abort    (sets US_FLIDX_TIMED_OUT for us->dflags
                                   calls stop_transport,
                                   and waits for)    usb_stor_control_thread (which is waiting for
                                                                    transport call to return inside
                                                                    usb_stor_invoke_transport)
                                                                    both usb_stor_control_thread and
                                                                    usb_stor_invoke_transport
                                                     check for us->dflags timed_out bit and
                                                      set the result as DID_ABORT
                                                      and signal completion for command_abort
                                                      to complete
......
sd_eh_action
checks for cmd->result and finds out that it's DID_ABORT rather than
DID_TIME_OUT.

This patch updates the command result to be TIME_OUT explicitly before
returning from command_abort in scsiglue.c.

I would like to know if this patch can work out for such USB Storage
devices? What would be the better way to do the same?


Regards,
Vishal Annapurve




[-- Attachment #2: usb_storage_scsi_medium_access_timeout.patch --]
[-- Type: text/x-patch, Size: 1147 bytes --]

From: Vishal Annapurve <vannapurve@nvidia.com>
Subject: [PATCH] usb-storage: Changing the command result

This change updates the returned result to scsi layer to use
DID_TIMEOUT flag as a result status rather than DID_ABORT for
commands aborted due to timeout.

Signed-off-by: Vishal Annapurve <vannapurve@nvidia.com>
---
 drivers/usb/storage/scsiglue.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index a74e9d8..e7b532e 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -353,6 +353,15 @@ static int command_abort(struct scsi_cmnd *srb)
 
 	/* Wait for the aborted command to finish */
 	wait_for_completion(&us->notify);
+	/* Given that this bit would be only set in case of timedout
+	 * command, we can return with DID_TIME_OUT, as scsi layer needs
+	 * to use this result rather than DID_ABORT.
+	 * This is a WR to avoid removing DID_ABORT altogether
+	 * and before the final control goes to scsi layer change the
+	 * result to be timedout.
+	 */
+	us->srb->result = DID_TIME_OUT << 16;
+
 	return SUCCESS;
 }
 
-- 
1.8.4

Fwd: [PATCH] usb-storage: scsiglue: Changing the command result

[Vishal Annapurve]

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

Hi,

There was a recent commit in mainline for the scsi devices which do not
respond properly to medium access command:

commit    18a4d0a22ed6c54b67af7718c305cd010f09ddf8

[SCSI] Handle disk devices which can not process medium access commands
We have experienced several devices which fail in a fashion we do not
currently handle gracefully in SCSI. After a failure these devices will
respond to the SCSI primary command set (INQUIRY, TEST UNIT READY, etc.)
but any command accessing the storage medium will time out.

I came across a USB drive which showed similar problem and what I see is
usb storage is still not able to cope with such devices properly.

The control flow downwards is like:
scsi_times_out --> Setting cmd->result as DID_TIME_OUT
scsi_error_handler
scsi_unjam_host
scsi_eh_abort_cmds
command_abort    (sets US_FLIDX_TIMED_OUT for us->dflags
                                    calls stop_transport,
                                    and waits for)    usb_stor_control_thread (which is waiting for
                                                                     transport call to return inside
                                                                     usb_stor_invoke_transport)
                                                                     both usb_stor_control_thread and
                                                                     usb_stor_invoke_transport
                                                      check for us->dflags timed_out bit and
                                                       set the result as DID_ABORT
                                                       and signal completion for command_abort
                                                       to complete
......
sd_eh_action
checks for cmd->result and finds out that it's DID_ABORT rather than
DID_TIME_OUT.

This patch updates the command result to be TIME_OUT explicitly before
returning from command_abort in scsiglue.c.

I would like to know if this patch can work out for such USB Storage
devices? What would be the better way to do the same?


Regards,
Vishal Annapurve


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

[-- Attachment #2: usb_storage_scsi_medium_access_timeout.patch --]
[-- Type: text/x-patch, Size: 1148 bytes --]

From: Vishal Annapurve <vannapurve@nvidia.com>
Subject: [PATCH] usb-storage: Changing the command result

This change updates the returned result to scsi layer to use
DID_TIMEOUT flag as a result status rather than DID_ABORT for
commands aborted due to timeout.

Signed-off-by: Vishal Annapurve <vannapurve@nvidia.com>
---
 drivers/usb/storage/scsiglue.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index a74e9d8..e7b532e 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -353,6 +353,15 @@ static int command_abort(struct scsi_cmnd *srb)
 
 	/* Wait for the aborted command to finish */
 	wait_for_completion(&us->notify);
+	/* Given that this bit would be only set in case of timedout
+	 * command, we can return with DID_TIME_OUT, as scsi layer needs
+	 * to use this result rather than DID_ABORT.
+	 * This is a WR to avoid removing DID_ABORT altogether
+	 * and before the final control goes to scsi layer change the
+	 * result to be timedout.
+	 */
+	us->srb->result = DID_TIME_OUT << 16;
+
 	return SUCCESS;
 }
 
-- 
1.8.4

Re: [PATCH] usb-storage: scsiglue: Changing the command result

[Ming Lei]

On Thu, Sep 26, 2013 at 7:35 PM, Vishal Annapurve <vannapurve@nvidia.com> wrote:
> Hi,
>
> There was a recent commit in mainline for the scsi devices which do not
> respond properly to medium access command:
>
> commit    18a4d0a22ed6c54b67af7718c305cd010f09ddf8
>
> [SCSI] Handle disk devices which can not process medium access commands
> We have experienced several devices which fail in a fashion we do not
> currently handle gracefully in SCSI. After a failure these devices will
> respond to the SCSI primary command set (INQUIRY, TEST UNIT READY, etc.)
> but any command accessing the storage medium will time out.
>
> I came across a USB drive which showed similar problem and what I see is
> usb storage is still not able to cope with such devices properly.
>
> The control flow downwards is like:
> scsi_times_out --> Setting cmd->result as DID_TIME_OUT
> scsi_error_handler
> scsi_unjam_host
> scsi_eh_abort_cmds
> command_abort    (sets US_FLIDX_TIMED_OUT for us->dflags
>                                   calls stop_transport,
>                                   and waits for)    usb_stor_control_thread
> (which is waiting for
>                                                                    transport
> call to return inside
>
> usb_stor_invoke_transport)
>                                                                    both
> usb_stor_control_thread and
>
> usb_stor_invoke_transport
>                                                     check for us->dflags
> timed_out bit and
>                                                      set the result as
> DID_ABORT
>                                                      and signal completion
> for command_abort
>                                                      to complete
> ......
> sd_eh_action
> checks for cmd->result and finds out that it's DID_ABORT rather than
> DID_TIME_OUT.
>
> This patch updates the command result to be TIME_OUT explicitly before
> returning from command_abort in scsiglue.c.
>
> I would like to know if this patch can work out for such USB Storage
> devices? What would be the better way to do the same?

Looks your diagnose is correct, and patch should be doable, the
only side effect is that previous returned DID_ABORT in srb->result
is replaced with DID_TIME_OUT now.

Another way is to implement .eh_timed_out callback to return different
error for the two failure, but looks not a big deal.

Thanks,
-- 
Ming Lei

Re: [PATCH] usb-storage: scsiglue: Changing the command result

[Alan Stern]

On Tue, 15 Oct 2013, Ming Lei wrote:

> On Thu, Sep 26, 2013 at 7:35 PM, Vishal Annapurve <vannapurve@nvidia.com> wrote:
> > Hi,
> >
> > There was a recent commit in mainline for the scsi devices which do not
> > respond properly to medium access command:
> >
> > commit    18a4d0a22ed6c54b67af7718c305cd010f09ddf8
> >
> > [SCSI] Handle disk devices which can not process medium access commands
> > We have experienced several devices which fail in a fashion we do not
> > currently handle gracefully in SCSI. After a failure these devices will
> > respond to the SCSI primary command set (INQUIRY, TEST UNIT READY, etc.)
> > but any command accessing the storage medium will time out.
> >
> > I came across a USB drive which showed similar problem and what I see is
> > usb storage is still not able to cope with such devices properly.
> >
> > The control flow downwards is like:
> > scsi_times_out --> Setting cmd->result as DID_TIME_OUT
> > scsi_error_handler
> > scsi_unjam_host
> > scsi_eh_abort_cmds
> > command_abort    (sets US_FLIDX_TIMED_OUT for us->dflags
> >                                   calls stop_transport,
> >                                   and waits for)    usb_stor_control_thread
> > (which is waiting for
> >                                                                    transport
> > call to return inside
> >
> > usb_stor_invoke_transport)
> >                                                                    both
> > usb_stor_control_thread and
> >
> > usb_stor_invoke_transport
> >                                                     check for us->dflags
> > timed_out bit and
> >                                                      set the result as
> > DID_ABORT
> >                                                      and signal completion
> > for command_abort
> >                                                      to complete
> > ......
> > sd_eh_action
> > checks for cmd->result and finds out that it's DID_ABORT rather than
> > DID_TIME_OUT.
> >
> > This patch updates the command result to be TIME_OUT explicitly before
> > returning from command_abort in scsiglue.c.
> >
> > I would like to know if this patch can work out for such USB Storage
> > devices? What would be the better way to do the same?
> 
> Looks your diagnose is correct, and patch should be doable, the
> only side effect is that previous returned DID_ABORT in srb->result
> is replaced with DID_TIME_OUT now.
> 
> Another way is to implement .eh_timed_out callback to return different
> error for the two failure, but looks not a big deal.

Instead of overriding the result code, a better way to do this would
simply be to change the places where srb->result is currently set to
DID_ABORT << 16.  Just make it DID_TIME_OUT << 16 instead.

Alan Stern

RE: [PATCH] usb-storage: scsiglue: Changing the command result

[Vishal Annapurve]

Hi Alan,

This issue seems more related to the devices using SCSI protocol and the
changes otherwise will be at more places giving the same end result.

I think as the comment says over the command_abort function,
intentional result change should only happen in case of timeout.

Regards,
Vishal

-----Original Message-----
From: Alan Stern [mailto:stern@rowland.harvard.edu] 
Sent: Tuesday, October 15, 2013 5:55 PM
To: Ming Lei
Cc: Vishal Annapurve; Linux Kernel Mailing List; linux-usb
Subject: Re: [PATCH] usb-storage: scsiglue: Changing the command result

On Tue, 15 Oct 2013, Ming Lei wrote:

> On Thu, Sep 26, 2013 at 7:35 PM, Vishal Annapurve <vannapurve@nvidia.com> wrote:
> > Hi,
> >
> > There was a recent commit in mainline for the scsi devices which do 
> > not respond properly to medium access command:
> >
> > commit    18a4d0a22ed6c54b67af7718c305cd010f09ddf8
> >
> > [SCSI] Handle disk devices which can not process medium access 
> > commands We have experienced several devices which fail in a fashion 
> > we do not currently handle gracefully in SCSI. After a failure these 
> > devices will respond to the SCSI primary command set (INQUIRY, TEST 
> > UNIT READY, etc.) but any command accessing the storage medium will time out.
> >
> > I came across a USB drive which showed similar problem and what I 
> > see is usb storage is still not able to cope with such devices properly.
> >
> > The control flow downwards is like:
> > scsi_times_out --> Setting cmd->result as DID_TIME_OUT 
> > scsi_error_handler scsi_unjam_host scsi_eh_abort_cmds
> > command_abort    (sets US_FLIDX_TIMED_OUT for us->dflags
> >                                   calls stop_transport,
> >                                   and waits for)    usb_stor_control_thread
> > (which is waiting for
> >                                                                    
> > transport call to return inside
> >
> > usb_stor_invoke_transport)
> >                                                                    
> > both usb_stor_control_thread and
> >
> > usb_stor_invoke_transport
> >                                                     check for 
> > us->dflags timed_out bit and
> >                                                      set the result 
> > as DID_ABORT
> >                                                      and signal 
> > completion for command_abort
> >                                                      to complete 
> > ......
> > sd_eh_action
> > checks for cmd->result and finds out that it's DID_ABORT rather than 
> > DID_TIME_OUT.
> >
> > This patch updates the command result to be TIME_OUT explicitly 
> > before returning from command_abort in scsiglue.c.
> >
> > I would like to know if this patch can work out for such USB Storage 
> > devices? What would be the better way to do the same?
> 
> Looks your diagnose is correct, and patch should be doable, the only 
> side effect is that previous returned DID_ABORT in srb->result is 
> replaced with DID_TIME_OUT now.
> 
> Another way is to implement .eh_timed_out callback to return different 
> error for the two failure, but looks not a big deal.

Instead of overriding the result code, a better way to do this would simply be to change the places where srb->result is currently set to DID_ABORT << 16.  Just make it DID_TIME_OUT << 16 instead.

Alan Stern

RE: [PATCH] usb-storage: scsiglue: Changing the command result

[Alan Stern]

On Tue, 15 Oct 2013, Vishal Annapurve wrote:

> Hi Alan,
> 
> This issue seems more related to the devices using SCSI protocol and the
> changes otherwise will be at more places giving the same end result.
> 
> I think as the comment says over the command_abort function,
> intentional result change should only happen in case of timeout.

usb-storage doesn't know _why_ a command was aborted; it knows only 
that the abort occurred.

If you look carefully at the code, you'll see that the result is set to 
DID_ABORT only when the US_FLIDX_TIMED_OUT bit is set, and this bit 
gets set only when a SCSI abort occurs.

Alan Stern

RE: [PATCH] usb-storage: scsiglue: Changing the command result

[Vishal Annapurve]

Hi Alan,

USB storage maybe just has to say that the abort occurred. By setting the
US_FLIDX_TIMED_OUT bit USB storage is getting signaled that the reason was
time out and the command is being aborted.
 
Now, it's arguable whether to change the implication of US_FLIDX_TIMED_OUT
bit for scsi - USB storage bridge or for entire usb storage Or maybe scsi has
decided to abort so it should override the result.

Regards,
Vishal

-----Original Message-----
From: Alan Stern [mailto:stern@rowland.harvard.edu] 
Sent: Tuesday, October 15, 2013 7:03 PM
To: Vishal Annapurve
Cc: Ming Lei; Linux Kernel Mailing List; linux-usb
Subject: RE: [PATCH] usb-storage: scsiglue: Changing the command result

On Tue, 15 Oct 2013, Vishal Annapurve wrote:

> Hi Alan,
> 
> This issue seems more related to the devices using SCSI protocol and 
> the changes otherwise will be at more places giving the same end result.
> 
> I think as the comment says over the command_abort function, 
> intentional result change should only happen in case of timeout.

usb-storage doesn't know _why_ a command was aborted; it knows only that the abort occurred.

If you look carefully at the code, you'll see that the result is set to DID_ABORT only when the US_FLIDX_TIMED_OUT bit is set, and this bit gets set only when a SCSI abort occurs.

Alan Stern

RE: [PATCH] usb-storage: scsiglue: Changing the command result

[Alan Stern]

On Tue, 15 Oct 2013, Vishal Annapurve wrote:

> Hi Alan,
> 
> USB storage maybe just has to say that the abort occurred. By setting the
> US_FLIDX_TIMED_OUT bit USB storage is getting signaled that the reason was
> time out and the command is being aborted.

No.  By setting the US_FLIDX_TIMED_OUT bit, usb-storage indicates that
the command was aborted.  This doesn't indicate anything about the
reason for the abort.  (Maybe this bit's name wasn't chosen very well.)

> Now, it's arguable whether to change the implication of US_FLIDX_TIMED_OUT
> bit for scsi - USB storage bridge or for entire usb storage

I don't understand this.  What's the difference between "scsi - USB 
storage bridge" and "entire usb storage"?  Aren't they the same thing?

>  Or maybe scsi has
> decided to abort so it should override the result.

Of course the SCSI midlayer has decided to abort.  That's the only way 
this bit can get set.  But usb-storage doesn't know why SCSI decided to 
abort.

Alan Stern

Re: [PATCH] usb-storage: scsiglue: Changing the command result

[Ming Lei]

On Wed, Oct 16, 2013 at 4:22 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 15 Oct 2013, Vishal Annapurve wrote:
>
>> Hi Alan,
>>
>> USB storage maybe just has to say that the abort occurred. By setting the
>> US_FLIDX_TIMED_OUT bit USB storage is getting signaled that the reason was
>> time out and the command is being aborted.
>
> No.  By setting the US_FLIDX_TIMED_OUT bit, usb-storage indicates that
> the command was aborted.  This doesn't indicate anything about the
> reason for the abort.  (Maybe this bit's name wasn't chosen very well.)
>
>> Now, it's arguable whether to change the implication of US_FLIDX_TIMED_OUT
>> bit for scsi - USB storage bridge or for entire usb storage
>
> I don't understand this.  What's the difference between "scsi - USB
> storage bridge" and "entire usb storage"?  Aren't they the same thing?
>
>>  Or maybe scsi has
>> decided to abort so it should override the result.
>
> Of course the SCSI midlayer has decided to abort.  That's the only way
> this bit can get set.  But usb-storage doesn't know why SCSI decided to
> abort.

usb-storage may know if it is caused by timeout via .eh_timed_out callback
if it wants to know.


Thanks,
-- 
Ming Lei

Re: [PATCH] usb-storage: scsiglue: Changing the command result

[Alan Stern]

On Wed, 16 Oct 2013, Ming Lei wrote:

> > Of course the SCSI midlayer has decided to abort.  That's the only way
> > this bit can get set.  But usb-storage doesn't know why SCSI decided to
> > abort.
> 
> usb-storage may know if it is caused by timeout via .eh_timed_out callback
> if it wants to know.

Ah, good point.

Vishal, since the only way an abort can occur is when the 
.eh_timeout_out callback runs, I think it makes sense to set the result 
code to DID_TIME_OUT instead of DID_ABORT in all cases.

Alan Stern

RE: [PATCH] usb-storage: scsiglue: Changing the command result

[Vishal Annapurve]

Hi Alan,

What I wanted to say was If the bit US_FLIDX_TIMED_OUT can have more
meanings than timed out then maybe it would be best to override the
results after usb-storage is done with the command maybe in scsi layer
itself who aborted it in the first place.

My concern was that overriding the result in usb storage or scsi layers
will have more side effects than doing it in scsiglue.c.
And by scsi-usb storage bridge what I meant was specifically the code in 
scsiglue.

Question about your last mail, do you want to change all the occurrences of
DID_ABORT from usb-storage to DID_TIMEOUT?

Regards,
Vishal

-----Original Message-----
From: Alan Stern [mailto:stern@rowland.harvard.edu] 
Sent: Tuesday, October 15, 2013 10:22 PM
To: Vishal Annapurve
Cc: Ming Lei; Linux Kernel Mailing List; linux-usb
Subject: RE: [PATCH] usb-storage: scsiglue: Changing the command result

On Tue, 15 Oct 2013, Vishal Annapurve wrote:

> Hi Alan,
> 
> USB storage maybe just has to say that the abort occurred. By setting 
> the US_FLIDX_TIMED_OUT bit USB storage is getting signaled that the 
> reason was time out and the command is being aborted.

No.  By setting the US_FLIDX_TIMED_OUT bit, usb-storage indicates that the command was aborted.  This doesn't indicate anything about the reason for the abort.  (Maybe this bit's name wasn't chosen very well.)

> Now, it's arguable whether to change the implication of 
> US_FLIDX_TIMED_OUT bit for scsi - USB storage bridge or for entire usb 
> storage

I don't understand this.  What's the difference between "scsi - USB storage bridge" and "entire usb storage"?  Aren't they the same thing?

>  Or maybe scsi has
> decided to abort so it should override the result.

Of course the SCSI midlayer has decided to abort.  That's the only way this bit can get set.  But usb-storage doesn't know why SCSI decided to abort.

Alan Stern

RE: [PATCH] usb-storage: scsiglue: Changing the command result

[Alan Stern]

On Fri, 18 Oct 2013, Vishal Annapurve wrote:

> Hi Alan,
> 
> What I wanted to say was If the bit US_FLIDX_TIMED_OUT can have more
> meanings than timed out then maybe it would be best to override the
> results after usb-storage is done with the command maybe in scsi layer
> itself who aborted it in the first place.

US_FLIDX_TIMED_OUT has one very specific meaning: command_abort() was
called.

Since command_abort() is the .eh_abort_handler routine, 
US_FLIDX_TIMED_OUT means that the SCSI layer decided to abort the 
command.

Does the SCSI layer ever abort a command for any reason other than a
timeout?  If not, you may conclude that US_FLIDX_TIMED_OUT indicates 
a timeout.  But if it does, you should not make this conclusion.

> My concern was that overriding the result in usb storage or scsi layers
> will have more side effects than doing it in scsiglue.c.
> And by scsi-usb storage bridge what I meant was specifically the code in 
> scsiglue.
> 
> Question about your last mail, do you want to change all the occurrences of
> DID_ABORT from usb-storage to DID_TIMEOUT?

Put it this way: There's no good reason for changing some of them but 
not all of them.

And if you're going to change them at all, it makes no sense to first 
set the result to DID_ABORT and then change it to DID_TIMEOUT.  You 
should simply set it to DID_TIMEOUT in the first place.

Alan Stern

RE: [PATCH] usb-storage: scsiglue: Changing the command result

[Vishal Annapurve]

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

Hi,

Attaching the new patch which will replace all the occurrences of DID_ABORT
with DID_TIMOUT in USB Storage whenever it sees US_FLIDX_TIMED_OUT bit
is set.
 
Regards,
Vishal

-----Original Message-----
From: Alan Stern [mailto:stern@rowland.harvard.edu] 
Sent: Friday, October 18, 2013 4:10 PM
To: Vishal Annapurve
Cc: Ming Lei; Linux Kernel Mailing List; linux-usb
Subject: RE: [PATCH] usb-storage: scsiglue: Changing the command result

On Fri, 18 Oct 2013, Vishal Annapurve wrote:

> Hi Alan,
> 
> What I wanted to say was If the bit US_FLIDX_TIMED_OUT can have more 
> meanings than timed out then maybe it would be best to override the 
> results after usb-storage is done with the command maybe in scsi layer 
> itself who aborted it in the first place.

US_FLIDX_TIMED_OUT has one very specific meaning: command_abort() was called.

Since command_abort() is the .eh_abort_handler routine, US_FLIDX_TIMED_OUT means that the SCSI layer decided to abort the command.

Does the SCSI layer ever abort a command for any reason other than a timeout?  If not, you may conclude that US_FLIDX_TIMED_OUT indicates a timeout.  But if it does, you should not make this conclusion.

> My concern was that overriding the result in usb storage or scsi 
> layers will have more side effects than doing it in scsiglue.c.
> And by scsi-usb storage bridge what I meant was specifically the code 
> in scsiglue.
> 
> Question about your last mail, do you want to change all the 
> occurrences of DID_ABORT from usb-storage to DID_TIMEOUT?

Put it this way: There's no good reason for changing some of them but not all of them.

And if you're going to change them at all, it makes no sense to first set the result to DID_ABORT and then change it to DID_TIMEOUT.  You should simply set it to DID_TIMEOUT in the first place.

Alan Stern


[-- Attachment #2: usb_storage_change_abort_to_timeout.patch --]
[-- Type: application/octet-stream, Size: 2754 bytes --]

From: Vishal Annapurve <vannapurve@nvidia.com>
Subject: [PATCH] usb: storage: Proper cmd result assignment

This change replaces DID_ABORT with DID_TIMEOUT from
usb storage assuming that the command will get aborted
only in case of timeout.

Signed-off-by: Vishal Annapurve <vannapurve@nvidia.com>
---
 drivers/usb/storage/transport.c | 8 ++++----
 drivers/usb/storage/usb.c       | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
index b2697dc..f68e4c6 100644
--- a/drivers/usb/storage/transport.c
+++ b/drivers/usb/storage/transport.c
@@ -608,8 +608,8 @@ void usb_stor_invoke_transport(struct scsi_cmnd *srb, struct us_data *us)
 	 * short-circuit all other processing
 	 */
 	if (test_bit(US_FLIDX_TIMED_OUT, &us->dflags)) {
-		US_DEBUGP("-- command was aborted\n");
-		srb->result = DID_ABORT << 16;
+		US_DEBUGP("-- command was aborted because of timeout\n");
+		srb->result = DID_TIME_OUT << 16;
 		goto Handle_Errors;
 	}
 
@@ -721,8 +721,8 @@ Retry_Sense:
 		scsi_eh_restore_cmnd(srb, &ses);
 
 		if (test_bit(US_FLIDX_TIMED_OUT, &us->dflags)) {
-			US_DEBUGP("-- auto-sense aborted\n");
-			srb->result = DID_ABORT << 16;
+			US_DEBUGP("-- auto-sense aborted due to timeout\n");
+			srb->result = DID_TIME_OUT << 16;
 
 			/* If SANE_SENSE caused this problem, disable it */
 			if (sense_size != US_SENSE_SIZE) {
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index c0d0915..263123b 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -290,7 +290,7 @@ static int usb_stor_control_thread(void * __us)
 
 		/* has the command timed out *already* ? */
 		if (test_bit(US_FLIDX_TIMED_OUT, &us->dflags)) {
-			us->srb->result = DID_ABORT << 16;
+			us->srb->result = DID_TIME_OUT << 16;
 			goto SkipForAbort;
 		}
 
@@ -344,18 +344,18 @@ static int usb_stor_control_thread(void * __us)
 		scsi_lock(host);
 
 		/* indicate that the command is done */
-		if (us->srb->result != DID_ABORT << 16) {
+		if (us->srb->result != DID_TIME_OUT << 16) {
 			US_DEBUGP("scsi cmd done, result=0x%x\n", 
 				   us->srb->result);
 			us->srb->scsi_done(us->srb);
 		} else {
 SkipForAbort:
-			US_DEBUGP("scsi command aborted\n");
+			US_DEBUGP("scsi command aborted due to timeout\n");
 		}
 
 		/* If an abort request was received we need to signal that
 		 * the abort has finished.  The proper test for this is
-		 * the TIMED_OUT flag, not srb->result == DID_ABORT, because
+		 * the TIMED_OUT flag, not srb->result == DID_TIME_OUT, because
 		 * the timeout might have occurred after the command had
 		 * already completed with a different result code. */
 		if (test_bit(US_FLIDX_TIMED_OUT, &us->dflags)) {
-- 
1.8.4

RE: [PATCH] usb-storage: scsiglue: Changing the command result

[Alan Stern]

On Sat, 19 Oct 2013, Vishal Annapurve wrote:

> Hi,
> 
> Attaching the new patch which will replace all the occurrences of DID_ABORT
> with DID_TIMOUT in USB Storage whenever it sees US_FLIDX_TIMED_OUT bit
> is set.

It seems okay, but you forgot to update the isd200.c and
cypress_atacb.c files.

Also, in the future please put your patches inline with the rest of the 
email, not as a separate attachment.

Alan Stern

RE: [PATCH] usb-storage: scsiglue: Changing the command result

[Vishal Annapurve]

Hi Alan,

Here is the new patch:

From: Vishal Annapurve <vannapurve@nvidia.com>
Date: Sat, 26 Oct 2013 21:10:11 +0530
Subject: [PATCH] usb: storage: Proper cmd result assignment

This change replaces DID_ABORT with DID_TIMEOUT as a command result
whenever US_FLIDX_TIMED_OUT bit is set.

This change is made to bring USB storage inline with a recent change:

commit    18a4d0a22ed6c54b67af7718c305cd010f09ddf8

[SCSI] Handle disk devices which can not process medium access commands
We have experienced several devices which fail in a fashion we do not
currently handle gracefully in SCSI. After a failure these devices will
respond to the SCSI primary command set (INQUIRY, TEST UNIT READY, etc.)
but any command accessing the storage medium will time out.

As the USB storage was setting command result as aborted rather than
timed out, SCSI layer was not recognizing the above mentioned failure
pattern.

Change-Id: Ic58e2247fed11649f4dbea56382354ba2fe0be1b
---
 drivers/staging/keucr/transport.c   | 6 +++---
 drivers/staging/keucr/usb.c         | 5 +++--
 drivers/usb/storage/cypress_atacb.c | 1 +
 drivers/usb/storage/isd200.c        | 2 +-
 drivers/usb/storage/transport.c     | 8 ++++----
 drivers/usb/storage/usb.c           | 9 +++++----
 6 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/keucr/transport.c b/drivers/staging/keucr/transport.c
index 1a8837d..ac0e3c2 100644
--- a/drivers/staging/keucr/transport.c
+++ b/drivers/staging/keucr/transport.c
@@ -312,7 +312,7 @@ void usb_stor_invoke_transport(struct scsi_cmnd *srb, struct us_data *us)
 		we need to short-circuit all other processing */
 	if (test_bit(US_FLIDX_TIMED_OUT, &us->dflags)) {
 		/* pr_info("-- command was aborted\n"); */
-		srb->result = DID_ABORT << 16;
+		srb->result = DID_TIME_OUT << 16;
 		goto Handle_Errors;
 	}
 
@@ -371,7 +371,7 @@ void usb_stor_invoke_transport(struct scsi_cmnd *srb, struct us_data *us)
 
 		if (test_bit(US_FLIDX_TIMED_OUT, &us->dflags)) {
 			/* pr_info("-- auto-sense aborted\n"); */
-			srb->result = DID_ABORT << 16;
+			srb->result = DID_TIME_OUT << 16;
 			goto Handle_Errors;
 		}
 		if (temp_result != USB_STOR_TRANSPORT_GOOD) {
@@ -447,7 +447,7 @@ void ENE_stor_invoke_transport(struct scsi_cmnd *srb, struct us_data *us)
 		we need to short-circuit all other processing */
 	if (test_bit(US_FLIDX_TIMED_OUT, &us->dflags)) {
 		/* pr_info("-- command was aborted\n"); */
-		srb->result = DID_ABORT << 16;
+		srb->result = DID_TIME_OUT << 16;
 		goto Handle_Errors;
 	}
 
diff --git a/drivers/staging/keucr/usb.c b/drivers/staging/keucr/usb.c
index 66aad3a..79405be 100644
--- a/drivers/staging/keucr/usb.c
+++ b/drivers/staging/keucr/usb.c
@@ -200,7 +200,7 @@ static int usb_stor_control_thread(void * __us)
 		/* has the command timed out *already* ? */
 		if (test_bit(US_FLIDX_TIMED_OUT, &us->dflags))
 		{
-			us->srb->result = DID_ABORT << 16;
+			us->srb->result = DID_TIME_OUT << 16;
 			goto SkipForAbort;
 		}
 
@@ -234,7 +234,8 @@ static int usb_stor_control_thread(void * __us)
 		scsi_lock(host);
 
 		/* indicate that the command is done */
-		if (us->srb->result != DID_ABORT << 16)
+		if ((us->srb->result != DID_ABORT << 16) &&
+			(us->srb->result != DID_TIME_OUT << 16))
 		{
 			us->srb->scsi_done(us->srb);
 		}
diff --git a/drivers/usb/storage/cypress_atacb.c b/drivers/usb/storage/cypress_atacb.c
index c844718..af24894 100644
--- a/drivers/usb/storage/cypress_atacb.c
+++ b/drivers/usb/storage/cypress_atacb.c
@@ -168,6 +168,7 @@ static void cypress_atacb_passthrough(struct scsi_cmnd *srb, struct us_data *us)
 	 */
 	if ((srb->result != (DID_ERROR << 16) &&
 				srb->result != (DID_ABORT << 16)) &&
+				srb->result != (DID_TIME_OUT << 16) &&
 			save_cmnd[2] & 0x20) {
 		struct scsi_eh_save ses;
 		unsigned char regs[8];
diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
index ffc4193..28b688b 100644
--- a/drivers/usb/storage/isd200.c
+++ b/drivers/usb/storage/isd200.c
@@ -705,7 +705,7 @@ static void isd200_invoke_transport( struct us_data *us,
 	/* abort processing: the bulk-only transport requires a reset
 	 * following an abort */
 	Handle_Abort:
-	srb->result = DID_ABORT << 16;
+	srb->result = DID_TIME_OUT << 16;
 
 	/* permit the reset transfer to take place */
 	clear_bit(US_FLIDX_ABORTING, &us->dflags);
diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
index b2697dc..f68e4c6 100644
--- a/drivers/usb/storage/transport.c
+++ b/drivers/usb/storage/transport.c
@@ -608,8 +608,8 @@ void usb_stor_invoke_transport(struct scsi_cmnd *srb, struct us_data *us)
 	 * short-circuit all other processing
 	 */
 	if (test_bit(US_FLIDX_TIMED_OUT, &us->dflags)) {
-		US_DEBUGP("-- command was aborted\n");
-		srb->result = DID_ABORT << 16;
+		US_DEBUGP("-- command was aborted because of timeout\n");
+		srb->result = DID_TIME_OUT << 16;
 		goto Handle_Errors;
 	}
 
@@ -721,8 +721,8 @@ Retry_Sense:
 		scsi_eh_restore_cmnd(srb, &ses);
 
 		if (test_bit(US_FLIDX_TIMED_OUT, &us->dflags)) {
-			US_DEBUGP("-- auto-sense aborted\n");
-			srb->result = DID_ABORT << 16;
+			US_DEBUGP("-- auto-sense aborted due to timeout\n");
+			srb->result = DID_TIME_OUT << 16;
 
 			/* If SANE_SENSE caused this problem, disable it */
 			if (sense_size != US_SENSE_SIZE) {
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index c0d0915..64eaca6 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -290,7 +290,7 @@ static int usb_stor_control_thread(void * __us)
 
 		/* has the command timed out *already* ? */
 		if (test_bit(US_FLIDX_TIMED_OUT, &us->dflags)) {
-			us->srb->result = DID_ABORT << 16;
+			us->srb->result = DID_TIME_OUT << 16;
 			goto SkipForAbort;
 		}
 
@@ -344,18 +344,19 @@ static int usb_stor_control_thread(void * __us)
 		scsi_lock(host);
 
 		/* indicate that the command is done */
-		if (us->srb->result != DID_ABORT << 16) {
+		if ((us->srb->result != (DID_TIME_OUT << 16)) &&
+			(us->srb->result != (DID_ABORT << 16)))  {
 			US_DEBUGP("scsi cmd done, result=0x%x\n", 
 				   us->srb->result);
 			us->srb->scsi_done(us->srb);
 		} else {
 SkipForAbort:
-			US_DEBUGP("scsi command aborted\n");
+			US_DEBUGP("scsi command aborted due to timeout\n");
 		}
 
 		/* If an abort request was received we need to signal that
 		 * the abort has finished.  The proper test for this is
-		 * the TIMED_OUT flag, not srb->result == DID_ABORT, because
+		 * the TIMED_OUT flag, not srb->result == DID_TIME_OUT, because
 		 * the timeout might have occurred after the command had
 		 * already completed with a different result code. */
 		if (test_bit(US_FLIDX_TIMED_OUT, &us->dflags)) {
-- 
1.8.4

Regards,
Vishal

-----Original Message-----
From: Alan Stern [mailto:stern@rowland.harvard.edu] 
Sent: Monday, October 21, 2013 7:23 PM
To: Vishal Annapurve
Cc: Ming Lei; Linux Kernel Mailing List; linux-usb
Subject: RE: [PATCH] usb-storage: scsiglue: Changing the command result

On Sat, 19 Oct 2013, Vishal Annapurve wrote:

> Hi,
> 
> Attaching the new patch which will replace all the occurrences of 
> DID_ABORT with DID_TIMOUT in USB Storage whenever it sees 
> US_FLIDX_TIMED_OUT bit is set.

It seems okay, but you forgot to update the isd200.c and cypress_atacb.c files.

Also, in the future please put your patches inline with the rest of the email, not as a separate attachment.

Alan Stern

RE: [PATCH] usb-storage: scsiglue: Changing the command result

[Alan Stern]

On Sat, 26 Oct 2013, Vishal Annapurve wrote:

> Hi Alan,
> 
> Here is the new patch:
> 
> From: Vishal Annapurve <vannapurve@nvidia.com>
> Date: Sat, 26 Oct 2013 21:10:11 +0530
> Subject: [PATCH] usb: storage: Proper cmd result assignment
> 
> This change replaces DID_ABORT with DID_TIMEOUT as a command result
> whenever US_FLIDX_TIMED_OUT bit is set.
> 
> This change is made to bring USB storage inline with a recent change:
> 
> commit    18a4d0a22ed6c54b67af7718c305cd010f09ddf8
> 
> [SCSI] Handle disk devices which can not process medium access commands
> We have experienced several devices which fail in a fashion we do not
> currently handle gracefully in SCSI. After a failure these devices will
> respond to the SCSI primary command set (INQUIRY, TEST UNIT READY, etc.)
> but any command accessing the storage medium will time out.
> 
> As the USB storage was setting command result as aborted rather than
> timed out, SCSI layer was not recognizing the above mentioned failure
> pattern.
> 
> Change-Id: Ic58e2247fed11649f4dbea56382354ba2fe0be1b
> ---
>  drivers/staging/keucr/transport.c   | 6 +++---
>  drivers/staging/keucr/usb.c         | 5 +++--

Those two files aren't part of usb-storage; they belong to a different 
driver.  It would be better to have a separate patch for them, and you 
should copy that patch to the driver's maintainer.

>  drivers/usb/storage/cypress_atacb.c | 1 +
>  drivers/usb/storage/isd200.c        | 2 +-
>  drivers/usb/storage/transport.c     | 8 ++++----
>  drivers/usb/storage/usb.c           | 9 +++++----
>  6 files changed, 17 insertions(+), 14 deletions(-)

Otherwise this is okay.  You might also want to submit a third patch 
for the uas driver.

Alan Stern

Re: [PATCH] usb-storage: scsiglue: Changing the command result

[Greg KH]

On Sat, 26 Oct 2013, Vishal Annapurve wrote:
> Hi Alan,
> 
> Here is the new patch:
> 
> From: Vishal Annapurve <vannapurve@nvidia.com>
> Date: Sat, 26 Oct 2013 21:10:11 +0530
> Subject: [PATCH] usb: storage: Proper cmd result assignment
> 
> This change replaces DID_ABORT with DID_TIMEOUT as a command result
> whenever US_FLIDX_TIMED_OUT bit is set.
> 
> This change is made to bring USB storage inline with a recent change:
> 
> commit    18a4d0a22ed6c54b67af7718c305cd010f09ddf8
> 
> [SCSI] Handle disk devices which can not process medium access commands
> We have experienced several devices which fail in a fashion we do not
> currently handle gracefully in SCSI. After a failure these devices will
> respond to the SCSI primary command set (INQUIRY, TEST UNIT READY, etc.)
> but any command accessing the storage medium will time out.
> 
> As the USB storage was setting command result as aborted rather than
> timed out, SCSI layer was not recognizing the above mentioned failure
> pattern.
> 
> Change-Id: Ic58e2247fed11649f4dbea56382354ba2fe0be1b

What's this line for?  (yeah, I know where it comes from, the point is
it doesn't belong here...)

Also, no signed-off-by, so I can't apply it...

greg k-h