Re: [PATCHv2 1/5] FWU: Add FWU metadata access driver for MTD storage regions
On Fri, Oct 14, 2022 at 2:07 AM Ilias Apalodimas
wrote:
>
> On Sun, Oct 02, 2022 at 06:51:32PM -0500, jassisinghb...@gmail.com wrote:
> > +
> > +static bool mtd_is_aligned_with_block_size(struct mtd_info *mtd, u64 size)
> > +{
> > + return !do_div(size, mtd->erasesize);
> > +}
> > +
>
> Can we please add some sphinx style documentation overall ?
>
I can but I thought that is for public api and not static helper functions?
> > +
> > +static int fwu_mtd_check_mdata(struct udevice *dev)
> > +{
> > + struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> > + struct fwu_mdata primary, secondary;
> > + bool pri = false, sec = false;
> > + int ret;
> > +
> > + ret = fwu_mtd_load_mdata(mtd_priv->mtd, &primary,
> > +
> > mtd_priv->pri_offset, FWU_MDATA_PRIMARY);
> > + if (ret < 0)
> > + log_debug("Failed to read the primary mdata: %d\n", ret);
> > + else
> > + pri = true;
> > +
> > + ret = fwu_mtd_load_mdata(mtd_priv->mtd, &secondary,
> > +
> > mtd_priv->sec_offset, FWU_MDATA_SECONDARY);
> > + if (ret < 0)
> > + log_debug("Failed to read the secondary mdata: %d\n", ret);
> > + else
> > + sec = true;
> > +
> > + if (pri && sec) {
> > + if (memcmp(&primary, &secondary, sizeof(struct fwu_mdata))) {
> > + log_debug("The primary and the secondary mdata are
> > different\n");
> > + ret = -1;
> > + }
> > + } else if (pri) {
> > + ret = fwu_mtd_save_secondary_mdata(mtd_priv, &primary);
> > + if (ret < 0)
> > + log_debug("Restoring secondary mdata partition
> > failed\n");
> > + } else if (sec) {
> > + ret = fwu_mtd_save_primary_mdata(mtd_priv, &secondary);
> > + if (ret < 0)
> > + log_debug("Restoring primary mdata partition
> > failed\n");
> > + }
>
> Same on this one. The requirements here are
> - Read our metadata
> - Compare the 2 partitions
>
> The only thing that's 'hardware' specific here is reading the metadata. We
> should at least unify the comparing part and restoration in case of
> failures, no ?
>
Yes. Since redundant copy of meta-data is a requirement of the spec,
we should maintain that in the fwu core code.
Maybe something like adding 'bool primary' argument to get_mdata() and
update_mdata()
thanks
Re: [PATCHv2 1/5] FWU: Add FWU metadata access driver for MTD storage regions
Hi Jassi,
Thanks for the patches and apologies for the late reply
On Sun, Oct 02, 2022 at 06:51:32PM -0500, jassisinghb...@gmail.com wrote:
> From: Sughosh Ganu
>
> In the FWU Multi Bank Update feature, the information about the
> updatable images is stored as part of the metadata, on a separate
> region. Add a driver for reading from and writing to the metadata
> when the updatable images and the metadata are stored on a raw
> MTD region.
>
> Signed-off-by: Sughosh Ganu
> Signed-off-by: Jassi Brar
> ---
> drivers/fwu-mdata/Kconfig | 17 +-
> drivers/fwu-mdata/Makefile | 1 +
[...]
> +
> +/* Internal helper structure to move data around */
> +struct fwu_mdata_mtd_priv {
> + struct mtd_info *mtd;
> + u32 pri_offset;
> + u32 sec_offset;
> +};
> +
> +enum fwu_mtd_op {
> + FWU_MTD_READ,
> + FWU_MTD_WRITE,
> +};
> +
> +#define FWU_MDATA_PRIMARYtrue
> +#define FWU_MDATA_SECONDARY false
> +
> +static bool mtd_is_aligned_with_block_size(struct mtd_info *mtd, u64 size)
> +{
> + return !do_div(size, mtd->erasesize);
> +}
> +
Can we please add some sphinx style documentation overall ?
> +static int mtd_io_data(struct mtd_info *mtd, u32 offs, u32 size, void *data,
> +enum fwu_mtd_op op)
> +{
> + struct mtd_oob_ops io_op ={};
> + u64 lock_offs, lock_len;
> + size_t len;
> + void *buf;
> + int ret;
> +
> + if (!mtd_is_aligned_with_block_size(mtd, offs)) {
> + log_err("Offset unaligned with a block (0x%x)\n",
> mtd->erasesize);
> + return -EINVAL;
> + }
> +
> + lock_offs = offs;
> + /* This will expand erase size to align with the block size */
> + lock_len = round_up(size, mtd->erasesize);
> +
> + ret = mtd_unlock(mtd, lock_offs, lock_len);
> + if (ret && ret != -EOPNOTSUPP)
> + return ret;
> +
> + if (op == FWU_MTD_WRITE) {
> + struct erase_info erase_op = {};
> +
> + erase_op.mtd = mtd;
> + erase_op.addr = lock_offs;
> + erase_op.len = lock_len;
> + erase_op.scrub = 0;
> +
> + ret = mtd_erase(mtd, &erase_op);
> + if (ret)
> + goto lock;
> + }
> +
> + /* Also, expand the write size to align with the write size */
> + len = round_up(size, mtd->writesize);
> +
> + buf = memalign(ARCH_DMA_MINALIGN, len);
> + if (!buf) {
> + ret = -ENOMEM;
> + goto lock;
> + }
> + memset(buf, 0xff, len);
> +
> + io_op.mode = MTD_OPS_AUTO_OOB;
> + io_op.len = len;
> + io_op.ooblen = 0;
> + io_op.datbuf = buf;
> + io_op.oobbuf = NULL;
> +
> + if (op == FWU_MTD_WRITE) {
> + memcpy(buf, data, size);
> + ret = mtd_write_oob(mtd, offs, &io_op);
> + } else {
> + ret = mtd_read_oob(mtd, offs, &io_op);
> + if (!ret)
> + memcpy(data, buf, size);
> + }
> + free(buf);
> +
> +lock:
> + mtd_lock(mtd, lock_offs, lock_len);
> +
> + return ret;
> +}
> +
> +static int fwu_mtd_load_mdata(struct mtd_info *mtd, struct fwu_mdata *mdata,
> + u32 offs, bool primary)
> +{
> + size_t size = sizeof(struct fwu_mdata);
> + int ret;
> +
> + ret = mtd_io_data(mtd, offs, size, mdata, FWU_MTD_READ);
> + if (ret >= 0)
> + ret = fwu_verify_mdata(mdata, primary);
> +
> + return ret;
> +}
> +
> +static int fwu_mtd_save_primary_mdata(struct fwu_mdata_mtd_priv *mtd_priv,
> + struct fwu_mdata *mdata)
> +{
> + return mtd_io_data(mtd_priv->mtd, mtd_priv->pri_offset,
> +sizeof(struct fwu_mdata), mdata, FWU_MTD_WRITE);
> +}
> +
> +static int fwu_mtd_save_secondary_mdata(struct fwu_mdata_mtd_priv *mtd_priv,
> +struct fwu_mdata *mdata)
> +{
> + return mtd_io_data(mtd_priv->mtd, mtd_priv->sec_offset,
> +sizeof(struct fwu_mdata), mdata, FWU_MTD_WRITE);
> +}
> +
> +static int fwu_mtd_get_valid_mdata(struct fwu_mdata_mtd_priv *mtd_priv,
> + struct fwu_mdata *mdata)
> +{
> + int ret;
> +
> + ret = fwu_mtd_load_mdata(mtd_priv->mtd, mdata,
> + mtd_priv->pri_offset,
> FWU_MDATA_PRIMARY);
> + if (!ret)
> + return 0;
> +
> + log_debug("Failed to load primary mdata. Trying secondary...\n");
> +
> + ret = fwu_mtd_load_mdata(mtd_priv->mtd, mdata,
> + mtd_priv->sec_offset,
> FWU_MDATA_SECONDARY);
> + if (ret) {
> + log_debug("Failed to load secondary mdata.\n");
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static int fwu_mtd_update_mdata(struct udevice *dev, struct fwu_mdata *mdata)
> +{
> + struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> + int ret;
> +
> + /* First write t
[PATCHv2 1/5] FWU: Add FWU metadata access driver for MTD storage regions
From: Sughosh Ganu
In the FWU Multi Bank Update feature, the information about the
updatable images is stored as part of the metadata, on a separate
region. Add a driver for reading from and writing to the metadata
when the updatable images and the metadata are stored on a raw
MTD region.
Signed-off-by: Sughosh Ganu
Signed-off-by: Jassi Brar
---
drivers/fwu-mdata/Kconfig | 17 +-
drivers/fwu-mdata/Makefile | 1 +
drivers/fwu-mdata/raw_mtd.c | 305
3 files changed, 322 insertions(+), 1 deletion(-)
create mode 100644 drivers/fwu-mdata/raw_mtd.c
diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig
index 36c4479a59..841c6f5b2d 100644
--- a/drivers/fwu-mdata/Kconfig
+++ b/drivers/fwu-mdata/Kconfig
@@ -6,11 +6,26 @@ config FWU_MDATA
FWU Metadata partitions reside on the same storage device
which contains the other FWU updatable firmware images.
+choice
+ prompt "Storage Layout Scheme"
+ depends on FWU_MDATA
+ default FWU_MDATA_GPT_BLK
+
config FWU_MDATA_GPT_BLK
- bool "FWU Metadata access for GPT partitioned Block devices"
+ bool "GPT Partitioned Block devices"
select PARTITION_TYPE_GUID
select PARTITION_UUIDS
depends on FWU_MDATA && BLK && EFI_PARTITION
help
Enable support for accessing FWU Metadata on GPT partitioned
block devices.
+
+config FWU_MDATA_MTD
+ bool "Raw MTD devices"
+ depends on MTD
+ help
+ Enable support for accessing FWU Metadata on non-partitioned
+ (or non-GPT partitioned, e.g. partition nodes in devicetree)
+ MTD devices.
+
+endchoice
diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile
index 3fee64c10c..06c49747ba 100644
--- a/drivers/fwu-mdata/Makefile
+++ b/drivers/fwu-mdata/Makefile
@@ -6,3 +6,4 @@
obj-$(CONFIG_FWU_MDATA) += fwu-mdata-uclass.o
obj-$(CONFIG_FWU_MDATA_GPT_BLK) += gpt_blk.o
+obj-$(CONFIG_FWU_MDATA_MTD) += raw_mtd.o
diff --git a/drivers/fwu-mdata/raw_mtd.c b/drivers/fwu-mdata/raw_mtd.c
new file mode 100644
index 00..ff2064b442
--- /dev/null
+++ b/drivers/fwu-mdata/raw_mtd.c
@@ -0,0 +1,305 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2022, Linaro Limited
+ */
+
+#define LOG_CATEGORY UCLASS_FWU_MDATA
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+/* Internal helper structure to move data around */
+struct fwu_mdata_mtd_priv {
+ struct mtd_info *mtd;
+ u32 pri_offset;
+ u32 sec_offset;
+};
+
+enum fwu_mtd_op {
+ FWU_MTD_READ,
+ FWU_MTD_WRITE,
+};
+
+#define FWU_MDATA_PRIMARY true
+#define FWU_MDATA_SECONDARYfalse
+
+static bool mtd_is_aligned_with_block_size(struct mtd_info *mtd, u64 size)
+{
+ return !do_div(size, mtd->erasesize);
+}
+
+static int mtd_io_data(struct mtd_info *mtd, u32 offs, u32 size, void *data,
+ enum fwu_mtd_op op)
+{
+ struct mtd_oob_ops io_op ={};
+ u64 lock_offs, lock_len;
+ size_t len;
+ void *buf;
+ int ret;
+
+ if (!mtd_is_aligned_with_block_size(mtd, offs)) {
+ log_err("Offset unaligned with a block (0x%x)\n",
mtd->erasesize);
+ return -EINVAL;
+ }
+
+ lock_offs = offs;
+ /* This will expand erase size to align with the block size */
+ lock_len = round_up(size, mtd->erasesize);
+
+ ret = mtd_unlock(mtd, lock_offs, lock_len);
+ if (ret && ret != -EOPNOTSUPP)
+ return ret;
+
+ if (op == FWU_MTD_WRITE) {
+ struct erase_info erase_op = {};
+
+ erase_op.mtd = mtd;
+ erase_op.addr = lock_offs;
+ erase_op.len = lock_len;
+ erase_op.scrub = 0;
+
+ ret = mtd_erase(mtd, &erase_op);
+ if (ret)
+ goto lock;
+ }
+
+ /* Also, expand the write size to align with the write size */
+ len = round_up(size, mtd->writesize);
+
+ buf = memalign(ARCH_DMA_MINALIGN, len);
+ if (!buf) {
+ ret = -ENOMEM;
+ goto lock;
+ }
+ memset(buf, 0xff, len);
+
+ io_op.mode = MTD_OPS_AUTO_OOB;
+ io_op.len = len;
+ io_op.ooblen = 0;
+ io_op.datbuf = buf;
+ io_op.oobbuf = NULL;
+
+ if (op == FWU_MTD_WRITE) {
+ memcpy(buf, data, size);
+ ret = mtd_write_oob(mtd, offs, &io_op);
+ } else {
+ ret = mtd_read_oob(mtd, offs, &io_op);
+ if (!ret)
+ memcpy(data, buf, size);
+ }
+ free(buf);
+
+lock:
+ mtd_lock(mtd, lock_offs, lock_len);
+
+ return ret;
+}
+
+static int fwu_mtd_load_mdata(struct mtd_info *mtd, struct fwu_mdata *mdata,
+ u32 offs, bool primary)
+{
+ size_t size = sizeof(struct fwu_mdata);
+ int ret;
+
+ ret
