-
Notifications
You must be signed in to change notification settings - Fork 7.6k
subsystem: settings: its: Add ITS backend #87778
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,5 +14,6 @@ supported: | |
- counter | ||
- pwm | ||
- spi | ||
- its | ||
ram: 192 | ||
flash: 576 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,284 @@ | ||
/* | ||
* Copyright (c) 2019 Laczen | ||
* Copyright (c) 2019 Nordic Semiconductor ASA | ||
* Copyright (c) 2025 Analog Devices, Inc. | ||
* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
#include <psa/internal_trusted_storage.h> | ||
#include <zephyr/settings/settings.h> | ||
#include <zephyr/kernel.h> | ||
#include <zephyr/logging/log.h> | ||
#include <zephyr/psa/key_ids.h> | ||
|
||
LOG_MODULE_DECLARE(settings, CONFIG_SETTINGS_LOG_LEVEL); | ||
|
||
K_MUTEX_DEFINE(worker_mutex); | ||
static struct k_work_delayable worker; | ||
|
||
struct setting_entry { | ||
char name[SETTINGS_MAX_NAME_LEN]; | ||
char value[SETTINGS_MAX_VAL_LEN]; | ||
size_t val_len; | ||
}; | ||
|
||
static struct setting_entry entries[CONFIG_SETTINGS_TFM_ITS_NUM_ENTRIES]; | ||
static int entries_count; | ||
|
||
static int settings_its_load(struct settings_store *cs, const struct settings_load_arg *arg); | ||
static int settings_its_save(struct settings_store *cs, const char *name, const char *value, | ||
size_t val_len); | ||
|
||
static const struct settings_store_itf settings_its_itf = { | ||
.csi_load = settings_its_load, | ||
.csi_save = settings_its_save, | ||
}; | ||
|
||
static struct settings_store default_settings_its = {.cs_itf = &settings_its_itf}; | ||
seankyer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
static int increment_uid(psa_storage_uid_t *uid) | ||
{ | ||
if (*uid < ZEPHYR_PSA_SETTINGS_TFM_ITS_KEY_ID_RANGE_BEGIN || | ||
*uid >= (ZEPHYR_PSA_SETTINGS_TFM_ITS_KEY_ID_RANGE_BEGIN + | ||
ZEPHYR_PSA_SETTINGS_TFM_ITS_KEY_ID_RANGE_SIZE) - 1) { | ||
LOG_ERR("UID out of range!"); | ||
return -EINVAL; | ||
} | ||
|
||
(*uid)++; | ||
return 0; | ||
} | ||
|
||
static int store_entries(void) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, IIUC you're writing all the entries to the ITS again every time a single value is updated. The flash won't like this, especially if there is any settings user which updates values frequently (which in my understanding there is). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with your point however I'll add some justification: As a reminder about implementation, I am compacting all settings into a single struct and trying to cram as many settings as possible into as few ITS nodes as possible. My priority is to optimize the number of ITS nodes I consume, as these are costly to allocate.
I think since this is already not a very likely case given a typical Bluetooth device, that I do not want to support this in this PR. If it becomes a performance issue in the future, could it instead be addressed as a future improvement? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the 1024-byte UID always the case? Doesn't an entry size depend on the TF-M configuration? (not sure) Sure, this could be improved later. Maybe add a warning or a note regarding that e.g. in the Kconfig option help? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the TF-M configuration controls this. So if a user wants to adjust it to something bigger or smaller, that is part of their design. I'll add a note about this behavior to the Kconfig help section. |
||
{ | ||
int err; | ||
psa_status_t status; | ||
psa_storage_uid_t uid = ZEPHYR_PSA_SETTINGS_TFM_ITS_KEY_ID_RANGE_BEGIN; | ||
size_t remaining = sizeof(entries); | ||
size_t chunk_size = CONFIG_TFM_ITS_MAX_ASSET_SIZE; | ||
const uint8_t *metadata_bytes = (const uint8_t *)&entries; | ||
|
||
/* | ||
* Each ITS UID is treated like a sector. Data is written to each ITS node until | ||
* that node is full, before incrementing the UID. This is done to minimize the | ||
* number of allocated ITS nodes and to avoid wasting allocated bytes. | ||
*/ | ||
while (remaining > 0) { | ||
seankyer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
size_t write_size = (remaining > chunk_size) ? chunk_size : remaining; | ||
|
||
status = psa_its_set(uid, write_size, metadata_bytes, PSA_STORAGE_FLAG_NONE); | ||
if (status) { | ||
LOG_ERR("Error storing %d bytes of metadata! Bytes Remaining: %d, status: " | ||
"%d", | ||
write_size, remaining, status); | ||
return status; | ||
} | ||
|
||
metadata_bytes += write_size; | ||
remaining -= write_size; | ||
err = increment_uid(&uid); | ||
if (err) { | ||
return err; | ||
} | ||
} | ||
|
||
LOG_DBG("ITS entries stored successfully - bytes_saved: %d num_entries: %d max_uid: %lld", | ||
sizeof(entries), entries_count, uid); | ||
|
||
return 0; | ||
} | ||
|
||
static int load_entries(void) | ||
{ | ||
int err; | ||
psa_status_t status; | ||
size_t bytes_read; | ||
psa_storage_uid_t uid = ZEPHYR_PSA_SETTINGS_TFM_ITS_KEY_ID_RANGE_BEGIN; | ||
size_t remaining = sizeof(entries); | ||
size_t chunk_size = CONFIG_TFM_ITS_MAX_ASSET_SIZE; | ||
uint8_t *metadata_bytes = (uint8_t *)&entries; | ||
|
||
/* | ||
* Each ITS UID is treated like a sector. Data is written to each ITS node until | ||
* that node is full, before incrementing the UID. This is done to minimize the | ||
* number of allocated ITS nodes and to avoid wasting allocated bytes. | ||
*/ | ||
while (remaining > 0) { | ||
size_t to_read = (remaining > chunk_size) ? chunk_size : remaining; | ||
|
||
status = psa_its_get(uid, 0, to_read, metadata_bytes, &bytes_read); | ||
if (status) { | ||
return status; | ||
} | ||
|
||
metadata_bytes += bytes_read; | ||
remaining -= bytes_read; | ||
err = increment_uid(&uid); | ||
if (err) { | ||
return err; | ||
} | ||
} | ||
|
||
for (int i = 0; i < CONFIG_SETTINGS_TFM_ITS_NUM_ENTRIES; i++) { | ||
if (strnlen(entries[i].name, SETTINGS_MAX_NAME_LEN) != 0) { | ||
entries_count++; | ||
} | ||
} | ||
|
||
LOG_DBG("ITS entries restored successfully - bytes_loaded: %d, num_entries: %d", | ||
sizeof(entries), entries_count); | ||
|
||
return 0; | ||
} | ||
|
||
/* void *back_end is the index of the entry in metadata entries struct */ | ||
static ssize_t settings_its_read_fn(void *back_end, void *data, size_t len) | ||
{ | ||
LOG_DBG("ITS Read - index: %d", *(int *)back_end); | ||
|
||
if (*(int *)back_end < 0 || *(int *)back_end >= CONFIG_SETTINGS_TFM_ITS_NUM_ENTRIES) { | ||
LOG_ERR("Invalid index %d in ITS metadata", *(int *)back_end); | ||
return 0; | ||
} | ||
|
||
memcpy(data, entries[*(int *)back_end].value, len); | ||
seankyer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/* | ||
* Callback expects return value of the number of bytes read | ||
*/ | ||
return entries[*(int *)back_end].val_len; | ||
} | ||
|
||
static int settings_its_load(struct settings_store *cs, const struct settings_load_arg *arg) | ||
{ | ||
int ret; | ||
|
||
for (int i = 0; i < entries_count; i++) { | ||
|
||
if (strnlen(entries[i].name, SETTINGS_MAX_NAME_LEN) != 0) { | ||
|
||
/* | ||
* Pass the key to the settings handler with it's index as an argument, | ||
* to be read during callback function later. | ||
*/ | ||
ret = settings_call_set_handler(entries[i].name, entries[i].val_len, | ||
settings_its_read_fn, (void *)&i, | ||
(void *)arg); | ||
if (ret) { | ||
return ret; | ||
} | ||
} | ||
} | ||
|
||
return 0; | ||
} | ||
|
||
static int settings_its_save(struct settings_store *cs, const char *name, const char *value, | ||
size_t val_len) | ||
{ | ||
if (entries_count >= CONFIG_SETTINGS_TFM_ITS_NUM_ENTRIES) { | ||
LOG_ERR("%s: Max settings reached: %d", __func__, | ||
CONFIG_SETTINGS_TFM_ITS_NUM_ENTRIES); | ||
return -ENOMEM; | ||
} | ||
|
||
if (val_len > SETTINGS_MAX_VAL_LEN) { | ||
LOG_ERR("%s: Invalid settings size - val_len: %d", __func__, val_len); | ||
return -EINVAL; | ||
} | ||
|
||
int index; | ||
bool delete; | ||
|
||
/* Find out if we are doing a delete */ | ||
delete = ((value == NULL) || (val_len == 0)); | ||
|
||
/* Lock mutex before manipulating settings array */ | ||
k_mutex_lock(&worker_mutex, K_FOREVER); | ||
|
||
/* | ||
* Search metadata to see if entry already exists. Array is compacted, so first blank entry | ||
* signals end of settings. | ||
*/ | ||
for (index = 0; index < CONFIG_SETTINGS_TFM_ITS_NUM_ENTRIES; index++) { | ||
if (strncmp(entries[index].name, name, SETTINGS_MAX_NAME_LEN) == 0) { | ||
break; | ||
} else if (entries[index].val_len == 0) { | ||
|
||
/* Setting already deleted */ | ||
if (delete) { | ||
LOG_DBG("%s: %s Already deleted!", __func__, name); | ||
k_mutex_unlock(&worker_mutex); | ||
return 0; | ||
} | ||
|
||
/* New setting being entered */ | ||
entries_count++; | ||
break; | ||
} | ||
} | ||
|
||
LOG_DBG("ITS Save - index %d: name %s, val_len %d", index, name, val_len); | ||
|
||
if (delete) { | ||
/* Clear metadata */ | ||
memset(entries[index].name, 0, SETTINGS_MAX_NAME_LEN); | ||
memset(entries[index].value, 0, SETTINGS_MAX_VAL_LEN); | ||
entries[index].val_len = 0; | ||
|
||
/* If setting not at end of array, shift entries */ | ||
if (index < entries_count - 1) { | ||
memcpy(&entries[index], &entries[index + 1], | ||
(entries_count - index - 1) * sizeof(struct setting_entry)); | ||
/* Remove duplicate entry at end of array */ | ||
memset(&entries[entries_count - 1], 0, sizeof(struct setting_entry)); | ||
} | ||
|
||
entries_count--; | ||
} else { | ||
/* Update metadata */ | ||
strncpy(entries[index].name, name, SETTINGS_MAX_NAME_LEN); | ||
memcpy(entries[index].value, value, val_len); | ||
entries[index].val_len = val_len; | ||
} | ||
|
||
k_mutex_unlock(&worker_mutex); | ||
k_work_schedule(&worker, K_MSEC(CONFIG_SETTINGS_TFM_ITS_LAZY_PERSIST_DELAY_MS)); | ||
|
||
return 0; | ||
} | ||
|
||
void worker_persist_entries_struct_fn(struct k_work *work) | ||
{ | ||
k_mutex_lock(&worker_mutex, K_FOREVER); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this usage of the mutex is enough. It does not prevent There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thank you |
||
store_entries(); | ||
k_mutex_unlock(&worker_mutex); | ||
} | ||
|
||
int settings_backend_init(void) | ||
{ | ||
psa_status_t status; | ||
|
||
/* Load ITS metadata */ | ||
status = load_entries(); | ||
|
||
/* If resource DNE, we need to allocate it */ | ||
if (status == PSA_ERROR_DOES_NOT_EXIST) { | ||
status = store_entries(); | ||
if (status) { | ||
LOG_ERR("Error storing metadata in %s: (status %d)", __func__, status); | ||
return -EIO; | ||
} | ||
} else if (status) { | ||
LOG_ERR("Error loading metadata in %s: (status %d)", __func__, status); | ||
return -EIO; | ||
} | ||
|
||
settings_dst_register(&default_settings_its); | ||
settings_src_register(&default_settings_its); | ||
|
||
k_work_init_delayable(&worker, worker_persist_entries_struct_fn); | ||
|
||
return 0; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
# Copyright (c) 2025 Analog Devices, Inc. | ||
# | ||
# SPDX-License-Identifier: Apache-2.0 | ||
|
||
cmake_minimum_required(VERSION 3.20.0) | ||
find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE}) | ||
project(test_settings_its_raw) | ||
|
||
add_subdirectory(./src its_test_bindir) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
# Copyright (c) 2025 Analog Devices, Inc. | ||
# | ||
# SPDX-License-Identifier: Apache-2.0 | ||
|
||
CONFIG_ZTEST=y | ||
CONFIG_FLASH=y | ||
CONFIG_FLASH_MAP=y | ||
CONFIG_TFM_PARTITION_INTERNAL_TRUSTED_STORAGE=y | ||
CONFIG_TFM_ITS_MAX_ASSET_SIZE_OVERRIDE=y | ||
CONFIG_TFM_ITS_MAX_ASSET_SIZE=1024 | ||
CONFIG_SETTINGS=y | ||
CONFIG_SETTINGS_RUNTIME=y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A note about the RAM consumption is missing: #87778 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it to the
SETTINGS_TFM_ITS_NUM_ENTRIES
Kconfig help section https://github.com/zephyrproject-rtos/zephyr/pull/87778/files/7a25bd6f81682de166513b2c9670fc5ab689a90f#diff-db8ba35bdefd7627bfa4c2a0d7d13c9d1f20787ddf7181333981f6c01a32cc34R247 should it be moved? Is the wording ok?