[PATCH 0/8] efi: Add docs and tests and a few tweaks
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=*&a...
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
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;
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);
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);
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
AttributesType GUIDPartition GUID- 1 0x0000202f 0x0000282e ""
attrs: 0x0000000000000000type: ebd0a0a2-b9e5-4433-87c0-68b6b72699c7(data)guid: 6b1e51e3-427c-9f45-a947-e467b7216356- 2 0x0000002d 0x0000082c ""
attrs: 0x0000000000000000type: fe3a2a5d-4f32-41a7-b725-accc3285a309(cros-kern)guid: dece619f-4876-e140-a6c9-8c208a0c9099- 3 0x0000202e 0x0000202e ""
attrs: 0x0000000000000000type: 3cb8e202-3b7e-47dd-8a3c-7ff2a13cfcec(cros-root)guid: 078cee87-a195-ae4c-a974-8ba3a3d783b3- 4 0x0000082d 0x0000102c ""
attrs: 0x0000000000000000type: fe3a2a5d-4f32-41a7-b725-accc3285a309(cros-kern)guid: 08d2f20f-d941-fc43-96f6-948931289d71- 5 0x0000202d 0x0000202d ""
attrs: 0x0000000000000000type: 3cb8e202-3b7e-47dd-8a3c-7ff2a13cfcec(cros-root)guid: 0b23ba00-a11c-ed4e-8b49-5e8738899569- 6 0x00000029 0x00000029 ""
attrs: 0x0000000000000000type: fe3a2a5d-4f32-41a7-b725-accc3285a309(cros-kern)guid: 6d8158a8-f82d-0d4d-8983-a3ada4eb9b73- 7 0x0000002a 0x0000002a ""
attrs: 0x0000000000000000type: 3cb8e202-3b7e-47dd-8a3c-7ff2a13cfcec(cros-root)guid: 76e8f9b0-7db7-3844-8f18-21de93485211- 8 0x0000102d 0x0000182c ""
attrs: 0x0000000000000000type: ebd0a0a2-b9e5-4433-87c0-68b6b72699c7(data)guid: 071dfd2d-173c-f64b-9474-3318665e1d24- 9 0x0000002b 0x0000002b ""
attrs: 0x0000000000000000type: 2e0a753d-9e48-43b0-8337-b15192cb1b5e(cros-rsrv)guid: b9d078c3-bafa-cd48-b771-a0aaa18d5008- 10 0x0000002c 0x0000002c ""
attrs: 0x0000000000000000type: 2e0a753d-9e48-43b0-8337-b15192cb1b5e(cros-rsrv)guid: 7b0c0234-1a29-0c4f-bceb-40fae8f7b27c- 11 0x00000028 0x00000028 ""
attrs: 0x0000000000000000type: cab6e88e-abf3-4102-a07a-d4bb9be3c1d3(cros-fw)guid: aced715d-cd1f-394a-9e3e-24b54a7b1472- 12 0x0000182d 0x0000202c ""
attrs: 0x0000000000000000type: 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);
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
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.
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
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/
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>
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; } }
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"
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);
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);
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);
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
participants (3)
-
Heinrich Schuchardt -
Simon Glass -
Tom Rini