[PATCH 1/2] usb: ci: Fix gadget reinit

The ChipIdea device controller wasn't properly cleaned up when disabled. So enabling it again left it in a broken state. The problem occurred for example when the host unbinds the driver and binds it again.
During the first setup, when the out request is queued, the endpoint is primed (`epprime`). If the endpoint is then disabled, it stayed primed with the initial buffer. So after the endpoint is re-enabled, the device controller and device driver were out of sync: the new out request was in the driver queue head, yet not submitted, but the "complete" function was still called, since the endpoint was primed with the old buffer.
With the fastboot function this error led to the (rather confusing) error message "buffer overflow".
Fixed by clearing the primed buffers with the `epflush` (`ENDPTFLUSH`) register.
Signed-off-by: Simon Holesch simon@holesch.de --- drivers/usb/gadget/ci_udc.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)
diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c index 2bfacfe59f..4ad991c72b 100644 --- a/drivers/usb/gadget/ci_udc.c +++ b/drivers/usb/gadget/ci_udc.c @@ -354,12 +354,45 @@ static int ci_ep_enable(struct usb_ep *ep, return 0; }
+static void ep_disable(int num, int in) +{ + struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor; + unsigned int ep_bit, n, enable_bit; + + if (in) { + ep_bit = EPT_TX(num); + enable_bit = CTRL_TXE; + } else { + ep_bit = EPT_RX(num); + enable_bit = CTRL_RXE; + } + + /* clear primed buffers */ + do { + writel(ep_bit, &udc->epflush); + while (readl(&udc->epflush) & ep_bit) + ; + } while (readl(&udc->epstat) & ep_bit); + + /* clear enable bit */ + n = readl(&udc->epctrl[num]); + n &= ~enable_bit; + writel(n, &udc->epctrl[num]); +} + static int ci_ep_disable(struct usb_ep *ep) { struct ci_ep *ci_ep = container_of(ep, struct ci_ep, ep); + int num, in; + + num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK; + in = (ci_ep->desc->bEndpointAddress & USB_DIR_IN) != 0; + + ep_disable(num, in);
ci_ep->desc = NULL; ep->desc = NULL; + ci_ep->req_primed = false; return 0; }

Signed-off-by: Simon Holesch simon@holesch.de --- drivers/usb/gadget/f_fastboot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 741775a7bc..9f322c9550 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -520,7 +520,7 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req) cmdbuf[req->actual] = '\0'; cmd = fastboot_handle_command(cmdbuf, response); } else { - pr_err("buffer overflow"); + pr_err("buffer overflow\n"); fastboot_fail("buffer overflow", response); }

On 11/19/23 02:04, Simon Holesch wrote:
Please also add missing commit message (can be just the subject copied into the body)
Signed-off-by: Simon Holesch simon@holesch.de
drivers/usb/gadget/f_fastboot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 741775a7bc..9f322c9550 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -520,7 +520,7 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req) cmdbuf[req->actual] = '\0'; cmd = fastboot_handle_command(cmdbuf, response); } else {
pr_err("buffer overflow");
fastboot_fail("buffer overflow", response); }pr_err("buffer overflow\n");
With that fixed:
Reviewed-by: Marek Vasut marex@denx.de

On 11/19/23 02:04, Simon Holesch wrote:
The ChipIdea device controller wasn't properly cleaned up when disabled. So enabling it again left it in a broken state. The problem occurred for example when the host unbinds the driver and binds it again.
During the first setup, when the out request is queued, the endpoint is primed (`epprime`). If the endpoint is then disabled, it stayed primed with the initial buffer. So after the endpoint is re-enabled, the device controller and device driver were out of sync: the new out request was in the driver queue head, yet not submitted, but the "complete" function was still called, since the endpoint was primed with the old buffer.
With the fastboot function this error led to the (rather confusing) error message "buffer overflow".
Fixed by clearing the primed buffers with the `epflush` (`ENDPTFLUSH`) register.
Signed-off-by: Simon Holesch simon@holesch.de
drivers/usb/gadget/ci_udc.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)
diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c index 2bfacfe59f..4ad991c72b 100644 --- a/drivers/usb/gadget/ci_udc.c +++ b/drivers/usb/gadget/ci_udc.c @@ -354,12 +354,45 @@ static int ci_ep_enable(struct usb_ep *ep, return 0; }
+static void ep_disable(int num, int in) +{
- struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
- unsigned int ep_bit, n, enable_bit;
- if (in) {
ep_bit = EPT_TX(num);
enable_bit = CTRL_TXE;
- } else {
ep_bit = EPT_RX(num);
enable_bit = CTRL_RXE;
- }
Please use setbits_le32()
- /* clear primed buffers */
- do {
writel(ep_bit, &udc->epflush);
while (readl(&udc->epflush) & ep_bit)
;
- } while (readl(&udc->epstat) & ep_bit);
wait_for_bit_le32() with suitable timeout
- /* clear enable bit */
- n = readl(&udc->epctrl[num]);
- n &= ~enable_bit;
- writel(n, &udc->epctrl[num]);
clrbits_le32()
+}
static int ci_ep_disable(struct usb_ep *ep) { struct ci_ep *ci_ep = container_of(ep, struct ci_ep, ep);
int num, in;
num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
in = (ci_ep->desc->bEndpointAddress & USB_DIR_IN) != 0;
ep_disable(num, in);
ci_ep->desc = NULL; ep->desc = NULL;
ci_ep->req_primed = false; return 0; }
With those fixed:
Reviewed-by: Marek Vasut marex@denx.de
Thanks !
participants (2)
-
Marek Vasut
-
Simon Holesch