Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.46%. Comparing base (8a56be6) to head (799356e).

@@           Coverage Diff           @@
##           master   #19140   +/-   ##
=======================================
  Coverage   98.46%   98.46%           
=======================================
  Files         176      176           
  Lines       22811    22811           
=======================================
  Hits        22460    22460           
  Misses        351      351           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Code size report:

Reference:  extmod/moductypes: Be more defensive with uctypes_struct_agg_size args. [8a56be6]
Comparison: sdcard: Use dma_protect_rx_region on read operations. [merge of 799356e]
  mpy-cross:    +0 +0.000% 
   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:   +56 +0.014% PYBV10
      esp32:    +0 +0.000% ESP32_GENERIC
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

@kwagyeman @peterzuger This PR should fix STM32 D-Cache corruption on SD reads.

Thanks for working on this! The fix (even if it's not the complete solution) looks relatively clean and simple, which is great.

There is still some potential for corruption if the buffer is not 4-byte aligned and the CPU writes to another byte in the same 32-bit word. This seems a limitation of the bus not the D-Cache, and won't trigger except in very unusual cases (as buffers will almost always be 4-byte aligned.) The tests are written to avoid this case.

sdcard_read_blocks() has a special case to handle destination buffers that are not 4-byte aligned. So this case you describe should never be hit.

Did you find issues with buffers that were not 4-byte aligned? If not, could the test be tweaked to test also cases of non-4-byte alignment?

New test cases test_interrupted_reads() and test_interrupted_read_write() fail immediately on PYBD_SF2 without this fix, do not fail after fix.

(I assume you mean that the tests fail with the first one/two commits from this PR, to keep IRQs on while SD card reads are active. The tests don't fail on master.)

I tested this PR on PYBD_SF2 and PYBD_SF6:

• including the MSC IRQ improvement from this PR, the test included in this PR fails on both boards (with about 110 failures reported by unittest)
• including the DMA fix in this PR, the test passes on both boards (I ran it a few times and it passed all times)

sdcard_read_blocks() has a special case to handle destination buffers that are not 4-byte aligned. So this case you describe should never be hit.

Oh, this case is the reason for what I described! That makes more sense, I thought it was a weird behaviour if the hardware was doing it.

What I was seeing is that if you (for example) have a DMA operation reading into a buffer starting at address 0x13 and during the operation you have an interrupt write the byte at offset 0x12 then the whole word starting at 0x10 is corrupted. I'm pretty sure it's a race between the interrupt and this fixup logic that's doing that.

The proposed "future work" change with temporary buffers will effectively align these buffers in another way, so that fixup logic can probably come out then (meaning we'll also not have the potential race any more if some code does write into that word from an interrupt).

Did you find issues with buffers that were not 4-byte aligned? If not, could the test be tweaked to test also cases of non-4-byte alignment?

The test_interrupted_read_write works around it by having the interrupt not write into the same word as the start or end of the buffer. I'll update the comment in the test to explain the real reason that's necessary.

We could change the test so instead of testing every possible offset it only tests offsets which are a multiple of 4, but then we're actually reducing the test coverage so I think this is better.

(I assume you mean that the tests fail with the first one/two commits from this PR, to keep IRQs on while SD card reads are active. The tests don't fail on master.)

Oh yes, my mistake. Those tests do fail on master but very infrequently, the same as test_reads(), because without the other changes there's no timer interrupts firing during the DMA operation so they're basically testing the same thing.

we need to use a different approach (probably temporary DMA-aligned buffer(s) covering any unaligned parts of the operation.)

Ah.. I'd been assuming we could use scatter/gather chained transfers, so we're only needing temporary buffers for the head & tail of any unaligned operation (this is how we did it when I was at Espressif). The newer ST GPDMA peripheral supports these, but the older ST DMA peripheral does not. Meaning H5,H7,U5,N6 all have GPDMA so we can have a generic solution with very little performance overhead, but the F7-based boards will be caught out (I think F7 is the only STM32 family we support that has D-Cache but doesn't have GPDMA).

There might be a related underlying issue I ran into on #18451 - the tests @robert-hh shared here did a great job of reproducing the intermittent issue. I gave up trying to fix it there but I'll take a look at what you've done here @projectgus to see if it's transferable.

Ah.. I'd been assuming we could use scatter/gather chained transfers, so we're only needing temporary buffers for the head & tail of any unaligned operation (this is how we did it when I was at Espressif). The newer ST GPDMA peripheral supports these, but the older ST DMA peripheral does not.

Maybe for the older parts (F7) we can just keep the current dma protect/unprotect and just have enough MPU regions for all possible concurrent DMA uses? Which might be just SD card and SPI.

The MIMXRT needs a fix for this too.