Hi,

On 17 October 2016 at 18:10, Felipe Balbi <ba...@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.w...@linaro.org> writes:
>> When we change the USB function with configfs dynamically, we possibly met 
>> this
>> situation: one core is doing the control transfer, another core is trying to
>> unregister the USB gadget from userspace, we must wait for completing this
>> control tranfer, or it will hang the controller to set the DEVCTRLHLT flag.
>>
>> Signed-off-by: Baolin Wang <baolin.w...@linaro.org>
>
> Can you make sure this still works?

With applying this patch, It can work well on my platform, but I have
some worries about the risk of accessing 'dwc->ep0state' without lock
protection in dwc3_gadget_pullup() function.

>
> 8<------------------------------------------------------------------------
> From 797c7caab630f650b9ab5e462e8588462e055073 Mon Sep 17 00:00:00 2001
> From: Baolin Wang <baolin.w...@linaro.org>
> Date: Fri, 14 Oct 2016 17:11:33 +0800
> Subject: [PATCH] usb: dwc3: gadget: don't clear RUN/STOP when it's invalid to
>  do so
>
> When we change the USB function with configfs dynamically, we possibly
> met this situation: one core is doing the control transfer, another core
> is trying to unregister the USB gadget from userspace, we must wait for
> completing this control tranfer, or it will hang the controller to set
> the DEVCTRLHLT flag.
>
> [ felipe.ba...@linux.intel.com: several fixes to the patch
>         - call complete() before starting following SETUP transfer
>         - add a macro for ep0_in_setup's timeout
>         - change commit subject slightly
>         - break lines at 72 characters (git adds an 8-character tab)
>         - avoid changes to dwc3_gadget_run_stop() ]
>
> Signed-off-by: Baolin Wang <baolin.w...@linaro.org>
> Signed-off-by: Felipe Balbi <felipe.ba...@linux.intel.com>
> ---
>  drivers/usb/dwc3/core.h   |  3 +++
>  drivers/usb/dwc3/ep0.c    |  2 ++
>  drivers/usb/dwc3/gadget.c | 17 +++++++++++++++++
>  3 files changed, 22 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 2f6f7c4bc8d4..80e4e9aa2d33 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -38,6 +38,7 @@
>  #define DWC3_MSG_MAX   500
>
>  /* Global constants */
> +#define DWC3_PULL_UP_TIMEOUT   500     /* ms */
>  #define DWC3_ZLP_BUF_SIZE      1024    /* size of a superspeed bulk */
>  #define DWC3_EP0_BOUNCE_SIZE   512
>  #define DWC3_ENDPOINTS_NUM     32
> @@ -756,6 +757,7 @@ struct dwc3_scratchpad_array {
>   * @ep0_usb_req: dummy req used while handling STD USB requests
>   * @ep0_bounce_addr: dma address of ep0_bounce
>   * @scratch_addr: dma address of scratchbuf
> + * @ep0_in_setup: one control transfer is completed and enter setup phase
>   * @lock: for synchronizing
>   * @dev: pointer to our struct device
>   * @xhci: pointer to our xHCI child
> @@ -853,6 +855,7 @@ struct dwc3 {
>         dma_addr_t              ep0_bounce_addr;
>         dma_addr_t              scratch_addr;
>         struct dwc3_request     ep0_usb_req;
> +       struct completion       ep0_in_setup;
>
>         /* device lock */
>         spinlock_t              lock;
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 5e642d65a3b2..417cfd3f04ab 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -284,6 +284,8 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>  {
>         int                             ret;
>
> +       complete(&dwc->ep0_in_setup);
> +
>         ret = dwc3_ep0_start_trans(dwc, 0, dwc->ctrl_req_addr, 8,
>                         DWC3_TRBCTL_CONTROL_SETUP, false);
>         WARN_ON(ret < 0);
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index b53712cbc499..da79716be50d 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1557,6 +1557,21 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, 
> int is_on)
>
>         is_on = !!is_on;
>
> +       /*
> +        * Per databook, when we want to stop the gadget, if a control 
> transfer
> +        * is still in process, complete it and get the core into setup phase.
> +        */
> +       if (!is_on && dwc->ep0state != EP0_SETUP_PHASE) {
> +               reinit_completion(&dwc->ep0_in_setup);
> +
> +               ret = wait_for_completion_timeout(&dwc->ep0_in_setup,
> +                               msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT));
> +               if (ret == 0) {
> +                       dev_err(dwc->dev, "timed out waiting for SETUP 
> phase\n");
> +                       return -ETIMEDOUT;
> +               }
> +       }
> +
>         spin_lock_irqsave(&dwc->lock, flags);
>         ret = dwc3_gadget_run_stop(dwc, is_on, false);
>         spin_unlock_irqrestore(&dwc->lock, flags);
> @@ -2954,6 +2969,8 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>                 goto err4;
>         }
>
> +       init_completion(&dwc->ep0_in_setup);
> +
>         dwc->gadget.ops                 = &dwc3_gadget_ops;
>         dwc->gadget.speed               = USB_SPEED_UNKNOWN;
>         dwc->gadget.sg_supported        = true;
> --
> 2.10.0.440.g21f862b
>
>
>
> --
> balbi



-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to