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@xxxxxxxxxxxxxx>
>>>> ---
>>>> 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.
> 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.
>
> We should do dwc->connected = true when we handle connection_done
> interrupt instead. The END_TRANSFER command should complete before this.
>
> Thanks,
> Thinh
>
Just want to clarify, I was referring to your previous commit
f09ddcfcb8c5, we'd still need dwc->connected = false when handling reset
interrupt as you've done here.
BR,
Thinh
