From c4eaf865161706bdd5fa62a98ec72688f65797fe Mon Sep 17 00:00:00 2001 From: "radek.tandler" Date: Wed, 17 Jan 2024 10:44:50 +0100 Subject: [PATCH] fix(nvs): Improved lockig mechanism for initialization phase --- components/nvs_flash/CMakeLists.txt | 5 +- components/nvs_flash/src/nvs_api.cpp | 10 +-- components/nvs_flash/src/nvs_platform.cpp | 50 +++++++++++ components/nvs_flash/src/nvs_platform.hpp | 92 ++++----------------- components/nvs_flash/src/nvs_storage.cpp | 3 +- components/nvs_flash/test_nvs_host/Makefile | 1 + tools/ci/check_copyright_ignore.txt | 1 - 7 files changed, 78 insertions(+), 84 deletions(-) create mode 100644 components/nvs_flash/src/nvs_platform.cpp diff --git a/components/nvs_flash/CMakeLists.txt b/components/nvs_flash/CMakeLists.txt index d54703ce9b..270f37ce49 100644 --- a/components/nvs_flash/CMakeLists.txt +++ b/components/nvs_flash/CMakeLists.txt @@ -11,11 +11,12 @@ set(srcs "src/nvs_api.cpp" "src/nvs_partition.cpp" "src/nvs_partition_lookup.cpp" "src/nvs_partition_manager.cpp" - "src/nvs_types.cpp") + "src/nvs_types.cpp" + "src/nvs_platform.cpp") idf_component_register(SRCS "${srcs}" REQUIRES "esp_partition" - PRIV_REQUIRES spi_flash + PRIV_REQUIRES spi_flash newlib INCLUDE_DIRS "include" "../spi_flash/include" PRIV_INCLUDE_DIRS "private_include") diff --git a/components/nvs_flash/src/nvs_api.cpp b/components/nvs_flash/src/nvs_api.cpp index d349aea466..effed329dd 100644 --- a/components/nvs_flash/src/nvs_api.cpp +++ b/components/nvs_flash/src/nvs_api.cpp @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -51,10 +51,6 @@ uint32_t NVSHandleEntry::s_nvs_next_handle; extern "C" void nvs_dump(const char *partName); -#ifndef LINUX_TARGET -SemaphoreHandle_t nvs::Lock::mSemaphore = nullptr; -#endif // ! LINUX_TARGET - using namespace std; using namespace nvs; @@ -268,6 +264,10 @@ static esp_err_t nvs_find_ns_handle(nvs_handle_t c_handle, NVSHandleSimple** han extern "C" esp_err_t nvs_open_from_partition(const char *part_name, const char* namespace_name, nvs_open_mode_t open_mode, nvs_handle_t *out_handle) { + esp_err_t lock_result = Lock::init(); + if (lock_result != ESP_OK) { + return lock_result; + } Lock lock; ESP_LOGD(TAG, "%s %s %d", __func__, namespace_name, open_mode); diff --git a/components/nvs_flash/src/nvs_platform.cpp b/components/nvs_flash/src/nvs_platform.cpp new file mode 100644 index 0000000000..045996bccd --- /dev/null +++ b/components/nvs_flash/src/nvs_platform.cpp @@ -0,0 +1,50 @@ +/* + * SPDX-FileCopyrightText: 2024 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ +#include "nvs_platform.hpp" + +using namespace nvs; + +#ifdef LINUX_TARGET +Lock::Lock() {} +Lock::~Lock() {} +esp_err_t nvs::Lock::init() {return ESP_OK;} +void Lock::uninit() {} +#else + +#include "sys/lock.h" + +Lock::Lock() +{ + // Newlib implementation ensures that even if mSemaphore was 0, it gets initialized. + // Locks mSemaphore + _lock_acquire(&mSemaphore); +} + +Lock::~Lock() +{ + // Unlocks mSemaphore + _lock_release(&mSemaphore); +} + +esp_err_t Lock::init() +{ + // Let postpone initialization to the Lock::Lock. + // It is designed to lazy initialize the semaphore in a properly guarded critical section + return ESP_OK; +} + +void Lock::uninit() +{ + // Uninitializes mSemaphore. Please be aware that uninitialization of semaphore shared and held by another thread + // can cause undefined behavior + if (mSemaphore) { + _lock_close(&mSemaphore); + } +} + +_lock_t Lock::mSemaphore = 0; + +#endif diff --git a/components/nvs_flash/src/nvs_platform.hpp b/components/nvs_flash/src/nvs_platform.hpp index c47fa40020..3c9275aa74 100644 --- a/components/nvs_flash/src/nvs_platform.hpp +++ b/components/nvs_flash/src/nvs_platform.hpp @@ -1,82 +1,24 @@ -// Copyright 2015-2016 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-2024 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ #pragma once -#ifdef LINUX_TARGET -namespace nvs -{ -class Lock -{ -public: - Lock() { } - ~Lock() { } - static esp_err_t init() - { - return ESP_OK; - } - - static void uninit() {} -}; -} // namespace nvs - -#else // LINUX_TARGET - -#include "freertos/FreeRTOS.h" -#include "freertos/semphr.h" +#include "esp_err.h" namespace nvs { - -class Lock -{ -public: - Lock() + class Lock { - if (mSemaphore) { - xSemaphoreTake(mSemaphore, portMAX_DELAY); - } - } - - ~Lock() - { - if (mSemaphore) { - xSemaphoreGive(mSemaphore); - } - } - - static esp_err_t init() - { - if (mSemaphore) { - return ESP_OK; - } - mSemaphore = xSemaphoreCreateMutex(); - if (!mSemaphore) { - return ESP_ERR_NO_MEM; - } - return ESP_OK; - } - - static void uninit() - { - if (mSemaphore) { - vSemaphoreDelete(mSemaphore); - } - mSemaphore = nullptr; - } - - static SemaphoreHandle_t mSemaphore; -}; + public: + Lock(); + ~Lock(); + static esp_err_t init(); + static void uninit(); +#ifndef LINUX_TARGET + private: + static _lock_t mSemaphore; +#endif + }; } // namespace nvs - -#endif // LINUX_TARGET diff --git a/components/nvs_flash/src/nvs_storage.cpp b/components/nvs_flash/src/nvs_storage.cpp index 3ce7e1be60..c1fcc3c9c4 100644 --- a/components/nvs_flash/src/nvs_storage.cpp +++ b/components/nvs_flash/src/nvs_storage.cpp @@ -210,7 +210,6 @@ esp_err_t Storage::init(uint32_t baseSector, uint32_t sectorCount) if (mNamespaceUsage.set(255, true) != ESP_OK) { return ESP_FAIL; } - mState = StorageState::ACTIVE; // Populate list of multi-page index entries. TBlobIndexList blobIdxList; @@ -229,6 +228,8 @@ esp_err_t Storage::init(uint32_t baseSector, uint32_t sectorCount) // Purge the blob index list blobIdxList.clearAndFreeNodes(); + mState = StorageState::ACTIVE; + #ifdef DEBUG_STORAGE debugCheck(); #endif diff --git a/components/nvs_flash/test_nvs_host/Makefile b/components/nvs_flash/test_nvs_host/Makefile index 0ac5df21fc..044a39e72a 100644 --- a/components/nvs_flash/test_nvs_host/Makefile +++ b/components/nvs_flash/test_nvs_host/Makefile @@ -15,6 +15,7 @@ SOURCE_FILES = \ nvs_partition.cpp \ nvs_encrypted_partition.cpp \ nvs_cxx_api.cpp \ + nvs_platform.cpp \ ) \ spi_flash_emulation.cpp \ test_compressed_enum_table.cpp \ diff --git a/tools/ci/check_copyright_ignore.txt b/tools/ci/check_copyright_ignore.txt index c01b3b48f7..8cabaeb70f 100644 --- a/tools/ci/check_copyright_ignore.txt +++ b/tools/ci/check_copyright_ignore.txt @@ -583,7 +583,6 @@ components/nvs_flash/src/nvs_item_hash_list.cpp components/nvs_flash/src/nvs_pagemanager.hpp components/nvs_flash/src/nvs_partition_lookup.cpp components/nvs_flash/src/nvs_partition_lookup.hpp -components/nvs_flash/src/nvs_platform.hpp components/nvs_flash/src/nvs_test_api.h components/nvs_flash/src/nvs_types.cpp components/nvs_flash/test_nvs_host/main.cpp