[PATCH 2/4] fwu: move meta-data management in core

[Etienne Carriere]

Hello Jassi,

On Mon, 7 Nov 2022 at 19:29, Jassi Brar <jassisinghbrar at gmail.com> wrote:
>
> On Mon, Nov 7, 2022 at 11:24 AM Etienne Carriere
> <etienne.carriere at linaro.org> wrote:
> > On Fri, 4 Nov 2022 at 03:43, <jassisinghbrar at gmail.com> wrote:
> > >
> > > +/**
> > > + * 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);
> >
> > Nitpicking: would you be ok to rename this function to fwu_get_mdata().
> > When getting fwu mdata, we obviously expect to get reliable data.
> >
> I originally called it fwu_get_mdata() in my local 1-patch change :)
> But there already is one such function and I had to rename, only to delete that.

Ok, maybe a later commit to rename the function once the old one has
been removed in patch 4/4.

>
> ....
> > > +
> > > +static struct fwu_mdata g_mdata = { 0 };
> >
> > Can remove "= { 0 };"
> >
> I knew, but it was supposed to imply that we want the first crc check
> to fail (because we set that to 0 here).

As zero init is implicit, I think maintainers will ask to remove it here.

Maybe add an inline comment above the global variable definition.
/* Crc32 of a zeroes produces a non 0 value */

>
> .....
> > > +static inline int mdata_crc_check(struct fwu_mdata *mdata)
> > > +{
> > > +       u32 calc_crc32 = crc32(0, &mdata->version, sizeof(*mdata) - sizeof(u32));
> >
> > Add an empty line below the above definition.
> >
> OK.
>
> ....
> > > +       /* if mdata already read and ready */
> > > +       err = mdata_crc_check(p_mdata, true);
> >
> > 2nd argument to be removed. Ditto for the other occurrences of
> > mdata_crc_check() calls.
> >
> Ouch, I 'cleaned' the patch before submission. Will fix that.
>
> > Note here I would pass straight &g_mdata as argument rather than
> > p_mdata indirection, for clarity.
> >
> Hmm... I wanted to keep the g_mdata usage clear and minimal.

It looks strange we need to check p_mdata init value to understand
we're accessing the cached mdata.
p_mdata is expected to be the primary part. Perfer keep it for this usage.

br,
etienne

>
> ....
> > > +
> > > +       if (!pri_ok) {
> > > +               memcpy(p_mdata, s_mdata, sizeof(struct fwu_mdata));
> > > +               err = fwu_sync_mdata(p_mdata, PRIMARY_PART);
> >
> > Should test return value.
> >
> > > +       }
> > > +
> > > +       if (!sec_ok) {
> > > +               memcpy(s_mdata, p_mdata, sizeof(struct fwu_mdata));
> > > +               err = fwu_sync_mdata(s_mdata, SECONDARY_PART);
> >
> > Ditto
> >
> > > +       }
> > > +
> > > +       /* make sure atleast one copy is good */
> >
> > s/atleast/at least/
> >
> ok
>
> > > +       err = mdata_crc_check(p_mdata, true);
> >
> > This is not needed, it's been already verified unless we want to catch
> > the case !pri_ok && !sec_ok. I think this case should be explicitly
> > handled above with a nice console trace message/
> >
> ok
>
> Thanks.