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

corymayer · 2024-12-17T18:15:54Z

When all sectors of the settings partition have some data that is not 0xFF, NVS will fail to initialize settings. Add a recovery path to erase the whole settings partition and recover. Should not happen unless there's some serious flash corruption, or possibly interrupted garbage collection. Verified by recreating the scenario with flash CLIs.

github-actions · 2024-12-17T18:16:39Z

Hello @corymayer, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

Laczen · 2024-12-17T19:13:33Z

Hi @corymayer, this is a nice addition. Thank you for the proposal.

If you agree I would like to see this possibility enabled by a Kconfig option.

I don't think the change in comment is required, when this problem occurs it is not by definition a corrupt NVS system, it can also be a non NVS partition.

corymayer · 2024-12-17T20:15:51Z

Hi @corymayer, this is a nice addition. Thank you for the proposal.

If you agree I would like to see this possibility enabled by a Kconfig option.

I don't think the change in comment is required, when this problem occurs it is not by definition a corrupt NVS system, it can also be a non NVS partition.

Sure, will update soon

Laczen · 2024-12-18T08:02:37Z

subsys/fs/nvs/Kconfig

@@ -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_BAD_PARTITION_RECOVERY


What do you think about "NVS_INIT_BAD_REGION" ?

I would like this option and help text to reflect what it does and not (less) why it does this. NVS has no knowledge of partitions so using "partition" should be avoided.

The help could be something like:
Enable automatic initialization of a NVS on a flash 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.

Makes sense, updated

Laczen · 2024-12-18T08:04:34Z

subsys/fs/nvs/nvs.c

+#ifdef CONFIG_NVS_BAD_PARTITION_RECOVERY
+		LOG_WRN("All sectors closed, erasing all settings...");
+		addr = 0U;
+		rc = flash_erase(fs->flash_device, 0, fs->sector_size * fs->sector_count);


I don't think this is erasing at the correct location (missing fs->offset).

My bad, thought that the offset in that API was relative to the flash device. Device I tested it on happened to have an offset of 0. Fixed.

Laczen · 2024-12-18T08:05:35Z

subsys/fs/nvs/nvs.c

+		if (rc) {
+			rc = -EIO;
+			goto end;
+		}


please insert a space after this line.

Line was removed

Laczen · 2024-12-18T08:20:01Z

subsys/fs/nvs/nvs.c

+			rc = -EIO;
+			goto end;
+		}
+		i = 0;


I think we want:
i = fs->sector_count;
addr = ((fs->sector_count - 1) << ADDR_SECT_SHIFT) + (uint16_t)(fs->sector_size - ate_size);

Looking closer, I think we can remove this line altogether since that will be guaranteed by the previous loop

rghaddab · 2024-12-18T10:05:11Z

subsys/fs/nvs/nvs.c

 	if (closed_sectors == fs->sector_count) {
+#ifdef CONFIG_NVS_BAD_PARTITION_RECOVERY
+		LOG_WRN("All sectors closed, erasing all settings...");


There should be no mention of settings here.
NVS and Settings are two different subsystems.
You should use "erase all entries" or maybe "erasing all sectors"

rghaddab · 2024-12-18T10:11:36Z

subsys/fs/nvs/nvs.c

+#ifdef CONFIG_NVS_BAD_PARTITION_RECOVERY
+		LOG_WRN("All sectors closed, erasing all settings...");
+		addr = 0U;
+		rc = flash_erase(fs->flash_device, 0, fs->sector_size * fs->sector_count);


You should use flash_flatten instead.

rghaddab · 2024-12-18T10:13:25Z

subsys/fs/nvs/nvs.c

+		addr = 0U;
+		rc = flash_erase(fs->flash_device, 0, fs->sector_size * fs->sector_count);
+		if (rc) {
+			rc = -EIO;


why do you need to change the error code returned by the above function. Just keep the same error code

Damian-Nordic · 2024-12-18T15:32:11Z

subsys/fs/nvs/nvs.c

 	if (closed_sectors == fs->sector_count) {
+#ifdef CONFIG_NVS_INIT_BAD_REGION
+		LOG_WRN("All sectors closed, erasing all sectors...");
+		addr = 0U;


Was this code tested? If you enter this if and you erase the sectors, then you also enter the if (i == fs->sector_count) condition below, where there is rc = nvs_flash_cmp_const(fs, addr - ate_size, erase_value, sizeof(struct nvs_ate)); that assumes that addr points to the last ATE within the last sector. Am I correct?

Please consider extending NVS unit tests with a scenario that you initialize NVS using a garbage data in all sectors.

Just did a full re-test with the latest version I pushed. Set the ATEs to 0xdeadbeef and verified the log is printed and the NVS settings are written correctly afterwards.

Can you point me to the unit tests for NVS? I don't see them in this directory.

Thank you. The tests are in tests/subsys/fs/nvs directory. Though feel free to merge this PR and add the tests later :)

Laczen · 2024-12-18T15:34:53Z

subsys/fs/nvs/nvs.c

 	if (closed_sectors == fs->sector_count) {
+#ifdef CONFIG_NVS_INIT_BAD_REGION
+		LOG_WRN("All sectors closed, erasing all sectors...");
+		addr = 0U;


It is still needed to modify addr, after all sectors have been erased we should set it at the last sector close ate, the same as would be found for an empty region. This line should also move after the rc check.

Missed this, we can just remove this line too right, since the previous loop will ensure addr is set to the last sector?

I think it is set to the first sector when all sectors are closed.

I see, updated to set them to the last sector as you suggested

Laczen

@corymayer thank you for this PR.

rghaddab · 2024-12-19T00:00:39Z

subsys/fs/nvs/Kconfig

+	  Enable automatic initialization of a NVS on a flash 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.


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

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".

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

"Memory region" is also existing terminology.

"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 ?

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.

"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.

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

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

Laczen · 2024-12-19T17:45:47Z

@corymayer are you a discord user? I would like to get in touch.

corymayer · 2024-12-19T17:47:27Z

@corymayer are you a discord user? I would like to get in touch.

Yes, just joined as corymeta_74139

Laczen

I find the term "bad memory region" confusing and long to set as config variable, but I'm not going to spend more time on terminology.

Thanks for adding the test.

corymayer · 2024-12-19T22:12:54Z

Fixed an off-by-one in the unit test. Now prints this in the twister log:

2024-12-19` 14:06:03,971 - twister - DEBUG - QEMU (2562101): W: All sectors closed, erasing all sectors...
2024-12-19 14:06:03,971 - twister - DEBUG - QEMU (2562100): PASS - test_nvs_full_sector in 0.624 seconds

Laczen · 2024-12-20T05:28:17Z

tests/subsys/fs/nvs/src/main.c

+		err = flash_write(fixture->fs.flash_device, fixture->fs.offset +
+						 (fixture->fs.sector_size * (i + 1)) -
+						  sizeof(struct nvs_ate), &data, sizeof(data));
+		zassert_true(err == 0,  "flash_write failed: %d", err);


One space to much after the ",".

Fixed these

Laczen · 2024-12-20T05:28:43Z

tests/subsys/fs/nvs/src/main.c

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


one space to much after ",".

Laczen · 2024-12-20T05:29:15Z

tests/subsys/fs/nvs/src/main.c

+	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);


One space to much after ",".

Laczen · 2024-12-20T07:47:06Z

tests/subsys/fs/nvs/src/main.c

+ */
+ZTEST_F(nvs, test_nvs_init_bad_memory_region)
+{
+


NIT: remove this line

Laczen · 2024-12-20T07:55:34Z

tests/subsys/fs/nvs/src/main.c

+	/* 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 +


NIT: improve the alignment or introduce a offset variable to make this line shorter.

de-nordic · 2024-12-20T13:10:25Z

@corymayer Please fix the compliance.

Laczen · 2024-12-20T15:41:08Z

tests/subsys/fs/nvs/src/main.c

+	for (uint16_t i = 0; i < TEST_SECTOR_COUNT; i++) {
+		data = 0xdeadbeef;
+		err = flash_write(fixture->fs.flash_device,
+			fixture->fs.offset +


The alignment should be with the start of fixture->fs.flash_device. The complete file however is no example for correct alignment :-).

When all sectors have non-zero data, NVS would fail to init. Add recovery path option to erase the all sectors and reinitialize. This could occur due to non-empty sectors, or corrupted data. Signed-off-by: Cory Andrew Mayer <[email protected]>

Add unit test to exercise bad memory region recovery path Signed-off-by: Cory Andrew Mayer <[email protected]>

corymayer · 2024-12-21T15:54:42Z

@corymayer Please fix the compliance.

I was able to run the compliance check locally and verify all issues are resolved

Laczen · 2024-12-23T07:27:15Z

tests/subsys/fs/nvs/src/main.c

+		data = 0xdeadbeef;
+		err = flash_write(fixture->fs.flash_device,
+				  fixture->fs.offset + (fixture->fs.sector_size * (i + 1)) -
+					  sizeof(struct nvs_ate),


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

github-actions · 2024-12-23T08:16:31Z

Hi @corymayer!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

zephyrbot added the area: File System label Dec 17, 2024

zephyrbot requested review from de-nordic, Laczen and nashif December 17, 2024 18:16

zephyrbot assigned de-nordic Dec 17, 2024

corymayer force-pushed the settings_nvs_corruption_recovery branch from ed01058 to 9fa7b6c Compare December 17, 2024 20:29

Laczen reviewed Dec 18, 2024

View reviewed changes

de-nordic assigned Laczen and unassigned de-nordic Dec 18, 2024

de-nordic requested review from rghaddab and Damian-Nordic December 18, 2024 08:51

rghaddab reviewed Dec 18, 2024

View reviewed changes

corymayer force-pushed the settings_nvs_corruption_recovery branch from 9fa7b6c to 6bab748 Compare December 18, 2024 15:03

Damian-Nordic suggested changes Dec 18, 2024

View reviewed changes

Laczen reviewed Dec 18, 2024

View reviewed changes

corymayer force-pushed the settings_nvs_corruption_recovery branch from 6bab748 to b6fe8e7 Compare December 18, 2024 16:53

Laczen previously approved these changes Dec 18, 2024

View reviewed changes

Damian-Nordic approved these changes Dec 18, 2024

View reviewed changes

corymayer requested a review from rghaddab December 18, 2024 19:38

rghaddab reviewed Dec 19, 2024

View reviewed changes

corymayer dismissed Laczen’s stale review via ede130b December 19, 2024 20:17

corymayer force-pushed the settings_nvs_corruption_recovery branch from b6fe8e7 to ede130b Compare December 19, 2024 20:17

Laczen previously approved these changes Dec 19, 2024

View reviewed changes

corymayer dismissed Laczen’s stale review via eac7018 December 19, 2024 22:11

corymayer force-pushed the settings_nvs_corruption_recovery branch from ede130b to eac7018 Compare December 19, 2024 22:11

Laczen reviewed Dec 20, 2024

View reviewed changes

corymayer force-pushed the settings_nvs_corruption_recovery branch from eac7018 to 54dc53e Compare December 20, 2024 06:23

Laczen reviewed Dec 20, 2024

View reviewed changes

tests/subsys/fs/nvs/src/main.c Outdated

*/

ZTEST_F(nvs, test_nvs_init_bad_memory_region)

{

Copy link

Collaborator

Laczen Dec 20, 2024

Choose a reason for hiding this comment

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

NIT: remove this line

Laczen reviewed Dec 20, 2024

View reviewed changes

corymayer force-pushed the settings_nvs_corruption_recovery branch 2 times, most recently from f1c1789 to 33e7164 Compare December 20, 2024 15:08

Laczen reviewed Dec 20, 2024

View reviewed changes

corymayer force-pushed the settings_nvs_corruption_recovery branch 2 times, most recently from 3d215ee to 05d5839 Compare December 21, 2024 14:49

corymayer added 2 commits December 21, 2024 07:50

tests: subsys/fs/nvs: add test for initializing bad region

5fa5854

Add unit test to exercise bad memory region recovery path Signed-off-by: Cory Andrew Mayer <[email protected]>

corymayer force-pushed the settings_nvs_corruption_recovery branch from 05d5839 to 5fa5854 Compare December 21, 2024 15:54

rghaddab approved these changes Dec 23, 2024

View reviewed changes

kartben approved these changes Dec 23, 2024

View reviewed changes

Laczen reviewed Dec 23, 2024

View reviewed changes

Laczen approved these changes Dec 23, 2024

View reviewed changes

kartben merged commit 8094f92 into zephyrproject-rtos:main Dec 23, 2024
24 checks passed

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

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

Conversation

corymayer commented Dec 17, 2024

github-actions bot commented Dec 17, 2024

Laczen commented Dec 17, 2024

corymayer commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Laczen Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Laczen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Laczen commented Dec 19, 2024

corymayer commented Dec 19, 2024

Laczen left a comment

Choose a reason for hiding this comment

corymayer commented Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

de-nordic commented Dec 20, 2024

Choose a reason for hiding this comment

corymayer commented Dec 21, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 23, 2024

Laczen Dec 18, 2024 •

edited

Loading

corymayer commented Dec 19, 2024 •

edited

Loading