RE: [RFC] usb: gadget: Ensure correct functionality for gadgets that wish to 'connect' only when directed by user-space

[Bhupesh SHARMA <bhupesh.sharma@xxxxxx>]

Hi,

> -----Original Message-----
> From: Bhupesh SHARMA
> Sent: Wednesday, June 27, 2012 9:08 AM
> To: balbi@xxxxxx; linux-usb@xxxxxxxxxxxxxxx
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; laurent.pinchart@xxxxxxxxxxxxxxxx
> Subject: [RFC] usb: gadget: Ensure correct functionality for gadgets
> that wish to 'connect' only when directed by user-space
> 
> Hi Felipe, All,
> 
> While working with the webcam gadget in recent past I came to realize
> that if the webcam gadget
> is compiled as a static part of the kernel, the USB webcam device when
> connected to the USB
> host will not successfully enumerate (as the USB host will send a
> GET_DEF class specific UVC
> request), which is handled by a user-space application responsible for
> ensuring command/data
> flow between the UVC gadget and a real V4L2 device.
> 
> I believe similar will be the case for other gadgets like obex while
> working with an underlying
> UDC during the start-up phase (during the enumeration process).
> 
> Gadgets like webcam/obex explicitly call the 'pullup' routine of the
> underlying
> UDC (with is_on argument set to 0) to issue soft-disconnect command at
> the very start. This is
> done to prevent the enumeration process to proceed unless a supporting
> user-space application
> is first up and running (which can handle the class-specific control
> requests on the basis of
> certain negotiations with other entities, like for e.g. a real video
> device in case of webcam).
> (see [1] for reference).
> 
> However with the current framework (newstyle UDC) just after the 'bind'
> of the
> function (f_uvc/f_obex) is called, the UDC framework calls the 'pullup'
> routine of the
> UDC again with a parameter is_on set to 1 to initiate connection with
> the Host.
> (see [2] for reference).
> 
> This seems to be an incorrect implementation as it causes the
> enumeration to
> start even for gadgets like webcam/obex which have explicitly requested
> only a soft-connect (as the user-space application has not been
> launched yet).
> 
> Please let me know your ideas on the same.
> At the moment I have used a temporary approach like the one given below
> (though it is in
> no way a very good way out :) ).
> 
> 
> [1] http://lxr.linux.no/linux+v3.4.4/drivers/usb/gadget/f_uvc.c#L548
> [2] http://lxr.linux.no/linux+v3.4.4/drivers/usb/gadget/udc-core.c#L341
> 
> 
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@xxxxxx>
> ---
>  drivers/usb/dwc3/core.h       |    1 +
>  drivers/usb/dwc3/gadget.c     |   24 ++++++++++++++++++------
>  drivers/usb/gadget/udc-core.c |   19 ++++++++++++++++++-
>  include/linux/usb/gadget.h    |    1 +
>  4 files changed, 38 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 2d8676c..9125ec9 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -712,6 +712,7 @@ struct dwc3 {
>  	struct dwc3_hwparams	hwparams;
>  	struct dentry		*root;
> +	bool			deactivated_at_start;
> 
>  	u8			test_mode;
>  	u8			test_mode_nr;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index b82fe61..0d6ea17 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1341,6 +1341,12 @@ static const struct usb_ep_ops
> dwc3_gadget_ep_ops = {
>  };
> 
>  /* -------------------------------------------------------------------
> ------- */
> +static bool dwc3_gadget_is_deactivated_at_start(struct usb_gadget *g)
> +{
> +	struct dwc3		*dwc = gadget_to_dwc(g);
> +
> +	return dwc->deactivated_at_start;
> +}
> 
>  static int dwc3_gadget_get_frame(struct usb_gadget *g)
>  {
> @@ -1448,6 +1454,7 @@ static void dwc3_gadget_run_stop(struct dwc3
> *dwc, int is_on)
>  {
>  	u32			reg;
>  	u32			timeout = 500;
> +	static bool		first_deactivation;
> 
>  	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>  	if (is_on) {
> @@ -1461,6 +1468,10 @@ static void dwc3_gadget_run_stop(struct dwc3
> *dwc, int is_on)
>  		reg |= DWC3_DCTL_RUN_STOP;
>  	} else {
>  		reg &= ~DWC3_DCTL_RUN_STOP;
> +		if (first_deactivation == false) {
> +			dwc->deactivated_at_start = true;
> +			first_deactivation = true;
> +		}
>  	}
> 
>  	dwc3_writel(dwc->regs, DWC3_DCTL, reg);
> @@ -1600,12 +1611,13 @@ static int dwc3_gadget_stop(struct usb_gadget
> *g,
>  }
> 
>  static const struct usb_gadget_ops dwc3_gadget_ops = {
> -	.get_frame		= dwc3_gadget_get_frame,
> -	.wakeup			= dwc3_gadget_wakeup,
> -	.set_selfpowered	= dwc3_gadget_set_selfpowered,
> -	.pullup			= dwc3_gadget_pullup,
> -	.udc_start		= dwc3_gadget_start,
> -	.udc_stop		= dwc3_gadget_stop,
> +	.is_deactivated_at_start	=
> dwc3_gadget_is_deactivated_at_start,
> +	.get_frame			= dwc3_gadget_get_frame,
> +	.wakeup				= dwc3_gadget_wakeup,
> +	.set_selfpowered		= dwc3_gadget_set_selfpowered,
> +	.pullup				= dwc3_gadget_pullup,
> +	.udc_start			= dwc3_gadget_start,
> +	.udc_stop			= dwc3_gadget_stop,
>  };
> 
>  /* -------------------------------------------------------------------
> ------- */
> diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-
> core.c
> index a6ebeec..c42ea92 100644
> --- a/drivers/usb/gadget/udc-core.c
> +++ b/drivers/usb/gadget/udc-core.c
> @@ -261,6 +261,13 @@ static int udc_is_newstyle(struct usb_udc *udc)
>  	return 0;
>  }
> 
> +static int udc_is_deactivated_at_start(struct usb_udc *udc)
> +{
> +	if (udc->gadget->ops->is_deactivated_at_start)
> +		return udc->gadget->ops->is_deactivated_at_start(udc-
> >gadget);
> +
> +	return 0;
> +}
> 
>  static void usb_gadget_remove_driver(struct usb_udc *udc)
>  {
> @@ -355,7 +362,17 @@ found:
>  			driver->unbind(udc->gadget);
>  			goto err1;
>  		}
> -		usb_gadget_connect(udc->gadget);
> +
> +		/*
> +		 * do not connect if the function has requested an
> +		 * explicit disconnect at the very start and hence
> +		 * does not want to connect unless a user-space application
> +		 * explicitly calls the 'function_activate' routine
> +		 * to connect (this is required in case of certain
> +		 * functions like obex and webcam).
> +		 */
> +		if (!udc_is_deactivated_at_start(udc))
> +			usb_gadget_connect(udc->gadget);
>  	} else {
> 
>  		ret = usb_gadget_start(udc->gadget, driver, bind);
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 665b1d1..4c4e665 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -507,6 +507,7 @@ struct usb_gadget_driver;
>   * which don't involve endpoints (or i/o).
>   */
>  struct usb_gadget_ops {
> +	bool	(*is_deactivated_at_start)(struct usb_gadget *);
>  	int	(*get_frame)(struct usb_gadget *);
>  	int	(*wakeup)(struct usb_gadget *);
>  	int	(*set_selfpowered) (struct usb_gadget *, int
> is_selfpowered);
> --
> 1.6.0.2
> 
> 

A much-better approach would be to modify the udc-core instead of
modifying each UDC controller driver. A sample patch which does the
same is given below. 

Please share your ideas/views whether this seems ok.


Signed-off-by: Bhupesh Sharma <bhupesh.sharma@xxxxxx>
Signed-off-by: Pratyush Anand <pratyush.anand@xxxxxx>
---
 drivers/usb/gadget/udc-core.c |    3 ++-
 include/linux/usb/gadget.h    |   10 +++++++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c index a6ebeec..4851d99 100644
--- a/drivers/usb/gadget/udc-core.c
+++ b/drivers/usb/gadget/udc-core.c
@@ -355,7 +355,8 @@ found:
 			driver->unbind(udc->gadget);
 			goto err1;
 		}
-		usb_gadget_connect(udc->gadget);
+		if (!udc->gadget->is_pulleddown)
+			usb_gadget_connect(udc->gadget);
 	} else {
 
 		ret = usb_gadget_start(udc->gadget, driver, bind); diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index 665b1d1..b1d997a 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -583,6 +583,7 @@ struct usb_gadget {
 	unsigned			b_hnp_enable:1;
 	unsigned			a_hnp_support:1;
 	unsigned			a_alt_hnp_support:1;
+	unsigned			is_pulleddown:1;
 	const char			*name;
 	struct device			dev;
 };
@@ -808,9 +809,16 @@ static inline int usb_gadget_connect(struct usb_gadget *gadget)
  */
 static inline int usb_gadget_disconnect(struct usb_gadget *gadget)  {
+	int ret;
+
 	if (!gadget->ops->pullup)
 		return -EOPNOTSUPP;
-	return gadget->ops->pullup(gadget, 0);
+	ret = gadget->ops->pullup(gadget, 0);
+
+	if (!ret)
+		gadget->is_pulleddown = true;
+
+	return ret;
 }
 
 
--
1.7.5.4

Regards,
Bhupesh
--
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