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

[Sughosh Ganu]

hi Patrick,

On Wed, 13 Jul 2022 at 18:47, Patrick DELAUNAY
<patrick.delaunay at foss.st.com> wrote:
>
> Hi,
>
> On 7/4/22 07:16, Sughosh Ganu wrote:
> > In the FWU Multi Bank Update feature, the information about the
> > updatable images is stored as part of the metadata, on a separate
> > partition. Add a driver for reading from and writing to the metadata
> > when the updatable images and the metadata are stored on a block
> > device which is formated with GPT based partition scheme.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > ---
> > Changes since V5:
> > * Changed the logic to store the GPT partitioned block device through
> >    a priv structure as suggested by Patrick
> > * Used dev_read_prop() to get the phandle_p instead of
> >    ofnode_get_property() used earlier as suggested by Patrick
> > * Make relevant functions static as suggested by Etienne
> >
> >   drivers/fwu-mdata/Kconfig             |   9 +
> >   drivers/fwu-mdata/Makefile            |   1 +
> >   drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 408 ++++++++++++++++++++++++++
> >   include/fwu.h                         |   5 +
> >   4 files changed, 423 insertions(+)
> >   create mode 100644 drivers/fwu-mdata/fwu_mdata_gpt_blk.c
> >
> > diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig
> > index d6a21c8e19..d5edef19d6 100644
> > --- a/drivers/fwu-mdata/Kconfig
> > +++ b/drivers/fwu-mdata/Kconfig
> > @@ -5,3 +5,12 @@ config DM_FWU_MDATA
> >         Enable support for accessing FWU Metadata partitions. The
> >         FWU Metadata partitions reside on the same storage device
> >         which contains the other FWU updatable firmware images.
> > +
> > +config FWU_MDATA_GPT_BLK
> > +     bool "FWU Metadata access for GPT partitioned Block devices"
> > +     select PARTITION_TYPE_GUID
> > +     select PARTITION_UUIDS
> > +     depends on DM && HAVE_BLOCK_DEVICE && EFI_PARTITION
> > +     help
> > +       Enable support for accessing FWU Metadata on GPT partitioned
> > +       block devices.
> > diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile
> > index e53a8c9983..313049f67a 100644
> > --- a/drivers/fwu-mdata/Makefile
> > +++ b/drivers/fwu-mdata/Makefile
> > @@ -4,3 +4,4 @@
> >   #
> >
> >   obj-$(CONFIG_DM_FWU_MDATA) += fwu-mdata-uclass.o
> > +obj-$(CONFIG_FWU_MDATA_GPT_BLK) += fwu_mdata_gpt_blk.o
> > diff --git a/drivers/fwu-mdata/fwu_mdata_gpt_blk.c b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c
> > new file mode 100644
> > index 0000000000..d904c9492d
> > --- /dev/null
> > +++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c
> > @@ -0,0 +1,408 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2022, Linaro Limited
> > + */
>
> #define LOG_CATEGORY UCLASS_FWU_MDATA

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

>
> > +
> > +#include <blk.h>
> > +#include <dm.h>
> > +#include <efi_loader.h>
> > +#include <fwu.h>
> > +#include <fwu_mdata.h>
> > +#include <log.h>
> > +#include <malloc.h>
> > +#include <memalign.h>
> > +#include <part.h>
> > +#include <part_efi.h>
> > +
> > +#include <dm/device-internal.h>
> > +#include <linux/errno.h>
> > +#include <linux/types.h>
> > +#include <u-boot/crc.h>
> > +
> > +#define PRIMARY_PART         BIT(0)
> > +#define SECONDARY_PART               BIT(1)
> > +#define BOTH_PARTS           (PRIMARY_PART | SECONDARY_PART)
> > +
> > +#define MDATA_READ           BIT(0)
> > +#define MDATA_WRITE          BIT(1)
> > +
>
> (...)
>
>
> > +
> > +static const struct fwu_mdata_ops fwu_gpt_blk_ops = {
> > +     .mdata_check = fwu_gpt_mdata_check,
> > +     .get_mdata = fwu_gpt_get_mdata,
> > +     .update_mdata = fwu_gpt_update_mdata,
> > +};UCLASS_FWU_MDATA
> >
> > +
> > +static const struct udevice_id fwu_mdata_ids[] = {
> > +     { .compatible = "u-boot,fwu-mdata-gpt" },
> > +     { }
> > +};
> > +
> > +U_BOOT_DRIVER(fwu_mdata_gpt_blk) = {
> > +     .name           = "fwu-mdata-gpt-blk",
> > +     .id             = UCLASS_FWU_MDATA,
> > +     .of_match       = fwu_mdata_ids,
> > +     .ops            = &fwu_gpt_blk_ops,
> > +     .probe          = fwu_mdata_gpt_blk_probe,
> > +     .priv_auto      = sizeof(struct fwu_mdata_gpt_blk_priv),
> > +};
> > diff --git a/include/fwu.h b/include/fwu.h
> > index e03cfff800..8259c75d12 100644
> > --- a/include/fwu.h
> > +++ b/include/fwu.h
> > @@ -14,6 +14,10 @@
> >   struct fwu_mdata;
> >   struct udevice;
> >
> > +struct fwu_mdata_gpt_blk_priv {
> > +     struct udevice *blk_dev;
> > +};
> > +
>
> NITS: really needed in .h file => private, only used by driver in .c ?

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

>
>
> >   /**
> >    * @mdata_check: check the validity of the FWU metadata partitions
> >    * @get_mdata() - Get a FWU metadata copy
> > @@ -39,6 +43,7 @@ int fwu_get_active_index(u32 *active_idx);
> >   int fwu_update_active_index(u32 active_idx);
> >   int fwu_get_image_alt_num(efi_guid_t *image_type_id, u32 update_bank,
> >                         int *alt_num);
> > +int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part);
> >   int fwu_mdata_check(void);
> >   int fwu_revert_boot_index(void);
> >   int fwu_accept_image(efi_guid_t *img_type_id, u32 bank);
>
>
> with the minor modification (LOG_CATEGORY)
>
>
> Reviewed-by: Patrick Delaunay <patrick.delaunay at foss.st.com>

Thanks.

-sughosh