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

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

[Olivier Louvignes]

When the B-session becomes invalid (the cable is pulled),
dwc2_hsotg_disconnect() disconnects the gadget and leaves it in L3.
Nothing brings it back, unless something outside the controller reports
VBUS through usb_gadget_vbus_connect(): the next USB reset does not
reinitialise a gadget that is not connected, and dwc2_hsotg_ep_queue()
then refuses EP0's setup request because lx_state is not L0. Every
enumeration after a replug fails with

  dwc2_hsotg_enqueue_setup: failed queue (-11)

On the session request interrupt that a new session raises, start the
gadget again when it is in L3, as dwc2_hsotg_vbus_session() does when a
session becomes active.

Seen on an Ingenic X1600 (HiBy R1) in peripheral mode with g_serial and
no VBUS notifier. After a replug the host enumerates the gadget again
and the serial shell comes back, without a reboot. Tested on v7.3.

Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
---
Tested on one board only, in peripheral mode. The alternative is for the
platform glue to report VBUS with usb_gadget_vbus_connect() (a
usb_role_switch or extcon notifier), which this board does not have: its
VBUS is seen by a PMIC with no such driver. If that is the preferred
place for this, I will look at doing it there instead. The patch is
build-tested on 734bef548a96 (v7.3-7363) with and without gadget support.

 drivers/usb/dwc2/core.h      |  2 ++
 drivers/usb/dwc2/core_intr.c |  3 +++
 drivers/usb/dwc2/gadget.c    | 29 +++++++++++++++++++++++++++++
 3 files changed, 34 insertions(+)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 767251aa1aa3..3d40eacc3d2e 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -1421,6 +1421,7 @@ void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *dwc2,
 				       bool reset);
 void dwc2_hsotg_core_disconnect(struct dwc2_hsotg *hsotg);
 void dwc2_hsotg_core_connect(struct dwc2_hsotg *hsotg);
+void dwc2_hsotg_session_start(struct dwc2_hsotg *hsotg);
 void dwc2_hsotg_disconnect(struct dwc2_hsotg *dwc2);
 int dwc2_hsotg_set_test_mode(struct dwc2_hsotg *hsotg, int testmode);
 #define dwc2_is_device_connected(hsotg) (hsotg->connected)
@@ -1459,6 +1460,7 @@ static inline void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *dwc2,
 						     bool reset) {}
 static inline void dwc2_hsotg_core_disconnect(struct dwc2_hsotg *hsotg) {}
 static inline void dwc2_hsotg_core_connect(struct dwc2_hsotg *hsotg) {}
+static inline void dwc2_hsotg_session_start(struct dwc2_hsotg *hsotg) {}
 static inline void dwc2_hsotg_disconnect(struct dwc2_hsotg *dwc2) {}
 static inline int dwc2_hsotg_set_test_mode(struct dwc2_hsotg *hsotg,
 					   int testmode)
diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index 9565e6a52725..836f123d294e 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 102169b40419..8abae65a9838 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);
+	}
+}
+
 static int dwc2_hsotg_vbus_session(struct usb_gadget *gadget, int is_active)
 {
 	struct dwc2_hsotg *hsotg = to_hsotg(gadget);

base-commit: 734bef548a96e8774d6eef5dfa2026c6edc4e80b
-- 
2.39.5

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

[sashiko-bot]

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