sdmmc: fix reads/writes to/from unaligned buffers

SDMMC hardware treats all buffers as aligned, and ignores 2 LSBs of
addresses written into DMA descriptors. Previously SDMMC host driver
assumed that data buffers passed from SDDMC command layer would be
aligned. However alignment checks were never implemented in the command
layer, as were the checks that the buffer coming from the application
would be in DMA capable memory. Most of the time this was indeed true.
However in some cases FATFS library can pass buffers offset by 2 bytes
from word boundary. “DMA capable” restriction may be broken if pSRAM
support is used.

This change adds buffer checks to the SDMMC host driver (alignment and
DMA capability), so that the host layer will error out for incompatible
buffers. In SDMMC command layer, a check is added to read and write
functions. If an incompatible buffer is passed from the application, new
buffer (512 bytes size) is allocated, and the transfer is performed
using {READ,WRITE}_SINGLE_BLOCK commands.

components/driver/include/driver/sdmmc_host.h

@@ -142,6 +142,8 @@ esp_err_t sdmmc_host_set_card_clk(int slot, uint32_t freq_khz);
  *       can call sdmmc_host_do_transaction as long as other sdmmc_host_*
  *       functions are not called.
  *
+ * @attention Data buffer passed in cmdinfo->data must be in DMA capable memory
+ *
  * @param slot  slot number (SDMMC_HOST_SLOT_0 or SDMMC_HOST_SLOT_1)
  * @param cmdinfo   pointer to structure describing command and data to transfer
  * @return
@@ -149,6 +151,8 @@ esp_err_t sdmmc_host_set_card_clk(int slot, uint32_t freq_khz);
  *      - ESP_ERR_TIMEOUT if response or data transfer has timed out
  *      - ESP_ERR_INVALID_CRC if response or data transfer CRC check has failed
  *      - ESP_ERR_INVALID_RESPONSE if the card has sent an invalid response
+ *      - ESP_ERR_INVALID_SIZE if the size of data transfer is not valid in SD protocol
+ *      - ESP_ERR_INVALID_ARG if the data buffer is not in DMA capable memory
  */
 esp_err_t sdmmc_host_do_transaction(int slot, sdmmc_command_t* cmdinfo);
 

components/driver/sdmmc_transaction.c

@@ -20,6 +20,7 @@
 #include "freertos/semphr.h"
 #include "soc/sdmmc_reg.h"
 #include "soc/sdmmc_struct.h"
+#include "soc/soc_memory_layout.h"
 #include "driver/sdmmc_types.h"
 #include "driver/sdmmc_defs.h"
 #include "driver/sdmmc_host.h"
@@ -105,9 +106,16 @@ esp_err_t sdmmc_host_do_transaction(int slot, sdmmc_command_t* cmdinfo)
     // convert cmdinfo to hardware register value
     sdmmc_hw_cmd_t hw_cmd = make_hw_cmd(cmdinfo);
     if (cmdinfo->data) {
-        // these constraints should be handled by upper layer
+        if (cmdinfo->datalen < 4 || cmdinfo->blklen % 4 != 0) {
-        assert(cmdinfo->datalen >= 4);
+            ESP_LOGD(TAG, "%s: invalid size: total=%d block=%d",
-        assert(cmdinfo->blklen % 4 == 0);
+                    __func__, cmdinfo->datalen, cmdinfo->blklen);
+            return ESP_ERR_INVALID_SIZE;
+        }
+        if ((intptr_t) cmdinfo->data % 4 != 0 ||
+                !esp_ptr_dma_capable(cmdinfo->data)) {
+            ESP_LOGD(TAG, "%s: buffer %p can not be used for DMA", __func__, cmdinfo->data);
+            return ESP_ERR_INVALID_ARG;
+        }
         // this clears "owned by IDMAC" bits
         memset(s_dma_desc, 0, sizeof(s_dma_desc));
         // initialize first descriptor

components/sdmmc/sdmmc_cmd.c

@@ -24,6 +24,7 @@
 #include "driver/sdmmc_types.h"
 #include "sdmmc_cmd.h"
 #include "sys/param.h"
+#include "soc/soc_memory_layout.h"
 
 #define SDMMC_GO_IDLE_DELAY_MS      20
 
@@ -51,6 +52,10 @@ static esp_err_t sdmmc_send_cmd_send_status(sdmmc_card_t* card, uint32_t* out_st
 static esp_err_t sdmmc_send_cmd_crc_on_off(sdmmc_card_t* card, bool crc_enable);
 static uint32_t  get_host_ocr(float voltage);
 static void response_ntoh(sdmmc_response_t response);
+static esp_err_t sdmmc_write_sectors_dma(sdmmc_card_t* card, const void* src,
+        size_t start_block, size_t block_count);
+static esp_err_t sdmmc_read_sectors_dma(sdmmc_card_t* card, void* dst,
+        size_t start_block, size_t block_count);
 
 
 static bool host_is_spi(const sdmmc_card_t* card)
@@ -618,6 +623,37 @@ static esp_err_t sdmmc_send_cmd_send_status(sdmmc_card_t* card, uint32_t* out_st
 
 esp_err_t sdmmc_write_sectors(sdmmc_card_t* card, const void* src,
         size_t start_block, size_t block_count)
+{
+    esp_err_t err = ESP_OK;
+    size_t block_size = card->csd.sector_size;
+    if (esp_ptr_dma_capable(src) && (intptr_t)src % 4 == 0) {
+        err = sdmmc_write_sectors_dma(card, src, start_block, block_count);
+    } else {
+        // SDMMC peripheral needs DMA-capable buffers. Split the write into
+        // separate single block writes, if needed, and allocate a temporary
+        // DMA-capable buffer.
+        void* tmp_buf = heap_caps_malloc(block_size, MALLOC_CAP_DMA);
+        if (tmp_buf == NULL) {
+            return ESP_ERR_NO_MEM;
+        }
+        const uint8_t* cur_src = (const uint8_t*) src;
+        for (size_t i = 0; i < block_count; ++i) {
+            memcpy(tmp_buf, cur_src, block_size);
+            cur_src += block_size;
+            err = sdmmc_write_sectors_dma(card, tmp_buf, start_block + i, 1);
+            if (err != ESP_OK) {
+                ESP_LOGD(TAG, "%s: error 0x%x writing block %d+%d",
+                        __func__, err, start_block, i);
+                break;
+            }
+        }
+        free(tmp_buf);
+    }
+    return err;
+}
+
+static esp_err_t sdmmc_write_sectors_dma(sdmmc_card_t* card, const void* src,
+        size_t start_block, size_t block_count)
 {
     if (start_block + block_count > card->csd.capacity) {
         return ESP_ERR_INVALID_SIZE;
@@ -661,6 +697,37 @@ esp_err_t sdmmc_write_sectors(sdmmc_card_t* card, const void* src,
 
 esp_err_t sdmmc_read_sectors(sdmmc_card_t* card, void* dst,
         size_t start_block, size_t block_count)
+{
+    esp_err_t err = ESP_OK;
+    size_t block_size = card->csd.sector_size;
+    if (esp_ptr_dma_capable(dst) && (intptr_t)dst % 4 == 0) {
+        err = sdmmc_read_sectors_dma(card, dst, start_block, block_count);
+    } else {
+        // SDMMC peripheral needs DMA-capable buffers. Split the read into
+        // separate single block reads, if needed, and allocate a temporary
+        // DMA-capable buffer.
+        void* tmp_buf = heap_caps_malloc(block_size, MALLOC_CAP_DMA);
+        if (tmp_buf == NULL) {
+            return ESP_ERR_NO_MEM;
+        }
+        uint8_t* cur_dst = (uint8_t*) dst;
+        for (size_t i = 0; i < block_count; ++i) {
+            err = sdmmc_read_sectors_dma(card, tmp_buf, start_block + i, 1);
+            if (err != ESP_OK) {
+                ESP_LOGD(TAG, "%s: error 0x%x writing block %d+%d",
+                        __func__, err, start_block, i);
+                break;
+            }
+            memcpy(cur_dst, tmp_buf, block_size);
+            cur_dst += block_size;
+        }
+        free(tmp_buf);
+    }
+    return err;
+}
+
+static esp_err_t sdmmc_read_sectors_dma(sdmmc_card_t* card, void* dst,
+        size_t start_block, size_t block_count)
 {
     if (start_block + block_count > card->csd.capacity) {
         return ESP_ERR_INVALID_SIZE;

components/sdmmc/test/test_sd.c

@@ -22,7 +22,7 @@
 #include "driver/sdmmc_defs.h"
 #include "sdmmc_cmd.h"
 #include "esp_log.h"
-#include "esp_heap_alloc_caps.h"
+#include "esp_heap_caps.h"
 #include <time.h>
 #include <sys/time.h>
 
@@ -55,61 +55,85 @@ TEST_CASE("can probe SD (using SPI)", "[sdspi][ignore]")
     free(card);
 }
 
+// Fill buffer pointed to by 'dst' with 'count' 32-bit ints generated
+// from 'rand' with the starting value of 'seed'
+static void fill_buffer(uint32_t seed, uint8_t* dst, size_t count) {
+    srand(seed);
+    for (size_t i = 0; i < count; ++i) {
+        uint32_t val = rand();
+        memcpy(dst + i * sizeof(uint32_t), &val, sizeof(val));
+    }
+}
+
+// Check if the buffer pointed to by 'dst' contains 'count' 32-bit
+// ints generated from 'rand' with the starting value of 'seed'
+static void check_buffer(uint32_t seed, const uint8_t* src, size_t count) {
+    srand(seed);
+    for (size_t i = 0; i < count; ++i) {
+        uint32_t val;
+        memcpy(&val, src + i * sizeof(uint32_t), sizeof(val));
+        TEST_ASSERT_EQUAL_HEX32(rand(), val);
+    }
+}
+
 static void do_single_write_read_test(sdmmc_card_t* card,
-        size_t start_block, size_t block_count)
+        size_t start_block, size_t block_count, size_t alignment)
 {
     size_t block_size = card->csd.sector_size;
     size_t total_size = block_size * block_count;
-    printf(" %8d |  %3d  |   %4.1f  ", start_block, block_count, total_size / 1024.0f);
+    printf(" %8d |  %3d  |   %d   |    %4.1f  ", start_block, block_count, alignment, total_size / 1024.0f);
-    uint32_t* buffer = pvPortMallocCaps(total_size, MALLOC_CAP_DMA);
+
-    srand(start_block);
+    uint32_t* buffer = heap_caps_malloc(total_size + 4, MALLOC_CAP_DMA);
-    for (size_t i = 0; i < total_size / sizeof(buffer[0]); ++i) {
+    size_t offset = alignment % 4;
-        buffer[i] = rand();
+    uint8_t* c_buffer = (uint8_t*) buffer + offset;
-    }
+    fill_buffer(start_block, c_buffer, total_size / sizeof(buffer[0]));
+
     struct timeval t_start_wr;
     gettimeofday(&t_start_wr, NULL);
-    TEST_ESP_OK(sdmmc_write_sectors(card, buffer, start_block, block_count));
+    TEST_ESP_OK(sdmmc_write_sectors(card, c_buffer, start_block, block_count));
     struct timeval t_stop_wr;
     gettimeofday(&t_stop_wr, NULL);
     float time_wr = 1e3f * (t_stop_wr.tv_sec - t_start_wr.tv_sec) + 1e-3f * (t_stop_wr.tv_usec - t_start_wr.tv_usec);
-    memset(buffer, 0xbb, total_size);
+
+    memset(buffer, 0xbb, total_size + 4);
+
     struct timeval t_start_rd;
     gettimeofday(&t_start_rd, NULL);
-    TEST_ESP_OK(sdmmc_read_sectors(card, buffer, start_block, block_count));
+    TEST_ESP_OK(sdmmc_read_sectors(card, c_buffer, start_block, block_count));
     struct timeval t_stop_rd;
     gettimeofday(&t_stop_rd, NULL);
     float time_rd = 1e3f * (t_stop_rd.tv_sec - t_start_rd.tv_sec) + 1e-3f * (t_stop_rd.tv_usec - t_start_rd.tv_usec);
 
-    printf(" |   %6.2f    |      %.2f      |    %.2f     |     %.2f\n",
+    printf(" |   %6.2f    |      %5.2f      |    %6.2f     |     %5.2f\n",
             time_wr, total_size / (time_wr / 1000) / (1024 * 1024),
             time_rd, total_size / (time_rd / 1000) / (1024 * 1024));
-    srand(start_block);
+    check_buffer(start_block, c_buffer, total_size / sizeof(buffer[0]));
-    for (size_t i = 0; i < total_size / sizeof(buffer[0]); ++i) {
-        TEST_ASSERT_EQUAL_HEX32(rand(), buffer[i]);
-    }
     free(buffer);
 }
 
 static void read_write_test(sdmmc_card_t* card)
 {
     sdmmc_card_print_info(stdout, card);
-    printf("  sector  | count | size(kB) | wr_time(ms) | wr_speed(MB/s) | rd_time(ms) | rd_speed(MB/s)\n");
+    printf("  sector  | count | align | size(kB)  | wr_time(ms) | wr_speed(MB/s)  |  rd_time(ms)  | rd_speed(MB/s)\n");
-    do_single_write_read_test(card, 0, 1);
+    do_single_write_read_test(card, 0, 1, 4);
-    do_single_write_read_test(card, 0, 4);
+    do_single_write_read_test(card, 0, 4, 4);
-    do_single_write_read_test(card, 1, 16);
+    do_single_write_read_test(card, 1, 16, 4);
-    do_single_write_read_test(card, 16, 32);
+    do_single_write_read_test(card, 16, 32, 4);
-    do_single_write_read_test(card, 48, 64);
+    do_single_write_read_test(card, 48, 64, 4);
-    do_single_write_read_test(card, 128, 128);
+    do_single_write_read_test(card, 128, 128, 4);
-    do_single_write_read_test(card, card->csd.capacity - 64, 32);
+    do_single_write_read_test(card, card->csd.capacity - 64, 32, 4);
-    do_single_write_read_test(card, card->csd.capacity - 64, 64);
+    do_single_write_read_test(card, card->csd.capacity - 64, 64, 4);
-    do_single_write_read_test(card, card->csd.capacity - 8, 1);
+    do_single_write_read_test(card, card->csd.capacity - 8, 1, 4);
-    do_single_write_read_test(card, card->csd.capacity/2, 1);
+    do_single_write_read_test(card, card->csd.capacity/2, 1, 4);
-    do_single_write_read_test(card, card->csd.capacity/2, 4);
+    do_single_write_read_test(card, card->csd.capacity/2, 4, 4);
-    do_single_write_read_test(card, card->csd.capacity/2, 8);
+    do_single_write_read_test(card, card->csd.capacity/2, 8, 4);
-    do_single_write_read_test(card, card->csd.capacity/2, 16);
+    do_single_write_read_test(card, card->csd.capacity/2, 16, 4);
-    do_single_write_read_test(card, card->csd.capacity/2, 32);
+    do_single_write_read_test(card, card->csd.capacity/2, 32, 4);
-    do_single_write_read_test(card, card->csd.capacity/2, 64);
+    do_single_write_read_test(card, card->csd.capacity/2, 64, 4);
-    do_single_write_read_test(card, card->csd.capacity/2, 128);
+    do_single_write_read_test(card, card->csd.capacity/2, 128, 4);
+    do_single_write_read_test(card, card->csd.capacity/2, 1, 1);
+    do_single_write_read_test(card, card->csd.capacity/2, 8, 1);
+    do_single_write_read_test(card, card->csd.capacity/2, 128, 1);
 }
 
 TEST_CASE("can write and read back blocks", "[sd][ignore]")
@@ -142,3 +166,37 @@ TEST_CASE("can write and read back blocks (using SPI)", "[sdspi][ignore]")
     TEST_ESP_OK(sdspi_host_deinit());
 }
 
+TEST_CASE("reads and writes with an unaligned buffer", "[sd]")
+{
+    sdmmc_host_t config = SDMMC_HOST_DEFAULT();
+    TEST_ESP_OK(sdmmc_host_init());
+    sdmmc_slot_config_t slot_config = SDMMC_SLOT_CONFIG_DEFAULT();
+    TEST_ESP_OK(sdmmc_host_init_slot(SDMMC_HOST_SLOT_1, &slot_config));
+    sdmmc_card_t* card = malloc(sizeof(sdmmc_card_t));
+    TEST_ASSERT_NOT_NULL(card);
+    TEST_ESP_OK(sdmmc_card_init(&config, card));
+
+    const size_t buffer_size = 4096;
+    const size_t block_count = buffer_size / 512;
+    const size_t extra = 4;
+    uint8_t* buffer = heap_caps_malloc(buffer_size + extra, MALLOC_CAP_DMA);
+
+    // Check read behavior: do aligned write, then unaligned read
+    const uint32_t seed = 0x89abcdef;
+    fill_buffer(seed, buffer, buffer_size / sizeof(uint32_t));
+    TEST_ESP_OK(sdmmc_write_sectors(card, buffer, 0, block_count));
+    memset(buffer, 0xcc, buffer_size + extra);
+    TEST_ESP_OK(sdmmc_read_sectors(card, buffer + 1, 0, block_count));
+    check_buffer(seed, buffer + 1, buffer_size / sizeof(uint32_t));
+
+    // Check write behavior: do unaligned write, then aligned read
+    fill_buffer(seed, buffer + 1, buffer_size / sizeof(uint32_t));
+    TEST_ESP_OK(sdmmc_write_sectors(card, buffer + 1, 8, block_count));
+    memset(buffer, 0xcc, buffer_size + extra);
+    TEST_ESP_OK(sdmmc_read_sectors(card, buffer, 8, block_count));
+    check_buffer(seed, buffer, buffer_size / sizeof(uint32_t));
+
+    free(buffer);
+    free(card);
+    TEST_ESP_OK(sdmmc_host_deinit());
+}