[PATCH v5 3/6] fwu: move meta-data management in core

[Jassi Brar]

On Wed, Mar 1, 2023 at 5:16 AM Etienne Carriere
<etienne.carriere at linaro.org> wrote:
> On Tue, 28 Feb 2023 at 01:52, <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.
> >
...

> > 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
> > + */
>
> Inline description only in header file, or duplicated in source and
> header files?
>
This is the original practice in FWU stack - to have the description
in header as well as source code. I just didn't want to stick out.


> > 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
>
> @mdata: Output FWU mdata read
>
> > +        * @primary: If primary or secondary copy of meta-data is to be read
>
> s/meta-data/FWU metadata/
> Ditto in .write_mdata description
>
ok

> > +/**
> > + * 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.
>
> @mdata: Output FWU metadata read or NULL
>
ok

> > +static int fwu_sync_mdata(struct fwu_mdata *mdata, int part)
> > +{
> > +       void *buf = &mdata->version;
> > +       int err;
> > +
> > +       if (part == BOTH_PARTS) {
> > +               err = fwu_sync_mdata(mdata, SECONDARY_PART);
> > +               if (err)
> > +                       return err;
> > +               part = PRIMARY_PART;
> > +       }
> > +
> > +       /*
> > +        * 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);
>
> Since this expects part is either PRIMARY_PART or SECONDARY_PART, prefer:
>  err = fwu_write_mdata(g_dev, mdata, part == PRIMARY_PART);
>
> And ditto below:
>       part == PRIMARY_PART ? "primary": "secondary");
>
yes, now that we handle out the BOTH_PARTS case already.


> > +int fwu_get_verified_mdata(struct fwu_mdata *mdata)
> > +{
> > +       int err;
> > +       bool parts_ok[2] = { false };
> > +       struct fwu_mdata s, *parts_mdata[2];
> > +
> > +       parts_mdata[0] = &g_mdata;
> > +       parts_mdata[1] = &s;
> > +
> > +       /* if mdata already read and ready */
> > +       err = mdata_crc_check(parts_mdata[0]);
> > +       if (!err)
> > +               goto ret_mdata;
> > +       /* else read, verify and, if needed, fix mdata */
> > +
> > +       for (int i = 0; i < 2; i++) {
>
> Define "int i;" at function block entry?
>
Hmm... I prefer this way - limiting scope of the scratch variables.

thanks.