[nrf fromtree] fs: fcb: add option to disable CRC for fcb entries

eivindj-nordic · rlubos · commit 765155f4716b · 2023-02-14T12:39:20.000+01:00
Add option to disable CRC for fcb entries. This improves the write throughput significantly at the cost of not detecting corrupted data in flash. This is beneficial for aplications that needs the extra write throughput, where error detection is done elsewhere. Allow the FCB entries in flash to have a valid CRC when CRC is disabled in the FCB. This allows existing solutions to disable CRC checking, while keeping the CRC areas intact. Note that this is a one-way option. Fixes #53707 Signed-off-by: Eivind Jølsgard <eivind.jolsgard@nordicsemi.no> (cherry picked from commit cfa1ba1)
diff --git a/doc/releases/release-notes-3.3.rst b/doc/releases/release-notes-3.3.rst
@@ -377,6 +377,9 @@ Libraries / Subsystems
 
   * Added new API call `fs_mkfs`.
   * Added new sample `samples/subsys/fs/format`.
+  * Added the option to disable CRC checking in :ref:`fcb_api` by enabling the
+    Kconfig option :kconfig:option:`CONFIG_FCB_ALLOW_FIXED_ENDMARKER`
+    and setting the `FCB_FLAGS_CRC_DISABLED` flag in the :c:struct:`fcb` struct.
 
 * Management
 
diff --git a/include/zephyr/fs/fcb.h b/include/zephyr/fs/fcb.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017-2020 Nordic Semiconductor ASA
+ * Copyright (c) 2017-2023 Nordic Semiconductor ASA
  * Copyright (c) 2015 Runtime Inc
  *
  * SPDX-License-Identifier: Apache-2.0
@@ -14,6 +14,7 @@
 #include <limits.h>
 
 #include <zephyr/storage/flash_map.h>
+#include <zephyr/sys/util_macro.h>
 
 #include <zephyr/kernel.h>
 
@@ -76,6 +77,11 @@ struct fcb_entry_ctx {
 	/**< Flash area where the entry is placed */
 };
 
+/**
+ * @brief Flag to disable CRC for the fcb_entries in flash.
+ */
+#define FCB_FLAGS_CRC_DISABLED BIT(0)
+
 /**
  * @brief FCB instance structure
  *
@@ -129,6 +135,10 @@ struct fcb {
 	/**< The value flash takes when it is erased. This is read from
 	 * flash parameters and initialized upon call to fcb_init.
 	 */
+#ifdef CONFIG_FCB_ALLOW_FIXED_ENDMARKER
+	const uint8_t f_flags;
+	/**< Flags for configuring the FCB. */
+#endif
 };
 
 /**
diff --git a/subsys/fs/fcb/Kconfig b/subsys/fs/fcb/Kconfig
@@ -1,6 +1,6 @@
 # Flash Circular Buffer module
 
-# Copyright (c) 2017 Nordic Semiconductor ASA
+# Copyright (c) 2017-2023 Nordic Semiconductor ASA
 # SPDX-License-Identifier: Apache-2.0
 
 #
@@ -13,3 +13,13 @@ config FCB
 	select CRC
 	help
 	  Enable support of Flash Circular Buffer.
+
+if FCB
+
+config FCB_ALLOW_FIXED_ENDMARKER
+	bool "Allow FCB instances to have a fixed endmarker"
+	help
+	  This allows the FCB instances to disable CRC checks in
+	  favor of increased write throughput.
+
+endif
diff --git a/subsys/fs/fcb/fcb_append.c b/subsys/fs/fcb/fcb_append.c
@@ -121,18 +121,18 @@ int
 fcb_append_finish(struct fcb *fcb, struct fcb_entry *loc)
 {
 	int rc;
-	uint8_t crc8[fcb->f_align];
+	uint8_t em[fcb->f_align];
 	off_t off;
 
-	(void)memset(crc8, 0xFF, sizeof(crc8));
+	(void)memset(em, 0xFF, sizeof(em));
 
-	rc = fcb_elem_crc8(fcb, loc, &crc8[0]);
+	rc = fcb_elem_endmarker(fcb, loc, &em[0]);
 	if (rc) {
 		return rc;
 	}
 	off = loc->fe_data_off + fcb_len_in_flash(fcb, loc->fe_data_len);
 
-	rc = fcb_flash_write(fcb, loc->fe_sector, off, crc8, fcb->f_align);
+	rc = fcb_flash_write(fcb, loc->fe_sector, off, em, fcb->f_align);
 	if (rc) {
 		return -EIO;
 	}
diff --git a/subsys/fs/fcb/fcb_elem_info.c b/subsys/fs/fcb/fcb_elem_info.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017 Nordic Semiconductor ASA
+ * Copyright (c) 2017-2023 Nordic Semiconductor ASA
  * Copyright (c) 2015 Runtime Inc
  *
  * SPDX-License-Identifier: Apache-2.0
@@ -10,12 +10,14 @@
 #include <zephyr/fs/fcb.h>
 #include "fcb_priv.h"
 
+#define FCB_FIXED_ENDMARKER 0xab
+
 /*
  * Given offset in flash sector, fill in rest of the fcb_entry, and crc8 over
  * the data.
  */
-int
-fcb_elem_crc8(struct fcb *fcb, struct fcb_entry *loc, uint8_t *c8p)
+static int
+fcb_elem_crc8(struct fcb *_fcb, struct fcb_entry *loc, uint8_t *c8p)
 {
 	uint8_t tmp_str[FCB_TMP_BUF_SZ];
 	int cnt;
@@ -29,16 +31,17 @@ fcb_elem_crc8(struct fcb *fcb, struct fcb_entry *loc, uint8_t *c8p)
 	if (loc->fe_elem_off + 2 > loc->fe_sector->fs_size) {
 		return -ENOTSUP;
 	}
-	rc = fcb_flash_read(fcb, loc->fe_sector, loc->fe_elem_off, tmp_str, 2);
+
+	rc = fcb_flash_read(_fcb, loc->fe_sector, loc->fe_elem_off, tmp_str, 2);
 	if (rc) {
 		return -EIO;
 	}
 
-	cnt = fcb_get_len(fcb, tmp_str, &len);
+	cnt = fcb_get_len(_fcb, tmp_str, &len);
 	if (cnt < 0) {
 		return cnt;
 	}
-	loc->fe_data_off = loc->fe_elem_off + fcb_len_in_flash(fcb, cnt);
+	loc->fe_data_off = loc->fe_elem_off + fcb_len_in_flash(_fcb, cnt);
 	loc->fe_data_len = len;
 
 	crc8 = CRC8_CCITT_INITIAL_VALUE;
@@ -52,7 +55,7 @@ fcb_elem_crc8(struct fcb *fcb, struct fcb_entry *loc, uint8_t *c8p)
 			blk_sz = sizeof(tmp_str);
 		}
 
-		rc = fcb_flash_read(fcb, loc->fe_sector, off, tmp_str, blk_sz);
+		rc = fcb_flash_read(_fcb, loc->fe_sector, off, tmp_str, blk_sz);
 		if (rc) {
 			return -EIO;
 		}
@@ -63,25 +66,83 @@ fcb_elem_crc8(struct fcb *fcb, struct fcb_entry *loc, uint8_t *c8p)
 	return 0;
 }
 
-int fcb_elem_info(struct fcb *fcb, struct fcb_entry *loc)
+#if IS_ENABLED(CONFIG_FCB_ALLOW_FIXED_ENDMARKER)
+/* Given the offset in flash sector, calculate the FCB entry data offset and size, and set
+ * the fixed endmarker.
+ */
+static int
+fcb_elem_endmarker_fixed(struct fcb *_fcb, struct fcb_entry *loc, uint8_t *em)
 {
+	uint8_t tmp_str[2];
+	int cnt;
+	uint16_t len;
 	int rc;
-	uint8_t crc8;
-	uint8_t fl_crc8;
+
+	if (loc->fe_elem_off + 2 > loc->fe_sector->fs_size) {
+		return -ENOTSUP;
+	}
+
+	rc = fcb_flash_read(_fcb, loc->fe_sector, loc->fe_elem_off, tmp_str, 2);
+	if (rc) {
+		return -EIO;
+	}
+
+	cnt = fcb_get_len(_fcb, tmp_str, &len);
+	if (cnt < 0) {
+		return cnt;
+	}
+	loc->fe_data_off = loc->fe_elem_off + fcb_len_in_flash(_fcb, cnt);
+	loc->fe_data_len = len;
+
+	*em = FCB_FIXED_ENDMARKER;
+	return 0;
+}
+#endif /* IS_ENABLED(CONFIG_FCB_ALLOW_FIXED_ENDMARKER) */
+
+/* Given the offset in flash sector, calculate the FCB entry data offset and size, and calculate
+ * the expected endmarker.
+ */
+int
+fcb_elem_endmarker(struct fcb *_fcb, struct fcb_entry *loc, uint8_t *em)
+{
+#if IS_ENABLED(CONFIG_FCB_ALLOW_FIXED_ENDMARKER)
+	if (_fcb->f_flags & FCB_FLAGS_CRC_DISABLED) {
+		return fcb_elem_endmarker_fixed(_fcb, loc, em);
+	}
+#endif /* IS_ENABLED(CONFIG_FCB_ALLOW_FIXED_ENDMARKER) */
+
+	return fcb_elem_crc8(_fcb, loc, em);
+}
+
+/* Given the offset in flash sector, calculate the FCB entry data offset and size, and verify that
+ * the FCB entry endmarker is correct.
+ */
+int fcb_elem_info(struct fcb *_fcb, struct fcb_entry *loc)
+{
+	int rc;
+	uint8_t em;
+	uint8_t fl_em;
 	off_t off;
 
-	rc = fcb_elem_crc8(fcb, loc, &crc8);
+	rc = fcb_elem_endmarker(_fcb, loc, &em);
 	if (rc) {
 		return rc;
 	}
-	off = loc->fe_data_off + fcb_len_in_flash(fcb, loc->fe_data_len);
+	off = loc->fe_data_off + fcb_len_in_flash(_fcb, loc->fe_data_len);
 
-	rc = fcb_flash_read(fcb, loc->fe_sector, off, &fl_crc8, sizeof(fl_crc8));
+	rc = fcb_flash_read(_fcb, loc->fe_sector, off, &fl_em, sizeof(fl_em));
 	if (rc) {
 		return -EIO;
 	}
 
-	if (fl_crc8 != crc8) {
+	if (IS_ENABLED(CONFIG_FCB_ALLOW_FIXED_ENDMARKER) && (fl_em != em)) {
+		rc = fcb_elem_crc8(_fcb, loc, &em);
+		if (rc) {
+			return rc;
+		}
+	}
+
+	if (fl_em != em) {
 		return -EBADMSG;
 	}
 	return 0;
diff --git a/subsys/fs/fcb/fcb_priv.h b/subsys/fs/fcb/fcb_priv.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017-2020 Nordic Semiconductor ASA
+ * Copyright (c) 2017-2023 Nordic Semiconductor ASA
  * Copyright (c) 2015 Runtime Inc
  *
  * SPDX-License-Identifier: Apache-2.0
@@ -70,7 +70,7 @@ struct flash_sector *fcb_getnext_sector(struct fcb *fcb,
 int fcb_getnext_nolock(struct fcb *fcb, struct fcb_entry *loc);
 
 int fcb_elem_info(struct fcb *fcb, struct fcb_entry *loc);
-int fcb_elem_crc8(struct fcb *fcb, struct fcb_entry *loc, uint8_t *crc8p);
+int fcb_elem_endmarker(struct fcb *fcb, struct fcb_entry *loc, uint8_t *crc8p);
 
 int fcb_sector_hdr_init(struct fcb *fcb, struct flash_sector *sector, uint16_t id);
 int fcb_sector_hdr_read(struct fcb *fcb, struct flash_sector *sector,
diff --git a/tests/subsys/fs/fcb/prj.conf b/tests/subsys/fs/fcb/prj.conf
@@ -5,5 +5,6 @@ CONFIG_FLASH_PAGE_LAYOUT=y
 CONFIG_FLASH_MAP=y
 CONFIG_ARM_MPU=n
 CONFIG_FCB=y
+CONFIG_FCB_ALLOW_FIXED_ENDMARKER=y
 CONFIG_SOC_FLASH_NRF_EMULATE_ONE_BYTE_WRITE_ACCESS=y
 CONFIG_ZTEST_NEW_API=y
diff --git a/tests/subsys/fs/fcb/prj_native_posix.conf b/tests/subsys/fs/fcb/prj_native_posix.conf
@@ -4,4 +4,5 @@ CONFIG_FLASH=y
 CONFIG_FLASH_PAGE_LAYOUT=y
 CONFIG_FLASH_MAP=y
 CONFIG_FCB=y
+CONFIG_FCB_ALLOW_FIXED_ENDMARKER=y
 CONFIG_ZTEST_NEW_API=y
diff --git a/tests/subsys/fs/fcb/prj_native_posix_64.conf b/tests/subsys/fs/fcb/prj_native_posix_64.conf
@@ -4,4 +4,5 @@ CONFIG_FLASH=y
 CONFIG_FLASH_PAGE_LAYOUT=y
 CONFIG_FLASH_MAP=y
 CONFIG_FCB=y
+CONFIG_FCB_ALLOW_FIXED_ENDMARKER=y
 CONFIG_ZTEST_NEW_API=y
diff --git a/tests/subsys/fs/fcb/prj_nucleo_h743zi.conf b/tests/subsys/fs/fcb/prj_nucleo_h743zi.conf
@@ -6,7 +6,7 @@ CONFIG_FLASH_PAGE_LAYOUT=y
 CONFIG_FLASH_MAP=y
 CONFIG_ARM_MPU=n
 CONFIG_FCB=y
-
+CONFIG_FCB_ALLOW_FIXED_ENDMARKER=y
 CONFIG_MAIN_STACK_SIZE=4096
 CONFIG_SYSTEM_WORKQUEUE_STACK_SIZE=4096
 CONFIG_ZTEST_NEW_API=y
diff --git a/tests/subsys/fs/fcb/prj_qemu_x86.conf b/tests/subsys/fs/fcb/prj_qemu_x86.conf
@@ -4,5 +4,6 @@ CONFIG_FLASH=y
 CONFIG_FLASH_PAGE_LAYOUT=y
 CONFIG_FLASH_MAP=y
 CONFIG_FCB=y
+CONFIG_FCB_ALLOW_FIXED_ENDMARKER=y
 CONFIG_FLASH_SIMULATOR_UNALIGNED_READ=y
 CONFIG_ZTEST_NEW_API=y
diff --git a/tests/subsys/fs/fcb/src/fcb_test.h b/tests/subsys/fs/fcb/src/fcb_test.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017-2020 Nordic Semiconductor ASA
+ * Copyright (c) 2017-2023 Nordic Semiconductor ASA
  * Copyright (c) 2015 Runtime Inc
  *
  * SPDX-License-Identifier: Apache-2.0
@@ -24,6 +24,7 @@ extern "C" {
 #define TEST_FCB_FLASH_AREA_ID		FIXED_PARTITION_ID(TEST_FCB_FLASH_AREA)
 
 extern struct fcb test_fcb;
+extern struct fcb test_fcb_crc_disabled;
 
 extern struct flash_sector test_fcb_sector[];
 
diff --git a/tests/subsys/fs/fcb/src/fcb_test_append.c b/tests/subsys/fs/fcb/src/fcb_test_append.c
@@ -1,40 +1,47 @@
 /*
- * Copyright (c) 2017 Nordic Semiconductor ASA
+ * Copyright (c) 2017-2023 Nordic Semiconductor ASA
  * Copyright (c) 2015 Runtime Inc
  *
  * SPDX-License-Identifier: Apache-2.0
  */
 
 #include "fcb_test.h"
 
-ZTEST(fcb_test_with_2sectors_set, test_fcb_append)
+static void test_fcb_append(struct fcb *_fcb)
 {
 	int rc;
-	struct fcb *fcb;
 	struct fcb_entry loc;
 	uint8_t test_data[128];
 	int i;
 	int j;
 	int var_cnt;
 
-	fcb = &test_fcb;
-
 	for (i = 0; i < sizeof(test_data); i++) {
 		for (j = 0; j < i; j++) {
 			test_data[j] = fcb_test_append_data(i, j);
 		}
-		rc = fcb_append(fcb, i, &loc);
+		rc = fcb_append(_fcb, i, &loc);
 		zassert_true(rc == 0, "fcb_append call failure");
-		rc = flash_area_write(fcb->fap, FCB_ENTRY_FA_DATA_OFF(loc),
+		rc = flash_area_write(_fcb->fap, FCB_ENTRY_FA_DATA_OFF(loc),
 				      test_data, i);
 		zassert_true(rc == 0, "flash_area_write call failure");
-		rc = fcb_append_finish(fcb, &loc);
+		rc = fcb_append_finish(_fcb, &loc);
 		zassert_true(rc == 0, "fcb_append_finish call failure");
 	}
 
 	var_cnt = 0;
-	rc = fcb_walk(fcb, 0, fcb_test_data_walk_cb, &var_cnt);
+	rc = fcb_walk(_fcb, 0, fcb_test_data_walk_cb, &var_cnt);
 	zassert_true(rc == 0, "fcb_walk call failure");
 	zassert_true(var_cnt == sizeof(test_data),
 		     "fetched data size not match to wrote data size");
 }
+
+ZTEST(fcb_test_with_2sectors_set, test_fcb_append_2sectors)
+{
+	test_fcb_append(&test_fcb);
+}
+
+ZTEST(fcb_test_crc_disabled, test_fcb_append_crc_disabled)
+{
+	test_fcb_append(&test_fcb_crc_disabled);
+}
diff --git a/tests/subsys/fs/fcb/src/fcb_test_append_too_big.c b/tests/subsys/fs/fcb/src/fcb_test_append_too_big.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017 Nordic Semiconductor ASA
+ * Copyright (c) 2017-2023 Nordic Semiconductor ASA
  * Copyright (c) 2015 Runtime Inc
  *
  * SPDX-License-Identifier: Apache-2.0
diff --git a/tests/subsys/fs/fcb/src/fcb_test_crc_disabled_after_enabled.c b/tests/subsys/fs/fcb/src/fcb_test_crc_disabled_after_enabled.c
diff --git a/tests/subsys/fs/fcb/src/main.c b/tests/subsys/fs/fcb/src/main.c
