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

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

[Simon Holesch]

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;
 }
 
-- 
2.42.1

[PATCH v2 2/2] usb: fastboot: Add missing newline in pr_err

[Simon Holesch]

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);
 	}
 
-- 
2.42.1

Re: [PATCH v2 2/2] usb: fastboot: Add missing newline in pr_err

[Marek Vasut]

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.

Re: [PATCH v2 2/2] usb: fastboot: Add missing newline in pr_err

[Mattijs Korpershoek]

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

Re: [PATCH v2 2/2] usb: fastboot: Add missing newline in pr_err

[Mattijs Korpershoek]

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");
> +		pr_err("buffer overflow\n");
>  		fastboot_fail("buffer overflow", response);
>  	}
>  
> -- 
> 2.42.1

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

[Mattijs Korpershoek]

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

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

[Mattijs Korpershoek]

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/b272c87925025a44ad6ff851711c68847a4cff4a
[2/2] usb: fastboot: Add missing newline in pr_err
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/6f68dcd0259f9f1c03f097669e9490b13b5325c7

--
Mattijs