[v12,06/15] FWU: Add helper functions for accessing FWU metadata

Add weak functions for getting the update index value and dfu
alternate number needed for FWU Multi Bank update
functionality.

The current implementation for getting the update index value is for
platforms with 2 banks. If a platform supports more than 2 banks, it
can implement it's own function. The function to get the dfu alternate
number has been added for platforms with GPT partitioned storage
devices. Platforms with other storage partition scheme need to
implement their own function.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
Acked-by: Etienne Carriere <etienne.carriere@linaro.org>
---
Changes since V11:
* Add a function fwu_alt_num_for_dfu_dev() to get the alt number based
  on the DFU backend device as suggested by Ilias
* Change the logic of fwu_gpt_get_alt_num() as suggested by Ilias

 include/fwu.h             |  29 ++++++++++
 lib/fwu_updates/fwu.c     |  27 +++++++++
 lib/fwu_updates/fwu_gpt.c | 119 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 175 insertions(+)
 create mode 100644 lib/fwu_updates/fwu_gpt.c

On Mon, 3 Oct 2022 at 16:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> Add weak functions for getting the update index value and dfu
> alternate number needed for FWU Multi Bank update
> functionality.
>
> The current implementation for getting the update index value is for
> platforms with 2 banks. If a platform supports more than 2 banks, it
> can implement it's own function. The function to get the dfu alternate
> number has been added for platforms with GPT partitioned storage
> devices. Platforms with other storage partition scheme need to
> implement their own function.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Acked-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
> Changes since V11:
> * Add a function fwu_alt_num_for_dfu_dev() to get the alt number based
>   on the DFU backend device as suggested by Ilias
> * Change the logic of fwu_gpt_get_alt_num() as suggested by Ilias
>
>  include/fwu.h             |  29 ++++++++++
>  lib/fwu_updates/fwu.c     |  27 +++++++++
>  lib/fwu_updates/fwu_gpt.c | 119 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 175 insertions(+)
>  create mode 100644 lib/fwu_updates/fwu_gpt.c
>
> diff --git a/include/fwu.h b/include/fwu.h
> index 167f91388e..99f43f07b0 100644
> --- a/include/fwu.h
> +++ b/include/fwu.h
> @@ -212,4 +212,33 @@ int fwu_accept_image(efi_guid_t *img_type_id, u32 bank);
>   */
>  int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank);
>
> +/**
> + * fwu_plat_get_alt_num() - Get the DFU Alt Num for the image from the platform
> + * @dev: FWU device
> + * @image_guid: Image GUID for which DFU alt number needs to be retrieved
> + * @alt_num: Pointer to the alt_num
> + *
> + * Get the DFU alt number from the platform for the image specified by the
> + * image GUID.
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid,
> +                        u8 *alt_num);
> +
> +/**
> + * fwu_plat_get_update_index() - Get the value of the update bank
> + * @update_idx: Bank number to which images are to be updated
> + *
> + * Get the value of the bank(partition) to which the update needs to be
> + * made.
> + *
> + * Note: This is a weak function and platforms can override this with
> + * their own implementation for selection of the update bank.
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_plat_get_update_index(uint *update_idx);
>  #endif /* _FWU_H_ */
> diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
> index 439f66aee4..70482d14a0 100644
> --- a/lib/fwu_updates/fwu.c
> +++ b/lib/fwu_updates/fwu.c
> @@ -362,3 +362,30 @@ int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank)
>         return fwu_clrset_image_accept(img_type_id, bank,
>                                        IMAGE_ACCEPT_CLEAR);
>  }
> +
> +/**
> + * fwu_plat_get_update_index() - Get the value of the update bank
> + * @update_idx: Bank number to which images are to be updated
> + *
> + * Get the value of the bank(partition) to which the update needs to be
> + * made.
> + *
> + * Note: This is a weak function and platforms can override this with
> + * their own implementation for selection of the update bank.
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +__weak int fwu_plat_get_update_index(uint *update_idx)
> +{
> +       int ret;
> +       u32 active_idx;
> +
> +       ret = fwu_get_active_index(&active_idx);
> +       if (ret < 0)
> +               return -1;
> +
> +       *update_idx = (active_idx + 1) % CONFIG_FWU_NUM_BANKS;
> +
> +       return ret;
> +}
> diff --git a/lib/fwu_updates/fwu_gpt.c b/lib/fwu_updates/fwu_gpt.c
> new file mode 100644
> index 0000000000..28bb9756ad
> --- /dev/null
> +++ b/lib/fwu_updates/fwu_gpt.c
> @@ -0,0 +1,119 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022, Linaro Limited
> + */
> +
> +#include <blk.h>
> +#include <dfu.h>
> +#include <efi.h>
> +#include <efi_loader.h>
> +#include <fwu.h>
> +#include <log.h>
> +#include <part.h>
> +
> +#include <linux/errno.h>
> +
> +static int get_gpt_dfu_identifier(struct blk_desc *desc, efi_guid_t *image_guid)
> +{
> +       int i;
> +       struct disk_partition info;
> +       efi_guid_t unique_part_guid;
> +
> +       for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) {
> +               if (part_get_info(desc, i, &info))
> +                       continue;
> +               uuid_str_to_bin(info.uuid, unique_part_guid.b,
> +                               UUID_STR_FORMAT_GUID);
> +
> +               if (!guidcmp(&unique_part_guid, image_guid))
> +                       return i;
> +       }
> +
> +       log_err("No partition found with image_guid %pUs\n", image_guid);
> +       return -ENOENT;
> +}
> +
> +static int fwu_alt_num_for_dfu_dev(struct dfu_entity *dfu, int dev_num,
> +                                  int part, unsigned char dfu_dev,
> +                                  u8 *alt_num)
> +{
> +       int ret;
> +
> +       switch(dfu_dev) {
> +       case DFU_DEV_MMC:
> +               if (dfu->layout == DFU_RAW_ADDR &&
> +                   dfu->data.mmc.dev_num == dev_num &&
> +                   dfu->data.mmc.part == part) {
> +                       *alt_num = dfu->alt;
> +                       ret = 0;
> +               } else {
> +                       ret = -ENOENT;
> +               }
> +               break;
> +       default:
> +               ret = -ENOENT;
> +       }
> +
> +       return ret;
> +}
> +
> +static int fwu_gpt_get_alt_num(struct blk_desc *desc, efi_guid_t *image_guid,
> +                              u8 *alt_num, unsigned char dfu_dev)
> +{
> +       int ret = -1;
> +       int i, part, dev_num;
> +       struct dfu_entity *dfu;
> +
> +       dev_num = desc->devnum;
> +       part = get_gpt_dfu_identifier(desc, image_guid);
> +       if (part < 0)
> +               return -ENOENT;
> +
> +       dfu_init_env_entities(NULL, NULL);
> +
> +       while (true) {
> +               dfu = dfu_get_entity(i);

'i' initialization and incrementation are missing.

br,
etienne

> +               if (!dfu) {
> +                       ret = -ENOENT;
> +                       break;
> +               }
> +
> +               if (dfu->dev_type != dfu_dev)
> +                       continue;
> +
> +               ret = fwu_alt_num_for_dfu_dev(dfu, dev_num, part, dfu_dev,
> +                                             alt_num);
> +               if (!ret)
> +                       break;
> +       }
> +
> +       dfu_free_entities();
> +
> +       return ret;
> +}
> +
> +/**
> + * fwu_plat_get_alt_num() - Get the DFU alt number
> + * @dev: FWU metadata device
> + * @image_guid: GUID value of the image for which the alt num is to
> + *              be obtained
> + * @alt_num: The DFU alt number for the image that is to be updated
> + *
> + * Get the DFU alt number for the image that is to be updated. The
> + * image is identified with the image_guid parameter that is passed
> + * to the function.
> + *
> + * Note: This is a weak function and platforms can override this with
> + * their own implementation for obtaining the alt number value.
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +__weak int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid,
> +                               u8 *alt_num)
> +{
> +       struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> +
> +       return fwu_gpt_get_alt_num(dev_get_uclass_plat(priv->blk_dev),
> +                                  image_guid, alt_num, DFU_DEV_MMC);
> +}
> --
> 2.34.1
>

On Mon, 3 Oct 2022 at 21:20, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> On Mon, 3 Oct 2022 at 16:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > Add weak functions for getting the update index value and dfu
> > alternate number needed for FWU Multi Bank update
> > functionality.
> >
> > The current implementation for getting the update index value is for
> > platforms with 2 banks. If a platform supports more than 2 banks, it
> > can implement it's own function. The function to get the dfu alternate
> > number has been added for platforms with GPT partitioned storage
> > devices. Platforms with other storage partition scheme need to
> > implement their own function.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> > Acked-by: Etienne Carriere <etienne.carriere@linaro.org>
> > ---
> > Changes since V11:
> > * Add a function fwu_alt_num_for_dfu_dev() to get the alt number based
> >   on the DFU backend device as suggested by Ilias
> > * Change the logic of fwu_gpt_get_alt_num() as suggested by Ilias
> >
> >  include/fwu.h             |  29 ++++++++++
> >  lib/fwu_updates/fwu.c     |  27 +++++++++
> >  lib/fwu_updates/fwu_gpt.c | 119 ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 175 insertions(+)
> >  create mode 100644 lib/fwu_updates/fwu_gpt.c
> >
> > diff --git a/include/fwu.h b/include/fwu.h
> > index 167f91388e..99f43f07b0 100644
> > --- a/include/fwu.h
> > +++ b/include/fwu.h
> > @@ -212,4 +212,33 @@ int fwu_accept_image(efi_guid_t *img_type_id, u32 bank);
> >   */
> >  int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank);
> >
> > +/**
> > + * fwu_plat_get_alt_num() - Get the DFU Alt Num for the image from the platform
> > + * @dev: FWU device
> > + * @image_guid: Image GUID for which DFU alt number needs to be retrieved
> > + * @alt_num: Pointer to the alt_num
> > + *
> > + * Get the DFU alt number from the platform for the image specified by the
> > + * image GUID.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid,
> > +                        u8 *alt_num);
> > +
> > +/**
> > + * fwu_plat_get_update_index() - Get the value of the update bank
> > + * @update_idx: Bank number to which images are to be updated
> > + *
> > + * Get the value of the bank(partition) to which the update needs to be
> > + * made.
> > + *
> > + * Note: This is a weak function and platforms can override this with
> > + * their own implementation for selection of the update bank.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_plat_get_update_index(uint *update_idx);
> >  #endif /* _FWU_H_ */
> > diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
> > index 439f66aee4..70482d14a0 100644
> > --- a/lib/fwu_updates/fwu.c
> > +++ b/lib/fwu_updates/fwu.c
> > @@ -362,3 +362,30 @@ int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank)
> >         return fwu_clrset_image_accept(img_type_id, bank,
> >                                        IMAGE_ACCEPT_CLEAR);
> >  }
> > +
> > +/**
> > + * fwu_plat_get_update_index() - Get the value of the update bank
> > + * @update_idx: Bank number to which images are to be updated
> > + *
> > + * Get the value of the bank(partition) to which the update needs to be
> > + * made.
> > + *
> > + * Note: This is a weak function and platforms can override this with
> > + * their own implementation for selection of the update bank.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +__weak int fwu_plat_get_update_index(uint *update_idx)
> > +{
> > +       int ret;
> > +       u32 active_idx;
> > +
> > +       ret = fwu_get_active_index(&active_idx);
> > +       if (ret < 0)
> > +               return -1;
> > +
> > +       *update_idx = (active_idx + 1) % CONFIG_FWU_NUM_BANKS;
> > +
> > +       return ret;
> > +}
> > diff --git a/lib/fwu_updates/fwu_gpt.c b/lib/fwu_updates/fwu_gpt.c
> > new file mode 100644
> > index 0000000000..28bb9756ad
> > --- /dev/null
> > +++ b/lib/fwu_updates/fwu_gpt.c
> > @@ -0,0 +1,119 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2022, Linaro Limited
> > + */
> > +
> > +#include <blk.h>
> > +#include <dfu.h>
> > +#include <efi.h>
> > +#include <efi_loader.h>
> > +#include <fwu.h>
> > +#include <log.h>
> > +#include <part.h>
> > +
> > +#include <linux/errno.h>
> > +
> > +static int get_gpt_dfu_identifier(struct blk_desc *desc, efi_guid_t *image_guid)
> > +{
> > +       int i;
> > +       struct disk_partition info;
> > +       efi_guid_t unique_part_guid;
> > +
> > +       for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) {
> > +               if (part_get_info(desc, i, &info))
> > +                       continue;
> > +               uuid_str_to_bin(info.uuid, unique_part_guid.b,
> > +                               UUID_STR_FORMAT_GUID);
> > +
> > +               if (!guidcmp(&unique_part_guid, image_guid))
> > +                       return i;
> > +       }
> > +
> > +       log_err("No partition found with image_guid %pUs\n", image_guid);
> > +       return -ENOENT;
> > +}
> > +
> > +static int fwu_alt_num_for_dfu_dev(struct dfu_entity *dfu, int dev_num,
> > +                                  int part, unsigned char dfu_dev,
> > +                                  u8 *alt_num)
> > +{
> > +       int ret;
> > +
> > +       switch(dfu_dev) {
> > +       case DFU_DEV_MMC:
> > +               if (dfu->layout == DFU_RAW_ADDR &&
> > +                   dfu->data.mmc.dev_num == dev_num &&
> > +                   dfu->data.mmc.part == part) {
> > +                       *alt_num = dfu->alt;
> > +                       ret = 0;
> > +               } else {
> > +                       ret = -ENOENT;
> > +               }
> > +               break;
> > +       default:
> > +               ret = -ENOENT;
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> > +static int fwu_gpt_get_alt_num(struct blk_desc *desc, efi_guid_t *image_guid,
> > +                              u8 *alt_num, unsigned char dfu_dev)
> > +{
> > +       int ret = -1;
> > +       int i, part, dev_num;
> > +       struct dfu_entity *dfu;
> > +
> > +       dev_num = desc->devnum;
> > +       part = get_gpt_dfu_identifier(desc, image_guid);
> > +       if (part < 0)
> > +               return -ENOENT;
> > +
> > +       dfu_init_env_entities(NULL, NULL);
> > +
> > +       while (true) {
> > +               dfu = dfu_get_entity(i);
>
> 'i' initialization and incrementation are missing.

Oops. I had tested the functionality before sending out the patches,
and it had worked(unfortunately). I will fix this in the next version.
Thanks for catching this Etienne!

-sughosh

>
> br,
> etienne
>
> > +               if (!dfu) {
> > +                       ret = -ENOENT;
> > +                       break;
> > +               }
> > +
> > +               if (dfu->dev_type != dfu_dev)
> > +                       continue;
> > +
> > +               ret = fwu_alt_num_for_dfu_dev(dfu, dev_num, part, dfu_dev,
> > +                                             alt_num);
> > +               if (!ret)
> > +                       break;
> > +       }
> > +
> > +       dfu_free_entities();
> > +
> > +       return ret;
> > +}
> > +
> > +/**
> > + * fwu_plat_get_alt_num() - Get the DFU alt number
> > + * @dev: FWU metadata device
> > + * @image_guid: GUID value of the image for which the alt num is to
> > + *              be obtained
> > + * @alt_num: The DFU alt number for the image that is to be updated
> > + *
> > + * Get the DFU alt number for the image that is to be updated. The
> > + * image is identified with the image_guid parameter that is passed
> > + * to the function.
> > + *
> > + * Note: This is a weak function and platforms can override this with
> > + * their own implementation for obtaining the alt number value.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +__weak int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid,
> > +                               u8 *alt_num)
> > +{
> > +       struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> > +
> > +       return fwu_gpt_get_alt_num(dev_get_uclass_plat(priv->blk_dev),
> > +                                  image_guid, alt_num, DFU_DEV_MMC);
> > +}
> > --
> > 2.34.1
> >

Hi Sughosh

[...]

> > > +
> > > +static int fwu_alt_num_for_dfu_dev(struct dfu_entity *dfu, int dev_num,
> > > +                                  int part, unsigned char dfu_dev,
> > > +                                  u8 *alt_num)
> > > +{
> > > +       int ret;
> > > +
> > > +       switch(dfu_dev) {
> > > +       case DFU_DEV_MMC:
> > > +               if (dfu->layout == DFU_RAW_ADDR &&
> > > +                   dfu->data.mmc.dev_num == dev_num &&
> > > +                   dfu->data.mmc.part == part) {
> > > +                       *alt_num = dfu->alt;
> > > +                       ret = 0;
> > > +               } else {
> > > +                       ret = -ENOENT;
> > > +               }
> > > +               break;
> > > +       default:
> > > +               ret = -ENOENT;
> > > +       }

I think that looks better than the previous version and it's also
easier to add new DFU cases. Thanks for fixing this

> > > +
> > > +       return ret;
> > > +}
> > > +
> > > +static int fwu_gpt_get_alt_num(struct blk_desc *desc, efi_guid_t *image_guid,
> > > +                              u8 *alt_num, unsigned char dfu_dev)
> > > +{
> > > +       int ret = -1;
> > > +       int i, part, dev_num;
> > > +       struct dfu_entity *dfu;
> > > +
> > > +       dev_num = desc->devnum;
> > > +       part = get_gpt_dfu_identifier(desc, image_guid);
> > > +       if (part < 0)
> > > +               return -ENOENT;
> > > +
> > > +       dfu_init_env_entities(NULL, NULL);
> > > +
> > > +       while (true) {
> > > +               dfu = dfu_get_entity(i);
> >
> > 'i' initialization and incrementation are missing.

Yea that's due to the change I requested on the previous version. But
that's fine, I still think checking against NULL is better approach
here.  When you send the next version please add a check for
dfu_init_env_entities() as well since it returns an int.

>
> Oops. I had tested the functionality before sending out the patches,
> and it had worked(unfortunately). I will fix this in the next version.
> Thanks for catching this Etienne!
>
> -sughosh
>
> >
> > br,
> > etienne
> >
> > > +               if (!dfu) {
> > > +                       ret = -ENOENT;
> > > +                       break;
> > > +               }
> > > +
> > > +               if (dfu->dev_type != dfu_dev)
> > > +                       continue;
> > > +
> > > +               ret = fwu_alt_num_for_dfu_dev(dfu, dev_num, part, dfu_dev,
> > > +                                             alt_num);
> > > +               if (!ret)
> > > +                       break;
> > > +       }
> > > +
> > > +       dfu_free_entities();
> > > +
> > > +       return ret;
> > > +}
> > > +
> > > +/**
> > > + * fwu_plat_get_alt_num() - Get the DFU alt number
> > > + * @dev: FWU metadata device
> > > + * @image_guid: GUID value of the image for which the alt num is to
> > > + *              be obtained
> > > + * @alt_num: The DFU alt number for the image that is to be updated
> > > + *
> > > + * Get the DFU alt number for the image that is to be updated. The
> > > + * image is identified with the image_guid parameter that is passed
> > > + * to the function.
> > > + *
> > > + * Note: This is a weak function and platforms can override this with
> > > + * their own implementation for obtaining the alt number value.
> > > + *
> > > + * Return: 0 if OK, -ve on error
> > > + *
> > > + */
> > > +__weak int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid,
> > > +                               u8 *alt_num)
> > > +{
> > > +       struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> > > +
> > > +       return fwu_gpt_get_alt_num(dev_get_uclass_plat(priv->blk_dev),
> > > +                                  image_guid, alt_num, DFU_DEV_MMC);
> > > +}
> > > --
> > > 2.34.1
> > >

Regards
/Ilias