Hello Jassi,
Few comments below.
On Tue, 11 Apr 2023 at 01:03, <jaswinder.si...@linaro.org> wrote:
>
> From: Masami Hiramatsu <masami.hirama...@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.
> The code is divided into core under drivers/fwu-mdata/ and some helper
> functions clubbed together under lib/fwu_updates/
>
> Signed-off-by: Masami Hiramatsu <masami.hirama...@linaro.org>
> Signed-off-by: Jassi Brar <jaswinder.si...@linaro.org>
> ---
> drivers/fwu-mdata/Kconfig | 15 ++
> drivers/fwu-mdata/Makefile | 1 +
> drivers/fwu-mdata/raw_mtd.c | 272 ++++++++++++++++++++++++++++++++++++
> include/fwu.h | 32 +++++
> lib/fwu_updates/Makefile | 1 +
> lib/fwu_updates/fwu_mtd.c | 185 ++++++++++++++++++++++++
> 6 files changed, 506 insertions(+)
> create mode 100644 drivers/fwu-mdata/raw_mtd.c
> create mode 100644 lib/fwu_updates/fwu_mtd.c
>
> diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig
> index 36c4479a59..42736a5e43 100644
> --- a/drivers/fwu-mdata/Kconfig
> +++ b/drivers/fwu-mdata/Kconfig
> @@ -6,6 +6,11 @@ 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"
> select PARTITION_TYPE_GUID
> @@ -14,3 +19,13 @@ config FWU_MDATA_GPT_BLK
> 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 0000000000..25c1aa33ec
> --- /dev/null
> +++ b/drivers/fwu-mdata/raw_mtd.c
> @@ -0,0 +1,272 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2023, Linaro Limited
> + */
> +
> +#define LOG_CATEGORY UCLASS_FWU_MDATA
> +
> +#include <fwu.h>
> +#include <fwu_mdata.h>
> +#include <memalign.h>
> +
I think you can remove the empty line.
> +#include <linux/errno.h>
> +#include <linux/types.h>
> +
> +/* Internal helper structure to move data around */
> +struct fwu_mdata_mtd_priv {
> + struct mtd_info *mtd;
> + char pri_label[50];
> + char sec_label[50];
50 chars is always enough? Maybe allocate buffer when size is known,
e.g. with strdup().
> + u32 pri_offset;
> + u32 sec_offset;
> +};
> +
> +enum fwu_mtd_op {
> + FWU_MTD_READ,
> + FWU_MTD_WRITE,
> +};
> +
> +extern struct fwu_mtd_image_info fwu_mtd_images[];
> +
> +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;
lock_offs and offs are the same. Could drop lock_offs and use only 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;
Could drop io_op.ooblen = 0 and 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_read_mdata(struct udevice *dev, struct fwu_mdata *mdata,
> bool primary)
> +{
> + struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> + struct mtd_info *mtd = mtd_priv->mtd;
> + u32 offs = primary ? mtd_priv->pri_offset : mtd_priv->sec_offset;
> +
> + return mtd_io_data(mtd, offs, sizeof(struct fwu_mdata), mdata,
> FWU_MTD_READ);
> +}
> +
> +static int fwu_mtd_write_mdata(struct udevice *dev, struct fwu_mdata *mdata,
> bool primary)
> +{
> + struct fwu_mdata_mtd_priv *mtd_priv = dev_get_priv(dev);
> + struct mtd_info *mtd = mtd_priv->mtd;
> + u32 offs = primary ? mtd_priv->pri_offset : mtd_priv->sec_offset;
> +
> + return mtd_io_data(mtd, offs, sizeof(struct fwu_mdata), mdata,
> FWU_MTD_WRITE);
> +}
> +
> +static int flash_partition_offset(struct udevice *dev, const char
> *part_name, fdt_addr_t *offset)
> +{
> + ofnode node, parts_node;
> + fdt_addr_t size = 0;
> +
> + parts_node = ofnode_by_compatible(dev_ofnode(dev),
> "fixed-partitions");
> + node = ofnode_by_prop_value(parts_node, "label", part_name,
> strlen(part_name) + 1);
> + if (!ofnode_valid(node)) {
> + log_err("Warning: Failed to find partition by label <%s>\n",
> part_name);
> + return -ENOENT;
> + }
> +
> + *offset = ofnode_get_addr_size_index_notrans(node, 0, &size);
> +
> + return (int)size;
> +}
> +
> +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;
> + const char *label;
> + fdt_addr_t offset;
> + int ret, size;
> + u32 phandle;
> + ofnode bank;
> + int off_img;
> +
> + /* Find the FWU mdata storage device */
> + phandle_p = ofnode_get_property(dev_ofnode(dev),
> + "fwu-mdata-store", &size);
> + if (!phandle_p) {
> + log_err("FWU meta data store not defined in device-tree\n");
> + return -ENOENT;
> + }
> +
> + phandle = fdt32_to_cpu(*phandle_p);
> +
> + ret = device_get_global_by_ofnode(ofnode_get_by_phandle(phandle),
> + &mtd_dev);
> + if (ret) {
> + log_err("FWU: failed to get mtd device\n");
> + return ret;
> + }
> +
> + 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 -ENODEV;
> + }
> +
> + /* Get the offset of primary and secondary mdata */
> + ret = ofnode_read_string_index(dev_ofnode(dev), "mdata-parts", 0,
> &label);
> + if (ret)
> + return ret;
> + strcpy(mtd_priv->pri_label, label);
should use strncpy().
> +
> + ret = flash_partition_offset(mtd_dev, mtd_priv->pri_label, &offset);
> + if (ret <= 0)
> + return ret;
> + mtd_priv->pri_offset = offset;
> +
> + ret = ofnode_read_string_index(dev_ofnode(dev), "mdata-parts", 1,
> &label);
> + if (ret)
> + return ret;
> + strcpy(mtd_priv->sec_label, label);
> +
> + ret = flash_partition_offset(mtd_dev, mtd_priv->sec_label, &offset);
> + if (ret <= 0)
> + return ret;
> + mtd_priv->sec_offset = offset;
> +
> + off_img = 0;
> +
> + ofnode_for_each_subnode(bank, dev_ofnode(dev)) {
> + int bank_num, bank_offset, bank_size;
> + const char *bank_name;
> + ofnode image;
> +
> + ofnode_read_u32(bank, "id", &bank_num);
> + bank_name = ofnode_read_string(bank, "label");
> + bank_size = flash_partition_offset(mtd_dev, bank_name,
> &offset);
> + if (bank_size <= 0)
> + return bank_size;
> + bank_offset = offset;
> + log_debug("Bank%d: %s [0x%x - 0x%x]\n",
> + bank_num, bank_name, bank_offset, bank_offset +
> bank_size);
> +
> + ofnode_for_each_subnode(image, bank) {
> + int image_num, image_offset, image_size;
> + const char *uuid;
> +
> + if (off_img == CONFIG_FWU_NUM_BANKS *
> +
> CONFIG_FWU_NUM_IMAGES_PER_BANK) {
> + log_err("DT provides images more than
> configured!\n");
nitpicking: s/images more/more images/
> + break;
> + }
> +
> + uuid = ofnode_read_string(image, "uuid");
> + ofnode_read_u32(image, "id", &image_num);
> + ofnode_read_u32(image, "offset", &image_offset);
> + ofnode_read_u32(image, "size", &image_size);
> +
> + fwu_mtd_images[off_img].start = bank_offset +
> image_offset;
> + fwu_mtd_images[off_img].size = image_size;
> + fwu_mtd_images[off_img].bank_num = bank_num;
> + fwu_mtd_images[off_img].image_num = image_num;
> + strcpy(fwu_mtd_images[off_img].uuidbuf, uuid);
> + log_debug("\tImage%d: %s @0x%x\n\n",
> + image_num, uuid, bank_offset +
> image_offset);
> + off_img++;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int fwu_mdata_mtd_probe(struct udevice *dev)
> +{
> + /* Ensure the metadata can be read. */
> + return fwu_get_mdata(NULL);
> +}
> +
> +static struct fwu_mdata_ops fwu_mtd_ops = {
> + .read_mdata = fwu_mtd_read_mdata,
> + .write_mdata = fwu_mtd_write_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),
> +};
> diff --git a/include/fwu.h b/include/fwu.h
> index 33b4d0b3db..edb7e9da90 100644
> --- a/include/fwu.h
> +++ b/include/fwu.h
> @@ -8,6 +8,8 @@
>
> #include <blk.h>
> #include <efi.h>
> +#include <mtd.h>
> +#include <uuid.h>
>
> #include <linux/types.h>
>
> @@ -18,6 +20,12 @@ struct fwu_mdata_gpt_blk_priv {
> struct udevice *blk_dev;
> };
>
> +struct fwu_mtd_image_info {
> + u32 start, size;
> + int bank_num, image_num;
1 field per line in structure definition?
> + char uuidbuf[UUID_STR_LEN + 1];
> +};
> +
> struct fwu_mdata_ops {
> /**
> * read_mdata() - Populate the asked FWU metadata copy
> @@ -251,4 +259,28 @@ u8 fwu_empty_capsule_checks_pass(void);
> */
> int fwu_trial_state_ctr_start(void);
>
> +/**
> + * fwu_gen_alt_info_from_mtd() - Parse dfu_alt_info from metadata in mtd
> + * @buf: Buffer into which the dfu_alt_info is filled
> + * @len: Maximum characters that can be written in buf
> + * @mtd: Pointer to underlying MTD device
> + *
> + * Parse dfu_alt_info from metadata in mtd. Used for setting the env.
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int fwu_gen_alt_info_from_mtd(char *buf, size_t len, struct mtd_info *mtd);
> +
> +/**
> + * fwu_mtd_get_alt_num() - Mapping of fwu_plat_get_alt_num for MTD device
> + * @image_guid: Image GUID for which DFU alt number needs to be retrieved
> + * @alt_num: Pointer to the alt_num
> + * @mtd_dev: Name of mtd device instance
> + *
> + * To map fwu_plat_get_alt_num onto mtd based metadata implementation.
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int fwu_mtd_get_alt_num(efi_guid_t *image_guid, u8 *alt_num, const char
> *mtd_dev);
> +
> #endif /* _FWU_H_ */
> diff --git a/lib/fwu_updates/Makefile b/lib/fwu_updates/Makefile
> index 1993088e5b..c9e3c06b48 100644
> --- a/lib/fwu_updates/Makefile
> +++ b/lib/fwu_updates/Makefile
> @@ -5,3 +5,4 @@
>
> obj-$(CONFIG_FWU_MULTI_BANK_UPDATE) += fwu.o
> obj-$(CONFIG_FWU_MDATA_GPT_BLK) += fwu_gpt.o
> +obj-$(CONFIG_FWU_MDATA_MTD) += fwu_mtd.o
> diff --git a/lib/fwu_updates/fwu_mtd.c b/lib/fwu_updates/fwu_mtd.c
> new file mode 100644
> index 0000000000..f22c8b9443
> --- /dev/null
> +++ b/lib/fwu_updates/fwu_mtd.c
> @@ -0,0 +1,185 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2023, Linaro Limited
> + */
> +
> +#include <dm.h>
> +#include <dfu.h>
> +#include <fwu.h>
> +#include <fwu_mdata.h>
> +#include <log.h>
> +#include <malloc.h>
> +#include <mtd.h>
> +#include <uuid.h>
> +#include <vsprintf.h>
> +
> +#include <dm/ofnode.h>
> +
> +struct fwu_mtd_image_info
> +fwu_mtd_images[CONFIG_FWU_NUM_BANKS * CONFIG_FWU_NUM_IMAGES_PER_BANK];
> +
> +static struct fwu_mtd_image_info *mtd_img_by_uuid(const char *uuidbuf)
> +{
> + int num_images = ARRAY_SIZE(fwu_mtd_images);
> +
> + for (int i = 0; i < num_images; i++)
> + if (!strcmp(uuidbuf, fwu_mtd_images[i].uuidbuf))
> + return &fwu_mtd_images[i];
> +
> + return NULL;
> +}
> +
> +int fwu_mtd_get_alt_num(efi_guid_t *image_id, u8 *alt_num,
> + const char *mtd_dev)
> +{
> + struct fwu_mtd_image_info *mtd_img_info;
> + char uuidbuf[UUID_STR_LEN + 1];
> + fdt_addr_t offset, size = 0;
> + struct dfu_entity *dfu;
> + int i, nalt, ret;
> +
> + mtd_probe_devices();
> +
> + uuid_bin_to_str(image_id->b, uuidbuf, UUID_STR_FORMAT_STD);
> +
> + mtd_img_info = mtd_img_by_uuid(uuidbuf);
> + if (!mtd_img_info) {
> + log_err("%s: Not found partition for image %s\n", __func__,
> uuidbuf);
> + return -ENOENT;
> + }
> +
> + offset = mtd_img_info->start;
> + size = mtd_img_info->size;
> +
> + ret = dfu_init_env_entities(NULL, NULL);
> + if (ret)
> + return -ENOENT;
> +
> + nalt = 0;
> + list_for_each_entry(dfu, &dfu_list, list)
> + nalt++;
> +
> + if (!nalt) {
> + log_warning("No entities in dfu_alt_info\n");
> + dfu_free_entities();
> + return -ENOENT;
> + }
> +
> + ret = -1;
ret = -ENOENT ?
> + for (i = 0; i < nalt; i++) {
> + dfu = dfu_get_entity(i);
> +
> + /* Only MTD RAW access */
> + if (!dfu || dfu->dev_type != DFU_DEV_MTD ||
> + dfu->layout != DFU_RAW_ADDR ||
> + dfu->data.mtd.start != offset ||
> + dfu->data.mtd.size != size)
Indentation
> + continue;
> +
> + *alt_num = dfu->alt;
> + ret = 0;
> + break;
> + }
> +
> + dfu_free_entities();
> +
> + log_debug("%s: %s -> %d\n", __func__, uuidbuf, *alt_num);
> + return ret;
> +}
> +
> +/**
> + * fwu_plat_get_alt_num() - Get the DFU Alt Num for the image from the
> platform
> + * @dev: FWU device
> + * @image_id: Image GUID for which DFU alt number needs to be retrieved
> + * @alt_num: Pointer to the alt_num
> + *
> + * Get the DFU alt number from the platform for the image specified by the
> + * image GUID.
> + *
> + * Note: This is a weak function and platforms can override this with
> + * their own implementation for obtaining the alt number value.
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +__weak int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_id,
> + u8 *alt_num)
> +{
> + return fwu_mtd_get_alt_num(image_id, alt_num, "nor1");
> +}
> +
> +static int gen_image_alt_info(char *buf, size_t len, int sidx,
> + struct fwu_image_entry *img, struct mtd_info
> *mtd)
> +{
> + char *p = buf, *end = buf + len;
> + int i;
> +
> + p += snprintf(p, end - p, "mtd %s", mtd->name);
> + if (end < p) {
> + log_err("%s:%d Run out of buffer\n", __func__, __LINE__);
> + return -E2BIG;
> + }
> +
> + /*
> + * List the image banks in the FWU mdata and search the corresponding
> + * partition based on partition's uuid.
> + */
> + for (i = 0; i < CONFIG_FWU_NUM_BANKS; i++) {
> + struct fwu_mtd_image_info *mtd_img_info;
> + struct fwu_image_bank_info *bank;
> + char uuidbuf[UUID_STR_LEN + 1];
> + u32 offset, size;
> +
> + /* Query a partition by image UUID */
> + bank = &img->img_bank_info[i];
> + uuid_bin_to_str(bank->image_uuid.b, uuidbuf,
> UUID_STR_FORMAT_STD);
> +
> + mtd_img_info = mtd_img_by_uuid(uuidbuf);
> + if (!mtd_img_info) {
> + log_err("%s: Not found partition for image %s\n",
> __func__, uuidbuf);
> + break;
> + }
> +
> + offset = mtd_img_info->start;
> + size = mtd_img_info->size;
> +
> + p += snprintf(p, end - p, "%sbank%d raw %x %x",
> + i == 0 ? "=" : ";", i, offset, size);
> + if (end < p) {
> + log_err("%s:%d Run out of buffer\n", __func__,
> __LINE__);
> + return -E2BIG;
> + }
> + }
> +
> + if (i == CONFIG_FWU_NUM_BANKS)
> + return 0;
> +
> + return -ENOENT;
> +}
> +
> +int fwu_gen_alt_info_from_mtd(char *buf, size_t len, struct mtd_info *mtd)
> +{
> + struct fwu_mdata mdata;
> + int i, l, ret;
> +
> + ret = fwu_get_mdata(&mdata);
> + if (ret < 0) {
> + log_err("Failed to get the FWU mdata.\n");
> + return ret;
> + }
> +
> + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) {
> + ret = gen_image_alt_info(buf, len, i * CONFIG_FWU_NUM_BANKS,
> + &mdata.img_entry[i], mtd);
> + if (ret)
> + break;
> +
> + l = strlen(buf);
> + /* Replace the last ';' with '&' if there is another image. */
> + if (i != CONFIG_FWU_NUM_IMAGES_PER_BANK - 1 && l)
> + buf[l - 1] = '&';
> + len -= l;
> + buf += l;
> + }
> +
> + return ret;
> +}
> --
> 2.34.1
>
