Re: [PATCH v5 02/23] FWU: Add FWU metadata structure and driver for accessing metadata
Hi Sughosh, On Thu, 23 Jun 2022 at 08:24, Sughosh Ganu wrote: > > hi Etienne, > > On Tue, 21 Jun 2022 at 16:24, Etienne Carriere > wrote: > > > > Hello Sughosh, > > > > > > > > On Thu, 9 Jun 2022 at 14:30, Sughosh Ganu 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 > > > --- > > > 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 > > > > > > > > > diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c > > > b/drivers/fwu-mdata/fwu-mdata-uclass.c > > > new file mode 100644 > > > index 00..1530ceb01d > > > --- /dev/null > > > +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c > > > +/** > > > + * 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
Re: [PATCH v5 02/23] FWU: Add FWU metadata structure and driver for accessing metadata
hi Etienne,
On Tue, 21 Jun 2022 at 16:24, Etienne Carriere
wrote:
>
> Hello Sughosh,
>
>
>
> On Thu, 9 Jun 2022 at 14:30, Sughosh Ganu 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
> > ---
> > 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
> >
> > diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c
> > b/drivers/fwu-mdata/fwu-mdata-uclass.c
> > new file mode 100644
> > index 00..1530ceb01d
> > --- /dev/null
> > +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
> > @@ -0,0 +1,459 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2022, Linaro Limited
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +#include
> > +#include
> > +
> > +#define IMAGE_ACCEPT_SET BIT(0)
> > +#define IMAGE_ACCEPT_CLEAR BIT(1)
> > +
> > +static int fwu_get_dev_ops(struct udevice **dev,
> > + const struct fwu_mdata_ops **ops)
> > +{
> > + int ret;
> > +
> > + ret = uclass_get_device(UCLASS_FWU_MDATA, 0, dev);
> > + if (ret) {
> > + log_debug("Cannot find fwu device\n");
> > + return ret;
> > + }
> > +
> > + if ((*ops = device_get_ops(*dev)) == NULL) {
> > + log_debug("Cannot get fwu device ops\n");
> > + return -ENOSYS;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * fwu_verify_mdata() - Verify the FWU metadata
> > + * @mdata: FWU metadata structure
> > + * @pri_part: FWU metadata partition is primary or secondary
> > + *
> > + * Verify the FWU metadata by computing the CRC32 for the metadata
> > + * structure and comparing it against the CRC32 value stored as part
> > + * of the structure.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part)
> > +{
> > + u32 calc_crc32;
> > + void *buf;
> > +
> > + buf = >version;
> > + calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
> > +
> > + if (calc_crc32 != mdata->crc32) {
> > + log_err("crc32 check failed for %s FWU metadata
> > partition\n",
> > + pri_part ? "primary" : "secondary");
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * fwu_get_active_index() - Get active_index from the FWU metadata
> > + * @active_idx: active_index value to be read
> > + *
> > + * Read the active_index field from the FWU metadata and place it in
> > + * the variable pointed to be the function argument.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_get_active_index(u32 *active_idx)
> > +{
> > + int ret;
> > + struct fwu_mdata *mdata = NULL;
> > +
> > + ret = fwu_get_mdata();
> > + if (ret < 0) {
> > + log_err("Unable to get valid FWU metadata\n");
> > + goto out;
> > + }
> > +
> > + /*
> > +* Found the FWU metadata partition, now read the active_index
> > +* value
> > +*/
> > + *active_idx = mdata->active_index;
> > + if (*active_idx > CONFIG_FWU_NUM_BANKS - 1) {
> > + log_err("Active index value read is incorrect\n");
> > + ret = -EINVAL;
> > + }
> > +
> > +out:
> > + free(mdata);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * fwu_update_active_index() - Update active_index from the FWU metadata
> > + * @active_idx: active_index value to be updated
> > + *
> > + * Update the active_index field in the FWU metadata
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_update_active_index(u32 active_idx)
> > +{
> > + int ret;
> > + struct fwu_mdata *mdata = NULL;
> > +
> > + if (active_idx > CONFIG_FWU_NUM_BANKS -
Re: [PATCH v5 02/23] FWU: Add FWU metadata structure and driver for accessing metadata
Hello Sughosh,
On Thu, 9 Jun 2022 at 14:30, Sughosh Ganu 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
> ---
> 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
>
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index b26ca8cf70..adc6079ecf 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -42,6 +42,8 @@ source "drivers/firmware/Kconfig"
>
> source "drivers/fpga/Kconfig"
>
> +source "drivers/fwu-mdata/Kconfig"
> +
> source "drivers/gpio/Kconfig"
>
> source "drivers/hwspinlock/Kconfig"
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 67c8af7442..901150bb35 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -83,6 +83,7 @@ obj-y += cache/
> obj-$(CONFIG_CPU) += cpu/
> obj-y += crypto/
> obj-$(CONFIG_FASTBOOT) += fastboot/
> +obj-$(CONFIG_DM_FWU_MDATA) += fwu-mdata/
> obj-y += misc/
> obj-$(CONFIG_MMC) += mmc/
> obj-$(CONFIG_NVME) += nvme/
> diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig
> new file mode 100644
> index 00..d6a21c8e19
> --- /dev/null
> +++ b/drivers/fwu-mdata/Kconfig
> @@ -0,0 +1,7 @@
> +config DM_FWU_MDATA
> + bool "Driver support for accessing FWU Metadata"
> + depends on DM
> + help
> + Enable support for accessing FWU Metadata partitions. The
> + FWU Metadata partitions reside on the same storage device
> + which contains the other FWU updatable firmware images.
> diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile
> new file mode 100644
> index 00..7fec7171f4
> --- /dev/null
> +++ b/drivers/fwu-mdata/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +#
> +# Copyright (c) 2022, Linaro Limited
> +#
> +
> +obj-$(CONFIG_DM_FWU_MDATA) += fwu-mdata-uclass.o
> diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c
> b/drivers/fwu-mdata/fwu-mdata-uclass.c
> new file mode 100644
> index 00..1530ceb01d
> --- /dev/null
> +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
> @@ -0,0 +1,459 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2022, Linaro Limited
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +
> +#define IMAGE_ACCEPT_SET BIT(0)
> +#define IMAGE_ACCEPT_CLEAR BIT(1)
> +
> +static int fwu_get_dev_ops(struct udevice **dev,
> + const struct fwu_mdata_ops **ops)
> +{
> + int ret;
> +
> + ret = uclass_get_device(UCLASS_FWU_MDATA, 0, dev);
> + if (ret) {
> + log_debug("Cannot find fwu device\n");
> + return ret;
> + }
> +
> + if ((*ops = device_get_ops(*dev)) == NULL) {
> + log_debug("Cannot get fwu device ops\n");
> + return -ENOSYS;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * fwu_verify_mdata() - Verify the FWU metadata
> + * @mdata: FWU metadata structure
> + * @pri_part: FWU metadata partition is primary or secondary
> + *
> + * Verify the FWU metadata by computing the CRC32 for the metadata
> + * structure and comparing it against the CRC32 value stored as part
> + * of the structure.
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part)
> +{
> + u32 calc_crc32;
> + void *buf;
> +
> + buf = >version;
> + calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
> +
> + if (calc_crc32 != mdata->crc32) {
> + log_err("crc32 check failed for %s FWU metadata partition\n",
> + pri_part ? "primary" : "secondary");
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * fwu_get_active_index() - Get active_index from the FWU metadata
> + * @active_idx: active_index value to be read
> + *
> + * Read the active_index field from the FWU metadata and place it in
> + * the variable
[PATCH v5 02/23] FWU: Add FWU metadata structure and driver for accessing metadata
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
---
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
diff --git a/drivers/Kconfig b/drivers/Kconfig
index b26ca8cf70..adc6079ecf 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -42,6 +42,8 @@ source "drivers/firmware/Kconfig"
source "drivers/fpga/Kconfig"
+source "drivers/fwu-mdata/Kconfig"
+
source "drivers/gpio/Kconfig"
source "drivers/hwspinlock/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index 67c8af7442..901150bb35 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -83,6 +83,7 @@ obj-y += cache/
obj-$(CONFIG_CPU) += cpu/
obj-y += crypto/
obj-$(CONFIG_FASTBOOT) += fastboot/
+obj-$(CONFIG_DM_FWU_MDATA) += fwu-mdata/
obj-y += misc/
obj-$(CONFIG_MMC) += mmc/
obj-$(CONFIG_NVME) += nvme/
diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig
new file mode 100644
index 00..d6a21c8e19
--- /dev/null
+++ b/drivers/fwu-mdata/Kconfig
@@ -0,0 +1,7 @@
+config DM_FWU_MDATA
+ bool "Driver support for accessing FWU Metadata"
+ depends on DM
+ help
+ Enable support for accessing FWU Metadata partitions. The
+ FWU Metadata partitions reside on the same storage device
+ which contains the other FWU updatable firmware images.
diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile
new file mode 100644
index 00..7fec7171f4
--- /dev/null
+++ b/drivers/fwu-mdata/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0+
+#
+# Copyright (c) 2022, Linaro Limited
+#
+
+obj-$(CONFIG_DM_FWU_MDATA) += fwu-mdata-uclass.o
diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c
b/drivers/fwu-mdata/fwu-mdata-uclass.c
new file mode 100644
index 00..1530ceb01d
--- /dev/null
+++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
@@ -0,0 +1,459 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2022, Linaro Limited
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+#define IMAGE_ACCEPT_SET BIT(0)
+#define IMAGE_ACCEPT_CLEAR BIT(1)
+
+static int fwu_get_dev_ops(struct udevice **dev,
+ const struct fwu_mdata_ops **ops)
+{
+ int ret;
+
+ ret = uclass_get_device(UCLASS_FWU_MDATA, 0, dev);
+ if (ret) {
+ log_debug("Cannot find fwu device\n");
+ return ret;
+ }
+
+ if ((*ops = device_get_ops(*dev)) == NULL) {
+ log_debug("Cannot get fwu device ops\n");
+ return -ENOSYS;
+ }
+
+ return 0;
+}
+
+/**
+ * fwu_verify_mdata() - Verify the FWU metadata
+ * @mdata: FWU metadata structure
+ * @pri_part: FWU metadata partition is primary or secondary
+ *
+ * Verify the FWU metadata by computing the CRC32 for the metadata
+ * structure and comparing it against the CRC32 value stored as part
+ * of the structure.
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part)
+{
+ u32 calc_crc32;
+ void *buf;
+
+ buf = >version;
+ calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
+
+ if (calc_crc32 != mdata->crc32) {
+ log_err("crc32 check failed for %s FWU metadata partition\n",
+ pri_part ? "primary" : "secondary");
+ return -1;
+ }
+
+ return 0;
+}
+
+/**
+ * fwu_get_active_index() - Get active_index from the FWU metadata
+ * @active_idx: active_index value to be read
+ *
+ * Read the active_index field from the FWU metadata and place it in
+ * the variable pointed to be the function argument.
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_get_active_index(u32 *active_idx)
+{
+ int ret;
+ struct fwu_mdata *mdata = NULL;
+
+ ret = fwu_get_mdata();
+ if (ret < 0) {
+ log_err("Unable to get valid FWU metadata\n");
+ goto out;
+ }
+
+ /*
+* Found the
