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

[Felipe Balbi <balbi@kernel.org>]

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?

> 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.

> 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 :-)

-- 
balbi