On Fri, 15 Mar 2013, Felipe Balbi wrote: > > Adding this back in revealed a bug in udc-core. It is fixed as > > follows: > > > > Index: usb-3.9/drivers/usb/gadget/udc-core.c > > =================================================================== > > --- usb-3.9.orig/drivers/usb/gadget/udc-core.c > > +++ usb-3.9/drivers/usb/gadget/udc-core.c > > @@ -216,7 +216,7 @@ static void usb_gadget_remove_driver(str > > usb_gadget_disconnect(udc->gadget); > > udc->driver->disconnect(udc->gadget); > > udc->driver->unbind(udc->gadget); > > - usb_gadget_udc_stop(udc->gadget, udc->driver); > > + usb_gadget_udc_stop(udc->gadget, NULL); > > BTW, this was only to maintain backwards compatibility during the > conversion. I guess it's safe to remove the extra parameter. Very nice > finding. It wasn't exactly hard to spot. When you get an oops, that's a pretty blatant indication that something's wrong. :-) > > udc->driver = NULL; > > udc->dev.driver = NULL; > > > > At the point where the UDC driver's stop routine is called, the gadget > > no longer has a driver. You might even want to move the line I changed > > down below the following two assignments, to make this more clear. > > > > By the way, what happens if the UDC hardware doesn't support software > > control of the D+ pullup? The usb_gadget_disconnect call above won't > > do anything, and that leaves open a window for a race. The host might > > send a SETUP packet between the unbind and stop calls. Should we > > insist that all UDC drivers do have a working ->pullup routine? > > that would be one way. But, as you said, some controllers simply can't > handle it. Have those controllers been subject to this race all along? If they have, I guess there's nothing we can do about it. Otherwise we should try to fix it. > > In addition, code-reading revealed a bug in an error pathway of > > net2280.c: > > > > Index: usb-3.9/drivers/usb/gadget/net2280.c > > =================================================================== > > --- usb-3.9.orig/drivers/usb/gadget/net2280.c > > +++ usb-3.9/drivers/usb/gadget/net2280.c > > @@ -1924,7 +1924,6 @@ static int net2280_start(struct usb_gadg > > err_func: > > device_remove_file (&dev->pdev->dev, &dev_attr_function); > > err_unbind: > > - driver->unbind (&dev->gadget); > > dev->gadget.dev.driver = NULL; > > dev->driver = NULL; > > return retval; > > > > If the start method fails, udc_bind_to_driver() calls driver->unbind. > > The UDC driver doesn't need to do it. I have not audited the other > > gadget drivers for similar problems (i.e., unnecessary cleanup calls in > > error pathways); it might be worth taking a quick look. net2272 at > > least probably has the same bug. > > > > With these two fixes applied, everything seemed to work okay. Should I > > submit them officially? > > please do. I guess we also want to Cc stable for v3.8 ;-) Right. Maybe even earlier; the udc-core bug has been present in one form or another since 3.1. Alan Stern -- 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
