Re: [PATCH 1/2] usb: gadget: add usb_gadget_activate/deactivate functions

[Felipe Balbi]

On Tue, Apr 07, 2015 at 10:31:52AM +0200, Robert Baldyga wrote:
> These functions allows to deactivate gadget to make it not visible to
> host and make it active again when gadget driver is finally ready.
> 
> They are needed to fix usb_function_activate() and usb_function_deactivate()
> functions which currently are not working as usb_gadget_connect() is
> called immediately after function bind regardless to previous calls of
> usb_gadget_disconnect() function.

and that's what needs to be fixed, a long time ago I wrote the patch
below which I never got to finishing:

commit a23800e2463ae1f4eafa7c0a15bb44afee75994f
Author: Felipe Balbi <balbi@xxxxxx>
Date:   Thu Jul 26 14:23:44 2012 +0300

    usb: gadget: let gadgets control pullup on their own
    
    This is useful on gadgets that depend on userland
    daemons to function properly. We can delay connection
    to the host until userland is ready.
    
    Signed-off-by: Felipe Balbi <balbi@xxxxxx>

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 7c821de8ce3d..790ccf29f2ee 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1784,8 +1784,9 @@ int usb_composite_probe(struct usb_composite_driver *driver)
 		driver->name = "composite";
 
 	driver->gadget_driver = composite_driver_template;
-	gadget_driver = &driver->gadget_driver;
 
+	gadget_driver = &driver->gadget_driver;
+	gadget_driver->controls_pullups = driver->controls_pullups;
 	gadget_driver->function =  (char *) driver->name;
 	gadget_driver->driver.name = driver->name;
 	gadget_driver->max_speed = driver->max_speed;
diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
index 8a1eeb24ae6a..c0f4fca9384b 100644
--- a/drivers/usb/gadget/udc-core.c
+++ b/drivers/usb/gadget/udc-core.c
@@ -235,7 +235,18 @@ static void usb_gadget_remove_driver(struct usb_udc *udc)
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 
-	usb_gadget_disconnect(udc->gadget);
+	/*
+	 * NOTICE: if gadget driver wants to control
+	 * pullup, it needs to make sure that when
+	 * user tries to rmmod the gadget driver, it
+	 * will disconnect the pullups before returning
+	 * from its ->unbind() method.
+	 *
+	 * We are truly trusting the gadget driver here.
+	 */
+	if (!udc->driver->controls_pullups)
+		usb_gadget_disconnect(udc->gadget);
+
 	udc->driver->disconnect(udc->gadget);
 	udc->driver->unbind(udc->gadget);
 	usb_gadget_udc_stop(udc->gadget, udc->driver);
@@ -300,7 +311,18 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri
 		driver->unbind(udc->gadget);
 		goto err1;
 	}
-	usb_gadget_connect(udc->gadget);
+
+	/*
+	 * NOTICE: if gadget driver wants to control
+	 * pullups, it needs to make sure its calls
+	 * to usb_function_activate() and
+	 * usb_function_deactivate() are balanced,
+	 * otherwise gadget_driver will never enumerate.
+	 *
+	 * We are truly trusting the gadget driver here.
+	 */
+	if (!driver->controls_pullups)
+		usb_gadget_connect(udc->gadget);
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 	return 0;
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 3c671c1b37f6..7ae797c85cb9 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -157,6 +157,7 @@ struct usb_function {
 	int			(*get_status)(struct usb_function *);
 	int			(*func_suspend)(struct usb_function *,
 						u8 suspend_opt);
+
 	/* private: */
 	/* internals */
 	struct list_head		list;
@@ -279,6 +280,8 @@ enum {
  * @max_speed: Highest speed the driver supports.
  * @needs_serial: set to 1 if the gadget needs userspace to provide
  * 	a serial number.  If one is not provided, warning will be printed.
+ * @controls_pullups: this driver will control pullup and udc-core shouldn't
+ *	enable it by default
  * @bind: (REQUIRED) Used to allocate resources that are shared across the
  *	whole device, such as string IDs, and add its configurations using
  *	@usb_add_config(). This may fail by returning a negative errno
@@ -308,6 +311,7 @@ struct usb_composite_driver {
 	struct usb_gadget_strings		**strings;
 	enum usb_device_speed			max_speed;
 	unsigned		needs_serial:1;
+	unsigned		controls_pullups:1;
 
 	int			(*bind)(struct usb_composite_dev *cdev);
 	int			(*unbind)(struct usb_composite_dev *);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 32b734d88d6b..87971fa38f08 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -774,6 +774,7 @@ static inline int usb_gadget_disconnect(struct usb_gadget *gadget)
  * @suspend: Invoked on USB suspend.  May be called in_interrupt.
  * @resume: Invoked on USB resume.  May be called in_interrupt.
  * @driver: Driver model state for this driver.
+ * @controls_pullups: tells udc-core to not enable pullups by default
  *
  * Devices are disabled till a gadget driver successfully bind()s, which
  * means the driver will handle setup() requests needed to enumerate (and
@@ -833,6 +834,8 @@ struct usb_gadget_driver {
 
 	/* FIXME support safe rmmod */
 	struct device_driver	driver;
+
+	unsigned		controls_pullups:1;
 };
 
 

Together with that, I wrote:

commit 0b733885e276a38cc9fa415c0977f063f9ce4d9d
Author: Felipe Balbi <balbi@xxxxxx>
Date:   Wed Feb 6 12:34:33 2013 +0200

    usb: gadget: webcam: let it control pullups
    
    this gadget driver needs to make sure that
    we will only enumerate after its userland
    counterpart is up and running.
    
    Signed-off-by: Felipe Balbi <balbi@xxxxxx>

diff --git a/drivers/usb/gadget/webcam.c b/drivers/usb/gadget/webcam.c
index 8cef1e658c29..41a4d03715bc 100644
--- a/drivers/usb/gadget/webcam.c
+++ b/drivers/usb/gadget/webcam.c
@@ -388,6 +388,7 @@ static __refdata struct usb_composite_driver webcam_driver = {
 	.max_speed	= USB_SPEED_SUPER,
 	.bind		= webcam_bind,
 	.unbind		= webcam_unbind,
+	.controls_pullups = true,
 };
 
 static int __init

This makes it really simple, either a gadget driver wants to control
pullups, or it doesn't. For those who wish to control pullups (for
whatever reason) we rely completely on the gadget telling us it's ok to
connect pullups by means of usb_function_activate()/deactivate(), for
all others, we just connect straight away.

cheers

-- 
balbi

Attachment: signature.asc
Description: Digital signature