Re: [PATCH] usb: dwc2: gadget: Restart the gadget when a new session follows a session end

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: dwc2: gadget: lx_state clobber skips dwc2_hsotg_session_start()
--

commit da705039966ae36f7cf570e450884a50dcb8e634
Author: Olivier Louvignes <olivier@mgcrea.io>

usb: dwc2: gadget: Restart the gadget when a new session follows a session end

This patch restarts the dwc2 gadget on a session request interrupt
(GINTSTS_SESSREQINT) when a prior B-session end left the controller in
DWC2_L3, allowing enumeration to succeed on cable replug when no external
VBUS notifier is present.

> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 9565e6a52725a..836f123d294e7 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -322,6 +322,9 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
>  		 * established
>  		 */
>  		dwc2_hsotg_disconnect(hsotg);
> +
> +		/* Start again if a session end left the gadget in L3. */
> +		dwc2_hsotg_session_start(hsotg);
>  	} else {
>  		/* Turn on the port power bit. */
>  		hprt0 = dwc2_read_hprt0(hsotg);
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 102169b404196..8abae65a98387 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -4718,6 +4718,35 @@ static int dwc2_hsotg_pullup(struct usb_gadget *gadget, int is_on)
>  	return ret;
>  }
>  
> +/**
> + * dwc2_hsotg_session_start - start the gadget again for a new session
> + * @hsotg: The driver state
> + *
> + * A session end (the B-session going invalid, as when the cable is pulled)
> + * disconnects the gadget and leaves it in L3. Unless something outside the
> + * controller reports VBUS through usb_gadget_vbus_connect(), nothing brings
> + * it back: the next USB reset does not reinitialise a gadget that is not
> + * connected, and EP0 then refuses its setup request (-EAGAIN) at every
> + * enumeration. Start it again as dwc2_hsotg_vbus_session() would.
> + *
> + * Called with hsotg->lock held, when a new session is requested.
> + */
> +void dwc2_hsotg_session_start(struct dwc2_hsotg *hsotg)
> +{
> +	if (hsotg->lx_state != DWC2_L3)
> +		return;
> +
> +	dev_dbg(hsotg->dev, "%s: new session after session end\n", __func__);
> +
> +	hsotg->op_state = OTG_STATE_B_PERIPHERAL;
> +	dwc2_hsotg_core_init_disconnected(hsotg, false);
> +	if (hsotg->enabled) {
> +		/* Enable ACG feature in device mode, if supported */
> +		dwc2_enable_acg(hsotg);
> +		dwc2_hsotg_core_connect(hsotg);
> +	}
> +}

[Severity: High]
What happens here if the controller entered bus suspend (partial power down
or gadget clock gating) before the previous session ended?

When a connected gadget sees bus inactivity before VBUS drops below the
B-session end threshold, GINTSTS_USBSUSP transitions the controller into
partial power down (setting hsotg->in_ppd = 1 and hsotg->lx_state = DWC2_L2
in dwc2_gadget_enter_partial_power_down()) or gadget clock gating (setting
hsotg->bus_suspended = true and hsotg->lx_state = DWC2_L2 in
dwc2_gadget_enter_clock_gating()).

When GOTGINT_SES_END_DET then fires in dwc2_handle_otg_intr(), it calls
dwc2_hsotg_disconnect(), which sets hsotg->connected = 0 and
hsotg->lx_state = DWC2_L3, while hsotg->in_ppd or hsotg->bus_suspended
remains set.

On replug, dwc2_handle_session_req_intr() runs:

drivers/usb/dwc2/core_intr.c:dwc2_handle_session_req_intr() {
	if (dwc2_is_device_mode(hsotg)) {
		if (hsotg->lx_state != DWC2_L0) {
			if (hsotg->in_ppd) {
				ret = dwc2_exit_partial_power_down(hsotg, 0,
								   true);
				...
			}

			/* Exit gadget mode clock gating. */
			if (hsotg->params.power_down ==
			    DWC2_POWER_DOWN_PARAM_NONE && hsotg->bus_suspended &&
			    !hsotg->params.no_clock_gating)
				dwc2_gadget_exit_clock_gating(hsotg, 0);
		}

		dwc2_hsotg_disconnect(hsotg);

		/* Start again if a session end left the gadget in L3. */
		dwc2_hsotg_session_start(hsotg);
}

Both dwc2_exit_partial_power_down() -> dwc2_gadget_exit_partial_power_down()
and dwc2_gadget_exit_clock_gating() unconditionally set:

	hsotg->lx_state = DWC2_L0;

And because GOTGINT_SES_END_DET already cleared hsotg->connected to 0, the
subsequent call to dwc2_hsotg_disconnect() returns immediately at:

	if (!hsotg->connected)
		return;

without setting hsotg->lx_state back to DWC2_L3.

Won't dwc2_hsotg_session_start() then see hsotg->lx_state == DWC2_L0 and
return early without reinitializing or reconnecting the gadget, leaving
hsotg->connected == 0 (and, in the in_ppd case, leaving stale pre-disconnect
register state restored in the hardware by dwc2_exit_partial_power_down()
without a subsequent core reset)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023091832.6765-1-olivier@mgcrea.io?part=1