Hi,
On 17 October 2016 at 18:10, Felipe Balbi <balbi@xxxxxxxxxx> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@xxxxxxxxxx> 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.wang@xxxxxxxxxx>
>
> 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.wang@xxxxxxxxxx>
> 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.balbi@xxxxxxxxxxxxxxx: 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.wang@xxxxxxxxxx>
> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
> ---
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
