Skip to main content
G
Glen Cook_2
Visitor II
May 26, 2019
Solved

HAL BUG : sd_diskio_dma_template in STM32Cube_FW_F4__V1.24.1?

  • May 26, 2019
  • 4 replies
  • 2516 views

I've found a bug in the ENABLE_SCRATCH_BUFFER implementation for sd_disk_io when using fatfs.

When this option is enabled, the code initially checks to see if the buffer is 4 byte aligned, and if it isn't aligned (quite often the case with fatfs) then the code is supposed to copy the buffer to align it to a 4 byte boundary.

The scratch buffer does not appear to be initially set up anywhere, and the indentation for DMAing the scratch buffer is also incorrect.

The complete SD_write function is a mess and needs to be reworked. At least 2 bugs that I can see.

1) Indentation for using the scratch buffer is wrong, it's current executed when BSP_SD_WriteBlocks_DMA does not return MSD_OK

2) Initial scratch buffer is not setup when buffer is not aligned

Can anyone confirm this?

Cheers

Glen.

    This topic has been closed for replies.
    Best answer by Glen Cook_2

    Hi Piers,

    The link and code I posted was the fixed code, not the bug ridden code that is in the cube repository.

    Below is talking about the SD_write (which is more buggy than the SD_read).

    The bracing level was incorrect, therefore the scratch buffer was never actually called correctly for an incorrect aligned buffer.

    Once that was corrected (so it will use the slow code if the buffer wasn't 4 byte aligned), the scratch buffer wasn't actually set up, and they had the memcpy after the dma transfer.

    Once the memcpy was moved to the correct location, the source and destination pointers were the wrong way around.

    This is a classic case of copy and paste the code from sd_read to sd_write and not actually making the correct changes to the copied code.

    Cheers

    Glen.

    4 replies

    G
    Glen Cook_2Author
    Visitor II
    May 26, 2019

    I've tested the fix and I don't get any more crashes...code below.

    This seems to also affect other libraries that use SDIO such as the F7

    Anyway, fixed code.

    DRESULT SD_read(BYTE lun, BYTE *buff, DWORD sector, UINT count)
{
 DRESULT res = RES_ERROR;
 uint32_t timeout;
 uint8_t ret;
#if (ENABLE_SD_DMA_CACHE_MAINTENANCE == 1)
 uint32_t alignedAddr;
#endif
 
 /*
 * ensure the SDCard is ready for a new operation
 */
 
 if (SD_CheckStatusWithTimeout(SD_TIMEOUT) < 0)
 {
 return res;
 }
 
#if defined(ENABLE_SCRATCH_BUFFER)
 if (!((uint32_t)buff & 0x3))
 {
#endif
 if (BSP_SD_ReadBlocks_DMA((uint32_t *)buff,
 (uint32_t)(sector),
 count) == MSD_OK)
 {
 ReadStatus = 0;
 /* Wait that the reading process is completed or a timeout occurs */
 timeout = HAL_GetTick();
 while ((ReadStatus == 0) && ((HAL_GetTick() - timeout) < SD_TIMEOUT))
 {
 }
 /* incase of a timeout return error */
 if (ReadStatus == 0)
 {
 res = RES_ERROR;
 }
 else
 {
 ReadStatus = 0;
 timeout = HAL_GetTick();
 
 while ((HAL_GetTick() - timeout) < SD_TIMEOUT)
 {
 if (BSP_SD_GetCardState() == SD_TRANSFER_OK)
 {
 res = RES_OK;
#if (ENABLE_SD_DMA_CACHE_MAINTENANCE == 1)
 /*
 the SCB_InvalidateDCache_by_Addr() requires a 32-Byte aligned address,
 adjust the address and the D-Cache size to invalidate accordingly.
 */
 alignedAddr = (uint32_t)buff & ~0x1F;
 SCB_InvalidateDCache_by_Addr((uint32_t *)alignedAddr, count * BLOCKSIZE + ((uint32_t)buff - alignedAddr));
#endif
 break;
 }
 }
 }
 }
 }
#if defined(ENABLE_SCRATCH_BUFFER)
 else
 {
 /* Slow path, fetch each sector a part and memcpy to destination buffer */
 int i;
 
 for (i = 0; i < count; i++)
 {
 ret = BSP_SD_ReadBlocks_DMA((uint32_t *)scratch, (uint32_t)sector++, 1);
 if (ret == MSD_OK)
 {
 /* wait until the read is successful or a timeout occurs */
 
 ReadStatus = 0;
 timeout = HAL_GetTick();
 while ((ReadStatus == 0) && ((HAL_GetTick() - timeout) < SD_TIMEOUT))
 {
 }
 if (ReadStatus == 0)
 {
 break;
 }
 
#if (ENABLE_SD_DMA_CACHE_MAINTENANCE == 1)
 /*
 *
 * invalidate the scratch buffer before the next read to get the actual data instead of the cached one
 */
 SCB_InvalidateDCache_by_Addr((uint32_t *)scratch, BLOCKSIZE);
#endif
 memcpy(buff, scratch, BLOCKSIZE);
 buff += BLOCKSIZE;
 }
 else
 {
 break;
 }
 }
 
 if ((i == count) && (ret == MSD_OK))
 res = RES_OK;
#endif
 }
 
 return res;
}
 
/* USER CODE BEGIN beforeWriteSection */
/* can be used to modify previous code / undefine following code / add new code */
/* USER CODE END beforeWriteSection */
/**
 * @brief Writes Sector(s)
 * @param lun : not used
 * @param *buff: Data to be written
 * @param sector: Sector address (LBA)
 * @param count: Number of sectors to write (1..128)
 * @retval DRESULT: Operation result
 */
#if _USE_WRITE == 1
 
DRESULT SD_write(BYTE lun, const BYTE *buff, DWORD sector, UINT count)
{
 DRESULT res = RES_ERROR;
 uint32_t timeout;
 uint8_t ret;
 int i;
 
 WriteStatus = 0;
#if (ENABLE_SD_DMA_CACHE_MAINTENANCE == 1)
 uint32_t alignedAddr;
#endif
 
 if (SD_CheckStatusWithTimeout(SD_TIMEOUT) < 0)
 {
 return res;
 }
 
#if defined(ENABLE_SCRATCH_BUFFER)
 if (!((uint32_t)buff & 0x3))
 {
#endif
#if (ENABLE_SD_DMA_CACHE_MAINTENANCE == 1)
 
 /*
 the SCB_CleanDCache_by_Addr() requires a 32-Byte aligned address
 adjust the address and the D-Cache size to clean accordingly.
 */
 alignedAddr = (uint32_t)buff & ~0x1F;
 SCB_CleanDCache_by_Addr((uint32_t *)alignedAddr, count * BLOCKSIZE + ((uint32_t)buff - alignedAddr));
#endif
 
 if (BSP_SD_WriteBlocks_DMA((uint32_t *)buff,
 (uint32_t)(sector),
 count) == MSD_OK)
 {
 /* Wait that writing process is completed or a timeout occurs */
 
 timeout = HAL_GetTick();
 while ((WriteStatus == 0) && ((HAL_GetTick() - timeout) < SD_TIMEOUT))
 {
 }
 /* incase of a timeout return error */
 if (WriteStatus == 0)
 {
 res = RES_ERROR;
 }
 else
 {
 WriteStatus = 0;
 timeout = HAL_GetTick();
 
 while ((HAL_GetTick() - timeout) < SD_TIMEOUT)
 {
 if (BSP_SD_GetCardState() == SD_TRANSFER_OK)
 {
 res = RES_OK;
 break;
 }
 }
 }
 }
 }
 else
 {
 /* Slow path, fetch each sector a part and memcpy to destination buffer */
#if (ENABLE_SD_DMA_CACHE_MAINTENANCE == 1)
 /*
 * invalidate the scratch buffer before the next write to get the actual data instead of the cached one
 */
 SCB_InvalidateDCache_by_Addr((uint32_t *)scratch, BLOCKSIZE);
#endif
 
 for (i = 0; i < count; i++)
 {
 WriteStatus = 0;
 
 memcpy((void *)scratch, (void *)buff, BLOCKSIZE);
 buff += BLOCKSIZE;
 
 ret = BSP_SD_WriteBlocks_DMA((uint32_t *)scratch, (uint32_t)sector++, 1);
 if (ret == MSD_OK)
 {
 /* wait for a message from the queue or a timeout */
 timeout = HAL_GetTick();
 while ((WriteStatus == 0) && ((HAL_GetTick() - timeout) < SD_TIMEOUT))
 {
 }
 if (WriteStatus == 0)
 {
 break;
 }
 }
 else
 {
 break;
 }
 }
 if ((i == count) && (ret == MSD_OK))
 res = RES_OK;
 }
 
 return res;
}

    Any comments or things I've missed please let me know.

    Cheers

    Glen.

    P
    Piers Hawksley
    Visitor II
    May 29, 2019

    I think you need to undef ENABLE_SCRATCH_BUFFER and check compilation. From a quick look at the code in a browser (hardly an ideal environment, so I could be missing something / wrong) you need to:

    Swap lines 103 & 104

    Add #if defined(ENABLE_SCRATCH_BUFFER) between lines 183 & 184

    Add #endif between lines 221 and 222

    For the F7, it looks like ENABLE_SCRATCH_BUFFER was introduced in STM32Cube_FW_F7_V1.15.0 ... it's not in STM32Cube_FW_F7_V1.14.0 ... so this is a recently introduced bug

    Are you able to highlight what code changes you made?

    Here the lack of any (sensible) means for the community to contribute adds considerably to the difficulty in providing potential fixes ...

    G
    Glen Cook_2AuthorAnswer
    Visitor II
    May 29, 2019

    Hi Piers,

    The link and code I posted was the fixed code, not the bug ridden code that is in the cube repository.

    Below is talking about the SD_write (which is more buggy than the SD_read).

    The bracing level was incorrect, therefore the scratch buffer was never actually called correctly for an incorrect aligned buffer.

    Once that was corrected (so it will use the slow code if the buffer wasn't 4 byte aligned), the scratch buffer wasn't actually set up, and they had the memcpy after the dma transfer.

    Once the memcpy was moved to the correct location, the source and destination pointers were the wrong way around.

    This is a classic case of copy and paste the code from sd_read to sd_write and not actually making the correct changes to the copied code.

    Cheers

    Glen.

    T
    TSche.17
    Visitor II
    October 1, 2019

    Hi Glen

    I can confirm these are huge, inacceptable bugs. Some other potential issues I see with the late clearing of 'ReadStatus' (after the BSP-call), and the call of BSP_SD_GetCardState only be done in case of aligned (non-loop) case.

    G
    Glen Cook_2Author
    Visitor II
    October 1, 2019

    Hi Thomas,

    I agree, these bugs should not have got through quality control / peer review. Maybe we are the peer reviewers :)

    I did post the issues to the github page for the source code, and they have fixed them (although not released yet, they did upload an attachment to the fixed ticket). If you spot any other problems, I would create an issue to make them aware of it.

    https://github.com/STMicroelectronics/STM32CubeF4

    Cheers

    Glen.

    A
    Amel NASRI
    Technical Moderator
    November 28, 2019

    Hello,

    Please refer to Github (https://github.com/STMicroelectronics/STM32CubeF4/issues/2) in order to get more details about the progress on fixing this issue.

    -Amel

    R
    ranciere
    Visitor II
    August 26, 2021

    Dear @Amel NASRI​ ,

    How can I achieve to fix this bug in the H7 framework also?

    R
    ranciere
    Visitor II
    August 26, 2021

    I use a Nucleo H743 board with STM32Cube_FW_H7_V1.9.0 FW, and I have the same bug. How it is possible?

    Terms & ConditionsAccessibility statement