[v5,02/23] FWU: Add FWU metadata structure and driver for accessing metadata

In the FWU Multi Bank Update feature, the information about the
updatable images is stored as part of the metadata, which is stored on
a dedicated partition. Add the metadata structure, and a driver model
uclass which provides functions to access the metadata. These are
generic API's, and implementations can be added based on parameters
like how the metadata partition is accessed and what type of storage
device houses the metadata.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 drivers/Kconfig                      |   2 +
 drivers/Makefile                     |   1 +
 drivers/fwu-mdata/Kconfig            |   7 +
 drivers/fwu-mdata/Makefile           |   6 +
 drivers/fwu-mdata/fwu-mdata-uclass.c | 459 +++++++++++++++++++++++++++
 include/dm/uclass-id.h               |   1 +
 include/fwu.h                        |  49 +++
 include/fwu_mdata.h                  |  67 ++++
 8 files changed, 592 insertions(+)
 create mode 100644 drivers/fwu-mdata/Kconfig
 create mode 100644 drivers/fwu-mdata/Makefile
 create mode 100644 drivers/fwu-mdata/fwu-mdata-uclass.c
 create mode 100644 include/fwu.h
 create mode 100644 include/fwu_mdata.h

Hello Sughosh,



On Thu, 9 Jun 2022 at 14:30, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> In the FWU Multi Bank Update feature, the information about the
> updatable images is stored as part of the metadata, which is stored on
> a dedicated partition. Add the metadata structure, and a driver model
> uclass which provides functions to access the metadata. These are
> generic API's, and implementations can be added based on parameters
> like how the metadata partition is accessed and what type of storage
> device houses the metadata.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  drivers/Kconfig                      |   2 +
>  drivers/Makefile                     |   1 +
>  drivers/fwu-mdata/Kconfig            |   7 +
>  drivers/fwu-mdata/Makefile           |   6 +
>  drivers/fwu-mdata/fwu-mdata-uclass.c | 459 +++++++++++++++++++++++++++
>  include/dm/uclass-id.h               |   1 +
>  include/fwu.h                        |  49 +++
>  include/fwu_mdata.h                  |  67 ++++
>  8 files changed, 592 insertions(+)
>  create mode 100644 drivers/fwu-mdata/Kconfig
>  create mode 100644 drivers/fwu-mdata/Makefile
>  create mode 100644 drivers/fwu-mdata/fwu-mdata-uclass.c
>  create mode 100644 include/fwu.h
>  create mode 100644 include/fwu_mdata.h
>
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index b26ca8cf70..adc6079ecf 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -42,6 +42,8 @@ source "drivers/firmware/Kconfig"
>
>  source "drivers/fpga/Kconfig"
>
> +source "drivers/fwu-mdata/Kconfig"
> +
>  source "drivers/gpio/Kconfig"
>
>  source "drivers/hwspinlock/Kconfig"
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 67c8af7442..901150bb35 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -83,6 +83,7 @@ obj-y += cache/
>  obj-$(CONFIG_CPU) += cpu/
>  obj-y += crypto/
>  obj-$(CONFIG_FASTBOOT) += fastboot/
> +obj-$(CONFIG_DM_FWU_MDATA) += fwu-mdata/
>  obj-y += misc/
>  obj-$(CONFIG_MMC) += mmc/
>  obj-$(CONFIG_NVME) += nvme/
> diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig
> new file mode 100644
> index 0000000000..d6a21c8e19
> --- /dev/null
> +++ b/drivers/fwu-mdata/Kconfig
> @@ -0,0 +1,7 @@
> +config DM_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.
> diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile
> new file mode 100644
> index 0000000000..7fec7171f4
> --- /dev/null
> +++ b/drivers/fwu-mdata/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +#
> +# Copyright (c) 2022, Linaro Limited
> +#
> +
> +obj-$(CONFIG_DM_FWU_MDATA) += fwu-mdata-uclass.o
> diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c
> new file mode 100644
> index 0000000000..1530ceb01d
> --- /dev/null
> +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
> @@ -0,0 +1,459 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2022, Linaro Limited
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <efi_loader.h>
> +#include <fwu.h>
> +#include <fwu_mdata.h>
> +#include <log.h>
> +#include <malloc.h>
> +
> +#include <linux/errno.h>
> +#include <linux/types.h>
> +#include <u-boot/crc.h>
> +
> +#define IMAGE_ACCEPT_SET       BIT(0)
> +#define IMAGE_ACCEPT_CLEAR     BIT(1)
> +
> +static int fwu_get_dev_ops(struct udevice **dev,
> +                          const struct fwu_mdata_ops **ops)
> +{
> +       int ret;
> +
> +       ret = uclass_get_device(UCLASS_FWU_MDATA, 0, dev);
> +       if (ret) {
> +               log_debug("Cannot find fwu device\n");
> +               return ret;
> +       }
> +
> +       if ((*ops = device_get_ops(*dev)) == NULL) {
> +               log_debug("Cannot get fwu device ops\n");
> +               return -ENOSYS;
> +       }
> +
> +       return 0;
> +}
> +
> +/**
> + * 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_err("crc32 check failed for %s FWU metadata partition\n",
> +                       pri_part ? "primary" : "secondary");
> +               return -1;
> +       }
> +
> +       return 0;
> +}
> +
> +/**
> + * fwu_get_active_index() - Get active_index from the FWU metadata
> + * @active_idx: active_index value to be read
> + *
> + * Read the active_index field from the FWU metadata and place it in
> + * the variable pointed to be the function argument.
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_get_active_index(u32 *active_idx)
> +{
> +       int ret;
> +       struct fwu_mdata *mdata = NULL;
> +
> +       ret = fwu_get_mdata(&mdata);
> +       if (ret < 0) {
> +               log_err("Unable to get valid FWU metadata\n");
> +               goto out;
> +       }
> +
> +       /*
> +        * Found the FWU metadata partition, now read the active_index
> +        * value
> +        */
> +       *active_idx = mdata->active_index;
> +       if (*active_idx > CONFIG_FWU_NUM_BANKS - 1) {
> +               log_err("Active index value read is incorrect\n");
> +               ret = -EINVAL;
> +       }
> +
> +out:
> +       free(mdata);
> +
> +       return ret;
> +}
> +
> +/**
> + * fwu_update_active_index() - Update active_index from the FWU metadata
> + * @active_idx: active_index value to be updated
> + *
> + * Update the active_index field in the FWU metadata
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_update_active_index(u32 active_idx)
> +{
> +       int ret;
> +       struct fwu_mdata *mdata = NULL;
> +
> +       if (active_idx > CONFIG_FWU_NUM_BANKS - 1) {
> +               log_err("Active index value to be updated is incorrect\n");
> +               return -1;
> +       }
> +
> +       ret = fwu_get_mdata(&mdata);
> +       if (ret < 0) {
> +               log_err("Unable to get valid FWU metadata\n");
> +               goto out;
> +       }
> +
> +       /*
> +        * 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;
> +
> +       /*
> +        * Now write this updated FWU metadata to both the
> +        * FWU metadata partitions
> +        */
> +       ret = fwu_update_mdata(mdata);
> +       if (ret < 0) {> +               log_err("Failed to update FWU metadata partitions\n");
> +               ret = -EIO;
> +       }
> +
> +out:
> +       free(mdata);
> +
> +       return ret;
> +}
> +
> +/**
> + * fwu_get_image_alt_num() - Get the dfu alt number to be used for capsule update
> + * @image_type_id: pointer to the image guid as passed in the capsule
> + * @update_bank: Bank to which the update is to be made
> + * @alt_num: The alt_num for the image
> + *
> + * Based on the guid value passed in the capsule, along with the bank to which the
> + * image needs to be updated, get the dfu alt number which will be used for the
> + * capsule update
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_get_image_alt_num(efi_guid_t *image_type_id, u32 update_bank,
> +                         int *alt_num)
> +{
> +       int ret, i;
> +       efi_guid_t *image_guid;
> +       struct udevice *dev = NULL;
> +       struct fwu_mdata *mdata = NULL;
> +       struct fwu_image_entry *img_entry;
> +       const struct fwu_mdata_ops *ops = NULL;
> +       struct fwu_image_bank_info *img_bank_info;
> +
> +       ret = fwu_get_dev_ops(&dev, &ops);
> +       if (ret)
> +               return ret;
> +
> +       ret = fwu_get_mdata(&mdata);
> +       if (ret) {
> +               log_err("Unable to get valid FWU metadata\n");
> +               goto out;
> +       }
> +
> +       /*
> +        * The FWU metadata has been read. Now get the image_uuid for the
> +        * image with the update_bank.
> +        */
> +       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];
> +                       img_bank_info = &img_entry->img_bank_info[update_bank];
> +                       image_guid = &img_bank_info->image_uuid;
> +                       ret = fwu_plat_get_alt_num(dev_get_priv(dev),
> +                                                  image_guid, alt_num);
> +                       break;
> +               }
> +       }
> +
> +       if (i == CONFIG_FWU_NUM_IMAGES_PER_BANK) {
> +               log_err("Partition with the image type %pUs not found\n",
> +                       image_type_id);
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       if (!ret) {
> +               log_debug("alt_num %d for partition %pUs\n",
> +                         *alt_num, &image_guid);

s/&image_guid/image_guid/
Ditto in trace below.

> +       } else {
> +               log_err("alt_num not found for partition with GUID %pUs\n",
> +                       &image_guid);
> +               ret = -EINVAL;
> +       }
> +
> +out:
> +       free(mdata);
> +
> +       return ret;
> +}
> +
> +/**
> + * fwu_mdata_check() - Check if the FWU metadata is valid
> + *
> + * Validate both copies of the FWU metadata. If one of the copies
> + * has gone bad, restore it from the other bad copy.
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_mdata_check(void)
> +{
> +       int ret;
> +       struct udevice *dev = NULL;
> +       const struct fwu_mdata_ops *ops = NULL;
> +
> +       ret = fwu_get_dev_ops(&dev, &ops);
> +       if (ret)
> +               return ret;
> +
> +       if (!ops->mdata_check) {
> +               log_err("mdata_check() method not defined\n");
> +               return -ENOSYS;
> +       }
> +
> +       return ops->mdata_check(dev);
> +}
> +
> +/**
> + * fwu_revert_boot_index() - Revert the active index in the FWU metadata
> + *
> + * Revert the active_index value in the FWU metadata, by swapping the values
> + * of active_index and previous_active_index in both copies of the
> + * FWU metadata.
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_revert_boot_index(void)
> +{
> +       int ret;
> +       u32 cur_active_index;
> +       struct fwu_mdata *mdata = NULL;
> +
> +       ret = fwu_get_mdata(&mdata);
> +       if (ret < 0) {
> +               log_err("Unable to get valid FWU metadata\n");
> +               goto out;
> +       }
> +
> +       /*
> +        * 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;
> +
> +       /*
> +        * Now write this updated FWU metadata to both the
> +        * FWU metadata partitions
> +        */
> +       ret = fwu_update_mdata(mdata);
> +       if (ret < 0) {
> +               log_err("Failed to update FWU metadata partitions\n");
> +               ret = -EIO;
> +       }
> +
> +out:
> +       free(mdata);
> +
> +       return ret;
> +}
> +
> +/**
> + * fwu_set_clear_image_accept() - Set or Clear the Acceptance bit for the image
> + * @img_type_id: Guid of the image type for which the accepted bit is to be
> + *               set or cleared
> + * @bank: Bank of which the image's Accept bit is to be set or cleared
> + * @action: Action which specifies whether image's Accept bit is to be set or
> + *          cleared
> + *
> + * Set/Clear the accepted bit for the image specified by the img_guid parameter.
> + * This indicates acceptance or rejection of image for subsequent boots by some
> + * governing component like OS(or firmware).
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +static int fwu_set_clear_image_accept(efi_guid_t *img_type_id,
> +                                     u32 bank, u8 action)
> +{
> +       int ret, i;
> +       u32 nimages;
> +       struct fwu_mdata *mdata = NULL;
> +       struct fwu_image_entry *img_entry;
> +       struct fwu_image_bank_info *img_bank_info;
> +
> +       ret = fwu_get_mdata(&mdata);
> +       if (ret < 0) {
> +               log_err("Unable to get valid FWU metadata\n");
> +               goto out;
> +       }
> +
> +       nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK;
> +       img_entry = &mdata->img_entry[0];
> +       for (i = 0; i < nimages; i++) {
> +               if (!guidcmp(&img_entry[i].image_type_uuid, img_type_id)) {
> +                       img_bank_info = &img_entry[i].img_bank_info[bank];
> +                       if (action == IMAGE_ACCEPT_SET)
> +                               img_bank_info->accepted |= FWU_IMAGE_ACCEPTED;
> +                       else
> +                               img_bank_info->accepted = 0;
> +
> +                       ret = fwu_update_mdata(mdata);
> +                       goto out;
> +               }
> +       }
> +
> +       /* Image not found */
> +       ret = -EINVAL;
> +
> +out:
> +       free(mdata);
> +
> +       return ret;
> +}
> +
> +/**
> + * 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
> + *               cleared
> + * @bank: Bank of which the image's Accept bit is to be set
> + *
> + * Set the accepted bit for the image specified by the img_guid parameter. This
> + * indicates acceptance of image for subsequent boots by some governing component
> + * like OS(or firmware).
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_accept_image(efi_guid_t *img_type_id, u32 bank)
> +{
> +       return fwu_set_clear_image_accept(img_type_id, bank,
> +                                         IMAGE_ACCEPT_SET);
> +}
> +
> +/**
> + * fwu_clear_accept_image() - Clear the Acceptance bit for the image
> + * @img_type_id: Guid of the image type for which the accepted bit is to be
> + *               cleared
> + * @bank: Bank of which the image's Accept bit is to be cleared
> + *
> + * Clear the accepted bit for the image type specified by the img_type_id parameter.
> + * This function is called after the image has been updated. The accepted bit is
> + * cleared to be set subsequently after passing the image acceptance criteria, by
> + * either the OS(or firmware)
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank)
> +{
> +       return fwu_set_clear_image_accept(img_type_id, bank,
> +                                         IMAGE_ACCEPT_CLEAR);
> +}
> +
> +/**
> + * fwu_get_mdata() - Get a FWU metadata copy
> + * @mdata: Copy of the FWU metadata
> + *
> + * Get a valid copy of the FWU metadata.
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_get_mdata(struct fwu_mdata **mdata)

Is there a real need for this function to allocate an instance of struct mdata.
I think it would be clearer if it was the caller's responsibility to
allocate/free the structure.

Or maybe rename this function fwu_alloc_and_copy_mdata() to highlight
that the function gives an allocated copy of the data.
One should be careful when calling these API functions as some act on
a local copy (retrieved from fw_get_mdata()) while other functions
modify straight fwu-mdata in the storage media.

Br,
etienne



> +{
> +       int ret;
> +       struct udevice *dev = NULL;
> +       const struct fwu_mdata_ops *ops = NULL;
> +
> +       ret = fwu_get_dev_ops(&dev, &ops);
> +       if (ret)
> +               return ret;
> +
> +       if (!ops->get_mdata) {
> +               log_err("get_mdata() method not defined\n");
> +               return -ENOSYS;
> +       }
> +
> +       return ops->get_mdata(dev, mdata);
> +}
> +
> +/**
> + * fwu_update_mdata() - Update the FWU metadata
> + * @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 fwu_update_mdata(struct fwu_mdata *mdata)
> +{
> +       int ret;
> +       void *buf;
> +       struct udevice *dev = NULL;
> +       const struct fwu_mdata_ops *ops = NULL;
> +
> +       ret = fwu_get_dev_ops(&dev, &ops);
> +       if (ret)
> +               return ret;
> +
> +       if (!ops->update_mdata) {
> +               log_err("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/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index 3ba69ad9a0..7da719c048 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -57,6 +57,7 @@ enum uclass_id {
>         UCLASS_ETH_PHY,         /* Ethernet PHY device */
>         UCLASS_FIRMWARE,        /* Firmware */
>         UCLASS_FS_FIRMWARE_LOADER,              /* Generic loader */
> +       UCLASS_FWU_MDATA,       /* FWU Metadata Access */
>         UCLASS_GPIO,            /* Bank of general-purpose I/O pins */
>         UCLASS_HASH,            /* Hash device */
>         UCLASS_HWSPINLOCK,      /* Hardware semaphores */
> diff --git a/include/fwu.h b/include/fwu.h
> new file mode 100644
> index 0000000000..f9e44e7b39
> --- /dev/null
> +++ b/include/fwu.h
> @@ -0,0 +1,49 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (c) 2022, Linaro Limited
> + */
> +
> +#if !defined _FWU_H_
> +#define _FWU_H_
> +
> +#include <blk.h>
> +#include <efi.h>
> +
> +#include <linux/types.h>
> +
> +struct fwu_mdata;
> +struct udevice;
> +
> +/**
> + * @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 {
> +       int (*mdata_check)(struct udevice *dev);
> +
> +       int (*get_mdata)(struct udevice *dev, struct fwu_mdata **mdata);
> +
> +       int (*update_mdata)(struct udevice *dev, struct fwu_mdata *mdata);
> +};
> +
> +#define FWU_MDATA_VERSION      0x1
> +
> +#define FWU_MDATA_GUID \
> +       EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \
> +                0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23)
> +
> +int fwu_get_mdata(struct fwu_mdata **mdata);
> +int fwu_update_mdata(struct fwu_mdata *mdata);
> +int fwu_get_active_index(u32 *active_idx);
> +int fwu_update_active_index(u32 active_idx);
> +int fwu_get_image_alt_num(efi_guid_t *image_type_id, u32 update_bank,
> +                         int *alt_num);
> +int fwu_mdata_check(void);
> +int fwu_revert_boot_index(void);
> +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);
> +
> +int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid,
> +                        int *alt_num);
> +#endif /* _FWU_H_ */
> diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h
> new file mode 100644
> index 0000000000..701efbba03
> --- /dev/null
> +++ b/include/fwu_mdata.h
> @@ -0,0 +1,67 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (c) 2022, Linaro Limited
> + */
> +
> +#if !defined _FWU_MDATA_H_
> +#define _FWU_MDATA_H_
> +
> +#include <efi.h>
> +
> +/**
> + * struct fwu_image_bank_info - firmware image information
> + * @image_uuid: Guid value of the image in this bank
> + * @accepted: Acceptance status of the image
> + * @reserved: Reserved
> + *
> + * The structure contains image specific fields which are
> + * used to identify the image and to specify the image's
> + * acceptance status
> + */
> +struct fwu_image_bank_info {
> +       efi_guid_t  image_uuid;
> +       uint32_t accepted;
> +       uint32_t reserved;
> +} __attribute__((__packed__));
> +
> +/**
> + * struct fwu_image_entry - information for a particular type of image
> + * @image_type_uuid: Guid value for identifying the image type
> + * @location_uuid: Guid of the storage volume where the image is located
> + * @img_bank_info: Array containing properties of images
> + *
> + * This structure contains information on various types of updatable
> + * firmware images. Each image type then contains an array of image
> + * information per bank.
> + */
> +struct fwu_image_entry {
> +       efi_guid_t image_type_uuid;
> +       efi_guid_t location_uuid;
> +       struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS];
> +} __attribute__((__packed__));
> +
> +/**
> + * struct fwu_mdata - FWU metadata structure for multi-bank updates
> + * @crc32: crc32 value for the FWU metadata
> + * @version: FWU metadata version
> + * @active_index: Index of the bank currently used for booting images
> + * @previous_active_inde: Index of the bank used before the current bank
> + *                        being used for booting
> + * @img_entry: Array of information on various firmware images that can
> + *             be updated
> + *
> + * This structure is used to store all the needed information for performing
> + * multi bank updates on the platform. This contains info on the bank being
> + * used to boot along with the information needed for identification of
> + * individual images
> + */
> +struct fwu_mdata {
> +       uint32_t crc32;
> +       uint32_t version;
> +       uint32_t active_index;
> +       uint32_t previous_active_index;
> +
> +       struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK];
> +} __attribute__((__packed__));
> +
> +#endif /* _FWU_MDATA_H_ */
> --
> 2.25.1
>

hi Etienne,

On Tue, 21 Jun 2022 at 16:24, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Hello Sughosh,
>
>
>
> On Thu, 9 Jun 2022 at 14:30, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > In the FWU Multi Bank Update feature, the information about the
> > updatable images is stored as part of the metadata, which is stored on
> > a dedicated partition. Add the metadata structure, and a driver model
> > uclass which provides functions to access the metadata. These are
> > generic API's, and implementations can be added based on parameters
> > like how the metadata partition is accessed and what type of storage
> > device houses the metadata.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >  drivers/Kconfig                      |   2 +
> >  drivers/Makefile                     |   1 +
> >  drivers/fwu-mdata/Kconfig            |   7 +
> >  drivers/fwu-mdata/Makefile           |   6 +
> >  drivers/fwu-mdata/fwu-mdata-uclass.c | 459 +++++++++++++++++++++++++++
> >  include/dm/uclass-id.h               |   1 +
> >  include/fwu.h                        |  49 +++
> >  include/fwu_mdata.h                  |  67 ++++
> >  8 files changed, 592 insertions(+)
> >  create mode 100644 drivers/fwu-mdata/Kconfig
> >  create mode 100644 drivers/fwu-mdata/Makefile
> >  create mode 100644 drivers/fwu-mdata/fwu-mdata-uclass.c
> >  create mode 100644 include/fwu.h
> >  create mode 100644 include/fwu_mdata.h
> >

<snip>

> > diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c
> > new file mode 100644
> > index 0000000000..1530ceb01d
> > --- /dev/null
> > +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
> > @@ -0,0 +1,459 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2022, Linaro Limited
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <efi_loader.h>
> > +#include <fwu.h>
> > +#include <fwu_mdata.h>
> > +#include <log.h>
> > +#include <malloc.h>
> > +
> > +#include <linux/errno.h>
> > +#include <linux/types.h>
> > +#include <u-boot/crc.h>
> > +
> > +#define IMAGE_ACCEPT_SET       BIT(0)
> > +#define IMAGE_ACCEPT_CLEAR     BIT(1)
> > +
> > +static int fwu_get_dev_ops(struct udevice **dev,
> > +                          const struct fwu_mdata_ops **ops)
> > +{
> > +       int ret;
> > +
> > +       ret = uclass_get_device(UCLASS_FWU_MDATA, 0, dev);
> > +       if (ret) {
> > +               log_debug("Cannot find fwu device\n");
> > +               return ret;
> > +       }
> > +
> > +       if ((*ops = device_get_ops(*dev)) == NULL) {
> > +               log_debug("Cannot get fwu device ops\n");
> > +               return -ENOSYS;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * 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_err("crc32 check failed for %s FWU metadata partition\n",
> > +                       pri_part ? "primary" : "secondary");
> > +               return -1;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * fwu_get_active_index() - Get active_index from the FWU metadata
> > + * @active_idx: active_index value to be read
> > + *
> > + * Read the active_index field from the FWU metadata and place it in
> > + * the variable pointed to be the function argument.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_get_active_index(u32 *active_idx)
> > +{
> > +       int ret;
> > +       struct fwu_mdata *mdata = NULL;
> > +
> > +       ret = fwu_get_mdata(&mdata);
> > +       if (ret < 0) {
> > +               log_err("Unable to get valid FWU metadata\n");
> > +               goto out;
> > +       }
> > +
> > +       /*
> > +        * Found the FWU metadata partition, now read the active_index
> > +        * value
> > +        */
> > +       *active_idx = mdata->active_index;
> > +       if (*active_idx > CONFIG_FWU_NUM_BANKS - 1) {
> > +               log_err("Active index value read is incorrect\n");
> > +               ret = -EINVAL;
> > +       }
> > +
> > +out:
> > +       free(mdata);
> > +
> > +       return ret;
> > +}
> > +
> > +/**
> > + * fwu_update_active_index() - Update active_index from the FWU metadata
> > + * @active_idx: active_index value to be updated
> > + *
> > + * Update the active_index field in the FWU metadata
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_update_active_index(u32 active_idx)
> > +{
> > +       int ret;
> > +       struct fwu_mdata *mdata = NULL;
> > +
> > +       if (active_idx > CONFIG_FWU_NUM_BANKS - 1) {
> > +               log_err("Active index value to be updated is incorrect\n");
> > +               return -1;
> > +       }
> > +
> > +       ret = fwu_get_mdata(&mdata);
> > +       if (ret < 0) {
> > +               log_err("Unable to get valid FWU metadata\n");
> > +               goto out;
> > +       }
> > +
> > +       /*
> > +        * 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;
> > +
> > +       /*
> > +        * Now write this updated FWU metadata to both the
> > +        * FWU metadata partitions
> > +        */
> > +       ret = fwu_update_mdata(mdata);
> > +       if (ret < 0) {> +               log_err("Failed to update FWU metadata partitions\n");
> > +               ret = -EIO;
> > +       }
> > +
> > +out:
> > +       free(mdata);
> > +
> > +       return ret;
> > +}
> > +
> > +/**
> > + * fwu_get_image_alt_num() - Get the dfu alt number to be used for capsule update
> > + * @image_type_id: pointer to the image guid as passed in the capsule
> > + * @update_bank: Bank to which the update is to be made
> > + * @alt_num: The alt_num for the image
> > + *
> > + * Based on the guid value passed in the capsule, along with the bank to which the
> > + * image needs to be updated, get the dfu alt number which will be used for the
> > + * capsule update
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_get_image_alt_num(efi_guid_t *image_type_id, u32 update_bank,
> > +                         int *alt_num)
> > +{
> > +       int ret, i;
> > +       efi_guid_t *image_guid;
> > +       struct udevice *dev = NULL;
> > +       struct fwu_mdata *mdata = NULL;
> > +       struct fwu_image_entry *img_entry;
> > +       const struct fwu_mdata_ops *ops = NULL;
> > +       struct fwu_image_bank_info *img_bank_info;
> > +
> > +       ret = fwu_get_dev_ops(&dev, &ops);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = fwu_get_mdata(&mdata);
> > +       if (ret) {
> > +               log_err("Unable to get valid FWU metadata\n");
> > +               goto out;
> > +       }
> > +
> > +       /*
> > +        * The FWU metadata has been read. Now get the image_uuid for the
> > +        * image with the update_bank.
> > +        */
> > +       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];
> > +                       img_bank_info = &img_entry->img_bank_info[update_bank];
> > +                       image_guid = &img_bank_info->image_uuid;
> > +                       ret = fwu_plat_get_alt_num(dev_get_priv(dev),
> > +                                                  image_guid, alt_num);
> > +                       break;
> > +               }
> > +       }
> > +
> > +       if (i == CONFIG_FWU_NUM_IMAGES_PER_BANK) {
> > +               log_err("Partition with the image type %pUs not found\n",
> > +                       image_type_id);
> > +               ret = -EINVAL;
> > +               goto out;
> > +       }
> > +
> > +       if (!ret) {
> > +               log_debug("alt_num %d for partition %pUs\n",
> > +                         *alt_num, &image_guid);
>
> s/&image_guid/image_guid/
> Ditto in trace below.

Will fix both.

>
> > +       } else {
> > +               log_err("alt_num not found for partition with GUID %pUs\n",
> > +                       &image_guid);
> > +               ret = -EINVAL;
> > +       }
> > +
> > +out:
> > +       free(mdata);
> > +
> > +       return ret;
> > +}
> > +
> > +/**
> > + * fwu_mdata_check() - Check if the FWU metadata is valid
> > + *
> > + * Validate both copies of the FWU metadata. If one of the copies
> > + * has gone bad, restore it from the other bad copy.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_mdata_check(void)
> > +{
> > +       int ret;
> > +       struct udevice *dev = NULL;
> > +       const struct fwu_mdata_ops *ops = NULL;
> > +
> > +       ret = fwu_get_dev_ops(&dev, &ops);
> > +       if (ret)
> > +               return ret;
> > +
> > +       if (!ops->mdata_check) {
> > +               log_err("mdata_check() method not defined\n");
> > +               return -ENOSYS;
> > +       }
> > +
> > +       return ops->mdata_check(dev);
> > +}
> > +
> > +/**
> > + * fwu_revert_boot_index() - Revert the active index in the FWU metadata
> > + *
> > + * Revert the active_index value in the FWU metadata, by swapping the values
> > + * of active_index and previous_active_index in both copies of the
> > + * FWU metadata.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_revert_boot_index(void)
> > +{
> > +       int ret;
> > +       u32 cur_active_index;
> > +       struct fwu_mdata *mdata = NULL;
> > +
> > +       ret = fwu_get_mdata(&mdata);
> > +       if (ret < 0) {
> > +               log_err("Unable to get valid FWU metadata\n");
> > +               goto out;
> > +       }
> > +
> > +       /*
> > +        * 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;
> > +
> > +       /*
> > +        * Now write this updated FWU metadata to both the
> > +        * FWU metadata partitions
> > +        */
> > +       ret = fwu_update_mdata(mdata);
> > +       if (ret < 0) {
> > +               log_err("Failed to update FWU metadata partitions\n");
> > +               ret = -EIO;
> > +       }
> > +
> > +out:
> > +       free(mdata);
> > +
> > +       return ret;
> > +}
> > +
> > +/**
> > + * fwu_set_clear_image_accept() - Set or Clear the Acceptance bit for the image
> > + * @img_type_id: Guid of the image type for which the accepted bit is to be
> > + *               set or cleared
> > + * @bank: Bank of which the image's Accept bit is to be set or cleared
> > + * @action: Action which specifies whether image's Accept bit is to be set or
> > + *          cleared
> > + *
> > + * Set/Clear the accepted bit for the image specified by the img_guid parameter.
> > + * This indicates acceptance or rejection of image for subsequent boots by some
> > + * governing component like OS(or firmware).
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +static int fwu_set_clear_image_accept(efi_guid_t *img_type_id,
> > +                                     u32 bank, u8 action)
> > +{
> > +       int ret, i;
> > +       u32 nimages;
> > +       struct fwu_mdata *mdata = NULL;
> > +       struct fwu_image_entry *img_entry;
> > +       struct fwu_image_bank_info *img_bank_info;
> > +
> > +       ret = fwu_get_mdata(&mdata);
> > +       if (ret < 0) {
> > +               log_err("Unable to get valid FWU metadata\n");
> > +               goto out;
> > +       }
> > +
> > +       nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK;
> > +       img_entry = &mdata->img_entry[0];
> > +       for (i = 0; i < nimages; i++) {
> > +               if (!guidcmp(&img_entry[i].image_type_uuid, img_type_id)) {
> > +                       img_bank_info = &img_entry[i].img_bank_info[bank];
> > +                       if (action == IMAGE_ACCEPT_SET)
> > +                               img_bank_info->accepted |= FWU_IMAGE_ACCEPTED;
> > +                       else
> > +                               img_bank_info->accepted = 0;
> > +
> > +                       ret = fwu_update_mdata(mdata);
> > +                       goto out;
> > +               }
> > +       }
> > +
> > +       /* Image not found */
> > +       ret = -EINVAL;
> > +
> > +out:
> > +       free(mdata);
> > +
> > +       return ret;
> > +}
> > +
> > +/**
> > + * 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
> > + *               cleared
> > + * @bank: Bank of which the image's Accept bit is to be set
> > + *
> > + * Set the accepted bit for the image specified by the img_guid parameter. This
> > + * indicates acceptance of image for subsequent boots by some governing component
> > + * like OS(or firmware).
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_accept_image(efi_guid_t *img_type_id, u32 bank)
> > +{
> > +       return fwu_set_clear_image_accept(img_type_id, bank,
> > +                                         IMAGE_ACCEPT_SET);
> > +}
> > +
> > +/**
> > + * fwu_clear_accept_image() - Clear the Acceptance bit for the image
> > + * @img_type_id: Guid of the image type for which the accepted bit is to be
> > + *               cleared
> > + * @bank: Bank of which the image's Accept bit is to be cleared
> > + *
> > + * Clear the accepted bit for the image type specified by the img_type_id parameter.
> > + * This function is called after the image has been updated. The accepted bit is
> > + * cleared to be set subsequently after passing the image acceptance criteria, by
> > + * either the OS(or firmware)
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank)
> > +{
> > +       return fwu_set_clear_image_accept(img_type_id, bank,
> > +                                         IMAGE_ACCEPT_CLEAR);
> > +}
> > +
> > +/**
> > + * fwu_get_mdata() - Get a FWU metadata copy
> > + * @mdata: Copy of the FWU metadata
> > + *
> > + * Get a valid copy of the FWU metadata.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_get_mdata(struct fwu_mdata **mdata)
>
> Is there a real need for this function to allocate an instance of struct mdata.
> I think it would be clearer if it was the caller's responsibility to
> allocate/free the structure.
>
> Or maybe rename this function fwu_alloc_and_copy_mdata() to highlight
> that the function gives an allocated copy of the data.

I guess I can put a comment in the function description saying that
the function is responsible for the allocation of the metadata
structure.

> One should be careful when calling these API functions as some act on
> a local copy (retrieved from fw_get_mdata()) while other functions
> modify straight fwu-mdata in the storage media.

Did you find any function which is modifying the metadata on the
storage device directly. The API fwu_update_mdata() is supposed to be
doing that. If you have come across any function which is directly
modifying the metadata on the storage media, please let me know and I
will fix it.

-sughosh

>
> Br,
> etienne
>
>
>
> > +{
> > +       int ret;
> > +       struct udevice *dev = NULL;
> > +       const struct fwu_mdata_ops *ops = NULL;
> > +
> > +       ret = fwu_get_dev_ops(&dev, &ops);
> > +       if (ret)
> > +               return ret;
> > +
> > +       if (!ops->get_mdata) {
> > +               log_err("get_mdata() method not defined\n");
> > +               return -ENOSYS;
> > +       }
> > +
> > +       return ops->get_mdata(dev, mdata);
> > +}
> > +
> > +/**
> > + * fwu_update_mdata() - Update the FWU metadata
> > + * @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 fwu_update_mdata(struct fwu_mdata *mdata)
> > +{
> > +       int ret;
> > +       void *buf;
> > +       struct udevice *dev = NULL;
> > +       const struct fwu_mdata_ops *ops = NULL;
> > +
> > +       ret = fwu_get_dev_ops(&dev, &ops);
> > +       if (ret)
> > +               return ret;
> > +
> > +       if (!ops->update_mdata) {
> > +               log_err("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/include/dm/uclass-id.h b/include/dm/uclass-id.h
> > index 3ba69ad9a0..7da719c048 100644
> > --- a/include/dm/uclass-id.h
> > +++ b/include/dm/uclass-id.h
> > @@ -57,6 +57,7 @@ enum uclass_id {
> >         UCLASS_ETH_PHY,         /* Ethernet PHY device */
> >         UCLASS_FIRMWARE,        /* Firmware */
> >         UCLASS_FS_FIRMWARE_LOADER,              /* Generic loader */
> > +       UCLASS_FWU_MDATA,       /* FWU Metadata Access */
> >         UCLASS_GPIO,            /* Bank of general-purpose I/O pins */
> >         UCLASS_HASH,            /* Hash device */
> >         UCLASS_HWSPINLOCK,      /* Hardware semaphores */
> > diff --git a/include/fwu.h b/include/fwu.h
> > new file mode 100644
> > index 0000000000..f9e44e7b39
> > --- /dev/null
> > +++ b/include/fwu.h
> > @@ -0,0 +1,49 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (c) 2022, Linaro Limited
> > + */
> > +
> > +#if !defined _FWU_H_
> > +#define _FWU_H_
> > +
> > +#include <blk.h>
> > +#include <efi.h>
> > +
> > +#include <linux/types.h>
> > +
> > +struct fwu_mdata;
> > +struct udevice;
> > +
> > +/**
> > + * @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 {
> > +       int (*mdata_check)(struct udevice *dev);
> > +
> > +       int (*get_mdata)(struct udevice *dev, struct fwu_mdata **mdata);
> > +
> > +       int (*update_mdata)(struct udevice *dev, struct fwu_mdata *mdata);
> > +};
> > +
> > +#define FWU_MDATA_VERSION      0x1
> > +
> > +#define FWU_MDATA_GUID \
> > +       EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \
> > +                0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23)
> > +
> > +int fwu_get_mdata(struct fwu_mdata **mdata);
> > +int fwu_update_mdata(struct fwu_mdata *mdata);
> > +int fwu_get_active_index(u32 *active_idx);
> > +int fwu_update_active_index(u32 active_idx);
> > +int fwu_get_image_alt_num(efi_guid_t *image_type_id, u32 update_bank,
> > +                         int *alt_num);
> > +int fwu_mdata_check(void);
> > +int fwu_revert_boot_index(void);
> > +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);
> > +
> > +int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid,
> > +                        int *alt_num);
> > +#endif /* _FWU_H_ */
> > diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h
> > new file mode 100644
> > index 0000000000..701efbba03
> > --- /dev/null
> > +++ b/include/fwu_mdata.h
> > @@ -0,0 +1,67 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (c) 2022, Linaro Limited
> > + */
> > +
> > +#if !defined _FWU_MDATA_H_
> > +#define _FWU_MDATA_H_
> > +
> > +#include <efi.h>
> > +
> > +/**
> > + * struct fwu_image_bank_info - firmware image information
> > + * @image_uuid: Guid value of the image in this bank
> > + * @accepted: Acceptance status of the image
> > + * @reserved: Reserved
> > + *
> > + * The structure contains image specific fields which are
> > + * used to identify the image and to specify the image's
> > + * acceptance status
> > + */
> > +struct fwu_image_bank_info {
> > +       efi_guid_t  image_uuid;
> > +       uint32_t accepted;
> > +       uint32_t reserved;
> > +} __attribute__((__packed__));
> > +
> > +/**
> > + * struct fwu_image_entry - information for a particular type of image
> > + * @image_type_uuid: Guid value for identifying the image type
> > + * @location_uuid: Guid of the storage volume where the image is located
> > + * @img_bank_info: Array containing properties of images
> > + *
> > + * This structure contains information on various types of updatable
> > + * firmware images. Each image type then contains an array of image
> > + * information per bank.
> > + */
> > +struct fwu_image_entry {
> > +       efi_guid_t image_type_uuid;
> > +       efi_guid_t location_uuid;
> > +       struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS];
> > +} __attribute__((__packed__));
> > +
> > +/**
> > + * struct fwu_mdata - FWU metadata structure for multi-bank updates
> > + * @crc32: crc32 value for the FWU metadata
> > + * @version: FWU metadata version
> > + * @active_index: Index of the bank currently used for booting images
> > + * @previous_active_inde: Index of the bank used before the current bank
> > + *                        being used for booting
> > + * @img_entry: Array of information on various firmware images that can
> > + *             be updated
> > + *
> > + * This structure is used to store all the needed information for performing
> > + * multi bank updates on the platform. This contains info on the bank being
> > + * used to boot along with the information needed for identification of
> > + * individual images
> > + */
> > +struct fwu_mdata {
> > +       uint32_t crc32;
> > +       uint32_t version;
> > +       uint32_t active_index;
> > +       uint32_t previous_active_index;
> > +
> > +       struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK];
> > +} __attribute__((__packed__));
> > +
> > +#endif /* _FWU_MDATA_H_ */
> > --
> > 2.25.1
> >

Hi Sughosh,

On Thu, 23 Jun 2022 at 08:24, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Etienne,
>
> On Tue, 21 Jun 2022 at 16:24, Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> >
> > Hello Sughosh,
> >
> >
> >
> > On Thu, 9 Jun 2022 at 14:30, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > In the FWU Multi Bank Update feature, the information about the
> > > updatable images is stored as part of the metadata, which is stored on
> > > a dedicated partition. Add the metadata structure, and a driver model
> > > uclass which provides functions to access the metadata. These are
> > > generic API's, and implementations can be added based on parameters
> > > like how the metadata partition is accessed and what type of storage
> > > device houses the metadata.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > >  drivers/Kconfig                      |   2 +
> > >  drivers/Makefile                     |   1 +
> > >  drivers/fwu-mdata/Kconfig            |   7 +
> > >  drivers/fwu-mdata/Makefile           |   6 +
> > >  drivers/fwu-mdata/fwu-mdata-uclass.c | 459 +++++++++++++++++++++++++++
> > >  include/dm/uclass-id.h               |   1 +
> > >  include/fwu.h                        |  49 +++
> > >  include/fwu_mdata.h                  |  67 ++++
> > >  8 files changed, 592 insertions(+)
> > >  create mode 100644 drivers/fwu-mdata/Kconfig
> > >  create mode 100644 drivers/fwu-mdata/Makefile
> > >  create mode 100644 drivers/fwu-mdata/fwu-mdata-uclass.c
> > >  create mode 100644 include/fwu.h
> > >  create mode 100644 include/fwu_mdata.h
> > >
>
> <snip>
>
> > > diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c
> > > new file mode 100644
> > > index 0000000000..1530ceb01d
> > > --- /dev/null
> > > +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c

<snip>

> > > +/**
> > > + * fwu_get_mdata() - Get a FWU metadata copy
> > > + * @mdata: Copy of the FWU metadata
> > > + *
> > > + * Get a valid copy of the FWU metadata.
> > > + *
> > > + * Return: 0 if OK, -ve on error
> > > + *
> > > + */
> > > +int fwu_get_mdata(struct fwu_mdata **mdata)
> >
> > Is there a real need for this function to allocate an instance of struct mdata.
> > I think it would be clearer if it was the caller's responsibility to
> > allocate/free the structure.
> >
> > Or maybe rename this function fwu_alloc_and_copy_mdata() to highlight
> > that the function gives an allocated copy of the data.
>
> I guess I can put a comment in the function description saying that
> the function is responsible for the allocation of the metadata
> structure.

I think it would be better.

>
> > One should be careful when calling these API functions as some act on
> > a local copy (retrieved from fw_get_mdata()) while other functions
> > modify straight fwu-mdata in the storage media.
>
> Did you find any function which is modifying the metadata on the
> storage device directly. The API fwu_update_mdata() is supposed to be
> doing that. If you have come across any function which is directly
> modifying the metadata on the storage media, please let me know and I
> will fix it.

Many functions do so: fwu_clear_accept_image(),
fwu_clear_accept_image(), fwu_resert_boot_index(), etc... Actually all
generic functions do so while only fwu_get_mdata() and
fwu_update_mdata() act on a RAM copy.

Maybe fwu-mdata ops should have a status field for when a RAM copy was
exported and used to prevent direct updates to mdata in storage until
caller releases (fw_put_mdata()?) the exposed copy. Would this scheme
be overkilling...

Or maybe fwu_clear_accept_image() and other helper functions could
also require a mdata RAM reference to act on, letting the caller also
go through fwu_get_mdata()/fwu_update_mdata().

etienne

<snip>