| Message ID | 20230205030146.61432-1-jassisinghbrar@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | FWU: Handle meta-data in common code | expand |
Hi Jassi, Apologies for the delay On Sat, Feb 04, 2023 at 09:01:46PM -0600, jassisinghbrar@gmail.com wrote: > From: Jassi Brar <jaswinder.singh@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.singh@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); > + if (err) { > + log_err("Unable to write %s mdata\n", > + part & PRIMARY_PART ? "primary": "secondary"); > + 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) > +{ > + void *buf = &mdata->version; > + u32 calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32)); > + > + 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); > + if (!err) > + goto ret_mdata; > + /* 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); > + 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); > + if (!err) > + sec_ok = true; > + else > + log_debug("secondary mdata: crc32 failed\n"); > + } Isn't it better to define pri_ok, sec_ok and their equivalent mdata as arrays ? IOW something along the lines of bool parts_ok[2] = { false }; struct fwu_mdata parts_mdata[2]; parts_mdata[0] = &g_mdata; parts_mdata[1] = ..... for (i = 0; i < 2; i++) { err = fwu_read_mdata(g_dev, parts_mdata[i], !(i % 2) ? true : false); if (!err) err = mdata_crc_check(parts_mdata[i]); etc.... } > + > + if (pri_ok && sec_ok) { And then also adjust this part? > + /* > + * Before returning, check that both the > + * FWU metadata copies are the same. > + */ > + err = memcmp(p_mdata, s_mdata, sizeof(struct fwu_mdata)); > + if (!err) > + goto ret_mdata; > + > + /* > + * If not, populate the secondary partition from the > + * primary partition copy. > + */ > + 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); > + if (err) { > + log_debug("mdata : primary write failed\n"); > + return err; > + } > + } > + > + if (!sec_ok) { > + memcpy(s_mdata, p_mdata, sizeof(struct fwu_mdata)); > + err = fwu_sync_mdata(s_mdata, SECONDARY_PART); > + if (err) { > + log_debug("mdata : secondary write failed\n"); > + return err; > + } > + } And this could also be folded into a for loop > + > +ret_mdata: > + if (!err && mdata) > + memcpy(mdata, p_mdata, sizeof(struct fwu_mdata)); > + > + return err; > +} > + > /** > * fwu_verify_mdata() - Verify the FWU metadata > * @mdata: FWU metadata structure > -- > 2.34.1 > Thanks /Ilias
Hello Jassi, On Sun, 5 Feb 2023 at 04:01, <jassisinghbrar@gmail.com> wrote: > > From: Jassi Brar <jaswinder.singh@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.singh@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. Br, etienne > + if (err) { > + log_err("Unable to write %s mdata\n", > + part & PRIMARY_PART ? "primary": "secondary"); > + 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) > +{ > + void *buf = &mdata->version; > + u32 calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32)); > + > + 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); > + if (!err) > + goto ret_mdata; > + /* 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); > + 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); > + 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. > + */ > + err = memcmp(p_mdata, s_mdata, sizeof(struct fwu_mdata)); > + if (!err) > + goto ret_mdata; > + > + /* > + * If not, populate the secondary partition from the > + * primary partition copy. > + */ > + 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); > + if (err) { > + log_debug("mdata : primary write failed\n"); > + return err; > + } > + } > + > + if (!sec_ok) { > + memcpy(s_mdata, p_mdata, sizeof(struct fwu_mdata)); > + err = fwu_sync_mdata(s_mdata, SECONDARY_PART); > + if (err) { > + log_debug("mdata : secondary write failed\n"); > + return err; > + } > + } > + > +ret_mdata: > + if (!err && mdata) > + memcpy(mdata, p_mdata, sizeof(struct fwu_mdata)); > + > + return err; > +} > + > /** > * fwu_verify_mdata() - Verify the FWU metadata > * @mdata: FWU metadata structure > -- > 2.34.1 >
On Mon, Feb 27, 2023 at 10:30 AM Etienne Carriere <etienne.carriere@linaro.org> wrote: > > Hello Jassi, > > On Sun, 5 Feb 2023 at 04:01, <jassisinghbrar@gmail.com> wrote: > > > > From: Jassi Brar <jaswinder.singh@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.singh@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.
- err = fwu_write_mdata(g_dev, mdata, part & PRIMARY_PART ? true : false); On Mon, 27 Feb 2023 at 17:46, Jassi Brar <jassisinghbrar@gmail.com> wrote: > > On Mon, Feb 27, 2023 at 10:30 AM Etienne Carriere > <etienne.carriere@linaro.org> wrote: > > > > Hello Jassi, > > > > On Sun, 5 Feb 2023 at 04:01, <jassisinghbrar@gmail.com> wrote: > > > > > > From: Jassi Brar <jaswinder.singh@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.singh@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. True :) , (with error code management) even if i think it makes the code less straightforward to read out. maybe also, for simplicity, - err = fwu_write_mdata(g_dev, mdata, part & PRIMARY_PART ? true : false); + err = fwu_write_mdata(g_dev, mdata, part == PRIMARY_PART); br, etienne
Hi Ilias, On Thu, Feb 23, 2023 at 2:36 AM Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > +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); > > + if (!err) > > + goto ret_mdata; > > + /* 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); > > + 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); > > + if (!err) > > + sec_ok = true; > > + else > > + log_debug("secondary mdata: crc32 failed\n"); > > + } > > Isn't it better to define pri_ok, sec_ok and their equivalent mdata as > arrays ? IOW something along the lines of > > bool parts_ok[2] = { false }; > struct fwu_mdata parts_mdata[2]; > > parts_mdata[0] = &g_mdata; > parts_mdata[1] = ..... > for (i = 0; i < 2; i++) { > err = fwu_read_mdata(g_dev, parts_mdata[i], !(i % 2) ? true : false); > if (!err) > err = mdata_crc_check(parts_mdata[i]); > etc.... > } > > > + > > + if (pri_ok && sec_ok) { > > And then also adjust this part? > > > + /* > > + * Before returning, check that both the > > + * FWU metadata copies are the same. > > + */ > > + err = memcmp(p_mdata, s_mdata, sizeof(struct fwu_mdata)); > > + if (!err) > > + goto ret_mdata; > > + > > + /* > > + * If not, populate the secondary partition from the > > + * primary partition copy. > > + */ > > + 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); > > + if (err) { > > + log_debug("mdata : primary write failed\n"); > > + return err; > > + } > > + } > > + > > + if (!sec_ok) { > > + memcpy(s_mdata, p_mdata, sizeof(struct fwu_mdata)); > > + err = fwu_sync_mdata(s_mdata, SECONDARY_PART); > > + if (err) { > > + log_debug("mdata : secondary write failed\n"); > > + return err; > > + } > > + } > > And this could also be folded into a for loop > I have done these modifications and submitted v5. Thanks.
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); + if (err) { + log_err("Unable to write %s mdata\n", + part & PRIMARY_PART ? "primary": "secondary"); + 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) +{ + void *buf = &mdata->version; + u32 calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32)); + + 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); + if (!err) + goto ret_mdata; + /* 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); + 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); + 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. + */ + err = memcmp(p_mdata, s_mdata, sizeof(struct fwu_mdata)); + if (!err) + goto ret_mdata; + + /* + * If not, populate the secondary partition from the + * primary partition copy. + */ + 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); + if (err) { + log_debug("mdata : primary write failed\n"); + return err; + } + } + + if (!sec_ok) { + memcpy(s_mdata, p_mdata, sizeof(struct fwu_mdata)); + err = fwu_sync_mdata(s_mdata, SECONDARY_PART); + if (err) { + log_debug("mdata : secondary write failed\n"); + return err; + } + } + +ret_mdata: + if (!err && mdata) + memcpy(mdata, p_mdata, sizeof(struct fwu_mdata)); + + return err; +} + /** * fwu_verify_mdata() - Verify the FWU metadata * @mdata: FWU metadata structure