[PATCH v2 0/5] usb: gadget: Fix gadget deactivaton feature

[PATCH v2 0/5] usb: gadget: Fix gadget deactivaton feature

[Robert Baldyga]

Hi,

This patch set introduces two functions usb_gadget_deactivate() and
usb_gadget_activate(), designed to prevent udc-core from showing binded
gadget to host until it will be ready to work. It also makes
usb_function_deactivate()/activate() using these functions.

So far gadget deactivation was made by calling usb_gadget_disconnect(),
but since we have usb_gadget_connect() called after gadget->bind()
(in udc_bind_to_driver()) this method doesn't provide expected result.
Calling function usb_gadget_disconnect() before gadget connection doesn't
prevent udc-core from connecting gadget to driver - usb_gadget_disconnect()
call is ignored and gadget is connected regardless to it. This usually
results with errors, for example because we binded gadget with 0
configurations.

This patch set fixes this problem adding functions allowing to perform
effective gadget deactivation from gadget->bind().

According to Felipe's suggestion, in v2 there is one new patch adding
'bind_deactivated' flag, which should be used by functions which want
to be binded as deactivated (for example because they need to wait for
userspace daemon). Functions using this flag have to call
usb_function_activate() to tell composite core they are ready to work.
I have also converted functions which are using deactivation feature
to make them using 'bind_deactivated' flag. Patches are also attached
to this patch set.

I was considering removing usb_function_deactivate() function since we
have this flag, but I see that some of USB functions use it not only
in bind() but also e.g. when userspace daemon disconnects. This is also
the reason why I haven't named this flag 'controls_pullup', because this
name doesn't describe precisely what does it mean.

We also need to consider what to do when deactivation fails (which can
happen if our UDC driver doesn't support the pullup callback). So far,
when usb_function_deactivate() was called in bind(), we have had ability
to decide what to do, when it fails. Now we preform function deactivation
before its bind(), and when deactivation fails we fail to add the function
to gadget. Maybe we should to allow it to continue despite deactivation
failure and inform function (somehow) about this situation. If only it
makes any sense, because in this form it looks more complicated that
it was, and moreover it actually doesn't add any new features.

Best regards,
Robert Baldyga

Changelog:

v2:
- add 'bind_deactivated' flag

v1: https://lkml.org/lkml/2015/4/7/92

Robert Baldyga (5):
  usb: gadget: add usb_gadget_activate/deactivate functions
  usb: composite: fix usb_function_activate/deactivate functions
  usb: composite: add bind_deactivated flag to usb_function
  usb: gadget: f_uvc: use bind_deactivated flag
  usb: gadget: f_obex: use bind_deactivated flag

 drivers/usb/gadget/composite.c       |  10 +++-
 drivers/usb/gadget/function/f_obex.c |  19 +------
 drivers/usb/gadget/function/f_uvc.c  |   7 +--
 include/linux/usb/composite.h        |   2 +
 include/linux/usb/gadget.h           | 100 ++++++++++++++++++++++++++++++++---
 5 files changed, 106 insertions(+), 32 deletions(-)

-- 
1.9.1

[PATCH v2 1/5] usb: gadget: add usb_gadget_activate/deactivate functions

[Robert Baldyga]

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.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 include/linux/usb/gadget.h | 100 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 94 insertions(+), 6 deletions(-)

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 4f3dfb7..15604bb 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -526,6 +526,9 @@ struct usb_gadget_ops {
  * @quirk_ep_out_aligned_size: epout requires buffer size to be aligned to
  *	MaxPacketSize.
  * @is_selfpowered: if the gadget is self-powered.
+ * @deactivated: True if gadget is deactivated - in deactivated state it cannot
+ *	be connected.
+ * @connected: True if gadget is connected.
  *
  * Gadgets have a mostly-portable "gadget driver" implementing device
  * functions, handling all usb configurations and interfaces.  Gadget
@@ -568,6 +571,8 @@ struct usb_gadget {
 	unsigned			a_alt_hnp_support:1;
 	unsigned			quirk_ep_out_aligned_size:1;
 	unsigned			is_selfpowered:1;
+	unsigned			deactivated:1;
+	unsigned			connected:1;
 };
 #define work_to_gadget(w)	(container_of((w), struct usb_gadget, work))
 
@@ -771,9 +776,24 @@ static inline int usb_gadget_vbus_disconnect(struct usb_gadget *gadget)
  */
 static inline int usb_gadget_connect(struct usb_gadget *gadget)
 {
+	int ret;
+
 	if (!gadget->ops->pullup)
 		return -EOPNOTSUPP;
-	return gadget->ops->pullup(gadget, 1);
+
+	if (gadget->deactivated) {
+		/*
+		 * If gadget is deactivated we only save new state.
+		 * Gadget will be connected automatically after activation.
+		 */
+		gadget->connected = true;
+		return 0;
+	}
+
+	ret = gadget->ops->pullup(gadget, 1);
+	if (!ret)
+		gadget->connected = 1;
+	return ret;
 }
 
 /**
@@ -784,20 +804,88 @@ static inline int usb_gadget_connect(struct usb_gadget *gadget)
  * as a disconnect (when a VBUS session is active).  Not all systems
  * support software pullup controls.
  *
+ * Returns zero on success, else negative errno.
+ */
+static inline int usb_gadget_disconnect(struct usb_gadget *gadget)
+{
+	int ret;
+
+	if (!gadget->ops->pullup)
+		return -EOPNOTSUPP;
+
+	if (gadget->deactivated) {
+		/*
+		 * If gadget is deactivated we only save new state.
+		 * Gadget will stay disconnected after activation.
+		 */
+		gadget->connected = false;
+		return 0;
+	}
+
+	ret = gadget->ops->pullup(gadget, 0);
+	if (!ret)
+		gadget->connected = 0;
+	return ret;
+}
+
+/**
+ * usb_gadget_deactivate - deactivate function which is not ready to work
+ * @gadget: the peripheral being deactivated
+ *
  * This routine may be used during the gadget driver bind() call to prevent
  * the peripheral from ever being visible to the USB host, unless later
- * usb_gadget_connect() is called.  For example, user mode components may
+ * usb_gadget_activate() is called.  For example, user mode components may
  * need to be activated before the system can talk to hosts.
  *
  * Returns zero on success, else negative errno.
  */
-static inline int usb_gadget_disconnect(struct usb_gadget *gadget)
+static inline int usb_gadget_deactivate(struct usb_gadget *gadget)
 {
-	if (!gadget->ops->pullup)
-		return -EOPNOTSUPP;
-	return gadget->ops->pullup(gadget, 0);
+	int ret;
+
+	if (gadget->deactivated)
+		return 0;
+
+	if (gadget->connected) {
+		ret = usb_gadget_disconnect(gadget);
+		if (ret)
+			return ret;
+		/*
+		 * If gadget was being connected before deactivation, we want
+		 * to reconnect it in usb_gadget_activate().
+		 */
+		gadget->connected = true;
+	}
+	gadget->deactivated = true;
+
+	return 0;
 }
 
+/**
+ * usb_gadget_activate - activate function which is not ready to work
+ * @gadget: the peripheral being activated
+ *
+ * This routine activates gadget which was previously deactivated with
+ * usb_gadget_deactivate() call. It calls usb_gadget_connect() if needed.
+ *
+ * Returns zero on success, else negative errno.
+ */
+static inline int usb_gadget_activate(struct usb_gadget *gadget)
+{
+	if (!gadget->deactivated)
+		return 0;
+
+	gadget->deactivated = false;
+
+	/*
+	 * If gadget has been connected before deactivation, or became connected
+	 * while it was being deactivated, we call usb_gadget_connect().
+	 */
+	if (gadget->connected)
+		return usb_gadget_connect(gadget);
+
+	return 0;
+}
 
 /*-------------------------------------------------------------------------*/
 
-- 
1.9.1

[PATCH v2 2/5] usb: composite: fix usb_function_activate/deactivate functions

[Robert Baldyga]

Using usb_gadget_disconnect to make gadget temporarily invisible to host
doesn't provide desired result, because gadget is connected immediately
after binding regardless to previous usb_gadget_disconnect() calls.

For this reason we use usb_gadget_deactivate() instead of
usb_gadget_disconnect() to make it working as expected.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/gadget/composite.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 4e3447b..9ed77f4 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -279,7 +279,7 @@ int usb_function_deactivate(struct usb_function *function)
 	spin_lock_irqsave(&cdev->lock, flags);
 
 	if (cdev->deactivations == 0)
-		status = usb_gadget_disconnect(cdev->gadget);
+		status = usb_gadget_deactivate(cdev->gadget);
 	if (status == 0)
 		cdev->deactivations++;
 
@@ -311,7 +311,7 @@ int usb_function_activate(struct usb_function *function)
 	else {
 		cdev->deactivations--;
 		if (cdev->deactivations == 0)
-			status = usb_gadget_connect(cdev->gadget);
+			status = usb_gadget_activate(cdev->gadget);
 	}
 
 	spin_unlock_irqrestore(&cdev->lock, flags);
-- 
1.9.1

[PATCH v2 3/5] usb: composite: add bind_deactivated flag to usb_function

[Robert Baldyga]

This patch introduces 'bind_deactivated' flag in struct usb_function.
Functions which don't want to be activated automatically after bind should
set this flag, and when they start to be ready to work they should call
usb_function_activate().

When USB function sets 'bind_deactivated' flag, initial deactivation
counter is incremented automatically, so there is no need to call
usb_function_deactivate() in function bind.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/gadget/composite.c | 6 ++++++
 include/linux/usb/composite.h  | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 9ed77f4..cdb9a5c 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -209,6 +209,12 @@ int usb_add_function(struct usb_configuration *config,
 	function->config = config;
 	list_add_tail(&function->list, &config->functions);
 
+	if (function->bind_deactivated) {
+		value = usb_function_deactivate(function);
+		if (value)
+			goto done;
+	}
+
 	/* REVISIT *require* function->bind? */
 	if (function->bind) {
 		value = function->bind(config, function);
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 2511469..1074b89 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -228,6 +228,8 @@ struct usb_function {
 	struct list_head		list;
 	DECLARE_BITMAP(endpoints, 32);
 	const struct usb_function_instance *fi;
+
+	unsigned int		bind_deactivated:1;
 };
 
 int usb_add_function(struct usb_configuration *, struct usb_function *);
-- 
1.9.1

[PATCH v2 4/5] usb: gadget: f_uvc: use bind_deactivated flag

[Robert Baldyga]

Use bind_deactivated flag instead of calling usb_function_deactivate()
in function bind().

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/gadget/function/f_uvc.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index cf0df8f..743be34 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -733,12 +733,6 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
 	uvc->control_req->complete = uvc_function_ep0_complete;
 	uvc->control_req->context = uvc;
 
-	/* Avoid letting this gadget enumerate until the userspace server is
-	 * active.
-	 */
-	if ((ret = usb_function_deactivate(f)) < 0)
-		goto error;
-
 	if (v4l2_device_register(&cdev->gadget->dev, &uvc->v4l2_dev)) {
 		printk(KERN_INFO "v4l2_device_register failed\n");
 		goto error;
@@ -949,6 +943,7 @@ static struct usb_function *uvc_alloc(struct usb_function_instance *fi)
 	uvc->func.disable = uvc_function_disable;
 	uvc->func.setup = uvc_function_setup;
 	uvc->func.free_func = uvc_free;
+	uvc->func.bind_deactivated = true;
 
 	return &uvc->func;
 }
-- 
1.9.1

[PATCH v2 5/5] usb: gadget: f_obex: use bind_deactivated flag

[Robert Baldyga]

Use bind_deactivated flag instead of calling usb_function_deactivate()
in function bind(). Field 'can_activate' in struct f_obex is no longer
needed as setting 'bind_deactivated' flag makes us sure, that the function
will be binded only if deactivation can be performed successfully.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/gadget/function/f_obex.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/drivers/usb/gadget/function/f_obex.c b/drivers/usb/gadget/function/f_obex.c
index a1b79c5..5519f85 100644
--- a/drivers/usb/gadget/function/f_obex.c
+++ b/drivers/usb/gadget/function/f_obex.c
@@ -37,7 +37,6 @@ struct f_obex {
 	u8				data_id;
 	u8				cur_alt;
 	u8				port_num;
-	u8				can_activate;
 };
 
 static inline struct f_obex *func_to_obex(struct usb_function *f)
@@ -268,9 +267,6 @@ static void obex_connect(struct gserial *g)
 	struct usb_composite_dev *cdev = g->func.config->cdev;
 	int			status;
 
-	if (!obex->can_activate)
-		return;
-
 	status = usb_function_activate(&g->func);
 	if (status)
 		dev_dbg(&cdev->gadget->dev,
@@ -284,9 +280,6 @@ static void obex_disconnect(struct gserial *g)
 	struct usb_composite_dev *cdev = g->func.config->cdev;
 	int			status;
 
-	if (!obex->can_activate)
-		return;
-
 	status = usb_function_deactivate(&g->func);
 	if (status)
 		dev_dbg(&cdev->gadget->dev,
@@ -378,17 +371,6 @@ static int obex_bind(struct usb_configuration *c, struct usb_function *f)
 	if (status)
 		goto fail;
 
-	/* Avoid letting this gadget enumerate until the userspace
-	 * OBEX server is active.
-	 */
-	status = usb_function_deactivate(f);
-	if (status < 0)
-		WARNING(cdev, "obex ttyGS%d: can't prevent enumeration, %d\n",
-			obex->port_num, status);
-	else
-		obex->can_activate = true;
-
-
 	dev_dbg(&cdev->gadget->dev, "obex ttyGS%d: %s speed IN/%s OUT/%s\n",
 		obex->port_num,
 		gadget_is_dualspeed(c->cdev->gadget) ? "dual" : "full",
@@ -529,6 +511,7 @@ static struct usb_function *obex_alloc(struct usb_function_instance *fi)
 	obex->port.func.get_alt = obex_get_alt;
 	obex->port.func.disable = obex_disable;
 	obex->port.func.free_func = obex_free;
+	obex->port.func.bind_deactivated = true;
 
 	return &obex->port.func;
 }
-- 
1.9.1

Re: [PATCH v2 0/5] usb: gadget: Fix gadget deactivaton feature

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2730 bytes --]

On Mon, May 04, 2015 at 02:55:10PM +0200, Robert Baldyga wrote:
> Hi,
> 
> This patch set introduces two functions usb_gadget_deactivate() and
> usb_gadget_activate(), designed to prevent udc-core from showing binded
> gadget to host until it will be ready to work. It also makes
> usb_function_deactivate()/activate() using these functions.
> 
> So far gadget deactivation was made by calling usb_gadget_disconnect(),
> but since we have usb_gadget_connect() called after gadget->bind()
> (in udc_bind_to_driver()) this method doesn't provide expected result.
> Calling function usb_gadget_disconnect() before gadget connection doesn't
> prevent udc-core from connecting gadget to driver - usb_gadget_disconnect()
> call is ignored and gadget is connected regardless to it. This usually
> results with errors, for example because we binded gadget with 0
> configurations.
> 
> This patch set fixes this problem adding functions allowing to perform
> effective gadget deactivation from gadget->bind().
> 
> According to Felipe's suggestion, in v2 there is one new patch adding
> 'bind_deactivated' flag, which should be used by functions which want
> to be binded as deactivated (for example because they need to wait for
> userspace daemon). Functions using this flag have to call
> usb_function_activate() to tell composite core they are ready to work.
> I have also converted functions which are using deactivation feature
> to make them using 'bind_deactivated' flag. Patches are also attached
> to this patch set.
> 
> I was considering removing usb_function_deactivate() function since we
> have this flag, but I see that some of USB functions use it not only
> in bind() but also e.g. when userspace daemon disconnects. This is also
> the reason why I haven't named this flag 'controls_pullup', because this
> name doesn't describe precisely what does it mean.
> 
> We also need to consider what to do when deactivation fails (which can
> happen if our UDC driver doesn't support the pullup callback). So far,
> when usb_function_deactivate() was called in bind(), we have had ability
> to decide what to do, when it fails. Now we preform function deactivation
> before its bind(), and when deactivation fails we fail to add the function
> to gadget. Maybe we should to allow it to continue despite deactivation
> failure and inform function (somehow) about this situation. If only it
> makes any sense, because in this form it looks more complicated that
> it was, and moreover it actually doesn't add any new features.

I'll defer this series for v4.3 merge window because I'm still not
entirely convinced we need this new reference counter. Sorry about that.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 0/5] usb: gadget: Fix gadget deactivaton feature

[Robert Baldyga]

Hi Felipe,

On 05/26/2015 07:08 PM, Felipe Balbi wrote:
> On Mon, May 04, 2015 at 02:55:10PM +0200, Robert Baldyga wrote:
>> Hi,
>>
>> This patch set introduces two functions usb_gadget_deactivate() and
>> usb_gadget_activate(), designed to prevent udc-core from showing binded
>> gadget to host until it will be ready to work. It also makes
>> usb_function_deactivate()/activate() using these functions.
>>
>> So far gadget deactivation was made by calling usb_gadget_disconnect(),
>> but since we have usb_gadget_connect() called after gadget->bind()
>> (in udc_bind_to_driver()) this method doesn't provide expected result.
>> Calling function usb_gadget_disconnect() before gadget connection doesn't
>> prevent udc-core from connecting gadget to driver - usb_gadget_disconnect()
>> call is ignored and gadget is connected regardless to it. This usually
>> results with errors, for example because we binded gadget with 0
>> configurations.
>>
>> This patch set fixes this problem adding functions allowing to perform
>> effective gadget deactivation from gadget->bind().
>>
>> According to Felipe's suggestion, in v2 there is one new patch adding
>> 'bind_deactivated' flag, which should be used by functions which want
>> to be binded as deactivated (for example because they need to wait for
>> userspace daemon). Functions using this flag have to call
>> usb_function_activate() to tell composite core they are ready to work.
>> I have also converted functions which are using deactivation feature
>> to make them using 'bind_deactivated' flag. Patches are also attached
>> to this patch set.
>>
>> I was considering removing usb_function_deactivate() function since we
>> have this flag, but I see that some of USB functions use it not only
>> in bind() but also e.g. when userspace daemon disconnects. This is also
>> the reason why I haven't named this flag 'controls_pullup', because this
>> name doesn't describe precisely what does it mean.
>>
>> We also need to consider what to do when deactivation fails (which can
>> happen if our UDC driver doesn't support the pullup callback). So far,
>> when usb_function_deactivate() was called in bind(), we have had ability
>> to decide what to do, when it fails. Now we preform function deactivation
>> before its bind(), and when deactivation fails we fail to add the function
>> to gadget. Maybe we should to allow it to continue despite deactivation
>> failure and inform function (somehow) about this situation. If only it
>> makes any sense, because in this form it looks more complicated that
>> it was, and moreover it actually doesn't add any new features.
> 
> I'll defer this series for v4.3 merge window because I'm still not
> entirely convinced we need this new reference counter. Sorry about that.
> 

What reference counter did you mean?

Thanks,
Robert