[PATCH 2/5] FWU: Add FWU metadata access driver for MTD storage regions

Heinrich Schuchardt xypron.glpk at gmx.de
Thu Sep 1 08:45:19 CEST 2022


On 7/22/22 19:43, jassisinghbrar at gmail.com wrote:
> From: Sughosh Ganu <sughosh.ganu at linaro.org>
>
> 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 <sughosh.ganu at linaro.org>
> Signed-off-by: Jassi Brar <jaswinder.singh at linaro.org>
> ---
>   drivers/fwu-mdata/Kconfig         |   8 +
>   drivers/fwu-mdata/Makefile        |   1 +
>   drivers/fwu-mdata/fwu_mdata_mtd.c | 308 ++++++++++++++++++++++++++++++
>   3 files changed, 317 insertions(+)
>   create mode 100644 drivers/fwu-mdata/fwu_mdata_mtd.c
>
> diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig
> index d5edef19d6..a8fa9ad783 100644
> --- a/drivers/fwu-mdata/Kconfig
> +++ b/drivers/fwu-mdata/Kconfig
> @@ -14,3 +14,11 @@ config FWU_MDATA_GPT_BLK
>   	help
>   	  Enable support for accessing FWU Metadata on GPT partitioned
>   	  block devices.
> +
> +config FWU_MDATA_MTD
> +	bool "FWU Metadata access for non-GPT MTD devices"
> +	depends on DM_FWU_MDATA && MTD
> +	help
> +	  Enable support for accessing FWU Metadata on non-partitioned
> +	  (or non-GPT partitioned, e.g. partition nodes in devicetree)
> +	  MTD devices.
> diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile
> index 313049f67a..58f8023f16 100644
> --- a/drivers/fwu-mdata/Makefile
> +++ b/drivers/fwu-mdata/Makefile
> @@ -5,3 +5,4 @@
>
>   obj-$(CONFIG_DM_FWU_MDATA) += fwu-mdata-uclass.o
>   obj-$(CONFIG_FWU_MDATA_GPT_BLK) += fwu_mdata_gpt_blk.o
> +obj-$(CONFIG_FWU_MDATA_MTD) += fwu_mdata_mtd.o
> diff --git a/drivers/fwu-mdata/fwu_mdata_mtd.c b/drivers/fwu-mdata/fwu_mdata_mtd.c
> new file mode 100644
> index 0000000000..d543a419fd
> --- /dev/null
> +++ b/drivers/fwu-mdata/fwu_mdata_mtd.c
> @@ -0,0 +1,308 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022, Linaro Limited
> + */
> +
> +#include <efi_loader.h>
> +#include <fwu.h>
> +#include <fwu_mdata.h>
> +#include <malloc.h>
> +#include <memalign.h>
> +#include <spi.h>
> +#include <spi_flash.h>
> +#include <flash.h>
> +
> +#include <linux/errno.h>
> +#include <linux/types.h>
> +#include <u-boot/crc.h>
> +

How should a reader know what pri_offset and sec_offset refer to?
Please, provide Sphix style comments describing the structure.

> +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,
> +};
> +

Please, document all functions.

> +static bool /(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)

Most of the functionality of this function should be in the mtd uclass.
It is not FWU specific.

> +{
> +	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))
> +		return -EINVAL;

I think this is the only place where you could write an error message
indicating that misplacement is the reason for the error.

> +	lock_offs = offs;
> +	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 = {};
> +
> +		/* This will expand erase size to align with the block size */

This comment is misplaced. It should be above the round_up() call.

> +		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_out;
> +	}
> +
> +	/* 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_out;
> +	}
> +	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);

Don't copy random bytes to the flash. You have to zero out the padding
bytes.

> +		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_out:

This label sound like you want to lock something out. Please use lock:
instead.

> +	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;
> +
> +	*mdata = malloc(size);
> +	if (!*mdata)
> +		return -ENOMEM;
> +
> +	ret = mtd_io_data(mtd, offs, size, (void *)*mdata, FWU_MTD_READ);

The conversion to (void *) is superfluous as the expected parameter is
of type void *.

> +	if (ret >= 0) {
> +		ret = fwu_verify_mdata(*mdata, primary);
> +		if (ret < 0) {
> +			free(*mdata);
> +			*mdata = NULL;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static int fwu_mtd_load_primary_mdata(struct fwu_mdata_mtd_priv *mtd_priv,
> +				     struct fwu_mdata **mdata)
> +{
> +	return fwu_mtd_load_mdata(mtd_priv->mtd, mdata, mtd_priv->pri_offset, true);

Instead of true and false use an enum like

enum fwu_sec_prim {
	FWU_META_PRIMARY,
	FWU_META_SECONDARY,
}

and get rid of these functions. That will give you the same readability
with less of complexity.

> +}
> +
> +static int fwu_mtd_load_secondary_mdata(struct fwu_mdata_mtd_priv *mtd_priv,
> +				       struct fwu_mdata **mdata)
> +{
> +	return fwu_mtd_load_mdata(mtd_priv->mtd, mdata, mtd_priv->sec_offset, false);
> +}
> +
> +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)
> +{
> +	if (fwu_mtd_load_primary_mdata(mtd_priv, mdata) == 0)

Please, don't use == 0. But "if (!".

> +		return 0;
> +
> +	log_err("Failed to load/verify primary mdata. Try secondary.\n");
> +
> +	if (fwu_mtd_load_secondary_mdata(mtd_priv, mdata) == 0)

ditto

> +		return 0;
> +
> +	log_err("Failed to load/verify secondary mdata.\n");
> +
> +	return -1;
> +}
> +
> +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;
> +
> +	/* Update mdata crc32 field */
> +	mdata->crc32 = crc32(0, (void *)&mdata->version,

Avoid superfluous conversions. There is already a conversion in the
defintion of the crc32 macro.

> +			     sizeof(*mdata) - sizeof(u32));
> +
> +	/* First write the primary mdata */
> +	ret = fwu_mtd_save_primary_mdata(mtd_priv, mdata);
> +	if (ret < 0) {
> +		log_err("Failed to update the primary mdata.\n");
> +		return ret;
> +	}
> +
> +	/* And now the replica */
> +	ret = fwu_mtd_save_secondary_mdata(mtd_priv, mdata);
> +	if (ret < 0) {
> +		log_err("Failed to update the secondary mdata.\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fwu_mtd_mdata_check(struct udevice *dev)
> +{
> +	struct fwu_mdata *primary = NULL, *secondary = NULL;
> +	struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> +	int ret;
> +
> +	ret = fwu_mtd_load_primary_mdata(mtd_priv, &primary);
> +	if (ret < 0)
> +		log_err("Failed to read the primary mdata: %d\n", ret);
> +
> +	ret = fwu_mtd_load_secondary_mdata(mtd_priv, &secondary);
> +	if (ret < 0)
> +		log_err("Failed to read the secondary mdata: %d\n", ret);
> +
> +	if (primary && secondary) {
> +		if (memcmp(primary, secondary, sizeof(struct fwu_mdata))) {
> +			log_err("The primary and the secondary mdata are different\n");
> +			ret = -1;
> +		}
> +	} else if (primary) {
> +		ret = fwu_mtd_save_secondary_mdata(mtd_priv, primary);
> +		if (ret < 0)
> +			log_err("Restoring secondary mdata partition failed\n");
> +	} else if (secondary) {
> +		ret = fwu_mtd_save_primary_mdata(mtd_priv, secondary);
> +		if (ret < 0)
> +			log_err("Restoring primary mdata partition failed\n");
> +	}

If neither primary nor secondary data is available, you are happy???

> +
> +	free(primary);
> +	free(secondary);
> +	return ret;
> +}
> +
> +static int fwu_mtd_get_mdata(struct udevice *dev, struct fwu_mdata **mdata)
> +{
> +	struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> +
> +	return fwu_mtd_get_valid_mdata(mtd_priv, mdata);
> +}
> +
> +/**
> + * fwu_mdata_mtd_of_to_plat() - Translate from DT to fwu mdata device
> + */
> +static int fwu_mdata_mtd_of_to_plat(struct udevice *dev)
> +{
> +	struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> +	const fdt32_t *phandle_p = NULL;
> +	struct udevice *mtd_dev;
> +	struct mtd_info *mtd;
> +	int ret, size;
> +	u32 phandle;
> +
> +	/* Find the FWU mdata storage device */
> +	phandle_p = ofnode_get_property(dev_ofnode(dev),
> +					"fwu-mdata-store", &size);
> +	if (!phandle_p) {
> +		log_err("fwu-mdata-store property not found\n");

A user needs to know that the problem is in the device-tree, e.g.
"FWU meta data store not defined in device-tree".

> +		return -ENOENT;
> +	}
> +
> +	phandle = fdt32_to_cpu(*phandle_p);
> +
> +	ret = device_get_global_by_ofnode(
> +		ofnode_get_by_phandle(phandle),
> +		&mtd_dev);
> +	if (ret)
> +		return ret;

No log message?

> +
> +	mtd_probe_devices();
> +
> +	mtd_for_each_device(mtd) {
> +		if (mtd->dev == mtd_dev) {
> +			mtd_priv->mtd = mtd;
> +			log_debug("Found the FWU mdata mtd device %s\n", mtd->name);
> +			break;
> +		}
> +	}
> +	if (!mtd_priv->mtd) {
> +		log_err("Failed to find mtd device by fwu-mdata-store\n");
> +		return -ENOENT;
> +	}
> +
> +	/* Get the offset of primary and seconday mdata */
> +	ret = ofnode_read_u32_index(dev_ofnode(dev), "mdata-offsets", 0,
> +				    &mtd_priv->pri_offset);
> +	if (ret)
> +		return ret;
> +	ret = ofnode_read_u32_index(dev_ofnode(dev), "mdata-offsets", 1,
> +				    &mtd_priv->sec_offset);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int fwu_mdata_mtd_probe(struct udevice *dev)
> +{
> +	/* Ensure the metadata can be read. */
> +	return fwu_mtd_mdata_check(dev);

If you don't like the name of fwu_mtd_mdata_check(), change it instead
of wrapping it in another function.

Best regards

Heinrich

> +}
> +
> +static struct fwu_mdata_ops fwu_mtd_ops = {
> +	.mdata_check = fwu_mtd_mdata_check,
> +	.get_mdata = fwu_mtd_get_mdata,
> +	.update_mdata = fwu_mtd_update_mdata,
> +};
> +
> +static const struct udevice_id fwu_mdata_ids[] = {
> +	{ .compatible = "u-boot,fwu-mdata-mtd" },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(fwu_mdata_mtd) = {
> +	.name		= "fwu-mdata-mtd",
> +	.id		= UCLASS_FWU_MDATA,
> +	.of_match	= fwu_mdata_ids,
> +	.ops		= &fwu_mtd_ops,
> +	.probe		= fwu_mdata_mtd_probe,
> +	.of_to_plat	= fwu_mdata_mtd_of_to_plat,
> +	.priv_auto	= sizeof(struct fwu_mdata_mtd_priv),
> +};



More information about the U-Boot mailing list