Hi,
On 19-01-16 11:39, Oliver Neukum wrote:
Issue a TASK_ABORT iu to the device and wait for it
to complete.
Signed-off-by: Oliver Neukum <oneukum@xxxxxxxx>
---
drivers/usb/storage/uas.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 06211ee..d8e70f7 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -55,6 +55,7 @@ struct uas_dev_info {
spinlock_t lock;
struct work_struct work;
struct mutex management_mutex;
+ struct completion deathknell;
};
enum {
@@ -431,6 +432,10 @@ static void uas_cmd_cmplt(struct urb *urb)
static void uas_tmf_cmplt(struct urb *urb)
{
+ struct scsi_cmnd *cmnd = urb->context;
+ struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
+
+ complete(&devinfo->deathknell);
}
static struct urb *uas_alloc_data_urb(struct uas_dev_info *devinfo, gfp_t gfp,
@@ -734,6 +739,7 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
cmdinfo->state |= COMMAND_ABORTED;
mutex_lock(&devinfo->management_mutex);
+ init_completion(&devinfo->deathknell);
usb_fill_bulk_urb(devinfo->management_urb,
devinfo->udev,
devinfo->cmd_pipe, /* shared */
@@ -750,6 +756,9 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
if (err < 0) /* unkillable */
goto give_up;
+ wait_for_completion_timeout(&devinfo->deathknell, USB_CTRL_GET_TIMEOUT);
+ /* in case of timeout */
+ usb_kill_urb(devinfo->management_urb);
give_up:
mutex_unlock(&devinfo->management_mutex);
Hmm, same problem here as before, you're using waiting functions in a piece
of code holding a spinlock. If waiting while in a scsi-eh function is ok
(not sure about that we need to ask the scsi people), then you must do this
outside of the code holding the spinlock.
Also there is a problem in the original uas_eh_abort_handler() code, which
gets exposed once uas_eh_abort_handler starts reporting success, the original
code stops uas_do_work() / uas_zap_pending() / uas_stat_cmplt() from touching
the scsi cmnd any further by clearing devinfo->cmnd[idx] but this means that
the tag / bulk-stream-id can be reused while the command is still being aborted
which is not good. AFAICT this currently is not a problem since we fail
uas_eh_abort_handler() which will cause uas_eh_bus_reset_handler to get
called which will zap everything, but once we add taskmanagement this is a
problem.
I believe that one thing to change before adding taskmanagement is to stop doing:
"devinfo->cmnd[cmdinfo->uas_tag - 1] = NULL;" in uas_eh_abort_handler
and instead start checking the COMMAND_ABORTED flag in uas_do_work() /
uas_zap_pending() / uas_stat_cmplt(), ignoring commands with this flag
set, and moving the "devinfo->cmnd[cmdinfo->uas_tag - 1] = NULL;" to
uas_zap_pending() for cmnd-s with the COMMAND_ABORTED flag set.
AFAIK cmnd-s which have been passed into uas_eh_abort_handler() should
not call scsi_done once that function has exited, hence we should simply
do devinfo->cmnd[cmdinfo->uas_tag - 1] = NULL;" for COMMAND_ABORTED
cmnds in uas_zap_pending(). We cannot do this before as we cannot use
the uas tag/stream-id until either tmf has successfully aborted the task,
or we have reset the device.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
