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