On Mon, Feb 27, 2023 at 10:30 AM Etienne Carriere <etienne.carri...@linaro.org> wrote: > > Hello Jassi, > > On Sun, 5 Feb 2023 at 04:01, <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 | 135 ++++++++++++++++++++++++++- > > 3 files changed, 206 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c > > b/drivers/fwu-mdata/fwu-mdata-uclass.c > > index b477e9603f..e03773c584 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); > > + > > /** > > * 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..56299f1b2f 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} makes uninit crc32 always > > invalid */ > > +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,133 @@ 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)); > > + > > + err = fwu_write_mdata(g_dev, mdata, part & PRIMARY_PART ? true : > > false); > > There is an issue here as arg @part can have 3 values: PRIMARY_PART, > SECONDARY_PART or BOTH_PARTS. > The implementation here does not consider the later case BOTH_PATHS. > I think something like the below code snippet should do the work: > > switch (part) { > case PRIMARY_PART: > case SECONDARY_PART: > err = fwu_write_mdata(g_dev, mdata, part == PRIMARY_PART); > break; > default: /* assuming BOTH_PARTS, or maybe we need sanitization of > invalid part values? */ > err = fwu_write_mdata(g_dev, mdata, true); > if (!err) > err = fwu_write_mdata(g_dev, mdata, false); > break; > } > and adaptation for the error message below. > > Or maybe fwu_write_mdata() should not have its 3rd argument and > backend handler should directly handle mdata replication update (if > needed) on any mdata update requests. > Good catch. Thanks.
Even simpler fix is ... static int fwu_sync_mdata(struct fwu_mdata *mdata, int part) { if (part == BOTH_PARTS) { fwu_sync_mdata(mdata, PRIMARY_PART); part = SECONDARY_PART; } ..... } Thanks.