Re: [PATCH v6 03/13] FWU: Add FWU metadata access driver for GPT partitioned block devices
hi Patrick,
On Wed, 13 Jul 2022 at 18:47, Patrick DELAUNAY
wrote:
>
> Hi,
>
> On 7/4/22 07:16, Sughosh Ganu wrote:
> > In the FWU Multi Bank Update feature, the information about the
> > updatable images is stored as part of the metadata, on a separate
> > partition. Add a driver for reading from and writing to the metadata
> > when the updatable images and the metadata are stored on a block
> > device which is formated with GPT based partition scheme.
> >
> > Signed-off-by: Sughosh Ganu
> > ---
> > Changes since V5:
> > * Changed the logic to store the GPT partitioned block device through
> >a priv structure as suggested by Patrick
> > * Used dev_read_prop() to get the phandle_p instead of
> >ofnode_get_property() used earlier as suggested by Patrick
> > * Make relevant functions static as suggested by Etienne
> >
> > drivers/fwu-mdata/Kconfig | 9 +
> > drivers/fwu-mdata/Makefile| 1 +
> > drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 408 ++
> > include/fwu.h | 5 +
> > 4 files changed, 423 insertions(+)
> > create mode 100644 drivers/fwu-mdata/fwu_mdata_gpt_blk.c
> >
> > diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig
> > index d6a21c8e19..d5edef19d6 100644
> > --- a/drivers/fwu-mdata/Kconfig
> > +++ b/drivers/fwu-mdata/Kconfig
> > @@ -5,3 +5,12 @@ config DM_FWU_MDATA
> > Enable support for accessing FWU Metadata partitions. The
> > FWU Metadata partitions reside on the same storage device
> > which contains the other FWU updatable firmware images.
> > +
> > +config FWU_MDATA_GPT_BLK
> > + bool "FWU Metadata access for GPT partitioned Block devices"
> > + select PARTITION_TYPE_GUID
> > + select PARTITION_UUIDS
> > + depends on DM && HAVE_BLOCK_DEVICE && EFI_PARTITION
> > + help
> > + Enable support for accessing FWU Metadata on GPT partitioned
> > + block devices.
> > diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile
> > index e53a8c9983..313049f67a 100644
> > --- a/drivers/fwu-mdata/Makefile
> > +++ b/drivers/fwu-mdata/Makefile
> > @@ -4,3 +4,4 @@
> > #
> >
> > obj-$(CONFIG_DM_FWU_MDATA) += fwu-mdata-uclass.o
> > +obj-$(CONFIG_FWU_MDATA_GPT_BLK) += fwu_mdata_gpt_blk.o
> > diff --git a/drivers/fwu-mdata/fwu_mdata_gpt_blk.c
> > b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c
> > new file mode 100644
> > index 00..d904c9492d
> > --- /dev/null
> > +++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c
> > @@ -0,0 +1,408 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2022, Linaro Limited
> > + */
>
> #define LOG_CATEGORY UCLASS_FWU_MDATA
I think you had mentioned this in your earlier review as well. Sorry,
I missed out on this. Will add in the next version.
>
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define PRIMARY_PART BIT(0)
> > +#define SECONDARY_PART BIT(1)
> > +#define BOTH_PARTS (PRIMARY_PART | SECONDARY_PART)
> > +
> > +#define MDATA_READ BIT(0)
> > +#define MDATA_WRITE BIT(1)
> > +
>
> (...)
>
>
> > +
> > +static const struct fwu_mdata_ops fwu_gpt_blk_ops = {
> > + .mdata_check = fwu_gpt_mdata_check,
> > + .get_mdata = fwu_gpt_get_mdata,
> > + .update_mdata = fwu_gpt_update_mdata,
> > +};UCLASS_FWU_MDATA
> >
> > +
> > +static const struct udevice_id fwu_mdata_ids[] = {
> > + { .compatible = "u-boot,fwu-mdata-gpt" },
> > + { }
> > +};
> > +
> > +U_BOOT_DRIVER(fwu_mdata_gpt_blk) = {
> > + .name = "fwu-mdata-gpt-blk",
> > + .id = UCLASS_FWU_MDATA,
> > + .of_match = fwu_mdata_ids,
> > + .ops= _gpt_blk_ops,
> > + .probe = fwu_mdata_gpt_blk_probe,
> > + .priv_auto = sizeof(struct fwu_mdata_gpt_blk_priv),
> > +};
> > diff --git a/include/fwu.h b/include/fwu.h
> > index e03cfff800..8259c75d12 100644
> > --- a/include/fwu.h
> > +++ b/include/fwu.h
> > @@ -14,6 +14,10 @@
> > struct fwu_mdata;
> > struct udevice;
> >
> > +struct fwu_mdata_gpt_blk_priv {
> > + struct udevice *blk_dev;
> > +};
> > +
>
> NITS: really needed in .h file => private, only used by driver in .c ?
I have put it here since this gets accessed in the board file as well
to retrieve the block device which has the fwu metadata and the
firmware images in fwu_plat_get_alt_num().
>
>
> > /**
> >* @mdata_check: check the validity of the FWU metadata partitions
> >* @get_mdata() - Get a FWU metadata copy
> > @@ -39,6 +43,7 @@ int fwu_get_active_index(u32 *active_idx);
> > int fwu_update_active_index(u32 active_idx);
> > int fwu_get_image_alt_num(efi_guid_t *image_type_id, u32 update_bank,
> > int *alt_num);
> > +int fwu_verify_mdata(struct fwu_mdata
Re: [PATCH v6 03/13] FWU: Add FWU metadata access driver for GPT partitioned block devices
Hi,
On 7/4/22 07:16, Sughosh Ganu wrote:
In the FWU Multi Bank Update feature, the information about the
updatable images is stored as part of the metadata, on a separate
partition. Add a driver for reading from and writing to the metadata
when the updatable images and the metadata are stored on a block
device which is formated with GPT based partition scheme.
Signed-off-by: Sughosh Ganu
---
Changes since V5:
* Changed the logic to store the GPT partitioned block device through
a priv structure as suggested by Patrick
* Used dev_read_prop() to get the phandle_p instead of
ofnode_get_property() used earlier as suggested by Patrick
* Make relevant functions static as suggested by Etienne
drivers/fwu-mdata/Kconfig | 9 +
drivers/fwu-mdata/Makefile| 1 +
drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 408 ++
include/fwu.h | 5 +
4 files changed, 423 insertions(+)
create mode 100644 drivers/fwu-mdata/fwu_mdata_gpt_blk.c
diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig
index d6a21c8e19..d5edef19d6 100644
--- a/drivers/fwu-mdata/Kconfig
+++ b/drivers/fwu-mdata/Kconfig
@@ -5,3 +5,12 @@ config DM_FWU_MDATA
Enable support for accessing FWU Metadata partitions. The
FWU Metadata partitions reside on the same storage device
which contains the other FWU updatable firmware images.
+
+config FWU_MDATA_GPT_BLK
+ bool "FWU Metadata access for GPT partitioned Block devices"
+ select PARTITION_TYPE_GUID
+ select PARTITION_UUIDS
+ depends on DM && HAVE_BLOCK_DEVICE && EFI_PARTITION
+ help
+ Enable support for accessing FWU Metadata on GPT partitioned
+ block devices.
diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile
index e53a8c9983..313049f67a 100644
--- a/drivers/fwu-mdata/Makefile
+++ b/drivers/fwu-mdata/Makefile
@@ -4,3 +4,4 @@
#
obj-$(CONFIG_DM_FWU_MDATA) += fwu-mdata-uclass.o
+obj-$(CONFIG_FWU_MDATA_GPT_BLK) += fwu_mdata_gpt_blk.o
diff --git a/drivers/fwu-mdata/fwu_mdata_gpt_blk.c
b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c
new file mode 100644
index 00..d904c9492d
--- /dev/null
+++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c
@@ -0,0 +1,408 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022, Linaro Limited
+ */
#define LOG_CATEGORY UCLASS_FWU_MDATA
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+
+#define PRIMARY_PART BIT(0)
+#define SECONDARY_PART BIT(1)
+#define BOTH_PARTS (PRIMARY_PART | SECONDARY_PART)
+
+#define MDATA_READ BIT(0)
+#define MDATA_WRITEBIT(1)
+
(...)
+
+static const struct fwu_mdata_ops fwu_gpt_blk_ops = {
+ .mdata_check = fwu_gpt_mdata_check,
+ .get_mdata = fwu_gpt_get_mdata,
+ .update_mdata = fwu_gpt_update_mdata,
+};UCLASS_FWU_MDATA
+
+static const struct udevice_id fwu_mdata_ids[] = {
+ { .compatible = "u-boot,fwu-mdata-gpt" },
+ { }
+};
+
+U_BOOT_DRIVER(fwu_mdata_gpt_blk) = {
+ .name = "fwu-mdata-gpt-blk",
+ .id = UCLASS_FWU_MDATA,
+ .of_match = fwu_mdata_ids,
+ .ops= _gpt_blk_ops,
+ .probe = fwu_mdata_gpt_blk_probe,
+ .priv_auto = sizeof(struct fwu_mdata_gpt_blk_priv),
+};
diff --git a/include/fwu.h b/include/fwu.h
index e03cfff800..8259c75d12 100644
--- a/include/fwu.h
+++ b/include/fwu.h
@@ -14,6 +14,10 @@
struct fwu_mdata;
struct udevice;
+struct fwu_mdata_gpt_blk_priv {
+ struct udevice *blk_dev;
+};
+
NITS: really needed in .h file => private, only used by driver in .c ?
/**
* @mdata_check: check the validity of the FWU metadata partitions
* @get_mdata() - Get a FWU metadata copy
@@ -39,6 +43,7 @@ int fwu_get_active_index(u32 *active_idx);
int fwu_update_active_index(u32 active_idx);
int fwu_get_image_alt_num(efi_guid_t *image_type_id, u32 update_bank,
int *alt_num);
+int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part);
int fwu_mdata_check(void);
int fwu_revert_boot_index(void);
int fwu_accept_image(efi_guid_t *img_type_id, u32 bank);
with the minor modification (LOG_CATEGORY)
Reviewed-by: Patrick Delaunay
Thanks
Patrick
[PATCH v6 03/13] FWU: Add FWU metadata access driver for GPT partitioned block devices
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
---
Changes since V5:
* Changed the logic to store the GPT partitioned block device through
a priv structure as suggested by Patrick
* Used dev_read_prop() to get the phandle_p instead of
ofnode_get_property() used earlier as suggested by Patrick
* Make relevant functions static as suggested by Etienne
drivers/fwu-mdata/Kconfig | 9 +
drivers/fwu-mdata/Makefile| 1 +
drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 408 ++
include/fwu.h | 5 +
4 files changed, 423 insertions(+)
create mode 100644 drivers/fwu-mdata/fwu_mdata_gpt_blk.c
diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig
index d6a21c8e19..d5edef19d6 100644
--- a/drivers/fwu-mdata/Kconfig
+++ b/drivers/fwu-mdata/Kconfig
@@ -5,3 +5,12 @@ config DM_FWU_MDATA
Enable support for accessing FWU Metadata partitions. The
FWU Metadata partitions reside on the same storage device
which contains the other FWU updatable firmware images.
+
+config FWU_MDATA_GPT_BLK
+ bool "FWU Metadata access for GPT partitioned Block devices"
+ select PARTITION_TYPE_GUID
+ select PARTITION_UUIDS
+ depends on DM && HAVE_BLOCK_DEVICE && EFI_PARTITION
+ help
+ Enable support for accessing FWU Metadata on GPT partitioned
+ block devices.
diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile
index e53a8c9983..313049f67a 100644
--- a/drivers/fwu-mdata/Makefile
+++ b/drivers/fwu-mdata/Makefile
@@ -4,3 +4,4 @@
#
obj-$(CONFIG_DM_FWU_MDATA) += fwu-mdata-uclass.o
+obj-$(CONFIG_FWU_MDATA_GPT_BLK) += fwu_mdata_gpt_blk.o
diff --git a/drivers/fwu-mdata/fwu_mdata_gpt_blk.c
b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c
new file mode 100644
index 00..d904c9492d
--- /dev/null
+++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c
@@ -0,0 +1,408 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022, Linaro Limited
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+
+#define PRIMARY_PART BIT(0)
+#define SECONDARY_PART BIT(1)
+#define BOTH_PARTS (PRIMARY_PART | SECONDARY_PART)
+
+#define MDATA_READ BIT(0)
+#define MDATA_WRITEBIT(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, ))
+ continue;
+ uuid_str_to_bin(info.type_guid, part_type_guid.b,
+ UUID_STR_FORMAT_GUID);
+
+ if (!guidcmp(_mdata_guid, _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,
+
