From 46b5363e392d4cc83ef681565df8fadcd5cb8bbb Mon Sep 17 00:00:00 2001 From: "Michael (XIAO Xufeng)" Date: Wed, 13 Apr 2022 17:23:57 +0800 Subject: [PATCH] spi_flash: forbid writing to main flash when using invalid init arguments Also refactored the init code to make the logic of device (CS) acquiring more centralized. Resolves: https://github.com/espressif/esp-idf/issues/8556 --- components/spi_flash/esp_flash_spi_init.c | 78 +++++++++++++++---- .../spi_flash/include/esp_flash_internal.h | 28 +++---- components/spi_flash/spi_flash_os_func_app.c | 46 ++++------- tools/ci/check_copyright_ignore.txt | 1 - 4 files changed, 89 insertions(+), 64 deletions(-) diff --git a/components/spi_flash/esp_flash_spi_init.c b/components/spi_flash/esp_flash_spi_init.c index 4dcb5e29b2..a8b650d18a 100644 --- a/components/spi_flash/esp_flash_spi_init.c +++ b/components/spi_flash/esp_flash_spi_init.c @@ -166,6 +166,52 @@ static IRAM_ATTR NOINLINE_ATTR void cs_initialize(esp_flash_t *chip, const esp_f chip->os_func->end(chip->os_func_data); } +static bool use_bus_lock(int host_id) +{ + if (host_id != SPI1_HOST) { + return true; + } +#if CONFIG_SPI_FLASH_SHARE_SPI1_BUS + return true; +#else + return false; +#endif +} + +static esp_err_t acquire_spi_device(const esp_flash_spi_device_config_t *config, int* out_dev_id, spi_bus_lock_dev_handle_t* out_dev_handle) +{ + esp_err_t ret = ESP_OK; + int dev_id = -1; + spi_bus_lock_dev_handle_t dev_handle = NULL; + + if (use_bus_lock(config->host_id)) { + spi_bus_lock_handle_t lock = spi_bus_lock_get_by_id(config->host_id); + spi_bus_lock_dev_config_t config = {.flags = SPI_BUS_LOCK_DEV_FLAG_CS_REQUIRED}; + + ret = spi_bus_lock_register_dev(lock, &config, &dev_handle); + if (ret == ESP_OK) { + dev_id = spi_bus_lock_get_dev_id(dev_handle); + } else if (ret == ESP_ERR_NOT_SUPPORTED) { + ESP_LOGE(TAG, "No free CS."); + } else if (ret == ESP_ERR_INVALID_ARG) { + ESP_LOGE(TAG, "Bus lock not initialized (check CONFIG_SPI_FLASH_SHARE_SPI1_BUS)."); + } + } else { + const bool is_main_flash = (config->host_id == SPI1_HOST && config->cs_id == 0); + if (config->cs_id >= SOC_SPI_PERIPH_CS_NUM(config->host_id) || config->cs_id < 0 || is_main_flash) { + ESP_LOGE(TAG, "Not valid CS."); + ret = ESP_ERR_INVALID_ARG; + } else { + dev_id = config->cs_id; + assert(dev_handle == NULL); + } + } + + *out_dev_handle = dev_handle; + *out_dev_id = dev_id; + return ret; +} + esp_err_t spi_bus_add_flash_device(esp_flash_t **out_chip, const esp_flash_spi_device_config_t *config) { if (out_chip == NULL) { @@ -197,25 +243,22 @@ esp_err_t spi_bus_add_flash_device(esp_flash_t **out_chip, const esp_flash_spi_d goto fail; } - int dev_id = -1; - esp_err_t err = esp_flash_init_os_functions(chip, config->host_id, &dev_id); - if (err == ESP_ERR_NOT_SUPPORTED) { - ESP_LOGE(TAG, "Init os functions failed! No free CS."); - } else if (err == ESP_ERR_INVALID_ARG) { - ESP_LOGE(TAG, "Init os functions failed! Bus lock not initialized (check CONFIG_SPI_FLASH_SHARE_SPI1_BUS)."); - } + int dev_id; + spi_bus_lock_dev_handle_t dev_handle; + esp_err_t err = acquire_spi_device(config, &dev_id, &dev_handle); if (err != ESP_OK) { ret = err; goto fail; } - // When `CONFIG_SPI_FLASH_SHARE_SPI1_BUS` is not enabled on SPI1 bus, the - // `esp_flash_init_os_functions` will not be able to assign a new device ID. In this case, we - // use the `cs_id` in the config structure. - if (dev_id == -1 && config->host_id == SPI1_HOST) { - dev_id = config->cs_id; - } - assert(dev_id < SOC_SPI_PERIPH_CS_NUM(config->host_id) && dev_id >= 0); + err = esp_flash_init_os_functions(chip, config->host_id, dev_handle); + if (err != ESP_OK) { + ret = err; + goto fail; + } + + //avoid conflicts with main flash + assert(config->host_id != SPI1_HOST || dev_id != 0); bool use_iomux = spicommon_bus_using_iomux(config->host_id); memspi_host_config_t host_cfg = { .host_id = config->host_id, @@ -245,7 +288,12 @@ esp_err_t spi_bus_remove_flash_device(esp_flash_t *chip) if (chip==NULL) { return ESP_ERR_INVALID_ARG; } - esp_flash_deinit_os_functions(chip); + + spi_bus_lock_dev_handle_t dev_handle = NULL; + esp_flash_deinit_os_functions(chip, &dev_handle); + if (dev_handle) { + spi_bus_lock_unregister_dev(dev_handle); + } free(chip->host); free(chip); return ESP_OK; diff --git a/components/spi_flash/include/esp_flash_internal.h b/components/spi_flash/include/esp_flash_internal.h index 1ef3ed1eaf..1ab1f49a05 100644 --- a/components/spi_flash/include/esp_flash_internal.h +++ b/components/spi_flash/include/esp_flash_internal.h @@ -1,16 +1,8 @@ -// Copyright 2015-2019 Espressif Systems (Shanghai) PTE LTD -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. +/* + * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ #pragma once #include "esp_err.h" @@ -69,21 +61,23 @@ esp_err_t esp_flash_app_disable_protect(bool disable); * * @param chip The chip to init os functions. * @param host_id Which SPI host to use, 1 for SPI1, 2 for SPI2 (HSPI), 3 for SPI3 (VSPI) - * @param out_dev_id Output of occupied device slot + * @param dev_handle SPI bus lock device handle to acquire during flash operations * * @return * - ESP_OK if success * - ESP_ERR_INVALID_ARG if host_id is invalid */ -esp_err_t esp_flash_init_os_functions(esp_flash_t *chip, int host_id, int *out_dev_id); +esp_err_t esp_flash_init_os_functions(esp_flash_t *chip, int host_id, spi_bus_lock_dev_handle_t dev_handle); /** * @brief Deinitialize OS-level functions * - * @param chip The chip to deinit os functions + * @param chip The chip to deinit os functions + * @param out_dev_handle The SPI bus lock passed from `esp_flash_init_os_functions`. The caller should deinitialize + * the lock. * @return always ESP_OK. */ -esp_err_t esp_flash_deinit_os_functions(esp_flash_t* chip); +esp_err_t esp_flash_deinit_os_functions(esp_flash_t* chip, spi_bus_lock_dev_handle_t* out_dev_handle); /** * @brief Initialize the bus lock on the SPI1 bus. Should be called if drivers (including esp_flash) diff --git a/components/spi_flash/spi_flash_os_func_app.c b/components/spi_flash/spi_flash_os_func_app.c index 410a8bc6d3..94a2e5089c 100644 --- a/components/spi_flash/spi_flash_os_func_app.c +++ b/components/spi_flash/spi_flash_os_func_app.c @@ -210,30 +210,22 @@ static const esp_flash_os_functions_t esp_flash_spi23_default_os_functions = { .yield = NULL, }; -static spi_bus_lock_dev_handle_t register_dev(int host_id) +static bool use_bus_lock(int host_id) { - spi_bus_lock_handle_t lock = spi_bus_lock_get_by_id(host_id); - spi_bus_lock_dev_handle_t dev_handle; - spi_bus_lock_dev_config_t config = {.flags = SPI_BUS_LOCK_DEV_FLAG_CS_REQUIRED}; - esp_err_t err = spi_bus_lock_register_dev(lock, &config, &dev_handle); - if (err != ESP_OK) { - return NULL; + if (host_id != SPI1_HOST) { + return true; } - return dev_handle; +#if CONFIG_SPI_FLASH_SHARE_SPI1_BUS + return true; +#else + return false; +#endif } -esp_err_t esp_flash_init_os_functions(esp_flash_t *chip, int host_id, int* out_dev_id) +esp_err_t esp_flash_init_os_functions(esp_flash_t *chip, int host_id, spi_bus_lock_dev_handle_t dev_handle) { - spi_bus_lock_dev_handle_t dev_handle = NULL; - - // Skip initializing the bus lock when the bus is SPI1 and the bus is not shared with SPI Master - // driver, leaving dev_handle = NULL - bool skip_register_dev = (host_id == SPI1_HOST); -#if CONFIG_SPI_FLASH_SHARE_SPI1_BUS - skip_register_dev = false; -#endif - if (!skip_register_dev) { - dev_handle = register_dev(host_id); + if (use_bus_lock(host_id) && !dev_handle) { + return ESP_ERR_INVALID_ARG; } if (host_id == SPI1_HOST) { @@ -259,28 +251,20 @@ esp_err_t esp_flash_init_os_functions(esp_flash_t *chip, int host_id, int* out_d return ESP_ERR_NO_MEM; } *(app_func_arg_t*) chip->os_func_data = (app_func_arg_t) { - .dev_lock = dev_handle, + .dev_lock = dev_handle, }; } else { return ESP_ERR_INVALID_ARG; } - // Bus lock not initialized, the device ID should be directly given by application. - if (dev_handle) { - *out_dev_id = spi_bus_lock_get_dev_id(dev_handle); - } - return ESP_OK; } -esp_err_t esp_flash_deinit_os_functions(esp_flash_t* chip) +esp_err_t esp_flash_deinit_os_functions(esp_flash_t* chip, spi_bus_lock_dev_handle_t* out_dev_handle) { if (chip->os_func_data) { - spi_bus_lock_dev_handle_t dev_lock = ((app_func_arg_t*)chip->os_func_data)->dev_lock; - // SPI bus lock is possible not used on SPI1 bus - if (dev_lock) { - spi_bus_lock_unregister_dev(dev_lock); - } + // SPI bus lock is possibly not used on SPI1 bus + *out_dev_handle = ((app_func_arg_t*)chip->os_func_data)->dev_lock; free(chip->os_func_data); } chip->os_func = NULL; diff --git a/tools/ci/check_copyright_ignore.txt b/tools/ci/check_copyright_ignore.txt index 965e64a6ad..7d914a1aeb 100644 --- a/tools/ci/check_copyright_ignore.txt +++ b/tools/ci/check_copyright_ignore.txt @@ -1611,7 +1611,6 @@ components/soc/lldesc.c components/soc/soc_include_legacy_warn.c components/spi_flash/cache_utils.h components/spi_flash/include/esp_flash.h -components/spi_flash/include/esp_flash_internal.h components/spi_flash/include/esp_flash_spi_init.h components/spi_flash/include/esp_spi_flash.h components/spi_flash/include/esp_spi_flash_counters.h