Hi, Wesley Cheng <[email protected]> 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 <[email protected]> > > --- > 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?
> @@ -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?
> @@ -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.
--
balbi
signature.asc
Description: PGP signature
