On 8/19/2020 4:37 AM, Felipe Balbi wrote:
>
> Hi,
>
> Wesley Cheng <wch...@codeaurora.org> writes:
>> In the DWC3 databook, for a device initiated disconnect, 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 <wch...@codeaurora.org>
>>
>> ---
>> 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 | 59 +++++++++++++++++++++++++++++++++++++--
>> 2 files changed, 57 insertions(+), 4 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) {
>
> these two should be the same. If pullups are not connected, there's no
> way we can have an endpoint descriptor. Did you find a race condition here?
>
Hi Felipe,
At least for EP0, I don't see us clearing the EP0 desc after we set it
during dwc3_gadget_init_endpoint(). In the dwc3_gadget_ep_disable() we
only clear the desc for non control EPs:
static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
{
...
/* Clear out the ep descriptors for non-ep0 */
if (dep->number > 1) {
dep->endpoint.comp_desc = NULL;
dep->endpoint.desc = NULL;
}
Is the desc for ep0 handled elsewhere? (checked ep0.c as well, but
couldn't find any references there)
>> @@ -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;
>> +
>> + dwc3_remove_requests(dwc, dep);
>> + }
>> +}
>> +
>> static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
>> {
>> u32 reg;
>> @@ -1950,16 +1968,37 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc,
>> int is_on, int suspend)
>>
>> dwc->pullups_connected = true;
>> } else {
>> + dwc->pullups_connected = false;
>> +
>> + __dwc3_gadget_ep_disable(dwc->eps[0]);
>> + __dwc3_gadget_ep_disable(dwc->eps[1]);
>> +
>> + /*
>> + * The databook explicitly mentions for a device-initiated
>> + * disconnect sequence, the SW needs to ensure that it ends any
>> + * active transfers.
>> + */
>> + dwc3_stop_active_transfers(dwc);
>
> IIRC, gadget driver is required to dequeue transfers before
> disconnecting. My memory is a bit fuzzy in that area, but anyway, how
> did you trigger this problem?
>
I had a script that just did the following to trigger the soft disconnect:
echo "" > /sys/kernel/config/usb_gadget/g1/UDC
sleep 4
echo "a600000.dwc3" > /sys/kernel/config/usb_gadget/g1/UDC
Then on the PC, I just had a batch file executing adb push (of a large
file), in order to create the situation where there was a device
initiated disconnect while an active transfer was occurring. After
maybe after 4-5 iterations, I saw that the controller halt failed.
[ 87.364252] dwc3_gadget_run_stop run stop = 0
[ 87.374168] ffs_epfile_io_complete: eshutdown
[ 87.376162] __dwc3_gadget_ep_queue
[ 87.386160] ffs_epfile_io_complete: eshutdown
I added some prints to hopefully show that while we are disabling the
controller, the gadget/function driver is still active. The eshutdown
prints happen due to the dwc3_stop_active_transfers() call, which means
there are still some pending/active reqs.
Thanks
Wesley
>> @@ -1994,9 +2033,15 @@ 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);
>
> looks like a call to synchronize_irq() would be enough here.
>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
