Hi Sughosh

On Thu, Sep 15, 2022 at 01:44:39PM +0530, Sughosh Ganu wrote:

In the FWU Multi Bank Update feature, the information about the updatable images is stored as part of the metadata, on a separate partition. Add a driver for reading from and writing to the metadata when the updatable images and the metadata are stored on a block device which is formatted with GPT based partition scheme.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Reviewed-by: Patrick Delaunay patrick.delaunay@foss.st.com Reviewed-by: Etienne Carriere etienne.carriere@linaro.org

---

Changes since V9:

• s/formated/formatted in the commit message
• Add a '\n' character in the log message

drivers/fwu-mdata/Kconfig | 16 ++ drivers/fwu-mdata/Makefile | 8 + drivers/fwu-mdata/gpt_blk.c | 378 ++++++++++++++++++++++++++++++++++++ include/fwu.h | 4 + 4 files changed, 406 insertions(+) create mode 100644 drivers/fwu-mdata/Kconfig create mode 100644 drivers/fwu-mdata/Makefile create mode 100644 drivers/fwu-mdata/gpt_blk.c

diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig new file mode 100644 index 0000000000..7322da48b1 --- /dev/null +++ b/drivers/fwu-mdata/Kconfig @@ -0,0 +1,16 @@ +config FWU_MDATA

• bool "Driver support for accessing FWU Metadata"
• depends on DM
• help

•  Enable support for accessing FWU Metadata partitions. The
  

•  FWU Metadata partitions reside on the same storage device
  

•  which contains the other FWU updatable firmware images.
  

+config FWU_MDATA_GPT_BLK

• bool "FWU Metadata access for GPT partitioned Block devices"
• select PARTITION_TYPE_GUID
• select PARTITION_UUIDS
• depends on DM && HAVE_BLOCK_DEVICE && EFI_PARTITION
• help

•  Enable support for accessing FWU Metadata on GPT partitioned
  

•  block devices.
  

diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile new file mode 100644 index 0000000000..3fee64c10c --- /dev/null +++ b/drivers/fwu-mdata/Makefile @@ -0,0 +1,8 @@ +# SPDX-License-Identifier: GPL-2.0-or-later +# +# Copyright (c) 2022, Linaro Limited +#

+obj-$(CONFIG_FWU_MDATA) += fwu-mdata-uclass.o +obj-$(CONFIG_FWU_MDATA_GPT_BLK) += gpt_blk.o diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c new file mode 100644 index 0000000000..df115575e0 --- /dev/null +++ b/drivers/fwu-mdata/gpt_blk.c @@ -0,0 +1,378 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/*

• • Copyright (c) 2022, Linaro Limited
• */

+#define LOG_CATEGORY UCLASS_FWU_MDATA

+#include <blk.h> +#include <dm.h> +#include <efi_loader.h> +#include <fwu.h> +#include <fwu_mdata.h> +#include <log.h> +#include <memalign.h> +#include <part.h> +#include <part_efi.h>

+#include <dm/device-internal.h> +#include <linux/errno.h> +#include <linux/types.h>

+#define PRIMARY_PART BIT(0) +#define SECONDARY_PART BIT(1) +#define BOTH_PARTS (PRIMARY_PART | SECONDARY_PART)

+#define MDATA_READ BIT(0) +#define MDATA_WRITE BIT(1)

+static int gpt_get_mdata_partitions(struct blk_desc *desc,

• 		    uint *primary_mpart,
  

• 		    uint *secondary_mpart)
  

+{

• int i, ret;
• u32 mdata_parts;
• efi_guid_t part_type_guid;
• struct disk_partition info;
• const efi_guid_t fwu_mdata_guid = FWU_MDATA_GUID;
• mdata_parts = 0;
• for (i = 1; i < MAX_SEARCH_PARTITIONS; 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)) {
  

• 	++mdata_parts;
  

• 	if (!*primary_mpart)
  

• 		*primary_mpart = i;
  

• 	else
  

• 		*secondary_mpart = i;
  

• }
  

• }
• if (mdata_parts != 2) {

• log_debug("Expect two copies of the FWU metadata instead of %d\n",
  

• 	  mdata_parts);
  

• ret = -EINVAL;
  

• } else {

• ret = 0;
  

• }

Can we change that a bit? There are are some assumptions in this code, e.g the user must pass the values zero'ed out. Can we instead get an array of 2 instead? In that case you don't care if any of the values are zero and you can just fill in the array. Something like:

if (!guidcmp(&fwu_mdata_guid, &part_type_guid)) { array[mdata_parts] = i; mdata_parts++; You can also move the check in that if and make the func a lot easier to read.

• return ret;

+}

+static int gpt_get_mdata_disk_part(struct blk_desc *desc,

• 		   struct disk_partition *info,
  

• 		   u32 part_num)
  

+{

• int ret;
• char *mdata_guid_str = "8a7a84a0-8387-40f6-ab41-a8b9a5a60d23";
• ret = part_get_info(desc, part_num, info);
• if (ret < 0) {

• log_debug("Unable to get the partition info for the FWU metadata part %d\n",
  

• 	  part_num);
  

• return -ENOENT;
  

• }
• /* Check that it is indeed the FWU metadata partition */
• if (!strncmp(info->type_guid, mdata_guid_str, UUID_STR_LEN)) {

• /* Found the FWU metadata partition */
  

• return 0;
  

• }
• return -ENOENT;

+}

+static int gpt_read_write_mdata(struct blk_desc *desc,

• 		struct fwu_mdata *mdata,
  

• 		u8 access, u32 part_num)
  

+{

• int ret;
• u32 len, blk_start, blkcnt;
• struct disk_partition info;
• ALLOC_CACHE_ALIGN_BUFFER_PAD(struct fwu_mdata, mdata_aligned, 1,

• 		     desc->blksz);
  

• ret = gpt_get_mdata_disk_part(desc, &info, part_num);
• if (ret < 0) {

• printf("Unable to get the FWU metadata partition\n");
  

• return -ENOENT;
  

• }
• len = sizeof(*mdata);
• blkcnt = BLOCK_CNT(len, desc);
• if (blkcnt > info.size) {

• log_debug("Block count exceeds FWU metadata partition size\n");
  

• return -ERANGE;
  

• }
• blk_start = info.start;
• if (access == MDATA_READ) {

• if (blk_dread(desc, blk_start, blkcnt, mdata_aligned) != blkcnt) {
  

• 	log_debug("Error reading FWU metadata from the device\n");
  

• 	return -EIO;
  

• }
  

• memcpy(mdata, mdata_aligned, sizeof(struct fwu_mdata));
  

• } else {

• if (blk_dwrite(desc, blk_start, blkcnt, mdata) != blkcnt) {
  

• 	log_debug("Error writing FWU metadata to the device\n");
  

• 	return -EIO;
  

• }
  

• }
• return 0;

+}

+static int gpt_read_mdata(struct blk_desc *desc,

• 	  struct fwu_mdata *mdata, u32 part_num)
  

+{

• return gpt_read_write_mdata(desc, mdata, MDATA_READ, part_num);

+}

+static int gpt_write_mdata_partition(struct blk_desc *desc,

• 			struct fwu_mdata *mdata,
  

• 			u32 part_num)
  

+{

• return gpt_read_write_mdata(desc, mdata, MDATA_WRITE, part_num);

+}

+static int fwu_gpt_update_mdata(struct udevice *dev, struct fwu_mdata *mdata) +{

• int ret;
• struct blk_desc *desc;
• uint primary_mpart = 0, secondary_mpart = 0;
• 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, &primary_mpart,

• 		       &secondary_mpart);
  

• if (ret < 0) {

• log_debug("Error getting the FWU metadata partitions\n");
  

• return -ENOENT;
  

• }
• /* First write the primary partition */
• ret = gpt_write_mdata_partition(desc, mdata, primary_mpart);
• if (ret < 0) {

• log_debug("Updating primary FWU metadata partition failed\n");
  

• return ret;
  

• }
• /* And now the replica */
• ret = gpt_write_mdata_partition(desc, mdata, secondary_mpart);
• 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;
• uint primary_mpart = 0, secondary_mpart = 0;
• ret = gpt_get_mdata_partitions(desc, &primary_mpart,

• 		       &secondary_mpart);
  

• if (ret < 0) {

• log_debug("Error getting the FWU metadata partitions\n");
  

• return -ENOENT;
  

• }
• ret = gpt_read_mdata(desc, mdata, primary_mpart);
• 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_mdata(desc, mdata, secondary_mpart);
• 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 gpt_check_mdata_validity(struct udevice *dev) +{

• int ret;
• struct blk_desc *desc;
• struct fwu_mdata pri_mdata;
• struct fwu_mdata secondary_mdata;
• uint primary_mpart = 0, secondary_mpart = 0;
• uint valid_partitions, invalid_partitions;
• struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
• desc = dev_get_uclass_plat(priv->blk_dev);
• /*

• * Two FWU metadata partitions are expected.
  

• * If we don't have two, user needs to create
  

• * them first
  

• */
  

• valid_partitions = 0;
• ret = gpt_get_mdata_partitions(desc, &primary_mpart,

• 		       &secondary_mpart);
  

• if (ret < 0) {

• log_debug("Error getting the FWU metadata partitions\n");
  

• return -ENOENT;
  

• }
• ret = gpt_read_mdata(desc, &pri_mdata, primary_mpart);
• if (ret < 0) {

• log_debug("Failed to read the FWU metadata from the device\n");
  

• goto secondary_read;
  

Can't we skip the goto here and just make the if above (if ret == 0)?

• }
• ret = fwu_verify_mdata(&pri_mdata, 1);
• if (!ret)

• valid_partitions |= PRIMARY_PART;
  

+secondary_read:

• /* Now check the secondary partition */
• ret = gpt_read_mdata(desc, &secondary_mdata, secondary_mpart);
• if (ret < 0) {

• log_debug("Failed to read the FWU metadata from the device\n");
  

Same here

• goto mdata_restore;
  

• }
• ret = fwu_verify_mdata(&secondary_mdata, 0);
• if (!ret)

• valid_partitions |= SECONDARY_PART;
  

+mdata_restore:

• if (valid_partitions == (PRIMARY_PART | SECONDARY_PART)) {

• ret = -1;
  

• /*
  

•  * Before returning, check that both the
  

•  * FWU metadata copies are the same. If not,
  

•  * the FWU metadata copies need to be
  

•  * re-populated.
  

•  */
  

• if (!memcmp(&pri_mdata, &secondary_mdata,
  

• 	    sizeof(struct fwu_mdata))) {
  

• 	ret = 0;
  

• } else {
  

• 	log_debug("Both FWU metadata copies are valid but do not match. Please check!\n");
  

• }
  

• goto out;
  

• }
• ret = -1;
• if (!(valid_partitions & BOTH_PARTS))

• goto out;
  

• invalid_partitions = valid_partitions ^ BOTH_PARTS;
• ret = gpt_write_mdata_partition(desc,

• 			(invalid_partitions == PRIMARY_PART) ?
  

• 			&secondary_mdata : &pri_mdata,
  

• 			(invalid_partitions == PRIMARY_PART) ?
  

• 			primary_mpart : secondary_mpart);
  

• if (ret < 0)

• log_debug("Restoring %s FWU metadata partition failed\n",
  

• 	  (invalid_partitions == PRIMARY_PART) ?
  

• 	  "primary" : "secondary");
  

+out:

• return ret;

+}

+static int fwu_gpt_mdata_check(struct udevice *dev) +{

• /*

• * Check if both the copies of the FWU
  

• * metadata are valid. If one has gone
  

• * bad, restore it from the other good
  

• * copy.
  

• */
  

• return gpt_check_mdata_validity(dev);

+}

+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_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev) +{

• u32 phandle;
• int ret, size;
• struct udevice *parent;
• const fdt32_t *phandle_p = NULL;
• phandle_p = dev_read_prop(dev, "fwu-mdata-store", &size);
• if (!phandle_p) {

• log_debug("fwu-mdata-store property not found\n");
  

• return -ENOENT;
  

• }
• phandle = fdt32_to_cpu(*phandle_p);
• ret = device_get_global_by_ofnode(ofnode_get_by_phandle(phandle),

• 			  &parent);
  

• if (ret)

• return ret;
  

• return blk_get_from_parent(parent, mdata_dev);

+}

+static int fwu_mdata_gpt_blk_probe(struct udevice *dev) +{

• int ret;
• struct udevice *mdata_dev = NULL;
• struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
• ret = fwu_get_mdata_device(dev, &mdata_dev);
• if (ret)

• return ret;
  

• priv->blk_dev = mdata_dev;
• return 0;

+}

+static const struct fwu_mdata_ops fwu_gpt_blk_ops = {

• .mdata_check = fwu_gpt_mdata_check,
• .get_mdata = fwu_gpt_get_mdata,
• .update_mdata = fwu_gpt_update_mdata,

+};

+static const struct udevice_id fwu_mdata_ids[] = {

• { .compatible = "u-boot,fwu-mdata-gpt" },
• { }

+};

+U_BOOT_DRIVER(fwu_mdata_gpt_blk) = {

• .name = "fwu-mdata-gpt-blk",
• .id = UCLASS_FWU_MDATA,
• .of_match = fwu_mdata_ids,
• .ops = &fwu_gpt_blk_ops,
• .probe = fwu_mdata_gpt_blk_probe,
• .priv_auto = sizeof(struct fwu_mdata_gpt_blk_priv),

+}; diff --git a/include/fwu.h b/include/fwu.h index 745f6225d0..1e16253f69 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -14,6 +14,10 @@ struct fwu_mdata; struct udevice;

+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

-- 2.34.1

Cheers /Ilias

Hello Sughosh and all,

On Mon, 26 Sept 2022 at 10:46, Sughosh Ganu sughosh.ganu@linaro.org wrote:

hi Ilias,

On Thu, 22 Sept 2022 at 14:16, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

...

Hi Sughosh

On Thu, Sep 15, 2022 at 01:44:39PM +0530, Sughosh Ganu wrote:

...

In the FWU Multi Bank Update feature, the information about the updatable images is stored as part of the metadata, on a separate partition. Add a driver for reading from and writing to the metadata when the updatable images and the metadata are stored on a block device which is formatted with GPT based partition scheme.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Reviewed-by: Patrick Delaunay patrick.delaunay@foss.st.com Reviewed-by: Etienne Carriere etienne.carriere@linaro.org

---

Changes since V9:

• s/formated/formatted in the commit message
• Add a '\n' character in the log message

drivers/fwu-mdata/Kconfig | 16 ++ drivers/fwu-mdata/Makefile | 8 + drivers/fwu-mdata/gpt_blk.c | 378 ++++++++++++++++++++++++++++++++++++ include/fwu.h | 4 + 4 files changed, 406 insertions(+) create mode 100644 drivers/fwu-mdata/Kconfig create mode 100644 drivers/fwu-mdata/Makefile create mode 100644 drivers/fwu-mdata/gpt_blk.c

diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig new file mode 100644 index 0000000000..7322da48b1 --- /dev/null +++ b/drivers/fwu-mdata/Kconfig @@ -0,0 +1,16 @@ +config FWU_MDATA

• bool "Driver support for accessing FWU Metadata"
  

• depends on DM
  

• help
  

•   Enable support for accessing FWU Metadata partitions. The
  

•   FWU Metadata partitions reside on the same storage device
  

•   which contains the other FWU updatable firmware images.
  

+config FWU_MDATA_GPT_BLK

• bool "FWU Metadata access for GPT partitioned Block devices"
  

• select PARTITION_TYPE_GUID
  

• select PARTITION_UUIDS
  

• depends on DM && HAVE_BLOCK_DEVICE && EFI_PARTITION
  

• help
  

•   Enable support for accessing FWU Metadata on GPT partitioned
  

•   block devices.
  

By the way, drop HAVE_BLOCK_DEVICE that is no more needed here. See https://source.denx.de/u-boot/u-boot/-/commit/7f8967c2b82f9917987b69fbf43f8f...

...

...

diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile new file mode 100644 index 0000000000..3fee64c10c --- /dev/null +++ b/drivers/fwu-mdata/Makefile @@ -0,0 +1,8 @@ +# SPDX-License-Identifier: GPL-2.0-or-later +# +# Copyright (c) 2022, Linaro Limited +#

+obj-$(CONFIG_FWU_MDATA) += fwu-mdata-uclass.o +obj-$(CONFIG_FWU_MDATA_GPT_BLK) += gpt_blk.o diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c new file mode 100644 index 0000000000..df115575e0 --- /dev/null +++ b/drivers/fwu-mdata/gpt_blk.c @@ -0,0 +1,378 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/*

• • Copyright (c) 2022, Linaro Limited
• */

+#define LOG_CATEGORY UCLASS_FWU_MDATA

+#include <blk.h> +#include <dm.h> +#include <efi_loader.h> +#include <fwu.h> +#include <fwu_mdata.h> +#include <log.h> +#include <memalign.h> +#include <part.h> +#include <part_efi.h>

+#include <dm/device-internal.h> +#include <linux/errno.h> +#include <linux/types.h>

+#define PRIMARY_PART BIT(0) +#define SECONDARY_PART BIT(1) +#define BOTH_PARTS (PRIMARY_PART | SECONDARY_PART)

+#define MDATA_READ BIT(0) +#define MDATA_WRITE BIT(1)

+static int gpt_get_mdata_partitions(struct blk_desc *desc,

•                             uint *primary_mpart,
  

•                             uint *secondary_mpart)
  

+{

• int i, ret;
  

• u32 mdata_parts;
  

• efi_guid_t part_type_guid;
  

• struct disk_partition info;
  

• const efi_guid_t fwu_mdata_guid = FWU_MDATA_GUID;
  

• mdata_parts = 0;
  

• for (i = 1; i < MAX_SEARCH_PARTITIONS; 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)) {
  

•                 ++mdata_parts;
  

•                 if (!*primary_mpart)
  

•                         *primary_mpart = i;
  

•                 else
  

•                         *secondary_mpart = i;
  

•         }
  

• }
  

• if (mdata_parts != 2) {
  

•         log_debug("Expect two copies of the FWU metadata instead of %d\n",
  

•                   mdata_parts);
  

•         ret = -EINVAL;
  

• } else {
  

•         ret = 0;
  

• }
  

Can we change that a bit? There are are some assumptions in this code, e.g the user must pass the values zero'ed out. Can we instead get an array of

I don't agree. If the function returns an error code, caller shall not use the output content. That is what callers of gpt_get_mdata_partitions() do in this patch, so returning -EINVAL if fine. (note that I don't mind this gets an array ref rather than 2 pointers).

best regards, etienne

...

2 instead? In that case you don't care if any of the values are zero and you can just fill in the array. Something like:

Okay, will make use of the array as you suggest.

...

if (!guidcmp(&fwu_mdata_guid, &part_type_guid)) { array[mdata_parts] = i; mdata_parts++; You can also move the check in that if and make the func a lot easier to read.

Sorry, I do not get this part.

...

...

• return ret;
  

+}

+static int gpt_get_mdata_disk_part(struct blk_desc *desc,

•                            struct disk_partition *info,
  

•                            u32 part_num)
  

+{

• int ret;
  

• char *mdata_guid_str = "8a7a84a0-8387-40f6-ab41-a8b9a5a60d23";
  

• ret = part_get_info(desc, part_num, info);
  

• if (ret < 0) {
  

•         log_debug("Unable to get the partition info for the FWU metadata part %d\n",
  

•                   part_num);
  

•         return -ENOENT;
  

• }
  

• /* Check that it is indeed the FWU metadata partition */
  

• if (!strncmp(info->type_guid, mdata_guid_str, UUID_STR_LEN)) {
  

•         /* Found the FWU metadata partition */
  

•         return 0;
  

• }
  

• return -ENOENT;
  

+}

+static int gpt_read_write_mdata(struct blk_desc *desc,

•                         struct fwu_mdata *mdata,
  

•                         u8 access, u32 part_num)
  

+{

• int ret;
  

• u32 len, blk_start, blkcnt;
  

• struct disk_partition info;
  

• ALLOC_CACHE_ALIGN_BUFFER_PAD(struct fwu_mdata, mdata_aligned, 1,
  

•                              desc->blksz);
  

• ret = gpt_get_mdata_disk_part(desc, &info, part_num);
  

• if (ret < 0) {
  

•         printf("Unable to get the FWU metadata partition\n");
  

•         return -ENOENT;
  

• }
  

• len = sizeof(*mdata);
  

• blkcnt = BLOCK_CNT(len, desc);
  

• if (blkcnt > info.size) {
  

•         log_debug("Block count exceeds FWU metadata partition size\n");
  

•         return -ERANGE;
  

• }
  

• blk_start = info.start;
  

• if (access == MDATA_READ) {
  

•         if (blk_dread(desc, blk_start, blkcnt, mdata_aligned) != blkcnt) {
  

•                 log_debug("Error reading FWU metadata from the device\n");
  

•                 return -EIO;
  

•         }
  

•         memcpy(mdata, mdata_aligned, sizeof(struct fwu_mdata));
  

• } else {
  

•         if (blk_dwrite(desc, blk_start, blkcnt, mdata) != blkcnt) {
  

•                 log_debug("Error writing FWU metadata to the device\n");
  

•                 return -EIO;
  

•         }
  

• }
  

• return 0;
  

+}

+static int gpt_read_mdata(struct blk_desc *desc,

•                   struct fwu_mdata *mdata, u32 part_num)
  

+{

• return gpt_read_write_mdata(desc, mdata, MDATA_READ, part_num);
  

+}

+static int gpt_write_mdata_partition(struct blk_desc *desc,

•                                 struct fwu_mdata *mdata,
  

•                                 u32 part_num)
  

+{

• return gpt_read_write_mdata(desc, mdata, MDATA_WRITE, part_num);
  

+}

+static int fwu_gpt_update_mdata(struct udevice *dev, struct fwu_mdata *mdata) +{

• int ret;
  

• struct blk_desc *desc;
  

• uint primary_mpart = 0, secondary_mpart = 0;
  

• 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, &primary_mpart,
  

•                                &secondary_mpart);
  

• if (ret < 0) {
  

•         log_debug("Error getting the FWU metadata partitions\n");
  

•         return -ENOENT;
  

• }
  

• /* First write the primary partition */
  

• ret = gpt_write_mdata_partition(desc, mdata, primary_mpart);
  

• if (ret < 0) {
  

•         log_debug("Updating primary FWU metadata partition failed\n");
  

•         return ret;
  

• }
  

• /* And now the replica */
  

• ret = gpt_write_mdata_partition(desc, mdata, secondary_mpart);
  

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

• uint primary_mpart = 0, secondary_mpart = 0;
  

• ret = gpt_get_mdata_partitions(desc, &primary_mpart,
  

•                                &secondary_mpart);
  

• if (ret < 0) {
  

•         log_debug("Error getting the FWU metadata partitions\n");
  

•         return -ENOENT;
  

• }
  

• ret = gpt_read_mdata(desc, mdata, primary_mpart);
  

• 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_mdata(desc, mdata, secondary_mpart);
  

• 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 gpt_check_mdata_validity(struct udevice *dev) +{

• int ret;
  

• struct blk_desc *desc;
  

• struct fwu_mdata pri_mdata;
  

• struct fwu_mdata secondary_mdata;
  

• uint primary_mpart = 0, secondary_mpart = 0;
  

• uint valid_partitions, invalid_partitions;
  

• struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
  

• desc = dev_get_uclass_plat(priv->blk_dev);
  

• /*
  

•  * Two FWU metadata partitions are expected.
  

•  * If we don't have two, user needs to create
  

•  * them first
  

•  */
  

• valid_partitions = 0;
  

• ret = gpt_get_mdata_partitions(desc, &primary_mpart,
  

•                                &secondary_mpart);
  

• if (ret < 0) {
  

•         log_debug("Error getting the FWU metadata partitions\n");
  

•         return -ENOENT;
  

• }
  

• ret = gpt_read_mdata(desc, &pri_mdata, primary_mpart);
  

• if (ret < 0) {
  

•         log_debug("Failed to read the FWU metadata from the device\n");
  

•         goto secondary_read;
  

Can't we skip the goto here and just make the if above (if ret == 0)?

Will change

...

...

• }
  

• ret = fwu_verify_mdata(&pri_mdata, 1);
  

• if (!ret)
  

•         valid_partitions |= PRIMARY_PART;
  

+secondary_read:

• /* Now check the secondary partition */
  

• ret = gpt_read_mdata(desc, &secondary_mdata, secondary_mpart);
  

• if (ret < 0) {
  

•         log_debug("Failed to read the FWU metadata from the device\n");
  

Same here

Will change

-sughosh

...

...

•         goto mdata_restore;
  

• }
  

• ret = fwu_verify_mdata(&secondary_mdata, 0);
  

• if (!ret)
  

•         valid_partitions |= SECONDARY_PART;
  

+mdata_restore:

• if (valid_partitions == (PRIMARY_PART | SECONDARY_PART)) {
  

•         ret = -1;
  

•         /*
  

•          * Before returning, check that both the
  

•          * FWU metadata copies are the same. If not,
  

•          * the FWU metadata copies need to be
  

•          * re-populated.
  

•          */
  

•         if (!memcmp(&pri_mdata, &secondary_mdata,
  

•                     sizeof(struct fwu_mdata))) {
  

•                 ret = 0;
  

•         } else {
  

•                 log_debug("Both FWU metadata copies are valid but do not match. Please check!\n");
  

•         }
  

•         goto out;
  

• }
  

• ret = -1;
  

• if (!(valid_partitions & BOTH_PARTS))
  

•         goto out;
  

• invalid_partitions = valid_partitions ^ BOTH_PARTS;
  

• ret = gpt_write_mdata_partition(desc,
  

•                                 (invalid_partitions == PRIMARY_PART) ?
  

•                                 &secondary_mdata : &pri_mdata,
  

•                                 (invalid_partitions == PRIMARY_PART) ?
  

•                                 primary_mpart : secondary_mpart);
  

• if (ret < 0)
  

•         log_debug("Restoring %s FWU metadata partition failed\n",
  

•                   (invalid_partitions == PRIMARY_PART) ?
  

•                   "primary" : "secondary");
  

+out:

• return ret;
  

+}

+static int fwu_gpt_mdata_check(struct udevice *dev) +{

• /*
  

•  * Check if both the copies of the FWU
  

•  * metadata are valid. If one has gone
  

•  * bad, restore it from the other good
  

•  * copy.
  

•  */
  

• return gpt_check_mdata_validity(dev);
  

+}

+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_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev) +{

• u32 phandle;
  

• int ret, size;
  

• struct udevice *parent;
  

• const fdt32_t *phandle_p = NULL;
  

• phandle_p = dev_read_prop(dev, "fwu-mdata-store", &size);
  

• if (!phandle_p) {
  

•         log_debug("fwu-mdata-store property not found\n");
  

•         return -ENOENT;
  

• }
  

• phandle = fdt32_to_cpu(*phandle_p);
  

• ret = device_get_global_by_ofnode(ofnode_get_by_phandle(phandle),
  

•                                   &parent);
  

• if (ret)
  

•         return ret;
  

• return blk_get_from_parent(parent, mdata_dev);
  

+}

+static int fwu_mdata_gpt_blk_probe(struct udevice *dev) +{

• int ret;
  

• struct udevice *mdata_dev = NULL;
  

• struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
  

• ret = fwu_get_mdata_device(dev, &mdata_dev);
  

• if (ret)
  

•         return ret;
  

• priv->blk_dev = mdata_dev;
  

• return 0;
  

+}

+static const struct fwu_mdata_ops fwu_gpt_blk_ops = {

• .mdata_check = fwu_gpt_mdata_check,
  

• .get_mdata = fwu_gpt_get_mdata,
  

• .update_mdata = fwu_gpt_update_mdata,
  

+};

+static const struct udevice_id fwu_mdata_ids[] = {

• { .compatible = "u-boot,fwu-mdata-gpt" },
  

• { }
  

+};

+U_BOOT_DRIVER(fwu_mdata_gpt_blk) = {

• .name           = "fwu-mdata-gpt-blk",
  

• .id             = UCLASS_FWU_MDATA,
  

• .of_match       = fwu_mdata_ids,
  

• .ops            = &fwu_gpt_blk_ops,
  

• .probe          = fwu_mdata_gpt_blk_probe,
  

• .priv_auto      = sizeof(struct fwu_mdata_gpt_blk_priv),
  

+}; diff --git a/include/fwu.h b/include/fwu.h index 745f6225d0..1e16253f69 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -14,6 +14,10 @@ struct fwu_mdata; struct udevice;

+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

-- 2.34.1

Cheers /Ilias

On Thu, Sep 15, 2022 at 3:16 AM Sughosh Ganu sughosh.ganu@linaro.org wrote:

diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig new file mode 100644 index 0000000000..7322da48b1 --- /dev/null +++ b/drivers/fwu-mdata/Kconfig @@ -0,0 +1,16 @@ +config FWU_MDATA

•   bool "Driver support for accessing FWU Metadata"
  

•   depends on DM
  

•   help
  

•     Enable support for accessing FWU Metadata partitions. The
  

•     FWU Metadata partitions reside on the same storage device
  

•     which contains the other FWU updatable firmware images.
  

+config FWU_MDATA_GPT_BLK

•   bool "FWU Metadata access for GPT partitioned Block devices"
  

•   select PARTITION_TYPE_GUID
  

•   select PARTITION_UUIDS
  

•   depends on DM && HAVE_BLOCK_DEVICE && EFI_PARTITION
  
  

It should depend on FWU_MDATA instead of DM. Though, eventually it will be a choice between GPT and MTD.

...

+static int fwu_gpt_mdata_check(struct udevice *dev) +{

•   /*
  

•    * Check if both the copies of the FWU
  

•    * metadata are valid. If one has gone
  

•    * bad, restore it from the other good
  

•    * copy.
  

•    */
  
  

This comment is repeated 3 more times in the patchset. Maybe get rid of this and some other.

cheers.

On Mon, Sep 26, 2022 at 3:48 AM Sughosh Ganu sughosh.ganu@linaro.org wrote:

On Mon, 26 Sept 2022 at 08:22, Jassi Brar jassisinghbrar@gmail.com wrote:

...

On Thu, Sep 15, 2022 at 3:16 AM Sughosh Ganu sughosh.ganu@linaro.org wrote:

...

diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig new file mode 100644 index 0000000000..7322da48b1 --- /dev/null +++ b/drivers/fwu-mdata/Kconfig @@ -0,0 +1,16 @@ +config FWU_MDATA

•   bool "Driver support for accessing FWU Metadata"
  

•   depends on DM
  

•   help
  

•     Enable support for accessing FWU Metadata partitions. The
  

•     FWU Metadata partitions reside on the same storage device
  

•     which contains the other FWU updatable firmware images.
  

+config FWU_MDATA_GPT_BLK

•   bool "FWU Metadata access for GPT partitioned Block devices"
  

•   select PARTITION_TYPE_GUID
  

•   select PARTITION_UUIDS
  

•   depends on DM && HAVE_BLOCK_DEVICE && EFI_PARTITION
  
  

It should depend on FWU_MDATA instead of DM. Though, eventually it will be a choice between GPT and MTD.

Yes, this should depend on FWU_MDATA. Will change.

...

...

...

+static int fwu_gpt_mdata_check(struct udevice *dev) +{

•   /*
  

•    * Check if both the copies of the FWU
  

•    * metadata are valid. If one has gone
  

•    * bad, restore it from the other good
  

•    * copy.
  

•    */
  
  

This comment is repeated 3 more times in the patchset. Maybe get rid of this and some other.

Sorry, I could not find any other comment like this in the file. Can you point me out where this comment has been repeated.

I said patchset, not file. grep'ing "restore it from the other" in the patches show drivers/fwu-mdata/gpt_blk.c, drivers/fwu-mdata/fwu-mdata-uclass.c and twice in include/fwu.h

Btw, the other three times it says "restore it from the other bad copy" :D

-j

Hi Sughosh

On Thu, Sep 15, 2022 at 01:44:42PM +0530, Sughosh Ganu wrote:

Add weak functions for getting the update index value and dfu alternate number needed for FWU Multi Bank update functionality.

The current implementation for getting the update index value is for platforms with 2 banks. If a platform supports more than 2 banks, it can implement it's own function. The function to get the dfu alternate number has been added for platforms with GPT partitioned storage devices. Platforms with other storage partition scheme need to implement their own function.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Reviewed-by: Patrick Delaunay patrick.delaunay@foss.st.com Acked-by: Etienne Carriere etienne.carriere@linaro.org

---

Changes since V9:

• Remove unneeded braces as suggested by Ilias

include/fwu.h | 29 ++++++++++ lib/fwu_updates/fwu.c | 27 +++++++++ lib/fwu_updates/fwu_gpt.c | 114 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 170 insertions(+) create mode 100644 lib/fwu_updates/fwu_gpt.c

diff --git a/include/fwu.h b/include/fwu.h index 1e16253f69..3ff37c628b 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -215,4 +215,33 @@ int fwu_accept_image(efi_guid_t *img_type_id, u32 bank); */ int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank);

+/**

• • fwu_plat_get_alt_num() - Get the DFU Alt Num for the image from the platform
• • @dev: FWU device
• • @image_guid: Image GUID for which DFU alt number needs to be retrieved
• • @alt_num: Pointer to the alt_num
• • Get the DFU alt number from the platform for the image specified by the
• • image GUID.
• • Return: 0 if OK, -ve on error
• */

+int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid,

• 	 u8 *alt_num);
  

+/**

• • fwu_plat_get_update_index() - Get the value of the update bank
• • @update_idx: Bank number to which images are to be updated
• • Get the value of the bank(partition) to which the update needs to be
• • made.
• • Note: This is a weak function and platforms can override this with
• • their own implementation for selection of the update bank.
• • Return: 0 if OK, -ve on error
• */

+int fwu_plat_get_update_index(uint *update_idx); #endif /* _FWU_H_ */ diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c index fb9dbca307..8e91b7aeae 100644 --- a/lib/fwu_updates/fwu.c +++ b/lib/fwu_updates/fwu.c @@ -331,3 +331,30 @@ int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank) return fwu_clrset_image_accept(img_type_id, bank, IMAGE_ACCEPT_CLEAR); }

+/**

• • fwu_plat_get_update_index() - Get the value of the update bank
• • @update_idx: Bank number to which images are to be updated
• • Get the value of the bank(partition) to which the update needs to be
• • made.
• • Note: This is a weak function and platforms can override this with
• • their own implementation for selection of the update bank.
• • Return: 0 if OK, -ve on error
• */

+__weak int fwu_plat_get_update_index(uint *update_idx) +{

• int ret;
• u32 active_idx;
• ret = fwu_get_active_index(&active_idx);
• if (ret < 0)

• return -1;
  

• *update_idx = (active_idx + 1) % CONFIG_FWU_NUM_BANKS;
• return ret;

+} diff --git a/lib/fwu_updates/fwu_gpt.c b/lib/fwu_updates/fwu_gpt.c new file mode 100644 index 0000000000..a1b0f3bea7 --- /dev/null +++ b/lib/fwu_updates/fwu_gpt.c @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/*

• • Copyright (c) 2022, Linaro Limited
• */

+#include <blk.h> +#include <dfu.h> +#include <efi.h> +#include <efi_loader.h> +#include <fwu.h> +#include <log.h> +#include <part.h>

+#include <linux/errno.h>

+static int get_gpt_dfu_identifier(struct blk_desc *desc, efi_guid_t *image_guid) +{

• int i;
• struct disk_partition info;
• efi_guid_t unique_part_guid;
• for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) {

• if (part_get_info(desc, i, &info))
  

• 	continue;
  

• uuid_str_to_bin(info.uuid, unique_part_guid.b,
  

• 		UUID_STR_FORMAT_GUID);
  

• if (!guidcmp(&unique_part_guid, image_guid))
  

• 	return i;
  

• }
• log_err("No partition found with image_guid %pUs\n", image_guid);
• return -ENOENT;

+}

+static int fwu_gpt_get_alt_num(struct blk_desc *desc, efi_guid_t *image_guid,

• 	       u8 *alt_num, unsigned char dfu_dev)
  

+{

• int ret = -1;
• int i, part, dev_num;
• int nalt;
• struct dfu_entity *dfu;
• dev_num = desc->devnum;
• part = get_gpt_dfu_identifier(desc, image_guid);
• if (part < 0)

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

• if (!dfu)
  

• 	continue;
  

• /*
  

•  * Currently, Multi Bank update
  

•  * feature is being supported
  

•  * only on GPT partitioned
  

•  * MMC/SD devices.
  

•  */
  

• if (dfu->dev_type != dfu_dev)
  

• 	continue;
  

• if (dfu->layout == DFU_RAW_ADDR &&
  

•     dfu->data.mmc.dev_num == dev_num &&
  

•     dfu->data.mmc.part == part) {
  

• 	*alt_num = dfu->alt;
  

• 	ret = 0;
  

• 	break;
  

I get that we only currently support it on mmc, but the if above is not going to scale as we add devices. Is there something better we can come up with? Probably a helper in the dfu layer?

Cheers /Ilias

• }
  

• }
• dfu_free_entities();
• return ret;

+}

+/**

• • fwu_plat_get_alt_num() - Get the DFU alt number
• • @dev: FWU metadata device
• • @image_guid: GUID value of the image for which the alt num is to

• •          be obtained
    

• • @alt_num: The DFU alt number for the image that is to be updated
• • Get the DFU alt number for the image that is to be updated. The
• • image is identified with the image_guid parameter that is passed
• • to the function.
• • Note: This is a weak function and platforms can override this with
• • their own implementation for obtaining the alt number value.
• • Return: 0 if OK, -ve on error
• */

+__weak int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid,

• 		u8 *alt_num)
  

+{

• struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
• return fwu_gpt_get_alt_num(dev_get_uclass_plat(priv->blk_dev),

• 		   image_guid, alt_num, DFU_DEV_MMC);
  

+}

2.34.1

On Thu, 15 Sept 2022 at 10:15, Sughosh Ganu sughosh.ganu@linaro.org wrote:

The FWU Multi Bank Update feature allows the platform to boot the firmware images from one of the partitions(banks). The first stage bootloader(fsbl) passes the value of the boot index, i.e. the bank from which the firmware images were booted from to U-Boot. On the STM32MP157C-DK2 board, this value is passed through one of the SoC's backup register. Add a function to read the boot index value from the backup register.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Reviewed-by: Patrick Delaunay patrick.delaunay@foss.st.com Acked-by: Ilias Apalodimas ilias.apalodimas@linaro.org

---

Changes since V9:

• Change the fwu_plat_get_bootidx() function to take an uint * as the function parameter instead of the void * as suggested by Etienne.

arch/arm/mach-stm32mp/include/mach/stm32.h | 5 +++++ board/st/stm32mp1/stm32mp1.c | 23 ++++++++++++++++++++++ include/fwu.h | 12 +++++++++++ 3 files changed, 40 insertions(+)

diff --git a/arch/arm/mach-stm32mp/include/mach/stm32.h b/arch/arm/mach-stm32mp/include/mach/stm32.h index c70375a723..c85ae6a34e 100644 --- a/arch/arm/mach-stm32mp/include/mach/stm32.h +++ b/arch/arm/mach-stm32mp/include/mach/stm32.h @@ -112,11 +112,16 @@ enum boot_device { #ifdef CONFIG_STM32MP15x #define TAMP_BACKUP_MAGIC_NUMBER TAMP_BACKUP_REGISTER(4) #define TAMP_BACKUP_BRANCH_ADDRESS TAMP_BACKUP_REGISTER(5) +#define TAMP_FWU_BOOT_INFO_REG TAMP_BACKUP_REGISTER(10) #define TAMP_COPRO_RSC_TBL_ADDRESS TAMP_BACKUP_REGISTER(17) #define TAMP_COPRO_STATE TAMP_BACKUP_REGISTER(18) #define TAMP_BOOT_CONTEXT TAMP_BACKUP_REGISTER(20) #define TAMP_BOOTCOUNT TAMP_BACKUP_REGISTER(21)

+#define TAMP_FWU_BOOT_IDX_MASK GENMASK(3, 0)

+#define TAMP_FWU_BOOT_IDX_OFFSET 0

#define TAMP_COPRO_STATE_OFF 0 #define TAMP_COPRO_STATE_INIT 1 #define TAMP_COPRO_STATE_CRUN 2 diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c index 6562bcf95b..54b9535405 100644 --- a/board/st/stm32mp1/stm32mp1.c +++ b/board/st/stm32mp1/stm32mp1.c @@ -960,3 +960,26 @@ static void board_copro_image_process(ulong fw_image, size_t fw_size) }

U_BOOT_FIT_LOADABLE_HANDLER(IH_TYPE_COPRO, board_copro_image_process);

+#if defined(CONFIG_FWU_MULTI_BANK_UPDATE)

+#include <fwu.h>

+/**

• • fwu_plat_get_bootidx() - Get the value of the boot index
• • @boot_idx: Boot index value
• • Get the value of the bank(partition) from which the platform
• • has booted. This value is passed to U-Boot from the earlier
• • stage bootloader which loads and boots all the relevant
• • firmware images
• */

+void fwu_plat_get_bootidx(uint *boot_idx) +{

•   u32 *bootidx = boot_idx;
  

•   *bootidx = (readl(TAMP_FWU_BOOT_INFO_REG) >>
  

•               TAMP_FWU_BOOT_IDX_OFFSET) & TAMP_FWU_BOOT_IDX_MASK;
  

+}

I may be nitpicking but despite this is fine in this source file build only for 32bit, I think the cast is not that clean. Would prefer:

void fwu_plat_get_bootidx(uint *boot_idx) { *boot_idx = (readl(TAMP_FWU_BOOT_INFO_REG) >> TAMP_FWU_BOOT_IDX_OFFSET) & TAMP_FWU_BOOT_IDX_MASK; }

br, etienne

+#endif /* CONFIG_FWU_MULTI_BANK_UPDATE */ diff --git a/include/fwu.h b/include/fwu.h index 3ff37c628b..484289ed4f 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -244,4 +244,16 @@ int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid,

*/ int fwu_plat_get_update_index(uint *update_idx);

+/**

• • fwu_plat_get_bootidx() - Get the value of the boot index
• • @boot_idx: Boot index value
• • Get the value of the bank(partition) from which the platform
• • has booted. This value is passed to U-Boot from the earlier
• • stage bootloader which loads and boots all the relevant
• • firmware images
• */

+void fwu_plat_get_bootidx(uint *boot_idx);

#endif /* _FWU_H_ */

2.34.1

On Thu, Sep 15, 2022 at 01:44:44PM +0530, Sughosh Ganu wrote:

Add an event type EVT_MAIN_LOOP that can be used for registering events that need to be run after the platform has been initialised and before the main_loop function is called.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Reviewed-by: Simon Glass sjg@chromium.org

---

Changes since V9:

• Add a comment to indicate where the event notification will be called, as suggested by Simon.

common/board_r.c | 3 +++ common/event.c | 3 +++ include/event.h | 3 +++ 3 files changed, 9 insertions(+)

diff --git a/common/board_r.c b/common/board_r.c index 56eb60fa27..1db061e65c 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -576,6 +576,9 @@ static int run_main_loop(void) #ifdef CONFIG_SANDBOX sandbox_main_loop_init(); #endif

• event_notify_null(EVT_MAIN_LOOP);
• /* main_loop() can return to retry autoboot, if so just run it again */ for (;;) main_loop();

diff --git a/common/event.c b/common/event.c index 3e34550978..231b9e6ffd 100644 --- a/common/event.c +++ b/common/event.c @@ -38,6 +38,9 @@ const char *const type_name[] = {

/* fdt hooks */ "ft_fixup",

• /* main loop events */
• "main_loop",

};

_Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size"); diff --git a/include/event.h b/include/event.h index e8f2f55c63..8089b464b1 100644 --- a/include/event.h +++ b/include/event.h @@ -34,6 +34,9 @@ enum event_t { /* Device tree fixups before booting */ EVT_FT_FIXUP,

• /* To be called once, before calling main_loop() */
• EVT_MAIN_LOOP,
• EVT_COUNT

};

-- 2.34.1

Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

On Thu, Sep 15, 2022 at 3:17 AM Sughosh Ganu sughosh.ganu@linaro.org wrote:

....

diff --git a/include/fwu.h b/include/fwu.h index 484289ed4f..d5f77ce83c 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -256,4 +256,17 @@ int fwu_plat_get_update_index(uint *update_idx);

*/ void fwu_plat_get_bootidx(uint *boot_idx);

Or simply return the boot_idx instead of modifying the pointed variable

.....

diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c index c633fcd91e..557e97de4a 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -199,6 +199,7 @@ static efi_status_t __efi_init_early(void) goto out;

    ret = efi_disk_init();

out: return ret; }

We can do without this change in this patchset :)

.....

+static int fwu_trial_state_check(struct udevice *dev) +{

•   int ret;
  

•   efi_status_t status;
  

•   efi_uintn_t var_size;
  

•   u16 trial_state_ctr;
  

•   u32 var_attributes;
  

•   struct fwu_mdata mdata = { 0 };
  

•   ret = fwu_get_mdata(dev, &mdata);
  

•   if (ret)
  

•           return ret;
  

•   if ((trial_state = in_trial_state(&mdata))) {
  
  

This may raise warnings on code checkers. So maybe move the assignment out of the check.

.....

+static int fwu_boottime_checks(void *ctx, struct event *event) +{

•   int ret;
  

•   struct udevice *dev;
  

•   u32 boot_idx, active_idx;
  

•   ret = fwu_get_dev_mdata(&dev, NULL);
  

•   if (ret)
  

•           return ret;
  

•   ret = fwu_mdata_check(dev);
  

•   if (ret) {
  

•           return 0;
  

•   }
  

•   /*
  

•    * Get the Boot Index, i.e. the bank from
  

•    * which the platform has booted. This value
  

•    * gets passed from the ealier stage bootloader
  

•    * which booted u-boot, e.g. tf-a. If the
  

•    * boot index is not the same as the
  

•    * active_index read from the FWU metadata,
  

•    * update the active_index.
  

•    */
  

•   fwu_plat_get_bootidx(&boot_idx);
  

•   if (boot_idx >= CONFIG_FWU_NUM_BANKS) {
  

•           log_err("Received incorrect value of boot_index\n");
  

•           return 0;
  

•   }
  

•   ret = fwu_get_active_index(&active_idx);
  

•   if (ret) {
  

•           log_err("Unable to read active_index\n");
  

•           return 0;
  

•   }
  

•   if (boot_idx != active_idx) {
  

•           log_info("Boot idx %u is not matching active idx %u, changing active_idx\n",
  

•                    boot_idx, active_idx);
  

•           ret = fwu_update_active_index(boot_idx);
  

•           if (!ret)
  

•                   boottime_check = 1;
  
  

We may not want to do anything FWU (accept, reject, modify mdata) until we reboot, if we are recovering from last bad upgrade. So maybe not set boottime_check

cheers