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.
> > After aa0747394337e50533badd46e8fb7106bad3311e (usb: gadget: dummy_hcd:
> > convert to new-style udc-probe) this problem should be reproducible, so
> > from v3.1 on.
> >
> > I can't pretend to fully comprehend the logic around set_link_state(),
> > but I'm fairly sure it shouldn't end up calling disconnect twice.
>
> It shouldn't. If it does, the reason is not what you think.
>
> > Signed-off-by: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> > ---
> > drivers/usb/gadget/dummy_hcd.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
> > index a6dfd21..66fce0e 100644
> > --- a/drivers/usb/gadget/dummy_hcd.c
> > +++ b/drivers/usb/gadget/dummy_hcd.c
> > @@ -371,7 +371,7 @@ static void set_link_state(struct dummy_hcd *dum_hcd)
> > * We're connected and not reset (reset occurred now),
> > * and driver attached - disconnect!
> > */
> > - if ((dum_hcd->old_status & USB_PORT_STAT_CONNECTION) != 0 &&
> > + if ((dum_hcd->port_status & USB_PORT_STAT_CONNECTION) != 0 &&
>
> Definitely should be old_status. The idea is that this code should run
> if we were connected the last time we checked but we aren't connected
> now.
Ok, that makes sense.
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
