usb: dwc3: gadget: Skip resizing EP's TX FIFO if already resized

Commit Message

Jack Pham Sept. 9, 2021, 8:31 a.m. UTC

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 <[e-mail]>
---
 drivers/usb/dwc3/gadget.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Felipe Balbi Sept. 9, 2021, 8:41 a.m. UTC | #1

Hi,

Jack Pham <[e-mail]> 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 <[e-mail]>
> ---
>  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 :-)

Jack Pham Sept. 9, 2021, 5:02 p.m. UTC | #2

Hi Felipe,

On Thu, Sep 09, 2021 at 11:41:38AM +0300, Felipe Balbi wrote:
> Jack Pham <[e-mail]> 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 <[e-mail]>
> > ---
> >  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

Thinh Nguyen Sept. 10, 2021, 1:15 a.m. UTC | #3

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 <[e-mail]>
> ---
>  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/[e-mail]/

[2]
https://lore.kernel.org/linux-usb/[e-mail]/

Thanks,
Thinh

Felipe Balbi Sept. 10, 2021, 5:17 a.m. UTC | #4

Hi Jack,

Jack Pham <[e-mail]> 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 :-)

Jack Pham Sept. 10, 2021, 5:20 p.m. UTC | #5

On Fri, Sep 10, 2021 at 08:17:51AM +0300, Felipe Balbi wrote:
> 
> Hi Jack,
> 
> Jack Pham <[e-mail]> 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

Wesley Cheng Sept. 11, 2021, 1:29 a.m. UTC | #6

On 9/9/2021 6:15 PM, Thinh Nguyen wrote:
> 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 <[e-mail]>
>> ---
>>  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 &&
>>

Hi Thinh,

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

Each alt interface will call usb_ep_autoconfig() which should be
assigned different endpoint numbers.  This would mean that if alt intf#0
gets selected, and EP is enabled, then we will resize the TXFIFO and map
that FIFO to the physical EP.  Then when/if the host requests the other
alt intf#1, and that calls EP enable, then the logic will then attempt
to resize based on the parameters, and again map that FIFO to the
physical EP. (since we call autoconfig on all interfaces, they should be
assigned different endpoints)

I agree that there is currently a limitation because we are going to
reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is
only 1 interface active at a time.  The missing logic that we might be
missing is seeing how we can re-purpose the FIFO that is being disabled.
 However, I think Jack's fix here would be applicable to the improvement
in logic to re-use/re-assign FIFO space allocated by disabled EPs also.

> different endpoints, the logic handling this may not be simple.
> 
> There are a few review comments for Wesley. Hopefully they get resolved
> eventually.

As mentioned above, there is a lot of considerations we need to make
when looking at the amount of combinations that can be done for a USB
configuration.  We obviously want to see if we can find a way to
re-allocate FIFO space, but it gets complicated if we run into a
"fragmented" situation where the RAM associated to the EP being
re-allocated is in between 2 that are active.

Thanks
Wesley Cheng

Thinh Nguyen Sept. 11, 2021, 3:08 a.m. UTC | #7

Wesley Cheng wrote:
> 
> 
> On 9/9/2021 6:15 PM, Thinh Nguyen wrote:
>> 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 <[e-mail]>
>>> ---
>>>  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 &&
>>>
> 
> Hi Thinh,
> 
>>
>> 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
> 
> Each alt interface will call usb_ep_autoconfig() which should be
> assigned different endpoint numbers.  This would mean that if alt intf#0
> gets selected, and EP is enabled, then we will resize the TXFIFO and map
> that FIFO to the physical EP.  Then when/if the host requests the other
> alt intf#1, and that calls EP enable, then the logic will then attempt
> to resize based on the parameters, and again map that FIFO to the
> physical EP. (since we call autoconfig on all interfaces, they should be
> assigned different endpoints)

That's not true. Different alt-settings of an interface can share
endpoint numbers. This is often the case for UASP driver where
alt-setting 0 is for BOT protocol and alt-setting 1 is UASP. When we
switch alt-setting, we disable the current endpoints and enable the
old/new ones.

> 
> I agree that there is currently a limitation because we are going to
> reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is
> only 1 interface active at a time.  The missing logic that we might be
> missing is seeing how we can re-purpose the FIFO that is being disabled.
>  However, I think Jack's fix here would be applicable to the improvement
> in logic to re-use/re-assign FIFO space allocated by disabled EPs also.
> 

Improvement is always great. I just hope we don't just stop where we are
now. Since you're working on this feature at the moment, it would be
good to also resolve some of the outstanding issues as Jack's fix seems
to be incomplete.

>> different endpoints, the logic handling this may not be simple.
>>
>> There are a few review comments for Wesley. Hopefully they get resolved
>> eventually.
> 
> As mentioned above, there is a lot of considerations we need to make
> when looking at the amount of combinations that can be done for a USB
> configuration.  We obviously want to see if we can find a way to
> re-allocate FIFO space, but it gets complicated if we run into a
> "fragmented" situation where the RAM associated to the EP being
> re-allocated is in between 2 that are active.
> 

I'd like to have this feature added, and it would be great if it can
overcome some of the current limitations. At the moment, if this feature
is enabled, it may improve some applications, but it may also cause
regression for some. As I noted, the fix may not be simple, but I hope
this feature can work for various applications and not just a limited few.

Thanks,
Thinh

Wesley Cheng Sept. 14, 2021, 2:01 a.m. UTC | #8

On 9/10/2021 8:08 PM, Thinh Nguyen wrote:
> Wesley Cheng wrote:
>>
>>
>> On 9/9/2021 6:15 PM, Thinh Nguyen wrote:
>>> 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 <[e-mail]>
>>>> ---
>>>>  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 &&
>>>>
>>
>> Hi Thinh,
>>
>>>
>>> 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
>>
>> Each alt interface will call usb_ep_autoconfig() which should be
>> assigned different endpoint numbers.  This would mean that if alt intf#0
>> gets selected, and EP is enabled, then we will resize the TXFIFO and map
>> that FIFO to the physical EP.  Then when/if the host requests the other
>> alt intf#1, and that calls EP enable, then the logic will then attempt
>> to resize based on the parameters, and again map that FIFO to the
>> physical EP. (since we call autoconfig on all interfaces, they should be
>> assigned different endpoints)

Hi Thinh,

> 
> That's not true. Different alt-settings of an interface can share
> endpoint numbers. This is often the case for UASP driver where
> alt-setting 0 is for BOT protocol and alt-setting 1 is UASP. When we
> switch alt-setting, we disable the current endpoints and enable the
> old/new ones.
> 

Thanks for pointing that use case out.  Maybe we can consider seeing if
we can walk through all alternate interfaces for a particular function,
and resize for the largest setting?  That might be a possible
improvement made to the check_config() function.  Let me start makign
the changes for this and verifying it.

>>
>> I agree that there is currently a limitation because we are going to
>> reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is
>> only 1 interface active at a time.  The missing logic that we might be
>> missing is seeing how we can re-purpose the FIFO that is being disabled.
>>  However, I think Jack's fix here would be applicable to the improvement
>> in logic to re-use/re-assign FIFO space allocated by disabled EPs also.
>>
> 
> Improvement is always great. I just hope we don't just stop where we are
> now. Since you're working on this feature at the moment, it would be
> good to also resolve some of the outstanding issues as Jack's fix seems
> to be incomplete.
> 

If we implement the improvement mentioned above, I think Jack's fix will
be applicable there as well.  If we resize for the largest alternate
interface, then there would be no reason for us to resize again.

>>> different endpoints, the logic handling this may not be simple.
>>>
>>> There are a few review comments for Wesley. Hopefully they get resolved
>>> eventually.
>>
>> As mentioned above, there is a lot of considerations we need to make
>> when looking at the amount of combinations that can be done for a USB
>> configuration.  We obviously want to see if we can find a way to
>> re-allocate FIFO space, but it gets complicated if we run into a
>> "fragmented" situation where the RAM associated to the EP being
>> re-allocated is in between 2 that are active.
>>
> 
> I'd like to have this feature added, and it would be great if it can
> overcome some of the current limitations. At the moment, if this feature
> is enabled, it may improve some applications, but it may also cause
> regression for some. As I noted, the fix may not be simple, but I hope
> this feature can work for various applications and not just a limited few.
> 

Agreed, there are some use cases that we may not consider in our
platform, so I appreciate the input.

Thanks
Wesley Cheng

Thinh Nguyen Oct. 8, 2021, 12:07 a.m. UTC | #9

Wesley Cheng wrote:
> 
> 
> On 9/10/2021 8:08 PM, Thinh Nguyen wrote:
>> Wesley Cheng wrote:
>>>
>>>
>>> On 9/9/2021 6:15 PM, Thinh Nguyen wrote:
>>>> 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 <[e-mail]>
>>>>> ---
>>>>>  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 &&
>>>>>
>>>
>>> Hi Thinh,
>>>
>>>>
>>>> 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
>>>
>>> Each alt interface will call usb_ep_autoconfig() which should be
>>> assigned different endpoint numbers.  This would mean that if alt intf#0
>>> gets selected, and EP is enabled, then we will resize the TXFIFO and map
>>> that FIFO to the physical EP.  Then when/if the host requests the other
>>> alt intf#1, and that calls EP enable, then the logic will then attempt
>>> to resize based on the parameters, and again map that FIFO to the
>>> physical EP. (since we call autoconfig on all interfaces, they should be
>>> assigned different endpoints)
> 
> Hi Thinh,
> 
>>
>> That's not true. Different alt-settings of an interface can share
>> endpoint numbers. This is often the case for UASP driver where
>> alt-setting 0 is for BOT protocol and alt-setting 1 is UASP. When we
>> switch alt-setting, we disable the current endpoints and enable the
>> old/new ones.
>>
> 
> Thanks for pointing that use case out.  Maybe we can consider seeing if
> we can walk through all alternate interfaces for a particular function,
> and resize for the largest setting?  That might be a possible
> improvement made to the check_config() function.  Let me start makign
> the changes for this and verifying it.
> 

Thanks!

Currently the gadget configures early and informs the gadget driver of
how many endpoints are available, which doesn't leave much room for the
gadget to do optimization/reconfiguration.

If there's an option for the composite layer to inform the controller
driver of the entire configuration, then we can take advantage of more
dwc3 controller capability/flexibility (not just resizing txfifo).

>>>
>>> I agree that there is currently a limitation because we are going to
>>> reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is
>>> only 1 interface active at a time.  The missing logic that we might be
>>> missing is seeing how we can re-purpose the FIFO that is being disabled.
>>>  However, I think Jack's fix here would be applicable to the improvement
>>> in logic to re-use/re-assign FIFO space allocated by disabled EPs also.
>>>
>>
>> Improvement is always great. I just hope we don't just stop where we are
>> now. Since you're working on this feature at the moment, it would be
>> good to also resolve some of the outstanding issues as Jack's fix seems
>> to be incomplete.
>>
> 
> If we implement the improvement mentioned above, I think Jack's fix will
> be applicable there as well.  If we resize for the largest alternate
> interface, then there would be no reason for us to resize again.
> 
As long as you have the above as part of your roadmap, I don't mind
Jack's fix for now. Depends on your implementation, Jack's fix may not
be applicable. Regardless, ultimately it's up to Felipe. :)

>>>> different endpoints, the logic handling this may not be simple.
>>>>
>>>> There are a few review comments for Wesley. Hopefully they get resolved
>>>> eventually.
>>>
>>> As mentioned above, there is a lot of considerations we need to make
>>> when looking at the amount of combinations that can be done for a USB
>>> configuration.  We obviously want to see if we can find a way to
>>> re-allocate FIFO space, but it gets complicated if we run into a
>>> "fragmented" situation where the RAM associated to the EP being
>>> re-allocated is in between 2 that are active.
>>>
>>
>> I'd like to have this feature added, and it would be great if it can
>> overcome some of the current limitations. At the moment, if this feature
>> is enabled, it may improve some applications, but it may also cause
>> regression for some. As I noted, the fix may not be simple, but I hope
>> this feature can work for various applications and not just a limited few.
>>
> 
> Agreed, there are some use cases that we may not consider in our
> platform, so I appreciate the input.
> 

(Sorry for the delay response. I was on vacation.)

BR,
Thinh

Jack Pham Oct. 15, 2021, 12:51 a.m. UTC | #10

On Fri, Oct 08, 2021 at 12:07:20AM +0000, Thinh Nguyen wrote:
> Wesley Cheng wrote:
> > 
> > 
> > On 9/10/2021 8:08 PM, Thinh Nguyen wrote:
> >> Wesley Cheng wrote:
> >>>
> >>>
> >>> On 9/9/2021 6:15 PM, Thinh Nguyen wrote:
> >>>> 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 <[e-mail]>
> >>>>> ---
> >>>>>  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 &&
> >>>>>
> >>>
> >>> Hi Thinh,
> >>>
> >>>>
> >>>> 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
> >>>
> >>> Each alt interface will call usb_ep_autoconfig() which should be
> >>> assigned different endpoint numbers.  This would mean that if alt intf#0
> >>> gets selected, and EP is enabled, then we will resize the TXFIFO and map
> >>> that FIFO to the physical EP.  Then when/if the host requests the other
> >>> alt intf#1, and that calls EP enable, then the logic will then attempt
> >>> to resize based on the parameters, and again map that FIFO to the
> >>> physical EP. (since we call autoconfig on all interfaces, they should be
> >>> assigned different endpoints)
> > 
> > Hi Thinh,
> > 
> >>
> >> That's not true. Different alt-settings of an interface can share
> >> endpoint numbers. This is often the case for UASP driver where
> >> alt-setting 0 is for BOT protocol and alt-setting 1 is UASP. When we
> >> switch alt-setting, we disable the current endpoints and enable the
> >> old/new ones.
> >>
> > 
> > Thanks for pointing that use case out.  Maybe we can consider seeing if
> > we can walk through all alternate interfaces for a particular function,
> > and resize for the largest setting?  That might be a possible
> > improvement made to the check_config() function.  Let me start makign
> > the changes for this and verifying it.
> > 
> 
> Thanks!
> 
> Currently the gadget configures early and informs the gadget driver of
> how many endpoints are available, which doesn't leave much room for the
> gadget to do optimization/reconfiguration.
> 
> If there's an option for the composite layer to inform the controller
> driver of the entire configuration, then we can take advantage of more
> dwc3 controller capability/flexibility (not just resizing txfifo).
> 
> >>>
> >>> I agree that there is currently a limitation because we are going to
> >>> reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is
> >>> only 1 interface active at a time.  The missing logic that we might be
> >>> missing is seeing how we can re-purpose the FIFO that is being disabled.
> >>>  However, I think Jack's fix here would be applicable to the improvement
> >>> in logic to re-use/re-assign FIFO space allocated by disabled EPs also.
> >>>
> >>
> >> Improvement is always great. I just hope we don't just stop where we are
> >> now. Since you're working on this feature at the moment, it would be
> >> good to also resolve some of the outstanding issues as Jack's fix seems
> >> to be incomplete.
> >>
> > 
> > If we implement the improvement mentioned above, I think Jack's fix will
> > be applicable there as well.  If we resize for the largest alternate
> > interface, then there would be no reason for us to resize again.
> > 
> As long as you have the above as part of your roadmap, I don't mind
> Jack's fix for now.

Thanks for your input as always Thinh.  Does the patch still look ok in
its current state?  Last time I had responded to Felipe that I would try
to entertain adding an explicit flag to keep track of whether an EP had
been resized yet or not.  When trying to implement this as another
DWC3_EP_* bit for dep->flags we'd then need to retain this flag across
ep_disable/enable calls, so it looks a tiny bit cumbersome because
dep->flags isn't neatly 0 anymore :-P.

So ep_disable() would need to look something like this:

 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;
 }

The flag would be initially set in dwc3_gadget_resize_tx_fifos() and
also would need to be checked there as well to avoid re-resizing.  It
would then get cleared in dwc3_gadget_clear_tx_fifos().

Is this still preferable to the current patch with just the single
register read?

> Depends on your implementation, Jack's fix may not
> be applicable. Regardless, ultimately it's up to Felipe. :)

Felipe, what do you think?  Wesley is already working on improving the
scheme to better handle endpoints that get activated only when switching
AltSettings.  But I'd still like to get my patch in as an incremental
fix though since it does take care of an existing issue with disabling
and re-enabling endpoints within the same Configuration & AltSetting.

Thanks,
Jack

> 
> >>>> different endpoints, the logic handling this may not be simple.
> >>>>
> >>>> There are a few review comments for Wesley. Hopefully they get resolved
> >>>> eventually.
> >>>
> >>> As mentioned above, there is a lot of considerations we need to make
> >>> when looking at the amount of combinations that can be done for a USB
> >>> configuration.  We obviously want to see if we can find a way to
> >>> re-allocate FIFO space, but it gets complicated if we run into a
> >>> "fragmented" situation where the RAM associated to the EP being
> >>> re-allocated is in between 2 that are active.
> >>>
> >>
> >> I'd like to have this feature added, and it would be great if it can
> >> overcome some of the current limitations. At the moment, if this feature
> >> is enabled, it may improve some applications, but it may also cause
> >> regression for some. As I noted, the fix may not be simple, but I hope
> >> this feature can work for various applications and not just a limited few.
> >>
> > 
> > Agreed, there are some use cases that we may not consider in our
> > platform, so I appreciate the input.
> >

Thinh Nguyen Oct. 15, 2021, 10:20 p.m. UTC | #11

Jack Pham wrote:
> On Fri, Oct 08, 2021 at 12:07:20AM +0000, Thinh Nguyen wrote:
>> Wesley Cheng wrote:
>>>
>>>
>>> On 9/10/2021 8:08 PM, Thinh Nguyen wrote:
>>>> Wesley Cheng wrote:
>>>>>
>>>>>
>>>>> On 9/9/2021 6:15 PM, Thinh Nguyen wrote:
>>>>>> 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 <[e-mail]>
>>>>>>> ---
>>>>>>>  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 &&
>>>>>>>
>>>>>
>>>>> Hi Thinh,
>>>>>
>>>>>>
>>>>>> 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
>>>>>
>>>>> Each alt interface will call usb_ep_autoconfig() which should be
>>>>> assigned different endpoint numbers.  This would mean that if alt intf#0
>>>>> gets selected, and EP is enabled, then we will resize the TXFIFO and map
>>>>> that FIFO to the physical EP.  Then when/if the host requests the other
>>>>> alt intf#1, and that calls EP enable, then the logic will then attempt
>>>>> to resize based on the parameters, and again map that FIFO to the
>>>>> physical EP. (since we call autoconfig on all interfaces, they should be
>>>>> assigned different endpoints)
>>>
>>> Hi Thinh,
>>>
>>>>
>>>> That's not true. Different alt-settings of an interface can share
>>>> endpoint numbers. This is often the case for UASP driver where
>>>> alt-setting 0 is for BOT protocol and alt-setting 1 is UASP. When we
>>>> switch alt-setting, we disable the current endpoints and enable the
>>>> old/new ones.
>>>>
>>>
>>> Thanks for pointing that use case out.  Maybe we can consider seeing if
>>> we can walk through all alternate interfaces for a particular function,
>>> and resize for the largest setting?  That might be a possible
>>> improvement made to the check_config() function.  Let me start makign
>>> the changes for this and verifying it.
>>>
>>
>> Thanks!
>>
>> Currently the gadget configures early and informs the gadget driver of
>> how many endpoints are available, which doesn't leave much room for the
>> gadget to do optimization/reconfiguration.
>>
>> If there's an option for the composite layer to inform the controller
>> driver of the entire configuration, then we can take advantage of more
>> dwc3 controller capability/flexibility (not just resizing txfifo).
>>
>>>>>
>>>>> I agree that there is currently a limitation because we are going to
>>>>> reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is
>>>>> only 1 interface active at a time.  The missing logic that we might be
>>>>> missing is seeing how we can re-purpose the FIFO that is being disabled.
>>>>>  However, I think Jack's fix here would be applicable to the improvement
>>>>> in logic to re-use/re-assign FIFO space allocated by disabled EPs also.
>>>>>
>>>>
>>>> Improvement is always great. I just hope we don't just stop where we are
>>>> now. Since you're working on this feature at the moment, it would be
>>>> good to also resolve some of the outstanding issues as Jack's fix seems
>>>> to be incomplete.
>>>>
>>>
>>> If we implement the improvement mentioned above, I think Jack's fix will
>>> be applicable there as well.  If we resize for the largest alternate
>>> interface, then there would be no reason for us to resize again.
>>>
>> As long as you have the above as part of your roadmap, I don't mind
>> Jack's fix for now.
> 
> Thanks for your input as always Thinh.  Does the patch still look ok in
> its current state?  Last time I had responded to Felipe that I would try

TX endpoints should have non-zero GTXFIFOSIZ. Using the register as a
flag to check whether it's been resized is not ok. Also, what happened
after resizing the txfifo? Do you restore its previous default value?

> to entertain adding an explicit flag to keep track of whether an EP had
> been resized yet or not.  When trying to implement this as another
> DWC3_EP_* bit for dep->flags we'd then need to retain this flag across
> ep_disable/enable calls, so it looks a tiny bit cumbersome because
> dep->flags isn't neatly 0 anymore :-P.
> 
> So ep_disable() would need to look something like this:
> 
>  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;

I think you mean this?
dep->flags &= ~DWC3_EP_TXFIFO_RESIZED;

> 
> 	return 0;
>  }
> 
> The flag would be initially set in dwc3_gadget_resize_tx_fifos() and
> also would need to be checked there as well to avoid re-resizing.  It
> would then get cleared in dwc3_gadget_clear_tx_fifos().
> 
> Is this still preferable to the current patch with just the single
> register read?
> 

What if we change the configuration from the configfs? Would the flag
persist? If that's the case, that doesn't look right.

Thanks,
Thinh

Thinh Nguyen Oct. 15, 2021, 11:52 p.m. UTC | #12

Thinh Nguyen wrote:
> Jack Pham wrote:
>> On Fri, Oct 08, 2021 at 12:07:20AM +0000, Thinh Nguyen wrote:
>>> Wesley Cheng wrote:
>>>>
>>>>
>>>> On 9/10/2021 8:08 PM, Thinh Nguyen wrote:
>>>>> Wesley Cheng wrote:
>>>>>>
>>>>>>
>>>>>> On 9/9/2021 6:15 PM, Thinh Nguyen wrote:
>>>>>>> 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 <[e-mail]>
>>>>>>>> ---
>>>>>>>>  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 &&
>>>>>>>>
>>>>>>
>>>>>> Hi Thinh,
>>>>>>
>>>>>>>
>>>>>>> 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
>>>>>>
>>>>>> Each alt interface will call usb_ep_autoconfig() which should be
>>>>>> assigned different endpoint numbers.  This would mean that if alt intf#0
>>>>>> gets selected, and EP is enabled, then we will resize the TXFIFO and map
>>>>>> that FIFO to the physical EP.  Then when/if the host requests the other
>>>>>> alt intf#1, and that calls EP enable, then the logic will then attempt
>>>>>> to resize based on the parameters, and again map that FIFO to the
>>>>>> physical EP. (since we call autoconfig on all interfaces, they should be
>>>>>> assigned different endpoints)
>>>>
>>>> Hi Thinh,
>>>>
>>>>>
>>>>> That's not true. Different alt-settings of an interface can share
>>>>> endpoint numbers. This is often the case for UASP driver where
>>>>> alt-setting 0 is for BOT protocol and alt-setting 1 is UASP. When we
>>>>> switch alt-setting, we disable the current endpoints and enable the
>>>>> old/new ones.
>>>>>
>>>>
>>>> Thanks for pointing that use case out.  Maybe we can consider seeing if
>>>> we can walk through all alternate interfaces for a particular function,
>>>> and resize for the largest setting?  That might be a possible
>>>> improvement made to the check_config() function.  Let me start makign
>>>> the changes for this and verifying it.
>>>>
>>>
>>> Thanks!
>>>
>>> Currently the gadget configures early and informs the gadget driver of
>>> how many endpoints are available, which doesn't leave much room for the
>>> gadget to do optimization/reconfiguration.
>>>
>>> If there's an option for the composite layer to inform the controller
>>> driver of the entire configuration, then we can take advantage of more
>>> dwc3 controller capability/flexibility (not just resizing txfifo).
>>>
>>>>>>
>>>>>> I agree that there is currently a limitation because we are going to
>>>>>> reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is
>>>>>> only 1 interface active at a time.  The missing logic that we might be
>>>>>> missing is seeing how we can re-purpose the FIFO that is being disabled.
>>>>>>  However, I think Jack's fix here would be applicable to the improvement
>>>>>> in logic to re-use/re-assign FIFO space allocated by disabled EPs also.
>>>>>>
>>>>>
>>>>> Improvement is always great. I just hope we don't just stop where we are
>>>>> now. Since you're working on this feature at the moment, it would be
>>>>> good to also resolve some of the outstanding issues as Jack's fix seems
>>>>> to be incomplete.
>>>>>
>>>>
>>>> If we implement the improvement mentioned above, I think Jack's fix will
>>>> be applicable there as well.  If we resize for the largest alternate
>>>> interface, then there would be no reason for us to resize again.
>>>>
>>> As long as you have the above as part of your roadmap, I don't mind
>>> Jack's fix for now.
>>
>> Thanks for your input as always Thinh.  Does the patch still look ok in
>> its current state?  Last time I had responded to Felipe that I would try
> 
> TX endpoints should have non-zero GTXFIFOSIZ. Using the register as a
> flag to check whether it's been resized is not ok. Also, what happened
> after resizing the txfifo? Do you restore its previous default value?
> 
>> to entertain adding an explicit flag to keep track of whether an EP had
>> been resized yet or not.  When trying to implement this as another
>> DWC3_EP_* bit for dep->flags we'd then need to retain this flag across
>> ep_disable/enable calls, so it looks a tiny bit cumbersome because
>> dep->flags isn't neatly 0 anymore :-P.
>>
>> So ep_disable() would need to look something like this:
>>
>>  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;
> 
> I think you mean this?
> dep->flags &= ~DWC3_EP_TXFIFO_RESIZED;
> 

nvm, ignore this comment. Your intention is
dep->flags &= DWC3_EP_TXFIFO_RESIZED;

Thinh

Thinh Nguyen Oct. 15, 2021, 11:55 p.m. UTC | #13

Thinh Nguyen wrote:
> Jack Pham wrote:
>> On Fri, Oct 08, 2021 at 12:07:20AM +0000, Thinh Nguyen wrote:
>>> Wesley Cheng wrote:
>>>>
>>>>
>>>> On 9/10/2021 8:08 PM, Thinh Nguyen wrote:
>>>>> Wesley Cheng wrote:
>>>>>>
>>>>>>
>>>>>> On 9/9/2021 6:15 PM, Thinh Nguyen wrote:
>>>>>>> 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 <[e-mail]>
>>>>>>>> ---
>>>>>>>>  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 &&
>>>>>>>>
>>>>>>
>>>>>> Hi Thinh,
>>>>>>
>>>>>>>
>>>>>>> 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
>>>>>>
>>>>>> Each alt interface will call usb_ep_autoconfig() which should be
>>>>>> assigned different endpoint numbers.  This would mean that if alt intf#0
>>>>>> gets selected, and EP is enabled, then we will resize the TXFIFO and map
>>>>>> that FIFO to the physical EP.  Then when/if the host requests the other
>>>>>> alt intf#1, and that calls EP enable, then the logic will then attempt
>>>>>> to resize based on the parameters, and again map that FIFO to the
>>>>>> physical EP. (since we call autoconfig on all interfaces, they should be
>>>>>> assigned different endpoints)
>>>>
>>>> Hi Thinh,
>>>>
>>>>>
>>>>> That's not true. Different alt-settings of an interface can share
>>>>> endpoint numbers. This is often the case for UASP driver where
>>>>> alt-setting 0 is for BOT protocol and alt-setting 1 is UASP. When we
>>>>> switch alt-setting, we disable the current endpoints and enable the
>>>>> old/new ones.
>>>>>
>>>>
>>>> Thanks for pointing that use case out.  Maybe we can consider seeing if
>>>> we can walk through all alternate interfaces for a particular function,
>>>> and resize for the largest setting?  That might be a possible
>>>> improvement made to the check_config() function.  Let me start makign
>>>> the changes for this and verifying it.
>>>>
>>>
>>> Thanks!
>>>
>>> Currently the gadget configures early and informs the gadget driver of
>>> how many endpoints are available, which doesn't leave much room for the
>>> gadget to do optimization/reconfiguration.
>>>
>>> If there's an option for the composite layer to inform the controller
>>> driver of the entire configuration, then we can take advantage of more
>>> dwc3 controller capability/flexibility (not just resizing txfifo).
>>>
>>>>>>
>>>>>> I agree that there is currently a limitation because we are going to
>>>>>> reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is
>>>>>> only 1 interface active at a time.  The missing logic that we might be
>>>>>> missing is seeing how we can re-purpose the FIFO that is being disabled.
>>>>>>  However, I think Jack's fix here would be applicable to the improvement
>>>>>> in logic to re-use/re-assign FIFO space allocated by disabled EPs also.
>>>>>>
>>>>>
>>>>> Improvement is always great. I just hope we don't just stop where we are
>>>>> now. Since you're working on this feature at the moment, it would be
>>>>> good to also resolve some of the outstanding issues as Jack's fix seems
>>>>> to be incomplete.
>>>>>
>>>>
>>>> If we implement the improvement mentioned above, I think Jack's fix will
>>>> be applicable there as well.  If we resize for the largest alternate
>>>> interface, then there would be no reason for us to resize again.
>>>>
>>> As long as you have the above as part of your roadmap, I don't mind
>>> Jack's fix for now.
>>
>> Thanks for your input as always Thinh.  Does the patch still look ok in
>> its current state?  Last time I had responded to Felipe that I would try
> 
> TX endpoints should have non-zero GTXFIFOSIZ. Using the register as a
> flag to check whether it's been resized is not ok. Also, what happened
> after resizing the txfifo? Do you restore its previous default value?
> 
>> to entertain adding an explicit flag to keep track of whether an EP had
>> been resized yet or not.  When trying to implement this as another
>> DWC3_EP_* bit for dep->flags we'd then need to retain this flag across
>> ep_disable/enable calls, so it looks a tiny bit cumbersome because
>> dep->flags isn't neatly 0 anymore :-P.
>>
>> So ep_disable() would need to look something like this:
>>
>>  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;
> 
> I think you mean this?
> dep->flags &= ~DWC3_EP_TXFIFO_RESIZED;
> 

nvm, ignore this comment. Your intention is
dep->flags &= DWC3_EP_TXFIFO_RESIZED;

Thinh

Jack Pham Oct. 18, 2021, 7:28 a.m. UTC | #14

On Fri, Oct 15, 2021 at 10:20:48PM +0000, Thinh Nguyen wrote:
> Jack Pham wrote:
> > On Fri, Oct 08, 2021 at 12:07:20AM +0000, Thinh Nguyen wrote:
> >> Wesley Cheng wrote:
> >>>
> >>>
> >>> On 9/10/2021 8:08 PM, Thinh Nguyen wrote:
> >>>> Wesley Cheng wrote:
> >>>>>
> >>>>>
> >>>>> On 9/9/2021 6:15 PM, Thinh Nguyen wrote:
> >>>>>> 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 <[e-mail]>
> >>>>>>> ---
> >>>>>>>  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 &&
> >>>>>>>
> >>>>>
> >>>>> Hi Thinh,
> >>>>>
> >>>>>>
> >>>>>> 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
> >>>>>
> >>>>> Each alt interface will call usb_ep_autoconfig() which should be
> >>>>> assigned different endpoint numbers.  This would mean that if alt intf#0
> >>>>> gets selected, and EP is enabled, then we will resize the TXFIFO and map
> >>>>> that FIFO to the physical EP.  Then when/if the host requests the other
> >>>>> alt intf#1, and that calls EP enable, then the logic will then attempt
> >>>>> to resize based on the parameters, and again map that FIFO to the
> >>>>> physical EP. (since we call autoconfig on all interfaces, they should be
> >>>>> assigned different endpoints)
> >>>
> >>> Hi Thinh,
> >>>
> >>>>
> >>>> That's not true. Different alt-settings of an interface can share
> >>>> endpoint numbers. This is often the case for UASP driver where
> >>>> alt-setting 0 is for BOT protocol and alt-setting 1 is UASP. When we
> >>>> switch alt-setting, we disable the current endpoints and enable the
> >>>> old/new ones.
> >>>>
> >>>
> >>> Thanks for pointing that use case out.  Maybe we can consider seeing if
> >>> we can walk through all alternate interfaces for a particular function,
> >>> and resize for the largest setting?  That might be a possible
> >>> improvement made to the check_config() function.  Let me start makign
> >>> the changes for this and verifying it.
> >>>
> >>
> >> Thanks!
> >>
> >> Currently the gadget configures early and informs the gadget driver of
> >> how many endpoints are available, which doesn't leave much room for the
> >> gadget to do optimization/reconfiguration.
> >>
> >> If there's an option for the composite layer to inform the controller
> >> driver of the entire configuration, then we can take advantage of more
> >> dwc3 controller capability/flexibility (not just resizing txfifo).
> >>
> >>>>>
> >>>>> I agree that there is currently a limitation because we are going to
> >>>>> reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is
> >>>>> only 1 interface active at a time.  The missing logic that we might be
> >>>>> missing is seeing how we can re-purpose the FIFO that is being disabled.
> >>>>>  However, I think Jack's fix here would be applicable to the improvement
> >>>>> in logic to re-use/re-assign FIFO space allocated by disabled EPs also.
> >>>>>
> >>>>
> >>>> Improvement is always great. I just hope we don't just stop where we are
> >>>> now. Since you're working on this feature at the moment, it would be
> >>>> good to also resolve some of the outstanding issues as Jack's fix seems
> >>>> to be incomplete.
> >>>>
> >>>
> >>> If we implement the improvement mentioned above, I think Jack's fix will
> >>> be applicable there as well.  If we resize for the largest alternate
> >>> interface, then there would be no reason for us to resize again.
> >>>
> >> As long as you have the above as part of your roadmap, I don't mind
> >> Jack's fix for now.
> > 
> > Thanks for your input as always Thinh.  Does the patch still look ok in
> > its current state?  Last time I had responded to Felipe that I would try
> 
> TX endpoints should have non-zero GTXFIFOSIZ. Using the register as a
> flag to check whether it's been resized is not ok. Also, what happened
> after resizing the txfifo? Do you restore its previous default value?

The choice to use the resizing algorithm is a fixed setting determined
by device property.  So if it is enabled for a particular platform
instance, it means we don't intend to use any of the default values.
All the registers will get cleared to 0 upon every Set Configuration so
that each EP enable call thereafter will be starting from a clean slate.

As you correctly pointed out however, this doesn't quite work well for
AltSettings where EPs may come and go well after the Set Config.  This
will be where future improvements will hopefully address.

> > to entertain adding an explicit flag to keep track of whether an EP had
> > been resized yet or not.  When trying to implement this as another
> > DWC3_EP_* bit for dep->flags we'd then need to retain this flag across
> > ep_disable/enable calls, so it looks a tiny bit cumbersome because
> > dep->flags isn't neatly 0 anymore :-P.
> > 
> > So ep_disable() would need to look something like this:
> > 
> >  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;
> 
> I think you mean this?
> dep->flags &= ~DWC3_EP_TXFIFO_RESIZED;

No I do mean AND without any inverting, since if that flag was set we
need to keep it still set after disabling.

> > 	return 0;
> >  }
> > 
> > The flag would be initially set in dwc3_gadget_resize_tx_fifos() and
> > also would need to be checked there as well to avoid re-resizing.  It
> > would then get cleared in dwc3_gadget_clear_tx_fifos().
> > 
> > Is this still preferable to the current patch with just the single
> > register read?
> > 
> 
> What if we change the configuration from the configfs? Would the flag
> persist? If that's the case, that doesn't look right.

Yes, it might still be set, but this is only a transient state.  It will
eventually get cleared at the time the GTXFIFOSIZ is reset back to 0
during dwc3_gadget_clear_tx_fifos() when the next Set Config (in
dwc3_ep0_set_config()) is received during enumeration in the new
configuration.

I can go ahead with V2 of this patch using this flag to be more clear.

Jack

Thinh Nguyen Oct. 19, 2021, 2:38 a.m. UTC | #15

Jack Pham wrote:
> On Fri, Oct 15, 2021 at 10:20:48PM +0000, Thinh Nguyen wrote:
>> Jack Pham wrote:
>>> On Fri, Oct 08, 2021 at 12:07:20AM +0000, Thinh Nguyen wrote:
>>>> Wesley Cheng wrote:
>>>>>
>>>>>
>>>>> On 9/10/2021 8:08 PM, Thinh Nguyen wrote:
>>>>>> Wesley Cheng wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 9/9/2021 6:15 PM, Thinh Nguyen wrote:
>>>>>>>> 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 <[e-mail]>
>>>>>>>>> ---
>>>>>>>>>  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 &&
>>>>>>>>>
>>>>>>>
>>>>>>> Hi Thinh,
>>>>>>>
>>>>>>>>
>>>>>>>> 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
>>>>>>>
>>>>>>> Each alt interface will call usb_ep_autoconfig() which should be
>>>>>>> assigned different endpoint numbers.  This would mean that if alt intf#0
>>>>>>> gets selected, and EP is enabled, then we will resize the TXFIFO and map
>>>>>>> that FIFO to the physical EP.  Then when/if the host requests the other
>>>>>>> alt intf#1, and that calls EP enable, then the logic will then attempt
>>>>>>> to resize based on the parameters, and again map that FIFO to the
>>>>>>> physical EP. (since we call autoconfig on all interfaces, they should be
>>>>>>> assigned different endpoints)
>>>>>
>>>>> Hi Thinh,
>>>>>
>>>>>>
>>>>>> That's not true. Different alt-settings of an interface can share
>>>>>> endpoint numbers. This is often the case for UASP driver where
>>>>>> alt-setting 0 is for BOT protocol and alt-setting 1 is UASP. When we
>>>>>> switch alt-setting, we disable the current endpoints and enable the
>>>>>> old/new ones.
>>>>>>
>>>>>
>>>>> Thanks for pointing that use case out.  Maybe we can consider seeing if
>>>>> we can walk through all alternate interfaces for a particular function,
>>>>> and resize for the largest setting?  That might be a possible
>>>>> improvement made to the check_config() function.  Let me start makign
>>>>> the changes for this and verifying it.
>>>>>
>>>>
>>>> Thanks!
>>>>
>>>> Currently the gadget configures early and informs the gadget driver of
>>>> how many endpoints are available, which doesn't leave much room for the
>>>> gadget to do optimization/reconfiguration.
>>>>
>>>> If there's an option for the composite layer to inform the controller
>>>> driver of the entire configuration, then we can take advantage of more
>>>> dwc3 controller capability/flexibility (not just resizing txfifo).
>>>>
>>>>>>>
>>>>>>> I agree that there is currently a limitation because we are going to
>>>>>>> reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is
>>>>>>> only 1 interface active at a time.  The missing logic that we might be
>>>>>>> missing is seeing how we can re-purpose the FIFO that is being disabled.
>>>>>>>  However, I think Jack's fix here would be applicable to the improvement
>>>>>>> in logic to re-use/re-assign FIFO space allocated by disabled EPs also.
>>>>>>>
>>>>>>
>>>>>> Improvement is always great. I just hope we don't just stop where we are
>>>>>> now. Since you're working on this feature at the moment, it would be
>>>>>> good to also resolve some of the outstanding issues as Jack's fix seems
>>>>>> to be incomplete.
>>>>>>
>>>>>
>>>>> If we implement the improvement mentioned above, I think Jack's fix will
>>>>> be applicable there as well.  If we resize for the largest alternate
>>>>> interface, then there would be no reason for us to resize again.
>>>>>
>>>> As long as you have the above as part of your roadmap, I don't mind
>>>> Jack's fix for now.
>>>
>>> Thanks for your input as always Thinh.  Does the patch still look ok in
>>> its current state?  Last time I had responded to Felipe that I would try
>>
>> TX endpoints should have non-zero GTXFIFOSIZ. Using the register as a
>> flag to check whether it's been resized is not ok. Also, what happened
>> after resizing the txfifo? Do you restore its previous default value?
> 
> The choice to use the resizing algorithm is a fixed setting determined
> by device property.  So if it is enabled for a particular platform
> instance, it means we don't intend to use any of the default values.
> All the registers will get cleared to 0 upon every Set Configuration so
> that each EP enable call thereafter will be starting from a clean slate.

Some fields of GTXFIFOSIZ may not get cleared. Depends on the controller
version, we introduce different fields (as the case for DWC32 and
DWC31). So this may not apply for all the time. I just noticed that the
entire GTXFIFOSIZ was written with 0. Please only write to the specific
fields with proper macros.

> 
> As you correctly pointed out however, this doesn't quite work well for
> AltSettings where EPs may come and go well after the Set Config.  This
> will be where future improvements will hopefully address.
> 
>>> to entertain adding an explicit flag to keep track of whether an EP had
>>> been resized yet or not.  When trying to implement this as another
>>> DWC3_EP_* bit for dep->flags we'd then need to retain this flag across
>>> ep_disable/enable calls, so it looks a tiny bit cumbersome because
>>> dep->flags isn't neatly 0 anymore :-P.
>>>
>>> So ep_disable() would need to look something like this:
>>>
>>>  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;
>>
>> I think you mean this?
>> dep->flags &= ~DWC3_EP_TXFIFO_RESIZED;
> 
> No I do mean AND without any inverting, since if that flag was set we
> need to keep it still set after disabling.
> 
>>> 	return 0;
>>>  }
>>>
>>> The flag would be initially set in dwc3_gadget_resize_tx_fifos() and
>>> also would need to be checked there as well to avoid re-resizing.  It
>>> would then get cleared in dwc3_gadget_clear_tx_fifos().
>>>
>>> Is this still preferable to the current patch with just the single
>>> register read?
>>>
>>
>> What if we change the configuration from the configfs? Would the flag
>> persist? If that's the case, that doesn't look right.
> 
> Yes, it might still be set, but this is only a transient state.  It will
> eventually get cleared at the time the GTXFIFOSIZ is reset back to 0
> during dwc3_gadget_clear_tx_fifos() when the next Set Config (in
> dwc3_ep0_set_config()) is received during enumeration in the new
> configuration.
> 
> I can go ahead with V2 of this patch using this flag to be more clear.
> 

I think using a flag is clearer also.

Thanks,
Thinh

Jack Pham Oct. 19, 2021, 5:38 a.m. UTC | #16

On Tue, Oct 19, 2021 at 02:38:30AM +0000, Thinh Nguyen wrote:
> Jack Pham wrote:
> > On Fri, Oct 15, 2021 at 10:20:48PM +0000, Thinh Nguyen wrote:
> >> TX endpoints should have non-zero GTXFIFOSIZ. Using the register as a
> >> flag to check whether it's been resized is not ok. Also, what happened
> >> after resizing the txfifo? Do you restore its previous default value?
> > 
> > The choice to use the resizing algorithm is a fixed setting determined
> > by device property.  So if it is enabled for a particular platform
> > instance, it means we don't intend to use any of the default values.
> > All the registers will get cleared to 0 upon every Set Configuration so
> > that each EP enable call thereafter will be starting from a clean slate.
> 
> Some fields of GTXFIFOSIZ may not get cleared. Depends on the controller
> version, we introduce different fields (as the case for DWC32 and
> DWC31). So this may not apply for all the time. I just noticed that the
> entire GTXFIFOSIZ was written with 0. Please only write to the specific
> fields with proper macros.

Hmm, I thought Wesley's original change already takes care to do that:

void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc)
{
...
	/* Clear existing TXFIFO for all IN eps except ep0 */
	for (num = 3; num < min_t(int, dwc->num_eps, DWC3_ENDPOINTS_NUM);
             num += 2) {
		dep = dwc->eps[num];
		/* Don't change TXFRAMNUM on usb31 version */
		size = DWC3_IP_IS(DWC3) ? 0 :
			dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(num >> 1)) &
                		DWC31_GTXFIFOSIZ_TXFRAMNUM;

		dwc3_writel(dwc->regs, DWC3_GTXFIFOSIZ(num >> 1), size);
		dep->flags &= ~DWC3_EP_TXFIFO_RESIZED;
	}
	dwc->num_ep_resized = 0;
}

Just before the write, the read is masked with the TXFRAMNUM bit in case
of !DWC3, i.e. DWC31 or DWC32.  The rest would be 0'ed out.  Sorry if my
previous reply implied the entire register was written as 0.

> > I can go ahead with V2 of this patch using this flag to be more clear.
> > 
> 
> I think using a flag is clearer also.

Thanks, and appreciate the reviewed-by you already replied with for v2!

Jack

Thinh Nguyen Oct. 20, 2021, 12:27 a.m. UTC | #17

Jack Pham wrote:
> On Tue, Oct 19, 2021 at 02:38:30AM +0000, Thinh Nguyen wrote:
>> Jack Pham wrote:
>>> On Fri, Oct 15, 2021 at 10:20:48PM +0000, Thinh Nguyen wrote:
>>>> TX endpoints should have non-zero GTXFIFOSIZ. Using the register as a
>>>> flag to check whether it's been resized is not ok. Also, what happened
>>>> after resizing the txfifo? Do you restore its previous default value?
>>>
>>> The choice to use the resizing algorithm is a fixed setting determined
>>> by device property.  So if it is enabled for a particular platform
>>> instance, it means we don't intend to use any of the default values.
>>> All the registers will get cleared to 0 upon every Set Configuration so
>>> that each EP enable call thereafter will be starting from a clean slate.
>>
>> Some fields of GTXFIFOSIZ may not get cleared. Depends on the controller
>> version, we introduce different fields (as the case for DWC32 and
>> DWC31). So this may not apply for all the time. I just noticed that the
>> entire GTXFIFOSIZ was written with 0. Please only write to the specific
>> fields with proper macros.
> 
> Hmm, I thought Wesley's original change already takes care to do that:
> 

I was referring to the field such as TXFRAMNUM that the check "if
(GTXFIFOSIZ)" may not work because this field may be non-zero by default.

> void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc)
> {
> ...
> 	/* Clear existing TXFIFO for all IN eps except ep0 */
> 	for (num = 3; num < min_t(int, dwc->num_eps, DWC3_ENDPOINTS_NUM);
>              num += 2) {
> 		dep = dwc->eps[num];
> 		/* Don't change TXFRAMNUM on usb31 version */
> 		size = DWC3_IP_IS(DWC3) ? 0 :
> 			dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(num >> 1)) &
>                 		DWC31_GTXFIFOSIZ_TXFRAMNUM;

I think it's better to use masks such as below since it's
self-documented, but it's probably fine as is.

size &= ~(DWC3x_GTXFIFOSIZ_TXFSADDR(~0) | DWC3x_GTXFIFOSIZ_TXFDEP(~0));

> 
> 		dwc3_writel(dwc->regs, DWC3_GTXFIFOSIZ(num >> 1), size);
> 		dep->flags &= ~DWC3_EP_TXFIFO_RESIZED;
> 	}
> 	dwc->num_ep_resized = 0;
> }
> 
> Just before the write, the read is masked with the TXFRAMNUM bit in case
> of !DWC3, i.e. DWC31 or DWC32.  The rest would be 0'ed out.  Sorry if my
> previous reply implied the entire register was written as 0.
> 

For DWC32, the field TXFRAMNUM is replaced with something else, but the
TXFSADDR and TXFDEP are unchanged since DWC31. Check that we don't
unintentionally overwrite bit(15) of GTXFIFOSIZ for DWC32 and DWC31 when
updating txfifo.

Thanks,
Thinh

Patch

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