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

On Wed, 13 Aug 2008, Inaky Perez-Gonzalez wrote:

> > 	What happens if a driver requests a reset and then is unbound
> > 	before the reset happens?
> 
> The reset is still pending until it happens -- the way I see it is: the
> intention of the driver was to reset the device. The fact that it is 
> delayed because it decided that from an atomic context is a mere artifact.
> 
> If the driver decided to reset the device, is because the device has to 
> be reset to bring it to a known state, so it should be completed.

I tend to agree.  Oliver is more concerned about this.

> > 	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?"  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...

> > 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.

The difference is that usb_lock_device_for_reset requires an interface 
argument.  With your current patch, the interface information has been 
lost when the time comes to do the locking.

> I need also some help here: how does that interact with being called from 
> .probe() or .disconnect()...as in where is the lock taken?

The situation is a bit of a mess.  In fact, usb_lock_device_for_reset
is strictly speaking not correct; it assumes that if it was called
while the interface is being bound then it was called by the binding 
thread.  That whole thing ought to be rewritten.

> Would it ok to do something dirty like?
> 
> result = usb_trylock_device(udev);
> if (result < 0)
> 	did_lock = 0;
> usb_reset_device(udev);
> if (did_lock)
> 	usb_unlock_device(udev);
> 
> [sorry, can't find the locking rules, but looks to me that at least
> .probe and .disconnect would have it locked and would need that
> hack or something equivalent and more proper].

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.

Alan Stern

--
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