* [PATCH 0/4] FWU: Handle meta-data in common code
@ 2022-11-04 2:42 jassisinghbrar
2022-11-04 2:42 ` [PATCH 1/4] fwu: gpt: use cached meta-data partition numbers jassisinghbrar
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: jassisinghbrar @ 2022-11-04 2:42 UTC (permalink / raw)
To: u-boot
Cc: ilias.apalodimas, sughosh.ganu, etienne.carriere, trini, sjg,
xypron.glpk, patrick.delaunay, patrice.chotard, Jassi Brar
From: Jassi Brar <jaswinder.singh@linaro.org>
Hi,
The patchset reduces 400 lines of code, while keeping the functionality same and making
meta-data operations much faster (by using cached structures).
Issue:
meta-data copies (primary and secondary) are being handled by the backend/storage layer
instead of the common core in fwu.c (as also noted by Ilias) that is, gpt_blk.c manages
meta-data and similarly raw_mtd.c will have to do the same when it arrives. The code
could by make smaller, cleaner and optimised.
Basic idea:
Introduce .read_mdata() and .write_mdata() in fwu_mdata_ops that simply read/write
meta-data copy. The core code takes care of integrity and redundancy of the meta-data,
as a result we can get rid of every other callback .get_mdata() .update_mdata()
.get_mdata_part_num() .read_mdata_partition() .write_mdata_partition() and the
corresponding wrapper functions thereby making the code 100s of LOC smaller.
Get rid of fwu_check_mdata_validity() and fwu_mdata_check() which expected underlying
layer to manage and verify mdata copies.
Implement fwu_get_verified_mdata(struct fwu_mdata *mdata) public function that reads,
verifies and, if needed, fixes the meta-data copies.
Verified copy of meta-data is now cached as 'g_mdata' in fwu.c, which avoids multiple
low-level expensive read and parse calls.
gpt meta-data partition numbers are now cached in gpt_blk.c, so that we don't have to do expensive part_get_info() and uid ops.
Jassi Brar (4):
fwu: gpt: use cached meta-data partition numbers
fwu: move meta-data management in core
fwu: gpt: implement read_mdata and write_mdata callbacks
fwu: meta-data: switch to management by common code
cmd/fwu_mdata.c | 17 +-
drivers/fwu-mdata/fwu-mdata-uclass.c | 151 +-------------
drivers/fwu-mdata/gpt_blk.c | 175 +++++------------
include/fwu.h | 198 ++-----------------
lib/fwu_updates/fwu.c | 283 +++++++++++----------------
5 files changed, 202 insertions(+), 622 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/4] fwu: gpt: use cached meta-data partition numbers
2022-11-04 2:42 [PATCH 0/4] FWU: Handle meta-data in common code jassisinghbrar
@ 2022-11-04 2:42 ` jassisinghbrar
2022-11-07 17:24 ` Etienne Carriere
2022-11-04 2:43 ` [PATCH 2/4] fwu: move meta-data management in core jassisinghbrar
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: jassisinghbrar @ 2022-11-04 2:42 UTC (permalink / raw)
To: u-boot
Cc: ilias.apalodimas, sughosh.ganu, etienne.carriere, trini, sjg,
xypron.glpk, patrick.delaunay, patrice.chotard, Jassi Brar
From: Jassi Brar <jaswinder.singh@linaro.org>
Use cached values and avoid parsing and scanning through partitions
everytime for meta-data partitions because they can't change after bootup.
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
drivers/fwu-mdata/gpt_blk.c | 43 +++++++++++++++++++++----------------
1 file changed, 24 insertions(+), 19 deletions(-)
diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c
index d35ce49c5c..d706e3d4e4 100644
--- a/drivers/fwu-mdata/gpt_blk.c
+++ b/drivers/fwu-mdata/gpt_blk.c
@@ -24,8 +24,9 @@ enum {
MDATA_WRITE,
};
-static int gpt_get_mdata_partitions(struct blk_desc *desc,
- uint mdata_parts[2])
+static uint g_mdata_part[2] = {0, 0};
+
+static int gpt_get_mdata_partitions(struct blk_desc *desc)
{
int i, ret;
u32 nparts;
@@ -33,18 +34,19 @@ static int gpt_get_mdata_partitions(struct blk_desc *desc,
struct disk_partition info;
const efi_guid_t fwu_mdata_guid = FWU_MDATA_GUID;
+ /* if primary and secondary partitons already found */
+ if (g_mdata_part[0] && g_mdata_part[1])
+ return 0;
+
nparts = 0;
- for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) {
+ for (i = 1; i < MAX_SEARCH_PARTITIONS && nparts < 2; i++) {
if (part_get_info(desc, i, &info))
continue;
uuid_str_to_bin(info.type_guid, part_type_guid.b,
UUID_STR_FORMAT_GUID);
- if (!guidcmp(&fwu_mdata_guid, &part_type_guid)) {
- if (nparts < 2)
- mdata_parts[nparts] = i;
- ++nparts;
- }
+ if (!guidcmp(&fwu_mdata_guid, &part_type_guid))
+ g_mdata_part[nparts++] = i;
}
if (nparts != 2) {
@@ -127,26 +129,25 @@ static int fwu_gpt_update_mdata(struct udevice *dev, struct fwu_mdata *mdata)
{
int ret;
struct blk_desc *desc;
- uint mdata_parts[2];
struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
desc = dev_get_uclass_plat(priv->blk_dev);
- ret = gpt_get_mdata_partitions(desc, mdata_parts);
+ ret = gpt_get_mdata_partitions(desc);
if (ret < 0) {
log_debug("Error getting the FWU metadata partitions\n");
return -ENOENT;
}
/* First write the primary partition */
- ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, mdata_parts[0]);
+ ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, g_mdata_part[0]);
if (ret < 0) {
log_debug("Updating primary FWU metadata partition failed\n");
return ret;
}
/* And now the replica */
- ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, mdata_parts[1]);
+ ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, g_mdata_part[1]);
if (ret < 0) {
log_debug("Updating secondary FWU metadata partition failed\n");
return ret;
@@ -158,16 +159,14 @@ static int fwu_gpt_update_mdata(struct udevice *dev, struct fwu_mdata *mdata)
static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata *mdata)
{
int ret;
- uint mdata_parts[2];
-
- ret = gpt_get_mdata_partitions(desc, mdata_parts);
+ ret = gpt_get_mdata_partitions(desc);
if (ret < 0) {
log_debug("Error getting the FWU metadata partitions\n");
return -ENOENT;
}
- ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, mdata_parts[0]);
+ ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, g_mdata_part[0]);
if (ret < 0) {
log_debug("Failed to read the FWU metadata from the device\n");
return -EIO;
@@ -182,7 +181,7 @@ static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata *mdata)
* Try to read the replica.
*/
memset(mdata, '\0', sizeof(struct fwu_mdata));
- ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, mdata_parts[1]);
+ ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, g_mdata_part[1]);
if (ret < 0) {
log_debug("Failed to read the FWU metadata from the device\n");
return -EIO;
@@ -206,9 +205,15 @@ static int fwu_gpt_get_mdata(struct udevice *dev, struct fwu_mdata *mdata)
static int fwu_gpt_get_mdata_partitions(struct udevice *dev, uint *mdata_parts)
{
struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
+ int err;
+
+ err = gpt_get_mdata_partitions(dev_get_uclass_plat(priv->blk_dev));
+ if (!err) {
+ mdata_parts[0] = g_mdata_part[0];
+ mdata_parts[1] = g_mdata_part[1];
+ }
- return gpt_get_mdata_partitions(dev_get_uclass_plat(priv->blk_dev),
- mdata_parts);
+ return err;
}
static int fwu_gpt_read_mdata_partition(struct udevice *dev,
--
2.25.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/4] fwu: move meta-data management in core
2022-11-04 2:42 [PATCH 0/4] FWU: Handle meta-data in common code jassisinghbrar
2022-11-04 2:42 ` [PATCH 1/4] fwu: gpt: use cached meta-data partition numbers jassisinghbrar
@ 2022-11-04 2:43 ` jassisinghbrar
2022-11-07 17:23 ` Etienne Carriere
2022-11-04 2:43 ` [PATCH 3/4] fwu: gpt: implement read_mdata and write_mdata callbacks jassisinghbrar
2022-11-04 2:43 ` [PATCH 4/4] fwu: meta-data: switch to management by common code jassisinghbrar
3 siblings, 1 reply; 15+ messages in thread
From: jassisinghbrar @ 2022-11-04 2:43 UTC (permalink / raw)
To: u-boot
Cc: ilias.apalodimas, sughosh.ganu, etienne.carriere, trini, sjg,
xypron.glpk, patrick.delaunay, patrice.chotard, Jassi Brar
From: Jassi Brar <jaswinder.singh@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.
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
drivers/fwu-mdata/fwu-mdata-uclass.c | 34 +++++++
include/fwu.h | 41 ++++++++
lib/fwu_updates/fwu.c | 136 ++++++++++++++++++++++++++-
3 files changed, 207 insertions(+), 4 deletions(-)
diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c
index b477e9603f..ded9902971 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
+ */
+int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
+{
+ const struct fwu_mdata_ops *ops = device_get_ops(dev);
+
+ if (!ops->read_mdata) {
+ log_debug("read_mdata() method not defined\n");
+ return -ENOSYS;
+ }
+
+ return ops->read_mdata(dev, mdata, primary);
+}
+
+/**
+ * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
+ *
+ * Return: 0 if OK, -ve on error
+ */
+int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
+{
+ const struct fwu_mdata_ops *ops = device_get_ops(dev);
+
+ if (!ops->write_mdata) {
+ log_debug("write_mdata() method not defined\n");
+ return -ENOSYS;
+ }
+
+ return ops->write_mdata(dev, mdata, primary);
+}
+
/**
* fwu_get_mdata_part_num() - Get the FWU metadata partition numbers
* @dev: FWU metadata device
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
+ * @primary: If primary or secondary copy of meta-data is to be read
+ *
+ * Return: 0 if OK, -ve on error
+ */
+ int (*read_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
+
+ /**
+ * write_mdata() - Write the given FWU metadata copy
+ * @dev: FWU metadata device
+ * @mdata: Copy of the FWU metadata
+ * @primary: If primary or secondary copy of meta-data is to be written
+ *
+ * Return: 0 if OK, -ve on error
+ */
+ int (*write_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
+
/**
* check_mdata() - Check if the FWU metadata is valid
* @dev: FWU device
@@ -126,6 +146,27 @@ struct fwu_mdata_ops {
EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \
0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8)
+/**
+ * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
+ */
+int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
+
+/**
+ * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
+ */
+int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
+
+/**
+ * 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.
+ *
+ * Return: 0 if OK, -ve on error
+*/
+int fwu_get_verified_mdata(struct fwu_mdata *mdata);
+
/**
* fwu_check_mdata_validity() - Check for validity of the FWU metadata copies
*
diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
index 5313d07302..3d51b91ed5 100644
--- a/lib/fwu_updates/fwu.c
+++ b/lib/fwu_updates/fwu.c
@@ -15,13 +15,13 @@
#include <linux/errno.h>
#include <linux/types.h>
+#include <u-boot/crc.h>
+
+static struct fwu_mdata g_mdata = { 0 };
+static struct udevice *g_dev;
static u8 in_trial;
static u8 boottime_check;
-#include <linux/errno.h>
-#include <linux/types.h>
-#include <u-boot/crc.h>
-
enum {
IMAGE_ACCEPT_SET = 1,
IMAGE_ACCEPT_CLEAR,
@@ -161,6 +161,134 @@ static int fwu_get_image_type_id(u8 *image_index, efi_guid_t *image_type_id)
return -ENOENT;
}
+/**
+ * fwu_sync_mdata() - Update given meta-data partition(s) with the copy provided
+ * @mdata: FWU metadata structure
+ * @part: Bitmask of FWU metadata partitions to be written to
+ *
+ * Return: 0 if OK, -ve on error
+ */
+static int fwu_sync_mdata(struct fwu_mdata *mdata, int part)
+{
+ void *buf = &mdata->version;
+ int err = 0;
+
+ /*
+ * 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));
+
+ if (part & PRIMARY_PART)
+ err = fwu_write_mdata(g_dev, mdata, true);
+
+ if (err) {
+ log_err("Unable to write primary mdata\n");
+ return err;
+ }
+
+ if (part & SECONDARY_PART)
+ err = fwu_write_mdata(g_dev, mdata, false);
+
+ if (err) {
+ log_err("Unable to write secondary mdata\n");
+ return err;
+ }
+
+ /* update the cached copy of meta-data */
+ memcpy(&g_mdata, mdata, sizeof(struct fwu_mdata));
+
+ return 0;
+}
+
+static inline int mdata_crc_check(struct fwu_mdata *mdata)
+{
+ u32 calc_crc32 = crc32(0, &mdata->version, sizeof(*mdata) - sizeof(u32));
+ return calc_crc32 == mdata->crc32 ? 0 : -EINVAL;
+}
+
+/**
+ * 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.
+ *
+ * Return: 0 if OK, -ve on error
+ */
+int fwu_get_verified_mdata(struct fwu_mdata *mdata)
+{
+ int err;
+ bool pri_ok, sec_ok;
+ struct fwu_mdata s, *p_mdata, *s_mdata;
+
+ p_mdata = &g_mdata;
+ s_mdata = &s;
+
+ /* if mdata already read and ready */
+ err = mdata_crc_check(p_mdata, true);
+ if (!err) {
+ if (mdata)
+ memcpy(mdata, p_mdata, sizeof(struct fwu_mdata));
+ return 0;
+ }
+ /* else read, verify and, if needed, fix mdata */
+
+ pri_ok = false;
+ err = fwu_read_mdata(g_dev, p_mdata, true);
+ if (!err) {
+ err = mdata_crc_check(p_mdata, true);
+ if (!err)
+ pri_ok = true;
+ else
+ log_debug("primary mdata: crc32 failed\n");
+ }
+
+ sec_ok = false;
+ err = fwu_read_mdata(g_dev, s_mdata, false);
+ if (!err) {
+ err = mdata_crc_check(s_mdata, false);
+ if (!err)
+ sec_ok = true;
+ else
+ log_debug("secondary mdata: crc32 failed\n");
+ }
+
+ if (pri_ok && sec_ok) {
+ /*
+ * Before returning, check that both the
+ * FWU metadata copies are the same. If not,
+ * populate the secondary partition from the
+ * primary partition copy.
+ */
+ if (memcmp(p_mdata, s_mdata, sizeof(struct fwu_mdata))) {
+ log_info("Both FWU metadata copies are valid but do not match.");
+ log_info(" Restoring the secondary partition from the primary\n");
+ sec_ok = false;
+ }
+ }
+
+ if (!pri_ok) {
+ memcpy(p_mdata, s_mdata, sizeof(struct fwu_mdata));
+ err = fwu_sync_mdata(p_mdata, PRIMARY_PART);
+ }
+
+ if (!sec_ok) {
+ memcpy(s_mdata, p_mdata, sizeof(struct fwu_mdata));
+ err = fwu_sync_mdata(s_mdata, SECONDARY_PART);
+ }
+
+ /* make sure atleast one copy is good */
+ err = mdata_crc_check(p_mdata, true);
+ if (err)
+ log_debug("mdata saving... crc32 failed\n");
+ else if (mdata)
+ memcpy(mdata, p_mdata, sizeof(struct fwu_mdata));
+
+ return err;
+}
+
/**
* fwu_verify_mdata() - Verify the FWU metadata
* @mdata: FWU metadata structure
--
2.25.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/4] fwu: gpt: implement read_mdata and write_mdata callbacks
2022-11-04 2:42 [PATCH 0/4] FWU: Handle meta-data in common code jassisinghbrar
2022-11-04 2:42 ` [PATCH 1/4] fwu: gpt: use cached meta-data partition numbers jassisinghbrar
2022-11-04 2:43 ` [PATCH 2/4] fwu: move meta-data management in core jassisinghbrar
@ 2022-11-04 2:43 ` jassisinghbrar
2022-11-07 17:23 ` Etienne Carriere
2022-11-04 2:43 ` [PATCH 4/4] fwu: meta-data: switch to management by common code jassisinghbrar
3 siblings, 1 reply; 15+ messages in thread
From: jassisinghbrar @ 2022-11-04 2:43 UTC (permalink / raw)
To: u-boot
Cc: ilias.apalodimas, sughosh.ganu, etienne.carriere, trini, sjg,
xypron.glpk, patrick.delaunay, patrice.chotard, Jassi Brar
From: Jassi Brar <jaswinder.singh@linaro.org>
Moving towards using common code for meta-data management,
implement the read/write mdata hooks.
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
drivers/fwu-mdata/gpt_blk.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c
index d706e3d4e4..7fda041302 100644
--- a/drivers/fwu-mdata/gpt_blk.c
+++ b/drivers/fwu-mdata/gpt_blk.c
@@ -272,7 +272,43 @@ static int fwu_mdata_gpt_blk_probe(struct udevice *dev)
return 0;
}
+static int fwu_gpt_read_mdata(struct udevice *dev, struct fwu_mdata *mdata,
+ bool primary)
+{
+ struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
+ struct blk_desc *desc = dev_get_uclass_plat(priv->blk_dev);
+ int ret;
+
+ ret = gpt_get_mdata_partitions(desc);
+ if (ret < 0) {
+ log_debug("Error getting the FWU metadata partitions\n");
+ return -ENOENT;
+ }
+
+ return gpt_read_write_mdata(desc, mdata, MDATA_READ,
+ primary ? g_mdata_part[0] : g_mdata_part[1]);
+}
+
+static int fwu_gpt_write_mdata(struct udevice *dev, struct fwu_mdata *mdata,
+ bool primary)
+{
+ struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
+ struct blk_desc *desc = dev_get_uclass_plat(priv->blk_dev);
+ int ret;
+
+ ret = gpt_get_mdata_partitions(desc);
+ if (ret < 0) {
+ log_debug("Error getting the FWU metadata partitions\n");
+ return -ENOENT;
+ }
+
+ return gpt_read_write_mdata(desc, mdata, MDATA_WRITE,
+ primary ? g_mdata_part[0] : g_mdata_part[1]);
+}
+
static const struct fwu_mdata_ops fwu_gpt_blk_ops = {
+ .read_mdata = fwu_gpt_read_mdata,
+ .write_mdata = fwu_gpt_write_mdata,
.get_mdata = fwu_gpt_get_mdata,
.update_mdata = fwu_gpt_update_mdata,
.get_mdata_part_num = fwu_gpt_get_mdata_partitions,
--
2.25.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 4/4] fwu: meta-data: switch to management by common code
2022-11-04 2:42 [PATCH 0/4] FWU: Handle meta-data in common code jassisinghbrar
` (2 preceding siblings ...)
2022-11-04 2:43 ` [PATCH 3/4] fwu: gpt: implement read_mdata and write_mdata callbacks jassisinghbrar
@ 2022-11-04 2:43 ` jassisinghbrar
2022-11-07 17:23 ` Etienne Carriere
3 siblings, 1 reply; 15+ messages in thread
From: jassisinghbrar @ 2022-11-04 2:43 UTC (permalink / raw)
To: u-boot
Cc: ilias.apalodimas, sughosh.ganu, etienne.carriere, trini, sjg,
xypron.glpk, patrick.delaunay, patrice.chotard, Jassi Brar
From: Jassi Brar <jaswinder.singh@linaro.org>
The common code can now read, verify and fix meta-data copies
while exposing one consistent structure to users.
Only the .read_mdata() and .write_mdata() callbacks of fwu_mdata_ops
are needed. Get rid of .get_mdata() .update_mdata() .get_mdata_part_num()
.read_mdata_partition() and .write_mdata_partition() and also the
corresponding wrapper functions.
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
cmd/fwu_mdata.c | 17 +-
drivers/fwu-mdata/fwu-mdata-uclass.c | 165 -------------------
drivers/fwu-mdata/gpt_blk.c | 124 +-------------
include/fwu.h | 199 -----------------------
lib/fwu_updates/fwu.c | 235 ++++-----------------------
5 files changed, 38 insertions(+), 702 deletions(-)
diff --git a/cmd/fwu_mdata.c b/cmd/fwu_mdata.c
index f04af27de6..9b70340368 100644
--- a/cmd/fwu_mdata.c
+++ b/cmd/fwu_mdata.c
@@ -43,23 +43,10 @@ static void print_mdata(struct fwu_mdata *mdata)
int do_fwu_mdata_read(struct cmd_tbl *cmdtp, int flag,
int argc, char * const argv[])
{
- struct udevice *dev;
int ret = CMD_RET_SUCCESS, res;
- struct fwu_mdata mdata = { 0 };
+ struct fwu_mdata mdata;
- if (uclass_get_device(UCLASS_FWU_MDATA, 0, &dev) || !dev) {
- log_err("Unable to get FWU metadata device\n");
- return CMD_RET_FAILURE;
- }
-
- res = fwu_check_mdata_validity();
- if (res < 0) {
- log_err("FWU Metadata check failed\n");
- ret = CMD_RET_FAILURE;
- goto out;
- }
-
- res = fwu_get_mdata(dev, &mdata);
+ res = fwu_get_verified_mdata(&mdata);
if (res < 0) {
log_err("Unable to get valid FWU metadata\n");
ret = CMD_RET_FAILURE;
diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c
index ded9902971..8190113843 100644
--- a/drivers/fwu-mdata/fwu-mdata-uclass.c
+++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
@@ -14,7 +14,6 @@
#include <linux/errno.h>
#include <linux/types.h>
-#include <u-boot/crc.h>
/**
* fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
@@ -50,170 +49,6 @@ int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
return ops->write_mdata(dev, mdata, primary);
}
-/**
- * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers
- * @dev: FWU metadata device
- * @mdata_parts: array for storing the metadata partition numbers
- *
- * Get the partition numbers on the storage device on which the
- * FWU metadata is stored. Two partition numbers will be returned.
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_get_mdata_part_num(struct udevice *dev, uint *mdata_parts)
-{
- const struct fwu_mdata_ops *ops = device_get_ops(dev);
-
- if (!ops->get_mdata_part_num) {
- log_debug("get_mdata_part_num() method not defined\n");
- return -ENOSYS;
- }
-
- return ops->get_mdata_part_num(dev, mdata_parts);
-}
-
-/**
- * fwu_read_mdata_partition() - Read the FWU metadata from a partition
- * @dev: FWU metadata device
- * @mdata: Copy of the FWU metadata
- * @part_num: Partition number from which FWU metadata is to be read
- *
- * Read the FWU metadata from the specified partition number
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_read_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata,
- uint part_num)
-{
- const struct fwu_mdata_ops *ops = device_get_ops(dev);
-
- if (!ops->read_mdata_partition) {
- log_debug("read_mdata_partition() method not defined\n");
- return -ENOSYS;
- }
-
- return ops->read_mdata_partition(dev, mdata, part_num);
-}
-
-/**
- * fwu_write_mdata_partition() - Write the FWU metadata to a partition
- * @dev: FWU metadata device
- * @mdata: Copy of the FWU metadata
- * @part_num: Partition number to which FWU metadata is to be written
- *
- * Write the FWU metadata to the specified partition number
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_write_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata,
- uint part_num)
-{
- const struct fwu_mdata_ops *ops = device_get_ops(dev);
-
- if (!ops->write_mdata_partition) {
- log_debug("write_mdata_partition() method not defined\n");
- return -ENOSYS;
- }
-
- return ops->write_mdata_partition(dev, mdata, part_num);
-}
-
-/**
- * fwu_mdata_check() - Check if the FWU metadata is valid
- * @dev: FWU metadata device
- *
- * Validate both copies of the FWU metadata. If one of the copies
- * has gone bad, restore it from the other copy.
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_mdata_check(struct udevice *dev)
-{
- const struct fwu_mdata_ops *ops = device_get_ops(dev);
-
- if (!ops->check_mdata) {
- log_debug("check_mdata() method not defined\n");
- return -ENOSYS;
- }
-
- return ops->check_mdata(dev);
-}
-
-/**
- * fwu_get_mdata() - Get a FWU metadata copy
- * @dev: FWU metadata device
- * @mdata: Copy of the FWU metadata
- *
- * Get a valid copy of the FWU metadata.
- *
- * Note: This function is to be called first when modifying any fields
- * in the metadata. The sequence of calls to modify any field in the
- * metadata would be 1) fwu_get_mdata 2) Modify metadata, followed by
- * 3) fwu_update_mdata
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_get_mdata(struct udevice *dev, struct fwu_mdata *mdata)
-{
- const struct fwu_mdata_ops *ops = device_get_ops(dev);
-
- if (!ops->get_mdata) {
- log_debug("get_mdata() method not defined\n");
- return -ENOSYS;
- }
-
- return ops->get_mdata(dev, mdata);
-}
-
-/**
- * fwu_update_mdata() - Update the FWU metadata
- * @dev: FWU metadata device
- * @mdata: Copy of the FWU metadata
- *
- * Update the FWU metadata structure by writing to the
- * FWU metadata partitions.
- *
- * Note: This function is not to be called directly to update the
- * metadata fields. The sequence of function calls should be
- * 1) fwu_get_mdata() 2) Modify the medata fields 3) fwu_update_mdata()
- *
- * The sequence of updating the partitions should be, update the
- * primary metadata partition (first partition encountered), followed
- * by updating the secondary partition. With this update sequence, in
- * the rare scenario that the two metadata partitions are valid but do
- * not match, maybe due to power outage at the time of updating the
- * metadata copies, the secondary partition can be updated from the
- * primary.
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_update_mdata(struct udevice *dev, struct fwu_mdata *mdata)
-{
- void *buf;
- const struct fwu_mdata_ops *ops = device_get_ops(dev);
-
- if (!ops->update_mdata) {
- log_debug("get_mdata() method not defined\n");
- return -ENOSYS;
- }
-
- /*
- * Calculate the crc32 for the updated FWU metadata
- * and put the updated value in the FWU metadata crc32
- * field
- */
- buf = &mdata->version;
- mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
-
- return ops->update_mdata(dev, mdata);
-}
-
UCLASS_DRIVER(fwu_mdata) = {
.id = UCLASS_FWU_MDATA,
.name = "fwu-mdata",
diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c
index 7fda041302..7f1782af1a 100644
--- a/drivers/fwu-mdata/gpt_blk.c
+++ b/drivers/fwu-mdata/gpt_blk.c
@@ -28,7 +28,7 @@ static uint g_mdata_part[2] = {0, 0};
static int gpt_get_mdata_partitions(struct blk_desc *desc)
{
- int i, ret;
+ int i;
u32 nparts;
efi_guid_t part_type_guid;
struct disk_partition info;
@@ -52,12 +52,12 @@ static int gpt_get_mdata_partitions(struct blk_desc *desc)
if (nparts != 2) {
log_debug("Expect two copies of the FWU metadata instead of %d\n",
nparts);
- ret = -EINVAL;
- } else {
- ret = 0;
+ g_mdata_part[0] = 0;
+ g_mdata_part[1] = 0;
+ return -EINVAL;
}
- return ret;
+ return 0;
}
static int gpt_get_mdata_disk_part(struct blk_desc *desc,
@@ -125,115 +125,6 @@ static int gpt_read_write_mdata(struct blk_desc *desc,
return 0;
}
-static int fwu_gpt_update_mdata(struct udevice *dev, struct fwu_mdata *mdata)
-{
- int ret;
- struct blk_desc *desc;
- struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
-
- desc = dev_get_uclass_plat(priv->blk_dev);
-
- ret = gpt_get_mdata_partitions(desc);
- if (ret < 0) {
- log_debug("Error getting the FWU metadata partitions\n");
- return -ENOENT;
- }
-
- /* First write the primary partition */
- ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, g_mdata_part[0]);
- if (ret < 0) {
- log_debug("Updating primary FWU metadata partition failed\n");
- return ret;
- }
-
- /* And now the replica */
- ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, g_mdata_part[1]);
- if (ret < 0) {
- log_debug("Updating secondary FWU metadata partition failed\n");
- return ret;
- }
-
- return 0;
-}
-
-static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata *mdata)
-{
- int ret;
-
- ret = gpt_get_mdata_partitions(desc);
- if (ret < 0) {
- log_debug("Error getting the FWU metadata partitions\n");
- return -ENOENT;
- }
-
- ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, g_mdata_part[0]);
- if (ret < 0) {
- log_debug("Failed to read the FWU metadata from the device\n");
- return -EIO;
- }
-
- ret = fwu_verify_mdata(mdata, 1);
- if (!ret)
- return 0;
-
- /*
- * Verification of the primary FWU metadata copy failed.
- * Try to read the replica.
- */
- memset(mdata, '\0', sizeof(struct fwu_mdata));
- ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, g_mdata_part[1]);
- if (ret < 0) {
- log_debug("Failed to read the FWU metadata from the device\n");
- return -EIO;
- }
-
- ret = fwu_verify_mdata(mdata, 0);
- if (!ret)
- return 0;
-
- /* Both the FWU metadata copies are corrupted. */
- return -EIO;
-}
-
-static int fwu_gpt_get_mdata(struct udevice *dev, struct fwu_mdata *mdata)
-{
- struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
-
- return gpt_get_mdata(dev_get_uclass_plat(priv->blk_dev), mdata);
-}
-
-static int fwu_gpt_get_mdata_partitions(struct udevice *dev, uint *mdata_parts)
-{
- struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
- int err;
-
- err = gpt_get_mdata_partitions(dev_get_uclass_plat(priv->blk_dev));
- if (!err) {
- mdata_parts[0] = g_mdata_part[0];
- mdata_parts[1] = g_mdata_part[1];
- }
-
- return err;
-}
-
-static int fwu_gpt_read_mdata_partition(struct udevice *dev,
- struct fwu_mdata *mdata, uint part_num)
-{
- struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
-
- return gpt_read_write_mdata(dev_get_uclass_plat(priv->blk_dev),
- mdata, MDATA_READ, part_num);
-}
-
-static int fwu_gpt_write_mdata_partition(struct udevice *dev,
- struct fwu_mdata *mdata, uint part_num)
-{
- struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
-
- return gpt_read_write_mdata(dev_get_uclass_plat(priv->blk_dev),
- mdata, MDATA_WRITE, part_num);
-}
-
static int fwu_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev)
{
u32 phandle;
@@ -309,11 +200,6 @@ static int fwu_gpt_write_mdata(struct udevice *dev, struct fwu_mdata *mdata,
static const struct fwu_mdata_ops fwu_gpt_blk_ops = {
.read_mdata = fwu_gpt_read_mdata,
.write_mdata = fwu_gpt_write_mdata,
- .get_mdata = fwu_gpt_get_mdata,
- .update_mdata = fwu_gpt_update_mdata,
- .get_mdata_part_num = fwu_gpt_get_mdata_partitions,
- .read_mdata_partition = fwu_gpt_read_mdata_partition,
- .write_mdata_partition = fwu_gpt_write_mdata_partition,
};
static const struct udevice_id fwu_mdata_ids[] = {
diff --git a/include/fwu.h b/include/fwu.h
index 1a700c9e6a..23bd97fe86 100644
--- a/include/fwu.h
+++ b/include/fwu.h
@@ -18,11 +18,6 @@ struct fwu_mdata_gpt_blk_priv {
struct udevice *blk_dev;
};
-/**
- * @mdata_check: check the validity of the FWU metadata partitions
- * @get_mdata() - Get a FWU metadata copy
- * @update_mdata() - Update the FWU metadata copy
- */
struct fwu_mdata_ops {
/**
* read_mdata() - Populate the asked FWU metadata copy
@@ -43,78 +38,6 @@ struct fwu_mdata_ops {
* Return: 0 if OK, -ve on error
*/
int (*write_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
-
- /**
- * check_mdata() - Check if the FWU metadata is valid
- * @dev: FWU device
- *
- * Validate both copies of the FWU metadata. If one of the copies
- * has gone bad, restore it from the other copy.
- *
- * Return: 0 if OK, -ve on error
- */
- int (*check_mdata)(struct udevice *dev);
-
- /**
- * get_mdata() - Get a FWU metadata copy
- * @dev: FWU device
- * @mdata: Pointer to FWU metadata
- *
- * Get a valid copy of the FWU metadata.
- *
- * Return: 0 if OK, -ve on error
- */
- int (*get_mdata)(struct udevice *dev, struct fwu_mdata *mdata);
-
- /**
- * update_mdata() - Update the FWU metadata
- * @dev: FWU device
- * @mdata: Copy of the FWU metadata
- *
- * Update the FWU metadata structure by writing to the
- * FWU metadata partitions.
- *
- * Return: 0 if OK, -ve on error
- */
- int (*update_mdata)(struct udevice *dev, struct fwu_mdata *mdata);
-
- /**
- * get_mdata_part_num() - Get the FWU metadata partition numbers
- * @dev: FWU metadata device
- * @mdata_parts: array for storing the metadata partition numbers
- *
- * Get the partition numbers on the storage device on which the
- * FWU metadata is stored. Two partition numbers will be returned.
- *
- * Return: 0 if OK, -ve on error
- */
- int (*get_mdata_part_num)(struct udevice *dev, uint *mdata_parts);
-
- /**
- * read_mdata_partition() - Read the FWU metadata from a partition
- * @dev: FWU metadata device
- * @mdata: Copy of the FWU metadata
- * @part_num: Partition number from which FWU metadata is to be read
- *
- * Read the FWU metadata from the specified partition number
- *
- * Return: 0 if OK, -ve on error
- */
- int (*read_mdata_partition)(struct udevice *dev,
- struct fwu_mdata *mdata, uint part_num);
-
- /**
- * write_mdata_partition() - Write the FWU metadata to a partition
- * @dev: FWU metadata device
- * @mdata: Copy of the FWU metadata
- * @part_num: Partition number to which FWU metadata is to be written
- *
- * Write the FWU metadata to the specified partition number
- *
- * Return: 0 if OK, -ve on error
- */
- int (*write_mdata_partition)(struct udevice *dev,
- struct fwu_mdata *mdata, uint part_num);
};
#define FWU_MDATA_VERSION 0x1
@@ -167,102 +90,6 @@ int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
*/
int fwu_get_verified_mdata(struct fwu_mdata *mdata);
-/**
- * fwu_check_mdata_validity() - Check for validity of the FWU metadata copies
- *
- * 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.
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_check_mdata_validity(void);
-
-/**
- * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers
- * @dev: FWU metadata device
- * @mdata_parts: array for storing the metadata partition numbers
- *
- * Get the partition numbers on the storage device on which the
- * FWU metadata is stored. Two partition numbers will be returned
- * through the array.
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_get_mdata_part_num(struct udevice *dev, uint *mdata_parts);
-
-/**
- * fwu_read_mdata_partition() - Read the FWU metadata from a partition
- * @dev: FWU metadata device
- * @mdata: Copy of the FWU metadata
- * @part_num: Partition number from which FWU metadata is to be read
- *
- * Read the FWU metadata from the specified partition number
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_read_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata,
- uint part_num);
-
-/**
- * fwu_write_mdata_partition() - Write the FWU metadata to a partition
- * @dev: FWU metadata device
- * @mdata: Copy of the FWU metadata
- * @part_num: Partition number to which FWU metadata is to be written
- *
- * Write the FWU metadata to the specified partition number
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_write_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata,
- uint part_num);
-
-/**
- * fwu_get_mdata() - Get a FWU metadata copy
- * @dev: FWU metadata device
- * @mdata: Copy of the FWU metadata
- *
- * Get a valid copy of the FWU metadata.
- *
- * Note: This function is to be called first when modifying any fields
- * in the metadata. The sequence of calls to modify any field in the
- * metadata would be 1) fwu_get_mdata 2) Modify metadata, followed by
- * 3) fwu_update_mdata
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_get_mdata(struct udevice *dev, struct fwu_mdata *mdata);
-
-/**
- * fwu_update_mdata() - Update the FWU metadata
- * @dev: FWU metadata device
- * @mdata: Copy of the FWU metadata
- *
- * Update the FWU metadata structure by writing to the
- * FWU metadata partitions.
- *
- * Note: This function is not to be called directly to update the
- * metadata fields. The sequence of function calls should be
- * 1) fwu_get_mdata() 2) Modify the medata fields 3) fwu_update_mdata()
- *
- * The sequence of updating the partitions should be, update the
- * primary metadata partition (first partition encountered), followed
- * by updating the secondary partition. With this update sequence, in
- * the rare scenario that the two metadata partitions are valid but do
- * not match, maybe due to power outage at the time of updating the
- * metadata copies, the secondary partition can be updated from the
- * primary.
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_update_mdata(struct udevice *dev, struct fwu_mdata *mdata);
-
/**
* fwu_get_active_index() - Get active_index from the FWU metadata
* @active_idxp: active_index value to be read
@@ -303,18 +130,6 @@ int fwu_set_active_index(uint active_idx);
*/
int fwu_get_image_index(u8 *image_index);
-/**
- * fwu_mdata_check() - Check if the FWU metadata is valid
- * @dev: FWU metadata device
- *
- * Validate both copies of the FWU metadata. If one of the copies
- * has gone bad, restore it from the other copy.
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_mdata_check(struct udevice *dev);
-
/**
* fwu_revert_boot_index() - Revert the active index in the FWU metadata
*
@@ -327,20 +142,6 @@ int fwu_mdata_check(struct udevice *dev);
*/
int fwu_revert_boot_index(void);
-/**
- * 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);
-
/**
* fwu_accept_image() - Set the Acceptance bit for the image
* @img_type_id: GUID of the image type for which the accepted bit is to be
diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
index 3d51b91ed5..116d862f45 100644
--- a/lib/fwu_updates/fwu.c
+++ b/lib/fwu_updates/fwu.c
@@ -33,26 +33,6 @@ enum {
BOTH_PARTS,
};
-static int fwu_get_dev_mdata(struct udevice **dev, struct fwu_mdata *mdata)
-{
- int ret;
-
- ret = uclass_first_device_err(UCLASS_FWU_MDATA, dev);
- if (ret) {
- log_debug("Cannot find fwu device\n");
- return ret;
- }
-
- if (!mdata)
- return 0;
-
- ret = fwu_get_mdata(*dev, mdata);
- if (ret < 0)
- log_debug("Unable to get valid FWU metadata\n");
-
- return ret;
-}
-
static int trial_counter_update(u16 *trial_state_ctr)
{
bool delete;
@@ -289,136 +269,6 @@ int fwu_get_verified_mdata(struct fwu_mdata *mdata)
return err;
}
-/**
- * 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 = &mdata->version;
- calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
-
- if (calc_crc32 != mdata->crc32) {
- log_debug("crc32 check failed for %s FWU metadata partition\n",
- pri_part ? "primary" : "secondary");
- return -EINVAL;
- }
-
- return 0;
-}
-
-/**
- * fwu_check_mdata_validity() - Check for validity of the FWU metadata copies
- *
- * 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.
- *
- * Return: 0 if OK, -ve on error
- *
- */
-int fwu_check_mdata_validity(void)
-{
- int ret;
- struct udevice *dev;
- struct fwu_mdata pri_mdata;
- struct fwu_mdata secondary_mdata;
- uint mdata_parts[2];
- uint valid_partitions, invalid_partitions;
-
- ret = fwu_get_dev_mdata(&dev, NULL);
- if (ret)
- return ret;
-
- /*
- * Check if the platform has defined its own
- * function to check the metadata partitions'
- * validity. If so, that takes precedence.
- */
- ret = fwu_mdata_check(dev);
- if (!ret || ret != -ENOSYS)
- return ret;
-
- /*
- * Two FWU metadata partitions are expected.
- * If we don't have two, user needs to create
- * them first
- */
- valid_partitions = 0;
- ret = fwu_get_mdata_part_num(dev, mdata_parts);
- if (ret < 0) {
- log_debug("Error getting the FWU metadata partitions\n");
- return -ENOENT;
- }
-
- ret = fwu_read_mdata_partition(dev, &pri_mdata, mdata_parts[0]);
- if (!ret) {
- ret = fwu_verify_mdata(&pri_mdata, 1);
- if (!ret)
- valid_partitions |= PRIMARY_PART;
- }
-
- ret = fwu_read_mdata_partition(dev, &secondary_mdata, mdata_parts[1]);
- if (!ret) {
- ret = fwu_verify_mdata(&secondary_mdata, 0);
- if (!ret)
- valid_partitions |= SECONDARY_PART;
- }
-
- if (valid_partitions == (PRIMARY_PART | SECONDARY_PART)) {
- /*
- * Before returning, check that both the
- * FWU metadata copies are the same. If not,
- * populate the secondary partition from the
- * primary partition copy.
- */
- if (!memcmp(&pri_mdata, &secondary_mdata,
- sizeof(struct fwu_mdata))) {
- ret = 0;
- } else {
- log_info("Both FWU metadata copies are valid but do not match.");
- log_info(" Restoring the secondary partition from the primary\n");
- ret = fwu_write_mdata_partition(dev, &pri_mdata,
- mdata_parts[1]);
- if (ret)
- log_debug("Restoring secondary FWU metadata partition failed\n");
- }
- goto out;
- }
-
- if (!(valid_partitions & BOTH_PARTS)) {
- log_info("Both FWU metadata partitions invalid\n");
- ret = -EBADMSG;
- goto out;
- }
-
- invalid_partitions = valid_partitions ^ BOTH_PARTS;
- ret = fwu_write_mdata_partition(dev,
- (invalid_partitions == PRIMARY_PART) ?
- &secondary_mdata : &pri_mdata,
- (invalid_partitions == PRIMARY_PART) ?
- mdata_parts[0] : mdata_parts[1]);
-
- if (ret)
- log_debug("Restoring %s FWU metadata partition failed\n",
- (invalid_partitions == PRIMARY_PART) ?
- "primary" : "secondary");
-
-out:
- return ret;
-}
-
/**
* fwu_get_active_index() - Get active_index from the FWU metadata
* @active_idx: active_index value to be read
@@ -431,19 +281,14 @@ out:
*/
int fwu_get_active_index(uint *active_idx)
{
- int ret;
- struct udevice *dev;
- struct fwu_mdata mdata = { 0 };
-
- ret = fwu_get_dev_mdata(&dev, &mdata);
- if (ret)
- return ret;
+ int ret = 0;
+ struct fwu_mdata *mdata = &g_mdata;
/*
* Found the FWU metadata partition, now read the active_index
* value
*/
- *active_idx = mdata.active_index;
+ *active_idx = mdata->active_index;
if (*active_idx >= CONFIG_FWU_NUM_BANKS) {
log_debug("Active index value read is incorrect\n");
ret = -EINVAL;
@@ -464,30 +309,25 @@ int fwu_get_active_index(uint *active_idx)
int fwu_set_active_index(uint active_idx)
{
int ret;
- struct udevice *dev;
- struct fwu_mdata mdata = { 0 };
+ struct fwu_mdata *mdata = &g_mdata;
if (active_idx >= CONFIG_FWU_NUM_BANKS) {
log_debug("Invalid active index value\n");
return -EINVAL;
}
- ret = fwu_get_dev_mdata(&dev, &mdata);
- if (ret)
- return ret;
-
/*
* Update the active index and previous_active_index fields
* in the FWU metadata
*/
- mdata.previous_active_index = mdata.active_index;
- mdata.active_index = active_idx;
+ mdata->previous_active_index = mdata->active_index;
+ mdata->active_index = active_idx;
/*
* Now write this updated FWU metadata to both the
* FWU metadata partitions
*/
- ret = fwu_update_mdata(dev, &mdata);
+ ret = fwu_sync_mdata(mdata, BOTH_PARTS);
if (ret) {
log_debug("Failed to update FWU metadata partitions\n");
ret = -EIO;
@@ -517,15 +357,10 @@ int fwu_get_image_index(u8 *image_index)
u8 alt_num;
uint update_bank;
efi_guid_t *image_guid, image_type_id;
- struct udevice *dev;
- struct fwu_mdata mdata = { 0 };
+ struct fwu_mdata *mdata = &g_mdata;
struct fwu_image_entry *img_entry;
struct fwu_image_bank_info *img_bank_info;
- ret = fwu_get_dev_mdata(&dev, &mdata);
- if (ret)
- return ret;
-
ret = fwu_plat_get_update_index(&update_bank);
if (ret) {
log_debug("Failed to get the FWU update bank\n");
@@ -546,11 +381,11 @@ int fwu_get_image_index(u8 *image_index)
*/
for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) {
if (!guidcmp(&image_type_id,
- &mdata.img_entry[i].image_type_uuid)) {
- img_entry = &mdata.img_entry[i];
+ &mdata->img_entry[i].image_type_uuid)) {
+ img_entry = &mdata->img_entry[i];
img_bank_info = &img_entry->img_bank_info[update_bank];
image_guid = &img_bank_info->image_uuid;
- ret = fwu_plat_get_alt_num(dev, image_guid, &alt_num);
+ ret = fwu_plat_get_alt_num(g_dev, image_guid, &alt_num);
if (ret) {
log_debug("alt_num not found for partition with GUID %pUs\n",
image_guid);
@@ -585,26 +420,21 @@ int fwu_revert_boot_index(void)
{
int ret;
u32 cur_active_index;
- struct udevice *dev;
- struct fwu_mdata mdata = { 0 };
-
- ret = fwu_get_dev_mdata(&dev, &mdata);
- if (ret)
- return ret;
+ struct fwu_mdata *mdata = &g_mdata;
/*
* Swap the active index and previous_active_index fields
* in the FWU metadata
*/
- cur_active_index = mdata.active_index;
- mdata.active_index = mdata.previous_active_index;
- mdata.previous_active_index = cur_active_index;
+ cur_active_index = mdata->active_index;
+ mdata->active_index = mdata->previous_active_index;
+ mdata->previous_active_index = cur_active_index;
/*
* Now write this updated FWU metadata to both the
* FWU metadata partitions
*/
- ret = fwu_update_mdata(dev, &mdata);
+ ret = fwu_sync_mdata(mdata, BOTH_PARTS);
if (ret) {
log_debug("Failed to update FWU metadata partitions\n");
ret = -EIO;
@@ -631,16 +461,11 @@ int fwu_revert_boot_index(void)
static int fwu_clrset_image_accept(efi_guid_t *img_type_id, u32 bank, u8 action)
{
int ret, i;
- struct udevice *dev;
- struct fwu_mdata mdata = { 0 };
+ struct fwu_mdata *mdata = &g_mdata;
struct fwu_image_entry *img_entry;
struct fwu_image_bank_info *img_bank_info;
- ret = fwu_get_dev_mdata(&dev, &mdata);
- if (ret)
- return ret;
-
- img_entry = &mdata.img_entry[0];
+ img_entry = &mdata->img_entry[0];
for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) {
if (!guidcmp(&img_entry[i].image_type_uuid, img_type_id)) {
img_bank_info = &img_entry[i].img_bank_info[bank];
@@ -649,7 +474,7 @@ static int fwu_clrset_image_accept(efi_guid_t *img_type_id, u32 bank, u8 action)
else
img_bank_info->accepted = 0;
- ret = fwu_update_mdata(dev, &mdata);
+ ret = fwu_sync_mdata(mdata, BOTH_PARTS);
goto out;
}
}
@@ -784,8 +609,6 @@ static int fwu_boottime_checks(void *ctx, struct event *event)
{
int ret;
u32 boot_idx, active_idx;
- struct udevice *dev;
- struct fwu_mdata mdata = { 0 };
/* Don't have boot time checks on sandbox */
if (IS_ENABLED(CONFIG_SANDBOX)) {
@@ -793,9 +616,17 @@ static int fwu_boottime_checks(void *ctx, struct event *event)
return 0;
}
- ret = fwu_check_mdata_validity();
- if (ret)
- return 0;
+ ret = uclass_first_device_err(UCLASS_FWU_MDATA, &g_dev);
+ if (ret) {
+ log_debug("Cannot find fwu device\n");
+ return ret;
+ }
+
+ ret = fwu_get_verified_mdata(NULL);
+ if (ret) {
+ log_debug("Unable to read meta-data\n");
+ return ret;
+ }
/*
* Get the Boot Index, i.e. the bank from
@@ -831,11 +662,7 @@ static int fwu_boottime_checks(void *ctx, struct event *event)
if (efi_init_obj_list() != EFI_SUCCESS)
return 0;
- ret = fwu_get_dev_mdata(&dev, &mdata);
- if (ret)
- return ret;
-
- in_trial = in_trial_state(&mdata);
+ in_trial = in_trial_state(&g_mdata);
if (!in_trial || (ret = fwu_trial_count_update()) > 0)
ret = trial_counter_update(NULL);
--
2.25.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] fwu: meta-data: switch to management by common code
2022-11-04 2:43 ` [PATCH 4/4] fwu: meta-data: switch to management by common code jassisinghbrar
@ 2022-11-07 17:23 ` Etienne Carriere
2022-11-07 18:39 ` Jassi Brar
0 siblings, 1 reply; 15+ messages in thread
From: Etienne Carriere @ 2022-11-07 17:23 UTC (permalink / raw)
To: jassisinghbrar
Cc: u-boot, ilias.apalodimas, sughosh.ganu, trini, sjg, xypron.glpk,
patrick.delaunay, patrice.chotard, Jassi Brar
Hello Jassi,
FYI, I've successfully tested this series on stm32mp1 for FWU, once
the few typos fixed to build it them.
On Fri, 4 Nov 2022 at 03:43, <jassisinghbrar@gmail.com> wrote:
>
> From: Jassi Brar <jaswinder.singh@linaro.org>
>
> The common code can now read, verify and fix meta-data copies
> while exposing one consistent structure to users.
> Only the .read_mdata() and .write_mdata() callbacks of fwu_mdata_ops
> are needed. Get rid of .get_mdata() .update_mdata() .get_mdata_part_num()
> .read_mdata_partition() and .write_mdata_partition() and also the
> corresponding wrapper functions.
>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
> cmd/fwu_mdata.c | 17 +-
> drivers/fwu-mdata/fwu-mdata-uclass.c | 165 -------------------
> drivers/fwu-mdata/gpt_blk.c | 124 +-------------
> include/fwu.h | 199 -----------------------
> lib/fwu_updates/fwu.c | 235 ++++-----------------------
> 5 files changed, 38 insertions(+), 702 deletions(-)
>
> diff --git a/cmd/fwu_mdata.c b/cmd/fwu_mdata.c
> index f04af27de6..9b70340368 100644
> --- a/cmd/fwu_mdata.c
> +++ b/cmd/fwu_mdata.c
> @@ -43,23 +43,10 @@ static void print_mdata(struct fwu_mdata *mdata)
> int do_fwu_mdata_read(struct cmd_tbl *cmdtp, int flag,
> int argc, char * const argv[])
> {
> - struct udevice *dev;
> int ret = CMD_RET_SUCCESS, res;
> - struct fwu_mdata mdata = { 0 };
> + struct fwu_mdata mdata;
>
> - if (uclass_get_device(UCLASS_FWU_MDATA, 0, &dev) || !dev) {
> - log_err("Unable to get FWU metadata device\n");
> - return CMD_RET_FAILURE;
> - }
> -
> - res = fwu_check_mdata_validity();
> - if (res < 0) {
> - log_err("FWU Metadata check failed\n");
> - ret = CMD_RET_FAILURE;
> - goto out;
> - }
> -
> - res = fwu_get_mdata(dev, &mdata);
> + res = fwu_get_verified_mdata(&mdata);
> if (res < 0) {
> log_err("Unable to get valid FWU metadata\n");
> ret = CMD_RET_FAILURE;
> diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c
> index ded9902971..8190113843 100644
> --- a/drivers/fwu-mdata/fwu-mdata-uclass.c
> +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
> @@ -14,7 +14,6 @@
>
> #include <linux/errno.h>
> #include <linux/types.h>
> -#include <u-boot/crc.h>
>
> /**
> * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
> @@ -50,170 +49,6 @@ int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> return ops->write_mdata(dev, mdata, primary);
> }
>
> -/**
> - * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers
> - * @dev: FWU metadata device
> - * @mdata_parts: array for storing the metadata partition numbers
> - *
> - * Get the partition numbers on the storage device on which the
> - * FWU metadata is stored. Two partition numbers will be returned.
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_get_mdata_part_num(struct udevice *dev, uint *mdata_parts)
> -{
> - const struct fwu_mdata_ops *ops = device_get_ops(dev);
> -
> - if (!ops->get_mdata_part_num) {
> - log_debug("get_mdata_part_num() method not defined\n");
> - return -ENOSYS;
> - }
> -
> - return ops->get_mdata_part_num(dev, mdata_parts);
> -}
> -
> -/**
> - * fwu_read_mdata_partition() - Read the FWU metadata from a partition
> - * @dev: FWU metadata device
> - * @mdata: Copy of the FWU metadata
> - * @part_num: Partition number from which FWU metadata is to be read
> - *
> - * Read the FWU metadata from the specified partition number
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_read_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata,
> - uint part_num)
> -{
> - const struct fwu_mdata_ops *ops = device_get_ops(dev);
> -
> - if (!ops->read_mdata_partition) {
> - log_debug("read_mdata_partition() method not defined\n");
> - return -ENOSYS;
> - }
> -
> - return ops->read_mdata_partition(dev, mdata, part_num);
> -}
> -
> -/**
> - * fwu_write_mdata_partition() - Write the FWU metadata to a partition
> - * @dev: FWU metadata device
> - * @mdata: Copy of the FWU metadata
> - * @part_num: Partition number to which FWU metadata is to be written
> - *
> - * Write the FWU metadata to the specified partition number
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_write_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata,
> - uint part_num)
> -{
> - const struct fwu_mdata_ops *ops = device_get_ops(dev);
> -
> - if (!ops->write_mdata_partition) {
> - log_debug("write_mdata_partition() method not defined\n");
> - return -ENOSYS;
> - }
> -
> - return ops->write_mdata_partition(dev, mdata, part_num);
> -}
> -
> -/**
> - * fwu_mdata_check() - Check if the FWU metadata is valid
> - * @dev: FWU metadata device
> - *
> - * Validate both copies of the FWU metadata. If one of the copies
> - * has gone bad, restore it from the other copy.
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_mdata_check(struct udevice *dev)
> -{
> - const struct fwu_mdata_ops *ops = device_get_ops(dev);
> -
> - if (!ops->check_mdata) {
> - log_debug("check_mdata() method not defined\n");
> - return -ENOSYS;
> - }
> -
> - return ops->check_mdata(dev);
> -}
> -
> -/**
> - * fwu_get_mdata() - Get a FWU metadata copy
> - * @dev: FWU metadata device
> - * @mdata: Copy of the FWU metadata
> - *
> - * Get a valid copy of the FWU metadata.
> - *
> - * Note: This function is to be called first when modifying any fields
> - * in the metadata. The sequence of calls to modify any field in the
> - * metadata would be 1) fwu_get_mdata 2) Modify metadata, followed by
> - * 3) fwu_update_mdata
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_get_mdata(struct udevice *dev, struct fwu_mdata *mdata)
> -{
> - const struct fwu_mdata_ops *ops = device_get_ops(dev);
> -
> - if (!ops->get_mdata) {
> - log_debug("get_mdata() method not defined\n");
> - return -ENOSYS;
> - }
> -
> - return ops->get_mdata(dev, mdata);
> -}
> -
> -/**
> - * fwu_update_mdata() - Update the FWU metadata
> - * @dev: FWU metadata device
> - * @mdata: Copy of the FWU metadata
> - *
> - * Update the FWU metadata structure by writing to the
> - * FWU metadata partitions.
> - *
> - * Note: This function is not to be called directly to update the
> - * metadata fields. The sequence of function calls should be
> - * 1) fwu_get_mdata() 2) Modify the medata fields 3) fwu_update_mdata()
> - *
> - * The sequence of updating the partitions should be, update the
> - * primary metadata partition (first partition encountered), followed
> - * by updating the secondary partition. With this update sequence, in
> - * the rare scenario that the two metadata partitions are valid but do
> - * not match, maybe due to power outage at the time of updating the
> - * metadata copies, the secondary partition can be updated from the
> - * primary.
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_update_mdata(struct udevice *dev, struct fwu_mdata *mdata)
> -{
> - void *buf;
> - const struct fwu_mdata_ops *ops = device_get_ops(dev);
> -
> - if (!ops->update_mdata) {
> - log_debug("get_mdata() method not defined\n");
> - return -ENOSYS;
> - }
> -
> - /*
> - * Calculate the crc32 for the updated FWU metadata
> - * and put the updated value in the FWU metadata crc32
> - * field
> - */
> - buf = &mdata->version;
> - mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
> -
> - return ops->update_mdata(dev, mdata);
> -}
> -
> UCLASS_DRIVER(fwu_mdata) = {
> .id = UCLASS_FWU_MDATA,
> .name = "fwu-mdata",
> diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c
> index 7fda041302..7f1782af1a 100644
> --- a/drivers/fwu-mdata/gpt_blk.c
> +++ b/drivers/fwu-mdata/gpt_blk.c
> @@ -28,7 +28,7 @@ static uint g_mdata_part[2] = {0, 0};
>
> static int gpt_get_mdata_partitions(struct blk_desc *desc)
> {
> - int i, ret;
> + int i;
> u32 nparts;
> efi_guid_t part_type_guid;
> struct disk_partition info;
> @@ -52,12 +52,12 @@ static int gpt_get_mdata_partitions(struct blk_desc *desc)
> if (nparts != 2) {
> log_debug("Expect two copies of the FWU metadata instead of %d\n",
> nparts);
> - ret = -EINVAL;
> - } else {
> - ret = 0;
> + g_mdata_part[0] = 0;
> + g_mdata_part[1] = 0;
> + return -EINVAL;
> }
>
> - return ret;
> + return 0;
> }
>
> static int gpt_get_mdata_disk_part(struct blk_desc *desc,
> @@ -125,115 +125,6 @@ static int gpt_read_write_mdata(struct blk_desc *desc,
> return 0;
> }
>
> -static int fwu_gpt_update_mdata(struct udevice *dev, struct fwu_mdata *mdata)
> -{
> - int ret;
> - struct blk_desc *desc;
> - struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> -
> - desc = dev_get_uclass_plat(priv->blk_dev);
> -
> - ret = gpt_get_mdata_partitions(desc);
> - if (ret < 0) {
> - log_debug("Error getting the FWU metadata partitions\n");
> - return -ENOENT;
> - }
> -
> - /* First write the primary partition */
> - ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, g_mdata_part[0]);
> - if (ret < 0) {
> - log_debug("Updating primary FWU metadata partition failed\n");
> - return ret;
> - }
> -
> - /* And now the replica */
> - ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, g_mdata_part[1]);
> - if (ret < 0) {
> - log_debug("Updating secondary FWU metadata partition failed\n");
> - return ret;
> - }
> -
> - return 0;
> -}
> -
> -static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata *mdata)
> -{
> - int ret;
> -
> - ret = gpt_get_mdata_partitions(desc);
> - if (ret < 0) {
> - log_debug("Error getting the FWU metadata partitions\n");
> - return -ENOENT;
> - }
> -
> - ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, g_mdata_part[0]);
> - if (ret < 0) {
> - log_debug("Failed to read the FWU metadata from the device\n");
> - return -EIO;
> - }
> -
> - ret = fwu_verify_mdata(mdata, 1);
> - if (!ret)
> - return 0;
> -
> - /*
> - * Verification of the primary FWU metadata copy failed.
> - * Try to read the replica.
> - */
> - memset(mdata, '\0', sizeof(struct fwu_mdata));
> - ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, g_mdata_part[1]);
> - if (ret < 0) {
> - log_debug("Failed to read the FWU metadata from the device\n");
> - return -EIO;
> - }
> -
> - ret = fwu_verify_mdata(mdata, 0);
> - if (!ret)
> - return 0;
> -
> - /* Both the FWU metadata copies are corrupted. */
> - return -EIO;
> -}
> -
> -static int fwu_gpt_get_mdata(struct udevice *dev, struct fwu_mdata *mdata)
> -{
> - struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> -
> - return gpt_get_mdata(dev_get_uclass_plat(priv->blk_dev), mdata);
> -}
> -
> -static int fwu_gpt_get_mdata_partitions(struct udevice *dev, uint *mdata_parts)
> -{
> - struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> - int err;
> -
> - err = gpt_get_mdata_partitions(dev_get_uclass_plat(priv->blk_dev));
> - if (!err) {
> - mdata_parts[0] = g_mdata_part[0];
> - mdata_parts[1] = g_mdata_part[1];
> - }
> -
> - return err;
> -}
> -
> -static int fwu_gpt_read_mdata_partition(struct udevice *dev,
> - struct fwu_mdata *mdata, uint part_num)
> -{
> - struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> -
> - return gpt_read_write_mdata(dev_get_uclass_plat(priv->blk_dev),
> - mdata, MDATA_READ, part_num);
> -}
> -
> -static int fwu_gpt_write_mdata_partition(struct udevice *dev,
> - struct fwu_mdata *mdata, uint part_num)
> -{
> - struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> -
> - return gpt_read_write_mdata(dev_get_uclass_plat(priv->blk_dev),
> - mdata, MDATA_WRITE, part_num);
> -}
> -
> static int fwu_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev)
> {
> u32 phandle;
> @@ -309,11 +200,6 @@ static int fwu_gpt_write_mdata(struct udevice *dev, struct fwu_mdata *mdata,
> static const struct fwu_mdata_ops fwu_gpt_blk_ops = {
> .read_mdata = fwu_gpt_read_mdata,
> .write_mdata = fwu_gpt_write_mdata,
> - .get_mdata = fwu_gpt_get_mdata,
> - .update_mdata = fwu_gpt_update_mdata,
> - .get_mdata_part_num = fwu_gpt_get_mdata_partitions,
> - .read_mdata_partition = fwu_gpt_read_mdata_partition,
> - .write_mdata_partition = fwu_gpt_write_mdata_partition,
> };
>
> static const struct udevice_id fwu_mdata_ids[] = {
> diff --git a/include/fwu.h b/include/fwu.h
> index 1a700c9e6a..23bd97fe86 100644
> --- a/include/fwu.h
> +++ b/include/fwu.h
> @@ -18,11 +18,6 @@ struct fwu_mdata_gpt_blk_priv {
> struct udevice *blk_dev;
> };
>
> -/**
> - * @mdata_check: check the validity of the FWU metadata partitions
> - * @get_mdata() - Get a FWU metadata copy
> - * @update_mdata() - Update the FWU metadata copy
> - */
> struct fwu_mdata_ops {
> /**
> * read_mdata() - Populate the asked FWU metadata copy
> @@ -43,78 +38,6 @@ struct fwu_mdata_ops {
> * Return: 0 if OK, -ve on error
> */
> int (*write_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> -
> - /**
> - * check_mdata() - Check if the FWU metadata is valid
> - * @dev: FWU device
> - *
> - * Validate both copies of the FWU metadata. If one of the copies
> - * has gone bad, restore it from the other copy.
> - *
> - * Return: 0 if OK, -ve on error
> - */
> - int (*check_mdata)(struct udevice *dev);
> -
> - /**
> - * get_mdata() - Get a FWU metadata copy
> - * @dev: FWU device
> - * @mdata: Pointer to FWU metadata
> - *
> - * Get a valid copy of the FWU metadata.
> - *
> - * Return: 0 if OK, -ve on error
> - */
> - int (*get_mdata)(struct udevice *dev, struct fwu_mdata *mdata);
> -
> - /**
> - * update_mdata() - Update the FWU metadata
> - * @dev: FWU device
> - * @mdata: Copy of the FWU metadata
> - *
> - * Update the FWU metadata structure by writing to the
> - * FWU metadata partitions.
> - *
> - * Return: 0 if OK, -ve on error
> - */
> - int (*update_mdata)(struct udevice *dev, struct fwu_mdata *mdata);
> -
> - /**
> - * get_mdata_part_num() - Get the FWU metadata partition numbers
> - * @dev: FWU metadata device
> - * @mdata_parts: array for storing the metadata partition numbers
> - *
> - * Get the partition numbers on the storage device on which the
> - * FWU metadata is stored. Two partition numbers will be returned.
> - *
> - * Return: 0 if OK, -ve on error
> - */
> - int (*get_mdata_part_num)(struct udevice *dev, uint *mdata_parts);
> -
> - /**
> - * read_mdata_partition() - Read the FWU metadata from a partition
> - * @dev: FWU metadata device
> - * @mdata: Copy of the FWU metadata
> - * @part_num: Partition number from which FWU metadata is to be read
> - *
> - * Read the FWU metadata from the specified partition number
> - *
> - * Return: 0 if OK, -ve on error
> - */
> - int (*read_mdata_partition)(struct udevice *dev,
> - struct fwu_mdata *mdata, uint part_num);
> -
> - /**
> - * write_mdata_partition() - Write the FWU metadata to a partition
> - * @dev: FWU metadata device
> - * @mdata: Copy of the FWU metadata
> - * @part_num: Partition number to which FWU metadata is to be written
> - *
> - * Write the FWU metadata to the specified partition number
> - *
> - * Return: 0 if OK, -ve on error
> - */
> - int (*write_mdata_partition)(struct udevice *dev,
> - struct fwu_mdata *mdata, uint part_num);
> };
>
> #define FWU_MDATA_VERSION 0x1
> @@ -167,102 +90,6 @@ int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> */
> int fwu_get_verified_mdata(struct fwu_mdata *mdata);
>
> -/**
> - * fwu_check_mdata_validity() - Check for validity of the FWU metadata copies
> - *
> - * 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.
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_check_mdata_validity(void);
> -
> -/**
> - * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers
> - * @dev: FWU metadata device
> - * @mdata_parts: array for storing the metadata partition numbers
> - *
> - * Get the partition numbers on the storage device on which the
> - * FWU metadata is stored. Two partition numbers will be returned
> - * through the array.
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_get_mdata_part_num(struct udevice *dev, uint *mdata_parts);
> -
> -/**
> - * fwu_read_mdata_partition() - Read the FWU metadata from a partition
> - * @dev: FWU metadata device
> - * @mdata: Copy of the FWU metadata
> - * @part_num: Partition number from which FWU metadata is to be read
> - *
> - * Read the FWU metadata from the specified partition number
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_read_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata,
> - uint part_num);
> -
> -/**
> - * fwu_write_mdata_partition() - Write the FWU metadata to a partition
> - * @dev: FWU metadata device
> - * @mdata: Copy of the FWU metadata
> - * @part_num: Partition number to which FWU metadata is to be written
> - *
> - * Write the FWU metadata to the specified partition number
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_write_mdata_partition(struct udevice *dev, struct fwu_mdata *mdata,
> - uint part_num);
> -
> -/**
> - * fwu_get_mdata() - Get a FWU metadata copy
> - * @dev: FWU metadata device
> - * @mdata: Copy of the FWU metadata
> - *
> - * Get a valid copy of the FWU metadata.
> - *
> - * Note: This function is to be called first when modifying any fields
> - * in the metadata. The sequence of calls to modify any field in the
> - * metadata would be 1) fwu_get_mdata 2) Modify metadata, followed by
> - * 3) fwu_update_mdata
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_get_mdata(struct udevice *dev, struct fwu_mdata *mdata);
> -
> -/**
> - * fwu_update_mdata() - Update the FWU metadata
> - * @dev: FWU metadata device
> - * @mdata: Copy of the FWU metadata
> - *
> - * Update the FWU metadata structure by writing to the
> - * FWU metadata partitions.
> - *
> - * Note: This function is not to be called directly to update the
> - * metadata fields. The sequence of function calls should be
> - * 1) fwu_get_mdata() 2) Modify the medata fields 3) fwu_update_mdata()
> - *
> - * The sequence of updating the partitions should be, update the
> - * primary metadata partition (first partition encountered), followed
> - * by updating the secondary partition. With this update sequence, in
> - * the rare scenario that the two metadata partitions are valid but do
> - * not match, maybe due to power outage at the time of updating the
> - * metadata copies, the secondary partition can be updated from the
> - * primary.
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_update_mdata(struct udevice *dev, struct fwu_mdata *mdata);
> -
> /**
> * fwu_get_active_index() - Get active_index from the FWU metadata
> * @active_idxp: active_index value to be read
> @@ -303,18 +130,6 @@ int fwu_set_active_index(uint active_idx);
> */
> int fwu_get_image_index(u8 *image_index);
>
> -/**
> - * fwu_mdata_check() - Check if the FWU metadata is valid
> - * @dev: FWU metadata device
> - *
> - * Validate both copies of the FWU metadata. If one of the copies
> - * has gone bad, restore it from the other copy.
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_mdata_check(struct udevice *dev);
> -
> /**
> * fwu_revert_boot_index() - Revert the active index in the FWU metadata
> *
> @@ -327,20 +142,6 @@ int fwu_mdata_check(struct udevice *dev);
> */
> int fwu_revert_boot_index(void);
>
> -/**
> - * 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);
> -
> /**
> * fwu_accept_image() - Set the Acceptance bit for the image
> * @img_type_id: GUID of the image type for which the accepted bit is to be
> diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
> index 3d51b91ed5..116d862f45 100644
> --- a/lib/fwu_updates/fwu.c
> +++ b/lib/fwu_updates/fwu.c
> @@ -33,26 +33,6 @@ enum {
> BOTH_PARTS,
> };
>
> -static int fwu_get_dev_mdata(struct udevice **dev, struct fwu_mdata *mdata)
> -{
> - int ret;
> -
> - ret = uclass_first_device_err(UCLASS_FWU_MDATA, dev);
> - if (ret) {
> - log_debug("Cannot find fwu device\n");
> - return ret;
> - }
> -
> - if (!mdata)
> - return 0;
> -
> - ret = fwu_get_mdata(*dev, mdata);
> - if (ret < 0)
> - log_debug("Unable to get valid FWU metadata\n");
> -
> - return ret;
> -}
> -
> static int trial_counter_update(u16 *trial_state_ctr)
> {
> bool delete;
> @@ -289,136 +269,6 @@ int fwu_get_verified_mdata(struct fwu_mdata *mdata)
> return err;
> }
>
> -/**
> - * 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 = &mdata->version;
> - calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
> -
> - if (calc_crc32 != mdata->crc32) {
> - log_debug("crc32 check failed for %s FWU metadata partition\n",
> - pri_part ? "primary" : "secondary");
> - return -EINVAL;
> - }
> -
> - return 0;
> -}
> -
> -/**
> - * fwu_check_mdata_validity() - Check for validity of the FWU metadata copies
> - *
> - * 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.
> - *
> - * Return: 0 if OK, -ve on error
> - *
> - */
> -int fwu_check_mdata_validity(void)
> -{
> - int ret;
> - struct udevice *dev;
> - struct fwu_mdata pri_mdata;
> - struct fwu_mdata secondary_mdata;
> - uint mdata_parts[2];
> - uint valid_partitions, invalid_partitions;
> -
> - ret = fwu_get_dev_mdata(&dev, NULL);
> - if (ret)
> - return ret;
> -
> - /*
> - * Check if the platform has defined its own
> - * function to check the metadata partitions'
> - * validity. If so, that takes precedence.
> - */
> - ret = fwu_mdata_check(dev);
> - if (!ret || ret != -ENOSYS)
> - return ret;
> -
> - /*
> - * Two FWU metadata partitions are expected.
> - * If we don't have two, user needs to create
> - * them first
> - */
> - valid_partitions = 0;
> - ret = fwu_get_mdata_part_num(dev, mdata_parts);
> - if (ret < 0) {
> - log_debug("Error getting the FWU metadata partitions\n");
> - return -ENOENT;
> - }
> -
> - ret = fwu_read_mdata_partition(dev, &pri_mdata, mdata_parts[0]);
> - if (!ret) {
> - ret = fwu_verify_mdata(&pri_mdata, 1);
> - if (!ret)
> - valid_partitions |= PRIMARY_PART;
> - }
> -
> - ret = fwu_read_mdata_partition(dev, &secondary_mdata, mdata_parts[1]);
> - if (!ret) {
> - ret = fwu_verify_mdata(&secondary_mdata, 0);
> - if (!ret)
> - valid_partitions |= SECONDARY_PART;
> - }
> -
> - if (valid_partitions == (PRIMARY_PART | SECONDARY_PART)) {
> - /*
> - * Before returning, check that both the
> - * FWU metadata copies are the same. If not,
> - * populate the secondary partition from the
> - * primary partition copy.
> - */
> - if (!memcmp(&pri_mdata, &secondary_mdata,
> - sizeof(struct fwu_mdata))) {
> - ret = 0;
> - } else {
> - log_info("Both FWU metadata copies are valid but do not match.");
> - log_info(" Restoring the secondary partition from the primary\n");
> - ret = fwu_write_mdata_partition(dev, &pri_mdata,
> - mdata_parts[1]);
> - if (ret)
> - log_debug("Restoring secondary FWU metadata partition failed\n");
> - }
> - goto out;
> - }
> -
> - if (!(valid_partitions & BOTH_PARTS)) {
> - log_info("Both FWU metadata partitions invalid\n");
> - ret = -EBADMSG;
> - goto out;
> - }
> -
> - invalid_partitions = valid_partitions ^ BOTH_PARTS;
> - ret = fwu_write_mdata_partition(dev,
> - (invalid_partitions == PRIMARY_PART) ?
> - &secondary_mdata : &pri_mdata,
> - (invalid_partitions == PRIMARY_PART) ?
> - mdata_parts[0] : mdata_parts[1]);
> -
> - if (ret)
> - log_debug("Restoring %s FWU metadata partition failed\n",
> - (invalid_partitions == PRIMARY_PART) ?
> - "primary" : "secondary");
> -
> -out:
> - return ret;
> -}
> -
> /**
> * fwu_get_active_index() - Get active_index from the FWU metadata
> * @active_idx: active_index value to be read
> @@ -431,19 +281,14 @@ out:
> */
> int fwu_get_active_index(uint *active_idx)
> {
> - int ret;
> - struct udevice *dev;
> - struct fwu_mdata mdata = { 0 };
> -
> - ret = fwu_get_dev_mdata(&dev, &mdata);
> - if (ret)
> - return ret;
> + int ret = 0;
> + struct fwu_mdata *mdata = &g_mdata;
>
> /*
> * Found the FWU metadata partition, now read the active_index
> * value
> */
> - *active_idx = mdata.active_index;
> + *active_idx = mdata->active_index;
> if (*active_idx >= CONFIG_FWU_NUM_BANKS) {
> log_debug("Active index value read is incorrect\n");
> ret = -EINVAL;
> @@ -464,30 +309,25 @@ int fwu_get_active_index(uint *active_idx)
> int fwu_set_active_index(uint active_idx)
> {
> int ret;
> - struct udevice *dev;
> - struct fwu_mdata mdata = { 0 };
> + struct fwu_mdata *mdata = &g_mdata;
>
> if (active_idx >= CONFIG_FWU_NUM_BANKS) {
> log_debug("Invalid active index value\n");
> return -EINVAL;
> }
>
> - ret = fwu_get_dev_mdata(&dev, &mdata);
> - if (ret)
> - return ret;
> -
> /*
> * Update the active index and previous_active_index fields
> * in the FWU metadata
> */
> - mdata.previous_active_index = mdata.active_index;
> - mdata.active_index = active_idx;
> + mdata->previous_active_index = mdata->active_index;
> + mdata->active_index = active_idx;
>
> /*
> * Now write this updated FWU metadata to both the
> * FWU metadata partitions
> */
> - ret = fwu_update_mdata(dev, &mdata);
> + ret = fwu_sync_mdata(mdata, BOTH_PARTS);
> if (ret) {
> log_debug("Failed to update FWU metadata partitions\n");
> ret = -EIO;
> @@ -517,15 +357,10 @@ int fwu_get_image_index(u8 *image_index)
> u8 alt_num;
> uint update_bank;
> efi_guid_t *image_guid, image_type_id;
> - struct udevice *dev;
> - struct fwu_mdata mdata = { 0 };
> + struct fwu_mdata *mdata = &g_mdata;
> struct fwu_image_entry *img_entry;
> struct fwu_image_bank_info *img_bank_info;
>
> - ret = fwu_get_dev_mdata(&dev, &mdata);
> - if (ret)
> - return ret;
> -
> ret = fwu_plat_get_update_index(&update_bank);
> if (ret) {
> log_debug("Failed to get the FWU update bank\n");
> @@ -546,11 +381,11 @@ int fwu_get_image_index(u8 *image_index)
> */
> for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) {
> if (!guidcmp(&image_type_id,
> - &mdata.img_entry[i].image_type_uuid)) {
> - img_entry = &mdata.img_entry[i];
> + &mdata->img_entry[i].image_type_uuid)) {
> + img_entry = &mdata->img_entry[i];
> img_bank_info = &img_entry->img_bank_info[update_bank];
> image_guid = &img_bank_info->image_uuid;
> - ret = fwu_plat_get_alt_num(dev, image_guid, &alt_num);
> + ret = fwu_plat_get_alt_num(g_dev, image_guid, &alt_num);
> if (ret) {
> log_debug("alt_num not found for partition with GUID %pUs\n",
> image_guid);
> @@ -585,26 +420,21 @@ int fwu_revert_boot_index(void)
> {
> int ret;
> u32 cur_active_index;
> - struct udevice *dev;
> - struct fwu_mdata mdata = { 0 };
> -
> - ret = fwu_get_dev_mdata(&dev, &mdata);
> - if (ret)
> - return ret;
> + struct fwu_mdata *mdata = &g_mdata;
>
> /*
> * Swap the active index and previous_active_index fields
> * in the FWU metadata
> */
> - cur_active_index = mdata.active_index;
> - mdata.active_index = mdata.previous_active_index;
> - mdata.previous_active_index = cur_active_index;
> + cur_active_index = mdata->active_index;
> + mdata->active_index = mdata->previous_active_index;
> + mdata->previous_active_index = cur_active_index;
>
> /*
> * Now write this updated FWU metadata to both the
> * FWU metadata partitions
> */
> - ret = fwu_update_mdata(dev, &mdata);
> + ret = fwu_sync_mdata(mdata, BOTH_PARTS);
> if (ret) {
> log_debug("Failed to update FWU metadata partitions\n");
> ret = -EIO;
> @@ -631,16 +461,11 @@ int fwu_revert_boot_index(void)
> static int fwu_clrset_image_accept(efi_guid_t *img_type_id, u32 bank, u8 action)
> {
> int ret, i;
> - struct udevice *dev;
> - struct fwu_mdata mdata = { 0 };
> + struct fwu_mdata *mdata = &g_mdata;
> struct fwu_image_entry *img_entry;
> struct fwu_image_bank_info *img_bank_info;
>
> - ret = fwu_get_dev_mdata(&dev, &mdata);
> - if (ret)
> - return ret;
> -
> - img_entry = &mdata.img_entry[0];
> + img_entry = &mdata->img_entry[0];
> for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) {
> if (!guidcmp(&img_entry[i].image_type_uuid, img_type_id)) {
> img_bank_info = &img_entry[i].img_bank_info[bank];
> @@ -649,7 +474,7 @@ static int fwu_clrset_image_accept(efi_guid_t *img_type_id, u32 bank, u8 action)
> else
> img_bank_info->accepted = 0;
>
> - ret = fwu_update_mdata(dev, &mdata);
> + ret = fwu_sync_mdata(mdata, BOTH_PARTS);
> goto out;
> }
> }
> @@ -784,8 +609,6 @@ static int fwu_boottime_checks(void *ctx, struct event *event)
> {
> int ret;
> u32 boot_idx, active_idx;
> - struct udevice *dev;
> - struct fwu_mdata mdata = { 0 };
>
> /* Don't have boot time checks on sandbox */
> if (IS_ENABLED(CONFIG_SANDBOX)) {
> @@ -793,9 +616,17 @@ static int fwu_boottime_checks(void *ctx, struct event *event)
> return 0;
> }
>
> - ret = fwu_check_mdata_validity();
> - if (ret)
> - return 0;
> + ret = uclass_first_device_err(UCLASS_FWU_MDATA, &g_dev);
> + if (ret) {
> + log_debug("Cannot find fwu device\n");
> + return ret;
> + }
> +
> + ret = fwu_get_verified_mdata(NULL);
> + if (ret) {
> + log_debug("Unable to read meta-data\n");
> + return ret;
> + }
>
> /*
> * Get the Boot Index, i.e. the bank from
> @@ -831,11 +662,7 @@ static int fwu_boottime_checks(void *ctx, struct event *event)
> if (efi_init_obj_list() != EFI_SUCCESS)
> return 0;
>
> - ret = fwu_get_dev_mdata(&dev, &mdata);
> - if (ret)
> - return ret;
> -
> - in_trial = in_trial_state(&mdata);
> + in_trial = in_trial_state(&g_mdata);
Since g_mdata is global, in_trial_state() could access it straight
without needing to pass g_mdata as argument.
Br,
etienne
> if (!in_trial || (ret = fwu_trial_count_update()) > 0)
> ret = trial_counter_update(NULL);
>
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] fwu: gpt: implement read_mdata and write_mdata callbacks
2022-11-04 2:43 ` [PATCH 3/4] fwu: gpt: implement read_mdata and write_mdata callbacks jassisinghbrar
@ 2022-11-07 17:23 ` Etienne Carriere
2022-11-07 18:30 ` Jassi Brar
0 siblings, 1 reply; 15+ messages in thread
From: Etienne Carriere @ 2022-11-07 17:23 UTC (permalink / raw)
To: jassisinghbrar
Cc: u-boot, ilias.apalodimas, sughosh.ganu, trini, sjg, xypron.glpk,
patrick.delaunay, patrice.chotard, Jassi Brar
On Fri, 4 Nov 2022 at 03:43, <jassisinghbrar@gmail.com> wrote:
>
> From: Jassi Brar <jaswinder.singh@linaro.org>
>
> Moving towards using common code for meta-data management,
> implement the read/write mdata hooks.
>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
> drivers/fwu-mdata/gpt_blk.c | 36 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c
> index d706e3d4e4..7fda041302 100644
> --- a/drivers/fwu-mdata/gpt_blk.c
> +++ b/drivers/fwu-mdata/gpt_blk.c
> @@ -272,7 +272,43 @@ static int fwu_mdata_gpt_blk_probe(struct udevice *dev)
> return 0;
> }
>
> +static int fwu_gpt_read_mdata(struct udevice *dev, struct fwu_mdata *mdata,
> + bool primary)
> +{
> + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> + struct blk_desc *desc = dev_get_uclass_plat(priv->blk_dev);
> + int ret;
> +
> + ret = gpt_get_mdata_partitions(desc);
> + if (ret < 0) {
> + log_debug("Error getting the FWU metadata partitions\n");
> + return -ENOENT;
Not returning ret value?
> + }
> +
> + return gpt_read_write_mdata(desc, mdata, MDATA_READ,
> + primary ? g_mdata_part[0] : g_mdata_part[1]);
Fix indentation.
> +}
> +
> +static int fwu_gpt_write_mdata(struct udevice *dev, struct fwu_mdata *mdata,
> + bool primary)
> +{
> + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> + struct blk_desc *desc = dev_get_uclass_plat(priv->blk_dev);
> + int ret;
> +
> + ret = gpt_get_mdata_partitions(desc);
> + if (ret < 0) {
> + log_debug("Error getting the FWU metadata partitions\n");
> + return -ENOENT;
> + }
> +
> + return gpt_read_write_mdata(desc, mdata, MDATA_WRITE,
> + primary ? g_mdata_part[0] : g_mdata_part[1]);
Dito
Br,
etienne
> +}
> +
> static const struct fwu_mdata_ops fwu_gpt_blk_ops = {
> + .read_mdata = fwu_gpt_read_mdata,
> + .write_mdata = fwu_gpt_write_mdata,
> .get_mdata = fwu_gpt_get_mdata,
> .update_mdata = fwu_gpt_update_mdata,
> .get_mdata_part_num = fwu_gpt_get_mdata_partitions,
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/4] fwu: move meta-data management in core
2022-11-04 2:43 ` [PATCH 2/4] fwu: move meta-data management in core jassisinghbrar
@ 2022-11-07 17:23 ` Etienne Carriere
2022-11-07 18:28 ` Jassi Brar
0 siblings, 1 reply; 15+ messages in thread
From: Etienne Carriere @ 2022-11-07 17:23 UTC (permalink / raw)
To: jassisinghbrar
Cc: u-boot, ilias.apalodimas, sughosh.ganu, trini, sjg, xypron.glpk,
patrick.delaunay, patrice.chotard, Jassi Brar
Hello Jassi,
On Fri, 4 Nov 2022 at 03:43, <jassisinghbrar@gmail.com> wrote:
>
> From: Jassi Brar <jaswinder.singh@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.
>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
> drivers/fwu-mdata/fwu-mdata-uclass.c | 34 +++++++
> include/fwu.h | 41 ++++++++
> lib/fwu_updates/fwu.c | 136 ++++++++++++++++++++++++++-
> 3 files changed, 207 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c
> index b477e9603f..ded9902971 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
> + */
> +int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> +{
> + const struct fwu_mdata_ops *ops = device_get_ops(dev);
> +
> + if (!ops->read_mdata) {
> + log_debug("read_mdata() method not defined\n");
> + return -ENOSYS;
> + }
> +
> + return ops->read_mdata(dev, mdata, primary);
> +}
> +
> +/**
> + * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> +{
> + const struct fwu_mdata_ops *ops = device_get_ops(dev);
> +
> + if (!ops->write_mdata) {
> + log_debug("write_mdata() method not defined\n");
> + return -ENOSYS;
> + }
> +
> + return ops->write_mdata(dev, mdata, primary);
> +}
> +
> /**
> * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers
> * @dev: FWU metadata device
> 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
> + * @primary: If primary or secondary copy of meta-data is to be read
> + *
> + * Return: 0 if OK, -ve on error
> + */
> + int (*read_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> +
> + /**
> + * write_mdata() - Write the given FWU metadata copy
> + * @dev: FWU metadata device
> + * @mdata: Copy of the FWU metadata
> + * @primary: If primary or secondary copy of meta-data is to be written
> + *
> + * Return: 0 if OK, -ve on error
> + */
> + int (*write_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> +
> /**
> * check_mdata() - Check if the FWU metadata is valid
> * @dev: FWU device
> @@ -126,6 +146,27 @@ struct fwu_mdata_ops {
> EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \
> 0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8)
>
> +/**
> + * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata()
> + */
> +int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> +
> +/**
> + * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata()
> + */
> +int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary);
> +
> +/**
> + * 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.
> + *
> + * Return: 0 if OK, -ve on error
> +*/
> +int fwu_get_verified_mdata(struct fwu_mdata *mdata);
Nitpicking: would you be ok to rename this function to fwu_get_mdata().
When getting fwu mdata, we obviously expect to get reliable data.
> +
> /**
> * fwu_check_mdata_validity() - Check for validity of the FWU metadata copies
> *
> diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
> index 5313d07302..3d51b91ed5 100644
> --- a/lib/fwu_updates/fwu.c
> +++ b/lib/fwu_updates/fwu.c
> @@ -15,13 +15,13 @@
> #include <linux/errno.h>
> #include <linux/types.h>
>
> +#include <u-boot/crc.h>
> +
> +static struct fwu_mdata g_mdata = { 0 };
Can remove "= { 0 };"
> +static struct udevice *g_dev;
> static u8 in_trial;
> static u8 boottime_check;
>
> -#include <linux/errno.h>
> -#include <linux/types.h>
> -#include <u-boot/crc.h>
> -
> enum {
> IMAGE_ACCEPT_SET = 1,
> IMAGE_ACCEPT_CLEAR,
> @@ -161,6 +161,134 @@ static int fwu_get_image_type_id(u8 *image_index, efi_guid_t *image_type_id)
> return -ENOENT;
> }
>
> +/**
> + * fwu_sync_mdata() - Update given meta-data partition(s) with the copy provided
> + * @mdata: FWU metadata structure
> + * @part: Bitmask of FWU metadata partitions to be written to
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +static int fwu_sync_mdata(struct fwu_mdata *mdata, int part)
> +{
> + void *buf = &mdata->version;
> + int err = 0;
> +
> + /*
> + * 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));
> +
> + if (part & PRIMARY_PART)
> + err = fwu_write_mdata(g_dev, mdata, true);
> +
> + if (err) {
> + log_err("Unable to write primary mdata\n");
> + return err;
> + }
> +
> + if (part & SECONDARY_PART)
> + err = fwu_write_mdata(g_dev, mdata, false);
> +
> + if (err) {
> + log_err("Unable to write secondary mdata\n");
> + return err;
> + }
> +
> + /* update the cached copy of meta-data */
> + memcpy(&g_mdata, mdata, sizeof(struct fwu_mdata));
> +
> + return 0;
> +}
> +
> +static inline int mdata_crc_check(struct fwu_mdata *mdata)
> +{
> + u32 calc_crc32 = crc32(0, &mdata->version, sizeof(*mdata) - sizeof(u32));
Add an empty line below the above definition.
> + return calc_crc32 == mdata->crc32 ? 0 : -EINVAL;
> +}
> +
> +/**
> + * 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.
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int fwu_get_verified_mdata(struct fwu_mdata *mdata)
> +{
> + int err;
> + bool pri_ok, sec_ok;
> + struct fwu_mdata s, *p_mdata, *s_mdata;
> +
> + p_mdata = &g_mdata;
> + s_mdata = &s;
> +
> + /* if mdata already read and ready */
> + err = mdata_crc_check(p_mdata, true);
2nd argument to be removed. Ditto for the other occurrences of
mdata_crc_check() calls.
Note here I would pass straight &g_mdata as argument rather than
p_mdata indirection, for clarity.
> + if (!err) {
> + if (mdata)
> + memcpy(mdata, p_mdata, sizeof(struct fwu_mdata));
> + return 0;
> + }
> + /* else read, verify and, if needed, fix mdata */
> +
> + pri_ok = false;
> + err = fwu_read_mdata(g_dev, p_mdata, true);
> + if (!err) {
> + err = mdata_crc_check(p_mdata, true);
> + if (!err)
> + pri_ok = true;
> + else
> + log_debug("primary mdata: crc32 failed\n");
> + }
> +
> + sec_ok = false;
> + err = fwu_read_mdata(g_dev, s_mdata, false);
> + if (!err) {
> + err = mdata_crc_check(s_mdata, false);
> + if (!err)
> + sec_ok = true;
> + else
> + log_debug("secondary mdata: crc32 failed\n");
> + }
> +
> + if (pri_ok && sec_ok) {
> + /*
> + * Before returning, check that both the
> + * FWU metadata copies are the same. If not,
> + * populate the secondary partition from the
> + * primary partition copy.
> + */
> + if (memcmp(p_mdata, s_mdata, sizeof(struct fwu_mdata))) {
> + log_info("Both FWU metadata copies are valid but do not match.");
> + log_info(" Restoring the secondary partition from the primary\n");
> + sec_ok = false;
> + }
> + }
> +
> + if (!pri_ok) {
> + memcpy(p_mdata, s_mdata, sizeof(struct fwu_mdata));
> + err = fwu_sync_mdata(p_mdata, PRIMARY_PART);
Should test return value.
> + }
> +
> + if (!sec_ok) {
> + memcpy(s_mdata, p_mdata, sizeof(struct fwu_mdata));
> + err = fwu_sync_mdata(s_mdata, SECONDARY_PART);
Ditto
> + }
> +
> + /* make sure atleast one copy is good */
s/atleast/at least/
> + err = mdata_crc_check(p_mdata, true);
This is not needed, it's been already verified unless we want to catch
the case !pri_ok && !sec_ok. I think this case should be explicitly
handled above with a nice console trace message/
if (!pri_ok && !sec_ok) {
log_err("No reliable fwu mdata found\n");
return -EINVAL;
}
> + if (err)
> + log_debug("mdata saving... crc32 failed\n");
> + else if (mdata)
> + memcpy(mdata, p_mdata, sizeof(struct fwu_mdata));
> +
> + return err;
> +}
> +
> /**
> * fwu_verify_mdata() - Verify the FWU metadata
> * @mdata: FWU metadata structure
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] fwu: gpt: use cached meta-data partition numbers
2022-11-04 2:42 ` [PATCH 1/4] fwu: gpt: use cached meta-data partition numbers jassisinghbrar
@ 2022-11-07 17:24 ` Etienne Carriere
2022-11-07 18:41 ` Jassi Brar
0 siblings, 1 reply; 15+ messages in thread
From: Etienne Carriere @ 2022-11-07 17:24 UTC (permalink / raw)
To: jassisinghbrar
Cc: u-boot, ilias.apalodimas, sughosh.ganu, trini, sjg, xypron.glpk,
patrick.delaunay, patrice.chotard, Jassi Brar
Hello Jassi,
Acked-by: Etienne Carriere <etienne.carriere@linaro.org>
with the 2 below comments addressed.
On Fri, 4 Nov 2022 at 03:42, <jassisinghbrar@gmail.com> wrote:
>
> From: Jassi Brar <jaswinder.singh@linaro.org>
>
> Use cached values and avoid parsing and scanning through partitions
> everytime for meta-data partitions because they can't change after bootup.
>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
> drivers/fwu-mdata/gpt_blk.c | 43 +++++++++++++++++++++----------------
> 1 file changed, 24 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c
> index d35ce49c5c..d706e3d4e4 100644
> --- a/drivers/fwu-mdata/gpt_blk.c
> +++ b/drivers/fwu-mdata/gpt_blk.c
> @@ -24,8 +24,9 @@ enum {
> MDATA_WRITE,
> };
>
> -static int gpt_get_mdata_partitions(struct blk_desc *desc,
> - uint mdata_parts[2])
> +static uint g_mdata_part[2] = {0, 0};
Not needed to initialize to 0.
"static uint g_mdata_part[2];" is enough.
> +
> +static int gpt_get_mdata_partitions(struct blk_desc *desc)
> {
> int i, ret;
> u32 nparts;
> @@ -33,18 +34,19 @@ static int gpt_get_mdata_partitions(struct blk_desc *desc,
> struct disk_partition info;
> const efi_guid_t fwu_mdata_guid = FWU_MDATA_GUID;
>
> + /* if primary and secondary partitons already found */
s/partitons/partitions/
> + if (g_mdata_part[0] && g_mdata_part[1])
> + return 0;
> +
> nparts = 0;
> - for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) {
> + for (i = 1; i < MAX_SEARCH_PARTITIONS && nparts < 2; i++) {
> if (part_get_info(desc, i, &info))
> continue;
> uuid_str_to_bin(info.type_guid, part_type_guid.b,
> UUID_STR_FORMAT_GUID);
>
> - if (!guidcmp(&fwu_mdata_guid, &part_type_guid)) {
> - if (nparts < 2)
> - mdata_parts[nparts] = i;
> - ++nparts;
> - }
> + if (!guidcmp(&fwu_mdata_guid, &part_type_guid))
> + g_mdata_part[nparts++] = i;
> }
>
> if (nparts != 2) {
> @@ -127,26 +129,25 @@ static int fwu_gpt_update_mdata(struct udevice *dev, struct fwu_mdata *mdata)
> {
> int ret;
> struct blk_desc *desc;
> - uint mdata_parts[2];
> struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
>
> desc = dev_get_uclass_plat(priv->blk_dev);
>
> - ret = gpt_get_mdata_partitions(desc, mdata_parts);
> + ret = gpt_get_mdata_partitions(desc);
> if (ret < 0) {
> log_debug("Error getting the FWU metadata partitions\n");
> return -ENOENT;
> }
>
> /* First write the primary partition */
> - ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, mdata_parts[0]);
> + ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, g_mdata_part[0]);
> if (ret < 0) {
> log_debug("Updating primary FWU metadata partition failed\n");
> return ret;
> }
>
> /* And now the replica */
> - ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, mdata_parts[1]);
> + ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, g_mdata_part[1]);
> if (ret < 0) {
> log_debug("Updating secondary FWU metadata partition failed\n");
> return ret;
> @@ -158,16 +159,14 @@ static int fwu_gpt_update_mdata(struct udevice *dev, struct fwu_mdata *mdata)
> static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata *mdata)
> {
> int ret;
> - uint mdata_parts[2];
> -
> - ret = gpt_get_mdata_partitions(desc, mdata_parts);
>
> + ret = gpt_get_mdata_partitions(desc);
> if (ret < 0) {
> log_debug("Error getting the FWU metadata partitions\n");
> return -ENOENT;
> }
>
> - ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, mdata_parts[0]);
> + ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, g_mdata_part[0]);
> if (ret < 0) {
> log_debug("Failed to read the FWU metadata from the device\n");
> return -EIO;
> @@ -182,7 +181,7 @@ static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata *mdata)
> * Try to read the replica.
> */
> memset(mdata, '\0', sizeof(struct fwu_mdata));
> - ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, mdata_parts[1]);
> + ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, g_mdata_part[1]);
> if (ret < 0) {
> log_debug("Failed to read the FWU metadata from the device\n");
> return -EIO;
> @@ -206,9 +205,15 @@ static int fwu_gpt_get_mdata(struct udevice *dev, struct fwu_mdata *mdata)
> static int fwu_gpt_get_mdata_partitions(struct udevice *dev, uint *mdata_parts)
> {
> struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> + int err;
> +
> + err = gpt_get_mdata_partitions(dev_get_uclass_plat(priv->blk_dev));
> + if (!err) {
> + mdata_parts[0] = g_mdata_part[0];
> + mdata_parts[1] = g_mdata_part[1];
> + }
>
> - return gpt_get_mdata_partitions(dev_get_uclass_plat(priv->blk_dev),
> - mdata_parts);
> + return err;
> }
>
> static int fwu_gpt_read_mdata_partition(struct udevice *dev,
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/4] fwu: move meta-data management in core
2022-11-07 17:23 ` Etienne Carriere
@ 2022-11-07 18:28 ` Jassi Brar
2022-11-07 20:13 ` Etienne Carriere
0 siblings, 1 reply; 15+ messages in thread
From: Jassi Brar @ 2022-11-07 18:28 UTC (permalink / raw)
To: Etienne Carriere
Cc: u-boot, ilias.apalodimas, sughosh.ganu, trini, sjg, xypron.glpk,
patrick.delaunay, patrice.chotard, Jassi Brar
On Mon, Nov 7, 2022 at 11:24 AM Etienne Carriere
<etienne.carriere@linaro.org> wrote:
> On Fri, 4 Nov 2022 at 03:43, <jassisinghbrar@gmail.com> wrote:
> >
> > +/**
> > + * 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.
> > + *
> > + * Return: 0 if OK, -ve on error
> > +*/
> > +int fwu_get_verified_mdata(struct fwu_mdata *mdata);
>
> Nitpicking: would you be ok to rename this function to fwu_get_mdata().
> When getting fwu mdata, we obviously expect to get reliable data.
>
I originally called it fwu_get_mdata() in my local 1-patch change :)
But there already is one such function and I had to rename, only to delete that.
....
> > +
> > +static struct fwu_mdata g_mdata = { 0 };
>
> Can remove "= { 0 };"
>
I knew, but it was supposed to imply that we want the first crc check
to fail (because we set that to 0 here).
.....
> > +static inline int mdata_crc_check(struct fwu_mdata *mdata)
> > +{
> > + u32 calc_crc32 = crc32(0, &mdata->version, sizeof(*mdata) - sizeof(u32));
>
> Add an empty line below the above definition.
>
OK.
....
> > + /* if mdata already read and ready */
> > + err = mdata_crc_check(p_mdata, true);
>
> 2nd argument to be removed. Ditto for the other occurrences of
> mdata_crc_check() calls.
>
Ouch, I 'cleaned' the patch before submission. Will fix that.
> Note here I would pass straight &g_mdata as argument rather than
> p_mdata indirection, for clarity.
>
Hmm... I wanted to keep the g_mdata usage clear and minimal.
....
> > +
> > + if (!pri_ok) {
> > + memcpy(p_mdata, s_mdata, sizeof(struct fwu_mdata));
> > + err = fwu_sync_mdata(p_mdata, PRIMARY_PART);
>
> Should test return value.
>
> > + }
> > +
> > + if (!sec_ok) {
> > + memcpy(s_mdata, p_mdata, sizeof(struct fwu_mdata));
> > + err = fwu_sync_mdata(s_mdata, SECONDARY_PART);
>
> Ditto
>
> > + }
> > +
> > + /* make sure atleast one copy is good */
>
> s/atleast/at least/
>
ok
> > + err = mdata_crc_check(p_mdata, true);
>
> This is not needed, it's been already verified unless we want to catch
> the case !pri_ok && !sec_ok. I think this case should be explicitly
> handled above with a nice console trace message/
>
ok
Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] fwu: gpt: implement read_mdata and write_mdata callbacks
2022-11-07 17:23 ` Etienne Carriere
@ 2022-11-07 18:30 ` Jassi Brar
0 siblings, 0 replies; 15+ messages in thread
From: Jassi Brar @ 2022-11-07 18:30 UTC (permalink / raw)
To: Etienne Carriere
Cc: u-boot, ilias.apalodimas, sughosh.ganu, trini, sjg, xypron.glpk,
patrick.delaunay, patrice.chotard, Jassi Brar
On Mon, Nov 7, 2022 at 11:23 AM Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> On Fri, 4 Nov 2022 at 03:43, <jassisinghbrar@gmail.com> wrote:
> >
> > From: Jassi Brar <jaswinder.singh@linaro.org>
> >
> > Moving towards using common code for meta-data management,
> > implement the read/write mdata hooks.
> >
> > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> > ---
> > drivers/fwu-mdata/gpt_blk.c | 36 ++++++++++++++++++++++++++++++++++++
> > 1 file changed, 36 insertions(+)
> >
> > diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c
> > index d706e3d4e4..7fda041302 100644
> > --- a/drivers/fwu-mdata/gpt_blk.c
> > +++ b/drivers/fwu-mdata/gpt_blk.c
> > @@ -272,7 +272,43 @@ static int fwu_mdata_gpt_blk_probe(struct udevice *dev)
> > return 0;
> > }
> >
> > +static int fwu_gpt_read_mdata(struct udevice *dev, struct fwu_mdata *mdata,
> > + bool primary)
> > +{
> > + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> > + struct blk_desc *desc = dev_get_uclass_plat(priv->blk_dev);
> > + int ret;
> > +
> > + ret = gpt_get_mdata_partitions(desc);
> > + if (ret < 0) {
> > + log_debug("Error getting the FWU metadata partitions\n");
> > + return -ENOENT;
>
> Not returning ret value?
>
It is unchanged from original behavior.
> > + }
> > +
> > + return gpt_read_write_mdata(desc, mdata, MDATA_READ,
> > + primary ? g_mdata_part[0] : g_mdata_part[1]);
>
> Fix indentation.
>
>
> > +}
> > +
> > +static int fwu_gpt_write_mdata(struct udevice *dev, struct fwu_mdata *mdata,
> > + bool primary)
> > +{
> > + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> > + struct blk_desc *desc = dev_get_uclass_plat(priv->blk_dev);
> > + int ret;
> > +
> > + ret = gpt_get_mdata_partitions(desc);
> > + if (ret < 0) {
> > + log_debug("Error getting the FWU metadata partitions\n");
> > + return -ENOENT;
> > + }
> > +
> > + return gpt_read_write_mdata(desc, mdata, MDATA_WRITE,
> > + primary ? g_mdata_part[0] : g_mdata_part[1]);
>
> Dito
>
OK.
thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] fwu: meta-data: switch to management by common code
2022-11-07 17:23 ` Etienne Carriere
@ 2022-11-07 18:39 ` Jassi Brar
0 siblings, 0 replies; 15+ messages in thread
From: Jassi Brar @ 2022-11-07 18:39 UTC (permalink / raw)
To: Etienne Carriere
Cc: u-boot, ilias.apalodimas, sughosh.ganu, trini, sjg, xypron.glpk,
patrick.delaunay, patrice.chotard, Jassi Brar
On Mon, Nov 7, 2022 at 11:23 AM Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Hello Jassi,
>
> FYI, I've successfully tested this series on stm32mp1 for FWU, once
> the few typos fixed to build it them.
>
Thanks for testing. I will fix the typos and resubmit.
.....
> > @@ -831,11 +662,7 @@ static int fwu_boottime_checks(void *ctx, struct event *event)
> > if (efi_init_obj_list() != EFI_SUCCESS)
> > return 0;
> >
> > - ret = fwu_get_dev_mdata(&dev, &mdata);
> > - if (ret)
> > - return ret;
> > -
> > - in_trial = in_trial_state(&mdata);
> > + in_trial = in_trial_state(&g_mdata);
>
> Since g_mdata is global, in_trial_state() could access it straight
> without needing to pass g_mdata as argument.
>
I wanted to keep changes to the minimum. Also using global variables
is ugly, so I wanted to keep the g_mdata usage to minimum (only for
initializing the local variable). And finally the signature
in_trial_state(void) hides the fact that it works on external data -
g_mdata.
Cheers!
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] fwu: gpt: use cached meta-data partition numbers
2022-11-07 17:24 ` Etienne Carriere
@ 2022-11-07 18:41 ` Jassi Brar
0 siblings, 0 replies; 15+ messages in thread
From: Jassi Brar @ 2022-11-07 18:41 UTC (permalink / raw)
To: Etienne Carriere
Cc: u-boot, ilias.apalodimas, sughosh.ganu, trini, sjg, xypron.glpk,
patrick.delaunay, patrice.chotard, Jassi Brar
On Mon, Nov 7, 2022 at 11:24 AM Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Hello Jassi,
>
> Acked-by: Etienne Carriere <etienne.carriere@linaro.org>
> with the 2 below comments addressed.
>
> On Fri, 4 Nov 2022 at 03:42, <jassisinghbrar@gmail.com> wrote:
> >
> > From: Jassi Brar <jaswinder.singh@linaro.org>
> >
> > Use cached values and avoid parsing and scanning through partitions
> > everytime for meta-data partitions because they can't change after bootup.
> >
> > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> > ---
> > drivers/fwu-mdata/gpt_blk.c | 43 +++++++++++++++++++++----------------
> > 1 file changed, 24 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c
> > index d35ce49c5c..d706e3d4e4 100644
> > --- a/drivers/fwu-mdata/gpt_blk.c
> > +++ b/drivers/fwu-mdata/gpt_blk.c
> > @@ -24,8 +24,9 @@ enum {
> > MDATA_WRITE,
> > };
> >
> > -static int gpt_get_mdata_partitions(struct blk_desc *desc,
> > - uint mdata_parts[2])
> > +static uint g_mdata_part[2] = {0, 0};
>
> Not needed to initialize to 0.
> "static uint g_mdata_part[2];" is enough.
>
I am aware. The 0 is like an explicit poison value that we test
against for un-initialized partition numbers.
> > +
> > +static int gpt_get_mdata_partitions(struct blk_desc *desc)
> > {
> > int i, ret;
> > u32 nparts;
> > @@ -33,18 +34,19 @@ static int gpt_get_mdata_partitions(struct blk_desc *desc,
> > struct disk_partition info;
> > const efi_guid_t fwu_mdata_guid = FWU_MDATA_GUID;
> >
> > + /* if primary and secondary partitons already found */
>
> s/partitons/partitions/
>
ok
Thank you!
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/4] fwu: move meta-data management in core
2022-11-07 18:28 ` Jassi Brar
@ 2022-11-07 20:13 ` Etienne Carriere
2022-11-08 4:27 ` Jassi Brar
0 siblings, 1 reply; 15+ messages in thread
From: Etienne Carriere @ 2022-11-07 20:13 UTC (permalink / raw)
To: Jassi Brar
Cc: u-boot, ilias.apalodimas, sughosh.ganu, trini, sjg, xypron.glpk,
patrick.delaunay, patrice.chotard, Jassi Brar
Hello Jassi,
On Mon, 7 Nov 2022 at 19:29, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> On Mon, Nov 7, 2022 at 11:24 AM Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> > On Fri, 4 Nov 2022 at 03:43, <jassisinghbrar@gmail.com> wrote:
> > >
> > > +/**
> > > + * 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.
> > > + *
> > > + * Return: 0 if OK, -ve on error
> > > +*/
> > > +int fwu_get_verified_mdata(struct fwu_mdata *mdata);
> >
> > Nitpicking: would you be ok to rename this function to fwu_get_mdata().
> > When getting fwu mdata, we obviously expect to get reliable data.
> >
> I originally called it fwu_get_mdata() in my local 1-patch change :)
> But there already is one such function and I had to rename, only to delete that.
Ok, maybe a later commit to rename the function once the old one has
been removed in patch 4/4.
>
> ....
> > > +
> > > +static struct fwu_mdata g_mdata = { 0 };
> >
> > Can remove "= { 0 };"
> >
> I knew, but it was supposed to imply that we want the first crc check
> to fail (because we set that to 0 here).
As zero init is implicit, I think maintainers will ask to remove it here.
Maybe add an inline comment above the global variable definition.
/* Crc32 of a zeroes produces a non 0 value */
>
> .....
> > > +static inline int mdata_crc_check(struct fwu_mdata *mdata)
> > > +{
> > > + u32 calc_crc32 = crc32(0, &mdata->version, sizeof(*mdata) - sizeof(u32));
> >
> > Add an empty line below the above definition.
> >
> OK.
>
> ....
> > > + /* if mdata already read and ready */
> > > + err = mdata_crc_check(p_mdata, true);
> >
> > 2nd argument to be removed. Ditto for the other occurrences of
> > mdata_crc_check() calls.
> >
> Ouch, I 'cleaned' the patch before submission. Will fix that.
>
> > Note here I would pass straight &g_mdata as argument rather than
> > p_mdata indirection, for clarity.
> >
> Hmm... I wanted to keep the g_mdata usage clear and minimal.
It looks strange we need to check p_mdata init value to understand
we're accessing the cached mdata.
p_mdata is expected to be the primary part. Perfer keep it for this usage.
br,
etienne
>
> ....
> > > +
> > > + if (!pri_ok) {
> > > + memcpy(p_mdata, s_mdata, sizeof(struct fwu_mdata));
> > > + err = fwu_sync_mdata(p_mdata, PRIMARY_PART);
> >
> > Should test return value.
> >
> > > + }
> > > +
> > > + if (!sec_ok) {
> > > + memcpy(s_mdata, p_mdata, sizeof(struct fwu_mdata));
> > > + err = fwu_sync_mdata(s_mdata, SECONDARY_PART);
> >
> > Ditto
> >
> > > + }
> > > +
> > > + /* make sure atleast one copy is good */
> >
> > s/atleast/at least/
> >
> ok
>
> > > + err = mdata_crc_check(p_mdata, true);
> >
> > This is not needed, it's been already verified unless we want to catch
> > the case !pri_ok && !sec_ok. I think this case should be explicitly
> > handled above with a nice console trace message/
> >
> ok
>
> Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/4] fwu: move meta-data management in core
2022-11-07 20:13 ` Etienne Carriere
@ 2022-11-08 4:27 ` Jassi Brar
0 siblings, 0 replies; 15+ messages in thread
From: Jassi Brar @ 2022-11-08 4:27 UTC (permalink / raw)
To: Etienne Carriere
Cc: u-boot, ilias.apalodimas, sughosh.ganu, trini, sjg, xypron.glpk,
patrick.delaunay, patrice.chotard, Jassi Brar
On Mon, Nov 7, 2022 at 2:13 PM Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Hello Jassi,
>
> On Mon, 7 Nov 2022 at 19:29, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >
> > On Mon, Nov 7, 2022 at 11:24 AM Etienne Carriere
> > <etienne.carriere@linaro.org> wrote:
> > > On Fri, 4 Nov 2022 at 03:43, <jassisinghbrar@gmail.com> wrote:
> > > >
> > > > +/**
> > > > + * 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.
> > > > + *
> > > > + * Return: 0 if OK, -ve on error
> > > > +*/
> > > > +int fwu_get_verified_mdata(struct fwu_mdata *mdata);
> > >
> > > Nitpicking: would you be ok to rename this function to fwu_get_mdata().
> > > When getting fwu mdata, we obviously expect to get reliable data.
> > >
> > I originally called it fwu_get_mdata() in my local 1-patch change :)
> > But there already is one such function and I had to rename, only to delete that.
>
> Ok, maybe a later commit to rename the function once the old one has
> been removed in patch 4/4.
>
ok.
> > ....
> > > > +
> > > > +static struct fwu_mdata g_mdata = { 0 };
> > >
> > > Can remove "= { 0 };"
> > >
> > I knew, but it was supposed to imply that we want the first crc check
> > to fail (because we set that to 0 here).
>
> As zero init is implicit, I think maintainers will ask to remove it here.
>
Though there are many such examples already and the compiler would
anyway ignore it.
> Maybe add an inline comment above the global variable definition.
> /* Crc32 of a zeroes produces a non 0 value */
>
ok.
thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-11-08 4:27 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-04 2:42 [PATCH 0/4] FWU: Handle meta-data in common code jassisinghbrar
2022-11-04 2:42 ` [PATCH 1/4] fwu: gpt: use cached meta-data partition numbers jassisinghbrar
2022-11-07 17:24 ` Etienne Carriere
2022-11-07 18:41 ` Jassi Brar
2022-11-04 2:43 ` [PATCH 2/4] fwu: move meta-data management in core jassisinghbrar
2022-11-07 17:23 ` Etienne Carriere
2022-11-07 18:28 ` Jassi Brar
2022-11-07 20:13 ` Etienne Carriere
2022-11-08 4:27 ` Jassi Brar
2022-11-04 2:43 ` [PATCH 3/4] fwu: gpt: implement read_mdata and write_mdata callbacks jassisinghbrar
2022-11-07 17:23 ` Etienne Carriere
2022-11-07 18:30 ` Jassi Brar
2022-11-04 2:43 ` [PATCH 4/4] fwu: meta-data: switch to management by common code jassisinghbrar
2022-11-07 17:23 ` Etienne Carriere
2022-11-07 18:39 ` Jassi Brar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.