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.
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.
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.
> 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.
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
