Re: [PATCH] usb: dwc3: gadget: Stall and restart EP0 if host is unresponsive

[Wesley Cheng]

Hi Thinh,

On 4/3/2023 6:11 PM, Thinh Nguyen wrote:
> On Fri, Mar 31, 2023, Wesley Cheng wrote:
> > It was observed that there are hosts that may complete pending SETUP
> > transactions before the stop active transfers and controller halt occurs,
> > leading to lingering endxfer commands on DEPs on subsequent pullup/gadget
> > start iterations.
>
> Can you clarify this a bit further? Even though the controller is
> halted, you still observed activity?

Yes...I didn't understand how that was possible either, but traces 
clearly showed that the controller halt was successful even though there 
were no endxfers issued on some EPs.  Although, I can't say for certain 
if those EPs were actively being used at that time.

> > dwc3_gadget_ep_disable   name=ep8in flags=0x3009  direction=1
> > dwc3_gadget_ep_disable   name=ep4in flags=1  direction=1
> > dwc3_gadget_ep_disable   name=ep3out flags=1  direction=0
> > usb_gadget_disconnect   deactivated=0  connected=0  ret=0
> >
> > The sequence shows that the USB gadget disconnect (dwc3_gadget_pullup(0))
> > routine completed successfully, allowing for the USB gadget to proceed with
> > a USB gadget connect.  However, if this occurs the system runs into an
> > issue where:
> >
> > BUG: spinlock already unlocked on CPU
> >   spin_bug+0x0
> >   dwc3_remove_requests+0x278
> >   dwc3_ep0_out_start+0xb0
> >   __dwc3_gadget_start+0x25c
> >
> > This is due to the pending endxfers, leading to gadget start (w/o lock
> > held) to execute the remove requests, which will unlock the dwc3 spinlock
> > as part of giveback.
> >
> > To mitigate this, resolve the pending endxfers on the pullup disable path
> > by:
> >   1. Re-locating the SETUP phase check after stop active transfers, since
> >   that is where the DWC3_EP_DELAY_STOP is potentially set.  This also allows
> >   for handling of a host that may be unresponsive by using the completion
> >   timeout to trigger the stall and restart for EP0.
> >
> >   2. Do not call gadget stop until the poll for controller halt is
> >   completed.  DEVTEN is cleared as part of gadget stop, so the intention to
> >   allow ep0 events to continue while waiting for controller halt is not
> >   happening.
> >
> > Fixes: c96683798e27 ("usb: dwc3: ep0: Don't prepare beyond Setup stage")
> > Signed-off-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
> > ---
> >   drivers/usb/dwc3/gadget.c | 101 ++++++++++++++++++++++----------------
> >   1 file changed, 58 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 3c63fa97a680..9715de8e99bc 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -139,6 +139,24 @@ int dwc3_gadget_set_link_state(struct dwc3 *dwc, enum dwc3_link_state state)
> >   	return -ETIMEDOUT;
> >   }
> >   
> > +static void dwc3_ep0_reset_state(struct dwc3 *dwc)
> > +{
> > +	unsigned int	dir;
> > +
> > +	if (dwc->ep0state != EP0_SETUP_PHASE) {
> > +		dir = !!dwc->ep0_expect_in;
> > +		if (dwc->ep0state == EP0_DATA_PHASE)
> > +			dwc3_ep0_end_control_data(dwc, dwc->eps[dir]);
> > +		else
> > +			dwc3_ep0_end_control_data(dwc, dwc->eps[!dir]);
> > +
> > +		dwc->eps[0]->trb_enqueue = 0;
> > +		dwc->eps[1]->trb_enqueue = 0;
> > +
> > +		dwc3_ep0_stall_and_restart(dwc);
> > +	}
> > +}
> > +
>
> Can we separate refactoring changes other functional changes? It's
> difficult to review with too many things to keep track of.

Sure I can split this into another patch.

Thanks
Wesley Cheng