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>
Subject: Re: [PATCH 2/2] usb: dwc3: gadget: Ignore EP queue requests during bus reset
Date: Tue, 23 Mar 2021 23:47:07 +0000 [thread overview]
Message-ID: <862b2b75-2c71-d805-086c-7fbcac73724d@synopsys.com> (raw)
In-Reply-To: <d77bbea0-04eb-9426-5b16-50e408b2e787@codeaurora.org>
Wesley Cheng wrote:
> Hi Thinh,
>
>
> On 3/19/2021 7:01 PM, Thinh Nguyen wrote:
>> Wesley Cheng wrote:
>>>
>>>
>>> On 3/19/2021 5:40 PM, Thinh Nguyen wrote:
>>>> Hi,
>>>>
>>>> Wesley Cheng wrote:
>>>>> The current dwc3_gadget_reset_interrupt() will stop any active
>>>>> transfers, but only addresses blocking of EP queuing for while we are
>>>>> coming from a disconnected scenario, i.e. after receiving the disconnect
>>>>> event. If the host decides to issue a bus reset on the device, the
>>>>> connected parameter will still be set to true, allowing for EP queuing
>>>>> to continue while we are disabling the functions. To avoid this, set the
>>>>> connected flag to false until the stop active transfers is complete.
>>>>>
>>>>> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
>>>>> ---
>>>>> drivers/usb/dwc3/gadget.c | 9 +++++++++
>>>>> 1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>>> index 6e14fdc..d5ed0f69 100644
>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>> @@ -3327,6 +3327,15 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
>>>>> u32 reg;
>>>>>
>>>>> /*
>>>>> + * Ideally, dwc3_reset_gadget() would trigger the function
>>>>> + * drivers to stop any active transfers through ep disable.
>>>>> + * However, for functions which defer ep disable, such as mass
>>>>> + * storage, we will need to rely on the call to stop active
>>>>> + * transfers here, and avoid allowing of request queuing.
>>>>> + */
>>>>> + dwc->connected = false;
>>>>> +
>>>>> + /*
>>>>> * WORKAROUND: DWC3 revisions <1.88a have an issue which
>>>>> * would cause a missing Disconnect Event if there's a
>>>>> * pending Setup Packet in the FIFO.
>>>>>
>>>>
>>>> This doesn't look right. Did you have rebase issue with your local
>>>> change again?
>>>>
>>>> BR,
>>>> Thinh
>>>>
>>> Hi Thinh,
>>>
>>> This was rebased on Greg's usb-linus branch, which has commit
>>> f09ddcfcb8c5 ("usb: dwc3: gadget: Prevent EP queuing while stopping
>>> transfers") merged.
>>
>> Ah I see.
>>
>>>
>>> commit f09ddcfcb8c5 moved the dwc->connected = true to after we have
>>> finished stop active transfers. However, this change will also ensure
>>> that the connected flag is set to false to ensure that when we call stop
>>> active transfers, nothing can prepare TRBs. (previous commit only
>>> addresses the case where we get the reset interrupt when coming from a
>>> disconnected state)
>>>
>>
>> That still doesn't address this issue.
>>
>> Because:
>> 1) We're still protected by the spin_lock_irq*(), so this change doesn't
>> make any difference while handling an event.
>
> Thank you for the feedback. So it is true that we lock dwc->lock while
> handling EP/device events, but what these changes are trying to address
> is that during dwc3_stop_active_transfers() we will eventually call
> dwc3_gadget_giveback() to call the complete() functions registered by
> the function driver. Before we call the complete() callbacks, we unlock
> dwc->lock, so we are no longer protected, and if there was a pending ep
> queue from a function driver, that would allow it to acquire the lock
> and continue preparing the TRBs.
>
Ah I forgot about that.
>> 2) We don't enable the interrupt for END_TRANSFER command completion
>> when doing dwc3_stop_active_transfers(), the
>> DWC3_EP_END_TRANSFER_PENDING flag will not be set to prevent preparing
>> new requests.
>>
> Agreed. That is the reason for adding the check to dwc->connected in
> __dwc3_gadget_ep_queue()
>
> if (!dep->endpoint.desc || !dwc->pullups_connected || !dwc->connected) {
> dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
> dep->name);
> return -ESHUTDOWN;
>
>> We should do dwc->connected = true when we handle connection_done
>> interrupt instead. The END_TRANSFER command should complete before this.
>>
> So how this change will address the issue is:
>
> 1. IRQ handler will acquire dwc->lock
> 2. dwc3_gadget_reset_handler() sets dwc->connected = false
> 3. Call to dwc3_stop_active_transfers()
> ---> dwc3_gadget_giveback() releases dwc->lock
> 4. If there was a pending ep queue (waiting for dwc->lock) it can
> continue here
> 5. __dwc3_gadget_ep_queue() exits early due to dwc->connected = false
> 6. dwc3_gadget_giveback() re-acquires dwc->lock and continues
>
Ok. I thought this was for different issue. I thought you were trying to
solve an issue where a request is queued immediately after handling the
reset interrupt but before the END_TRANSFER command completion.
Thanks for the clarification.
BR,
Thinh
prev parent reply other threads:[~2021-03-23 23:53 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-19 9:31 [PATCH 0/2] Fix allowing of ep queuing while stopping transfers Wesley Cheng
2021-03-19 9:31 ` [PATCH 1/2] usb: dwc3: gadget: Avoid continuing preparing TRBs during teardown Wesley Cheng
2021-03-19 9:31 ` [PATCH 2/2] usb: dwc3: gadget: Ignore EP queue requests during bus reset Wesley Cheng
2021-03-20 0:40 ` Thinh Nguyen
2021-03-20 1:07 ` Wesley Cheng
2021-03-20 2:01 ` Thinh Nguyen
2021-03-20 2:14 ` Thinh Nguyen
2021-03-20 5:30 ` Wesley Cheng
2021-03-23 23:47 ` 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=862b2b75-2c71-d805-086c-7fbcac73724d@synopsys.com \
--to=thinh.nguyen@synopsys.com \
--cc=balbi@kernel.org \
--cc=gregkh@linuxfoundation.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).