Hello Jassi, On Fri, 4 Nov 2022 at 03:43, <jassisinghb...@gmail.com> wrote: > > From: Jassi Brar <jaswinder.si...@linaro.org> > > 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.si...@linaro.org> > --- > drivers/fwu-mdata/fwu-mdata-uclass.c | 34 +++++++ > include/fwu.h | 41 ++++++++ > lib/fwu_updates/fwu.c | 136 ++++++++++++++++++++++++++- > 3 files changed, 207 insertions(+), 4 deletions(-) > > diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c > b/drivers/fwu-mdata/fwu-mdata-uclass.c > index b477e9603f..ded9902971 100644 > --- a/drivers/fwu-mdata/fwu-mdata-uclass.c > +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c > @@ -16,6 +16,40 @@ > #include <linux/types.h> > #include <u-boot/crc.h> > > +/** > + * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata() > + * > + * Return: 0 if OK, -ve on error > + */ > +int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool > primary); > +{ > + const struct fwu_mdata_ops *ops = device_get_ops(dev); > + > + if (!ops->read_mdata) { > + log_debug("read_mdata() method not defined\n"); > + return -ENOSYS; > + } > + > + return ops->read_mdata(dev, mdata, primary); > +} > + > +/** > + * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata() > + * > + * Return: 0 if OK, -ve on error > + */ > +int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool > primary); > +{ > + const struct fwu_mdata_ops *ops = device_get_ops(dev); > + > + if (!ops->write_mdata) { > + log_debug("write_mdata() method not defined\n"); > + return -ENOSYS; > + } > + > + return ops->write_mdata(dev, mdata, primary); > +} > + > /** > * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers > * @dev: FWU metadata device > diff --git a/include/fwu.h b/include/fwu.h > index 0919ced812..1a700c9e6a 100644 > --- a/include/fwu.h > +++ b/include/fwu.h > @@ -24,6 +24,26 @@ struct fwu_mdata_gpt_blk_priv { > * @update_mdata() - Update the FWU metadata copy > */ > struct fwu_mdata_ops { > + /** > + * read_mdata() - Populate the asked FWU metadata copy > + * @dev: FWU metadata device > + * @mdata: Copy of the FWU metadata > + * @primary: If primary or secondary copy of meta-data is to be read > + * > + * Return: 0 if OK, -ve on error > + */ > + int (*read_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool > primary); > + > + /** > + * write_mdata() - Write the given FWU metadata copy > + * @dev: FWU metadata device > + * @mdata: Copy of the FWU metadata > + * @primary: If primary or secondary copy of meta-data is to be > written > + * > + * Return: 0 if OK, -ve on error > + */ > + int (*write_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool > primary); > + > /** > * check_mdata() - Check if the FWU metadata is valid > * @dev: FWU device > @@ -126,6 +146,27 @@ struct fwu_mdata_ops { > EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \ > 0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8) > > +/** > + * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata() > + */ > +int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool > primary); > + > +/** > + * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata() > + */ > +int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool > primary); > + > +/** > + * 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. > + > /** > * fwu_check_mdata_validity() - Check for validity of the FWU metadata copies > * > diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c > index 5313d07302..3d51b91ed5 100644 > --- a/lib/fwu_updates/fwu.c > +++ b/lib/fwu_updates/fwu.c > @@ -15,13 +15,13 @@ > #include <linux/errno.h> > #include <linux/types.h> > > +#include <u-boot/crc.h> > + > +static struct fwu_mdata g_mdata = { 0 }; Can remove "= { 0 };" > +static struct udevice *g_dev; > static u8 in_trial; > static u8 boottime_check; > > -#include <linux/errno.h> > -#include <linux/types.h> > -#include <u-boot/crc.h> > - > enum { > IMAGE_ACCEPT_SET = 1, > IMAGE_ACCEPT_CLEAR, > @@ -161,6 +161,134 @@ static int fwu_get_image_type_id(u8 *image_index, > efi_guid_t *image_type_id) > return -ENOENT; > } > > +/** > + * fwu_sync_mdata() - Update given meta-data partition(s) with the copy > provided > + * @mdata: FWU metadata structure > + * @part: Bitmask of FWU metadata partitions to be written to > + * > + * Return: 0 if OK, -ve on error > + */ > +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; > + } > + > + /* update the cached copy of meta-data */ > + memcpy(&g_mdata, mdata, sizeof(struct fwu_mdata)); > + > + return 0; > +} > + > +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. > + return calc_crc32 == mdata->crc32 ? 0 : -EINVAL; > +} > + > +/** > + * 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) > +{ > + int err; > + bool pri_ok, sec_ok; > + struct fwu_mdata s, *p_mdata, *s_mdata; > + > + p_mdata = &g_mdata; > + s_mdata = &s; > + > + /* 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. Note here I would pass straight &g_mdata as argument rather than p_mdata indirection, for clarity. > + if (!err) { > + if (mdata) > + memcpy(mdata, p_mdata, sizeof(struct fwu_mdata)); > + return 0; > + } > + /* 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, true); > + 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, false); > + 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. If not, > + * populate the secondary partition from the > + * primary partition copy. > + */ > + if (memcmp(p_mdata, s_mdata, sizeof(struct fwu_mdata))) { > + 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); 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/ > + 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/ if (!pri_ok && !sec_ok) { log_err("No reliable fwu mdata found\n"); return -EINVAL; } > + if (err) > + log_debug("mdata saving... crc32 failed\n"); > + else if (mdata) > + memcpy(mdata, p_mdata, sizeof(struct fwu_mdata)); > + > + return err; > +} > + > /** > * fwu_verify_mdata() - Verify the FWU metadata > * @mdata: FWU metadata structure > -- > 2.25.1 >