Hi,
On Thu, Apr 19, 2012 at 12:29:17PM +0300, Alexander Shishkin wrote:
> 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.
indeed, udc_stop() is likely to cut clocks which would render register
access impossible ;-)
I still think your original patch is correct. Alan, is your tree really
clean ?
--
balbi
Attachment:
signature.asc
Description: Digital signature
