2017-12-01 06:42 PM
Hi,
I just took two days to trace a bug that only appeared when I was doing concurrent reads & writes on my SD card from separate threads in FREERTOS.
The bug is in sd_diskio.c and needs fixing intwo functions. The issue was that one thread doing a read could get interrupted during SD_status after it had set Stat = STA_NOINIT.
This would then cause the other thread callinga FATFS function that checks for Stat == STA_NOINIT to fail erroneously.
This is how to fix the first bug:
/**
* @brief Initializes a Drive
* @param lun : not used
* @retval DSTATUS: Operation status
*/
DSTATUS SD_initialize(BYTE lun) {
// Stat = STA_NOINIT; // FIX: DON'T modify this as not reentrant!
/* Configure the uSD device */
if (BSP_SD_Init() == MSD_OK) {
Stat &= ~STA_NOINIT;
} else {
Stat = STA_NOINIT; // FIX: instead change here in case of failure
}
return Stat;
}
/**
* @brief Gets Disk Status
* @param lun : not used
* @retval DSTATUS: Operation status
*/
DSTATUS SD_status(BYTE lun) {
// Stat = STA_NOINIT; // FIX: DON'T modify this as not reentrant!
if (BSP_SD_GetStatus() == MSD_OK) {
Stat &= ~STA_NOINIT;
} else {
Stat = STA_NOINIT; // FIX: instead change here in case of failure
}
return Stat;
}�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?
I omitted the mutex locks in hereto highlight theSTA_NOINIT issue, which is separate from the mutex locks.
Part 2:
It seems like the whole set of ST's sdio drivers is not reentrant!
In order to make it work, one needs to enclose all non-ISR functions in sd_diskio.c, bsp_driver_sd.c and stm32f2xx_hal_sd.c with a Mutex lock to a single mutex.
An example code snippet here:
/*
* hal_sd_driver_mutex.h
*
* Created on: 2/12/2017
* Author: Valentin
*/
#ifndef HAL_MODS_INC_HAL_SD_DRIVER_MUTEX_H_
#define HAL_MODS_INC_HAL_SD_DRIVER_MUTEX_H_
#include 'FreeRTOS.h'
#include 'semphr.h'
#ifdef __cplusplus
extern 'C' {
#endif
#define DEBUG_BlockSD_Mutex 1
#if DEBUG_BlockSD_Mutex == 1
extern SemaphoreHandle_t mutex;
extern volatile uint8_t taken;
void takeMutex();
void giveMutex();
#endif
#ifdef __cplusplus
}
#endif
#endif /* HAL_MODS_INC_HAL_SD_DRIVER_MUTEX_H_ */
�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?
in hal_sd_driver_mutex.c:
#if DEBUG_BlockSD_Mutex == 1
#include 'hal_sd_driver_mutex.h'
SemaphoreHandle_t mutex;
void takeMutex() {
if (!mutex) {
mutex = xSemaphoreCreateRecursiveMutex();
assert_param(mutex);
}
uint8_t res = (uint8_t) xSemaphoreTakeRecursive(mutex, 480);
assert_param(res);
}
void giveMutex() {
uint8_t res = (uint8_t) xSemaphoreGiveRecursive(mutex);
assert_param(res);
}
#endif�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?
(changed
stm32f2xx_hal_sd.c tostm32f2xx_hal_sd.cpp to use RAII mutex lock/unlock):
#if DEBUG_BlockSD_Mutex == 1
#include 'hal_sd_driver_mutex.h'
class Mutexer {
Mutexer() {
takeMutex();
}
~Mutexer() {
giveMutex();
}
};
#endif
/**
* @brief Returns the SD current state.
* @param hsd: SD handle
* @retval SD card current state
*/
static HAL_SD_CardStateTypedef SD_GetState(SD_HandleTypeDef *hsd) {
#if DEBUG_BlockSD_Mutex == 1
const Mutexer mtx();
#endif
uint32_t resp1 = 0U;
if (SD_SendStatus(hsd, &resp1) != SD_OK) {
return SD_CARD_ERROR;
} else {
return (HAL_SD_CardStateTypedef) ((resp1 >> 9U) & 0x0FU);
}
}�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?
-> do this for all non-ISR functions in this file!
and in
sd_diskio
.c[...]
#if DEBUG_BlockSD_Mutex == 1
#include 'hal_sd_driver_mutex.h'
#endif
[...]
/**
* @brief Reads Sector(s)
* @param lun : not used
* @param *buff: Data buffer to store read data
* @param sector: Sector address (LBA)
* @param count: Number of sectors to read (1..128)
* @retval DRESULT: Operation result
*/
DRESULT SD_read(BYTE lun, BYTE *buff, DWORD sector, UINT count) {
DRESULT res = RES_OK;
#if DEBUG_BlockSD_Mutex == 1
takeMutex();
#endif
if (BSP_SD_ReadBlocks_DMA((uint32_t*) buff, (uint64_t) (sector * BLOCK_SIZE),
BLOCK_SIZE, count) != MSD_OK) {
res = RES_ERROR;
}
#if DEBUG_BlockSD_Mutex == 1
giveMutex();
#endif
return res;
}�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?�?
Before these fixes I was getting crashes on concurrent SD read/writes, either because of Stat ==STA_NOINIT or of lacking sync in the low level driver calls to the sd card.
Opinions, feedback welcome.
@ST please fixSTA_NOINIT part and maybe add a note somewhere about sd driver mutex requirements for FreeRTOS or even better: implement a lock in case FreeRTOS is active.
I was actually under the impression that the SD drivers are completely thread-safe, given that they come via Cube with FreeRTOS enabled, FATFS having a FS_REENTRANT switch and no notes about SD reentrancy being an issue anywhere.
Anyway, here's the solution.
Thanks!
Edit:
Actually now when I look at it again, the
Stat ==STA_NOINIT part is also just part of the synchronisation issue and would be fixed by adding the lock. Still doesn't hurt to do it. One operation less.
#reentrancy #freertos+fat #driver #sdio #bug #hal