[PATCH] usb: chipidea: udc: give up when the endpoint halt bit is not taken

[PATCH] usb: chipidea: udc: give up when the endpoint halt bit is not taken

[Mario Behn]

hw_ep_set_halt() writes the stall bit of an endpoint and loops until it
reads back. On endpoint 0 the controller clears the stall bit itself when
a SETUP packet arrives, so when one arrives while the driver stalls the
endpoint the loop never ends. It runs in the UDC interrupt thread with the
controller lock held, and the SETUP packet it is waiting out can only be
consumed by that same thread.

Seen on a Zynq-7000 (an ADALM-PLUTO class board, PREEMPT_RT) during the
host RNDIS initialisation: the gadget stalls the GET_ENCAPSULATED_RESPONSE
poll that has no answer yet, the stall is refused, the interrupt thread
spins for a minute, RCU stalls and every other USB operation blocks on the
lock. With a bounded loop the condition shows on 1 to 3 percent of boots
and is harmless: the pending SETUP packet is handled next and the host
repeats its poll.

Retry for about a millisecond, then warn with the endpoint control and
setup status registers and return -EBUSY. The callers already report a
failed stall.

A loop timeout for the same hang was proposed in 2021 (usb: chipidea: add
loop timeout for hw_ep_set_halt(), Jeaho Hwang) and not applied.

Signed-off-by: Mario Behn <mario.behn@gmail.com>
---
 drivers/usb/chipidea/udc.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index d52f89489893..4f4e6d296de5 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -224,20 +224,28 @@ static int hw_ep_prime(struct ci_hdrc *ci, int num, int dir, int is_ctrl)
  */
 static int hw_ep_set_halt(struct ci_hdrc *ci, int num, int dir, int value)
 {
+	enum ci_hw_regs reg = OP_ENDPTCTRL + num;
+	u32 mask_xs = (dir == TX) ? ENDPTCTRL_TXS : ENDPTCTRL_RXS;
+	u32 mask_xr = (dir == TX) ? ENDPTCTRL_TXR : ENDPTCTRL_RXR;
+	int retries = 1000;
+
 	if (value != 0 && value != 1)
 		return -EINVAL;
 
 	do {
-		enum ci_hw_regs reg = OP_ENDPTCTRL + num;
-		u32 mask_xs = (dir == TX) ? ENDPTCTRL_TXS : ENDPTCTRL_RXS;
-		u32 mask_xr = (dir == TX) ? ENDPTCTRL_TXR : ENDPTCTRL_RXR;
-
 		/* data toggle - reserved for EP0 but it's in ESS */
 		hw_write(ci, reg, mask_xs|mask_xr,
 			  value ? mask_xs : mask_xr);
-	} while (value != hw_ep_get_halt(ci, num, dir));
-
-	return 0;
+		if (value == hw_ep_get_halt(ci, num, dir))
+			return 0;
+		udelay(1);
+	} while (--retries);
+
+	dev_warn_ratelimited(ci->dev,
+			     "ep%d%s: halt=%d not taken, ENDPTCTRL 0x%08x ENDPTSETUPSTAT 0x%08x\n",
+			     num, dir == TX ? "in" : "out", value, hw_read(ci, reg, ~0),
+			     hw_read(ci, OP_ENDPTSETUPSTAT, ~0));
+	return -EBUSY;
 }
 
 /**
-- 
2.53.0

Re: [PATCH] usb: chipidea: udc: give up when the endpoint halt bit is not taken

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] usb: chipidea: udc: missing Fixes trailer for hw_ep_set_halt()
  hang fix
--

commit a303d4e574823163c056eaedbfb37d82c4fbaacb
Author: Mario Behn <mario.behn@gmail.com>

> usb: chipidea: udc: give up when the endpoint halt bit is not taken
>
> hw_ep_set_halt() writes the stall bit of an endpoint and loops until it
> reads back. On endpoint 0 the controller clears the stall bit itself when
> a SETUP packet arrives, so when one arrives while the driver stalls the
> endpoint the loop never ends. It runs in the UDC interrupt thread with the
> controller lock held, and the SETUP packet it is waiting out can only be
> consumed by that same thread.
>
> Seen on a Zynq-7000 (an ADALM-PLUTO class board, PREEMPT_RT) during the
> host RNDIS initialisation: the gadget stalls the GET_ENCAPSULATED_RESPONSE
> poll that has no answer yet, the stall is refused, the interrupt thread
> spins for a minute, RCU stalls and every other USB operation blocks on the
> lock. With a bounded loop the condition shows on 1 to 3 percent of boots
> and is harmless: the pending SETUP packet is handled next and the host
> repeats its poll.
>
> Retry for about a millisecond, then warn with the endpoint control and
> setup status registers and return -EBUSY. The callers already report a
> failed stall.
>
> A loop timeout for the same hang was proposed in 2021 (usb: chipidea: add
> loop timeout for hw_ep_set_halt(), Jeaho Hwang) and not applied.
>
> Signed-off-by: Mario Behn <mario.behn@gmail.com>

[Severity: Low]
This fixes the unbounded polling loop in hw_ep_set_halt() introduced with the
initial driver.  Should this include:

  Fixes: aa69a8093ff9 ("USB: gadget: MIPS ci13xxx_udc")

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105205343.982465-1-mario.behn@gmail.com?part=1