Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nrf52: usb: better check state on IN resume #1947

Merged
merged 1 commit into from Jun 17, 2020
Merged

Conversation

bradjc
Copy link
Contributor

@bradjc bradjc commented Jun 16, 2020

While running a modified printf_long test app that prints a long string in a loop on nano33ble, I got an assertion failure:

Kernel panic at chips/nrf52/src/usbd.rs:1749:
	"assertion failed: `(left == right)`
  left: `Some(InData)`,
 right: `Some(Init)`"
	Kernel version release-1.5-500-g9c520bd6b

This happens because the resume_in function was only checking the state of the DMA, but not of the IN endpoint, which might be finished with DMA but the entire transfer hasn't finished. I think this wasn't occurring before because with short IN transfers, the end of the DMA and the end of the transfer occur very close to each other and the endpoint wasn't in the InData state very long. However, with a longer transfer the InData state is longer and thus this assertion could fail.

The fix is just to check both states, and enqueue the resume in request if either is true.

Testing Strategy

This pull request was tested by running printf_long in a loop on nano33ble. It actually works!

TODO or Help Wanted

You can see my comment about the driver calling packet_in() as a direct result of the client calling endpoint_resume_in(). That should be fixed at some point, or the USB HIL should be changed so that there are no longer upcalls asking for data.

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

Just checking for DMA isn't quite enough, since the DMA could have
finished but the previous IN transfer hasn't necessarily finished
yet.
@bradjc bradjc added the nrf Change pertains to the nRF5x family of MCUs. label Jun 16, 2020
Copy link
Contributor

@brghena brghena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix.

As for the TODO, lots of areas of the USB stack are pretty crufty and in need of a rewrite. Documenting problem areas when we find them is at least helpful towards that future goal.

Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

@bors bors bot merged commit 46c6f2c into master Jun 17, 2020
@bors bors bot deleted the nrf52-usb-resume-in branch June 17, 2020 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nrf Change pertains to the nRF5x family of MCUs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants