[PATCH 0/6] rockchip: puma-rk3399/ringneck-px30: fix check against value in default environment variable

[PATCH 0/6] rockchip: puma-rk3399/ringneck-px30: fix check against value in default environment variable

[Quentin Schulz]

For puma-rk3399 and ringneck-px30, U-Boot proper automatically modifies
boot_targets to swap the order in which MMC storage media are used for
standard boot based on which MMC storage medium was used to load U-Boot
proper. This is however only done if the user has not changed it
manually, therefore a check between the default and current value is
done.

This used to work fine until the migration to standard boot where
boot_targets value size in the default environment went above the 32
characters that env_get_default function can return, thus resulting in a
truncated variable.

Therefore the check between default and current value would always fail.

By adding the env_get_default_into function, a buffer of the appropriate
size can be allocated on the stack to get the whole value of
boot_targets in the default environment and thus fixing the check.

While at it, only compile functions in the U-Boot boot phase they are
used in. This fixes an issue where BOOT_TARGETS constant isn't defined
in SPL or TPL and thus failing the build.

Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
Quentin Schulz (6):
      env: allow to copy value from default environment into a buffer
      env: migrate env_get_default to call env_get_default_into
      rockchip: puma-rk3399: only declare functions in U-Boot proper
      rockchip: ringneck-px30: only declare functions in U-Boot proper
      rockchip: puma-rk3399: fix modified boot_targets detection
      rockchip: ringneck-px30: fix modified boot_targets detection

 board/theobroma-systems/puma_rk3399/puma-rk3399.c     | 11 +++++++++--
 board/theobroma-systems/ringneck_px30/ringneck-px30.c | 11 +++++++++--
 env/common.c                                          | 16 +++++++++++++---
 include/env.h                                         | 10 ++++++++++
 4 files changed, 41 insertions(+), 7 deletions(-)
---
base-commit: e17d174773e9ba9447596708e702b7382e47a6cf
change-id: 20231108-env_default_theobroma-33774a337fe6

Best regards,
-- 
Quentin Schulz <quentin.schulz@theobroma-systems.com>

[PATCH 1/6] env: allow to copy value from default environment into a buffer

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

env_get_default suffers from a particular issue int that it can only
return a value truncated to gd->env_buf (32) characters. This may be
enough for most variables but it isn't for others, so let's allow users
to provide a preallocated buffer to copy the value into instead,
allowing for more control, though it'll still be truncated if the value
size is bigger than the preallocated buffer.

Cc: Quentin Schulz <foss+uboot@0leil.net>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
 env/common.c  |  8 ++++++++
 include/env.h | 10 ++++++++++
 2 files changed, 18 insertions(+)

diff --git a/env/common.c b/env/common.c
index eb1a9137953..307003099dd 100644
--- a/env/common.c
+++ b/env/common.c
@@ -254,6 +254,14 @@ char *env_get_default(const char *name)
 	return NULL;
 }
 
+/*
+ * Look up the variable from the default environment and store its value in buf
+ */
+int env_get_default_into(const char *name, char *buf, unsigned int len)
+{
+	return env_get_from_linear(default_environment, name, buf, len);
+}
+
 void env_set_default(const char *s, int flags)
 {
 	if (s) {
diff --git a/include/env.h b/include/env.h
index 430c4fa94a4..9a406de3781 100644
--- a/include/env.h
+++ b/include/env.h
@@ -348,6 +348,16 @@ int env_import_redund(const char *buf1, int buf1_read_fail,
  */
 char *env_get_default(const char *name);
 
+/**
+ * env_get_default_into() - Look up a variable from the default environment and
+ * copy its value in buf.
+ *
+ * @name: Variable to look up
+ * Return: actual length of the variable value excluding the terminating
+ *	NULL-byte, or -1 if the variable is not found
+ */
+int env_get_default_into(const char *name, char *buf, unsigned int len);
+
 /* [re]set to the default environment */
 void env_set_default(const char *s, int flags);
 

-- 
2.41.0

[PATCH 2/6] env: migrate env_get_default to call env_get_default_into

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Since both functions share a similar goal and env_get_default_into can
do what env_get_default wants to do with specific arguments, let's make
env_get_default call env_get_default_into so as to avoid code
duplication.

Cc: Quentin Schulz <foss+uboot@0leil.net>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
 env/common.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/env/common.c b/env/common.c
index 307003099dd..930bdef2f58 100644
--- a/env/common.c
+++ b/env/common.c
@@ -246,9 +246,11 @@ bool env_get_autostart(void)
  */
 char *env_get_default(const char *name)
 {
-	if (env_get_from_linear(default_environment, name,
-				(char *)(gd->env_buf),
-				sizeof(gd->env_buf)) >= 0)
+	int ret;
+
+	ret = env_get_default_into(name, (char *)(gd->env_buf),
+				   sizeof(gd->env_buf));
+	if (ret >= 0)
 		return (char *)(gd->env_buf);
 
 	return NULL;

-- 
2.41.0

[PATCH 3/6] rockchip: puma-rk3399: only declare functions in U-Boot proper

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Those functions are useless in anything but U-Boot proper for now, so
let's compile them out explicitly.

Cc: Quentin Schulz <foss+uboot@0leil.net>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
 board/theobroma-systems/puma_rk3399/puma-rk3399.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/board/theobroma-systems/puma_rk3399/puma-rk3399.c b/board/theobroma-systems/puma_rk3399/puma-rk3399.c
index 614a60ee8f9..1b7a39b0474 100644
--- a/board/theobroma-systems/puma_rk3399/puma-rk3399.c
+++ b/board/theobroma-systems/puma_rk3399/puma-rk3399.c
@@ -26,6 +26,7 @@
 #include <power/regulator.h>
 #include <u-boot/sha256.h>
 
+#ifndef CONFIG_SPL_BUILD
 static void setup_iodomain(void)
 {
 	const u32 GRF_IO_VSEL_GPIO4CD_SHIFT = 3;
@@ -192,3 +193,4 @@ int misc_init_r(void)
 
 	return 0;
 }
+#endif

-- 
2.41.0

[PATCH 4/6] rockchip: ringneck-px30: only declare functions in U-Boot proper

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Those functions are useless in anything but U-Boot proper for now, so
let's compile them out explicitly.

Cc: Quentin Schulz <foss+uboot@0leil.net>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
 board/theobroma-systems/ringneck_px30/ringneck-px30.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/board/theobroma-systems/ringneck_px30/ringneck-px30.c b/board/theobroma-systems/ringneck_px30/ringneck-px30.c
index bb1bb4acf5c..dd711cd05de 100644
--- a/board/theobroma-systems/ringneck_px30/ringneck-px30.c
+++ b/board/theobroma-systems/ringneck_px30/ringneck-px30.c
@@ -25,6 +25,7 @@
 #include <power/regulator.h>
 #include <u-boot/sha256.h>
 
+#ifndef CONFIG_SPL_BUILD
 /*
  * Swap mmc0 and mmc1 in boot_targets if booted from SD-Card.
  *
@@ -169,3 +170,4 @@ int misc_init_r(void)
 
 	return 0;
 }
+#endif

-- 
2.41.0

[PATCH 5/6] rockchip: puma-rk3399: fix modified boot_targets detection

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

U-Boot proper automatically modifies boot_targets to swap the order in
which MMC storage media are used for standard boot based on which MMC
storage medium was used to load U-Boot proper. This is however only done
if the user has not changed it manually, therefore a check between the
default and current value is done.

This used to work fine until the migration to standard boot where
boot_targets value size in the default environment went above the 32
characters that env_get_default function can return, thus resulting in a
truncated variable.

Therefore the check between default and current value would always fail.

By using the newly added env_get_default_into function, a buffer of
the appropriate size can be allocated on the stack to get the whole
value of boot_targets in the default environment and thus fixing the
check.

Cc: Quentin Schulz <foss+uboot@0leil.net>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
 board/theobroma-systems/puma_rk3399/puma-rk3399.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/board/theobroma-systems/puma_rk3399/puma-rk3399.c b/board/theobroma-systems/puma_rk3399/puma-rk3399.c
index 1b7a39b0474..df667fae5fc 100644
--- a/board/theobroma-systems/puma_rk3399/puma-rk3399.c
+++ b/board/theobroma-systems/puma_rk3399/puma-rk3399.c
@@ -54,7 +54,9 @@ static int setup_boottargets(void)
 {
 	const char *boot_device =
 		ofnode_read_chosen_string("u-boot,spl-boot-device");
-	char *env_default, *env;
+	char env_default[sizeof(BOOT_TARGETS)];
+	char *env;
+	int ret;
 
 	if (!boot_device) {
 		debug("%s: /chosen/u-boot,spl-boot-device not set\n",
@@ -63,7 +65,10 @@ static int setup_boottargets(void)
 	}
 	debug("%s: booted from %s\n", __func__, boot_device);
 
-	env_default = env_get_default("boot_targets");
+	ret = env_get_default_into("boot_targets", env_default,
+				   sizeof(env_default));
+	if (ret < 0)
+		env_default[0] = '\0';
 	env = env_get("boot_targets");
 	if (!env) {
 		debug("%s: boot_targets does not exist\n", __func__);

-- 
2.41.0

[PATCH 6/6] rockchip: ringneck-px30: fix modified boot_targets detection

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

U-Boot proper automatically modifies boot_targets to swap the order in
which MMC storage media are used for standard boot based on which MMC
storage medium was used to load U-Boot proper. This is however only done
if the user has not changed it manually, therefore a check between the
default and current value is done.

This used to work fine until the migration to standard boot where
boot_targets value size in the default environment went above the 32
characters that env_get_default function can return, thus resulting in a
truncated variable.

Therefore the check between default and current value would always fail.

By using the newly added env_get_default_into function, a buffer of
the appropriate size can be allocated on the stack to get the whole
value of boot_targets in the default environment and thus fixing the
check.

Cc: Quentin Schulz <foss+uboot@0leil.net>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
 board/theobroma-systems/ringneck_px30/ringneck-px30.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/board/theobroma-systems/ringneck_px30/ringneck-px30.c b/board/theobroma-systems/ringneck_px30/ringneck-px30.c
index dd711cd05de..2bce8147eeb 100644
--- a/board/theobroma-systems/ringneck_px30/ringneck-px30.c
+++ b/board/theobroma-systems/ringneck_px30/ringneck-px30.c
@@ -39,7 +39,9 @@ static int setup_boottargets(void)
 {
 	const char *boot_device =
 		ofnode_read_chosen_string("u-boot,spl-boot-device");
-	char *env_default, *env;
+	char env_default[sizeof(BOOT_TARGETS)];
+	char *env;
+	int ret;
 
 	if (!boot_device) {
 		debug("%s: /chosen/u-boot,spl-boot-device not set\n",
@@ -48,7 +50,10 @@ static int setup_boottargets(void)
 	}
 	debug("%s: booted from %s\n", __func__, boot_device);
 
-	env_default = env_get_default("boot_targets");
+	ret = env_get_default_into("boot_targets", env_default,
+				   sizeof(env_default));
+	if (ret < 0)
+		env_default[0] = '\0';
 	env = env_get("boot_targets");
 	if (!env) {
 		debug("%s: boot_targets does not exist\n", __func__);

-- 
2.41.0

Re: [PATCH 1/6] env: allow to copy value from default environment into a buffer

[Kever Yang]

On 2023/11/8 22:20, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> env_get_default suffers from a particular issue int that it can only
> return a value truncated to gd->env_buf (32) characters. This may be
> enough for most variables but it isn't for others, so let's allow users
> to provide a preallocated buffer to copy the value into instead,
> allowing for more control, though it'll still be truncated if the value
> size is bigger than the preallocated buffer.
>
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
>   env/common.c  |  8 ++++++++
>   include/env.h | 10 ++++++++++
>   2 files changed, 18 insertions(+)
>
> diff --git a/env/common.c b/env/common.c
> index eb1a9137953..307003099dd 100644
> --- a/env/common.c
> +++ b/env/common.c
> @@ -254,6 +254,14 @@ char *env_get_default(const char *name)
>   	return NULL;
>   }
>   
> +/*
> + * Look up the variable from the default environment and store its value in buf
> + */
> +int env_get_default_into(const char *name, char *buf, unsigned int len)
> +{
> +	return env_get_from_linear(default_environment, name, buf, len);
> +}
> +
>   void env_set_default(const char *s, int flags)
>   {
>   	if (s) {
> diff --git a/include/env.h b/include/env.h
> index 430c4fa94a4..9a406de3781 100644
> --- a/include/env.h
> +++ b/include/env.h
> @@ -348,6 +348,16 @@ int env_import_redund(const char *buf1, int buf1_read_fail,
>    */
>   char *env_get_default(const char *name);
>   
> +/**
> + * env_get_default_into() - Look up a variable from the default environment and
> + * copy its value in buf.
> + *
> + * @name: Variable to look up
> + * Return: actual length of the variable value excluding the terminating
> + *	NULL-byte, or -1 if the variable is not found
> + */
> +int env_get_default_into(const char *name, char *buf, unsigned int len);
> +
>   /* [re]set to the default environment */
>   void env_set_default(const char *s, int flags);
>   
>

Re: [PATCH 2/6] env: migrate env_get_default to call env_get_default_into

[Kever Yang]

On 2023/11/8 22:20, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> Since both functions share a similar goal and env_get_default_into can
> do what env_get_default wants to do with specific arguments, let's make
> env_get_default call env_get_default_into so as to avoid code
> duplication.
>
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
>   env/common.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/env/common.c b/env/common.c
> index 307003099dd..930bdef2f58 100644
> --- a/env/common.c
> +++ b/env/common.c
> @@ -246,9 +246,11 @@ bool env_get_autostart(void)
>    */
>   char *env_get_default(const char *name)
>   {
> -	if (env_get_from_linear(default_environment, name,
> -				(char *)(gd->env_buf),
> -				sizeof(gd->env_buf)) >= 0)
> +	int ret;
> +
> +	ret = env_get_default_into(name, (char *)(gd->env_buf),
> +				   sizeof(gd->env_buf));
> +	if (ret >= 0)
>   		return (char *)(gd->env_buf);
>   
>   	return NULL;
>

Re: [PATCH 3/6] rockchip: puma-rk3399: only declare functions in U-Boot proper

[Kever Yang]

On 2023/11/8 22:20, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> Those functions are useless in anything but U-Boot proper for now, so
> let's compile them out explicitly.
>
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
>   board/theobroma-systems/puma_rk3399/puma-rk3399.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/board/theobroma-systems/puma_rk3399/puma-rk3399.c b/board/theobroma-systems/puma_rk3399/puma-rk3399.c
> index 614a60ee8f9..1b7a39b0474 100644
> --- a/board/theobroma-systems/puma_rk3399/puma-rk3399.c
> +++ b/board/theobroma-systems/puma_rk3399/puma-rk3399.c
> @@ -26,6 +26,7 @@
>   #include <power/regulator.h>
>   #include <u-boot/sha256.h>
>   
> +#ifndef CONFIG_SPL_BUILD
>   static void setup_iodomain(void)
>   {
>   	const u32 GRF_IO_VSEL_GPIO4CD_SHIFT = 3;
> @@ -192,3 +193,4 @@ int misc_init_r(void)
>   
>   	return 0;
>   }
> +#endif
>

Re: [PATCH 4/6] rockchip: ringneck-px30: only declare functions in U-Boot proper

[Kever Yang]

On 2023/11/8 22:20, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> Those functions are useless in anything but U-Boot proper for now, so
> let's compile them out explicitly.
>
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
>   board/theobroma-systems/ringneck_px30/ringneck-px30.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/board/theobroma-systems/ringneck_px30/ringneck-px30.c b/board/theobroma-systems/ringneck_px30/ringneck-px30.c
> index bb1bb4acf5c..dd711cd05de 100644
> --- a/board/theobroma-systems/ringneck_px30/ringneck-px30.c
> +++ b/board/theobroma-systems/ringneck_px30/ringneck-px30.c
> @@ -25,6 +25,7 @@
>   #include <power/regulator.h>
>   #include <u-boot/sha256.h>
>   
> +#ifndef CONFIG_SPL_BUILD
>   /*
>    * Swap mmc0 and mmc1 in boot_targets if booted from SD-Card.
>    *
> @@ -169,3 +170,4 @@ int misc_init_r(void)
>   
>   	return 0;
>   }
> +#endif
>

Re: [PATCH 5/6] rockchip: puma-rk3399: fix modified boot_targets detection

[Kever Yang]

On 2023/11/8 22:20, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> U-Boot proper automatically modifies boot_targets to swap the order in
> which MMC storage media are used for standard boot based on which MMC
> storage medium was used to load U-Boot proper. This is however only done
> if the user has not changed it manually, therefore a check between the
> default and current value is done.
>
> This used to work fine until the migration to standard boot where
> boot_targets value size in the default environment went above the 32
> characters that env_get_default function can return, thus resulting in a
> truncated variable.
>
> Therefore the check between default and current value would always fail.
>
> By using the newly added env_get_default_into function, a buffer of
> the appropriate size can be allocated on the stack to get the whole
> value of boot_targets in the default environment and thus fixing the
> check.
>
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
>   board/theobroma-systems/puma_rk3399/puma-rk3399.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/board/theobroma-systems/puma_rk3399/puma-rk3399.c b/board/theobroma-systems/puma_rk3399/puma-rk3399.c
> index 1b7a39b0474..df667fae5fc 100644
> --- a/board/theobroma-systems/puma_rk3399/puma-rk3399.c
> +++ b/board/theobroma-systems/puma_rk3399/puma-rk3399.c
> @@ -54,7 +54,9 @@ static int setup_boottargets(void)
>   {
>   	const char *boot_device =
>   		ofnode_read_chosen_string("u-boot,spl-boot-device");
> -	char *env_default, *env;
> +	char env_default[sizeof(BOOT_TARGETS)];
> +	char *env;
> +	int ret;
>   
>   	if (!boot_device) {
>   		debug("%s: /chosen/u-boot,spl-boot-device not set\n",
> @@ -63,7 +65,10 @@ static int setup_boottargets(void)
>   	}
>   	debug("%s: booted from %s\n", __func__, boot_device);
>   
> -	env_default = env_get_default("boot_targets");
> +	ret = env_get_default_into("boot_targets", env_default,
> +				   sizeof(env_default));
> +	if (ret < 0)
> +		env_default[0] = '\0';
>   	env = env_get("boot_targets");
>   	if (!env) {
>   		debug("%s: boot_targets does not exist\n", __func__);
>

Re: [PATCH 6/6] rockchip: ringneck-px30: fix modified boot_targets detection

[Kever Yang]

On 2023/11/8 22:20, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> U-Boot proper automatically modifies boot_targets to swap the order in
> which MMC storage media are used for standard boot based on which MMC
> storage medium was used to load U-Boot proper. This is however only done
> if the user has not changed it manually, therefore a check between the
> default and current value is done.
>
> This used to work fine until the migration to standard boot where
> boot_targets value size in the default environment went above the 32
> characters that env_get_default function can return, thus resulting in a
> truncated variable.
>
> Therefore the check between default and current value would always fail.
>
> By using the newly added env_get_default_into function, a buffer of
> the appropriate size can be allocated on the stack to get the whole
> value of boot_targets in the default environment and thus fixing the
> check.
>
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
>   board/theobroma-systems/ringneck_px30/ringneck-px30.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/board/theobroma-systems/ringneck_px30/ringneck-px30.c b/board/theobroma-systems/ringneck_px30/ringneck-px30.c
> index dd711cd05de..2bce8147eeb 100644
> --- a/board/theobroma-systems/ringneck_px30/ringneck-px30.c
> +++ b/board/theobroma-systems/ringneck_px30/ringneck-px30.c
> @@ -39,7 +39,9 @@ static int setup_boottargets(void)
>   {
>   	const char *boot_device =
>   		ofnode_read_chosen_string("u-boot,spl-boot-device");
> -	char *env_default, *env;
> +	char env_default[sizeof(BOOT_TARGETS)];
> +	char *env;
> +	int ret;
>   
>   	if (!boot_device) {
>   		debug("%s: /chosen/u-boot,spl-boot-device not set\n",
> @@ -48,7 +50,10 @@ static int setup_boottargets(void)
>   	}
>   	debug("%s: booted from %s\n", __func__, boot_device);
>   
> -	env_default = env_get_default("boot_targets");
> +	ret = env_get_default_into("boot_targets", env_default,
> +				   sizeof(env_default));
> +	if (ret < 0)
> +		env_default[0] = '\0';
>   	env = env_get("boot_targets");
>   	if (!env) {
>   		debug("%s: boot_targets does not exist\n", __func__);
>

Re: [PATCH 1/6] env: allow to copy value from default environment into a buffer

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

On Wed, Nov 08, 2023 at 03:20:30PM +0100, Quentin Schulz wrote:

> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> 
> env_get_default suffers from a particular issue int that it can only
> return a value truncated to gd->env_buf (32) characters. This may be
> enough for most variables but it isn't for others, so let's allow users
> to provide a preallocated buffer to copy the value into instead,
> allowing for more control, though it'll still be truncated if the value
> size is bigger than the preallocated buffer.
> 
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/6] env: migrate env_get_default to call env_get_default_into

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 543 bytes --]

On Wed, Nov 08, 2023 at 03:20:31PM +0100, Quentin Schulz wrote:

> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> 
> Since both functions share a similar goal and env_get_default_into can
> do what env_get_default wants to do with specific arguments, let's make
> env_get_default call env_get_default_into so as to avoid code
> duplication.
> 
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 3/6] rockchip: puma-rk3399: only declare functions in U-Boot proper

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 589 bytes --]

On Wed, Nov 08, 2023 at 03:20:32PM +0100, Quentin Schulz wrote:

> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> 
> Those functions are useless in anything but U-Boot proper for now, so
> let's compile them out explicitly.
> 
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>  board/theobroma-systems/puma_rk3399/puma-rk3399.c | 2 ++
>  1 file changed, 2 insertions(+)

Please do this at the Makefile level and just not build the file rather
than #if out the entire thing.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 4/6] rockchip: ringneck-px30: only declare functions in U-Boot proper

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 520 bytes --]

On Wed, Nov 08, 2023 at 03:20:33PM +0100, Quentin Schulz wrote:

> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> 
> Those functions are useless in anything but U-Boot proper for now, so
> let's compile them out explicitly.
> 
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>  board/theobroma-systems/ringneck_px30/ringneck-px30.c | 2 ++
>  1 file changed, 2 insertions(+)

Same request as the puma case.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 6/6] rockchip: ringneck-px30: fix modified boot_targets detection

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1425 bytes --]

On Wed, Nov 08, 2023 at 03:20:35PM +0100, Quentin Schulz wrote:

> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> 
> U-Boot proper automatically modifies boot_targets to swap the order in
> which MMC storage media are used for standard boot based on which MMC
> storage medium was used to load U-Boot proper. This is however only done
> if the user has not changed it manually, therefore a check between the
> default and current value is done.
> 
> This used to work fine until the migration to standard boot where
> boot_targets value size in the default environment went above the 32
> characters that env_get_default function can return, thus resulting in a
> truncated variable.
> 
> Therefore the check between default and current value would always fail.
> 
> By using the newly added env_get_default_into function, a buffer of
> the appropriate size can be allocated on the stack to get the whole
> value of boot_targets in the default environment and thus fixing the
> check.
> 
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>  board/theobroma-systems/ringneck_px30/ringneck-px30.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

This looks like what the puma-rk3399 is doing in 5/6. Can we have this
in a more common location? Or would there be a downside to that?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 6/6] rockchip: ringneck-px30: fix modified boot_targets detection

[Quentin Schulz]

Hi Tom,

On 11/9/23 22:28, Tom Rini wrote:
> On Wed, Nov 08, 2023 at 03:20:35PM +0100, Quentin Schulz wrote:
> 
>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>
>> U-Boot proper automatically modifies boot_targets to swap the order in
>> which MMC storage media are used for standard boot based on which MMC
>> storage medium was used to load U-Boot proper. This is however only done
>> if the user has not changed it manually, therefore a check between the
>> default and current value is done.
>>
>> This used to work fine until the migration to standard boot where
>> boot_targets value size in the default environment went above the 32
>> characters that env_get_default function can return, thus resulting in a
>> truncated variable.
>>
>> Therefore the check between default and current value would always fail.
>>
>> By using the newly added env_get_default_into function, a buffer of
>> the appropriate size can be allocated on the stack to get the whole
>> value of boot_targets in the default environment and thus fixing the
>> check.
>>
>> Cc: Quentin Schulz <foss+uboot@0leil.net>
>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>> ---
>>   board/theobroma-systems/ringneck_px30/ringneck-px30.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> This looks like what the puma-rk3399 is doing in 5/6. Can we have this
> in a more common location? Or would there be a downside to that?
> 

I can think of a few things, yes. Now wondering how far I want to push it :)

I could simply pass the DT paths to a function for eMMC/SD controller.

Or I could derive those from spl_boot_devices array in spl-boot-order.c 
and do everything automagically somehow.

Will try a v2 next week.

Cheers,
Quentin