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

[Jack Pham <jackp@codeaurora.org>]

On Fri, Sep 10, 2021 at 08:17:51AM +0300, Felipe Balbi wrote:
> 
> Hi Jack,
> 
> Jack Pham <jackp@codeaurora.org> writes:
> >> > 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.
> 
> that's not a safe assumption, though. What happens in cases where
> Configuration 1 is wildly different from Configuration 2? Say Config 1
> is a mass storage device and Config 2 is a collection of several CDC
> interfaces?

The resizing algorithm operates only on one Configuration at a time.
If a new Configuration is selected by the host, the algorithm starts
over again, so all of the endpoints in the new config will need to be
evaluated and resized again.  The number of total endpoints in a
configuration (including across multiple Alt Settings) is taken into
account to ensure there is enough room to guarantee each IN endpoint
will have a minimum amount of FIFO space, but also allowing an
opportunity for those endpoints who could benefit (based on their
MaxBurst setting) to be allocated more.

> > Since we lack any boolean state variable in struct dwc3_ep reflecting
> > whether it had already been resized, re-reading the GTXFIFOSIZ register
> 
> it might be a better idea to introduce such a flag and make the
> intention clear. But in any case, I still think the assumption you're
> making is not very good.

IMHO I think it's pretty straightforward, as the GTXFIFOSIZ(n) register
must have a valid value for the EP to be operational.  The assumption is
if it's 0, that means it was cleared by dwc3_gadget_clear_tx_fifos() and
we haven't yet visited this EP for resizing.  So the check within this
resize routine is basically a combination of the following:

 if (dwc->do_fifo_resize && DWC3_GTXFIFOSIZ(N) != 0)
    // this IN EP has been resized

But I hear you about using an explicit flag as it would make it crystal
clear to the reader.  I can prepare something for V2.

> > 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.
> 
> Right, that's fine :-)

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