RE: [PATCH] usb: uas: fix a plug & unplug racing

[EJ Hsu <ejh@nvidia.com>]

Oliver Neukum wrote:

> Am Sonntag, den 12.01.2020, 19:30 -0800 schrieb EJ Hsu:
> 
> Isn't that the bug? A command to a detached device should fail.
> Could you please elaborate? This issue would not be limited to uas.
> 
In the case I mentioned, the hub thread of external hub running 
uas_probe() will get stuck waiting for the completion of scsi scan. 

The scsi scan will try to probe a single LUN using a SCSI INQUIRY.
If the external hub has been unplugged before LUN probe, the device 
state of uas device will be set to USB_STATE_NOTATTACHED by the 
root hub thread. So, all the following calls to usb_submit_urb() in 
uas driver will return -NODEV, and accordingly uas_queuecommand_lck() 
will return SCSI_MLQUEUE_DEVICE_BUSY to scsi_request_fn().

scsi_request_fn() then puts this scsi command back into request queue.
Because this scsi device is just created and during LUN probe process, 
this scsi command is the only one in the request queue. So, it will be picked
up soon and dispatched to uas driver again. This cycle will continue until
uas_disconnect() is called and its "resetting" flag is set. However, the 
hub thread of external hub still got stuck waiting for the completion of
this scsi command, and may not be able to run uas_disconnect(). 
A deadlock happened.

> > +static void uas_scan_work(struct work_struct *work) {
> > +     struct uas_dev_info *devinfo =
> > +             container_of(work, struct uas_dev_info, scan_work);
> > +     struct Scsi_Host *shost = usb_get_intfdata(devinfo->intf);
> > +
> > +     dev_dbg(&devinfo->intf->dev, "starting scan\n");
> > +     scsi_scan_host(shost);
> > +     dev_dbg(&devinfo->intf->dev, "scan complete\n");
> > +
> > +     usb_autopm_put_interface(devinfo->intf);
> 
> scsi_scan_host() does runtime PM on the SCSI level. There is no need for us to
> duplicate that.
>

In my opinion, if scsi_scan_host() will be run asynchronously, this interface 
needs to be guarded against runtime PM between uas_probe() & uas_scan_work().
 
> 
> It is not enough to do this in disconnect() We are guarded against runtime PM,
> but not against system sleep.
> You'd need to handle this in suspend() and resume(), too.
> And, unfortunately, the device could be reset from another interface.
> 

As Allen said, this is a copy of the way usb-storage works and I did not see any
related protection in usb-storage. But I will do more check on it. 

Thanks,
EJ
--nvpublic