[PATCH 00/25] Tidy up use of CONFIG_CMDLINE
It should be possible to disable CONFIG_CMDLINE and have all commands and related functionality dropped from U-Boot. This is useful when trying to reduce the size of U-Boot.
Recent changes have stopped this from working.
This series repairs the feature for sandbox and adds a test to stop it breaking again.
Note that quite a lot of functionality is lost of CONFIG_CMDLINE is disabled, e.g. networking and most booting options. Further work is needed to make the option more generally useful.
Simon Glass (25): buildman: Use oldconfig when adjusting the config bootstd: Correct dependencies on CMDLINE autoboot: Correct dependencies on CMDLINE cmd: Add a few more dependencies on CMDLINE treewide: Correct use of long help test: Make UNIT_TEST depend on CMDLINE tegra: Change #ifdef for nop fastboot: Avoid depending on CMDLINE cli: Always build cli_getch cmd: Use an #ifdef around run_commandf() Move bootmenu_conv_key() into its own file armffa: Correct command help condition pxe: Depend on CMDLINE env: Split out non-command code into a new file console: Move SYS_PBSIZE into common/ bootm: Allow building when cleanup functions are missing fdt: Move working_fdt into fdt_support net: Depend on CONFIG_CMDLINE log: Allow use without CONFIG_CMDLINE video: Allow use without CONFIG_CMDLINE video: Dont require the font command efi: Depend on CMDLINE for efi_loader cmd: Make all commands depend on CMDLINE sandbox: Avoid requiring cmdline sandbox: Add a test for disabling CONFIG_CMDLINE
arch/Kconfig | 6 +- arch/arm/lib/bootm.c | 2 + arch/arm/mach-imx/cmd_dek.c | 3 +- arch/arm/mach-imx/cmd_mfgprot.c | 3 +- arch/arm/mach-imx/imx8/snvs_security_sc.c | 10 ++ arch/arm/mach-stm32mp/cmd_stm32key.c | 2 + board/freescale/common/cmd_esbc_validate.c | 3 +- board/kontron/sl28/cmds.c | 2 + boot/Kconfig | 42 ++++--- boot/bootm.c | 10 +- boot/fdt_support.c | 5 + cmd/Kconfig | 25 ++-- cmd/Makefile | 2 +- cmd/adc.c | 2 + cmd/arm/exception.c | 2 + cmd/arm/exception64.c | 2 + cmd/armffa.c | 2 + cmd/axi.c | 2 + cmd/blob.c | 2 + cmd/cyclic.c | 2 + cmd/fdt.c | 5 - cmd/mux.c | 2 + cmd/nvedit.c | 122 +------------------ cmd/osd.c | 2 + cmd/pcap.c | 2 + cmd/riscv/exception.c | 2 + cmd/sandbox/exception.c | 2 + cmd/scp03.c | 2 + cmd/wdt.c | 2 + cmd/x86/exception.c | 2 + common/Kconfig | 5 + common/Makefile | 3 +- common/cli.c | 2 + common/cli_getch.c | 1 + common/log.c | 4 +- common/menu.c | 40 ------- common/menu_key.c | 49 ++++++++ drivers/fastboot/fb_command.c | 3 +- drivers/fastboot/fb_common.c | 15 ++- drivers/video/Kconfig | 2 +- drivers/video/console_truetype.c | 4 + env/Makefile | 1 + env/env_set.c | 132 +++++++++++++++++++++ include/bootm.h | 15 ++- include/env_internal.h | 23 ++++ include/k210/pll.h | 2 +- lib/efi_loader/Kconfig | 2 + net/Kconfig | 1 + test/Kconfig | 1 + test/py/tests/test_sandbox_opts.py | 20 ++++ tools/buildman/builder.py | 2 +- tools/buildman/builderthread.py | 6 + tools/buildman/func_test.py | 4 +- 53 files changed, 388 insertions(+), 221 deletions(-) create mode 100644 common/menu_key.c create mode 100644 env/env_set.c create mode 100644 test/py/tests/test_sandbox_opts.py
We cannot be sure that the new config is consistent, particularly when changing a major item like CONFIG_CMDLINE. Use 'make oldconfig' to check that and avoid any such problems.
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/buildman/builder.py | 2 +- tools/buildman/builderthread.py | 6 ++++++ tools/buildman/func_test.py | 4 +++- 3 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py index ecbd368c47a5..24a94e8ae3e5 100644 --- a/tools/buildman/builder.py +++ b/tools/buildman/builder.py @@ -476,7 +476,7 @@ class Builder: Args: commit: Commit object that is being built brd: Board object that is being built - stage: Stage that we are at (mrproper, config, build) + stage: Stage that we are at (mrproper, config, oldconfig, build) cwd: Directory where make should be run args: Arguments to pass to make kwargs: Arguments to pass to command.run_pipe() diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py index 25f460c207db..035d4e9f235c 100644 --- a/tools/buildman/builderthread.py +++ b/tools/buildman/builderthread.py @@ -423,6 +423,12 @@ class BuilderThread(threading.Thread):
# Now do the build, if everything looks OK if result.return_code == 0: + if adjust_cfg: + oldc_args = list(args) + ['oldconfig'] + oldc_result = self.make(commit, brd, 'oldconfig', cwd, + *oldc_args, env=env) + if oldc_result.return_code: + return oldc_result result = self._build(commit, brd, cwd, args, env, cmd_list, config_only) if adjust_cfg: diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py index 55dd494fe8ee..6b88ed815d65 100644 --- a/tools/buildman/func_test.py +++ b/tools/buildman/func_test.py @@ -439,6 +439,8 @@ class TestFunctional(unittest.TestCase): tools.write_file(fname, b'CONFIG_SOMETHING=1') return command.CommandResult(return_code=0, combined='Test configuration complete') + elif stage == 'oldconfig': + return command.CommandResult(return_code=0) elif stage == 'build': stderr = '' fname = os.path.join(cwd or '', out_dir, 'u-boot') @@ -461,7 +463,7 @@ Some images are invalid''' return command.CommandResult(return_code=0)
# Not handled, so abort - print('make', stage) + print('_HandleMake failure: make', stage) sys.exit(1)
# Example function to print output lines
With recent changes in boot/Kconfig it is no-longer possible to disable CMDLINE. It results in various link errors because some options which require CMDLINE are enabled regardless of whether it is available.
Add the necessary conditions to fix this.
Note that it would be better to have all commands depend on CMDLINE, but that is extremely difficult at present, since some functions use CMD_xxx to enable feature xxx. For example networking and environment have a number of problems to tease apart.
Signed-off-by: Simon Glass sjg@chromium.org ---
boot/Kconfig | 19 ++++++++++++------- lib/efi_loader/Kconfig | 1 + 2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/boot/Kconfig b/boot/Kconfig index a01e6cb8aafe..f74ac7e9cc72 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -342,6 +342,7 @@ endif # FIT
config PXE_UTILS bool + depends on CMDLINE select MENU help Utilities for parsing PXE file formats. @@ -357,7 +358,7 @@ config BOOT_DEFAULTS select CMD_PART if PARTITIONS select CMD_DHCP if CMD_NET select CMD_PING if CMD_NET - select CMD_PXE if CMD_NET + select CMD_PXE if CMDLINE && CMD_NET select SUPPORT_RAW_INITRD select ENV_VARS_UBOOT_CONFIG select CMD_BOOTI if ARM64 @@ -461,6 +462,7 @@ config BOOTMETH_GLOBAL
config BOOTMETH_CROS bool "Bootdev support for Chromium OS" + depends on CMDLINE depends on X86 || ARM || SANDBOX default y if !ARM select EFI_PARTITION @@ -475,6 +477,7 @@ config BOOTMETH_CROS
config BOOTMETH_EXTLINUX bool "Bootdev support for extlinux boot" + depends on CMDLINE select PXE_UTILS default y help @@ -490,7 +493,7 @@ config BOOTMETH_EXTLINUX
config BOOTMETH_EXTLINUX_PXE bool "Bootdev support for extlinux boot over network" - depends on CMD_PXE && CMD_NET && DM_ETH + depends on CMDLINE && CMD_PXE && CMD_NET && DM_ETH default y help Enables support for extlinux boot using bootdevs. This makes the @@ -504,7 +507,7 @@ config BOOTMETH_EXTLINUX_PXE
config BOOTMETH_EFILOADER bool "Bootdev support for EFI boot" - depends on EFI_LOADER + depends on EFI_LOADER && CMDLINE default y help Enables support for EFI boot using bootdevs. This makes the @@ -536,10 +539,10 @@ config BOOTMETH_VBE
config BOOTMETH_DISTRO bool # Options needed to boot any distro - select BOOTMETH_SCRIPT # E.g. Armbian uses scripts - select BOOTMETH_EXTLINUX # E.g. Debian uses these - select BOOTMETH_EXTLINUX_PXE if CMD_PXE && CMD_NET && DM_ETH - select BOOTMETH_EFILOADER if EFI_LOADER # E.g. Ubuntu uses this + select BOOTMETH_SCRIPT if CMDLINE # E.g. Armbian uses scripts + select BOOTMETH_EXTLINUX if CMDLINE # E.g. Debian uses these + select BOOTMETH_EXTLINUX_PXE if CMDLINE && CMD_PXE && CMD_NET && DM_ETH + select BOOTMETH_EFILOADER if CMDLINE && EFI_LOADER # E.g. Ubuntu uses this
config SPL_BOOTMETH_VBE bool "Bootdev support for Verified Boot for Embedded (SPL)" @@ -663,6 +666,7 @@ config BOOTMETH_SANDBOX
config BOOTMETH_SCRIPT bool "Bootdev support for U-Boot scripts" + depends on CMDLINE default y if BOOTSTD_FULL select HUSH_PARSER help @@ -777,6 +781,7 @@ endmenu # Boot images
config DISTRO_DEFAULTS bool "(deprecated) Script-based booting of Linux distributions" + depends on CMDLINE select BOOT_DEFAULTS select AUTO_COMPLETE select CMDLINE_EDITING diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index d20aaab6dba4..621ed5e5b0fb 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -34,6 +34,7 @@ if EFI_LOADER
config CMD_BOOTEFI_BOOTMGR bool "UEFI Boot Manager" + depends on CMDLINE default y select BOOTMETH_GLOBAL if BOOTSTD help
Make AUTOBOOT depend on CMDLINE since it is mostly meaningless without it.
Signed-off-by: Simon Glass sjg@chromium.org ---
boot/Kconfig | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/boot/Kconfig b/boot/Kconfig index f74ac7e9cc72..41ec2c34bf74 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -1167,14 +1167,16 @@ menu "Autoboot options"
config AUTOBOOT bool "Autoboot" + depends on CMDLINE default y help This enables the autoboot. See doc/README.autoboot for detail.
+if AUTOBOOT + config BOOTDELAY int "delay in seconds before automatically booting" default 2 - depends on AUTOBOOT help Delay before automatically running bootcmd; set to 0 to autoboot with no delay, but you can stop it by key input. @@ -1196,9 +1198,11 @@ config AUTOBOOT_KEYED U-Boot automatic booting process and bring the device to the U-Boot prompt for user input.
+if AUTOBOOT_KEYED + config AUTOBOOT_FLUSH_STDIN bool "Enable flushing stdin before starting to read the password" - depends on AUTOBOOT_KEYED && !SANDBOX + depends on !SANDBOX help When this option is enabled stdin buffer will be flushed before starting to read the password. @@ -1207,7 +1211,6 @@ config AUTOBOOT_FLUSH_STDIN
config AUTOBOOT_PROMPT string "Autoboot stop prompt" - depends on AUTOBOOT_KEYED default "Autoboot in %d seconds\n" help This string is displayed before the boot delay selected by @@ -1223,7 +1226,6 @@ config AUTOBOOT_PROMPT
config AUTOBOOT_ENCRYPTION bool "Enable encryption in autoboot stopping" - depends on AUTOBOOT_KEYED help This option allows a string to be entered into U-Boot to stop the autoboot. @@ -1250,7 +1252,7 @@ config AUTOBOOT_SHA256_FALLBACK
config AUTOBOOT_DELAY_STR string "Delay autobooting via specific input key / string" - depends on AUTOBOOT_KEYED && !AUTOBOOT_ENCRYPTION + depends on !AUTOBOOT_ENCRYPTION help This option delays the automatic boot feature by issuing a specific input key or string. If CONFIG_AUTOBOOT_DELAY_STR @@ -1262,7 +1264,7 @@ config AUTOBOOT_DELAY_STR
config AUTOBOOT_STOP_STR string "Stop autobooting via specific input key / string" - depends on AUTOBOOT_KEYED && !AUTOBOOT_ENCRYPTION + depends on !AUTOBOOT_ENCRYPTION help This option enables stopping (aborting) of the automatic boot feature only by issuing a specific input key or @@ -1274,7 +1276,7 @@ config AUTOBOOT_STOP_STR
config AUTOBOOT_KEYED_CTRLC bool "Enable Ctrl-C autoboot interruption" - depends on AUTOBOOT_KEYED && !AUTOBOOT_ENCRYPTION + depends on !AUTOBOOT_ENCRYPTION help This option allows for the boot sequence to be interrupted by ctrl-c, in addition to the "bootdelaykey" and "bootstopkey". @@ -1283,7 +1285,7 @@ config AUTOBOOT_KEYED_CTRLC
config AUTOBOOT_NEVER_TIMEOUT bool "Make the password entry never time-out" - depends on AUTOBOOT_KEYED && AUTOBOOT_ENCRYPTION && CRYPT_PW + depends on AUTOBOOT_ENCRYPTION && CRYPT_PW help This option removes the timeout from the password entry when the user first presses the <Enter> key before entering @@ -1291,7 +1293,7 @@ config AUTOBOOT_NEVER_TIMEOUT
config AUTOBOOT_STOP_STR_ENABLE bool "Enable fixed string to stop autobooting" - depends on AUTOBOOT_KEYED && AUTOBOOT_ENCRYPTION + depends on AUTOBOOT_ENCRYPTION help This option enables the feature to add a fixed stop string that is defined at compile time. @@ -1322,6 +1324,9 @@ config AUTOBOOT_STOP_STR_SHA256 includes a ":", the portion prior to the ":" will be treated as a salt value.
+endif # AUTOBOOT_KEYED +endif # AUTOBOOT + config AUTOBOOT_USE_MENUKEY bool "Allow a specify key to run a menu from the environment" depends on !AUTOBOOT_KEYED
On Sun, Sep 24, 2023 at 02:39:21PM -0600, Simon Glass wrote:
Make AUTOBOOT depend on CMDLINE since it is mostly meaningless without it.
Signed-off-by: Simon Glass sjg@chromium.org
boot/Kconfig | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/boot/Kconfig b/boot/Kconfig index f74ac7e9cc72..41ec2c34bf74 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -1167,14 +1167,16 @@ menu "Autoboot options"
config AUTOBOOT bool "Autoboot"
- depends on CMDLINE default y help This enables the autoboot. See doc/README.autoboot for detail.
This is fine and correct.
+if AUTOBOOT
config BOOTDELAY int "delay in seconds before automatically booting" default 2
- depends on AUTOBOOT
[snip]
@@ -1322,6 +1324,9 @@ config AUTOBOOT_STOP_STR_SHA256 includes a ":", the portion prior to the ":" will be treated as a salt value.
+endif # AUTOBOOT_KEYED +endif # AUTOBOOT
So it's ~200 lines, yes, hiding this under an if, or perhaps a menu makes sense, however...
config AUTOBOOT_USE_MENUKEY bool "Allow a specify key to run a menu from the environment" depends on !AUTOBOOT_KEYED
It looks like there's more stuff to move under a menu/if here?
Hi Tom,
On Sun, 24 Sept 2023 at 18:40, Tom Rini trini@konsulko.com wrote:
On Sun, Sep 24, 2023 at 02:39:21PM -0600, Simon Glass wrote:
Make AUTOBOOT depend on CMDLINE since it is mostly meaningless without it.
Signed-off-by: Simon Glass sjg@chromium.org
boot/Kconfig | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/boot/Kconfig b/boot/Kconfig index f74ac7e9cc72..41ec2c34bf74 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -1167,14 +1167,16 @@ menu "Autoboot options"
config AUTOBOOT bool "Autoboot"
depends on CMDLINE default y help This enables the autoboot. See doc/README.autoboot for detail.This is fine and correct.
+if AUTOBOOT
config BOOTDELAY int "delay in seconds before automatically booting" default 2
depends on AUTOBOOT[snip]
@@ -1322,6 +1324,9 @@ config AUTOBOOT_STOP_STR_SHA256 includes a ":", the portion prior to the ":" will be treated as a salt value.
+endif # AUTOBOOT_KEYED +endif # AUTOBOOT
So it's ~200 lines, yes, hiding this under an if, or perhaps a menu makes sense, however...
config AUTOBOOT_USE_MENUKEY bool "Allow a specify key to run a menu from the environment" depends on !AUTOBOOT_KEYED
It looks like there's more stuff to move under a menu/if here?
Well this depends on !AUTOBOOT_KEYED so can't be in the 'if AUTOBOOT_KEYED'. But yes I can create a new 'if !AUTOBOOT_KEYED' for these two items. Normally we want 2-3 options to warrant an 'if', so I don't see this as a strong case.
Regards, Simon
On Tue, Oct 03, 2023 at 08:11:11PM -0600, Simon Glass wrote:
Hi Tom,
On Sun, 24 Sept 2023 at 18:40, Tom Rini trini@konsulko.com wrote:
On Sun, Sep 24, 2023 at 02:39:21PM -0600, Simon Glass wrote:
Make AUTOBOOT depend on CMDLINE since it is mostly meaningless without it.
Signed-off-by: Simon Glass sjg@chromium.org
boot/Kconfig | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/boot/Kconfig b/boot/Kconfig index f74ac7e9cc72..41ec2c34bf74 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -1167,14 +1167,16 @@ menu "Autoboot options"
config AUTOBOOT bool "Autoboot"
depends on CMDLINE default y help This enables the autoboot. See doc/README.autoboot for detail.This is fine and correct.
+if AUTOBOOT
config BOOTDELAY int "delay in seconds before automatically booting" default 2
depends on AUTOBOOT[snip]
@@ -1322,6 +1324,9 @@ config AUTOBOOT_STOP_STR_SHA256 includes a ":", the portion prior to the ":" will be treated as a salt value.
+endif # AUTOBOOT_KEYED +endif # AUTOBOOT
So it's ~200 lines, yes, hiding this under an if, or perhaps a menu makes sense, however...
config AUTOBOOT_USE_MENUKEY bool "Allow a specify key to run a menu from the environment" depends on !AUTOBOOT_KEYED
It looks like there's more stuff to move under a menu/if here?
Well this depends on !AUTOBOOT_KEYED so can't be in the 'if AUTOBOOT_KEYED'. But yes I can create a new 'if !AUTOBOOT_KEYED' for these two items. Normally we want 2-3 options to warrant an 'if', so I don't see this as a strong case.
Well, sometimes it seems like we abuse the "if" mechanic too. It's not short-hand for "avoid saying depends on" a few times, it's "hide these things until we turn on another feature".
But right here it looks like AUTOBOOT_USE_MENUKEY still needs to be under "if AUTOBOOT" yes?
Add this to some more commands to avoid build errors with sandbox.
Signed-off-by: Simon Glass sjg@chromium.org ---
cmd/Kconfig | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 64d723bd483b..cdc22a067b27 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -224,6 +224,7 @@ menu "Boot commands"
config CMD_BOOTD bool "bootd" + depends on CMDLINE default y help Run the command stored in the environment "bootcmd", i.e. @@ -413,6 +414,7 @@ source lib/efi_selftest/Kconfig
config CMD_BOOTMENU bool "bootmenu" + depends on CMDLINE select MENU select CHARSET help @@ -479,6 +481,7 @@ config CMD_GO
config CMD_RUN bool "run" + depends on CMDLINE default y help Run the command in the given environment variable. @@ -574,6 +577,7 @@ menu "Environment commands"
config CMD_ASKENV bool "ask for env variable" + depends on CMDLINE help Ask for environment variable
@@ -2125,6 +2129,7 @@ config CMD_EFICONFIG
config CMD_EXCEPTION bool "exception - raise exception" + depends on CMDLINE depends on ARM || RISCV || SANDBOX || X86 help Enable the 'exception' command which allows to raise an exception. @@ -2231,6 +2236,7 @@ config CMD_SYSBOOT
config CMD_QFW bool "qfw" + depends on CMDLINE select QFW help This provides access to the QEMU firmware interface. The main
Some commands assume that CONFIG_SYS_LONGHELP is always defined. Declaration of long help should be bracketed by an #ifdef to avoid an 'unused variable' warning.
Fix this treewide.
Signed-off-by: Simon Glass sjg@chromium.org ---
arch/arm/mach-imx/cmd_dek.c | 3 ++- arch/arm/mach-imx/cmd_mfgprot.c | 3 ++- arch/arm/mach-imx/imx8/snvs_security_sc.c | 10 ++++++++++ arch/arm/mach-stm32mp/cmd_stm32key.c | 2 ++ board/freescale/common/cmd_esbc_validate.c | 3 ++- board/kontron/sl28/cmds.c | 2 ++ cmd/adc.c | 2 ++ cmd/arm/exception.c | 2 ++ cmd/arm/exception64.c | 2 ++ cmd/axi.c | 2 ++ cmd/blob.c | 2 ++ cmd/cyclic.c | 2 ++ cmd/mux.c | 2 ++ cmd/osd.c | 2 ++ cmd/pcap.c | 2 ++ cmd/riscv/exception.c | 2 ++ cmd/sandbox/exception.c | 2 ++ cmd/scp03.c | 2 ++ cmd/wdt.c | 2 ++ cmd/x86/exception.c | 2 ++ 20 files changed, 48 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c index 6fa5b41fcd38..25ea7d3b37da 100644 --- a/arch/arm/mach-imx/cmd_dek.c +++ b/arch/arm/mach-imx/cmd_dek.c @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc, return blob_encap_dek(src_addr, dst_addr, len); }
-/***************************************************/ +#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char dek_blob_help_text[] = "src dst len - Encapsulate and create blob of data\n" " $len bits long at address $src and\n" " store the result at address $dst.\n"; +#endif
U_BOOT_CMD( dek_blob, 4, 1, do_dek_blob, diff --git a/arch/arm/mach-imx/cmd_mfgprot.c b/arch/arm/mach-imx/cmd_mfgprot.c index 9576b48dde30..bf19f80dde9b 100644 --- a/arch/arm/mach-imx/cmd_mfgprot.c +++ b/arch/arm/mach-imx/cmd_mfgprot.c @@ -133,13 +133,14 @@ free_m: return ret; }
-/***************************************************/ +#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char mfgprot_help_text[] = "Usage:\n" "Print the public key for Manufacturing Protection\n" "\tmfgprot pubk\n" "Generates a Manufacturing Protection signature\n" "\tmfgprot sign <data_addr> <size>"; +#endif
U_BOOT_CMD( mfgprot, 4, 1, do_mfgprot, diff --git a/arch/arm/mach-imx/imx8/snvs_security_sc.c b/arch/arm/mach-imx/imx8/snvs_security_sc.c index 1eaa68f8d5ff..e14727d7ca0b 100644 --- a/arch/arm/mach-imx/imx8/snvs_security_sc.c +++ b/arch/arm/mach-imx/imx8/snvs_security_sc.c @@ -598,6 +598,7 @@ exit: } #endif /* CONFIG_IMX_SNVS_SEC_SC_AUTO */
+#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char snvs_cfg_help_text[] = "snvs_cfg\n" "\thp.lock\n" @@ -620,6 +621,7 @@ static char snvs_cfg_help_text[] = "\tlp.act_tamper_routing_ctl2\n" "\n" "ALL values should be in hexadecimal format"; +#endif
#define NB_REGISTERS 18 static int do_snvs_cfg(struct cmd_tbl *cmdtp, int flag, int argc, @@ -663,6 +665,7 @@ U_BOOT_CMD(snvs_cfg, snvs_cfg_help_text );
+#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char snvs_dgo_cfg_help_text[] = "snvs_dgo_cfg\n" "\ttamper_offset_ctl\n" @@ -673,6 +676,7 @@ static char snvs_dgo_cfg_help_text[] = "\ttamper_core_volt_mon_ctl\n" "\n" "ALL values should be in hexadecimal format"; +#endif
static int do_snvs_dgo_cfg(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) @@ -703,12 +707,14 @@ U_BOOT_CMD(snvs_dgo_cfg, snvs_dgo_cfg_help_text );
+#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char tamper_pin_cfg_help_text[] = "snvs_dgo_cfg\n" "\tpad\n" "\tvalue\n" "\n" "ALL values should be in hexadecimal format"; +#endif
static int do_tamper_pin_cfg(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) @@ -735,6 +741,7 @@ U_BOOT_CMD(tamper_pin_cfg, tamper_pin_cfg_help_text );
+#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char snvs_clear_status_help_text[] = "snvs_clear_status\n" "\tHPSR\n" @@ -744,6 +751,7 @@ static char snvs_clear_status_help_text[] = "\n" "Write the status registers with the value provided," " clearing the status"; +#endif
static int do_snvs_clear_status(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) @@ -779,9 +787,11 @@ U_BOOT_CMD(snvs_clear_status, snvs_clear_status_help_text );
+#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char snvs_sec_status_help_text[] = "snvs_sec_status\n" "Display information about the security related to tamper and secvio"; +#endif
static int do_snvs_sec_status(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) diff --git a/arch/arm/mach-stm32mp/cmd_stm32key.c b/arch/arm/mach-stm32mp/cmd_stm32key.c index 85be8e23bdba..0f27fa128148 100644 --- a/arch/arm/mach-stm32mp/cmd_stm32key.c +++ b/arch/arm/mach-stm32mp/cmd_stm32key.c @@ -419,12 +419,14 @@ static int do_stm32key_close(struct cmd_tbl *cmdtp, int flag, int argc, char *co return CMD_RET_SUCCESS; }
+#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char stm32key_help_text[] = "list : list the supported key with description\n" "stm32key select [<key>] : Select the key identified by <key> or display the key used for read/fuse command\n" "stm32key read [<addr> | -a ] : Read the curent key at <addr> or current / all (-a) key in OTP\n" "stm32key fuse [-y] <addr> : Fuse the current key at addr in OTP\n" "stm32key close [-y] : Close the device\n"; +#endif
U_BOOT_CMD_WITH_SUBCMDS(stm32key, "Manage key on STM32", stm32key_help_text, U_BOOT_SUBCMD_MKENT(list, 1, 0, do_stm32key_list), diff --git a/board/freescale/common/cmd_esbc_validate.c b/board/freescale/common/cmd_esbc_validate.c index 6c096266b484..e678a5768117 100644 --- a/board/freescale/common/cmd_esbc_validate.c +++ b/board/freescale/common/cmd_esbc_validate.c @@ -62,7 +62,7 @@ static int do_esbc_validate(struct cmd_tbl *cmdtp, int flag, int argc, return 0; }
-/***************************************************/ +#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char esbc_validate_help_text[] = "esbc_validate hdr_addr <hash_val> - Validates signature using\n" " RSA verification\n" @@ -71,6 +71,7 @@ static char esbc_validate_help_text[] = " $hash_val -Optional\n" " It provides Hash of public/srk key to be\n" " used to verify signature.\n"; +#endif
U_BOOT_CMD( esbc_validate, 3, 0, do_esbc_validate, diff --git a/board/kontron/sl28/cmds.c b/board/kontron/sl28/cmds.c index 08a22b5d01e0..c83f3245d5f8 100644 --- a/board/kontron/sl28/cmds.c +++ b/board/kontron/sl28/cmds.c @@ -171,8 +171,10 @@ out: return CMD_RET_FAILURE; }
+#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char sl28_help_text[] = "nvm [<hex>] - display/set the 16 non-volatile bits\n"; +#endif
U_BOOT_CMD_WITH_SUBCMDS(sl28, "SMARC-sAL28 specific", sl28_help_text, U_BOOT_SUBCMD_MKENT(nvm, 2, 1, do_sl28_nvm)); diff --git a/cmd/adc.c b/cmd/adc.c index a739d9e46411..ffd112581797 100644 --- a/cmd/adc.c +++ b/cmd/adc.c @@ -152,11 +152,13 @@ static int do_adc_scan(struct cmd_tbl *cmdtp, int flag, int argc, return CMD_RET_SUCCESS; }
+#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char adc_help_text[] = "list - list ADC devices\n" "adc info <name> - Get ADC device info\n" "adc single <name> <channel> [varname] - Get Single data of ADC device channel\n" "adc scan <name> [channel mask] - Scan all [or masked] ADC channels"; +#endif
U_BOOT_CMD_WITH_SUBCMDS(adc, "ADC sub-system", adc_help_text, U_BOOT_SUBCMD_MKENT(list, 1, 1, do_adc_list), diff --git a/cmd/arm/exception.c b/cmd/arm/exception.c index 522f6dff53f2..6f2cdfe17346 100644 --- a/cmd/arm/exception.c +++ b/cmd/arm/exception.c @@ -50,6 +50,7 @@ static struct cmd_tbl cmd_sub[] = { "", ""), };
+#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char exception_help_text[] = "<ex>\n" " The following exceptions are available:\n" @@ -57,5 +58,6 @@ static char exception_help_text[] = " unaligned - data abort\n" " undefined - undefined instruction\n" ; +#endif
#include <exception.h> diff --git a/cmd/arm/exception64.c b/cmd/arm/exception64.c index 589a23115b04..6afe5e2ab5b6 100644 --- a/cmd/arm/exception64.c +++ b/cmd/arm/exception64.c @@ -78,6 +78,7 @@ static struct cmd_tbl cmd_sub[] = { "", ""), };
+#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char exception_help_text[] = "<ex>\n" " The following exceptions are available:\n" @@ -85,5 +86,6 @@ static char exception_help_text[] = " unaligned - unaligned LDAR data abort\n" " undefined - undefined instruction exception\n" ; +#endif
#include <exception.h> diff --git a/cmd/axi.c b/cmd/axi.c index b97b43eb7d01..272fc6131f07 100644 --- a/cmd/axi.c +++ b/cmd/axi.c @@ -344,11 +344,13 @@ static int do_ihs_axi(struct cmd_tbl *cmdtp, int flag, int argc, return CMD_RET_USAGE; }
+#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char axi_help_text[] = "bus - show AXI bus info\n" "axi dev [bus] - show or set current AXI bus to bus number [bus]\n" "axi md size addr [# of objects] - read from AXI device at address [addr] and data width [size] (one of 8, 16, 32)\n" "axi mw size addr value [count] - write data [value] to AXI device at address [addr] and data width [size] (one of 8, 16, 32)\n"; +#endif
U_BOOT_CMD(axi, 7, 1, do_ihs_axi, "AXI sub-system", diff --git a/cmd/blob.c b/cmd/blob.c index 7c77c410d528..70b0df1114ba 100644 --- a/cmd/blob.c +++ b/cmd/blob.c @@ -99,6 +99,7 @@ static int do_blob(struct cmd_tbl *cmdtp, int flag, int argc, }
/***************************************************/ +#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char blob_help_text[] = "enc src dst len km - Encapsulate and create blob of data\n" " $len bytes long at address $src and\n" @@ -116,6 +117,7 @@ static char blob_help_text[] = " The modifier is required for generation\n" " /use as key for cryptographic operation.\n" " Key modifier should be 16 byte long.\n"; +#endif
U_BOOT_CMD( blob, 6, 1, do_blob, diff --git a/cmd/cyclic.c b/cmd/cyclic.c index 946f1d78184d..789eba4eb046 100644 --- a/cmd/cyclic.c +++ b/cmd/cyclic.c @@ -76,9 +76,11 @@ static int do_cyclic_list(struct cmd_tbl *cmdtp, int flag, int argc, return 0; }
+#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char cyclic_help_text[] = "demo <cycletime_ms> <delay_us> - register cyclic demo function\n" "cyclic list - list cyclic functions\n"; +#endif
U_BOOT_CMD_WITH_SUBCMDS(cyclic, "Cyclic", cyclic_help_text, U_BOOT_SUBCMD_MKENT(demo, 3, 1, do_cyclic_demo), diff --git a/cmd/mux.c b/cmd/mux.c index c75907af7726..d20a151948dc 100644 --- a/cmd/mux.c +++ b/cmd/mux.c @@ -173,10 +173,12 @@ static int do_mux_deselect(struct cmd_tbl *cmdtp, int flag, int argc, return CMD_RET_SUCCESS; }
+#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char mux_help_text[] = "list - List all Muxes and their states\n" "select <chip> <id> <state> - Select the given mux state\n" "deselect <chip> <id> - Deselect the given mux and reset it to its idle state"; +#endif
U_BOOT_CMD_WITH_SUBCMDS(mux, "List, select, and deselect muxes", mux_help_text, U_BOOT_SUBCMD_MKENT(list, 1, 1, do_mux_list), diff --git a/cmd/osd.c b/cmd/osd.c index c8c62d4a2ab3..9cf7cc91b283 100644 --- a/cmd/osd.c +++ b/cmd/osd.c @@ -278,12 +278,14 @@ static int do_osd(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) return CMD_RET_USAGE; }
+#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char osd_help_text[] = "show - show OSD info\n" "osd dev [dev] - show or set current OSD\n" "write [pos_x] [pos_y] [buffer] [count] - write 8-bit hex encoded buffer to osd memory at a given position\n" "print [pos_x] [pos_y] [color] [text] - write ASCII buffer (given by text data and driver-specific color information) to osd memory\n" "size [size_x] [size_y] - set OSD XY size in characters\n"; +#endif
U_BOOT_CMD( osd, 6, 1, do_osd, diff --git a/cmd/pcap.c b/cmd/pcap.c index ab5c1a7e8737..ac09a0756102 100644 --- a/cmd/pcap.c +++ b/cmd/pcap.c @@ -48,6 +48,7 @@ static int do_pcap_clear(struct cmd_tbl *cmdtp, int flag, int argc, return pcap_clear() ? CMD_RET_FAILURE : CMD_RET_SUCCESS; }
+#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char pcap_help_text[] = "- network packet capture\n\n" "pcap\n" @@ -61,6 +62,7 @@ static char pcap_help_text[] = "\t<addr>: user address to which pcap will be stored (hexedcimal)\n" "\t<max_size>: Maximum size of pcap file (decimal)\n" "\n"; +#endif
U_BOOT_CMD_WITH_SUBCMDS(pcap, "pcap", pcap_help_text, U_BOOT_SUBCMD_MKENT(init, 3, 0, do_pcap_init), diff --git a/cmd/riscv/exception.c b/cmd/riscv/exception.c index 7a08061d1203..f71d3e3252d7 100644 --- a/cmd/riscv/exception.c +++ b/cmd/riscv/exception.c @@ -43,6 +43,7 @@ static struct cmd_tbl cmd_sub[] = { "", ""), };
+#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char exception_help_text[] = "<ex>\n" " The following exceptions are available:\n" @@ -50,5 +51,6 @@ static char exception_help_text[] = " undefined - illegal instruction\n" " unaligned - load address misaligned\n" ; +#endif
#include <exception.h> diff --git a/cmd/sandbox/exception.c b/cmd/sandbox/exception.c index 1aa1d673aed4..7837f5de285f 100644 --- a/cmd/sandbox/exception.c +++ b/cmd/sandbox/exception.c @@ -31,11 +31,13 @@ static struct cmd_tbl cmd_sub[] = { "", ""), };
+#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char exception_help_text[] = "<ex>\n" " The following exceptions are available:\n" " undefined - undefined instruction\n" " sigsegv - illegal memory access\n" ; +#endif
#include <exception.h> diff --git a/cmd/scp03.c b/cmd/scp03.c index 216c942dd48b..611112059a43 100644 --- a/cmd/scp03.c +++ b/cmd/scp03.c @@ -41,10 +41,12 @@ int do_scp03_provision(struct cmd_tbl *cmdtp, int flag, int argc, return CMD_RET_SUCCESS; }
+#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char text[] = "provides a command to enable SCP03 and provision the SCP03 keys\n" " enable - enable SCP03 on the TEE\n" " provision - provision SCP03 on the TEE\n"; +#endif
U_BOOT_CMD_WITH_SUBCMDS(scp03, "Secure Channel Protocol 03 control", text, U_BOOT_SUBCMD_MKENT(enable, 1, 1, do_scp03_enable), diff --git a/cmd/wdt.c b/cmd/wdt.c index 27410981e7bf..255dfc0b0ffd 100644 --- a/cmd/wdt.c +++ b/cmd/wdt.c @@ -157,6 +157,7 @@ static int do_wdt_expire(struct cmd_tbl *cmdtp, int flag, int argc, return CMD_RET_SUCCESS; }
+#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char wdt_help_text[] = "list - list watchdog devices\n" "wdt dev [<name>] - get/set current watchdog device\n" @@ -164,6 +165,7 @@ static char wdt_help_text[] = "wdt stop - stop watchdog timer\n" "wdt reset - reset watchdog timer\n" "wdt expire [flags] - expire watchdog timer immediately\n"; +#endif
U_BOOT_CMD_WITH_SUBCMDS(wdt, "Watchdog sub-system", wdt_help_text, U_BOOT_SUBCMD_MKENT(list, 1, 1, do_wdt_list), diff --git a/cmd/x86/exception.c b/cmd/x86/exception.c index 82faaa913e5c..8f2f6c135952 100644 --- a/cmd/x86/exception.c +++ b/cmd/x86/exception.c @@ -20,10 +20,12 @@ static struct cmd_tbl cmd_sub[] = { "", ""), };
+#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char exception_help_text[] = "<ex>\n" " The following exceptions are available:\n" " undefined - undefined instruction\n" ; +#endif
#include <exception.h>
On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote:
Some commands assume that CONFIG_SYS_LONGHELP is always defined. Declaration of long help should be bracketed by an #ifdef to avoid an 'unused variable' warning.
Fix this treewide.
Signed-off-by: Simon Glass sjg@chromium.org
[snip]
diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c index 6fa5b41fcd38..25ea7d3b37da 100644 --- a/arch/arm/mach-imx/cmd_dek.c +++ b/arch/arm/mach-imx/cmd_dek.c @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc, return blob_encap_dek(src_addr, dst_addr, len); }
-/***************************************************/ +#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char dek_blob_help_text[] = "src dst len - Encapsulate and create blob of data\n" " $len bits long at address $src and\n" " store the result at address $dst.\n"; +#endif
U_BOOT_CMD( dek_blob, 4, 1, do_dek_blob,
This really doesn't read nicely. I would rather (globally and fix existing users) __maybe_unused this instead. I think there's just one example today that isn't "foo_help_text".
Hi Tom,
On Sun, 24 Sept 2023 at 17:27, Tom Rini trini@konsulko.com wrote:
On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote:
Some commands assume that CONFIG_SYS_LONGHELP is always defined. Declaration of long help should be bracketed by an #ifdef to avoid an 'unused variable' warning.
Fix this treewide.
Signed-off-by: Simon Glass sjg@chromium.org
[snip]
diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c index 6fa5b41fcd38..25ea7d3b37da 100644 --- a/arch/arm/mach-imx/cmd_dek.c +++ b/arch/arm/mach-imx/cmd_dek.c @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc, return blob_encap_dek(src_addr, dst_addr, len); }
-/***************************************************/ +#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char dek_blob_help_text[] = "src dst len - Encapsulate and create blob of data\n" " $len bits long at address $src and\n" " store the result at address $dst.\n"; +#endif
U_BOOT_CMD( dek_blob, 4, 1, do_dek_blob,
This really doesn't read nicely. I would rather (globally and fix existing users) __maybe_unused this instead. I think there's just one example today that isn't "foo_help_text".
Hmm, what do you think about adding a __longhelp symbol to cause the linker to discard it when not needed?
Regards, Simon
On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote:
Hi Tom,
On Sun, 24 Sept 2023 at 17:27, Tom Rini trini@konsulko.com wrote:
On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote:
Some commands assume that CONFIG_SYS_LONGHELP is always defined. Declaration of long help should be bracketed by an #ifdef to avoid an 'unused variable' warning.
Fix this treewide.
Signed-off-by: Simon Glass sjg@chromium.org
[snip]
diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c index 6fa5b41fcd38..25ea7d3b37da 100644 --- a/arch/arm/mach-imx/cmd_dek.c +++ b/arch/arm/mach-imx/cmd_dek.c @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc, return blob_encap_dek(src_addr, dst_addr, len); }
-/***************************************************/ +#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char dek_blob_help_text[] = "src dst len - Encapsulate and create blob of data\n" " $len bits long at address $src and\n" " store the result at address $dst.\n"; +#endif
U_BOOT_CMD( dek_blob, 4, 1, do_dek_blob,
This really doesn't read nicely. I would rather (globally and fix existing users) __maybe_unused this instead. I think there's just one example today that isn't "foo_help_text".
Hmm, what do you think about adding a __longhelp symbol to cause the linker to discard it when not needed?
Well, I don't think we need linker list magic when __maybe_unused will just have them be discarded normally.
Hi Tom,
On Thu, 5 Oct 2023 at 08:53, Tom Rini trini@konsulko.com wrote:
On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote:
Hi Tom,
On Sun, 24 Sept 2023 at 17:27, Tom Rini trini@konsulko.com wrote:
On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote:
Some commands assume that CONFIG_SYS_LONGHELP is always defined. Declaration of long help should be bracketed by an #ifdef to avoid an 'unused variable' warning.
Fix this treewide.
Signed-off-by: Simon Glass sjg@chromium.org
[snip]
diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c index 6fa5b41fcd38..25ea7d3b37da 100644 --- a/arch/arm/mach-imx/cmd_dek.c +++ b/arch/arm/mach-imx/cmd_dek.c @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc, return blob_encap_dek(src_addr, dst_addr, len); }
-/***************************************************/ +#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char dek_blob_help_text[] = "src dst len - Encapsulate and create blob of data\n" " $len bits long at address $src and\n" " store the result at address $dst.\n"; +#endif
U_BOOT_CMD( dek_blob, 4, 1, do_dek_blob,
This really doesn't read nicely. I would rather (globally and fix existing users) __maybe_unused this instead. I think there's just one example today that isn't "foo_help_text".
Hmm, what do you think about adding a __longhelp symbol to cause the linker to discard it when not needed?
Well, I don't think we need linker list magic when __maybe_unused will just have them be discarded normally.
Yes, perhaps things are in a better state than they used to be, but there is a linker discard for commands at present.
SECTIONS { #ifndef CONFIG_CMDLINE /DISCARD/ : { *(__u_boot_list_2_cmd_*) } #endif ...
from:
c1352119fd0 arm: x86: Drop command-line code when CONFIG_CMDLINE is disabled
I wonder if we can remove it? I suppose once this series is sorted out we could have a test to make sure no command is making it into the image when ~CMDLINE
Regards, Simon
On Thu, Oct 05, 2023 at 07:41:49PM -0600, Simon Glass wrote:
Hi Tom,
On Thu, 5 Oct 2023 at 08:53, Tom Rini trini@konsulko.com wrote:
On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote:
Hi Tom,
On Sun, 24 Sept 2023 at 17:27, Tom Rini trini@konsulko.com wrote:
On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote:
Some commands assume that CONFIG_SYS_LONGHELP is always defined. Declaration of long help should be bracketed by an #ifdef to avoid an 'unused variable' warning.
Fix this treewide.
Signed-off-by: Simon Glass sjg@chromium.org
[snip]
diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c index 6fa5b41fcd38..25ea7d3b37da 100644 --- a/arch/arm/mach-imx/cmd_dek.c +++ b/arch/arm/mach-imx/cmd_dek.c @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc, return blob_encap_dek(src_addr, dst_addr, len); }
-/***************************************************/ +#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char dek_blob_help_text[] = "src dst len - Encapsulate and create blob of data\n" " $len bits long at address $src and\n" " store the result at address $dst.\n"; +#endif
U_BOOT_CMD( dek_blob, 4, 1, do_dek_blob,
This really doesn't read nicely. I would rather (globally and fix existing users) __maybe_unused this instead. I think there's just one example today that isn't "foo_help_text".
Hmm, what do you think about adding a __longhelp symbol to cause the linker to discard it when not needed?
Well, I don't think we need linker list magic when __maybe_unused will just have them be discarded normally.
Yes, perhaps things are in a better state than they used to be, but there is a linker discard for commands at present.
Yes, but __maybe_unused means we don't get a warning about it, and it's literally discarded as part of --gc-sections as it done everywhere with maybe 3 exceptions?
Hi Tom,
On Thu, 5 Oct 2023 at 20:16, Tom Rini trini@konsulko.com wrote:
On Thu, Oct 05, 2023 at 07:41:49PM -0600, Simon Glass wrote:
Hi Tom,
On Thu, 5 Oct 2023 at 08:53, Tom Rini trini@konsulko.com wrote:
On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote:
Hi Tom,
On Sun, 24 Sept 2023 at 17:27, Tom Rini trini@konsulko.com wrote:
On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote:
Some commands assume that CONFIG_SYS_LONGHELP is always defined. Declaration of long help should be bracketed by an #ifdef to avoid an 'unused variable' warning.
Fix this treewide.
Signed-off-by: Simon Glass sjg@chromium.org
[snip]
diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c index 6fa5b41fcd38..25ea7d3b37da 100644 --- a/arch/arm/mach-imx/cmd_dek.c +++ b/arch/arm/mach-imx/cmd_dek.c @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc, return blob_encap_dek(src_addr, dst_addr, len); }
-/***************************************************/ +#if IS_ENABLED(CONFIG_SYS_LONGHELP) static char dek_blob_help_text[] = "src dst len - Encapsulate and create blob of data\n" " $len bits long at address $src and\n" " store the result at address $dst.\n"; +#endif
U_BOOT_CMD( dek_blob, 4, 1, do_dek_blob,
This really doesn't read nicely. I would rather (globally and fix existing users) __maybe_unused this instead. I think there's just one example today that isn't "foo_help_text".
Hmm, what do you think about adding a __longhelp symbol to cause the linker to discard it when not needed?
Well, I don't think we need linker list magic when __maybe_unused will just have them be discarded normally.
Yes, perhaps things are in a better state than they used to be, but there is a linker discard for commands at present.
Yes, but __maybe_unused means we don't get a warning about it, and it's literally discarded as part of --gc-sections as it done everywhere with maybe 3 exceptions?
Actually with this series we get a lot closer to that. The problem with the status quo is that disabling CMDLINE doesn't disable most commands, relying instead on discarding them at link time.
With this series, it looks like we can in fact do that, so I should remove the discards as well.
The one proviso is that this does drop a lot of things we want...e.g. BOOTSTD_DEFAULTS cannot be enabled due to its use of 'select', meaning that common filesystems are dropped. So we'll need more effort after this, to allow (for example) bootmeths that don't need commands to work correctly. But I think this series at least provides a better starting point for teasing things apart.
So OK I'll go with __maybe_unused for the ones that need it, which isn't too many given that many of the strings are just included directly in the macro. It is considerably easier than trying to be to clever about things.
Regards, Simon
On Fri, Oct 06, 2023 at 07:03:17AM -0600, Simon Glass wrote:
Hi Tom,
On Thu, 5 Oct 2023 at 20:16, Tom Rini trini@konsulko.com wrote:
On Thu, Oct 05, 2023 at 07:41:49PM -0600, Simon Glass wrote:
Hi Tom,
On Thu, 5 Oct 2023 at 08:53, Tom Rini trini@konsulko.com wrote:
On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote:
Hi Tom,
On Sun, 24 Sept 2023 at 17:27, Tom Rini trini@konsulko.com wrote:
On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote: > Some commands assume that CONFIG_SYS_LONGHELP is always defined. > Declaration of long help should be bracketed by an #ifdef to avoid an > 'unused variable' warning. > > Fix this treewide. > > Signed-off-by: Simon Glass sjg@chromium.org [snip] > diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c > index 6fa5b41fcd38..25ea7d3b37da 100644 > --- a/arch/arm/mach-imx/cmd_dek.c > +++ b/arch/arm/mach-imx/cmd_dek.c > @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc, > return blob_encap_dek(src_addr, dst_addr, len); > } > > -/***************************************************/ > +#if IS_ENABLED(CONFIG_SYS_LONGHELP) > static char dek_blob_help_text[] = > "src dst len - Encapsulate and create blob of data\n" > " $len bits long at address $src and\n" > " store the result at address $dst.\n"; > +#endif > > U_BOOT_CMD( > dek_blob, 4, 1, do_dek_blob,
This really doesn't read nicely. I would rather (globally and fix existing users) __maybe_unused this instead. I think there's just one example today that isn't "foo_help_text".
Hmm, what do you think about adding a __longhelp symbol to cause the linker to discard it when not needed?
Well, I don't think we need linker list magic when __maybe_unused will just have them be discarded normally.
Yes, perhaps things are in a better state than they used to be, but there is a linker discard for commands at present.
Yes, but __maybe_unused means we don't get a warning about it, and it's literally discarded as part of --gc-sections as it done everywhere with maybe 3 exceptions?
Actually with this series we get a lot closer to that. The problem with the status quo is that disabling CMDLINE doesn't disable most commands, relying instead on discarding them at link time.
I don't follow you here. We're talking only about the long help. There's already an option to enable/disable it. When disabled all of the long help text should be discarded, because we reference it via U_BOOT_CMD macro which in turn cares about it, or not. What's missing is a U_BOOT_LONGHELP macro so that instead of: #ifdef CONFIG_SYS_LONGHELP static char cat_help_text[] = "<interface> <dev[:part]> <file>\n" " - Print file from 'dev' on 'interface' to standard output\n"; #endif
We do: U_BOOT_LONGHELP(cat, "<interface> <dev[:part]> <file>\n" " - Print file from 'dev' on 'interface' to standard output\n" );
Which then does: static __maybe_unused char cat_help_text[] = ... ;
And we discard the text automatically due to --gc-sections. We possibly haven't already been doing this since back when we first started using --gc-sections there was a bug in binutils that caused text like the above to still get combined in to other objects and not discarded. That's been fixed for ages.
And the above macro would also let us clean up U_BOOT_CMD macro slightly to just omit the longhelp option and instead always do _CMD_HELP(_name) inside the macros. It'll also make it harder to add new commands without a long help by accident.
With this series, it looks like we can in fact do that, so I should remove the discards as well.
The one proviso is that this does drop a lot of things we want...e.g. BOOTSTD_DEFAULTS cannot be enabled due to its use of 'select', meaning that common filesystems are dropped. So we'll need more effort after this, to allow (for example) bootmeths that don't need commands to work correctly. But I think this series at least provides a better starting point for teasing things apart.
So OK I'll go with __maybe_unused for the ones that need it, which isn't too many given that many of the strings are just included directly in the macro. It is considerably easier than trying to be to clever about things.
Yes, this series itself has a lot of problems that need to be addressed before reposting because I don't like "hiding" that network stuff no longer works, and I also saw that DFU also silently failing.
Hi Tom,
On Fri, 6 Oct 2023 at 10:55, Tom Rini trini@konsulko.com wrote:
On Fri, Oct 06, 2023 at 07:03:17AM -0600, Simon Glass wrote:
Hi Tom,
On Thu, 5 Oct 2023 at 20:16, Tom Rini trini@konsulko.com wrote:
On Thu, Oct 05, 2023 at 07:41:49PM -0600, Simon Glass wrote:
Hi Tom,
On Thu, 5 Oct 2023 at 08:53, Tom Rini trini@konsulko.com wrote:
On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote:
Hi Tom,
On Sun, 24 Sept 2023 at 17:27, Tom Rini trini@konsulko.com wrote: > > On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote: > > Some commands assume that CONFIG_SYS_LONGHELP is always defined. > > Declaration of long help should be bracketed by an #ifdef to avoid an > > 'unused variable' warning. > > > > Fix this treewide. > > > > Signed-off-by: Simon Glass sjg@chromium.org > [snip] > > diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c > > index 6fa5b41fcd38..25ea7d3b37da 100644 > > --- a/arch/arm/mach-imx/cmd_dek.c > > +++ b/arch/arm/mach-imx/cmd_dek.c > > @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc, > > return blob_encap_dek(src_addr, dst_addr, len); > > } > > > > -/***************************************************/ > > +#if IS_ENABLED(CONFIG_SYS_LONGHELP) > > static char dek_blob_help_text[] = > > "src dst len - Encapsulate and create blob of data\n" > > " $len bits long at address $src and\n" > > " store the result at address $dst.\n"; > > +#endif > > > > U_BOOT_CMD( > > dek_blob, 4, 1, do_dek_blob, > > This really doesn't read nicely. I would rather (globally and fix > existing users) __maybe_unused this instead. I think there's just one > example today that isn't "foo_help_text".
Hmm, what do you think about adding a __longhelp symbol to cause the linker to discard it when not needed?
Well, I don't think we need linker list magic when __maybe_unused will just have them be discarded normally.
Yes, perhaps things are in a better state than they used to be, but there is a linker discard for commands at present.
Yes, but __maybe_unused means we don't get a warning about it, and it's literally discarded as part of --gc-sections as it done everywhere with maybe 3 exceptions?
Actually with this series we get a lot closer to that. The problem with the status quo is that disabling CMDLINE doesn't disable most commands, relying instead on discarding them at link time.
I don't follow you here. We're talking only about the long help.
I was actually talking about how the command code is removed. This series allows that to happen via Kconfig rather than needing a linker-script discard rule, something I only just fully appreciated.
There's already an option to enable/disable it. When disabled all of the long help text should be discarded, because we reference it via U_BOOT_CMD macro which in turn cares about it, or not. What's missing is a U_BOOT_LONGHELP macro so that instead of: #ifdef CONFIG_SYS_LONGHELP static char cat_help_text[] = "<interface> <dev[:part]> <file>\n" " - Print file from 'dev' on 'interface' to standard output\n"; #endif
We do: U_BOOT_LONGHELP(cat, "<interface> <dev[:part]> <file>\n" " - Print file from 'dev' on 'interface' to standard output\n" );
Which then does: static __maybe_unused char cat_help_text[] = ... ;
And we discard the text automatically due to --gc-sections. We possibly haven't already been doing this since back when we first started using --gc-sections there was a bug in binutils that caused text like the above to still get combined in to other objects and not discarded. That's been fixed for ages.
And the above macro would also let us clean up U_BOOT_CMD macro slightly to just omit the longhelp option and instead always do _CMD_HELP(_name) inside the macros. It'll also make it harder to add new commands without a long help by accident.
Gosh this is complicated.
At present the U_BOOT_CMD() macro just drops the strings...the problem with the unused var only happens in a small number of cases where an explicit var is used. I don't see why creating a var (when none is there today) helps anything? It doesn't detect missing longhelp, since this is already an error (missing argument). Sure, "" can be provided, but your macro doesn't stop that either.
If you are suggesting a new U_BOOT_LONGHELP() macro, bear in mind that we already have quite a lot...each new 'top-level' macro results in more combinations.
With this series, it looks like we can in fact do that, so I should remove the discards as well.
The one proviso is that this does drop a lot of things we want...e.g. BOOTSTD_DEFAULTS cannot be enabled due to its use of 'select', meaning that common filesystems are dropped. So we'll need more effort after this, to allow (for example) bootmeths that don't need commands to work correctly. But I think this series at least provides a better starting point for teasing things apart.
So OK I'll go with __maybe_unused for the ones that need it, which isn't too many given that many of the strings are just included directly in the macro. It is considerably easier than trying to be to clever about things.
Yes, this series itself has a lot of problems that need to be addressed before reposting because I don't like "hiding" that network stuff no longer works, and I also saw that DFU also silently failing.
Yes I saw your other comments. But I think this patch is the big area of confusion.
Regards, Simon
On Fri, Oct 06, 2023 at 04:42:44PM -0600, Simon Glass wrote:
Hi Tom,
On Fri, 6 Oct 2023 at 10:55, Tom Rini trini@konsulko.com wrote:
On Fri, Oct 06, 2023 at 07:03:17AM -0600, Simon Glass wrote:
Hi Tom,
On Thu, 5 Oct 2023 at 20:16, Tom Rini trini@konsulko.com wrote:
On Thu, Oct 05, 2023 at 07:41:49PM -0600, Simon Glass wrote:
Hi Tom,
On Thu, 5 Oct 2023 at 08:53, Tom Rini trini@konsulko.com wrote:
On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote: > Hi Tom, > > On Sun, 24 Sept 2023 at 17:27, Tom Rini trini@konsulko.com wrote: > > > > On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote: > > > Some commands assume that CONFIG_SYS_LONGHELP is always defined. > > > Declaration of long help should be bracketed by an #ifdef to avoid an > > > 'unused variable' warning. > > > > > > Fix this treewide. > > > > > > Signed-off-by: Simon Glass sjg@chromium.org > > [snip] > > > diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c > > > index 6fa5b41fcd38..25ea7d3b37da 100644 > > > --- a/arch/arm/mach-imx/cmd_dek.c > > > +++ b/arch/arm/mach-imx/cmd_dek.c > > > @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc, > > > return blob_encap_dek(src_addr, dst_addr, len); > > > } > > > > > > -/***************************************************/ > > > +#if IS_ENABLED(CONFIG_SYS_LONGHELP) > > > static char dek_blob_help_text[] = > > > "src dst len - Encapsulate and create blob of data\n" > > > " $len bits long at address $src and\n" > > > " store the result at address $dst.\n"; > > > +#endif > > > > > > U_BOOT_CMD( > > > dek_blob, 4, 1, do_dek_blob, > > > > This really doesn't read nicely. I would rather (globally and fix > > existing users) __maybe_unused this instead. I think there's just one > > example today that isn't "foo_help_text". > > Hmm, what do you think about adding a __longhelp symbol to cause the > linker to discard it when not needed?
Well, I don't think we need linker list magic when __maybe_unused will just have them be discarded normally.
Yes, perhaps things are in a better state than they used to be, but there is a linker discard for commands at present.
Yes, but __maybe_unused means we don't get a warning about it, and it's literally discarded as part of --gc-sections as it done everywhere with maybe 3 exceptions?
Actually with this series we get a lot closer to that. The problem with the status quo is that disabling CMDLINE doesn't disable most commands, relying instead on discarding them at link time.
I don't follow you here. We're talking only about the long help.
I was actually talking about how the command code is removed. This series allows that to happen via Kconfig rather than needing a linker-script discard rule, something I only just fully appreciated.
OK. But this series as-is has a lot of problems and I don't see it as more than a proof of concept.
There's already an option to enable/disable it. When disabled all of the long help text should be discarded, because we reference it via U_BOOT_CMD macro which in turn cares about it, or not. What's missing is a U_BOOT_LONGHELP macro so that instead of: #ifdef CONFIG_SYS_LONGHELP static char cat_help_text[] = "<interface> <dev[:part]> <file>\n" " - Print file from 'dev' on 'interface' to standard output\n"; #endif
We do: U_BOOT_LONGHELP(cat, "<interface> <dev[:part]> <file>\n" " - Print file from 'dev' on 'interface' to standard output\n" );
Which then does: static __maybe_unused char cat_help_text[] = ... ;
And we discard the text automatically due to --gc-sections. We possibly haven't already been doing this since back when we first started using --gc-sections there was a bug in binutils that caused text like the above to still get combined in to other objects and not discarded. That's been fixed for ages.
And the above macro would also let us clean up U_BOOT_CMD macro slightly to just omit the longhelp option and instead always do _CMD_HELP(_name) inside the macros. It'll also make it harder to add new commands without a long help by accident.
Gosh this is complicated.
At present the U_BOOT_CMD() macro just drops the strings...the problem with the unused var only happens in a small number of cases where an explicit var is used. I don't see why creating a var (when none is there today) helps anything? It doesn't detect missing longhelp, since this is already an error (missing argument). Sure, "" can be provided, but your macro doesn't stop that either.
The problem today is that 95% of the cases surround the help text with #ifdef CONFIG_SYS_LONGHELP ... #endif. That's why the rest of the macros are the way they are today. And that in turn is due to (in part at least) old compiler bugs.
If you are suggesting a new U_BOOT_LONGHELP() macro, bear in mind that we already have quite a lot...each new 'top-level' macro results in more combinations.
It really should just be a single macro. I think I'll make an attempt at this, to show what I'm talking about.
With this series, it looks like we can in fact do that, so I should remove the discards as well.
The one proviso is that this does drop a lot of things we want...e.g. BOOTSTD_DEFAULTS cannot be enabled due to its use of 'select', meaning that common filesystems are dropped. So we'll need more effort after this, to allow (for example) bootmeths that don't need commands to work correctly. But I think this series at least provides a better starting point for teasing things apart.
So OK I'll go with __maybe_unused for the ones that need it, which isn't too many given that many of the strings are just included directly in the macro. It is considerably easier than trying to be to clever about things.
Yes, this series itself has a lot of problems that need to be addressed before reposting because I don't like "hiding" that network stuff no longer works, and I also saw that DFU also silently failing.
Yes I saw your other comments. But I think this patch is the big area of confusion.
This isn't the tricky part of the series that I'm saying has problems, that part is all of the functionality that's not being untangled and I think leads to the question of what exactly is the use case where we do remove the command line but don't need some of that functionality still.
Hi Tom,
On Fri, 6 Oct 2023 at 19:01, Tom Rini trini@konsulko.com wrote:
On Fri, Oct 06, 2023 at 04:42:44PM -0600, Simon Glass wrote:
Hi Tom,
On Fri, 6 Oct 2023 at 10:55, Tom Rini trini@konsulko.com wrote:
On Fri, Oct 06, 2023 at 07:03:17AM -0600, Simon Glass wrote:
Hi Tom,
On Thu, 5 Oct 2023 at 20:16, Tom Rini trini@konsulko.com wrote:
On Thu, Oct 05, 2023 at 07:41:49PM -0600, Simon Glass wrote:
Hi Tom,
On Thu, 5 Oct 2023 at 08:53, Tom Rini trini@konsulko.com wrote: > > On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote: > > Hi Tom, > > > > On Sun, 24 Sept 2023 at 17:27, Tom Rini trini@konsulko.com wrote: > > > > > > On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote: > > > > Some commands assume that CONFIG_SYS_LONGHELP is always defined. > > > > Declaration of long help should be bracketed by an #ifdef to avoid an > > > > 'unused variable' warning. > > > > > > > > Fix this treewide. > > > > > > > > Signed-off-by: Simon Glass sjg@chromium.org > > > [snip] > > > > diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c > > > > index 6fa5b41fcd38..25ea7d3b37da 100644 > > > > --- a/arch/arm/mach-imx/cmd_dek.c > > > > +++ b/arch/arm/mach-imx/cmd_dek.c > > > > @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc, > > > > return blob_encap_dek(src_addr, dst_addr, len); > > > > } > > > > > > > > -/***************************************************/ > > > > +#if IS_ENABLED(CONFIG_SYS_LONGHELP) > > > > static char dek_blob_help_text[] = > > > > "src dst len - Encapsulate and create blob of data\n" > > > > " $len bits long at address $src and\n" > > > > " store the result at address $dst.\n"; > > > > +#endif > > > > > > > > U_BOOT_CMD( > > > > dek_blob, 4, 1, do_dek_blob, > > > > > > This really doesn't read nicely. I would rather (globally and fix > > > existing users) __maybe_unused this instead. I think there's just one > > > example today that isn't "foo_help_text". > > > > Hmm, what do you think about adding a __longhelp symbol to cause the > > linker to discard it when not needed? > > Well, I don't think we need linker list magic when __maybe_unused will > just have them be discarded normally.
Yes, perhaps things are in a better state than they used to be, but there is a linker discard for commands at present.
Yes, but __maybe_unused means we don't get a warning about it, and it's literally discarded as part of --gc-sections as it done everywhere with maybe 3 exceptions?
Actually with this series we get a lot closer to that. The problem with the status quo is that disabling CMDLINE doesn't disable most commands, relying instead on discarding them at link time.
I don't follow you here. We're talking only about the long help.
I was actually talking about how the command code is removed. This series allows that to happen via Kconfig rather than needing a linker-script discard rule, something I only just fully appreciated.
OK. But this series as-is has a lot of problems and I don't see it as more than a proof of concept.
Probably I need to send a few version as this discussion is becoming a bit theoretical.
There's already an option to enable/disable it. When disabled all of the long help text should be discarded, because we reference it via U_BOOT_CMD macro which in turn cares about it, or not. What's missing is a U_BOOT_LONGHELP macro so that instead of: #ifdef CONFIG_SYS_LONGHELP static char cat_help_text[] = "<interface> <dev[:part]> <file>\n" " - Print file from 'dev' on 'interface' to standard output\n"; #endif
We do: U_BOOT_LONGHELP(cat, "<interface> <dev[:part]> <file>\n" " - Print file from 'dev' on 'interface' to standard output\n" );
Which then does: static __maybe_unused char cat_help_text[] = ... ;
And we discard the text automatically due to --gc-sections. We possibly haven't already been doing this since back when we first started using --gc-sections there was a bug in binutils that caused text like the above to still get combined in to other objects and not discarded. That's been fixed for ages.
And the above macro would also let us clean up U_BOOT_CMD macro slightly to just omit the longhelp option and instead always do _CMD_HELP(_name) inside the macros. It'll also make it harder to add new commands without a long help by accident.
Gosh this is complicated.
At present the U_BOOT_CMD() macro just drops the strings...the problem with the unused var only happens in a small number of cases where an explicit var is used. I don't see why creating a var (when none is there today) helps anything? It doesn't detect missing longhelp, since this is already an error (missing argument). Sure, "" can be provided, but your macro doesn't stop that either.
The problem today is that 95% of the cases surround the help text with #ifdef CONFIG_SYS_LONGHELP ... #endif. That's why the rest of the macros are the way they are today. And that in turn is due to (in part at least) old compiler bugs.
I see about 46 #idefs for that and nearly 300 commands that don't have one.
If you are suggesting a new U_BOOT_LONGHELP() macro, bear in mind that we already have quite a lot...each new 'top-level' macro results in more combinations.
It really should just be a single macro. I think I'll make an attempt at this, to show what I'm talking about.
OK thanks.
With this series, it looks like we can in fact do that, so I should remove the discards as well.
The one proviso is that this does drop a lot of things we want...e.g. BOOTSTD_DEFAULTS cannot be enabled due to its use of 'select', meaning that common filesystems are dropped. So we'll need more effort after this, to allow (for example) bootmeths that don't need commands to work correctly. But I think this series at least provides a better starting point for teasing things apart.
So OK I'll go with __maybe_unused for the ones that need it, which isn't too many given that many of the strings are just included directly in the macro. It is considerably easier than trying to be to clever about things.
Yes, this series itself has a lot of problems that need to be addressed before reposting because I don't like "hiding" that network stuff no longer works, and I also saw that DFU also silently failing.
Yes I saw your other comments. But I think this patch is the big area of confusion.
This isn't the tricky part of the series that I'm saying has problems, that part is all of the functionality that's not being untangled and I think leads to the question of what exactly is the use case where we do remove the command line but don't need some of that functionality still.
For security and code-size reasons it is useful to disable commands, perhaps all of them. Quite a few features don't need it, but it certainly would take more effort to get a usable version. The goal of this series is to avoid people adding Kconfig mistakes which need to be cleaned up later.
Regards, Simon
On Sat, Oct 07, 2023 at 09:37:07AM -0600, Simon Glass wrote:
Hi Tom,
On Fri, 6 Oct 2023 at 19:01, Tom Rini trini@konsulko.com wrote:
On Fri, Oct 06, 2023 at 04:42:44PM -0600, Simon Glass wrote:
Hi Tom,
On Fri, 6 Oct 2023 at 10:55, Tom Rini trini@konsulko.com wrote:
On Fri, Oct 06, 2023 at 07:03:17AM -0600, Simon Glass wrote:
Hi Tom,
On Thu, 5 Oct 2023 at 20:16, Tom Rini trini@konsulko.com wrote:
On Thu, Oct 05, 2023 at 07:41:49PM -0600, Simon Glass wrote: > Hi Tom, > > On Thu, 5 Oct 2023 at 08:53, Tom Rini trini@konsulko.com wrote: > > > > On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote: > > > Hi Tom, > > > > > > On Sun, 24 Sept 2023 at 17:27, Tom Rini trini@konsulko.com wrote: > > > > > > > > On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote: > > > > > Some commands assume that CONFIG_SYS_LONGHELP is always defined. > > > > > Declaration of long help should be bracketed by an #ifdef to avoid an > > > > > 'unused variable' warning. > > > > > > > > > > Fix this treewide. > > > > > > > > > > Signed-off-by: Simon Glass sjg@chromium.org > > > > [snip] > > > > > diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c > > > > > index 6fa5b41fcd38..25ea7d3b37da 100644 > > > > > --- a/arch/arm/mach-imx/cmd_dek.c > > > > > +++ b/arch/arm/mach-imx/cmd_dek.c > > > > > @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > return blob_encap_dek(src_addr, dst_addr, len); > > > > > } > > > > > > > > > > -/***************************************************/ > > > > > +#if IS_ENABLED(CONFIG_SYS_LONGHELP) > > > > > static char dek_blob_help_text[] = > > > > > "src dst len - Encapsulate and create blob of data\n" > > > > > " $len bits long at address $src and\n" > > > > > " store the result at address $dst.\n"; > > > > > +#endif > > > > > > > > > > U_BOOT_CMD( > > > > > dek_blob, 4, 1, do_dek_blob, > > > > > > > > This really doesn't read nicely. I would rather (globally and fix > > > > existing users) __maybe_unused this instead. I think there's just one > > > > example today that isn't "foo_help_text". > > > > > > Hmm, what do you think about adding a __longhelp symbol to cause the > > > linker to discard it when not needed? > > > > Well, I don't think we need linker list magic when __maybe_unused will > > just have them be discarded normally. > > Yes, perhaps things are in a better state than they used to be, but > there is a linker discard for commands at present.
Yes, but __maybe_unused means we don't get a warning about it, and it's literally discarded as part of --gc-sections as it done everywhere with maybe 3 exceptions?
Actually with this series we get a lot closer to that. The problem with the status quo is that disabling CMDLINE doesn't disable most commands, relying instead on discarding them at link time.
I don't follow you here. We're talking only about the long help.
I was actually talking about how the command code is removed. This series allows that to happen via Kconfig rather than needing a linker-script discard rule, something I only just fully appreciated.
OK. But this series as-is has a lot of problems and I don't see it as more than a proof of concept.
Probably I need to send a few version as this discussion is becoming a bit theoretical.
There's already an option to enable/disable it. When disabled all of the long help text should be discarded, because we reference it via U_BOOT_CMD macro which in turn cares about it, or not. What's missing is a U_BOOT_LONGHELP macro so that instead of: #ifdef CONFIG_SYS_LONGHELP static char cat_help_text[] = "<interface> <dev[:part]> <file>\n" " - Print file from 'dev' on 'interface' to standard output\n"; #endif
We do: U_BOOT_LONGHELP(cat, "<interface> <dev[:part]> <file>\n" " - Print file from 'dev' on 'interface' to standard output\n" );
Which then does: static __maybe_unused char cat_help_text[] = ... ;
And we discard the text automatically due to --gc-sections. We possibly haven't already been doing this since back when we first started using --gc-sections there was a bug in binutils that caused text like the above to still get combined in to other objects and not discarded. That's been fixed for ages.
And the above macro would also let us clean up U_BOOT_CMD macro slightly to just omit the longhelp option and instead always do _CMD_HELP(_name) inside the macros. It'll also make it harder to add new commands without a long help by accident.
Gosh this is complicated.
At present the U_BOOT_CMD() macro just drops the strings...the problem with the unused var only happens in a small number of cases where an explicit var is used. I don't see why creating a var (when none is there today) helps anything? It doesn't detect missing longhelp, since this is already an error (missing argument). Sure, "" can be provided, but your macro doesn't stop that either.
The problem today is that 95% of the cases surround the help text with #ifdef CONFIG_SYS_LONGHELP ... #endif. That's why the rest of the macros are the way they are today. And that in turn is due to (in part at least) old compiler bugs.
I see about 46 #idefs for that and nearly 300 commands that don't have one.
If you are suggesting a new U_BOOT_LONGHELP() macro, bear in mind that we already have quite a lot...each new 'top-level' macro results in more combinations.
It really should just be a single macro. I think I'll make an attempt at this, to show what I'm talking about.
OK thanks.
With this series, it looks like we can in fact do that, so I should remove the discards as well.
The one proviso is that this does drop a lot of things we want...e.g. BOOTSTD_DEFAULTS cannot be enabled due to its use of 'select', meaning that common filesystems are dropped. So we'll need more effort after this, to allow (for example) bootmeths that don't need commands to work correctly. But I think this series at least provides a better starting point for teasing things apart.
So OK I'll go with __maybe_unused for the ones that need it, which isn't too many given that many of the strings are just included directly in the macro. It is considerably easier than trying to be to clever about things.
Yes, this series itself has a lot of problems that need to be addressed before reposting because I don't like "hiding" that network stuff no longer works, and I also saw that DFU also silently failing.
Yes I saw your other comments. But I think this patch is the big area of confusion.
This isn't the tricky part of the series that I'm saying has problems, that part is all of the functionality that's not being untangled and I think leads to the question of what exactly is the use case where we do remove the command line but don't need some of that functionality still.
For security and code-size reasons it is useful to disable commands, perhaps all of them. Quite a few features don't need it, but it certainly would take more effort to get a usable version. The goal of this series is to avoid people adding Kconfig mistakes which need to be cleaned up later.
Maybe the biggest take away should be to do things smaller. DFU intentionally and fundamentally constructs something for the CLI parser to use. Rework that. Same for the network stack. Do some slight re-org / fixing of more intentional dependencies on their own.
Hi Tom,
On Sat, 7 Oct 2023 at 11:25, Tom Rini trini@konsulko.com wrote:
On Sat, Oct 07, 2023 at 09:37:07AM -0600, Simon Glass wrote:
Hi Tom,
On Fri, 6 Oct 2023 at 19:01, Tom Rini trini@konsulko.com wrote:
On Fri, Oct 06, 2023 at 04:42:44PM -0600, Simon Glass wrote:
Hi Tom,
On Fri, 6 Oct 2023 at 10:55, Tom Rini trini@konsulko.com wrote:
On Fri, Oct 06, 2023 at 07:03:17AM -0600, Simon Glass wrote:
Hi Tom,
On Thu, 5 Oct 2023 at 20:16, Tom Rini trini@konsulko.com wrote: > > On Thu, Oct 05, 2023 at 07:41:49PM -0600, Simon Glass wrote: > > Hi Tom, > > > > On Thu, 5 Oct 2023 at 08:53, Tom Rini trini@konsulko.com wrote: > > > > > > On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote: > > > > Hi Tom, > > > > > > > > On Sun, 24 Sept 2023 at 17:27, Tom Rini trini@konsulko.com wrote: > > > > > > > > > > On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote: > > > > > > Some commands assume that CONFIG_SYS_LONGHELP is always defined. > > > > > > Declaration of long help should be bracketed by an #ifdef to avoid an > > > > > > 'unused variable' warning. > > > > > > > > > > > > Fix this treewide. > > > > > > > > > > > > Signed-off-by: Simon Glass sjg@chromium.org > > > > > [snip] > > > > > > diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c > > > > > > index 6fa5b41fcd38..25ea7d3b37da 100644 > > > > > > --- a/arch/arm/mach-imx/cmd_dek.c > > > > > > +++ b/arch/arm/mach-imx/cmd_dek.c > > > > > > @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > return blob_encap_dek(src_addr, dst_addr, len); > > > > > > } > > > > > > > > > > > > -/***************************************************/ > > > > > > +#if IS_ENABLED(CONFIG_SYS_LONGHELP) > > > > > > static char dek_blob_help_text[] = > > > > > > "src dst len - Encapsulate and create blob of data\n" > > > > > > " $len bits long at address $src and\n" > > > > > > " store the result at address $dst.\n"; > > > > > > +#endif > > > > > > > > > > > > U_BOOT_CMD( > > > > > > dek_blob, 4, 1, do_dek_blob, > > > > > > > > > > This really doesn't read nicely. I would rather (globally and fix > > > > > existing users) __maybe_unused this instead. I think there's just one > > > > > example today that isn't "foo_help_text". > > > > > > > > Hmm, what do you think about adding a __longhelp symbol to cause the > > > > linker to discard it when not needed? > > > > > > Well, I don't think we need linker list magic when __maybe_unused will > > > just have them be discarded normally. > > > > Yes, perhaps things are in a better state than they used to be, but > > there is a linker discard for commands at present. > > Yes, but __maybe_unused means we don't get a warning about it, and it's > literally discarded as part of --gc-sections as it done everywhere with > maybe 3 exceptions?
Actually with this series we get a lot closer to that. The problem with the status quo is that disabling CMDLINE doesn't disable most commands, relying instead on discarding them at link time.
I don't follow you here. We're talking only about the long help.
I was actually talking about how the command code is removed. This series allows that to happen via Kconfig rather than needing a linker-script discard rule, something I only just fully appreciated.
OK. But this series as-is has a lot of problems and I don't see it as more than a proof of concept.
Probably I need to send a few version as this discussion is becoming a bit theoretical.
There's already an option to enable/disable it. When disabled all of the long help text should be discarded, because we reference it via U_BOOT_CMD macro which in turn cares about it, or not. What's missing is a U_BOOT_LONGHELP macro so that instead of: #ifdef CONFIG_SYS_LONGHELP static char cat_help_text[] = "<interface> <dev[:part]> <file>\n" " - Print file from 'dev' on 'interface' to standard output\n"; #endif
We do: U_BOOT_LONGHELP(cat, "<interface> <dev[:part]> <file>\n" " - Print file from 'dev' on 'interface' to standard output\n" );
Which then does: static __maybe_unused char cat_help_text[] = ... ;
And we discard the text automatically due to --gc-sections. We possibly haven't already been doing this since back when we first started using --gc-sections there was a bug in binutils that caused text like the above to still get combined in to other objects and not discarded. That's been fixed for ages.
And the above macro would also let us clean up U_BOOT_CMD macro slightly to just omit the longhelp option and instead always do _CMD_HELP(_name) inside the macros. It'll also make it harder to add new commands without a long help by accident.
Gosh this is complicated.
At present the U_BOOT_CMD() macro just drops the strings...the problem with the unused var only happens in a small number of cases where an explicit var is used. I don't see why creating a var (when none is there today) helps anything? It doesn't detect missing longhelp, since this is already an error (missing argument). Sure, "" can be provided, but your macro doesn't stop that either.
The problem today is that 95% of the cases surround the help text with #ifdef CONFIG_SYS_LONGHELP ... #endif. That's why the rest of the macros are the way they are today. And that in turn is due to (in part at least) old compiler bugs.
I see about 46 #idefs for that and nearly 300 commands that don't have one.
If you are suggesting a new U_BOOT_LONGHELP() macro, bear in mind that we already have quite a lot...each new 'top-level' macro results in more combinations.
It really should just be a single macro. I think I'll make an attempt at this, to show what I'm talking about.
OK thanks.
With this series, it looks like we can in fact do that, so I should remove the discards as well.
The one proviso is that this does drop a lot of things we want...e.g. BOOTSTD_DEFAULTS cannot be enabled due to its use of 'select', meaning that common filesystems are dropped. So we'll need more effort after this, to allow (for example) bootmeths that don't need commands to work correctly. But I think this series at least provides a better starting point for teasing things apart.
So OK I'll go with __maybe_unused for the ones that need it, which isn't too many given that many of the strings are just included directly in the macro. It is considerably easier than trying to be to clever about things.
Yes, this series itself has a lot of problems that need to be addressed before reposting because I don't like "hiding" that network stuff no longer works, and I also saw that DFU also silently failing.
Yes I saw your other comments. But I think this patch is the big area of confusion.
This isn't the tricky part of the series that I'm saying has problems, that part is all of the functionality that's not being untangled and I think leads to the question of what exactly is the use case where we do remove the command line but don't need some of that functionality still.
For security and code-size reasons it is useful to disable commands, perhaps all of them. Quite a few features don't need it, but it certainly would take more effort to get a usable version. The goal of this series is to avoid people adding Kconfig mistakes which need to be cleaned up later.
Maybe the biggest take away should be to do things smaller. DFU intentionally and fundamentally constructs something for the CLI parser to use. Rework that. Same for the network stack. Do some slight re-org / fixing of more intentional dependencies on their own.
I think it is better to do it the other way around, to avoid further Kconfig regressions. At the moment we can't actually turn off CMDLINE, so none of the work can actually be used.
The particular thing I am interested in is booting an OS without CONFIG_CMDLINE
Regards, Simon
Many tests make some use of the command line, so require it for all test code.
This could be teased apart, perhaps with a test flag indicating that ut uses the command line. Leave that for later.
Signed-off-by: Simon Glass sjg@chromium.org ---
test/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/test/Kconfig b/test/Kconfig index 830245b6f9a9..2b4036704f91 100644 --- a/test/Kconfig +++ b/test/Kconfig @@ -2,6 +2,7 @@ menu "Testing"
config UNIT_TEST bool "Unit tests" + depends on CMDLINE help Select this to compile in unit tests for various parts of U-Boot. Test suites will be subcommands of the "ut" command.
This code is normally compiled for Tegra, but sandbox can also compile it. We should not use UNIT_TEST as a synonym for SANDBOX, since it is possible to disable UNIT_TEST for sandbox.
Correct the condition.
Signed-off-by: Simon Glass sjg@chromium.org ---
include/k210/pll.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/k210/pll.h b/include/k210/pll.h index fd16a89cb203..6dd60b2eb4fc 100644 --- a/include/k210/pll.h +++ b/include/k210/pll.h @@ -13,7 +13,7 @@ struct k210_pll_config { u8 od; };
-#ifdef CONFIG_UNIT_TEST +#ifdef CONFIG_SANDBOX TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in, struct k210_pll_config *best); #ifndef nop
On Sun, Sep 24, 2023 at 02:39:25PM -0600, Simon Glass wrote:
This code is normally compiled for Tegra, but sandbox can also compile it. We should not use UNIT_TEST as a synonym for SANDBOX, since it is possible to disable UNIT_TEST for sandbox.
Correct the condition.
Signed-off-by: Simon Glass sjg@chromium.org
include/k210/pll.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/k210/pll.h b/include/k210/pll.h index fd16a89cb203..6dd60b2eb4fc 100644 --- a/include/k210/pll.h +++ b/include/k210/pll.h @@ -13,7 +13,7 @@ struct k210_pll_config { u8 od; };
-#ifdef CONFIG_UNIT_TEST +#ifdef CONFIG_SANDBOX TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in, struct k210_pll_config *best); #ifndef nop
Tegra? Do you mean sifive? That's where CLK_K210 stuff is... but it also seems wrong, you can run unit test on real hardware, and this is a test that could (should?) be run on that platform.
Hi Tom.
On Sun, 24 Sept 2023 at 18:43, Tom Rini trini@konsulko.com wrote:
On Sun, Sep 24, 2023 at 02:39:25PM -0600, Simon Glass wrote:
This code is normally compiled for Tegra, but sandbox can also compile it. We should not use UNIT_TEST as a synonym for SANDBOX, since it is possible to disable UNIT_TEST for sandbox.
Correct the condition.
Signed-off-by: Simon Glass sjg@chromium.org
include/k210/pll.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/k210/pll.h b/include/k210/pll.h index fd16a89cb203..6dd60b2eb4fc 100644 --- a/include/k210/pll.h +++ b/include/k210/pll.h @@ -13,7 +13,7 @@ struct k210_pll_config { u8 od; };
-#ifdef CONFIG_UNIT_TEST +#ifdef CONFIG_SANDBOX TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in, struct k210_pll_config *best); #ifndef nop
Tegra? Do you mean sifive? That's where CLK_K210 stuff is... but it
Oh yes, I got confused.
also seems wrong, you can run unit test on real hardware, and this is a test that could (should?) be run on that platform.
Only if it enables UNIT_TEST. You cannot run unit tests without that. The current tests are designed for sandbox.
-- Tom
Regards, Simon
On 10/7/23 19:10, Simon Glass wrote:
Hi Tom.
On Sun, 24 Sept 2023 at 18:43, Tom Rini trini@konsulko.com wrote:
On Sun, Sep 24, 2023 at 02:39:25PM -0600, Simon Glass wrote:
This code is normally compiled for Tegra, but sandbox can also compile it. We should not use UNIT_TEST as a synonym for SANDBOX, since it is possible to disable UNIT_TEST for sandbox.
Correct the condition.
Signed-off-by: Simon Glass sjg@chromium.org
include/k210/pll.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/k210/pll.h b/include/k210/pll.h index fd16a89cb203..6dd60b2eb4fc 100644 --- a/include/k210/pll.h +++ b/include/k210/pll.h @@ -13,7 +13,7 @@ struct k210_pll_config { u8 od; };
-#ifdef CONFIG_UNIT_TEST +#ifdef CONFIG_SANDBOX TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in, struct k210_pll_config *best); #ifndef nop
Tegra? Do you mean sifive? That's where CLK_K210 stuff is... but it
Oh yes, I got confused.
also seems wrong, you can run unit test on real hardware, and this is a test that could (should?) be run on that platform.
Only if it enables UNIT_TEST. You cannot run unit tests without that. The current tests are designed for sandbox.
FWIW I have run this test on actual hardware. My intent here was to allow unit tests to access functions which would otherwise be declared static.
--Sean
Hi Sean,
On Sat, 7 Oct 2023 at 17:21, Sean Anderson seanga2@gmail.com wrote:
On 10/7/23 19:10, Simon Glass wrote:
Hi Tom.
On Sun, 24 Sept 2023 at 18:43, Tom Rini trini@konsulko.com wrote:
On Sun, Sep 24, 2023 at 02:39:25PM -0600, Simon Glass wrote:
This code is normally compiled for Tegra, but sandbox can also compile it. We should not use UNIT_TEST as a synonym for SANDBOX, since it is possible to disable UNIT_TEST for sandbox.
Correct the condition.
Signed-off-by: Simon Glass sjg@chromium.org
include/k210/pll.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/k210/pll.h b/include/k210/pll.h index fd16a89cb203..6dd60b2eb4fc 100644 --- a/include/k210/pll.h +++ b/include/k210/pll.h @@ -13,7 +13,7 @@ struct k210_pll_config { u8 od; };
-#ifdef CONFIG_UNIT_TEST +#ifdef CONFIG_SANDBOX TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in, struct k210_pll_config *best); #ifndef nop
Tegra? Do you mean sifive? That's where CLK_K210 stuff is... but it
Oh yes, I got confused.
also seems wrong, you can run unit test on real hardware, and this is a test that could (should?) be run on that platform.
Only if it enables UNIT_TEST. You cannot run unit tests without that. The current tests are designed for sandbox.
FWIW I have run this test on actual hardware. My intent here was to allow unit tests to access functions which would otherwise be declared static.
Er, with or without UNIT_TEST enabled? How can it even build if this declaration is only for sandbox?
Regards, Simon
On 10/9/23 11:32, Simon Glass wrote:
Hi Sean,
On Sat, 7 Oct 2023 at 17:21, Sean Anderson seanga2@gmail.com wrote:
On 10/7/23 19:10, Simon Glass wrote:
Hi Tom.
On Sun, 24 Sept 2023 at 18:43, Tom Rini trini@konsulko.com wrote:
On Sun, Sep 24, 2023 at 02:39:25PM -0600, Simon Glass wrote:
This code is normally compiled for Tegra, but sandbox can also compile it. We should not use UNIT_TEST as a synonym for SANDBOX, since it is possible to disable UNIT_TEST for sandbox.
Correct the condition.
Signed-off-by: Simon Glass sjg@chromium.org
include/k210/pll.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/k210/pll.h b/include/k210/pll.h index fd16a89cb203..6dd60b2eb4fc 100644 --- a/include/k210/pll.h +++ b/include/k210/pll.h @@ -13,7 +13,7 @@ struct k210_pll_config { u8 od; };
-#ifdef CONFIG_UNIT_TEST +#ifdef CONFIG_SANDBOX TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in, struct k210_pll_config *best); #ifndef nop
Tegra? Do you mean sifive? That's where CLK_K210 stuff is... but it
Oh yes, I got confused.
also seems wrong, you can run unit test on real hardware, and this is a test that could (should?) be run on that platform.
Only if it enables UNIT_TEST. You cannot run unit tests without that. The current tests are designed for sandbox.
FWIW I have run this test on actual hardware. My intent here was to allow unit tests to access functions which would otherwise be declared static.
Er, with or without UNIT_TEST enabled? How can it even build if this declaration is only for sandbox?
With UNIT_TEST of course. Although since this is a forward-declaration, the UNIT_TEST ifdef isn't really even necessary. If it's on actual hardware, nop should already be defined. So maybe this should be something like
#if CONFIG_SANDBOX #define nop() #endif
--Sean
Hi Sean,
On Mon, 9 Oct 2023 at 17:40, Sean Anderson seanga2@gmail.com wrote:
On 10/9/23 11:32, Simon Glass wrote:
Hi Sean,
On Sat, 7 Oct 2023 at 17:21, Sean Anderson seanga2@gmail.com wrote:
On 10/7/23 19:10, Simon Glass wrote:
Hi Tom.
On Sun, 24 Sept 2023 at 18:43, Tom Rini trini@konsulko.com wrote:
On Sun, Sep 24, 2023 at 02:39:25PM -0600, Simon Glass wrote:
This code is normally compiled for Tegra, but sandbox can also compile it. We should not use UNIT_TEST as a synonym for SANDBOX, since it is possible to disable UNIT_TEST for sandbox.
Correct the condition.
Signed-off-by: Simon Glass sjg@chromium.org
include/k210/pll.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/k210/pll.h b/include/k210/pll.h index fd16a89cb203..6dd60b2eb4fc 100644 --- a/include/k210/pll.h +++ b/include/k210/pll.h @@ -13,7 +13,7 @@ struct k210_pll_config { u8 od; };
-#ifdef CONFIG_UNIT_TEST +#ifdef CONFIG_SANDBOX TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in, struct k210_pll_config *best); #ifndef nop
Tegra? Do you mean sifive? That's where CLK_K210 stuff is... but it
Oh yes, I got confused.
also seems wrong, you can run unit test on real hardware, and this is a test that could (should?) be run on that platform.
Only if it enables UNIT_TEST. You cannot run unit tests without that. The current tests are designed for sandbox.
FWIW I have run this test on actual hardware. My intent here was to allow unit tests to access functions which would otherwise be declared static.
Er, with or without UNIT_TEST enabled? How can it even build if this declaration is only for sandbox?
With UNIT_TEST of course. Although since this is a forward-declaration, the UNIT_TEST ifdef isn't really even necessary. If it's on actual hardware, nop should already be defined. So maybe this should be something like
#if CONFIG_SANDBOX #define nop() #endif
It is the CONFIG_SANDBOX that I am trying to remove. Can it be CONFIG_UNIT_TEST instead?
Regards, Simon
On 10/10/23 10:42, Simon Glass wrote:
Hi Sean,
On Mon, 9 Oct 2023 at 17:40, Sean Anderson seanga2@gmail.com wrote:
On 10/9/23 11:32, Simon Glass wrote:
Hi Sean,
On Sat, 7 Oct 2023 at 17:21, Sean Anderson seanga2@gmail.com wrote:
On 10/7/23 19:10, Simon Glass wrote:
Hi Tom.
On Sun, 24 Sept 2023 at 18:43, Tom Rini trini@konsulko.com wrote:
On Sun, Sep 24, 2023 at 02:39:25PM -0600, Simon Glass wrote:
> This code is normally compiled for Tegra, but sandbox can also compile > it. We should not use UNIT_TEST as a synonym for SANDBOX, since it is > possible to disable UNIT_TEST for sandbox. > > Correct the condition. > > Signed-off-by: Simon Glass sjg@chromium.org > --- > > include/k210/pll.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/k210/pll.h b/include/k210/pll.h > index fd16a89cb203..6dd60b2eb4fc 100644 > --- a/include/k210/pll.h > +++ b/include/k210/pll.h > @@ -13,7 +13,7 @@ struct k210_pll_config { > u8 od; > }; > > -#ifdef CONFIG_UNIT_TEST > +#ifdef CONFIG_SANDBOX > TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in, > struct k210_pll_config *best); > #ifndef nop
Tegra? Do you mean sifive? That's where CLK_K210 stuff is... but it
Oh yes, I got confused.
also seems wrong, you can run unit test on real hardware, and this is a test that could (should?) be run on that platform.
Only if it enables UNIT_TEST. You cannot run unit tests without that. The current tests are designed for sandbox.
FWIW I have run this test on actual hardware. My intent here was to allow unit tests to access functions which would otherwise be declared static.
Er, with or without UNIT_TEST enabled? How can it even build if this declaration is only for sandbox?
With UNIT_TEST of course. Although since this is a forward-declaration, the UNIT_TEST ifdef isn't really even necessary. If it's on actual hardware, nop should already be defined. So maybe this should be something like
#if CONFIG_SANDBOX #define nop() #endif
It is the CONFIG_SANDBOX that I am trying to remove. Can it be CONFIG_UNIT_TEST instead?
Well, you can just remove the `ifdef UNIT_TEST` then.
--Sean
When CMDLINE is not enabled, this code fails to build. Correct this by adding conditions.
Note that this should not happen in normal use, since the use of 'select CMDLINE' will cause a visible warning. But it is needed for the sandbox build to pass without CMDLINE.
Signed-off-by: Simon Glass sjg@chromium.org ---
drivers/fastboot/fb_command.c | 3 ++- drivers/fastboot/fb_common.c | 15 +++++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c index 71cfaec6e9dc..4e52e6f0f8bf 100644 --- a/drivers/fastboot/fb_command.c +++ b/drivers/fastboot/fb_command.c @@ -346,7 +346,8 @@ static char g_a_cmd_buff[64];
void fastboot_acmd_complete(void) { - run_command(g_a_cmd_buff, 0); + if (IS_ENABLED(CONFIG_CMDLINE)) + run_command(g_a_cmd_buff, 0); }
/** diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c index 4e9d9b719c6f..35b7aafe5af3 100644 --- a/drivers/fastboot/fb_common.c +++ b/drivers/fastboot/fb_common.c @@ -132,6 +132,13 @@ void fastboot_boot(void) { char *s;
+ /* + * Avoid a build error; this will always have generated a Kconfig + * warning about CMDLINE not being enabled + */ + if (!IS_ENABLED(CONFIG_CMDLINE)) + return; + s = env_get("fastboot_bootcmd"); if (s) { run_command(s, CMD_FLAG_ENV); @@ -170,8 +177,12 @@ void fastboot_handle_boot(int command, bool success)
switch (command) { case FASTBOOT_COMMAND_BOOT: - fastboot_boot(); - net_set_state(NETLOOP_SUCCESS); + if (IS_ENABLED(CONFIG_CMDLINE)) { + fastboot_boot(); + net_set_state(NETLOOP_SUCCESS); + } else { + net_set_state(NETLOOP_FAIL); + } break;
case FASTBOOT_COMMAND_CONTINUE:
On Sun, Sep 24, 2023 at 02:39:26PM -0600, Simon Glass wrote:
When CMDLINE is not enabled, this code fails to build. Correct this by adding conditions.
Note that this should not happen in normal use, since the use of 'select CMDLINE' will cause a visible warning. But it is needed for the sandbox build to pass without CMDLINE.
Signed-off-by: Simon Glass sjg@chromium.org
drivers/fastboot/fb_command.c | 3 ++- drivers/fastboot/fb_common.c | 15 +++++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c index 71cfaec6e9dc..4e52e6f0f8bf 100644 --- a/drivers/fastboot/fb_command.c +++ b/drivers/fastboot/fb_command.c @@ -346,7 +346,8 @@ static char g_a_cmd_buff[64];
void fastboot_acmd_complete(void) {
- run_command(g_a_cmd_buff, 0);
- if (IS_ENABLED(CONFIG_CMDLINE))
run_command(g_a_cmd_buff, 0);}
/** diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c index 4e9d9b719c6f..35b7aafe5af3 100644 --- a/drivers/fastboot/fb_common.c +++ b/drivers/fastboot/fb_common.c @@ -132,6 +132,13 @@ void fastboot_boot(void) { char *s;
- /*
* Avoid a build error; this will always have generated a Kconfig* warning about CMDLINE not being enabled*/- if (!IS_ENABLED(CONFIG_CMDLINE))
return;- s = env_get("fastboot_bootcmd"); if (s) { run_command(s, CMD_FLAG_ENV);
@@ -170,8 +177,12 @@ void fastboot_handle_boot(int command, bool success)
switch (command) { case FASTBOOT_COMMAND_BOOT:
fastboot_boot();net_set_state(NETLOOP_SUCCESS);
if (IS_ENABLED(CONFIG_CMDLINE)) {fastboot_boot();net_set_state(NETLOOP_SUCCESS);} else {net_set_state(NETLOOP_FAIL);}break;
case FASTBOOT_COMMAND_CONTINUE:
All of this just means it now fails to work, yes?
Hi Tom,
On Sun, 24 Sept 2023 at 16:59, Tom Rini trini@konsulko.com wrote:
On Sun, Sep 24, 2023 at 02:39:26PM -0600, Simon Glass wrote:
When CMDLINE is not enabled, this code fails to build. Correct this by adding conditions.
Note that this should not happen in normal use, since the use of 'select CMDLINE' will cause a visible warning. But it is needed for the sandbox build to pass without CMDLINE.
Signed-off-by: Simon Glass sjg@chromium.org
drivers/fastboot/fb_command.c | 3 ++- drivers/fastboot/fb_common.c | 15 +++++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c index 71cfaec6e9dc..4e52e6f0f8bf 100644 --- a/drivers/fastboot/fb_command.c +++ b/drivers/fastboot/fb_command.c @@ -346,7 +346,8 @@ static char g_a_cmd_buff[64];
void fastboot_acmd_complete(void) {
run_command(g_a_cmd_buff, 0);
if (IS_ENABLED(CONFIG_CMDLINE))run_command(g_a_cmd_buff, 0);}
/** diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c index 4e9d9b719c6f..35b7aafe5af3 100644 --- a/drivers/fastboot/fb_common.c +++ b/drivers/fastboot/fb_common.c @@ -132,6 +132,13 @@ void fastboot_boot(void) { char *s;
/** Avoid a build error; this will always have generated a Kconfig* warning about CMDLINE not being enabled*/if (!IS_ENABLED(CONFIG_CMDLINE))return;s = env_get("fastboot_bootcmd"); if (s) { run_command(s, CMD_FLAG_ENV);@@ -170,8 +177,12 @@ void fastboot_handle_boot(int command, bool success)
switch (command) { case FASTBOOT_COMMAND_BOOT:
fastboot_boot();net_set_state(NETLOOP_SUCCESS);
if (IS_ENABLED(CONFIG_CMDLINE)) {fastboot_boot();net_set_state(NETLOOP_SUCCESS);} else {net_set_state(NETLOOP_FAIL);} break; case FASTBOOT_COMMAND_CONTINUE:All of this just means it now fails to work, yes?
It actually fails to build, since there is a Kconfig conflict, as mentioned in the commit message. The use of 'select FASTBOOT' when CMDLINE is not enabled produces an error.
I will see if I can do this another way.
Regards, Simon
This module is used for user input with menus, not just with the command line. Compile it always, so it is available even when CMDLINE is disabled.
Signed-off-by: Simon Glass sjg@chromium.org ---
common/Makefile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/common/Makefile b/common/Makefile index 5c1617206f07..15ae46f596d0 100644 --- a/common/Makefile +++ b/common/Makefile @@ -8,6 +8,7 @@ ifndef CONFIG_SPL_BUILD obj-y += init/ obj-y += main.o obj-y += exports.o +obj-y += cli_getch.o obj-$(CONFIG_HUSH_PARSER) += cli_hush.o obj-$(CONFIG_AUTOBOOT) += autoboot.o
@@ -37,7 +38,7 @@ obj-$(CONFIG_SPLASH_SOURCE) += splash_source.o obj-$(CONFIG_MENU) += menu.o obj-$(CONFIG_UPDATE_COMMON) += update.o obj-$(CONFIG_USB_KEYBOARD) += usb_kbd.o -obj-$(CONFIG_CMDLINE) += cli_getch.o cli_readline.o cli_simple.o +obj-$(CONFIG_CMDLINE) += cli_readline.o cli_simple.o
endif # !CONFIG_SPL_BUILD
This is not available if CMDLINE is disabled, so add an #ifdef to correct this.
Signed-off-by: Simon Glass sjg@chromium.org ---
common/cli.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/common/cli.c b/common/cli.c index 3916a7b10a7d..4d0fea4387f2 100644 --- a/common/cli.c +++ b/common/cli.c @@ -129,6 +129,7 @@ int run_command_list(const char *cmd, int len, int flag) return rcode; }
+#ifdef CONFIG_CMDLINE int run_commandf(const char *fmt, ...) { va_list args; @@ -153,6 +154,7 @@ int run_commandf(const char *fmt, ...) } return run_command(console_buffer, 0); } +#endif /* CMDLINE */
/****************************************************************************/
This conversion function is used by expo which does not require CMDLINE. The menu feature does require CMDLINE.
Move the function into a separate file so that it can be used even when CMDLINE is not enabled.
Signed-off-by: Simon Glass sjg@chromium.org ---
common/Makefile | 2 +- common/cli_getch.c | 1 + common/menu.c | 40 ------------------------------------- common/menu_key.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 51 insertions(+), 41 deletions(-) create mode 100644 common/menu_key.c
diff --git a/common/Makefile b/common/Makefile index 15ae46f596d0..29a4c818a700 100644 --- a/common/Makefile +++ b/common/Makefile @@ -8,7 +8,7 @@ ifndef CONFIG_SPL_BUILD obj-y += init/ obj-y += main.o obj-y += exports.o -obj-y += cli_getch.o +obj-y += cli_getch.o menu_key.o obj-$(CONFIG_HUSH_PARSER) += cli_hush.o obj-$(CONFIG_AUTOBOOT) += autoboot.o
diff --git a/common/cli_getch.c b/common/cli_getch.c index 61d4cb261b81..c3332dc27fae 100644 --- a/common/cli_getch.c +++ b/common/cli_getch.c @@ -8,6 +8,7 @@
#include <common.h> #include <cli.h> +#include <menu.h>
/** * enum cli_esc_state_t - indicates what to do with an escape character diff --git a/common/menu.c b/common/menu.c index b55cf7b99967..844d0ec52af3 100644 --- a/common/menu.c +++ b/common/menu.c @@ -483,46 +483,6 @@ enum bootmenu_key bootmenu_autoboot_loop(struct bootmenu_data *menu, return key; }
-enum bootmenu_key bootmenu_conv_key(int ichar) -{ - enum bootmenu_key key; - - switch (ichar) { - case '\n': - /* enter key was pressed */ - key = BKEY_SELECT; - break; - case CTL_CH('c'): - case '\e': - /* ^C was pressed */ - key = BKEY_QUIT; - break; - case CTL_CH('p'): - key = BKEY_UP; - break; - case CTL_CH('n'): - key = BKEY_DOWN; - break; - case CTL_CH('s'): - key = BKEY_SAVE; - break; - case '+': - key = BKEY_PLUS; - break; - case '-': - key = BKEY_MINUS; - break; - case ' ': - key = BKEY_SPACE; - break; - default: - key = BKEY_NONE; - break; - } - - return key; -} - enum bootmenu_key bootmenu_loop(struct bootmenu_data *menu, struct cli_ch_state *cch) { diff --git a/common/menu_key.c b/common/menu_key.c new file mode 100644 index 000000000000..4e9c3b426b0c --- /dev/null +++ b/common/menu_key.c @@ -0,0 +1,49 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright 2010-2011 Calxeda, Inc. + * Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved. + */ + +#include <common.h> +#include <cli.h> +#include <menu.h> + +enum bootmenu_key bootmenu_conv_key(int ichar) +{ + enum bootmenu_key key; + + switch (ichar) { + case '\n': + /* enter key was pressed */ + key = BKEY_SELECT; + break; + case CTL_CH('c'): + case '\e': + /* ^C was pressed */ + key = BKEY_QUIT; + break; + case CTL_CH('p'): + key = BKEY_UP; + break; + case CTL_CH('n'): + key = BKEY_DOWN; + break; + case CTL_CH('s'): + key = BKEY_SAVE; + break; + case '+': + key = BKEY_PLUS; + break; + case '-': + key = BKEY_MINUS; + break; + case ' ': + key = BKEY_SPACE; + break; + default: + key = BKEY_NONE; + break; + } + + return key; +}
The help text should not be build unless CONFIG_SYS_LONGHELP is enabled. Add this as a condition.
Signed-off-by: Simon Glass sjg@chromium.org ---
cmd/armffa.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/cmd/armffa.c b/cmd/armffa.c index 7e6eafc03ad7..28b65678a8e9 100644 --- a/cmd/armffa.c +++ b/cmd/armffa.c @@ -188,6 +188,7 @@ static int do_ffa_devlist(struct cmd_tbl *cmdtp, int flag, int argc, char *const return CMD_RET_SUCCESS; }
+#ifdef CONFIG_SYS_LONGHELP static char armffa_help_text[] = "getpart <partition UUID>\n" " - lists the partition(s) info\n" @@ -195,6 +196,7 @@ static char armffa_help_text[] = " - sends a data pattern to the specified partition\n" "devlist\n" " - displays information about the FF-A device/driver\n"; +#endif
U_BOOT_CMD_WITH_SUBCMDS(armffa, "Arm FF-A test command", armffa_help_text, U_BOOT_SUBCMD_MKENT(getpart, 2, 1, do_ffa_getpart),
We cannot use PXE or sysboot commands without CONFIG_CMDLINE so add the required condition.
Signed-off-by: Simon Glass sjg@chromium.org ---
cmd/Kconfig | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/cmd/Kconfig b/cmd/Kconfig index cdc22a067b27..a254c41110ec 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1995,6 +1995,7 @@ config CMD_ETHSW
config CMD_PXE bool "pxe" + depends on CMDLINE select PXE_UTILS help Boot image via network using PXE protocol @@ -2230,6 +2231,7 @@ config CMD_SOUND
config CMD_SYSBOOT bool "sysboot" + depends on CMDLINE select PXE_UTILS help Boot image via local extlinux.conf file
It is not possible to set environment variables without having CONFIG_CMD_NVEDIT enabled. When CONFIG_CMDLINE is disabled, we need a way to set variables.
Split the setting code out into its own file, so that env_set() is available even when CONFIG_CMDLINE is not. If it is never called, the code will be dropped at link time.
Update the Makefile rule to only include the env commands when CONFIG_CMD_NVEDIT is enabled.
Signed-off-by: Simon Glass sjg@chromium.org ---
cmd/Makefile | 2 +- cmd/nvedit.c | 122 ++----------------------------------- env/Makefile | 1 + env/env_set.c | 132 +++++++++++++++++++++++++++++++++++++++++ include/env_internal.h | 23 +++++++ 5 files changed, 161 insertions(+), 119 deletions(-) create mode 100644 env/env_set.c
diff --git a/cmd/Makefile b/cmd/Makefile index 9bebf321c397..5be2b4bd3800 100644 --- a/cmd/Makefile +++ b/cmd/Makefile @@ -240,7 +240,7 @@ endif # !CONFIG_SPL_BUILD obj-$(CONFIG_$(SPL_)CMD_TLV_EEPROM) += tlv_eeprom.o
# core command -obj-y += nvedit.o +obj-$(CONFIG_$(SPL_)CMDLINE) += nvedit.o
obj-$(CONFIG_CMD_BCM_EXT_UTILS) += broadcom/
diff --git a/cmd/nvedit.c b/cmd/nvedit.c index fe99157fd17b..0c9efa052baf 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -48,20 +48,6 @@ DECLARE_GLOBAL_DATA_PTR; */ #define MAX_ENV_SIZE (1 << 20) /* 1 MiB */
-/* - * This variable is incremented on each do_env_set(), so it can - * be used via env_get_id() as an indication, if the environment - * has changed or not. So it is possible to reread an environment - * variable only if the environment was changed ... done so for - * example in NetInitLoop() - */ -static int env_id = 1; - -int env_get_id(void) -{ - return env_id; -} - #ifndef CONFIG_SPL_BUILD /* * Command interface: print one or all environment variables @@ -194,108 +180,8 @@ DONE:
return 0; } -#endif -#endif /* CONFIG_SPL_BUILD */ - -/* - * Set a new environment variable, - * or replace or delete an existing one. - */ -static int _do_env_set(int flag, int argc, char *const argv[], int env_flag) -{ - int i, len; - char *name, *value, *s; - struct env_entry e, *ep; - - debug("Initial value for argc=%d\n", argc); - -#if !IS_ENABLED(CONFIG_SPL_BUILD) && IS_ENABLED(CONFIG_CMD_NVEDIT_EFI) - if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') - return do_env_set_efi(NULL, flag, --argc, ++argv); -#endif - - while (argc > 1 && **(argv + 1) == '-') { - char *arg = *++argv; - - --argc; - while (*++arg) { - switch (*arg) { - case 'f': /* force */ - env_flag |= H_FORCE; - break; - default: - return CMD_RET_USAGE; - } - } - } - debug("Final value for argc=%d\n", argc); - name = argv[1]; - - if (strchr(name, '=')) { - printf("## Error: illegal character '='" - "in variable name "%s"\n", name); - return 1; - } - - env_id++; - - /* Delete only ? */ - if (argc < 3 || argv[2] == NULL) { - int rc = hdelete_r(name, &env_htab, env_flag); - - /* If the variable didn't exist, don't report an error */ - return rc && rc != -ENOENT ? 1 : 0; - } - - /* - * Insert / replace new value - */ - for (i = 2, len = 0; i < argc; ++i) - len += strlen(argv[i]) + 1; - - value = malloc(len); - if (value == NULL) { - printf("## Can't malloc %d bytes\n", len); - return 1; - } - for (i = 2, s = value; i < argc; ++i) { - char *v = argv[i]; - - while ((*s++ = *v++) != '\0') - ; - *(s - 1) = ' '; - } - if (s != value) - *--s = '\0'; - - e.key = name; - e.data = value; - hsearch_r(e, ENV_ENTER, &ep, &env_htab, env_flag); - free(value); - if (!ep) { - printf("## Error inserting "%s" variable, errno=%d\n", - name, errno); - return 1; - } - - return 0; -} - -int env_set(const char *varname, const char *varvalue) -{ - const char * const argv[4] = { "setenv", varname, varvalue, NULL }; - - /* before import into hashtable */ - if (!(gd->flags & GD_FLG_ENV_READY)) - return 1; - - if (varvalue == NULL || varvalue[0] == '\0') - return _do_env_set(0, 2, (char * const *)argv, H_PROGRAMMATIC); - else - return _do_env_set(0, 3, (char * const *)argv, H_PROGRAMMATIC); -} +#endif /* CONFIG_CMD_GREPENV */
-#ifndef CONFIG_SPL_BUILD static int do_env_set(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { @@ -455,7 +341,7 @@ int do_env_callback(struct cmd_tbl *cmdtp, int flag, int argc, hwalk_r(&env_htab, print_active_callback); return 0; } -#endif +#endif /* CONFIG_CMD_ENV_CALLBACK */
#if defined(CONFIG_CMD_ENV_FLAGS) static int print_static_flags(const char *var_name, const char *flags, @@ -528,7 +414,7 @@ int do_env_flags(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) hwalk_r(&env_htab, print_active_flags); return 0; } -#endif +#endif /* CONFIG_CMD_ENV_FLAGS */
/* * Interactively edit an environment variable @@ -678,7 +564,7 @@ static int do_env_delete(struct cmd_tbl *cmdtp, int flag, } debug("Final value for argc=%d\n", argc);
- env_id++; + env_inc_id();
while (--argc > 0) { char *name = *++argv; diff --git a/env/Makefile b/env/Makefile index 673b979fdfa9..5250b6df2cfc 100644 --- a/env/Makefile +++ b/env/Makefile @@ -5,6 +5,7 @@
obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += common.o obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += env.o +obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += env_set.o obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += attr.o obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += flags.o
diff --git a/env/env_set.c b/env/env_set.c new file mode 100644 index 000000000000..eccbda6a791c --- /dev/null +++ b/env/env_set.c @@ -0,0 +1,132 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * (C) Copyright 2000-2013 + * Wolfgang Denk, DENX Software Engineering, wd@denx.de. + * + * (C) Copyright 2001 Sysgo Real-Time Solutions, GmbH <www.elinos.com> + * Andreas Heppel aheppel@sysgo.de + * + * Copyright 2011 Freescale Semiconductor, Inc. + */ + +#include <common.h> +#include <command.h> +#include <env.h> +#include <env_internal.h> +#include <malloc.h> +#include <asm/global_data.h> + +DECLARE_GLOBAL_DATA_PTR; + +/* + * This variable is incremented on each do_env_set(), so it can + * be used via env_get_id() as an indication, if the environment + * has changed or not. So it is possible to reread an environment + * variable only if the environment was changed ... done so for + * example in NetInitLoop() + */ +static int env_id = 1; + +int env_get_id(void) +{ + return env_id; +} + +void env_inc_id(void) +{ + env_id++; +} + +int _do_env_set(int flag, int argc, char *const argv[], int env_flag) +{ + int i, len; + char *name, *value, *s; + struct env_entry e, *ep; + + debug("Initial value for argc=%d\n", argc); + +#if !IS_ENABLED(CONFIG_SPL_BUILD) && IS_ENABLED(CONFIG_CMD_NVEDIT_EFI) + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') + return do_env_set_efi(NULL, flag, --argc, ++argv); +#endif + + while (argc > 1 && **(argv + 1) == '-') { + char *arg = *++argv; + + --argc; + while (*++arg) { + switch (*arg) { + case 'f': /* force */ + env_flag |= H_FORCE; + break; + default: + return CMD_RET_USAGE; + } + } + } + debug("Final value for argc=%d\n", argc); + name = argv[1]; + + if (strchr(name, '=')) { + printf("## Error: illegal character '=' in variable name "%s"\n", + name); + return 1; + } + + env_id++; + + /* Delete only ? */ + if (argc < 3 || !argv[2]) { + int rc = hdelete_r(name, &env_htab, env_flag); + + /* If the variable didn't exist, don't report an error */ + return rc && rc != -ENOENT ? 1 : 0; + } + + /* + * Insert / replace new value + */ + for (i = 2, len = 0; i < argc; ++i) + len += strlen(argv[i]) + 1; + + value = malloc(len); + if (!value) { + printf("## Can't malloc %d bytes\n", len); + return 1; + } + for (i = 2, s = value; i < argc; ++i) { + char *v = argv[i]; + + while ((*s++ = *v++) != '\0') + ; + *(s - 1) = ' '; + } + if (s != value) + *--s = '\0'; + + e.key = name; + e.data = value; + hsearch_r(e, ENV_ENTER, &ep, &env_htab, env_flag); + free(value); + if (!ep) { + printf("## Error inserting "%s" variable, errno=%d\n", + name, errno); + return 1; + } + + return 0; +} + +int env_set(const char *varname, const char *varvalue) +{ + const char * const argv[4] = { "setenv", varname, varvalue, NULL }; + + /* before import into hashtable */ + if (!(gd->flags & GD_FLG_ENV_READY)) + return 1; + + if (!varvalue || !varvalue[0]) + return _do_env_set(0, 2, (char * const *)argv, H_PROGRAMMATIC); + else + return _do_env_set(0, 3, (char * const *)argv, H_PROGRAMMATIC); +} diff --git a/include/env_internal.h b/include/env_internal.h index 6a6949464689..99e358b6d8a5 100644 --- a/include/env_internal.h +++ b/include/env_internal.h @@ -260,6 +260,29 @@ const char *env_fat_get_intf(void); * Return: string of device and partition */ char *env_fat_get_dev_part(void); + +/* + * _do_env_set() - Add / replace / delete an environment variable + * + * This implements the bulk of the 'env set' command: + * + * env set [-f] name [value] + * + * Sets the value of variable <name> + * If <value> is NULL, it removes the variable + * Use the -f flag to overwrite read-only/write-once variables + * + * @flag: CMD_FLAG_... value + * @argc: Number of arguments + * @args: List of arguments + * @env_flag: H_... flags from search.h + * Return: 0 if OK, 1 on failure, or CMD_RET_USAGE for invalid flag + */ +int _do_env_set(int flag, int argc, char *const argv[], int env_flag); + +/** env_inc_id() - Increment the environment ID */ +void env_inc_id(void); + #endif /* DO_DEPS_ONLY */
#endif /* _ENV_INTERNAL_H_ */
This relates to printing output and does not need a command line. Move it next to the other console-related options.
Signed-off-by: Simon Glass sjg@chromium.org ---
cmd/Kconfig | 5 ----- common/Kconfig | 5 +++++ 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/cmd/Kconfig b/cmd/Kconfig index a254c41110ec..5f6834b335dc 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -84,11 +84,6 @@ config SYS_CBSIZE default 256 if M68K || PPC default 1024
-config SYS_PBSIZE - int "Buffer size for console output" - default 1024 if ARCH_SUNXI - default 1044 - config SYS_XTRACE bool "Command execution tracer" depends on CMDLINE diff --git a/common/Kconfig b/common/Kconfig index 21eaa5e815f9..08d490ad4776 100644 --- a/common/Kconfig +++ b/common/Kconfig @@ -52,6 +52,11 @@ config CONSOLE_RECORD_IN_SIZE The buffer is allocated immediately after the malloc() region is ready.
+config SYS_PBSIZE + int "Buffer size for console output" + default 1024 if ARCH_SUNXI + default 1044 + config DISABLE_CONSOLE bool "Add functionality to disable console completely" help
There are two cleanup functions needed during boot which depend on CMD_BOOTM: bootm_disable_interrupts() and board_quiesce_devices()
Provide static inline versions of these for when commands are not enabled.
Signed-off-by: Simon Glass sjg@chromium.org ---
arch/arm/lib/bootm.c | 2 ++ boot/bootm.c | 10 ++++------ include/bootm.h | 15 +++++++++++++-- 3 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c index c56285738a26..db8df57cb56e 100644 --- a/arch/arm/lib/bootm.c +++ b/arch/arm/lib/bootm.c @@ -42,9 +42,11 @@ DECLARE_GLOBAL_DATA_PTR;
static struct tag *params;
+#ifdef CONFIG_CMD_BOOTM __weak void board_quiesce_devices(void) { } +#endif
/** * announce_and_cleanup() - Print message and prepare for kernel boot diff --git a/boot/bootm.c b/boot/bootm.c index b1c3afe0a3ad..ee8c79a650c2 100644 --- a/boot/bootm.c +++ b/boot/bootm.c @@ -47,9 +47,11 @@ static const void *boot_get_kernel(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[], struct bootm_headers *images, ulong *os_data, ulong *os_len);
+#ifdef CONFIG_CMD_BOOTM __weak void board_quiesce_devices(void) { } +#endif
#ifdef CONFIG_LMB static void boot_start_lmb(struct bootm_headers *images) @@ -470,12 +472,7 @@ static int bootm_load_os(struct bootm_headers *images, int boot_progress) return 0; }
-/** - * bootm_disable_interrupts() - Disable interrupts in preparation for load/boot - * - * Return: interrupt flag (0 if interrupts were disabled, non-zero if they were - * enabled) - */ +#ifdef CONFIG_CMD_BOOTM ulong bootm_disable_interrupts(void) { ulong iflag; @@ -505,6 +502,7 @@ ulong bootm_disable_interrupts(void) #endif return iflag; } +#endif
#define CONSOLE_ARG "console=" #define NULL_CONSOLE (CONSOLE_ARG "ttynull") diff --git a/include/bootm.h b/include/bootm.h index c3c7336207b1..17c740449efd 100644 --- a/include/bootm.h +++ b/include/bootm.h @@ -50,8 +50,6 @@ int bootm_host_load_images(const void *fit, int cfg_noffset); int boot_selected_os(int argc, char *const argv[], int state, struct bootm_headers *images, boot_os_fn *boot_fn);
-ulong bootm_disable_interrupts(void); - /* This is a special function used by booti/bootz */ int bootm_find_images(int flag, int argc, char *const argv[], ulong start, ulong size); @@ -62,6 +60,15 @@ int do_bootm_states(struct cmd_tbl *cmdtp, int flag, int argc,
void arch_preboot_os(void);
+#ifdef CONFIG_CMD_BOOTM +/** + * bootm_disable_interrupts() - Disable interrupts, stop Ethernet and USB + * + * Return: interrupt flag (0 if interrupts were disabled, non-zero if they were + * enabled) + */ +ulong bootm_disable_interrupts(void); + /* * boards should define this to disable devices when EFI exits from boot * services. @@ -69,6 +76,10 @@ void arch_preboot_os(void); * TODO(sjg@chromium.org>): Update this to use driver model's device_remove(). */ void board_quiesce_devices(void); +#else +static inline ulong bootm_disable_interrupts(void) { return 0; } +static inline void board_quiesce_devices(void) {} +#endif
/** * switch_to_non_secure_mode() - switch to non-secure mode
This can be accessed even when commands are not enabled. Move it into the fdt_support.c file, which is where most of the FDT helpers are.
Signed-off-by: Simon Glass sjg@chromium.org ---
boot/fdt_support.c | 5 +++++ cmd/fdt.c | 5 ----- 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/boot/fdt_support.c b/boot/fdt_support.c index 5e49078f8c35..6ae7b8e20f65 100644 --- a/boot/fdt_support.c +++ b/boot/fdt_support.c @@ -23,6 +23,11 @@ #include <fdtdec.h> #include <version.h>
+/* + * The working_fdt points to our working flattened device tree. + */ +struct fdt_header *working_fdt; + /** * fdt_getprop_u32_default_node - Return a node's property or a default * diff --git a/cmd/fdt.c b/cmd/fdt.c index 2401ea8b44cb..f842fd84b4b2 100644 --- a/cmd/fdt.c +++ b/cmd/fdt.c @@ -31,11 +31,6 @@ static int fdt_parse_prop(char *const*newval, int count, char *data, int *len); static int fdt_print(const char *pathp, char *prop, int depth); static int is_printable_string(const void *data, int len);
-/* - * The working_fdt points to our working flattened device tree. - */ -struct fdt_header *working_fdt; - static void set_working_fdt_addr_quiet(ulong addr) { void *buf;
At present it isn't possible to use networking without the command line enabled. Add this as a condition.
Signed-off-by: Simon Glass sjg@chromium.org ---
cmd/Kconfig | 1 + net/Kconfig | 1 + 2 files changed, 2 insertions(+)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 5f6834b335dc..c3428d19f31d 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1687,6 +1687,7 @@ if NET
menuconfig CMD_NET bool "Network commands" + depends on CMDLINE default y imply NETDEVICES
diff --git a/net/Kconfig b/net/Kconfig index 4215889127c9..25d494e1db46 100644 --- a/net/Kconfig +++ b/net/Kconfig @@ -4,6 +4,7 @@
menuconfig NET bool "Networking support" + depends on CMDLINE default y
if NET
On Sun, Sep 24, 2023 at 11:40 PM Simon Glass sjg@chromium.org wrote:
At present it isn't possible to use networking without the command line enabled. Add this as a condition.
Signed-off-by: Simon Glass sjg@chromium.org
cmd/Kconfig | 1 + net/Kconfig | 1 + 2 files changed, 2 insertions(+)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 5f6834b335dc..c3428d19f31d 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1687,6 +1687,7 @@ if NET
menuconfig CMD_NET bool "Network commands"
depends on CMDLINE default y imply NETDEVICESdiff --git a/net/Kconfig b/net/Kconfig index 4215889127c9..25d494e1db46 100644 --- a/net/Kconfig +++ b/net/Kconfig @@ -4,6 +4,7 @@
menuconfig NET bool "Networking support"
depends on CMDLINE default yif NET
2.42.0.515.g380fc7ccd1-goog
Reviewed-by: Ramon Fried rfried.dev@gmail.com
When CONFIG_SYS_CBSIZE is not used we need an alternative. For logging it seems that CONFIG_SYS_PBSIZE is a better choice anyway, so update this.
Signed-off-by: Simon Glass sjg@chromium.org ---
common/log.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/common/log.c b/common/log.c index b2de57fcb3b8..72a4de3274c7 100644 --- a/common/log.c +++ b/common/log.c @@ -206,7 +206,7 @@ static bool log_passes_filters(struct log_device *ldev, struct log_rec *rec) static int log_dispatch(struct log_rec *rec, const char *fmt, va_list args) { struct log_device *ldev; - char buf[CONFIG_SYS_CBSIZE]; + char buf[CONFIG_SYS_PBSIZE];
/* * When a log driver writes messages (e.g. via the network stack) this @@ -268,7 +268,7 @@ int _log(enum log_category_t cat, enum log_level_t level, const char *file, /* display dropped traces with console puts and DEBUG_UART */ if (rec.level <= CONFIG_LOG_DEFAULT_LEVEL || rec.flags & LOGRECF_FORCE_DEBUG) { - char buf[CONFIG_SYS_CBSIZE]; + char buf[CONFIG_SYS_PBSIZE];
va_start(args, fmt); vsnprintf(buf, sizeof(buf), fmt, args);
Provide a fallback for when CONFIG_SYS_CBSIZE is not provided, so that the console can still be used.
Signed-off-by: Simon Glass sjg@chromium.org ---
drivers/video/console_truetype.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/video/console_truetype.c b/drivers/video/console_truetype.c index 0f9bb49e44f7..8186b5b49f0b 100644 --- a/drivers/video/console_truetype.c +++ b/drivers/video/console_truetype.c @@ -124,7 +124,11 @@ struct pos_info { * Allow one for each character on the command line plus one for each newline. * This is just an estimate, but it should not be exceeded. */ +#ifdef CONFIG_SYS_CBSIZE #define POS_HISTORY_SIZE (CONFIG_SYS_CBSIZE * 11 / 10) +#else +#define POS_HISTORY_SIZE (250 * 11 / 10) +#endif
/** * struct console_tt_metrics - Information about a font / size combination
While it is nice to have the font command, using 'select' makes it impossible to build the console code without it. Change this to use 'imply' instead.
Signed-off-by: Simon Glass sjg@chromium.org ---
drivers/video/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index ab927641bb7a..21ea5c860cca 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -180,7 +180,7 @@ config CONSOLE_ROTATION
config CONSOLE_TRUETYPE bool "Support a console that uses TrueType fonts" - select CMD_SELECT_FONT + imply CMD_SELECT_FONT help TrueTrype fonts can provide outline-drawing capability rather than needing to provide a bitmap for each font and size that is needed.
This features currently requires the command line, so make this explicit. Future work could adjust this, but it needs effort within the booting support first, like the bootm command.
Signed-off-by: Simon Glass sjg@chromium.org ---
lib/efi_loader/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 621ed5e5b0fb..2aef9336034e 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -12,6 +12,7 @@ config EFI_LOADER depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT depends on BLK depends on !EFI_APP + depends on CMDLINE default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8 select CHARSET # We need to send DM events, dynamically, in the EFI block driver
If this option is disabled, commands should not be available. Convert the CMDLINE option into a menuconfig and make every command in cmd/Kconfig depend on it.
Signed-off-by: Simon Glass sjg@chromium.org ---
cmd/Kconfig | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-)
diff --git a/cmd/Kconfig b/cmd/Kconfig index c3428d19f31d..7aa689e91f6f 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1,7 +1,5 @@ -menu "Command line interface" - -config CMDLINE - bool "Support U-Boot commands" +menuconfig CMDLINE + bool "Command line interface" default y help Enable U-Boot's command-line functions. This provides a means @@ -11,9 +9,10 @@ config CMDLINE Depending on the number of commands enabled, this can add substantially to the size of U-Boot.
+if CMDLINE + config HUSH_PARSER bool "Use hush shell" - depends on CMDLINE help This option enables the "hush" shell (from Busybox) as command line interpreter, thus enabling powerful command line syntax like @@ -25,7 +24,6 @@ config HUSH_PARSER
config CMDLINE_EDITING bool "Enable command line editing" - depends on CMDLINE default y help Enable editing and History functions for interactive command line @@ -40,15 +38,13 @@ config CMDLINE_PS_SUPPORT
config AUTO_COMPLETE bool "Enable auto complete using TAB" - depends on CMDLINE default y help Enable auto completion of commands using TAB.
config SYS_LONGHELP bool "Enable long help messages" - depends on CMDLINE - default y if CMDLINE + default y help Defined when you want long help messages included Do not set this option when short of memory. @@ -86,8 +82,7 @@ config SYS_CBSIZE
config SYS_XTRACE bool "Command execution tracer" - depends on CMDLINE - default y if CMDLINE + default y help This option enables the possiblity to print all commands before executing them and after all variables are evaluated (similar @@ -219,7 +214,6 @@ menu "Boot commands"
config CMD_BOOTD bool "bootd" - depends on CMDLINE default y help Run the command stored in the environment "bootcmd", i.e. @@ -409,7 +403,6 @@ source lib/efi_selftest/Kconfig
config CMD_BOOTMENU bool "bootmenu" - depends on CMDLINE select MENU select CHARSET help @@ -476,7 +469,6 @@ config CMD_GO
config CMD_RUN bool "run" - depends on CMDLINE default y help Run the command in the given environment variable. @@ -572,7 +564,6 @@ menu "Environment commands"
config CMD_ASKENV bool "ask for env variable" - depends on CMDLINE help Ask for environment variable
@@ -1687,7 +1678,6 @@ if NET
menuconfig CMD_NET bool "Network commands" - depends on CMDLINE default y imply NETDEVICES
@@ -1991,7 +1981,6 @@ config CMD_ETHSW
config CMD_PXE bool "pxe" - depends on CMDLINE select PXE_UTILS help Boot image via network using PXE protocol @@ -2126,7 +2115,6 @@ config CMD_EFICONFIG
config CMD_EXCEPTION bool "exception - raise exception" - depends on CMDLINE depends on ARM || RISCV || SANDBOX || X86 help Enable the 'exception' command which allows to raise an exception. @@ -2227,14 +2215,12 @@ config CMD_SOUND
config CMD_SYSBOOT bool "sysboot" - depends on CMDLINE select PXE_UTILS help Boot image via local extlinux.conf file
config CMD_QFW bool "qfw" - depends on CMDLINE select QFW help This provides access to the QEMU firmware interface. The main @@ -2883,4 +2869,5 @@ config CMD_MESON default y help Enable useful commands for the Meson Soc family developed by Amlogic Inc. -endmenu + +endif # CMDLINE
Use 'imply' rather than 'select' for command-related options, so that it is possible to build sandbox without CONFIG_CMDLINE enabled.
Signed-off-by: Simon Glass sjg@chromium.org ---
arch/Kconfig | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig index 19f2891ba1c5..789be7a9f1e8 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -134,7 +134,6 @@ config SANDBOX select ARCH_SUPPORTS_LTO select BOARD_LATE_INIT select BZIP2 - select CMD_POWEROFF select DM select DM_EVENT select DM_FUZZING_ENGINE @@ -152,10 +151,8 @@ config SANDBOX select PCI_ENDPOINT select SPI select SUPPORT_OF_CONTROL - select SYSRESET_CMD_POWEROFF select SYS_CACHE_SHIFT_4 select IRQ - select SUPPORT_EXTENSION_SCAN select SUPPORT_ACPI imply BITREVERSE select BLOBLIST @@ -167,6 +164,7 @@ config SANDBOX imply CMD_IO imply CMD_IOTRACE imply CMD_LZMADEC + imply CMD_POWEROFF imply CMD_SF imply CMD_SF_TEST imply CRC32_VERIFY @@ -208,6 +206,8 @@ config SANDBOX imply PHYSMEM imply GENERATE_ACPI_TABLE imply BINMAN + imply SYSRESET_CMD_POWEROFF + imply SUPPORT_EXTENSION_SCAN
config SH bool "SuperH architecture"
Now that everything is working, add a test to make sure that this builds correctly.
Signed-off-by: Simon Glass sjg@chromium.org ---
test/py/tests/test_sandbox_opts.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 test/py/tests/test_sandbox_opts.py
diff --git a/test/py/tests/test_sandbox_opts.py b/test/py/tests/test_sandbox_opts.py new file mode 100644 index 000000000000..91790b3374b4 --- /dev/null +++ b/test/py/tests/test_sandbox_opts.py @@ -0,0 +1,20 @@ +# SPDX-License-Identifier: GPL-2.0 +# Copyright 2022 Google LLC +# Written by Simon Glass sjg@chromium.org + +import pytest + +import u_boot_utils as util + +# This is needed for Azure, since the default '..' directory is not writeable +TMPDIR = '/tmp/test_cmdline' + +@pytest.mark.slow +@pytest.mark.boardspec('sandbox') +def test_sandbox_cmdline(u_boot_console): + """Test building sandbox without CONFIG_CMDLINE""" + cons = u_boot_console + + out = util.run_and_log( + cons, ['./tools/buildman/buildman', '-m', '--board', 'sandbox', + '-a', '~CMDLINE', '-o', TMPDIR])
On Sun, Sep 24, 2023 at 02:39:18PM -0600, Simon Glass wrote:
It should be possible to disable CONFIG_CMDLINE and have all commands and related functionality dropped from U-Boot. This is useful when trying to reduce the size of U-Boot.
Recent changes have stopped this from working.
This series repairs the feature for sandbox and adds a test to stop it breaking again.
Note that quite a lot of functionality is lost of CONFIG_CMDLINE is disabled, e.g. networking and most booting options. Further work is needed to make the option more generally useful.
I worry there's a lot of "make it compile, deal with it later" in here rather than unwinding so that $X works with CMDLINE disabled or we truly must have CMDLINE. Perhaps it would be better to start with to take one of the platforms that enables say networking in SPL, where we functionally don't have a cmdline, and make that build with CMDLINE off. Having *PL build and link and work with CMDLINE disabled would possibly save some space too, which is always a good thing there.
Hi Tom,
On Sun, 24 Sept 2023 at 18:37, Tom Rini trini@konsulko.com wrote:
On Sun, Sep 24, 2023 at 02:39:18PM -0600, Simon Glass wrote:
It should be possible to disable CONFIG_CMDLINE and have all commands and related functionality dropped from U-Boot. This is useful when trying to reduce the size of U-Boot.
Recent changes have stopped this from working.
This series repairs the feature for sandbox and adds a test to stop it breaking again.
Note that quite a lot of functionality is lost of CONFIG_CMDLINE is disabled, e.g. networking and most booting options. Further work is needed to make the option more generally useful.
I worry there's a lot of "make it compile, deal with it later" in here rather than unwinding so that $X works with CMDLINE disabled or we truly must have CMDLINE. Perhaps it would be better to start with to take one of the platforms that enables say networking in SPL, where we functionally don't have a cmdline, and make that build with CMDLINE off. Having *PL build and link and work with CMDLINE disabled would possibly save some space too, which is always a good thing there.
So long as it gets us closer to the goal, I think the work is valuable. Also it prevents additional regressions from creeping in, at least for sandbox. I am under no illusions about how big an effort this is, but we have to start somewhere. To me the easiest first step would be to decouple bootm functionality from the bootm command. Likely that would be <40 patches.
Regards, Simon
participants (4)
-
Ramon Fried -
Sean Anderson -
Simon Glass -
Tom Rini