Re: [PATCH 3/7] usb: dwc3: gadget: Clear delayed stop on End Transfer completion

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: dwc3: gadget: skipped request cleanup on DWC3_EP_DELAY_STOP clear
--

commit f4ade355ebc7b629210edc8108aa9f5fb3f0bcc2
Author: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

usb: dwc3: gadget: Clear delayed stop on End Transfer completion

This commit clears the DWC3_EP_DELAY_STOP flag in
dwc3_gadget_endpoint_command_complete() when an End Transfer command
completes, preventing a later retry on the next SETUP phase from stopping a
newly started transfer.

> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index df5407e757884..4de3b81cb6781 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -3875,6 +3875,7 @@ static void dwc3_gadget_endpoint_command_complete(struct dwc3_ep *dep,
>  	if (dep->stream_capable)
>  		dep->flags |= DWC3_EP_IGNORE_NEXT_NOSTREAM;
>  
> +	dep->flags &= ~DWC3_EP_DELAY_STOP;
>  	dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
>  	dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>  	dwc3_gadget_ep_cleanup_cancelled_requests(dep);

[Severity: High]
Can clearing DWC3_EP_DELAY_STOP unconditionally here in
dwc3_gadget_endpoint_command_complete() in drivers/usb/dwc3/gadget.c skip
deferred request cleanup and leak usb_request objects (and their mapped DMA
buffers) if the endpoint was disabled or the controller disconnected while
DWC3_EP_DELAY_STOP was set?

In __dwc3_stop_active_transfer(), if dwc3_send_gadget_ep_cmd() with
DWC3_DEPCMD_CMDIOC returns -ETIMEDOUT while dep->dwc->ep0state !=
EP0_SETUP_PHASE (for example, while dwc->delayed_status is true in
dwc3_gadget_ep_dequeue(), __dwc3_gadget_ep_set_halt(),
__dwc3_gadget_kick_transfer(), or __dwc3_gadget_ep_disable() after commit
"usb: dwc3: gadget: Wait for End Transfer completion on ep disable", or via
__dwc3_gadget_start_isoc()), the driver sets DWC3_EP_DELAY_STOP and returns
0 without clearing DWC3_EP_TRANSFER_STARTED (and before scheduling error
recovery in commit "usb: dwc3: gadget: Recover from End Transfer timeout"),
while the End Transfer command remains in flight in hardware.

If __dwc3_gadget_ep_disable() or dwc3_gadget_soft_disconnect() then runs
before EP0 reaches EP0_SETUP_PHASE, dwc3_remove_requests() returns early
without giving back dep->started_list, dep->pending_list, or
dep->cancelled_list:

drivers/usb/dwc3/gadget.c:dwc3_remove_requests() {
	...
	dwc3_stop_active_transfer(dep, false, false);

	/* If endxfer is delayed, avoid unmapping requests */
	if (dep->flags & DWC3_EP_DELAY_STOP)
		return;
	...
}

And in __dwc3_gadget_ep_disable(), dep->endpoint.desc is cleared while
DWC3_EP_DELAY_STOP | DWC3_EP_TRANSFER_STARTED remain set:

drivers/usb/dwc3/gadget.c:__dwc3_gadget_ep_disable() {
	...
	if (dep->flags & DWC3_EP_DELAY_STOP)
		mask |= (DWC3_EP_DELAY_STOP | DWC3_EP_TRANSFER_STARTED);
	dep->flags &= mask;

	/* Clear out the ep descriptors for non-ep0 */
	if (dep->number > 1) {
		dep->endpoint.comp_desc = NULL;
		dep->endpoint.desc = NULL;
	}
	...
}

If the in-flight End Transfer command completes before dwc3_ep0_out_start()
runs, dwc3_gadget_endpoint_command_complete() now clears DWC3_EP_DELAY_STOP
and only calls dwc3_gadget_ep_cleanup_cancelled_requests(dep), which does
not clean up dep->started_list or dep->pending_list, and stops after the
first request in dep->cancelled_list when the endpoint is disabled:

drivers/usb/dwc3/gadget.c:dwc3_gadget_ep_cleanup_cancelled_requests() {
	...
		/*
		 * The endpoint is disabled, let the dwc3_remove_requests()
		 * handle the cleanup.
		 */
		if (!dep->endpoint.desc)
			break;
	}
}

Because DWC3_EP_DELAY_STOP has already been cleared, when
dwc3_ep0_out_start() later runs on entering EP0_SETUP_PHASE, it will skip
the endpoint instead of finishing the deferred dwc3_remove_requests()
cleanup:

drivers/usb/dwc3/ep0.c:dwc3_ep0_out_start() {
	...
		if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
			continue;

		dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
		if (dwc->connected)
			dwc3_stop_active_transfer(dwc3_ep, false, true);
		else
			dwc3_remove_requests(dwc, dwc3_ep, -ESHUTDOWN);
	...
}

Additionally, in the dwc3_gadget_soft_disconnect() path where
!dwc->connected (while ep0state is still EP0_STATUS_PHASE after
dwc3_ep0_send_delayed_status() clears dwc->delayed_status),
dep->endpoint.desc is still non-NULL and DWC3_EP_ENABLED is still set. If
DWC3_EP_DELAY_START was set prior to disconnect, could
dwc3_gadget_endpoint_command_complete() also call
__dwc3_gadget_kick_transfer(dep) and start a new hardware transfer from
dep->pending_list while disconnecting?

Should dwc3_gadget_endpoint_command_complete() check whether the endpoint
was disabled (!dep->endpoint.desc) or disconnected (!dwc->connected) when
DWC3_EP_DELAY_STOP was set and invoke dwc3_remove_requests(dwc, dep,
-ESHUTDOWN) instead of kicking delayed transfers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792542945.git.Thinh.Nguyen@synopsys.com?part=3