mimxrt: Fix corruption due to sdcard transfer alignment.#19304
mimxrt: Fix corruption due to sdcard transfer alignment.#19304projectgus wants to merge 2 commits into
Conversation
Now runs on mimxrt as well (less effective due to virtual timers only). Can be easily extended to run on other ports. This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>
- Invalidate the DCache after reads complete, to avoid stale cache lines that were read during the operation (the driver already cleans the cache before the operation). - Disable DMA if the read buffer isn't DCache aligned, to avoid corruption if the CPU writes to the same cache line as the buffer during the operation. The sdcard_dma_align.py test added in the parent commit is fixed by this commit. This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>
|
@andrewleech @robert-hh This may be relevant to your discussions in #18451 (apologies for not engaging with that PR, yet, as I see it has some overlapping changes.) @kwagyeman Hopefully of use to you as well. 🙂 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19304 +/- ##
=======================================
Coverage 98.47% 98.47%
=======================================
Files 176 176
Lines 22845 22845
=======================================
Hits 22497 22497
Misses 348 348 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Code size report: |
|
@projectgus I verified that this PR has a good effect. Testing with a MIXRT1050EVK, the test reports errors during interrupted read using the master branch, and it passes fine with this PR. The test was repeated 5 times. Also tested with MIMXRT1020_EVK, Teensy 4.1 and MIMXRT1176_EVK with 3 test runs each. No fail. |
|
I checked the driver. I see clean on tx data, and with the changes applied, you have a clean/invalidate on rx before and an invalidate afterward. This is the correct behavior. If the test is passing, then it should be good! |
dpgeorge
left a comment
There was a problem hiding this comment.
Look good! I verified that the test still passes on PYBD_SF6. I did not test any mimxrt board.
| @@ -1,30 +1,44 @@ | |||
| # Test DMA read operations when the buffer alignment in RAM varies. | |||
There was a problem hiding this comment.
Does it make sense to call this test machine_sdcard_dma_align.py since it's using machine.SDCard?
(Eventually we'd also add machine.SDCard for stm32 port, so that the use of pyb can be removed from this test.)
| print("SKIP") | ||
| raise SystemExit | ||
|
|
||
| READBLOCKS_SUCCESS = (0, True) # some drivers return True for success, some return 0... |
There was a problem hiding this comment.
Ah, this will be fixed by #16223 ... I've rebased that so that PR should be ready to go.
Nothing to change here. I'll have to update that PR if this is merged before it.
Summary
Fixes #19010. Follow-up to PR #19140 which fixed this bug on stm32, and this PR is to fix the same bug on mimxrt.
This PR has some overlap with #18451, I apologise that I haven't looked super closely at that PR yet.
First commit is to add a test:
sdcard_dma_align.pytest from stm32: Fix cache corruption on unaligned SDCard reads #19140 so it now works on mimxrt as well. This test relies on a high frequency timer interrupt where the CPU reads/writes memory throughout the SD transfer. It's a little less effective on mimxrt because the max timer frequency is only 1kHz - however it still seemed able to trigger the bug.Then the fix:
Testing
sdcard_dma_align.pybefore and after the fix on TEENSY41 & MIMXRT1050. Both boards fail without the fix, and pass with the fix. (An interesting note, the first part of the fix - disabling DMA - doesn't seem to be necessary to pass the test on MIMXRT1050, even running a high number of repeats. I think this is because the interrupts only trigger at 1kHz so it's very hard to have one trigger while the start or end cache line is being read, and the driver itself disables DMA internally if a buffer isn't 4-byte aligned so we're aiming for a very small target. This change was, however, very necessary for the test to pass on the Teensy - not entirely sure what the difference is there.)REPEATSvalue by 30 on both boards, checked these extended ~20 minute test runs also pass with the fix applied.Trade-offs and Alternatives
dma_protect_rx_region()method that we used on stm32 instead, however (a) that method doesn't scale to multiple concurrent DMA transfers, and (b) I couldn't actually figure out where the MPU is configured on mimxrt! (Although I didn't look for that long.)Generative AI
I did not use generative AI tools when creating this PR.