spi_flash: Fix over-allocation and OOM crash when reading from SPI flash to PSRAM buffers

Previously would try allocate buffer of minimum size 16KB not maximum size 16KB, causing
out of memory errors for any large reads, or if less than 16KB contiguous free heap.

Also, if using legacy API and internal allocation failed then implementation would abort()
instead of returning the error to the caller.

Added test for using large buffers in PSRAM.

Closes #4769

Also reported on forum: https://esp32.com/viewtopic.php?f=13&t=14304&p=55972

Author: projectgus

components/spi_flash/esp_flash_api.c

@@ -498,11 +498,13 @@ esp_err_t IRAM_ATTR esp_flash_read(esp_flash_t *chip, void *buffer, uint32_t add
     uint8_t* temp_buffer = NULL;
 
     if (!direct_read) {
-        uint32_t length_to_allocate = MAX(MAX_READ_CHUNK, length);
+        uint32_t length_to_allocate = MIN(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;
+        ESP_LOGV(TAG, "allocate temp buffer: %p (%d)", temp_buffer, length_to_allocate);
+        if (temp_buffer == NULL) {
+            return ESP_ERR_NO_MEM;
+        }
     }
 
     esp_err_t err = ESP_OK;
@@ -682,27 +684,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

@@ -68,17 +68,25 @@ static uint8_t sector_buf[4096];
 #define VSPI_PIN_NUM_CS     FSPI_PIN_NUM_CS
 #endif
 
-#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);}
+    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) || TEMPORARY_DISABLED_FOR_TARGETS(ESP32S2BETA)
-// These tests needs external flash, right on the place of psram
 #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
@@ -271,12 +279,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 ------------*/
@@ -615,7 +625,7 @@ TEST_CASE("SPI flash test reading with all speed/mode permutations", "[esp_flash
 #if !TEMPORARY_DISABLED_FOR_TARGETS(ESP32S2BETA)
 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]);
     }
 }
@@ -685,4 +695,35 @@ 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
+
+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 INTERNAL_BUF_SZ = 1024; // Should fit in internal RAM
+    _Static_assert(BUF_SZ % INTERNAL_BUF_SZ == 0, "should be a multiple");
+    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
+    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 < BUF_SZ; i += INTERNAL_BUF_SZ) {
+        ESP_ERROR_CHECK( esp_flash_read(chip, ibuf, TEST_OFFS + i, INTERNAL_BUF_SZ) );
+        TEST_ASSERT_EQUAL_HEX8_ARRAY(buf + i, ibuf, INTERNAL_BUF_SZ);
+    }
+
+    free(ibuf);
+    free(buf);
+}
+
+FLASH_TEST_CASE("esp_flash_read large PSRAM buffer", test_flash_read_large_psram_buffer);
+
+#endif