[PATCH 00/11] imagemap: fixes from the v1 review

[PATCH 00/11] imagemap: fixes from the v1 review

[Daniel Golle]

Simon Glass reviewed the v1 posting of the imagemap series
(cover.1787272661.git.daniel@makrotopia.org) after v2 had already been
applied, so his comments arrive against code that is now in next. This
series carries the correctness half of them; a second series follows
with the structural and cosmetic half.

Tested on sandbox: every commit builds, "ut imagemap" passes 13/13 and
survives "ut -r3", and a full test.py run shows the same 11 failures and
20 errors as unmodified next, all of them EFI-secboot tooling, ext4 and
config-rebuild tests unrelated to this code. Also built with
CONFIG_IMAGEMAP=n.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>

Daniel Golle (11):
  mtd: bind the block device without leaking on failure
  cmd: ubi: report a failed UBI block device bind
  cmd: ubi: unbind the block device when detaching UBI
  boot: imagemap: free the reservation a replaced record owned
  boot: imagemap: do not resize a region the caller owns
  boot: imagemap: read only the bytes an extend adds
  boot: fit: unmap the load address after a storage read
  boot: fit: keep the load message for RAM-backed images
  boot: bootm: release the imagemap on every failing exit
  test: boot: imagemap: detach UBI when the test is done
  test: boot: imagemap: reset driver-model state between tests

 boot/bootm.c            |  9 +++---
 boot/image-fit.c        |  7 +++--
 boot/imagemap.c         | 55 ++++++++++++++++++++++++---------
 cmd/ubi.c               | 67 +++++++++++++++++++++++++++--------------
 drivers/mtd/mtdcore.c   | 37 ++++++++++++++++-------
 drivers/mtd/ubi/block.c |  4 ++-
 include/ubi_uboot.h     |  4 +--
 test/boot/imagemap.c    | 36 ++++++++++++++--------
 8 files changed, 147 insertions(+), 72 deletions(-)


base-commit: 2b8902913c19eb581fb0d567201c09b8fa46ac74
-- 
2.55.0

[PATCH 01/11] mtd: bind the block device without leaking on failure

[Daniel Golle]

add_mtd_device() allocates a slot to hold the mtd_info pointer that
mtd_bind() keeps, but drops the return value. mtd_bind() only logs and
returns on failure, so the slot leaks. A failed kmalloc() is ignored
too, leaving the master without a block device and no hint why its
partitions are unreachable.

Move the bind into a helper that frees the slot when mtd_bind() fails
and warns with the errno in both cases.

Fixes: e8a7453824b7 ("mtd: bind an mtd_blk device for non-NAND MTD masters")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/mtd/mtdcore.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index e0d0b5ae2e9..3d3a84df0a6 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -396,6 +396,31 @@ static struct device_type mtd_devtype = {
 };
 #endif
 
+static void mtd_blk_bind_master(struct mtd_info *mtd)
+{
+	struct mtd_info **mtdp;
+	int ret;
+
+	/*
+	 * mtd_bind() keeps the passed pointer, so it needs storage that lives
+	 * as long as the block device; an MTD master is never removed.
+	 */
+	mtdp = kmalloc(sizeof(*mtdp), GFP_KERNEL);
+	if (!mtdp) {
+		ret = -ENOMEM;
+		goto warn;
+	}
+
+	*mtdp = mtd;
+	ret = mtd_bind(mtd->dev, mtdp);
+	if (!ret)
+		return;
+
+	kfree(mtdp);
+warn:
+	pr_warn("mtd: %s: cannot bind a block device: %d\n", mtd->name, ret);
+}
+
 /**
  *	add_mtd_device - register an MTD device
  *	@mtd: pointer to new MTD device info structure
@@ -504,18 +529,8 @@ int add_mtd_device(struct mtd_info *mtd)
 		 * NAND is excluded: it needs UBI, which brings its own block
 		 * device. Partitions inherit the master's block device via the
 		 * "mtd" partition type, so only masters are bound.
-		 *
-		 * mtd_bind() keeps a pointer to the passed mtd_info pointer, so
-		 * it needs storage that lives as long as the block device; the
-		 * MTD master is never removed in practice, so a small heap slot
-		 * is fine.
 		 */
-		struct mtd_info **mtdp = kmalloc(sizeof(*mtdp), GFP_KERNEL);
-
-		if (mtdp) {
-			*mtdp = mtd;
-			mtd_bind(mtd->dev, mtdp);
-		}
+		mtd_blk_bind_master(mtd);
 	}
 
 	/* We _know_ we aren't being removed, because
-- 
2.55.0

[PATCH 02/11] cmd: ubi: report a failed UBI block device bind

[Daniel Golle]

ubi_blk_bind_once() drops the ubi_bind() return value, so when the bind
fails the attach still reports success and the caller finds no ubi_blk
device with nothing to explain it.

Report the errno. The attach itself is not failed: UBI is available
either way and the block device is an optional view of it, so a UBIFS
user must not lose the partition because the block layer could not be
set up.

Fixes: dec405d1653a ("cmd: ubi: create a ubi_blk device when attaching UBI")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 cmd/ubi.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/cmd/ubi.c b/cmd/ubi.c
index 91ba21504f4..7cbb3ee8668 100644
--- a/cmd/ubi.c
+++ b/cmd/ubi.c
@@ -676,7 +676,9 @@ int ubi_detach(void)
 #if CONFIG_IS_ENABLED(UBI_BLOCK)
 static void ubi_blk_bind_once(void)
 {
+	struct udevice *parent;
 	struct udevice *dev;
+	int ret;
 
 	/*
 	 * A single ubi_blk device serves all volumes of the attached UBI
@@ -690,7 +692,11 @@ static void ubi_blk_bind_once(void)
 			return;
 	}
 
-	ubi_bind(ubi && ubi->mtd && ubi->mtd->dev ? ubi->mtd->dev : dm_root());
+	parent = ubi && ubi->mtd && ubi->mtd->dev ? ubi->mtd->dev : dm_root();
+
+	ret = ubi_bind(parent);
+	if (ret)
+		printf("Cannot create UBI block device: %d\n", ret);
 }
 #else
 static inline void ubi_blk_bind_once(void) { }
-- 
2.55.0

[PATCH 03/11] cmd: ubi: unbind the block device when detaching UBI

[Daniel Golle]

The ubi_blk device is parented to the MTD device UBI was attached to,
but ubi_detach() leaves it bound. After 'ubi part A; ubi detach; ubi
part B' the device still hangs off A's MTD while UBI sits on B, so the
driver-model tree describes a parent that is no longer in use and A's
MTD can no longer be removed. Reads happen to keep working only because
get_ubi_device() always returns ubi_devices[0].

Return the new device from ubi_bind(), remember it, and unbind it in
ubi_detach() so the next attach binds a fresh one under the right
parent. Tracking the device also replaces the scan of every UCLASS_BLK
device that was used to find it.

Fixes: dec405d1653a ("cmd: ubi: create a ubi_blk device when attaching UBI")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 cmd/ubi.c               | 73 +++++++++++++++++++++++++----------------
 drivers/mtd/ubi/block.c |  4 ++-
 include/ubi_uboot.h     |  4 +--
 3 files changed, 49 insertions(+), 32 deletions(-)

diff --git a/cmd/ubi.c b/cmd/ubi.c
index 7cbb3ee8668..ca8184aa736 100644
--- a/cmd/ubi.c
+++ b/cmd/ubi.c
@@ -21,6 +21,7 @@
 #include <nand.h>
 #include <onenand_uboot.h>
 #include <dm/device.h>
+#include <dm/device-internal.h>
 #include <dm/devres.h>
 #include <dm/root.h>
 #include <dm/uclass-internal.h>
@@ -650,6 +651,47 @@ static int ubi_set_skip_check(const char *volume, bool skip_check)
 	return ubi_change_vtbl_record(ubi, vol->vol_id, &vtbl_rec);
 }
 
+#if CONFIG_IS_ENABLED(UBI_BLOCK)
+static struct udevice *ubi_blk_dev;
+
+static void ubi_blk_bind_once(void)
+{
+	struct udevice *parent;
+	int ret;
+
+	/*
+	 * A single ubi_blk device serves all volumes of the attached UBI
+	 * device, the volume being selected through the block descriptor's
+	 * hwpart. It is parented to the MTD device UBI sits on, so it must
+	 * not outlive the attach that created it.
+	 */
+	if (ubi_blk_dev)
+		return;
+
+	parent = ubi && ubi->mtd && ubi->mtd->dev ? ubi->mtd->dev : dm_root();
+
+	ret = ubi_bind(parent, &ubi_blk_dev);
+	if (ret)
+		printf("Cannot create UBI block device: %d\n", ret);
+}
+
+static void ubi_blk_unbind(void)
+{
+	if (!ubi_blk_dev)
+		return;
+
+	device_remove(ubi_blk_dev, DM_REMOVE_NORMAL);
+	device_unbind(ubi_blk_dev);
+	ubi_blk_dev = NULL;
+}
+#else
+static inline void ubi_blk_bind_once(void) { }
+
+static inline void ubi_blk_unbind(void)
+{
+}
+#endif
+
 int ubi_detach(void)
 {
 #ifdef CONFIG_CMD_UBIFS
@@ -662,6 +704,8 @@ int ubi_detach(void)
 		cmd_ubifs_umount();
 #endif
 
+	ubi_blk_unbind();
+
 	/*
 	 * Call ubi_exit() before re-initializing the UBI subsystem
 	 */
@@ -673,35 +717,6 @@ int ubi_detach(void)
 	return 0;
 }
 
-#if CONFIG_IS_ENABLED(UBI_BLOCK)
-static void ubi_blk_bind_once(void)
-{
-	struct udevice *parent;
-	struct udevice *dev;
-	int ret;
-
-	/*
-	 * A single ubi_blk device serves all volumes of the attached UBI
-	 * device, the volume being selected through the block descriptor's
-	 * hwpart. Bind one when UBI is attached -- unless a previous attach
-	 * already did -- parented to the MTD device UBI sits on.
-	 */
-	for (uclass_find_first_device(UCLASS_BLK, &dev); dev;
-	     uclass_find_next_device(&dev)) {
-		if (dev->driver == DM_DRIVER_GET(ubi_blk))
-			return;
-	}
-
-	parent = ubi && ubi->mtd && ubi->mtd->dev ? ubi->mtd->dev : dm_root();
-
-	ret = ubi_bind(parent);
-	if (ret)
-		printf("Cannot create UBI block device: %d\n", ret);
-}
-#else
-static inline void ubi_blk_bind_once(void) { }
-#endif
-
 int ubi_part(const char *part_name, const char *vid_header_offset)
 {
 	struct mtd_info *mtd;
diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index 99d55282cd7..f5a79e4f475 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -11,7 +11,7 @@
 #include <dm/device.h>
 #include <dm/device-internal.h>
 
-int ubi_bind(struct udevice *dev)
+int ubi_bind(struct udevice *dev, struct udevice **bdevp)
 {
 	struct blk_desc *bdesc;
 	struct udevice *bdev;
@@ -29,6 +29,8 @@ int ubi_bind(struct udevice *dev)
 	bdesc->bdev = bdev;
 	bdesc->part_type = PART_TYPE_UBI;
 
+	*bdevp = bdev;
+
 	return 0;
 }
 
diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
index bdf5645de3d..9faecc3870c 100644
--- a/include/ubi_uboot.h
+++ b/include/ubi_uboot.h
@@ -148,9 +148,9 @@ int cmd_ubifs_mount(const char *vol_name);
 int cmd_ubifs_umount(void);
 
 #if IS_ENABLED(CONFIG_UBI_BLOCK)
-int ubi_bind(struct udevice *dev);
+int ubi_bind(struct udevice *dev, struct udevice **bdevp);
 #else
-static inline int ubi_bind(struct udevice *dev)
+static inline int ubi_bind(struct udevice *dev, struct udevice **bdevp)
 {
 	return -EOPNOTSUPP;
 }
-- 
2.55.0

[PATCH 04/11] boot: imagemap: free the reservation a replaced record owned

[Daniel Golle]

imagemap_record() overwrites an entry that already covers the same image
offset, including its lmb_reserved flag. When imagemap_map_to() replaces
a region that imagemap_map() had allocated from LMB, the reservation is
forgotten with the address that owned it, so imagemap_cleanup() walks a
table that no longer mentions it and the memory stays reserved for the
rest of the boot.

Release the reservation before the entry is pointed at different memory.

Fixes: 9b41c7dbee96 ("boot: add imagemap on-demand loading from storage")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 boot/imagemap.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/boot/imagemap.c b/boot/imagemap.c
index 9e77ed25d00..89396a03730 100644
--- a/boot/imagemap.c
+++ b/boot/imagemap.c
@@ -114,12 +114,22 @@ imagemap_record(struct udevice *dev, loff_t img_offset, ulong size,
 
 	/* Check for an existing entry at the same base that we can extend */
 	alist_for_each(r, &priv->regions) {
-		if (r->img_offset == img_offset) {
-			r->size = size;
-			r->ram = ram;
-			r->lmb_reserved = lmb_reserved;
-			return r;
-		}
+		if (r->img_offset != img_offset)
+			continue;
+
+		/*
+		 * The entry is about to describe different memory, so give up
+		 * the reservation it owns while its address is still known.
+		 */
+		if (r->lmb_reserved && r->ram != ram)
+			lmb_free(map_to_sysmem(r->ram),
+				 ALIGN(r->size, ARCH_DMA_MINALIGN), LMB_NONE);
+
+		r->size = size;
+		r->ram = ram;
+		r->lmb_reserved = lmb_reserved;
+
+		return r;
 	}
 
 	/* Append new region */
-- 
2.55.0

[PATCH 05/11] boot: imagemap: do not resize a region the caller owns

[Daniel Golle]

The extend path reserves memory at the recorded address and re-reads the
region into it. For a region recorded by imagemap_map_to() that address
belongs to the caller, which asked for the payload to be placed there:
imagemap reserves memory it was never given and overwrites whatever the
caller had put at the destination.

Restrict the extend path to regions allocated from LMB by imagemap
itself. A larger request against a caller-owned region now falls through
to a fresh allocation, leaving the caller's memory untouched.

Fixes: 9b41c7dbee96 ("boot: add imagemap on-demand loading from storage")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 boot/imagemap.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/boot/imagemap.c b/boot/imagemap.c
index 89396a03730..a28c7928e46 100644
--- a/boot/imagemap.c
+++ b/boot/imagemap.c
@@ -233,13 +233,18 @@ void *imagemap_map(struct udevice *dev, loff_t img_offset, ulong size)
 	 */
 	alist_for_each(r, &priv->regions) {
 		if (r->img_offset == base_off && r->size < read_size) {
+			/*
+			 * Only a region this device allocated may be resized;
+			 * one recorded by imagemap_map_to() lives at an address
+			 * the caller chose and owns.
+			 */
+			if (!r->lmb_reserved)
+				break;
+
 			addr = map_to_sysmem(r->ram);
 
-			/* Free old LMB reservation if we own it */
-			if (r->lmb_reserved)
-				lmb_free(addr,
-					 ALIGN(r->size, ARCH_DMA_MINALIGN),
-					 LMB_NONE);
+			lmb_free(addr, ALIGN(r->size, ARCH_DMA_MINALIGN),
+				 LMB_NONE);
 
 			/* Try to re-reserve at the same address with new size */
 			if (lmb_alloc_mem(LMB_MEM_ALLOC_ADDR, 0, &addr,
-- 
2.55.0

[PATCH 06/11] boot: imagemap: read only the bytes an extend adds

[Daniel Golle]

Growing a region re-reads it from the start even when the reservation
was renewed at the same address and the leading bytes are still valid.
The translation table exists to avoid re-reading data that is already in
RAM, so the common case of probing a FIT header and then mapping the
whole structure pays for the header twice.

Read only the range the larger request adds when the region has not
moved. A reservation that had to be placed elsewhere still needs the
whole range.

Fixes: 9b41c7dbee96 ("boot: add imagemap on-demand loading from storage")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 boot/imagemap.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/boot/imagemap.c b/boot/imagemap.c
index a28c7928e46..b004aa1644b 100644
--- a/boot/imagemap.c
+++ b/boot/imagemap.c
@@ -213,8 +213,10 @@ void *imagemap_map(struct udevice *dev, loff_t img_offset, ulong size)
 	ulong overhead = img_offset & (bl_len - 1);
 	loff_t base_off = img_offset - overhead;
 	ulong read_size = ALIGN(size + overhead, bl_len);
+	phys_addr_t old_addr;
 	phys_addr_t addr;
 	phys_size_t alloc_size;
+	ulong old_size;
 	void *base;
 	int ret;
 	struct imagemap_region *r;
@@ -241,9 +243,11 @@ void *imagemap_map(struct udevice *dev, loff_t img_offset, ulong size)
 			if (!r->lmb_reserved)
 				break;
 
-			addr = map_to_sysmem(r->ram);
+			old_size = r->size;
+			old_addr = map_to_sysmem(r->ram);
+			addr = old_addr;
 
-			lmb_free(addr, ALIGN(r->size, ARCH_DMA_MINALIGN),
+			lmb_free(addr, ALIGN(old_size, ARCH_DMA_MINALIGN),
 				 LMB_NONE);
 
 			/* Try to re-reserve at the same address with new size */
@@ -261,8 +265,14 @@ void *imagemap_map(struct udevice *dev, loff_t img_offset, ulong size)
 			}
 			base = map_sysmem(addr, alloc_size);
 
-			ret = spl_load_region(&priv->info, img_offset, size,
-					      base);
+			if (addr == old_addr)
+				ret = spl_load_region(&priv->info,
+						      base_off + old_size,
+						      read_size - old_size,
+						      (char *)base + old_size);
+			else
+				ret = spl_load_region(&priv->info, img_offset,
+						      size, base);
 			if (ret < 0) {
 				log_err("imagemap: read failed at offset 0x%llx (size 0x%lx): %d\n",
 					(unsigned long long)img_offset, size, ret);
-- 
2.55.0

[PATCH 07/11] boot: fit: unmap the load address after a storage read

[Daniel Golle]

fit_image_load_storage() maps the load address to get a destination for
imagemap_map_to() and never unmaps it. The mapping is a no-op on most
architectures but not on sandbox, which is where the unit tests run.

Pair the map_sysmem() with an unmap_sysmem() once the read is done.

Fixes: 46d32e38ee4e ("boot: fit: support on-demand loading in fit_image_load()")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 boot/image-fit.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/boot/image-fit.c b/boot/image-fit.c
index 60e47db5fb0..5d86342852c 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -2190,6 +2190,7 @@ static int fit_image_load_storage(struct bootm_headers *images, const void *fit,
 	ulong img_load;
 	u8 img_comp = IH_COMP_NONE;
 	void *mapped;
+	void *dst;
 
 	if (CONFIG_IS_ENABLED(FIT_VERITY)) {
 		u8 img_type;
@@ -2222,10 +2223,11 @@ static int fit_image_load_storage(struct bootm_headers *images, const void *fit,
 
 	if (img_comp == IH_COMP_NONE && load_op != FIT_LOAD_IGNORED &&
 	    !fit_image_get_load(fit, noffset, &img_load)) {
-		void *dst = map_sysmem(img_load, data_sz);
+		dst = map_sysmem(img_load, data_sz);
 
 		mapped = imagemap_map_to(images->imagemap, data_off, data_sz,
 					 dst);
+		unmap_sysmem(dst);
 	} else {
 		mapped = imagemap_map(images->imagemap, data_off, data_sz);
 	}
-- 
2.55.0

[PATCH 08/11] boot: fit: keep the load message for RAM-backed images

[Daniel Golle]

The "Loading ... from ... to ..." line is suppressed whenever
CONFIG_IMAGEMAP is built in and the source happens to equal the load
address. The test is a build-time one, so a board that enables imagemap
but boots a FIT from RAM loses the line as well.

Suppress it only when an imagemap is actually in use, which is the one
case where the data was read straight to its load address and there is
no copy to report.

Fixes: 46d32e38ee4e ("boot: fit: support on-demand loading in fit_image_load()")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 boot/image-fit.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/boot/image-fit.c b/boot/image-fit.c
index 5d86342852c..aacd3fa3622 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -2468,7 +2468,8 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 			return -EXDEV;
 		}
 
-		if (!CONFIG_IS_ENABLED(IMAGEMAP) || data != load)
+		/* A storage-backed image was read straight to its load address */
+		if (!images->imagemap || data != load)
 			printf("   Loading %s from 0x%08lx to 0x%08lx\n",
 			       prop_name, data, load);
 	} else {
-- 
2.55.0

[PATCH 09/11] boot: bootm: release the imagemap on every failing exit

[Daniel Golle]

Three error paths in bootm_run_states() return directly instead of
reaching the err label, so the imagemap device and the LMB reservations
its translation table holds survive a failed bootm: the missing OS boot
function, an unsupported subcommand, and any error carried out of the
earlier states. The first of those also left interrupts disabled.

Route all three through err, which already releases the imagemap and
re-enables interrupts.

Fixes: 46d32e38ee4e ("boot: fit: support on-demand loading in fit_image_load()")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 boot/bootm.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/boot/bootm.c b/boot/bootm.c
index b6ae0163b27..92c8452a5d3 100644
--- a/boot/bootm.c
+++ b/boot/bootm.c
@@ -1265,18 +1265,17 @@ int bootm_run_states(struct bootm_info *bmi, int states)
 
 	/* From now on, we need the OS boot function */
 	if (ret)
-		return ret;
+		goto err;
 	boot_fn = bootm_os_get_boot_func(images->os.os);
 	need_boot_fn = states & (BOOTM_STATE_OS_CMDLINE |
 			BOOTM_STATE_OS_BD_T | BOOTM_STATE_OS_PREP |
 			BOOTM_STATE_OS_FAKE_GO | BOOTM_STATE_OS_GO);
 	if (boot_fn == NULL && need_boot_fn) {
-		if (iflag)
-			enable_interrupts();
 		printf("ERROR: booting os '%s' (%d) is not supported\n",
 		       genimg_get_os_name(images->os.os), images->os.os);
 		bootstage_error(BOOTSTAGE_ID_CHECK_BOOT_OS);
-		return 1;
+		ret = 1;
+		goto err;
 	}
 
 	/* Call various other states that are not generally used */
@@ -1318,7 +1317,7 @@ int bootm_run_states(struct bootm_info *bmi, int states)
 	/* Check for unsupported subcommand. */
 	if (ret) {
 		printf("subcommand failed (err=%d)\n", ret);
-		return ret;
+		goto err;
 	}
 
 	/* Now run the OS! We hope this doesn't return */
-- 
2.55.0

[PATCH 10/11] test: boot: imagemap: detach UBI when the test is done

[Daniel Golle]

The UBI test attaches UBI to the sandbox NAND and leaves it attached, so
UBI still holds a reference to the MTD device when the test ends. Running
the suite a second time crashes in mtd_partitions_used(): the test calls
mtd_probe_devices(), which rebuilds the partition list of a master whose
partitions UBI is still using, and the walk then follows a freed entry.

Detach UBI at the end of the test, and again at the start so a device
left behind by anything else is released before the partitions are
rebuilt.

Fixes: 05c1fbbfa79f ("test: boot: add imagemap unit tests")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 test/boot/imagemap.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/test/boot/imagemap.c b/test/boot/imagemap.c
index da038a33243..20a8f150a48 100644
--- a/test/boot/imagemap.c
+++ b/test/boot/imagemap.c
@@ -592,6 +592,13 @@ static int imagemap_test_ubiblock(struct unit_test_state *uts)
 	u64 off;
 	int i;
 
+	/*
+	 * Release any device a previous run attached: UBI keeps a reference to
+	 * the MTD device, and mtd_probe_devices() would then rebuild the
+	 * partition list behind its back.
+	 */
+	ubi_detach();
+
 	mtd_probe_devices();
 	mtd = get_mtd_device_nm("nand2");
 	ut_assert(!IS_ERR_OR_NULL(mtd));
@@ -638,6 +645,7 @@ static int imagemap_test_ubiblock(struct unit_test_state *uts)
 
 	imagemap_cleanup(imdev);
 	free(wbuf);
+	ubi_detach();
 
 	return 0;
 }
-- 
2.55.0

[PATCH 11/11] test: boot: imagemap: reset driver-model state between tests

[Daniel Golle]

The tests bind and probe devices under dm_root() and the storage tests
depend on real driver-model state, but none of them asks for the DM
init and uninit the test framework provides. A test that fails before
imagemap_cleanup() therefore leaves its bound device behind, and the next
one trips over the name clash in device_bind_driver().

Pass UTF_DM throughout, and UTF_SCAN_FDT for the two tests that walk
device-tree partitions. Those two are also marked UTF_LIVE_TREE: they
drive mtd_probe_devices(), which cannot parse the partitions of a master
bound from a flat tree.

Fixes: 05c1fbbfa79f ("test: boot: add imagemap unit tests")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 test/boot/imagemap.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/test/boot/imagemap.c b/test/boot/imagemap.c
index 20a8f150a48..de00a174470 100644
--- a/test/boot/imagemap.c
+++ b/test/boot/imagemap.c
@@ -155,7 +155,7 @@ static int imagemap_test_map_basic(struct unit_test_state *uts)
 	return 0;
 }
 
-IMAGEMAP_TEST(imagemap_test_map_basic, 0);
+IMAGEMAP_TEST(imagemap_test_map_basic, UTF_DM);
 
 /* Test: map() returns cached pointer for already-mapped range */
 static int imagemap_test_map_cached(struct unit_test_state *uts)
@@ -188,7 +188,7 @@ static int imagemap_test_map_cached(struct unit_test_state *uts)
 	return 0;
 }
 
-IMAGEMAP_TEST(imagemap_test_map_cached, 0);
+IMAGEMAP_TEST(imagemap_test_map_cached, UTF_DM);
 
 /* Test: map() returns correct offset within a larger region */
 static int imagemap_test_map_offset(struct unit_test_state *uts)
@@ -222,7 +222,7 @@ static int imagemap_test_map_offset(struct unit_test_state *uts)
 	return 0;
 }
 
-IMAGEMAP_TEST(imagemap_test_map_offset, 0);
+IMAGEMAP_TEST(imagemap_test_map_offset, UTF_DM);
 
 /* Test: map() re-reads when extending a region to a larger size */
 static int imagemap_test_map_extend(struct unit_test_state *uts)
@@ -281,7 +281,7 @@ static int imagemap_test_map_extend(struct unit_test_state *uts)
 	return 0;
 }
 
-IMAGEMAP_TEST(imagemap_test_map_extend, 0);
+IMAGEMAP_TEST(imagemap_test_map_extend, UTF_DM);
 
 /* Test: map_to() reads to a specified address and records it */
 static int imagemap_test_map_to(struct unit_test_state *uts)
@@ -315,7 +315,7 @@ static int imagemap_test_map_to(struct unit_test_state *uts)
 	return 0;
 }
 
-IMAGEMAP_TEST(imagemap_test_map_to, 0);
+IMAGEMAP_TEST(imagemap_test_map_to, UTF_DM);
 
 /* Test: lookup() returns NULL for unmapped ranges */
 static int imagemap_test_lookup_miss(struct unit_test_state *uts)
@@ -350,7 +350,7 @@ static int imagemap_test_lookup_miss(struct unit_test_state *uts)
 	return 0;
 }
 
-IMAGEMAP_TEST(imagemap_test_lookup_miss, 0);
+IMAGEMAP_TEST(imagemap_test_lookup_miss, UTF_DM);
 
 /* Test: LMB reservations are made for map() allocations */
 static int imagemap_test_lmb_reserve(struct unit_test_state *uts)
@@ -383,7 +383,7 @@ static int imagemap_test_lmb_reserve(struct unit_test_state *uts)
 	return 0;
 }
 
-IMAGEMAP_TEST(imagemap_test_lmb_reserve, 0);
+IMAGEMAP_TEST(imagemap_test_lmb_reserve, UTF_DM);
 
 /* Test: cleanup() removes the device */
 static int imagemap_test_cleanup(struct unit_test_state *uts)
@@ -405,7 +405,7 @@ static int imagemap_test_cleanup(struct unit_test_state *uts)
 	return 0;
 }
 
-IMAGEMAP_TEST(imagemap_test_cleanup, 0);
+IMAGEMAP_TEST(imagemap_test_cleanup, UTF_DM);
 
 /* Test: map() with multiple disjoint regions */
 static int imagemap_test_multi_region(struct unit_test_state *uts)
@@ -448,7 +448,7 @@ static int imagemap_test_multi_region(struct unit_test_state *uts)
 	return 0;
 }
 
-IMAGEMAP_TEST(imagemap_test_multi_region, 0);
+IMAGEMAP_TEST(imagemap_test_multi_region, UTF_DM);
 
 /* Test: read beyond image size returns error */
 static int imagemap_test_read_oob(struct unit_test_state *uts)
@@ -474,7 +474,7 @@ static int imagemap_test_read_oob(struct unit_test_state *uts)
 	return 0;
 }
 
-IMAGEMAP_TEST(imagemap_test_read_oob, 0);
+IMAGEMAP_TEST(imagemap_test_read_oob, UTF_DM);
 
 /*
  * Test: block-addressed backend (bl_len > 1) still returns/places the
@@ -514,7 +514,7 @@ static int imagemap_test_blocklen(struct unit_test_state *uts)
 	return 0;
 }
 
-IMAGEMAP_TEST(imagemap_test_blocklen, 0);
+IMAGEMAP_TEST(imagemap_test_blocklen, UTF_DM);
 
 #if CONFIG_IS_ENABLED(MTD_BLOCK)
 /*
@@ -571,7 +571,8 @@ static int imagemap_test_mtd_blk(struct unit_test_state *uts)
 
 	return 0;
 }
-IMAGEMAP_TEST(imagemap_test_mtd_blk, 0);
+
+IMAGEMAP_TEST(imagemap_test_mtd_blk, UTF_DM | UTF_SCAN_FDT | UTF_LIVE_TREE);
 #endif /* MTD_BLOCK */
 
 #if CONFIG_IS_ENABLED(MTD_UBI) && CONFIG_IS_ENABLED(UBI_BLOCK)
@@ -649,5 +650,6 @@ static int imagemap_test_ubiblock(struct unit_test_state *uts)
 
 	return 0;
 }
-IMAGEMAP_TEST(imagemap_test_ubiblock, 0);
+
+IMAGEMAP_TEST(imagemap_test_ubiblock, UTF_DM | UTF_SCAN_FDT | UTF_LIVE_TREE);
 #endif /* MTD_UBI && UBI_BLOCK */
-- 
2.55.0

Re: [PATCH 01/11] mtd: bind the block device without leaking on failure

[Simon Glass]

Hi Daniel,

On 2026-09-29T00:04:16, Daniel Golle <daniel@makrotopia.org> wrote:
> mtd: bind the block device without leaking on failure
>
> add_mtd_device() allocates a slot to hold the mtd_info pointer that
> mtd_bind() keeps, but drops the return value. mtd_bind() only logs and
> returns on failure, so the slot leaks. A failed kmalloc() is ignored
> too, leaving the master without a block device and no hint why its
> partitions are unreachable.
>
> Move the bind into a helper that frees the slot when mtd_bind() fails
> and warns with the errno in both cases.
>
> Fixes: e8a7453824b7 ("mtd: bind an mtd_blk device for non-NAND MTD masters")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>
> drivers/mtd/mtdcore.c | 37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)

> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> @@ -396,6 +396,31 @@ static struct device_type mtd_devtype = {
> +     /*
> +      * mtd_bind() keeps the passed pointer, so it needs storage that lives
> +      * as long as the block device; an MTD master is never removed.
> +      */

I don't think this holds for SPI flash. With DM_SPI_FLASH, 'sf probe'
calls device_remove() on the existing flash device before probing it
again (cmd/sf.c:135). spi_flash_std_remove() then calls
spi_flash_mtd_unregister() -> del_mtd_device(), and the new probe
calls add_mtd_device() again. Neither del_mtd_device() nor
device_remove() unbinds the mtd_blk child, so each 'sf probe'
allocates another slot and binds a second mtd_blk device under the
same flash device, while the old one still points at the old slot.
That leak is more frequent than the mtd_bind() failure fixed here.
Please can you either skip the bind when mtd->dev already has a
UCLASS_BLK child (device_find_first_child_by_uclass()), or unbind the
block device and free the slot in del_mtd_device(), as patch 3 does
for UBI? Either way, please update the comment.

> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> @@ -396,6 +396,31 @@ static struct device_type mtd_devtype = {
> +     kfree(mtdp);
> +warn:
> +     pr_warn("mtd: %s: cannot bind a block device: %d\n", mtd->name, ret);

mtd_bind() already does pr_err("Cannot create block device\n") on
failure, so this prints two lines for one failure. Not a big deal, but
you could drop the message from mtd_bind() (the SPI NAND caller in
nand/spi/core.c already checks the return value) and keep this one,
since it has the name and errno.

Regards,
Simon

Re: [PATCH 02/11] cmd: ubi: report a failed UBI block device bind

[Simon Glass]

Hi Daniel,

On 2026-09-29T00:04:16, Daniel Golle <daniel@makrotopia.org> wrote:
> cmd: ubi: report a failed UBI block device bind
>
> ubi_blk_bind_once() drops the ubi_bind() return value, so when the bind
> fails the attach still reports success and the caller finds no ubi_blk
> device with nothing to explain it.

Not quite. The only failure path in ubi_bind() is
blk_create_devicef(), which already calls pr_err("Cannot create block
device") at the default LOGLEVEL, so the user does get a message
today. What it lacks is the errno and any mention of UBI. Please can
you reword this to match?

>
> Report the errno. The attach itself is not failed: UBI is available
> either way and the block device is an optional view of it, so a UBIFS
> user must not lose the partition because the block layer could not be
> set up.
>
> Fixes: dec405d1653a ("cmd: ubi: create a ubi_blk device when attaching UBI")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>
> cmd/ubi.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

> diff --git a/cmd/ubi.c b/cmd/ubi.c
> @@ -690,7 +692,11 @@ static void ubi_blk_bind_once(void)
> +     ret = ubi_bind(parent);
> +     if (ret)
> +             printf("Cannot create UBI block device: %d\n", ret);

A failure now prints two messages, and since the pr_err() in
ubi_bind() has no trailing newline they run together:

    Cannot create block deviceCannot create UBI block device: -12

I suggest dropping the pr_err() from ubi_bind() so the caller does the
reporting, as your patch 1 does in mtd_blk_bind_master(). Patch 3
already changes ubi_bind(), so it could go there or here.
Alternatively, keep the message in ubi_bind(), add the errno and a
newline, and leave this caller silent. What do you think?

I agree with not failing the attach.

Regards,
Simon

Re: [PATCH 03/11] cmd: ubi: unbind the block device when detaching UBI

[Simon Glass]

Hi Daniel,

On 2026-09-29T00:04:16, Daniel Golle <daniel@makrotopia.org> wrote:
> cmd: ubi: unbind the block device when detaching UBI
>
> The ubi_blk device is parented to the MTD device UBI was attached to,
> but ubi_detach() leaves it bound. After 'ubi part A; ubi detach; ubi
> part B' the device still hangs off A's MTD while UBI sits on B, so the
> driver-model tree describes a parent that is no longer in use and A's
> MTD can no longer be removed. Reads happen to keep working only because
> get_ubi_device() always returns ubi_devices[0].
>
> Return the new device from ubi_bind(), remember it, and unbind it in
> ubi_detach() so the next attach binds a fresh one under the right
> parent. Tracking the device also replaces the scan of every UCLASS_BLK
> device that was used to find it.
>
> Fixes: dec405d1653a ("cmd: ubi: create a ubi_blk device when attaching UBI")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>
> cmd/ubi.c               | 73 +++++++++++++++++++++++++++++--------------------
>  drivers/mtd/ubi/block.c |  4 ++-
>  include/ubi_uboot.h     |  4 +--
>  3 files changed, 49 insertions(+), 32 deletions(-)

> diff --git a/cmd/ubi.c b/cmd/ubi.c
> @@ -650,6 +651,47 @@ static int ubi_set_skip_check(const char *volume, bool skip_check)
> +#if CONFIG_IS_ENABLED(UBI_BLOCK)
> +static struct udevice *ubi_blk_dev;
> +
> +static void ubi_blk_bind_once(void)
> +{
> +     struct udevice *parent;
> +     int ret;
> +
> +     /*
> +      * A single ubi_blk device serves all volumes of the attached UBI
> +      * device, the volume being selected through the block descriptor's
> +      * hwpart. It is parented to the MTD device UBI sits on, so it must
> +      * not outlive the attach that created it.
> +      */
> +     if (ubi_blk_dev)
> +             return;

Driver model can free this device without cmd/ubi.c knowing. If
anything unbinds the MTD parent (e.g. 'unbind' on the flash device),
DM unbinds the ubi_blk child too, leaving ubi_blk_dev pointing at
freed memory. The next ubi_detach() passes it to device_remove(), and
the next ubi_part() returns early and never binds a new device.

Patch 11 makes this easier to hit. With UTF_DM the DM tree is torn
down after each test, so if imagemap_test_ubiblock() fails an
assertion between ubi_part() and the final ubi_detach(), the next
run's initial ubi_detach() uses the stale pointer. The UCLASS_BLK scan
you are removing does not have this problem, since it can only find
devices that still exist.

Please can you keep the scan, and use it in ubi_blk_unbind() too to
find the device to unbind? That still fixes the wrong-parent problem
without global state. What do you think?

> diff --git a/cmd/ubi.c b/cmd/ubi.c
> @@ -650,6 +651,47 @@ static int ubi_set_skip_check(const char *volume, bool skip_check)
> +     device_remove(ubi_blk_dev, DM_REMOVE_NORMAL);
> +     device_unbind(ubi_blk_dev);
> +     ubi_blk_dev = NULL;

Both return values are ignored. If device_remove() fails,
device_unbind() returns -EINVAL for an activated device, and the
pointer is cleared anyway, so the device stays bound under the old
parent, which is the case this patch is trying to fix. Please at least
log the error.

Regards,
Simon

Re: [PATCH 04/11] boot: imagemap: free the reservation a replaced record owned

[Simon Glass]

Hi Daniel,

On 2026-09-29T00:04:16, Daniel Golle <daniel@makrotopia.org> wrote:
> boot: imagemap: free the reservation a replaced record owned
>
> imagemap_record() overwrites an entry that already covers the same image
> offset, including its lmb_reserved flag. When imagemap_map_to() replaces
> a region that imagemap_map() had allocated from LMB, the reservation is
> forgotten with the address that owned it, so imagemap_cleanup() walks a
> table that no longer mentions it and the memory stays reserved for the
> rest of the boot.
>
> Release the reservation before the entry is pointed at different memory.
>
> Fixes: 9b41c7dbee96 ("boot: add imagemap on-demand loading from storage")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>
> boot/imagemap.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)

> diff --git a/boot/imagemap.c b/boot/imagemap.c
> @@ -114,12 +114,22 @@ imagemap_record(struct udevice *dev, loff_t img_offset, ulong size,
> +             /*
> +              * The entry is about to describe different memory, so give up
> +              * the reservation it owns while its address is still known.
> +              */
> +             if (r->lmb_reserved && r->ram != ram)
> +                     lmb_free(map_to_sysmem(r->ram),
> +                              ALIGN(r->size, ARCH_DMA_MINALIGN), LMB_NONE);

This fixes the leak but adds a lifetime problem. imagemap_map() has
already given its caller a pointer into the old buffer, which used to
stay valid until imagemap_cleanup(). Now the memory goes back to LMB
while the caller may still hold the pointer, and a later lmb_alloc()
can hand it out again, e.g. for the FDT or ramdisk relocation in
bootm. One way to hit this is two FIT images sharing the same external
data position, where only one has a load address. The first gets an
imagemap buffer, which fit_image_get_data() returns via
imagemap_lookup(). The second then calls imagemap_map_to() at the same
offset and frees that buffer under the first.

My suggestion is to stop imagemap_map_to() overwriting an LMB-owned
entry. If the existing entry is lmb_reserved and the memory differs,
append a new entry instead. imagemap_cleanup() still frees the
reservation, so nothing leaks, and earlier pointers stay valid until
then. What do you think?

> diff --git a/boot/imagemap.c b/boot/imagemap.c
> @@ -114,12 +114,22 @@ imagemap_record(struct udevice *dev, loff_t img_offset, ulong size,
> +             r->size = size;
> +             r->ram = ram;
> +             r->lmb_reserved = lmb_reserved;

Just to check: if r->ram == ram, r->lmb_reserved is true and the new
lmb_reserved is false, the flag is cleared and the reservation still
leaks. That happens if a caller passes an imagemap-owned pointer as
dst to imagemap_map_to(). Unlikely, but the fix is simple: keep the
flag when the memory doesn't change. Also, r->size can shrink here,
and the lmb_free() in imagemap_cleanup() then uses the smaller size.

> diff --git a/boot/imagemap.c b/boot/imagemap.c
> @@ -114,12 +114,22 @@ imagemap_record(struct udevice *dev, loff_t img_offset, ulong size,
>       /* Check for an existing entry at the same base that we can extend */

This comment no longer fits, since the loop now replaces the entry
rather than extending it. The function comment also says the entry is
updated only 'if the new size is larger', which the code has never
done. Please can you update both?

Please can you also add a test to test/boot/imagemap.c?
imagemap_test_lmb_reserve() is a good model: call imagemap_map() then
imagemap_map_to() at the same block-aligned offset, then check that
the first buffer is no longer reserved after cleanup.

Regards,
Simon

Re: [PATCH 05/11] boot: imagemap: do not resize a region the caller owns

[Simon Glass]

Hi Daniel,

On 2026-09-29T00:04:16, Daniel Golle <daniel@makrotopia.org> wrote:
> boot: imagemap: do not resize a region the caller owns
>
> The extend path reserves memory at the recorded address and re-reads the
> region into it. For a region recorded by imagemap_map_to() that address
> belongs to the caller, which asked for the payload to be placed there:
> imagemap reserves memory it was never given and overwrites whatever the
> caller had put at the destination.
>
> Restrict the extend path to regions allocated from LMB by imagemap
> itself. A larger request against a caller-owned region now falls through
> to a fresh allocation, leaving the caller's memory untouched.
>
> Fixes: 9b41c7dbee96 ("boot: add imagemap on-demand loading from storage")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>
> boot/imagemap.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)

> diff --git a/boot/imagemap.c b/boot/imagemap.c
> @@ -233,13 +233,18 @@ void *imagemap_map(struct udevice *dev, loff_t img_offset, ulong size)
> +                     /*
> +                      * Only a region this device allocated may be resized;
> +                      * one recorded by imagemap_map_to() lives at an address
> +                      * the caller chose and owns.
> +                      */
> +                     if (!r->lmb_reserved)
> +                             break;

The logic looks right to me. imagemap_record() dedups on img_offset,
so there is at most one match and the break is fine.

Please can you add a test? For example, call imagemap_map_to(dev, 0,
64, dst) with a guard pattern in dst beyond 64 bytes, then
imagemap_map(dev, 0, 256), and check that the guard bytes are
unchanged, the returned pointer is not dst, and the new region is
LMB-reserved. Without a test this is easy to break again.

It may also be worth saying in the commit message that the
fall-through replaces the caller's record in the table, via
imagemap_record(). A later imagemap_map_to() to the same dst then
misses the 'already mapped' shortcut and reads the data again. That is
harmless, but not obvious from the description.

> +                     lmb_free(addr, ALIGN(r->size, ARCH_DMA_MINALIGN),
> +                              LMB_NONE);

BTW, this is not from your patch, but it is the same class of bug the
series is fixing. The old reservation is freed here, but r is only
updated on success. Both the -ENOMEM and read-failure exits leave
r->lmb_reserved true and r->ram pointing at memory that is no longer
reserved. imagemap_cleanup() then frees it a second time, which could
drop someone else's reservation, and a later imagemap_lookup() can
return a pointer into unreserved memory. On the in-place read-failure
path, r also still claims r->size valid bytes, although the partial
read may have overwritten them. Perhaps those exits should drop the
entry or re-reserve the old range? After patch 6, the in-place case
could instead shrink the reservation back to old_size, since those
bytes are still valid. It could go in this series or the follow-up,
whichever you prefer.

Regards,
Simon

Re: [PATCH 06/11] boot: imagemap: read only the bytes an extend adds

[Simon Glass]

Hi Daniel,

On 2026-09-29T00:04:16, Daniel Golle <daniel@makrotopia.org> wrote:
> boot: imagemap: read only the bytes an extend adds
>
> Growing a region re-reads it from the start even when the reservation
> was renewed at the same address and the leading bytes are still valid.
> The translation table exists to avoid re-reading data that is already in
> RAM, so the common case of probing a FIT header and then mapping the
> whole structure pays for the header twice.
>
> Read only the range the larger request adds when the region has not
> moved. A reservation that had to be placed elsewhere still needs the
> whole range.
>
> Fixes: 9b41c7dbee96 ("boot: add imagemap on-demand loading from storage")

This is an optimisation rather than a bug fix: the current code reads
more than it needs to, but the result is correct. Please can you drop
the Fixes: tag so this is not picked up as a stable fix?

> Read only the range the larger request adds when the region has not
> moved. A reservation that had to be placed elsewhere still needs the
> whole range.

Strictly, it doesn't need the whole range from storage. The old
reservation has been freed but nothing has written over it, so the old
bytes are still in RAM. A memmove() from the old address to the new
one would keep them, and copes with the new block overlapping the old
one, which is quite likely here. See also below.

> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>
> boot/imagemap.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)

> diff --git a/boot/imagemap.c b/boot/imagemap.c
> @@ -261,8 +265,14 @@ void *imagemap_map(struct udevice *dev, loff_t img_offset, ulong size)
> -                     ret = spl_load_region(&priv->info, img_offset, size,
> -                                           base);
> +                     if (addr == old_addr)
> +                             ret = spl_load_region(&priv->info,
> +                                                   base_off + old_size,
> +                                                   read_size - old_size,
> +                                                   (char *)base + old_size);
> +                     else
> +                             ret = spl_load_region(&priv->info, img_offset,
> +                                                   size, base);

Just to check: how often is the addr == old_addr case taken in
practice? LMB_MEM_ALLOC_ANY allocates top-down, so the FIT header
region is normally the last allocation, sitting directly below the
previous one or below U-Boot's own reservation. Growing it upwards at
the same address runs into that memory and fails, so we end up in the
else branch. If so, the 'common case' in the commit message mostly
does not benefit. Copying the already-loaded bytes across in the else
branch would make the saving apply in both cases.

The comment above the loop still says "re-reading the full range from
storage", so please update it too.

For the test, imagemap_test_map_extend() only checks read_count, which
is 2 whether the code reads the whole range or just the extra bytes.
The mock already records last_off and last_size. Please can you assert
that the second read is (64, 192), and add a variant using
create_mock_loader_bl() with a block size larger than 1? That would
show the new path is really used and the offsets are right for block
media.

Regards,
Simon

Re: [PATCH 11/11] test: boot: imagemap: reset driver-model state between tests

[Simon Glass]

Hi Daniel,

On 2026-09-29T00:04:16, Daniel Golle <daniel@makrotopia.org> wrote:
> test: boot: imagemap: reset driver-model state between tests
>
> The tests bind and probe devices under dm_root() and the storage tests
> depend on real driver-model state, but none of them asks for the DM
> init and uninit the test framework provides. A test that fails before
> imagemap_cleanup() therefore leaves its bound device behind, and the next
> one trips over the name clash in device_bind_driver().
>
> Pass UTF_DM throughout, and UTF_SCAN_FDT for the two tests that walk
> device-tree partitions. Those two are also marked UTF_LIVE_TREE: they
> drive mtd_probe_devices(), which cannot parse the partitions of a master
> bound from a flat tree.

I'm not sure about this. add_mtd_partitions_of() reads the partitions
with the ofnode API (mtd_get_ofnode(), ofnode_find_subnode(),
ofnode_for_each_subnode()), so I would expect it to work with a flat
tree too. What actually fails in the flat-tree run? If the cause is
something else, such as the static old_mtdparts /
mtd_dev_list_updated() state in mtd_probe_devices() or MTD state left
over from the live-tree run, please can you describe that instead?
Otherwise UTF_LIVE_TREE just hides the problem.

>
> Fixes: 05c1fbbfa79f ("test: boot: add imagemap unit tests")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>
> test/boot/imagemap.c | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)

> diff --git a/test/boot/imagemap.c b/test/boot/imagemap.c
> @@ -649,5 +650,6 @@ static int imagemap_test_ubiblock(struct unit_test_state *uts)
> -IMAGEMAP_TEST(imagemap_test_ubiblock, 0);
> +
> +IMAGEMAP_TEST(imagemap_test_ubiblock, UTF_DM | UTF_SCAN_FDT | UTF_LIVE_TREE);

The motivation is a test that fails part-way, but for this test UTF_DM
makes that case worse. If an assertion fails after ubi_part(), the
test returns without calling ubi_detach(), and dm_test_post_run() then
destroys every uclass. That frees the sand-nand device and its chips,
which UBI still points to, as well as the ubi_blk device.
nand_unregister() cannot delete the MTD either, since UBI holds a use
count. Without UTF_DM, the leading ubi_detach() from patch 10 would
clean up properly.

Please can you make sure every failure after ubi_part() calls
imagemap_cleanup(), free() and ubi_detach() (see my comment on patch
10), so the DM teardown never runs while UBI is still attached?
Alternatively, keep this test out of UTF_DM. What do you think?

> diff --git a/test/boot/imagemap.c b/test/boot/imagemap.c
> @@ -571,7 +571,8 @@ static int imagemap_test_mtd_blk(struct unit_test_state *uts)
> -IMAGEMAP_TEST(imagemap_test_mtd_blk, 0);
> +
> +IMAGEMAP_TEST(imagemap_test_mtd_blk, UTF_DM | UTF_SCAN_FDT | UTF_LIVE_TREE);

Regards,
Simon

Re: [PATCH 07/11] boot: fit: unmap the load address after a storage read

[Simon Glass]

Hi Daniel,

On 2026-09-29T00:04:16, Daniel Golle <daniel@makrotopia.org> wrote:
> boot: fit: unmap the load address after a storage read
>
> fit_image_load_storage() maps the load address to get a destination for
> imagemap_map_to() and never unmaps it. The mapping is a no-op on most
> architectures but not on sandbox, which is where the unit tests run.
>
> Pair the map_sysmem() with an unmap_sysmem() once the read is done.
>
> Fixes: 46d32e38ee4e ("boot: fit: support on-demand loading in fit_image_load()")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>
> boot/image-fit.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

> The mapping is a no-op on most architectures but not on sandbox, which is where the unit tests run.

Just FYI, on sandbox, unmap_physmem() returns early for anything
inside emulated RAM (is_in_sandbox_mem()), and a FIT load address is
always in emulated RAM, so in practice it is a no-op there too. I'm
fine with adding the unmap for consistency (this is definitely an
arcane part of sandbox!), but please can you reword this so it doesn't
suggest the tests were leaking something? The Fixes: tag also feels a
bit strong, but OK if you want it.

> diff --git a/boot/image-fit.c b/boot/image-fit.c
> @@ -2222,10 +2223,11 @@ static int fit_image_load_storage(struct bootm_headers *images, const void *fit,
>               mapped = imagemap_map_to(images->imagemap, data_off, data_sz,
>                                        dst);
> +             unmap_sysmem(dst);

imagemap_map_to() saves dst in the region list through
imagemap_record(), and fit_image_get_data() later hands that same
pointer back through imagemap_lookup(). So this unmaps a pointer the
imagemap still holds and keeps giving to callers, which only works
because unmap_sysmem() does nothing here. If the unmap is meant to be
correct, it should happen when the record is released (e.g. in
imagemap_cleanup() for regions where lmb_reserved is false), or the
region should store a physical address and map it on lookup. Otherwise
I'd suggest leaving the code alone and adding a comment that the
mapping lives as long as the imagemap record. What do you think?

> diff --git a/boot/image-fit.c b/boot/image-fit.c
> @@ -2190,6 +2190,7 @@ static int fit_image_load_storage(struct bootm_headers *images, const void *fit,
>       void *mapped;
> +     void *dst;

There's no need to move this to function scope; it is only used inside
the if() block. Sorry if I got that wrong in the previous review.

Regards,
Simon

Re: [PATCH 10/11] test: boot: imagemap: detach UBI when the test is done

[Simon Glass]

Hi Daniel,

On 2026-09-29T00:04:16, Daniel Golle <daniel@makrotopia.org> wrote:
> test: boot: imagemap: detach UBI when the test is done
>
> The UBI test attaches UBI to the sandbox NAND and leaves it attached, so
> UBI still holds a reference to the MTD device when the test ends. Running
> the suite a second time crashes in mtd_partitions_used(): the test calls
> mtd_probe_devices(), which rebuilds the partition list of a master whose
> partitions UBI is still using, and the walk then follows a freed entry.

Just to check: mtd_del_parts() checks mtd_partitions_used() precisely
so that it does not delete partitions still in use. If the walk
follows a freed entry, something has already freed a partition while
UBI held it. That looks like an MTD bug which 'ubi part nand2'
followed by anything that reprobes could also hit. Please can you
explain what frees the entry? If it is a real core bug, I suspect it
should be fixed there, with this patch only tidying up the test.

>
> Detach UBI at the end of the test, and again at the start so a device
> left behind by anything else is released before the partitions are
> rebuilt.
>
> Fixes: 05c1fbbfa79f ("test: boot: add imagemap unit tests")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>
> test/boot/imagemap.c | 8 ++++++++
>  1 file changed, 8 insertions(+)

> diff --git a/test/boot/imagemap.c b/test/boot/imagemap.c
> @@ -592,6 +592,13 @@ static int imagemap_test_ubiblock(struct unit_test_state *uts)
> +     /*
> +      * Release any device a previous run attached: UBI keeps a reference to
> +      * the MTD device, and mtd_probe_devices() would then rebuild the
> +      * partition list behind its back.
> +      */
> +     ubi_detach();

This only helps if the device was left behind by an earlier run of
this same test. Linker-list order puts imagemap_test_mtd_blk before
this one, and it also calls mtd_probe_devices(), so if this test fails
part-way, the next pass crashes in mtd_blk before this detach is
reached.

After the next patch it also acts on freed memory (see my comment on
patch 3), and the 'ubi' global is left holding an MTD whose udevice
has gone.

> diff --git a/test/boot/imagemap.c b/test/boot/imagemap.c
> @@ -638,6 +645,7 @@ static int imagemap_test_ubiblock(struct unit_test_state *uts)
>
>       imagemap_cleanup(imdev);
>       free(wbuf);
> +     ubi_detach();
>
>       return 0;
>  }

This is skipped whenever a ut_assert...() between ubi_part() and here
fails, which is exactly the case the start-of-test detach is meant to
cover, and wbuf leaks too. Please can you move the body into a helper
and have the test function always detach afterwards, something like:

    static int imagemap_test_ubiblock(struct unit_test_state *uts)
    {
        int ret;

        ret = do_ubiblock_test(uts);
        ubi_detach();

        return ret;
    }

That keeps the teardown on every exit path, so you can drop the detach
at the start. What do you think?

Regards,
Simon

Re: [PATCH 08/11] boot: fit: keep the load message for RAM-backed images

[Simon Glass]

Hi Daniel,

On 2026-09-29T00:04:16, Daniel Golle <daniel@makrotopia.org> wrote:
> boot: fit: keep the load message for RAM-backed images
>
> The "Loading ... from ... to ..." line is suppressed whenever
> CONFIG_IMAGEMAP is built in and the source happens to equal the load
> address. The test is a build-time one, so a board that enables imagemap
> but boots a FIT from RAM loses the line as well.
>
> Suppress it only when an imagemap is actually in use, which is the one
> case where the data was read straight to its load address and there is
> no copy to report.
>
> Fixes: 46d32e38ee4e ("boot: fit: support on-demand loading in fit_image_load()")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>
> boot/image-fit.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

> diff --git a/boot/image-fit.c b/boot/image-fit.c
> @@ -2468,7 +2468,8 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
> -             if (!CONFIG_IS_ENABLED(IMAGEMAP) || data != load)
> +             /* A storage-backed image was read straight to its load address */
> +             if (!images->imagemap || data != load)

Dropping the CONFIG_IS_ENABLED(IMAGEMAP) term means images->imagemap
is read at runtime even when imagemap is not built, which isn't safe
for every caller. spl_load_fit_image() declares 'struct bootm_headers
images' on the stack and only sets 'verify', so in SPL (there is no
SPL_IMAGEMAP, so the old expression was always true) this now tests
uninitialised stack memory, and the message can disappear at random
when data == load. The storage check further up keeps the build-time
guard in front of the pointer test. Please can you do the same here:

    if (!CONFIG_IS_ENABLED(IMAGEMAP) || !images->imagemap || data != load)

That keeps the runtime fix for RAM-backed boots with imagemap enabled,
and lets the compiler drop the test when it is disabled. Separately,
it would be worth zeroing 'images' in spl_load_fit_image() as
vbe_common.c does, but that belongs in its own patch.

Regards,
Simon

Re: [PATCH 09/11] boot: bootm: release the imagemap on every failing exit

[Simon Glass]

Hi Daniel,

On 2026-09-29T00:04:16, Daniel Golle <daniel@makrotopia.org> wrote:
> boot: bootm: release the imagemap on every failing exit
>
> Three error paths in bootm_run_states() return directly instead of
> reaching the err label, so the imagemap device and the LMB reservations
> its translation table holds survive a failed bootm: the missing OS boot
> function, an unsupported subcommand, and any error carried out of the
> earlier states. The first of those also left interrupts disabled.
>
> Route all three through err, which already releases the imagemap and
> re-enables interrupts.
>
> Fixes: 46d32e38ee4e ("boot: fit: support on-demand loading in fit_image_load()")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>
> boot/bootm.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)

> The first of those also left interrupts disabled.

Just to check: iflag is set as soon as BOOTM_STATE_LOADOS runs, so a
later ramdisk/FDT failure or a failed subcommand also returned with
interrupts disabled. All three paths had this problem, not just the
first.

> diff --git a/boot/bootm.c b/boot/bootm.c
> @@ -1265,18 +1265,17 @@ int bootm_run_states(struct bootm_info *bmi, int states)
>       /* From now on, we need the OS boot function */
>       if (ret)
> -             return ret;
> +             goto err;

The err label does more than release resources. It also handles
BOOTM_ERR_RESET / BOOTM_ERR_UNIMPLEMENTED, which was only meant for
errors from bootm_load_os(). BOOTM_ERR_RESET is -1, and
boot_ramdisk_high() returns -1 on any failure, so with this patch a
'ramdisk - allocation error' or 'in-place initrd alloc failed' now
prints 'Resetting the board...' and calls reset_cpu() instead of
returning the error. The same applies to the subcommand path below: on
ARM, do_bootm_linux() returns -1 for BOOTM_STATE_OS_BD_T and
BOOTM_STATE_OS_CMDLINE, so 'bootm bdt' would reset the board. This
happens whether or not an imagemap is in use.

Please can you keep the reset/unimplemented handling for the load_os
path only? One way is to give that path its own label that falls
through to the common cleanup. Another is to move the imagemap release
and enable_interrupts() into a separate label after the BOOTM_ERR_*
checks, and send these three exits there. What do you think?

> diff --git a/boot/bootm.c b/boot/bootm.c
> @@ -1318,7 +1317,7 @@ int bootm_run_states(struct bootm_info *bmi, int states)
>       if (ret) {
>               printf("subcommand failed (err=%d)\n", ret);
> -             return ret;
> +             goto err;
>       }

See above. It would also be worth adding a sandbox test for a failing
bootm subcommand with an imagemap attached, so this path is covered.

Regards,
Simon

Re: [00/11] imagemap: fixes from the v1 review

[Simon Glass]

Hi Daniel,

On 2026-09-29T00:04:16, Daniel Golle <daniel@makrotopia.org> wrote:

> Simon Glass reviewed the v1 posting of the imagemap series
> (cover.1787272661.git.daniel@makrotopia.org) after v2 had already been
> applied, so his comments arrive against code that is now in next. This
> series carries the correctness half of them; a second series follows
> with the structural and cosmetic half.

Thanks for picking these up so quickly, and for splitting the
correctness fixes from the cosmetic ones. This is one of the most
tricky series I've looked at in a while!

Patches 4, 5 and 6 change how imagemap behaves but add no tests, so
'ut imagemap' passes both before and after each one. I've suggested a
test on each patch.

> Tested on sandbox: every commit builds, "ut imagemap" passes 13/13 and
> survives "ut -r3"

The -r3 run only exercises the passing path. If the UBI test fails
part-way, the MTD table, the attached UBI device and the static
ubi_blk_dev from patch 3 all survive the driver-model teardown and
point into the freed tree. So I think patches 3, 10 and 11 need a bit
more thought. Details are on those patches.

Smaller point: patch 2 adds a 'parent'/'ret' version of
ubi_blk_bind_once(), then patch 3 moves it and rewrites most of it.
Please consider doing the move first, or folding patch 2's error
report into patch 3, so the reader only reviews the function once.

Overall I am not thrilled with how driver model is holding up in this
case, e.g. the interaction with UBI and tests. I recently did some
work on putting the EFI state into a struct, so we can create a new
state for tests. I'd be interested in your thoughts on this, given
your effort in putting this series together.

Regards,
Simon