Hi,
looking at this code it struck me as not really good
at its purpose, if there is ever contention.
If we really run in the repeat loop
while (!usb_trylock_device(udev)) {
/* If we can't acquire the lock after waiting one second,
* we're probably deadlocked */
because somebody else is resetting the device, what
are we really waiting for? Do we really want to even
reset the device in that case? That has just happened.
It seems to me that in that case we should really wait
the reset to proceed and then report its outcome.
When we use it, for example in uas:
static int uas_eh_device_reset_handler(struct scsi_cmnd *cmnd)
{
struct scsi_device *sdev = cmnd->device;
struct uas_dev_info *devinfo = sdev->hostdata;
struct usb_device *udev = devinfo->udev;
unsigned long flags;
int err;
err = usb_lock_device_for_reset(udev, devinfo->intf);
if (err) {
shost_printk(KERN_ERR, sdev->host,
"%s FAILED to get lock err %d\n", __func__, err);
return FAILED;
}
We happily wait for the first reset to take place, only then to do it again,
or, worse, we wait for shorter than the timeout for the control
message we use to perform the actual reset:
unsigned long jiffies_expire = jiffies + HZ;
Now, this may just not be important enough to change, but
it really does not look good. What do you think?
Regards
Oliver
