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

On Wednesday 13 August 2008, Alan Stern wrote:
> On Tue, 12 Aug 2008, Inaky Perez-Gonzalez wrote:
> 

> I'm not sure I remember all of them either!  The main issues that 
> spring to mind are:
> 
> 	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.
 
> 	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().

> Neither of these appears to be a showstopper for your patch.  But there 
> are some questionable aspects, noted below.
> 
> > diff -r d60c9a74e581 drivers/usb/core/hub.c
> > --- a/drivers/usb/core/hub.c	Tue Aug 12 09:14:24 2008 -0700
> > +++ b/drivers/usb/core/hub.c	Tue Aug 12 15:14:57 2008 -0700
> > @@ -3447,3 +3447,33 @@ int usb_reset_device(struct usb_device *
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(usb_reset_device);
> > +
> > +
> > +void __usb_queue_reset_device(struct work_struct *ws)
> > +{
> > +	struct usb_device *udev =
> > +		container_of(ws, struct usb_device, reset_ws);
> > +	usb_reset_device(udev);
> > +	usb_put_dev(udev);
> > +}
> 
> You don't need usb_put_dev here (or the corresponding usb_get_dev)
> since the workqueue entry is reliably cancelled before the device 
> structure is destroyed.

/me mumbles to himself how still he feels unueasy about it, but
yeah, you are right. I guess this is me still thinking about the
first patch I implemented where this was completely outside the stack.

In fact, there was also a bug in there -- when cancelling the workqueue
on device_disable() we also had to drop that reference count if work
was pending.
 
> However you _do_ need to lock and unlock the device.

lock_device_for_reset? or just lock_device?

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

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].
 
> If this routine were put in usb.c rather than hub.c then it could be 
> made static.

Good point.
 
> > +/**
> > + * Reset a USB device from an atomic context
> > + *
> > + * @udev: USB device to reset
> > + *
> > + * This function can be used to reset a USB device from an atomic
> > + * context, where usb_composite_device_reset() or usb_device_reset()
> > + * won't work. 
> > + * 
> > + * If a reset is pending already, it won't schedule another one.
> > + * No need to lock/unlock because schedule_work() does it's own.
> 
> s/it's/its/

ack
 
> > diff -r d60c9a74e581 include/linux/usb.h
> > --- a/include/linux/usb.h	Tue Aug 12 09:14:24 2008 -0700
> > +++ b/include/linux/usb.h	Tue Aug 12 15:14:57 2008 -0700
> > @@ -326,6 +326,7 @@ struct usb_bus {
> >  	struct mon_bus *mon_bus;	/* non-null when associated */
> >  	int monitored;			/* non-zero when monitored */
> >  #endif
> > +	struct work_struct reset_ws;	/* for resets in atomic context */
> 
> This appears to be a mistake, left over from an earlier incarnation.

Yeah, it was a pasting error.

Ok, I'll work on the fixes and repost the patch -- I'll add that ugly locking
stuff waiting for a better sollution or confirmation.

Thanks!


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