Re: [RFC PATCH] tools: env: Add an option to have an empty default environment
On Wed, Aug 12, 2020 at 8:37 PM 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=0x1000
> kernel_addr_r=0x100
> fdt_addr_r=0xc0
> ramdisk_addr_r=0x200
> 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
Acked-by: Joe Hershberger
Re: [RFC PATCH] tools: env: Add an option to have an empty default environment
Dear Chris Packham,
In message <20200813013727.8186-1-judge.pack...@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
--
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: w...@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_
Re: [RFC PATCH] tools: env: Add an option to have an empty default environment
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=0x1000
> kernel_addr_r=0x100
> fdt_addr_r=0xc0
> ramdisk_addr_r=0x200
> 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
> ---
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 */
>
--
=
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: sba...@denx.de
=
Re: [RFC PATCH] tools: env: Add an option to have an empty default environment
On Tue, 18 Aug 2020, 11:02 PM Stefano Babic, 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=0x1000
> > kernel_addr_r=0x100
> > fdt_addr_r=0xc0
> > ramdisk_addr_r=0x200
> > 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
> > ---
>
> 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: sba...@denx.de
> =
>
Re: [RFC PATCH] tools: env: Add an option to have an empty default environment
Hi Wolfgang,
On Tue, 18 Aug 2020, 10:27 PM Wolfgang Denk, wrote:
> Dear Chris Packham,
>
> In message <20200813013727.8186-1-judge.pack...@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: w...@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_
>
Re: [RFC PATCH] tools: env: Add an option to have an empty default environment
Dear Chris, In message 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 -- 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: w...@denx.de "Pull the wool over your own eyes!"- J.R. "Bob" Dobbs
