Re: [PATCH 6/7] usb: dwc3: gadget: Reset endpoint flags on controller start

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: dwc3: gadget: __dwc3_gadget_start() clears DWC3_EP_ENABLED on active eps
--

commit 4093b4f01f6fe6cc6713a8ff97925d5c1b4491d8
Author: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

usb: dwc3: gadget: Reset endpoint flags on controller start

This commit resets the endpoint flags of all non-control endpoints in
__dwc3_gadget_start(), preserving only DWC3_EP_TXFIFO_RESIZED and
DWC3_EP_RESOURCE_ALLOCATED so stale flags from a pending End Transfer command
do not persist across a soft disconnect or error recovery.

> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index e531adc02a401..a8f12e583a0f4 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2972,6 +2972,7 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
>  {
>  	struct dwc3_ep		*dep;
>  	int			ret = 0;
> +	u32			epnum;
>  	u32			reg;
>  
>  	/*
> @@ -3029,6 +3030,15 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
>  		return ret;
>  	}
>  
> +	for (epnum = 2; epnum < dwc->num_eps; epnum++) {
> +		dep = dwc->eps[epnum];
> +		if (!dep)
> +			continue;
> +
> +		dep->flags &= (DWC3_EP_TXFIFO_RESIZED |
> +			       DWC3_EP_RESOURCE_ALLOCATED);
> +	}

[Severity: High]
What happens if __dwc3_gadget_start() is called while non-control endpoints
are still enabled by the gadget driver (dep->endpoint.enabled is true)?

For example, in dwc3_gadget_suspend(), if dwc3_gadget_soft_disconnect() fails,
dwc3_gadget_soft_connect() -> __dwc3_gadget_start() is called before
dwc3_disconnect_gadget_sleepable() runs:

	ret = dwc3_gadget_soft_disconnect(dwc);
	if (ret && dwc->softconnect) {
		dwc3_gadget_soft_connect(dwc);
		return -EAGAIN;
	}

	dwc3_disconnect_gadget_sleepable(dwc);

Similarly, in dwc3_err_recovery_work(), dwc3_gadget_suspend() /
dwc3_gadget_resume(), or dwc3_gadget_pullup(), gadget drivers such as
raw_gadget and gadgetfs defer usb_ep_disable() to userspace rather than
disabling endpoints synchronously during their disconnect callback, so
endpoints can still be enabled when __dwc3_gadget_start() executes.

Clearing DWC3_EP_ENABLED here while dep->endpoint.enabled remains true causes
a subsequent usb_ep_disable() -> dwc3_gadget_ep_disable() call to hit
dev_WARN_ONCE() and return early without calling __dwc3_gadget_ep_disable():

	if (dev_WARN_ONCE(dwc->dev, !(dep->flags & DWC3_EP_ENABLED),
					"%s is already disabled\n",
					dep->name))
		return 0;

	spin_lock_irqsave(&dwc->lock, flags);
	ret = __dwc3_gadget_ep_disable(dep);
	spin_unlock_irqrestore(&dwc->lock, flags);

Because __dwc3_gadget_ep_disable() is skipped, dwc3_remove_requests() is not
run and dep->stream_capable, dep->type, dep->endpoint.comp_desc, and
dep->endpoint.desc are not cleared:

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

In drivers/usb/gadget/legacy/raw_gadget.c, raw_ioctl_ep_disable() frees
ep->desc immediately after usb_ep_disable(), which leaves dep->endpoint.desc
as a dangling pointer:

	usb_ep_disable(dev->eps[i].ep);

	spin_lock_irqsave(&dev->lock, flags);
	usb_ep_free_request(dev->eps[i].ep, dev->eps[i].req);
	kfree(dev->eps[i].ep->desc);

Conversely, if the gadget driver calls usb_ep_enable() while ep->enabled is
still true, usb_ep_enable() returns early without calling into dwc3, leaving
DWC3_EP_ENABLED cleared so subsequent usb_ep_queue() calls fail with
-ESHUTDOWN.

Could this regression be avoided if __dwc3_gadget_start() keeps
dep->flags synchronized with dep->endpoint.enabled, or properly cleans up any
still-enabled endpoints when resetting their flags?

> +
>  	dep = dwc->eps[0];
>  	dep->flags = 0;
>  	ret = __dwc3_gadget_ep_enable(dep, DWC3_DEPCFG_ACTION_INIT);

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