On Wed, Mar 1, 2023 at 5:16 AM Etienne Carriere <etienne.carri...@linaro.org> wrote: > On Tue, 28 Feb 2023 at 01:52, <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. > > ...
> > 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 > > + */ > > Inline description only in header file, or duplicated in source and > header files? > This is the original practice in FWU stack - to have the description in header as well as source code. I just didn't want to stick out. > > 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 > > @mdata: Output FWU mdata read > > > + * @primary: If primary or secondary copy of meta-data is to be read > > s/meta-data/FWU metadata/ > Ditto in .write_mdata description > ok > > +/** > > + * 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. > > @mdata: Output FWU metadata read or NULL > ok > > +static int fwu_sync_mdata(struct fwu_mdata *mdata, int part) > > +{ > > + void *buf = &mdata->version; > > + int err; > > + > > + if (part == BOTH_PARTS) { > > + err = fwu_sync_mdata(mdata, SECONDARY_PART); > > + if (err) > > + return err; > > + part = PRIMARY_PART; > > + } > > + > > + /* > > + * 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); > > Since this expects part is either PRIMARY_PART or SECONDARY_PART, prefer: > err = fwu_write_mdata(g_dev, mdata, part == PRIMARY_PART); > > And ditto below: > part == PRIMARY_PART ? "primary": "secondary"); > yes, now that we handle out the BOTH_PARTS case already. > > +int fwu_get_verified_mdata(struct fwu_mdata *mdata) > > +{ > > + int err; > > + bool parts_ok[2] = { false }; > > + struct fwu_mdata s, *parts_mdata[2]; > > + > > + parts_mdata[0] = &g_mdata; > > + parts_mdata[1] = &s; > > + > > + /* if mdata already read and ready */ > > + err = mdata_crc_check(parts_mdata[0]); > > + if (!err) > > + goto ret_mdata; > > + /* else read, verify and, if needed, fix mdata */ > > + > > + for (int i = 0; i < 2; i++) { > > Define "int i;" at function block entry? > Hmm... I prefer this way - limiting scope of the scratch variables. thanks.