On Mon, Nov 7, 2022 at 11:24 AM Etienne Carriere
<etienne.carri...@linaro.org> wrote:
> On Fri, 4 Nov 2022 at 03:43, <jassisinghb...@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.
....
> > +
> > +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).
.....
> > +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.
....
> > +
> > + 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.
