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.

diff --git a/components/driver/include/driver/sdmmc_host.h b/components/driver/include/driver/sdmmc_host.h
index dc3e9ef6bf997a2f30226c13218dae73bbf3d25a..f2e2d66d5d0ad0cff07759215fa17780921be206 100644 (file)
--- a/components/driver/include/driver/sdmmc_host.h
+++ b/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);
 

diff --git a/components/driver/sdmmc_transaction.c b/components/driver/sdmmc_transaction.c
index 3adc9ce68ef7b781fe87601bde3f67ffa416dc9d..744a2436942b4b6a0298fe9f39745e8248c2c690 100644 (file)
--- a/components/driver/sdmmc_transaction.c
+++ b/components/driver/sdmmc_transaction.c
@@ -20,6 +20,7 @@
 #include "freertos/semphr.h"
 #include "soc/sdmmc_reg.h"
 #include "soc/sdmmc_struct.h"
+#include "esp_heap_alloc_caps.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
-        assert(cmdinfo->datalen >= 4);
-        assert(cmdinfo->blklen % 4 == 0);
+        if (cmdinfo->datalen < 4 || cmdinfo->blklen % 4 != 0) {
+            ESP_LOGD(TAG, "%s: invalid size: total=%d block=%d",
+                    __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

diff --git a/components/sdmmc/sdmmc_cmd.c b/components/sdmmc/sdmmc_cmd.c
index 659ff5dd6cde560d587e372e6bbb252d6894c588..85791ab276aa1163ff0deb4f93b218ec391256c4 100644 (file)
--- a/components/sdmmc/sdmmc_cmd.c
+++ b/components/sdmmc/sdmmc_cmd.c
@@ -46,6 +46,10 @@ static esp_err_t sdmmc_send_cmd_set_bus_width(sdmmc_card_t* card, int width);
 static esp_err_t sdmmc_send_cmd_stop_transmission(sdmmc_card_t* card, uint32_t* status);
 static esp_err_t sdmmc_send_cmd_send_status(sdmmc_card_t* card, uint32_t* out_status);
 static uint32_t  get_host_ocr(float voltage);
+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);
 
 
 esp_err_t sdmmc_card_init(const sdmmc_host_t* config,
@@ -486,6 +490,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 = pvPortMallocCaps(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;
@@ -529,6 +564,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 = pvPortMallocCaps(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;

diff --git a/components/sdmmc/test/test_sd.c b/components/sdmmc/test/test_sd.c
index 768fecc258c0a7036c5d19d4b9c9b95ab0019da5..32ebc1e901d799300ab2fe3dfe9be225ef9377b4 100644 (file)
--- a/components/sdmmc/test/test_sd.c
+++ b/components/sdmmc/test/test_sd.c
@@ -41,38 +41,59 @@ TEST_CASE("can probe SD", "[sd][ignore]")
 }
 
 
+// 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);
-    uint32_t* buffer = pvPortMallocCaps(total_size, MALLOC_CAP_DMA);
-    srand(start_block);
-    for (size_t i = 0; i < total_size / sizeof(buffer[0]); ++i) {
-        buffer[i] = rand();
-    }
+    printf(" %8d |  %3d  |   %d   |    %4.1f  ", start_block, block_count, alignment, total_size / 1024.0f);
+
+    uint32_t* buffer = pvPortMallocCaps(total_size + 4, MALLOC_CAP_DMA);
+    size_t offset = alignment % 4;
+    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      |    %.2fs    |     %.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);
-    for (size_t i = 0; i < total_size / sizeof(buffer[0]); ++i) {
-        TEST_ASSERT_EQUAL_HEX32(rand(), buffer[i]);
-    }
+    check_buffer(start_block, c_buffer, total_size / sizeof(buffer[0]));
     free(buffer);
 }
 
@@ -87,23 +108,61 @@ TEST_CASE("can write and read back blocks", "[sd][ignore]")
     TEST_ASSERT_NOT_NULL(card);
     TEST_ESP_OK(sdmmc_card_init(&config, 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");
-    do_single_write_read_test(card, 0, 1);
-    do_single_write_read_test(card, 0, 4);
-    do_single_write_read_test(card, 1, 16);
-    do_single_write_read_test(card, 16, 32);
-    do_single_write_read_test(card, 48, 64);
-    do_single_write_read_test(card, 128, 128);
-    do_single_write_read_test(card, card->csd.capacity - 64, 32);
-    do_single_write_read_test(card, card->csd.capacity - 64, 64);
-    do_single_write_read_test(card, card->csd.capacity - 8, 1);
-    do_single_write_read_test(card, card->csd.capacity/2, 1);
-    do_single_write_read_test(card, card->csd.capacity/2, 4);
-    do_single_write_read_test(card, card->csd.capacity/2, 8);
-    do_single_write_read_test(card, card->csd.capacity/2, 16);
-    do_single_write_read_test(card, card->csd.capacity/2, 32);
-    do_single_write_read_test(card, card->csd.capacity/2, 64);
-    do_single_write_read_test(card, card->csd.capacity/2, 128);
+    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, 4);
+    do_single_write_read_test(card, 0, 4, 4);
+    do_single_write_read_test(card, 1, 16, 4);
+    do_single_write_read_test(card, 16, 32, 4);
+    do_single_write_read_test(card, 48, 64, 4);
+    do_single_write_read_test(card, 128, 128, 4);
+    do_single_write_read_test(card, card->csd.capacity - 64, 32, 4);
+    do_single_write_read_test(card, card->csd.capacity - 64, 64, 4);
+    do_single_write_read_test(card, card->csd.capacity - 8, 1, 4);
+    do_single_write_read_test(card, card->csd.capacity/2, 1, 4);
+    do_single_write_read_test(card, card->csd.capacity/2, 4, 4);
+    do_single_write_read_test(card, card->csd.capacity/2, 8, 4);
+    do_single_write_read_test(card, card->csd.capacity/2, 16, 4);
+    do_single_write_read_test(card, card->csd.capacity/2, 32, 4);
+    do_single_write_read_test(card, card->csd.capacity/2, 64, 4);
+    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);
     free(card);
     sdmmc_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 = pvPortMallocCaps(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());
+}