Am Mittwoch, 11. November 2009 21:49:59 schrieb Alan Stern:
> On Wed, 11 Nov 2009, Oliver Neukum wrote:
> > Hi,
> >
> > it seems to me that we have a subtle bug in usb_reset_device().
> > If we reset a device we may reprobe an interface. If that causes
> > another interface to be claimed we may call post_reset() for that
> > interface without having called pre_reset().
> >
> > I think it is necessary to do the rebinding in a second pass after
> > doing the notifications. But this is subtle. Alan, what do you think?
>
> You are right. But I would make the code a little cleaner...
>
> > @@ -3696,13 +3696,15 @@ int usb_reset_device(struct usb_device *udev)
>
> A few lines above this, it would be good to change
>
> if (config) {
>
> to
>
> if (ret != -ENODEV && config) {
>
> There's no point sending post_reset notifications if the reset caused
> the device to go away.
IIRC the reasoning was that many drivers implement pre_reset() as mutex_lock()
of a mutex also needed in disconnect(), so that pre/post_reset are always
supposed to be strictly balanced.
> > if (!rebind && cintf->dev.driver) {
> > drv = to_usb_driver(cintf->dev.driver);
> > - if (drv->post_reset)
> > - rebind = (drv->post_reset)(cintf);
> > - else if (cintf->condition ==
> > - USB_INTERFACE_BOUND)
> > - rebind = 1;
> > + rebind = (drv->post_reset)(cintf);
> > }
> > - if (ret == 0 && rebind)
> > + cintf->needs_binding = rebind;
>
> You don't need the "rebind" variable here. Directly set
> cintf->needs_binding to the result of the method call. Or rather, if
> the method call returns nonzero, set cintf->needs_binding to 1 (since
> it is a bitflag, not an int).
Roger
>
> > + }
> > +
> > + /* second pass to not call post_reset on new drivers */
>
> Rephrase the comment to make it more clear:
>
> /* Don't rebind until after all the post_reset calls */
>
> > + for (i = config->desc.bNumInterfaces - 1; i >= 0; --i){
> > + struct usb_interface *cintf = config->interface[i];
>
> Missing blank line.
>
> > + if (ret == 0 && cintf->needs_binding)
>
> If ret is tested above then it doesn't need to be tested here. And in
> fact the existing test is wrong; it should be against -ENODEV rather
> than 0. A reset can fail for reasons that don't cause the device to go
> away.
But in that case the reason for resetting is likely to persist and the device
won't work. Do we really want to rebind under such conditions?
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
