From: Felipe Balbi <balbi@kernel.org>
To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Cc: Wesley Cheng <wcheng@codeaurora.org>,
"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"jackp@codeauora.org" <jackp@codeauora.org>
Subject: Re: [RFC][PATCH] usb: dwc3: usb: dwc3: Force stop EP0 transfers during pullup disable
Date: Sun, 15 Aug 2021 09:06:00 +0300 [thread overview]
Message-ID: <875yw7jkz6.fsf@kernel.org> (raw)
In-Reply-To: <00952bdc-acc2-f373-9286-6a8380e0b7d1@synopsys.com>
Hi,
Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>>>>>>> If this occurs, then the entire pullup disable routine is skipped and
>>>>>>>>> proper cleanup and halting of the controller does not complete.
>>>>>>>>> Instead of returning an error (which is ignored from the UDC
>>>>>>>>> perspective), do what is mentioned in the comments and force the
>>>>>>>>> transaction to complete and put the ep0state back to the SETUP phase.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
>>>>>>>>> ---
>>>>>>>>> drivers/usb/dwc3/ep0.c | 4 ++--
>>>>>>>>> drivers/usb/dwc3/gadget.c | 6 +++++-
>>>>>>>>> drivers/usb/dwc3/gadget.h | 3 +++
>>>>>>>>> 3 files changed, 10 insertions(+), 3 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>>>>>>>> index 6587394..abfc42b 100644
>>>>>>>>> --- a/drivers/usb/dwc3/ep0.c
>>>>>>>>> +++ b/drivers/usb/dwc3/ep0.c
>>>>>>>>> @@ -218,7 +218,7 @@ 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;
>>>>>>>>>
>>>>>>>>> @@ -1073,7 +1073,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;
>>>>>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>>>>>>> index 54c5a08..a0e2e4d 100644
>>>>>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>>>>>> @@ -2437,7 +2437,11 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>>>>>>>> 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);
>>>>>>>>> + dwc3_ep0_end_control_data(dwc, dwc->eps[0]);
>>>>>>>>> + dwc3_ep0_end_control_data(dwc, dwc->eps[1]);
>>>>>>>>
>>>>>>>> End transfer command takes time, need to wait for it to complete before
>>>>>>>> issuing Start transfer again. Also, why restart again when it's about to
>>>>>>>> be disconnected.
>>>>>>>
>>>>>>> I can try without restarting it again, and see if that works. Instead
>>>>>>> of waiting for the command complete event, can we set the ForceRM bit,
>>>>>>> similar to what we do for dwc3_remove_requests()?
>>>>>>>
>>>>>>
>>>>>> ForceRM=1 means that the controller will ignore updating the TRBs
>>>>>> (including not clearing the HWO and remain transfer size). The driver
>>>>>> still needs to wait for the command to complete before issuing Start
>>>>>> Transfer command. Otherwise Start Transfer won't go through. If we know
>>>>>> that we're not going to issue Start Transfer any time soon, then we may
>>>>>> be able to get away with ignoring End Transfer command completion.
>>>>>>
>>>>>
>>>>> I see. Currently, in the place that we do use
>>>>> dwc3_ep0_end_control_data(), its followed by
>>>>> dwc3_ep0_stall_and_restart() which would execute start transfer. For
>>>>
>>>> That doesn't look right. You can try to see if it can recover from a
>>>> control write request. Often time we do control read and not write.
>>>> (i.e. try to End Transfer and immediately Start Transfer on the same
>>>> direction control endpoint).
>>>>
>>> OK, I can try, but just to clarify, I was referring to how it was being
>>> done in:
>>>
>>> static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
>>> const struct dwc3_event_depevt *event)
>>> {
>>> ...
>>> if (dwc->ep0_expect_in != event->endpoint_number) {
>>> struct dwc3_ep *dep = dwc->eps[dwc->ep0_expect_in];
>>>
>>> dev_err(dwc->dev, "unexpected direction for Data Phase\n");
>>> dwc3_ep0_end_control_data(dwc, dep);
>>> dwc3_ep0_stall_and_restart(dwc);
>>> return;
>>> }
>>>
>
> Looking at this snippet again, it looks wrong. For control write
> unexpected direction, if the driver hasn't setup and started the DATA
> phase yet, then it's fine, but there is a problem if it did.
>
> Since dwc3_ep0_end_control_data() doesn't issue End Transfer command to
> ep0 due to the resource_index check, it doesn't follow the control
IIRC resource_index is always non-zero, so the command should be
triggered. If you have access to a Lecroy USB Trainer, could you script
this very scenario for verification?
> transfer flow model in the programming guide. This may cause
> dwc3_ep0_stall_and_restart() to overwrite the TRBs for the DATA phase
> with SETUP stage. Also, if the ep0 is already started, the driver won't
> issue Start Transfer command again.
> This issue is unlikely to occur unless we see a misbehave host for
> control write request. Regardless, we need to fix this. I may need some
right, it would be a misbehaving host, however databook called it out as
something that _can_ happen. Moreover, I have vague memories of this
being one of the test cases in Lecroy's USB Certification Suite.
> time before I can create a patch and test it. If you or anyone is up to
> take this on, it'd be highly appreciated.
Before we go ahead writing a patch for this, I'd really like to see
traces showing this failure and a minimal reproducer. The reproducer
would probably have to be a script for Lecroy's USB Trainer.
Keep in mind this entire ep0 stack used to pass USBCV on every -rc and
major release (before I lost access to all my USB gear heh).
--
balbi
next prev parent reply other threads:[~2021-08-15 6:11 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-11 2:23 [RFC][PATCH] usb: dwc3: usb: dwc3: Force stop EP0 transfers during pullup disable Wesley Cheng
2021-08-12 0:47 ` Thinh Nguyen
2021-08-12 17:12 ` Wesley Cheng
2021-08-12 21:31 ` Thinh Nguyen
2021-08-12 23:19 ` Wesley Cheng
2021-08-13 23:21 ` Thinh Nguyen
2021-08-13 23:49 ` Wesley Cheng
2021-08-14 0:30 ` Thinh Nguyen
2021-08-15 0:26 ` Thinh Nguyen
2021-08-15 6:06 ` Felipe Balbi [this message]
2021-08-16 0:33 ` Thinh Nguyen
2021-08-16 5:15 ` Felipe Balbi
2021-08-17 1:25 ` Thinh Nguyen
2021-08-16 19:13 ` Wesley Cheng
2021-08-18 21:24 ` Wesley Cheng
2021-08-19 0:40 ` Thinh Nguyen
2021-08-19 20:51 ` Wesley Cheng
2021-08-20 1:52 ` Thinh Nguyen
2021-08-20 3:05 ` Wesley Cheng
2021-08-19 5:36 ` Felipe Balbi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=875yw7jkz6.fsf@kernel.org \
--to=balbi@kernel.org \
--cc=Thinh.Nguyen@synopsys.com \
--cc=gregkh@linuxfoundation.org \
--cc=jackp@codeauora.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=wcheng@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).