diff mbox series

[v8,06/13] FWU: Add helper functions for accessing FWU metadata

Message ID 20220817124323.375968-7-sughosh.ganu@linaro.org
State New
Headers show
Series FWU: Add FWU Multi Bank Update feature support | expand

Commit Message

Sughosh Ganu Aug. 17, 2022, 12:43 p.m. UTC 
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>
---
Changes since V7:
* Moved the API's fwu_plat_get_update_index() and
  fwu_plat_get_alt_num() as weak functions in common code as suggested
  by Ilias.

 include/fwu.h             |   1 +
 lib/fwu_updates/Makefile  |   7 +++
 lib/fwu_updates/fwu.c     |  22 ++++++++
 lib/fwu_updates/fwu_gpt.c | 104 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 134 insertions(+)
 create mode 100644 lib/fwu_updates/Makefile
 create mode 100644 lib/fwu_updates/fwu.c
 create mode 100644 lib/fwu_updates/fwu_gpt.c

Comments

Jassi Brar Aug. 17, 2022, 5 p.m. UTC | #1 
On Wed, 17 Aug 2022 at 07:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
.....
> diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
> new file mode 100644
> index 0000000000..9808036eec
> --- /dev/null
> +++ b/lib/fwu_updates/fwu.c
> @@ -0,0 +1,22 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022, Linaro Limited
> + */
> +
> +#include <fwu.h>
> +#include <fwu_mdata.h>
> +
> +__weak int fwu_plat_get_update_index(u32 *update_idx)
> +{
> +       int ret;
> +       u32 active_idx;
> +
> +       ret = fwu_get_active_index(&active_idx);
> +
> +       if (ret < 0)
> +               return -1;
> +
> +       *update_idx = active_idx ^= 0x1;
> +
It shoud be
        *update_idx = (active_idx + 1) % CONFIG_FWU_NUM_BANKS;

cheers.
Sughosh Ganu Aug. 17, 2022, 5:55 p.m. UTC | #2 
On Wed, 17 Aug 2022 at 22:30, Jassi Brar <jaswinder.singh@linaro.org> wrote:
>
> On Wed, 17 Aug 2022 at 07:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> .....
> > diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
> > new file mode 100644
> > index 0000000000..9808036eec
> > --- /dev/null
> > +++ b/lib/fwu_updates/fwu.c
> > @@ -0,0 +1,22 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2022, Linaro Limited
> > + */
> > +
> > +#include <fwu.h>
> > +#include <fwu_mdata.h>
> > +
> > +__weak int fwu_plat_get_update_index(u32 *update_idx)
> > +{
> > +       int ret;
> > +       u32 active_idx;
> > +
> > +       ret = fwu_get_active_index(&active_idx);
> > +
> > +       if (ret < 0)
> > +               return -1;
> > +
> > +       *update_idx = active_idx ^= 0x1;
> > +
> It shoud be
>         *update_idx = (active_idx + 1) % CONFIG_FWU_NUM_BANKS;

As mentioned in the commit message, this is a weak function for the
case where CONFIG_FWU_NUM_BANKS = 2, where the above logic works since
the fwu_get_active_index() function checks that a sane value is being
returned for the active_index. However, with the logic that you
suggest, this function can be extended for platforms with the number
of banks more than 2. I will incorporate this in the next version.
Thanks.

-sughosh


>
> cheers.
Simon Glass Aug. 18, 2022, 5:50 p.m. UTC | #3 
Hi Sughosh,

On Wed, 17 Aug 2022 at 06:44, 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>
> ---
> Changes since V7:
> * Moved the API's fwu_plat_get_update_index() and
>   fwu_plat_get_alt_num() as weak functions in common code as suggested
>   by Ilias.
>
>  include/fwu.h             |   1 +
>  lib/fwu_updates/Makefile  |   7 +++
>  lib/fwu_updates/fwu.c     |  22 ++++++++
>  lib/fwu_updates/fwu_gpt.c | 104 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 134 insertions(+)
>  create mode 100644 lib/fwu_updates/Makefile
>  create mode 100644 lib/fwu_updates/fwu.c
>  create mode 100644 lib/fwu_updates/fwu_gpt.c
>
> diff --git a/include/fwu.h b/include/fwu.h
> index 8259c75d12..f14175cc9a 100644
> --- a/include/fwu.h
> +++ b/include/fwu.h
> @@ -51,4 +51,5 @@ int fwu_clear_accept_image(efi_guid_t *img_type_id, u32
bank);
>
>  int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid,
>                          int *alt_num);
> +int fwu_plat_get_update_index(u32 *update_idx);

Don't forget to add a proper comment, also this should be uint, not u32

>  #endif /* _FWU_H_ */
> diff --git a/lib/fwu_updates/Makefile b/lib/fwu_updates/Makefile
> new file mode 100644
> index 0000000000..1993088e5b
> --- /dev/null
> +++ b/lib/fwu_updates/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# Copyright (c) 2022, Linaro Limited
> +#
> +
> +obj-$(CONFIG_FWU_MULTI_BANK_UPDATE) += fwu.o
> +obj-$(CONFIG_FWU_MDATA_GPT_BLK) += fwu_gpt.o
> diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
> new file mode 100644
> index 0000000000..9808036eec
> --- /dev/null
> +++ b/lib/fwu_updates/fwu.c
> @@ -0,0 +1,22 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022, Linaro Limited
> + */
> +
> +#include <fwu.h>
> +#include <fwu_mdata.h>
> +
> +__weak int fwu_plat_get_update_index(u32 *update_idx)

This should come from the device tree, right?

> +{
> +       int ret;
> +       u32 active_idx;
> +
> +       ret = fwu_get_active_index(&active_idx);
> +
> +       if (ret < 0)
> +               return -1;
> +
> +       *update_idx = active_idx ^= 0x1;
> +
> +       return ret;
> +}
> diff --git a/lib/fwu_updates/fwu_gpt.c b/lib/fwu_updates/fwu_gpt.c
> new file mode 100644
> index 0000000000..b7ccca3645
> --- /dev/null
> +++ b/lib/fwu_updates/fwu_gpt.c
> @@ -0,0 +1,104 @@
> +// 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_gpt_get_alt_num(struct blk_desc *desc, efi_guid_t
*image_guid,
> +                              int *alt_num, unsigned char dfu_dev)
> +{
> +       int ret = -1;
> +       int i, part, dev_num;
> +       int nalt;
> +       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);
> +
> +       nalt = 0;
> +       list_for_each_entry(dfu, &dfu_list, list) {
> +               nalt++;
> +       }
> +
> +       if (!nalt) {
> +               log_warning("No entities in dfu_alt_info\n");
> +               dfu_free_entities();
> +               return -ENOENT;
> +       }
> +
> +       for (i = 0; i < nalt; i++) {
> +               dfu = dfu_get_entity(i);
> +
> +               if (!dfu)
> +                       continue;
> +
> +               /*
> +                * Currently, Multi Bank update
> +                * feature is being supported
> +                * only on GPT partitioned
> +                * MMC/SD devices.
> +                */
> +               if (dfu->dev_type != dfu_dev)
> +                       continue;
> +
> +               if (dfu->layout == DFU_RAW_ADDR &&
> +                   dfu->data.mmc.dev_num == dev_num &&
> +                   dfu->data.mmc.part == part) {
> +                       *alt_num = dfu->alt;
> +                       ret = 0;
> +                       break;
> +               }
> +       }
> +
> +       dfu_free_entities();
> +
> +       return ret;
> +}
> +
> +__weak int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t
*image_guid,
> +                               int *alt_num)
> +{
> +       struct blk_desc *desc;
> +       struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> +
> +       desc = dev_get_uclass_plat(priv->blk_dev);
> +       if (!desc) {
> +               log_err("Block device not found\n");
> +               return -ENODEV;
> +       }

Please remove all of these checks. They do nothing since it cannot happen.
Also please check you don't use -ENODEV anywhere.

You cannot use a device (in most cases) unless it is probed, so make sure
you are using the 'get' uclass functions rather than the 'find' functions

> +
> +       return fwu_gpt_get_alt_num(desc, image_guid, alt_num,
DFU_DEV_MMC);
> +}
> --
> 2.34.1
>

Regards,
Simon
diff mbox series

Patch

diff --git a/include/fwu.h b/include/fwu.h
index 8259c75d12..f14175cc9a 100644
--- a/include/fwu.h
+++ b/include/fwu.h
@@ -51,4 +51,5 @@  int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank);
 
 int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid,
 			 int *alt_num);
+int fwu_plat_get_update_index(u32 *update_idx);
 #endif /* _FWU_H_ */
diff --git a/lib/fwu_updates/Makefile b/lib/fwu_updates/Makefile
new file mode 100644
index 0000000000..1993088e5b
--- /dev/null
+++ b/lib/fwu_updates/Makefile
@@ -0,0 +1,7 @@ 
+# SPDX-License-Identifier: GPL-2.0-or-later
+#
+# Copyright (c) 2022, Linaro Limited
+#
+
+obj-$(CONFIG_FWU_MULTI_BANK_UPDATE) += fwu.o
+obj-$(CONFIG_FWU_MDATA_GPT_BLK) += fwu_gpt.o
diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
new file mode 100644
index 0000000000..9808036eec
--- /dev/null
+++ b/lib/fwu_updates/fwu.c
@@ -0,0 +1,22 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022, Linaro Limited
+ */
+
+#include <fwu.h>
+#include <fwu_mdata.h>
+
+__weak int fwu_plat_get_update_index(u32 *update_idx)
+{
+	int ret;
+	u32 active_idx;
+
+	ret = fwu_get_active_index(&active_idx);
+
+	if (ret < 0)
+		return -1;
+
+	*update_idx = active_idx ^= 0x1;
+
+	return ret;
+}
diff --git a/lib/fwu_updates/fwu_gpt.c b/lib/fwu_updates/fwu_gpt.c
new file mode 100644
index 0000000000..b7ccca3645
--- /dev/null
+++ b/lib/fwu_updates/fwu_gpt.c
@@ -0,0 +1,104 @@ 
+// 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_gpt_get_alt_num(struct blk_desc *desc, efi_guid_t *image_guid,
+			       int *alt_num, unsigned char dfu_dev)
+{
+	int ret = -1;
+	int i, part, dev_num;
+	int nalt;
+	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);
+
+	nalt = 0;
+	list_for_each_entry(dfu, &dfu_list, list) {
+		nalt++;
+	}
+
+	if (!nalt) {
+		log_warning("No entities in dfu_alt_info\n");
+		dfu_free_entities();
+		return -ENOENT;
+	}
+
+	for (i = 0; i < nalt; i++) {
+		dfu = dfu_get_entity(i);
+
+		if (!dfu)
+			continue;
+
+		/*
+		 * Currently, Multi Bank update
+		 * feature is being supported
+		 * only on GPT partitioned
+		 * MMC/SD devices.
+		 */
+		if (dfu->dev_type != dfu_dev)
+			continue;
+
+		if (dfu->layout == DFU_RAW_ADDR &&
+		    dfu->data.mmc.dev_num == dev_num &&
+		    dfu->data.mmc.part == part) {
+			*alt_num = dfu->alt;
+			ret = 0;
+			break;
+		}
+	}
+
+	dfu_free_entities();
+
+	return ret;
+}
+
+__weak int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid,
+				int *alt_num)
+{
+	struct blk_desc *desc;
+	struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
+
+	desc = dev_get_uclass_plat(priv->blk_dev);
+	if (!desc) {
+		log_err("Block device not found\n");
+		return -ENODEV;
+	}
+
+	return fwu_gpt_get_alt_num(desc, image_guid, alt_num, DFU_DEV_MMC);
+}