Re: [PATCH v4] usb: dwc3: Stop active transfers before halting the controller

From: Wesley Cheng
Date: Wed Nov 18 2020 - 14:02:15 EST

Next message: Rafael J. Wysocki: "[PATCH] cpufreq: Fix up several kerneldoc comments"
Previous message: Stefan Chulski: "RE: [PATCH] net: mvpp2: divide fifo for dts-active ports only"
In reply to: Michael Tretter: "Re: [PATCH v4] usb: dwc3: Stop active transfers before halting the controller"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 11/18/2020 2:14 AM, Michael Tretter wrote:
> Hello,
>
> On Mon, 28 Sep 2020 17:20:59 -0700, Wesley Cheng wrote:
>> In the DWC3 databook, for a device initiated disconnect or bus reset, the
>> driver is required to send dependxfer commands for any pending transfers.
>> In addition, before the controller can move to the halted state, the SW
>> needs to acknowledge any pending events. If the controller is not halted
>> properly, there is a chance the controller will continue accessing stale or
>> freed TRBs and buffers.
>
> This patch causes a regression when using the uvc gadget with the dwc3 gadget
> driver, which causes host to not be able to enumerate the USB device.
>
> The regression can be reproduced as follows:
>
> Configure the uvc gadget via configfs, which in the end binds to the driver
> and calls dwc3_gadget_start. Start the uvc-gadget user space application,
> which activates the function and the gadget and calls pullup enable. The UVC
> Device is now detected by a USB host.
>
> Stop the uvc gadget application, which deactivates the gadget, calls pullup
> disable and, thus, stops the dwc3 gadget.
>
Hi Michael,

Thanks for the analysis. I think specifically, the f_uvc will use the
usb_function_deactivate() API to disable the pullup (w/o calling
udc_stop()) and using usb_function_activate() to do the opposite. These
are triggered when an application opens/closes the V4L2 device. (your
application)

> Restart the uvc gadget application; the gadget is activated and pullup enable
> is called, but the dwc3 gadget is not started.
>

Seems like the deactivate/activate calls avoid any UDC start stop
operations, and only use the pullup executions to issue a soft disconnect.

> The USB Host shows the following error messages and the USB device cannot be
> enumerated.
>
> usb 3-1.1: Device not responding to setup address.
> usb 3-1.1: Device not responding to setup address.
> usb 3-1.1: device not accepting address 10, error -71
> usb 3-1.1: Device not responding to setup address.
> usb 3-1.1: Device not responding to setup address.
> usb 3-1.1: device not accepting address 11, error -71
> usb 3-1-port1: unable to enumerate USB device
>
>>
>> Signed-off-by: Wesley Cheng <wcheng@xxxxxxxxxxxxxx>
>> Reviewed-by: Thinh Nguyen <thinhn@xxxxxxxxxxxx>
>>
>> ---
>> Changes in v4:
>> - Updated comments to reference DWC3 databook sections and added direct
>> quotes.
>> - Changed the stop active transfer EP loop to use dwc->num_eps.
>> - Moved to using dwc3_gadget_disable_irq/synchronize_irq instead of
>> enable_irq/disable_irq for ensuring the interrupt handler is not pending.
>>
>> Changes in v3:
>> - Removed DWC3_EP_ENABLED check from dwc3_gadget_stop_active_transfers()
>> as dwc3_stop_active_transfer() has a check already in place.
>> - Calling __dwc3_gadget_stop() which ensures that DWC3 interrupt events
>> are cleared, and ep0 eps are cleared for the pullup disabled case. Not
>> required to call __dwc3_gadget_start() on pullup enable, as the
>> composite driver will execute udc_start() before calling pullup().
>
> This change seems to be related to the regression. Maybe it is required to
> call __dwc3_gadget_start() on pullup enable, but I am not sure, how this
> should be handled.
>
> Michael
>

Definitely, in this situation, we would not call the udc_start()
callback, which will affect the functionality after re-enabling the
device. I wonder why the deactivate/activate routines don't explicitly
need to start/stop the UDC?

Thanks
Wesley Cheng

>>
>> Changes in v2:
>> - Moved cleanup code to the pullup() API to differentiate between device
>> disconnect and hibernation.
>> - Added cleanup code to the bus reset case as well.
>> - Verified the move to pullup() did not reproduce the problen using the
>> same test sequence.
>>
>> Verified fix by adding a check for ETIMEDOUT during the run stop call.
>> Shell script writing to the configfs UDC file to trigger disconnect and
>> connect. Batch script to have PC execute data transfers over adb (ie adb
>> push) After a few iterations, we'd run into a scenario where the
>> controller wasn't halted. With the following change, no failed halts after
>> many iterations.
>> ---
>> drivers/usb/dwc3/ep0.c | 2 +-
>> drivers/usb/dwc3/gadget.c | 66 ++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 66 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index 59f2e8c31bd1..456aa87e8778 100644
>> --- a/drivers/usb/dwc3/ep0.c
>> +++ b/drivers/usb/dwc3/ep0.c
>> @@ -197,7 +197,7 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
>> int ret;
>>
>> spin_lock_irqsave(&dwc->lock, flags);
>> - if (!dep->endpoint.desc) {
>> + if (!dep->endpoint.desc || !dwc->pullups_connected) {
>> dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
>> dep->name);
>> ret = -ESHUTDOWN;
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 3ab6f118c508..5d879b7606d5 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1516,7 +1516,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>> {
>> struct dwc3 *dwc = dep->dwc;
>>
>> - if (!dep->endpoint.desc) {
>> + if (!dep->endpoint.desc || !dwc->pullups_connected) {
>> dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
>> dep->name);
>> return -ESHUTDOWN;
>> @@ -1926,6 +1926,21 @@ static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,
>> return 0;
>> }
>>
>> +static void dwc3_stop_active_transfers(struct dwc3 *dwc)
>> +{
>> + u32 epnum;
>> +
>> + for (epnum = 2; epnum < dwc->num_eps; epnum++) {
>> + struct dwc3_ep *dep;
>> +
>> + dep = dwc->eps[epnum];
>> + if (!dep)
>> + continue;
>> +
>> + dwc3_remove_requests(dwc, dep);
>> + }
>> +}
>> +
>> static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
>> {
>> u32 reg;
>> @@ -1971,6 +1986,9 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
>> return 0;
>> }
>>
>> +static void dwc3_gadget_disable_irq(struct dwc3 *dwc);
>> +static void __dwc3_gadget_stop(struct dwc3 *dwc);
>> +
>> static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>> {
>> struct dwc3 *dwc = gadget_to_dwc(g);
>> @@ -1994,7 +2012,46 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>> }
>> }
>>
>> + /*
>> + * Synchronize any pending event handling before executing the controller
>> + * halt routine.
>> + */
>> + if (!is_on) {
>> + dwc3_gadget_disable_irq(dwc);
>> + synchronize_irq(dwc->irq_gadget);
>> + }
>> +
>> spin_lock_irqsave(&dwc->lock, flags);
>> +
>> + if (!is_on) {
>> + u32 count;
>> +
>> + /*
>> + * In the Synopsis DesignWare Cores USB3 Databook Rev. 3.30a
>> + * Section 4.1.8 Table 4-7, it states that for a device-initiated
>> + * disconnect, the SW needs to ensure that it sends "a DEPENDXFER
>> + * command for any active transfers" before clearing the RunStop
>> + * bit.
>> + */
>> + dwc3_stop_active_transfers(dwc);
>> + __dwc3_gadget_stop(dwc);
>> +
>> + /*
>> + * In the Synopsis DesignWare Cores USB3 Databook Rev. 3.30a
>> + * Section 1.3.4, it mentions that for the DEVCTRLHLT bit, the
>> + * "software needs to acknowledge the events that are generated
>> + * (by writing to GEVNTCOUNTn) while it is waiting for this bit
>> + * to be set to '1'."
>> + */
>> + count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
>> + count &= DWC3_GEVNTCOUNT_MASK;
>> + if (count > 0) {
>> + dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count);
>> + dwc->ev_buf->lpos = (dwc->ev_buf->lpos + count) %
>> + dwc->ev_buf->length;
>> + }
>> + }
>> +
>> ret = dwc3_gadget_run_stop(dwc, is_on, false);
>> spin_unlock_irqrestore(&dwc->lock, flags);
>>
>> @@ -3100,6 +3157,13 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
>> }
>>
>> dwc3_reset_gadget(dwc);
>> + /*
>> + * In the Synopsis DesignWare Cores USB3 Databook Rev. 3.30a
>> + * Section 4.1.2 Table 4-2, it states that during a USB reset, the SW
>> + * needs to ensure that it sends "a DEPENDXFER command for any active
>> + * transfers."
>> + */
>> + dwc3_stop_active_transfers(dwc);
>>
>> reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>> reg &= ~DWC3_DCTL_TSTCTRL_MASK;
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
>>

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Next message: Rafael J. Wysocki: "[PATCH] cpufreq: Fix up several kerneldoc comments"
Previous message: Stefan Chulski: "RE: [PATCH] net: mvpp2: divide fifo for dts-active ports only"
In reply to: Michael Tretter: "Re: [PATCH v4] usb: dwc3: Stop active transfers before halting the controller"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]