fix(dcd): Fixed race condition on device disconnect

[tore-espressif]

TinyUSB does not provide any locking means to protect the DCD variables. This can lead to race conditions when the user is trying to submit a transfer while the device is being disconnected. This can cause the device to be in an inconsistent state, leading to a crash or undefined behavior.

This commit adds a spin-lock to protect the DCD variables during device disconnect.

• Closes crash in tud_hid_keyboard_report() due to race condition (IDFGH-8198) esp-idf#9691
• Also reported in fix(tusb_msc): Cache sector metrics, enable storage buffering esp-usb#131
• Upstream PR fix(dcd): Fixed race condition on device disconnect hathach/tinyusb#3109

[igi540]

LGTM.

[roma-jam]

I think that for better compatibility with the upstream, we need to provide this mechanism in the dwc2_esp32.h file with default definition for all other controllers. In other words, make a minimum changes in the upstream dcd_dwc2.c file.

Otherwise we just break all the builds for other controllers family which potentially will lead to difficulties during next synchronizations.

[tore-espressif]

I think that for better compatibility with the upstream, we need to provide this mechanism in the dwc2_esp32.h file with default definition for all other controllers. In other words, make a minimum changes in the upstream dcd_dwc2.c file.

Otherwise we just break all the builds for other controllers family which potentially will lead to difficulties during next synchronizations.

@roma-jam Thank you for the review!

I added new file dwc2_critical.h because adding it to dwc2_esp32.h produced too many compiler warnings

[tore-espressif]

@roma-jam do you agree with the changes now? can we merge and release?

[roma-jam]

@tore-espressif give me a moment, I want to check it on a hardware.

[roma-jam]

@tore-espressif Unfortunately, I'm not able to reproduce the problem neither with tusb_hid, nor with tusb_msc examples. I will try more.

May I ask you to share the log, how the application handled the situation, while we return false; // Endpoint is closed at the line 593?

[tore-espressif]

The bug can be 100% reproduced in https://github.com/espressif/esp-usb/tree/master/host/class/msc/usb_host_msc/test_app test number 2 and esp-idf release/v5.2 branch.

[roma-jam]

The bug can be 100% reproduced in https://github.com/espressif/esp-usb/tree/master/host/class/msc/usb_host_msc/test_app test number 2 and esp-idf release/v5.2 branch.

but only in espressif/esp-usb#131, not in master branch.

[roma-jam]

One thing: is there could be a possible situation when GINTSTS_USBRST and GINTSTS_OEPINT or GINTSTS_IEPINT will be set in the same time, we could get the division by zero again.

Maybe we need to cover all the calls of edpt_schedule_packets() in this case to prevent such logic scenario.

Otherwise, LGTM.

[tore-espressif]

One thing: is there could be a possible situation when GINTSTS_USBRST and GINTSTS_OEPINT or GINTSTS_IEPINT will be set in the same time, we could get the division by zero again.

I see. However, practically, having both Rest and EP interrupt at the same time should never happen if we handle interrupts reasonably fast...

I'll merge as it is and will continue watching the long-term stability