AnsweredAssumed Answered

[BUG] SDIO + RTOS (Stat = STA_NOINIT)

Question asked by Valentin on Dec 2, 2017

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 in two 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 calling a 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 here to highlight the STA_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 to stm32f2xx_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 fix STA_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.

Outcomes