Re: [PATCH 2/2] usb: dwc3: gadget: Ignore EP queue requests during bus reset

[Thinh Nguyen]

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