Hi Sughosh, On Thu, 23 Jun 2022 at 08:24, Sughosh Ganu <sughosh.g...@linaro.org> wrote: > > hi Etienne, > > On Tue, 21 Jun 2022 at 16:24, Etienne Carriere > <etienne.carri...@linaro.org> wrote: > > > > Hello Sughosh, > > > > > > > > On Thu, 9 Jun 2022 at 14:30, Sughosh Ganu <sughosh.g...@linaro.org> wrote: > > > > > > In the FWU Multi Bank Update feature, the information about the > > > updatable images is stored as part of the metadata, which is stored on > > > a dedicated partition. Add the metadata structure, and a driver model > > > uclass which provides functions to access the metadata. These are > > > generic API's, and implementations can be added based on parameters > > > like how the metadata partition is accessed and what type of storage > > > device houses the metadata. > > > > > > Signed-off-by: Sughosh Ganu <sughosh.g...@linaro.org> > > > --- > > > drivers/Kconfig | 2 + > > > drivers/Makefile | 1 + > > > drivers/fwu-mdata/Kconfig | 7 + > > > drivers/fwu-mdata/Makefile | 6 + > > > drivers/fwu-mdata/fwu-mdata-uclass.c | 459 +++++++++++++++++++++++++++ > > > include/dm/uclass-id.h | 1 + > > > include/fwu.h | 49 +++ > > > include/fwu_mdata.h | 67 ++++ > > > 8 files changed, 592 insertions(+) > > > create mode 100644 drivers/fwu-mdata/Kconfig > > > create mode 100644 drivers/fwu-mdata/Makefile > > > create mode 100644 drivers/fwu-mdata/fwu-mdata-uclass.c > > > create mode 100644 include/fwu.h > > > create mode 100644 include/fwu_mdata.h > > > > > <snip> > > > > diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c > > > b/drivers/fwu-mdata/fwu-mdata-uclass.c > > > new file mode 100644 > > > index 0000000000..1530ceb01d > > > --- /dev/null > > > +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
<snip> > > > +/** > > > + * fwu_get_mdata() - Get a FWU metadata copy > > > + * @mdata: Copy of the FWU metadata > > > + * > > > + * Get a valid copy of the FWU metadata. > > > + * > > > + * Return: 0 if OK, -ve on error > > > + * > > > + */ > > > +int fwu_get_mdata(struct fwu_mdata **mdata) > > > > Is there a real need for this function to allocate an instance of struct > > mdata. > > I think it would be clearer if it was the caller's responsibility to > > allocate/free the structure. > > > > Or maybe rename this function fwu_alloc_and_copy_mdata() to highlight > > that the function gives an allocated copy of the data. > > I guess I can put a comment in the function description saying that > the function is responsible for the allocation of the metadata > structure. I think it would be better. > > > One should be careful when calling these API functions as some act on > > a local copy (retrieved from fw_get_mdata()) while other functions > > modify straight fwu-mdata in the storage media. > > Did you find any function which is modifying the metadata on the > storage device directly. The API fwu_update_mdata() is supposed to be > doing that. If you have come across any function which is directly > modifying the metadata on the storage media, please let me know and I > will fix it. Many functions do so: fwu_clear_accept_image(), fwu_clear_accept_image(), fwu_resert_boot_index(), etc... Actually all generic functions do so while only fwu_get_mdata() and fwu_update_mdata() act on a RAM copy. Maybe fwu-mdata ops should have a status field for when a RAM copy was exported and used to prevent direct updates to mdata in storage until caller releases (fw_put_mdata()?) the exposed copy. Would this scheme be overkilling... Or maybe fwu_clear_accept_image() and other helper functions could also require a mdata RAM reference to act on, letting the caller also go through fwu_get_mdata()/fwu_update_mdata(). etienne <snip>