| Message ID | 20220817124323.375968-4-sughosh.ganu@linaro.org |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | FWU: Add FWU Multi Bank Update feature support | expand |
Hi Sughosh, On Wed, 17 Aug 2022 at 06:44, 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> > Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com> > --- > Changes since V7: None > > drivers/fwu-mdata/Kconfig | 9 + > drivers/fwu-mdata/Makefile | 1 + > drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 410 ++++++++++++++++++++++++++ > include/fwu.h | 5 + > 4 files changed, 425 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. Can we link to the docs here, or will it be easy for people to find in the U-Boot docs? > + > +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. GPT-partitioned (I think) > 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 Perhaps just call it gpt_blk.c since it is in this directory > new file mode 100644 > index 0000000000..f694c4369b > --- /dev/null > +++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c > @@ -0,0 +1,410 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (c) 2022, Linaro Limited > + */ > + > +#define LOG_CATEGORY UCLASS_FWU_MDATA > + > +#include <blk.h> > +#include <dm.h> > +#include <efi_loader.h> > +#include <fwu.h> > +#include <fwu_mdata.h> > +#include <log.h> > +#include <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) > + comment? > +static int gpt_get_mdata_partitions(struct blk_desc *desc, > + u16 *primary_mpart, > + u16 *secondary_mpart) Should use uint, no need to select a 16-bit var so far as I can get > +{ > + 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"; This is fine, but I wonder if it should that go in a header with all the other GUIDs? Or does it go hear because it is a string? > + > + 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; How about -ENOENT ? There is a device, so we cannot use -ENODEV Please fix below too > + } > + > + 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; uint > + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev); > + > + desc = dev_get_uclass_plat(priv->blk_dev); > + if (!desc) { > + log_err("Block device not found\n"); > + return -ENODEV; This cannot happen, drop this if() > + } > + > + ret = gpt_get_mdata_partitions(desc, &primary_mpart, > + &secondary_mpart); > + drop blank line so the error check is next to the thing that reads it > + if (ret < 0) { > + log_err("Error getting the FWU metadata partitions\n"); > + return -ENODEV; > + } > + > + /* First write the primary partition*/ space before */ > + ret = gpt_write_mdata_partition(desc, mdata, primary_mpart); > + if (ret < 0) { > + log_err("Updating primary FWU metadata partition failed\n"); This is very error-heavy. The log_err() things produce a string in the code so this ends up being very large. Is that what you want? If these things can happen in the real world and users need to debug them, then I suppose it is OK. But normally with driver model we report errors in the command code, or the logic, not in drivers themselves. Otherwise when you write tests you get lots of confusing error output that needs to be ignored. > + 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) mdatap > +{ > + int ret; > + u16 primary_mpart = 0, secondary_mpart = 0; uint Also do you need to set them to 0 Declare a locate mdata and use that here instead of *mdata, which is a pain. > + > + 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)); '\0' > + 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; This is -EPERM Perhaps choose something else? > +} > + > +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; > + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev); > + > + desc = dev_get_uclass_plat(priv->blk_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"); Honestly the error handling needs a rethink IMO. You should try to return error codes that indicate what went wrong (you mostly do in this file) and then have the top-level code decode those errors and produce an error message. > + } > + 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; > +} > + > +static int fwu_gpt_mdata_check(struct udevice *dev) > +{ > + /* > + * Check if both the copies of the FWU metadata are > + * valid. If one has gone bad, restore it from the > + * other good copy. word wrap wider > + */ > + return gpt_check_mdata_validity(dev); > +} > + > +static int fwu_gpt_get_mdata(struct udevice *dev, struct fwu_mdata **mdata) > +{ > + struct blk_desc *desc; > + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev); > + > + desc = dev_get_uclass_plat(priv->blk_dev); > + if (!desc) { > + log_err("Block device not found\n"); > + return -ENODEV; Again this cannot happen > + } > + > + return gpt_get_mdata(desc, mdata); > +} > + > +static 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 = dev_read_prop(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); Can you use dev_read_u32() ? > + > + ret = device_get_global_by_ofnode(ofnode_get_by_phandle(phandle), > + &parent); > + if (ret) > + return ret; > + > + ret = -ENODEV; Here it is correct to return -ENODEV I think > + 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; See blk_get_from_parent() > + } > + } > + > + return ret; > +} > + > +static int fwu_mdata_gpt_blk_probe(struct udevice *dev) > +{ > + int ret; > + struct udevice *mdata_dev = NULL; > + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev); > + > + ret = fwu_get_mdata_device(dev, &mdata_dev); > + if (ret) > + return ret; > + > + priv->blk_dev = mdata_dev; > + > + return 0; > +} > + > +static const struct fwu_mdata_ops fwu_gpt_blk_ops = { > + .mdata_check = fwu_gpt_mdata_check, > + .get_mdata = fwu_gpt_get_mdata, > + .update_mdata = fwu_gpt_update_mdata, > +}; > + > +static const struct udevice_id fwu_mdata_ids[] = { > + { .compatible = "u-boot,fwu-mdata-gpt" }, > + { } > +}; > + > +U_BOOT_DRIVER(fwu_mdata_gpt_blk) = { > + .name = "fwu-mdata-gpt-blk", > + .id = UCLASS_FWU_MDATA, > + .of_match = fwu_mdata_ids, > + .ops = &fwu_gpt_blk_ops, > + .probe = fwu_mdata_gpt_blk_probe, > + .priv_auto = sizeof(struct fwu_mdata_gpt_blk_priv), > +}; > diff --git a/include/fwu.h b/include/fwu.h > index 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; > +}; > + > /** > * @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); comments again > int fwu_mdata_check(void); > int fwu_revert_boot_index(void); > int fwu_accept_image(efi_guid_t *img_type_id, u32 bank); > -- > 2.34.1 > Regards, Simon
hi Simon, On Thu, 18 Aug 2022 at 08:51, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Wed, 17 Aug 2022 at 06:44, 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> > > Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com> > > --- > > Changes since V7: None > > > > drivers/fwu-mdata/Kconfig | 9 + > > drivers/fwu-mdata/Makefile | 1 + > > drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 410 ++++++++++++++++++++++++++ > > include/fwu.h | 5 + > > 4 files changed, 425 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. > > Can we link to the docs here, or will it be easy for people to find in > the U-Boot docs? The link to the spec is being provided in the documentation for the feature. I guess that should suffice. > > > + > > +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. > > GPT-partitioned (I think) I see "GPT partition" being used elsewhere. I don't have a strong opinion on this though. > > > 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 > > Perhaps just call it gpt_blk.c since it is in this directory Actually, there are both type of examples that can be seen under drivers/. For e.g. the drivers/clk/ has all files starting with clk_*. Similarly, under drivers/reset/. But there are other examples as well. But this is not a big effort. I will change the name as per your suggestion. > > > new file mode 100644 > > index 0000000000..f694c4369b > > --- /dev/null > > +++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c > > @@ -0,0 +1,410 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * Copyright (c) 2022, Linaro Limited > > + */ > > + > > +#define LOG_CATEGORY UCLASS_FWU_MDATA > > + > > +#include <blk.h> > > +#include <dm.h> > > +#include <efi_loader.h> > > +#include <fwu.h> > > +#include <fwu_mdata.h> > > +#include <log.h> > > +#include <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) > > + > > comment? Umm, is the macro name not descriptive enough? They are just being used to select a read or write operation. > > > +static int gpt_get_mdata_partitions(struct blk_desc *desc, > > + u16 *primary_mpart, > > + u16 *secondary_mpart) > > Should use uint, no need to select a 16-bit var so far as I can get Okay > > > +{ > > + 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"; > > This is fine, but I wonder if it should that go in a header with all > the other GUIDs? Or does it go hear because it is a string? Yes, this is because it is a string > > > + > > + 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; > > How about -ENOENT ? > > There is a device, so we cannot use -ENODEV > > Please fix below too Okay > > > + } > > + > > + 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; > > uint Okay > > > + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev); > > + > > + desc = dev_get_uclass_plat(priv->blk_dev); > > + if (!desc) { > > + log_err("Block device not found\n"); > > + return -ENODEV; > > This cannot happen, drop this if() Okay > > > + } > > + > > + ret = gpt_get_mdata_partitions(desc, &primary_mpart, > > + &secondary_mpart); > > + > > drop blank line so the error check is next to the thing that reads it Will fix > > > + if (ret < 0) { > > + log_err("Error getting the FWU metadata partitions\n"); > > + return -ENODEV; > > + } > > + > > + /* First write the primary partition*/ > > space before */ Will fix > > > + ret = gpt_write_mdata_partition(desc, mdata, primary_mpart); > > + if (ret < 0) { > > + log_err("Updating primary FWU metadata partition failed\n"); > > This is very error-heavy. The log_err() things produce a string in the > code so this ends up being very large. Is that what you want? If these > things can happen in the real world and users need to debug them, then > I suppose it is OK. But normally with driver model we report errors in > the command code, or the logic, not in drivers themselves. Otherwise > when you write tests you get lots of confusing error output that needs > to be ignored. I see your point, but in this specific case, it is only the driver which will be able to tell about the error in some detail. I can change this to a log_debug if you so wish. Although I do see quite a few instances of log_err being used in the driver code as well. > > > + 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) > > mdatap Okay > > > +{ > > + int ret; > > + u16 primary_mpart = 0, secondary_mpart = 0; > > uint Okay > > Also do you need to set them to 0 > > Declare a locate mdata and use that here instead of *mdata, which is a pain. This function is copying the metadata into the callers copy. Unless I don't understand your comment. > > > + > > + 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)); > > '\0' Okay > > > + 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; > > This is -EPERM > > Perhaps choose something else? Again, I am not sure what would be a suitable error to return in this case. > > > +} > > + > > +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; > > + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev); > > + > > + desc = dev_get_uclass_plat(priv->blk_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"); > > Honestly the error handling needs a rethink IMO. You should try to > return error codes that indicate what went wrong (you mostly do in > this file) and then have the top-level code decode those errors and > produce an error message. The code in efi_capsule.c which is calling the APIs is checking the error codes and printing the error messages. > > > + } > > + 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; > > +} > > + > > +static int fwu_gpt_mdata_check(struct udevice *dev) > > +{ > > + /* > > + * Check if both the copies of the FWU metadata are > > + * valid. If one has gone bad, restore it from the > > + * other good copy. > > word wrap wider Okay > > > + */ > > + return gpt_check_mdata_validity(dev); > > +} > > + > > +static int fwu_gpt_get_mdata(struct udevice *dev, struct fwu_mdata **mdata) > > +{ > > + struct blk_desc *desc; > > + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev); > > + > > + desc = dev_get_uclass_plat(priv->blk_dev); > > + if (!desc) { > > + log_err("Block device not found\n"); > > + return -ENODEV; > > Again this cannot happen Will remove > > > + } > > + > > + return gpt_get_mdata(desc, mdata); > > +} > > + > > +static 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 = dev_read_prop(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); > > Can you use dev_read_u32() ? Will check if this can be used. > > > + > > + ret = device_get_global_by_ofnode(ofnode_get_by_phandle(phandle), > > + &parent); > > + if (ret) > > + return ret; > > + > > + ret = -ENODEV; > > Here it is correct to return -ENODEV I think > > > + 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; > > See blk_get_from_parent() Will check > > > + } > > + } > > + > > + return ret; > > +} > > + > > +static int fwu_mdata_gpt_blk_probe(struct udevice *dev) > > +{ > > + int ret; > > + struct udevice *mdata_dev = NULL; > > + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev); > > + > > + ret = fwu_get_mdata_device(dev, &mdata_dev); > > + if (ret) > > + return ret; > > + > > + priv->blk_dev = mdata_dev; > > + > > + return 0; > > +} > > + > > +static const struct fwu_mdata_ops fwu_gpt_blk_ops = { > > + .mdata_check = fwu_gpt_mdata_check, > > + .get_mdata = fwu_gpt_get_mdata, > > + .update_mdata = fwu_gpt_update_mdata, > > +}; > > + > > +static const struct udevice_id fwu_mdata_ids[] = { > > + { .compatible = "u-boot,fwu-mdata-gpt" }, > > + { } > > +}; > > + > > +U_BOOT_DRIVER(fwu_mdata_gpt_blk) = { > > + .name = "fwu-mdata-gpt-blk", > > + .id = UCLASS_FWU_MDATA, > > + .of_match = fwu_mdata_ids, > > + .ops = &fwu_gpt_blk_ops, > > + .probe = fwu_mdata_gpt_blk_probe, > > + .priv_auto = sizeof(struct fwu_mdata_gpt_blk_priv), > > +}; > > diff --git a/include/fwu.h b/include/fwu.h > > index 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; > > +}; > > + > > /** > > * @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); > > comments again Will add -sughosh > > > int fwu_mdata_check(void); > > int fwu_revert_boot_index(void); > > int fwu_accept_image(efi_guid_t *img_type_id, u32 bank); > > -- > > 2.34.1 > > > > Regards, > Simon
Hi Sughosh, On Thu, 18 Aug 2022 at 05:39, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > hi Simon, > > On Thu, 18 Aug 2022 at 08:51, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Wed, 17 Aug 2022 at 06:44, 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> > > > Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com> > > > --- > > > Changes since V7: None > > > > > > drivers/fwu-mdata/Kconfig | 9 + > > > drivers/fwu-mdata/Makefile | 1 + > > > drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 410 ++++++++++++++++++++++++++ > > > include/fwu.h | 5 + > > > 4 files changed, 425 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. > > > > Can we link to the docs here, or will it be easy for people to find in > > the U-Boot docs? > > The link to the spec is being provided in the documentation for the > feature. I guess that should suffice. Yes it's the U-Boot documentation that I am referring to. I assume people can type 'FWU metadata' in the docs and it will come up. If so, that seems OK. > > > > > > + > > > +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. > > > > GPT-partitioned (I think) > > I see "GPT partition" being used elsewhere. I don't have a strong > opinion on this though. > > > > > > 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 > > > > Perhaps just call it gpt_blk.c since it is in this directory > > Actually, there are both type of examples that can be seen under > drivers/. For e.g. the drivers/clk/ has all files starting with clk_*. > Similarly, under drivers/reset/. But there are other examples as well. > But this is not a big effort. I will change the name as per your > suggestion. > > > > > > new file mode 100644 > > > index 0000000000..f694c4369b > > > --- /dev/null > > > +++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c > > > @@ -0,0 +1,410 @@ > > > +// SPDX-License-Identifier: GPL-2.0-or-later > > > +/* > > > + * Copyright (c) 2022, Linaro Limited > > > + */ > > > + > > > +#define LOG_CATEGORY UCLASS_FWU_MDATA > > > + > > > +#include <blk.h> > > > +#include <dm.h> > > > +#include <efi_loader.h> > > > +#include <fwu.h> > > > +#include <fwu_mdata.h> > > > +#include <log.h> > > > +#include <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) > > > + > > > > comment? > > Umm, is the macro name not descriptive enough? They are just being > used to select a read or write operation. I mean the function below > > > > > > +static int gpt_get_mdata_partitions(struct blk_desc *desc, > > > + u16 *primary_mpart, > > > + u16 *secondary_mpart) > > > > Should use uint, no need to select a 16-bit var so far as I can get > > Okay > [..] > > > +static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata **mdata) > > > > mdatap > > Okay > > > > > > +{ > > > + int ret; > > > + u16 primary_mpart = 0, secondary_mpart = 0; > > > > uint > > Okay > > > > > Also do you need to set them to 0 > > > > Declare a locate mdata and use that here instead of *mdata, which is a pain. > > This function is copying the metadata into the callers copy. Unless I > don't understand your comment. I mean it is easier to do this (the pattern common in driver model); static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata **mdatap) { struct fwu_mdata *mdata; mdata = malloc(sizeof(struct fwu_mdata)); ... (use mdata throughout function) *mdatap = mdata; return 0; } [..] > > > > > + 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; > > > > This is -EPERM > > > > Perhaps choose something else? > > Again, I am not sure what would be a suitable error to return in this case. Take a look at errno.h and pick one. Maybe -ELIBBAD ? It doesn't matter much, so long as your upper-level command code can decode the error and report it. > > > > > > +} > > > + > > > +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; > > > + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev); > > > + > > > + desc = dev_get_uclass_plat(priv->blk_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"); > > > > Honestly the error handling needs a rethink IMO. You should try to > > return error codes that indicate what went wrong (you mostly do in > > this file) and then have the top-level code decode those errors and > > produce an error message. > > The code in efi_capsule.c which is calling the APIs is checking the > error codes and printing the error messages. log_err() prints a message, though. Change this to log_debug() and it won't appear unless specifically enabled. [..] Regards, Simon
hi Simon, On Thu, 18 Aug 2022 at 23:19, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Thu, 18 Aug 2022 at 05:39, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > hi Simon, > > > > On Thu, 18 Aug 2022 at 08:51, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Wed, 17 Aug 2022 at 06:44, 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> > > > > Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com> > > > > --- > > > > Changes since V7: None > > > > > > > > drivers/fwu-mdata/Kconfig | 9 + > > > > drivers/fwu-mdata/Makefile | 1 + > > > > drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 410 ++++++++++++++++++++++++++ > > > > include/fwu.h | 5 + > > > > 4 files changed, 425 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. > > > > > > Can we link to the docs here, or will it be easy for people to find in > > > the U-Boot docs? > > > > The link to the spec is being provided in the documentation for the > > feature. I guess that should suffice. > > Yes it's the U-Boot documentation that I am referring to. I assume people can type 'FWU metadata' in the docs and it will come up. If so, that seems OK. > > > > > > > > > > + > > > > +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. > > > > > > GPT-partitioned (I think) > > > > I see "GPT partition" being used elsewhere. I don't have a strong > > opinion on this though. > > > > > > > > > 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 > > > > > > Perhaps just call it gpt_blk.c since it is in this directory > > > > Actually, there are both type of examples that can be seen under > > drivers/. For e.g. the drivers/clk/ has all files starting with clk_*. > > Similarly, under drivers/reset/. But there are other examples as well. > > But this is not a big effort. I will change the name as per your > > suggestion. > > > > > > > > > new file mode 100644 > > > > index 0000000000..f694c4369b > > > > --- /dev/null > > > > +++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c > > > > @@ -0,0 +1,410 @@ > > > > +// SPDX-License-Identifier: GPL-2.0-or-later > > > > +/* > > > > + * Copyright (c) 2022, Linaro Limited > > > > + */ > > > > + > > > > +#define LOG_CATEGORY UCLASS_FWU_MDATA > > > > + > > > > +#include <blk.h> > > > > +#include <dm.h> > > > > +#include <efi_loader.h> > > > > +#include <fwu.h> > > > > +#include <fwu_mdata.h> > > > > +#include <log.h> > > > > +#include <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) > > > > + > > > > > > comment? > > > > Umm, is the macro name not descriptive enough? They are just being > > used to select a read or write operation. > > I mean the function below > > > > > > > > > > +static int gpt_get_mdata_partitions(struct blk_desc *desc, > > > > + u16 *primary_mpart, > > > > + u16 *secondary_mpart) > > > > > > Should use uint, no need to select a 16-bit var so far as I can get > > > > Okay > > > > [..] > > > > > +static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata **mdata) > > > > > > mdatap > > > > Okay > > > > > > > > > +{ > > > > + int ret; > > > > + u16 primary_mpart = 0, secondary_mpart = 0; > > > > > > uint > > > > Okay > > > > > > > > Also do you need to set them to 0 > > > > > > Declare a locate mdata and use that here instead of *mdata, which is a pain. > > > > This function is copying the metadata into the callers copy. Unless I > > don't understand your comment. > > I mean it is easier to do this (the pattern common in driver model); > > static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata **mdatap) > { > struct fwu_mdata *mdata; > > mdata = malloc(sizeof(struct fwu_mdata)); > ... (use mdata throughout function) > > *mdatap = mdata; > > return 0; > } Fair enough. Based on your comment in the other patch, I will look at the code to see where I can use the stack instead of allocating memory for the metadata. > > [..] > > > > > > > > + 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; > > > > > > This is -EPERM > > > > > > Perhaps choose something else? > > > > Again, I am not sure what would be a suitable error to return in this case. > > Take a look at errno.h and pick one. Maybe -ELIBBAD ? > > It doesn't matter much, so long as your upper-level command code can decode the error and report it. Okay. I will change log_err to log_debug in the driver code. -sughosh > > > > > > > > > > +} > > > > + > > > > +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; > > > > + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev); > > > > + > > > > + desc = dev_get_uclass_plat(priv->blk_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"); > > > > > > Honestly the error handling needs a rethink IMO. You should try to > > > return error codes that indicate what went wrong (you mostly do in > > > this file) and then have the top-level code decode those errors and > > > produce an error message. > > > > The code in efi_capsule.c which is calling the APIs is checking the > > error codes and printing the error messages. > > log_err() prints a message, though. Change this to log_debug() and it won't appear unless specifically enabled. > > [..] > > Regards, > Simon
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..f694c4369b --- /dev/null +++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c @@ -0,0 +1,410 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2022, Linaro Limited + */ + +#define LOG_CATEGORY UCLASS_FWU_MDATA + +#include <blk.h> +#include <dm.h> +#include <efi_loader.h> +#include <fwu.h> +#include <fwu_mdata.h> +#include <log.h> +#include <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; + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev); + + desc = dev_get_uclass_plat(priv->blk_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; + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev); + + desc = dev_get_uclass_plat(priv->blk_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; +} + +static int fwu_gpt_mdata_check(struct udevice *dev) +{ + /* + * Check if both the copies of the FWU metadata are + * valid. If one has gone bad, restore it from the + * other good copy. + */ + return gpt_check_mdata_validity(dev); +} + +static int fwu_gpt_get_mdata(struct udevice *dev, struct fwu_mdata **mdata) +{ + struct blk_desc *desc; + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev); + + 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); +} + +static 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 = dev_read_prop(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; + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev); + + ret = fwu_get_mdata_device(dev, &mdata_dev); + if (ret) + return ret; + + priv->blk_dev = mdata_dev; + + return 0; +} + +static const struct fwu_mdata_ops fwu_gpt_blk_ops = { + .mdata_check = fwu_gpt_mdata_check, + .get_mdata = fwu_gpt_get_mdata, + .update_mdata = fwu_gpt_update_mdata, +}; + +static const struct udevice_id fwu_mdata_ids[] = { + { .compatible = "u-boot,fwu-mdata-gpt" }, + { } +}; + +U_BOOT_DRIVER(fwu_mdata_gpt_blk) = { + .name = "fwu-mdata-gpt-blk", + .id = UCLASS_FWU_MDATA, + .of_match = fwu_mdata_ids, + .ops = &fwu_gpt_blk_ops, + .probe = fwu_mdata_gpt_blk_probe, + .priv_auto = sizeof(struct fwu_mdata_gpt_blk_priv), +}; diff --git a/include/fwu.h b/include/fwu.h index 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; +}; + /** * @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);