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 <oneu...@suse.com> --- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
