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

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

[Jassi Brar]

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 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                | 301 ++++++++++++---------------
 5 files changed, 214 insertions(+), 628 deletions(-)

-- 
2.34.1

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

[Jassi Brar]

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

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

[Ilias Apalodimas]

On Mon, Jan 02, 2023 at 12:26:19PM -0600, Jassi Brar wrote:
> 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>
> 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
> 
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

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

[Jassi Brar]

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                | 142 ++++++++++++++++++++++++++-
 3 files changed, 213 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..4554654727 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,140 @@ 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));
+
+	if (part & PRIMARY_PART)
+		err = fwu_write_mdata(g_dev, mdata, true);
+
+	if (err) {
+		log_err("Unable to write primary mdata\n");
+		return err;
+	}
+
+	if (part & SECONDARY_PART)
+		err = fwu_write_mdata(g_dev, mdata, false);
+
+	if (err) {
+		log_err("Unable to write secondary mdata\n");
+		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)
+			goto ret_mdata;
+	}
+
+	if (!sec_ok) {
+		memcpy(s_mdata, p_mdata, sizeof(struct fwu_mdata));
+		err = fwu_sync_mdata(s_mdata, SECONDARY_PART);
+		if (err)
+			goto ret_mdata;
+	}
+
+ret_mdata:
+	if (err)
+		log_debug("mdata : crc32 failed\n");
+	else if (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: [PATCHv3 2/5] fwu: move meta-data management in core

[Ilias Apalodimas]

Hi Jassi,

On Mon, Jan 02, 2023 at 12:26:40PM -0600, Jassi Brar wrote:
> 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                | 142 ++++++++++++++++++++++++++-
>  3 files changed, 213 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_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));
> +
> +	if (part & PRIMARY_PART)
> +		err = fwu_write_mdata(g_dev, mdata, true);
> +
> +	if (err) {
> +		log_err("Unable to write primary mdata\n");
> +		return err;
> +	}
> +
> +	if (part & SECONDARY_PART)
> +		err = fwu_write_mdata(g_dev, mdata, false);
> +
> +	if (err) {
> +		log_err("Unable to write secondary mdata\n");
> +		return err;
> +	}

Can we write this 
err = fwu_write_mdata(g_dev, mdata, part & PRIMARY_PART ? true: false);
if (err)
    log_err("Unable to write %s partition\n", part & PRIMARY_PART ?  "primary": "secondary" );
    ....

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

Why are we defining it like this? Readability to have pointers for primary
and secondary metadata?

> +
> +	/* if mdata already read and ready */
> +	err = mdata_crc_check(p_mdata);
> +	if (!err)
> +		goto ret_mdata;

Shouldn't we check the secondary metadata ? At least that's what the old
fwu_check_mdata_validity() was doing.

> +	/* 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)
> +			goto ret_mdata;

The error print here is a bit misleading.  It's a failed write, not a crc32
mismatch

> +	}
> +
> +	if (!sec_ok) {
> +		memcpy(s_mdata, p_mdata, sizeof(struct fwu_mdata));
> +		err = fwu_sync_mdata(s_mdata, SECONDARY_PART);
> +		if (err)
> +			goto ret_mdata;
> +	}
> +
> +ret_mdata:
> +	if (err)
> +		log_debug("mdata : crc32 failed\n");
> +	else if (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
> 

Regards
/Ilias

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

[Jassi Brar]

On Mon, 9 Jan 2023 at 06:54, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Jassi,
>
> On Mon, Jan 02, 2023 at 12:26:40PM -0600, Jassi Brar wrote:
> > 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                | 142 ++++++++++++++++++++++++++-
> >  3 files changed, 213 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
.....
> > + */
> > +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));
> > +
> > +     if (part & PRIMARY_PART)
> > +             err = fwu_write_mdata(g_dev, mdata, true);
> > +
> > +     if (err) {
> > +             log_err("Unable to write primary mdata\n");
> > +             return err;
> > +     }
> > +
> > +     if (part & SECONDARY_PART)
> > +             err = fwu_write_mdata(g_dev, mdata, false);
> > +
> > +     if (err) {
> > +             log_err("Unable to write secondary mdata\n");
> > +             return err;
> > +     }
>
> Can we write this
> err = fwu_write_mdata(g_dev, mdata, part & PRIMARY_PART ? true: false);
> if (err)
>     log_err("Unable to write %s partition\n", part & PRIMARY_PART ?  "primary": "secondary" );
>     ....
>
of course :)


> > +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;
>
> Why are we defining it like this? Readability to have pointers for primary
> and secondary metadata?
>
that's the idea.


> > +
> > +     /* if mdata already read and ready */
> > +     err = mdata_crc_check(p_mdata);
> > +     if (!err)
> > +             goto ret_mdata;
>
> Shouldn't we check the secondary metadata ? At least that's what the old
> fwu_check_mdata_validity() was doing.
>
During the first run after boot, both copies are checked. Also when we
update the mdata.
Othwise we have a good primary copy, even if the secondary is
corrupted for some mysterious (corrupted in readonly mode) reason
maybe we should let that be fixed after reboot and not add crc
checking cost to every call?


> > +     /* 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)
> > +                     goto ret_mdata;
>
> The error print here is a bit misleading.  It's a failed write, not a crc32
> mismatch
>
Fixed.

Thanks.

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

[Jassi Brar]

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

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

[Jassi Brar]

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>
---
 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 4554654727..234882af9a 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;
@@ -295,136 +275,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
@@ -437,19 +287,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;
@@ -470,30 +315,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;
@@ -523,15 +363,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");
@@ -552,11 +387,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);
@@ -591,26 +426,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;
@@ -637,16 +467,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];
@@ -655,7 +480,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;
 		}
 	}
@@ -790,8 +615,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)) {
@@ -799,9 +622,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
@@ -837,11 +668,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

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

[Jassi Brar]

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 234882af9a..75bcc01c13 100644
--- a/lib/fwu_updates/fwu.c
+++ b/lib/fwu_updates/fwu.c
@@ -191,7 +191,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,
@@ -199,7 +199,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;
@@ -628,7 +628,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

[PATCHv3 0/5] FWU: Add support for mtd backed feature on DeveloperBox

[Jassi Brar]

Introduce support for mtd backed storage for FWU feature and enable it on
Synquacer platform based DeveloperBox.

This revision is rebased onto patchset that trims the FWU api
  https://lore.kernel.org/u-boot/20230102182532.2411125-1-jaswinder.singh@linaro.org/

Jassi Brar (2):
  dt: fwu: developerbox: enable fwu banks and mdata regions
  fwu: DeveloperBox: add support for FWU

Masami Hiramatsu (1):
  tools: Add mkfwumdata tool for FWU metadata image

Sughosh Ganu (2):
  FWU: Add FWU metadata access driver for MTD storage regions
  FWU: mtd: Add helper functions for accessing FWU metadata

 .../synquacer-sc2a11-developerbox-u-boot.dtsi |  22 +-
 board/socionext/developerbox/Makefile         |   1 +
 board/socionext/developerbox/developerbox.c   |   8 +
 board/socionext/developerbox/fwu_plat.c       |  57 +++
 configs/synquacer_developerbox_defconfig      |  13 +-
 doc/board/socionext/developerbox.rst          |  96 ++++++
 drivers/fwu-mdata/Kconfig                     |  15 +
 drivers/fwu-mdata/Makefile                    |   1 +
 drivers/fwu-mdata/raw_mtd.c                   | 201 +++++++++++
 include/configs/synquacer.h                   |  10 +
 include/fwu.h                                 |  27 ++
 lib/fwu_updates/Makefile                      |   1 +
 lib/fwu_updates/fwu_mtd.c                     | 172 +++++++++
 tools/Kconfig                                 |   9 +
 tools/Makefile                                |   4 +
 tools/mkfwumdata.c                            | 326 ++++++++++++++++++
 16 files changed, 960 insertions(+), 3 deletions(-)
 create mode 100644 board/socionext/developerbox/fwu_plat.c
 create mode 100644 drivers/fwu-mdata/raw_mtd.c
 create mode 100644 lib/fwu_updates/fwu_mtd.c
 create mode 100644 tools/mkfwumdata.c

-- 
2.34.1

[PATCHv3 1/5] FWU: Add FWU metadata access driver for MTD storage regions

[Jassi Brar]

From: Sughosh Ganu <sughosh.ganu@linaro.org>

In the FWU Multi Bank Update feature, the information about the
updatable images is stored as part of the metadata, on a separate
region. Add a driver for reading from and writing to the metadata
when the updatable images and the metadata are stored on a raw
MTD region.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 drivers/fwu-mdata/Kconfig   |  15 +++
 drivers/fwu-mdata/Makefile  |   1 +
 drivers/fwu-mdata/raw_mtd.c | 201 ++++++++++++++++++++++++++++++++++++
 3 files changed, 217 insertions(+)
 create mode 100644 drivers/fwu-mdata/raw_mtd.c

diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig
index 36c4479a59..42736a5e43 100644
--- a/drivers/fwu-mdata/Kconfig
+++ b/drivers/fwu-mdata/Kconfig
@@ -6,6 +6,11 @@ config FWU_MDATA
 	  FWU Metadata partitions reside on the same storage device
 	  which contains the other FWU updatable firmware images.
 
+choice
+	prompt "Storage Layout Scheme"
+	depends on FWU_MDATA
+	default FWU_MDATA_GPT_BLK
+
 config FWU_MDATA_GPT_BLK
 	bool "FWU Metadata access for GPT partitioned Block devices"
 	select PARTITION_TYPE_GUID
@@ -14,3 +19,13 @@ config FWU_MDATA_GPT_BLK
 	help
 	  Enable support for accessing FWU Metadata on GPT partitioned
 	  block devices.
+
+config FWU_MDATA_MTD
+	bool "Raw MTD devices"
+	depends on MTD
+	help
+	  Enable support for accessing FWU Metadata on non-partitioned
+	  (or non-GPT partitioned, e.g. partition nodes in devicetree)
+	  MTD devices.
+
+endchoice
diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile
index 3fee64c10c..06c49747ba 100644
--- a/drivers/fwu-mdata/Makefile
+++ b/drivers/fwu-mdata/Makefile
@@ -6,3 +6,4 @@
 
 obj-$(CONFIG_FWU_MDATA) += fwu-mdata-uclass.o
 obj-$(CONFIG_FWU_MDATA_GPT_BLK) += gpt_blk.o
+obj-$(CONFIG_FWU_MDATA_MTD) += raw_mtd.o
diff --git a/drivers/fwu-mdata/raw_mtd.c b/drivers/fwu-mdata/raw_mtd.c
new file mode 100644
index 0000000000..edd28b4525
--- /dev/null
+++ b/drivers/fwu-mdata/raw_mtd.c
@@ -0,0 +1,201 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2023, Linaro Limited
+ */
+
+#define LOG_CATEGORY UCLASS_FWU_MDATA
+
+#include <fwu.h>
+#include <fwu_mdata.h>
+#include <memalign.h>
+#include <flash.h>
+
+#include <linux/errno.h>
+#include <linux/types.h>
+
+/* Internal helper structure to move data around */
+struct fwu_mdata_mtd_priv {
+	struct mtd_info *mtd;
+	u32 pri_offset;
+	u32 sec_offset;
+};
+
+enum fwu_mtd_op {
+	FWU_MTD_READ,
+	FWU_MTD_WRITE,
+};
+
+#define FWU_MDATA_PRIMARY	true
+#define FWU_MDATA_SECONDARY	false
+
+static bool mtd_is_aligned_with_block_size(struct mtd_info *mtd, u64 size)
+{
+	return !do_div(size, mtd->erasesize);
+}
+
+static int mtd_io_data(struct mtd_info *mtd, u32 offs, u32 size, void *data,
+		       enum fwu_mtd_op op)
+{
+	struct mtd_oob_ops io_op ={};
+	u64 lock_offs, lock_len;
+	size_t len;
+	void *buf;
+	int ret;
+
+	if (!mtd_is_aligned_with_block_size(mtd, offs)) {
+		log_err("Offset unaligned with a block (0x%x)\n", mtd->erasesize);
+		return -EINVAL;
+	}
+
+	lock_offs = offs;
+	/* This will expand erase size to align with the block size */
+	lock_len = round_up(size, mtd->erasesize);
+
+	ret = mtd_unlock(mtd, lock_offs, lock_len);
+	if (ret && ret != -EOPNOTSUPP)
+		return ret;
+
+	if (op == FWU_MTD_WRITE) {
+		struct erase_info erase_op = {};
+
+		erase_op.mtd = mtd;
+		erase_op.addr = lock_offs;
+		erase_op.len = lock_len;
+		erase_op.scrub = 0;
+
+		ret = mtd_erase(mtd, &erase_op);
+		if (ret)
+			goto lock;
+	}
+
+	/* Also, expand the write size to align with the write size */
+	len = round_up(size, mtd->writesize);
+
+	buf = memalign(ARCH_DMA_MINALIGN, len);
+	if (!buf) {
+		ret = -ENOMEM;
+		goto lock;
+	}
+	memset(buf, 0xff, len);
+
+	io_op.mode = MTD_OPS_AUTO_OOB;
+	io_op.len = len;
+	io_op.ooblen = 0;
+	io_op.datbuf = buf;
+	io_op.oobbuf = NULL;
+
+	if (op == FWU_MTD_WRITE) {
+		memcpy(buf, data, size);
+		ret = mtd_write_oob(mtd, offs, &io_op);
+	} else {
+		ret = mtd_read_oob(mtd, offs, &io_op);
+		if (!ret)
+			memcpy(data, buf, size);
+	}
+	free(buf);
+
+lock:
+	mtd_lock(mtd, lock_offs, lock_len);
+
+	return ret;
+}
+
+static int fwu_mtd_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
+{
+	struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
+	struct mtd_info *mtd = mtd_priv->mtd;
+	u32 offs = primary ? mtd_priv->pri_offset : mtd_priv->sec_offset;
+
+	return mtd_io_data(mtd, offs, sizeof(struct fwu_mdata), mdata, FWU_MTD_READ);
+}
+
+static int fwu_mtd_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
+{
+	struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
+	struct mtd_info *mtd = mtd_priv->mtd;
+	u32 offs = primary ? mtd_priv->pri_offset : mtd_priv->sec_offset;
+
+	return mtd_io_data(mtd, offs, sizeof(struct fwu_mdata), mdata, FWU_MTD_WRITE);
+}
+
+/**
+ * fwu_mdata_mtd_of_to_plat() - Translate from DT to fwu mdata device
+ */
+static int fwu_mdata_mtd_of_to_plat(struct udevice *dev)
+{
+	struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
+	const fdt32_t *phandle_p = NULL;
+	struct udevice *mtd_dev;
+	struct mtd_info *mtd;
+	int ret, size;
+	u32 phandle;
+
+	/* Find the FWU mdata storage device */
+	phandle_p = ofnode_get_property(dev_ofnode(dev),
+					"fwu-mdata-store", &size);
+	if (!phandle_p) {
+		log_err("FWU meta data store not defined in device-tree\n");
+		return -ENOENT;
+	}
+
+	phandle = fdt32_to_cpu(*phandle_p);
+
+	ret = device_get_global_by_ofnode(ofnode_get_by_phandle(phandle),
+									  &mtd_dev);
+	if (ret) {
+		log_err("FWU: failed to get mtd device\n");
+		return ret;
+	}
+
+	mtd_probe_devices();
+
+	mtd_for_each_device(mtd) {
+		if (mtd->dev == mtd_dev) {
+			mtd_priv->mtd = mtd;
+			log_debug("Found the FWU mdata mtd device %s\n", mtd->name);
+			break;
+		}
+	}
+	if (!mtd_priv->mtd) {
+		log_err("Failed to find mtd device by fwu-mdata-store\n");
+		return -ENOENT;
+	}
+
+	/* Get the offset of primary and seconday mdata */
+	ret = ofnode_read_u32_index(dev_ofnode(dev), "mdata-offsets", 0,
+				    &mtd_priv->pri_offset);
+	if (ret)
+		return ret;
+	ret = ofnode_read_u32_index(dev_ofnode(dev), "mdata-offsets", 1,
+				    &mtd_priv->sec_offset);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int fwu_mdata_mtd_probe(struct udevice *dev)
+{
+	/* Ensure the metadata can be read. */
+	return fwu_get_mdata(NULL);
+}
+
+static struct fwu_mdata_ops fwu_mtd_ops = {
+	.read_mdata = fwu_mtd_read_mdata,
+	.write_mdata = fwu_mtd_write_mdata,
+};
+
+static const struct udevice_id fwu_mdata_ids[] = {
+	{ .compatible = "u-boot,fwu-mdata-mtd" },
+	{ }
+};
+
+U_BOOT_DRIVER(fwu_mdata_mtd) = {
+	.name		= "fwu-mdata-mtd",
+	.id		= UCLASS_FWU_MDATA,
+	.of_match	= fwu_mdata_ids,
+	.ops		= &fwu_mtd_ops,
+	.probe		= fwu_mdata_mtd_probe,
+	.of_to_plat	= fwu_mdata_mtd_of_to_plat,
+	.priv_auto	= sizeof(struct fwu_mdata_mtd_priv),
+};
-- 
2.34.1

Re: [PATCHv3 1/5] FWU: Add FWU metadata access driver for MTD storage regions

[Sughosh Ganu]

On Mon, 9 Jan 2023 at 06:36, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> From: Sughosh Ganu <sughosh.ganu@linaro.org>
>
> In the FWU Multi Bank Update feature, the information about the
> updatable images is stored as part of the metadata, on a separate
> region. Add a driver for reading from and writing to the metadata
> when the updatable images and the metadata are stored on a raw
> MTD region.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---

This patch is authored by Masami Hiramatsu. Please attribute it to him.

-sughosh

Re: [PATCHv3 1/5] FWU: Add FWU metadata access driver for MTD storage regions

[Michal Simek]

On 1/9/23 02:06, Jassi Brar wrote:
> From: Sughosh Ganu <sughosh.ganu@linaro.org>
> 
> In the FWU Multi Bank Update feature, the information about the
> updatable images is stored as part of the metadata, on a separate
> region. Add a driver for reading from and writing to the metadata
> when the updatable images and the metadata are stored on a raw
> MTD region.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>   drivers/fwu-mdata/Kconfig   |  15 +++
>   drivers/fwu-mdata/Makefile  |   1 +
>   drivers/fwu-mdata/raw_mtd.c | 201 ++++++++++++++++++++++++++++++++++++
>   3 files changed, 217 insertions(+)
>   create mode 100644 drivers/fwu-mdata/raw_mtd.c
> 
> diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig
> index 36c4479a59..42736a5e43 100644
> --- a/drivers/fwu-mdata/Kconfig
> +++ b/drivers/fwu-mdata/Kconfig
> @@ -6,6 +6,11 @@ config FWU_MDATA
>   	  FWU Metadata partitions reside on the same storage device
>   	  which contains the other FWU updatable firmware images.
>   
> +choice
> +	prompt "Storage Layout Scheme"
> +	depends on FWU_MDATA
> +	default FWU_MDATA_GPT_BLK
> +
>   config FWU_MDATA_GPT_BLK
>   	bool "FWU Metadata access for GPT partitioned Block devices"
>   	select PARTITION_TYPE_GUID
> @@ -14,3 +19,13 @@ config FWU_MDATA_GPT_BLK
>   	help
>   	  Enable support for accessing FWU Metadata on GPT partitioned
>   	  block devices.
> +
> +config FWU_MDATA_MTD
> +	bool "Raw MTD devices"
> +	depends on MTD
> +	help
> +	  Enable support for accessing FWU Metadata on non-partitioned
> +	  (or non-GPT partitioned, e.g. partition nodes in devicetree)
> +	  MTD devices.
> +
> +endchoice
> diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile
> index 3fee64c10c..06c49747ba 100644
> --- a/drivers/fwu-mdata/Makefile
> +++ b/drivers/fwu-mdata/Makefile
> @@ -6,3 +6,4 @@
>   
>   obj-$(CONFIG_FWU_MDATA) += fwu-mdata-uclass.o
>   obj-$(CONFIG_FWU_MDATA_GPT_BLK) += gpt_blk.o
> +obj-$(CONFIG_FWU_MDATA_MTD) += raw_mtd.o
> diff --git a/drivers/fwu-mdata/raw_mtd.c b/drivers/fwu-mdata/raw_mtd.c
> new file mode 100644
> index 0000000000..edd28b4525
> --- /dev/null
> +++ b/drivers/fwu-mdata/raw_mtd.c
> @@ -0,0 +1,201 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2023, Linaro Limited
> + */
> +
> +#define LOG_CATEGORY UCLASS_FWU_MDATA
> +
> +#include <fwu.h>
> +#include <fwu_mdata.h>
> +#include <memalign.h>
> +#include <flash.h>

sort them.

> +
> +#include <linux/errno.h>
> +#include <linux/types.h>
> +
> +/* Internal helper structure to move data around */
> +struct fwu_mdata_mtd_priv {
> +	struct mtd_info *mtd;
> +	u32 pri_offset;
> +	u32 sec_offset;
> +};
> +
> +enum fwu_mtd_op {
> +	FWU_MTD_READ,
> +	FWU_MTD_WRITE,
> +};
> +
> +#define FWU_MDATA_PRIMARY	true
> +#define FWU_MDATA_SECONDARY	false

Completely unused.

> +
> +static bool mtd_is_aligned_with_block_size(struct mtd_info *mtd, u64 size)
> +{
> +	return !do_div(size, mtd->erasesize);
> +}
> +
> +static int mtd_io_data(struct mtd_info *mtd, u32 offs, u32 size, void *data,
> +		       enum fwu_mtd_op op)
> +{
> +	struct mtd_oob_ops io_op ={};
> +	u64 lock_offs, lock_len;
> +	size_t len;
> +	void *buf;
> +	int ret;
> +
> +	if (!mtd_is_aligned_with_block_size(mtd, offs)) {
> +		log_err("Offset unaligned with a block (0x%x)\n", mtd->erasesize);
> +		return -EINVAL;
> +	}
> +
> +	lock_offs = offs;
> +	/* This will expand erase size to align with the block size */
> +	lock_len = round_up(size, mtd->erasesize);
> +
> +	ret = mtd_unlock(mtd, lock_offs, lock_len);
> +	if (ret && ret != -EOPNOTSUPP)
> +		return ret;
> +
> +	if (op == FWU_MTD_WRITE) {
> +		struct erase_info erase_op = {};
> +
> +		erase_op.mtd = mtd;
> +		erase_op.addr = lock_offs;
> +		erase_op.len = lock_len;
> +		erase_op.scrub = 0;
> +
> +		ret = mtd_erase(mtd, &erase_op);
> +		if (ret)
> +			goto lock;
> +	}
> +
> +	/* Also, expand the write size to align with the write size */
> +	len = round_up(size, mtd->writesize);
> +
> +	buf = memalign(ARCH_DMA_MINALIGN, len);
> +	if (!buf) {
> +		ret = -ENOMEM;
> +		goto lock;
> +	}
> +	memset(buf, 0xff, len);
> +
> +	io_op.mode = MTD_OPS_AUTO_OOB;
> +	io_op.len = len;
> +	io_op.ooblen = 0;
> +	io_op.datbuf = buf;
> +	io_op.oobbuf = NULL;
> +
> +	if (op == FWU_MTD_WRITE) {
> +		memcpy(buf, data, size);
> +		ret = mtd_write_oob(mtd, offs, &io_op);
> +	} else {
> +		ret = mtd_read_oob(mtd, offs, &io_op);
> +		if (!ret)
> +			memcpy(data, buf, size);
> +	}
> +	free(buf);
> +
> +lock:
> +	mtd_lock(mtd, lock_offs, lock_len);
> +
> +	return ret;
> +}
> +
> +static int fwu_mtd_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
> +{
> +	struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> +	struct mtd_info *mtd = mtd_priv->mtd;
> +	u32 offs = primary ? mtd_priv->pri_offset : mtd_priv->sec_offset;
> +
> +	return mtd_io_data(mtd, offs, sizeof(struct fwu_mdata), mdata, FWU_MTD_READ);
> +}
> +
> +static int fwu_mtd_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
> +{
> +	struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> +	struct mtd_info *mtd = mtd_priv->mtd;
> +	u32 offs = primary ? mtd_priv->pri_offset : mtd_priv->sec_offset;
> +
> +	return mtd_io_data(mtd, offs, sizeof(struct fwu_mdata), mdata, FWU_MTD_WRITE);
> +}
> +
> +/**
> + * fwu_mdata_mtd_of_to_plat() - Translate from DT to fwu mdata device
> + */

Fix kernel-doc format.

./scripts/kernel-doc -man -v drivers/fwu-mdata/raw_mtd.c 1>/dev/null
drivers/fwu-mdata/raw_mtd.c:122: info: Scanning doc for fwu_mdata_mtd_of_to_plat
drivers/fwu-mdata/raw_mtd.c:125: warning: Function parameter or member 'dev' not 
described in 'fwu_mdata_mtd_of_to_plat'
drivers/fwu-mdata/raw_mtd.c:125: warning: No description found for return value 
of 'fwu_mdata_mtd_of_to_plat'



> +static int fwu_mdata_mtd_of_to_plat(struct udevice *dev)
> +{
> +	struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> +	const fdt32_t *phandle_p = NULL;
> +	struct udevice *mtd_dev;
> +	struct mtd_info *mtd;
> +	int ret, size;
> +	u32 phandle;
> +
> +	/* Find the FWU mdata storage device */
> +	phandle_p = ofnode_get_property(dev_ofnode(dev),
> +					"fwu-mdata-store", &size);
> +	if (!phandle_p) {
> +		log_err("FWU meta data store not defined in device-tree\n");
> +		return -ENOENT;
> +	}
> +
> +	phandle = fdt32_to_cpu(*phandle_p);
> +
> +	ret = device_get_global_by_ofnode(ofnode_get_by_phandle(phandle),
> +									  &mtd_dev);
> +	if (ret) {
> +		log_err("FWU: failed to get mtd device\n");
> +		return ret;
> +	}
> +
> +	mtd_probe_devices();
> +
> +	mtd_for_each_device(mtd) {
> +		if (mtd->dev == mtd_dev) {
> +			mtd_priv->mtd = mtd;
> +			log_debug("Found the FWU mdata mtd device %s\n", mtd->name);
> +			break;
> +		}
> +	}
> +	if (!mtd_priv->mtd) {
> +		log_err("Failed to find mtd device by fwu-mdata-store\n");
> +		return -ENOENT;

-ENODEV is likely better.

> +	}
> +
> +	/* Get the offset of primary and seconday mdata */
> +	ret = ofnode_read_u32_index(dev_ofnode(dev), "mdata-offsets", 0,
> +				    &mtd_priv->pri_offset);
> +	if (ret)
> +		return ret;
> +	ret = ofnode_read_u32_index(dev_ofnode(dev), "mdata-offsets", 1,
> +				    &mtd_priv->sec_offset);
> +	if (ret)
> +		return ret;

I am not getting usage of these offsets.
First of all in DT patch you are using

+		fwu-mdata {
+			compatible = "u-boot,fwu-mdata-mtd";
+			fwu-mdata-store = <&spi_flash>;
+			mdata-offsets = <0x500000 0x530000>;
+		};

which is based on DT going to location which is already labelled.

  79                                 partition@500000 {
  80                                         label = "Ex-OPTEE";
  81                                         reg = <0x500000 0x200000>;
  82                                 };

I don't know what this space is used for but the whole code around is using MTD 
partitions and it's infrastructure and this is using RAW access without MTD.

Why not to create separate partitions just for storing metadata?
And also identify them like that.

Or just switch it to complete RAW mode without MTD and then offsets can be used 
(but I expect with different dt description).


> +
> +	return 0;
> +}
> +
> +static int fwu_mdata_mtd_probe(struct udevice *dev)
> +{
> +	/* Ensure the metadata can be read. */
> +	return fwu_get_mdata(NULL);

Likely I am not getting how this is used but I would expect that ops->get_mdata 
function is going to befined here too.

And also ops->update_mdata


> +}
> +
> +static struct fwu_mdata_ops fwu_mtd_ops = {
> +	.read_mdata = fwu_mtd_read_mdata,
> +	.write_mdata = fwu_mtd_write_mdata,
> +};
> +
> +static const struct udevice_id fwu_mdata_ids[] = {
> +	{ .compatible = "u-boot,fwu-mdata-mtd" },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(fwu_mdata_mtd) = {
> +	.name		= "fwu-mdata-mtd",
> +	.id		= UCLASS_FWU_MDATA,
> +	.of_match	= fwu_mdata_ids,
> +	.ops		= &fwu_mtd_ops,
> +	.probe		= fwu_mdata_mtd_probe,
> +	.of_to_plat	= fwu_mdata_mtd_of_to_plat,
> +	.priv_auto	= sizeof(struct fwu_mdata_mtd_priv),
> +};

Thanks,
Michal

Re: [PATCHv3 1/5] FWU: Add FWU metadata access driver for MTD storage regions

[Jassi Brar]

On Wed, 18 Jan 2023 at 08:24, Michal Simek <michal.simek@amd.com> wrote:
>
>
>
> On 1/9/23 02:06, Jassi Brar wrote:
> > From: Sughosh Ganu <sughosh.ganu@linaro.org>
> >
> > In the FWU Multi Bank Update feature, the information about the
> > updatable images is stored as part of the metadata, on a separate
> > region. Add a driver for reading from and writing to the metadata
> > when the updatable images and the metadata are stored on a raw
> > MTD region.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> > ---
> >   drivers/fwu-mdata/Kconfig   |  15 +++
> >   drivers/fwu-mdata/Makefile  |   1 +
> >   drivers/fwu-mdata/raw_mtd.c | 201 ++++++++++++++++++++++++++++++++++++
> >   3 files changed, 217 insertions(+)
> >   create mode 100644 drivers/fwu-mdata/raw_mtd.c
> >
> > diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig
> > index 36c4479a59..42736a5e43 100644
> > --- a/drivers/fwu-mdata/Kconfig
> > +++ b/drivers/fwu-mdata/Kconfig
> > @@ -6,6 +6,11 @@ config FWU_MDATA
> >         FWU Metadata partitions reside on the same storage device
> >         which contains the other FWU updatable firmware images.
> >
> > +choice
> > +     prompt "Storage Layout Scheme"
> > +     depends on FWU_MDATA
> > +     default FWU_MDATA_GPT_BLK
> > +
> >   config FWU_MDATA_GPT_BLK
> >       bool "FWU Metadata access for GPT partitioned Block devices"
> >       select PARTITION_TYPE_GUID
> > @@ -14,3 +19,13 @@ config FWU_MDATA_GPT_BLK
> >       help
> >         Enable support for accessing FWU Metadata on GPT partitioned
> >         block devices.
> > +
> > +config FWU_MDATA_MTD
> > +     bool "Raw MTD devices"
> > +     depends on MTD
> > +     help
> > +       Enable support for accessing FWU Metadata on non-partitioned
> > +       (or non-GPT partitioned, e.g. partition nodes in devicetree)
> > +       MTD devices.
> > +
> > +endchoice
> > diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile
> > index 3fee64c10c..06c49747ba 100644
> > --- a/drivers/fwu-mdata/Makefile
> > +++ b/drivers/fwu-mdata/Makefile
> > @@ -6,3 +6,4 @@
> >
> >   obj-$(CONFIG_FWU_MDATA) += fwu-mdata-uclass.o
> >   obj-$(CONFIG_FWU_MDATA_GPT_BLK) += gpt_blk.o
> > +obj-$(CONFIG_FWU_MDATA_MTD) += raw_mtd.o
> > diff --git a/drivers/fwu-mdata/raw_mtd.c b/drivers/fwu-mdata/raw_mtd.c
> > new file mode 100644
> > index 0000000000..edd28b4525
> > --- /dev/null
> > +++ b/drivers/fwu-mdata/raw_mtd.c
> > @@ -0,0 +1,201 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2023, Linaro Limited
> > + */
> > +
> > +#define LOG_CATEGORY UCLASS_FWU_MDATA
> > +
> > +#include <fwu.h>
> > +#include <fwu_mdata.h>
> > +#include <memalign.h>
> > +#include <flash.h>
>
> sort them.
>
actually flash.h is not required.


> > +
> > +#include <linux/errno.h>
> > +#include <linux/types.h>
> > +
> > +/* Internal helper structure to move data around */
> > +struct fwu_mdata_mtd_priv {
> > +     struct mtd_info *mtd;
> > +     u32 pri_offset;
> > +     u32 sec_offset;
> > +};
> > +
> > +enum fwu_mtd_op {
> > +     FWU_MTD_READ,
> > +     FWU_MTD_WRITE,
> > +};
> > +
> > +#define FWU_MDATA_PRIMARY    true
> > +#define FWU_MDATA_SECONDARY  false
>
> Completely unused.
>
yes, removed.


> > +
> > +static bool mtd_is_aligned_with_block_size(struct mtd_info *mtd, u64 size)
> > +{
> > +     return !do_div(size, mtd->erasesize);
> > +}
> > +
> > +static int mtd_io_data(struct mtd_info *mtd, u32 offs, u32 size, void *data,
> > +                    enum fwu_mtd_op op)
> > +{
> > +     struct mtd_oob_ops io_op ={};
> > +     u64 lock_offs, lock_len;
> > +     size_t len;
> > +     void *buf;
> > +     int ret;
> > +
> > +     if (!mtd_is_aligned_with_block_size(mtd, offs)) {
> > +             log_err("Offset unaligned with a block (0x%x)\n", mtd->erasesize);
> > +             return -EINVAL;
> > +     }
> > +
> > +     lock_offs = offs;
> > +     /* This will expand erase size to align with the block size */
> > +     lock_len = round_up(size, mtd->erasesize);
> > +
> > +     ret = mtd_unlock(mtd, lock_offs, lock_len);
> > +     if (ret && ret != -EOPNOTSUPP)
> > +             return ret;
> > +
> > +     if (op == FWU_MTD_WRITE) {
> > +             struct erase_info erase_op = {};
> > +
> > +             erase_op.mtd = mtd;
> > +             erase_op.addr = lock_offs;
> > +             erase_op.len = lock_len;
> > +             erase_op.scrub = 0;
> > +
> > +             ret = mtd_erase(mtd, &erase_op);
> > +             if (ret)
> > +                     goto lock;
> > +     }
> > +
> > +     /* Also, expand the write size to align with the write size */
> > +     len = round_up(size, mtd->writesize);
> > +
> > +     buf = memalign(ARCH_DMA_MINALIGN, len);
> > +     if (!buf) {
> > +             ret = -ENOMEM;
> > +             goto lock;
> > +     }
> > +     memset(buf, 0xff, len);
> > +
> > +     io_op.mode = MTD_OPS_AUTO_OOB;
> > +     io_op.len = len;
> > +     io_op.ooblen = 0;
> > +     io_op.datbuf = buf;
> > +     io_op.oobbuf = NULL;
> > +
> > +     if (op == FWU_MTD_WRITE) {
> > +             memcpy(buf, data, size);
> > +             ret = mtd_write_oob(mtd, offs, &io_op);
> > +     } else {
> > +             ret = mtd_read_oob(mtd, offs, &io_op);
> > +             if (!ret)
> > +                     memcpy(data, buf, size);
> > +     }
> > +     free(buf);
> > +
> > +lock:
> > +     mtd_lock(mtd, lock_offs, lock_len);
> > +
> > +     return ret;
> > +}
> > +
> > +static int fwu_mtd_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
> > +{
> > +     struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> > +     struct mtd_info *mtd = mtd_priv->mtd;
> > +     u32 offs = primary ? mtd_priv->pri_offset : mtd_priv->sec_offset;
> > +
> > +     return mtd_io_data(mtd, offs, sizeof(struct fwu_mdata), mdata, FWU_MTD_READ);
> > +}
> > +
> > +static int fwu_mtd_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
> > +{
> > +     struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> > +     struct mtd_info *mtd = mtd_priv->mtd;
> > +     u32 offs = primary ? mtd_priv->pri_offset : mtd_priv->sec_offset;
> > +
> > +     return mtd_io_data(mtd, offs, sizeof(struct fwu_mdata), mdata, FWU_MTD_WRITE);
> > +}
> > +
> > +/**
> > + * fwu_mdata_mtd_of_to_plat() - Translate from DT to fwu mdata device
> > + */
>
> Fix kernel-doc format.
>
I think we don't even need to document the static .of_to_plat() callback


>
> > +static int fwu_mdata_mtd_of_to_plat(struct udevice *dev)
> > +{
> > +     struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> > +     const fdt32_t *phandle_p = NULL;
> > +     struct udevice *mtd_dev;
> > +     struct mtd_info *mtd;
> > +     int ret, size;
> > +     u32 phandle;
> > +
> > +     /* Find the FWU mdata storage device */
> > +     phandle_p = ofnode_get_property(dev_ofnode(dev),
> > +                                     "fwu-mdata-store", &size);
> > +     if (!phandle_p) {
> > +             log_err("FWU meta data store not defined in device-tree\n");
> > +             return -ENOENT;
> > +     }
> > +
> > +     phandle = fdt32_to_cpu(*phandle_p);
> > +
> > +     ret = device_get_global_by_ofnode(ofnode_get_by_phandle(phandle),
> > +                                                                       &mtd_dev);
> > +     if (ret) {
> > +             log_err("FWU: failed to get mtd device\n");
> > +             return ret;
> > +     }
> > +
> > +     mtd_probe_devices();
> > +
> > +     mtd_for_each_device(mtd) {
> > +             if (mtd->dev == mtd_dev) {
> > +                     mtd_priv->mtd = mtd;
> > +                     log_debug("Found the FWU mdata mtd device %s\n", mtd->name);
> > +                     break;
> > +             }
> > +     }
> > +     if (!mtd_priv->mtd) {
> > +             log_err("Failed to find mtd device by fwu-mdata-store\n");
> > +             return -ENOENT;
>
> -ENODEV is likely better.
>
ok.


> > +     }
> > +
> > +     /* Get the offset of primary and seconday mdata */
> > +     ret = ofnode_read_u32_index(dev_ofnode(dev), "mdata-offsets", 0,
> > +                                 &mtd_priv->pri_offset);
> > +     if (ret)
> > +             return ret;
> > +     ret = ofnode_read_u32_index(dev_ofnode(dev), "mdata-offsets", 1,
> > +                                 &mtd_priv->sec_offset);
> > +     if (ret)
> > +             return ret;
>
> I am not getting usage of these offsets.
> First of all in DT patch you are using
>
> +               fwu-mdata {
> +                       compatible = "u-boot,fwu-mdata-mtd";
> +                       fwu-mdata-store = <&spi_flash>;
> +                       mdata-offsets = <0x500000 0x530000>;
> +               };
>
> which is based on DT going to location which is already labelled.
>
>   79                                 partition@500000 {
>   80                                         label = "Ex-OPTEE";
>   81                                         reg = <0x500000 0x200000>;
>   82                                 };
>
The 'ex-optee' is actually unused so we never faced any issue. But
yes, this is inconsistent.

> I don't know what this space is used for but the whole code around is using MTD
> partitions and it's infrastructure and this is using RAW access without MTD.
>
> Why not to create separate partitions just for storing metadata?
> And also identify them like that.
>
> Or just switch it to complete RAW mode without MTD and then offsets can be used
> (but I expect with different dt description).
>
The design predates my involvement in fwu. I guess the reason was to
have a mechanism similar to GPT based implementation which uses a
similar fwu-mdata {} node structure.  It does make sense to have
dedicated partitions for primary and  secondary meta-data, identified
by uuid (like Banks) or standard labels. But may be Sughosh/Ilias know
why the current approach was chosen.


>
> > +
> > +     return 0;
> > +}
> > +
> > +static int fwu_mdata_mtd_probe(struct udevice *dev)
> > +{
> > +     /* Ensure the metadata can be read. */
> > +     return fwu_get_mdata(NULL);
>
> Likely I am not getting how this is used but I would expect that ops->get_mdata
> function is going to befined here too.
>
> And also ops->update_mdata
>
This just caches the meta-data after checking/fixing for its
integrity. This way we avoid reading from mtd for each mdata fetch.

thanks

Re: [PATCHv3 1/5] FWU: Add FWU metadata access driver for MTD storage regions

[Jassi Brar]

On Sat, 4 Feb 2023 at 22:09, Jassi Brar <jaswinder.singh@linaro.org> wrote:
> >
> > +               fwu-mdata {
> > +                       compatible = "u-boot,fwu-mdata-mtd";
> > +                       fwu-mdata-store = <&spi_flash>;
> > +                       mdata-offsets = <0x500000 0x530000>;
> > +               };
> >
> > which is based on DT going to location which is already labelled.
> >
> >   79                                 partition@500000 {
> >   80                                         label = "Ex-OPTEE";
> >   81                                         reg = <0x500000 0x200000>;
> >   82                                 };
> >
> The 'ex-optee' is actually unused so we never faced any issue. But
> yes, this is inconsistent.
>
> > I don't know what this space is used for but the whole code around is using MTD
> > partitions and it's infrastructure and this is using RAW access without MTD.
> >
> > Why not to create separate partitions just for storing metadata?
> > And also identify them like that.
> >
> > Or just switch it to complete RAW mode without MTD and then offsets can be used
> > (but I expect with different dt description).
> >
> The design predates my involvement in fwu. I guess the reason was to
> have a mechanism similar to GPT based implementation which uses a
> similar fwu-mdata {} node structure.  It does make sense to have
> dedicated partitions for primary and  secondary meta-data, identified
> by uuid (like Banks) or standard labels. But may be Sughosh/Ilias know
> why the current approach was chosen.
>
I changed the bindings to not require any changes to any non-FWU subsystem.
Now every bit of info is contained in 'fwu-mdata' node.
   https://lore.kernel.org/u-boot/20230228005218.1635781-1-jassisinghbrar@gmail.com/T/#u

Thanks.

[PATCHv3 2/5] FWU: mtd: Add helper functions for accessing FWU metadata

[Jassi Brar]

From: Sughosh Ganu <sughosh.ganu@linaro.org>

Add helper functions needed for accessing the FWU metadata which
contains information on the updatable images.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 include/fwu.h             |  27 ++++++
 lib/fwu_updates/Makefile  |   1 +
 lib/fwu_updates/fwu_mtd.c | 172 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 200 insertions(+)
 create mode 100644 lib/fwu_updates/fwu_mtd.c

diff --git a/include/fwu.h b/include/fwu.h
index ea25aca2cd..2edfa72caf 100644
--- a/include/fwu.h
+++ b/include/fwu.h
@@ -8,6 +8,7 @@
 
 #include <blk.h>
 #include <efi.h>
+#include <mtd.h>
 
 #include <linux/types.h>
 
@@ -251,4 +252,30 @@ u8 fwu_empty_capsule_checks_pass(void);
  */
 int fwu_trial_state_ctr_start(void);
 
+/**
+ * fwu_gen_alt_info_from_mtd() - Parse dfu_alt_info from metadata in mtd
+ * @buf: Buffer into which the dfu_alt_info is filled
+ * @len: Maximum charaters that can be written in buf
+ * @mtd: Pointer to underlying MTD device
+ *
+ * Parse dfu_alt_info from metadata in mtd. Used for setting the env.
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_gen_alt_info_from_mtd(char *buf, size_t len, struct mtd_info *mtd);
+
+/**
+ * fwu_mtd_get_alt_num() - Mapping of fwu_plat_get_alt_num for MTD device
+ * @image_guid: Image GUID for which DFU alt number needs to be retrieved
+ * @alt_num: Pointer to the alt_num
+ * @mtd_dev: Name of mtd device instance
+ *
+ * To map fwu_plat_get_alt_num onto mtd based metadata implementation.
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_mtd_get_alt_num(efi_guid_t *image_id, u8 *alt_num, const char *mtd_dev);
+
 #endif /* _FWU_H_ */
diff --git a/lib/fwu_updates/Makefile b/lib/fwu_updates/Makefile
index 1993088e5b..c9e3c06b48 100644
--- a/lib/fwu_updates/Makefile
+++ b/lib/fwu_updates/Makefile
@@ -5,3 +5,4 @@
 
 obj-$(CONFIG_FWU_MULTI_BANK_UPDATE) += fwu.o
 obj-$(CONFIG_FWU_MDATA_GPT_BLK) += fwu_gpt.o
+obj-$(CONFIG_FWU_MDATA_MTD) += fwu_mtd.o
diff --git a/lib/fwu_updates/fwu_mtd.c b/lib/fwu_updates/fwu_mtd.c
new file mode 100644
index 0000000000..1895b8fab3
--- /dev/null
+++ b/lib/fwu_updates/fwu_mtd.c
@@ -0,0 +1,172 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023, Linaro Limited
+ */
+
+#include <dm.h>
+#include <dfu.h>
+#include <fwu.h>
+#include <fwu_mdata.h>
+#include <log.h>
+#include <malloc.h>
+#include <mtd.h>
+#include <uuid.h>
+#include <vsprintf.h>
+
+#include <dm/ofnode.h>
+
+int fwu_mtd_get_alt_num(efi_guid_t *image_id, u8 *alt_num,
+			const char *mtd_dev)
+{
+	int i, nalt;
+	int ret = -1;
+	struct mtd_info *mtd;
+	struct dfu_entity *dfu;
+	ofnode node, parts_node;
+	fdt_addr_t offset, size;
+	char uuidbuf[UUID_STR_LEN + 1];
+
+	mtd_probe_devices();
+	mtd = get_mtd_device_nm(mtd_dev);
+
+	/* Find partition node under given MTD device. */
+	parts_node = ofnode_by_compatible(mtd_get_ofnode(mtd),
+					  "fixed-partitions");
+
+	uuid_bin_to_str(image_id->b, uuidbuf, UUID_STR_FORMAT_STD);
+	node = ofnode_by_prop_value(parts_node, "uuid", uuidbuf,
+				    sizeof(uuidbuf));
+	if (!ofnode_valid(node)) {
+		log_warning("Warning: Failed to find partition by image UUID\n");
+		return -ENOENT;
+	}
+
+	offset = ofnode_get_addr_size_index_notrans(node, 0, &size);
+	if (offset == FDT_ADDR_T_NONE || !size)
+		return -ENOENT;
+
+	dfu_init_env_entities(NULL, NULL);
+
+	nalt = 0;
+	list_for_each_entry(dfu, &dfu_list, list)
+		nalt++;
+
+	if (!nalt) {
+		log_warning("No entities in dfu_alt_info\n");
+		dfu_free_entities();
+		return -ENOENT;
+	}
+
+	for (i = 0; i < nalt; i++) {
+		dfu = dfu_get_entity(i);
+
+		/* Only MTD RAW access */
+		if (!dfu || dfu->dev_type != DFU_DEV_MTD ||
+			dfu->layout != DFU_RAW_ADDR ||
+			dfu->data.mtd.start != offset ||
+			dfu->data.mtd.size != size)
+			continue;
+
+		*alt_num = dfu->alt;
+		ret = 0;
+		break;
+	}
+
+	dfu_free_entities();
+
+	return ret;
+}
+
+static int gen_image_alt_info(char *buf, size_t len, int sidx,
+		       struct fwu_image_entry *img, struct mtd_info *mtd)
+{
+	int i;
+	const char *suuid;
+	ofnode node, parts_node;
+	char uuidbuf[UUID_STR_LEN + 1];
+	char *p = buf, *end = buf + len;
+
+	/* Find partition node under given MTD device. */
+	parts_node = ofnode_by_compatible(mtd_get_ofnode(mtd),
+					  "fixed-partitions");
+	if (!ofnode_valid(parts_node))
+		return -ENOENT;
+
+	/* Check the media UUID if exist. */
+	suuid = ofnode_read_string(parts_node, "uuid");
+	if (suuid) {
+		log_debug("Got location uuid %s\n", suuid);
+		uuid_bin_to_str(img->location_uuid.b, uuidbuf, UUID_STR_FORMAT_STD);
+		if (strcasecmp(suuid, uuidbuf))
+			log_warning("Warning: Location UUID does not match!\n");
+	}
+
+	p += snprintf(p, end - p, "mtd %s", mtd->name);
+	if (end < p) {
+		log_err("%s:%d Run out of buffer\n", __func__, __LINE__);
+		return -E2BIG;
+	}
+
+	/*
+	 * List the image banks in the FWU mdata and search the corresponding
+	 * partition based on partition's uuid.
+	 */
+	for (i = 0; i < CONFIG_FWU_NUM_BANKS; i++) {
+		struct fwu_image_bank_info *bank;
+		fdt_addr_t offset, size;
+
+		/* Query a partition by image UUID */
+		bank = &img->img_bank_info[i];
+		uuid_bin_to_str(bank->image_uuid.b, uuidbuf, UUID_STR_FORMAT_STD);
+		node = ofnode_by_prop_value(parts_node, "uuid", uuidbuf,
+					    sizeof(uuidbuf));
+		if (!ofnode_valid(node)) {
+			log_err("Failed to find partition by image UUID\n");
+			break;
+		}
+
+		offset = ofnode_get_addr_size_index_notrans(node, 0, &size);
+		if (offset == FDT_ADDR_T_NONE || !size)
+			break;
+
+		p += snprintf(p, end - p, "%sbank%d raw %lx %lx",
+			      i == 0 ? "=" : ";", i, (unsigned long)offset,
+			      (unsigned long)size);
+		if (end < p) {
+			log_err("%s:%d Run out of buffer\n", __func__, __LINE__);
+			return -E2BIG;
+		}
+	}
+
+	if (i == CONFIG_FWU_NUM_BANKS)
+		return 0;
+
+	return -ENOENT;
+}
+
+int fwu_gen_alt_info_from_mtd(char *buf, size_t len, struct mtd_info *mtd)
+{
+	struct fwu_mdata mdata;
+	int i, l, ret;
+
+	ret = fwu_get_mdata(&mdata);
+	if (ret < 0) {
+		log_debug("Failed to get the FWU mdata.\n");
+		return ret;
+	}
+
+	for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) {
+		ret = gen_image_alt_info(buf, len, i * CONFIG_FWU_NUM_BANKS,
+					 &mdata.img_entry[i], mtd);
+		if (ret)
+			break;
+		l = strlen(buf);
+		/* Replace the last ';' with '&' if there is another image. */
+		if (i != CONFIG_FWU_NUM_IMAGES_PER_BANK - 1 && l)
+			buf[l - 1] = '&';
+		len -= l;
+		buf += l;
+	}
+
+	return ret;
+}
-- 
2.34.1

Re: [PATCHv3 2/5] FWU: mtd: Add helper functions for accessing FWU metadata

[Sughosh Ganu]

On Mon, 9 Jan 2023 at 06:36, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> From: Sughosh Ganu <sughosh.ganu@linaro.org>
>
> Add helper functions needed for accessing the FWU metadata which
> contains information on the updatable images.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---

This patch is authored by Masami Hiramatsu. Please attribute it to
him. I had moved the code from a platform specific location to under
lib/fwu_updates/. You can mention me as a co-developer on this patch.
Thanks.

-sughosh

[PATCHv3 3/5] dt: fwu: developerbox: enable fwu banks and mdata regions

[Jassi Brar]

Specify Bank-0/1 and fwu metadata mtd regions.

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 .../synquacer-sc2a11-developerbox-u-boot.dtsi | 22 ++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi b/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
index 7a56116d6f..62eee0aaf0 100644
--- a/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
+++ b/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
@@ -23,7 +23,7 @@
 		active_clk_edges;
 		chipselect_num = <1>;
 
-		spi-flash@0 {
+		spi_flash: spi-flash@0 {
 			#address-cells = <1>;
 			#size-cells = <1>;
 			compatible = "jedec,spi-nor";
@@ -36,6 +36,7 @@
 				compatible = "fixed-partitions";
 				#address-cells = <1>;
 				#size-cells = <1>;
+				uuid = "17e86d77-41f9-4fd7-87ec-a55df9842de5";
 
 				partition@0 {
 					label = "BootStrap-BL1";
@@ -79,6 +80,19 @@
 					label = "Ex-OPTEE";
 					reg = <0x500000 0x200000>;
 				};
+
+				/* FWU Multi bank update partitions */
+				partition@600000 {
+					label = "FIP-Bank0";
+					reg = <0x600000 0x400000>;
+					uuid = "5a66a702-99fd-4fef-a392-c26e261a2828";
+				};
+
+				partition@a00000 {
+					label = "FIP-Bank1";
+					reg = <0xa00000 0x400000>;
+					uuid = "a8f868a1-6e5c-4757-878d-ce63375ef2c0";
+				};
 			};
 		};
 	};
@@ -104,6 +118,12 @@
 		optee {
 			status = "okay";
 		};
+
+		fwu-mdata {
+			compatible = "u-boot,fwu-mdata-mtd";
+			fwu-mdata-store = <&spi_flash>;
+			mdata-offsets = <0x500000 0x530000>;
+		};
 	};
 };
 
-- 
2.34.1

Re: [PATCHv3 3/5] dt: fwu: developerbox: enable fwu banks and mdata regions

[Michal Simek]

On 1/9/23 02:07, Jassi Brar wrote:
> Specify Bank-0/1 and fwu metadata mtd regions.
> 
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   .../synquacer-sc2a11-developerbox-u-boot.dtsi | 22 ++++++++++++++++++-
>   1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi b/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
> index 7a56116d6f..62eee0aaf0 100644
> --- a/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
> +++ b/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
> @@ -23,7 +23,7 @@
>   		active_clk_edges;
>   		chipselect_num = <1>;
>   
> -		spi-flash@0 {
> +		spi_flash: spi-flash@0 {
>   			#address-cells = <1>;
>   			#size-cells = <1>;
>   			compatible = "jedec,spi-nor";
> @@ -36,6 +36,7 @@
>   				compatible = "fixed-partitions";
>   				#address-cells = <1>;
>   				#size-cells = <1>;
> +				uuid = "17e86d77-41f9-4fd7-87ec-a55df9842de5";

I looked at dt-schema and also to linus tree and linux-next and I can't see any 
record for uuid in bindings.
That's why this is something what it is not documented that's why you shouldn't 
really use it.

>   
>   				partition@0 {
>   					label = "BootStrap-BL1";
> @@ -79,6 +80,19 @@
>   					label = "Ex-OPTEE";
>   					reg = <0x500000 0x200000>;
>   				};
> +
> +				/* FWU Multi bank update partitions */
> +				partition@600000 {
> +					label = "FIP-Bank0";
> +					reg = <0x600000 0x400000>;
> +					uuid = "5a66a702-99fd-4fef-a392-c26e261a2828";
> +				};
> +
> +				partition@a00000 {
> +					label = "FIP-Bank1";
> +					reg = <0xa00000 0x400000>;
> +					uuid = "a8f868a1-6e5c-4757-878d-ce63375ef2c0";
> +				};
>   			};
>   		};
>   	};
> @@ -104,6 +118,12 @@
>   		optee {
>   			status = "okay";
>   		};
> +
> +		fwu-mdata {
> +			compatible = "u-boot,fwu-mdata-mtd";
> +			fwu-mdata-store = <&spi_flash>;
> +			mdata-offsets = <0x500000 0x530000>;
> +		};

SR IR 2.0 has to pass binding check and I can't see any acked binding for it.
It means please get this approve first.

Thanks,
Michal

[PATCHv3 4/5] fwu: DeveloperBox: add support for FWU

[Jassi Brar]

Add code to support FWU_MULTI_BANK_UPDATE.
The platform does not have gpt-partition storage for
Banks and MetaData, rather it used SPI-NOR backed
mtd regions for the purpose.

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 board/socionext/developerbox/Makefile       |  1 +
 board/socionext/developerbox/developerbox.c |  8 ++
 board/socionext/developerbox/fwu_plat.c     | 57 ++++++++++++
 configs/synquacer_developerbox_defconfig    | 13 ++-
 doc/board/socionext/developerbox.rst        | 96 +++++++++++++++++++++
 include/configs/synquacer.h                 | 10 +++
 6 files changed, 183 insertions(+), 2 deletions(-)
 create mode 100644 board/socionext/developerbox/fwu_plat.c

diff --git a/board/socionext/developerbox/Makefile b/board/socionext/developerbox/Makefile
index 4a46de995a..9b80ee38e7 100644
--- a/board/socionext/developerbox/Makefile
+++ b/board/socionext/developerbox/Makefile
@@ -7,3 +7,4 @@
 #
 
 obj-y	:= developerbox.o
+obj-$(CONFIG_FWU_MULTI_BANK_UPDATE) += fwu_plat.o
diff --git a/board/socionext/developerbox/developerbox.c b/board/socionext/developerbox/developerbox.c
index 6415c90c1c..2123914f21 100644
--- a/board/socionext/developerbox/developerbox.c
+++ b/board/socionext/developerbox/developerbox.c
@@ -20,6 +20,13 @@
 
 #if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)
 struct efi_fw_image fw_images[] = {
+#if defined(CONFIG_FWU_MULTI_BANK_UPDATE)
+	{
+		.image_type_id = DEVELOPERBOX_FIP_IMAGE_GUID,
+		.fw_name = u"DEVELOPERBOX-FIP",
+		.image_index = 1,
+	},
+#else
 	{
 		.image_type_id = DEVELOPERBOX_UBOOT_IMAGE_GUID,
 		.fw_name = u"DEVELOPERBOX-UBOOT",
@@ -35,6 +42,7 @@ struct efi_fw_image fw_images[] = {
 		.fw_name = u"DEVELOPERBOX-OPTEE",
 		.image_index = 3,
 	},
+#endif
 };
 
 struct efi_capsule_update_info update_info = {
diff --git a/board/socionext/developerbox/fwu_plat.c b/board/socionext/developerbox/fwu_plat.c
new file mode 100644
index 0000000000..29b258f86c
--- /dev/null
+++ b/board/socionext/developerbox/fwu_plat.c
@@ -0,0 +1,57 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023, Linaro Limited
+ */
+
+#include <efi_loader.h>
+#include <fwu.h>
+#include <fwu_mdata.h>
+#include <memalign.h>
+#include <mtd.h>
+
+#define DFU_ALT_BUF_LEN 256
+
+/* Generate dfu_alt_info from partitions */
+void set_dfu_alt_info(char *interface, char *devstr)
+{
+	int ret;
+	struct mtd_info *mtd;
+
+	ALLOC_CACHE_ALIGN_BUFFER(char, buf, DFU_ALT_BUF_LEN);
+	memset(buf, 0, sizeof(buf));
+
+	mtd_probe_devices();
+
+	mtd = get_mtd_device_nm("nor1");
+	if (IS_ERR_OR_NULL(mtd))
+		return;
+
+	ret = fwu_gen_alt_info_from_mtd(buf, DFU_ALT_BUF_LEN, mtd);
+	if (ret < 0) {
+		log_err("Error: Failed to generate dfu_alt_info. (%d)\n", ret);
+		return;
+	}
+	log_debug("Make dfu_alt_info: '%s'\n", buf);
+
+	env_set("dfu_alt_info", buf);
+}
+
+int fwu_plat_get_alt_num(struct udevice __always_unused *dev,
+			 efi_guid_t *image_id, u8 *alt_num)
+{
+	return fwu_mtd_get_alt_num(image_id, alt_num, "nor1");
+}
+
+void fwu_plat_get_bootidx(uint *boot_idx)
+{
+	int ret;
+	u32 active_idx;
+	u32 *bootidx = boot_idx;
+
+	ret = fwu_get_active_index(&active_idx);
+
+	if (ret < 0)
+		*bootidx = -1;
+
+	*bootidx = active_idx;
+}
diff --git a/configs/synquacer_developerbox_defconfig b/configs/synquacer_developerbox_defconfig
index f69b873a36..b1085a388e 100644
--- a/configs/synquacer_developerbox_defconfig
+++ b/configs/synquacer_developerbox_defconfig
@@ -1,10 +1,11 @@
 CONFIG_ARM=y
 CONFIG_ARCH_SYNQUACER=y
-CONFIG_TEXT_BASE=0x08200000
+CONFIG_POSITION_INDEPENDENT=y
+CONFIG_SYS_TEXT_BASE=0
 CONFIG_SYS_MALLOC_LEN=0x1000000
 CONFIG_SYS_MALLOC_F_LEN=0x400
 CONFIG_ENV_SIZE=0x30000
-CONFIG_ENV_OFFSET=0x300000
+CONFIG_ENV_OFFSET=0x580000
 CONFIG_ENV_SECT_SIZE=0x10000
 CONFIG_DM_GPIO=y
 CONFIG_DEFAULT_DEVICE_TREE="synquacer-sc2a11-developerbox"
@@ -96,3 +97,11 @@ CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
 CONFIG_EFI_CAPSULE_ON_DISK=y
 CONFIG_EFI_IGNORE_OSINDICATIONS=y
 CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
+CONFIG_EFI_SECURE_BOOT=y
+CONFIG_FWU_MULTI_BANK_UPDATE=y
+CONFIG_FWU_MDATA=y
+CONFIG_FWU_MDATA_MTD=y
+CONFIG_FWU_NUM_BANKS=2
+CONFIG_FWU_NUM_IMAGES_PER_BANK=1
+CONFIG_CMD_FWU_METADATA=y
+CONFIG_TOOLS_MKFWUMDATA=y
diff --git a/doc/board/socionext/developerbox.rst b/doc/board/socionext/developerbox.rst
index 2d943c23be..be872aa79d 100644
--- a/doc/board/socionext/developerbox.rst
+++ b/doc/board/socionext/developerbox.rst
@@ -85,3 +85,99 @@ Once the flasher tool is running we are ready flash the UEFI image::
 
 After transferring the SPI_NOR_UBOOT.fd, turn off the DSW2-7 and reset the board.
 
+
+Enable FWU Multi Bank Update
+============================
+
+DeveloperBox supports the FWU Multi Bank Update. You *MUST* update both *SCP firmware* and *TF-A* for this feature. This will change the layout and the boot process but you can switch back to the normal one by changing the DSW 1-4 off.
+
+Configure U-Boot
+----------------
+
+To enable the FWU Multi Bank Update on the DeveloperBox, you need to add following configurations to configs/synquacer_developerbox_defconfig ::
+
+ CONFIG_FWU_MULTI_BANK_UPDATE=y
+ CONFIG_FWU_MDATA=y
+ CONFIG_FWU_MDATA_MTD=y
+ CONFIG_FWU_NUM_BANKS=2
+ CONFIG_FWU_NUM_IMAGES_PER_BANK=1
+ CONFIG_CMD_FWU_METADATA=y
+
+And build it::
+
+  cd u-boot/
+  export ARCH=arm64
+  export CROSS_COMPILE=aarch64-linux-gnu-
+  make synqucer_developerbox_defconfig
+  make -j `noproc`
+  cd ../
+
+By default, the CONFIG_FWU_NUM_BANKS and COFNIG_FWU_NUM_IMAGES_PER_BANKS are set to 2 and 1 respectively. This uses FIP (Firmware Image Package) type image which contains TF-A, U-Boot and OP-TEE (the OP-TEE is optional.)
+You can use fiptool to compose the FIP image from those firmware images.
+
+Rebuild SCP firmware
+--------------------
+
+Rebuild SCP firmware which supports FWU Multi Bank Update as below::
+
+  cd SCP-firmware/
+  OUT=./build/product/synquacer
+  ROMFW_FILE=$OUT/scp_romfw/$SCP_BUILD_MODE/bin/scp_romfw.bin
+  RAMFW_FILE=$OUT/scp_ramfw/$SCP_BUILD_MODE/bin/scp_ramfw.bin
+  ROMRAMFW_FILE=scp_romramfw_release.bin
+
+  make CC=$ARM_EMB_GCC PRODUCT=synquacer MODE=release
+  tr "\000" "\377" < /dev/zero | dd of=${ROMRAMFW_FILE} bs=1 count=196608
+  dd if=${ROMFW_FILE} of=${ROMRAMFW_FILE} bs=1 conv=notrunc seek=0
+  dd if=${RAMFW_FILE} of=${ROMRAMFW_FILE} bs=1 seek=65536
+  cd ../
+
+And you can get the `scp_romramfw_release.bin` file
+
+Rebuild TF-A and FIP
+--------------------
+
+Rebuild TF-A which supports FWU Multi Bank Update as below::
+
+  cd arm-trusted-firmware/
+  make CROSS_COMPILE=aarch64-linux-gnu- -j`nproc` PLAT=synquacer \
+     SPD=opteed SQ_RESET_TO_BL2=1 GENERATE_COT=1 MBEDTLS_DIR=../mbedtls \
+     BL33=../u-boot/u-boot.bin all fip fiptool
+
+And make a FIP image.::
+
+  cp build/synquacer/release/fip.bin SPI_NOR_NEWFIP.fd
+  tools/fiptool/fiptool update --tb-fw build/synquacer/release/bl2.bin SPI_NOR_NEWFIP.fd
+
+
+UUIDs for the FWU Multi Bank Update
+-----------------------------------
+
+FWU multi-bank update requires some UUIDs. The DeveloperBox platform uses following UUIDs.
+
+ - Location UUID for the FIP image: 17e86d77-41f9-4fd7-87ec-a55df9842de5
+ - Image type UUID for the FIP image: 10c36d7d-ca52-b843-b7b9-f9d6c501d108
+ - Image UUID for Bank0 : 5a66a702-99fd-4fef-a392-c26e261a2828
+ - Image UUID for Bank1 : a8f868a1-6e5c-4757-878d-ce63375ef2c0
+
+These UUIDs are used for making a FWU metadata image.
+
+Install via flash writer
+------------------------
+
+As explained in above section, the new FIP image and the FWU metadata image can be installed via NOR flash writer. Note that the installation offsets for the FWU multi bank update supported firmware.
+
+Once the flasher tool is running we are ready flash the images.::
+Write the FIP image to the 0x600000 offset.::
+
+  flash rawwrite 600000 180000
+  >> Send SPI_NOR_NEWFIP.fd via XMODEM (Control-A S in minicom) <<
+
+And write the new SCP firmware.::
+
+  flash write cm3
+  >> Send scp_romramfw_release.bin via XMODEM (Control-A S in minicom) <<
+
+At last, turn on the DSW 3-4 on the board, and reboot.
+Note that if DSW 3-4 is turned off, the DeveloperBox will boot from
+the original EDK2 firmware (or non-FWU U-Boot if you already installed.)
diff --git a/include/configs/synquacer.h b/include/configs/synquacer.h
index 63d897d090..c798a23bed 100644
--- a/include/configs/synquacer.h
+++ b/include/configs/synquacer.h
@@ -41,19 +41,29 @@
 
 /* Since U-Boot 64bit PCIe support is limited, disable 64bit MMIO support */
 
+#ifdef CONFIG_FWU_MULTI_BANK_UPDATE
+#define DEFAULT_DFU_ALT_INFO
+#else
 #define DEFAULT_DFU_ALT_INFO "dfu_alt_info="				\
 			"mtd nor1=u-boot.bin raw 200000 100000;"	\
 			"fip.bin raw 180000 78000;"			\
 			"optee.bin raw 500000 100000\0"
+#endif
 
 /* GUIDs for capsule updatable firmware images */
 #define DEVELOPERBOX_UBOOT_IMAGE_GUID \
 	EFI_GUID(0x53a92e83, 0x4ef4, 0x473a, 0x8b, 0x0d, \
 		 0xb5, 0xd8, 0xc7, 0xb2, 0xd6, 0x00)
 
+#ifdef CONFIG_FWU_MULTI_BANK_UPDATE
+#define DEVELOPERBOX_FIP_IMAGE_GUID \
+	EFI_GUID(0x7d6dc310, 0x52ca, 0x43b8, 0xb7, 0xb9, \
+		 0xf9, 0xd6, 0xc5, 0x01, 0xd1, 0x08)
+#else
 #define DEVELOPERBOX_FIP_IMAGE_GUID \
 	EFI_GUID(0x880866e9, 0x84ba, 0x4793, 0xa9, 0x08, \
 		 0x33, 0xe0, 0xb9, 0x16, 0xf3, 0x98)
+#endif
 
 #define DEVELOPERBOX_OPTEE_IMAGE_GUID \
 	EFI_GUID(0xc1b629f1, 0xce0e, 0x4894, 0x82, 0xbf, \
-- 
2.34.1

Re: [PATCHv3 4/5] fwu: DeveloperBox: add support for FWU

[Michal Simek]

On 1/9/23 02:07, Jassi Brar wrote:
> Add code to support FWU_MULTI_BANK_UPDATE.
> The platform does not have gpt-partition storage for
> Banks and MetaData, rather it used SPI-NOR backed
> mtd regions for the purpose.
> 
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>   board/socionext/developerbox/Makefile       |  1 +
>   board/socionext/developerbox/developerbox.c |  8 ++
>   board/socionext/developerbox/fwu_plat.c     | 57 ++++++++++++
>   configs/synquacer_developerbox_defconfig    | 13 ++-
>   doc/board/socionext/developerbox.rst        | 96 +++++++++++++++++++++
>   include/configs/synquacer.h                 | 10 +++
>   6 files changed, 183 insertions(+), 2 deletions(-)
>   create mode 100644 board/socionext/developerbox/fwu_plat.c
> 
> diff --git a/board/socionext/developerbox/Makefile b/board/socionext/developerbox/Makefile
> index 4a46de995a..9b80ee38e7 100644
> --- a/board/socionext/developerbox/Makefile
> +++ b/board/socionext/developerbox/Makefile
> @@ -7,3 +7,4 @@
>   #
>   
>   obj-y	:= developerbox.o
> +obj-$(CONFIG_FWU_MULTI_BANK_UPDATE) += fwu_plat.o
> diff --git a/board/socionext/developerbox/developerbox.c b/board/socionext/developerbox/developerbox.c
> index 6415c90c1c..2123914f21 100644
> --- a/board/socionext/developerbox/developerbox.c
> +++ b/board/socionext/developerbox/developerbox.c
> @@ -20,6 +20,13 @@
>   
>   #if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)
>   struct efi_fw_image fw_images[] = {
> +#if defined(CONFIG_FWU_MULTI_BANK_UPDATE)
> +	{
> +		.image_type_id = DEVELOPERBOX_FIP_IMAGE_GUID,
> +		.fw_name = u"DEVELOPERBOX-FIP",
> +		.image_index = 1,
> +	},
> +#else
>   	{
>   		.image_type_id = DEVELOPERBOX_UBOOT_IMAGE_GUID,
>   		.fw_name = u"DEVELOPERBOX-UBOOT",
> @@ -35,6 +42,7 @@ struct efi_fw_image fw_images[] = {
>   		.fw_name = u"DEVELOPERBOX-OPTEE",
>   		.image_index = 3,
>   	},
> +#endif
>   };
>   
>   struct efi_capsule_update_info update_info = {
> diff --git a/board/socionext/developerbox/fwu_plat.c b/board/socionext/developerbox/fwu_plat.c
> new file mode 100644
> index 0000000000..29b258f86c
> --- /dev/null
> +++ b/board/socionext/developerbox/fwu_plat.c
> @@ -0,0 +1,57 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2023, Linaro Limited
> + */
> +
> +#include <efi_loader.h>
> +#include <fwu.h>
> +#include <fwu_mdata.h>
> +#include <memalign.h>
> +#include <mtd.h>
> +
> +#define DFU_ALT_BUF_LEN 256
> +
> +/* Generate dfu_alt_info from partitions */
> +void set_dfu_alt_info(char *interface, char *devstr)
> +{
> +	int ret;
> +	struct mtd_info *mtd;
> +
> +	ALLOC_CACHE_ALIGN_BUFFER(char, buf, DFU_ALT_BUF_LEN);
> +	memset(buf, 0, sizeof(buf));
> +
> +	mtd_probe_devices();


When you disable CONFIG_MTD for this board this code will fail.
You need to cover this dependency via different Kconfig symbol or somewhere else.
Maybe this file should be compiled only when CONFIG_FWU_MDATA_MTD is present.

> +
> +	mtd = get_mtd_device_nm("nor1");
> +	if (IS_ERR_OR_NULL(mtd))
> +		return;
> +
> +	ret = fwu_gen_alt_info_from_mtd(buf, DFU_ALT_BUF_LEN, mtd);
> +	if (ret < 0) {
> +		log_err("Error: Failed to generate dfu_alt_info. (%d)\n", ret);
> +		return;
> +	}
> +	log_debug("Make dfu_alt_info: '%s'\n", buf);
> +
> +	env_set("dfu_alt_info", buf);
> +}
> +
> +int fwu_plat_get_alt_num(struct udevice __always_unused *dev,
> +			 efi_guid_t *image_id, u8 *alt_num)
> +{
> +	return fwu_mtd_get_alt_num(image_id, alt_num, "nor1");
> +}
> +
> +void fwu_plat_get_bootidx(uint *boot_idx)
> +{
> +	int ret;
> +	u32 active_idx;
> +	u32 *bootidx = boot_idx;
> +
> +	ret = fwu_get_active_index(&active_idx);
> +
> +	if (ret < 0)
> +		*bootidx = -1;
> +
> +	*bootidx = active_idx;
> +}
> diff --git a/configs/synquacer_developerbox_defconfig b/configs/synquacer_developerbox_defconfig
> index f69b873a36..b1085a388e 100644
> --- a/configs/synquacer_developerbox_defconfig
> +++ b/configs/synquacer_developerbox_defconfig
> @@ -1,10 +1,11 @@
>   CONFIG_ARM=y
>   CONFIG_ARCH_SYNQUACER=y
> -CONFIG_TEXT_BASE=0x08200000
> +CONFIG_POSITION_INDEPENDENT=y
> +CONFIG_SYS_TEXT_BASE=0

How is this related to subject? I can't see any single line of description why 
you are doing this change.

>   CONFIG_SYS_MALLOC_LEN=0x1000000
>   CONFIG_SYS_MALLOC_F_LEN=0x400
>   CONFIG_ENV_SIZE=0x30000
> -CONFIG_ENV_OFFSET=0x300000
> +CONFIG_ENV_OFFSET=0x580000

Why are you changing this offset?

>   CONFIG_ENV_SECT_SIZE=0x10000
>   CONFIG_DM_GPIO=y
>   CONFIG_DEFAULT_DEVICE_TREE="synquacer-sc2a11-developerbox"
> @@ -96,3 +97,11 @@ CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
>   CONFIG_EFI_CAPSULE_ON_DISK=y
>   CONFIG_EFI_IGNORE_OSINDICATIONS=y
>   CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> +CONFIG_EFI_SECURE_BOOT=y
> +CONFIG_FWU_MULTI_BANK_UPDATE=y
> +CONFIG_FWU_MDATA=y
> +CONFIG_FWU_MDATA_MTD=y
> +CONFIG_FWU_NUM_BANKS=2
> +CONFIG_FWU_NUM_IMAGES_PER_BANK=1
> +CONFIG_CMD_FWU_METADATA=y
> +CONFIG_TOOLS_MKFWUMDATA=y
> diff --git a/doc/board/socionext/developerbox.rst b/doc/board/socionext/developerbox.rst
> index 2d943c23be..be872aa79d 100644
> --- a/doc/board/socionext/developerbox.rst
> +++ b/doc/board/socionext/developerbox.rst
> @@ -85,3 +85,99 @@ Once the flasher tool is running we are ready flash the UEFI image::
>   
>   After transferring the SPI_NOR_UBOOT.fd, turn off the DSW2-7 and reset the board.
>   
> +
> +Enable FWU Multi Bank Update
> +============================
> +
> +DeveloperBox supports the FWU Multi Bank Update. You *MUST* update both *SCP firmware* and *TF-A* for this feature. This will change the layout and the boot process but you can switch back to the normal one by changing the DSW 1-4 off.

Isn't here also certain limit for number of chars on the line?

> +
> +Configure U-Boot
> +----------------
> +
> +To enable the FWU Multi Bank Update on the DeveloperBox, you need to add following configurations to configs/synquacer_developerbox_defconfig ::
> +
> + CONFIG_FWU_MULTI_BANK_UPDATE=y
> + CONFIG_FWU_MDATA=y
> + CONFIG_FWU_MDATA_MTD=y
> + CONFIG_FWU_NUM_BANKS=2
> + CONFIG_FWU_NUM_IMAGES_PER_BANK=1
> + CONFIG_CMD_FWU_METADATA=y
> +
> +And build it::
> +
> +  cd u-boot/
> +  export ARCH=arm64
> +  export CROSS_COMPILE=aarch64-linux-gnu-
> +  make synqucer_developerbox_defconfig
> +  make -j `noproc`
> +  cd ../
> +
> +By default, the CONFIG_FWU_NUM_BANKS and COFNIG_FWU_NUM_IMAGES_PER_BANKS are set to 2 and 1 respectively. This uses FIP (Firmware Image Package) type image which contains TF-A, U-Boot and OP-TEE (the OP-TEE is optional.)

long line and type CONFIG


I would also prefer to describe the logic used on TF-A side. I saw that you are 
using one register to find out how to boot it.

commit a19382521c583b3dde89df14678b011960097f6c
Author:     Jassi Brar <jaswinder.singh@linaro.org>
AuthorDate: Mon May 23 13:16:01 2022 -0500
Commit:     Jassi Brar <jaswinder.singh@linaro.org>
CommitDate: Mon Jun 27 13:12:24 2022 -0500

     feat(synquacer): add FWU Multi Bank Update support

     Add FWU Multi Bank Update support. This reads the platform metadata
     and update the FIP base address so that BL2 can load correct BL3X
     based on the boot index.

     Cc: Sumit Garg <sumit.garg@linaro.org>
     Cc: Masahisa Kojima <masahisa.kojima@linaro.org>
     Cc: Manish V Badarkhe <manish.badarkhe@arm.com>
     Cc: Leonardo Sandoval <leonardo.sandoval@linaro.org>
     Change-Id: I5d96972bc4b3b9a12a8157117e53a05da5ce89f6
     Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
     Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>


but it is not obviously the code which is decoding mdata structure.
It means boot flow description would be good to have.



> +You can use fiptool to compose the FIP image from those firmware images.
> +
> +Rebuild SCP firmware
> +--------------------
> +
> +Rebuild SCP firmware which supports FWU Multi Bank Update as below::
> +
> +  cd SCP-firmware/
> +  OUT=./build/product/synquacer
> +  ROMFW_FILE=$OUT/scp_romfw/$SCP_BUILD_MODE/bin/scp_romfw.bin
> +  RAMFW_FILE=$OUT/scp_ramfw/$SCP_BUILD_MODE/bin/scp_ramfw.bin
> +  ROMRAMFW_FILE=scp_romramfw_release.bin
> +
> +  make CC=$ARM_EMB_GCC PRODUCT=synquacer MODE=release
> +  tr "\000" "\377" < /dev/zero | dd of=${ROMRAMFW_FILE} bs=1 count=196608
> +  dd if=${ROMFW_FILE} of=${ROMRAMFW_FILE} bs=1 conv=notrunc seek=0
> +  dd if=${RAMFW_FILE} of=${ROMRAMFW_FILE} bs=1 seek=65536
> +  cd ../
> +
> +And you can get the `scp_romramfw_release.bin` file
> +
> +Rebuild TF-A and FIP
> +--------------------
> +
> +Rebuild TF-A which supports FWU Multi Bank Update as below::
> +
> +  cd arm-trusted-firmware/
> +  make CROSS_COMPILE=aarch64-linux-gnu- -j`nproc` PLAT=synquacer \
> +     SPD=opteed SQ_RESET_TO_BL2=1 GENERATE_COT=1 MBEDTLS_DIR=../mbedtls \
> +     BL33=../u-boot/u-boot.bin all fip fiptool
> +
> +And make a FIP image.::
> +
> +  cp build/synquacer/release/fip.bin SPI_NOR_NEWFIP.fd
> +  tools/fiptool/fiptool update --tb-fw build/synquacer/release/bl2.bin SPI_NOR_NEWFIP.fd
> +
> +
> +UUIDs for the FWU Multi Bank Update
> +-----------------------------------
> +
> +FWU multi-bank update requires some UUIDs. The DeveloperBox platform uses following UUIDs.
> +
> + - Location UUID for the FIP image: 17e86d77-41f9-4fd7-87ec-a55df9842de5
> + - Image type UUID for the FIP image: 10c36d7d-ca52-b843-b7b9-f9d6c501d108
> + - Image UUID for Bank0 : 5a66a702-99fd-4fef-a392-c26e261a2828
> + - Image UUID for Bank1 : a8f868a1-6e5c-4757-878d-ce63375ef2c0
> +
> +These UUIDs are used for making a FWU metadata image.
> +
> +Install via flash writer
> +------------------------
> +
> +As explained in above section, the new FIP image and the FWU metadata image can be installed via NOR flash writer. Note that the installation offsets for the FWU multi bank update supported firmware.
> +
> +Once the flasher tool is running we are ready flash the images.::
> +Write the FIP image to the 0x600000 offset.::
> +
> +  flash rawwrite 600000 180000
> +  >> Send SPI_NOR_NEWFIP.fd via XMODEM (Control-A S in minicom) <<
> +
> +And write the new SCP firmware.::
> +
> +  flash write cm3
> +  >> Send scp_romramfw_release.bin via XMODEM (Control-A S in minicom) <<
> +
> +At last, turn on the DSW 3-4 on the board, and reboot.
> +Note that if DSW 3-4 is turned off, the DeveloperBox will boot from
> +the original EDK2 firmware (or non-FWU U-Boot if you already installed.)
> diff --git a/include/configs/synquacer.h b/include/configs/synquacer.h
> index 63d897d090..c798a23bed 100644
> --- a/include/configs/synquacer.h
> +++ b/include/configs/synquacer.h
> @@ -41,19 +41,29 @@
>   
>   /* Since U-Boot 64bit PCIe support is limited, disable 64bit MMIO support */
>   
> +#ifdef CONFIG_FWU_MULTI_BANK_UPDATE
> +#define DEFAULT_DFU_ALT_INFO
> +#else
>   #define DEFAULT_DFU_ALT_INFO "dfu_alt_info="				\
>   			"mtd nor1=u-boot.bin raw 200000 100000;"	\
>   			"fip.bin raw 180000 78000;"			\
>   			"optee.bin raw 500000 100000\0"
> +#endif
>   
>   /* GUIDs for capsule updatable firmware images */
>   #define DEVELOPERBOX_UBOOT_IMAGE_GUID \
>   	EFI_GUID(0x53a92e83, 0x4ef4, 0x473a, 0x8b, 0x0d, \
>   		 0xb5, 0xd8, 0xc7, 0xb2, 0xd6, 0x00)
>   
> +#ifdef CONFIG_FWU_MULTI_BANK_UPDATE
> +#define DEVELOPERBOX_FIP_IMAGE_GUID \
> +	EFI_GUID(0x7d6dc310, 0x52ca, 0x43b8, 0xb7, 0xb9, \
> +		 0xf9, 0xd6, 0xc5, 0x01, 0xd1, 0x08)
> +#else
>   #define DEVELOPERBOX_FIP_IMAGE_GUID \
>   	EFI_GUID(0x880866e9, 0x84ba, 0x4793, 0xa9, 0x08, \
>   		 0x33, 0xe0, 0xb9, 0x16, 0xf3, 0x98)
> +#endif
>   
>   #define DEVELOPERBOX_OPTEE_IMAGE_GUID \
>   	EFI_GUID(0xc1b629f1, 0xce0e, 0x4894, 0x82, 0xbf, \

Thanks,
Michal

Re: [PATCHv3 4/5] fwu: DeveloperBox: add support for FWU

[Jassi Brar]

On Wed, Jan 18, 2023 at 8:46 AM Michal Simek <michal.simek@amd.com> wrote:
>
>
>
> On 1/9/23 02:07, Jassi Brar wrote:
> > Add code to support FWU_MULTI_BANK_UPDATE.
> > The platform does not have gpt-partition storage for
> > Banks and MetaData, rather it used SPI-NOR backed
> > mtd regions for the purpose.
> >
> > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> > ---
> >   board/socionext/developerbox/Makefile       |  1 +
> >   board/socionext/developerbox/developerbox.c |  8 ++
> >   board/socionext/developerbox/fwu_plat.c     | 57 ++++++++++++
> >   configs/synquacer_developerbox_defconfig    | 13 ++-
> >   doc/board/socionext/developerbox.rst        | 96 +++++++++++++++++++++
> >   include/configs/synquacer.h                 | 10 +++
> >   6 files changed, 183 insertions(+), 2 deletions(-)
> >   create mode 100644 board/socionext/developerbox/fwu_plat.c
> >
> > diff --git a/board/socionext/developerbox/Makefile b/board/socionext/developerbox/Makefile
> > index 4a46de995a..9b80ee38e7 100644
> > --- a/board/socionext/developerbox/Makefile
> > +++ b/board/socionext/developerbox/Makefile
> > @@ -7,3 +7,4 @@
> >   #
> >
> >   obj-y       := developerbox.o
> > +obj-$(CONFIG_FWU_MULTI_BANK_UPDATE) += fwu_plat.o
> > diff --git a/board/socionext/developerbox/developerbox.c b/board/socionext/developerbox/developerbox.c
> > index 6415c90c1c..2123914f21 100644
> > --- a/board/socionext/developerbox/developerbox.c
> > +++ b/board/socionext/developerbox/developerbox.c
> > @@ -20,6 +20,13 @@
> >
> >   #if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)
> >   struct efi_fw_image fw_images[] = {
> > +#if defined(CONFIG_FWU_MULTI_BANK_UPDATE)
> > +     {
> > +             .image_type_id = DEVELOPERBOX_FIP_IMAGE_GUID,
> > +             .fw_name = u"DEVELOPERBOX-FIP",
> > +             .image_index = 1,
> > +     },
> > +#else
> >       {
> >               .image_type_id = DEVELOPERBOX_UBOOT_IMAGE_GUID,
> >               .fw_name = u"DEVELOPERBOX-UBOOT",
> > @@ -35,6 +42,7 @@ struct efi_fw_image fw_images[] = {
> >               .fw_name = u"DEVELOPERBOX-OPTEE",
> >               .image_index = 3,
> >       },
> > +#endif
> >   };
> >
> >   struct efi_capsule_update_info update_info = {
> > diff --git a/board/socionext/developerbox/fwu_plat.c b/board/socionext/developerbox/fwu_plat.c
> > new file mode 100644
> > index 0000000000..29b258f86c
> > --- /dev/null
> > +++ b/board/socionext/developerbox/fwu_plat.c
> > @@ -0,0 +1,57 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2023, Linaro Limited
> > + */
> > +
> > +#include <efi_loader.h>
> > +#include <fwu.h>
> > +#include <fwu_mdata.h>
> > +#include <memalign.h>
> > +#include <mtd.h>
> > +
> > +#define DFU_ALT_BUF_LEN 256
> > +
> > +/* Generate dfu_alt_info from partitions */
> > +void set_dfu_alt_info(char *interface, char *devstr)
> > +{
> > +     int ret;
> > +     struct mtd_info *mtd;
> > +
> > +     ALLOC_CACHE_ALIGN_BUFFER(char, buf, DFU_ALT_BUF_LEN);
> > +     memset(buf, 0, sizeof(buf));
> > +
> > +     mtd_probe_devices();
>
>
> When you disable CONFIG_MTD for this board this code will fail.
> You need to cover this dependency via different Kconfig symbol or somewhere else.
> Maybe this file should be compiled only when CONFIG_FWU_MDATA_MTD is present.
>
Yes, i agree.


> > +
> > +     mtd = get_mtd_device_nm("nor1");
> > +     if (IS_ERR_OR_NULL(mtd))
> > +             return;
> > +
> > +     ret = fwu_gen_alt_info_from_mtd(buf, DFU_ALT_BUF_LEN, mtd);
> > +     if (ret < 0) {
> > +             log_err("Error: Failed to generate dfu_alt_info. (%d)\n", ret);
> > +             return;
> > +     }
> > +     log_debug("Make dfu_alt_info: '%s'\n", buf);
> > +
> > +     env_set("dfu_alt_info", buf);
> > +}
> > +
> > +int fwu_plat_get_alt_num(struct udevice __always_unused *dev,
> > +                      efi_guid_t *image_id, u8 *alt_num)
> > +{
> > +     return fwu_mtd_get_alt_num(image_id, alt_num, "nor1");
> > +}
> > +
> > +void fwu_plat_get_bootidx(uint *boot_idx)
> > +{
> > +     int ret;
> > +     u32 active_idx;
> > +     u32 *bootidx = boot_idx;
> > +
> > +     ret = fwu_get_active_index(&active_idx);
> > +
> > +     if (ret < 0)
> > +             *bootidx = -1;
> > +
> > +     *bootidx = active_idx;
> > +}
> > diff --git a/configs/synquacer_developerbox_defconfig b/configs/synquacer_developerbox_defconfig
> > index f69b873a36..b1085a388e 100644
> > --- a/configs/synquacer_developerbox_defconfig
> > +++ b/configs/synquacer_developerbox_defconfig
> > @@ -1,10 +1,11 @@
> >   CONFIG_ARM=y
> >   CONFIG_ARCH_SYNQUACER=y
> > -CONFIG_TEXT_BASE=0x08200000
> > +CONFIG_POSITION_INDEPENDENT=y
> > +CONFIG_SYS_TEXT_BASE=0
>
> How is this related to subject? I can't see any single line of description why
> you are doing this change.
>
> >   CONFIG_SYS_MALLOC_LEN=0x1000000
> >   CONFIG_SYS_MALLOC_F_LEN=0x400
> >   CONFIG_ENV_SIZE=0x30000
> > -CONFIG_ENV_OFFSET=0x300000
> > +CONFIG_ENV_OFFSET=0x580000
>
> Why are you changing this offset?
>
Synquacer's boot flow is revamped with FWU support. So I think these
should probably come in a separate patch


> > diff --git a/doc/board/socionext/developerbox.rst b/doc/board/socionext/developerbox.rst
> > index 2d943c23be..be872aa79d 100644
> > --- a/doc/board/socionext/developerbox.rst
> > +++ b/doc/board/socionext/developerbox.rst
> > @@ -85,3 +85,99 @@ Once the flasher tool is running we are ready flash the UEFI image::
> >
> >   After transferring the SPI_NOR_UBOOT.fd, turn off the DSW2-7 and reset the board.
> >
> > +
> > +Enable FWU Multi Bank Update
> > +============================
> > +
> > +DeveloperBox supports the FWU Multi Bank Update. You *MUST* update both *SCP firmware* and *TF-A* for this feature. This will change the layout and the boot process but you can switch back to the normal one by changing the DSW 1-4 off.
>
> Isn't here also certain limit for number of chars on the line?
>
ok.


> > +
> > +By default, the CONFIG_FWU_NUM_BANKS and COFNIG_FWU_NUM_IMAGES_PER_BANKS are set to 2 and 1 respectively. This uses FIP (Firmware Image Package) type image which contains TF-A, U-Boot and OP-TEE (the OP-TEE is optional.)
>
> long line and type CONFIG
>
ok

>
> I would also prefer to describe the logic used on TF-A side. I saw that you are
> using one register to find out how to boot it.
>
That is not a register, but a location in NOR that contains flag to

> commit a19382521c583b3dde89df14678b011960097f6c
> Author:     Jassi Brar <jaswinder.singh@linaro.org>
> AuthorDate: Mon May 23 13:16:01 2022 -0500
> Commit:     Jassi Brar <jaswinder.singh@linaro.org>
> CommitDate: Mon Jun 27 13:12:24 2022 -0500
>
>      feat(synquacer): add FWU Multi Bank Update support
>
>      Add FWU Multi Bank Update support. This reads the platform metadata
>      and update the FIP base address so that BL2 can load correct BL3X
>      based on the boot index.
>
>      Cc: Sumit Garg <sumit.garg@linaro.org>
>      Cc: Masahisa Kojima <masahisa.kojima@linaro.org>
>      Cc: Manish V Badarkhe <manish.badarkhe@arm.com>
>      Cc: Leonardo Sandoval <leonardo.sandoval@linaro.org>
>      Change-Id: I5d96972bc4b3b9a12a8157117e53a05da5ce89f6
>      Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
>      Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>
>
> but it is not obviously the code which is decoding mdata structure.
> It means boot flow description would be good to have.
>
The 'platform metadata' is not the fwu-mdata. It is a choice of fip
(with different bootloader) to boot from. FWU A/B support is not yet
implemented.

thanks.

[PATCHv3 5/5] tools: Add mkfwumdata tool for FWU metadata image

[Jassi Brar]

From: Masami Hiramatsu <masami.hiramatsu@linaro.org>

Add 'mkfwumdata' tool to generate FWU metadata image for the meta-data
partition to be used in A/B Update imeplementation.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 tools/Kconfig      |   9 ++
 tools/Makefile     |   4 +
 tools/mkfwumdata.c | 326 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 339 insertions(+)
 create mode 100644 tools/mkfwumdata.c

diff --git a/tools/Kconfig b/tools/Kconfig
index 539708f277..6e23f44d55 100644
--- a/tools/Kconfig
+++ b/tools/Kconfig
@@ -157,4 +157,13 @@ config LUT_SEQUENCE
 	help
 	  Look Up Table Sequence
 
+config TOOLS_MKFWUMDATA
+	bool "Build mkfwumdata command"
+	default y if FWU_MULTI_BANK_UPDATE
+	help
+	  This command allows users to create a raw image of the FWU
+	  metadata for initial installation of the FWU multi bank
+	  update on the board. The installation method depends on
+	  the platform.
+
 endmenu
diff --git a/tools/Makefile b/tools/Makefile
index 26be0a7ba2..024d6c8eca 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -255,6 +255,10 @@ HOSTLDLIBS_mkeficapsule += \
 	$(shell pkg-config --libs uuid 2> /dev/null || echo "-luuid")
 hostprogs-$(CONFIG_TOOLS_MKEFICAPSULE) += mkeficapsule
 
+mkfwumdata-objs := mkfwumdata.o lib/crc32.o
+HOSTLDLIBS_mkfwumdata += -luuid
+hostprogs-$(CONFIG_TOOLS_MKFWUMDATA) += mkfwumdata
+
 # We build some files with extra pedantic flags to try to minimize things
 # that won't build on some weird host compiler -- though there are lots of
 # exceptions for files that aren't complaint.
diff --git a/tools/mkfwumdata.c b/tools/mkfwumdata.c
new file mode 100644
index 0000000000..e0b6702039
--- /dev/null
+++ b/tools/mkfwumdata.c
@@ -0,0 +1,326 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2023, Linaro Limited
+ */
+
+#include <errno.h>
+#include <getopt.h>
+#include <limits.h>
+#include <stdio.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <string.h>
+#include <u-boot/crc.h>
+#include <unistd.h>
+#include <uuid/uuid.h>
+
+/* This will dynamically allocate the fwu_mdata */
+#define CONFIG_FWU_NUM_BANKS		0
+#define CONFIG_FWU_NUM_IMAGES_PER_BANK	0
+
+/* Since we can not include fwu.h, redefine version here. */
+#define FWU_MDATA_VERSION		1
+
+typedef uint8_t u8;
+typedef int16_t s16;
+typedef uint16_t u16;
+typedef uint32_t u32;
+typedef uint64_t u64;
+
+#include <fwu_mdata.h>
+
+/* TODO: Endianess conversion may be required for some arch. */
+
+static const char *opts_short = "b:i:a:p:gh";
+
+static struct option options[] = {
+	{"banks", required_argument, NULL, 'b'},
+	{"images", required_argument, NULL, 'i'},
+	{"guid", required_argument, NULL, 'g'},
+	{"active-bank", required_argument, NULL, 'a'},
+	{"previous-bank", required_argument, NULL, 'p'},
+	{"help", no_argument, NULL, 'h'},
+	{NULL, 0, NULL, 0},
+};
+
+static void print_usage(void)
+{
+	fprintf(stderr, "Usage: mkfwumdata [options] <UUIDs list> <output file>\n");
+	fprintf(stderr, "Options:\n"
+		"\t-i, --images <num>          Number of images\n"
+		"\t-b, --banks  <num>          Number of banks\n"
+		"\t-a, --active-bank  <num>    Active bank\n"
+		"\t-p, --previous-bank  <num>  Previous active bank\n"
+		"\t-g, --guid                  Use GUID instead of UUID\n"
+		"\t-h, --help                  print a help message\n"
+		);
+	fprintf(stderr, "  UUIDs list syntax:\n"
+		"\t  <location uuid>,<image type uuid>,<images uuid list>\n"
+		"\t    images uuid list syntax:\n"
+		"\t\t    img_uuid_00,img_uuid_01...img_uuid_0b,\n"
+		"\t\t    img_uuid_10,img_uuid_11...img_uuid_1b,\n"
+		"\t\t    ...,\n"
+		"\t\t    img_uuid_i0,img_uuid_i1...img_uuid_ib,\n"
+		"\t\t    where 'b' and 'i' are number of banks and numbder of images in a bank respectively.\n"
+	       );
+}
+
+static int active_bank = 0;
+static int previous_bank = INT_MAX; /* unset */
+static bool __use_guid = false;
+
+struct fwu_mdata_object {
+	size_t images;
+	size_t banks;
+	size_t size;
+	struct fwu_mdata *mdata;
+};
+
+struct fwu_mdata_object *fwu_alloc_mdata(size_t images, size_t banks)
+{
+	struct fwu_mdata_object *mobj;
+
+	mobj = calloc(1, sizeof(*mobj));
+	if (!mobj)
+		return NULL;
+
+	mobj->size = sizeof(struct fwu_mdata) +
+		(sizeof(struct fwu_image_entry) +
+		 sizeof(struct fwu_image_bank_info) * banks) * images;
+	mobj->images = images;
+	mobj->banks = banks;
+
+	mobj->mdata = calloc(1, mobj->size);
+	if (!mobj->mdata) {
+		free(mobj);
+		return NULL;
+	}
+
+	return mobj;
+}
+
+struct fwu_image_entry *fwu_get_image(struct fwu_mdata_object *mobj, size_t idx)
+{
+	size_t offset;
+
+	offset = sizeof(struct fwu_mdata) +
+		(sizeof(struct fwu_image_entry) +
+		 sizeof(struct fwu_image_bank_info) * mobj->banks) * idx;
+
+	return (struct fwu_image_entry *)((char *)mobj->mdata + offset);
+}
+
+struct fwu_image_bank_info *fwu_get_bank(struct fwu_mdata_object *mobj,
+					 size_t img_idx, size_t bnk_idx)
+{
+	size_t offset;
+
+	offset = sizeof(struct fwu_mdata) +
+		(sizeof(struct fwu_image_entry) +
+		 sizeof(struct fwu_image_bank_info) * mobj->banks) * img_idx +
+		sizeof(struct fwu_image_entry) +
+		sizeof(struct fwu_image_bank_info) * bnk_idx;
+
+	return (struct fwu_image_bank_info *)((char *)mobj->mdata + offset);
+}
+
+/**
+ * convert_uuid_to_guid() - convert UUID to GUID
+ * @buf:	UUID binary
+ *
+ * UUID and GUID have the same data structure, but their binary
+ * formats are different due to the endianness. See lib/uuid.c.
+ * Since uuid_parse() can handle only UUID, this function must
+ * be called to get correct data for GUID when parsing a string.
+ *
+ * The correct data will be returned in @buf.
+ */
+void convert_uuid_to_guid(unsigned char *buf)
+{
+	unsigned char c;
+
+	c = buf[0];
+	buf[0] = buf[3];
+	buf[3] = c;
+	c = buf[1];
+	buf[1] = buf[2];
+	buf[2] = c;
+
+	c = buf[4];
+	buf[4] = buf[5];
+	buf[5] = c;
+
+	c = buf[6];
+	buf[6] = buf[7];
+	buf[7] = c;
+}
+
+int uuid_guid_parse(char *uuidstr, unsigned char *uuid)
+{
+	int ret;
+
+	ret = uuid_parse(uuidstr, uuid);
+	if (ret < 0)
+		return ret;
+
+	if (__use_guid)
+		convert_uuid_to_guid(uuid);
+
+	return ret;
+}
+
+int fwu_parse_fill_image_uuid(struct fwu_mdata_object *mobj,
+			      size_t idx, char *uuids)
+{
+	struct fwu_image_entry *image = fwu_get_image(mobj, idx);
+	struct fwu_image_bank_info *bank;
+	char *p = uuids, *uuid;
+	int i;
+
+	if (!image)
+		return -ENOENT;
+
+	/* Image location UUID */
+	uuid = strsep(&p, ",");
+	if (!uuid)
+		return -EINVAL;
+
+	if (strcmp(uuid, "0") &&
+	    uuid_guid_parse(uuid, (unsigned char *)&image->location_uuid) < 0)
+		return -EINVAL;
+
+	/* Image type UUID */
+	uuid = strsep(&p, ",");
+	if (!uuid)
+		return -EINVAL;
+
+	if (uuid_guid_parse(uuid, (unsigned char *)&image->image_type_uuid) < 0)
+		return -EINVAL;
+
+	/* Fill bank image-UUID */
+	for (i = 0; i < mobj->banks; i++) {
+		bank = fwu_get_bank(mobj, idx, i);
+		if (!bank)
+			return -ENOENT;
+		bank->accepted = 1;
+		uuid = strsep(&p, ",");
+		if (!uuid)
+			return -EINVAL;
+
+		if (strcmp(uuid, "0") &&
+		    uuid_guid_parse(uuid, (unsigned char *)&bank->image_uuid) < 0)
+			return -EINVAL;
+	}
+	return 0;
+}
+
+/* Caller must ensure that @uuids[] has @mobj->images entries. */
+int fwu_parse_fill_uuids(struct fwu_mdata_object *mobj, char *uuids[])
+{
+	struct fwu_mdata *mdata = mobj->mdata;
+	int i, ret;
+
+	mdata->version = FWU_MDATA_VERSION;
+	mdata->active_index = active_bank;
+	mdata->previous_active_index = previous_bank;
+
+	for (i = 0; i < mobj->images; i++) {
+		ret = fwu_parse_fill_image_uuid(mobj, i, uuids[i]);
+		if (ret < 0)
+			return ret;
+	}
+
+	mdata->crc32 = crc32(0, (const unsigned char *)&mdata->version,
+                             mobj->size - sizeof(uint32_t));
+
+	return 0;
+}
+
+int fwu_make_mdata(size_t images, size_t banks, char *uuids[], char *output)
+{
+	struct fwu_mdata_object *mobj;
+	FILE *file;
+	int ret;
+
+	mobj = fwu_alloc_mdata(images, banks);
+	if (!mobj)
+		return -ENOMEM;
+
+	ret = fwu_parse_fill_uuids(mobj, uuids);
+	if (ret < 0)
+		goto done_make;
+
+	file = fopen(output, "w");
+	if (!file) {
+		ret = -errno;
+		goto done_make;
+	}
+
+	ret = fwrite(mobj->mdata, mobj->size, 1, file);
+	if (ret != mobj->size)
+		ret = -errno;
+	else
+		ret = 0;
+
+	fclose(file);
+
+done_make:
+	free(mobj->mdata);
+	free(mobj);
+
+	return ret;
+}
+
+int main(int argc, char *argv[])
+{
+	unsigned long banks = 0, images = 0;
+	int c, ret;
+
+	do {
+		c = getopt_long(argc, argv, opts_short, options, NULL);
+		switch (c) {
+		case 'h':
+			print_usage();
+			return 0;
+		case 'b':
+			banks = strtoul(optarg, NULL, 0);
+			break;
+		case 'i':
+			images = strtoul(optarg, NULL, 0);
+			break;
+		case 'g':
+			__use_guid = true;
+			break;
+		case 'p':
+			previous_bank = strtoul(optarg, NULL, 0);
+			break;
+		case 'a':
+			active_bank = strtoul(optarg, NULL, 0);
+			break;
+		}
+	} while (c != -1);
+
+	if (!banks || !images) {
+		fprintf(stderr, "Error: The number of banks and images must not be 0.\n");
+		return -EINVAL;
+	}
+
+	/* This command takes UUIDs * images and output file. */
+	if (optind + images + 1 != argc) {
+		fprintf(stderr, "Error: UUID list or output file is not specified or too much.\n");
+		print_usage();
+		return -ERANGE;
+	}
+
+	if (previous_bank == INT_MAX) {
+		/* set to the earlier bank in round-robin scheme */
+		previous_bank = active_bank > 0 ? active_bank - 1 : banks - 1;
+	}
+
+	ret = fwu_make_mdata(images, banks, argv + optind, argv[argc - 1]);
+	if (ret < 0)
+		fprintf(stderr, "Error: Failed to parse and write image: %s\n",
+			strerror(-ret));
+
+	return ret;
+}
-- 
2.34.1

Re: [PATCHv3 5/5] tools: Add mkfwumdata tool for FWU metadata image

[Michal Simek]

On 1/9/23 02:07, Jassi Brar wrote:
> From: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> 
> Add 'mkfwumdata' tool to generate FWU metadata image for the meta-data
> partition to be used in A/B Update imeplementation.
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---

./scripts/checkpatch.pl --strict tools/mkfwumdata.c
CHECK: 'Endianess' may be misspelled - perhaps 'Endianness'?
#32: FILE: tools/mkfwumdata.c:32:
+/* TODO: Endianess conversion may be required for some arch. */
           ^^^^^^^^^

ERROR: do not initialise statics to 0
#68: FILE: tools/mkfwumdata.c:68:
+static int active_bank = 0;

ERROR: do not initialise statics to false
#70: FILE: tools/mkfwumdata.c:70:
+static bool __use_guid = false;

ERROR: code indent should use tabs where possible
#234: FILE: tools/mkfwumdata.c:234:
+                             mobj->size - sizeof(uint32_t));$

CHECK: Alignment should match open parenthesis
#234: FILE: tools/mkfwumdata.c:234:
+	mdata->crc32 = crc32(0, (const unsigned char *)&mdata->version,
+                             mobj->size - sizeof(uint32_t));

WARNING: please, no spaces at the start of a line
#234: FILE: tools/mkfwumdata.c:234:
+                             mobj->size - sizeof(uint32_t));$

total: 3 errors, 1 warnings, 2 checks, 326 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
       mechanically convert to the typical style using --fix or --fix-inplace.

NOTE: Whitespace errors detected.
       You may wish to use scripts/cleanpatch or scripts/cleanfile



>   tools/Kconfig      |   9 ++
>   tools/Makefile     |   4 +
>   tools/mkfwumdata.c | 326 +++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 339 insertions(+)
>   create mode 100644 tools/mkfwumdata.c

This is good but actually I can't see any example how you use this tool on your 
board. It would be good to list it in documentation you have written in previous 
patch.

> 
> diff --git a/tools/Kconfig b/tools/Kconfig
> index 539708f277..6e23f44d55 100644
> --- a/tools/Kconfig
> +++ b/tools/Kconfig
> @@ -157,4 +157,13 @@ config LUT_SEQUENCE
>   	help
>   	  Look Up Table Sequence
>   
> +config TOOLS_MKFWUMDATA
> +	bool "Build mkfwumdata command"
> +	default y if FWU_MULTI_BANK_UPDATE
> +	help
> +	  This command allows users to create a raw image of the FWU
> +	  metadata for initial installation of the FWU multi bank
> +	  update on the board. The installation method depends on
> +	  the platform.
> +
>   endmenu
> diff --git a/tools/Makefile b/tools/Makefile
> index 26be0a7ba2..024d6c8eca 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -255,6 +255,10 @@ HOSTLDLIBS_mkeficapsule += \
>   	$(shell pkg-config --libs uuid 2> /dev/null || echo "-luuid")
>   hostprogs-$(CONFIG_TOOLS_MKEFICAPSULE) += mkeficapsule
>   
> +mkfwumdata-objs := mkfwumdata.o lib/crc32.o
> +HOSTLDLIBS_mkfwumdata += -luuid
> +hostprogs-$(CONFIG_TOOLS_MKFWUMDATA) += mkfwumdata
> +
>   # We build some files with extra pedantic flags to try to minimize things
>   # that won't build on some weird host compiler -- though there are lots of
>   # exceptions for files that aren't complaint.
> diff --git a/tools/mkfwumdata.c b/tools/mkfwumdata.c
> new file mode 100644
> index 0000000000..e0b6702039
> --- /dev/null
> +++ b/tools/mkfwumdata.c
> @@ -0,0 +1,326 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2023, Linaro Limited
> + */
> +
> +#include <errno.h>
> +#include <getopt.h>
> +#include <limits.h>
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <u-boot/crc.h>
> +#include <unistd.h>
> +#include <uuid/uuid.h>
> +
> +/* This will dynamically allocate the fwu_mdata */
> +#define CONFIG_FWU_NUM_BANKS		0
> +#define CONFIG_FWU_NUM_IMAGES_PER_BANK	0
> +
> +/* Since we can not include fwu.h, redefine version here. */
> +#define FWU_MDATA_VERSION		1
> +
> +typedef uint8_t u8;
> +typedef int16_t s16;
> +typedef uint16_t u16;
> +typedef uint32_t u32;
> +typedef uint64_t u64;
> +
> +#include <fwu_mdata.h>
> +
> +/* TODO: Endianess conversion may be required for some arch. */
> +
> +static const char *opts_short = "b:i:a:p:gh";
> +
> +static struct option options[] = {
> +	{"banks", required_argument, NULL, 'b'},
> +	{"images", required_argument, NULL, 'i'},
> +	{"guid", required_argument, NULL, 'g'},
> +	{"active-bank", required_argument, NULL, 'a'},
> +	{"previous-bank", required_argument, NULL, 'p'},
> +	{"help", no_argument, NULL, 'h'},
> +	{NULL, 0, NULL, 0},
> +};
> +
> +static void print_usage(void)
> +{
> +	fprintf(stderr, "Usage: mkfwumdata [options] <UUIDs list> <output file>\n");
> +	fprintf(stderr, "Options:\n"
> +		"\t-i, --images <num>          Number of images\n"
> +		"\t-b, --banks  <num>          Number of banks\n"
> +		"\t-a, --active-bank  <num>    Active bank\n"
> +		"\t-p, --previous-bank  <num>  Previous active bank\n"
> +		"\t-g, --guid                  Use GUID instead of UUID\n"
> +		"\t-h, --help                  print a help message\n"
> +		);
> +	fprintf(stderr, "  UUIDs list syntax:\n"
> +		"\t  <location uuid>,<image type uuid>,<images uuid list>\n"
> +		"\t    images uuid list syntax:\n"
> +		"\t\t    img_uuid_00,img_uuid_01...img_uuid_0b,\n"
> +		"\t\t    img_uuid_10,img_uuid_11...img_uuid_1b,\n"
> +		"\t\t    ...,\n"
> +		"\t\t    img_uuid_i0,img_uuid_i1...img_uuid_ib,\n"
> +		"\t\t    where 'b' and 'i' are number of banks and numbder of images in a bank respectively.\n"

typo

And personally \t\t is already nice indentation that you don't need to use 
additional spaces.
And last line would be good to fit to 80 char limit.


> +	       );
> +}
> +
> +static int active_bank = 0;
> +static int previous_bank = INT_MAX; /* unset */
> +static bool __use_guid = false;
> +
> +struct fwu_mdata_object {
> +	size_t images;
> +	size_t banks;
> +	size_t size;
> +	struct fwu_mdata *mdata;
> +};
> +
> +struct fwu_mdata_object *fwu_alloc_mdata(size_t images, size_t banks)

static?

> +{
> +	struct fwu_mdata_object *mobj;
> +
> +	mobj = calloc(1, sizeof(*mobj));
> +	if (!mobj)
> +		return NULL;
> +
> +	mobj->size = sizeof(struct fwu_mdata) +
> +		(sizeof(struct fwu_image_entry) +
> +		 sizeof(struct fwu_image_bank_info) * banks) * images;
> +	mobj->images = images;
> +	mobj->banks = banks;
> +
> +	mobj->mdata = calloc(1, mobj->size);
> +	if (!mobj->mdata) {
> +		free(mobj);
> +		return NULL;
> +	}
> +
> +	return mobj;
> +}
> +
> +struct fwu_image_entry *fwu_get_image(struct fwu_mdata_object *mobj, size_t idx)

static?

> +{
> +	size_t offset;
> +
> +	offset = sizeof(struct fwu_mdata) +
> +		(sizeof(struct fwu_image_entry) +
> +		 sizeof(struct fwu_image_bank_info) * mobj->banks) * idx;
> +
> +	return (struct fwu_image_entry *)((char *)mobj->mdata + offset);
> +}
> +
> +struct fwu_image_bank_info *fwu_get_bank(struct fwu_mdata_object *mobj,
> +					 size_t img_idx, size_t bnk_idx)

static?

> +{
> +	size_t offset;
> +
> +	offset = sizeof(struct fwu_mdata) +
> +		(sizeof(struct fwu_image_entry) +
> +		 sizeof(struct fwu_image_bank_info) * mobj->banks) * img_idx +
> +		sizeof(struct fwu_image_entry) +
> +		sizeof(struct fwu_image_bank_info) * bnk_idx;
> +
> +	return (struct fwu_image_bank_info *)((char *)mobj->mdata + offset);
> +}
> +
> +/**
> + * convert_uuid_to_guid() - convert UUID to GUID
> + * @buf:	UUID binary
> + *
> + * UUID and GUID have the same data structure, but their binary
> + * formats are different due to the endianness. See lib/uuid.c.
> + * Since uuid_parse() can handle only UUID, this function must
> + * be called to get correct data for GUID when parsing a string.
> + *
> + * The correct data will be returned in @buf.
> + */
> +void convert_uuid_to_guid(unsigned char *buf)

static?

> +{
> +	unsigned char c;
> +
> +	c = buf[0];
> +	buf[0] = buf[3];
> +	buf[3] = c;
> +	c = buf[1];
> +	buf[1] = buf[2];
> +	buf[2] = c;
> +
> +	c = buf[4];
> +	buf[4] = buf[5];
> +	buf[5] = c;
> +
> +	c = buf[6];
> +	buf[6] = buf[7];
> +	buf[7] = c;

if this is just endian convertion isn't there any standard function which you 
can just call.


> +}
> +
> +int uuid_guid_parse(char *uuidstr, unsigned char *uuid)

static

> +{
> +	int ret;
> +
> +	ret = uuid_parse(uuidstr, uuid);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (__use_guid)
> +		convert_uuid_to_guid(uuid);
> +
> +	return ret;
> +}
> +
> +int fwu_parse_fill_image_uuid(struct fwu_mdata_object *mobj,
> +			      size_t idx, char *uuids)

static - fix it everywhere.

> +{
> +	struct fwu_image_entry *image = fwu_get_image(mobj, idx);
> +	struct fwu_image_bank_info *bank;
> +	char *p = uuids, *uuid;
> +	int i;
> +
> +	if (!image)
> +		return -ENOENT;
> +
> +	/* Image location UUID */
> +	uuid = strsep(&p, ",");
> +	if (!uuid)
> +		return -EINVAL;
> +
> +	if (strcmp(uuid, "0") &&
> +	    uuid_guid_parse(uuid, (unsigned char *)&image->location_uuid) < 0)
> +		return -EINVAL;
> +
> +	/* Image type UUID */
> +	uuid = strsep(&p, ",");
> +	if (!uuid)
> +		return -EINVAL;
> +
> +	if (uuid_guid_parse(uuid, (unsigned char *)&image->image_type_uuid) < 0)
> +		return -EINVAL;
> +
> +	/* Fill bank image-UUID */
> +	for (i = 0; i < mobj->banks; i++) {
> +		bank = fwu_get_bank(mobj, idx, i);
> +		if (!bank)
> +			return -ENOENT;
> +		bank->accepted = 1;
> +		uuid = strsep(&p, ",");
> +		if (!uuid)
> +			return -EINVAL;
> +
> +		if (strcmp(uuid, "0") &&
> +		    uuid_guid_parse(uuid, (unsigned char *)&bank->image_uuid) < 0)
> +			return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +/* Caller must ensure that @uuids[] has @mobj->images entries. */
> +int fwu_parse_fill_uuids(struct fwu_mdata_object *mobj, char *uuids[])
> +{
> +	struct fwu_mdata *mdata = mobj->mdata;
> +	int i, ret;
> +
> +	mdata->version = FWU_MDATA_VERSION;
> +	mdata->active_index = active_bank;
> +	mdata->previous_active_index = previous_bank;
> +
> +	for (i = 0; i < mobj->images; i++) {
> +		ret = fwu_parse_fill_image_uuid(mobj, i, uuids[i]);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	mdata->crc32 = crc32(0, (const unsigned char *)&mdata->version,
> +                             mobj->size - sizeof(uint32_t));
> +
> +	return 0;
> +}
> +
> +int fwu_make_mdata(size_t images, size_t banks, char *uuids[], char *output)
> +{
> +	struct fwu_mdata_object *mobj;
> +	FILE *file;
> +	int ret;
> +
> +	mobj = fwu_alloc_mdata(images, banks);
> +	if (!mobj)
> +		return -ENOMEM;
> +
> +	ret = fwu_parse_fill_uuids(mobj, uuids);
> +	if (ret < 0)
> +		goto done_make;
> +
> +	file = fopen(output, "w");
> +	if (!file) {
> +		ret = -errno;
> +		goto done_make;
> +	}
> +
> +	ret = fwrite(mobj->mdata, mobj->size, 1, file);
> +	if (ret != mobj->size)
> +		ret = -errno;
> +	else
> +		ret = 0;
> +
> +	fclose(file);
> +
> +done_make:
> +	free(mobj->mdata);
> +	free(mobj);
> +
> +	return ret;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	unsigned long banks = 0, images = 0;
> +	int c, ret;
> +
> +	do {
> +		c = getopt_long(argc, argv, opts_short, options, NULL);
> +		switch (c) {
> +		case 'h':
> +			print_usage();
> +			return 0;
> +		case 'b':
> +			banks = strtoul(optarg, NULL, 0);
> +			break;
> +		case 'i':
> +			images = strtoul(optarg, NULL, 0);
> +			break;
> +		case 'g':
> +			__use_guid = true;
> +			break;
> +		case 'p':
> +			previous_bank = strtoul(optarg, NULL, 0);
> +			break;
> +		case 'a':
> +			active_bank = strtoul(optarg, NULL, 0);
> +			break;
> +		}
> +	} while (c != -1);
> +
> +	if (!banks || !images) {
> +		fprintf(stderr, "Error: The number of banks and images must not be 0.\n");
> +		return -EINVAL;
> +	}
> +
> +	/* This command takes UUIDs * images and output file. */
> +	if (optind + images + 1 != argc) {
> +		fprintf(stderr, "Error: UUID list or output file is not specified or too much.\n");
> +		print_usage();
> +		return -ERANGE;
> +	}
> +
> +	if (previous_bank == INT_MAX) {
> +		/* set to the earlier bank in round-robin scheme */
> +		previous_bank = active_bank > 0 ? active_bank - 1 : banks - 1;
> +	}
> +
> +	ret = fwu_make_mdata(images, banks, argv + optind, argv[argc - 1]);
> +	if (ret < 0)
> +		fprintf(stderr, "Error: Failed to parse and write image: %s\n",
> +			strerror(-ret));
> +
> +	return ret;
> +}

M

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

[Michal Simek]

Hi,

On 1/2/23 19:25, Jassi Brar wrote:
> 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.

First of all I have strong suspicious that this series are pretty much two 
series at once.
You can look at
https://lore.kernel.org/all/20230102182532.2411125-1-jaswinder.singh@linaro.org/#r

Where two 3/5 patches are listed

[PATCHv3 3/5] fwu: gpt: implement read_mdata and write_mdata callbacks
[PATCHv3 3/5] dt: fwu: developerbox: enable fwu banks and mdata regions

I think you are maybe updating the same cover letter with previous IDs.
I would recommend you to use patman to handle this properly.

The second issue is that you are sending patches from
Jassi Brar <jassisinghbrar@gmail.com>
but SOB is
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>

And Tom said in past that they should match. There is a hook for it to check it 
which everybody should be using. That's why please fix this in the next series.

I will comment other things in the particular patch.

Thanks,
Michal

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

[Jassi Brar]

On Wed, Jan 18, 2023 at 7:28 AM Michal Simek <michal.simek@amd.com> wrote:
>
> Hi,
>
> On 1/2/23 19:25, Jassi Brar wrote:
> > 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.
>
> First of all I have strong suspicious that this series are pretty much two
> series at once.
>
Yes, I submitted two patchsets.
1) Optimizing the api of current fwu.
2) Introduce support for mtd backed storage (DeveloperBox platform as
an instance) using the new api.

They appear just fine in my inbox. Do they appear bad to you?

>
> The second issue is that you are sending patches from
> Jassi Brar <jassisinghbrar@gmail.com>
> but SOB is
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>
> And Tom said in past that they should match. There is a hook for it to check it
> which everybody should be using. That's why please fix this in the next series.
>
I have submitted dozens of patches and pull requests over the last
many years. This never occurred to anybody.
BTW, the 'Author' and 'Signed-off-by' appear consistent in git log.
And there are very recent instances in uboot git log where even they
actually differ.

But if Tom really wants, I am happy to send-email from my other account.

Thanks.

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

[Tom Rini]

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

On Wed, Jan 18, 2023 at 08:13:01AM -0600, Jassi Brar wrote:
> On Wed, Jan 18, 2023 at 7:28 AM Michal Simek <michal.simek@amd.com> wrote:
> >
> > Hi,
> >
> > On 1/2/23 19:25, Jassi Brar wrote:
> > > 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.
> >
> > First of all I have strong suspicious that this series are pretty much two
> > series at once.
> >
> Yes, I submitted two patchsets.
> 1) Optimizing the api of current fwu.
> 2) Introduce support for mtd backed storage (DeveloperBox platform as
> an instance) using the new api.
> 
> They appear just fine in my inbox. Do they appear bad to you?
> 
> >
> > The second issue is that you are sending patches from
> > Jassi Brar <jassisinghbrar@gmail.com>
> > but SOB is
> > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> >
> > And Tom said in past that they should match. There is a hook for it to check it
> > which everybody should be using. That's why please fix this in the next series.
> >
> I have submitted dozens of patches and pull requests over the last
> many years. This never occurred to anybody.
> BTW, the 'Author' and 'Signed-off-by' appear consistent in git log.
> And there are very recent instances in uboot git log where even they
> actually differ.
> 
> But if Tom really wants, I am happy to send-email from my other account.

It doesn't matter what the email you send from is, but Author and Signed-off-by
really really should match in the commit itself. It can be annoying,
depending on corporate setup / policies, to have send-email work
directly with your corporate account, but if you're allowed to be
submitting patches, that's where the match between Author and
Signed-off-by matters.

-- 
Tom

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

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

[Michal Simek]

On 1/18/23 15:13, Jassi Brar wrote:
> On Wed, Jan 18, 2023 at 7:28 AM Michal Simek <michal.simek@amd.com> wrote:
>>
>> Hi,
>>
>> On 1/2/23 19:25, Jassi Brar wrote:
>>> 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.
>>
>> First of all I have strong suspicious that this series are pretty much two
>> series at once.
>>
> Yes, I submitted two patchsets.
> 1) Optimizing the api of current fwu.
> 2) Introduce support for mtd backed storage (DeveloperBox platform as
> an instance) using the new api.
> 
> They appear just fine in my inbox. Do they appear bad to you?


Take a look here.
https://lore.kernel.org/all/20230102182532.2411125-1-jaswinder.singh@linaro.org/#r
where you can see two series in the same thread.

And this pretty much confuse b4.

> 
>>
>> The second issue is that you are sending patches from
>> Jassi Brar <jassisinghbrar@gmail.com>
>> but SOB is
>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>>
>> And Tom said in past that they should match. There is a hook for it to check it
>> which everybody should be using. That's why please fix this in the next series.
>>
> I have submitted dozens of patches and pull requests over the last
> many years. This never occurred to anybody.

It really depends how you download that patches.

Thanks,
Michal