On Thu, 2016-01-21 at 16:04 +0100, Hans de Goede wrote:
> Hi,
>
> On 19-01-16 11:39, Oliver Neukum wrote:
> > We have exactly one TMF. It must be locked against
> > reuse.
> >
> > Signed-off-by: Oliver Neukum <oneukum@xxxxxxxx>
> > ---
> > drivers/usb/storage/uas.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> > index 05b6f80..06211ee 100644
> > --- a/drivers/usb/storage/uas.c
> > +++ b/drivers/usb/storage/uas.c
> > @@ -54,6 +54,7 @@ struct uas_dev_info {
> > struct scsi_cmnd *cmnd[MAX_CMNDS];
> > spinlock_t lock;
> > struct work_struct work;
> > + struct mutex management_mutex;
> > };
> >
> > enum {
> > @@ -732,6 +733,7 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
> > /* Ensure that try_complete does not call scsi_done */
> > cmdinfo->state |= COMMAND_ABORTED;
> >
> > + mutex_lock(&devinfo->management_mutex);
>
> You cannot lock a mutex while holding a spinlock, and you're holding
That is problematic. The ABORT_TMF does need IO for which we need
to wait. And if we just submit and pretend the abort worked, the tag
will be reused.
Perhaps we could drop the spinlock. I think it is time to talk to
the SCSI people.
> the devinfo->lock spinlock here. also I'm not sure if taking mutex-s
> from a scsi-eh is allowed.
I think so, as long as no holder of the mutex does IO.
>
> > usb_fill_bulk_urb(devinfo->management_urb,
> > devinfo->udev,
> > devinfo->cmd_pipe, /* shared */
> > @@ -749,6 +751,7 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
> > goto give_up;
> >
> > give_up:
> > + mutex_unlock(&devinfo->management_mutex);
>
> Your commit msg says that you're trying to protect the urb against re-use,
> but you've only submitted it here, it has not completed yet. You probably
> want to use a flag protected by the existing devinfo->lock instead of using
> a new mutex for this.
But it is finished for sure at that point:
time = wait_for_completion_timeout(&devinfo->deathknell, USB_CTRL_GET_TIMEOUT);
/* in case of timeout */
usb_kill_urb(devinfo->management_urb);
Regards
Oliver
--
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
