[RFC,v3,1/9] FWU: Add FWU metadata structure and functions 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 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>
---

Changes since V2:
* Use uint*_t types in fwu_mdata.h since the file is to be reused in
  other projects
* Keep only the FWU metadata structures in fwu_mdata.h
* Move all other function and macro declarations in fwu.h
* Keep common implementations of fwu_update_active_index and
  fwu_revert_boot_index in fwu_mdata.c
* Add a update_mdata function pointer in the fwu_mdata_ops structure

 include/fwu.h               |  61 +++++++
 include/fwu_mdata.h         |  67 ++++++++
 lib/fwu_updates/fwu_mdata.c | 329 ++++++++++++++++++++++++++++++++++++
 3 files changed, 457 insertions(+)
 create mode 100644 include/fwu.h
 create mode 100644 include/fwu_mdata.h
 create mode 100644 lib/fwu_updates/fwu_mdata.c

Hello Sughosh,

2022年1月20日(木) 3:56 Sughosh Ganu <sughosh.ganu@linaro.org>:
>
> In the FWU Multi Bank Update feature, the information about the
> updatable images is stored as part of the metadata, which is stored on
> a dedicated partition. Add the metadata structure, and 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>
> ---
>
> Changes since V2:
> * Use uint*_t types in fwu_mdata.h since the file is to be reused in
>   other projects
> * Keep only the FWU metadata structures in fwu_mdata.h
> * Move all other function and macro declarations in fwu.h
> * Keep common implementations of fwu_update_active_index and
>   fwu_revert_boot_index in fwu_mdata.c
> * Add a update_mdata function pointer in the fwu_mdata_ops structure
>
>  include/fwu.h               |  61 +++++++
>  include/fwu_mdata.h         |  67 ++++++++
>  lib/fwu_updates/fwu_mdata.c | 329 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 457 insertions(+)
>  create mode 100644 include/fwu.h
>  create mode 100644 include/fwu_mdata.h
>  create mode 100644 lib/fwu_updates/fwu_mdata.c
>
> diff --git a/include/fwu.h b/include/fwu.h
> new file mode 100644
> index 0000000000..acba725bc8
> --- /dev/null
> +++ b/include/fwu.h
> @@ -0,0 +1,61 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (c) 2021, Linaro Limited
> + */
> +
> +#if !defined _FWU_H_
> +#define _FWU_H_
> +
> +#include <blk.h>
> +#include <efi.h>
> +
> +#include <linux/types.h>
> +
> +struct fwu_mdata;
> +
> +/**
> + * @get_active_index: get the current active_index value
> + * @get_image_alt_num: get the alt number to be used for the image
> + * @mdata_check: check the validity of the FWU metadata partitions
> + * @set_accept_image: set the accepted bit for the image
> + * @clear_accept_image: clear the accepted bit for the image
> + * @get_mdata() - Get a FWU metadata copy
> + * @update_mdata() - Update the FWU metadata copy
> + */
> +struct fwu_mdata_ops {
> +       int (*get_active_index)(u32 *active_idx);
> +
> +       int (*get_image_alt_num)(efi_guid_t image_type_id, u32 update_bank,
> +                                int *alt_num);
> +
> +       int (*mdata_check)(void);
> +
> +       int (*set_accept_image)(efi_guid_t *img_type_id, u32 bank);
> +
> +       int (*clear_accept_image)(efi_guid_t *img_type_id, u32 bank);
> +
> +       int (*get_mdata)(struct fwu_mdata **mdata);
> +
> +       int (*update_mdata)(struct fwu_mdata *mdata);
> +};

We also can remove
- get_active_index
- set_accept_image
- clear_accept_image
from these operations, because those are just reading or modifying metadata.
Such functions can be written as below

ops()
{
   op->get_mdata(&mdata);
   do_some_operation(mdata);
   if (updated)
     op->update_mdata(mdata);
}

I'll make a series of patches on top of your series for DeveloperBox,
which does not use GPT.

Thank you,

Hi Sughosh,

I have another comment on this patch.

2022年1月20日(木) 3:56 Sughosh Ganu <sughosh.ganu@linaro.org>:

> +/**
> + * 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;
> +       void *buf;
> +       struct fwu_mdata *mdata = NULL;
> +
> +       if (active_idx > CONFIG_FWU_NUM_BANKS) {

Since active_index must be 0 .. CONFIG_FWU_NUM_BANKS - 1, it should
cap the new active_index with "CONFIG_FWU_NUM_BANKS - 1" instead of
"CONFIG_FWU_NUM_BANKS".

> +               printf("Active index value to be updated is incorrect\n");

Could you use log_err() instead of printf() for error messages?

Thank you,

On 1/19/22 19:55, Sughosh Ganu 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 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>

I would have expected a new uclass implementing

* FFA_PARTITION_INFO_GET()

as defined in [1] and suggested by [2].

Reading the data from a partition could be implemented in one of the
drivers.

Please, have a look at
https://u-boot.readthedocs.io/en/latest/develop/driver-model/index.html

[1] Arm Firmware Framework for Arm A-profile
(https://developer.arm.com/documentation/den0077/latest)
[2] Platform Security Firmware Update for the A-profile Arm Architecture
Beta
(https://developer.arm.com/documentation/den0118/a)

Best regards

Heinrich

> ---
>
> Changes since V2:
> * Use uint*_t types in fwu_mdata.h since the file is to be reused in
>    other projects
> * Keep only the FWU metadata structures in fwu_mdata.h
> * Move all other function and macro declarations in fwu.h
> * Keep common implementations of fwu_update_active_index and
>    fwu_revert_boot_index in fwu_mdata.c
> * Add a update_mdata function pointer in the fwu_mdata_ops structure
>
>   include/fwu.h               |  61 +++++++
>   include/fwu_mdata.h         |  67 ++++++++
>   lib/fwu_updates/fwu_mdata.c | 329 ++++++++++++++++++++++++++++++++++++
>   3 files changed, 457 insertions(+)
>   create mode 100644 include/fwu.h
>   create mode 100644 include/fwu_mdata.h
>   create mode 100644 lib/fwu_updates/fwu_mdata.c
>
> diff --git a/include/fwu.h b/include/fwu.h
> new file mode 100644
> index 0000000000..acba725bc8
> --- /dev/null
> +++ b/include/fwu.h
> @@ -0,0 +1,61 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (c) 2021, Linaro Limited
> + */
> +
> +#if !defined _FWU_H_
> +#define _FWU_H_
> +
> +#include <blk.h>
> +#include <efi.h>
> +
> +#include <linux/types.h>
> +
> +struct fwu_mdata;
> +
> +/**
> + * @get_active_index: get the current active_index value
> + * @get_image_alt_num: get the alt number to be used for the image
> + * @mdata_check: check the validity of the FWU metadata partitions
> + * @set_accept_image: set the accepted bit for the image
> + * @clear_accept_image: clear the accepted bit for the image
> + * @get_mdata() - Get a FWU metadata copy
> + * @update_mdata() - Update the FWU metadata copy
> + */
> +struct fwu_mdata_ops {
> +	int (*get_active_index)(u32 *active_idx);
> +
> +	int (*get_image_alt_num)(efi_guid_t image_type_id, u32 update_bank,
> +				 int *alt_num);
> +
> +	int (*mdata_check)(void);
> +
> +	int (*set_accept_image)(efi_guid_t *img_type_id, u32 bank);
> +
> +	int (*clear_accept_image)(efi_guid_t *img_type_id, u32 bank);
> +
> +	int (*get_mdata)(struct fwu_mdata **mdata);
> +
> +	int (*update_mdata)(struct fwu_mdata *mdata);
> +};
> +
> +struct fwu_mdata_ops *get_plat_fwu_mdata_ops(void);
> +
> +#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);
> +
> +#endif /* _FWU_H_ */
> diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h
> new file mode 100644
> index 0000000000..d788eb69e7
> --- /dev/null
> +++ b/include/fwu_mdata.h
> @@ -0,0 +1,67 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (c) 2021, 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_ */
> diff --git a/lib/fwu_updates/fwu_mdata.c b/lib/fwu_updates/fwu_mdata.c
> new file mode 100644
> index 0000000000..58e838fe28
> --- /dev/null
> +++ b/lib/fwu_updates/fwu_mdata.c
> @@ -0,0 +1,329 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2021, Linaro Limited
> + */
> +
> +#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>
> +
> +static struct fwu_mdata_ops *get_fwu_mdata_ops(void)
> +{
> +	struct fwu_mdata_ops *ops;
> +
> +	ops =  get_plat_fwu_mdata_ops();
> +	if (!ops) {
> +		log_err("Unable to get fwu ops\n");
> +		return NULL;
> +	}
> +
> +	return ops;
> +}
> +
> +/**
> + * 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)
> +{
> +	struct fwu_mdata_ops *ops;
> +
> +	ops = get_fwu_mdata_ops();
> +	if (!ops)
> +		return -EPROTONOSUPPORT;
> +
> +	if (!ops->get_active_index) {
> +		log_err("get_active_index() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->get_active_index(active_idx);
> +}
> +
> +/**
> + * 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;
> +	void *buf;
> +	struct fwu_mdata *mdata = NULL;
> +
> +	if (active_idx > CONFIG_FWU_NUM_BANKS) {
> +		printf("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;
> +
> +	/*
> +	 * 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));
> +
> +	/*
> +	 * 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: 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)
> +{
> +	struct fwu_mdata_ops *ops;
> +
> +	ops = get_fwu_mdata_ops();
> +	if (!ops)
> +		return -EPROTONOSUPPORT;
> +
> +	if (!ops->get_image_alt_num) {
> +		log_err("get_image_alt_num() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->get_image_alt_num(image_type_id, update_bank, alt_num);
> +}
> +
> +/**
> + * 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)
> +{
> +	struct fwu_mdata_ops *ops;
> +
> +	ops = get_fwu_mdata_ops();
> +	if (!ops)
> +		return -EPROTONOSUPPORT;
> +
> +	if (!ops->mdata_check) {
> +		log_err("mdata_check() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->mdata_check();
> +}
> +
> +/**
> + * 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;
> +	void *buf;
> +	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;
> +
> +	/*
> +	 * 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));
> +
> +	/*
> +	 * 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_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)
> +{
> +	struct fwu_mdata_ops *ops;
> +
> +	ops = get_fwu_mdata_ops();
> +	if (!ops)
> +		return -EPROTONOSUPPORT;
> +
> +	if (!ops->set_accept_image) {
> +		log_err("set_accept_image() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->set_accept_image(img_type_id, bank);
> +}
> +
> +/**
> + * 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)
> +{
> +	struct fwu_mdata_ops *ops;
> +
> +	ops = get_fwu_mdata_ops();
> +	if (!ops)
> +		return -EPROTONOSUPPORT;
> +
> +	if (!ops->clear_accept_image) {
> +		log_err("clear_accept_image() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->clear_accept_image(img_type_id, bank);
> +}
> +
> +/**
> + * 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)
> +{
> +	struct fwu_mdata_ops *ops;
> +
> +	ops = get_fwu_mdata_ops();
> +	if (!ops)
> +		return -EPROTONOSUPPORT;
> +
> +	if (!ops->get_mdata) {
> +		log_err("get_mdata() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->get_mdata(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)
> +{
> +	struct fwu_mdata_ops *ops;
> +
> +	ops = get_fwu_mdata_ops();
> +	if (!ops)
> +		return -EPROTONOSUPPORT;
> +
> +	if (!ops->update_mdata) {
> +		log_err("get_mdata() method not defined for the platform\n");
> +		return -ENOSYS;
> +	}
> +
> +	return ops->update_mdata(mdata);
> +}

hi Masami,

On Thu, 20 Jan 2022 at 11:23, Masami Hiramatsu
<masami.hiramatsu@linaro.org> wrote:
>
> Hello Sughosh,
>
> 2022年1月20日(木) 3:56 Sughosh Ganu <sughosh.ganu@linaro.org>:
> >
> > In the FWU Multi Bank Update feature, the information about the
> > updatable images is stored as part of the metadata, which is stored on
> > a dedicated partition. Add the metadata structure, and 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>
> > ---
> >
> > Changes since V2:
> > * Use uint*_t types in fwu_mdata.h since the file is to be reused in
> >   other projects
> > * Keep only the FWU metadata structures in fwu_mdata.h
> > * Move all other function and macro declarations in fwu.h
> > * Keep common implementations of fwu_update_active_index and
> >   fwu_revert_boot_index in fwu_mdata.c
> > * Add a update_mdata function pointer in the fwu_mdata_ops structure
> >
> >  include/fwu.h               |  61 +++++++
> >  include/fwu_mdata.h         |  67 ++++++++
> >  lib/fwu_updates/fwu_mdata.c | 329 ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 457 insertions(+)
> >  create mode 100644 include/fwu.h
> >  create mode 100644 include/fwu_mdata.h
> >  create mode 100644 lib/fwu_updates/fwu_mdata.c
> >
> > diff --git a/include/fwu.h b/include/fwu.h
> > new file mode 100644
> > index 0000000000..acba725bc8
> > --- /dev/null
> > +++ b/include/fwu.h
> > @@ -0,0 +1,61 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (c) 2021, Linaro Limited
> > + */
> > +
> > +#if !defined _FWU_H_
> > +#define _FWU_H_
> > +
> > +#include <blk.h>
> > +#include <efi.h>
> > +
> > +#include <linux/types.h>
> > +
> > +struct fwu_mdata;
> > +
> > +/**
> > + * @get_active_index: get the current active_index value
> > + * @get_image_alt_num: get the alt number to be used for the image
> > + * @mdata_check: check the validity of the FWU metadata partitions
> > + * @set_accept_image: set the accepted bit for the image
> > + * @clear_accept_image: clear the accepted bit for the image
> > + * @get_mdata() - Get a FWU metadata copy
> > + * @update_mdata() - Update the FWU metadata copy
> > + */
> > +struct fwu_mdata_ops {
> > +       int (*get_active_index)(u32 *active_idx);
> > +
> > +       int (*get_image_alt_num)(efi_guid_t image_type_id, u32 update_bank,
> > +                                int *alt_num);
> > +
> > +       int (*mdata_check)(void);
> > +
> > +       int (*set_accept_image)(efi_guid_t *img_type_id, u32 bank);
> > +
> > +       int (*clear_accept_image)(efi_guid_t *img_type_id, u32 bank);
> > +
> > +       int (*get_mdata)(struct fwu_mdata **mdata);
> > +
> > +       int (*update_mdata)(struct fwu_mdata *mdata);
> > +};
>
> We also can remove
> - get_active_index
> - set_accept_image
> - clear_accept_image
> from these operations, because those are just reading or modifying metadata.
> Such functions can be written as below
>
> ops()
> {
>    op->get_mdata(&mdata);
>    do_some_operation(mdata);
>    if (updated)
>      op->update_mdata(mdata);
> }
>
> I'll make a series of patches on top of your series for DeveloperBox,
> which does not use GPT.

I will check how these functions can be put in the common fwu_mdata.c
file. Thanks.

-sughosh

>
> Thank you,
>
> --
> Masami Hiramatsu

hi Masami,

On Thu, 20 Jan 2022 at 11:35, Masami Hiramatsu
<masami.hiramatsu@linaro.org> wrote:
>
> Hi Sughosh,
>
> I have another comment on this patch.
>
> 2022年1月20日(木) 3:56 Sughosh Ganu <sughosh.ganu@linaro.org>:
>
> > +/**
> > + * 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;
> > +       void *buf;
> > +       struct fwu_mdata *mdata = NULL;
> > +
> > +       if (active_idx > CONFIG_FWU_NUM_BANKS) {
>
> Since active_index must be 0 .. CONFIG_FWU_NUM_BANKS - 1, it should
> cap the new active_index with "CONFIG_FWU_NUM_BANKS - 1" instead of
> "CONFIG_FWU_NUM_BANKS".

Will update the check. Thanks.

>
> > +               printf("Active index value to be updated is incorrect\n");
>
> Could you use log_err() instead of printf() for error messages?

Will do.

-sughosh

>
> Thank you,
>
>
> --
> Masami Hiramatsu