[PATCH v2 0/4] cmd: source: Support specifying config name

This series adds support for using configs with the source command. See the third patch for details.
Changes in v2: - Add test for source command
Sean Anderson (4): test: Add test for source command treewide: Use NULL for script image name cmd: source: Clean up a few lines cmd: source: Support specifying config name
.../cmd_stm32prog/cmd_stm32prog.c | 2 +- boot/bootmeth_script.c | 2 +- cmd/source.c | 83 +++++++++++++------ doc/uImage.FIT/source_file_format.txt | 3 + drivers/usb/gadget/f_sdp.c | 2 +- include/image.h | 19 +++-- test/py/tests/source.its | 43 ++++++++++ test/py/tests/test_source.py | 37 +++++++++ 8 files changed, 156 insertions(+), 35 deletions(-) create mode 100644 test/py/tests/source.its create mode 100644 test/py/tests/test_source.py

This adds a basic test for FIT image handling by the source command. It's a python test becase we need to run mkimage.
Signed-off-by: Sean Anderson sean.anderson@seco.com ---
Changes in v2: - New
test/py/tests/source.its | 43 ++++++++++++++++++++++++++++++++++++ test/py/tests/test_source.py | 28 +++++++++++++++++++++++ 2 files changed, 71 insertions(+) create mode 100644 test/py/tests/source.its create mode 100644 test/py/tests/test_source.py
diff --git a/test/py/tests/source.its b/test/py/tests/source.its new file mode 100644 index 00000000000..3c62f777f17 --- /dev/null +++ b/test/py/tests/source.its @@ -0,0 +1,43 @@ +/dts-v1/; + +/ { + description = "FIT image to test the source command"; + #address-cells = <1>; + + images { + default = "script-1"; + + script-1 { + data = "echo 1"; + type = "script"; + arch = "sandbox"; + compression = "none"; + }; + + script-2 { + data = "echo 2"; + type = "script"; + arch = "sandbox"; + compression = "none"; + }; + + not-a-script { + data = "echo 3"; + type = "kernel"; + arch = "sandbox"; + compression = "none"; + }; + }; + + configurations { + default = "conf-2"; + + conf-1 { + script = "script-1"; + }; + + conf-2 { + script = "script-2"; + }; + }; +}; diff --git a/test/py/tests/test_source.py b/test/py/tests/test_source.py new file mode 100644 index 00000000000..85751ee6f76 --- /dev/null +++ b/test/py/tests/test_source.py @@ -0,0 +1,28 @@ +# SPDX-License-Identifier: GPL-2.0+ +# Copyright (C) 2022 Sean Anderson sean.anderson@seco.com + +import os +import pytest +import u_boot_utils as util + +@pytest.mark.boardspec('sandbox') +@pytest.mark.buildconfigspec('cmd_echo') +@pytest.mark.buildconfigspec('cmd_source') +@pytest.mark.buildconfigspec('fit') +def test_source(u_boot_console): + # Compile our test script image + cons = u_boot_console + mkimage = os.path.join(cons.config.build_dir, "tools/mkimage") + its = os.path.join(cons.config.source_dir, "test/py/tests/source.its") + fit = os.path.join(cons.config.build_dir, "source.itb") + util.run_and_log(cons, (mkimage, '-f', its, fit)) + cons.run_command(f"host load hostfs - $loadaddr {fit}") + + assert "1" in cons.run_command("source") + assert "1" in cons.run_command("source :script-1") + assert "2" in cons.run_command("source :script-2") + assert "Fail" in cons.run_command("source :not-a-script || echo Fail") + + cons.run_command("fdt addr $loadaddr") + cons.run_command("fdt rm /images default") + assert "Fail" in cons.run_command("source || echo Fail")

On Thu, 20 Oct 2022 at 13:24, Sean Anderson sean.anderson@seco.com wrote:
This adds a basic test for FIT image handling by the source command. It's a python test becase we need to run mkimage.
Signed-off-by: Sean Anderson sean.anderson@seco.com
Changes in v2:
- New
test/py/tests/source.its | 43 ++++++++++++++++++++++++++++++++++++ test/py/tests/test_source.py | 28 +++++++++++++++++++++++ 2 files changed, 71 insertions(+) create mode 100644 test/py/tests/source.its create mode 100644 test/py/tests/test_source.py
Reviewed-by: Simon Glass sjg@chromium.org
Please use single quotes in Python

On Thu, Oct 20, 2022 at 03:24:01PM -0400, Sean Anderson wrote:
This adds a basic test for FIT image handling by the source command. It's a python test becase we need to run mkimage.
Signed-off-by: Sean Anderson sean.anderson@seco.com Reviewed-by: Simon Glass sjg@chromium.org
Changes in v2:
- New
The series needs to be rebased on to master now that Simon's VPL series has been merged, sorry.

Two callers of image_source_script specify an image name. However, both use the deprecated @ syntax, indicating that they have not been updated in a while. If CONFIG_FIT_SIGNATURE is enabled, we will reject such names outright. Back in commit 152576a598c ("stm32mp: stm32prog: handle U-Boot script in flashlayout alternate"), we even renamed one of the nodes. Instead of hard-coding a script image name, just use the default image.
Signed-off-by: Sean Anderson sean.anderson@seco.com Reviewed-by: Simon Glass sjg@chromium.org --- This has a non-zero chance of breaking something, especially for SDP. It's not strictly necessary for the following patches, so I can drop it if there are any issues.
(no changes since v1)
arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c | 2 +- drivers/usb/gadget/f_sdp.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c b/arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c index f59414e716f..9b4098da461 100644 --- a/arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c +++ b/arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c @@ -154,7 +154,7 @@ static int do_stm32prog(struct cmd_tbl *cmdtp, int flag, int argc, do_bootz(cmdtp, 0, 4, bootm_argv); } if (data->script) - image_source_script(data->script, "script@stm32prog"); + image_source_script(data->script, NULL);
if (reset) { puts("Reset...\n"); diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c index 0fa7230b992..c80698663cd 100644 --- a/drivers/usb/gadget/f_sdp.c +++ b/drivers/usb/gadget/f_sdp.c @@ -868,7 +868,7 @@ static int sdp_handle_in_ep(struct spl_image_info *spl_image, jump_to_image_no_args(&spl_image); #else /* In U-Boot, allow jumps to scripts */ - image_source_script(sdp_func->jmp_address, "script@1"); + image_source_script(sdp_func->jmp_address, NULL); #endif }

Hi,
On 10/20/22 21:24, Sean Anderson wrote:
Two callers of image_source_script specify an image name. However, both use the deprecated @ syntax, indicating that they have not been updated in a while. If CONFIG_FIT_SIGNATURE is enabled, we will reject such names outright. Back in commit 152576a598c ("stm32mp: stm32prog: handle U-Boot script in flashlayout alternate"), we even renamed one of the nodes. Instead of hard-coding a script image name, just use the default image.
Signed-off-by: Sean Anderson sean.anderson@seco.com Reviewed-by: Simon Glass sjg@chromium.org
This has a non-zero chance of breaking something, especially for SDP. It's not strictly necessary for the following patches, so I can drop it if there are any issues.
(no changes since v1)
arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c | 2 +- drivers/usb/gadget/f_sdp.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c b/arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c index f59414e716f..9b4098da461 100644 --- a/arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c +++ b/arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c @@ -154,7 +154,7 @@ static int do_stm32prog(struct cmd_tbl *cmdtp, int flag, int argc, do_bootz(cmdtp, 0, 4, bootm_argv); } if (data->script)
image_source_script(data->script, "script@stm32prog");
image_source_script(data->script, NULL);
if (reset) { puts("Reset...\n");
diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c index 0fa7230b992..c80698663cd 100644 --- a/drivers/usb/gadget/f_sdp.c +++ b/drivers/usb/gadget/f_sdp.c @@ -868,7 +868,7 @@ static int sdp_handle_in_ep(struct spl_image_info *spl_image, jump_to_image_no_args(&spl_image); #else /* In U-Boot, allow jumps to scripts */
image_source_script(sdp_func->jmp_address, "script@1");
#endif }image_source_script(sdp_func->jmp_address, NULL);
Reviewed-by: Patrick Delaunay patrick.delaunay@foss.st.com
for the stm32mp part: arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c
Thanks Patrick

This simplifies a few lines and corrects an error message.
Signed-off-by: Sean Anderson sean.anderson@seco.com Reviewed-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
cmd/source.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/cmd/source.c b/cmd/source.c index bb98255fe86..f08e6d34172 100644 --- a/cmd/source.c +++ b/cmd/source.c @@ -128,16 +128,14 @@ int image_source_script(ulong addr, const char *fit_uname) }
if (!fit_image_check_type (fit_hdr, noffset, IH_TYPE_SCRIPT)) { - puts ("Not a image image\n"); + puts("Not a script image\n"); return 1; }
/* verify integrity */ - if (verify) { - if (!fit_image_verify(fit_hdr, noffset)) { - puts ("Bad Data Hash\n"); - return 1; - } + if (verify && !fit_image_verify(fit_hdr, noffset)) { + puts("Bad Data Hash\n"); + return 1; }
/* get script subimage data address and length */

As discussed previously [1,2], the source command is not safe to use with verified boot unless there is a key with required = "images" (which has its own problems). This is because if such a key is absent, signatures are verified but not required. It is assumed that configuration nodes will provide the signature. Because the source command does not use configurations to determine the image to source, effectively no verification takes place.
To address this, allow specifying configuration nodes. We use the same syntax as the bootm command (helpfully provided for us by fit_parse_conf). By default, we first try the default config and then the default image. To force using a config, # must be present in the command (e.g. `source $loadaddr#my-conf`). For convenience, the config may be omitted, just like the address may be (e.g. `source #`). This also works for images (`source :` behaves exactly like `source` currently does).
[1] https://lore.kernel.org/u-boot/7d711133-d513-5bcb-52f2-a9dbaa9eeded@prevas.d... [2] https://lore.kernel.org/u-boot/042dcb34-f85f-351e-1b0e-513f89005fdd@gmail.co...
Signed-off-by: Sean Anderson sean.anderson@seco.com ---
(no changes since v1)
.../cmd_stm32prog/cmd_stm32prog.c | 2 +- boot/bootmeth_script.c | 2 +- cmd/source.c | 73 +++++++++++++------ doc/uImage.FIT/source_file_format.txt | 3 + drivers/usb/gadget/f_sdp.c | 2 +- include/image.h | 19 +++-- test/py/tests/test_source.py | 11 ++- 7 files changed, 82 insertions(+), 30 deletions(-)
diff --git a/arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c b/arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c index 9b4098da461..b26dcfddbc6 100644 --- a/arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c +++ b/arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c @@ -154,7 +154,7 @@ static int do_stm32prog(struct cmd_tbl *cmdtp, int flag, int argc, do_bootz(cmdtp, 0, 4, bootm_argv); } if (data->script) - image_source_script(data->script, NULL); + image_source_script(data->script, NULL, NULL);
if (reset) { puts("Reset...\n"); diff --git a/boot/bootmeth_script.c b/boot/bootmeth_script.c index d1c3f940037..6c84721d1cd 100644 --- a/boot/bootmeth_script.c +++ b/boot/bootmeth_script.c @@ -101,7 +101,7 @@ static int script_boot(struct udevice *dev, struct bootflow *bflow) log_debug("mmc_bootdev: %s\n", env_get("mmc_bootdev"));
addr = map_to_sysmem(bflow->buf); - ret = image_source_script(addr, NULL); + ret = image_source_script(addr, NULL, NULL); if (ret) return log_msg_ret("boot", ret);
diff --git a/cmd/source.c b/cmd/source.c index f08e6d34172..17300bdc746 100644 --- a/cmd/source.c +++ b/cmd/source.c @@ -42,7 +42,7 @@ static const char *get_default_image(const void *fit) } #endif
-int image_source_script(ulong addr, const char *fit_uname) +int image_source_script(ulong addr, const char *fit_uname, const char *confname) { ulong len; #if defined(CONFIG_LEGACY_IMAGE_FORMAT) @@ -112,19 +112,46 @@ int image_source_script(ulong addr, const char *fit_uname) return 1; }
- if (!fit_uname) - fit_uname = get_default_image(fit_hdr); - if (!fit_uname) { - puts("No FIT subimage unit name\n"); - return 1; - } + /* If confname is empty, use the default */ + if (confname && *confname) + noffset = fit_conf_get_node(fit_hdr, confname); + else + noffset = fit_conf_get_node(fit_hdr, NULL); + if (noffset < 0) { + if (!confname) + goto fallback; + printf("Could not find config %s\n", confname); + return 1; + }
- /* get script component image node offset */ - noffset = fit_image_get_node (fit_hdr, fit_uname); - if (noffset < 0) { - printf ("Can't find '%s' FIT subimage\n", fit_uname); - return 1; + if (verify && fit_config_verify(fit_hdr, noffset)) + return 1; + + noffset = fit_conf_get_prop_node(fit_hdr, noffset, + FIT_SCRIPT_PROP); + if (noffset < 0) { + if (!confname) + goto fallback; + printf("Could not find script in %s\n", confname); + return 1; + } + } else { +fallback: + if (!fit_uname || !*fit_uname) + fit_uname = get_default_image(fit_hdr); + if (!fit_uname) { + puts("No FIT subimage unit name\n"); + return 1; + } + + /* get script component image node offset */ + noffset = fit_image_get_node(fit_hdr, fit_uname); + if (noffset < 0) { + printf("Can't find '%s' FIT subimage\n", + fit_uname); + return 1; + } }
if (!fit_image_check_type (fit_hdr, noffset, IH_TYPE_SCRIPT)) { @@ -164,7 +191,7 @@ static int do_source(struct cmd_tbl *cmdtp, int flag, int argc, { ulong addr; int rcode; - const char *fit_uname = NULL; + const char *fit_uname = NULL, *confname = NULL;
/* Find script image */ if (argc < 2) { @@ -175,6 +202,9 @@ static int do_source(struct cmd_tbl *cmdtp, int flag, int argc, &fit_uname)) { debug("* source: subimage '%s' from FIT image at 0x%08lx\n", fit_uname, addr); + } else if (fit_parse_conf(argv[1], image_load_addr, &addr, &confname)) { + debug("* source: config '%s' from FIT image at 0x%08lx\n", + confname, addr); #endif } else { addr = hextoul(argv[1], NULL); @@ -182,21 +212,22 @@ static int do_source(struct cmd_tbl *cmdtp, int flag, int argc, }
printf ("## Executing script at %08lx\n", addr); - rcode = image_source_script(addr, fit_uname); + rcode = image_source_script(addr, fit_uname, confname); return rcode; }
#ifdef CONFIG_SYS_LONGHELP static char source_help_text[] = - "[addr]\n" - "\t- run script starting at addr\n" - "\t- A valid image header must be present" #if defined(CONFIG_FIT) - "\n" - "For FIT format uImage addr must include subimage\n" - "unit name in the form of addr:<subimg_uname>" + "[<addr>][:[<image>]|#[<config>]]\n" + "\t- Run script starting at addr\n" + "\t- A FIT config name or subimage name may be specified with : or #\n" + "\t (like bootm). If the image or config name is omitted, the\n" + "\t default is used."; +#else + "[<addr>]\n" + "\t- Run script starting at addr"; #endif - ""; #endif
U_BOOT_CMD( diff --git a/doc/uImage.FIT/source_file_format.txt b/doc/uImage.FIT/source_file_format.txt index f93ac6d1c7b..fae81d4a9b7 100644 --- a/doc/uImage.FIT/source_file_format.txt +++ b/doc/uImage.FIT/source_file_format.txt @@ -239,6 +239,7 @@ o config-1 |- kernel = "kernel sub-node unit name" |- fdt = "fdt sub-node unit-name" [, "fdt overlay sub-node unit-name", ...] |- loadables = "loadables sub-node unit-name" + |- script = " |- compatible = "vendor,board-style device tree compatible string"
@@ -260,6 +261,8 @@ o config-1 of strings. U-Boot will load each binary at its given start-address and may optionally invoke additional post-processing steps on this binary based on its component image node type. + - script : The image to use when loading a U-Boot script (for use with the + source command). - compatible : The root compatible string of the U-Boot device tree that this configuration shall automatically match when CONFIG_FIT_BEST_MATCH is enabled. If this property is not provided, the compatible string will be diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c index c80698663cd..8bded9231ce 100644 --- a/drivers/usb/gadget/f_sdp.c +++ b/drivers/usb/gadget/f_sdp.c @@ -868,7 +868,7 @@ static int sdp_handle_in_ep(struct spl_image_info *spl_image, jump_to_image_no_args(&spl_image); #else /* In U-Boot, allow jumps to scripts */ - image_source_script(sdp_func->jmp_address, NULL); + image_source_script(sdp_func->jmp_address, NULL, NULL); #endif }
diff --git a/include/image.h b/include/image.h index d7d756c6453..a83d3699e98 100644 --- a/include/image.h +++ b/include/image.h @@ -634,15 +634,23 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
/** * image_source_script() - Execute a script - * - * Executes a U-Boot script at a particular address in memory. The script should - * have a header (FIT or legacy) with the script type (IH_TYPE_SCRIPT). - * * @addr: Address of script * @fit_uname: FIT subimage name + * @confname: FIT config name. The subimage is chosen based on FIT_SCRIPT_PROP. + * + * Executes a U-Boot script at a particular address in memory. The script should + * have a header (FIT or legacy) with the script type (IH_TYPE_SCRIPT). + * + * If @fit_uname is the empty string, then the default image is used. If + * @confname is the empty string, the default config is used. If @confname and + * @fit_uname are both non-%NULL, then @confname is ignored. If @confname and + * @fit_uname are both %NULL, then first the default config is tried, and then + * the default image. + * * Return: result code (enum command_ret_t) */ -int image_source_script(ulong addr, const char *fit_uname); +int image_source_script(ulong addr, const char *fit_uname, + const char *confname);
/** * fit_get_node_from_config() - Look up an image a FIT by type @@ -945,6 +953,7 @@ int booti_setup(ulong image, ulong *relocated_addr, ulong *size, #define FIT_FPGA_PROP "fpga" #define FIT_FIRMWARE_PROP "firmware" #define FIT_STANDALONE_PROP "standalone" +#define FIT_SCRIPT_PROP "script"
#define FIT_MAX_HASH_LEN HASH_MAX_DIGEST_SIZE
diff --git a/test/py/tests/test_source.py b/test/py/tests/test_source.py index 85751ee6f76..91ad379d6f4 100644 --- a/test/py/tests/test_source.py +++ b/test/py/tests/test_source.py @@ -18,11 +18,20 @@ def test_source(u_boot_console): util.run_and_log(cons, (mkimage, '-f', its, fit)) cons.run_command(f"host load hostfs - $loadaddr {fit}")
- assert "1" in cons.run_command("source") + assert "2" in cons.run_command("source") + assert "1" in cons.run_command("source :") assert "1" in cons.run_command("source :script-1") assert "2" in cons.run_command("source :script-2") assert "Fail" in cons.run_command("source :not-a-script || echo Fail") + assert "2" in cons.run_command("source \#") + assert "1" in cons.run_command("source \#conf-1") + assert "2" in cons.run_command("source \#conf-2")
cons.run_command("fdt addr $loadaddr") + cons.run_command("fdt rm /configurations default") + assert "1" in cons.run_command("source") + assert "Fail" in cons.run_command("source \# || echo Fail") + cons.run_command("fdt rm /images default") assert "Fail" in cons.run_command("source || echo Fail") + assert "Fail" in cons.run_command("source \# || echo Fail")

On Thu, 20 Oct 2022 at 13:24, Sean Anderson sean.anderson@seco.com wrote:
As discussed previously [1,2], the source command is not safe to use with verified boot unless there is a key with required = "images" (which has its own problems). This is because if such a key is absent, signatures are verified but not required. It is assumed that configuration nodes will provide the signature. Because the source command does not use configurations to determine the image to source, effectively no verification takes place.
To address this, allow specifying configuration nodes. We use the same syntax as the bootm command (helpfully provided for us by fit_parse_conf). By default, we first try the default config and then the default image. To force using a config, # must be present in the command (e.g. `source $loadaddr#my-conf`). For convenience, the config may be omitted, just like the address may be (e.g. `source #`). This also works for images (`source :` behaves exactly like `source` currently does).
[1] https://lore.kernel.org/u-boot/7d711133-d513-5bcb-52f2-a9dbaa9eeded@prevas.d... [2] https://lore.kernel.org/u-boot/042dcb34-f85f-351e-1b0e-513f89005fdd@gmail.co...
Signed-off-by: Sean Anderson sean.anderson@seco.com
(no changes since v1)
.../cmd_stm32prog/cmd_stm32prog.c | 2 +- boot/bootmeth_script.c | 2 +- cmd/source.c | 73 +++++++++++++------ doc/uImage.FIT/source_file_format.txt | 3 + drivers/usb/gadget/f_sdp.c | 2 +- include/image.h | 19 +++-- test/py/tests/test_source.py | 11 ++- 7 files changed, 82 insertions(+), 30 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
But please use single quotes in Python. Double quotes should only be used when the string includes single quotes.

On 10/21/22 4:17 PM, Simon Glass wrote:
On Thu, 20 Oct 2022 at 13:24, Sean Anderson sean.anderson@seco.com wrote:
As discussed previously [1,2], the source command is not safe to use with verified boot unless there is a key with required = "images" (which has its own problems). This is because if such a key is absent, signatures are verified but not required. It is assumed that configuration nodes will provide the signature. Because the source command does not use configurations to determine the image to source, effectively no verification takes place.
To address this, allow specifying configuration nodes. We use the same syntax as the bootm command (helpfully provided for us by fit_parse_conf). By default, we first try the default config and then the default image. To force using a config, # must be present in the command (e.g. `source $loadaddr#my-conf`). For convenience, the config may be omitted, just like the address may be (e.g. `source #`). This also works for images (`source :` behaves exactly like `source` currently does).
[1] https://lore.kernel.org/u-boot/7d711133-d513-5bcb-52f2-a9dbaa9eeded@prevas.d... [2] https://lore.kernel.org/u-boot/042dcb34-f85f-351e-1b0e-513f89005fdd@gmail.co...
Signed-off-by: Sean Anderson sean.anderson@seco.com
(no changes since v1)
.../cmd_stm32prog/cmd_stm32prog.c | 2 +- boot/bootmeth_script.c | 2 +- cmd/source.c | 73 +++++++++++++------ doc/uImage.FIT/source_file_format.txt | 3 + drivers/usb/gadget/f_sdp.c | 2 +- include/image.h | 19 +++-- test/py/tests/test_source.py | 11 ++- 7 files changed, 82 insertions(+), 30 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
But please use single quotes in Python. Double quotes should only be used when the string includes single quotes.
Do we have a style guide for python? Judging by `git grep '"' '**.py'`, double quoting is endemic. IMO single quotes should be used for identifiers (or things which would be enums in C), and double quotes elsewhere. But if you want to go the other way, perhaps add something to checkpatch.
--Sean

Hi Sean,
On Fri, 21 Oct 2022 at 15:04, Sean Anderson sean.anderson@seco.com wrote:
On 10/21/22 4:17 PM, Simon Glass wrote:
On Thu, 20 Oct 2022 at 13:24, Sean Anderson sean.anderson@seco.com wrote:
As discussed previously [1,2], the source command is not safe to use with verified boot unless there is a key with required = "images" (which has its own problems). This is because if such a key is absent, signatures are verified but not required. It is assumed that configuration nodes will provide the signature. Because the source command does not use configurations to determine the image to source, effectively no verification takes place.
To address this, allow specifying configuration nodes. We use the same syntax as the bootm command (helpfully provided for us by fit_parse_conf). By default, we first try the default config and then the default image. To force using a config, # must be present in the command (e.g. `source $loadaddr#my-conf`). For convenience, the config may be omitted, just like the address may be (e.g. `source #`). This also works for images (`source :` behaves exactly like `source` currently does).
[1] https://lore.kernel.org/u-boot/7d711133-d513-5bcb-52f2-a9dbaa9eeded@prevas.d... [2] https://lore.kernel.org/u-boot/042dcb34-f85f-351e-1b0e-513f89005fdd@gmail.co...
Signed-off-by: Sean Anderson sean.anderson@seco.com
(no changes since v1)
.../cmd_stm32prog/cmd_stm32prog.c | 2 +- boot/bootmeth_script.c | 2 +- cmd/source.c | 73 +++++++++++++------ doc/uImage.FIT/source_file_format.txt | 3 + drivers/usb/gadget/f_sdp.c | 2 +- include/image.h | 19 +++-- test/py/tests/test_source.py | 11 ++- 7 files changed, 82 insertions(+), 30 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
But please use single quotes in Python. Double quotes should only be used when the string includes single quotes.
Do we have a style guide for python? Judging by `git grep '"' '**.py'`, double quoting is endemic. IMO single quotes should be used for identifiers (or things which would be enums in C), and double quotes elsewhere. But if you want to go the other way, perhaps add something to checkpatch.
Well we use PEP8, with single quotes used for nearly everything. The exceptions are the one I mentioned, and module/function comments.
Do we use checkpatch for Python?
Regards, Simon

On Sat, Oct 29, 2022 at 07:44:00PM -0600, Simon Glass wrote:
Hi Sean,
On Fri, 21 Oct 2022 at 15:04, Sean Anderson sean.anderson@seco.com wrote:
On 10/21/22 4:17 PM, Simon Glass wrote:
On Thu, 20 Oct 2022 at 13:24, Sean Anderson sean.anderson@seco.com wrote:
As discussed previously [1,2], the source command is not safe to use with verified boot unless there is a key with required = "images" (which has its own problems). This is because if such a key is absent, signatures are verified but not required. It is assumed that configuration nodes will provide the signature. Because the source command does not use configurations to determine the image to source, effectively no verification takes place.
To address this, allow specifying configuration nodes. We use the same syntax as the bootm command (helpfully provided for us by fit_parse_conf). By default, we first try the default config and then the default image. To force using a config, # must be present in the command (e.g. `source $loadaddr#my-conf`). For convenience, the config may be omitted, just like the address may be (e.g. `source #`). This also works for images (`source :` behaves exactly like `source` currently does).
[1] https://lore.kernel.org/u-boot/7d711133-d513-5bcb-52f2-a9dbaa9eeded@prevas.d... [2] https://lore.kernel.org/u-boot/042dcb34-f85f-351e-1b0e-513f89005fdd@gmail.co...
Signed-off-by: Sean Anderson sean.anderson@seco.com
(no changes since v1)
.../cmd_stm32prog/cmd_stm32prog.c | 2 +- boot/bootmeth_script.c | 2 +- cmd/source.c | 73 +++++++++++++------ doc/uImage.FIT/source_file_format.txt | 3 + drivers/usb/gadget/f_sdp.c | 2 +- include/image.h | 19 +++-- test/py/tests/test_source.py | 11 ++- 7 files changed, 82 insertions(+), 30 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
But please use single quotes in Python. Double quotes should only be used when the string includes single quotes.
Do we have a style guide for python? Judging by `git grep '"' '**.py'`, double quoting is endemic. IMO single quotes should be used for identifiers (or things which would be enums in C), and double quotes elsewhere. But if you want to go the other way, perhaps add something to checkpatch.
Well we use PEP8, with single quotes used for nearly everything. The exceptions are the one I mentioned, and module/function comments.
Do we use checkpatch for Python?
Is there a standard python PEP8 checking tool? We should see if upstream is interested in a flag or similar to call another tool for python linting.

Hi Tom,
On Sun, 30 Oct 2022 at 08:40, Tom Rini trini@konsulko.com wrote:
On Sat, Oct 29, 2022 at 07:44:00PM -0600, Simon Glass wrote:
Hi Sean,
On Fri, 21 Oct 2022 at 15:04, Sean Anderson sean.anderson@seco.com wrote:
On 10/21/22 4:17 PM, Simon Glass wrote:
On Thu, 20 Oct 2022 at 13:24, Sean Anderson sean.anderson@seco.com wrote:
As discussed previously [1,2], the source command is not safe to use with verified boot unless there is a key with required = "images" (which has its own problems). This is because if such a key is absent, signatures are verified but not required. It is assumed that configuration nodes will provide the signature. Because the source command does not use configurations to determine the image to source, effectively no verification takes place.
To address this, allow specifying configuration nodes. We use the same syntax as the bootm command (helpfully provided for us by fit_parse_conf). By default, we first try the default config and then the default image. To force using a config, # must be present in the command (e.g. `source $loadaddr#my-conf`). For convenience, the config may be omitted, just like the address may be (e.g. `source #`). This also works for images (`source :` behaves exactly like `source` currently does).
[1] https://lore.kernel.org/u-boot/7d711133-d513-5bcb-52f2-a9dbaa9eeded@prevas.d... [2] https://lore.kernel.org/u-boot/042dcb34-f85f-351e-1b0e-513f89005fdd@gmail.co...
Signed-off-by: Sean Anderson sean.anderson@seco.com
(no changes since v1)
.../cmd_stm32prog/cmd_stm32prog.c | 2 +- boot/bootmeth_script.c | 2 +- cmd/source.c | 73 +++++++++++++------ doc/uImage.FIT/source_file_format.txt | 3 + drivers/usb/gadget/f_sdp.c | 2 +- include/image.h | 19 +++-- test/py/tests/test_source.py | 11 ++- 7 files changed, 82 insertions(+), 30 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
But please use single quotes in Python. Double quotes should only be used when the string includes single quotes.
Do we have a style guide for python? Judging by `git grep '"' '**.py'`, double quoting is endemic. IMO single quotes should be used for identifiers (or things which would be enums in C), and double quotes elsewhere. But if you want to go the other way, perhaps add something to checkpatch.
Well we use PEP8, with single quotes used for nearly everything. The exceptions are the one I mentioned, and module/function comments.
Do we use checkpatch for Python?
Is there a standard python PEP8 checking tool? We should see if upstream is interested in a flag or similar to call another tool for python linting.
Yes this is pylint and we do already use it, but not in patman so far. It would be a good thing to add.
Regards, Simon

On Mon, Oct 31, 2022 at 01:27:08PM -0600, Simon Glass wrote:
Hi Tom,
On Sun, 30 Oct 2022 at 08:40, Tom Rini trini@konsulko.com wrote:
On Sat, Oct 29, 2022 at 07:44:00PM -0600, Simon Glass wrote:
Hi Sean,
On Fri, 21 Oct 2022 at 15:04, Sean Anderson sean.anderson@seco.com wrote:
On 10/21/22 4:17 PM, Simon Glass wrote:
On Thu, 20 Oct 2022 at 13:24, Sean Anderson sean.anderson@seco.com wrote:
As discussed previously [1,2], the source command is not safe to use with verified boot unless there is a key with required = "images" (which has its own problems). This is because if such a key is absent, signatures are verified but not required. It is assumed that configuration nodes will provide the signature. Because the source command does not use configurations to determine the image to source, effectively no verification takes place.
To address this, allow specifying configuration nodes. We use the same syntax as the bootm command (helpfully provided for us by fit_parse_conf). By default, we first try the default config and then the default image. To force using a config, # must be present in the command (e.g. `source $loadaddr#my-conf`). For convenience, the config may be omitted, just like the address may be (e.g. `source #`). This also works for images (`source :` behaves exactly like `source` currently does).
[1] https://lore.kernel.org/u-boot/7d711133-d513-5bcb-52f2-a9dbaa9eeded@prevas.d... [2] https://lore.kernel.org/u-boot/042dcb34-f85f-351e-1b0e-513f89005fdd@gmail.co...
Signed-off-by: Sean Anderson sean.anderson@seco.com
(no changes since v1)
.../cmd_stm32prog/cmd_stm32prog.c | 2 +- boot/bootmeth_script.c | 2 +- cmd/source.c | 73 +++++++++++++------ doc/uImage.FIT/source_file_format.txt | 3 + drivers/usb/gadget/f_sdp.c | 2 +- include/image.h | 19 +++-- test/py/tests/test_source.py | 11 ++- 7 files changed, 82 insertions(+), 30 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
But please use single quotes in Python. Double quotes should only be used when the string includes single quotes.
Do we have a style guide for python? Judging by `git grep '"' '**.py'`, double quoting is endemic. IMO single quotes should be used for identifiers (or things which would be enums in C), and double quotes elsewhere. But if you want to go the other way, perhaps add something to checkpatch.
Well we use PEP8, with single quotes used for nearly everything. The exceptions are the one I mentioned, and module/function comments.
Do we use checkpatch for Python?
Is there a standard python PEP8 checking tool? We should see if upstream is interested in a flag or similar to call another tool for python linting.
Yes this is pylint and we do already use it, but not in patman so far. It would be a good thing to add.
I'd start by trying to put it in checkpatch.pl and seeing if upstream is interested, the kernel has a bunch of python tools too these days.

Hi Tom,
On Mon, 31 Oct 2022 at 13:37, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 31, 2022 at 01:27:08PM -0600, Simon Glass wrote:
Hi Tom,
On Sun, 30 Oct 2022 at 08:40, Tom Rini trini@konsulko.com wrote:
On Sat, Oct 29, 2022 at 07:44:00PM -0600, Simon Glass wrote:
Hi Sean,
On Fri, 21 Oct 2022 at 15:04, Sean Anderson sean.anderson@seco.com wrote:
On 10/21/22 4:17 PM, Simon Glass wrote:
On Thu, 20 Oct 2022 at 13:24, Sean Anderson sean.anderson@seco.com wrote: > > As discussed previously [1,2], the source command is not safe to use with > verified boot unless there is a key with required = "images" (which has its > own problems). This is because if such a key is absent, signatures are > verified but not required. It is assumed that configuration nodes will > provide the signature. Because the source command does not use > configurations to determine the image to source, effectively no > verification takes place. > > To address this, allow specifying configuration nodes. We use the same > syntax as the bootm command (helpfully provided for us by fit_parse_conf). > By default, we first try the default config and then the default image. To > force using a config, # must be present in the command (e.g. `source > $loadaddr#my-conf`). For convenience, the config may be omitted, just like > the address may be (e.g. `source #`). This also works for images > (`source :` behaves exactly like `source` currently does). > > [1] https://lore.kernel.org/u-boot/7d711133-d513-5bcb-52f2-a9dbaa9eeded@prevas.d... > [2] https://lore.kernel.org/u-boot/042dcb34-f85f-351e-1b0e-513f89005fdd@gmail.co... > > Signed-off-by: Sean Anderson sean.anderson@seco.com > --- > > (no changes since v1) > > .../cmd_stm32prog/cmd_stm32prog.c | 2 +- > boot/bootmeth_script.c | 2 +- > cmd/source.c | 73 +++++++++++++------ > doc/uImage.FIT/source_file_format.txt | 3 + > drivers/usb/gadget/f_sdp.c | 2 +- > include/image.h | 19 +++-- > test/py/tests/test_source.py | 11 ++- > 7 files changed, 82 insertions(+), 30 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
But please use single quotes in Python. Double quotes should only be used when the string includes single quotes.
Do we have a style guide for python? Judging by `git grep '"' '**.py'`, double quoting is endemic. IMO single quotes should be used for identifiers (or things which would be enums in C), and double quotes elsewhere. But if you want to go the other way, perhaps add something to checkpatch.
Well we use PEP8, with single quotes used for nearly everything. The exceptions are the one I mentioned, and module/function comments.
Do we use checkpatch for Python?
Is there a standard python PEP8 checking tool? We should see if upstream is interested in a flag or similar to call another tool for python linting.
Yes this is pylint and we do already use it, but not in patman so far. It would be a good thing to add.
I'd start by trying to put it in checkpatch.pl and seeing if upstream is interested, the kernel has a bunch of python tools too these days.
Oh dear that seems a bit tricky. Checkpatch does its own checking of C patches. It would be better to just call pylint I think. Adding a call to pylint from checkpatch seems odd, but perhaps it would fly? I don't know how the output would work though. So I think adding to patman makes more sense.
Regards, Simon

On Thu, Nov 03, 2022 at 02:46:35PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 31 Oct 2022 at 13:37, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 31, 2022 at 01:27:08PM -0600, Simon Glass wrote:
Hi Tom,
On Sun, 30 Oct 2022 at 08:40, Tom Rini trini@konsulko.com wrote:
On Sat, Oct 29, 2022 at 07:44:00PM -0600, Simon Glass wrote:
Hi Sean,
On Fri, 21 Oct 2022 at 15:04, Sean Anderson sean.anderson@seco.com wrote:
On 10/21/22 4:17 PM, Simon Glass wrote: > On Thu, 20 Oct 2022 at 13:24, Sean Anderson sean.anderson@seco.com wrote: >> >> As discussed previously [1,2], the source command is not safe to use with >> verified boot unless there is a key with required = "images" (which has its >> own problems). This is because if such a key is absent, signatures are >> verified but not required. It is assumed that configuration nodes will >> provide the signature. Because the source command does not use >> configurations to determine the image to source, effectively no >> verification takes place. >> >> To address this, allow specifying configuration nodes. We use the same >> syntax as the bootm command (helpfully provided for us by fit_parse_conf). >> By default, we first try the default config and then the default image. To >> force using a config, # must be present in the command (e.g. `source >> $loadaddr#my-conf`). For convenience, the config may be omitted, just like >> the address may be (e.g. `source #`). This also works for images >> (`source :` behaves exactly like `source` currently does). >> >> [1] https://lore.kernel.org/u-boot/7d711133-d513-5bcb-52f2-a9dbaa9eeded@prevas.d... >> [2] https://lore.kernel.org/u-boot/042dcb34-f85f-351e-1b0e-513f89005fdd@gmail.co... >> >> Signed-off-by: Sean Anderson sean.anderson@seco.com >> --- >> >> (no changes since v1) >> >> .../cmd_stm32prog/cmd_stm32prog.c | 2 +- >> boot/bootmeth_script.c | 2 +- >> cmd/source.c | 73 +++++++++++++------ >> doc/uImage.FIT/source_file_format.txt | 3 + >> drivers/usb/gadget/f_sdp.c | 2 +- >> include/image.h | 19 +++-- >> test/py/tests/test_source.py | 11 ++- >> 7 files changed, 82 insertions(+), 30 deletions(-) > > Reviewed-by: Simon Glass sjg@chromium.org > > But please use single quotes in Python. Double quotes should only be > used when the string includes single quotes. >
Do we have a style guide for python? Judging by `git grep '"' '**.py'`, double quoting is endemic. IMO single quotes should be used for identifiers (or things which would be enums in C), and double quotes elsewhere. But if you want to go the other way, perhaps add something to checkpatch.
Well we use PEP8, with single quotes used for nearly everything. The exceptions are the one I mentioned, and module/function comments.
Do we use checkpatch for Python?
Is there a standard python PEP8 checking tool? We should see if upstream is interested in a flag or similar to call another tool for python linting.
Yes this is pylint and we do already use it, but not in patman so far. It would be a good thing to add.
I'd start by trying to put it in checkpatch.pl and seeing if upstream is interested, the kernel has a bunch of python tools too these days.
Oh dear that seems a bit tricky. Checkpatch does its own checking of C patches. It would be better to just call pylint I think. Adding a call to pylint from checkpatch seems odd, but perhaps it would fly? I don't know how the output would work though. So I think adding to patman makes more sense.
Yes, sorry, I was suggesting that checkpatch.pl call out to pylint. I assumed it would be straightforward as it already calls out to other tools for things like SPDX tag checking.
participants (4)
-
Patrick DELAUNAY
-
Sean Anderson
-
Simon Glass
-
Tom Rini