| Message ID | 20220609123010.1017463-4-sughosh.ganu@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | FWU: Add FWU Multi Bank Update feature support | expand |
Hi, On 6/9/22 14:29, 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> > --- > drivers/fwu-mdata/Kconfig | 9 + > drivers/fwu-mdata/Makefile | 1 + > drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 404 ++++++++++++++++++++++++++ > include/fwu.h | 2 + > 4 files changed, 416 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 7fec7171f4..12a5b4fe04 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 It is strange to have '_' and '-' in file name for the same directory => to be coherent = fwu-mdata-gpt-blk.c > 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..329bd3779b > --- /dev/null > +++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c > @@ -0,0 +1,404 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (c) 2022, Linaro Limited > + */ > + > +#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) > + > + [...] > + > +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); > +} > + > +int fwu_gpt_get_mdata(struct udevice *dev, struct fwu_mdata **mdata) > +{ > + struct blk_desc *desc; > + > + desc = dev_get_uclass_plat(dev_get_priv(dev)); as dev = fwu_mdata_gpt_blk(UCLASS_FWU_MDATA) dev_get_priv(dev) => get value saved in dev_set_priv(dev, mdata_dev); even if it is OK, it not clear here. can you add a struct to prepare addition of other elements in privdata: struct fwu_mdata_gpt_blk_priv { struct udevice *blk_dev; } + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev); + struct blk_desc *desc; + desc = dev_get_uclass_plat(priv->blk_dev); > + if (!desc) { > + log_err("Block device not found\n"); > + return -ENODEV; > + } > + > + return gpt_get_mdata(desc, mdata); > +} > + > +int fwu_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev) > +{ > + u32 phandle; > + int ret, size; > + struct udevice *parent, *child; > + const fdt32_t *phandle_p = NULL; > + > + phandle_p = ofnode_get_property(dev_ofnode(dev), "fwu-mdata-store", > + &size); phandle_p = dev_read_prop(dev, "fwu-mdata-store", &size); it is more simple > + if (!phandle_p) { > + log_err("fwu-mdata-store property not found\n"); > + return -ENOENT; > + } > + > + phandle = fdt32_to_cpu(*phandle_p); or phandle can be read directly by + ret =dev_read_phandle_with_args(dev, "fwu-mdata-store", NULL, 0, 0, phandle_p) + if (ret) { + log_err("fwu-mdata-store property not found\n"); + return ret; + } > + > + ret = device_get_global_by_ofnode(ofnode_get_by_phandle(phandle), > + &parent); > + if (ret) > + return ret; > + > + ret = -ENODEV; > + for (device_find_first_child(parent, &child); child; > + device_find_next_child(&child)) { > + if (device_get_uclass_id(child) == UCLASS_BLK) { > + *mdata_dev = child; > + ret = 0; > + } > + } > + > + return ret; > +} > + > +static int fwu_mdata_gpt_blk_probe(struct udevice *dev) > +{ > + int ret; > + struct udevice *mdata_dev = NULL; > + > + ret = fwu_get_mdata_device(dev, &mdata_dev); > + if (ret) > + return ret; > + > + dev_set_priv(dev, mdata_dev); Avoid to use dev_set_priv() in driver : /** * dev_set_priv() - Set the private data for a device * * This is normally handled by driver model, which automatically allocates * private data when an 'auto' size if provided by the driver. * * Use this function to override normal operation for special situations, such * as needing to allocate a variable amount of data. * ... + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev); + 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 f9e44e7b39..3b1ee4e83e 100644 > --- a/include/fwu.h > +++ b/include/fwu.h > @@ -39,6 +39,8 @@ 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_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev); > +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); Regards Patrick
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, 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> > --- > drivers/fwu-mdata/Kconfig | 9 + > drivers/fwu-mdata/Makefile | 1 + > drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 404 ++++++++++++++++++++++++++ > include/fwu.h | 2 + > 4 files changed, 416 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 7fec7171f4..12a5b4fe04 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..329bd3779b > --- /dev/null > +++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c > @@ -0,0 +1,404 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (c) 2022, Linaro Limited > + */ > + > +#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 int gpt_get_mdata_partitions(struct blk_desc *desc, > + u16 *primary_mpart, > + u16 *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_err("Expect two copies of the FWU metadata instead of %d\n", > + mdata_parts); > + ret = -EINVAL; > + } else { > + ret = 0; > + } > + > + 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_err("Unable to get the partition info for the FWU metadata part %d", > + part_num); > + return -1; > + } > + > + /* 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 -1; > +} > + > +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 -ENODEV; > + } > + > + len = sizeof(*mdata); > + blkcnt = BLOCK_CNT(len, desc); > + if (blkcnt > info.size) { > + log_err("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_err("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_err("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; > + u16 primary_mpart = 0, secondary_mpart = 0; > + > + desc = dev_get_uclass_plat(dev_get_priv(dev)); > + if (!desc) { > + log_err("Block device not found\n"); > + return -ENODEV; > + } > + > + ret = gpt_get_mdata_partitions(desc, &primary_mpart, > + &secondary_mpart); > + > + if (ret < 0) { > + log_err("Error getting the FWU metadata partitions\n"); > + return -ENODEV; > + } > + > + /* First write the primary partition*/ > + ret = gpt_write_mdata_partition(desc, mdata, primary_mpart); > + if (ret < 0) { > + log_err("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_err("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; > + u16 primary_mpart = 0, secondary_mpart = 0; > + > + ret = gpt_get_mdata_partitions(desc, &primary_mpart, > + &secondary_mpart); > + > + if (ret < 0) { > + log_err("Error getting the FWU metadata partitions\n"); > + return -ENODEV; > + } > + > + *mdata = malloc(sizeof(struct fwu_mdata)); > + if (!*mdata) { > + log_err("Unable to allocate memory for reading FWU metadata\n"); > + return -ENOMEM; > + } > + > + ret = gpt_read_mdata(desc, *mdata, primary_mpart); > + if (ret < 0) { > + log_err("Failed to read the FWU metadata from the device\n"); > + return -EIO; > + } > + > + ret = fwu_verify_mdata(*mdata, 1); > + if (!ret) Upon success, I think this function should also either ensure secondary_part contains a valid copy of primary part, Maybe this function should call gpt_check_mdata_validity() and then read mdata content. > + 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_err("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 -1; > +} > + > +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; > + u16 primary_mpart = 0, secondary_mpart = 0; > + u16 valid_partitions, invalid_partitions; > + > + desc = dev_get_uclass_plat(dev_get_priv(dev)); > + if (!desc) { > + log_err("Block device not found\n"); > + return -ENODEV; > + } > + > + /* > + * 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_err("Error getting the FWU metadata partitions\n"); > + return -ENODEV; > + } > + > + ret = gpt_read_mdata(desc, &pri_mdata, primary_mpart); > + if (ret < 0) { > + log_err("Failed to read the FWU metadata from the device\n"); > + goto secondary_read; > + } > + > + 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_err("Failed to read the FWU metadata from the device\n"); > + 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_err("Both FWU metadata copies are valid but do not match. Please check!\n"); I think this function should select one of the part and copies to the other, e.g. assume primary is fine, copy primary to secondary and return upon success. > + } > + 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_err("Restoring %s FWU metadata partition failed\n", > + (invalid_partitions == PRIMARY_PART) ? > + "primary" : "secondary"); > + > +out: > + return ret; > +} > + > +int fwu_gpt_mdata_check(struct udevice *dev) Function should be static. > +{ > + /* > + * 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); > +} > + > +int fwu_gpt_get_mdata(struct udevice *dev, struct fwu_mdata **mdata) Function should be static. > +{ > + struct blk_desc *desc; > + > + desc = dev_get_uclass_plat(dev_get_priv(dev)); > + if (!desc) { > + log_err("Block device not found\n"); > + return -ENODEV; > + } > + > + return gpt_get_mdata(desc, mdata); > +} > + > +int fwu_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev) Function could be static (and declaration removed from fwu.h). > +{ > + u32 phandle; > + int ret, size; > + struct udevice *parent, *child; > + const fdt32_t *phandle_p = NULL; > + > + phandle_p = ofnode_get_property(dev_ofnode(dev), "fwu-mdata-store", > + &size); Should this be retrieved from driver's ::of_to_plat method? Br, etienne > + if (!phandle_p) { > + log_err("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; > + > + ret = -ENODEV; > + for (device_find_first_child(parent, &child); child; > + device_find_next_child(&child)) { > + if (device_get_uclass_id(child) == UCLASS_BLK) { > + *mdata_dev = child; > + ret = 0; > + } > + } > + > + return ret; > +} > + > +static int fwu_mdata_gpt_blk_probe(struct udevice *dev) > +{ > + int ret; > + struct udevice *mdata_dev = NULL; > + > + ret = fwu_get_mdata_device(dev, &mdata_dev); > + if (ret) > + return ret; > + > + dev_set_priv(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, > +}; > diff --git a/include/fwu.h b/include/fwu.h > index f9e44e7b39..3b1ee4e83e 100644 > --- a/include/fwu.h > +++ b/include/fwu.h > @@ -39,6 +39,8 @@ 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_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev); > +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); > -- > 2.25.1 >
Hi On 6/21/22 11:34, Patrick DELAUNAY wrote: > Hi, > > On 6/9/22 14:29, 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> >> --- >> drivers/fwu-mdata/Kconfig | 9 + >> drivers/fwu-mdata/Makefile | 1 + >> drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 404 ++++++++++++++++++++++++++ >> include/fwu.h | 2 + >> 4 files changed, 416 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 7fec7171f4..12a5b4fe04 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 > > > It is strange to have '_' and '-' in file name for the same directory > > => to be coherent = fwu-mdata-gpt-blk.c > > >> 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..329bd3779b >> --- /dev/null >> +++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c >> @@ -0,0 +1,404 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Copyright (c) 2022, Linaro Limited >> + */ + #define LOG_CATEGORY UCLASS_FWU_MDATA For command log filtering by uclass >> +#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> >> + [...] > > Regards > > Patrick >
hi Patrick, Apologies for the late reply. I had missed out replying to the review comments on this patch. There are some review comments on the Synquacer patches which need to be taken care of by another engineer. Once those review comments are taken care of, I will be sending the next version. On Tue, 21 Jun 2022 at 15:04, Patrick DELAUNAY <patrick.delaunay@foss.st.com> wrote: > > Hi, > > On 6/9/22 14:29, 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> > > --- > > drivers/fwu-mdata/Kconfig | 9 + > > drivers/fwu-mdata/Makefile | 1 + > > drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 404 ++++++++++++++++++++++++++ > > include/fwu.h | 2 + > > 4 files changed, 416 insertions(+) > > create mode 100644 drivers/fwu-mdata/fwu_mdata_gpt_blk.c > > <snip> > > diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile > > index 7fec7171f4..12a5b4fe04 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 > > > It is strange to have '_' and '-' in file name for the same directory > > => to be coherent = fwu-mdata-gpt-blk.c I see this kind of naming style in many other directories under drivers/. The uclass file is named using the foo-uclass.c, while the other driver files are named bar_driver.c > > > > 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..329bd3779b > > --- /dev/null > > +++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c > > @@ -0,0 +1,404 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright (c) 2022, Linaro Limited > > + */ > > + > > +#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) > > + > > + > > > [...] > > > + > > +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); > > +} > > + > > +int fwu_gpt_get_mdata(struct udevice *dev, struct fwu_mdata **mdata) > > +{ > > + struct blk_desc *desc; > > + > > + desc = dev_get_uclass_plat(dev_get_priv(dev)); > > as dev = fwu_mdata_gpt_blk(UCLASS_FWU_MDATA) > > dev_get_priv(dev) => get value saved in dev_set_priv(dev, mdata_dev); > > even if it is OK, it not clear here. > > can you add a struct to prepare addition of other elements in privdata: > > struct fwu_mdata_gpt_blk_priv { > struct udevice *blk_dev; > } > > > + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev); > > + struct blk_desc *desc; > > + desc = dev_get_uclass_plat(priv->blk_dev); Okay. Will add a priv structure as you suggest. > > > > + if (!desc) { > > + log_err("Block device not found\n"); > > + return -ENODEV; > > + } > > + > > + return gpt_get_mdata(desc, mdata); > > +} > > + > > +int fwu_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev) > > +{ > > + u32 phandle; > > + int ret, size; > > + struct udevice *parent, *child; > > + const fdt32_t *phandle_p = NULL; > > + > > + phandle_p = ofnode_get_property(dev_ofnode(dev), "fwu-mdata-store", > > + &size); > > phandle_p = dev_read_prop(dev, "fwu-mdata-store", &size); > > > it is more simple Okay > > > > + if (!phandle_p) { > > + log_err("fwu-mdata-store property not found\n"); > > + return -ENOENT; > > + } > > + > > + phandle = fdt32_to_cpu(*phandle_p); > > > or phandle can be read directly by > > + ret =dev_read_phandle_with_args(dev, "fwu-mdata-store", NULL, 0, 0, > phandle_p) I did not understand this review comment properly. In any case, I am using the API, dev_read_prop that you suggested above to read the phandle pointer. > > + if (ret) { > + log_err("fwu-mdata-store property not found\n"); > + return ret; > + } > > > + > > + ret = device_get_global_by_ofnode(ofnode_get_by_phandle(phandle), > > + &parent); > > + if (ret) > > + return ret; > > + > > + ret = -ENODEV; > > + for (device_find_first_child(parent, &child); child; > > + device_find_next_child(&child)) { > > + if (device_get_uclass_id(child) == UCLASS_BLK) { > > + *mdata_dev = child; > > + ret = 0; > > + } > > + } > > + > > + return ret; > > +} > > + > > +static int fwu_mdata_gpt_blk_probe(struct udevice *dev) > > +{ > > + int ret; > > + struct udevice *mdata_dev = NULL; > > + > > + ret = fwu_get_mdata_device(dev, &mdata_dev); > > + if (ret) > > + return ret; > > + > > + dev_set_priv(dev, mdata_dev); > > Avoid to use dev_set_priv() in driver : > > /** > * dev_set_priv() - Set the private data for a device > * > * This is normally handled by driver model, which automatically allocates > * private data when an 'auto' size if provided by the driver. > * > * Use this function to override normal operation for special > situations, such > * as needing to allocate a variable amount of data. > * > > ... > > + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev); > > + priv->blk_dev = mdata_dev; Okay > > > + > > + 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), Okay. I have incorporated these review comments. Thanks. -sughosh > > > > +}; > > diff --git a/include/fwu.h b/include/fwu.h > > index f9e44e7b39..3b1ee4e83e 100644 > > --- a/include/fwu.h > > +++ b/include/fwu.h > > @@ -39,6 +39,8 @@ 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_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev); > > +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); > > > Regards > > Patrick >
hi Etienne, On Tue, 21 Jun 2022 at 16:26, 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, 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> > > --- > > drivers/fwu-mdata/Kconfig | 9 + > > drivers/fwu-mdata/Makefile | 1 + > > drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 404 ++++++++++++++++++++++++++ > > include/fwu.h | 2 + > > 4 files changed, 416 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 7fec7171f4..12a5b4fe04 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..329bd3779b > > --- /dev/null > > +++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c > > @@ -0,0 +1,404 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright (c) 2022, Linaro Limited > > + */ > > + > > +#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) > > + <snip> > > + > > +static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata **mdata) > > +{ > > + int ret; > > + u16 primary_mpart = 0, secondary_mpart = 0; > > + > > + ret = gpt_get_mdata_partitions(desc, &primary_mpart, > > + &secondary_mpart); > > + > > + if (ret < 0) { > > + log_err("Error getting the FWU metadata partitions\n"); > > + return -ENODEV; > > + } > > + > > + *mdata = malloc(sizeof(struct fwu_mdata)); > > + if (!*mdata) { > > + log_err("Unable to allocate memory for reading FWU metadata\n"); > > + return -ENOMEM; > > + } > > + > > + ret = gpt_read_mdata(desc, *mdata, primary_mpart); > > + if (ret < 0) { > > + log_err("Failed to read the FWU metadata from the device\n"); > > + return -EIO; > > + } > > + > > + ret = fwu_verify_mdata(*mdata, 1); > > + if (!ret) > > Upon success, I think this function should also either ensure > secondary_part contains a valid copy of primary part, > Maybe this function should call gpt_check_mdata_validity() and then > read mdata content. Okay > > > + 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_err("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 -1; > > +} > > + > > +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; > > + u16 primary_mpart = 0, secondary_mpart = 0; > > + u16 valid_partitions, invalid_partitions; > > + > > + desc = dev_get_uclass_plat(dev_get_priv(dev)); > > + if (!desc) { > > + log_err("Block device not found\n"); > > + return -ENODEV; > > + } > > + > > + /* > > + * 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_err("Error getting the FWU metadata partitions\n"); > > + return -ENODEV; > > + } > > + > > + ret = gpt_read_mdata(desc, &pri_mdata, primary_mpart); > > + if (ret < 0) { > > + log_err("Failed to read the FWU metadata from the device\n"); > > + goto secondary_read; > > + } > > + > > + 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_err("Failed to read the FWU metadata from the device\n"); > > + 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_err("Both FWU metadata copies are valid but do not match. Please check!\n"); > > I think this function should select one of the part and copies to the > other, e.g. assume primary is fine, copy primary to secondary and > return upon success. In this case, I think there is some kind of an unexpected scenario which has resulted in both the metadata partitions being valid but not being the same. Should this not be flagged as an error for the user to handle? Moreover, which metadata partition do we assume to be the correct one . We are just calling one partition as primary, and another one secondary. But the spec does not give any more importance to one over the other -- the spec just says that both the copies should be the same. > > > + } > > + 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_err("Restoring %s FWU metadata partition failed\n", > > + (invalid_partitions == PRIMARY_PART) ? > > + "primary" : "secondary"); > > + > > +out: > > + return ret; > > +} > > + > > +int fwu_gpt_mdata_check(struct udevice *dev) > > Function should be static. Will change all the relevant functions as per your comment. > > > +{ > > + /* > > + * 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); > > +} > > + > > +int fwu_gpt_get_mdata(struct udevice *dev, struct fwu_mdata **mdata) > > Function should be static. > > > +{ > > + struct blk_desc *desc; > > + > > + desc = dev_get_uclass_plat(dev_get_priv(dev)); > > + if (!desc) { > > + log_err("Block device not found\n"); > > + return -ENODEV; > > + } > > + > > + return gpt_get_mdata(desc, mdata); > > +} > > + > > +int fwu_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev) > > Function could be static (and declaration removed from fwu.h). Okay > > > +{ > > + u32 phandle; > > + int ret, size; > > + struct udevice *parent, *child; > > + const fdt32_t *phandle_p = NULL; > > + > > + phandle_p = ofnode_get_property(dev_ofnode(dev), "fwu-mdata-store", > > + &size); > > Should this be retrieved from driver's ::of_to_plat method? Patrick has suggested another API for getting the phandle. Thanks. -sughosh > > Br, > etienne > > > > + if (!phandle_p) { > > + log_err("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; > > + > > + ret = -ENODEV; > > + for (device_find_first_child(parent, &child); child; > > + device_find_next_child(&child)) { > > + if (device_get_uclass_id(child) == UCLASS_BLK) { > > + *mdata_dev = child; > > + ret = 0; > > + } > > + } > > + > > + return ret; > > +} > > + > > +static int fwu_mdata_gpt_blk_probe(struct udevice *dev) > > +{ > > + int ret; > > + struct udevice *mdata_dev = NULL; > > + > > + ret = fwu_get_mdata_device(dev, &mdata_dev); > > + if (ret) > > + return ret; > > + > > + dev_set_priv(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, > > +}; > > diff --git a/include/fwu.h b/include/fwu.h > > index f9e44e7b39..3b1ee4e83e 100644 > > --- a/include/fwu.h > > +++ b/include/fwu.h > > @@ -39,6 +39,8 @@ 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_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev); > > +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); > > -- > > 2.25.1 > >
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 7fec7171f4..12a5b4fe04 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..329bd3779b --- /dev/null +++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c @@ -0,0 +1,404 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (c) 2022, Linaro Limited + */ + +#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 int gpt_get_mdata_partitions(struct blk_desc *desc, + u16 *primary_mpart, + u16 *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_err("Expect two copies of the FWU metadata instead of %d\n", + mdata_parts); + ret = -EINVAL; + } else { + ret = 0; + } + + 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_err("Unable to get the partition info for the FWU metadata part %d", + part_num); + return -1; + } + + /* 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 -1; +} + +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 -ENODEV; + } + + len = sizeof(*mdata); + blkcnt = BLOCK_CNT(len, desc); + if (blkcnt > info.size) { + log_err("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_err("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_err("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; + u16 primary_mpart = 0, secondary_mpart = 0; + + desc = dev_get_uclass_plat(dev_get_priv(dev)); + if (!desc) { + log_err("Block device not found\n"); + return -ENODEV; + } + + ret = gpt_get_mdata_partitions(desc, &primary_mpart, + &secondary_mpart); + + if (ret < 0) { + log_err("Error getting the FWU metadata partitions\n"); + return -ENODEV; + } + + /* First write the primary partition*/ + ret = gpt_write_mdata_partition(desc, mdata, primary_mpart); + if (ret < 0) { + log_err("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_err("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; + u16 primary_mpart = 0, secondary_mpart = 0; + + ret = gpt_get_mdata_partitions(desc, &primary_mpart, + &secondary_mpart); + + if (ret < 0) { + log_err("Error getting the FWU metadata partitions\n"); + return -ENODEV; + } + + *mdata = malloc(sizeof(struct fwu_mdata)); + if (!*mdata) { + log_err("Unable to allocate memory for reading FWU metadata\n"); + return -ENOMEM; + } + + ret = gpt_read_mdata(desc, *mdata, primary_mpart); + if (ret < 0) { + log_err("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_err("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 -1; +} + +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; + u16 primary_mpart = 0, secondary_mpart = 0; + u16 valid_partitions, invalid_partitions; + + desc = dev_get_uclass_plat(dev_get_priv(dev)); + if (!desc) { + log_err("Block device not found\n"); + return -ENODEV; + } + + /* + * 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_err("Error getting the FWU metadata partitions\n"); + return -ENODEV; + } + + ret = gpt_read_mdata(desc, &pri_mdata, primary_mpart); + if (ret < 0) { + log_err("Failed to read the FWU metadata from the device\n"); + goto secondary_read; + } + + 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_err("Failed to read the FWU metadata from the device\n"); + 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_err("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_err("Restoring %s FWU metadata partition failed\n", + (invalid_partitions == PRIMARY_PART) ? + "primary" : "secondary"); + +out: + return ret; +} + +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); +} + +int fwu_gpt_get_mdata(struct udevice *dev, struct fwu_mdata **mdata) +{ + struct blk_desc *desc; + + desc = dev_get_uclass_plat(dev_get_priv(dev)); + if (!desc) { + log_err("Block device not found\n"); + return -ENODEV; + } + + return gpt_get_mdata(desc, mdata); +} + +int fwu_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev) +{ + u32 phandle; + int ret, size; + struct udevice *parent, *child; + const fdt32_t *phandle_p = NULL; + + phandle_p = ofnode_get_property(dev_ofnode(dev), "fwu-mdata-store", + &size); + if (!phandle_p) { + log_err("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; + + ret = -ENODEV; + for (device_find_first_child(parent, &child); child; + device_find_next_child(&child)) { + if (device_get_uclass_id(child) == UCLASS_BLK) { + *mdata_dev = child; + ret = 0; + } + } + + return ret; +} + +static int fwu_mdata_gpt_blk_probe(struct udevice *dev) +{ + int ret; + struct udevice *mdata_dev = NULL; + + ret = fwu_get_mdata_device(dev, &mdata_dev); + if (ret) + return ret; + + dev_set_priv(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, +}; diff --git a/include/fwu.h b/include/fwu.h index f9e44e7b39..3b1ee4e83e 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -39,6 +39,8 @@ 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_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev); +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);
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> --- drivers/fwu-mdata/Kconfig | 9 + drivers/fwu-mdata/Makefile | 1 + drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 404 ++++++++++++++++++++++++++ include/fwu.h | 2 + 4 files changed, 416 insertions(+) create mode 100644 drivers/fwu-mdata/fwu_mdata_gpt_blk.c