Re: [PATCH v5 1/6] FWU: Add FWU metadata access driver for MTD storage regions

Hello Jassi,

Few comments below.

On Tue, 11 Apr 2023 at 01:03, jaswinder.singh@linaro.org wrote:

From: Masami Hiramatsu masami.hiramatsu@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.hiramatsu@linaro.org Signed-off-by: Jassi Brar jaswinder.singh@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