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

From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
To: Jack Pham <jackp@codeaurora.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Felipe Balbi <balbi@kernel.org>,
	Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	Wesley Cheng <wcheng@codeaurora.org>
Cc: "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"linux-arm-msm@vger.kernel.org" <linux-arm-msm@vger.kernel.org>
Subject: Re: [PATCH] usb: dwc3: gadget: Skip resizing EP's TX FIFO if already resized
Date: Fri, 10 Sep 2021 01:15:22 +0000	[thread overview]
Message-ID: <6a4bb7a9-2c63-5e1e-f4fc-a5bbc7aaa168@synopsys.com> (raw)
In-Reply-To: <20210909083120.15350-1-jackp@codeaurora.org>

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
> 
> This is easily fixed by bailing out of dwc3_gadget_resize_tx_fifos()
> if an endpoint is already resized, avoiding the calculation error
> resulting from accumulating the EP's FIFO depth repeatedly.
> 
> Fixes: 9f607a309fbe9 ("usb: dwc3: Resize TX FIFOs to meet EP bursting requirements")
> Signed-off-by: Jack Pham <jackp@codeaurora.org>
> ---
>  drivers/usb/dwc3/gadget.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 804b50548163..c647c76d7361 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -747,6 +747,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 (dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(dep->number >> 1)))
> +		return 0;
> +
>  	ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7);
>  
>  	if ((dep->endpoint.maxburst > 1 &&
> 

This seems like a workaround more than a fix. As previously pointed out,
there will be problems when there are multiple alternate setting
interface [2]. If we're doing this way, are we properly allocating the
fifo size for the alternate setting that requires the most fifo size and
not just the first alt-setting 0? Also different alt-setting can have
different endpoints, the logic handling this may not be simple.

There are a few review comments for Wesley. Hopefully they get resolved
eventually.

[1]
https://lore.kernel.org/linux-usb/b65463e9-3a8d-1ee5-3e26-09990aa8ec53@synopsys.com/

[2]
https://lore.kernel.org/linux-usb/0f8384bc-18a6-8ca5-c38b-6b5523c60a68@synopsys.com/

Thanks,
Thinh