[PATCH 0/8] efi: Add docs and tests and a few tweaks

[PATCH 0/8] efi: Add docs and tests and a few tweaks

[Simon Glass]

This series addresses feedback provided on the original series,
including:

- adding documentation and tests for the two new commands
- fixing an issue that somehow didn't fail in CI
- splitting up a large function into two parts
- using lmb for the allocation, instead of looking at internal tables
- dropping use of config.h

Link: https://lore.kernel.org/u-boot/20241123195616.305687-1-mjg59@srcf.ucam.org/
Link: https://patchwork.ozlabs.org/project/uboot/list/?series=433949&state=*&archive=both


Simon Glass (8):
  fdtdec: Correct devicetree symbol for EFI app
  part_find: Enable for sandbox and correct radix and calls
  doc: test: Add docs and test for part_find
  efi: Drop config.h
  cmd: Refactor part_find() into separate functions
  addr_find: Use a simple lmb allocation
  doc: test: Add docs and test for addr_find
  efi: Avoid a memory leak in efi_bind_block() on error path

 cmd/Kconfig                 |   2 +
 cmd/addr_find.c             |  41 +++-------
 cmd/part_find.c             | 153 ++++++++++++++++++++++--------------
 doc/usage/cmd/addr_find.rst |  63 +++++++++++++++
 doc/usage/cmd/part_find.rst | 119 ++++++++++++++++++++++++++++
 doc/usage/index.rst         |   2 +
 drivers/net/efi_net.c       |   1 -
 drivers/tpm/tpm2_efi.c      |   1 -
 lib/efi/efi_app_init.c      |   4 +-
 lib/fdtdec.c                |   2 +-
 test/cmd/Makefile           |   2 +
 test/cmd/addr_find.c        |  27 +++++++
 test/cmd/part_find.c        |  42 ++++++++++
 13 files changed, 367 insertions(+), 92 deletions(-)
 create mode 100644 doc/usage/cmd/addr_find.rst
 create mode 100644 doc/usage/cmd/part_find.rst
 create mode 100644 test/cmd/addr_find.c
 create mode 100644 test/cmd/part_find.c

-- 
2.34.1

[PATCH 1/8] fdtdec: Correct devicetree symbol for EFI app

[Simon Glass]

This should have a single underscore to match the declaration in
include/asm-generic/sections.h so fix it.

For some reason it was not picked up in CI

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 lib/fdtdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index c1eb1c9f825..e87e537f0c3 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -1238,7 +1238,7 @@ static void *fdt_find_separate(void)
 		fdt_blob = (ulong *)__bss_end;
 #elif defined CONFIG_EFI_APP
 	/* FDT is in a separate section */
-	fdt_blob = (ulong *)__dtb;
+	fdt_blob = (ulong *)_dtb;
 #else
 	/* FDT is at end of image */
 	fdt_blob = (ulong *)_end;
-- 
2.34.1

[PATCH 2/8] part_find: Enable for sandbox and correct radix and calls

[Simon Glass]

Enable this command for sandbox and the EFI app.

Recent work removed the if_type member. Update the code to use the new
method for obtaining the interface type.

Use hex so that large partition numbers (>=10) work correctly.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 cmd/Kconfig     | 1 +
 cmd/part_find.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 8fc76778d87..c8ca55194c0 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1552,6 +1552,7 @@ config CMD_PART
 config CMD_PART_FIND
 	bool "part_find"
 	depends on PARTITIONS
+	default y if SANDBOX || EFI_APP
 	select HAVE_BLOCK_DEVICE
 	select PARTITION_UUIDS
 	select PARTITION_TYPE_GUID
diff --git a/cmd/part_find.c b/cmd/part_find.c
index 8c071d3c374..38919935641 100644
--- a/cmd/part_find.c
+++ b/cmd/part_find.c
@@ -107,7 +107,7 @@ static int part_find(int argc, char *const argv[])
 								   &loader_part_no)) {
 						char env[256];
 
-						ret = snprintf(env, sizeof(env), "%s %d:%d", blk_get_if_type_name(desc->if_type), desc->devnum, loader_part_no);
+						ret = snprintf(env, sizeof(env), "%s %x:%x", blk_get_if_type_name(desc->if_type), desc->devnum, loader_part_no);
 						if (ret < 0 || ret == sizeof(env))
 							return CMD_RET_FAILURE;
 						if (env_set("target_part", env))
@@ -123,7 +123,7 @@ static int part_find(int argc, char *const argv[])
 						break;
 					if (strcasecmp(argv[1], info.type_guid) == 0) {
 						char env[256];
-						ret = snprintf(env, sizeof(env), "%s %d:%d", blk_get_if_type_name(desc->if_type), desc->devnum, i);
+						ret = snprintf(env, sizeof(env), "%s %x:%x", blk_get_uclass_name(desc->uclass_id), desc->devnum, i);
 						if (ret < 0 || ret == sizeof(env))
 							return CMD_RET_FAILURE;
 						env_set("target_part", env);
-- 
2.34.1

[PATCH 3/8] doc: test: Add docs and test for part_find

[Simon Glass]

Add some documentation and a test for this new command.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 doc/usage/cmd/part_find.rst | 119 ++++++++++++++++++++++++++++++++++++
 doc/usage/index.rst         |   1 +
 test/cmd/Makefile           |   1 +
 test/cmd/part_find.c        |  42 +++++++++++++
 4 files changed, 163 insertions(+)
 create mode 100644 doc/usage/cmd/part_find.rst
 create mode 100644 test/cmd/part_find.c

diff --git a/doc/usage/cmd/part_find.rst b/doc/usage/cmd/part_find.rst
new file mode 100644
index 00000000000..fd5bd6578d5
--- /dev/null
+++ b/doc/usage/cmd/part_find.rst
@@ -0,0 +1,119 @@
+.. SPDX-License-Identifier: GPL-2.0+:
+
+.. index::
+   single: part_find (command)
+
+part_find command
+=================
+
+Synopsis
+--------
+
+::
+
+    part_find <uuid>
+    part_find self
+
+Description
+-----------
+
+The `part_find` command is used to find a partition with a given type GUID. When
+it finds one, it sets the target_part environment variable to the corresponding
+``interface dev:part`` string.
+
+uuid
+    Universally Unique Identifier (UUID) to search, expressed as a string
+
+self
+    This is only permitted in the EFI app. It indicates that the required
+    partition is the one from which the app was started.
+
+Example
+-------
+
+This shows searching for an EFI system partition and looking at the files on
+that partition::
+
+    => host bind 1 mmc5.img
+    => part list host 0
+
+    Partition Map for host device 0  --   Partition Type: EFI
+
+    Part    Start LBA       End LBA         Name
+            Attributes
+            Type GUID
+            Partition GUID
+    1     0x0000202f      0x0000282e      ""
+            attrs:  0x0000000000000000
+            type:   ebd0a0a2-b9e5-4433-87c0-68b6b72699c7
+                    (data)
+            guid:   6b1e51e3-427c-9f45-a947-e467b7216356
+    2     0x0000002d      0x0000082c      ""
+            attrs:  0x0000000000000000
+            type:   fe3a2a5d-4f32-41a7-b725-accc3285a309
+                    (cros-kern)
+            guid:   dece619f-4876-e140-a6c9-8c208a0c9099
+    3     0x0000202e      0x0000202e      ""
+            attrs:  0x0000000000000000
+            type:   3cb8e202-3b7e-47dd-8a3c-7ff2a13cfcec
+                    (cros-root)
+            guid:   078cee87-a195-ae4c-a974-8ba3a3d783b3
+    4     0x0000082d      0x0000102c      ""
+            attrs:  0x0000000000000000
+            type:   fe3a2a5d-4f32-41a7-b725-accc3285a309
+                    (cros-kern)
+            guid:   08d2f20f-d941-fc43-96f6-948931289d71
+    5     0x0000202d      0x0000202d      ""
+            attrs:  0x0000000000000000
+            type:   3cb8e202-3b7e-47dd-8a3c-7ff2a13cfcec
+                    (cros-root)
+            guid:   0b23ba00-a11c-ed4e-8b49-5e8738899569
+    6     0x00000029      0x00000029      ""
+            attrs:  0x0000000000000000
+            type:   fe3a2a5d-4f32-41a7-b725-accc3285a309
+                    (cros-kern)
+            guid:   6d8158a8-f82d-0d4d-8983-a3ada4eb9b73
+    7     0x0000002a      0x0000002a      ""
+            attrs:  0x0000000000000000
+            type:   3cb8e202-3b7e-47dd-8a3c-7ff2a13cfcec
+                    (cros-root)
+            guid:   76e8f9b0-7db7-3844-8f18-21de93485211
+    8     0x0000102d      0x0000182c      ""
+            attrs:  0x0000000000000000
+            type:   ebd0a0a2-b9e5-4433-87c0-68b6b72699c7
+                    (data)
+            guid:   071dfd2d-173c-f64b-9474-3318665e1d24
+    9     0x0000002b      0x0000002b      ""
+            attrs:  0x0000000000000000
+            type:   2e0a753d-9e48-43b0-8337-b15192cb1b5e
+                    (cros-rsrv)
+            guid:   b9d078c3-bafa-cd48-b771-a0aaa18d5008
+    10     0x0000002c      0x0000002c      ""
+            attrs:  0x0000000000000000
+            type:   2e0a753d-9e48-43b0-8337-b15192cb1b5e
+                    (cros-rsrv)
+            guid:   7b0c0234-1a29-0c4f-bceb-40fae8f7b27c
+    11     0x00000028      0x00000028      ""
+            attrs:  0x0000000000000000
+            type:   cab6e88e-abf3-4102-a07a-d4bb9be3c1d3
+                    (cros-fw)
+            guid:   aced715d-cd1f-394a-9e3e-24b54a7b1472
+    12     0x0000182d      0x0000202c      ""
+            attrs:  0x0000000000000000
+            type:   c12a7328-f81f-11d2-ba4b-00a0c93ec93b
+                    (system)
+            guid:   e1672afd-75ee-d74e-be95-8726b12b5e74
+    => part_find c12a7328-f81f-11d2-ba4b-00a0c93ec93b
+    => print target_part
+    target_part=host 0:c
+    => ls $target_part
+                EFI/
+
+    0 file(s), 1 dir(s)
+
+
+Return value
+------------
+
+The return value $? is set to 0 (true) if the command succeeds. If no partition
+could be found, the return value $? is set to 1 (false).
diff --git a/doc/usage/index.rst b/doc/usage/index.rst
index 71970d5f2b3..64fb91bc5a6 100644
--- a/doc/usage/index.rst
+++ b/doc/usage/index.rst
@@ -95,6 +95,7 @@ Shell commands
    cmd/mtrr
    cmd/panic
    cmd/part
+   cmd/part_find
    cmd/pause
    cmd/pinmux
    cmd/printenv
diff --git a/test/cmd/Makefile b/test/cmd/Makefile
index 583e7c2eec4..dfc49af7bf3 100644
--- a/test/cmd/Makefile
+++ b/test/cmd/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_CMD_LOADM) += loadm.o
 obj-$(CONFIG_CMD_MEMINFO) += meminfo.o
 obj-$(CONFIG_CMD_MEMORY) += mem_copy.o
 obj-$(CONFIG_CMD_MEM_SEARCH) += mem_search.o
+obj-$(CONFIG_CMD_PART_FIND) += part_find.o
 ifdef CONFIG_CMD_PCI
 obj-$(CONFIG_CMD_PCI_MPS) += pci_mps.o
 endif
diff --git a/test/cmd/part_find.c b/test/cmd/part_find.c
new file mode 100644
index 00000000000..1663d4a654f
--- /dev/null
+++ b/test/cmd/part_find.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Test for 'part_find' command
+ *
+ * Copyright 2024 Google LLC
+ * Written by Simon Glass <sjg@chromium.org>
+ */
+
+#include <dm/device-internal.h>
+#include <dm/lists.h>
+#include <dm/ofnode.h>
+#include <dm/test.h>
+#include <test/cmd.h>
+#include <test/ut.h>
+
+/* Test 'part_find' command */
+static int cmd_test_part_find(struct unit_test_state *uts)
+{
+	struct udevice *dev;
+	ofnode root, node;
+
+	/* Enable the requested mmc node since we need a second bootflow */
+	root = oftree_root(oftree_default());
+	node = ofnode_find_subnode(root, "mmc5");
+	ut_assert(ofnode_valid(node));
+	ut_assertok(lists_bind_fdt(gd->dm_root, node, &dev, NULL, false));
+
+	ut_assertok(device_probe(dev));
+
+	ut_assertok(env_set("target_part", NULL));
+	ut_assertok(run_command("part_find c12a7328-f81f-11d2-ba4b-00a0c93ec93b", 0));
+	ut_assert_console_end();
+	ut_asserteq_str("mmc 5:c", env_get("target_part"));
+
+	ut_asserteq(1, run_command("part_find invalid", 0));
+	ut_asserteq_str("mmc 5:c", env_get("target_part"));
+
+	ut_assert_console_end();
+
+	return 0;
+}
+CMD_TEST(cmd_test_part_find, UTF_CONSOLE | UTF_DM | UTF_SCAN_FDT);
-- 
2.34.1

[PATCH 4/8] efi: Drop config.h

[Simon Glass]

We don't need to include this file anymore, so drop it.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/net/efi_net.c  | 1 -
 drivers/tpm/tpm2_efi.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/net/efi_net.c b/drivers/net/efi_net.c
index 370056040f3..abc69058437 100644
--- a/drivers/net/efi_net.c
+++ b/drivers/net/efi_net.c
@@ -4,7 +4,6 @@
  *
  */
 
-#include <config.h>
 #include <dm.h>
 #include <efi.h>
 #include <efi_api.h>
diff --git a/drivers/tpm/tpm2_efi.c b/drivers/tpm/tpm2_efi.c
index 2eb144891d8..c6e7db762b5 100644
--- a/drivers/tpm/tpm2_efi.c
+++ b/drivers/tpm/tpm2_efi.c
@@ -4,7 +4,6 @@
  *
  */
 
-#include <config.h>
 #include <dm.h>
 #include <efi.h>
 #include <efi_api.h>
-- 
2.34.1

[PATCH 5/8] cmd: Refactor part_find() into separate functions

[Simon Glass]

This function uses #ifdefs and ends up with a very large indent, Split
it into two separate functions, one for the EFI app and one for other
builds.

Drop the use of config.h while we are here.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 cmd/part_find.c | 153 +++++++++++++++++++++++++++++-------------------
 1 file changed, 93 insertions(+), 60 deletions(-)

diff --git a/cmd/part_find.c b/cmd/part_find.c
index 38919935641..0864c3f1042 100644
--- a/cmd/part_find.c
+++ b/cmd/part_find.c
@@ -5,12 +5,10 @@
  */
 
 #include <blk.h>
-#include <config.h>
 #include <command.h>
 #include <dm.h>
 #include <env.h>
 #include <part.h>
-#if defined(CONFIG_EFI) || defined(CONFIG_EFI_APP)
 #include <efi.h>
 #include <efi_api.h>
 
@@ -47,21 +45,86 @@ static bool partition_is_on_device(const struct efi_device_path *device,
 	}
 	return false;
 }
-#endif
+
+/**
+ * part_self_find() - Check if a device contains the loaded-image path
+ *
+ * @udev: Block device to check
+ * @loaded_image_path: EFI path of the loaded image
+ * Return 0 if found, -ENOENT if not, other -ve value on error
+ */
+static int part_self_find(struct udevice *udev,
+			  struct efi_device_path *loaded_image_path)
+{
+	struct blk_desc *desc = dev_get_uclass_plat(udev);
+
+	if (desc->uclass_id == UCLASS_EFI_MEDIA) {
+		struct efi_media_plat *plat = dev_get_plat(udev->parent);
+		u32 loader_part_no;
+
+		if (partition_is_on_device(plat->device_path, loaded_image_path,
+					   &loader_part_no)) {
+			char env[256];
+			int ret;
+
+			ret = snprintf(env, sizeof(env), "%s %x:%x",
+				       blk_get_uclass_name(desc->uclass_id),
+				       desc->devnum, loader_part_no);
+			if (ret < 0 || ret == sizeof(env))
+				return -ENOSPC;
+			if (env_set("target_part", env))
+				return -ENOMEM;
+			return 0;
+		}
+	}
+
+	return -ENOENT;
+}
+
+/**
+ * part_blk_find() - Check if a device contains a partition with a type uuid
+ *
+ * @udev: Block device to check
+ * @uuid: UUID to search for (in string form)
+ * Return 0 if found, -ENOENT if not, other -ve value on error
+ */
+static int part_blk_find(struct udevice *udev, const char *uuid)
+{
+	struct blk_desc *desc = dev_get_uclass_plat(udev);
+	int i;
+
+	for (i = 1; i <= MAX_SEARCH_PARTITIONS; i++) {
+		struct disk_partition info;
+		int ret;
+
+		ret = part_get_info(desc, i, &info);
+		if (ret)
+			break;
+		if (strcasecmp(uuid, info.type_guid) == 0) {
+			char env[256];
+
+			ret = snprintf(env, sizeof(env), "%s %x:%x",
+				       blk_get_uclass_name(desc->uclass_id),
+				       desc->devnum, i);
+			if (ret < 0 || ret == sizeof(env))
+				return -ENOSPC;
+			debug("Setting target_part to %s\n", env);
+			if (env_set("target_part", env))
+				return -ENOMEM;
+			return 0;
+		}
+	}
+
+	return -ENOENT;
+}
 
 static int part_find(int argc, char *const argv[])
 {
-#if defined(CONFIG_EFI) || defined(CONFIG_EFI_APP)
 	efi_guid_t efi_devpath_guid = EFI_DEVICE_PATH_PROTOCOL_GUID;
 	struct efi_device_path *loaded_image_path = NULL;
-	struct efi_boot_services *boot = efi_get_boot();
-	struct efi_priv *priv = efi_get_priv();
 	bool part_self = false;
-#endif
 	struct driver *d = ll_entry_start(struct driver, driver);
 	const int n_ents = ll_entry_count(struct driver, driver);
-	struct disk_partition info;
-	struct blk_desc *desc;
 	struct driver *entry;
 	struct udevice *udev;
 	struct uclass *uc;
@@ -70,16 +133,20 @@ static int part_find(int argc, char *const argv[])
 	if (argc != 2)
 		return CMD_RET_USAGE;
 
-#if defined(CONFIG_EFI) || defined (CONFIG_EFI_APP)
-	part_self = !strncmp(argv[1], "self", 6);
-	if (part_self) {
-		ret = boot->handle_protocol(priv->loaded_image->device_handle,
-					&efi_devpath_guid,
-					(void **)&loaded_image_path);
-		if (ret)
-			log_warning("failed to get device path for loaded image (ret=%d)", ret);
+	if (IS_ENABLED(CONFIG_EFI)) {
+		struct efi_boot_services *boot = efi_get_boot();
+		struct efi_priv *priv = efi_get_priv();
+
+		part_self = !strncmp(argv[1], "self", 6);
+		if (part_self) {
+			ret = boot->handle_protocol(priv->loaded_image->device_handle,
+						    &efi_devpath_guid,
+						    (void **)&loaded_image_path);
+			if (ret)
+				log_warning("failed to get device path for loaded image (ret=%d)",
+					    ret);
+		}
 	}
-#endif
 
 	ret = uclass_get(UCLASS_BLK, &uc);
 	if (ret) {
@@ -90,50 +157,16 @@ static int part_find(int argc, char *const argv[])
 		if (entry->id != UCLASS_BLK)
 			continue;
 		uclass_foreach_dev(udev, uc) {
-			int i;
-
 			if (udev->driver != entry)
 				continue;
-			desc = dev_get_uclass_plat(udev);
-#if defined(CONFIG_EFI) || defined(CONFIG_EFI_APP)
-			if (part_self) {
-				if (desc->if_type == IF_TYPE_EFI_MEDIA) {
-					struct efi_media_plat *plat =
-						dev_get_plat(udev->parent);
-					__u32 loader_part_no;
-
-					if (partition_is_on_device(plat->device_path,
-								   loaded_image_path,
-								   &loader_part_no)) {
-						char env[256];
-
-						ret = snprintf(env, sizeof(env), "%s %x:%x", blk_get_if_type_name(desc->if_type), desc->devnum, loader_part_no);
-						if (ret < 0 || ret == sizeof(env))
-							return CMD_RET_FAILURE;
-						if (env_set("target_part", env))
-							return CMD_RET_FAILURE;
-						return CMD_RET_SUCCESS;
-					}
-				}
-			} else {
-#endif
-				for (i = 1; i <= MAX_SEARCH_PARTITIONS; i++) {
-					ret = part_get_info(desc, i, &info);
-					if (ret)
-						break;
-					if (strcasecmp(argv[1], info.type_guid) == 0) {
-						char env[256];
-						ret = snprintf(env, sizeof(env), "%s %x:%x", blk_get_uclass_name(desc->uclass_id), desc->devnum, i);
-						if (ret < 0 || ret == sizeof(env))
-							return CMD_RET_FAILURE;
-						env_set("target_part", env);
-						debug("Setting target_part to %s\n", env);
-						return CMD_RET_SUCCESS;
-					}
-				}
-#if defined(CONFIG_EFI) || defined(CONFIG_EFI_APP)
-			}
-#endif
+			if (IS_ENABLED(CONFIG_EFI) && part_self)
+				ret = part_self_find(udev, loaded_image_path);
+			else
+				ret = part_blk_find(udev, argv[1]);
+			if (!ret)
+				return 0;
+			if (ret != -ENOENT)
+				break;
 		}
 	}
 
-- 
2.34.1

[PATCH 6/8] addr_find: Use a simple lmb allocation

[Simon Glass]

There should be no need to parse the LMB tables manually. Use the
allocation-function provided instead. Adjust the argument checks while
we are here.

Also enable this command for sandbox and the EFI app, so it is built in
CI.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 cmd/Kconfig     |  1 +
 cmd/addr_find.c | 40 +++++++++++++---------------------------
 2 files changed, 14 insertions(+), 27 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index c8ca55194c0..d4da504bea4 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -130,6 +130,7 @@ config CMD_ACPI
 
 config CMD_ADDR_FIND
         bool "addr_find"
+        default y if SANDBOX || EFI_APP
 	help
 	  This command searches for an unused region of address space
 	  sufficiently large to hold a file. If successful, it sets the
diff --git a/cmd/addr_find.c b/cmd/addr_find.c
index b187337d885..2c20b959031 100644
--- a/cmd/addr_find.c
+++ b/cmd/addr_find.c
@@ -16,12 +16,13 @@ DECLARE_GLOBAL_DATA_PTR;
 
 int do_addr_find(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
-	struct lmb_region *mem, *reserved;
 	const char *filename;
-	struct lmb lmb;
+	phys_addr_t start;
 	loff_t size;
 	int ret;
-	int i, j;
+
+	if (argc < 3)
+		return CMD_RET_USAGE;
 
 	if (!gd->fdt_blob) {
 		log_err("No FDT setup\n");
@@ -49,36 +50,21 @@ int do_addr_find(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 		return CMD_RET_FAILURE;
 	}
 
-	lmb_init_and_reserve(&lmb, gd->bd, (void *)gd->fdt_blob);
-	mem = &lmb.memory;
-	reserved = &lmb.reserved;
-
-	for (i = 0; i < mem->cnt; i++) {
-		unsigned long long start, end;
+	start = lmb_alloc(size, SZ_2M);
+	if ((long)start < 0) {
+		log_err("Failed to find enough RAM for 0x%llx bytes\n", size);
 
-		start = mem->region[i].base;
-		end = mem->region[i].base + mem->region[i].size - 1;
-		if ((start + size) > end)
-			continue;
-		for (j = 0; j < reserved->cnt; j++) {
-			if ((reserved->region[j].base + reserved->region[j].size) < start)
-				continue;
-			if ((start + size) > reserved->region[j].base)
-				start = reserved->region[j].base + reserved->region[j].size;
-		}
-		if ((start + size) <= end) {
-			env_set_hex("loadaddr", start);
-			debug("Set loadaddr to 0x%llx\n", start);
-			return CMD_RET_SUCCESS;
-		}
+		return CMD_RET_FAILURE;
 	}
 
-	log_err("Failed to find enough RAM for 0x%llx bytes\n", size);
-	return CMD_RET_FAILURE;
+	env_set_hex("loadaddr", start);
+	debug("Set loadaddr to %llx\n", (u64)start);
+
+	return 0;
 }
 
 U_BOOT_CMD(
-	addr_find, 7, 1, do_addr_find,
+	addr_find, 4, 1, do_addr_find,
 	"find a load address suitable for a file",
 	"<interface> [<dev[:part]>] <filename>\n"
 	"- find a consecutive region of memory sufficiently large to hold\n"
-- 
2.34.1

[PATCH 7/8] doc: test: Add docs and test for addr_find

[Simon Glass]

Add documentation and a test for this command.

Drop the use of config.h while we are here.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 cmd/addr_find.c             |  1 -
 doc/usage/cmd/addr_find.rst | 63 +++++++++++++++++++++++++++++++++++++
 doc/usage/index.rst         |  1 +
 test/cmd/Makefile           |  1 +
 test/cmd/addr_find.c        | 27 ++++++++++++++++
 5 files changed, 92 insertions(+), 1 deletion(-)
 create mode 100644 doc/usage/cmd/addr_find.rst
 create mode 100644 test/cmd/addr_find.c

diff --git a/cmd/addr_find.c b/cmd/addr_find.c
index 2c20b959031..601b8b2566b 100644
--- a/cmd/addr_find.c
+++ b/cmd/addr_find.c
@@ -5,7 +5,6 @@
  */
 
 #include <blk.h>
-#include <config.h>
 #include <command.h>
 #include <env.h>
 #include <fs.h>
diff --git a/doc/usage/cmd/addr_find.rst b/doc/usage/cmd/addr_find.rst
new file mode 100644
index 00000000000..0d16ffd236b
--- /dev/null
+++ b/doc/usage/cmd/addr_find.rst
@@ -0,0 +1,63 @@
+.. SPDX-License-Identifier: GPL-2.0+:
+
+.. index::
+   single: addr_find (command)
+
+addr_find command
+=================
+
+Synopsis
+--------
+
+::
+
+    addr_find <interface> [<dev[:part]> [<filename>]]
+
+Description
+-----------
+
+The addr_find command is used to find a consecutive region of memory
+sufficiently large to hold a file, ensuring that the memory is not currently in
+use for another file, etc.
+
+If successful, 'loadaddr' is set to the located address.
+
+The number of transferred bytes is saved in the environment variable filesize.
+The load address is saved in the environment variable fileaddr.
+
+interface
+    interface for accessing the block device (mmc, sata, scsi, usb, ....)
+
+dev
+    device number
+
+part
+    partition number, defaults to 0 (whole device)
+
+filename
+    path to file, defaults to environment variable 'bootfile'
+
+Example
+-------
+
+This shows obtaining an address suitable for a file on an mmc disk::
+
+    => ls mmc 1
+                extlinux/
+     97135227   initramfs-5.3.7-301.fc31.armv7hl.img
+                dtb-5.3.7-301.fc31.armv7hl/
+     12531628   vmlinuz-5.3.7-301.fc31.armv7hl
+
+    2 file(s), 2 dir(s)
+
+    => addr_find mmc 1 vmlinuz-5.3.7-301.fc31.armv7hl
+    => print loadaddr
+    loadaddr=7c00000
+    =>
+
+
+Return value
+------------
+
+The return value $? is set to 0 (true) if the command succeeds. If no suitable
+address could be found, the return value $? is set to 1 (false).
diff --git a/doc/usage/index.rst b/doc/usage/index.rst
index 64fb91bc5a6..9cbc9a2c9f9 100644
--- a/doc/usage/index.rst
+++ b/doc/usage/index.rst
@@ -24,6 +24,7 @@ Shell commands
    :maxdepth: 1
 
    cmd/acpi
+   cmd/addr_find
    cmd/addrmap
    cmd/armffa
    cmd/askenv
diff --git a/test/cmd/Makefile b/test/cmd/Makefile
index dfc49af7bf3..912579e0c6b 100644
--- a/test/cmd/Makefile
+++ b/test/cmd/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_CMD_PAUSE) += test_pause.o
 endif
 obj-y += exit.o mem.o
 obj-$(CONFIG_X86) += cpuid.o msr.o
+obj-$(CONFIG_CMD_ADDR_FIND) += addr_find.o
 obj-$(CONFIG_CMD_ADDRMAP) += addrmap.o
 obj-$(CONFIG_CMD_BDI) += bdinfo.o
 obj-$(CONFIG_COREBOOT_SYSINFO) += coreboot.o
diff --git a/test/cmd/addr_find.c b/test/cmd/addr_find.c
new file mode 100644
index 00000000000..ce087759d9e
--- /dev/null
+++ b/test/cmd/addr_find.c
@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Test for 'part_find' command
+ *
+ * Copyright 2024 Google LLC
+ * Written by Simon Glass <sjg@chromium.org>
+ */
+
+#include <dm/device-internal.h>
+#include <dm/lists.h>
+#include <dm/ofnode.h>
+#include <dm/test.h>
+#include <test/cmd.h>
+#include <test/ut.h>
+
+/* Test 'addr_find' command */
+static int cmd_test_addr_find(struct unit_test_state *uts)
+{
+	ut_assertok(env_set("loadaddr", NULL));
+	ut_assertok(run_command("addr_find mmc 1:1 vmlinuz-5.3.7-301.fc31.armv7hl", 0));
+	ut_assert_console_end();
+
+	ut_assertnonnull(env_get("loadaddr"));
+
+	return 0;
+}
+CMD_TEST(cmd_test_addr_find, UTF_CONSOLE | UTF_DM | UTF_SCAN_FDT);
-- 
2.34.1

[PATCH 8/8] efi: Avoid a memory leak in efi_bind_block() on error path

[Simon Glass]

Free the memory used if an error occurs.

Signed-off-by: Simon Glass <sjg@chromium.org>
Suggested-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---

 lib/efi/efi_app_init.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/efi/efi_app_init.c b/lib/efi/efi_app_init.c
index cc91e1d74b8..fec3f348cfb 100644
--- a/lib/efi/efi_app_init.c
+++ b/lib/efi/efi_app_init.c
@@ -60,8 +60,10 @@ int efi_bind_block(efi_handle_t handle, struct efi_block_io *blkio,
 	plat->handle = handle;
 	plat->blkio = blkio;
 	plat->device_path = malloc(device_path_len);
-	if (!plat->device_path)
+	if (!plat->device_path) {
+		free(plat);
 		return log_msg_ret("path", -ENOMEM);
+	}
 	memcpy(plat->device_path, device_path, device_path_len);
 	ret = device_bind(dm_root(), DM_DRIVER_GET(efi_media), "efi_media",
 			  plat, ofnode_null(), &dev);
-- 
2.34.1

Re: [PATCH 8/8] efi: Avoid a memory leak in efi_bind_block() on error path

[Heinrich Schuchardt]

On 09.12.24 17:28, Simon Glass wrote:
> Free the memory used if an error occurs.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Suggested-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>
>   lib/efi/efi_app_init.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/lib/efi/efi_app_init.c b/lib/efi/efi_app_init.c
> index cc91e1d74b8..fec3f348cfb 100644
> --- a/lib/efi/efi_app_init.c
> +++ b/lib/efi/efi_app_init.c
> @@ -60,8 +60,10 @@ int efi_bind_block(efi_handle_t handle, struct efi_block_io *blkio,
>   	plat->handle = handle;
>   	plat->blkio = blkio;
>   	plat->device_path = malloc(device_path_len);
> -	if (!plat->device_path)
> +	if (!plat->device_path) {
> +		free(plat);

As of origin master origin/next (9dd0a9ecaa539ad) plat is a local
variable and not a pointer. You cannot free it.

If there is some prerequisite patch that is making plat a pointer, you
should add this correction to that patch.

Best regards

Heinrich

>   		return log_msg_ret("path", -ENOMEM);
> +	}
>   	memcpy(plat->device_path, device_path, device_path_len);
>   	ret = device_bind(dm_root(), DM_DRIVER_GET(efi_media), "efi_media",
>   			  plat, ofnode_null(), &dev);

Re: [PATCH 3/8] doc: test: Add docs and test for part_find

[Heinrich Schuchardt]

On 09.12.24 17:27, Simon Glass wrote:
> Add some documentation and a test for this new command.

Shouldn't this be two patches?

>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>   doc/usage/cmd/part_find.rst | 119 ++++++++++++++++++++++++++++++++++++
>   doc/usage/index.rst         |   1 +
>   test/cmd/Makefile           |   1 +
>   test/cmd/part_find.c        |  42 +++++++++++++
>   4 files changed, 163 insertions(+)
>   create mode 100644 doc/usage/cmd/part_find.rst
>   create mode 100644 test/cmd/part_find.c
>
> diff --git a/doc/usage/cmd/part_find.rst b/doc/usage/cmd/part_find.rst
> new file mode 100644
> index 00000000000..fd5bd6578d5
> --- /dev/null
> +++ b/doc/usage/cmd/part_find.rst
> @@ -0,0 +1,119 @@
> +.. SPDX-License-Identifier: GPL-2.0+:

This is not a valid SPDX identifier.
Cf. https://spdx.org/licenses/GPL-2.0-or-later.html

> +
> +.. index::
> +   single: part_find (command)
> +
> +part_find command
> +=================
> +
> +Synopsis
> +--------
> +
> +::
> +
> +    part_find <uuid>
> +    part_find self

We already have a part command.
Remembering the command would be much easier if you would add the
functionality there.

> +
> +Description
> +-----------
> +
> +The `part_find` command is used to find a partition with a given type GUID. When
> +it finds one, it sets the target_part environment variable to the corresponding
> +``interface dev:part`` string.
> +
> +uuid
> +    Universally Unique Identifier (UUID) to search, expressed as a string
> +
> +self
> +    This is only permitted in the EFI app. It indicates that the required
> +    partition is the one from which the app was started.
> +
> +Example
> +-------
> +
> +This shows searching for an EFI system partition and looking at the files on
> +that partition::
> +
> +    => host bind 1 mmc5.img
> +    => part list host 0
> +
> +    Partition Map for host device 0  --   Partition Type: EFI
> +
> +    Part    Start LBA       End LBA         Name
> +            Attributes
> +            Type GUID
> +            Partition GUID
> +    1     0x0000202f      0x0000282e      ""
> +            attrs:  0x0000000000000000
> +            type:   ebd0a0a2-b9e5-4433-87c0-68b6b72699c7
> +                    (data)
> +            guid:   6b1e51e3-427c-9f45-a947-e467b7216356
> +    2     0x0000002d      0x0000082c      ""
> +            attrs:  0x0000000000000000
> +            type:   fe3a2a5d-4f32-41a7-b725-accc3285a309
> +                    (cros-kern)
> +            guid:   dece619f-4876-e140-a6c9-8c208a0c9099
> +    3     0x0000202e      0x0000202e      ""
> +            attrs:  0x0000000000000000
> +            type:   3cb8e202-3b7e-47dd-8a3c-7ff2a13cfcec
> +                    (cros-root)
> +            guid:   078cee87-a195-ae4c-a974-8ba3a3d783b3
> +    4     0x0000082d      0x0000102c      ""
> +            attrs:  0x0000000000000000
> +            type:   fe3a2a5d-4f32-41a7-b725-accc3285a309
> +                    (cros-kern)
> +            guid:   08d2f20f-d941-fc43-96f6-948931289d71
> +    5     0x0000202d      0x0000202d      ""
> +            attrs:  0x0000000000000000
> +            type:   3cb8e202-3b7e-47dd-8a3c-7ff2a13cfcec
> +                    (cros-root)
> +            guid:   0b23ba00-a11c-ed4e-8b49-5e8738899569
> +    6     0x00000029      0x00000029      ""
> +            attrs:  0x0000000000000000
> +            type:   fe3a2a5d-4f32-41a7-b725-accc3285a309
> +                    (cros-kern)
> +            guid:   6d8158a8-f82d-0d4d-8983-a3ada4eb9b73
> +    7     0x0000002a      0x0000002a      ""
> +            attrs:  0x0000000000000000
> +            type:   3cb8e202-3b7e-47dd-8a3c-7ff2a13cfcec
> +                    (cros-root)
> +            guid:   76e8f9b0-7db7-3844-8f18-21de93485211
> +    8     0x0000102d      0x0000182c      ""
> +            attrs:  0x0000000000000000
> +            type:   ebd0a0a2-b9e5-4433-87c0-68b6b72699c7
> +                    (data)
> +            guid:   071dfd2d-173c-f64b-9474-3318665e1d24
> +    9     0x0000002b      0x0000002b      ""
> +            attrs:  0x0000000000000000
> +            type:   2e0a753d-9e48-43b0-8337-b15192cb1b5e
> +                    (cros-rsrv)
> +            guid:   b9d078c3-bafa-cd48-b771-a0aaa18d5008
> +    10     0x0000002c      0x0000002c      ""
> +            attrs:  0x0000000000000000
> +            type:   2e0a753d-9e48-43b0-8337-b15192cb1b5e
> +                    (cros-rsrv)
> +            guid:   7b0c0234-1a29-0c4f-bceb-40fae8f7b27c
> +    11     0x00000028      0x00000028      ""
> +            attrs:  0x0000000000000000
> +            type:   cab6e88e-abf3-4102-a07a-d4bb9be3c1d3
> +                    (cros-fw)
> +            guid:   aced715d-cd1f-394a-9e3e-24b54a7b1472
> +    12     0x0000182d      0x0000202c      ""
> +            attrs:  0x0000000000000000
> +            type:   c12a7328-f81f-11d2-ba4b-00a0c93ec93b
> +                    (system)
> +            guid:   e1672afd-75ee-d74e-be95-8726b12b5e74

This excessive list is just distracting in the documentation. Two
partitions are enough.

> +    => part_find c12a7328-f81f-11d2-ba4b-00a0c93ec93b
> +    => print target_part
> +    target_part=host 0:c
> +    => ls $target_part
> +                EFI/
> +
> +    0 file(s), 1 dir(s)
> +
> +
> +Return value
> +------------
> +
> +The return value $? is set to 0 (true) if the command succeeds. If no partition
> +could be found, the return value $? is set to 1 (false).
> diff --git a/doc/usage/index.rst b/doc/usage/index.rst
> index 71970d5f2b3..64fb91bc5a6 100644
> --- a/doc/usage/index.rst
> +++ b/doc/usage/index.rst
> @@ -95,6 +95,7 @@ Shell commands
>      cmd/mtrr
>      cmd/panic
>      cmd/part
> +   cmd/part_find
>      cmd/pause
>      cmd/pinmux
>      cmd/printenv
> diff --git a/test/cmd/Makefile b/test/cmd/Makefile
> index 583e7c2eec4..dfc49af7bf3 100644
> --- a/test/cmd/Makefile
> +++ b/test/cmd/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_CMD_LOADM) += loadm.o
>   obj-$(CONFIG_CMD_MEMINFO) += meminfo.o
>   obj-$(CONFIG_CMD_MEMORY) += mem_copy.o
>   obj-$(CONFIG_CMD_MEM_SEARCH) += mem_search.o
> +obj-$(CONFIG_CMD_PART_FIND) += part_find.o
>   ifdef CONFIG_CMD_PCI
>   obj-$(CONFIG_CMD_PCI_MPS) += pci_mps.o
>   endif
> diff --git a/test/cmd/part_find.c b/test/cmd/part_find.c
> new file mode 100644
> index 00000000000..1663d4a654f
> --- /dev/null
> +++ b/test/cmd/part_find.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0+

This is not a valid SPDX identifier.
Cf. https://spdx.org/licenses/GPL-2.0-or-later.html

> +/*
> + * Test for 'part_find' command
> + *
> + * Copyright 2024 Google LLC
> + * Written by Simon Glass <sjg@chromium.org>
> + */
> +
> +#include <dm/device-internal.h>
> +#include <dm/lists.h>
> +#include <dm/ofnode.h>
> +#include <dm/test.h>
> +#include <test/cmd.h>
> +#include <test/ut.h>
> +
> +/* Test 'part_find' command */
> +static int cmd_test_part_find(struct unit_test_state *uts)
> +{
> +	struct udevice *dev;
> +	ofnode root, node;
> +
> +	/* Enable the requested mmc node since we need a second bootflow */
> +	root = oftree_root(oftree_default());
> +	node = ofnode_find_subnode(root, "mmc5");
> +	ut_assert(ofnode_valid(node));
> +	ut_assertok(lists_bind_fdt(gd->dm_root, node, &dev, NULL, false));
> +
> +	ut_assertok(device_probe(dev));
> +
> +	ut_assertok(env_set("target_part", NULL));
> +	ut_assertok(run_command("part_find c12a7328-f81f-11d2-ba4b-00a0c93ec93b", 0));
> +	ut_assert_console_end();
> +	ut_asserteq_str("mmc 5:c", env_get("target_part"));
> +
> +	ut_asserteq(1, run_command("part_find invalid", 0));
> +	ut_asserteq_str("mmc 5:c", env_get("target_part"));
> +
> +	ut_assert_console_end();
> +
> +	return 0;
> +}
> +CMD_TEST(cmd_test_part_find, UTF_CONSOLE | UTF_DM | UTF_SCAN_FDT);

Re: [PATCH 3/8] doc: test: Add docs and test for part_find

[Simon Glass]

Hi Heinrich,

On Tue, 10 Dec 2024 at 01:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 09.12.24 17:27, Simon Glass wrote:
> > Add some documentation and a test for this new command.
>
> Shouldn't this be two patches?

Often we put the new command, its docs and tests in the same commit,
since the question I always ask when looking at a command is, where
are the docs and tests!

>
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >   doc/usage/cmd/part_find.rst | 119 ++++++++++++++++++++++++++++++++++++
> >   doc/usage/index.rst         |   1 +
> >   test/cmd/Makefile           |   1 +
> >   test/cmd/part_find.c        |  42 +++++++++++++
> >   4 files changed, 163 insertions(+)
> >   create mode 100644 doc/usage/cmd/part_find.rst
> >   create mode 100644 test/cmd/part_find.c
> >
> > diff --git a/doc/usage/cmd/part_find.rst b/doc/usage/cmd/part_find.rst
> > new file mode 100644
> > index 00000000000..fd5bd6578d5
> > --- /dev/null
> > +++ b/doc/usage/cmd/part_find.rst
> > @@ -0,0 +1,119 @@
> > +.. SPDX-License-Identifier: GPL-2.0+:
>
> This is not a valid SPDX identifier.
> Cf. https://spdx.org/licenses/GPL-2.0-or-later.html

I have seen this point made a few times, but I'm afraid I still don't
fully understand it:

The Licenses/README lists the licenses and GPL-2.0+ appears in there.
In the source tree:

$ git grep GPL-2.0+ |wc -l
13406
$ git grep GPL-2.0-or-later |wc -l
1847

I have to say I much prefer GPL-2.0+ as it is easier to remember.

But if we are planning to change, could you update checkpatch to throw
a warning?

Regards,
SImon

Re: [PATCH 3/8] doc: test: Add docs and test for part_find

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2073 bytes --]

On Tue, Dec 10, 2024 at 09:16:57AM -0700, Simon Glass wrote:
> Hi Heinrich,
> 
> On Tue, 10 Dec 2024 at 01:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 09.12.24 17:27, Simon Glass wrote:
> > > Add some documentation and a test for this new command.
> >
> > Shouldn't this be two patches?
> 
> Often we put the new command, its docs and tests in the same commit,
> since the question I always ask when looking at a command is, where
> are the docs and tests!
> 
> >
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > >   doc/usage/cmd/part_find.rst | 119 ++++++++++++++++++++++++++++++++++++
> > >   doc/usage/index.rst         |   1 +
> > >   test/cmd/Makefile           |   1 +
> > >   test/cmd/part_find.c        |  42 +++++++++++++
> > >   4 files changed, 163 insertions(+)
> > >   create mode 100644 doc/usage/cmd/part_find.rst
> > >   create mode 100644 test/cmd/part_find.c
> > >
> > > diff --git a/doc/usage/cmd/part_find.rst b/doc/usage/cmd/part_find.rst
> > > new file mode 100644
> > > index 00000000000..fd5bd6578d5
> > > --- /dev/null
> > > +++ b/doc/usage/cmd/part_find.rst
> > > @@ -0,0 +1,119 @@
> > > +.. SPDX-License-Identifier: GPL-2.0+:
> >
> > This is not a valid SPDX identifier.
> > Cf. https://spdx.org/licenses/GPL-2.0-or-later.html
> 
> I have seen this point made a few times, but I'm afraid I still don't
> fully understand it:
> 
> The Licenses/README lists the licenses and GPL-2.0+ appears in there.
> In the source tree:
> 
> $ git grep GPL-2.0+ |wc -l
> 13406
> $ git grep GPL-2.0-or-later |wc -l
> 1847
> 
> I have to say I much prefer GPL-2.0+ as it is easier to remember.
> 
> But if we are planning to change, could you update checkpatch to throw
> a warning?

As I've said before too, GPL-2.0+ is deprecated by SPDX and
GPL-2.0-or-later is the correct tag. But we aren't, sadly, right now a
best practices example for SPDX anyhow and so it's not a deal breaker to
use the old tag, just something that should be avoided.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 3/8] doc: test: Add docs and test for part_find

[Simon Glass]

Hi Tom,

On Tue, 10 Dec 2024 at 10:09, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Dec 10, 2024 at 09:16:57AM -0700, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Tue, 10 Dec 2024 at 01:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > > On 09.12.24 17:27, Simon Glass wrote:
> > > > Add some documentation and a test for this new command.
> > >
> > > Shouldn't this be two patches?
> >
> > Often we put the new command, its docs and tests in the same commit,
> > since the question I always ask when looking at a command is, where
> > are the docs and tests!
> >
> > >
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > ---
> > > >
> > > >   doc/usage/cmd/part_find.rst | 119 ++++++++++++++++++++++++++++++++++++
> > > >   doc/usage/index.rst         |   1 +
> > > >   test/cmd/Makefile           |   1 +
> > > >   test/cmd/part_find.c        |  42 +++++++++++++
> > > >   4 files changed, 163 insertions(+)
> > > >   create mode 100644 doc/usage/cmd/part_find.rst
> > > >   create mode 100644 test/cmd/part_find.c
> > > >
> > > > diff --git a/doc/usage/cmd/part_find.rst b/doc/usage/cmd/part_find.rst
> > > > new file mode 100644
> > > > index 00000000000..fd5bd6578d5
> > > > --- /dev/null
> > > > +++ b/doc/usage/cmd/part_find.rst
> > > > @@ -0,0 +1,119 @@
> > > > +.. SPDX-License-Identifier: GPL-2.0+:
> > >
> > > This is not a valid SPDX identifier.
> > > Cf. https://spdx.org/licenses/GPL-2.0-or-later.html
> >
> > I have seen this point made a few times, but I'm afraid I still don't
> > fully understand it:
> >
> > The Licenses/README lists the licenses and GPL-2.0+ appears in there.
> > In the source tree:
> >
> > $ git grep GPL-2.0+ |wc -l
> > 13406
> > $ git grep GPL-2.0-or-later |wc -l
> > 1847
> >
> > I have to say I much prefer GPL-2.0+ as it is easier to remember.
> >
> > But if we are planning to change, could you update checkpatch to throw
> > a warning?
>
> As I've said before too, GPL-2.0+ is deprecated by SPDX and
> GPL-2.0-or-later is the correct tag. But we aren't, sadly, right now a
> best practices example for SPDX anyhow and so it's not a deal breaker to
> use the old tag, just something that should be avoided.

OK, I will try to remember this.

At minimum, if this is important, Licenses/README should be updated to
drop the old license?

Heinrich, please update checkpatch to warn about this.

Regards,
Simon

Re: [PATCH 3/8] doc: test: Add docs and test for part_find

[Heinrich Schuchardt]

Am 16. Dezember 2024 01:29:04 MEZ schrieb Simon Glass <sjg@chromium.org>:
>Hi Tom,
>
>On Tue, 10 Dec 2024 at 10:09, Tom Rini <trini@konsulko.com> wrote:
>>
>> On Tue, Dec 10, 2024 at 09:16:57AM -0700, Simon Glass wrote:
>> > Hi Heinrich,
>> >
>> > On Tue, 10 Dec 2024 at 01:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> > >
>> > > On 09.12.24 17:27, Simon Glass wrote:
>> > > > Add some documentation and a test for this new command.
>> > >
>> > > Shouldn't this be two patches?
>> >
>> > Often we put the new command, its docs and tests in the same commit,
>> > since the question I always ask when looking at a command is, where
>> > are the docs and tests!
>> >
>> > >
>> > > >
>> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
>> > > > ---
>> > > >
>> > > >   doc/usage/cmd/part_find.rst | 119 ++++++++++++++++++++++++++++++++++++
>> > > >   doc/usage/index.rst         |   1 +
>> > > >   test/cmd/Makefile           |   1 +
>> > > >   test/cmd/part_find.c        |  42 +++++++++++++
>> > > >   4 files changed, 163 insertions(+)
>> > > >   create mode 100644 doc/usage/cmd/part_find.rst
>> > > >   create mode 100644 test/cmd/part_find.c
>> > > >
>> > > > diff --git a/doc/usage/cmd/part_find.rst b/doc/usage/cmd/part_find.rst
>> > > > new file mode 100644
>> > > > index 00000000000..fd5bd6578d5
>> > > > --- /dev/null
>> > > > +++ b/doc/usage/cmd/part_find.rst
>> > > > @@ -0,0 +1,119 @@
>> > > > +.. SPDX-License-Identifier: GPL-2.0+:
>> > >
>> > > This is not a valid SPDX identifier.
>> > > Cf. https://spdx.org/licenses/GPL-2.0-or-later.html
>> >
>> > I have seen this point made a few times, but I'm afraid I still don't
>> > fully understand it:
>> >
>> > The Licenses/README lists the licenses and GPL-2.0+ appears in there.
>> > In the source tree:
>> >
>> > $ git grep GPL-2.0+ |wc -l
>> > 13406
>> > $ git grep GPL-2.0-or-later |wc -l
>> > 1847
>> >
>> > I have to say I much prefer GPL-2.0+ as it is easier to remember.
>> >
>> > But if we are planning to change, could you update checkpatch to throw
>> > a warning?
>>
>> As I've said before too, GPL-2.0+ is deprecated by SPDX and
>> GPL-2.0-or-later is the correct tag. But we aren't, sadly, right now a
>> best practices example for SPDX anyhow and so it's not a deal breaker to
>> use the old tag, just something that should be avoided.
>
>OK, I will try to remember this.
>
>At minimum, if this is important, Licenses/README should be updated to
>drop the old license?
>
>Heinrich, please update checkpatch to warn about this.
>
>Regards,
>Simon

See
<https://lore.kernel.org/u-boot/20241215023732.68902-1-heinrich.schuchardt@canonical.com/>

Re: [PATCH 8/8] efi: Avoid a memory leak in efi_bind_block() on error path

[Simon Glass]

Hi Heinrich,

On Tue, 10 Dec 2024 at 01:15, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 09.12.24 17:28, Simon Glass wrote:
> > Free the memory used if an error occurs.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > Suggested-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >
> >   lib/efi/efi_app_init.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/efi/efi_app_init.c b/lib/efi/efi_app_init.c
> > index cc91e1d74b8..fec3f348cfb 100644
> > --- a/lib/efi/efi_app_init.c
> > +++ b/lib/efi/efi_app_init.c
> > @@ -60,8 +60,10 @@ int efi_bind_block(efi_handle_t handle, struct efi_block_io *blkio,
> >       plat->handle = handle;
> >       plat->blkio = blkio;
> >       plat->device_path = malloc(device_path_len);
> > -     if (!plat->device_path)
> > +     if (!plat->device_path) {
> > +             free(plat);
>
> As of origin master origin/next (9dd0a9ecaa539ad) plat is a local
> variable and not a pointer. You cannot free it.
>
> If there is some prerequisite patch that is making plat a pointer, you
> should add this correction to that patch.

This patch is for sjg/master as the original series has not landed in
Tom's tree (yet).

>
> Best regards
>
> Heinrich
>
> >               return log_msg_ret("path", -ENOMEM);
> > +     }
> >       memcpy(plat->device_path, device_path, device_path_len);
> >       ret = device_bind(dm_root(), DM_DRIVER_GET(efi_media), "efi_media",
> >                         plat, ofnode_null(), &dev);
>

Regards,
Simon