Hi,
Pavan Kondeti wrote:
> On Thu, Apr 21, 2022 at 07:22:50PM -0700, Thinh Nguyen wrote:
>> Since we can't guarantee that the host won't send new Setup packet
>> before going through the device-initiated disconnect, don't prepare
>> beyond the Setup stage and keep the device in EP0_SETUP_PHASE. This
>> ensures that the device-initated disconnect sequence can go through
>> gracefully. Note that the controller won't service the End Transfer
>> command if it can't DMA out the Setup packet.
>>
>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx>
>> ---
>> drivers/usb/dwc3/ep0.c | 2 +-
>> drivers/usb/dwc3/gadget.c | 29 +++++++++++++++++------------
>> 2 files changed, 18 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index 1064be5518f6..c47c696316dd 100644
>> --- a/drivers/usb/dwc3/ep0.c
>> +++ b/drivers/usb/dwc3/ep0.c
>> @@ -813,7 +813,7 @@ static void dwc3_ep0_inspect_setup(struct dwc3 *dwc,
>> int ret = -EINVAL;
>> u32 len;
>>
>> - if (!dwc->gadget_driver)
>> + if (!dwc->gadget_driver || !dwc->connected)
>> goto out;
>>
>> trace_dwc3_ctrl_req(ctrl);
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index a86225dbaa2c..e5f07c0e8ad9 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2505,6 +2505,23 @@ static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc)
>> spin_lock_irqsave(&dwc->lock, flags);
>> dwc->connected = false;
>>
>> + /*
>> + * 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 (dwc->ep0state != EP0_SETUP_PHASE) {
>> + int ret;
>> +
>> + reinit_completion(&dwc->ep0_in_setup);
>> +
>> + spin_unlock_irqrestore(&dwc->lock, flags);
>> + ret = wait_for_completion_timeout(&dwc->ep0_in_setup,
>> + msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT));
>> + spin_lock_irqsave(&dwc->lock, flags);
>> + if (ret == 0)
>> + dev_warn(dwc->dev, "timed out waiting for SETUP phase\n");
>
> DWC3_PULL_UP_TIMEOUT is 500 msec. If the ongoing control transfer is delayed
> (dwc3::delayed_status), for whatever reason, would there be a problem?
>
Sorry for the delayed response. I was away.
If the control transfer takes longer than 500ms, then we'd get this
timed out warning. However, it should be fine because
1) If the function driver hasn't sent the status, then the host won't be
sending a new SETUP packet.
2) If the delayed status was sent and completed immediately after the
timeout but before the dwc3_gadget_soft_disconnect holding the
spin_lock, then we may see End Transfer command timeout. It may not look
like the cleanup was done gracefully, but that should be fine. The
command should be able to complete once the spin_lock is released and
Setup packet handled. The controller should halt within the polling period.
3) If the host misbehaves and ignores the status stage/abort the control
transfer to send a new setup packet, I don't think the current dwc3
driver handles that case properly. But that should be for a separate
patch fix.
BR,
Thinh
