Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2764 bytes --]


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
> When handing the SETUP packet by composite_setup(), we will release the
> dwc->lock. If we get the 'USB_GADGET_DELAYED_STATUS' result from setup
> function, which means we need to delay handling the STATUS phase.

this sentence needs a little work. Seems like it's missing some
information.

anyway, I get that we release the lock but...

> But during the lock release period, maybe the request for handling delay

execution of ->setup() itself should be locked. I can see that it's only
locked for set_config() which is rather peculiar.

What exact request you had when you triggered this? (Hint: dwc3
tracepoints print out ctrl request bytes). IIRC, only set_config() or
f->set_alt() can actually return USB_GADGET_DELAYED_STATUS.

Which gadget driver were you using when you triggered this?

Another point here is that the really robust way of fixing this is to
get rid of USB_GADGET_DELAYED_STATUS altogether and just make sure
gadget drivers know how to queue requests for all three phases of a
Control Transfer.

A lot of code will be removed from all gadget drivers and UDC drivers
while combining all of it in a single place in composite.c.

The reason I'm saying this is that other UDC drivers might have similar
races already but they just haven't triggered.

> STATUS phase has been queued into list before we set 'dwc->delayed_status'
> flag or entering 'EP0_STATUS_PHASE' phase, then we will miss the chance
> to handle the STATUS phase. Thus we should check if the request for delay
> STATUS phase has been enqueued when entering 'EP0_STATUS_PHASE' phase in
> dwc3_ep0_xfernotready(), if so, we should handle it.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  drivers/usb/dwc3/ep0.c |   14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 9bb1f85..e689ced 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -1123,7 +1123,21 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
>  		dwc->ep0state = EP0_STATUS_PHASE;
>  
>  		if (dwc->delayed_status) {
> +			struct dwc3_ep *dep = dwc->eps[0];
> +
>  			WARN_ON_ONCE(event->endpoint_number != 1);
> +			/*
> +			 * We should handle the delay STATUS phase here if the
> +			 * request for handling delay STATUS has been queued
> +			 * into the list.
> +			 */
> +			if (!list_empty(&dep->pending_list)) {
> +				dwc->delayed_status = false;
> +				usb_gadget_set_state(&dwc->gadget,
> +						     USB_STATE_CONFIGURED);

Isn't this patch also changing the normal case when usb_ep_queue() comes
later? I guess list_empty() protects against that...

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]