USB Assert - In Endpoint

[AGlass0fMilk]

Description

After my PR #10689 was recently merged, I synced a project I'm working on up to Mbed-OS master. This project uses USB MSD.

Now, when I connect my device to a host (tried on Linux and Windows), I hit the following assert:

++ MbedOS Error Info ++                                                                                                       
Error Status: 0x80FF0144 Code: 324 Module: 255                                                                                
Error Message: Assertion failed: info->pending >= 1                                                                           
Location: 0x6549                                                                                                              
File: ./mbed-os/usb/device/USBDevice/USBDevice.cpp+955                                                                        
Error Value: 0x0                                                                                                              
Current Thread: main  Id: 0x2000155C Entry: 0x76B5 StackSize: 0x1000 StackMem: 0x200023B0 SP: 0x2003FF00                      
For more info, visit: https://mbed.com/s/error?error=0x80FF0144&tgt=NRF52840_DK                                               
-- MbedOS Error Info --       

I then changed back to my feature branch (https://github.com/AGlass0fMilk/mbed-os/tree/nrf52840-usbphy-implementation), rebuild and the issue is gone. My device mounts as /dev/sdb fine.

Not sure if anything changed in the USB stack that might be causing this. I don't see any other recent changes.

Essentially, my main code ends with (after setting up FAT file system from example):

USBMSD usb(&qspi_blockdevice);
while (true) {
	usb.process();
}

I am also printing to the debug UART. Other than that, there's not much else going on in the project.

I tested this on my USB Serial example here: https://github.com/AGlass0fMilk/mbed-usb-cdc-example.git

Same results. When I'm on my feature branch it works as expected. When I pull master (currently at 14b77c9) the code hits the assert as soon as I plug in USB.

I haven't had luck tracing down the commit that introduced this issue. My feature branch isn't that stale so that should help narrow it down.

Testing on nRF52840_DK with GCC_ARM toolchain.

Issue request type

[ ] Question
[ ] Enhancement
[X] Bug

[AGlass0fMilk]

Did some testing where I rebased my feature branch against some newer commits that have been pulled into master until the issue came back. Rebasing on dc77c40 causes the issue to appear so it seems to be related to Cordio.

Maybe the USB interrupt priority needs to be higher?

[AGlass0fMilk]

Rebased with df84eb1 (just before Cordio PR) and I can confirm it works as expected.

I tried adjusting the USB interrupt priority all the way to 0 (highest) and still the issue appeared, albeit maybe a little delayed compared to the lower priority.

I also tried removing Cordio from the build by removing the BLE feature from the nRF52840_DK and the issue still occurred. Not sure how that's happening.

@paul-szczepanek-arm Any idea what could be happening?

It'd be understandable if USB and BLE could not be used at the same time but simply linking BLE in shouldn't disable USB.

[AGlass0fMilk]

If I revert the changes to targets.json introduced in dc77c40 the issue goes away. May be related to removing the soft device (although I don't use any soft-device aware functions that I know of. Maybe the power peripheral driver.)

The following changes to the MCU_NRF52840 target produces the issue:

Hopefully there isn't some weird reliance on the softdevice that the USB driver/hardware has...

[ciarmcom]

Internal Jira reference: https://jira.arm.com/browse/MBOCUSTRIA-1302

[AGlass0fMilk]

My current theory is that the issue is related to how critical regions are handled when using the softdevice vs when not using the softdevice.

It seems that an IN interrupt is happening before the upper stack can officially queue a write operation. This violates the stack's logic and results in an assert. It seems to happen only on non-control endpoints? I have to do some more testing with different USB classes to see how the error changes.

Below are two screenshots of the same code (the usb serial example I linked to above). One compiled with softdevice support and one compiled without. The one without the SD abruptly stops after the assert happens.

Failure

No Failure:

[AGlass0fMilk]

It seems the cause of the issue lies in the simplified critical region API being used by the Nordic USB driver (and presumably all Nordic drivers). Normally, with a softdevice-enabled build, the critical region API used by the Nordic drivers is redirected to sd_nvic_critical_region_enter/exit:

mbed-os/targets/TARGET_NORDIC/TARGET_NRF5x/TARGET_SDK_15_0/components/libraries/util/app_util_platform.c

Lines 64 to 90 in bf78dc4

	void app_util_critical_region_enter(uint8_t *p_nested)
	{
	#if __CORTEX_M == (0x04U)
	ASSERT(APP_LEVEL_PRIVILEGED == privilege_level_get())
	#endif
	#if defined(SOFTDEVICE_PRESENT)
	/* return value can be safely ignored */
	(void) sd_nvic_critical_region_enter(p_nested);
	#else
	app_util_disable_irq();
	#endif
	}
	void app_util_critical_region_exit(uint8_t nested)
	{
	#if __CORTEX_M == (0x04U)
	ASSERT(APP_LEVEL_PRIVILEGED == privilege_level_get())
	#endif
	#if defined(SOFTDEVICE_PRESENT)
	/* return value can be safely ignored */
	(void) sd_nvic_critical_region_exit(nested);
	#else
	app_util_enable_irq();
	#endif
	}

This function is defined in nrf_nvic.h/c, which changes based on which softdevice "target" you enable (in this case, it's SOFTDEVICE_NONE):

mbed-os/targets/TARGET_NORDIC/TARGET_NRF5x/TARGET_SDK_15_0/TARGET_SOFTDEVICE_NONE/nrf_soc_nosd/nrf_nvic.c

Lines 103 to 123 in bf78dc4

	uint32_t sd_nvic_critical_region_enter(uint8_t * p_is_nested_critical_region)
	{
	__disable_irq();
	*p_is_nested_critical_region = (m_in_critical_region != 0);
	m_in_critical_region++;
	return NRF_SUCCESS;
	}
	uint32_t sd_nvic_critical_region_exit(uint8_t is_nested_critical_region)
	{
	m_in_critical_region--;
	if (is_nested_critical_region == 0)
	{
	m_in_critical_region = 0;
	__enable_irq();
	}
	return NRF_SUCCESS;
	}

This is the same function used by the Mbed critical_section HAL implementation for nRF52-based targets:

mbed-os/targets/TARGET_NORDIC/TARGET_NRF5x/TARGET_NRF52/critical_section_api.c

Lines 25 to 43 in bf78dc4

	void hal_critical_section_enter()
	{
	/* expect 1 or N calls but only lock on the first one */
	if (outside_critical_region) {
	sd_nvic_critical_region_enter(&outside_critical_region);
	}
	}
	void hal_critical_section_exit()
	{
	/* unlock on first call */
	outside_critical_region = 1;
	sd_nvic_critical_region_exit(0);
	}
	bool hal_in_critical_section(void)
	{
	return !outside_critical_region;
	}

However, when the macro SOFTDEVICE_PRESENT is disabled, you see in the first code block above the Nordic app_util critical region API gets retargeted to app_util_disable/enable_irq.

mbed-os/targets/TARGET_NORDIC/TARGET_NRF5x/TARGET_SDK_15_0/components/libraries/util/app_util_platform.c

Lines 47 to 62 in bf78dc4

	static uint32_t m_in_critical_region = 0;
	void app_util_disable_irq(void)
	{
	__disable_irq();
	m_in_critical_region++;
	}
	void app_util_enable_irq(void)
	{
	m_in_critical_region--;
	if (m_in_critical_region == 0)
	{
	__enable_irq();
	}
	}

This has a critical region counter that is entirely unaware of the number of nested critical regions Mbed may be in at the time and so if it isn't in a nested critical region, it globally enables interrupts again and completely crashes Mbed's expectation of being in a critical section.

I tested the fix in my fork and it fixed this issue. Needs to be CI tested with all the other Nordic drivers (which I'm surprised still worked with this going on):

https://github.com/AGlass0fMilk/mbed-os/blob/f548f558da387be2ac88249c87d14dbd784ad81e/targets/TARGET_NORDIC/TARGET_NRF5x/TARGET_SDK_15_0/components/libraries/util/app_util_platform.c#L66-L101

I don't see a nice clean way of fixing this given the software architecture of Nordic's SDK. That discussion will be saved for the associated PR: #10881

[paul-szczepanek-arm]

That is some sterling work, thank you.

[linlingao]

Excellent work indeed. Looks like PRs have been merged, can you close this?

[AGlass0fMilk]

Resolved by #10881