Re: [PATCH v2] usb: dwc3: gadget: Skip resizing EP's TX FIFO if already resized

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

Jack Pham wrote:
> Some functions may dynamically enable and disable their endpoints
> regularly throughout their operation, particularly when Set Interface
> is employed to switch between Alternate Settings.  For instance the
> UAC2 function has its respective endpoints for playback & capture
> associated with AltSetting 1, in which case those endpoints would not
> get enabled until the host activates the AltSetting.  And they
> conversely become disabled when the interfaces' AltSetting 0 is
> chosen.
> 
> With the DWC3 FIFO resizing algorithm recently added, every
> usb_ep_enable() call results in a call to resize that EP's TXFIFO,
> but if the same endpoint is enabled again and again, this incorrectly
> leads to FIFO RAM allocation exhaustion as the mechanism did not
> account for the possibility that endpoints can be re-enabled many
> times.
> 
> Example log splat:
> 
> 	dwc3 a600000.dwc3: Fifosize(3717) > RAM size(3462) ep3in depth:217973127
> 	configfs-gadget gadget: u_audio_start_capture:521 Error!
> 	dwc3 a600000.dwc3: request 000000000be13e18 was not queued to ep3in
> 
> Add another bit DWC3_EP_TXFIFO_RESIZED to dep->flags to keep track of
> whether an EP had already been resized in the current configuration.
> If so, bail out of dwc3_gadget_resize_tx_fifos() to avoid the
> calculation error resulting from accumulating the EP's FIFO depth
> repeatedly.  This flag is retained across multiple ep_disable() and
> ep_enable() calls and is cleared when GTXFIFOSIZn is reset in
> dwc3_gadget_clear_tx_fifos() upon receiving the next Set Config.
> 
> Fixes: 9f607a309fbe9 ("usb: dwc3: Resize TX FIFOs to meet EP bursting requirements")
> Signed-off-by: Jack Pham <jackp@codeaurora.org>
> ---
> v2: Added explicit flag to dep->flags and check that instead of directly
>     reading the GTXFIFOSIZn register.
> 
>  drivers/usb/dwc3/core.h   | 1 +
>  drivers/usb/dwc3/gadget.c | 8 +++++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 5612bfdf37da..f033063f6948 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -723,6 +723,7 @@ struct dwc3_ep {
>  #define DWC3_EP_FORCE_RESTART_STREAM	BIT(9)
>  #define DWC3_EP_FIRST_STREAM_PRIMED	BIT(10)
>  #define DWC3_EP_PENDING_CLEAR_STALL	BIT(11)
> +#define DWC3_EP_TXFIFO_RESIZED	BIT(12)
>  
>  	/* This last one is specific to EP0 */
>  #define DWC3_EP0_DIR_IN		BIT(31)
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 4519d06c9ca2..ed97e47d3261 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -702,6 +702,7 @@ void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc)
>  				   DWC31_GTXFIFOSIZ_TXFRAMNUM;
>  
>  		dwc3_writel(dwc->regs, DWC3_GTXFIFOSIZ(num >> 1), size);
> +		dep->flags &= ~DWC3_EP_TXFIFO_RESIZED;
>  	}
>  	dwc->num_ep_resized = 0;
>  }
> @@ -747,6 +748,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep)
>  	if (!usb_endpoint_dir_in(dep->endpoint.desc) || dep->number <= 1)
>  		return 0;
>  
> +	/* bail if already resized */
> +	if (dep->flags & DWC3_EP_TXFIFO_RESIZED)
> +		return 0;
> +
>  	ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7);
>  
>  	if ((dep->endpoint.maxburst > 1 &&
> @@ -807,6 +812,7 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep)
>  	}
>  
>  	dwc3_writel(dwc->regs, DWC3_GTXFIFOSIZ(dep->number >> 1), fifo_size);
> +	dep->flags |= DWC3_EP_TXFIFO_RESIZED;
>  	dwc->num_ep_resized++;
>  
>  	return 0;
> @@ -995,7 +1001,7 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
>  
>  	dep->stream_capable = false;
>  	dep->type = 0;
> -	dep->flags = 0;
> +	dep->flags &= DWC3_EP_TXFIFO_RESIZED;
>  
>  	return 0;
>  }
> 

Reviewed-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh