fix(sdcard): migrate dmaInit() to ownership-checked dmaAllocate() - #1398
Conversation
sdcard.c (SPI-mode) and sdio_f4xx.c/sdio_f7xx.c (SDIO-mode) called dmaInit(), which silently overwrites any existing DMA owner. Replace with dmaAllocate() + dmaEnable(), matching the pattern already used by ADC, transponder, LED strip, and motors. sdcard.c falls back to polled SPI (clearing useDMAForTx) on claim failure instead of aborting card init entirely -- useDMAForTx is already checked as a runtime gate at every transfer/completion site, so this is a supported fallback, not a new code path. Losing DMA acceleration is preferable to losing the SD card (and blackbox logging) over an unrelated DMA conflict. sdio_f4xx.c/sdio_f7xx.c change SD_Initialize_LL() from void to bool, matching BF's return type, with the claim check moved to the very top before any RCC/GPIO/DMA register access. sdio_h7xx.c's no-op stub (H7 uses internal IDMA) is now bool too, matching BF exactly. The claim intentionally resolves the DMA identifier from the `dma` parameter, not a file-scope static -- BF 4.5-maintenance's own equivalent computes it from a stale static assigned only after the check, making dmaAllocate() operate on an invalid identifier and the function always return false. That bug was fixed in BF master (PR#14990, function since renamed SD_InitialiseHardware) as a side effect of an unrelated DMA-API refactor, not a targeted fix. Not filed upstream per explicit instruction; tracked in BF45-SDIO-DMASTREAM-BUG.md instead. Two callers (usbd_storage_sdio.c, sdcard_sdio_baremetal.c) previously ignored SD_Initialize_LL()'s return value entirely. Since neither caller configures the DMA stream itself, a claim failure would leave the file-scope dma_stream pointer NULL, and the first real card transfer would dereference it in SD_StartBlockTransfert() -- both callers now check the return and bail to SDCARD_STATE_NOT_PRESENT (or an equivalent failure return) instead. sdcard_init() and SD_Initialize_LL() can each be called twice per boot (once from fc_init.c, once from the USB MSC passthrough path) for the same logical OWNER_SDCARD. Added sdcardDmaClaim()/ sdioDmaClaim(), matching the existing uartDmaClaim()/dshotDmaClaim() pattern: a stream already held by the same owner+resourceIndex is treated as an already-successful claim, so the second call is not mistaken for a real conflict.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (13)
🔇 Additional comments (7)
📝 WalkthroughWalkthroughSD-card and SDIO initialization now claim DMA resources with owner-aware reuse. F4 and F7 drivers return initialization status. SD-card and USB storage callers handle DMA or low-level initialization failures. Unit tests cover claim reuse and conflicts. ChangesSD DMA initialization
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This localized DMA ownership change includes a polled-transfer fallback and updated failure handling, with reported build and unit-test coverage. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant STORAGE_Init
participant SD_Initialize_LL
participant DMA
STORAGE_Init->>SD_Initialize_LL: initialize SDIO DMA
SD_Initialize_LL->>DMA: claim and enable DMA stream
alt initialization succeeds
SD_Initialize_LL-->>STORAGE_Init: return true
else initialization fails
SD_Initialize_LL-->>STORAGE_Init: return false
STORAGE_Init-->>STORAGE_Init: return failure
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai, AI generated comment Requesting analysis, not a full review. sdcard.c (SPI-mode) and sdio_f4xx.c/sdio_f7xx.c (SDIO-mode) migrate dmaInit() to dmaAllocate() + dmaEnable(). Two things specific to this PR worth independent verification:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Locate candidate files ==='
fd -t f -a '^(sdcard|sdio_f4xx|sdio_f7xx|fc_init|usbd_storage_sd_spi|usbd_storage_sdio|serial_uart_stm32f4xx)\.(c|h)$' . || true
echo '=== Function maps ==='
for f in $(fd -t f -a '^(sdcard|sdio_f4xx|sdio_f7xx|fc_init|usbd_storage_sd_spi|usbd_storage_sdio|serial_uart_stm32f4xx)\.c$' .); do
echo "--- $f"
ast-grep outline "$f" --items all --type function 2>/dev/null || true
done
echo '=== Relevant declarations, assignments, claims, and call sites ==='
rg -n -C 5 -P '\b(sdcardDmaClaim|sdioDmaClaim|uartDmaClaim|sdcard_init|SD_Initialize_LL|dmaAllocate|dmaEnable|dmaGetIdentifier|dma_stream|dmaStream|useDMAForTx)\b' \
-g 'sdcard.c' -g 'sdio_f4xx.c' -g 'sdio_f7xx.c' -g 'fc_init.c' \
-g 'usbd_storage_sd_spi.c' -g 'usbd_storage_sdio.c' -g 'serial_uart_stm32f4xx.c' .
echo '=== DMA-related static declarations and writes in SDIO implementations ==='
rg -n -C 4 -P '^\s*static\b.*\bdma|^\s*(dma_stream|dmaStream|dma)\s*=|\bdma_stream\b|\bdmaStream\b|\bdmaGetIdentifier\s*\(' \
-g 'sdio_f4xx.c' -g 'sdio_f7xx.c' .Length of output: 61568 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== SPI initialization and USB-MSC path ==='
sed -n '488,545p' src/main/drivers/sdcard.c
sed -n '570,610p' src/main/fc/fc_init.c
sed -n '130,165p' src/main/msc/usbd_storage_sd_spi.c
echo '=== All SDIO low-level initialization call sites and declarations ==='
rg -n -C 8 -P '\bSD_Initialize_LL\s*\(' -g '*.[ch]' .
echo '=== SDIO-mode initialization dispatch and USB-MSC path ==='
rg -n -C 10 -P '\b(SDIO_DMA|SD_Initialize_LL|SD_Init|sdcard_init)\b' \
-g 'sdcard.c' -g 'fc_init.c' -g 'usbd_storage_sdio.c' -g '*sdio*.[ch]' .
echo '=== Every dma_stream occurrence, with function boundaries ==='
for f in src/main/drivers/sdio_f4xx.c src/main/drivers/sdio_f7xx.c; do
echo "--- $f"
rg -n -C 12 -P '\bdma_stream\b' "$f"
done
echo '=== DMA-related file-scope state and direct DMA API calls in SDIO implementations ==='
for f in src/main/drivers/sdio_f4xx.c src/main/drivers/sdio_f7xx.c; do
echo "--- $f"
rg -n -C 5 -P '^\s*(static\s+)?(?:DMA_\w+|DMA_Stream_TypeDef|dma\w*)[^;]*;|\b(dmaAllocate|dmaEnable|dmaGetIdentifier|dmaGetDescriptorByIdentifier|dmaSetHandler|dmaGetOwner|dmaGetResourceIndex)\s*\(' "$f"
doneLength of output: 73785 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== SD-card configuration declaration, registration, and mutation sites ==='
rg -n -C 6 -P '\b(dmaIdentifier|sdcardConfig|sdcardConfigMutable|PG_SDCARD_CONFIG|pgResetFn_sdcardConfig)\b' \
-g '*.[ch]' .
echo '=== SDIO_DMA definitions and board-level overrides ==='
rg -n -C 5 -P '^\s*#\s*define\s+SDIO_DMA\b|\bSDIO_DMA\b' \
-g '*.[ch]' .
echo '=== DMA identifier-to-reference mapping API and validation ==='
rg -n -C 6 -P '\b(dmaGetRefByIdentifier|dmaGetIdentifier|dmaAllocate|dmaGetOwner|dmaGetResourceIndex)\s*\(' \
-g '*.[ch]' src/mainLength of output: 69896
You are interacting with an AI system. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
sdcardDmaClaim()/sdioDmaClaim() had zero host coverage. Mirrors the same fake-DMA-state convention already used by serial_uart_dma_claim_unittest.cc for uartDmaClaim() -- compiling the real sdcard.c/sdio_f4xx.c/sdio_f7xx.c under UNIT_TEST needs a disproportionate register-mock surface (90+ SDIO->/RCC-> field accesses in sdio_f4xx.c alone) for logic these two functions don't touch, same class of tradeoff already made for bus_spi_ll.c/bus_spi_stdperiph.c in feat/dma-ll-unittest-infra. Covers: fresh claim on a free stream, same-owner/index reopen (the fc_init.c-vs-USB-MSC-passthrough dual-caller scenario), foreign-owner conflict, same-owner-different-index conflict.
|
AI Generated comment Added host unit-test coverage for Testing approach: mirrors the claim logic with fake DMA state rather than compiling the real
Second-opinion review on the current diff (post-fix, both files touched by this PR): opencode ran clean, 0 new findings, explicitly re-confirmed the two previously-fixed issues (unchecked No SD-card-capable hardware exists in inventory to verify this on target — confirmed again this session that EmuFlight's own SITL target does not compile |
|
AI Generated comment @coderabbitai full review New commit (678da01) adds host unit-test coverage for sdcardDmaClaim()/sdioDmaClaim() (src/test/unit/sdcard_dma_claim_unittest.cc, src/test/Makefile) since the prior APPROVED review predates it. |
|
✅ Action performedFull review finished. |
1 participant
AI Generated pull-request
Summary
Stage 5 of 5 in IT#1363's fleet-wide
dmaInit()→dmaAllocate()migration (stage 1 ADC, stage 2 transponder, stage 3 LED strip all merged; stage 4 motors/DSHOT MERGED). Convertssdcard.c(SPI-mode) andsdio_f4xx.c/sdio_f7xx.c(SDIO-mode) from uncheckeddmaInit()to ownership-checkeddmaAllocate()+dmaEnable().Why this needed more than a mechanical swap
sdcard.c(SPI-mode): falls back to polled SPI (clearinguseDMAForTx) on claim failure instead of aborting card init entirely.useDMAForTxis already checked as a runtime gate at every transfer/completion site in this file — this is a supported, pre-existing fallback path, not new code. Losing DMA acceleration is preferable to losing the SD card (and blackbox logging) over an unrelated DMA conflict.sdio_f4xx.c/sdio_f7xx.c:SD_Initialize_LL()changed fromvoidtobool, matching BF's actual return type, with the claim check moved to the very top of the function — before any RCC/GPIO/DMA register access.sdio_h7xx.c's no-op stub (H7 uses internal IDMA, no external stream) is nowbooltoo, matching BF exactly.SD_Initialize_LL()computes the DMA identifier from a file-scope static (dmaStream) that's only assigned after the ownership check — meaningdmaAllocate()always operates on an invalid/NULL-derived identifier and the function always returnsfalsein 4.5-maintenance. This EF implementation resolves the identifier from thedmaparameter instead, matching what BF's code clearly intended. That bug was independently confirmed fixed in current BF master (commitb551d654a, PR#14990, "Refactor DMA driver to abstract platform-specific types...", function renamedSD_InitialiseHardware) as a side effect of an unrelated, much larger DMA-API refactor — not a targeted fix. Per explicit instruction this is not filed upstream; tracked independently in~/SYNC/nerdCopter-GIT/AI/EF/fix/dmainit-sdcard-migration/BF45-SDIO-DMASTREAM-BUG.md.usbd_storage_sdio.candsdcard_sdio_baremetal.cpreviously ignoredSD_Initialize_LL()'s return value entirely. Since neither caller configures the DMA stream itself, a claim failure would leave the file-scopedma_streampointerNULL, and the first real card transfer would dereference it inSD_StartBlockTransfert()— a hard fault, not a graceful failure. Both callers now check the return and bail toSDCARD_STATE_NOT_PRESENT(or an equivalent failure return) instead. Found via an opencode second-opinion pass, verified directly against source before fixing.sdcard_init()/SD_Initialize_LL()can each be called twice per boot — once fromfc_init.c, once from the USB MSC passthrough path — for the same logicalOWNER_SDCARD. AddedsdcardDmaClaim()/sdioDmaClaim(), matching the existinguartDmaClaim()/dshotDmaClaim()pattern already in this codebase: a stream already held by the same owner+resourceIndex is treated as an already-successful claim, so the second call isn't mistaken for a real conflict.Known architectural gap this PR does not close (tracked separately, not blocking)
sdcard.c(SPI-mode) still uses its own legacy per-driver DMA claim and the old raw-SPI polled API (spiInstanceByDevice(),spiTransfer()). BF 4.5-maintenance's equivalent (sdcard_spi.c) has no DMA-claim code of its own — it's fully migrated to the genericextDevice_t/spiSequence()bus-level mechanism (spiInitBusDMA()already claims DMA per-bus). This PR'sdmaAllocate()fix is correct and safe for IT#1363's actual scope (ownership-check migration) but does not migratesdcard.conto that generic mechanism. Filed as #1399, sized comparable to the IMUF9001 legacy-DMA migration (#1143, closed) — a full driver rewrite, not part of this PR. SDIO mode (sdio_f4xx.c/sdio_f7xx.c) has no equivalent gap — BF's own SDIO driver also claims DMA per-driver (SDIO isn't part of the generic SPI-bus abstraction).BF-master verdict check
Per the corrected
/bf-gapsprocess (4.5-maintenance is the reference, master checked as fallback for bugfixes/architecture direction, not the primary diff target):git log 4.5-maintenance..masterconfirms theSD_Initialize_LL()identifier bug above as4.5-only(fixed in master'sb551d654a). A second verdict for thesdcard.carchitectural gap (#1399):not-comparable— EF has no BF-equivalent file to diff against, since BF'ssdcard_spi.cnever had per-driver DMA-claim code to begin with. Full inventory:fix/dmainit-sdcard-migration/dmainit-sdcard-migration-bf-gaps.md; both verdicts logged inMIGRATION.md§ BF-branch register.Review history
Local
coderabbit review --agent: 0 findings, both rounds. An opencode second-opinion pass on the first round found two real issues (the NULL-deref and double-init cases above), both fixed and re-verified; a follow-up opencode pass on the fixed diff could not complete (infra instability, multiple models failing) and was not retried further, matching prior-session precedent for this tool. GitHub CodeRabbit's formal review:APPROVED, no actionable comments, all 7 changed files LGTM. A targeted analysis request (idempotent-reclaim logic + BF-bug avoidance) came back clean: confirmed both SPI-mode and SDIO-mode reclaim logic handle either boot-order permutation correctly, and confirmed no equivalent stale-static-before-assignment bug exists elsewhere in either file.Verification
USE_SDCARD), plus MLTYPHF4 (F4, SPI-mode + DMA), FLYSPARKF4 (F4, SDIO-mode), SPEEDYBEEF7V3 (F7, SDIO-mode), plus SITL.make test— 48/48 test binaries pass (up from 43; addedsdcard_dma_claim_unittest.cc, host coverage forsdcardDmaClaim()/sdioDmaClaim()— see PR comment for scope/rationale).USE_SDCARD, and noUSE_SDCARD-capable board (flight or bench) exists in inventory at all.target.h/target.mkedits on two owned bench boards, SKYSTARSF405AIO/F4-STDPERIPH and TMOTORF7/F7-HAL, neither a real-aircraft target).USE_SDCARDwas temporarily enabled sharing an existing SPI bus with a spare, verified-free CS pin, with the SD card's DMA stream deliberately pointed at the same stream the board's own ADC already owns (DMA2_Stream3/DMA2_Stream4respectively) — a real, physical ownership conflict, not simulated. On both boards, withsdcard_dmaconfirmedON(get sdcard_dma),dmaCLI output showed the ADC's stream unchanged after reboot:sdcardDmaClaim()'s ownership check correctly rejected the conflicting claim and fell back to polled SPI, no crash, no stolen stream. This does not test a real card's read/write path (none was present, none is needed to exercise the claim/conflict logic) and does not change the risk framing below for a normally-configured target — it confirms the specific new mechanism this PR adds actually works on hardware, across both MCU families.Related
Summary by CodeRabbit