Merge branch 'bugfix/spiflash_read_psram_v4.0' into 'release/v4.0'

spi_flash: Fix over-allocation and OOM crash when reading from SPI flash to PSRAM buffers (v4.0)

See merge request espressif/esp-idf!7878

components/spi_flash/esp_flash_api.c

@@ -497,12 +497,28 @@ esp_err_t IRAM_ATTR esp_flash_read(esp_flash_t *chip, void *buffer, uint32_t add
     bool direct_read = chip->host->supports_direct_read(chip->host, buffer);
     uint8_t* temp_buffer = NULL;
 
+    //each time, we at most read this length
+    //after that, we release the lock to allow some other operations
+    size_t read_chunk_size = MIN(MAX_READ_CHUNK, length);
+
     if (!direct_read) {
-        uint32_t length_to_allocate = MAX(MAX_READ_CHUNK, length);
-        length_to_allocate = (length_to_allocate+3)&(~3);
-        temp_buffer = heap_caps_malloc(length_to_allocate, MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT);
-        ESP_LOGV(TAG, "allocate temp buffer: %p", temp_buffer);
-        if (temp_buffer == NULL) return ESP_ERR_NO_MEM;
+        /* Allocate temporary internal buffer to use for the actual read. If the preferred size
+           doesn't fit in free internal memory, allocate the largest available free block.
+
+           (May need to shrink read_chunk_size and retry due to race conditions with other tasks
+           also allocating from the heap.)
+        */
+        unsigned retries = 5;
+        while(temp_buffer == NULL && retries--) {
+            read_chunk_size = MIN(read_chunk_size, heap_caps_get_largest_free_block(MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT));
+            read_chunk_size = (read_chunk_size + 3) & ~3;
+            temp_buffer = heap_caps_malloc(read_chunk_size, MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT);
+        }
+        ESP_LOGV(TAG, "allocate temp buffer: %p (%d)", temp_buffer, read_chunk_size);
+
+        if (temp_buffer == NULL) {
+            return ESP_ERR_NO_MEM;
+        }
     }
 
     esp_err_t err = ESP_OK;
@@ -514,9 +530,9 @@ esp_err_t IRAM_ATTR esp_flash_read(esp_flash_t *chip, void *buffer, uint32_t add
         }
         //if required (dma buffer allocated), read to the buffer instead of the original buffer
         uint8_t* buffer_to_read = (temp_buffer)? temp_buffer : buffer;
-        //each time, we at most read this length
-        //after that, we release the lock to allow some other operations
-        uint32_t length_to_read = MIN(MAX_READ_CHUNK, length);
+
+        // Length we will read this iteration is either the chunk size or the remaining length, whichever is smaller
+        size_t length_to_read = MIN(read_chunk_size, length);
 
         if (err == ESP_OK) {
             err = chip->chip_drv->read(chip, buffer_to_read, address, length_to_read);
@@ -682,27 +698,32 @@ esp_err_t esp_flash_app_disable_protect(bool disable)
 
 #ifndef CONFIG_SPI_FLASH_USE_LEGACY_IMPL
 
+/* Translate any ESP_ERR_FLASH_xxx error code (new API) to a generic ESP_ERR_xyz error code
+ */
 static IRAM_ATTR esp_err_t spi_flash_translate_rc(esp_err_t err)
 {
     switch (err) {
         case ESP_OK:
-            return ESP_OK;
         case ESP_ERR_INVALID_ARG:
-            return ESP_ERR_INVALID_ARG;
+        case ESP_ERR_NO_MEM:
+            return err;
+
         case ESP_ERR_FLASH_NOT_INITIALISED:
         case ESP_ERR_FLASH_PROTECTED:
             return ESP_ERR_INVALID_STATE;
+
         case ESP_ERR_NOT_FOUND:
         case ESP_ERR_FLASH_UNSUPPORTED_HOST:
         case ESP_ERR_FLASH_UNSUPPORTED_CHIP:
             return ESP_ERR_NOT_SUPPORTED;
+
         case ESP_ERR_FLASH_NO_RESPONSE:
             return ESP_ERR_INVALID_RESPONSE;
+
         default:
-            ESP_EARLY_LOGE(TAG, "unexpected spi flash error code: %x", err);
+            ESP_EARLY_LOGE(TAG, "unexpected spi flash error code: 0x%x", err);
             abort();
     }
-    return ESP_OK;
 }
 
 esp_err_t IRAM_ATTR spi_flash_erase_range(uint32_t start_addr, uint32_t size)

components/spi_flash/include/esp_flash.h

@@ -230,8 +230,7 @@ esp_err_t esp_flash_set_protected_region(esp_flash_t *chip, const esp_flash_regi
  *
  * @return
  *      - ESP_OK: success
- *      - ESP_ERR_NO_MEM: the buffer is not valid, however failed to malloc on
- *        the heap.
+ *      - ESP_ERR_NO_MEM: Buffer is in external PSRAM which cannot be concurrently accessed, and a temporary internal buffer could not be allocated.
  *      - or a flash error code if operation failed.
  */
 esp_err_t esp_flash_read(esp_flash_t *chip, void *buffer, uint32_t address, uint32_t length);

components/spi_flash/test/test_esp_flash.c

@@ -3,7 +3,7 @@
 #include <freertos/FreeRTOS.h>
 #include <freertos/task.h>
 #include <freertos/semphr.h>
-
+#include <sys/param.h>
 #include <unity.h>
 #include "esp_flash.h"
 #include "driver/spi_common_internal.h"
@@ -40,17 +40,25 @@ static uint8_t sector_buf[4096];
 #define VSPI_PIN_NUM_HD     VSPI_IOMUX_PIN_NUM_HD
 #define VSPI_PIN_NUM_WP     VSPI_IOMUX_PIN_NUM_WP
 
-#define ALL_TEST_NUM (sizeof(config_list)/sizeof(flashtest_config_t))
+#define TEST_CONFIG_NUM (sizeof(config_list)/sizeof(flashtest_config_t))
+
 typedef void (*flash_test_func_t)(esp_flash_t* chip);
 
+/* Use FLASH_TEST_CASE for SPI flash tests that only use the main SPI flash chip
+*/
 #define FLASH_TEST_CASE(STR, FUNC_TO_RUN) \
-    TEST_CASE(STR, "[esp_flash]") {flash_test_func(FUNC_TO_RUN, 1);}
-#ifdef CONFIG_ESP32_SPIRAM_SUPPORT
-// These tests needs external flash, right on the place of psram
+    TEST_CASE(STR, "[esp_flash]") {flash_test_func(FUNC_TO_RUN, 1 /* first index reserved for main flash */ );}
+
+/* Use FLASH_TEST_CASE_3 for tests which also run on external flash, which sits in the place of PSRAM
+   (these tests are incompatible with PSRAM)
+
+   These tests run for all the flash chip configs shown in config_list, below (internal and external).
+ */
+#if defined(CONFIG_SPIRAM_SUPPORT)
 #define FLASH_TEST_CASE_3(STR, FUNCT_TO_RUN)
 #else
 #define FLASH_TEST_CASE_3(STR, FUNC_TO_RUN) \
-    TEST_CASE(STR", 3 chips", "[esp_flash][test_env=UT_T1_ESP_FLASH]") {flash_test_func(FUNC_TO_RUN, ALL_TEST_NUM);}
+    TEST_CASE(STR", 3 chips", "[esp_flash][test_env=UT_T1_ESP_FLASH]") {flash_test_func(FUNC_TO_RUN, TEST_CONFIG_NUM);}
 #endif
 
 //currently all the configs are the same with esp_flash_spi_device_config_t, no more information required
@@ -177,12 +185,14 @@ void teardown_test_chip(esp_flash_t* chip, spi_host_device_t host)
 static void flash_test_func(flash_test_func_t func, int test_num)
 {
     for (int i = 0; i < test_num; i++) {
+        ESP_LOGI(TAG, "Testing config %d/%d", i, test_num);
         flashtest_config_t* config = &config_list[i];
         esp_flash_t* chip;
         setup_new_chip(config, &chip);
         (*func)(chip);
         teardown_test_chip(chip, config->host_id);
     }
+    ESP_LOGI(TAG, "Completed %d configs", test_num);
 }
 
 /* ---------- Test code start ------------*/
@@ -514,7 +524,7 @@ TEST_CASE("SPI flash test reading with all speed/mode permutations", "[esp_flash
 #ifndef CONFIG_ESP32_SPIRAM_SUPPORT
 TEST_CASE("SPI flash test reading with all speed/mode permutations, 3 chips", "[esp_flash][test_env=UT_T1_ESP_FLASH]")
 {
-    for (int i = 0; i < ALL_TEST_NUM; i++) {
+    for (int i = 0; i < TEST_CONFIG_NUM; i++) {
         test_permutations(&config_list[i]);
     }
 }
@@ -583,4 +593,73 @@ static void test_write_large_buffer(esp_flash_t *chip, const uint8_t *source, si
 
     write_large_buffer(chip, part, source, length);
     read_and_check(chip, part, source, length);
-}
+}
+
+#ifdef CONFIG_SPIRAM_USE_MALLOC
+
+/* Utility: Read into a small internal RAM buffer using esp_flash_read() and compare what
+   we read with 'buffer' */
+static void s_test_compare_flash_contents_small_reads(esp_flash_t *chip, const uint8_t *buffer, size_t offs, size_t len)
+{
+    const size_t INTERNAL_BUF_SZ = 1024; // Should fit in internal RAM
+    uint8_t *ibuf = heap_caps_malloc(INTERNAL_BUF_SZ, MALLOC_CAP_8BIT|MALLOC_CAP_INTERNAL);
+    TEST_ASSERT_NOT_NULL(ibuf);
+
+    for (int i = 0; i < len; i += INTERNAL_BUF_SZ) {
+        size_t to_read = MIN(INTERNAL_BUF_SZ, len - i);
+        ESP_ERROR_CHECK( esp_flash_read(chip, ibuf, offs + i, to_read) );
+        TEST_ASSERT_EQUAL_HEX8_ARRAY(buffer + i, ibuf, to_read);
+    }
+
+    free(ibuf);
+}
+
+static void test_flash_read_large_psram_buffer(esp_flash_t *chip)
+{
+    const size_t BUF_SZ = 256 * 1024;    // Too large for internal RAM
+    const size_t TEST_OFFS = 0x1000; // Can be any offset, really
+
+    uint8_t *buf = heap_caps_malloc(BUF_SZ, MALLOC_CAP_8BIT|MALLOC_CAP_SPIRAM);
+    TEST_ASSERT_NOT_NULL(buf);
+
+    ESP_ERROR_CHECK( esp_flash_read(chip, buf, TEST_OFFS, BUF_SZ) );
+
+    // Read back the same into smaller internal memory buffer and check it all matches
+    s_test_compare_flash_contents_small_reads(chip, buf, TEST_OFFS, BUF_SZ);
+
+    free(buf);
+}
+
+FLASH_TEST_CASE("esp_flash_read large PSRAM buffer", test_flash_read_large_psram_buffer);
+
+
+/* similar to above test, but perform it under memory pressure */
+static void test_flash_read_large_psram_buffer_low_internal_mem(esp_flash_t *chip)
+{
+    const size_t BUF_SZ = 256 * 1024;    // Too large for internal RAM
+    const size_t REMAINING_INTERNAL = 1024; // Exhaust internal memory until maximum free block is less than this
+    const size_t TEST_OFFS = 0x8000;
+
+    /* Exhaust the available free internal memory */
+    test_utils_exhaust_memory_rec erec = test_utils_exhaust_memory(MALLOC_CAP_INTERNAL|MALLOC_CAP_8BIT, REMAINING_INTERNAL);
+
+    uint8_t *buf = heap_caps_malloc(BUF_SZ, MALLOC_CAP_8BIT|MALLOC_CAP_SPIRAM);
+    TEST_ASSERT_NOT_NULL(buf);
+
+    /* Calling esp_flash_read() here will need to allocate a small internal buffer,
+       so check it works. */
+    ESP_ERROR_CHECK( esp_flash_read(chip, buf, TEST_OFFS, BUF_SZ) );
+
+    test_utils_free_exhausted_memory(erec);
+
+    // Read back the same into smaller internal memory buffer and check it all matches
+    s_test_compare_flash_contents_small_reads(chip, buf, TEST_OFFS, BUF_SZ);
+
+    free(buf);
+}
+
+FLASH_TEST_CASE("esp_flash_read large PSRAM buffer low memory", test_flash_read_large_psram_buffer_low_internal_mem);
+
+
+
+#endif

tools/unit-test-app/components/test_utils/include/test_utils.h

@@ -236,3 +236,31 @@ esp_err_t test_utils_set_leak_level(size_t leak_level, esp_type_leak_t type, esp
  * return Leak level
  */
 size_t test_utils_get_leak_level(esp_type_leak_t type, esp_comp_leak_t component);
+
+
+
+typedef struct test_utils_exhaust_memory_record_s *test_utils_exhaust_memory_rec;
+
+/**
+ * Limit the largest free block of memory with a particular capability set to
+ * 'limit' bytes (meaning an allocation of 'limit' should succeed at least once,
+ * but any allocation of more bytes will fail.)
+ *
+ * Returns a record pointer which needs to be passed back in to test_utils_free_exhausted_memory
+ * before the test completes, to avoid a major memory leak.
+ *
+ * @param caps Capabilities of memory to exhause
+ * @param limit The size to limit largest free block to
+ * @return Record pointer to pass to test_utils_free_exhausted_memory() once done
+ */
+test_utils_exhaust_memory_rec test_utils_exhaust_memory(uint32_t caps, size_t limit);
+
+
+/**
+ * Call to free memory which was taken up by test_utils_exhaust_memory() call
+ *
+ * @param rec Result previously returned from test_utils_exhaust_memory()
+ */
+void test_utils_free_exhausted_memory(test_utils_exhaust_memory_rec rec);
+
+

tools/unit-test-app/components/test_utils/test_utils.c

@@ -143,3 +143,41 @@ size_t test_utils_get_leak_level(esp_type_leak_t type_of_leak, esp_comp_leak_t c
     }
     return leak_level;
 }
+
+
+#define EXHAUST_MEMORY_ENTRIES 100
+
+struct test_utils_exhaust_memory_record_s {
+    int *entries[EXHAUST_MEMORY_ENTRIES];
+};
+
+test_utils_exhaust_memory_rec test_utils_exhaust_memory(uint32_t caps, size_t limit)
+{
+    int idx = 0;
+    test_utils_exhaust_memory_rec rec = calloc(1, sizeof(struct test_utils_exhaust_memory_record_s));
+    TEST_ASSERT_NOT_NULL_MESSAGE(rec, "test_utils_exhaust_memory: not enough free memory to allocate record structure!");
+
+    while (idx < EXHAUST_MEMORY_ENTRIES) {
+        size_t free_caps = heap_caps_get_largest_free_block(caps);
+        if (free_caps <= limit) {
+            return rec; // done!
+        }
+        rec->entries[idx] = heap_caps_malloc(free_caps - limit, caps);
+        TEST_ASSERT_NOT_NULL_MESSAGE(rec->entries[idx],
+                                     "test_utils_exhaust_memory: something went wrong while freeing up memory, is another task using heap?");
+        heap_caps_check_integrity_all(true);
+        idx++;
+    }
+
+    TEST_FAIL_MESSAGE("test_utils_exhaust_memory: The heap with the requested caps is too fragmented, increase EXHAUST_MEMORY_ENTRIES or defrag the heap!");
+    abort();
+}
+
+void test_utils_free_exhausted_memory(test_utils_exhaust_memory_rec rec)
+{
+    for (int i = 0; i < EXHAUST_MEMORY_ENTRIES; i++) {
+        free(rec->entries[i]);
+    }
+    free(rec);
+}
+