Skip to content
This repository was archived by the owner on Jun 5, 2019. It is now read-only.

Correct code with wrong call to USB_OpenStream #439

Closed
wants to merge 1 commit into from

Conversation

josesimoes
Copy link
Contributor

If USB_ALLOW_CONFIGURATION_OVERRIDE is defined and the call to USB_Initialize has a debugger port in the argument, USB_OpenStream is already called there (@ line 265). After a successful return from Initialize the call to USB_OpenStream will fail because the stream is already opened.

Note: I've stumbled onto this when debugging the CMSIS USB driver I'm working on. The result is that the USB device is enumerated and shows in the device manager but it's not picked up by MFDeploy or VS because after the initial enumeration the USB module is too busy being initialized again and again. This may explain the behaviour reported in issue #376.
The root cause is that the call sequence (...) DebuggerPort_Initialize -> USB_Initialize is repeated over and over from CLR_HW_Hardware::ProcessActivity() because the debugger port never has the chance to initialize. This is not 100% repetible. It seems to occur only if the USB init falis for some reason when sending the initial debug ping message, which call the above sequence. After that the ProcessActivity() starts calling that sequence again and most likely a race condition occurs with the USB stream.

If USB_ALLOW_CONFIGURATION_OVERRIDE is defined and the call to USB_Initialize has a debugger port in the argument, USB_OpenStream is already called there (@ line 265). After a successful return from Initialize the call to USB_OpenStream will fail because the stream is already opened.

Note: I've stumbled onto this when debugging the CMSIS USB driver I'm working on. The result is that the USB device is enumerated and show in device manager but it's not picked up by MFDeploy or VS because after the initial enumeration the USB module is too busy being initialized again and again. This may explain the behaviour reported in issue NETMF#376.
The root cause is that the call sequence (...) DebuggerPort_Initialize -> USB_Initialize is repeated over and over from CLR_HW_Hardware::ProcessActivity() because the debugger port never has the chance to initialize. This is not 100% repetible. It seems to occur only if the USB init falis for some reason when sending the initial debug ping message, which call the above sequence. After that the  ProcessActivity() starts calling that sequence again and most likely a race condition occurs with the USB stream.
josesimoes added a commit to Eclo/netmf-interpreter that referenced this pull request Apr 20, 2016
@smaillet-ms
Copy link
Member

USB_ALLOW_CONFIGURATION_OVERRIDE is technically orthogonal to the use of USB as a debug transport. Your PR here is merging the two concepts, which is something we don't want to do. USB_ALLOW_CONFIGURATION_OVERRIDE is a build time configuration to allow MFDeploy and managed apps to change the USB configuration from what it booted as. While this is obviously something you don't want while using USB AS your debug transport it isn't unique to that scenario.
A cleaner, long term solution would be to make the initialization process consistent so we don't have the multiple initialization calls, or add a requirement that the initialization is idempotent

@josesimoes
Copy link
Contributor Author

WOW 😲

There is clearly an issue with the call to USB_OpenStream after a successful return from USB_Initialize.
That call can - and will - fail depending on how fast things are moving.
The outcome is that it will lead to an unresponsive USB device because it will enter into a loop of initializing it over and over.
The bug is identified so feel free to implement a correction that you see appropriate.

@smaillet-ms
Copy link
Member

Closing this PR I've opened bug #476 to cover the issue.

@smaillet-ms smaillet-ms closed this Jul 9, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants