DWC2: usbd_edpt_close() does not free TX FIFO RAM

manuelbl commented 2 months ago

Operating System

Others

Board

STM32F401CC

Firmware

See https://github.com/manuelbl/JavaDoesUSB/blob/main/test-devices/loopback-stm32/src/vendor_custom.c

What happened ?

I am maintaining code with a custom device class implementation that has to support changing the alternate interface settings. When the alternate interface is changed, the code closes the current endpoints (in reverse order) and opens the interfaces of the selected interface.

Until recently, this has worked. But now the firmware hits an assert in the dwc2 driver checking for sufficient TX FIFO RAM space if the alternate interface is changed multiple times. The reason is that usbd_edpt_close() (or rather dcd_edpt_close() in dcd_dwc2.c) no longer frees the TX FIFO RAM.

The change https://github.com/hathach/tinyusb/commit/e160366a1e3fc26242daf0e31c0d0fafcc51b83c has changed the implementation of dcd_edpt_close() and in particular it has removed this line:

https://github.com/hathach/tinyusb/blob/85420c61c780e260d0afd21de4a39c17ceba5d87/src/portable/synopsys/dwc2/dcd_dwc2.c#L810 ``

Possibly, it was a deliberate decision that usbd_edpt_close() does not free the TX FIFO RAM. But if so, how can setting the interface be correctly implemented without hitting the assert?

dcd_edpt_close_all() would reset the TX FIFO RAM. But the function is not available to a custom device class implementation. The FIFO is also reset if the configuration is changed, or if a bus reset occurs. But there seems to be no way to do it from a class implementation when changing the alternate interface.

A possible option would be to introduce a usbd_edpt_close_all() function.

How to reproduce ?

To reproduce it:

Use a board with the DWC2 driver
call usbd_edpt_open() and usb_edpt_close() multiple times (for the same IN endpoint)

The code will stop in in this ASSERT:

https://github.com/hathach/tinyusb/blob/master/src/portable/synopsys/dwc2/dcd_dwc2.c#L175

Debug Log as txt file (LOG/CFG_TUSB_DEBUG=2)

n/a

Screenshots

No response

I have checked existing issues, dicussion and documentation

[X] I confirm I have checked existing issues, dicussion and documentation.

HiFiPhile commented 2 months ago

Possibly, it was a deliberate decision that usbd_edpt_close() does not free the TX FIFO RAM.

Yes. usbd_edpt_close() will be removed later.

I am maintaining code with a custom device class implementation that has to support changing the alternate interface settings.

Your implementation is out of USB specification. These FIFO alloc/free routines are used for ISO endpoints where EP size changes in each alt settings, they are deprecated since too buggy and replaced by dcd_edpt_iso_alloc() and dcd_edpt_iso_activate().

https://github.com/hathach/tinyusb/pull/2333

A possible option would be to introduce a usbd_edpt_close_all() function.

No it's a global reset and should only be used by set configuration request, one class behavior shouldn't affect others.

manuelbl commented 2 months ago

@HiFiPhile Thanks for the prompt response. Can you be more specific: what exactly is "out of USB specification"?

HiFiPhile commented 2 months ago

It seems you are switching bulk and interrupt ep with alt settings.

Alternate settings are mean to be used to adjust bandwidth allocation, such as in UAC class.

While it's not forbidden, AFAIK there is no standard class defined by USB-IF has such behavior. It's impossible to maintain a stack supporting all kind of behaviors.

For your case there are 2 ways:

Use Set configuration to make major changes
dcd_edpt_iso_alloc() and dcd_edpt_iso_activate() should work

manuelbl commented 2 months ago

@HiFiPhile I agree that it's mainly used for adjusting the bandwidth. The only exceptions that I'm aware are the DFU and the NCM classes. DFU is very simple as it only uses the control endpoint. With NCM, I'm not really familiar.

It's reasonable decision to restrict alternate interface to bandwidth adjustment. It would be even better if it was documented. Is there a place where this could be added? I would submit a PR.

Thanks for the pointing out dcd_edpt_iso_alloc() and dcd_edpt_iso_activate(). They might work for my unusual case.

HiFiPhile commented 2 months ago

It's reasonable decision to restrict alternate interface to bandwidth adjustment. It would be even better if it was documented. Is there a place where this could be added?

I agree there isn't much documentation for advanced usage. And I don't have idea where it should be put...

If you use NCM you can give a try the new driver https://github.com/hathach/tinyusb/pull/2227

@hathach Should we rename dcd_edpt_iso_alloc() and dcd_edpt_iso_activate() to cover all endpoint types ?

9.2.4  Data Transfer
Data may be transferred between a USB device endpoint and the host in one of four ways.  Refer to
Chapter 5 for the definition of the four types of transfers.  An endpoint number may be used for different
types of data transfers in different alternate settings.  However, once an alternate setting is selected
(including the default setting of an interface), a USB device endpoint uses only one data transfer method
until a different alternate setting is selected.

hathach / tinyusb