From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
To: Wesley Cheng <wcheng@codeaurora.org>,
Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
"balbi@kernel.org" <balbi@kernel.org>,
"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
"jackp@codeaurora.org" <jackp@codeaurora.org>
Subject: Re: [PATCH v2] usb: dwc3: Stop active transfers before halting the controller
Date: Wed, 2 Sep 2020 01:55:15 +0000 [thread overview]
Message-ID: <57529302-d43c-c0fc-ab31-ab45a177fd05@synopsys.com> (raw)
In-Reply-To: <ee3b5178-1c8e-0758-1181-b28af2a51572@codeaurora.org>
Wesley Cheng wrote:
>
> On 9/1/2020 3:14 PM, Wesley Cheng wrote:
>>
>> On 8/29/2020 2:35 PM, Thinh Nguyen wrote:
>>> 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.
>>>>
>>>> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
>>>>
>>>> ---
>>>> 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 | 52 ++++++++++++++++++++++++++++++++++++++-
>>>> 2 files changed, 52 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..df8d89d6bdc9 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,24 @@ 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 < DWC3_ENDPOINTS_NUM; epnum++) {
>>>> + struct dwc3_ep *dep;
>>>> +
>>>> + dep = dwc->eps[epnum];
>>>> + if (!dep)
>>>> + continue;
>>>> +
>>>> + if (!(dep->flags & DWC3_EP_ENABLED))
>>>> + continue;
>>> Don't do the enabled check here. Let the dwc3_stop_active_transfer() do
>>> that checking.
>>>
>> Hi Thinh,
>>
>> Thanks for the detailed review, as always. Got it, we can allow that to
>> catch it based off the DWC3_EP_TRANSFER_STARTED.
>>
>>>> +
>>>> + dwc3_remove_requests(dwc, dep);
>>>> + }
>>>> +}
>>>> +
>>>> static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
>>>> {
>>>> u32 reg;
>>>> @@ -1994,9 +2012,39 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>>> }
>>>> }
>>>>
>>>> + /*
>>>> + * Synchronize and disable any further event handling while controller
>>>> + * is being enabled/disabled.
>>>> + */
>>>> + disable_irq(dwc->irq_gadget);
>>> I think it's better to do dwc3_gadget_disable_irq(). This only stops
>>> handling events. Although it's unlikely, the controller may still
>>> generate events before it's halted.
>>>
>> I think its better if we can do both. At least with the disable_irq()
>> call present, we can ensure the irq handlers are complete, or we can do
>> as Felipe suggested, and first disable the controller events (using
>> dwc3_gadget_disable_irq()) and then calling synchronize_irq().
>>
>> The concern I had is the pullup() API updating the lpos, and the hardirq
>> handler referencing it to update the evt buf cache and waking up the
>> threaded irq handler. (since we don't clear the evt->count explicitly,
>> it may reference empty/stale events)
I see, then we should do both.
>>
>>>> spin_lock_irqsave(&dwc->lock, flags);
>>>> +
>>>> + /* Controller is not halted until pending events are acknowledged */
>>>> + if (!is_on) {
>>>> + u32 reg;
>>>> +
>>>> + __dwc3_gadget_ep_disable(dwc->eps[0]);
>>>> + __dwc3_gadget_ep_disable(dwc->eps[1]);
>>> You can just do __dwc3_gadget_stop(), and do that after
>>> dwc3_stop_active_transfers().
>>>
>> Got it.
>>
> Hi Thinh,
>
> Maybe we can ignore calling dwc3_gadget_disable_irq() separately if we
> are going to use __dwc3_gadget_stop(), since gadget stop will call
> dwc3_gadget_disable_irq(). Also, it would be executed before the event
> count clearing, so if there was an event (unlikely) that was generated,
> we would discard it.
That was my intention. Just make sure to re-enable the events on pull_up(on)
>
> Something like:
>
> disable_irq(dwc->irq_gadget);
> spin_lock_irqsave(&dwc->lock, flags);
>
> if (!is_on) {
> u32 count;
>
> dwc3_stop_active_transfers(dwc);
> __dwc3_gadget_stop(dwc);
>
> count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
> count &= DWC3_GEVNTCOUNT_MASK;
>
> Thanks
> Wesley
>
>
Thanks,
Thinh
prev parent reply other threads:[~2020-09-02 1:56 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-28 22:44 [PATCH v2] usb: dwc3: Stop active transfers before halting the controller Wesley Cheng
2020-08-29 21:35 ` Thinh Nguyen
2020-09-01 22:14 ` Wesley Cheng
2020-09-02 1:50 ` Wesley Cheng
2020-09-02 1:55 ` Thinh Nguyen [this message]
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=57529302-d43c-c0fc-ab31-ab45a177fd05@synopsys.com \
--to=thinh.nguyen@synopsys.com \
--cc=balbi@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=jackp@codeaurora.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).