Re: [PATCH v6 03/13] FWU: Add FWU metadata access driver for GPT partitioned block devices

hi Patrick,

On Wed, 13 Jul 2022 at 18:47, Patrick DELAUNAY patrick.delaunay@foss.st.com wrote:

Hi,

On 7/4/22 07:16, 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 formated with GPT based partition scheme.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

Changes since V5:

• Changed the logic to store the GPT partitioned block device through a priv structure as suggested by Patrick

• Used dev_read_prop() to get the phandle_p instead of ofnode_get_property() used earlier as suggested by Patrick

• Make relevant functions static as suggested by Etienne

  drivers/fwu-mdata/Kconfig | 9 + drivers/fwu-mdata/Makefile | 1 + drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 408 ++++++++++++++++++++++++++ include/fwu.h | 5 + 4 files changed, 423 insertions(+) create mode 100644 drivers/fwu-mdata/fwu_mdata_gpt_blk.c

diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig index d6a21c8e19..d5edef19d6 100644 --- a/drivers/fwu-mdata/Kconfig +++ b/drivers/fwu-mdata/Kconfig @@ -5,3 +5,12 @@ config DM_FWU_MDATA 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 index e53a8c9983..313049f67a 100644 --- a/drivers/fwu-mdata/Makefile +++ b/drivers/fwu-mdata/Makefile @@ -4,3 +4,4 @@ #

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

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

#define LOG_CATEGORY UCLASS_FWU_MDATA

I think you had mentioned this in your earlier review as well. Sorry, I missed out on this. Will add in the next version.

...

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

+#include <dm/device-internal.h> +#include <linux/errno.h> +#include <linux/types.h> +#include <u-boot/crc.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 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,
  

+};UCLASS_FWU_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 e03cfff800..8259c75d12 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;
  

+};

NITS: really needed in .h file => private, only used by driver in .c ?

I have put it here since this gets accessed in the board file as well to retrieve the block device which has the fwu metadata and the firmware images in fwu_plat_get_alt_num().

...

/**

• @mdata_check: check the validity of the FWU metadata partitions
• @get_mdata() - Get a FWU metadata copy

@@ -39,6 +43,7 @@ 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_verify_mdata(struct fwu_mdata *mdata, bool pri_part); int fwu_mdata_check(void); int fwu_revert_boot_index(void); int fwu_accept_image(efi_guid_t *img_type_id, u32 bank);

with the minor modification (LOG_CATEGORY)

Reviewed-by: Patrick Delaunay patrick.delaunay@foss.st.com

Thanks.

-sughosh