Hi,
On Wed, Apr 18, 2012 at 12:17:48PM -0400, Alan Stern 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);
> }
>
> Notice the relative order of the usb_gadget_disconnect() and
> usb_gadget_udc_stop() calls. This hasn't been changed since 3.4-rc2,
> has it? I hope not -- if it has changed then it is now wrong.
you need to fetch Greg's usb-linus branch ;-)
> On the other hand, it's certainly true that dummy_udc_stop() doesn't
> need to call dummy_pullup() -- usb_gadget_disconnect() calls it for us.
fixed in my tree, will send a patch shortly.
> On Wed, 18 Apr 2012, Felipe Balbi wrote:
>
> > Alan, maybe you're more aware of how dummy_hcd works, but can we get a
> > disconnect event initiated by the dummy_hcd instead of the dummy_udc ?
>
> As a matter of fact, we can. If the host turns off the port power
> feature then there's an immediate disconnect. This is the way hubs
> (including root hubs) are supposed to behave, although a lot of them
> probably don't.
ok, good.
> > looking closer at that part from dummy, it's quite badly written.
> > There's no easy way for us to differentiate a bus reset from a gadget
> > driver removal.
>
> Gadget driver removal is indicated by the fact that dum->driver is
> NULL.
>
> > I think your patch is closer to being right than mine. We still need to
> > notify gadget driver about a disconnect when a reset happens and my
> > patch will break that, while yours won't.
>
> I'm still not convinced the problem has been properly identified. In
> fact, I'm not convinced there's any problem at all. I just booted my
> test kernel (a lightly modified 3.4-rc2), loaded dummy-hcd and g-zero,
> and then removed both of them. Nothing went wrong.
>
> A patch to remove the unneeded call of dummy_pullup() would be
> acceptable.
I have that...
--
balbi
Attachment:
signature.asc
Description: Digital signature
