Hello Jassi, On Mon, 7 Nov 2022 at 19:29, Jassi Brar <jassisinghb...@gmail.com> wrote: > > 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.
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.