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

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

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

Not strictly true.  The call is allowed, and if the drivers have proper 
pre_reset and post_reset methods then they won't be unbound.  
(Conversely, drivers that don't have pre_reset and post_reset methods 
_will_ be unbound -- except for the driver currently being probed.)

> So
> when .probe() finishes, .disconnect() is inmediately called. I don't 
> really see an issue with it.

It isn't an issue, except as regards the locking.  If a driver calls 
usb_reset_device directly from within its probe thread, there's no 
problem.  But what if it queues a delayed reset request?

My feeling is that the queued request should wait until probe returns.  
If the probe was successful then the reset can be carried out; 
otherwise it should be cancelled just as in the case of unbinding.

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

Well, there's the difficulty.  After disconnect has returned, the
driver has absolutely no claim on the device.  It shouldn't be able to
_touch_ the device, let alone reset it!  That's Oliver's main argument.

If a driver feels strongly that a reset really is needed to clean the
hardware state before unbinding, it should call usb_reset_device
directly from its disconnect method.  It should _not_ force the
disconnect method to block until a queued reset has finished.  That
approach is very likely to cause deadlock.

(Note incidentally, that usb-storage's disconnect method _does_ wait 
until a reset running in another thread has finished.  Fortunately that 
other thread isn't doing the equivalent of a delayed reset, but it's 
still something to watch out for -- the same locking mechanism has to 
work for both cases.)

> If my device fails to come out of suspend, I might want to use
> reset to recover it.

usbcore already does this.  Well, in reality it does a logical 
disconnect followed by a reset and re-enumeration.

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

Agreed.  For this reason, if we want interface drivers to be able to 
use usb_queue_reset_device while binding or unbinding, 
usb_lock_device_for_reset will have to be rewritten first.

On the other hand, if you want to call usb_queue_reset_device from some 
part of the core without regard to interfaces or their drivers, then 
usb_lock_device is the way to go.  I don't know why you would want to 
do this, but maybe there are good reasons for it in Wireless USB.

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

Unfortunately we _can't_ simply delay until disconnect is done.  Not if
we want the queued reset to be cancelled when the interface is unbound.  
There needs to be some synchronization between the unbinding code and
the workqueue code.  In fact, the unbinding routine has to wait until
the workqueue routine has agreed to cancel the reset.

This means the workqueue routine can't simply use usb_lock_device,
since it may need to wake up while the device is still locked.  This is
another reason for rewriting usb_lock_device_for_reset.

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

A workqueue should _never_ be flushed except by the code that manages 
the workqueue itself.

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

s390 doesn't.  Maybe others as well.

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