sdmmc_sdio_hal: two Cortex-M7 cache coherency defects in SD DMA read path (safe on single-core, risk on future multi-core targets)

[daijoubu]

Summary

Two memory-ordering defects exist in src/main/drivers/sdcard/sdmmc_sdio_hal.c in the DMA read path. Both are safe in practice on all current single-core INAV targets (STM32F4/F7/H7), but would become genuine race conditions on any future multi-core target such as the RP2350 (dual Cortex-M33).

The same code is present verbatim in Betaflight (src/platform/STM32/sdio_h7xx.c).

---

Finding 1 — sdReadParameters missing volatile

Location: sdmmc_sdio_hal.c, near line 633

// current
sdReadParameters_t sdReadParameters;

// correct
static volatile sdReadParameters_t sdReadParameters;

sdReadParameters holds the DMA receive buffer pointer, block size, and block count. It is written by SD_ReadBlocks_DMA() (main context) and read by HAL_SD_RxCpltCallback() (ISR context) to calculate the D-cache invalidation range. Without volatile, the C standard permits the compiler to cache these fields in registers and not reload from memory inside the ISR, meaning cache invalidation could use a stale buffer address or wrong size.

In practice GCC flushes globals to memory at external function call boundaries, so this does not currently manifest. On a multi-core target a second core running the ISR would have no such guarantee.

The variable also has unnecessary external linkage; making it static is a clean-up with no functional impact.

---

Finding 2 — RXCplt completion flag cleared before D-cache invalidation

Location: sdmmc_sdio_hal.c, HAL_SD_RxCpltCallback()

// current — semantically wrong ordering
void HAL_SD_RxCpltCallback(SD_HandleTypeDef *hsd)
{
    SD_Handle.RXCplt = 0;                        // signals "done" ...
    uint32_t alignedAddr = ...;
    SCB_InvalidateDCache_by_Addr(...);            // ... before cache is coherent
}

// correct — invalidate first, then signal done
void HAL_SD_RxCpltCallback(SD_HandleTypeDef *hsd)
{
    uint32_t alignedAddr = ...;
    SCB_InvalidateDCache_by_Addr(...);
    __DMB();                                     // drain before flag store
    SD_Handle.RXCplt = 0;
}

The ready flag RXCplt = 0 is stored before SCB_InvalidateDCache_by_Addr() runs. The main loop polls SD_CheckRead() which reads RXCplt; once it observes zero it immediately accesses the receive buffer. If RXCplt = 0 becomes visible to another core before the cache invalidation completes, that core reads stale cached data.

On current single-core targets the ISR runs to completion before the main loop resumes, and SCB_InvalidateDCache_by_Addr contains an internal __DSB() that incidentally drains the write buffer anyway — so the ordering bug has no observable effect. On a multi-core target it is a real data race.

The fix mirrors the store-release pattern established for the CAN TX ISR SPSC queue (DMB between data and flag store).

---

Affected code paths

The read path is actively used: asyncfatfs calls sdcard_readBlock() to read FAT sectors (boot sector, FAT tables, directory entries) whenever it needs a sector not in its cache. This occurs at every SD card mount and throughout blackbox logging sessions.

Affected targets: all 20 targets defining USE_SDCARD_SDIO, including MATEKF765, MATEKH743, KAKUTEH7WING, and others.

---

Proposed fix

-sdReadParameters_t sdReadParameters;
+static volatile sdReadParameters_t sdReadParameters;

 void HAL_SD_RxCpltCallback(SD_HandleTypeDef *hsd)
 {
     UNUSED(hsd);
 
-    SD_Handle.RXCplt = 0;
-
     /*
        the SCB_InvalidateDCache_by_Addr() requires a 32-Byte aligned address,
        adjust the address and the D-Cache size to invalidate accordingly.
+       Invalidate before clearing RXCplt so the buffer is coherent by the
+       time the main loop observes the transfer as complete.
      */
-    uint32_t alignedAddr = (uint32_t)sdReadParameters.buffer &  ~0x1F;
+    uint32_t alignedAddr = (uint32_t)sdReadParameters.buffer & ~0x1F;
     SCB_InvalidateDCache_by_Addr((uint32_t*)alignedAddr, sdReadParameters.NumberOfBlocks * sdReadParameters.BlockSize + ((uint32_t)sdReadParameters.buffer - alignedAddr));
+    __DMB();
+    SD_Handle.RXCplt = 0;
 }

The same fix should be applied to Betaflight's src/platform/STM32/sdio_h7xx.c.

[daijoubu]

As I can't think of a great way to validate this fix is safe, and it's never caused a problem with all the single core targets, pushing the fix to maintenance-9.x isn't justified. If the rp2350 port goes forward, it's worth fixing.

[sensei-hacker]

I think the volatile qualifier can be treated as safe because the effect is the same as what we likely already get on a lot of targets - the variable is accessed from SRAM.

The "cost" of that is up to one clock cycle. While doing IO with flash memory that is a thousand times slower than that. So worst-case cost of a CPU cycle is inconsequential.

Similarly, __DMB() just guarantees what already happens anyway on single cores, most likely. Seems very unlikely to cause a problem.

[daijoubu]

I agree. I'd only opened it because I know you have the rp2350 porting project in Inav claude.