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

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

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