Re: [RFC PATCH 1/3] usb: dwc3: Flush pending SETUP data during stop active xfers

From: Wesley Cheng
Date: Thu Feb 10 2022 - 14:46:27 EST

Next message: Christophe JAILLET: "Re: [PATCH] i2c: bcm2835: Fix the error handling in 'bcm2835_i2c_probe()'"
Previous message: Liang, Kan: "Re: [PATCH kvm/queue v2 2/3] perf: x86/core: Add interface to query perfmon_event_map[] directly"
In reply to: Jack Pham: "Re: [RFC PATCH 1/3] usb: dwc3: Flush pending SETUP data during stop active xfers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Jack,

On 2/8/2022 9:33 AM, Jack Pham wrote:
> Hi Wesley,
>
> On Thu, Feb 03, 2022 at 12:00:15AM -0800, Wesley Cheng wrote:
>> While running the pullup disable sequence, if there are pending SETUP
>> transfers stored in the controller, then the endxfer commands will
>
> Should mention specifically that the endxfer commands *on non-EP0*
> endpoints will time out.
>
Thanks for the feedback and initial review. Done.
>
> Also let's use the same terminology as defined by the macros, i.e.
>
> endxfer/ENDXFER -> ENDTRANSFER
> STARTXFER -> STARTTRANSFER
>
>> fail w/ ETIMEDOUT. As a suggestion from SNPS, in order to drain the
>> SETUP packets, SW needs to issue a STARTXFER command. After issuing
> ^^^^ on EP0.
>
Sure will add explicit EP0 statement.

>> the STARTXFER, and retrying the ENDXFER, the command should succeed.
>> Else, if the endpoints are not properly stopped, the controller halt
>> sequence will fail as well.
>>
>> One limitation is that the current logic will drop the SETUP data
>> being received (ie dropping the SETUP packet), however, it should be
>> acceptable in the pullup disable case, as the device is eventually
>> going to disconnect from the host.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
>> ---
>> drivers/usb/dwc3/core.h | 7 +++++++
>> drivers/usb/dwc3/ep0.c | 21 ++++++++++++--------
>> drivers/usb/dwc3/gadget.c | 42 ++++++++++++++++++++++++++++++++++-----
>> 3 files changed, 57 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index e1cc3f7398fb..a124694c0038 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -1546,6 +1546,8 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd,
>> int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
>> u32 param);
>> void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc);
>> +void dwc3_ep0_stall_and_restart(struct dwc3 *dwc);
>> +void dwc3_ep0_end_control_data(struct dwc3 *dwc, struct dwc3_ep *dep);
>> #else
>> static inline int dwc3_gadget_init(struct dwc3 *dwc)
>> { return 0; }
>> @@ -1567,6 +1569,11 @@ static inline int dwc3_send_gadget_generic_command(struct dwc3 *dwc,
>> { return 0; }
>> static inline void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc)
>> { }
>> +static inline void dwc3_ep0_stall_and_restart(struct dwc3 *dwc)
>> +{ }
>> +static inline void dwc3_ep0_end_control_data(struct dwc3 *dwc,
>> + struct dwc3_ep *dep)
>> +{ }
>> #endif
>>
>> #if IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index 658739410992..eb677b888610 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 || !dwc->pullups_connected) {
>> + if (!dep->endpoint.desc || !dwc->pullups_connected || !dwc->connected) {
>> dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
>> dep->name);
>> ret = -ESHUTDOWN;
>> @@ -218,19 +218,21 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
>> return ret;
>> }
>>
>> -static void dwc3_ep0_stall_and_restart(struct dwc3 *dwc)
>> +void dwc3_ep0_stall_and_restart(struct dwc3 *dwc)
>> {
>> struct dwc3_ep *dep;
>>
>> /* reinitialize physical ep1 */
>> dep = dwc->eps[1];
>> dep->flags = DWC3_EP_ENABLED;
>> + dep->trb_enqueue = 0;
>>
>> /* stall is always issued on EP0 */
>> dep = dwc->eps[0];
>> __dwc3_gadget_ep_set_halt(dep, 1, false);
>> dep->flags = DWC3_EP_ENABLED;
>> dwc->delayed_status = false;
>> + dep->trb_enqueue = 0;
>>
>> if (!list_empty(&dep->pending_list)) {
>> struct dwc3_request *req;
>> @@ -240,7 +242,9 @@ static void dwc3_ep0_stall_and_restart(struct dwc3 *dwc)
>> }
>>
>> dwc->ep0state = EP0_SETUP_PHASE;
>> - dwc3_ep0_out_start(dwc);
>> + complete(&dwc->ep0_in_setup);
>> + if (dwc->softconnect)
>> + dwc3_ep0_out_start(dwc);
>> }
>>
>> int __dwc3_gadget_ep0_set_halt(struct usb_ep *ep, int value)
>> @@ -272,8 +276,6 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>> struct dwc3_ep *dep;
>> int ret;
>>
>> - complete(&dwc->ep0_in_setup);
>> -
>> dep = dwc->eps[0];
>> dwc3_ep0_prepare_one_trb(dep, dwc->ep0_trb_addr, 8,
>> DWC3_TRBCTL_CONTROL_SETUP, false);
>> @@ -922,7 +924,9 @@ static void dwc3_ep0_complete_status(struct dwc3 *dwc,
>> dwc->setup_packet_pending = true;
>>
>> dwc->ep0state = EP0_SETUP_PHASE;
>> - dwc3_ep0_out_start(dwc);
>> + complete(&dwc->ep0_in_setup);
>> + if (dwc->softconnect)
>> + dwc3_ep0_out_start(dwc);
>> }
>>
>> static void dwc3_ep0_xfer_complete(struct dwc3 *dwc,
>> @@ -1073,7 +1077,7 @@ void dwc3_ep0_send_delayed_status(struct dwc3 *dwc)
>> __dwc3_ep0_do_control_status(dwc, dwc->eps[direction]);
>> }
>>
>> -static void dwc3_ep0_end_control_data(struct dwc3 *dwc, struct dwc3_ep *dep)
>> +void dwc3_ep0_end_control_data(struct dwc3 *dwc, struct dwc3_ep *dep)
>> {
>> struct dwc3_gadget_ep_cmd_params params;
>> u32 cmd;
>> @@ -1083,7 +1087,8 @@ static void dwc3_ep0_end_control_data(struct dwc3 *dwc, struct dwc3_ep *dep)
>> return;
>>
>> cmd = DWC3_DEPCMD_ENDTRANSFER;
>> - cmd |= DWC3_DEPCMD_CMDIOC;
>> + cmd |= dwc->connected ? 0 : DWC3_DEPCMD_HIPRI_FORCERM;
>> + cmd |= dwc->connected ? DWC3_DEPCMD_CMDIOC : 0;
>
> Can this be combined?
>
Agreed. Will combine them.

Thanks
Wesley Cheng

> cmd |= dwc->connected ? DWC3_DEPCMD_CMDIOC : DWC3_DEPCMD_HIPRI_FORCERM;
>
>> cmd |= DWC3_DEPCMD_PARAM(dep->resource_index);
>> memset(&params, 0, sizeof(params));
>> ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 520031ba38aa..19b8d837e9d0 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -885,12 +885,13 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action)
>> reg |= DWC3_DALEPENA_EP(dep->number);
>> dwc3_writel(dwc->regs, DWC3_DALEPENA, reg);
>>
>> - if (usb_endpoint_xfer_control(desc))
>> - goto out;
>> -
>> /* Initialize the TRB ring */
>> dep->trb_dequeue = 0;
>> dep->trb_enqueue = 0;
>> +
>> + if (usb_endpoint_xfer_control(desc))
>> + goto out;
>> +
>> memset(dep->trb_pool, 0,
>> sizeof(struct dwc3_trb) * DWC3_TRB_NUM);
>>
>> @@ -2463,7 +2464,8 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int 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) {
>> + if ((!is_on && (dwc->ep0state != EP0_SETUP_PHASE ||
>> + dwc->ep0_next_event != DWC3_EP0_COMPLETE))) {
>> reinit_completion(&dwc->ep0_in_setup);
>>
>> ret = wait_for_completion_timeout(&dwc->ep0_in_setup,
>> @@ -2506,6 +2508,17 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>> u32 count;
>>
>> dwc->connected = false;
>> +
>> + /*
>> + * Ensure no pending data/setup stages, and disable ep0 to
>> + * block further EP0 transactions before stopping pending
>> + * transfers.
>> + */
>> + dwc3_ep0_end_control_data(dwc, dwc->eps[1]);
>> + dwc3_ep0_stall_and_restart(dwc);
>> + __dwc3_gadget_ep_disable(dwc->eps[0]);
>> + __dwc3_gadget_ep_disable(dwc->eps[1]);
>> +
>> /*
>> * In the Synopsis DesignWare Cores USB3 Databook Rev. 3.30a
>> * Section 4.1.8 Table 4-7, it states that for a device-initiated
>> @@ -3587,8 +3600,10 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>> bool interrupt)
>> {
>> struct dwc3_gadget_ep_cmd_params params;
>> + struct dwc3 *dwc = dep->dwc;
>> u32 cmd;
>> int ret;
>> + int retries = 1;
>>
>> if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>> (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>> @@ -3620,7 +3635,7 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>> *
>> * This mode is NOT available on the DWC_usb31 IP.
>> */
>> -
>> +retry:
>> cmd = DWC3_DEPCMD_ENDTRANSFER;
>> cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0;
>> cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0;
>> @@ -3628,6 +3643,23 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>> memset(&params, 0, sizeof(params));
>> ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
>> WARN_ON_ONCE(ret);
>> + if (ret == -ETIMEDOUT) {
>> + if (!dwc->connected) {
>> + /*
>> + * While the controller is in an active setup/control
>> + * transfer, the HW is unable to service other eps
>> + * potentially leading to an endxfer command timeout.
>> + * It was recommended to ensure that there are no
>> + * pending/cached setup packets stored in internal
>> + * memory. Only way to achieve this is to issue another
>> + * start transfer, and retry.
>> + */
>> + if (retries--) {
>> + dwc3_ep0_out_start(dwc);
>> + goto retry;
>> + }
>> + }
>> + }
>> dep->resource_index = 0;
>>
>> if (!interrupt)

Next message: Christophe JAILLET: "Re: [PATCH] i2c: bcm2835: Fix the error handling in 'bcm2835_i2c_probe()'"
Previous message: Liang, Kan: "Re: [PATCH kvm/queue v2 2/3] perf: x86/core: Add interface to query perfmon_event_map[] directly"
In reply to: Jack Pham: "Re: [RFC PATCH 1/3] usb: dwc3: Flush pending SETUP data during stop active xfers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]