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

[Jassi Brar <jaswinder.singh@linaro.org>]

On Mon, 9 Jan 2023 at 06:54, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Jassi,
>
> On Mon, Jan 02, 2023 at 12:26:40PM -0600, Jassi Brar wrote:
> > 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@linaro.org>
> > ---
> >  drivers/fwu-mdata/fwu-mdata-uclass.c |  34 +++++++
> >  include/fwu.h                        |  41 ++++++++
> >  lib/fwu_updates/fwu.c                | 142 ++++++++++++++++++++++++++-
> >  3 files changed, 213 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
.....
> > + */
> > +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));
> > +
> > +     if (part & PRIMARY_PART)
> > +             err = fwu_write_mdata(g_dev, mdata, true);
> > +
> > +     if (err) {
> > +             log_err("Unable to write primary mdata\n");
> > +             return err;
> > +     }
> > +
> > +     if (part & SECONDARY_PART)
> > +             err = fwu_write_mdata(g_dev, mdata, false);
> > +
> > +     if (err) {
> > +             log_err("Unable to write secondary mdata\n");
> > +             return err;
> > +     }
>
> Can we write this
> err = fwu_write_mdata(g_dev, mdata, part & PRIMARY_PART ? true: false);
> if (err)
>     log_err("Unable to write %s partition\n", part & PRIMARY_PART ?  "primary": "secondary" );
>     ....
>
of course :)


> > +int fwu_get_verified_mdata(struct fwu_mdata *mdata)
> > +{
> > +     int err;
> > +     bool pri_ok, sec_ok;
> > +     struct fwu_mdata s, *p_mdata, *s_mdata;
> > +
> > +     p_mdata = &g_mdata;
> > +     s_mdata = &s;
>
> Why are we defining it like this? Readability to have pointers for primary
> and secondary metadata?
>
that's the idea.


> > +
> > +     /* if mdata already read and ready */
> > +     err = mdata_crc_check(p_mdata);
> > +     if (!err)
> > +             goto ret_mdata;
>
> Shouldn't we check the secondary metadata ? At least that's what the old
> fwu_check_mdata_validity() was doing.
>
During the first run after boot, both copies are checked. Also when we
update the mdata.
Othwise we have a good primary copy, even if the secondary is
corrupted for some mysterious (corrupted in readonly mode) reason
maybe we should let that be fixed after reboot and not add crc
checking cost to every call?


> > +     /* else read, verify and, if needed, fix mdata */
> > +
> > +     pri_ok = false;
> > +     err = fwu_read_mdata(g_dev, p_mdata, true);
> > +     if (!err) {
> > +             err = mdata_crc_check(p_mdata);
> > +             if (!err)
> > +                     pri_ok = true;
> > +             else
> > +                     log_debug("primary mdata: crc32 failed\n");
> > +     }
> > +
> > +     sec_ok = false;
> > +     err = fwu_read_mdata(g_dev, s_mdata, false);
> > +     if (!err) {
> > +             err = mdata_crc_check(s_mdata);
> > +             if (!err)
> > +                     sec_ok = true;
> > +             else
> > +                     log_debug("secondary mdata: crc32 failed\n");
> > +     }
> > +
> > +     if (pri_ok && sec_ok) {
> > +             /*
> > +              * Before returning, check that both the
> > +              * FWU metadata copies are the same.
> > +              */
> > +             err = memcmp(p_mdata, s_mdata, sizeof(struct fwu_mdata));
> > +             if (!err)
> > +                     goto ret_mdata;
> > +
> > +             /*
> > +              * If not, populate the secondary partition from the
> > +              * primary partition copy.
> > +              */
> > +             log_info("Both FWU metadata copies are valid but do not match.");
> > +             log_info(" Restoring the secondary partition from the primary\n");
> > +             sec_ok = false;
> > +     }
> > +
> > +     if (!pri_ok) {
> > +             memcpy(p_mdata, s_mdata, sizeof(struct fwu_mdata));
> > +             err = fwu_sync_mdata(p_mdata, PRIMARY_PART);
> > +             if (err)
> > +                     goto ret_mdata;
>
> The error print here is a bit misleading.  It's a failed write, not a crc32
> mismatch
>
Fixed.

Thanks.