[PATCH v2 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 Reviewed-by: Marek Vasut marex@denx.de ---
Changes in v2: - use wait_for_bit_le32(), clrbits_le32
I didn't use setbits_le32(), because the epflush register ignores 0 bits.
Thanks for the review!
drivers/usb/gadget/ci_udc.c | 38 +++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)
diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c index 2bfacfe59f..750d471487 100644 --- a/drivers/usb/gadget/ci_udc.c +++ b/drivers/usb/gadget/ci_udc.c @@ -13,6 +13,7 @@ #include <cpu_func.h> #include <net.h> #include <malloc.h> +#include <wait_bit.h> #include <asm/byteorder.h> #include <asm/cache.h> #include <linux/delay.h> @@ -354,12 +355,49 @@ static int ci_ep_enable(struct usb_ep *ep, return 0; }
+static int ep_disable(int num, int in) +{ + struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor; + unsigned int ep_bit, enable_bit; + int err; + + 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); + err = wait_for_bit_le32(&udc->epflush, ep_bit, false, 1000, false); + if (err) + return err; + } while (readl(&udc->epstat) & ep_bit); + + /* clear enable bit */ + clrbits_le32(&udc->epctrl[num], enable_bit); + + return 0; +} + static int ci_ep_disable(struct usb_ep *ep) { struct ci_ep *ci_ep = container_of(ep, struct ci_ep, ep); + int num, in, err; + + num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK; + in = (ci_ep->desc->bEndpointAddress & USB_DIR_IN) != 0; + + err = ep_disable(num, in); + if (err) + return err;
ci_ep->desc = NULL; ep->desc = NULL; + ci_ep->req_primed = false; return 0; }

Add missing newline in pr_err.
Signed-off-by: Simon Holesch simon@holesch.de Reviewed-by: Marek Vasut marex@denx.de ---
Changes in v2: - add commit message body
Thanks for the review!
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/20/23 01:08, Simon Holesch wrote:
Add missing newline in pr_err.
Signed-off-by: Simon Holesch simon@holesch.de Reviewed-by: Marek Vasut marex@denx.de
Changes in v2:
- add commit message body
Thanks for the review!
Thanks for V2, now it's up to Mattijs to sort this out.

Hi Marek,
On lun., nov. 20, 2023 at 03:07, Marek Vasut marex@denx.de wrote:
On 11/20/23 01:08, Simon Holesch wrote:
Add missing newline in pr_err.
Signed-off-by: Simon Holesch simon@holesch.de Reviewed-by: Marek Vasut marex@denx.de
Changes in v2:
- add commit message body
Thanks for the review!
Thanks for V2, now it's up to Mattijs to sort this out.
Thank you for helping with review.
I will apply today and send this to Tom by end of this week (if no CI errors)
Regards Mattijs

Hi Simon,
Thank you for your patch.
On lun., nov. 20, 2023 at 01:08, Simon Holesch simon@holesch.de wrote:
Add missing newline in pr_err.
Signed-off-by: Simon Holesch simon@holesch.de Reviewed-by: Marek Vasut marex@denx.de
Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com
Changes in v2:
- add commit message body
Thanks for the review!
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");
-- 2.42.1

Hi Simon,
Thank you for your patch.
On lun., nov. 20, 2023 at 01:08, Simon Holesch simon@holesch.de 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 Reviewed-by: Marek Vasut marex@denx.de
Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com
Changes in v2:
- use wait_for_bit_le32(), clrbits_le32
I didn't use setbits_le32(), because the epflush register ignores 0 bits.
Thanks for the review!
drivers/usb/gadget/ci_udc.c | 38 +++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)
diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c index 2bfacfe59f..750d471487 100644 --- a/drivers/usb/gadget/ci_udc.c +++ b/drivers/usb/gadget/ci_udc.c @@ -13,6 +13,7 @@ #include <cpu_func.h> #include <net.h> #include <malloc.h> +#include <wait_bit.h> #include <asm/byteorder.h> #include <asm/cache.h> #include <linux/delay.h> @@ -354,12 +355,49 @@ static int ci_ep_enable(struct usb_ep *ep, return 0; }
+static int ep_disable(int num, int in) +{
- struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
- unsigned int ep_bit, enable_bit;
- int err;
- 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);
err = wait_for_bit_le32(&udc->epflush, ep_bit, false, 1000, false);
if (err)
return err;
- } while (readl(&udc->epstat) & ep_bit);
- /* clear enable bit */
- clrbits_le32(&udc->epctrl[num], enable_bit);
- return 0;
+}
static int ci_ep_disable(struct usb_ep *ep) { struct ci_ep *ci_ep = container_of(ep, struct ci_ep, ep);
int num, in, err;
num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
in = (ci_ep->desc->bEndpointAddress & USB_DIR_IN) != 0;
err = ep_disable(num, in);
if (err)
return err;
ci_ep->desc = NULL; ep->desc = NULL;
ci_ep->req_primed = false; return 0;
}
-- 2.42.1

Hi,
On Mon, 20 Nov 2023 01:08:34 +0100, 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.
[...]
Thanks, Applied to https://source.denx.de/u-boot/custodians/u-boot-dfu (u-boot-dfu)
[1/2] usb: ci: Fix gadget reinit https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/b272c87925025a4... [2/2] usb: fastboot: Add missing newline in pr_err https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/6f68dcd0259f9f1...
-- Mattijs
participants (3)
-
Marek Vasut
-
Mattijs Korpershoek
-
Simon Holesch