Skip to content

Commit 332779b

Browse files
authored
fix(sdcard): migrate dmaInit() to ownership-checked dmaAllocate() (#1398)
* fix(sdcard): migrate dmaInit() to ownership-checked dmaAllocate() 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. * test(sdcard): add host unit-test coverage for DMA-claim ownership check 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.
1 parent a539314 commit 332779b

9 files changed

Lines changed: 222 additions & 14 deletions

File tree

src/main/drivers/sdcard.c

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -473,6 +473,15 @@ static bool sdcard_checkInitDone(void) {
473473
return status == 0x00;
474474
}
475475

476+
// sdcard_init() can be called from both boot init and the USB MSC passthrough path -- a
477+
// stream already held by this same owner must be re-claimable.
478+
static bool sdcardDmaClaim(dmaIdentifier_e identifier, resourceOwner_e owner, uint8_t resourceIndex) {
479+
if (dmaGetOwner(identifier) == owner && dmaGetResourceIndex(identifier) == resourceIndex) {
480+
return true;
481+
}
482+
return dmaAllocate(identifier, owner, resourceIndex);
483+
}
484+
476485
/**
477486
* Begin the initialization process for the SD card. This must be called first before any other sdcard_ routine.
478487
*/
@@ -489,11 +498,17 @@ void sdcard_init(const sdcardConfig_t *config) {
489498
sdcard.useDMAForTx = config->useDma;
490499
#endif
491500
if (sdcard.useDMAForTx) {
501+
if (sdcardDmaClaim(config->dmaIdentifier, OWNER_SDCARD, 0)) {
492502
#if defined(STM32F4) || defined(STM32F7)
493-
sdcard.dmaChannel = config->dmaChannel;
503+
sdcard.dmaChannel = config->dmaChannel;
494504
#endif
495-
sdcard.dma = dmaGetDescriptorByIdentifier(config->dmaIdentifier);
496-
dmaInit(config->dmaIdentifier, OWNER_SDCARD, 0);
505+
sdcard.dma = dmaGetDescriptorByIdentifier(config->dmaIdentifier);
506+
dmaEnable(config->dmaIdentifier);
507+
} else {
508+
// Stream already owned by another peripheral -- fall back to polled SPI
509+
// rather than losing the card (and blackbox logging) entirely.
510+
sdcard.useDMAForTx = false;
511+
}
497512
}
498513
if (config->chipSelectTag) {
499514
sdcard.chipSelectPin = IOGetByTag(config->chipSelectTag);

src/main/drivers/sdcard_sdio_baremetal.c

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -286,10 +286,14 @@ void sdcard_init(const sdcardConfig_t *config) {
286286
sdcard.useCache = 0;
287287
}
288288
#if defined(STM32H7)
289-
SD_Initialize_LL(NULL);
289+
if (!SD_Initialize_LL(NULL)) {
290290
#else
291-
SD_Initialize_LL(dmaGetRefByIdentifier(sdcard.dma));
291+
if (!SD_Initialize_LL(dmaGetRefByIdentifier(sdcard.dma))) {
292292
#endif
293+
sdcard.state = SDCARD_STATE_NOT_PRESENT;
294+
sdcard.failureCount++;
295+
return;
296+
}
293297
if (SD_IsDetected()) {
294298
if (SD_Init() != 0) {
295299
sdcard.state = SDCARD_STATE_NOT_PRESENT;

src/main/drivers/sdio_f4xx.c

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1305,11 +1305,24 @@ static SD_Error_t SD_IsCardProgramming(uint8_t *pStatus)
13051305
}
13061306
*/
13071307

1308+
// SD_Initialize_LL() can be called from both the sdcard driver's own init and the USB MSC
1309+
// passthrough path -- a stream already held by this same owner must be re-claimable.
1310+
static bool sdioDmaClaim(dmaIdentifier_e identifier, resourceOwner_e owner, uint8_t resourceIndex) {
1311+
if (dmaGetOwner(identifier) == owner && dmaGetResourceIndex(identifier) == resourceIndex) {
1312+
return true;
1313+
}
1314+
return dmaAllocate(identifier, owner, resourceIndex);
1315+
}
1316+
13081317
/** -----------------------------------------------------------------------------------------------------------------*/
13091318
/**
13101319
* @brief Initialize the SDIO module, DMA, and IO
13111320
*/
1312-
void SD_Initialize_LL(DMA_Stream_TypeDef *dma) {
1321+
bool SD_Initialize_LL(DMA_Stream_TypeDef *dma) {
1322+
if (!(dma == DMA2_Stream3 || dma == DMA2_Stream6) || !sdioDmaClaim(dmaGetIdentifier(dma), OWNER_SDCARD, 0)) {
1323+
return false;
1324+
}
1325+
dmaEnable(dmaGetIdentifier(dma));
13131326
// Reset SDIO Module
13141327
RCC->APB2RSTR |= RCC_APB2RSTR_SDIORST;
13151328
delay(1);
@@ -1362,7 +1375,6 @@ void SD_Initialize_LL(DMA_Stream_TypeDef *dma) {
13621375
DMA_MBURST_INC4 | DMA_PBURST_INC4 |
13631376
DMA_MEMORY_TO_PERIPH);
13641377
DMA2_Stream3->FCR = (DMA_SxFCR_DMDIS | DMA_SxFCR_FTH); // Configuration FIFO control register
1365-
dmaInit(dmaGetIdentifier(DMA2_Stream3), OWNER_SDCARD, 0);
13661378
dmaSetHandler(dmaGetIdentifier(DMA2_Stream3), SDIO_DMA_ST3_IRQHandler, 1, 0);
13671379
} else {
13681380
// Initialize DMA2 channel 6
@@ -1375,9 +1387,9 @@ void SD_Initialize_LL(DMA_Stream_TypeDef *dma) {
13751387
DMA_MBURST_INC4 | DMA_PBURST_INC4 |
13761388
DMA_MEMORY_TO_PERIPH);
13771389
DMA2_Stream6->FCR = (DMA_SxFCR_DMDIS | DMA_SxFCR_FTH); // Configuration FIFO control register
1378-
dmaInit(dmaGetIdentifier(DMA2_Stream6), OWNER_SDCARD, 0);
13791390
dmaSetHandler(dmaGetIdentifier(DMA2_Stream6), SDIO_DMA_ST6_IRQHandler, 1, 0);
13801391
}
1392+
return true;
13811393
}
13821394

13831395

src/main/drivers/sdio_f7xx.c

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1326,11 +1326,24 @@ static SD_Error_t SD_IsCardProgramming(uint8_t *pStatus)
13261326
}
13271327
*/
13281328

1329+
// SD_Initialize_LL() can be called from both the sdcard driver's own init and the USB MSC
1330+
// passthrough path -- a stream already held by this same owner must be re-claimable.
1331+
static bool sdioDmaClaim(dmaIdentifier_e identifier, resourceOwner_e owner, uint8_t resourceIndex) {
1332+
if (dmaGetOwner(identifier) == owner && dmaGetResourceIndex(identifier) == resourceIndex) {
1333+
return true;
1334+
}
1335+
return dmaAllocate(identifier, owner, resourceIndex);
1336+
}
1337+
13291338
/** -----------------------------------------------------------------------------------------------------------------*/
13301339
/**
13311340
* @brief Initialize the SDMMC1 module, DMA, and IO
13321341
*/
1333-
void SD_Initialize_LL(DMA_Stream_TypeDef *dma) {
1342+
bool SD_Initialize_LL(DMA_Stream_TypeDef *dma) {
1343+
if (!(dma == DMA2_Stream3 || dma == DMA2_Stream6) || !sdioDmaClaim(dmaGetIdentifier(dma), OWNER_SDCARD, 0)) {
1344+
return false;
1345+
}
1346+
dmaEnable(dmaGetIdentifier(dma));
13341347
// Reset SDMMC1 Module
13351348
RCC->APB2RSTR |= RCC_APB2RSTR_SDMMC1RST;
13361349
delay(1);
@@ -1386,7 +1399,6 @@ void SD_Initialize_LL(DMA_Stream_TypeDef *dma) {
13861399
DMA_MBURST_INC4 | DMA_PBURST_INC4 |
13871400
DMA_MEMORY_TO_PERIPH);
13881401
DMA2_Stream3->FCR = (DMA_SxFCR_DMDIS | DMA_SxFCR_FTH); // Configuration FIFO control register
1389-
dmaInit(dmaGetIdentifier(DMA2_Stream3), OWNER_SDCARD, 0);
13901402
dmaSetHandler(dmaGetIdentifier(DMA2_Stream3), SDMMC_DMA_ST3_IRQHandler, 1, 0);
13911403
} else {
13921404
// Initialize DMA2 channel 6
@@ -1399,9 +1411,9 @@ void SD_Initialize_LL(DMA_Stream_TypeDef *dma) {
13991411
DMA_MBURST_INC4 | DMA_PBURST_INC4 |
14001412
DMA_MEMORY_TO_PERIPH);
14011413
DMA2_Stream6->FCR = (DMA_SxFCR_DMDIS | DMA_SxFCR_FTH); // Configuration FIFO control register
1402-
dmaInit(dmaGetIdentifier(DMA2_Stream6), OWNER_SDCARD, 0);
14031414
dmaSetHandler(dmaGetIdentifier(DMA2_Stream6), SDMMC_DMA_ST6_IRQHandler, 1, 0);
14041415
}
1416+
return true;
14051417
}
14061418

14071419

src/main/drivers/sdio_h7xx.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -734,9 +734,10 @@ bool SD_IsDetected(void) {
734734
return status;
735735
}
736736

737-
void SD_Initialize_LL(DMA_Stream_TypeDef *dma) {
737+
bool SD_Initialize_LL(DMA_Stream_TypeDef *dma) {
738738
UNUSED(dma);
739739
// H7 uses SDMMC internal DMA (IDMA) — no external DMA stream configuration needed.
740+
return true;
740741
}
741742

742743
void SDMMC1_IRQHandler(void)

src/main/drivers/sdmmc_sdio.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -216,7 +216,7 @@ typedef struct {
216216
extern SD_CardInfo_t SD_CardInfo;
217217
extern SD_CardType_t SD_CardType;
218218

219-
void SD_Initialize_LL (DMA_Stream_TypeDef *dma);
219+
bool SD_Initialize_LL (DMA_Stream_TypeDef *dma);
220220
SD_Error_t SD_Init (void);
221221
bool SD_IsDetected (void);
222222
bool SD_GetState (void);

src/main/msc/usbd_storage_sdio.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ static int8_t STORAGE_Init (uint8_t lun) {
152152
#endif
153153
UNUSED(lun);
154154
LED0_OFF;
155-
SD_Initialize_LL(SDIO_DMA);
155+
if (!SD_Initialize_LL(SDIO_DMA)) return 1;
156156
if (SD_Init() != 0) return 1;
157157
LED0_ON;
158158
return 0;

src/test/Makefile

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -348,6 +348,10 @@ serial_uart_dma_claim_unittest_DEFINES := \
348348
STM32F4
349349

350350

351+
sdcard_dma_claim_unittest_DEFINES := \
352+
STM32F4
353+
354+
351355
flight_failsafe_unittest_SRC := \
352356
$(USER_DIR)/common/bitarray.c \
353357
$(USER_DIR)/fc/rc_modes.c \
Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,160 @@
1+
/*
2+
* This file is part of Cleanflight and Betaflight.
3+
*
4+
* Cleanflight and Betaflight are free software. You can redistribute
5+
* this software and/or modify this software under the terms of the
6+
* GNU General Public License as published by the Free Software
7+
* Foundation, either version 3 of the License, or (at your option)
8+
* any later version.
9+
*
10+
* Cleanflight and Betaflight are distributed in the hope that they
11+
* will be useful, but WITHOUT ANY WARRANTY; without even the implied
12+
* warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
13+
* See the GNU General Public License for more details.
14+
*
15+
* You should have received a copy of the GNU General Public License
16+
* along with this software.
17+
*
18+
* If not, see <http://www.gnu.org/licenses/>.
19+
*/
20+
21+
extern "C" {
22+
23+
#include "platform.h"
24+
#include "drivers/dma.h"
25+
26+
}
27+
28+
#include "unittest_macros.h"
29+
#include "gtest/gtest.h"
30+
31+
// Mirrors sdcardDmaClaim() (drivers/sdcard.c) and sdioDmaClaim() (drivers/sdio_f4xx.c,
32+
// drivers/sdio_f7xx.c -- byte-identical body, confirmed by direct read) rather than compiling
33+
// those files. Real-compile was investigated, not assumed disproportionate: sdcard.c's
34+
// SDCARD_STATE_SENDING_WRITE completion path (STM32F4 branch) needs a real DMA_Stream_TypeDef
35+
// with ->ref/->completeFlag fields, a two-arg DMA_GetFlagStatus()/DMA_ClearFlag() pair distinct
36+
// from the one-arg mock already in test/unit/platform.h, DMA_Cmd(), SPI_I2S_GetFlagStatus(),
37+
// LL_SPI_DisableDMAReq_TX(), and direct ->DR register access -- none of which the file's own
38+
// DMA-claim logic uses, but all of which must still link since the whole .c compiles as one
39+
// translation unit. sdio_f4xx.c/f7xx.c need the same class of surface an order of magnitude
40+
// larger (118 direct SDIO->/RCC-> field accesses, real GPIO AF config, NVIC). Both are the
41+
// same disproportionate-mocking case bus_spi_ll.c/bus_spi_stdperiph.c already were in
42+
// feat/dma-ll-unittest-infra (20+ mocks needed for functions that weren't even the test
43+
// target). Follows the same mirror-with-fake-DMA-state convention already established for
44+
// uartDmaClaim() in serial_uart_dma_claim_unittest.cc.
45+
//
46+
// sdcard_init() (SPI mode) can be called from both fc_init.c (boot) and
47+
// usbd_storage_sd_spi.c (USB MSC passthrough). SD_Initialize_LL() (SDIO mode) can be called
48+
// from both sdcard_sdio_baremetal.c (boot) and usbd_storage_sdio.c (USB MSC passthrough). In
49+
// both cases the second call is a legitimate reopen of the same OWNER_SDCARD claim, not a
50+
// conflict, and must not be misidentified as one now that the caller-side return check exists.
51+
namespace {
52+
53+
resourceOwner_e fakeOwner = OWNER_FREE;
54+
uint8_t fakeResourceIndex = 0;
55+
56+
void resetFakeDmaState() {
57+
fakeOwner = OWNER_FREE;
58+
fakeResourceIndex = 0;
59+
}
60+
61+
resourceOwner_e fakeDmaGetOwner(dmaIdentifier_e) {
62+
return fakeOwner;
63+
}
64+
65+
uint8_t fakeDmaGetResourceIndex(dmaIdentifier_e) {
66+
return fakeResourceIndex;
67+
}
68+
69+
bool fakeDmaAllocate(dmaIdentifier_e, resourceOwner_e owner, uint8_t resourceIndex) {
70+
if (fakeOwner != OWNER_FREE) {
71+
return false;
72+
}
73+
fakeOwner = owner;
74+
fakeResourceIndex = resourceIndex;
75+
return true;
76+
}
77+
78+
bool sdcardDmaClaim(dmaIdentifier_e identifier, resourceOwner_e owner, uint8_t resourceIndex) {
79+
if (fakeDmaGetOwner(identifier) == owner && fakeDmaGetResourceIndex(identifier) == resourceIndex) {
80+
return true;
81+
}
82+
return fakeDmaAllocate(identifier, owner, resourceIndex);
83+
}
84+
85+
bool sdioDmaClaim(dmaIdentifier_e identifier, resourceOwner_e owner, uint8_t resourceIndex) {
86+
if (fakeDmaGetOwner(identifier) == owner && fakeDmaGetResourceIndex(identifier) == resourceIndex) {
87+
return true;
88+
}
89+
return fakeDmaAllocate(identifier, owner, resourceIndex);
90+
}
91+
92+
const dmaIdentifier_e kStream = DMA1_ST3_HANDLER;
93+
94+
} // namespace
95+
96+
// sdcardDmaClaim() -- SPI-mode (drivers/sdcard.c), real callers always pass OWNER_SDCARD / index 0.
97+
98+
TEST(SdcardDmaClaimUnittest, FirstClaimOnFreeStreamSucceeds) {
99+
resetFakeDmaState();
100+
EXPECT_TRUE(sdcardDmaClaim(kStream, OWNER_SDCARD, 0));
101+
EXPECT_EQ(fakeOwner, OWNER_SDCARD);
102+
}
103+
104+
TEST(SdcardDmaClaimUnittest, UsbMscReopenBySameOwnerAndIndexSucceeds) {
105+
resetFakeDmaState();
106+
fakeOwner = OWNER_SDCARD;
107+
fakeResourceIndex = 0;
108+
// sdcard_init() called again from usbd_storage_sd_spi.c after fc_init.c already claimed it.
109+
EXPECT_TRUE(sdcardDmaClaim(kStream, OWNER_SDCARD, 0));
110+
}
111+
112+
TEST(SdcardDmaClaimUnittest, ConflictWithForeignOwnerFails) {
113+
resetFakeDmaState();
114+
fakeOwner = OWNER_SPI_SDI;
115+
fakeResourceIndex = 0;
116+
EXPECT_FALSE(sdcardDmaClaim(kStream, OWNER_SDCARD, 0));
117+
// failed claim must not disturb the existing owner's bookkeeping.
118+
EXPECT_EQ(fakeOwner, OWNER_SPI_SDI);
119+
}
120+
121+
TEST(SdcardDmaClaimUnittest, SameOwnerDifferentResourceIndexFails) {
122+
resetFakeDmaState();
123+
fakeOwner = OWNER_SDCARD;
124+
fakeResourceIndex = 1;
125+
// spec-level guard: same owner enum alone is not sufficient, index must match too.
126+
EXPECT_FALSE(sdcardDmaClaim(kStream, OWNER_SDCARD, 0));
127+
}
128+
129+
// sdioDmaClaim() -- SDIO-mode (drivers/sdio_f4xx.c, drivers/sdio_f7xx.c), same real callers
130+
// always pass OWNER_SDCARD / index 0.
131+
132+
TEST(SdioDmaClaimUnittest, FirstClaimOnFreeStreamSucceeds) {
133+
resetFakeDmaState();
134+
EXPECT_TRUE(sdioDmaClaim(kStream, OWNER_SDCARD, 0));
135+
EXPECT_EQ(fakeOwner, OWNER_SDCARD);
136+
}
137+
138+
TEST(SdioDmaClaimUnittest, UsbMscReopenBySameOwnerAndIndexSucceeds) {
139+
resetFakeDmaState();
140+
fakeOwner = OWNER_SDCARD;
141+
fakeResourceIndex = 0;
142+
// SD_Initialize_LL() called again from usbd_storage_sdio.c after sdcard_sdio_baremetal.c
143+
// already claimed it during boot.
144+
EXPECT_TRUE(sdioDmaClaim(kStream, OWNER_SDCARD, 0));
145+
}
146+
147+
TEST(SdioDmaClaimUnittest, ConflictWithForeignOwnerFails) {
148+
resetFakeDmaState();
149+
fakeOwner = OWNER_SPI_SDI;
150+
fakeResourceIndex = 0;
151+
EXPECT_FALSE(sdioDmaClaim(kStream, OWNER_SDCARD, 0));
152+
EXPECT_EQ(fakeOwner, OWNER_SPI_SDI);
153+
}
154+
155+
TEST(SdioDmaClaimUnittest, SameOwnerDifferentResourceIndexFails) {
156+
resetFakeDmaState();
157+
fakeOwner = OWNER_SDCARD;
158+
fakeResourceIndex = 1;
159+
EXPECT_FALSE(sdioDmaClaim(kStream, OWNER_SDCARD, 0));
160+
}

0 commit comments

Comments
 (0)