[PATCHv4 2/5] fwu: move meta-data management in core

Etienne Carriere etienne.carriere at linaro.org
Tue Feb 28 00:15:13 CET 2023


-       err = fwu_write_mdata(g_dev, mdata, part & PRIMARY_PART ? true : false);

On Mon, 27 Feb 2023 at 17:46, Jassi Brar <jassisinghbrar at gmail.com> wrote:
>
> On Mon, Feb 27, 2023 at 10:30 AM Etienne Carriere
> <etienne.carriere at linaro.org> wrote:
> >
> > Hello Jassi,
> >
> > On Sun, 5 Feb 2023 at 04:01, <jassisinghbrar at gmail.com> wrote:
> > >
> > > From: Jassi Brar <jaswinder.singh at linaro.org>
> > >
> > > Instead of each i/f having to implement their own meta-data verification
> > > and storage, move the logic in common code. This simplifies the i/f code
> > > much simpler and compact.
> > >
> > > Signed-off-by: Jassi Brar <jaswinder.singh at linaro.org>
> > > ---
> > >  drivers/fwu-mdata/fwu-mdata-uclass.c |  34 +++++++
> > >  include/fwu.h                        |  41 ++++++++
> > >  lib/fwu_updates/fwu.c                | 135 ++++++++++++++++++++++++++-
> > >  3 files changed, 206 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c
> > > index b477e9603f..e03773c584 100644
> > > --- a/drivers/fwu-mdata/fwu-mdata-uclass.c
> > > +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
> > > @@ -16,6 +16,40 @@
> > >  #include <linux/types.h>
> > >  #include <u-boot/crc.h>
> > >
> > > +/**
> > > + * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
> > > + *
> > > + * Return: 0 if OK, -ve on error
> > > + */
> > > +int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
> > > +{
> > > +       const struct fwu_mdata_ops *ops = device_get_ops(dev);
> > > +
> > > +       if (!ops->read_mdata) {
> > > +               log_debug("read_mdata() method not defined\n");
> > > +               return -ENOSYS;
> > > +       }
> > > +
> > > +       return ops->read_mdata(dev, mdata, primary);
> > > +}
> > > +
> > > +/**
> > > + * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
> > > + *
> > > + * Return: 0 if OK, -ve on error
> > > + */
> > > +int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary)
> > > +{
> > > +       const struct fwu_mdata_ops *ops = device_get_ops(dev);
> > > +
> > > +       if (!ops->write_mdata) {
> > > +               log_debug("write_mdata() method not defined\n");
> > > +               return -ENOSYS;
> > > +       }
> > > +
> > > +       return ops->write_mdata(dev, mdata, primary);
> > > +}
> > > +
> > >  /**
> > >   * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers
> > >   * @dev: FWU metadata device
> > > diff --git a/include/fwu.h b/include/fwu.h
> > > index 0919ced812..1a700c9e6a 100644
> > > --- a/include/fwu.h
> > > +++ b/include/fwu.h
> > > @@ -24,6 +24,26 @@ struct fwu_mdata_gpt_blk_priv {
> > >   * @update_mdata() - Update the FWU metadata copy
> > >   */
> > >  struct fwu_mdata_ops {
> > > +       /**
> > > +        * read_mdata() - Populate the asked FWU metadata copy
> > > +        * @dev: FWU metadata device
> > > +        * @mdata: Copy of the FWU metadata
> > > +        * @primary: If primary or secondary copy of meta-data is to be read
> > > +        *
> > > +        * Return: 0 if OK, -ve on error
> > > +        */
> > > +       int (*read_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> > > +
> > > +       /**
> > > +        * write_mdata() - Write the given FWU metadata copy
> > > +        * @dev: FWU metadata device
> > > +        * @mdata: Copy of the FWU metadata
> > > +        * @primary: If primary or secondary copy of meta-data is to be written
> > > +        *
> > > +        * Return: 0 if OK, -ve on error
> > > +        */
> > > +       int (*write_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> > > +
> > >         /**
> > >          * check_mdata() - Check if the FWU metadata is valid
> > >          * @dev:        FWU device
> > > @@ -126,6 +146,27 @@ struct fwu_mdata_ops {
> > >         EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \
> > >                  0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8)
> > >
> > > +/**
> > > + * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
> > > + */
> > > +int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> > > +
> > > +/**
> > > + * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
> > > + */
> > > +int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> > > +
> > > +/**
> > > + * fwu_get_verified_mdata() - Read, verify and return the FWU metadata
> > > + *
> > > + * Read both the metadata copies from the storage media, verify their checksum,
> > > + * and ascertain that both copies match. If one of the copies has gone bad,
> > > + * restore it from the good copy.
> > > + *
> > > + * Return: 0 if OK, -ve on error
> > > +*/
> > > +int fwu_get_verified_mdata(struct fwu_mdata *mdata);
> > > +
> > >  /**
> > >   * fwu_check_mdata_validity() - Check for validity of the FWU metadata copies
> > >   *
> > > diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
> > > index 5313d07302..56299f1b2f 100644
> > > --- a/lib/fwu_updates/fwu.c
> > > +++ b/lib/fwu_updates/fwu.c
> > > @@ -15,13 +15,13 @@
> > >  #include <linux/errno.h>
> > >  #include <linux/types.h>
> > >
> > > +#include <u-boot/crc.h>
> > > +
> > > +static struct fwu_mdata g_mdata; /* = {0} makes uninit crc32 always invalid */
> > > +static struct udevice *g_dev;
> > >  static u8 in_trial;
> > >  static u8 boottime_check;
> > >
> > > -#include <linux/errno.h>
> > > -#include <linux/types.h>
> > > -#include <u-boot/crc.h>
> > > -
> > >  enum {
> > >         IMAGE_ACCEPT_SET = 1,
> > >         IMAGE_ACCEPT_CLEAR,
> > > @@ -161,6 +161,133 @@ static int fwu_get_image_type_id(u8 *image_index, efi_guid_t *image_type_id)
> > >         return -ENOENT;
> > >  }
> > >
> > > +/**
> > > + * fwu_sync_mdata() - Update given meta-data partition(s) with the copy provided
> > > + * @mdata: FWU metadata structure
> > > + * @part: Bitmask of FWU metadata partitions to be written to
> > > + *
> > > + * Return: 0 if OK, -ve on error
> > > + */
> > > +static int fwu_sync_mdata(struct fwu_mdata *mdata, int part)
> > > +{
> > > +       void *buf = &mdata->version;
> > > +       int err = 0;
> > > +
> > > +       /*
> > > +        * Calculate the crc32 for the updated FWU metadata
> > > +        * and put the updated value in the FWU metadata crc32
> > > +        * field
> > > +        */
> > > +       mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
> > > +
> > > +       err = fwu_write_mdata(g_dev, mdata, part & PRIMARY_PART ? true : false);
> >
> > There is an issue here as arg @part can have 3 values: PRIMARY_PART,
> > SECONDARY_PART or BOTH_PARTS.
> > The implementation here does not consider the later case BOTH_PATHS.
> > I think something like the below code snippet should do the work:
> >
> >    switch (part) {
> >    case PRIMARY_PART:
> >    case SECONDARY_PART:
> >        err = fwu_write_mdata(g_dev, mdata, part == PRIMARY_PART);
> >        break;
> >    default: /* assuming BOTH_PARTS, or maybe we need sanitization of
> > invalid part values? */
> >        err = fwu_write_mdata(g_dev, mdata, true);
> >         if (!err)
> >            err = fwu_write_mdata(g_dev, mdata, false);
> >         break;
> >    }
> > and  adaptation for the error message below.
> >
> > Or maybe fwu_write_mdata() should not have its 3rd argument and
> > backend handler should directly handle mdata replication update (if
> > needed) on any mdata update requests.
> >
> Good catch. Thanks.
>
> Even simpler fix is ...
>
> static int fwu_sync_mdata(struct fwu_mdata *mdata, int part)
> {
>     if (part == BOTH_PARTS) {
>           fwu_sync_mdata(mdata, PRIMARY_PART);
>           part = SECONDARY_PART;
>     }
>    .....
> }
>
> Thanks.

True :)   , (with error code management)
even if i think it makes the code less straightforward to read out.

maybe also, for simplicity,
-       err = fwu_write_mdata(g_dev, mdata, part & PRIMARY_PART ? true : false);
+       err = fwu_write_mdata(g_dev, mdata, part == PRIMARY_PART);

br,
etienne


More information about the U-Boot mailing list