On Thu, Mar 14, 2013 at 03:34:31PM -0400, Alan Stern wrote:
> On Thu, 14 Mar 2013, Felipe Balbi wrote:
>
> > Here's the diff which needs testing:
>
> Can't test the net2272 part.
>
> > diff --git a/drivers/usb/gadget/net2280.c b/drivers/usb/gadget/net2280.c
> > index 691cc65..d23c1b8 100644
> > --- a/drivers/usb/gadget/net2280.c
> > +++ b/drivers/usb/gadget/net2280.c
> > @@ -1941,6 +1941,13 @@ stop_activity (struct net2280 *dev, struct usb_gadget_driver *driver)
> > for (i = 0; i < 7; i++)
> > nuke (&dev->ep [i]);
> >
> > + /* report disconnect; the driver is already quiesced */
> > + if (driver) {
> > + spin_unlock (&dev->lock);
> > + driver->disconnect (&dev->gadget);
> > + spin_lock (&dev->lock);
> > + }
> > +
> > usb_reinit (dev);
> > }
>
> 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.
> 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.
> 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 ;-)
--
balbi
Attachment:
signature.asc
Description: Digital signature
