On 3/11/2021 3:18 PM, Thinh Nguyen wrote:
> Wesley Cheng wrote:
>> In the situations where the DWC3 gadget stops active transfers, once
>> calling the dwc3_gadget_giveback(), there is a chance where a function
>> driver can queue a new USB request in between the time where the dwc3
>> lock has been released and re-aquired. This occurs after we've already
>> issued an ENDXFER command. When the stop active transfers continues
>> to remove USB requests from all dep lists, the newly added request will
>> also be removed, while controller still has an active TRB for it.
>> This can lead to the controller accessing an unmapped memory address.
>>
>> Fix this by ensuring parameters to prevent EP queuing are set before
>> calling the stop active transfers API.
>>
>> Fixes: ae7e86108b12 ("usb: dwc3: Stop active transfers before halting the
>> controller")
>> Signed-off-by: Wesley Cheng <wch...@codeaurora.org>
>> ---
>> Changes since V1:
>> - Added Fixes tag to point to the commit this is addressing
>>
>> drivers/usb/dwc3/gadget.c | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 4780983..4d98fbf 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -783,8 +783,6 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
>>
>> trace_dwc3_gadget_ep_disable(dep);
>>
>> - dwc3_remove_requests(dwc, dep);
>> -
>> /* make sure HW endpoint isn't stalled */
>> if (dep->flags & DWC3_EP_STALL)
>> __dwc3_gadget_ep_set_halt(dep, 0, false);
>> @@ -803,6 +801,8 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
>> dep->endpoint.desc = NULL;
>> }
>>
>> + dwc3_remove_requests(dwc, dep);
>> +
>> return 0;
>> }
>>
>> @@ -1617,7 +1617,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep,
>> struct dwc3_request *req)
>> {
>> struct dwc3 *dwc = dep->dwc;
>>
>> - 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);
>> return -ESHUTDOWN;
>> @@ -2247,6 +2247,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g,
>> int is_on)
>> if (!is_on) {
>> u32 count;
>>
>> + dwc->connected = false;
>
>
> You want to set this before __dwc3_gadget_stop() right? Then you may
> want to remove this same setting few lines below this. Otherwise, this
> looks good.
>
> Thanks,
> Thinh
>
Hi Thinh,
Ah, good catch, must've missed that while porting this over from my
local branch. Will remove that connected false setting a few lines down.
Thanks
Wesley Cheng
>
>> /*
>> * In the Synopsis DesignWare Cores USB3 Databook Rev. 3.30a
>> * Section 4.1.8 Table 4-7, it states that for a
>> device-initiated
>> @@ -3329,8 +3330,6 @@ static void dwc3_gadget_reset_interrupt(struct dwc3
>> *dwc)
>> {
>> u32 reg;
>>
>> - dwc->connected = true;
>> -
>> /*
>> * WORKAROUND: DWC3 revisions <1.88a have an issue which
>> * would cause a missing Disconnect Event if there's a
>> @@ -3370,6 +3369,7 @@ static void dwc3_gadget_reset_interrupt(struct dwc3
>> *dwc)
>> * transfers."
>> */
>> dwc3_stop_active_transfers(dwc);
>> + dwc->connected = true;
>>
>> reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>> reg &= ~DWC3_DCTL_TSTCTRL_MASK;
>>
>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
