Skip to content
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.

Sign up for GitHub

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

fs: nvs: Add recovery path for corrupted NVS #83128

Merged
merged 2 commits into from
Dec 23, 2024
Merged

fs: nvs: Add recovery path for corrupted NVS #83128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
310e5f6
fs: nvs: Add recovery path for corrupted NVS
corymayer Dec 21, 2024
5fa5854
tests: subsys/fs/nvs: add test for initializing bad region
corymayer Dec 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions subsys/fs/nvs/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@ config NVS_DATA_CRC
The CRC-32 is transparently stored at the end of the data field,
in the NVS data section, so 4 more bytes are needed per NVS element.

config NVS_INIT_BAD_MEMORY_REGION
bool "Non-volatile Storage bad memory region recovery"
help
Enable automatic initialization of a NVS on a memory region that does
not contain a valid NVS. A region containing an invalid NVS can be
caused by corruption or by providing a non-empty region. This option
ensures a new NVS can be created.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming here is confusing. You are talking about region but in the description you are referring to "bad partition recovery"
AFAIK the term "Region" doesn't seem to be used anywhere in NVS.
NVS knows only about Sectors of data
Could be NVS_INIT_BAD_SECTORS

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The terminology region was carefully chosen not to overlap with partition (as defined in dts), area (as used in flash_area_... routines, there also are no bad sectors (which would be used by nand devices).

@corymayer, please update the bool to "Non-volatile Storage bad region recovery".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Laczen So maybe it is better to use the terminology "memory region" instead of just "region"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Memory region" is also existing terminology.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"memory region" is used to describe a part of the memory. This is what this PR is about, right ? Am I missing something ? Or do you mean that the "memory region" terminology is reserved for other purposes in zephyr ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything is existing terminology, so context matters. If something does not exist in a given context than it can be used to describe some characteristic in that context. And NAND actually uses Bad Block Management, for example winbond devices, to manage sectors in pages, which is again vendor specific nomenclature, because sectors/pages/block naming varies between devices.

So, Region or Memory region, when defined within context of NVS as a specific offset/range tuple may be fine, as long as it's relation to sectors or partitions, as used by NVS, is clear.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"memory region" is used in dts for linker memory regions, in user mode for memory domains... It is in no way reserved exclusively for these purposes but might be confusing to use it here as some people associate memory with RAM.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case I prefer "memory region" which covers (flash/ram/... regions) instead of just "region"
but that's not a blocker for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the name to "bad memory region" and added a unit test


module = NVS
module-str = nvs
source "subsys/logging/Kconfig.template.log_config"
Expand Down
15 changes: 14 additions & 1 deletion subsys/fs/nvs/nvs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -781,10 +781,23 @@ static int nvs_startup(struct nvs_fs *fs)
}
}
}
/* all sectors are closed, this is not a nvs fs */
/* all sectors are closed, this is not a nvs fs or irreparably corrupted */
if (closed_sectors == fs->sector_count) {
#ifdef CONFIG_NVS_INIT_BAD_MEMORY_REGION
LOG_WRN("All sectors closed, erasing all sectors...");
rc = flash_flatten(fs->flash_device, fs->offset,
fs->sector_size * fs->sector_count);
if (rc) {
goto end;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please insert a space after this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line was removed


i = fs->sector_count;
addr = ((fs->sector_count - 1) << ADDR_SECT_SHIFT) +
(uint16_t)(fs->sector_size - ate_size);
#else
rc = -EDEADLK;
goto end;
#endif
}

if (i == fs->sector_count) {
Expand Down
1 change: 1 addition & 0 deletions tests/subsys/fs/nvs/prj.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ CONFIG_FLASH_MAP=y
CONFIG_NVS=y
CONFIG_LOG=y
CONFIG_NVS_LOG_LEVEL_DBG=y
CONFIG_NVS_INIT_BAD_MEMORY_REGION=y
37 changes: 37 additions & 0 deletions tests/subsys/fs/nvs/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -959,3 +959,40 @@ ZTEST_F(nvs, test_nvs_cache_hash_quality)

#endif
}

/*
* Test NVS bad region initialization recovery.
*/
ZTEST_F(nvs, test_nvs_init_bad_memory_region)
{
int err;
uint32_t data;

err = nvs_mount(&fixture->fs);
zassert_true(err == 0, "nvs_mount call failure: %d", err);

/* Write bad ATE to each sector */
for (uint16_t i = 0; i < TEST_SECTOR_COUNT; i++) {
data = 0xdeadbeef;
err = flash_write(fixture->fs.flash_device,
fixture->fs.offset + (fixture->fs.sector_size * (i + 1)) -
sizeof(struct nvs_ate),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT, I think this alignment might still be considered incorrect.

&data, sizeof(data));
zassert_true(err == 0, "flash_write failed: %d", err);
}

/* Reinitialize the NVS. */
memset(&fixture->fs, 0, sizeof(fixture->fs));
(void)setup();

#ifdef CONFIG_NVS_INIT_BAD_MEMORY_REGION
err = nvs_mount(&fixture->fs);
zassert_true(err == 0, "nvs_mount call failure: %d", err);

/* Ensure that the NVS is able to store new content. */
execute_long_pattern_write(TEST_DATA_ID, &fixture->fs);
#else
err = nvs_mount(&fixture->fs);
zassert_true(err == -EDEADLK, "nvs_mount call ok, expect fail: %d", err);
#endif
}
Loading