On Wed, 18 Apr 2012 12:17:48 -0400 (EDT), Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, 18 Apr 2012, Alexander Shishkin wrote:
>
> > On Tue, 17 Apr 2012 10:49:52 -0400 (EDT), Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> > > On Tue, 17 Apr 2012, Alexander Shishkin wrote:
> > >
> > > > Currently, on gadget removal path, dummy_pullup() gets called after
> > > > gadget's disconnect(). Now, dummy_pullup() decides that it needs to
> > > > disconnect again, resulting in a crash. IOW, rmmod g_zero will crash
> > > > with dummy_hcd.
> > >
> > > This doesn't sound right. The gadget removal path is dummy_udc_stop(),
> > > right? That routine sets dum->driver to NULL before calling
> > > dummy_pullup(). Consequently the last test in the "if" statement you
> > > are changing won't succeed, so the gadget's disconnect method won't be
> > > called.
> >
> > Since the new style gadgets were introduced, the removal path is like
> > this:
> >
> > udc->driver->disconnect(udc->gadget);
> > udc->driver->unbind(udc->gadget);
> > usb_gadget_disconnect(udc->gadget);
> > usb_gadget_udc_stop(udc->gadget, udc->driver);
> >
> > where usb_gadget_disconnect() calls gadget's pullup(), which in case of
> > dummy_hcd will end up calling driver->disconnect(), which has already
> > been called higher up in usb_gadget_remove. And all that before
> > dummy_udc_stop(), which indeed gets it right.
>
> You'd be right except for one thing: You listed the code from
> udc-core.c:usb_gadget_remove_driver() wrongly. In my tree it actually
> looks like this:
>
> if (udc_is_newstyle(udc)) {
> udc->driver->disconnect(udc->gadget);
> udc->driver->unbind(udc->gadget);
> usb_gadget_udc_stop(udc->gadget, udc->driver);
> usb_gadget_disconnect(udc->gadget);
> } else {
> usb_gadget_stop(udc->gadget, udc->driver);
> }
I'm using linux-next, and as of today, the call order is still like I
listed before. The change that introduced this order is 8ae8090c (usb:
gadget: udc-core: fix asymmetric calls in remove_driver), which is also
present in "usb-linus" branch of linux-usb tree (that's how, I assume
it's got to linux-next) and Felipe's "fixes" and "master".
So no, I didn't make this up. ;)
The change seems logical though: udc_stop() may have disabled the usb
controller, and calling pullup() after that doesn't look very useful.
Regards,
--
Alex
--
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
