Re: [PATCH] usb: gadget: dummy: don't disconnect on gadget removal

[Felipe Balbi <balbi@xxxxxx>]

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