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

work around possible RP2040 erratum #2492

Merged
merged 4 commits into from
Mar 13, 2024
Merged

Conversation

tlyu
Copy link
Contributor

@tlyu tlyu commented Feb 25, 2024

RP2040 device controller does not seem to clear pending transactions configured in EP0 buffer controls when the host aborts a control transfer. This causes assertion failures, including when a buffer AVAILABLE flag set for a previous transfer causes an unexpected transaction completion.

Describe the PR
Fix assertion failures or panics in the RP2040 DCD when handling aborted control transfers.

Additional context
See adafruit/circuitpython#8824 for analysis and traces.

It's possible this is a hardware erratum. There is no documentation in the RP2040 datasheet about how aborted control transfers are handled. The datasheet does say that the STALL bits are cleared, but nothing about the buffer controls.

There is still a possibility of a race condition, if the control request IRQ isn't serviced before the previously set buffer control configuration causes the peripheral to do a transaction.

@tlyu
Copy link
Contributor Author

tlyu commented Feb 26, 2024

I'll have to revise this patch, both to clear the ABORT_DONE bits, and to only use ABORT on hardware B2 or later. See adafruit/circuitpython#8824 for details.

RP2040 device controller does not seem to clear pending transactions
configured in EP0 buffer controls when the host aborts a control
transfer. This causes assertion failures, including when a buffer
AVAILABLE flag set for a previous transfer causes an unexpected
transaction completion.
@tlyu tlyu force-pushed the fix-rp2040-ctrl-xfer branch from dc8cb27 to 473d400 Compare February 26, 2024 15:26
@tlyu tlyu marked this pull request as ready for review February 26, 2024 15:26
@hathach
Copy link
Owner

hathach commented Mar 5, 2024

thank you I am able to reproduce the issue mentioned in adafruit/circuitpython#8824 with stock cdc_dual_ports example using my intel macbook (os 12.6) using tio quit. Indeed this seems to be macos specific issue (probably related to how its serial driver is implemented). I will investigate more and review suggested fix by this PR.

@hathach
Copy link
Owner

hathach commented Mar 5, 2024

as expected, macos send 2 setup consecutive probably due to late response from rp2040. Per USB specs, host can send up to 3 setup packet consecutively, for this tinyusb should drop the previous setup request, abort pending transfer and process this one instead. (Note: since the 2 setup packets are indentical, we can actuall drop the 2nd setup and continue to process which is simpler).

image

@tlyu
Copy link
Contributor Author

tlyu commented Mar 5, 2024

Standard timeout for control requests with no data stage is 50ms. The second SETUP arrives within the same millisecond as the ACK for the first. This is more likely the host application (probably actually the kernel serial driver) cancelling the control request IRP before completion.

I don't think we can depend on the second control request being identical to the first, in that case, so it's safer to process both. Skipping the data or status stages when the interrupt flags show that a SETUP packet has arrived might also be a good addition to my current patch.

@tlyu
Copy link
Contributor Author

tlyu commented Mar 5, 2024

USB 2.0 §5.5.5

If a Setup transaction is received by an endpoint before a previously initiated control transfer is completed, the device must abort the current transfer/operation and handle the new control Setup transaction. A Setup transaction should not normally be sent before the completion of a previous control transfer. However, if a transfer is aborted, for example, due to errors on the bus, the host can send the next Setup transaction prematurely from the endpoint’s perspective.

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

superb! Thank you very much for investigation and fixing the issue. I have also made some tweaks to the PR

  • only abort ep0 transfer if it is currently active
  • only process the last setup packet in event queues. While testing, I found that some time macos send 2 setup packets fast enough that the 1st setup isn't processed yet. Which can still cause issue
  • other minor log

For B0, B1 variant since we cannot abort the current transfer, setting buffer control can cause unexpected behavior, but I guess those variant will fade away anyway.

@hathach hathach merged commit 1577572 into hathach:master Mar 13, 2024
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants