| Message ID | 20200813013727.8186-1-judge.packham@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | Wolfgang Denk |
| Headers | show |
| Series | [RFC] tools: env: Add an option to have an empty default environment | expand |
On Wed, Aug 12, 2020 at 8:37 PM Chris Packham <judge.packham@gmail.com> wrote: > > When building envtools via tools-only_defconfig the builtin defaults > are based on options in the defconfig. For example: > > bootcmd=bootp; setenv bootargs root=/dev/nfs nfsroot=${serverip}:${rootpath} ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}:${hostname}::off; bootm > bootdelay=2 > baudrate=115200 > stdin=serial,cros-ec-keyb,usbkbd > stdout=serial,vidconsole > stderr=serial,vidconsole > ethaddr=00:00:11:22:33:44 > eth3addr=00:00:11:22:33:45 > eth5addr=00:00:11:22:33:46 > eth6addr=00:00:11:22:33:47 > ipaddr=1.2.3.4 > bootm_size=0x10000000 > kernel_addr_r=0x1000000 > fdt_addr_r=0xc00000 > ramdisk_addr_r=0x2000000 > scriptaddr=0x1000 > pxefile_addr_r=0x2000 > > These may or may not be sensible for any particular target. Rather than > trying to have a set of defaults that work for every target add a config > option to make the default environment completely empty. > > Signed-off-by: Chris Packham <judge.packham@gmail.com> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
Dear Chris Packham, In message <20200813013727.8186-1-judge.packham@gmail.com> you wrote: > When building envtools via tools-only_defconfig the builtin defaults > are based on options in the defconfig. For example: > > bootcmd=bootp; setenv bootargs root=/dev/nfs nfsroot=${serverip}:${rootpath} ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}:${hostname}::off; bootm > bootdelay=2 > baudrate=115200 > stdin=serial,cros-ec-keyb,usbkbd > stdout=serial,vidconsole > stderr=serial,vidconsole > ethaddr=00:00:11:22:33:44 > eth3addr=00:00:11:22:33:45 > eth5addr=00:00:11:22:33:46 > eth6addr=00:00:11:22:33:47 Having any specific MAC addresses set in the default environment has always been a strict No-Go as it is just begging for trouble. If this was a real example, that code should be fixed!! > These may or may not be sensible for any particular target. Rather than > trying to have a set of defaults that work for every target add a config > option to make the default environment completely empty. An empty default environment is even worse, as you are missing vital settings like baudrate, bootdelay etc. There are good chances to brick your board with such settings. Best regards, Wolfgang Denk
Hi Chris, On 13.08.20 03:37, Chris Packham wrote: > When building envtools via tools-only_defconfig the builtin defaults > are based on options in the defconfig. For example: > > bootcmd=bootp; setenv bootargs root=/dev/nfs nfsroot=${serverip}:${rootpath} ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}:${hostname}::off; bootm > bootdelay=2 > baudrate=115200 > stdin=serial,cros-ec-keyb,usbkbd > stdout=serial,vidconsole > stderr=serial,vidconsole > ethaddr=00:00:11:22:33:44 > eth3addr=00:00:11:22:33:45 > eth5addr=00:00:11:22:33:46 > eth6addr=00:00:11:22:33:47 > ipaddr=1.2.3.4 > bootm_size=0x10000000 > kernel_addr_r=0x1000000 > fdt_addr_r=0xc00000 > ramdisk_addr_r=0x2000000 > scriptaddr=0x1000 > pxefile_addr_r=0x2000 > > These may or may not be sensible for any particular target. Rather than > trying to have a set of defaults that work for every target add a config > option to make the default environment completely empty. > > Signed-off-by: Chris Packham <judge.packham@gmail.com> > --- Which is the added value compared to use CONFIG_USE_DEFAULT_ENV_FILE=y and adding your file with no variables ? Best regards, Stefano > > configs/tools-only_defconfig | 1 + > env/Kconfig | 7 +++++++ > include/env_default.h | 2 ++ > 3 files changed, 10 insertions(+) > > diff --git a/configs/tools-only_defconfig b/configs/tools-only_defconfig > index a853abf2b8fc..0ddd0518ef9a 100644 > --- a/configs/tools-only_defconfig > +++ b/configs/tools-only_defconfig > @@ -12,6 +12,7 @@ CONFIG_BOOTP_DNS2=y > CONFIG_OF_CONTROL=y > CONFIG_OF_HOSTFILE=y > CONFIG_SYS_RELOC_GD_ENV_ADDR=y > +CONFIG_EMPTY_DEFAULT_ENV=y > CONFIG_BOOTP_SEND_HOSTNAME=y > CONFIG_IP_DEFRAG=y > # CONFIG_ACPIGEN is not set > diff --git a/env/Kconfig b/env/Kconfig > index af4d9cbaa4d8..f7860f01cc16 100644 > --- a/env/Kconfig > +++ b/env/Kconfig > @@ -614,6 +614,13 @@ config DEFAULT_ENV_FILE > containing key=value pairs, blank lines and lines beginning > with # are ignored. > > +config EMPTY_DEFAULT_ENV > + bool "Create an empty default environment" > + help > + Create an empty default environment. This is intended to be > + used when building generic target tools and no sensible > + default that can be included. > + > config ENV_VARS_UBOOT_RUNTIME_CONFIG > bool "Add run-time information to the environment" > help > diff --git a/include/env_default.h b/include/env_default.h > index 8a0c3057f0aa..859188a8e7c9 100644 > --- a/include/env_default.h > +++ b/include/env_default.h > @@ -23,6 +23,7 @@ static char default_environment[] = { > const uchar default_environment[] = { > #endif > #ifndef CONFIG_USE_DEFAULT_ENV_FILE > +#ifndef CONFIG_EMPTY_DEFAULT_ENV > #ifdef CONFIG_ENV_CALLBACK_LIST_DEFAULT > ENV_CALLBACK_VAR "=" CONFIG_ENV_CALLBACK_LIST_DEFAULT "\0" > #endif > @@ -107,6 +108,7 @@ const uchar default_environment[] = { > #endif > #ifdef CONFIG_EXTRA_ENV_SETTINGS > CONFIG_EXTRA_ENV_SETTINGS > +#endif > #endif > "\0" > #else /* CONFIG_USE_DEFAULT_ENV_FILE */ >
On Tue, 18 Aug 2020, 11:02 PM Stefano Babic, <sbabic@denx.de> wrote: > Hi Chris, > > On 13.08.20 03:37, Chris Packham wrote: > > When building envtools via tools-only_defconfig the builtin defaults > > are based on options in the defconfig. For example: > > > > bootcmd=bootp; setenv bootargs root=/dev/nfs > nfsroot=${serverip}:${rootpath} > ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}:${hostname}::off; bootm > > bootdelay=2 > > baudrate=115200 > > stdin=serial,cros-ec-keyb,usbkbd > > stdout=serial,vidconsole > > stderr=serial,vidconsole > > ethaddr=00:00:11:22:33:44 > > eth3addr=00:00:11:22:33:45 > > eth5addr=00:00:11:22:33:46 > > eth6addr=00:00:11:22:33:47 > > ipaddr=1.2.3.4 > > bootm_size=0x10000000 > > kernel_addr_r=0x1000000 > > fdt_addr_r=0xc00000 > > ramdisk_addr_r=0x2000000 > > scriptaddr=0x1000 > > pxefile_addr_r=0x2000 > > > > These may or may not be sensible for any particular target. Rather than > > trying to have a set of defaults that work for every target add a config > > option to make the default environment completely empty. > > > > Signed-off-by: Chris Packham <judge.packham@gmail.com> > > --- > > Which is the added value compared to use CONFIG_USE_DEFAULT_ENV_FILE=y > and adding your file with no variables ? > Long story short that was what I tried first but I was having trouble making it work with tools-only_defconfig. But I think the point remains, there's just no sane set of defaults that can be built-in to a generic fw_setenv tool. > Best regards, > Stefano > > > > > configs/tools-only_defconfig | 1 + > > env/Kconfig | 7 +++++++ > > include/env_default.h | 2 ++ > > 3 files changed, 10 insertions(+) > > > > diff --git a/configs/tools-only_defconfig b/configs/tools-only_defconfig > > index a853abf2b8fc..0ddd0518ef9a 100644 > > --- a/configs/tools-only_defconfig > > +++ b/configs/tools-only_defconfig > > @@ -12,6 +12,7 @@ CONFIG_BOOTP_DNS2=y > > CONFIG_OF_CONTROL=y > > CONFIG_OF_HOSTFILE=y > > CONFIG_SYS_RELOC_GD_ENV_ADDR=y > > +CONFIG_EMPTY_DEFAULT_ENV=y > > CONFIG_BOOTP_SEND_HOSTNAME=y > > CONFIG_IP_DEFRAG=y > > # CONFIG_ACPIGEN is not set > > diff --git a/env/Kconfig b/env/Kconfig > > index af4d9cbaa4d8..f7860f01cc16 100644 > > --- a/env/Kconfig > > +++ b/env/Kconfig > > @@ -614,6 +614,13 @@ config DEFAULT_ENV_FILE > > containing key=value pairs, blank lines and lines beginning > > with # are ignored. > > > > +config EMPTY_DEFAULT_ENV > > + bool "Create an empty default environment" > > + help > > + Create an empty default environment. This is intended to be > > + used when building generic target tools and no sensible > > + default that can be included. > > + > > config ENV_VARS_UBOOT_RUNTIME_CONFIG > > bool "Add run-time information to the environment" > > help > > diff --git a/include/env_default.h b/include/env_default.h > > index 8a0c3057f0aa..859188a8e7c9 100644 > > --- a/include/env_default.h > > +++ b/include/env_default.h > > @@ -23,6 +23,7 @@ static char default_environment[] = { > > const uchar default_environment[] = { > > #endif > > #ifndef CONFIG_USE_DEFAULT_ENV_FILE > > +#ifndef CONFIG_EMPTY_DEFAULT_ENV > > #ifdef CONFIG_ENV_CALLBACK_LIST_DEFAULT > > ENV_CALLBACK_VAR "=" CONFIG_ENV_CALLBACK_LIST_DEFAULT "\0" > > #endif > > @@ -107,6 +108,7 @@ const uchar default_environment[] = { > > #endif > > #ifdef CONFIG_EXTRA_ENV_SETTINGS > > CONFIG_EXTRA_ENV_SETTINGS > > +#endif > > #endif > > "\0" > > #else /* CONFIG_USE_DEFAULT_ENV_FILE */ > > > > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > ===================================================================== >
Hi Wolfgang, On Tue, 18 Aug 2020, 10:27 PM Wolfgang Denk, <wd@denx.de> wrote: > Dear Chris Packham, > > In message <20200813013727.8186-1-judge.packham@gmail.com> you wrote: > > When building envtools via tools-only_defconfig the builtin defaults > > are based on options in the defconfig. For example: > > > > bootcmd=bootp; setenv bootargs root=/dev/nfs > nfsroot=${serverip}:${rootpath} > ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}:${hostname}::off; bootm > > bootdelay=2 > > baudrate=115200 > > stdin=serial,cros-ec-keyb,usbkbd > > stdout=serial,vidconsole > > stderr=serial,vidconsole > > ethaddr=00:00:11:22:33:44 > > eth3addr=00:00:11:22:33:45 > > eth5addr=00:00:11:22:33:46 > > eth6addr=00:00:11:22:33:47 > > Having any specific MAC addresses set in the default environment has > always been a strict No-Go as it is just begging for trouble. > > If this was a real example, that code should be fixed!! > Yes it's what you get if you build tools-only_defconfig as of 2020.07. There's also various addresses that are bad for the boards I deal with. > > These may or may not be sensible for any particular target. Rather than > > trying to have a set of defaults that work for every target add a config > > option to make the default environment completely empty. > > An empty default environment is even worse, as you are missing vital > settings like baudrate, bootdelay etc. There are good chances to > brick your board with such settings. > I believe the baudrate has build time default that is suitable if nothing is set. But yes there are things necessary for a board to boot an OS that are undone by a blank environment. But the same also applies to the wrong value. The current defaults render my boards expensive bricks. What I think might be the best path forward is being able to supply defaults via a file at run time. That way we can have a generic fw_setenv but supply board specific defaults via a config file. > Best regards, > > Wolfgang Denk > > -- > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de > The word "fit", as I understand it, means "appropriate to a purpose", > and I would say the body of the Dean is supremely appropriate to the > purpose of sitting around all day and eating big heavy meals. > - Terry Pratchett, _Moving Pictures_ >
Dear Chris, In message <CAFOYHZBz3gYXH=a9bj1BEnM4ztVgxjDUYzQGCsJy47Exggb0TA@mail.gmail.com> you wrote: > > What I think might be the best path forward is being able to supply > defaults via a file at run time. That way we can have a generic fw_setenv > but supply board specific defaults via a config file. This would indeed make sense (though I would not call this any longer "default" then). Patches welcome! > Content-Type: text/html; charset="UTF-8" > Content-Transfer-Encoding: quoted-printable And please - stop sending HTML to the list! Thanks. Best regards, Wolfgang Denk
diff --git a/configs/tools-only_defconfig b/configs/tools-only_defconfig index a853abf2b8fc..0ddd0518ef9a 100644 --- a/configs/tools-only_defconfig +++ b/configs/tools-only_defconfig @@ -12,6 +12,7 @@ CONFIG_BOOTP_DNS2=y CONFIG_OF_CONTROL=y CONFIG_OF_HOSTFILE=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y +CONFIG_EMPTY_DEFAULT_ENV=y CONFIG_BOOTP_SEND_HOSTNAME=y CONFIG_IP_DEFRAG=y # CONFIG_ACPIGEN is not set diff --git a/env/Kconfig b/env/Kconfig index af4d9cbaa4d8..f7860f01cc16 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -614,6 +614,13 @@ config DEFAULT_ENV_FILE containing key=value pairs, blank lines and lines beginning with # are ignored. +config EMPTY_DEFAULT_ENV + bool "Create an empty default environment" + help + Create an empty default environment. This is intended to be + used when building generic target tools and no sensible + default that can be included. + config ENV_VARS_UBOOT_RUNTIME_CONFIG bool "Add run-time information to the environment" help diff --git a/include/env_default.h b/include/env_default.h index 8a0c3057f0aa..859188a8e7c9 100644 --- a/include/env_default.h +++ b/include/env_default.h @@ -23,6 +23,7 @@ static char default_environment[] = { const uchar default_environment[] = { #endif #ifndef CONFIG_USE_DEFAULT_ENV_FILE +#ifndef CONFIG_EMPTY_DEFAULT_ENV #ifdef CONFIG_ENV_CALLBACK_LIST_DEFAULT ENV_CALLBACK_VAR "=" CONFIG_ENV_CALLBACK_LIST_DEFAULT "\0" #endif @@ -107,6 +108,7 @@ const uchar default_environment[] = { #endif #ifdef CONFIG_EXTRA_ENV_SETTINGS CONFIG_EXTRA_ENV_SETTINGS +#endif #endif "\0" #else /* CONFIG_USE_DEFAULT_ENV_FILE */
When building envtools via tools-only_defconfig the builtin defaults are based on options in the defconfig. For example: bootcmd=bootp; setenv bootargs root=/dev/nfs nfsroot=${serverip}:${rootpath} ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}:${hostname}::off; bootm bootdelay=2 baudrate=115200 stdin=serial,cros-ec-keyb,usbkbd stdout=serial,vidconsole stderr=serial,vidconsole ethaddr=00:00:11:22:33:44 eth3addr=00:00:11:22:33:45 eth5addr=00:00:11:22:33:46 eth6addr=00:00:11:22:33:47 ipaddr=1.2.3.4 bootm_size=0x10000000 kernel_addr_r=0x1000000 fdt_addr_r=0xc00000 ramdisk_addr_r=0x2000000 scriptaddr=0x1000 pxefile_addr_r=0x2000 These may or may not be sensible for any particular target. Rather than trying to have a set of defaults that work for every target add a config option to make the default environment completely empty. Signed-off-by: Chris Packham <judge.packham@gmail.com> --- configs/tools-only_defconfig | 1 + env/Kconfig | 7 +++++++ include/env_default.h | 2 ++ 3 files changed, 10 insertions(+)