[PATCHv4 0/5] FWU: Handle meta-data in common code

[PATCHv4 0/5] FWU: Handle meta-data in common code

[jassisinghbrar]

From: Jassi Brar <jaswinder.singh@linaro.org>

The patchset reduces ~400 lines of code, while keeping the functionality same and making
meta-data operations much faster (by using cached structures).

Issue:
 meta-data copies (primary and secondary) are being handled by the backend/storage layer
instead of the common core in fwu.c (as also noted by Ilias)  that is, gpt_blk.c manages
meta-data and similarly raw_mtd.c will have to do the same when it arrives. The code
could by make smaller, cleaner and optimised.

Basic idea:
 Introduce  .read_mdata() and .write_mdata() in fwu_mdata_ops  that simply read/write
meta-data copy. The core code takes care of integrity and redundancy of the meta-data,
as a result we can get rid of every other callback .get_mdata() .update_mdata()
.get_mdata_part_num()  .read_mdata_partition()  .write_mdata_partition() and the
corresponding wrapper functions thereby making the code 100s of LOC smaller.

Get rid of fwu_check_mdata_validity() and fwu_mdata_check() which expected underlying
layer to manage and verify mdata copies.
Implement  fwu_get_verified_mdata(struct fwu_mdata *mdata) public function that reads,
verifies and, if needed, fixes the meta-data copies.

Verified copy of meta-data is now cached as 'g_mdata' in fwu.c, which avoids multiple
low-level expensive read and parse calls.
gpt meta-data partition numbers are now cached in gpt_blk.c, so that we don't have to do expensive part_get_info() and uid ops.

Changes since v3:
        * Fix error log wording
        * call fwu_write_mdata() with part & PRIMARY_PART ? true: false

Changes since v2:
        * Drop whitespace changes
        * Fix missing mdata copy before return

Changes since v1:
        * Fix typos and misc cosmetic changes
        * Catch error returns


Jassi Brar (5):
  fwu: gpt: use cached meta-data partition numbers
  fwu: move meta-data management in core
  fwu: gpt: implement read_mdata and write_mdata callbacks
  fwu: meta-data: switch to management by common code
  fwu: rename fwu_get_verified_mdata to fwu_get_mdata

 cmd/fwu_mdata.c                      |  17 +-
 drivers/fwu-mdata/fwu-mdata-uclass.c | 151 +-------------
 drivers/fwu-mdata/gpt_blk.c          | 175 +++++-----------
 include/fwu.h                        | 198 ++----------------
 lib/fwu_updates/fwu.c                | 294 +++++++++++----------------
 5 files changed, 207 insertions(+), 628 deletions(-)

-- 
2.34.1

[PATCHv4 1/5] fwu: gpt: use cached meta-data partition numbers

[jassisinghbrar]

From: Jassi Brar <jaswinder.singh@linaro.org>

Use cached values and avoid parsing and scanning through partitions
everytime for meta-data partitions because they can't change after bootup.

Acked-by: Etienne Carriere <etienne.carriere@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 drivers/fwu-mdata/gpt_blk.c | 43 +++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c
index d35ce49c5c..28f5d23e1e 100644
--- a/drivers/fwu-mdata/gpt_blk.c
+++ b/drivers/fwu-mdata/gpt_blk.c
@@ -24,8 +24,9 @@ enum {
 	MDATA_WRITE,
 };
 
-static int gpt_get_mdata_partitions(struct blk_desc *desc,
-				    uint mdata_parts[2])
+static uint g_mdata_part[2]; /* = {0, 0} to check against uninit parts */
+
+static int gpt_get_mdata_partitions(struct blk_desc *desc)
 {
 	int i, ret;
 	u32 nparts;
@@ -33,18 +34,19 @@ static int gpt_get_mdata_partitions(struct blk_desc *desc,
 	struct disk_partition info;
 	const efi_guid_t fwu_mdata_guid = FWU_MDATA_GUID;
 
+	/* if primary and secondary partitions already found */
+	if (g_mdata_part[0] && g_mdata_part[1])
+		return 0;
+
 	nparts = 0;
-	for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) {
+	for (i = 1; i < MAX_SEARCH_PARTITIONS && nparts < 2; i++) {
 		if (part_get_info(desc, i, &info))
 			continue;
 		uuid_str_to_bin(info.type_guid, part_type_guid.b,
 				UUID_STR_FORMAT_GUID);
 
-		if (!guidcmp(&fwu_mdata_guid, &part_type_guid)) {
-			if (nparts < 2)
-				mdata_parts[nparts] = i;
-			++nparts;
-		}
+		if (!guidcmp(&fwu_mdata_guid, &part_type_guid))
+			g_mdata_part[nparts++] = i;
 	}
 
 	if (nparts != 2) {
@@ -127,26 +129,25 @@ static int fwu_gpt_update_mdata(struct udevice *dev, struct fwu_mdata *mdata)
 {
 	int ret;
 	struct blk_desc *desc;
-	uint mdata_parts[2];
 	struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
 
 	desc = dev_get_uclass_plat(priv->blk_dev);
 
-	ret = gpt_get_mdata_partitions(desc, mdata_parts);
+	ret = gpt_get_mdata_partitions(desc);
 	if (ret < 0) {
 		log_debug("Error getting the FWU metadata partitions\n");
 		return -ENOENT;
 	}
 
 	/* First write the primary partition */
-	ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, mdata_parts[0]);
+	ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, g_mdata_part[0]);
 	if (ret < 0) {
 		log_debug("Updating primary FWU metadata partition failed\n");
 		return ret;
 	}
 
 	/* And now the replica */
-	ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, mdata_parts[1]);
+	ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, g_mdata_part[1]);
 	if (ret < 0) {
 		log_debug("Updating secondary FWU metadata partition failed\n");
 		return ret;
@@ -158,16 +159,14 @@ static int fwu_gpt_update_mdata(struct udevice *dev, struct fwu_mdata *mdata)
 static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata *mdata)
 {
 	int ret;
-	uint mdata_parts[2];
-
-	ret = gpt_get_mdata_partitions(desc, mdata_parts);
 
+	ret = gpt_get_mdata_partitions(desc);
 	if (ret < 0) {
 		log_debug("Error getting the FWU metadata partitions\n");
 		return -ENOENT;
 	}
 
-	ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, mdata_parts[0]);
+	ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, g_mdata_part[0]);
 	if (ret < 0) {
 		log_debug("Failed to read the FWU metadata from the device\n");
 		return -EIO;
@@ -182,7 +181,7 @@ static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata *mdata)
 	 * Try to read the replica.
 	 */
 	memset(mdata, '\0', sizeof(struct fwu_mdata));
-	ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, mdata_parts[1]);
+	ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, g_mdata_part[1]);
 	if (ret < 0) {
 		log_debug("Failed to read the FWU metadata from the device\n");
 		return -EIO;
@@ -206,9 +205,15 @@ static int fwu_gpt_get_mdata(struct udevice *dev, struct fwu_mdata *mdata)
 static int fwu_gpt_get_mdata_partitions(struct udevice *dev, uint *mdata_parts)
 {
 	struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
+	int err;
+
+	err = gpt_get_mdata_partitions(dev_get_uclass_plat(priv->blk_dev));
+	if (!err) {
+		mdata_parts[0] = g_mdata_part[0];
+		mdata_parts[1] = g_mdata_part[1];
+	}
 
-	return gpt_get_mdata_partitions(dev_get_uclass_plat(priv->blk_dev),
-					mdata_parts);
+	return err;
 }
 
 static int fwu_gpt_read_mdata_partition(struct udevice *dev,
-- 
2.34.1

[PATCHv4 2/5] fwu: move meta-data management in core

[jassisinghbrar]

From: Jassi Brar <jaswinder.singh@linaro.org>

Instead of each i/f having to implement their own meta-data verification
and storage, move the logic in common code. This simplifies the i/f code
much simpler and compact.

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 drivers/fwu-mdata/fwu-mdata-uclass.c |  34 +++++++
 include/fwu.h                        |  41 ++++++++
 lib/fwu_updates/fwu.c                | 135 ++++++++++++++++++++++++++-
 3 files changed, 206 insertions(+), 4 deletions(-)

diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c
index b477e9603f..e03773c584 100644
--- a/drivers/fwu-mdata/fwu-mdata-uclass.c
+++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
@@ -16,6 +16,40 @@
 #include <linux/types.h>
 #include <u-boot/crc.h>
 
+/**
+ * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
+ *
+ * Return: 0 if OK, -ve on error
+ */
+int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
+{
+	const struct fwu_mdata_ops *ops = device_get_ops(dev);
+
+	if (!ops->read_mdata) {
+		log_debug("read_mdata() method not defined\n");
+		return -ENOSYS;
+	}
+
+	return ops->read_mdata(dev, mdata, primary);
+}
+
+/**
+ * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
+ *
+ * Return: 0 if OK, -ve on error
+ */
+int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
+{
+	const struct fwu_mdata_ops *ops = device_get_ops(dev);
+
+	if (!ops->write_mdata) {
+		log_debug("write_mdata() method not defined\n");
+		return -ENOSYS;
+	}
+
+	return ops->write_mdata(dev, mdata, primary);
+}
+
 /**
  * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers
  * @dev: FWU metadata device
diff --git a/include/fwu.h b/include/fwu.h
index 0919ced812..1a700c9e6a 100644
--- a/include/fwu.h
+++ b/include/fwu.h
@@ -24,6 +24,26 @@ struct fwu_mdata_gpt_blk_priv {
  * @update_mdata() - Update the FWU metadata copy
  */
 struct fwu_mdata_ops {
+	/**
+	 * read_mdata() - Populate the asked FWU metadata copy
+	 * @dev: FWU metadata device
+	 * @mdata: Copy of the FWU metadata
+	 * @primary: If primary or secondary copy of meta-data is to be read
+	 *
+	 * Return: 0 if OK, -ve on error
+	 */
+	int (*read_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
+
+	/**
+	 * write_mdata() - Write the given FWU metadata copy
+	 * @dev: FWU metadata device
+	 * @mdata: Copy of the FWU metadata
+	 * @primary: If primary or secondary copy of meta-data is to be written
+	 *
+	 * Return: 0 if OK, -ve on error
+	 */
+	int (*write_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
+
 	/**
 	 * check_mdata() - Check if the FWU metadata is valid
 	 * @dev:	FWU device
@@ -126,6 +146,27 @@ struct fwu_mdata_ops {
 	EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \
 		 0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8)
 
+/**
+ * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
+ */
+int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
+
+/**
+ * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
+ */
+int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
+
+/**
+ * fwu_get_verified_mdata() - Read, verify and return the FWU metadata
+ *
+ * Read both the metadata copies from the storage media, verify their checksum,
+ * and ascertain that both copies match. If one of the copies has gone bad,
+ * restore it from the good copy.
+ *
+ * Return: 0 if OK, -ve on error
+*/
+int fwu_get_verified_mdata(struct fwu_mdata *mdata);
+
 /**
  * fwu_check_mdata_validity() - Check for validity of the FWU metadata copies
  *
diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
index 5313d07302..56299f1b2f 100644
--- a/lib/fwu_updates/fwu.c
+++ b/lib/fwu_updates/fwu.c
@@ -15,13 +15,13 @@
 #include <linux/errno.h>
 #include <linux/types.h>
 
+#include <u-boot/crc.h>
+
+static struct fwu_mdata g_mdata; /* = {0} makes uninit crc32 always invalid */
+static struct udevice *g_dev;
 static u8 in_trial;
 static u8 boottime_check;
 
-#include <linux/errno.h>
-#include <linux/types.h>
-#include <u-boot/crc.h>
-
 enum {
 	IMAGE_ACCEPT_SET = 1,
 	IMAGE_ACCEPT_CLEAR,
@@ -161,6 +161,133 @@ static int fwu_get_image_type_id(u8 *image_index, efi_guid_t *image_type_id)
 	return -ENOENT;
 }
 
+/**
+ * fwu_sync_mdata() - Update given meta-data partition(s) with the copy provided
+ * @mdata: FWU metadata structure
+ * @part: Bitmask of FWU metadata partitions to be written to
+ *
+ * Return: 0 if OK, -ve on error
+ */
+static int fwu_sync_mdata(struct fwu_mdata *mdata, int part)
+{
+	void *buf = &mdata->version;
+	int err = 0;
+
+	/*
+	 * Calculate the crc32 for the updated FWU metadata
+	 * and put the updated value in the FWU metadata crc32
+	 * field
+	 */
+	mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
+
+	err = fwu_write_mdata(g_dev, mdata, part & PRIMARY_PART ? true : false);
+	if (err) {
+		log_err("Unable to write %s mdata\n",
+				part & PRIMARY_PART ?  "primary": "secondary");
+		return err;
+	}
+
+	/* update the cached copy of meta-data */
+	memcpy(&g_mdata, mdata, sizeof(struct fwu_mdata));
+
+	return 0;
+}
+
+static inline int mdata_crc_check(struct fwu_mdata *mdata)
+{
+	void *buf = &mdata->version;
+	u32 calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
+
+	return calc_crc32 == mdata->crc32 ? 0 : -EINVAL;
+}
+
+/**
+ * fwu_get_verified_mdata() - Read, verify and return the FWU metadata
+ *
+ * Read both the metadata copies from the storage media, verify their checksum,
+ * and ascertain that both copies match. If one of the copies has gone bad,
+ * restore it from the good copy.
+ *
+ * Return: 0 if OK, -ve on error
+ */
+int fwu_get_verified_mdata(struct fwu_mdata *mdata)
+{
+	int err;
+	bool pri_ok, sec_ok;
+	struct fwu_mdata s, *p_mdata, *s_mdata;
+
+	p_mdata = &g_mdata;
+	s_mdata = &s;
+
+	/* if mdata already read and ready */
+	err = mdata_crc_check(p_mdata);
+	if (!err)
+		goto ret_mdata;
+	/* else read, verify and, if needed, fix mdata */
+
+	pri_ok = false;
+	err = fwu_read_mdata(g_dev, p_mdata, true);
+	if (!err) {
+		err = mdata_crc_check(p_mdata);
+		if (!err)
+			pri_ok = true;
+		else
+			log_debug("primary mdata: crc32 failed\n");
+	}
+
+	sec_ok = false;
+	err = fwu_read_mdata(g_dev, s_mdata, false);
+	if (!err) {
+		err = mdata_crc_check(s_mdata);
+		if (!err)
+			sec_ok = true;
+		else
+			log_debug("secondary mdata: crc32 failed\n");
+	}
+
+	if (pri_ok && sec_ok) {
+		/*
+		 * Before returning, check that both the
+		 * FWU metadata copies are the same.
+		 */
+		err = memcmp(p_mdata, s_mdata, sizeof(struct fwu_mdata));
+		if (!err)
+			goto ret_mdata;
+
+		/*
+		 * If not, populate the secondary partition from the
+		 * primary partition copy.
+		 */
+		log_info("Both FWU metadata copies are valid but do not match.");
+		log_info(" Restoring the secondary partition from the primary\n");
+		sec_ok = false;
+	}
+
+	if (!pri_ok) {
+		memcpy(p_mdata, s_mdata, sizeof(struct fwu_mdata));
+		err = fwu_sync_mdata(p_mdata, PRIMARY_PART);
+		if (err) {
+			log_debug("mdata : primary write failed\n");
+			return err;
+		}
+	}
+
+	if (!sec_ok) {
+		memcpy(s_mdata, p_mdata, sizeof(struct fwu_mdata));
+		err = fwu_sync_mdata(s_mdata, SECONDARY_PART);
+		if (err) {
+			log_debug("mdata : secondary write failed\n");
+			return err;
+		}
+	}
+
+ret_mdata:
+	if (!err && mdata)
+		memcpy(mdata, p_mdata, sizeof(struct fwu_mdata));
+
+	return err;
+}
+
 /**
  * fwu_verify_mdata() - Verify the FWU metadata
  * @mdata: FWU metadata structure
-- 
2.34.1

[PATCHv4 3/5] fwu: gpt: implement read_mdata and write_mdata callbacks

[jassisinghbrar]

From: Jassi Brar <jaswinder.singh@linaro.org>

Moving towards using common code for meta-data management,
implement the read/write mdata hooks.

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 drivers/fwu-mdata/gpt_blk.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c
index 28f5d23e1e..bdaa10cd1d 100644
--- a/drivers/fwu-mdata/gpt_blk.c
+++ b/drivers/fwu-mdata/gpt_blk.c
@@ -272,7 +272,43 @@ static int fwu_mdata_gpt_blk_probe(struct udevice *dev)
 	return 0;
 }
 
+static int fwu_gpt_read_mdata(struct udevice *dev, struct fwu_mdata *mdata,
+						 bool primary)
+{
+	struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
+	struct blk_desc *desc = dev_get_uclass_plat(priv->blk_dev);
+	int ret;
+
+	ret = gpt_get_mdata_partitions(desc);
+	if (ret < 0) {
+		log_debug("Error getting the FWU metadata partitions\n");
+		return -ENOENT;
+	}
+
+	return gpt_read_write_mdata(desc, mdata, MDATA_READ,
+                                primary ? g_mdata_part[0] : g_mdata_part[1]);
+}
+
+static int fwu_gpt_write_mdata(struct udevice *dev, struct fwu_mdata *mdata,
+						 bool primary)
+{
+	struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
+	struct blk_desc *desc = dev_get_uclass_plat(priv->blk_dev);
+	int ret;
+
+	ret = gpt_get_mdata_partitions(desc);
+	if (ret < 0) {
+		log_debug("Error getting the FWU metadata partitions\n");
+		return -ENOENT;
+	}
+
+	return gpt_read_write_mdata(desc, mdata, MDATA_WRITE,
+                                primary ? g_mdata_part[0] : g_mdata_part[1]);
+}
+
 static const struct fwu_mdata_ops fwu_gpt_blk_ops = {
+	.read_mdata = fwu_gpt_read_mdata,
+	.write_mdata = fwu_gpt_write_mdata,
 	.get_mdata = fwu_gpt_get_mdata,
 	.update_mdata = fwu_gpt_update_mdata,
 	.get_mdata_part_num = fwu_gpt_get_mdata_partitions,
-- 
2.34.1

[PATCHv4 4/5] fwu: meta-data: switch to management by common code

[jassisinghbrar]

From: Jassi Brar <jaswinder.singh@linaro.org>

The common code can now read, verify and fix meta-data copies
while exposing one consistent structure to users.
 Only the .read_mdata() and .write_mdata() callbacks of fwu_mdata_ops
are needed. Get rid of .get_mdata() .update_mdata() .get_mdata_part_num()
.read_mdata_partition() and .write_mdata_partition() and also the
corresponding wrapper functions.

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
---
 cmd/fwu_mdata.c                      |  17 +-
 drivers/fwu-mdata/fwu-mdata-uclass.c | 165 -------------------
 drivers/fwu-mdata/gpt_blk.c          | 124 +-------------
 include/fwu.h                        | 199 -----------------------
 lib/fwu_updates/fwu.c                | 235 ++++-----------------------
 5 files changed, 38 insertions(+), 702 deletions(-)

diff --git a/cmd/fwu_mdata.c b/cmd/fwu_mdata.c
index f04af27de6..9b70340368 100644
--- a/cmd/fwu_mdata.c
+++ b/cmd/fwu_mdata.c
@@ -43,23 +43,10 @@ static void print_mdata(struct fwu_mdata *mdata)
 int do_fwu_mdata_read(struct cmd_tbl *cmdtp, int flag,
 		     int argc, char * const argv[])
 {
-	struct udevice *dev;
 	int ret = CMD_RET_SUCCESS, res;
-	struct fwu_mdata mdata = { 0 };
+	struct fwu_mdata mdata;
 
-	if (uclass_get_device(UCLASS_FWU_MDATA, 0, &dev) || !dev) {
-		log_err("Unable to get FWU metadata device\n");
-		return CMD_RET_FAILURE;
-	}
-
-	res = fwu_check_mdata_validity();
-	if (res < 0) {
-		log_err("FWU Metadata check failed\n");
-		ret = CMD_RET_FAILURE;
-		goto out;
-	}
-
-	res = fwu_get_mdata(dev, &mdata);
+	res = fwu_get_verified_mdata(&mdata);
 	if (res < 0) {
 		log_err("Unable to get valid FWU metadata\n");
 		ret = CMD_RET_FAILURE;
diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c
index e03773c584..0a8edaaa41 100644
--- a/drivers/fwu-mdata/fwu-mdata-uclass.c
+++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
@@ -14,7 +14,6 @@
 
 #include <linux/errno.h>
 #include <linux/types.h>
-#include <u-boot/crc.h>
 
 /**
  * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
@@ -50,170 +49,6 @@ int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
 	return ops->write_mdata(dev, mdata, primary);
 }
 
-/**
- * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers
- * @dev: FWU metadata device
- * @mdata_parts: array for storing the metadata partition numbers
- *
- * Get the partition numbers on the storage device on which the
- * FWU metadata is stored. Two partition numbers will be returned.
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_get_mdata_part_num(struct udevice *dev, uint *mdata_parts)
-{
-	const struct fwu_mdata_ops *ops = device_get_ops(dev);
-
-	if (!ops->get_mdata_part_num) {
-		log_debug("get_mdata_part_num() method not defined\n");
-		return -ENOSYS;
-	}
-
-	return ops->get_mdata_part_num(dev, mdata_parts);
-}
-
-/**
- * fwu_read_mdata_partition() - Read the FWU metadata from a partition
- * @dev: FWU metadata device
- * @mdata: Copy of the FWU metadata
- * @part_num: Partition number from which FWU metadata is to be read
- *
- * Read the FWU metadata from the specified partition number
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_read_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata,
-			     uint part_num)
-{
-	const struct fwu_mdata_ops *ops = device_get_ops(dev);
-
-	if (!ops->read_mdata_partition) {
-		log_debug("read_mdata_partition() method not defined\n");
-		return -ENOSYS;
-	}
-
-	return ops->read_mdata_partition(dev, mdata, part_num);
-}
-
-/**
- * fwu_write_mdata_partition() - Write the FWU metadata to a partition
- * @dev: FWU metadata device
- * @mdata: Copy of the FWU metadata
- * @part_num: Partition number to which FWU metadata is to be written
- *
- * Write the FWU metadata to the specified partition number
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_write_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata,
-			      uint part_num)
-{
-	const struct fwu_mdata_ops *ops = device_get_ops(dev);
-
-	if (!ops->write_mdata_partition) {
-		log_debug("write_mdata_partition() method not defined\n");
-		return -ENOSYS;
-	}
-
-	return ops->write_mdata_partition(dev, mdata, part_num);
-}
-
-/**
- * fwu_mdata_check() - Check if the FWU metadata is valid
- * @dev: FWU metadata device
- *
- * Validate both copies of the FWU metadata. If one of the copies
- * has gone bad, restore it from the other copy.
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_mdata_check(struct udevice *dev)
-{
-	const struct fwu_mdata_ops *ops = device_get_ops(dev);
-
-	if (!ops->check_mdata) {
-		log_debug("check_mdata() method not defined\n");
-		return -ENOSYS;
-	}
-
-	return ops->check_mdata(dev);
-}
-
-/**
- * fwu_get_mdata() - Get a FWU metadata copy
- * @dev: FWU metadata device
- * @mdata: Copy of the FWU metadata
- *
- * Get a valid copy of the FWU metadata.
- *
- * Note: This function is to be called first when modifying any fields
- * in the metadata. The sequence of calls to modify any field in the
- * metadata would  be 1) fwu_get_mdata 2) Modify metadata, followed by
- * 3) fwu_update_mdata
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_get_mdata(struct udevice *dev, struct fwu_mdata *mdata)
-{
-	const struct fwu_mdata_ops *ops = device_get_ops(dev);
-
-	if (!ops->get_mdata) {
-		log_debug("get_mdata() method not defined\n");
-		return -ENOSYS;
-	}
-
-	return ops->get_mdata(dev, mdata);
-}
-
-/**
- * fwu_update_mdata() - Update the FWU metadata
- * @dev: FWU metadata device
- * @mdata: Copy of the FWU metadata
- *
- * Update the FWU metadata structure by writing to the
- * FWU metadata partitions.
- *
- * Note: This function is not to be called directly to update the
- * metadata fields. The sequence of function calls should be
- * 1) fwu_get_mdata() 2) Modify the medata fields 3) fwu_update_mdata()
- *
- * The sequence of updating the partitions should be, update the
- * primary metadata partition (first partition encountered), followed
- * by updating the secondary partition. With this update sequence, in
- * the rare scenario that the two metadata partitions are valid but do
- * not match, maybe due to power outage at the time of updating the
- * metadata copies, the secondary partition can be updated from the
- * primary.
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_update_mdata(struct udevice *dev, struct fwu_mdata *mdata)
-{
-	void *buf;
-	const struct fwu_mdata_ops *ops = device_get_ops(dev);
-
-	if (!ops->update_mdata) {
-		log_debug("get_mdata() method not defined\n");
-		return -ENOSYS;
-	}
-
-	/*
-	 * Calculate the crc32 for the updated FWU metadata
-	 * and put the updated value in the FWU metadata crc32
-	 * field
-	 */
-	buf = &mdata->version;
-	mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
-
-	return ops->update_mdata(dev, mdata);
-}
-
 UCLASS_DRIVER(fwu_mdata) = {
 	.id		= UCLASS_FWU_MDATA,
 	.name		= "fwu-mdata",
diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c
index bdaa10cd1d..b3c6953a6e 100644
--- a/drivers/fwu-mdata/gpt_blk.c
+++ b/drivers/fwu-mdata/gpt_blk.c
@@ -28,7 +28,7 @@ static uint g_mdata_part[2]; /* = {0, 0} to check against uninit parts */
 
 static int gpt_get_mdata_partitions(struct blk_desc *desc)
 {
-	int i, ret;
+	int i;
 	u32 nparts;
 	efi_guid_t part_type_guid;
 	struct disk_partition info;
@@ -52,12 +52,12 @@ static int gpt_get_mdata_partitions(struct blk_desc *desc)
 	if (nparts != 2) {
 		log_debug("Expect two copies of the FWU metadata instead of %d\n",
 			  nparts);
-		ret = -EINVAL;
-	} else {
-		ret = 0;
+		g_mdata_part[0] = 0;
+		g_mdata_part[1] = 0;
+		return -EINVAL;
 	}
 
-	return ret;
+	return 0;
 }
 
 static int gpt_get_mdata_disk_part(struct blk_desc *desc,
@@ -125,115 +125,6 @@ static int gpt_read_write_mdata(struct blk_desc *desc,
 	return 0;
 }
 
-static int fwu_gpt_update_mdata(struct udevice *dev, struct fwu_mdata *mdata)
-{
-	int ret;
-	struct blk_desc *desc;
-	struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
-
-	desc = dev_get_uclass_plat(priv->blk_dev);
-
-	ret = gpt_get_mdata_partitions(desc);
-	if (ret < 0) {
-		log_debug("Error getting the FWU metadata partitions\n");
-		return -ENOENT;
-	}
-
-	/* First write the primary partition */
-	ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, g_mdata_part[0]);
-	if (ret < 0) {
-		log_debug("Updating primary FWU metadata partition failed\n");
-		return ret;
-	}
-
-	/* And now the replica */
-	ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, g_mdata_part[1]);
-	if (ret < 0) {
-		log_debug("Updating secondary FWU metadata partition failed\n");
-		return ret;
-	}
-
-	return 0;
-}
-
-static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata *mdata)
-{
-	int ret;
-
-	ret = gpt_get_mdata_partitions(desc);
-	if (ret < 0) {
-		log_debug("Error getting the FWU metadata partitions\n");
-		return -ENOENT;
-	}
-
-	ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, g_mdata_part[0]);
-	if (ret < 0) {
-		log_debug("Failed to read the FWU metadata from the device\n");
-		return -EIO;
-	}
-
-	ret = fwu_verify_mdata(mdata, 1);
-	if (!ret)
-		return 0;
-
-	/*
-	 * Verification of the primary FWU metadata copy failed.
-	 * Try to read the replica.
-	 */
-	memset(mdata, '\0', sizeof(struct fwu_mdata));
-	ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, g_mdata_part[1]);
-	if (ret < 0) {
-		log_debug("Failed to read the FWU metadata from the device\n");
-		return -EIO;
-	}
-
-	ret = fwu_verify_mdata(mdata, 0);
-	if (!ret)
-		return 0;
-
-	/* Both the FWU metadata copies are corrupted. */
-	return -EIO;
-}
-
-static int fwu_gpt_get_mdata(struct udevice *dev, struct fwu_mdata *mdata)
-{
-	struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
-
-	return gpt_get_mdata(dev_get_uclass_plat(priv->blk_dev), mdata);
-}
-
-static int fwu_gpt_get_mdata_partitions(struct udevice *dev, uint *mdata_parts)
-{
-	struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
-	int err;
-
-	err = gpt_get_mdata_partitions(dev_get_uclass_plat(priv->blk_dev));
-	if (!err) {
-		mdata_parts[0] = g_mdata_part[0];
-		mdata_parts[1] = g_mdata_part[1];
-	}
-
-	return err;
-}
-
-static int fwu_gpt_read_mdata_partition(struct udevice *dev,
-					struct fwu_mdata *mdata, uint part_num)
-{
-	struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
-
-	return gpt_read_write_mdata(dev_get_uclass_plat(priv->blk_dev),
-				    mdata, MDATA_READ, part_num);
-}
-
-static int fwu_gpt_write_mdata_partition(struct udevice *dev,
-					struct fwu_mdata *mdata, uint part_num)
-{
-	struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
-
-	return gpt_read_write_mdata(dev_get_uclass_plat(priv->blk_dev),
-				    mdata, MDATA_WRITE, part_num);
-}
-
 static int fwu_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev)
 {
 	u32 phandle;
@@ -309,11 +200,6 @@ static int fwu_gpt_write_mdata(struct udevice *dev, struct fwu_mdata *mdata,
 static const struct fwu_mdata_ops fwu_gpt_blk_ops = {
 	.read_mdata = fwu_gpt_read_mdata,
 	.write_mdata = fwu_gpt_write_mdata,
-	.get_mdata = fwu_gpt_get_mdata,
-	.update_mdata = fwu_gpt_update_mdata,
-	.get_mdata_part_num = fwu_gpt_get_mdata_partitions,
-	.read_mdata_partition = fwu_gpt_read_mdata_partition,
-	.write_mdata_partition = fwu_gpt_write_mdata_partition,
 };
 
 static const struct udevice_id fwu_mdata_ids[] = {
diff --git a/include/fwu.h b/include/fwu.h
index 1a700c9e6a..23bd97fe86 100644
--- a/include/fwu.h
+++ b/include/fwu.h
@@ -18,11 +18,6 @@ struct fwu_mdata_gpt_blk_priv {
 	struct udevice *blk_dev;
 };
 
-/**
- * @mdata_check: check the validity of the FWU metadata partitions
- * @get_mdata() - Get a FWU metadata copy
- * @update_mdata() - Update the FWU metadata copy
- */
 struct fwu_mdata_ops {
 	/**
 	 * read_mdata() - Populate the asked FWU metadata copy
@@ -43,78 +38,6 @@ struct fwu_mdata_ops {
 	 * Return: 0 if OK, -ve on error
 	 */
 	int (*write_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
-
-	/**
-	 * check_mdata() - Check if the FWU metadata is valid
-	 * @dev:	FWU device
-	 *
-	 * Validate both copies of the FWU metadata. If one of the copies
-	 * has gone bad, restore it from the other copy.
-	 *
-	 * Return: 0 if OK, -ve on error
-	 */
-	int (*check_mdata)(struct udevice *dev);
-
-	/**
-	 * get_mdata() - Get a FWU metadata copy
-	 * @dev:	FWU device
-	 * @mdata:	Pointer to FWU metadata
-	 *
-	 * Get a valid copy of the FWU metadata.
-	 *
-	 * Return: 0 if OK, -ve on error
-	 */
-	int (*get_mdata)(struct udevice *dev, struct fwu_mdata *mdata);
-
-	/**
-	 * update_mdata() - Update the FWU metadata
-	 * @dev:	FWU device
-	 * @mdata:	Copy of the FWU metadata
-	 *
-	 * Update the FWU metadata structure by writing to the
-	 * FWU metadata partitions.
-	 *
-	 * Return: 0 if OK, -ve on error
-	 */
-	int (*update_mdata)(struct udevice *dev, struct fwu_mdata *mdata);
-
-	/**
-	 * get_mdata_part_num() - Get the FWU metadata partition numbers
-	 * @dev:		FWU metadata device
-	 * @mdata_parts:	 array for storing the metadata partition numbers
-	 *
-	 * Get the partition numbers on the storage device on which the
-	 * FWU metadata is stored. Two partition numbers will be returned.
-	 *
-	 * Return: 0 if OK, -ve on error
-	 */
-	int (*get_mdata_part_num)(struct udevice *dev, uint *mdata_parts);
-
-	/**
-	 * read_mdata_partition() - Read the FWU metadata from a partition
-	 * @dev:	FWU metadata device
-	 * @mdata:	Copy of the FWU metadata
-	 * @part_num:	Partition number from which FWU metadata is to be read
-	 *
-	 * Read the FWU metadata from the specified partition number
-	 *
-	 * Return: 0 if OK, -ve on error
-	 */
-	int (*read_mdata_partition)(struct udevice *dev,
-				    struct fwu_mdata *mdata, uint part_num);
-
-	/**
-	 * write_mdata_partition() - Write the FWU metadata to a partition
-	 * @dev:	FWU metadata device
-	 * @mdata:	Copy of the FWU metadata
-	 * @part_num:	Partition number to which FWU metadata is to be written
-	 *
-	 * Write the FWU metadata to the specified partition number
-	 *
-	 * Return: 0 if OK, -ve on error
-	 */
-	int (*write_mdata_partition)(struct udevice *dev,
-				     struct fwu_mdata *mdata, uint part_num);
 };
 
 #define FWU_MDATA_VERSION	0x1
@@ -167,102 +90,6 @@ int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
 */
 int fwu_get_verified_mdata(struct fwu_mdata *mdata);
 
-/**
- * fwu_check_mdata_validity() - Check for validity of the FWU metadata copies
- *
- * Read both the metadata copies from the storage media, verify their
- * checksum, and ascertain that both copies match. If one of the copies
- * has gone bad, restore it from the good copy.
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_check_mdata_validity(void);
-
-/**
- * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers
- * @dev: FWU metadata device
- * @mdata_parts: array for storing the metadata partition numbers
- *
- * Get the partition numbers on the storage device on which the
- * FWU metadata is stored. Two partition numbers will be returned
- * through the array.
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_get_mdata_part_num(struct udevice *dev, uint *mdata_parts);
-
-/**
- * fwu_read_mdata_partition() - Read the FWU metadata from a partition
- * @dev: FWU metadata device
- * @mdata: Copy of the FWU metadata
- * @part_num: Partition number from which FWU metadata is to be read
- *
- * Read the FWU metadata from the specified partition number
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_read_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata,
-			     uint part_num);
-
-/**
- * fwu_write_mdata_partition() - Write the FWU metadata to a partition
- * @dev: FWU metadata device
- * @mdata: Copy of the FWU metadata
- * @part_num: Partition number to which FWU metadata is to be written
- *
- * Write the FWU metadata to the specified partition number
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_write_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata,
-			      uint part_num);
-
-/**
- * fwu_get_mdata() - Get a FWU metadata copy
- * @dev: FWU metadata device
- * @mdata: Copy of the FWU metadata
- *
- * Get a valid copy of the FWU metadata.
- *
- * Note: This function is to be called first when modifying any fields
- * in the metadata. The sequence of calls to modify any field in the
- * metadata would  be 1) fwu_get_mdata 2) Modify metadata, followed by
- * 3) fwu_update_mdata
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_get_mdata(struct udevice *dev, struct fwu_mdata *mdata);
-
-/**
- * fwu_update_mdata() - Update the FWU metadata
- * @dev: FWU metadata device
- * @mdata: Copy of the FWU metadata
- *
- * Update the FWU metadata structure by writing to the
- * FWU metadata partitions.
- *
- * Note: This function is not to be called directly to update the
- * metadata fields. The sequence of function calls should be
- * 1) fwu_get_mdata() 2) Modify the medata fields 3) fwu_update_mdata()
- *
- * The sequence of updating the partitions should be, update the
- * primary metadata partition (first partition encountered), followed
- * by updating the secondary partition. With this update sequence, in
- * the rare scenario that the two metadata partitions are valid but do
- * not match, maybe due to power outage at the time of updating the
- * metadata copies, the secondary partition can be updated from the
- * primary.
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_update_mdata(struct udevice *dev, struct fwu_mdata *mdata);
-
 /**
  * fwu_get_active_index() - Get active_index from the FWU metadata
  * @active_idxp: active_index value to be read
@@ -303,18 +130,6 @@ int fwu_set_active_index(uint active_idx);
  */
 int fwu_get_image_index(u8 *image_index);
 
-/**
- * fwu_mdata_check() - Check if the FWU metadata is valid
- * @dev: FWU metadata device
- *
- * Validate both copies of the FWU metadata. If one of the copies
- * has gone bad, restore it from the other copy.
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_mdata_check(struct udevice *dev);
-
 /**
  * fwu_revert_boot_index() - Revert the active index in the FWU metadata
  *
@@ -327,20 +142,6 @@ int fwu_mdata_check(struct udevice *dev);
  */
 int fwu_revert_boot_index(void);
 
-/**
- * fwu_verify_mdata() - Verify the FWU metadata
- * @mdata: FWU metadata structure
- * @pri_part: FWU metadata partition is primary or secondary
- *
- * Verify the FWU metadata by computing the CRC32 for the metadata
- * structure and comparing it against the CRC32 value stored as part
- * of the structure.
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part);
-
 /**
  * fwu_accept_image() - Set the Acceptance bit for the image
  * @img_type_id: GUID of the image type for which the accepted bit is to be
diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
index 56299f1b2f..9c0a8e2bb1 100644
--- a/lib/fwu_updates/fwu.c
+++ b/lib/fwu_updates/fwu.c
@@ -33,26 +33,6 @@ enum {
 	BOTH_PARTS,
 };
 
-static int fwu_get_dev_mdata(struct udevice **dev, struct fwu_mdata *mdata)
-{
-	int ret;
-
-	ret = uclass_first_device_err(UCLASS_FWU_MDATA, dev);
-	if (ret) {
-		log_debug("Cannot find fwu device\n");
-		return ret;
-	}
-
-	if (!mdata)
-		return 0;
-
-	ret = fwu_get_mdata(*dev, mdata);
-	if (ret < 0)
-		log_debug("Unable to get valid FWU metadata\n");
-
-	return ret;
-}
-
 static int trial_counter_update(u16 *trial_state_ctr)
 {
 	bool delete;
@@ -288,136 +268,6 @@ ret_mdata:
 	return err;
 }
 
-/**
- * fwu_verify_mdata() - Verify the FWU metadata
- * @mdata: FWU metadata structure
- * @pri_part: FWU metadata partition is primary or secondary
- *
- * Verify the FWU metadata by computing the CRC32 for the metadata
- * structure and comparing it against the CRC32 value stored as part
- * of the structure.
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part)
-{
-	u32 calc_crc32;
-	void *buf;
-
-	buf = &mdata->version;
-	calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
-
-	if (calc_crc32 != mdata->crc32) {
-		log_debug("crc32 check failed for %s FWU metadata partition\n",
-			  pri_part ? "primary" : "secondary");
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
-/**
- * fwu_check_mdata_validity() - Check for validity of the FWU metadata copies
- *
- * Read both the metadata copies from the storage media, verify their checksum,
- * and ascertain that both copies match. If one of the copies has gone bad,
- * restore it from the good copy.
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_check_mdata_validity(void)
-{
-	int ret;
-	struct udevice *dev;
-	struct fwu_mdata pri_mdata;
-	struct fwu_mdata secondary_mdata;
-	uint mdata_parts[2];
-	uint valid_partitions, invalid_partitions;
-
-	ret = fwu_get_dev_mdata(&dev, NULL);
-	if (ret)
-		return ret;
-
-	/*
-	 * Check if the platform has defined its own
-	 * function to check the metadata partitions'
-	 * validity. If so, that takes precedence.
-	 */
-	ret = fwu_mdata_check(dev);
-	if (!ret || ret != -ENOSYS)
-		return ret;
-
-	/*
-	 * Two FWU metadata partitions are expected.
-	 * If we don't have two, user needs to create
-	 * them first
-	 */
-	valid_partitions = 0;
-	ret = fwu_get_mdata_part_num(dev, mdata_parts);
-	if (ret < 0) {
-		log_debug("Error getting the FWU metadata partitions\n");
-		return -ENOENT;
-	}
-
-	ret = fwu_read_mdata_partition(dev, &pri_mdata, mdata_parts[0]);
-	if (!ret) {
-		ret = fwu_verify_mdata(&pri_mdata, 1);
-		if (!ret)
-			valid_partitions |= PRIMARY_PART;
-	}
-
-	ret = fwu_read_mdata_partition(dev, &secondary_mdata, mdata_parts[1]);
-	if (!ret) {
-		ret = fwu_verify_mdata(&secondary_mdata, 0);
-		if (!ret)
-			valid_partitions |= SECONDARY_PART;
-	}
-
-	if (valid_partitions == (PRIMARY_PART | SECONDARY_PART)) {
-		/*
-		 * Before returning, check that both the
-		 * FWU metadata copies are the same. If not,
-		 * populate the secondary partition from the
-		 * primary partition copy.
-		 */
-		if (!memcmp(&pri_mdata, &secondary_mdata,
-			    sizeof(struct fwu_mdata))) {
-			ret = 0;
-		} else {
-			log_info("Both FWU metadata copies are valid but do not match.");
-			log_info(" Restoring the secondary partition from the primary\n");
-			ret = fwu_write_mdata_partition(dev, &pri_mdata,
-							mdata_parts[1]);
-			if (ret)
-				log_debug("Restoring secondary FWU metadata partition failed\n");
-		}
-		goto out;
-	}
-
-	if (!(valid_partitions & BOTH_PARTS)) {
-		log_info("Both FWU metadata partitions invalid\n");
-		ret = -EBADMSG;
-		goto out;
-	}
-
-	invalid_partitions = valid_partitions ^ BOTH_PARTS;
-	ret = fwu_write_mdata_partition(dev,
-					(invalid_partitions == PRIMARY_PART) ?
-					&secondary_mdata : &pri_mdata,
-					(invalid_partitions == PRIMARY_PART) ?
-					mdata_parts[0] : mdata_parts[1]);
-
-	if (ret)
-		log_debug("Restoring %s FWU metadata partition failed\n",
-			  (invalid_partitions == PRIMARY_PART) ?
-			  "primary" : "secondary");
-
-out:
-	return ret;
-}
-
 /**
  * fwu_get_active_index() - Get active_index from the FWU metadata
  * @active_idx: active_index value to be read
@@ -430,19 +280,14 @@ out:
  */
 int fwu_get_active_index(uint *active_idx)
 {
-	int ret;
-	struct udevice *dev;
-	struct fwu_mdata mdata = { 0 };
-
-	ret = fwu_get_dev_mdata(&dev, &mdata);
-	if (ret)
-		return ret;
+	int ret = 0;
+	struct fwu_mdata *mdata = &g_mdata;
 
 	/*
 	 * Found the FWU metadata partition, now read the active_index
 	 * value
 	 */
-	*active_idx = mdata.active_index;
+	*active_idx = mdata->active_index;
 	if (*active_idx >= CONFIG_FWU_NUM_BANKS) {
 		log_debug("Active index value read is incorrect\n");
 		ret = -EINVAL;
@@ -463,30 +308,25 @@ int fwu_get_active_index(uint *active_idx)
 int fwu_set_active_index(uint active_idx)
 {
 	int ret;
-	struct udevice *dev;
-	struct fwu_mdata mdata = { 0 };
+	struct fwu_mdata *mdata = &g_mdata;
 
 	if (active_idx >= CONFIG_FWU_NUM_BANKS) {
 		log_debug("Invalid active index value\n");
 		return -EINVAL;
 	}
 
-	ret = fwu_get_dev_mdata(&dev, &mdata);
-	if (ret)
-		return ret;
-
 	/*
 	 * Update the active index and previous_active_index fields
 	 * in the FWU metadata
 	 */
-	mdata.previous_active_index = mdata.active_index;
-	mdata.active_index = active_idx;
+	mdata->previous_active_index = mdata->active_index;
+	mdata->active_index = active_idx;
 
 	/*
 	 * Now write this updated FWU metadata to both the
 	 * FWU metadata partitions
 	 */
-	ret = fwu_update_mdata(dev, &mdata);
+	ret = fwu_sync_mdata(mdata, BOTH_PARTS);
 	if (ret) {
 		log_debug("Failed to update FWU metadata partitions\n");
 		ret = -EIO;
@@ -516,15 +356,10 @@ int fwu_get_image_index(u8 *image_index)
 	u8 alt_num;
 	uint update_bank;
 	efi_guid_t *image_guid, image_type_id;
-	struct udevice *dev;
-	struct fwu_mdata mdata = { 0 };
+	struct fwu_mdata *mdata = &g_mdata;
 	struct fwu_image_entry *img_entry;
 	struct fwu_image_bank_info *img_bank_info;
 
-	ret = fwu_get_dev_mdata(&dev, &mdata);
-	if (ret)
-		return ret;
-
 	ret = fwu_plat_get_update_index(&update_bank);
 	if (ret) {
 		log_debug("Failed to get the FWU update bank\n");
@@ -545,11 +380,11 @@ int fwu_get_image_index(u8 *image_index)
 	 */
 	for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) {
 		if (!guidcmp(&image_type_id,
-			     &mdata.img_entry[i].image_type_uuid)) {
-			img_entry = &mdata.img_entry[i];
+			     &mdata->img_entry[i].image_type_uuid)) {
+			img_entry = &mdata->img_entry[i];
 			img_bank_info = &img_entry->img_bank_info[update_bank];
 			image_guid = &img_bank_info->image_uuid;
-			ret = fwu_plat_get_alt_num(dev, image_guid, &alt_num);
+			ret = fwu_plat_get_alt_num(g_dev, image_guid, &alt_num);
 			if (ret) {
 				log_debug("alt_num not found for partition with GUID %pUs\n",
 					  image_guid);
@@ -584,26 +419,21 @@ int fwu_revert_boot_index(void)
 {
 	int ret;
 	u32 cur_active_index;
-	struct udevice *dev;
-	struct fwu_mdata mdata = { 0 };
-
-	ret = fwu_get_dev_mdata(&dev, &mdata);
-	if (ret)
-		return ret;
+	struct fwu_mdata *mdata = &g_mdata;
 
 	/*
 	 * Swap the active index and previous_active_index fields
 	 * in the FWU metadata
 	 */
-	cur_active_index = mdata.active_index;
-	mdata.active_index = mdata.previous_active_index;
-	mdata.previous_active_index = cur_active_index;
+	cur_active_index = mdata->active_index;
+	mdata->active_index = mdata->previous_active_index;
+	mdata->previous_active_index = cur_active_index;
 
 	/*
 	 * Now write this updated FWU metadata to both the
 	 * FWU metadata partitions
 	 */
-	ret = fwu_update_mdata(dev, &mdata);
+	ret = fwu_sync_mdata(mdata, BOTH_PARTS);
 	if (ret) {
 		log_debug("Failed to update FWU metadata partitions\n");
 		ret = -EIO;
@@ -630,16 +460,11 @@ int fwu_revert_boot_index(void)
 static int fwu_clrset_image_accept(efi_guid_t *img_type_id, u32 bank, u8 action)
 {
 	int ret, i;
-	struct udevice *dev;
-	struct fwu_mdata mdata = { 0 };
+	struct fwu_mdata *mdata = &g_mdata;
 	struct fwu_image_entry *img_entry;
 	struct fwu_image_bank_info *img_bank_info;
 
-	ret = fwu_get_dev_mdata(&dev, &mdata);
-	if (ret)
-		return ret;
-
-	img_entry = &mdata.img_entry[0];
+	img_entry = &mdata->img_entry[0];
 	for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) {
 		if (!guidcmp(&img_entry[i].image_type_uuid, img_type_id)) {
 			img_bank_info = &img_entry[i].img_bank_info[bank];
@@ -648,7 +473,7 @@ static int fwu_clrset_image_accept(efi_guid_t *img_type_id, u32 bank, u8 action)
 			else
 				img_bank_info->accepted = 0;
 
-			ret = fwu_update_mdata(dev, &mdata);
+			ret = fwu_sync_mdata(mdata, BOTH_PARTS);
 			goto out;
 		}
 	}
@@ -783,8 +608,6 @@ static int fwu_boottime_checks(void *ctx, struct event *event)
 {
 	int ret;
 	u32 boot_idx, active_idx;
-	struct udevice *dev;
-	struct fwu_mdata mdata = { 0 };
 
 	/* Don't have boot time checks on sandbox */
 	if (IS_ENABLED(CONFIG_SANDBOX)) {
@@ -792,9 +615,17 @@ static int fwu_boottime_checks(void *ctx, struct event *event)
 		return 0;
 	}
 
-	ret = fwu_check_mdata_validity();
-	if (ret)
-		return 0;
+	ret = uclass_first_device_err(UCLASS_FWU_MDATA, &g_dev);
+	if (ret) {
+		log_debug("Cannot find fwu device\n");
+		return ret;
+	}
+
+	ret = fwu_get_verified_mdata(NULL);
+	if (ret) {
+		log_debug("Unable to read meta-data\n");
+		return ret;
+	}
 
 	/*
 	 * Get the Boot Index, i.e. the bank from
@@ -830,11 +661,7 @@ static int fwu_boottime_checks(void *ctx, struct event *event)
 	if (efi_init_obj_list() != EFI_SUCCESS)
 		return 0;
 
-	ret = fwu_get_dev_mdata(&dev, &mdata);
-	if (ret)
-		return ret;
-
-	in_trial = in_trial_state(&mdata);
+	in_trial = in_trial_state(&g_mdata);
 	if (!in_trial || (ret = fwu_trial_count_update()) > 0)
 		ret = trial_counter_update(NULL);
 
-- 
2.34.1

[PATCHv4 5/5] fwu: rename fwu_get_verified_mdata to fwu_get_mdata

[jassisinghbrar]

From: Jassi Brar <jaswinder.singh@linaro.org>

fwu_get_mdata() sounds more appropriate than fwu_get_verified_mdata()

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 cmd/fwu_mdata.c       | 2 +-
 include/fwu.h         | 4 ++--
 lib/fwu_updates/fwu.c | 6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/cmd/fwu_mdata.c b/cmd/fwu_mdata.c
index 9b70340368..5ecda455df 100644
--- a/cmd/fwu_mdata.c
+++ b/cmd/fwu_mdata.c
@@ -46,7 +46,7 @@ int do_fwu_mdata_read(struct cmd_tbl *cmdtp, int flag,
 	int ret = CMD_RET_SUCCESS, res;
 	struct fwu_mdata mdata;
 
-	res = fwu_get_verified_mdata(&mdata);
+	res = fwu_get_mdata(&mdata);
 	if (res < 0) {
 		log_err("Unable to get valid FWU metadata\n");
 		ret = CMD_RET_FAILURE;
diff --git a/include/fwu.h b/include/fwu.h
index 23bd97fe86..ea25aca2cd 100644
--- a/include/fwu.h
+++ b/include/fwu.h
@@ -80,7 +80,7 @@ int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
 int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
 
 /**
- * fwu_get_verified_mdata() - Read, verify and return the FWU metadata
+ * fwu_get_mdata() - Read, verify and return the FWU metadata
  *
  * Read both the metadata copies from the storage media, verify their checksum,
  * and ascertain that both copies match. If one of the copies has gone bad,
@@ -88,7 +88,7 @@ int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
  *
  * Return: 0 if OK, -ve on error
 */
-int fwu_get_verified_mdata(struct fwu_mdata *mdata);
+int fwu_get_mdata(struct fwu_mdata *mdata);
 
 /**
  * fwu_get_active_index() - Get active_index from the FWU metadata
diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
index 9c0a8e2bb1..cf7e0ad078 100644
--- a/lib/fwu_updates/fwu.c
+++ b/lib/fwu_updates/fwu.c
@@ -182,7 +182,7 @@ static inline int mdata_crc_check(struct fwu_mdata *mdata)
 }
 
 /**
- * fwu_get_verified_mdata() - Read, verify and return the FWU metadata
+ * fwu_get_mdata() - Read, verify and return the FWU metadata
  *
  * Read both the metadata copies from the storage media, verify their checksum,
  * and ascertain that both copies match. If one of the copies has gone bad,
@@ -190,7 +190,7 @@ static inline int mdata_crc_check(struct fwu_mdata *mdata)
  *
  * Return: 0 if OK, -ve on error
  */
-int fwu_get_verified_mdata(struct fwu_mdata *mdata)
+int fwu_get_mdata(struct fwu_mdata *mdata)
 {
 	int err;
 	bool pri_ok, sec_ok;
@@ -621,7 +621,7 @@ static int fwu_boottime_checks(void *ctx, struct event *event)
 		return ret;
 	}
 
-	ret = fwu_get_verified_mdata(NULL);
+	ret = fwu_get_mdata(NULL);
 	if (ret) {
 		log_debug("Unable to read meta-data\n");
 		return ret;
-- 
2.34.1

Re: [PATCHv4 0/5] FWU: Handle meta-data in common code

[Simon Glass]

Hi,

On Sat, 4 Feb 2023 at 20:01, <jassisinghbrar@gmail.com> wrote:
>
> From: Jassi Brar <jaswinder.singh@linaro.org>
>
> The patchset reduces ~400 lines of code, while keeping the functionality same and making
> meta-data operations much faster (by using cached structures).
>
> Issue:
>  meta-data copies (primary and secondary) are being handled by the backend/storage layer
> instead of the common core in fwu.c (as also noted by Ilias)  that is, gpt_blk.c manages
> meta-data and similarly raw_mtd.c will have to do the same when it arrives. The code
> could by make smaller, cleaner and optimised.
>
> Basic idea:
>  Introduce  .read_mdata() and .write_mdata() in fwu_mdata_ops  that simply read/write
> meta-data copy. The core code takes care of integrity and redundancy of the meta-data,
> as a result we can get rid of every other callback .get_mdata() .update_mdata()
> .get_mdata_part_num()  .read_mdata_partition()  .write_mdata_partition() and the
> corresponding wrapper functions thereby making the code 100s of LOC smaller.
>
> Get rid of fwu_check_mdata_validity() and fwu_mdata_check() which expected underlying
> layer to manage and verify mdata copies.
> Implement  fwu_get_verified_mdata(struct fwu_mdata *mdata) public function that reads,
> verifies and, if needed, fixes the meta-data copies.
>
> Verified copy of meta-data is now cached as 'g_mdata' in fwu.c, which avoids multiple
> low-level expensive read and parse calls.
> gpt meta-data partition numbers are now cached in gpt_blk.c, so that we don't have to do expensive part_get_info() and uid ops.
>
> Changes since v3:
>         * Fix error log wording
>         * call fwu_write_mdata() with part & PRIMARY_PART ? true: false
>
> Changes since v2:
>         * Drop whitespace changes
>         * Fix missing mdata copy before return
>
> Changes since v1:
>         * Fix typos and misc cosmetic changes
>         * Catch error returns
>
>
> Jassi Brar (5):
>   fwu: gpt: use cached meta-data partition numbers
>   fwu: move meta-data management in core
>   fwu: gpt: implement read_mdata and write_mdata callbacks
>   fwu: meta-data: switch to management by common code
>   fwu: rename fwu_get_verified_mdata to fwu_get_mdata
>
>  cmd/fwu_mdata.c                      |  17 +-
>  drivers/fwu-mdata/fwu-mdata-uclass.c | 151 +-------------
>  drivers/fwu-mdata/gpt_blk.c          | 175 +++++-----------
>  include/fwu.h                        | 198 ++----------------
>  lib/fwu_updates/fwu.c                | 294 +++++++++++----------------
>  5 files changed, 207 insertions(+), 628 deletions(-)
>
> --
> 2.34.1
>

Nice to see this. It would be great to expand the tests in
test/dm/fwu_mdata.c at some point.

Regards,
Simon

Re: [PATCHv4 4/5] fwu: meta-data: switch to management by common code

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1147 bytes --]

On Sat, Feb 04, 2023 at 09:02:02PM -0600, jassisinghbrar@gmail.com wrote:

> From: Jassi Brar <jaswinder.singh@linaro.org>
> 
> The common code can now read, verify and fix meta-data copies
> while exposing one consistent structure to users.
>  Only the .read_mdata() and .write_mdata() callbacks of fwu_mdata_ops
> are needed. Get rid of .get_mdata() .update_mdata() .get_mdata_part_num()
> .read_mdata_partition() and .write_mdata_partition() and also the
> corresponding wrapper functions.
> 
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>  cmd/fwu_mdata.c                      |  17 +-
>  drivers/fwu-mdata/fwu-mdata-uclass.c | 165 -------------------
>  drivers/fwu-mdata/gpt_blk.c          | 124 +-------------
>  include/fwu.h                        | 199 -----------------------
>  lib/fwu_updates/fwu.c                | 235 ++++-----------------------
>  5 files changed, 38 insertions(+), 702 deletions(-)

This forgot to update test/dm/fwu_mdata.c and so CI fails:
https://source.denx.de/u-boot/u-boot/-/jobs/580820#L347

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCHv4 0/5] FWU: Handle meta-data in common code

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2737 bytes --]

On Sat, Feb 04, 2023 at 09:00:56PM -0600, jassisinghbrar@gmail.com wrote:

> From: Jassi Brar <jaswinder.singh@linaro.org>
> 
> The patchset reduces ~400 lines of code, while keeping the functionality same and making
> meta-data operations much faster (by using cached structures).
> 
> Issue:
>  meta-data copies (primary and secondary) are being handled by the backend/storage layer
> instead of the common core in fwu.c (as also noted by Ilias)  that is, gpt_blk.c manages
> meta-data and similarly raw_mtd.c will have to do the same when it arrives. The code
> could by make smaller, cleaner and optimised.
> 
> Basic idea:
>  Introduce  .read_mdata() and .write_mdata() in fwu_mdata_ops  that simply read/write
> meta-data copy. The core code takes care of integrity and redundancy of the meta-data,
> as a result we can get rid of every other callback .get_mdata() .update_mdata()
> .get_mdata_part_num()  .read_mdata_partition()  .write_mdata_partition() and the
> corresponding wrapper functions thereby making the code 100s of LOC smaller.
> 
> Get rid of fwu_check_mdata_validity() and fwu_mdata_check() which expected underlying
> layer to manage and verify mdata copies.
> Implement  fwu_get_verified_mdata(struct fwu_mdata *mdata) public function that reads,
> verifies and, if needed, fixes the meta-data copies.
> 
> Verified copy of meta-data is now cached as 'g_mdata' in fwu.c, which avoids multiple
> low-level expensive read and parse calls.
> gpt meta-data partition numbers are now cached in gpt_blk.c, so that we don't have to do expensive part_get_info() and uid ops.
> 
> Changes since v3:
>         * Fix error log wording
>         * call fwu_write_mdata() with part & PRIMARY_PART ? true: false
> 
> Changes since v2:
>         * Drop whitespace changes
>         * Fix missing mdata copy before return
> 
> Changes since v1:
>         * Fix typos and misc cosmetic changes
>         * Catch error returns
> 
> 
> Jassi Brar (5):
>   fwu: gpt: use cached meta-data partition numbers
>   fwu: move meta-data management in core
>   fwu: gpt: implement read_mdata and write_mdata callbacks
>   fwu: meta-data: switch to management by common code
>   fwu: rename fwu_get_verified_mdata to fwu_get_mdata
> 
>  cmd/fwu_mdata.c                      |  17 +-
>  drivers/fwu-mdata/fwu-mdata-uclass.c | 151 +-------------
>  drivers/fwu-mdata/gpt_blk.c          | 175 +++++-----------
>  include/fwu.h                        | 198 ++----------------
>  lib/fwu_updates/fwu.c                | 294 +++++++++++----------------
>  5 files changed, 207 insertions(+), 628 deletions(-)

FWIW, aside from the failure I reported, everything else seems fine.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCHv4 2/5] fwu: move meta-data management in core

[Ilias Apalodimas]

Hi Jassi,

Apologies for the delay

On Sat, Feb 04, 2023 at 09:01:46PM -0600, jassisinghbrar@gmail.com wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
>
> Instead of each i/f having to implement their own meta-data verification
> and storage, move the logic in common code. This simplifies the i/f code
> much simpler and compact.
>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>  drivers/fwu-mdata/fwu-mdata-uclass.c |  34 +++++++
>  include/fwu.h                        |  41 ++++++++
>  lib/fwu_updates/fwu.c                | 135 ++++++++++++++++++++++++++-
>  3 files changed, 206 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c
> index b477e9603f..e03773c584 100644
> --- a/drivers/fwu-mdata/fwu-mdata-uclass.c
> +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
> @@ -16,6 +16,40 @@
>  #include <linux/types.h>
>  #include <u-boot/crc.h>
>
> +/**
> + * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
> +{
> +	const struct fwu_mdata_ops *ops = device_get_ops(dev);
> +
> +	if (!ops->read_mdata) {
> +		log_debug("read_mdata() method not defined\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->read_mdata(dev, mdata, primary);
> +}
> +
> +/**
> + * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
> +{
> +	const struct fwu_mdata_ops *ops = device_get_ops(dev);
> +
> +	if (!ops->write_mdata) {
> +		log_debug("write_mdata() method not defined\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->write_mdata(dev, mdata, primary);
> +}
> +
>  /**
>   * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers
>   * @dev: FWU metadata device
> diff --git a/include/fwu.h b/include/fwu.h
> index 0919ced812..1a700c9e6a 100644
> --- a/include/fwu.h
> +++ b/include/fwu.h
> @@ -24,6 +24,26 @@ struct fwu_mdata_gpt_blk_priv {
>   * @update_mdata() - Update the FWU metadata copy
>   */
>  struct fwu_mdata_ops {
> +	/**
> +	 * read_mdata() - Populate the asked FWU metadata copy
> +	 * @dev: FWU metadata device
> +	 * @mdata: Copy of the FWU metadata
> +	 * @primary: If primary or secondary copy of meta-data is to be read
> +	 *
> +	 * Return: 0 if OK, -ve on error
> +	 */
> +	int (*read_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> +
> +	/**
> +	 * write_mdata() - Write the given FWU metadata copy
> +	 * @dev: FWU metadata device
> +	 * @mdata: Copy of the FWU metadata
> +	 * @primary: If primary or secondary copy of meta-data is to be written
> +	 *
> +	 * Return: 0 if OK, -ve on error
> +	 */
> +	int (*write_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> +
>  	/**
>  	 * check_mdata() - Check if the FWU metadata is valid
>  	 * @dev:	FWU device
> @@ -126,6 +146,27 @@ struct fwu_mdata_ops {
>  	EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \
>  		 0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8)
>
> +/**
> + * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
> + */
> +int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> +
> +/**
> + * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
> + */
> +int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> +
> +/**
> + * fwu_get_verified_mdata() - Read, verify and return the FWU metadata
> + *
> + * Read both the metadata copies from the storage media, verify their checksum,
> + * and ascertain that both copies match. If one of the copies has gone bad,
> + * restore it from the good copy.
> + *
> + * Return: 0 if OK, -ve on error
> +*/
> +int fwu_get_verified_mdata(struct fwu_mdata *mdata);
> +
>  /**
>   * fwu_check_mdata_validity() - Check for validity of the FWU metadata copies
>   *
> diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
> index 5313d07302..56299f1b2f 100644
> --- a/lib/fwu_updates/fwu.c
> +++ b/lib/fwu_updates/fwu.c
> @@ -15,13 +15,13 @@
>  #include <linux/errno.h>
>  #include <linux/types.h>
>
> +#include <u-boot/crc.h>
> +
> +static struct fwu_mdata g_mdata; /* = {0} makes uninit crc32 always invalid */
> +static struct udevice *g_dev;
>  static u8 in_trial;
>  static u8 boottime_check;
>
> -#include <linux/errno.h>
> -#include <linux/types.h>
> -#include <u-boot/crc.h>
> -
>  enum {
>  	IMAGE_ACCEPT_SET = 1,
>  	IMAGE_ACCEPT_CLEAR,
> @@ -161,6 +161,133 @@ static int fwu_get_image_type_id(u8 *image_index, efi_guid_t *image_type_id)
>  	return -ENOENT;
>  }
>
> +/**
> + * fwu_sync_mdata() - Update given meta-data partition(s) with the copy provided
> + * @mdata: FWU metadata structure
> + * @part: Bitmask of FWU metadata partitions to be written to
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +static int fwu_sync_mdata(struct fwu_mdata *mdata, int part)
> +{
> +	void *buf = &mdata->version;
> +	int err = 0;
> +
> +	/*
> +	 * Calculate the crc32 for the updated FWU metadata
> +	 * and put the updated value in the FWU metadata crc32
> +	 * field
> +	 */
> +	mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
> +
> +	err = fwu_write_mdata(g_dev, mdata, part & PRIMARY_PART ? true : false);
> +	if (err) {
> +		log_err("Unable to write %s mdata\n",
> +				part & PRIMARY_PART ?  "primary": "secondary");
> +		return err;
> +	}
> +
> +	/* update the cached copy of meta-data */
> +	memcpy(&g_mdata, mdata, sizeof(struct fwu_mdata));
> +
> +	return 0;
> +}
> +
> +static inline int mdata_crc_check(struct fwu_mdata *mdata)
> +{
> +	void *buf = &mdata->version;
> +	u32 calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
> +
> +	return calc_crc32 == mdata->crc32 ? 0 : -EINVAL;
> +}
> +
> +/**
> + * fwu_get_verified_mdata() - Read, verify and return the FWU metadata
> + *
> + * Read both the metadata copies from the storage media, verify their checksum,
> + * and ascertain that both copies match. If one of the copies has gone bad,
> + * restore it from the good copy.
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int fwu_get_verified_mdata(struct fwu_mdata *mdata)
> +{
> +	int err;
> +	bool pri_ok, sec_ok;
> +	struct fwu_mdata s, *p_mdata, *s_mdata;
> +
> +	p_mdata = &g_mdata;
> +	s_mdata = &s;
> +
> +	/* if mdata already read and ready */
> +	err = mdata_crc_check(p_mdata);
> +	if (!err)
> +		goto ret_mdata;
> +	/* else read, verify and, if needed, fix mdata */
> +
> +	pri_ok = false;
> +	err = fwu_read_mdata(g_dev, p_mdata, true);
> +	if (!err) {
> +		err = mdata_crc_check(p_mdata);
> +		if (!err)
> +			pri_ok = true;
> +		else
> +			log_debug("primary mdata: crc32 failed\n");
> +	}
> +
> +	sec_ok = false;
> +	err = fwu_read_mdata(g_dev, s_mdata, false);
> +	if (!err) {
> +		err = mdata_crc_check(s_mdata);
> +		if (!err)
> +			sec_ok = true;
> +		else
> +			log_debug("secondary mdata: crc32 failed\n");
> +	}

Isn't it better to define pri_ok, sec_ok and their equivalent mdata as
arrays ? IOW something along the lines of

bool parts_ok[2] = { false };
struct fwu_mdata parts_mdata[2];

parts_mdata[0] = &g_mdata;
parts_mdata[1] = .....
for (i = 0; i < 2; i++) {
    err = fwu_read_mdata(g_dev, parts_mdata[i], !(i % 2) ? true : false);
    if (!err)
	err = mdata_crc_check(parts_mdata[i]);
	etc....
}

> +
> +	if (pri_ok && sec_ok) {

And then also adjust this part?

> +		/*
> +		 * Before returning, check that both the
> +		 * FWU metadata copies are the same.
> +		 */
> +		err = memcmp(p_mdata, s_mdata, sizeof(struct fwu_mdata));
> +		if (!err)
> +			goto ret_mdata;
> +
> +		/*
> +		 * If not, populate the secondary partition from the
> +		 * primary partition copy.
> +		 */
> +		log_info("Both FWU metadata copies are valid but do not match.");
> +		log_info(" Restoring the secondary partition from the primary\n");
> +		sec_ok = false;
> +	}
> +
> +	if (!pri_ok) {
> +		memcpy(p_mdata, s_mdata, sizeof(struct fwu_mdata));
> +		err = fwu_sync_mdata(p_mdata, PRIMARY_PART);
> +		if (err) {
> +			log_debug("mdata : primary write failed\n");
> +			return err;
> +		}
> +	}
> +
> +	if (!sec_ok) {
> +		memcpy(s_mdata, p_mdata, sizeof(struct fwu_mdata));
> +		err = fwu_sync_mdata(s_mdata, SECONDARY_PART);
> +		if (err) {
> +			log_debug("mdata : secondary write failed\n");
> +			return err;
> +		}
> +	}

And this could also be folded into a for loop

> +
> +ret_mdata:
> +	if (!err && mdata)
> +		memcpy(mdata, p_mdata, sizeof(struct fwu_mdata));
> +
> +	return err;
> +}
> +
>  /**
>   * fwu_verify_mdata() - Verify the FWU metadata
>   * @mdata: FWU metadata structure
> --
> 2.34.1
>

Thanks
/Ilias

Re: [PATCHv4 4/5] fwu: meta-data: switch to management by common code

[Ilias Apalodimas]

Hi Jassi,

This looks good to me, but Sughosh will test in on the ST board.

Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

On Sat, Feb 04, 2023 at 09:02:02PM -0600, jassisinghbrar@gmail.com wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
>
> The common code can now read, verify and fix meta-data copies
> while exposing one consistent structure to users.
>  Only the .read_mdata() and .write_mdata() callbacks of fwu_mdata_ops
> are needed. Get rid of .get_mdata() .update_mdata() .get_mdata_part_num()
> .read_mdata_partition() and .write_mdata_partition() and also the
> corresponding wrapper functions.
>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>  cmd/fwu_mdata.c                      |  17 +-
>  drivers/fwu-mdata/fwu-mdata-uclass.c | 165 -------------------
>  drivers/fwu-mdata/gpt_blk.c          | 124 +-------------
>  include/fwu.h                        | 199 -----------------------
>  lib/fwu_updates/fwu.c                | 235 ++++-----------------------
>  5 files changed, 38 insertions(+), 702 deletions(-)
>
> diff --git a/cmd/fwu_mdata.c b/cmd/fwu_mdata.c
> index f04af27de6..9b70340368 100644
> --- a/cmd/fwu_mdata.c
> +++ b/cmd/fwu_mdata.c
> @@ -43,23 +43,10 @@ static void print_mdata(struct fwu_mdata *mdata)
>  int do_fwu_mdata_read(struct cmd_tbl *cmdtp, int flag,
>  		     int argc, char * const argv[])
>  {
> -	struct udevice *dev;
>  	int ret = CMD_RET_SUCCESS, res;
> -	struct fwu_mdata mdata = { 0 };
> +	struct fwu_mdata mdata;
>
> -	if (uclass_get_device(UCLASS_FWU_MDATA, 0, &dev) || !dev) {
> -		log_err("Unable to get FWU metadata device\n");
> -		return CMD_RET_FAILURE;
> -	}
> -
> -	res = fwu_check_mdata_validity();
> -	if (res < 0) {
> -		log_err("FWU Metadata check failed\n");
> -		ret = CMD_RET_FAILURE;
> -		goto out;
> -	}
> -
> -	res = fwu_get_mdata(dev, &mdata);
> +	res = fwu_get_verified_mdata(&mdata);
>  	if (res < 0) {
>  		log_err("Unable to get valid FWU metadata\n");
>  		ret = CMD_RET_FAILURE;
> diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c
> index e03773c584..0a8edaaa41 100644
> --- a/drivers/fwu-mdata/fwu-mdata-uclass.c
> +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
> @@ -14,7 +14,6 @@
>
>  #include <linux/errno.h>
>  #include <linux/types.h>
> -#include <u-boot/crc.h>
>
>  /**
>   * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
> @@ -50,170 +49,6 @@ int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
>  	return ops->write_mdata(dev, mdata, primary);
>  }
>
> -/**
> - * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers
> - * @dev: FWU metadata device
> - * @mdata_parts: array for storing the metadata partition numbers
> - *
> - * Get the partition numbers on the storage device on which the
> - * FWU metadata is stored. Two partition numbers will be returned.
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_get_mdata_part_num(struct udevice *dev, uint *mdata_parts)
> -{
> -	const struct fwu_mdata_ops *ops = device_get_ops(dev);
> -
> -	if (!ops->get_mdata_part_num) {
> -		log_debug("get_mdata_part_num() method not defined\n");
> -		return -ENOSYS;
> -	}
> -
> -	return ops->get_mdata_part_num(dev, mdata_parts);
> -}
> -
> -/**
> - * fwu_read_mdata_partition() - Read the FWU metadata from a partition
> - * @dev: FWU metadata device
> - * @mdata: Copy of the FWU metadata
> - * @part_num: Partition number from which FWU metadata is to be read
> - *
> - * Read the FWU metadata from the specified partition number
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_read_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata,
> -			     uint part_num)
> -{
> -	const struct fwu_mdata_ops *ops = device_get_ops(dev);
> -
> -	if (!ops->read_mdata_partition) {
> -		log_debug("read_mdata_partition() method not defined\n");
> -		return -ENOSYS;
> -	}
> -
> -	return ops->read_mdata_partition(dev, mdata, part_num);
> -}
> -
> -/**
> - * fwu_write_mdata_partition() - Write the FWU metadata to a partition
> - * @dev: FWU metadata device
> - * @mdata: Copy of the FWU metadata
> - * @part_num: Partition number to which FWU metadata is to be written
> - *
> - * Write the FWU metadata to the specified partition number
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_write_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata,
> -			      uint part_num)
> -{
> -	const struct fwu_mdata_ops *ops = device_get_ops(dev);
> -
> -	if (!ops->write_mdata_partition) {
> -		log_debug("write_mdata_partition() method not defined\n");
> -		return -ENOSYS;
> -	}
> -
> -	return ops->write_mdata_partition(dev, mdata, part_num);
> -}
> -
> -/**
> - * fwu_mdata_check() - Check if the FWU metadata is valid
> - * @dev: FWU metadata device
> - *
> - * Validate both copies of the FWU metadata. If one of the copies
> - * has gone bad, restore it from the other copy.
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_mdata_check(struct udevice *dev)
> -{
> -	const struct fwu_mdata_ops *ops = device_get_ops(dev);
> -
> -	if (!ops->check_mdata) {
> -		log_debug("check_mdata() method not defined\n");
> -		return -ENOSYS;
> -	}
> -
> -	return ops->check_mdata(dev);
> -}
> -
> -/**
> - * fwu_get_mdata() - Get a FWU metadata copy
> - * @dev: FWU metadata device
> - * @mdata: Copy of the FWU metadata
> - *
> - * Get a valid copy of the FWU metadata.
> - *
> - * Note: This function is to be called first when modifying any fields
> - * in the metadata. The sequence of calls to modify any field in the
> - * metadata would  be 1) fwu_get_mdata 2) Modify metadata, followed by
> - * 3) fwu_update_mdata
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_get_mdata(struct udevice *dev, struct fwu_mdata *mdata)
> -{
> -	const struct fwu_mdata_ops *ops = device_get_ops(dev);
> -
> -	if (!ops->get_mdata) {
> -		log_debug("get_mdata() method not defined\n");
> -		return -ENOSYS;
> -	}
> -
> -	return ops->get_mdata(dev, mdata);
> -}
> -
> -/**
> - * fwu_update_mdata() - Update the FWU metadata
> - * @dev: FWU metadata device
> - * @mdata: Copy of the FWU metadata
> - *
> - * Update the FWU metadata structure by writing to the
> - * FWU metadata partitions.
> - *
> - * Note: This function is not to be called directly to update the
> - * metadata fields. The sequence of function calls should be
> - * 1) fwu_get_mdata() 2) Modify the medata fields 3) fwu_update_mdata()
> - *
> - * The sequence of updating the partitions should be, update the
> - * primary metadata partition (first partition encountered), followed
> - * by updating the secondary partition. With this update sequence, in
> - * the rare scenario that the two metadata partitions are valid but do
> - * not match, maybe due to power outage at the time of updating the
> - * metadata copies, the secondary partition can be updated from the
> - * primary.
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_update_mdata(struct udevice *dev, struct fwu_mdata *mdata)
> -{
> -	void *buf;
> -	const struct fwu_mdata_ops *ops = device_get_ops(dev);
> -
> -	if (!ops->update_mdata) {
> -		log_debug("get_mdata() method not defined\n");
> -		return -ENOSYS;
> -	}
> -
> -	/*
> -	 * Calculate the crc32 for the updated FWU metadata
> -	 * and put the updated value in the FWU metadata crc32
> -	 * field
> -	 */
> -	buf = &mdata->version;
> -	mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
> -
> -	return ops->update_mdata(dev, mdata);
> -}
> -
>  UCLASS_DRIVER(fwu_mdata) = {
>  	.id		= UCLASS_FWU_MDATA,
>  	.name		= "fwu-mdata",
> diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c
> index bdaa10cd1d..b3c6953a6e 100644
> --- a/drivers/fwu-mdata/gpt_blk.c
> +++ b/drivers/fwu-mdata/gpt_blk.c
> @@ -28,7 +28,7 @@ static uint g_mdata_part[2]; /* = {0, 0} to check against uninit parts */
>
>  static int gpt_get_mdata_partitions(struct blk_desc *desc)
>  {
> -	int i, ret;
> +	int i;
>  	u32 nparts;
>  	efi_guid_t part_type_guid;
>  	struct disk_partition info;
> @@ -52,12 +52,12 @@ static int gpt_get_mdata_partitions(struct blk_desc *desc)
>  	if (nparts != 2) {
>  		log_debug("Expect two copies of the FWU metadata instead of %d\n",
>  			  nparts);
> -		ret = -EINVAL;
> -	} else {
> -		ret = 0;
> +		g_mdata_part[0] = 0;
> +		g_mdata_part[1] = 0;
> +		return -EINVAL;
>  	}
>
> -	return ret;
> +	return 0;
>  }
>
>  static int gpt_get_mdata_disk_part(struct blk_desc *desc,
> @@ -125,115 +125,6 @@ static int gpt_read_write_mdata(struct blk_desc *desc,
>  	return 0;
>  }
>
> -static int fwu_gpt_update_mdata(struct udevice *dev, struct fwu_mdata *mdata)
> -{
> -	int ret;
> -	struct blk_desc *desc;
> -	struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> -
> -	desc = dev_get_uclass_plat(priv->blk_dev);
> -
> -	ret = gpt_get_mdata_partitions(desc);
> -	if (ret < 0) {
> -		log_debug("Error getting the FWU metadata partitions\n");
> -		return -ENOENT;
> -	}
> -
> -	/* First write the primary partition */
> -	ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, g_mdata_part[0]);
> -	if (ret < 0) {
> -		log_debug("Updating primary FWU metadata partition failed\n");
> -		return ret;
> -	}
> -
> -	/* And now the replica */
> -	ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, g_mdata_part[1]);
> -	if (ret < 0) {
> -		log_debug("Updating secondary FWU metadata partition failed\n");
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
> -static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata *mdata)
> -{
> -	int ret;
> -
> -	ret = gpt_get_mdata_partitions(desc);
> -	if (ret < 0) {
> -		log_debug("Error getting the FWU metadata partitions\n");
> -		return -ENOENT;
> -	}
> -
> -	ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, g_mdata_part[0]);
> -	if (ret < 0) {
> -		log_debug("Failed to read the FWU metadata from the device\n");
> -		return -EIO;
> -	}
> -
> -	ret = fwu_verify_mdata(mdata, 1);
> -	if (!ret)
> -		return 0;
> -
> -	/*
> -	 * Verification of the primary FWU metadata copy failed.
> -	 * Try to read the replica.
> -	 */
> -	memset(mdata, '\0', sizeof(struct fwu_mdata));
> -	ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, g_mdata_part[1]);
> -	if (ret < 0) {
> -		log_debug("Failed to read the FWU metadata from the device\n");
> -		return -EIO;
> -	}
> -
> -	ret = fwu_verify_mdata(mdata, 0);
> -	if (!ret)
> -		return 0;
> -
> -	/* Both the FWU metadata copies are corrupted. */
> -	return -EIO;
> -}
> -
> -static int fwu_gpt_get_mdata(struct udevice *dev, struct fwu_mdata *mdata)
> -{
> -	struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> -
> -	return gpt_get_mdata(dev_get_uclass_plat(priv->blk_dev), mdata);
> -}
> -
> -static int fwu_gpt_get_mdata_partitions(struct udevice *dev, uint *mdata_parts)
> -{
> -	struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> -	int err;
> -
> -	err = gpt_get_mdata_partitions(dev_get_uclass_plat(priv->blk_dev));
> -	if (!err) {
> -		mdata_parts[0] = g_mdata_part[0];
> -		mdata_parts[1] = g_mdata_part[1];
> -	}
> -
> -	return err;
> -}
> -
> -static int fwu_gpt_read_mdata_partition(struct udevice *dev,
> -					struct fwu_mdata *mdata, uint part_num)
> -{
> -	struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> -
> -	return gpt_read_write_mdata(dev_get_uclass_plat(priv->blk_dev),
> -				    mdata, MDATA_READ, part_num);
> -}
> -
> -static int fwu_gpt_write_mdata_partition(struct udevice *dev,
> -					struct fwu_mdata *mdata, uint part_num)
> -{
> -	struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> -
> -	return gpt_read_write_mdata(dev_get_uclass_plat(priv->blk_dev),
> -				    mdata, MDATA_WRITE, part_num);
> -}
> -
>  static int fwu_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev)
>  {
>  	u32 phandle;
> @@ -309,11 +200,6 @@ static int fwu_gpt_write_mdata(struct udevice *dev, struct fwu_mdata *mdata,
>  static const struct fwu_mdata_ops fwu_gpt_blk_ops = {
>  	.read_mdata = fwu_gpt_read_mdata,
>  	.write_mdata = fwu_gpt_write_mdata,
> -	.get_mdata = fwu_gpt_get_mdata,
> -	.update_mdata = fwu_gpt_update_mdata,
> -	.get_mdata_part_num = fwu_gpt_get_mdata_partitions,
> -	.read_mdata_partition = fwu_gpt_read_mdata_partition,
> -	.write_mdata_partition = fwu_gpt_write_mdata_partition,
>  };
>
>  static const struct udevice_id fwu_mdata_ids[] = {
> diff --git a/include/fwu.h b/include/fwu.h
> index 1a700c9e6a..23bd97fe86 100644
> --- a/include/fwu.h
> +++ b/include/fwu.h
> @@ -18,11 +18,6 @@ struct fwu_mdata_gpt_blk_priv {
>  	struct udevice *blk_dev;
>  };
>
> -/**
> - * @mdata_check: check the validity of the FWU metadata partitions
> - * @get_mdata() - Get a FWU metadata copy
> - * @update_mdata() - Update the FWU metadata copy
> - */
>  struct fwu_mdata_ops {
>  	/**
>  	 * read_mdata() - Populate the asked FWU metadata copy
> @@ -43,78 +38,6 @@ struct fwu_mdata_ops {
>  	 * Return: 0 if OK, -ve on error
>  	 */
>  	int (*write_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> -
> -	/**
> -	 * check_mdata() - Check if the FWU metadata is valid
> -	 * @dev:	FWU device
> -	 *
> -	 * Validate both copies of the FWU metadata. If one of the copies
> -	 * has gone bad, restore it from the other copy.
> -	 *
> -	 * Return: 0 if OK, -ve on error
> -	 */
> -	int (*check_mdata)(struct udevice *dev);
> -
> -	/**
> -	 * get_mdata() - Get a FWU metadata copy
> -	 * @dev:	FWU device
> -	 * @mdata:	Pointer to FWU metadata
> -	 *
> -	 * Get a valid copy of the FWU metadata.
> -	 *
> -	 * Return: 0 if OK, -ve on error
> -	 */
> -	int (*get_mdata)(struct udevice *dev, struct fwu_mdata *mdata);
> -
> -	/**
> -	 * update_mdata() - Update the FWU metadata
> -	 * @dev:	FWU device
> -	 * @mdata:	Copy of the FWU metadata
> -	 *
> -	 * Update the FWU metadata structure by writing to the
> -	 * FWU metadata partitions.
> -	 *
> -	 * Return: 0 if OK, -ve on error
> -	 */
> -	int (*update_mdata)(struct udevice *dev, struct fwu_mdata *mdata);
> -
> -	/**
> -	 * get_mdata_part_num() - Get the FWU metadata partition numbers
> -	 * @dev:		FWU metadata device
> -	 * @mdata_parts:	 array for storing the metadata partition numbers
> -	 *
> -	 * Get the partition numbers on the storage device on which the
> -	 * FWU metadata is stored. Two partition numbers will be returned.
> -	 *
> -	 * Return: 0 if OK, -ve on error
> -	 */
> -	int (*get_mdata_part_num)(struct udevice *dev, uint *mdata_parts);
> -
> -	/**
> -	 * read_mdata_partition() - Read the FWU metadata from a partition
> -	 * @dev:	FWU metadata device
> -	 * @mdata:	Copy of the FWU metadata
> -	 * @part_num:	Partition number from which FWU metadata is to be read
> -	 *
> -	 * Read the FWU metadata from the specified partition number
> -	 *
> -	 * Return: 0 if OK, -ve on error
> -	 */
> -	int (*read_mdata_partition)(struct udevice *dev,
> -				    struct fwu_mdata *mdata, uint part_num);
> -
> -	/**
> -	 * write_mdata_partition() - Write the FWU metadata to a partition
> -	 * @dev:	FWU metadata device
> -	 * @mdata:	Copy of the FWU metadata
> -	 * @part_num:	Partition number to which FWU metadata is to be written
> -	 *
> -	 * Write the FWU metadata to the specified partition number
> -	 *
> -	 * Return: 0 if OK, -ve on error
> -	 */
> -	int (*write_mdata_partition)(struct udevice *dev,
> -				     struct fwu_mdata *mdata, uint part_num);
>  };
>
>  #define FWU_MDATA_VERSION	0x1
> @@ -167,102 +90,6 @@ int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
>  */
>  int fwu_get_verified_mdata(struct fwu_mdata *mdata);
>
> -/**
> - * fwu_check_mdata_validity() - Check for validity of the FWU metadata copies
> - *
> - * Read both the metadata copies from the storage media, verify their
> - * checksum, and ascertain that both copies match. If one of the copies
> - * has gone bad, restore it from the good copy.
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_check_mdata_validity(void);
> -
> -/**
> - * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers
> - * @dev: FWU metadata device
> - * @mdata_parts: array for storing the metadata partition numbers
> - *
> - * Get the partition numbers on the storage device on which the
> - * FWU metadata is stored. Two partition numbers will be returned
> - * through the array.
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_get_mdata_part_num(struct udevice *dev, uint *mdata_parts);
> -
> -/**
> - * fwu_read_mdata_partition() - Read the FWU metadata from a partition
> - * @dev: FWU metadata device
> - * @mdata: Copy of the FWU metadata
> - * @part_num: Partition number from which FWU metadata is to be read
> - *
> - * Read the FWU metadata from the specified partition number
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_read_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata,
> -			     uint part_num);
> -
> -/**
> - * fwu_write_mdata_partition() - Write the FWU metadata to a partition
> - * @dev: FWU metadata device
> - * @mdata: Copy of the FWU metadata
> - * @part_num: Partition number to which FWU metadata is to be written
> - *
> - * Write the FWU metadata to the specified partition number
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_write_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata,
> -			      uint part_num);
> -
> -/**
> - * fwu_get_mdata() - Get a FWU metadata copy
> - * @dev: FWU metadata device
> - * @mdata: Copy of the FWU metadata
> - *
> - * Get a valid copy of the FWU metadata.
> - *
> - * Note: This function is to be called first when modifying any fields
> - * in the metadata. The sequence of calls to modify any field in the
> - * metadata would  be 1) fwu_get_mdata 2) Modify metadata, followed by
> - * 3) fwu_update_mdata
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_get_mdata(struct udevice *dev, struct fwu_mdata *mdata);
> -
> -/**
> - * fwu_update_mdata() - Update the FWU metadata
> - * @dev: FWU metadata device
> - * @mdata: Copy of the FWU metadata
> - *
> - * Update the FWU metadata structure by writing to the
> - * FWU metadata partitions.
> - *
> - * Note: This function is not to be called directly to update the
> - * metadata fields. The sequence of function calls should be
> - * 1) fwu_get_mdata() 2) Modify the medata fields 3) fwu_update_mdata()
> - *
> - * The sequence of updating the partitions should be, update the
> - * primary metadata partition (first partition encountered), followed
> - * by updating the secondary partition. With this update sequence, in
> - * the rare scenario that the two metadata partitions are valid but do
> - * not match, maybe due to power outage at the time of updating the
> - * metadata copies, the secondary partition can be updated from the
> - * primary.
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_update_mdata(struct udevice *dev, struct fwu_mdata *mdata);
> -
>  /**
>   * fwu_get_active_index() - Get active_index from the FWU metadata
>   * @active_idxp: active_index value to be read
> @@ -303,18 +130,6 @@ int fwu_set_active_index(uint active_idx);
>   */
>  int fwu_get_image_index(u8 *image_index);
>
> -/**
> - * fwu_mdata_check() - Check if the FWU metadata is valid
> - * @dev: FWU metadata device
> - *
> - * Validate both copies of the FWU metadata. If one of the copies
> - * has gone bad, restore it from the other copy.
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_mdata_check(struct udevice *dev);
> -
>  /**
>   * fwu_revert_boot_index() - Revert the active index in the FWU metadata
>   *
> @@ -327,20 +142,6 @@ int fwu_mdata_check(struct udevice *dev);
>   */
>  int fwu_revert_boot_index(void);
>
> -/**
> - * fwu_verify_mdata() - Verify the FWU metadata
> - * @mdata: FWU metadata structure
> - * @pri_part: FWU metadata partition is primary or secondary
> - *
> - * Verify the FWU metadata by computing the CRC32 for the metadata
> - * structure and comparing it against the CRC32 value stored as part
> - * of the structure.
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part);
> -
>  /**
>   * fwu_accept_image() - Set the Acceptance bit for the image
>   * @img_type_id: GUID of the image type for which the accepted bit is to be
> diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
> index 56299f1b2f..9c0a8e2bb1 100644
> --- a/lib/fwu_updates/fwu.c
> +++ b/lib/fwu_updates/fwu.c
> @@ -33,26 +33,6 @@ enum {
>  	BOTH_PARTS,
>  };
>
> -static int fwu_get_dev_mdata(struct udevice **dev, struct fwu_mdata *mdata)
> -{
> -	int ret;
> -
> -	ret = uclass_first_device_err(UCLASS_FWU_MDATA, dev);
> -	if (ret) {
> -		log_debug("Cannot find fwu device\n");
> -		return ret;
> -	}
> -
> -	if (!mdata)
> -		return 0;
> -
> -	ret = fwu_get_mdata(*dev, mdata);
> -	if (ret < 0)
> -		log_debug("Unable to get valid FWU metadata\n");
> -
> -	return ret;
> -}
> -
>  static int trial_counter_update(u16 *trial_state_ctr)
>  {
>  	bool delete;
> @@ -288,136 +268,6 @@ ret_mdata:
>  	return err;
>  }
>
> -/**
> - * fwu_verify_mdata() - Verify the FWU metadata
> - * @mdata: FWU metadata structure
> - * @pri_part: FWU metadata partition is primary or secondary
> - *
> - * Verify the FWU metadata by computing the CRC32 for the metadata
> - * structure and comparing it against the CRC32 value stored as part
> - * of the structure.
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part)
> -{
> -	u32 calc_crc32;
> -	void *buf;
> -
> -	buf = &mdata->version;
> -	calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
> -
> -	if (calc_crc32 != mdata->crc32) {
> -		log_debug("crc32 check failed for %s FWU metadata partition\n",
> -			  pri_part ? "primary" : "secondary");
> -		return -EINVAL;
> -	}
> -
> -	return 0;
> -}
> -
> -/**
> - * fwu_check_mdata_validity() - Check for validity of the FWU metadata copies
> - *
> - * Read both the metadata copies from the storage media, verify their checksum,
> - * and ascertain that both copies match. If one of the copies has gone bad,
> - * restore it from the good copy.
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_check_mdata_validity(void)
> -{
> -	int ret;
> -	struct udevice *dev;
> -	struct fwu_mdata pri_mdata;
> -	struct fwu_mdata secondary_mdata;
> -	uint mdata_parts[2];
> -	uint valid_partitions, invalid_partitions;
> -
> -	ret = fwu_get_dev_mdata(&dev, NULL);
> -	if (ret)
> -		return ret;
> -
> -	/*
> -	 * Check if the platform has defined its own
> -	 * function to check the metadata partitions'
> -	 * validity. If so, that takes precedence.
> -	 */
> -	ret = fwu_mdata_check(dev);
> -	if (!ret || ret != -ENOSYS)
> -		return ret;
> -
> -	/*
> -	 * Two FWU metadata partitions are expected.
> -	 * If we don't have two, user needs to create
> -	 * them first
> -	 */
> -	valid_partitions = 0;
> -	ret = fwu_get_mdata_part_num(dev, mdata_parts);
> -	if (ret < 0) {
> -		log_debug("Error getting the FWU metadata partitions\n");
> -		return -ENOENT;
> -	}
> -
> -	ret = fwu_read_mdata_partition(dev, &pri_mdata, mdata_parts[0]);
> -	if (!ret) {
> -		ret = fwu_verify_mdata(&pri_mdata, 1);
> -		if (!ret)
> -			valid_partitions |= PRIMARY_PART;
> -	}
> -
> -	ret = fwu_read_mdata_partition(dev, &secondary_mdata, mdata_parts[1]);
> -	if (!ret) {
> -		ret = fwu_verify_mdata(&secondary_mdata, 0);
> -		if (!ret)
> -			valid_partitions |= SECONDARY_PART;
> -	}
> -
> -	if (valid_partitions == (PRIMARY_PART | SECONDARY_PART)) {
> -		/*
> -		 * Before returning, check that both the
> -		 * FWU metadata copies are the same. If not,
> -		 * populate the secondary partition from the
> -		 * primary partition copy.
> -		 */
> -		if (!memcmp(&pri_mdata, &secondary_mdata,
> -			    sizeof(struct fwu_mdata))) {
> -			ret = 0;
> -		} else {
> -			log_info("Both FWU metadata copies are valid but do not match.");
> -			log_info(" Restoring the secondary partition from the primary\n");
> -			ret = fwu_write_mdata_partition(dev, &pri_mdata,
> -							mdata_parts[1]);
> -			if (ret)
> -				log_debug("Restoring secondary FWU metadata partition failed\n");
> -		}
> -		goto out;
> -	}
> -
> -	if (!(valid_partitions & BOTH_PARTS)) {
> -		log_info("Both FWU metadata partitions invalid\n");
> -		ret = -EBADMSG;
> -		goto out;
> -	}
> -
> -	invalid_partitions = valid_partitions ^ BOTH_PARTS;
> -	ret = fwu_write_mdata_partition(dev,
> -					(invalid_partitions == PRIMARY_PART) ?
> -					&secondary_mdata : &pri_mdata,
> -					(invalid_partitions == PRIMARY_PART) ?
> -					mdata_parts[0] : mdata_parts[1]);
> -
> -	if (ret)
> -		log_debug("Restoring %s FWU metadata partition failed\n",
> -			  (invalid_partitions == PRIMARY_PART) ?
> -			  "primary" : "secondary");
> -
> -out:
> -	return ret;
> -}
> -
>  /**
>   * fwu_get_active_index() - Get active_index from the FWU metadata
>   * @active_idx: active_index value to be read
> @@ -430,19 +280,14 @@ out:
>   */
>  int fwu_get_active_index(uint *active_idx)
>  {
> -	int ret;
> -	struct udevice *dev;
> -	struct fwu_mdata mdata = { 0 };
> -
> -	ret = fwu_get_dev_mdata(&dev, &mdata);
> -	if (ret)
> -		return ret;
> +	int ret = 0;
> +	struct fwu_mdata *mdata = &g_mdata;
>
>  	/*
>  	 * Found the FWU metadata partition, now read the active_index
>  	 * value
>  	 */
> -	*active_idx = mdata.active_index;
> +	*active_idx = mdata->active_index;
>  	if (*active_idx >= CONFIG_FWU_NUM_BANKS) {
>  		log_debug("Active index value read is incorrect\n");
>  		ret = -EINVAL;
> @@ -463,30 +308,25 @@ int fwu_get_active_index(uint *active_idx)
>  int fwu_set_active_index(uint active_idx)
>  {
>  	int ret;
> -	struct udevice *dev;
> -	struct fwu_mdata mdata = { 0 };
> +	struct fwu_mdata *mdata = &g_mdata;
>
>  	if (active_idx >= CONFIG_FWU_NUM_BANKS) {
>  		log_debug("Invalid active index value\n");
>  		return -EINVAL;
>  	}
>
> -	ret = fwu_get_dev_mdata(&dev, &mdata);
> -	if (ret)
> -		return ret;
> -
>  	/*
>  	 * Update the active index and previous_active_index fields
>  	 * in the FWU metadata
>  	 */
> -	mdata.previous_active_index = mdata.active_index;
> -	mdata.active_index = active_idx;
> +	mdata->previous_active_index = mdata->active_index;
> +	mdata->active_index = active_idx;
>
>  	/*
>  	 * Now write this updated FWU metadata to both the
>  	 * FWU metadata partitions
>  	 */
> -	ret = fwu_update_mdata(dev, &mdata);
> +	ret = fwu_sync_mdata(mdata, BOTH_PARTS);
>  	if (ret) {
>  		log_debug("Failed to update FWU metadata partitions\n");
>  		ret = -EIO;
> @@ -516,15 +356,10 @@ int fwu_get_image_index(u8 *image_index)
>  	u8 alt_num;
>  	uint update_bank;
>  	efi_guid_t *image_guid, image_type_id;
> -	struct udevice *dev;
> -	struct fwu_mdata mdata = { 0 };
> +	struct fwu_mdata *mdata = &g_mdata;
>  	struct fwu_image_entry *img_entry;
>  	struct fwu_image_bank_info *img_bank_info;
>
> -	ret = fwu_get_dev_mdata(&dev, &mdata);
> -	if (ret)
> -		return ret;
> -
>  	ret = fwu_plat_get_update_index(&update_bank);
>  	if (ret) {
>  		log_debug("Failed to get the FWU update bank\n");
> @@ -545,11 +380,11 @@ int fwu_get_image_index(u8 *image_index)
>  	 */
>  	for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) {
>  		if (!guidcmp(&image_type_id,
> -			     &mdata.img_entry[i].image_type_uuid)) {
> -			img_entry = &mdata.img_entry[i];
> +			     &mdata->img_entry[i].image_type_uuid)) {
> +			img_entry = &mdata->img_entry[i];
>  			img_bank_info = &img_entry->img_bank_info[update_bank];
>  			image_guid = &img_bank_info->image_uuid;
> -			ret = fwu_plat_get_alt_num(dev, image_guid, &alt_num);
> +			ret = fwu_plat_get_alt_num(g_dev, image_guid, &alt_num);
>  			if (ret) {
>  				log_debug("alt_num not found for partition with GUID %pUs\n",
>  					  image_guid);
> @@ -584,26 +419,21 @@ int fwu_revert_boot_index(void)
>  {
>  	int ret;
>  	u32 cur_active_index;
> -	struct udevice *dev;
> -	struct fwu_mdata mdata = { 0 };
> -
> -	ret = fwu_get_dev_mdata(&dev, &mdata);
> -	if (ret)
> -		return ret;
> +	struct fwu_mdata *mdata = &g_mdata;
>
>  	/*
>  	 * Swap the active index and previous_active_index fields
>  	 * in the FWU metadata
>  	 */
> -	cur_active_index = mdata.active_index;
> -	mdata.active_index = mdata.previous_active_index;
> -	mdata.previous_active_index = cur_active_index;
> +	cur_active_index = mdata->active_index;
> +	mdata->active_index = mdata->previous_active_index;
> +	mdata->previous_active_index = cur_active_index;
>
>  	/*
>  	 * Now write this updated FWU metadata to both the
>  	 * FWU metadata partitions
>  	 */
> -	ret = fwu_update_mdata(dev, &mdata);
> +	ret = fwu_sync_mdata(mdata, BOTH_PARTS);
>  	if (ret) {
>  		log_debug("Failed to update FWU metadata partitions\n");
>  		ret = -EIO;
> @@ -630,16 +460,11 @@ int fwu_revert_boot_index(void)
>  static int fwu_clrset_image_accept(efi_guid_t *img_type_id, u32 bank, u8 action)
>  {
>  	int ret, i;
> -	struct udevice *dev;
> -	struct fwu_mdata mdata = { 0 };
> +	struct fwu_mdata *mdata = &g_mdata;
>  	struct fwu_image_entry *img_entry;
>  	struct fwu_image_bank_info *img_bank_info;
>
> -	ret = fwu_get_dev_mdata(&dev, &mdata);
> -	if (ret)
> -		return ret;
> -
> -	img_entry = &mdata.img_entry[0];
> +	img_entry = &mdata->img_entry[0];
>  	for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) {
>  		if (!guidcmp(&img_entry[i].image_type_uuid, img_type_id)) {
>  			img_bank_info = &img_entry[i].img_bank_info[bank];
> @@ -648,7 +473,7 @@ static int fwu_clrset_image_accept(efi_guid_t *img_type_id, u32 bank, u8 action)
>  			else
>  				img_bank_info->accepted = 0;
>
> -			ret = fwu_update_mdata(dev, &mdata);
> +			ret = fwu_sync_mdata(mdata, BOTH_PARTS);
>  			goto out;
>  		}
>  	}
> @@ -783,8 +608,6 @@ static int fwu_boottime_checks(void *ctx, struct event *event)
>  {
>  	int ret;
>  	u32 boot_idx, active_idx;
> -	struct udevice *dev;
> -	struct fwu_mdata mdata = { 0 };
>
>  	/* Don't have boot time checks on sandbox */
>  	if (IS_ENABLED(CONFIG_SANDBOX)) {
> @@ -792,9 +615,17 @@ static int fwu_boottime_checks(void *ctx, struct event *event)
>  		return 0;
>  	}
>
> -	ret = fwu_check_mdata_validity();
> -	if (ret)
> -		return 0;
> +	ret = uclass_first_device_err(UCLASS_FWU_MDATA, &g_dev);
> +	if (ret) {
> +		log_debug("Cannot find fwu device\n");
> +		return ret;
> +	}
> +
> +	ret = fwu_get_verified_mdata(NULL);
> +	if (ret) {
> +		log_debug("Unable to read meta-data\n");
> +		return ret;
> +	}
>
>  	/*
>  	 * Get the Boot Index, i.e. the bank from
> @@ -830,11 +661,7 @@ static int fwu_boottime_checks(void *ctx, struct event *event)
>  	if (efi_init_obj_list() != EFI_SUCCESS)
>  		return 0;
>
> -	ret = fwu_get_dev_mdata(&dev, &mdata);
> -	if (ret)
> -		return ret;
> -
> -	in_trial = in_trial_state(&mdata);
> +	in_trial = in_trial_state(&g_mdata);
>  	if (!in_trial || (ret = fwu_trial_count_update()) > 0)
>  		ret = trial_counter_update(NULL);
>
> --
> 2.34.1
>

Re: [PATCHv4 2/5] fwu: move meta-data management in core

[Etienne Carriere]

Hello Jassi,

On Sun, 5 Feb 2023 at 04:01, <jassisinghbrar@gmail.com> wrote:
>
> From: Jassi Brar <jaswinder.singh@linaro.org>
>
> Instead of each i/f having to implement their own meta-data verification
> and storage, move the logic in common code. This simplifies the i/f code
> much simpler and compact.
>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>  drivers/fwu-mdata/fwu-mdata-uclass.c |  34 +++++++
>  include/fwu.h                        |  41 ++++++++
>  lib/fwu_updates/fwu.c                | 135 ++++++++++++++++++++++++++-
>  3 files changed, 206 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c
> index b477e9603f..e03773c584 100644
> --- a/drivers/fwu-mdata/fwu-mdata-uclass.c
> +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
> @@ -16,6 +16,40 @@
>  #include <linux/types.h>
>  #include <u-boot/crc.h>
>
> +/**
> + * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
> +{
> +       const struct fwu_mdata_ops *ops = device_get_ops(dev);
> +
> +       if (!ops->read_mdata) {
> +               log_debug("read_mdata() method not defined\n");
> +               return -ENOSYS;
> +       }
> +
> +       return ops->read_mdata(dev, mdata, primary);
> +}
> +
> +/**
> + * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
> +{
> +       const struct fwu_mdata_ops *ops = device_get_ops(dev);
> +
> +       if (!ops->write_mdata) {
> +               log_debug("write_mdata() method not defined\n");
> +               return -ENOSYS;
> +       }
> +
> +       return ops->write_mdata(dev, mdata, primary);
> +}
> +
>  /**
>   * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers
>   * @dev: FWU metadata device
> diff --git a/include/fwu.h b/include/fwu.h
> index 0919ced812..1a700c9e6a 100644
> --- a/include/fwu.h
> +++ b/include/fwu.h
> @@ -24,6 +24,26 @@ struct fwu_mdata_gpt_blk_priv {
>   * @update_mdata() - Update the FWU metadata copy
>   */
>  struct fwu_mdata_ops {
> +       /**
> +        * read_mdata() - Populate the asked FWU metadata copy
> +        * @dev: FWU metadata device
> +        * @mdata: Copy of the FWU metadata
> +        * @primary: If primary or secondary copy of meta-data is to be read
> +        *
> +        * Return: 0 if OK, -ve on error
> +        */
> +       int (*read_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> +
> +       /**
> +        * write_mdata() - Write the given FWU metadata copy
> +        * @dev: FWU metadata device
> +        * @mdata: Copy of the FWU metadata
> +        * @primary: If primary or secondary copy of meta-data is to be written
> +        *
> +        * Return: 0 if OK, -ve on error
> +        */
> +       int (*write_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> +
>         /**
>          * check_mdata() - Check if the FWU metadata is valid
>          * @dev:        FWU device
> @@ -126,6 +146,27 @@ struct fwu_mdata_ops {
>         EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \
>                  0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8)
>
> +/**
> + * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
> + */
> +int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> +
> +/**
> + * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
> + */
> +int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> +
> +/**
> + * fwu_get_verified_mdata() - Read, verify and return the FWU metadata
> + *
> + * Read both the metadata copies from the storage media, verify their checksum,
> + * and ascertain that both copies match. If one of the copies has gone bad,
> + * restore it from the good copy.
> + *
> + * Return: 0 if OK, -ve on error
> +*/
> +int fwu_get_verified_mdata(struct fwu_mdata *mdata);
> +
>  /**
>   * fwu_check_mdata_validity() - Check for validity of the FWU metadata copies
>   *
> diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
> index 5313d07302..56299f1b2f 100644
> --- a/lib/fwu_updates/fwu.c
> +++ b/lib/fwu_updates/fwu.c
> @@ -15,13 +15,13 @@
>  #include <linux/errno.h>
>  #include <linux/types.h>
>
> +#include <u-boot/crc.h>
> +
> +static struct fwu_mdata g_mdata; /* = {0} makes uninit crc32 always invalid */
> +static struct udevice *g_dev;
>  static u8 in_trial;
>  static u8 boottime_check;
>
> -#include <linux/errno.h>
> -#include <linux/types.h>
> -#include <u-boot/crc.h>
> -
>  enum {
>         IMAGE_ACCEPT_SET = 1,
>         IMAGE_ACCEPT_CLEAR,
> @@ -161,6 +161,133 @@ static int fwu_get_image_type_id(u8 *image_index, efi_guid_t *image_type_id)
>         return -ENOENT;
>  }
>
> +/**
> + * fwu_sync_mdata() - Update given meta-data partition(s) with the copy provided
> + * @mdata: FWU metadata structure
> + * @part: Bitmask of FWU metadata partitions to be written to
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +static int fwu_sync_mdata(struct fwu_mdata *mdata, int part)
> +{
> +       void *buf = &mdata->version;
> +       int err = 0;
> +
> +       /*
> +        * Calculate the crc32 for the updated FWU metadata
> +        * and put the updated value in the FWU metadata crc32
> +        * field
> +        */
> +       mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
> +
> +       err = fwu_write_mdata(g_dev, mdata, part & PRIMARY_PART ? true : false);

There is an issue here as arg @part can have 3 values: PRIMARY_PART,
SECONDARY_PART or BOTH_PARTS.
The implementation here does not consider the later case BOTH_PATHS.
I think something like the below code snippet should do the work:

   switch (part) {
   case PRIMARY_PART:
   case SECONDARY_PART:
       err = fwu_write_mdata(g_dev, mdata, part == PRIMARY_PART);
       break;
   default: /* assuming BOTH_PARTS, or maybe we need sanitization of
invalid part values? */
       err = fwu_write_mdata(g_dev, mdata, true);
        if (!err)
           err = fwu_write_mdata(g_dev, mdata, false);
        break;
   }
and  adaptation for the error message below.

Or maybe fwu_write_mdata() should not have its 3rd argument and
backend handler should directly handle mdata replication update (if
needed) on any mdata update requests.


Br,
etienne



> +       if (err) {
> +               log_err("Unable to write %s mdata\n",
> +                               part & PRIMARY_PART ?  "primary": "secondary");
> +               return err;
> +       }
> +
> +       /* update the cached copy of meta-data */
> +       memcpy(&g_mdata, mdata, sizeof(struct fwu_mdata));
> +
> +       return 0;
> +}
> +
> +static inline int mdata_crc_check(struct fwu_mdata *mdata)
> +{
> +       void *buf = &mdata->version;
> +       u32 calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
> +
> +       return calc_crc32 == mdata->crc32 ? 0 : -EINVAL;
> +}
> +
> +/**
> + * fwu_get_verified_mdata() - Read, verify and return the FWU metadata
> + *
> + * Read both the metadata copies from the storage media, verify their checksum,
> + * and ascertain that both copies match. If one of the copies has gone bad,
> + * restore it from the good copy.
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int fwu_get_verified_mdata(struct fwu_mdata *mdata)
> +{
> +       int err;
> +       bool pri_ok, sec_ok;
> +       struct fwu_mdata s, *p_mdata, *s_mdata;
> +
> +       p_mdata = &g_mdata;
> +       s_mdata = &s;
> +
> +       /* if mdata already read and ready */
> +       err = mdata_crc_check(p_mdata);
> +       if (!err)
> +               goto ret_mdata;
> +       /* else read, verify and, if needed, fix mdata */
> +
> +       pri_ok = false;
> +       err = fwu_read_mdata(g_dev, p_mdata, true);
> +       if (!err) {
> +               err = mdata_crc_check(p_mdata);
> +               if (!err)
> +                       pri_ok = true;
> +               else
> +                       log_debug("primary mdata: crc32 failed\n");
> +       }
> +
> +       sec_ok = false;
> +       err = fwu_read_mdata(g_dev, s_mdata, false);
> +       if (!err) {
> +               err = mdata_crc_check(s_mdata);
> +               if (!err)
> +                       sec_ok = true;
> +               else
> +                       log_debug("secondary mdata: crc32 failed\n");
> +       }
> +
> +       if (pri_ok && sec_ok) {
> +               /*
> +                * Before returning, check that both the
> +                * FWU metadata copies are the same.
> +                */
> +               err = memcmp(p_mdata, s_mdata, sizeof(struct fwu_mdata));
> +               if (!err)
> +                       goto ret_mdata;
> +
> +               /*
> +                * If not, populate the secondary partition from the
> +                * primary partition copy.
> +                */
> +               log_info("Both FWU metadata copies are valid but do not match.");
> +               log_info(" Restoring the secondary partition from the primary\n");
> +               sec_ok = false;
> +       }
> +
> +       if (!pri_ok) {
> +               memcpy(p_mdata, s_mdata, sizeof(struct fwu_mdata));
> +               err = fwu_sync_mdata(p_mdata, PRIMARY_PART);
> +               if (err) {
> +                       log_debug("mdata : primary write failed\n");
> +                       return err;
> +               }
> +       }
> +
> +       if (!sec_ok) {
> +               memcpy(s_mdata, p_mdata, sizeof(struct fwu_mdata));
> +               err = fwu_sync_mdata(s_mdata, SECONDARY_PART);
> +               if (err) {
> +                       log_debug("mdata : secondary write failed\n");
> +                       return err;
> +               }
> +       }
> +
> +ret_mdata:
> +       if (!err && mdata)
> +               memcpy(mdata, p_mdata, sizeof(struct fwu_mdata));
> +
> +       return err;
> +}
> +
>  /**
>   * fwu_verify_mdata() - Verify the FWU metadata
>   * @mdata: FWU metadata structure
> --
> 2.34.1
>

Re: [PATCHv4 2/5] fwu: move meta-data management in core

[Jassi Brar]

On Mon, Feb 27, 2023 at 10:30 AM Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Hello Jassi,
>
> On Sun, 5 Feb 2023 at 04:01, <jassisinghbrar@gmail.com> wrote:
> >
> > From: Jassi Brar <jaswinder.singh@linaro.org>
> >
> > Instead of each i/f having to implement their own meta-data verification
> > and storage, move the logic in common code. This simplifies the i/f code
> > much simpler and compact.
> >
> > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> > ---
> >  drivers/fwu-mdata/fwu-mdata-uclass.c |  34 +++++++
> >  include/fwu.h                        |  41 ++++++++
> >  lib/fwu_updates/fwu.c                | 135 ++++++++++++++++++++++++++-
> >  3 files changed, 206 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c
> > index b477e9603f..e03773c584 100644
> > --- a/drivers/fwu-mdata/fwu-mdata-uclass.c
> > +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
> > @@ -16,6 +16,40 @@
> >  #include <linux/types.h>
> >  #include <u-boot/crc.h>
> >
> > +/**
> > + * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
> > + *
> > + * Return: 0 if OK, -ve on error
> > + */
> > +int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
> > +{
> > +       const struct fwu_mdata_ops *ops = device_get_ops(dev);
> > +
> > +       if (!ops->read_mdata) {
> > +               log_debug("read_mdata() method not defined\n");
> > +               return -ENOSYS;
> > +       }
> > +
> > +       return ops->read_mdata(dev, mdata, primary);
> > +}
> > +
> > +/**
> > + * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
> > + *
> > + * Return: 0 if OK, -ve on error
> > + */
> > +int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
> > +{
> > +       const struct fwu_mdata_ops *ops = device_get_ops(dev);
> > +
> > +       if (!ops->write_mdata) {
> > +               log_debug("write_mdata() method not defined\n");
> > +               return -ENOSYS;
> > +       }
> > +
> > +       return ops->write_mdata(dev, mdata, primary);
> > +}
> > +
> >  /**
> >   * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers
> >   * @dev: FWU metadata device
> > diff --git a/include/fwu.h b/include/fwu.h
> > index 0919ced812..1a700c9e6a 100644
> > --- a/include/fwu.h
> > +++ b/include/fwu.h
> > @@ -24,6 +24,26 @@ struct fwu_mdata_gpt_blk_priv {
> >   * @update_mdata() - Update the FWU metadata copy
> >   */
> >  struct fwu_mdata_ops {
> > +       /**
> > +        * read_mdata() - Populate the asked FWU metadata copy
> > +        * @dev: FWU metadata device
> > +        * @mdata: Copy of the FWU metadata
> > +        * @primary: If primary or secondary copy of meta-data is to be read
> > +        *
> > +        * Return: 0 if OK, -ve on error
> > +        */
> > +       int (*read_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> > +
> > +       /**
> > +        * write_mdata() - Write the given FWU metadata copy
> > +        * @dev: FWU metadata device
> > +        * @mdata: Copy of the FWU metadata
> > +        * @primary: If primary or secondary copy of meta-data is to be written
> > +        *
> > +        * Return: 0 if OK, -ve on error
> > +        */
> > +       int (*write_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> > +
> >         /**
> >          * check_mdata() - Check if the FWU metadata is valid
> >          * @dev:        FWU device
> > @@ -126,6 +146,27 @@ struct fwu_mdata_ops {
> >         EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \
> >                  0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8)
> >
> > +/**
> > + * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
> > + */
> > +int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> > +
> > +/**
> > + * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
> > + */
> > +int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> > +
> > +/**
> > + * fwu_get_verified_mdata() - Read, verify and return the FWU metadata
> > + *
> > + * Read both the metadata copies from the storage media, verify their checksum,
> > + * and ascertain that both copies match. If one of the copies has gone bad,
> > + * restore it from the good copy.
> > + *
> > + * Return: 0 if OK, -ve on error
> > +*/
> > +int fwu_get_verified_mdata(struct fwu_mdata *mdata);
> > +
> >  /**
> >   * fwu_check_mdata_validity() - Check for validity of the FWU metadata copies
> >   *
> > diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
> > index 5313d07302..56299f1b2f 100644
> > --- a/lib/fwu_updates/fwu.c
> > +++ b/lib/fwu_updates/fwu.c
> > @@ -15,13 +15,13 @@
> >  #include <linux/errno.h>
> >  #include <linux/types.h>
> >
> > +#include <u-boot/crc.h>
> > +
> > +static struct fwu_mdata g_mdata; /* = {0} makes uninit crc32 always invalid */
> > +static struct udevice *g_dev;
> >  static u8 in_trial;
> >  static u8 boottime_check;
> >
> > -#include <linux/errno.h>
> > -#include <linux/types.h>
> > -#include <u-boot/crc.h>
> > -
> >  enum {
> >         IMAGE_ACCEPT_SET = 1,
> >         IMAGE_ACCEPT_CLEAR,
> > @@ -161,6 +161,133 @@ static int fwu_get_image_type_id(u8 *image_index, efi_guid_t *image_type_id)
> >         return -ENOENT;
> >  }
> >
> > +/**
> > + * fwu_sync_mdata() - Update given meta-data partition(s) with the copy provided
> > + * @mdata: FWU metadata structure
> > + * @part: Bitmask of FWU metadata partitions to be written to
> > + *
> > + * Return: 0 if OK, -ve on error
> > + */
> > +static int fwu_sync_mdata(struct fwu_mdata *mdata, int part)
> > +{
> > +       void *buf = &mdata->version;
> > +       int err = 0;
> > +
> > +       /*
> > +        * Calculate the crc32 for the updated FWU metadata
> > +        * and put the updated value in the FWU metadata crc32
> > +        * field
> > +        */
> > +       mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
> > +
> > +       err = fwu_write_mdata(g_dev, mdata, part & PRIMARY_PART ? true : false);
>
> There is an issue here as arg @part can have 3 values: PRIMARY_PART,
> SECONDARY_PART or BOTH_PARTS.
> The implementation here does not consider the later case BOTH_PATHS.
> I think something like the below code snippet should do the work:
>
>    switch (part) {
>    case PRIMARY_PART:
>    case SECONDARY_PART:
>        err = fwu_write_mdata(g_dev, mdata, part == PRIMARY_PART);
>        break;
>    default: /* assuming BOTH_PARTS, or maybe we need sanitization of
> invalid part values? */
>        err = fwu_write_mdata(g_dev, mdata, true);
>         if (!err)
>            err = fwu_write_mdata(g_dev, mdata, false);
>         break;
>    }
> and  adaptation for the error message below.
>
> Or maybe fwu_write_mdata() should not have its 3rd argument and
> backend handler should directly handle mdata replication update (if
> needed) on any mdata update requests.
>
Good catch. Thanks.

Even simpler fix is ...

static int fwu_sync_mdata(struct fwu_mdata *mdata, int part)
{
    if (part == BOTH_PARTS) {
          fwu_sync_mdata(mdata, PRIMARY_PART);
          part = SECONDARY_PART;
    }
   .....
}

Thanks.

Re: [PATCHv4 2/5] fwu: move meta-data management in core

[Etienne Carriere]

-       err = fwu_write_mdata(g_dev, mdata, part & PRIMARY_PART ? true : false);

On Mon, 27 Feb 2023 at 17:46, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> On Mon, Feb 27, 2023 at 10:30 AM Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> >
> > Hello Jassi,
> >
> > On Sun, 5 Feb 2023 at 04:01, <jassisinghbrar@gmail.com> wrote:
> > >
> > > From: Jassi Brar <jaswinder.singh@linaro.org>
> > >
> > > Instead of each i/f having to implement their own meta-data verification
> > > and storage, move the logic in common code. This simplifies the i/f code
> > > much simpler and compact.
> > >
> > > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> > > ---
> > >  drivers/fwu-mdata/fwu-mdata-uclass.c |  34 +++++++
> > >  include/fwu.h                        |  41 ++++++++
> > >  lib/fwu_updates/fwu.c                | 135 ++++++++++++++++++++++++++-
> > >  3 files changed, 206 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c
> > > index b477e9603f..e03773c584 100644
> > > --- a/drivers/fwu-mdata/fwu-mdata-uclass.c
> > > +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
> > > @@ -16,6 +16,40 @@
> > >  #include <linux/types.h>
> > >  #include <u-boot/crc.h>
> > >
> > > +/**
> > > + * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
> > > + *
> > > + * Return: 0 if OK, -ve on error
> > > + */
> > > +int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
> > > +{
> > > +       const struct fwu_mdata_ops *ops = device_get_ops(dev);
> > > +
> > > +       if (!ops->read_mdata) {
> > > +               log_debug("read_mdata() method not defined\n");
> > > +               return -ENOSYS;
> > > +       }
> > > +
> > > +       return ops->read_mdata(dev, mdata, primary);
> > > +}
> > > +
> > > +/**
> > > + * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
> > > + *
> > > + * Return: 0 if OK, -ve on error
> > > + */
> > > +int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
> > > +{
> > > +       const struct fwu_mdata_ops *ops = device_get_ops(dev);
> > > +
> > > +       if (!ops->write_mdata) {
> > > +               log_debug("write_mdata() method not defined\n");
> > > +               return -ENOSYS;
> > > +       }
> > > +
> > > +       return ops->write_mdata(dev, mdata, primary);
> > > +}
> > > +
> > >  /**
> > >   * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers
> > >   * @dev: FWU metadata device
> > > diff --git a/include/fwu.h b/include/fwu.h
> > > index 0919ced812..1a700c9e6a 100644
> > > --- a/include/fwu.h
> > > +++ b/include/fwu.h
> > > @@ -24,6 +24,26 @@ struct fwu_mdata_gpt_blk_priv {
> > >   * @update_mdata() - Update the FWU metadata copy
> > >   */
> > >  struct fwu_mdata_ops {
> > > +       /**
> > > +        * read_mdata() - Populate the asked FWU metadata copy
> > > +        * @dev: FWU metadata device
> > > +        * @mdata: Copy of the FWU metadata
> > > +        * @primary: If primary or secondary copy of meta-data is to be read
> > > +        *
> > > +        * Return: 0 if OK, -ve on error
> > > +        */
> > > +       int (*read_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> > > +
> > > +       /**
> > > +        * write_mdata() - Write the given FWU metadata copy
> > > +        * @dev: FWU metadata device
> > > +        * @mdata: Copy of the FWU metadata
> > > +        * @primary: If primary or secondary copy of meta-data is to be written
> > > +        *
> > > +        * Return: 0 if OK, -ve on error
> > > +        */
> > > +       int (*write_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> > > +
> > >         /**
> > >          * check_mdata() - Check if the FWU metadata is valid
> > >          * @dev:        FWU device
> > > @@ -126,6 +146,27 @@ struct fwu_mdata_ops {
> > >         EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \
> > >                  0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8)
> > >
> > > +/**
> > > + * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
> > > + */
> > > +int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> > > +
> > > +/**
> > > + * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
> > > + */
> > > +int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> > > +
> > > +/**
> > > + * fwu_get_verified_mdata() - Read, verify and return the FWU metadata
> > > + *
> > > + * Read both the metadata copies from the storage media, verify their checksum,
> > > + * and ascertain that both copies match. If one of the copies has gone bad,
> > > + * restore it from the good copy.
> > > + *
> > > + * Return: 0 if OK, -ve on error
> > > +*/
> > > +int fwu_get_verified_mdata(struct fwu_mdata *mdata);
> > > +
> > >  /**
> > >   * fwu_check_mdata_validity() - Check for validity of the FWU metadata copies
> > >   *
> > > diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
> > > index 5313d07302..56299f1b2f 100644
> > > --- a/lib/fwu_updates/fwu.c
> > > +++ b/lib/fwu_updates/fwu.c
> > > @@ -15,13 +15,13 @@
> > >  #include <linux/errno.h>
> > >  #include <linux/types.h>
> > >
> > > +#include <u-boot/crc.h>
> > > +
> > > +static struct fwu_mdata g_mdata; /* = {0} makes uninit crc32 always invalid */
> > > +static struct udevice *g_dev;
> > >  static u8 in_trial;
> > >  static u8 boottime_check;
> > >
> > > -#include <linux/errno.h>
> > > -#include <linux/types.h>
> > > -#include <u-boot/crc.h>
> > > -
> > >  enum {
> > >         IMAGE_ACCEPT_SET = 1,
> > >         IMAGE_ACCEPT_CLEAR,
> > > @@ -161,6 +161,133 @@ static int fwu_get_image_type_id(u8 *image_index, efi_guid_t *image_type_id)
> > >         return -ENOENT;
> > >  }
> > >
> > > +/**
> > > + * fwu_sync_mdata() - Update given meta-data partition(s) with the copy provided
> > > + * @mdata: FWU metadata structure
> > > + * @part: Bitmask of FWU metadata partitions to be written to
> > > + *
> > > + * Return: 0 if OK, -ve on error
> > > + */
> > > +static int fwu_sync_mdata(struct fwu_mdata *mdata, int part)
> > > +{
> > > +       void *buf = &mdata->version;
> > > +       int err = 0;
> > > +
> > > +       /*
> > > +        * Calculate the crc32 for the updated FWU metadata
> > > +        * and put the updated value in the FWU metadata crc32
> > > +        * field
> > > +        */
> > > +       mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
> > > +
> > > +       err = fwu_write_mdata(g_dev, mdata, part & PRIMARY_PART ? true : false);
> >
> > There is an issue here as arg @part can have 3 values: PRIMARY_PART,
> > SECONDARY_PART or BOTH_PARTS.
> > The implementation here does not consider the later case BOTH_PATHS.
> > I think something like the below code snippet should do the work:
> >
> >    switch (part) {
> >    case PRIMARY_PART:
> >    case SECONDARY_PART:
> >        err = fwu_write_mdata(g_dev, mdata, part == PRIMARY_PART);
> >        break;
> >    default: /* assuming BOTH_PARTS, or maybe we need sanitization of
> > invalid part values? */
> >        err = fwu_write_mdata(g_dev, mdata, true);
> >         if (!err)
> >            err = fwu_write_mdata(g_dev, mdata, false);
> >         break;
> >    }
> > and  adaptation for the error message below.
> >
> > Or maybe fwu_write_mdata() should not have its 3rd argument and
> > backend handler should directly handle mdata replication update (if
> > needed) on any mdata update requests.
> >
> Good catch. Thanks.
>
> Even simpler fix is ...
>
> static int fwu_sync_mdata(struct fwu_mdata *mdata, int part)
> {
>     if (part == BOTH_PARTS) {
>           fwu_sync_mdata(mdata, PRIMARY_PART);
>           part = SECONDARY_PART;
>     }
>    .....
> }
>
> Thanks.

True :)   , (with error code management)
even if i think it makes the code less straightforward to read out.

maybe also, for simplicity,
-       err = fwu_write_mdata(g_dev, mdata, part & PRIMARY_PART ? true : false);
+       err = fwu_write_mdata(g_dev, mdata, part == PRIMARY_PART);

br,
etienne

Re: [PATCHv4 2/5] fwu: move meta-data management in core

[Jassi Brar]

Hi Ilias,

On Thu, Feb 23, 2023 at 2:36 AM Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
> > +int fwu_get_verified_mdata(struct fwu_mdata *mdata)
> > +{
> > +     int err;
> > +     bool pri_ok, sec_ok;
> > +     struct fwu_mdata s, *p_mdata, *s_mdata;
> > +
> > +     p_mdata = &g_mdata;
> > +     s_mdata = &s;
> > +
> > +     /* if mdata already read and ready */
> > +     err = mdata_crc_check(p_mdata);
> > +     if (!err)
> > +             goto ret_mdata;
> > +     /* else read, verify and, if needed, fix mdata */
> > +
> > +     pri_ok = false;
> > +     err = fwu_read_mdata(g_dev, p_mdata, true);
> > +     if (!err) {
> > +             err = mdata_crc_check(p_mdata);
> > +             if (!err)
> > +                     pri_ok = true;
> > +             else
> > +                     log_debug("primary mdata: crc32 failed\n");
> > +     }
> > +
> > +     sec_ok = false;
> > +     err = fwu_read_mdata(g_dev, s_mdata, false);
> > +     if (!err) {
> > +             err = mdata_crc_check(s_mdata);
> > +             if (!err)
> > +                     sec_ok = true;
> > +             else
> > +                     log_debug("secondary mdata: crc32 failed\n");
> > +     }
>
> Isn't it better to define pri_ok, sec_ok and their equivalent mdata as
> arrays ? IOW something along the lines of
>
> bool parts_ok[2] = { false };
> struct fwu_mdata parts_mdata[2];
>
> parts_mdata[0] = &g_mdata;
> parts_mdata[1] = .....
> for (i = 0; i < 2; i++) {
>     err = fwu_read_mdata(g_dev, parts_mdata[i], !(i % 2) ? true : false);
>     if (!err)
>         err = mdata_crc_check(parts_mdata[i]);
>         etc....
> }
>
> > +
> > +     if (pri_ok && sec_ok) {
>
> And then also adjust this part?
>
> > +             /*
> > +              * Before returning, check that both the
> > +              * FWU metadata copies are the same.
> > +              */
> > +             err = memcmp(p_mdata, s_mdata, sizeof(struct fwu_mdata));
> > +             if (!err)
> > +                     goto ret_mdata;
> > +
> > +             /*
> > +              * If not, populate the secondary partition from the
> > +              * primary partition copy.
> > +              */
> > +             log_info("Both FWU metadata copies are valid but do not match.");
> > +             log_info(" Restoring the secondary partition from the primary\n");
> > +             sec_ok = false;
> > +     }
> > +
> > +     if (!pri_ok) {
> > +             memcpy(p_mdata, s_mdata, sizeof(struct fwu_mdata));
> > +             err = fwu_sync_mdata(p_mdata, PRIMARY_PART);
> > +             if (err) {
> > +                     log_debug("mdata : primary write failed\n");
> > +                     return err;
> > +             }
> > +     }
> > +
> > +     if (!sec_ok) {
> > +             memcpy(s_mdata, p_mdata, sizeof(struct fwu_mdata));
> > +             err = fwu_sync_mdata(s_mdata, SECONDARY_PART);
> > +             if (err) {
> > +                     log_debug("mdata : secondary write failed\n");
> > +                     return err;
> > +             }
> > +     }
>
> And this could also be folded into a for loop
>
I have done these modifications and submitted v5.

Thanks.