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

[Jack Pham <jackp@codeaurora.org>]

Hi Felipe,

On Thu, Sep 09, 2021 at 11:41:38AM +0300, Felipe Balbi wrote:
> Jack Pham <jackp@codeaurora.org> writes:
> 
> > 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;
> > +
> 
> heh, not to say "I told you so", but...
> 
> That being said, your test is not very good. The whole idea for resizing
> the FIFOs is that in some applications we only use e.g. 2 endpoints and
> there is considerable FIFO space left unused.
> 
> The goal is to use that unused FIFO space to squeeze more throughput out
> of the pipe, since it amortizes SW latency.
> 
> This patch is essentially the same as reverting the original commit :-)

No, it's not quite the same as nullifying the resizing algorithm.  This
patch is predicated on a key part of the resizing algorithm where
dwc3_gadget_clear_tx_fifos() occurs upon receiving Set_Configuration in
ep0.c.  Which means that each new connection starts off with a blank
slate with all the GTXFIFOSIZ(n) registers cleared.  Then each EP gets
resized one at a time when usb_ep_enable() is called.

The problem this patch is fixing is avoiding *re-resizing*, the idea
being that if an EP was already resized once during a session (from
Set Configuration until the next reset or disconnect), then 
it should be good to go even if it gets disabled and re-enabled again.
Since we lack any boolean state variable in struct dwc3_ep reflecting
whether it had already been resized, re-reading the GTXFIFOSIZ register
is the next best equivalent.  Note also that this check occurs after
the if (!dwc->do_fifo_resize) check so this is applicable only if the
entire "tx-fifo-resize" mechanism is enabled.

Jack
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project