[PATCH 0/9] imagemap: Fix boot errors and resource lifetimes

[PATCH 0/9] imagemap: Fix boot errors and resource lifetimes

[Daniel Golle]

This series fixes bootm error handling and resource lifetime issues
found while testing the imagemap boot path. It applies on top of main.

Ordinary allocation failures and unsupported bootm subcommands return
-1, which is also BOOTM_ERR_RESET. Return -ENOMEM and -ENOSYS for these
failures so they return to the shell. Sandbox gains the same subcommand
behaviour, with tests for unsupported subcommands and failed ramdisk
allocation.

A failed imagemap extension can leave the original mapping pointing at
unreserved memory, which cleanup may free after another user has
allocated it. Keep the original mapping and reservation until the larger
region has been read successfully. When the region moves, copy its
cached data and read only the added range. Document region replacement
and pointer lifetime, and add regression coverage for failed extensions
and relocation.

Removing and re-probing a SPI flash leaves its old mtd_blk device bound,
so each probe adds another block device. Unbind it and free its pointer
storage when deleting the MTD master, with a test for the block count
after re-probing.

Built for sandbox and qemu_arm64. The sandbox bootm and imagemap suites
pass all nine and 16 tests respectively. On QEMU, checked that
unsupported bdt and cmdline subcommands and a ramdisk allocation failure
return to the shell.

Daniel Golle (9):
  boot: Return distinct errors for ordinary bootm failures
  sandbox: bootm: Reject unsupported subcommands
  test: boot: bootm: Check ordinary failures
  boot: imagemap: Preserve mappings when an extend fails
  test: boot: imagemap: Check a failed extend
  boot: imagemap: Clarify region replacement
  mtd: Unbind the block device when removing a master
  test: dm: sf: Check the block count after re-probing
  boot: imagemap: Copy cached data when moving a region

 arch/arc/lib/bootm.c        |   5 +-
 arch/arm/lib/bootm.c        |   3 +-
 arch/microblaze/lib/bootm.c |   3 +-
 arch/mips/lib/bootm.c       |   3 +-
 arch/riscv/lib/bootm.c      |   3 +-
 arch/sandbox/lib/bootm.c    |   5 ++
 arch/x86/lib/bootm.c        |   2 +-
 boot/image-board.c          |  13 ++--
 boot/imagemap.c             | 151 ++++++++++++++++++++++--------------
 drivers/mtd/mtdcore.c       |  48 +++++++++++-
 include/imagemap.h          |  13 +++-
 test/boot/bootm.c           |  53 +++++++++++++
 test/boot/imagemap.c        |  85 ++++++++++++++++++++
 test/dm/sf.c                |  32 ++++++++
 14 files changed, 340 insertions(+), 79 deletions(-)


base-commit: 6487c50b8a40f2668ab98a892b9300ad5c181f38
-- 
2.56.0

[PATCH 1/9] boot: Return distinct errors for ordinary bootm failures

[Daniel Golle]

The image allocation helpers and several Linux boot handlers return -1
for ordinary failures. This is BOOTM_ERR_RESET, so bootm_run_states()
resets the board for an allocation failure or an unsupported subcommand.

Return -ENOMEM for allocation failures and -ENOSYS for unsupported
subcommands.

Fixes: 148c55c66c33 ("boot: bootm: release the imagemap on every failing exit")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 arch/arc/lib/bootm.c        |  5 +++--
 arch/arm/lib/bootm.c        |  3 ++-
 arch/microblaze/lib/bootm.c |  3 ++-
 arch/mips/lib/bootm.c       |  3 ++-
 arch/riscv/lib/bootm.c      |  3 ++-
 arch/x86/lib/bootm.c        |  2 +-
 boot/image-board.c          | 13 +++++++------
 7 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/arch/arc/lib/bootm.c b/arch/arc/lib/bootm.c
index 91165a06a46..b41422f2216 100644
--- a/arch/arc/lib/bootm.c
+++ b/arch/arc/lib/bootm.c
@@ -6,6 +6,7 @@
 #include <bootm.h>
 #include <bootstage.h>
 #include <env.h>
+#include <errno.h>
 #include <image.h>
 #include <irq_func.h>
 #include <log.h>
@@ -78,7 +79,7 @@ int do_bootm_linux(int flag, struct bootm_info *bmi)
 
 	/* No need for those on ARC */
 	if ((flag & BOOTM_STATE_OS_BD_T) || (flag & BOOTM_STATE_OS_CMDLINE))
-		return -1;
+		return -ENOSYS;
 
 	if (flag & BOOTM_STATE_OS_PREP)
 		return boot_prep_linux(images);
@@ -88,5 +89,5 @@ int do_bootm_linux(int flag, struct bootm_info *bmi)
 		return 0;
 	}
 
-	return -1;
+	return -ENOSYS;
 }
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
index 9a115cc6078..753ec0016e7 100644
--- a/arch/arm/lib/bootm.c
+++ b/arch/arm/lib/bootm.c
@@ -20,6 +20,7 @@
 #include <asm/global_data.h>
 #include <dm/root.h>
 #include <env.h>
+#include <errno.h>
 #include <image.h>
 #include <u-boot/zlib.h>
 #include <asm/byteorder.h>
@@ -380,7 +381,7 @@ int do_bootm_linux(int flag, struct bootm_info *bmi)
 
 	/* No need for those on ARM */
 	if (flag & BOOTM_STATE_OS_BD_T || flag & BOOTM_STATE_OS_CMDLINE)
-		return -1;
+		return -ENOSYS;
 
 	if (flag & BOOTM_STATE_OS_PREP) {
 		boot_prep_linux(images);
diff --git a/arch/microblaze/lib/bootm.c b/arch/microblaze/lib/bootm.c
index b54c902602f..257ca42fa7c 100644
--- a/arch/microblaze/lib/bootm.c
+++ b/arch/microblaze/lib/bootm.c
@@ -12,6 +12,7 @@
 #include <command.h>
 #include <cpu_func.h>
 #include <env.h>
+#include <errno.h>
 #include <fdt_support.h>
 #include <hang.h>
 #include <image.h>
@@ -69,7 +70,7 @@ int do_bootm_linux(int flag, struct bootm_info *bmi)
 
 	/* cmdline init is the part of 'prep' and nothing to do for 'bdt' */
 	if (flag & BOOTM_STATE_OS_BD_T || flag & BOOTM_STATE_OS_CMDLINE)
-		return -1;
+		return -ENOSYS;
 
 	if (flag & BOOTM_STATE_OS_PREP) {
 		boot_prep_linux(images);
diff --git a/arch/mips/lib/bootm.c b/arch/mips/lib/bootm.c
index 066c830f3fa..cc2b94a8efb 100644
--- a/arch/mips/lib/bootm.c
+++ b/arch/mips/lib/bootm.c
@@ -7,6 +7,7 @@
 #include <bootm.h>
 #include <bootstage.h>
 #include <env.h>
+#include <errno.h>
 #include <image.h>
 #include <fdt_support.h>
 #include <log.h>
@@ -286,7 +287,7 @@ int do_bootm_linux(int flag, struct bootm_info *bmi)
 
 	/* No need for those on MIPS */
 	if (flag & BOOTM_STATE_OS_BD_T)
-		return -1;
+		return -ENOSYS;
 
 	/*
 	 * Cmdline init has been moved to 'bootm prep' because it has to be
diff --git a/arch/riscv/lib/bootm.c b/arch/riscv/lib/bootm.c
index 69c9ca5c487..0500387ded7 100644
--- a/arch/riscv/lib/bootm.c
+++ b/arch/riscv/lib/bootm.c
@@ -10,6 +10,7 @@
 #include <bootm.h>
 #include <command.h>
 #include <dm.h>
+#include <errno.h>
 #include <fdt_support.h>
 #include <hang.h>
 #include <log.h>
@@ -75,7 +76,7 @@ int do_bootm_linux(int flag, struct bootm_info *bmi)
 
 	/* No need for those on RISC-V */
 	if (flag & BOOTM_STATE_OS_BD_T || flag & BOOTM_STATE_OS_CMDLINE)
-		return -1;
+		return -ENOSYS;
 
 	if (flag & BOOTM_STATE_OS_PREP) {
 		boot_prep_linux(images);
diff --git a/arch/x86/lib/bootm.c b/arch/x86/lib/bootm.c
index e054f42fa86..2647ec0c950 100644
--- a/arch/x86/lib/bootm.c
+++ b/arch/x86/lib/bootm.c
@@ -236,7 +236,7 @@ int do_bootm_linux(int flag, struct bootm_info *bmi)
 
 	/* No need for those on x86 */
 	if (flag & BOOTM_STATE_OS_BD_T || flag & BOOTM_STATE_OS_CMDLINE)
-		return -1;
+		return -ENOSYS;
 
 	if (flag & BOOTM_STATE_OS_PREP)
 		return boot_prep_linux(images);
diff --git a/boot/image-board.c b/boot/image-board.c
index 7602f5bfaaa..020d417a378 100644
--- a/boot/image-board.c
+++ b/boot/image-board.c
@@ -13,6 +13,7 @@
 #include <cpu_func.h>
 #include <display_options.h>
 #include <env.h>
+#include <errno.h>
 #include <fpga.h>
 #include <image.h>
 #include <init.h>
@@ -534,7 +535,7 @@ int boot_get_ramdisk(char const *select, struct bootm_headers *images,
  *
  * returns:
  *      0 - success
- *     -1 - failure
+ *     -ENOMEM - allocation failure
  */
 int boot_ramdisk_high(ulong rd_data, ulong rd_len, ulong *initrd_start,
 		      ulong *initrd_end)
@@ -615,7 +616,7 @@ int boot_ramdisk_high(ulong rd_data, ulong rd_len, ulong *initrd_start,
 	return 0;
 
 error:
-	return -1;
+	return -ENOMEM;
 }
 
 int boot_get_setup(struct bootm_headers *images, u8 arch,
@@ -837,7 +838,7 @@ int boot_get_loadable(struct bootm_headers *images)
  *
  * returns:
  *      0 - success
- *     -1 - failure
+ *     -ENOMEM - allocation failure
  */
 int boot_get_cmdline(ulong *cmd_start, ulong *cmd_end)
 {
@@ -858,7 +859,7 @@ int boot_get_cmdline(ulong *cmd_start, ulong *cmd_end)
 
 	err = lmb_alloc_mem(LMB_MEM_ALLOC_MAX, 0xf, &addr, barg, LMB_NONE);
 	if (err)
-		return -1;
+		return -ENOMEM;
 
 	cmdline = (char *)(uintptr_t)addr;
 
@@ -886,7 +887,7 @@ int boot_get_cmdline(ulong *cmd_start, ulong *cmd_end)
  *
  * returns:
  *      0 - success
- *     -1 - failure
+ *     -ENOMEM - allocation failure
  */
 int boot_get_kbd(struct bd_info **kbd)
 {
@@ -897,7 +898,7 @@ int boot_get_kbd(struct bd_info **kbd)
 	err = lmb_alloc_mem(LMB_MEM_ALLOC_MAX, 0xf, &addr,
 			    sizeof(struct bd_info), LMB_NONE);
 	if (err)
-		return -1;
+		return -ENOMEM;
 
 	*kbd = (struct bd_info *)(uintptr_t)addr;
 	**kbd = *gd->bd;
-- 
2.56.0

[PATCH 2/9] sandbox: bootm: Reject unsupported subcommands

[Daniel Golle]

Sandbox accepts the bdt and cmdline subcommands without doing anything.
Return -ENOSYS as other architectures do, so sandbox tests can exercise
bootm's subcommand error handling.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 arch/sandbox/lib/bootm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/sandbox/lib/bootm.c b/arch/sandbox/lib/bootm.c
index 7a5f6f7d36e..da03f451671 100644
--- a/arch/sandbox/lib/bootm.c
+++ b/arch/sandbox/lib/bootm.c
@@ -6,6 +6,7 @@
 
 #include <bootm.h>
 #include <bootstage.h>
+#include <errno.h>
 #include <image.h>
 #include <asm/io.h>
 
@@ -68,6 +69,10 @@ int do_bootm_linux(int flag, struct bootm_info *bmi)
 {
 	struct bootm_headers *images = bmi->images;
 
+	/* No need for those on sandbox */
+	if (flag & BOOTM_STATE_OS_BD_T || flag & BOOTM_STATE_OS_CMDLINE)
+		return -ENOSYS;
+
 	if (flag & BOOTM_STATE_OS_PREP)
 		return boot_prep_linux(images);
 
-- 
2.56.0

[PATCH 3/9] test: boot: bootm: Check ordinary failures

[Daniel Golle]

Check that a failing bootm subcommand returns -ENOSYS with the expected
console message. Force a ramdisk allocation failure and check that it
returns -ENOMEM.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 test/boot/bootm.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/test/boot/bootm.c b/test/boot/bootm.c
index ed60094f3c1..5f013298f8c 100644
--- a/test/boot/bootm.c
+++ b/test/boot/bootm.c
@@ -7,6 +7,8 @@
 
 #include <bootm.h>
 #include <env.h>
+#include <errno.h>
+#include <malloc.h>
 #include <asm/global_data.h>
 #include <test/test.h>
 #include <test/ut.h>
@@ -249,3 +251,54 @@ static int bootm_test_subst_both(struct unit_test_state *uts)
 	return 0;
 }
 BOOTM_TEST(bootm_test_subst_both, 0);
+
+/* Test that a failing subcommand returns its error to the caller */
+static int bootm_test_subcmd_fail(struct unit_test_state *uts)
+{
+	struct bootm_info bmi;
+	int old_state;
+	u8 old_os;
+	int ret;
+
+	bootm_init(&bmi);
+	old_state = bmi.images->state;
+	old_os = bmi.images->os.os;
+	bmi.images->os.os = IH_OS_LINUX;
+
+	ret = bootm_run_states(&bmi, BOOTM_STATE_OS_BD_T);
+
+	bmi.images->os.os = old_os;
+	bmi.images->state = old_state;
+
+	ut_asserteq(-ENOSYS, ret);
+	ut_assert_nextline("subcommand failed (err=%d)", -ENOSYS);
+	ut_assert_console_end();
+
+	return 0;
+}
+BOOTM_TEST(bootm_test_subcmd_fail, UTF_CONSOLE);
+
+static int bootm_test_ramdisk_nomem(struct unit_test_state *uts)
+{
+	ulong initrd_start, initrd_end;
+	char *old_high, *high;
+	int ret;
+
+	high = env_get("initrd_high");
+	old_high = high ? strdup(high) : NULL;
+	ut_assert(!high || old_high);
+	ut_assertok(env_set_hex("initrd_high", 0));
+
+	ret = boot_ramdisk_high(1, ~0UL, &initrd_start, &initrd_end);
+
+	ut_assertok(env_set("initrd_high", old_high));
+	free(old_high);
+
+	ut_asserteq(-ENOMEM, ret);
+	ut_assert_nextline("ramdisk - allocation error");
+	ut_assert_console_end();
+
+	return 0;
+}
+
+BOOTM_TEST(bootm_test_ramdisk_nomem, UTF_CONSOLE);
-- 
2.56.0

[PATCH 4/9] boot: imagemap: Preserve mappings when an extend fails

[Daniel Golle]

Extending a region releases its reservation before allocating and
reading the larger range. A failure leaves the old mapping pointing at
unreserved memory, which cleanup may free again after another user
has allocated it.

Keep the original reservation until the read succeeds, reserving the
added space or a new block first. Update the mapping after success.

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

diff --git a/boot/imagemap.c b/boot/imagemap.c
index d46c1430fb7..0e3fbd0934a 100644
--- a/boot/imagemap.c
+++ b/boot/imagemap.c
@@ -208,6 +208,82 @@ static int imagemap_read_exact(struct spl_load_info *info, loff_t off,
 	return 0;
 }
 
+/**
+ * imagemap_extend() - Grow a region this device allocated
+ *
+ * The region keeps its reservation and its record until the larger one
+ * is reserved and read, so a failure leaves the existing mapping valid.
+ *
+ * @priv:	The imagemap uclass data
+ * @r:		Region to grow, with lmb_reserved set
+ * @img_offset:	Byte offset within the source image
+ * @size:	Number of bytes wanted from @img_offset
+ * Return: pointer to the data at @img_offset, or ERR_PTR on failure
+ */
+static void *imagemap_extend(struct imagemap_priv *priv,
+			     struct imagemap_region *r, loff_t img_offset,
+			     ulong size)
+{
+	phys_size_t old_alloc = ALIGN(r->size, ARCH_DMA_MINALIGN);
+	ulong bl_len = spl_get_bl_len(&priv->info);
+	ulong overhead = img_offset & (bl_len - 1);
+	ulong read_size = ALIGN(size + overhead, bl_len);
+	phys_addr_t old_addr = map_to_sysmem(r->ram);
+	phys_size_t alloc_size, tail_size;
+	phys_addr_t addr, tail;
+	void *base;
+	int ret;
+
+	alloc_size = ALIGN(read_size, ARCH_DMA_MINALIGN);
+	tail_size = alloc_size - old_alloc;
+	tail = old_addr + old_alloc;
+
+	ret = 0;
+	if (tail_size)
+		ret = lmb_alloc_mem(LMB_MEM_ALLOC_ADDR, 0, &tail, tail_size,
+				    LMB_NONE);
+	if (!ret) {
+		base = map_sysmem(old_addr, alloc_size);
+		ret = spl_load_region(&priv->info, r->img_offset + r->size,
+				      read_size - r->size,
+				      (char *)base + r->size);
+		if (ret < 0) {
+			if (tail_size)
+				lmb_free(tail, tail_size, LMB_NONE);
+			goto err_read;
+		}
+		r->size = read_size;
+
+		return base + overhead;
+	}
+
+	if (lmb_alloc_mem(LMB_MEM_ALLOC_ANY, ARCH_DMA_MINALIGN, &addr,
+			  alloc_size, LMB_NONE)) {
+		log_err("imagemap: LMB alloc failed (0x%lx bytes)\n",
+			(ulong)alloc_size);
+		return ERR_PTR(-ENOMEM);
+	}
+	base = map_sysmem(addr, alloc_size);
+
+	ret = spl_load_region(&priv->info, img_offset, size, base);
+	if (ret < 0) {
+		lmb_free(addr, alloc_size, LMB_NONE);
+		goto err_read;
+	}
+
+	lmb_free(old_addr, old_alloc, LMB_NONE);
+	r->size = read_size;
+	r->ram = base;
+
+	return base + overhead;
+
+err_read:
+	log_err("imagemap: read failed at offset 0x%llx (size 0x%lx): %d\n",
+		(unsigned long long)img_offset, size, ret);
+
+	return ERR_PTR(ret);
+}
+
 void *imagemap_map(struct udevice *dev, loff_t img_offset, ulong size)
 {
 	struct imagemap_priv *priv = dev_get_uclass_priv(dev);
@@ -215,10 +291,8 @@ 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;
@@ -231,62 +305,19 @@ void *imagemap_map(struct udevice *dev, loff_t img_offset, ulong size)
 
 	alloc_size = ALIGN(read_size, ARCH_DMA_MINALIGN);
 
-	/*
-	 * Extend a block-aligned region at the same base offset to the
-	 * larger size, re-reading the full range from storage.
-	 */
 	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;
-
-			old_size = r->size;
-			old_addr = map_to_sysmem(r->ram);
-			addr = old_addr;
-
-			lmb_free(addr, ALIGN(old_size, ARCH_DMA_MINALIGN),
-				 LMB_NONE);
+		if (r->img_offset != base_off || r->size >= read_size)
+			continue;
 
-			/* Try to re-reserve at the same address with new size */
-			if (lmb_alloc_mem(LMB_MEM_ALLOC_ADDR, 0, &addr,
-					  alloc_size, LMB_NONE)) {
-				/* In-place extend failed, allocate elsewhere */
-				if (lmb_alloc_mem(LMB_MEM_ALLOC_ANY,
-						  ARCH_DMA_MINALIGN,
-						  &addr, alloc_size,
-						  LMB_NONE)) {
-					log_err("imagemap: LMB alloc failed (0x%lx bytes)\n",
-						(ulong)alloc_size);
-					return ERR_PTR(-ENOMEM);
-				}
-			}
-			base = map_sysmem(addr, alloc_size);
-
-			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);
-				lmb_free(addr, alloc_size, LMB_NONE);
-				return ERR_PTR(ret);
-			}
-			r->size = read_size;
-			r->ram = base;
-			r->lmb_reserved = true;
-
-			return base + overhead;
-		}
+		/*
+		 * 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;
+
+		return imagemap_extend(priv, r, img_offset, size);
 	}
 
 	/* New region - allocate from LMB */
-- 
2.56.0

[PATCH 5/9] test: boot: imagemap: Check a failed extend

[Daniel Golle]

Add a test which extends a mapping past the end of the image. Check
that the failed read preserves the mapping, data and reservation, and
that cleanup preserves a subsequent allocation.

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

diff --git a/test/boot/imagemap.c b/test/boot/imagemap.c
index 2c6f23fb4f0..05bba8776d7 100644
--- a/test/boot/imagemap.c
+++ b/test/boot/imagemap.c
@@ -282,6 +282,51 @@ static int imagemap_test_map_extend(struct unit_test_state *uts)
 
 IMAGEMAP_TEST(imagemap_test_map_extend, UTF_DM);
 
+/* Test: a failed extend leaves the original mapping intact */
+static int imagemap_test_map_extend_fail(struct unit_test_state *uts)
+{
+	const struct imagemap_region *r;
+	struct imagemap_priv *priv;
+	u8 image[IMAGE_SIZE];
+	struct udevice *dev;
+	phys_addr_t other;
+	void *p1, *p2;
+	int i;
+
+	for (i = 0; i < IMAGE_SIZE; i++)
+		image[i] = (u8)(i & 0xff);
+
+	ut_assertok(create_mock_loader(image, IMAGE_SIZE, &dev));
+	priv = dev_get_uclass_priv(dev);
+
+	p1 = imagemap_map(dev, 0, 64);
+	ut_assert(!IS_ERR(p1));
+
+	/* The mock refuses a read past the end of the image */
+	p2 = imagemap_map(dev, 0, IMAGE_SIZE + 64);
+	ut_assert(IS_ERR(p2));
+
+	ut_asserteq(1, priv->regions.count);
+	r = alist_get(&priv->regions, 0, struct imagemap_region);
+	ut_assert(r->lmb_reserved);
+	ut_asserteq_ptr(p1, r->ram);
+	ut_asserteq(64, (int)r->size);
+	ut_assert(lmb_is_reserved_flags(map_to_sysmem(p1), LMB_NONE));
+	ut_asserteq_ptr(p1, imagemap_lookup(dev, 0, 64));
+	ut_asserteq_mem(image, p1, 64);
+
+	/* Memory handed out afterwards belongs to its new owner */
+	ut_assertok(lmb_alloc_mem(LMB_MEM_ALLOC_ANY, ARCH_DMA_MINALIGN, &other,
+				  ARCH_DMA_MINALIGN, LMB_NONE));
+	imagemap_cleanup(dev);
+	ut_assert(lmb_is_reserved_flags(other, LMB_NONE));
+	ut_assertok(lmb_free(other, ARCH_DMA_MINALIGN, LMB_NONE));
+
+	return 0;
+}
+
+IMAGEMAP_TEST(imagemap_test_map_extend_fail, UTF_DM);
+
 /* Test: map_to() reads to a specified address and records it */
 static int imagemap_test_map_to(struct unit_test_state *uts)
 {
-- 
2.56.0

[PATCH 6/9] boot: imagemap: Clarify region replacement

[Daniel Golle]

The comments describe replacing a region only when it grows, but the
table keeps one region per image offset. Document replacement and
pointer lifetime, and update the allocation and extension descriptions.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 boot/imagemap.c    |  6 +++---
 include/imagemap.h | 13 +++++++++----
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/boot/imagemap.c b/boot/imagemap.c
index 0e3fbd0934a..8455763815e 100644
--- a/boot/imagemap.c
+++ b/boot/imagemap.c
@@ -96,8 +96,8 @@ void *imagemap_lookup(struct udevice *dev, loff_t img_offset, ulong size)
 /**
  * imagemap_record() - Record a region in the translation table
  *
- * If an entry with the same img_offset already exists and the new size
- * is larger, update the existing entry. Otherwise add a new entry.
+ * An entry with the same img_offset is replaced, releasing a reservation
+ * it owns at a different address. Otherwise a new entry is added.
  *
  * @dev:		The imagemap device
  * @img_offset:		Byte offset within the source image
@@ -114,7 +114,7 @@ imagemap_record(struct udevice *dev, loff_t img_offset, ulong size,
 	struct imagemap_region *r;
 	struct imagemap_region entry;
 
-	/* Check for an existing entry at the same base that we can extend */
+	/* Each image offset has one entry */
 	alist_for_each(r, &priv->regions) {
 		if (r->img_offset != img_offset)
 			continue;
diff --git a/include/imagemap.h b/include/imagemap.h
index 984a2fa8934..dad438094af 100644
--- a/include/imagemap.h
+++ b/include/imagemap.h
@@ -74,8 +74,9 @@ void *imagemap_lookup(struct udevice *dev, loff_t img_offset, ulong size);
  * new pointer.
  *
  * If the requested range starts at the same offset as an existing
- * region but is larger, the existing region is extended (LMB
- * reservation adjusted, data re-read).
+ * region but is larger, the existing region is extended and only the
+ * added bytes are read. The region may move to a new address, after
+ * which pointers returned for it earlier are no longer valid.
  *
  * @dev:	The imagemap device
  * @img_offset:	Byte offset within the source image
@@ -88,8 +89,12 @@ void *imagemap_map(struct udevice *dev, loff_t img_offset, ulong size);
  * imagemap_map_to() - Load an image region to a specific RAM address
  *
  * Like imagemap_map() but reads into a caller-specified address
- * instead of allocating from the scratch area. Used when the sub-image
- * has a known load address for a zero-copy path.
+ * instead of allocating one. Used when the sub-image has a known load
+ * address for a zero-copy path.
+ *
+ * Each image offset has one region. A region already recorded at
+ * @img_offset is replaced: a reservation it owns is released and
+ * pointers returned for it earlier are no longer valid.
  *
  * @dev:	The imagemap device
  * @img_offset:	Byte offset within the source image
-- 
2.56.0

[PATCH 7/9] mtd: Unbind the block device when removing a master

[Daniel Golle]

Removing an MTD master leaves its block device bound, so each SPI
flash re-probe adds another block device and leaks its pointer storage.
Unbind the block device and free the storage when deleting the master.
Share the condition used to identify masters which have block devices.

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 | 48 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 3d3a84df0a6..34009f997f8 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -30,6 +30,7 @@
 #include <linux/bug.h>
 #include <linux/err.h>
 #include <ubi_uboot.h>
+#include <dm/device-internal.h>
 #endif
 
 #include <linux/log2.h>
@@ -396,6 +397,44 @@ static struct device_type mtd_devtype = {
 };
 #endif
 
+static bool mtd_is_blk_master(struct mtd_info *mtd)
+{
+	return IS_ENABLED(CONFIG_MTD_BLOCK) && mtd->dev &&
+	       !mtd_is_partition(mtd) && !mtd_type_is_nand(mtd);
+}
+
+static void mtd_blk_unbind(struct mtd_info *mtd, struct udevice *blk)
+{
+	struct mtd_info **mtdp;
+	int ret;
+
+	if (strcmp(blk->driver->name, "mtd_blk"))
+		return;
+
+	mtdp = dev_get_priv(blk);
+	if (!mtdp || *mtdp != mtd)
+		return;
+
+	ret = device_remove(blk, DM_REMOVE_NORMAL);
+	if (!ret)
+		ret = device_unbind(blk);
+	if (ret) {
+		pr_warn("mtd: %s: cannot unbind the block device: %d\n",
+			mtd->name, ret);
+		return;
+	}
+
+	kfree(mtdp);
+}
+
+static void mtd_blk_unbind_master(struct mtd_info *mtd)
+{
+	struct udevice *blk, *next;
+
+	list_for_each_entry_safe(blk, next, &mtd->dev->child_head, sibling_node)
+		mtd_blk_unbind(mtd, blk);
+}
+
 static void mtd_blk_bind_master(struct mtd_info *mtd)
 {
 	struct mtd_info **mtdp;
@@ -403,7 +442,7 @@ static void mtd_blk_bind_master(struct mtd_info *mtd)
 
 	/*
 	 * mtd_bind() keeps the passed pointer, so it needs storage that lives
-	 * as long as the block device; an MTD master is never removed.
+	 * as long as the block device.
 	 */
 	mtdp = kmalloc(sizeof(*mtdp), GFP_KERNEL);
 	if (!mtdp) {
@@ -520,8 +559,7 @@ int add_mtd_device(struct mtd_info *mtd)
 
 	mutex_unlock(&mtd_table_mutex);
 
-	if (IS_ENABLED(CONFIG_MTD_BLOCK) && mtd->dev &&
-	    !mtd_is_partition(mtd) && !mtd_type_is_nand(mtd)) {
+	if (mtd_is_blk_master(mtd)) {
 		/*
 		 * Expose a non-NAND MTD master (parallel NOR, SPI-NOR, ...) as
 		 * a mtd_blk block device, so its partitions are reachable
@@ -596,6 +634,10 @@ int del_mtd_device(struct mtd_info *mtd)
 		device_unregister(&mtd->dev);
 #endif
 
+		if (CONFIG_IS_ENABLED(DM_DEVICE_REMOVE) &&
+		    mtd_is_blk_master(mtd))
+			mtd_blk_unbind_master(mtd);
+
 		idr_remove(&mtd_idr, mtd->index);
 
 		module_put(THIS_MODULE);
-- 
2.56.0

[PATCH 8/9] test: dm: sf: Check the block count after re-probing

[Daniel Golle]

Add a test which removes and probes the SPI flash again, checking that
the number of child block devices stays the same. Compare with the
initial count so the test also works with CONFIG_MTD_BLOCK disabled.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 test/dm/sf.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/test/dm/sf.c b/test/dm/sf.c
index 0ec02ac694b..5c3a02d6f79 100644
--- a/test/dm/sf.c
+++ b/test/dm/sf.c
@@ -12,6 +12,7 @@
 #include <spi_flash.h>
 #include <asm/state.h>
 #include <asm/test.h>
+#include <dm/device-internal.h>
 #include <dm/test.h>
 #include <dm/util.h>
 #include <test/test.h>
@@ -106,3 +107,34 @@ static int dm_test_spi_flash_func(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_spi_flash_func, UTF_SCAN_PDATA | UTF_SCAN_FDT);
+
+static int sf_count_blk(struct udevice *dev)
+{
+	struct udevice *child;
+	int count = 0;
+
+	list_for_each_entry(child, &dev->child_head, sibling_node)
+		if (device_get_uclass_id(child) == UCLASS_BLK)
+			count++;
+
+	return count;
+}
+
+/* Test that probing the flash again does not add a block device */
+static int dm_test_spi_flash_reprobe_blk(struct unit_test_state *uts)
+{
+	struct udevice *dev;
+	int count;
+
+	ut_assertok(uclass_first_device_err(UCLASS_SPI_FLASH, &dev));
+	count = sf_count_blk(dev);
+
+	ut_assertok(device_remove(dev, DM_REMOVE_NORMAL));
+	ut_assertok(device_probe(dev));
+	ut_asserteq(count, sf_count_blk(dev));
+
+	sandbox_sf_unbind_emul(state_get_current(), 0, 0);
+
+	return 0;
+}
+DM_TEST(dm_test_spi_flash_reprobe_blk, UTF_SCAN_PDATA | UTF_SCAN_FDT);
-- 
2.56.0

[PATCH 9/9] boot: imagemap: Copy cached data when moving a region

[Daniel Golle]

Moving a region while extending it reads the cached data from storage
again. Copy the existing data into the new allocation and read only the
added range, and include a sandbox unit test covering all of that.

Fixes: 9b41c7dbee96 ("boot: add imagemap on-demand loading from storage")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 boot/imagemap.c      |  4 +++-
 test/boot/imagemap.c | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/boot/imagemap.c b/boot/imagemap.c
index 8455763815e..32ea2682895 100644
--- a/boot/imagemap.c
+++ b/boot/imagemap.c
@@ -265,7 +265,9 @@ static void *imagemap_extend(struct imagemap_priv *priv,
 	}
 	base = map_sysmem(addr, alloc_size);
 
-	ret = spl_load_region(&priv->info, img_offset, size, base);
+	memcpy(base, r->ram, r->size);
+	ret = spl_load_region(&priv->info, r->img_offset + r->size,
+			      read_size - r->size, (char *)base + r->size);
 	if (ret < 0) {
 		lmb_free(addr, alloc_size, LMB_NONE);
 		goto err_read;
diff --git a/test/boot/imagemap.c b/test/boot/imagemap.c
index 05bba8776d7..682f6d5542b 100644
--- a/test/boot/imagemap.c
+++ b/test/boot/imagemap.c
@@ -250,6 +250,8 @@ static int imagemap_test_map_extend(struct unit_test_state *uts)
 	p2 = imagemap_map(dev, 0, 256);
 	ut_assert(!IS_ERR(p2));
 	ut_asserteq(2, mock->read_count);
+	ut_asserteq(64, mock->last_off);
+	ut_asserteq(192, mock->last_size);
 	ut_asserteq_mem(image, p2, 256);
 
 	/* Region count should still be 1 (updated, not added) */
@@ -282,6 +284,44 @@ static int imagemap_test_map_extend(struct unit_test_state *uts)
 
 IMAGEMAP_TEST(imagemap_test_map_extend, UTF_DM);
 
+static int imagemap_test_map_extend_move(struct unit_test_state *uts)
+{
+	struct imagemap_priv *priv;
+	struct mock_priv *mock;
+	u8 image[IMAGE_SIZE];
+	struct udevice *dev;
+	void *p1, *p2;
+	int i;
+
+	for (i = 0; i < IMAGE_SIZE; i++)
+		image[i] = (u8)(i & 0xff);
+
+	ut_assertok(create_mock_loader_bl(image, IMAGE_SIZE, 512, &dev));
+	mock = get_mock_priv(dev);
+	priv = dev_get_uclass_priv(dev);
+
+	p1 = imagemap_map(dev, 612, 200);
+	ut_assert(!IS_ERR(p1));
+	ut_asserteq_mem(image + 612, p1, 200);
+	ut_asserteq(512, mock->last_off);
+	ut_asserteq(512, mock->last_size);
+
+	p2 = imagemap_map(dev, 612, 900);
+	ut_assert(!IS_ERR(p2));
+	ut_assert(p1 != p2);
+	ut_asserteq(2, mock->read_count);
+	ut_asserteq(1024, mock->last_off);
+	ut_asserteq(512, mock->last_size);
+	ut_asserteq_mem(image + 612, p2, 900);
+	ut_asserteq(1, priv->regions.count);
+
+	imagemap_cleanup(dev);
+
+	return 0;
+}
+
+IMAGEMAP_TEST(imagemap_test_map_extend_move, UTF_DM);
+
 /* Test: a failed extend leaves the original mapping intact */
 static int imagemap_test_map_extend_fail(struct unit_test_state *uts)
 {
-- 
2.56.0