Re: [RFC/PATCH] USB reset on atomic context

On Wednesday 13 August 2008, Alan Stern wrote:
> On Wed, 13 Aug 2008, Inaky Perez-Gonzalez wrote:
> 
> > > 	What happens if a driver requests a reset while it is being
> > > 	bound or unbound?
> > 
> > Same as above. While bound it would cause the driver to be unbound and
> > then reenumeration. While unbinding it'd cause nothing but the reset,
> > as driver is "unbound" once we exit .disconnect().
> 
> Yes -- what I meant was more like "What _should_ happen if a driver 
> requests a reset while it is being bound or unbound?"

What happens now when we call usb_reset_device() from .probe()? The call
is allowed, and it will unbind the drivers and reset the device. So
when .probe() finishes, .disconnect() is inmediately called. I don't 
really see an issue with it.

> In particular,  
> if the request is made while the disconnect method is running, should 
> it be ignored?
>
> Or even more to the point, if the actual reset is delayed until the 
> disconnect method is running (or has completed!), should it be skipped?  
> This is related to the locking question...

No, it should not. Reset cleans hw state and thus, if issues, it is because
it is needed. It should proceed. It might have to wait until .disconnect() 
finishes though (due to locking).
 
> 
> > > However you _do_ need to lock and unlock the device.
> > 
> > lock_device_for_reset? or just lock_device?
> 
> It depends.  For the patch in its current state, plain old 
> usb_lock_device.  For what Oliver wants to do, it should be 
> usb_lock_device_for_reset.

If my device fails to come out of suspend, I might want to use
reset to recover it. Likewise, using lock_device_for_reset()
would rule out being able to use queue_reset_device() while 
the device is probing or releasing (versys lock_device() just
delaying it until .probe() or .disconnect() finish).


> The intended usage goes like this:
> 
> 	did_lock = usb_trylock_device(udev);
> 	if (did_lock >= 0) {
> 		usb_reset_device(udev);
> 		if (did_lock)
> 			usb_unlock_device(udev);
> 	}
> 
> This also could stand some improvement.

I'd say move to usb_lock_device(); that way when in .probe() or .disconnect()
context it still works, just delayed until those calls are done.

My only concern is then if anything in .probe() or .disconnect() flushes
the system workqueue...then we have a deadlock. That's technically a bug in
the driver, but caused by the patch. 

I could hijack the ksuspend_usbd workqueue to do it, as that is internal, but 
that'd also mean ksuspend_usb_{init,cleanup} need to be taken out of the 
#if CONFIG_PM (who doesn't do PM these days, anyway?).


-- 
Inaky
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html