Re: Gadget driver ->disconnect callback during unbinding

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jun 14, 2017 at 10:26:29AM -0400, Alan Stern wrote:
> On Wed, 14 Jun 2017, Peter Chen wrote:
> 
> > On Tue, Jun 13, 2017 at 10:22:26AM -0400, Alan Stern wrote:
> > > On Tue, 13 Jun 2017, Felipe Balbi wrote:
> > > 
> > > > 
> > > > Hi Alan,
> > > > 
> > > > Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes:
> > > > > Felipe:
> > > > >
> > > > > A UDC driver will invoke the gadget driver's ->disconnect callback
> > > > > whenever the D+ pullup is turned off.  During gadget driver unbinding,
> > > > > this happens automatically when usb_gadget_remove_driver() calls
> > > > > usb_gadget_disconnect().
> > > > 
> > > > usb_gadget_disconnect() only calls UDC's ->pullup(), not gadget driver's
> > > > ->disconnect()
> > > > 
> > > > int usb_gadget_disconnect(struct usb_gadget *gadget)
> > > > {
> > > > 	int ret = 0;
> > > > 
> > > > 	if (!gadget->ops->pullup) {
> > > > 		ret = -EOPNOTSUPP;
> > > > 		goto out;
> > > > 	}
> > > > 
> > > > 	if (gadget->deactivated) {
> > > > 		/*
> > > > 		 * If gadget is deactivated we only save new state.
> > > > 		 * Gadget will stay disconnected after activation.
> > > > 		 */
> > > > 		gadget->connected = false;
> > > > 		goto out;
> > > > 	}
> > > > 
> > > > 	ret = gadget->ops->pullup(gadget, 0);
> > > > 	if (!ret)
> > > > 		gadget->connected = 0;
> > > > 
> > > > out:
> > > > 	trace_usb_gadget_disconnect(gadget, ret);
> > > > 
> > > > 	return ret;
> > > > }
> > > > EXPORT_SYMBOL_GPL(usb_gadget_disconnect);
> > > 
> > > Yes, but as I mentioned above, the pullup routine calls the gadget
> > > driver's disconnect method.
> > > 
> > > > > But immediately thereafter, usb_gadget_remove_driver() calls the gadget 
> > > > > driver's ->disconnect callback directly!  Do we have any reason for 
> > > > > doing this?  I don't see point to it.  Should that call be removed?
> > > > 
> > > > Unless I'm missing something, that call is necessary :-) Have you faced
> > > > any issues with it? Are there any UDCs calling gadget driver's
> > > > ->disconnect() from ->pullup() ?
> > > 
> > > I haven't faced any issues because gadget drivers don't seem to mind 
> > > ->disconnect getting called twice in a row.  :-)  (And note that the 
> > > API doesn't have a corresponding ->connect callback...  Although to 
> > > tell the truth, it's not clear what a gadget driver needs to do in 
> > > either callback.  Can we simply remove ->disconnect altogether?)
> > > 
> > > Both dummy-hcd and net2280 call ->disconnect from ->pullup.  I haven't
> > > checked other UDC drivers, but it seems like something they should all
> > > do.  Except perhaps in the case where the UDC driver doesn't have a
> > > pullup method.
> > > 
> > 
> > No, ->pullup is expected to be called from UDC core, and the UDC core
> > makes sure the coming ->disconnect at kinds of situations.
> 
> I think you are wrong.  The UDC core calls ->pullup from the 
> usb_gadget_disconnect() routine, but it does not call ->disconnect from 
> that routine.
> 

You may misunderstand my point, I mean there are always ->disconnect
call after ->pullup at UDC core, eg, usb_gadget_remove_driver and
usb_udc_softconn_store. So individual UDC driver does not need
->disconnect at its ->pullup implementation.

> Furthermore, there is an advantage to having the UDC driver call
> ->disconnect from within its ->pullup method: It can retain its private
> spinlock across the ->disconnect call.  This will help prevent races.  
> For example, what would happen if ->disconnect was called while a
> ->setup callback was in progress?  Or vice versa?
> 

You mean the private UDC driver spinlock can help prevent races well for
->setup and ->pullup in UDC driver? Yes, it is the good point. I am
wondering why composite_disconnect has spinlock and without for
composite_setup.

-- 

Best Regards,
Peter Chen
--
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



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux