Merge branch 'bugfix/fix_spi_flash_api_concurrency_issue' into 'master'

spi_flash: fix concurrency issue when concurrently calling esp_flash apis under xip_psram or auto_suspend

See merge request espressif/esp-idf!24215

components/spi_flash/spi_flash_os_func_app.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -22,9 +22,13 @@
 
 #include "esp_private/spi_common_internal.h"
 
-#define SPI_FLASH_CACHE_NO_DISABLE  (CONFIG_SPI_FLASH_AUTO_SUSPEND || (CONFIG_SPIRAM_FETCH_INSTRUCTIONS && CONFIG_SPIRAM_RODATA))
+#define SPI_FLASH_CACHE_NO_DISABLE  (CONFIG_SPI_FLASH_AUTO_SUSPEND || (CONFIG_SPIRAM_FETCH_INSTRUCTIONS && CONFIG_SPIRAM_RODATA) || CONFIG_APP_BUILD_TYPE_RAM)
 static const char TAG[] = "spi_flash";
 
+#if SPI_FLASH_CACHE_NO_DISABLE
+static _lock_t s_spi1_flash_mutex;
+#endif  //  #if SPI_FLASH_CACHE_NO_DISABLE
+
 /*
  * OS functions providing delay service and arbitration among chips, and with the cache.
  *
@@ -55,21 +59,19 @@ static inline void on_spi_acquired(app_func_arg_t* ctx);
 static inline void on_spi_yielded(app_func_arg_t* ctx);
 static inline bool on_spi_check_yield(app_func_arg_t* ctx);
 
+#if !SPI_FLASH_CACHE_NO_DISABLE
 IRAM_ATTR static void cache_enable(void* arg)
 {
-#if !SPI_FLASH_CACHE_NO_DISABLE
     spi_flash_enable_interrupts_caches_and_other_cpu();
-#endif
 }
 
 IRAM_ATTR static void cache_disable(void* arg)
 {
-#if !SPI_FLASH_CACHE_NO_DISABLE
     spi_flash_disable_interrupts_caches_and_other_cpu();
-#endif
 }
+#endif  //#if !SPI_FLASH_CACHE_NO_DISABLE
 
-static IRAM_ATTR esp_err_t spi_start(void *arg)
+static IRAM_ATTR esp_err_t acquire_spi_bus_lock(void *arg)
 {
     spi_bus_lock_dev_handle_t dev_lock = ((app_func_arg_t *)arg)->dev_lock;
 
@@ -82,41 +84,58 @@ static IRAM_ATTR esp_err_t spi_start(void *arg)
     return ESP_OK;
 }
 
-static IRAM_ATTR esp_err_t spi_end(void *arg)
+static IRAM_ATTR esp_err_t release_spi_bus_lock(void *arg)
 {
     return spi_bus_lock_acquire_end(((app_func_arg_t *)arg)->dev_lock);
 }
 
 static IRAM_ATTR esp_err_t spi23_start(void *arg){
-    esp_err_t ret = spi_start(arg);
+    esp_err_t ret = acquire_spi_bus_lock(arg);
     on_spi_acquired((app_func_arg_t*)arg);
     return ret;
 }
 
 static IRAM_ATTR esp_err_t spi23_end(void *arg){
-    esp_err_t ret = spi_end(arg);
+    esp_err_t ret = release_spi_bus_lock(arg);
     on_spi_released((app_func_arg_t*)arg);
     return ret;
 }
 
 static IRAM_ATTR esp_err_t spi1_start(void *arg)
 {
+    esp_err_t ret = ESP_OK;
+    /**
+     * There are three ways for ESP Flash API lock:
+     * 1. spi bus lock, this is used when SPI1 is shared with GPSPI Master Driver
+     * 2. mutex, this is used when the Cache isn't need to be disabled.
+     * 3. cache lock (from cache_utils.h), this is used when we need to disable Cache to avoid access from SPI0
+     *
+     * From 1 to 3, the lock efficiency decreases.
+     */
 #if CONFIG_SPI_FLASH_SHARE_SPI1_BUS
     //use the lock to disable the cache and interrupts before using the SPI bus
-    return spi_start(arg);
+    ret = acquire_spi_bus_lock(arg);
+#elif SPI_FLASH_CACHE_NO_DISABLE
+    _lock_acquire(&s_spi1_flash_mutex);
 #else
     //directly disable the cache and interrupts when lock is not used
     cache_disable(NULL);
-    on_spi_acquired((app_func_arg_t*)arg);
-    return ESP_OK;
 #endif
+    on_spi_acquired((app_func_arg_t*)arg);
+    return ret;
 }
 
 static IRAM_ATTR esp_err_t spi1_end(void *arg)
 {
     esp_err_t ret = ESP_OK;
+
+    /**
+     * There are three ways for ESP Flash API lock, see `spi1_start`
+     */
 #if CONFIG_SPI_FLASH_SHARE_SPI1_BUS
-    ret = spi_end(arg);
+    ret = release_spi_bus_lock(arg);
+#elif SPI_FLASH_CACHE_NO_DISABLE
+    _lock_release(&s_spi1_flash_mutex);
 #else
     cache_enable(NULL);
 #endif

components/spi_flash/test_apps/components/test_flash_utils/CMakeLists.txt

@@ -0,0 +1,5 @@
+set(srcs "test_flash_utils.c")
+
+idf_component_register(SRCS ${srcs}
+                       INCLUDE_DIRS include
+                       REQUIRES esp_partition)

components/spi_flash/test_apps/components/test_flash_utils/include/test_flash_utils.h

@@ -0,0 +1,27 @@
+/*
+ * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#pragma once
+
+#include <esp_types.h>
+#include "esp_partition.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+
+/**
+ * Return the 'flash_test' custom data partition
+ * defined in the custom partition table.
+ *
+ * @return partition handle
+ */
+const esp_partition_t *get_test_flash_partition(void);
+
+#ifdef __cplusplus
+}
+#endif

components/spi_flash/test_apps/components/test_flash_utils/test_flash_utils.c

@@ -0,0 +1,17 @@
+/*
+ * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#include <string.h>
+#include "test_flash_utils.h"
+
+const esp_partition_t *get_test_flash_partition(void)
+{
+    /* This finds "flash_test" partition defined in custom partitions.csv */
+    const esp_partition_t *result = esp_partition_find_first(ESP_PARTITION_TYPE_DATA,
+            ESP_PARTITION_SUBTYPE_ANY, "flash_test");
+    assert(result != NULL); /* means partition table set wrong */
+    return result;
+}

components/spi_flash/test_apps/esp_flash_stress/CMakeLists.txt

@@ -0,0 +1,8 @@
+# This is the project CMakeLists.txt file for the test subproject
+cmake_minimum_required(VERSION 3.16)
+
+set(EXTRA_COMPONENT_DIRS "$ENV{IDF_PATH}/components/spi_flash/test_apps/components")
+
+include($ENV{IDF_PATH}/tools/cmake/project.cmake)
+
+project(test_esp_flash_stress)

components/spi_flash/test_apps/esp_flash_stress/README.md

@@ -0,0 +1,2 @@
+| Supported Targets | ESP32 | ESP32-C2 | ESP32-C3 | ESP32-C6 | ESP32-H2 | ESP32-S2 | ESP32-S3 |
+| ----------------- | ----- | -------- | -------- | -------- | -------- | -------- | -------- |

components/spi_flash/test_apps/esp_flash_stress/main/CMakeLists.txt

@@ -0,0 +1,8 @@
+set(srcs "test_app_main.c"
+         "test_esp_flash_stress.c")
+
+# In order for the cases defined by `TEST_CASE` to be linked into the final elf,
+# the component can be registered as WHOLE_ARCHIVE
+idf_component_register(SRCS ${srcs}
+                       PRIV_REQUIRES test_flash_utils spi_flash unity
+                       WHOLE_ARCHIVE)

components/spi_flash/test_apps/esp_flash_stress/main/test_app_main.c

@@ -0,0 +1,35 @@
+/*
+ * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#include "unity.h"
+#include "unity_test_utils.h"
+#include "esp_heap_caps.h"
+
+// load partition table in tests will use memory
+#define TEST_MEMORY_LEAK_THRESHOLD (600)
+
+void setUp(void)
+{
+    unity_utils_record_free_mem();
+}
+
+void tearDown(void)
+{
+    esp_reent_cleanup();    //clean up some of the newlib's lazy allocations
+    unity_utils_evaluate_leaks_direct(TEST_MEMORY_LEAK_THRESHOLD);
+}
+
+void app_main(void)
+{
+    printf(" ______  _____ _____    ______ _           _        _____ _                     \n");
+    printf("|  ____|/ ____|  __ \\  |  ____| |         | |      / ____| |                    \n");
+    printf("| |__  | (___ | |__) | | |__  | | __ _ ___| |__   | (___ | |_ _ __ ___  ___ ___ \n");
+    printf("|  __|  \\___ \\|  ___/  |  __| | |/ _` / __| '_ \\   \\___ \\| __| '__/ _ \\/ __/ __|\n");
+    printf("| |____ ____) | |      | |    | | (_| \\__ \\ | | |  ____) | |_| | |  __/\\__ \\__ \\\n");
+    printf("|______|_____/|_|      |_|    |_|\\__,_|___/_| |_| |_____/ \\__|_|  \\___||___/___/\n");
+
+    unity_run_menu();
+}

components/spi_flash/test_apps/esp_flash_stress/main/test_esp_flash_stress.c

@@ -0,0 +1,200 @@
+/*
+ * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include "unity.h"
+#include "sdkconfig.h"
+#include "freertos/FreeRTOS.h"
+#include "freertos/task.h"
+#include "freertos/semphr.h"
+#include "esp_system.h"
+#include "esp_check.h"
+#include "esp_attr.h"
+#include "esp_flash.h"
+#include "test_flash_utils.h"
+
+portMUX_TYPE static s_test_spinlock = portMUX_INITIALIZER_UNLOCKED;
+
+/*---------------------------------------------------------------
+            ESP Flash API Concurrency Pressure Test
+---------------------------------------------------------------*/
+#define TEST_FLASH_CONCURRENCY_TASK_NUM    12
+
+static int s_test_concurrency_id;
+//only used for finite loop for CI test
+SemaphoreHandle_t s_test_concurrency_smphr;
+
+typedef struct {
+    bool is_pressure_test;
+    const esp_partition_t *part;
+} test_concurrency_ctx_t;
+
+static void s_test_flash_ops_task(void *arg)
+{
+    test_concurrency_ctx_t *test_ctx = (test_concurrency_ctx_t *)arg;
+    bool is_pressure_test = test_ctx->is_pressure_test;;
+    esp_flash_t *chip = test_ctx->part->flash_chip;
+    uint32_t offs = test_ctx->part->address;
+
+    portENTER_CRITICAL(&s_test_spinlock);
+    int id = s_test_concurrency_id;
+    s_test_concurrency_id++;
+    portEXIT_CRITICAL(&s_test_spinlock);
+
+    int flash_block_sz = 4096;
+    char *buffer = calloc(1, flash_block_sz);
+    TEST_ASSERT(buffer);
+    srand(id + 299);
+    int delay_us = rand()  % 1000;
+    printf("delay_us: %d\n", delay_us);
+
+    if (is_pressure_test) {
+        printf("INFINITE_LOOP\n");
+        while (1) {
+            printf("%s %d is running\n", __func__, id);
+            TEST_ESP_OK(esp_flash_erase_region(chip, id * flash_block_sz + offs, flash_block_sz));
+            TEST_ESP_OK(esp_flash_write(chip, buffer, id * flash_block_sz + offs, flash_block_sz));
+            TEST_ESP_OK(esp_flash_read(chip, buffer, id * flash_block_sz + offs, flash_block_sz));
+            vTaskDelay(pdMS_TO_TICKS(delay_us));
+        }
+    } else {
+        printf("FINITE_LOOP\n");
+        for (int i = 0; i < 10; i++) {
+            TEST_ESP_OK(esp_flash_erase_region(chip, id * flash_block_sz + offs, 4096));
+            TEST_ESP_OK(esp_flash_write(chip, buffer, id * flash_block_sz + offs, flash_block_sz));
+            TEST_ESP_OK(esp_flash_read(chip, buffer, id * flash_block_sz + offs, flash_block_sz));
+            vTaskDelay(pdMS_TO_TICKS(delay_us));
+        }
+        printf("%s %d finishes\n", __func__, id);
+        free(buffer);
+        xSemaphoreGive(s_test_concurrency_smphr);
+        vTaskDelete(NULL);
+    }
+}
+
+/*-------------------------------------------
+            Uni Core
+-------------------------------------------*/
+TEST_CASE("Flash UniCore: Test ESP Flash API Concurrency", "[esp_flash]")
+{
+    const esp_partition_t* part = get_test_flash_partition();
+
+    s_test_concurrency_id = 0;
+    s_test_concurrency_smphr = xSemaphoreCreateCounting(TEST_FLASH_CONCURRENCY_TASK_NUM, 0);
+    TEST_ASSERT(s_test_concurrency_smphr);
+
+    char flash_task_name[32];
+    test_concurrency_ctx_t ctx = {
+        .is_pressure_test = false,
+        .part = part,
+    };
+
+    for (int i = 0; i < TEST_FLASH_CONCURRENCY_TASK_NUM; i++) {
+        int l = snprintf(flash_task_name, 32, "flash_ops_task%d", i);
+        flash_task_name[l] = '\0';
+        xTaskCreatePinnedToCore(&s_test_flash_ops_task, flash_task_name, 4096, (void *)(&ctx), 5, NULL, 0);
+    }
+
+    for (int i = 0; i < TEST_FLASH_CONCURRENCY_TASK_NUM; i++) {
+        TEST_ASSERT(xSemaphoreTake(s_test_concurrency_smphr, portMAX_DELAY) == pdTRUE);
+    }
+    vTaskDelay(1);
+    vSemaphoreDelete(s_test_concurrency_smphr);
+}
+
+/**
+ * esp_flash APIs concurrency pressure test
+ * This test is for manually test
+ */
+TEST_CASE("Flash UniCore: Test ESP Flash API Concurrency [Stress]", "[esp_flash][stress][manual][ignore]")
+{
+    const esp_partition_t* part = get_test_flash_partition();
+
+    s_test_concurrency_id = 0;
+    char flash_task_name[32];
+    test_concurrency_ctx_t ctx = {
+        .is_pressure_test = true,
+        .part = part,
+    };
+
+    for (int i = 0; i < TEST_FLASH_CONCURRENCY_TASK_NUM; i++) {
+        int l = snprintf(flash_task_name, 32, "flash_ops_task%d", i);
+        flash_task_name[l] = '\0';
+        xTaskCreatePinnedToCore(&s_test_flash_ops_task, flash_task_name, 4096, (void *)(&ctx), 5, NULL, 0);
+    }
+
+    while(1);
+}
+
+#if !CONFIG_FREERTOS_UNICORE
+/*-------------------------------------------
+            Dual Core
+-------------------------------------------*/
+TEST_CASE("Flash DualCore: Test ESP Flash API Concurrency", "[esp_flash]")
+{
+    const esp_partition_t* part = get_test_flash_partition();
+
+    s_test_concurrency_id = 0;
+    s_test_concurrency_smphr = xSemaphoreCreateCounting(TEST_FLASH_CONCURRENCY_TASK_NUM, 0);
+    TEST_ASSERT(s_test_concurrency_smphr);
+
+    char flash_task_name[32];
+    test_concurrency_ctx_t ctx = {
+        .is_pressure_test = false,
+        .part = part,
+    };
+
+    for (int i = 0; i < TEST_FLASH_CONCURRENCY_TASK_NUM / 2; i++) {
+        int l = snprintf(flash_task_name, 32, "core0_flash_ops_task%d", i);
+        flash_task_name[l] = '\0';
+        xTaskCreatePinnedToCore(&s_test_flash_ops_task, flash_task_name, 4096, (void *)(&ctx), 5, NULL, 0);
+    }
+
+    for (int i = 0; i < TEST_FLASH_CONCURRENCY_TASK_NUM / 2; i++) {
+        int l = snprintf(flash_task_name, 32, "core1_flash_ops_task%d", i);
+        flash_task_name[l] = '\0';
+        xTaskCreatePinnedToCore(&s_test_flash_ops_task, flash_task_name, 4096, (void *)(&ctx), 5, NULL, 1);
+    }
+
+    for (int i = 0; i < TEST_FLASH_CONCURRENCY_TASK_NUM; i++) {
+        TEST_ASSERT(xSemaphoreTake(s_test_concurrency_smphr, portMAX_DELAY) == pdTRUE);
+    }
+    vTaskDelay(1);
+    vSemaphoreDelete(s_test_concurrency_smphr);
+}
+
+
+/**
+ * esp_flash APIs concurrency pressure test
+ * This test is for manually test
+ */
+TEST_CASE("Flash DualCore: Test ESP Flash API Concurrency [Stress]", "[esp_flash][stress][manual][ignore]")
+{
+    const esp_partition_t* part = get_test_flash_partition();
+
+    s_test_concurrency_id = 0;
+    char flash_task_name[32];
+    test_concurrency_ctx_t ctx = {
+        .is_pressure_test = true,
+        .part = part,
+    };
+
+    for (int i = 0; i < TEST_FLASH_CONCURRENCY_TASK_NUM / 2; i++) {
+        int l = snprintf(flash_task_name, 32, "core0_flash_ops_task%d", i);
+        flash_task_name[l] = '\0';
+        xTaskCreatePinnedToCore(&s_test_flash_ops_task, flash_task_name, 4096, (void *)(&ctx), 5, NULL, 0);
+    }
+
+    for (int i = 0; i < TEST_FLASH_CONCURRENCY_TASK_NUM / 2; i++) {
+        int l = snprintf(flash_task_name, 32, "core1_flash_ops_task%d", i);
+        flash_task_name[l] = '\0';
+        xTaskCreatePinnedToCore(&s_test_flash_ops_task, flash_task_name, 4096, (void *)(&ctx), 5, NULL, 1);
+    }
+
+    while(1);
+}
+#endif  //#if !CONFIG_FREERTOS_UNICORE

components/spi_flash/test_apps/esp_flash_stress/partitions.csv

@@ -0,0 +1,6 @@
+# Name,   Type, SubType, Offset,  Size, Flags
+# Note: if you have increased the bootloader size, make sure to update the offsets to avoid overlap
+nvs,        data, nvs,      0x9000,  0x6000,
+phy_init,   data, phy,      0xf000,  0x1000,
+factory,    app,  factory,  0x10000, 1M,
+flash_test,   data, fat,             , 512K,

components/spi_flash/test_apps/esp_flash_stress/pytest_esp_flash_stress.py

@@ -0,0 +1,57 @@
+# SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD
+# SPDX-License-Identifier: Apache-2.0
+
+import pytest
+from pytest_embedded import Dut
+
+
+@pytest.mark.supported_targets
+@pytest.mark.generic
+@pytest.mark.parametrize(
+    'config',
+    [
+        'release',
+    ],
+    indirect=True,
+)
+def test_esp_flash_stress(dut: Dut) -> None:
+    dut.run_all_single_board_cases(group='esp_flash')
+
+
+@pytest.mark.esp32s3
+@pytest.mark.MSPI_F8R8
+@pytest.mark.parametrize(
+    'config',
+    [
+        'esp32s3_f8r8',
+    ],
+    indirect=True,
+)
+def test_esp_flash_stress_f8r8(dut: Dut) -> None:
+    dut.run_all_single_board_cases()
+
+
+@pytest.mark.esp32s3
+@pytest.mark.generic
+@pytest.mark.parametrize(
+    'config',
+    [
+        'esp32s3_rom_xip_psram',
+    ],
+    indirect=True,
+)
+def test_esp_flash_stress_rom_xip_psram(dut: Dut) -> None:
+    dut.run_all_single_board_cases()
+
+
+@pytest.mark.esp32c3
+@pytest.mark.flash_suspend
+@pytest.mark.parametrize(
+    'config',
+    [
+        'esp32c3_suspend',
+    ],
+    indirect=True,
+)
+def test_flash_auto_suspend(dut: Dut) -> None:
+    dut.run_all_single_board_cases()

components/spi_flash/test_apps/esp_flash_stress/sdkconfig.ci.esp32c3_suspend

@@ -0,0 +1,4 @@
+# ESP32S3, F8R8, Flash 80M DDR, PSRAM 80M DDR, XIP_PSRAM
+
+CONFIG_IDF_TARGET="esp32c3"
+CONFIG_SPI_FLASH_AUTO_SUSPEND=y

components/spi_flash/test_apps/esp_flash_stress/sdkconfig.ci.esp32s3_f8r8

@@ -0,0 +1,12 @@
+# ESP32S3, F8R8, Flash 80M DDR, PSRAM 80M DDR, XIP_PSRAM
+
+CONFIG_IDF_TARGET="esp32s3"
+CONFIG_ESPTOOLPY_OCT_FLASH=y
+CONFIG_ESPTOOLPY_FLASH_SAMPLE_MODE_DTR=y
+CONFIG_ESPTOOLPY_FLASHFREQ_80M=y
+CONFIG_ESPTOOLPY_FLASHSIZE_4MB=y
+CONFIG_SPIRAM=y
+CONFIG_SPIRAM_MODE_OCT=y
+CONFIG_SPIRAM_SPEED_80M=y
+CONFIG_SPIRAM_FETCH_INSTRUCTIONS=y
+CONFIG_SPIRAM_RODATA=y

components/spi_flash/test_apps/esp_flash_stress/sdkconfig.ci.esp32s3_rom_xip_psram

@@ -0,0 +1,8 @@
+# ESP32S3, F8R8, ESP Flash ROM Version, XIP_PSRAM
+
+CONFIG_IDF_TARGET="esp32s3"
+
+CONFIG_SPIRAM=y
+CONFIG_SPIRAM_FETCH_INSTRUCTIONS=y
+CONFIG_SPIRAM_RODATA=y
+CONFIG_SPI_FLASH_ROM_IMPL=y

components/spi_flash/test_apps/esp_flash_stress/sdkconfig.ci.release

@@ -0,0 +1,3 @@
+CONFIG_COMPILER_OPTIMIZATION_SIZE=y
+CONFIG_BOOTLOADER_COMPILER_OPTIMIZATION_SIZE=y
+CONFIG_COMPILER_OPTIMIZATION_ASSERTIONS_SILENT=y

components/spi_flash/test_apps/esp_flash_stress/sdkconfig.defaults

@@ -0,0 +1,4 @@
+CONFIG_ESP_TASK_WDT_EN=n
+CONFIG_PARTITION_TABLE_CUSTOM=y
+CONFIG_PARTITION_TABLE_CUSTOM_FILENAME="partitions.csv"
+CONFIG_PARTITION_TABLE_FILENAME="partitions.csv"