[PATCH v2 00/12] Board specific runtime determined default env

[PATCH v2 00/12] Board specific runtime determined default env

[Marek Behún]

From: Marek Behún <marek.behun@nic.cz>

Hello Simon, Pali,

this is v2 of the series that adds support for board specific runtime
determined default environment variables.

(Went through CI on github.)

As requested, I converted it to use sysinfo (and added support for some
new things into sysinfo).

I haven't tested the change on ESPRESSObin yet, because I don't have the
board at home. But I copy-pasted the code to turris_mox and it worked
as expected there. Nonetheless it should be tested, I or Pali can do it
today or tomorrow.

Marek

Marek Behún (12):
  env: Don't set ready flag if import failed in env_set_default()
  env: Fix env_get() when returning empty string using env_get_f()
  env: Simplify env_get_default()
  sysinfo: Make sysinfo_get_str() behave like snprintf()
  test: Use ut_asserteq_str() instead of ut_assertok(strcmp())
  sysinfo: Add get_str_list() method
  sysinfo: Make .detect() non-mandatory
  sysinfo: Add support for iterating string list
  env: Change return behaviour of env_set_default_vars()
  env: Add support for overwriting default environment via sysinfo
  arm: mvebu: Espressobin: Use new API for setting default env at
    runtime
  env: Remove support for read-write default_environment[]

 board/Marvell/mvebu_armada-37xx/board.c     | 135 ++++++++++------
 board/google/chromebook_coral/coral.c       |  13 +-
 common/board_info.c                         |   2 +-
 configs/mvebu_espressobin-88f3720_defconfig |   1 +
 drivers/sysinfo/gpio.c                      |   2 +-
 drivers/sysinfo/rcar3.c                     |   2 +-
 drivers/sysinfo/sandbox.c                   |  20 ++-
 drivers/sysinfo/sysinfo-uclass.c            |  99 +++++++++++-
 env/common.c                                | 165 +++++++++++++++++---
 include/configs/mvebu_armada-37xx.h         |  17 +-
 include/env.h                               |   1 +
 include/env_default.h                       |   2 -
 include/env_internal.h                      |   4 -
 include/sysinfo.h                           | 163 ++++++++++++++++++-
 lib/smbios.c                                |   2 +-
 test/dm/cpu.c                               |   4 +-
 test/dm/soc.c                               |   4 +-
 test/dm/sysinfo-gpio.c                      |  12 +-
 test/dm/sysinfo.c                           |  66 ++++++--
 test/dm/usb.c                               |   2 +-
 test/dm/virtio.c                            |   2 +-
 test/print_ut.c                             |  32 ++--
 22 files changed, 595 insertions(+), 155 deletions(-)

-- 
2.32.0

[PATCH v2 01/12] env: Don't set ready flag if import failed in env_set_default()

[Marek Behún]

From: Marek Behún <marek.behun@nic.cz>

Do not set GD_FLG_ENV_READY nor GD_FLG_ENV_DEFAULT if failed importing
in env_set_default().

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 env/common.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/env/common.c b/env/common.c
index 99729ca002..2aa23545ba 100644
--- a/env/common.c
+++ b/env/common.c
@@ -261,9 +261,11 @@ void env_set_default(const char *s, int flags)
 	flags |= H_DEFAULT;
 	if (himport_r(&env_htab, default_environment,
 			sizeof(default_environment), '\0', flags, 0,
-			0, NULL) == 0)
+			0, NULL) == 0) {
 		pr_err("## Error: Environment import failed: errno = %d\n",
 		       errno);
+		return;
+	}
 
 	gd->flags |= GD_FLG_ENV_READY;
 	gd->flags |= GD_FLG_ENV_DEFAULT;
-- 
2.32.0

[PATCH v2 02/12] env: Fix env_get() when returning empty string using env_get_f()

[Marek Behún]

From: Marek Behún <marek.behun@nic.cz>

The env_get_f() function returns -1 on failure. Returning 0 means that
the variable exists, and is empty string.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 env/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/env/common.c b/env/common.c
index 2aa23545ba..757c5f9ecd 100644
--- a/env/common.c
+++ b/env/common.c
@@ -125,7 +125,7 @@ char *env_get(const char *name)
 	}
 
 	/* restricted capabilities before import */
-	if (env_get_f(name, (char *)(gd->env_buf), sizeof(gd->env_buf)) > 0)
+	if (env_get_f(name, (char *)(gd->env_buf), sizeof(gd->env_buf)) >= 0)
 		return (char *)(gd->env_buf);
 
 	return NULL;
-- 
2.32.0

[PATCH v2 03/12] env: Simplify env_get_default()

[Marek Behún]

From: Marek Behún <marek.behun@nic.cz>

Instead of pretending that we don't have environment to force searching
default environment in env_get_default(), get the data from the
default_environment[] buffer directly.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 env/common.c | 45 ++++++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/env/common.c b/env/common.c
index 757c5f9ecd..208e2adaa0 100644
--- a/env/common.c
+++ b/env/common.c
@@ -148,12 +148,10 @@ char *from_env(const char *envvar)
 	return ret;
 }
 
-/*
- * Look up variable from environment for restricted C runtime env.
- */
-int env_get_f(const char *name, char *buf, unsigned len)
+static int env_get_from_linear(const char *env, const char *name, char *buf,
+			       unsigned len)
 {
-	const char *env, *p, *end;
+	const char *p, *end;
 	size_t name_len;
 
 	if (name == NULL || *name == '\0')
@@ -161,11 +159,6 @@ int env_get_f(const char *name, char *buf, unsigned len)
 
 	name_len = strlen(name);
 
-	if (gd->env_valid == ENV_INVALID)
-		env = default_environment;
-	else
-		env = (const char *)gd->env_addr;
-
 	for (p = env; *p != '\0'; p = end + 1) {
 		const char *value;
 		unsigned res;
@@ -193,6 +186,21 @@ int env_get_f(const char *name, char *buf, unsigned len)
 	return -1;
 }
 
+/*
+ * Look up variable from environment for restricted C runtime env.
+ */
+int env_get_f(const char *name, char *buf, unsigned len)
+{
+	const char *env;
+
+	if (gd->env_valid == ENV_INVALID)
+		env = default_environment;
+	else
+		env = (const char *)gd->env_addr;
+
+	return env_get_from_linear(env, name, buf, len);
+}
+
 /**
  * Decode the integer value of an environment variable and return it.
  *
@@ -232,17 +240,12 @@ int env_get_yesno(const char *var)
  */
 char *env_get_default(const char *name)
 {
-	char *ret_val;
-	unsigned long really_valid = gd->env_valid;
-	unsigned long real_gd_flags = gd->flags;
-
-	/* Pretend that the image is bad. */
-	gd->flags &= ~GD_FLG_ENV_READY;
-	gd->env_valid = ENV_INVALID;
-	ret_val = env_get(name);
-	gd->env_valid = really_valid;
-	gd->flags = real_gd_flags;
-	return ret_val;
+	if (env_get_from_linear(default_environment, name,
+				(char *)(gd->env_buf),
+				sizeof(gd->env_buf)) >= 0)
+		return (char *)(gd->env_buf);
+
+	return NULL;
 }
 
 void env_set_default(const char *s, int flags)
-- 
2.32.0

[PATCH v2 04/12] sysinfo: Make sysinfo_get_str() behave like snprintf()

[Marek Behún]

From: Marek Behún <marek.behun@nic.cz>

Currently sysinfo_get_str() returns 0 if a string is filled in the
given buffer, and otherwise gives no simple mechanism to determine
actual string length.

One implementation returns -ENOSPC if buffer is not large enough.

Change the behaviour of the function to that of snprintf(): i.e. the
buffer is always filled in as much as possible if the string exists, and
the function returns the actual length of the string (excluding the
terminating NULL-byte).

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 board/google/chromebook_coral/coral.c | 13 ++++---------
 common/board_info.c                   |  2 +-
 drivers/sysinfo/gpio.c                |  2 +-
 drivers/sysinfo/rcar3.c               |  2 +-
 drivers/sysinfo/sandbox.c             |  5 +++--
 include/sysinfo.h                     | 16 ++++++++++++----
 lib/smbios.c                          |  2 +-
 test/dm/sysinfo-gpio.c                | 12 ++++++------
 test/dm/sysinfo.c                     | 12 ++++++------
 9 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/board/google/chromebook_coral/coral.c b/board/google/chromebook_coral/coral.c
index 53c5171d02..124fc49998 100644
--- a/board/google/chromebook_coral/coral.c
+++ b/board/google/chromebook_coral/coral.c
@@ -155,10 +155,8 @@ static int coral_get_str(struct udevice *dev, int id, size_t size, char *val)
 
 		if (ret < 0)
 			return ret;
-		if (size < 15)
-			return -ENOSPC;
-		sprintf(val, "rev%d", ret);
-		break;
+
+		return snprintf(val, size, "rev%d", ret);
 	}
 	case SYSINFO_ID_BOARD_MODEL: {
 		int mem_config, sku_config;
@@ -173,15 +171,12 @@ static int coral_get_str(struct udevice *dev, int id, size_t size, char *val)
 			log_warning("Unable to read skuconfig (err=%d)\n", ret);
 		sku_config = ret;
 		model = fdt_getprop(gd->fdt_blob, 0, "model", NULL);
-		snprintf(val, size, "%s (memconfig %d, SKU %d)", model,
-			 mem_config, sku_config);
-		break;
+		return snprintf(val, size, "%s (memconfig %d, SKU %d)", model,
+				mem_config, sku_config);
 	}
 	default:
 		return -ENOENT;
 	}
-
-	return 0;
 }
 
 int chromeos_get_gpio(const struct udevice *dev, const char *prop,
diff --git a/common/board_info.c b/common/board_info.c
index e0f2d93922..fd7bd1deea 100644
--- a/common/board_info.c
+++ b/common/board_info.c
@@ -43,7 +43,7 @@ int __weak show_board_info(void)
 		}
 
 		/* Fail back to the main 'model' if available */
-		if (ret)
+		if (ret <= 0)
 			model = fdt_getprop(gd->fdt_blob, 0, "model", NULL);
 		else
 			model = str;
diff --git a/drivers/sysinfo/gpio.c b/drivers/sysinfo/gpio.c
index 1d7f050998..8a9238b589 100644
--- a/drivers/sysinfo/gpio.c
+++ b/drivers/sysinfo/gpio.c
@@ -79,7 +79,7 @@ static int sysinfo_gpio_get_str(struct udevice *dev, int id, size_t size, char *
 
 		strncpy(val, name, size);
 		val[size - 1] = '\0';
-		return 0;
+		return strlen(name);
 	} default:
 		return -EINVAL;
 	};
diff --git a/drivers/sysinfo/rcar3.c b/drivers/sysinfo/rcar3.c
index c2f4ddfbbe..3bfd9bc39d 100644
--- a/drivers/sysinfo/rcar3.c
+++ b/drivers/sysinfo/rcar3.c
@@ -48,7 +48,7 @@ static int sysinfo_rcar_get_str(struct udevice *dev, int id, size_t size, char *
 	case SYSINFO_ID_BOARD_MODEL:
 		strncpy(val, priv->boardmodel, size);
 		val[size - 1] = '\0';
-		return 0;
+		return strlen(priv->boardmodel);
 	default:
 		return -EINVAL;
 	};
diff --git a/drivers/sysinfo/sandbox.c b/drivers/sysinfo/sandbox.c
index d270a26aa4..01c845e310 100644
--- a/drivers/sysinfo/sandbox.c
+++ b/drivers/sysinfo/sandbox.c
@@ -73,8 +73,9 @@ int sysinfo_sandbox_get_str(struct udevice *dev, int id, size_t size, char *val)
 	switch (id) {
 	case STR_VACATIONSPOT:
 		/* Picks a vacation spot depending on i1 and i2 */
-		snprintf(val, size, vacation_spots[index]);
-		return 0;
+		strncpy(val, vacation_spots[index], size);
+		val[size - 1] = '\0';
+		return strlen(vacation_spots[index]);
 	}
 
 	return -ENOENT;
diff --git a/include/sysinfo.h b/include/sysinfo.h
index b140d742e9..6031aec578 100644
--- a/include/sysinfo.h
+++ b/include/sysinfo.h
@@ -95,10 +95,15 @@ struct sysinfo_ops {
 	 * @dev:	The sysinfo instance to gather the data.
 	 * @id:		A unique identifier for the string value to be read.
 	 * @size:	The size of the buffer to receive the string data.
+	 *		If the buffer is not large enough to contain the whole
+	 *		string, the string must be trimmed to fit in the
+	 *		buffer including the terminating NULL-byte.
 	 * @val:	Pointer to a buffer that receives the value read.
 	 *
-	 * Return: 0 if OK, -ve on error.
+	 * Return: Actual length of the string excluding the terminating
+	 *	   NULL-byte if OK, -ve on error.
 	 */
+
 	int (*get_str)(struct udevice *dev, int id, size_t size, char *val);
 
 	/**
@@ -164,11 +169,14 @@ int sysinfo_get_int(struct udevice *dev, int id, int *val);
  *		     hardware setup.
  * @dev:	The sysinfo instance to gather the data.
  * @id:		A unique identifier for the string value to be read.
- * @size:	The size of the buffer to receive the string data.
+ * @size:	The size of the buffer to receive the string data. If the buffer
+ *		is not large enough to contain the whole string, the string will
+ *		be trimmed to fit in the buffer including the terminating
+ *		NULL-byte.
  * @val:	Pointer to a buffer that receives the value read.
  *
- * Return: 0 if OK, -EPERM if called before sysinfo_detect(), else -ve on
- * error.
+ * Return: Actual length of the string excluding the terminating NULL-byte if
+ *	   OK, -EPERM if called before sysinfo_detect(), else -ve on error.
  */
 int sysinfo_get_str(struct udevice *dev, int id, size_t size, char *val);
 
diff --git a/lib/smbios.c b/lib/smbios.c
index d7f4999e8b..0e55a0e49f 100644
--- a/lib/smbios.c
+++ b/lib/smbios.c
@@ -144,7 +144,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
 		int ret;
 
 		ret = sysinfo_get_str(ctx->dev, sysinfo_id, sizeof(val), val);
-		if (!ret)
+		if (ret >= 0)
 			return smbios_add_string(ctx, val);
 	}
 	if (IS_ENABLED(CONFIG_OF_CONTROL)) {
diff --git a/test/dm/sysinfo-gpio.c b/test/dm/sysinfo-gpio.c
index 2e494b3f34..2136b2000d 100644
--- a/test/dm/sysinfo-gpio.c
+++ b/test/dm/sysinfo-gpio.c
@@ -32,8 +32,8 @@ static int dm_test_sysinfo_gpio(struct unit_test_state *uts)
 	ut_assertok(sysinfo_detect(sysinfo));
 	ut_assertok(sysinfo_get_int(sysinfo, SYSINFO_ID_BOARD_MODEL, &val));
 	ut_asserteq(19, val);
-	ut_assertok(sysinfo_get_str(sysinfo, SYSINFO_ID_BOARD_MODEL, sizeof(buf),
-				    buf));
+	ut_asserteq(5, sysinfo_get_str(sysinfo, SYSINFO_ID_BOARD_MODEL, sizeof(buf),
+				       buf));
 	ut_asserteq_str("rev_a", buf);
 
 	/*
@@ -46,8 +46,8 @@ static int dm_test_sysinfo_gpio(struct unit_test_state *uts)
 	ut_assertok(sysinfo_detect(sysinfo));
 	ut_assertok(sysinfo_get_int(sysinfo, SYSINFO_ID_BOARD_MODEL, &val));
 	ut_asserteq(5, val);
-	ut_assertok(sysinfo_get_str(sysinfo, SYSINFO_ID_BOARD_MODEL, sizeof(buf),
-				    buf));
+	ut_asserteq(3, sysinfo_get_str(sysinfo, SYSINFO_ID_BOARD_MODEL, sizeof(buf),
+				       buf));
 	ut_asserteq_str("foo", buf);
 
 	/*
@@ -60,8 +60,8 @@ static int dm_test_sysinfo_gpio(struct unit_test_state *uts)
 	ut_assertok(sysinfo_detect(sysinfo));
 	ut_assertok(sysinfo_get_int(sysinfo, SYSINFO_ID_BOARD_MODEL, &val));
 	ut_asserteq(15, val);
-	ut_assertok(sysinfo_get_str(sysinfo, SYSINFO_ID_BOARD_MODEL, sizeof(buf),
-				    buf));
+	ut_asserteq(7, sysinfo_get_str(sysinfo, SYSINFO_ID_BOARD_MODEL, sizeof(buf),
+				       buf));
 	ut_asserteq_str("unknown", buf);
 
 	return 0;
diff --git a/test/dm/sysinfo.c b/test/dm/sysinfo.c
index 96b3a8ebab..488412ab14 100644
--- a/test/dm/sysinfo.c
+++ b/test/dm/sysinfo.c
@@ -34,8 +34,8 @@ static int dm_test_sysinfo(struct unit_test_state *uts)
 				     &called_detect));
 	ut_assert(called_detect);
 
-	ut_assertok(sysinfo_get_str(sysinfo, STR_VACATIONSPOT, sizeof(str),
-				    str));
+	ut_asserteq(6, sysinfo_get_str(sysinfo, STR_VACATIONSPOT, sizeof(str),
+				       str));
 	ut_assertok(strcmp(str, "R'lyeh"));
 
 	ut_assertok(sysinfo_get_int(sysinfo, INT_TEST1, &i));
@@ -44,8 +44,8 @@ static int dm_test_sysinfo(struct unit_test_state *uts)
 	ut_assertok(sysinfo_get_int(sysinfo, INT_TEST2, &i));
 	ut_asserteq(100, i);
 
-	ut_assertok(sysinfo_get_str(sysinfo, STR_VACATIONSPOT, sizeof(str),
-				    str));
+	ut_asserteq(7, sysinfo_get_str(sysinfo, STR_VACATIONSPOT, sizeof(str),
+				       str));
 	ut_assertok(strcmp(str, "Carcosa"));
 
 	ut_assertok(sysinfo_get_int(sysinfo, INT_TEST1, &i));
@@ -54,8 +54,8 @@ static int dm_test_sysinfo(struct unit_test_state *uts)
 	ut_assertok(sysinfo_get_int(sysinfo, INT_TEST2, &i));
 	ut_asserteq(99, i);
 
-	ut_assertok(sysinfo_get_str(sysinfo, STR_VACATIONSPOT, sizeof(str),
-				    str));
+	ut_asserteq(7, sysinfo_get_str(sysinfo, STR_VACATIONSPOT, sizeof(str),
+				       str));
 	ut_assertok(strcmp(str, "Yuggoth"));
 
 	return 0;
-- 
2.32.0

Re: [PATCH v2 04/12] sysinfo: Make sysinfo_get_str() behave like snprintf()

[Simon Glass]

Hi Marek,

On Wed, 3 Nov 2021 at 17:23, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> Currently sysinfo_get_str() returns 0 if a string is filled in the
> given buffer, and otherwise gives no simple mechanism to determine
> actual string length.
>
> One implementation returns -ENOSPC if buffer is not large enough.
>
> Change the behaviour of the function to that of snprintf(): i.e. the
> buffer is always filled in as much as possible if the string exists, and
> the function returns the actual length of the string (excluding the
> terminating NULL-byte).
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  board/google/chromebook_coral/coral.c | 13 ++++---------
>  common/board_info.c                   |  2 +-
>  drivers/sysinfo/gpio.c                |  2 +-
>  drivers/sysinfo/rcar3.c               |  2 +-
>  drivers/sysinfo/sandbox.c             |  5 +++--
>  include/sysinfo.h                     | 16 ++++++++++++----
>  lib/smbios.c                          |  2 +-
>  test/dm/sysinfo-gpio.c                | 12 ++++++------
>  test/dm/sysinfo.c                     | 12 ++++++------
>  9 files changed, 35 insertions(+), 31 deletions(-)

So how do we know if the size is too small? The string is silently truncated?

I think -ENOSPC is better?

[..]

Regards,
Simon

Re: [PATCH v2 04/12] sysinfo: Make sysinfo_get_str() behave like snprintf()

[Marek Behún]

On Thu, 4 Nov 2021 20:02:25 -0600
Simon Glass <sjg@chromium.org> wrote:

> Hi Marek,
> 
> On Wed, 3 Nov 2021 at 17:23, Marek Behún <kabel@kernel.org> wrote:
> >
> > From: Marek Behún <marek.behun@nic.cz>
> >
> > Currently sysinfo_get_str() returns 0 if a string is filled in the
> > given buffer, and otherwise gives no simple mechanism to determine
> > actual string length.
> >
> > One implementation returns -ENOSPC if buffer is not large enough.
> >
> > Change the behaviour of the function to that of snprintf(): i.e. the
> > buffer is always filled in as much as possible if the string exists, and
> > the function returns the actual length of the string (excluding the
> > terminating NULL-byte).
> >
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > ---
> >  board/google/chromebook_coral/coral.c | 13 ++++---------
> >  common/board_info.c                   |  2 +-
> >  drivers/sysinfo/gpio.c                |  2 +-
> >  drivers/sysinfo/rcar3.c               |  2 +-
> >  drivers/sysinfo/sandbox.c             |  5 +++--
> >  include/sysinfo.h                     | 16 ++++++++++++----
> >  lib/smbios.c                          |  2 +-
> >  test/dm/sysinfo-gpio.c                | 12 ++++++------
> >  test/dm/sysinfo.c                     | 12 ++++++------
> >  9 files changed, 35 insertions(+), 31 deletions(-)  
> 
> So how do we know if the size is too small? The string is silently truncated?

The same way as in snprintf.
If the return value is >= size, then size is too small.
(The return value is the length of the whole string (excluding \0 at
end), not just the part that was copied to buffer.)

Marek

Re: [PATCH v2 04/12] sysinfo: Make sysinfo_get_str() behave like snprintf()

[Simon Glass]

Hi Marek,

On Fri, 5 Nov 2021 at 05:19, Marek Behún <kabel@kernel.org> wrote:
>
> On Thu, 4 Nov 2021 20:02:25 -0600
> Simon Glass <sjg@chromium.org> wrote:
>
> > Hi Marek,
> >
> > On Wed, 3 Nov 2021 at 17:23, Marek Behún <kabel@kernel.org> wrote:
> > >
> > > From: Marek Behún <marek.behun@nic.cz>
> > >
> > > Currently sysinfo_get_str() returns 0 if a string is filled in the
> > > given buffer, and otherwise gives no simple mechanism to determine
> > > actual string length.
> > >
> > > One implementation returns -ENOSPC if buffer is not large enough.
> > >
> > > Change the behaviour of the function to that of snprintf(): i.e. the
> > > buffer is always filled in as much as possible if the string exists, and
> > > the function returns the actual length of the string (excluding the
> > > terminating NULL-byte).
> > >
> > > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > > ---
> > >  board/google/chromebook_coral/coral.c | 13 ++++---------
> > >  common/board_info.c                   |  2 +-
> > >  drivers/sysinfo/gpio.c                |  2 +-
> > >  drivers/sysinfo/rcar3.c               |  2 +-
> > >  drivers/sysinfo/sandbox.c             |  5 +++--
> > >  include/sysinfo.h                     | 16 ++++++++++++----
> > >  lib/smbios.c                          |  2 +-
> > >  test/dm/sysinfo-gpio.c                | 12 ++++++------
> > >  test/dm/sysinfo.c                     | 12 ++++++------
> > >  9 files changed, 35 insertions(+), 31 deletions(-)
> >
> > So how do we know if the size is too small? The string is silently truncated?
>
> The same way as in snprintf.
> If the return value is >= size, then size is too small.
> (The return value is the length of the whole string (excluding \0 at
> end), not just the part that was copied to buffer.)

OK, as on the other patch for where I missed this. It is in the commit
message which I did not read carefully enough, but we need it in the
header file / sphinx docs too.

Regards,
Simon

>
> Marek

[PATCH v2 05/12] test: Use ut_asserteq_str() instead of ut_assertok(strcmp())

[Marek Behún]

From: Marek Behún <marek.behun@nic.cz>

Use the ut_asserteq_str() assertion instead of strcmp() function and
ut_assertok()ing it's result.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 test/dm/cpu.c     |  4 ++--
 test/dm/soc.c     |  4 ++--
 test/dm/sysinfo.c |  6 +++---
 test/dm/usb.c     |  2 +-
 test/dm/virtio.c  |  2 +-
 test/print_ut.c   | 32 ++++++++++++++++----------------
 6 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/test/dm/cpu.c b/test/dm/cpu.c
index d7e596ee39..0cda364131 100644
--- a/test/dm/cpu.c
+++ b/test/dm/cpu.c
@@ -32,7 +32,7 @@ static int dm_test_cpu(struct unit_test_state *uts)
 	ut_asserteq(cpu_is_current(dev), 1);
 
 	ut_assertok(cpu_get_desc(dev, text, sizeof(text)));
-	ut_assertok(strcmp(text, "LEG Inc. SuperMegaUltraTurbo CPU No. 1"));
+	ut_asserteq_str(text, "LEG Inc. SuperMegaUltraTurbo CPU No. 1");
 
 	ut_assertok(cpu_get_info(dev, &info));
 	ut_asserteq(info.cpu_freq, 42 * 42 * 42 * 42 * 42);
@@ -42,7 +42,7 @@ static int dm_test_cpu(struct unit_test_state *uts)
 	ut_asserteq(cpu_get_count(dev), 42);
 
 	ut_assertok(cpu_get_vendor(dev, text, sizeof(text)));
-	ut_assertok(strcmp(text, "Languid Example Garbage Inc."));
+	ut_asserteq_str(text, "Languid Example Garbage Inc.");
 
 	return 0;
 }
diff --git a/test/dm/soc.c b/test/dm/soc.c
index 17e1b5ba01..6e14c98391 100644
--- a/test/dm/soc.c
+++ b/test/dm/soc.c
@@ -91,10 +91,10 @@ static int dm_test_soc(struct unit_test_state *uts)
 	ut_assertok(soc_get(&dev));
 
 	ut_assertok(soc_get_machine(dev, text, sizeof(text)));
-	ut_assertok(strcmp(text, "SANDBOX123"));
+	ut_asserteq_str(text, "SANDBOX123");
 
 	ut_assertok(soc_get_family(dev, text, sizeof(text)));
-	ut_assertok(strcmp(text, "SANDBOX1xx"));
+	ut_asserteq_str(text, "SANDBOX1xx");
 
 	ut_assertok(soc_get_revision(dev, text, sizeof(text)));
 	ut_asserteq_str(text, "1.0");
diff --git a/test/dm/sysinfo.c b/test/dm/sysinfo.c
index 488412ab14..2c1bd1ce40 100644
--- a/test/dm/sysinfo.c
+++ b/test/dm/sysinfo.c
@@ -36,7 +36,7 @@ static int dm_test_sysinfo(struct unit_test_state *uts)
 
 	ut_asserteq(6, sysinfo_get_str(sysinfo, STR_VACATIONSPOT, sizeof(str),
 				       str));
-	ut_assertok(strcmp(str, "R'lyeh"));
+	ut_asserteq_str(str, "R'lyeh");
 
 	ut_assertok(sysinfo_get_int(sysinfo, INT_TEST1, &i));
 	ut_asserteq(0, i);
@@ -46,7 +46,7 @@ static int dm_test_sysinfo(struct unit_test_state *uts)
 
 	ut_asserteq(7, sysinfo_get_str(sysinfo, STR_VACATIONSPOT, sizeof(str),
 				       str));
-	ut_assertok(strcmp(str, "Carcosa"));
+	ut_asserteq_str(str, "Carcosa");
 
 	ut_assertok(sysinfo_get_int(sysinfo, INT_TEST1, &i));
 	ut_asserteq(1, i);
@@ -56,7 +56,7 @@ static int dm_test_sysinfo(struct unit_test_state *uts)
 
 	ut_asserteq(7, sysinfo_get_str(sysinfo, STR_VACATIONSPOT, sizeof(str),
 				       str));
-	ut_assertok(strcmp(str, "Yuggoth"));
+	ut_asserteq_str(str, "Yuggoth");
 
 	return 0;
 }
diff --git a/test/dm/usb.c b/test/dm/usb.c
index 5d6ceefce0..055134a485 100644
--- a/test/dm/usb.c
+++ b/test/dm/usb.c
@@ -56,7 +56,7 @@ static int dm_test_usb_flash(struct unit_test_state *uts)
 	ut_asserteq(512, dev_desc->blksz);
 	memset(cmp, '\0', sizeof(cmp));
 	ut_asserteq(2, blk_dread(dev_desc, 0, 2, cmp));
-	ut_assertok(strcmp(cmp, "this is a test"));
+	ut_asserteq_str(cmp, "this is a test");
 	ut_assertok(usb_stop());
 
 	return 0;
diff --git a/test/dm/virtio.c b/test/dm/virtio.c
index 9a7e658cce..7154449f60 100644
--- a/test/dm/virtio.c
+++ b/test/dm/virtio.c
@@ -28,7 +28,7 @@ static int dm_test_virtio_base(struct unit_test_state *uts)
 	/* check the child virtio-blk device is bound */
 	ut_assertok(device_find_first_child(bus, &dev));
 	ut_assertnonnull(dev);
-	ut_assertok(strcmp(dev->name, "virtio-blk#0"));
+	ut_asserteq_str(dev->name, "virtio-blk#0");
 
 	/* check driver status */
 	ut_assertok(virtio_get_status(dev, &status));
diff --git a/test/print_ut.c b/test/print_ut.c
index 11d8580e55..70670a3a14 100644
--- a/test/print_ut.c
+++ b/test/print_ut.c
@@ -32,13 +32,13 @@ static int print_guid(struct unit_test_state *uts)
 	char str[40];
 
 	sprintf(str, "%pUb", guid);
-	ut_assertok(strcmp("01020304-0506-0708-090a-0b0c0d0e0f10", str));
+	ut_asserteq_str("01020304-0506-0708-090a-0b0c0d0e0f10", str);
 	sprintf(str, "%pUB", guid);
-	ut_assertok(strcmp("01020304-0506-0708-090A-0B0C0D0E0F10", str));
+	ut_asserteq_str("01020304-0506-0708-090A-0B0C0D0E0F10", str);
 	sprintf(str, "%pUl", guid);
-	ut_assertok(strcmp("04030201-0605-0807-090a-0b0c0d0e0f10", str));
+	ut_asserteq_str("04030201-0605-0807-090a-0b0c0d0e0f10", str);
 	sprintf(str, "%pUL", guid);
-	ut_assertok(strcmp("04030201-0605-0807-090A-0B0C0D0E0F10", str));
+	ut_asserteq_str("04030201-0605-0807-090A-0B0C0D0E0F10", str);
 
 	return 0;
 }
@@ -70,11 +70,11 @@ static int print_efi_ut(struct unit_test_state *uts)
 	dp_end->length = sizeof(struct efi_device_path);
 
 	snprintf(str, sizeof(str), "_%pD_", buf);
-	ut_assertok(strcmp("_/SD(3)_", str));
+	ut_asserteq_str("_/SD(3)_", str);
 
 	/* NULL device path */
 	snprintf(str, sizeof(str), "_%pD_", NULL);
-	ut_assertok(strcmp("_<NULL>_", str));
+	ut_asserteq_str("_<NULL>_", str);
 
 	return 0;
 }
@@ -89,41 +89,41 @@ static int print_printf(struct unit_test_state *uts)
 	int len;
 
 	snprintf(str, sizeof(str), "testing");
-	ut_assertok(strcmp("testing", str));
+	ut_asserteq_str("testing", str);
 
 	snprintf(str, sizeof(str), "testing but too long");
-	ut_assertok(strcmp("testing b", str));
+	ut_asserteq_str("testing b", str);
 
 	snprintf(str, 1, "testing none");
-	ut_assertok(strcmp("", str));
+	ut_asserteq_str("", str);
 
 	*str = 'x';
 	snprintf(str, 0, "testing none");
 	ut_asserteq('x', *str);
 
 	sprintf(big_str, "_%ls_", L"foo");
-	ut_assertok(strcmp("_foo_", big_str));
+	ut_asserteq_str("_foo_", big_str);
 
 	/* Test the banner function */
 	s = display_options_get_banner(true, str, sizeof(str));
 	ut_asserteq_ptr(str, s);
-	ut_assertok(strcmp("\n\nU-Boo\n\n", s));
+	ut_asserteq_str("\n\nU-Boo\n\n", s);
 
 	/* Assert that we do not overwrite memory before the buffer */
 	str[0] = '`';
 	s = display_options_get_banner(true, str + 1, 1);
 	ut_asserteq_ptr(str + 1, s);
-	ut_assertok(strcmp("`", str));
+	ut_asserteq_str("`", str);
 
 	str[0] = '~';
 	s = display_options_get_banner(true, str + 1, 2);
 	ut_asserteq_ptr(str + 1, s);
-	ut_assertok(strcmp("~\n", str));
+	ut_asserteq_str("~\n", str);
 
 	/* The last two characters are set to \n\n for all buffer sizes > 2 */
 	s = display_options_get_banner(false, str, sizeof(str));
 	ut_asserteq_ptr(str, s);
-	ut_assertok(strcmp("U-Boot \n\n", s));
+	ut_asserteq_str("U-Boot \n\n", s);
 
 	/* Give it enough space for some of the version */
 	big_str_len = strlen(version_string) - 5;
@@ -131,7 +131,7 @@ static int print_printf(struct unit_test_state *uts)
 					    big_str_len);
 	ut_asserteq_ptr(big_str, s);
 	ut_assertok(strncmp(version_string, s, big_str_len - 3));
-	ut_assertok(strcmp("\n\n", s + big_str_len - 3));
+	ut_asserteq_str("\n\n", s + big_str_len - 3);
 
 	/* Give it enough space for the version and some of the build tag */
 	big_str_len = strlen(version_string) + 9 + 20;
@@ -142,7 +142,7 @@ static int print_printf(struct unit_test_state *uts)
 	ut_assertok(strncmp(version_string, s, len));
 	ut_assertok(strncmp(", Build: ", s + len, 9));
 	ut_assertok(strncmp(FAKE_BUILD_TAG, s + 9 + len, 12));
-	ut_assertok(strcmp("\n\n", s + big_str_len - 3));
+	ut_asserteq_str("\n\n", s + big_str_len - 3);
 
 	return 0;
 }
-- 
2.32.0

Re: [PATCH v2 05/12] test: Use ut_asserteq_str() instead of ut_assertok(strcmp())

[Simon Glass]

On Wed, 3 Nov 2021 at 17:23, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> Use the ut_asserteq_str() assertion instead of strcmp() function and
> ut_assertok()ing it's result.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  test/dm/cpu.c     |  4 ++--
>  test/dm/soc.c     |  4 ++--
>  test/dm/sysinfo.c |  6 +++---
>  test/dm/usb.c     |  2 +-
>  test/dm/virtio.c  |  2 +-
>  test/print_ut.c   | 32 ++++++++++++++++----------------
>  6 files changed, 25 insertions(+), 25 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 06/12] sysinfo: Add get_str_list() method

[Marek Behún]

From: Marek Behún <marek.behun@nic.cz>

Add get_str_list() method to sysinfo operations.

The get_str_list() method is similar to get_str(), but receives one
additional argument, @idx, and fills in the @idx-th string from a given
list.

Add sandbox implementation together with a unittest.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/sysinfo/sandbox.c        | 15 +++++++++++
 drivers/sysinfo/sysinfo-uclass.c | 15 +++++++++++
 include/sysinfo.h                | 44 ++++++++++++++++++++++++++++++++
 test/dm/sysinfo.c                | 13 ++++++++++
 4 files changed, 87 insertions(+)

diff --git a/drivers/sysinfo/sandbox.c b/drivers/sysinfo/sandbox.c
index 01c845e310..bede40b662 100644
--- a/drivers/sysinfo/sandbox.c
+++ b/drivers/sysinfo/sandbox.c
@@ -81,6 +81,20 @@ int sysinfo_sandbox_get_str(struct udevice *dev, int id, size_t size, char *val)
 	return -ENOENT;
 }
 
+int sysinfo_sandbox_get_str_list(struct udevice *dev, int id, unsigned idx,
+				 size_t size, char *val)
+{
+	if (id != STR_VACATIONSPOT)
+		return -ENOENT;
+
+	if (idx >= ARRAY_SIZE(vacation_spots))
+		return -ERANGE;
+
+	strncpy(val, vacation_spots[idx], size);
+	val[size - 1] = '\0';
+	return strlen(vacation_spots[idx]);
+}
+
 static const struct udevice_id sysinfo_sandbox_ids[] = {
 	{ .compatible = "sandbox,sysinfo-sandbox" },
 	{ /* sentinel */ }
@@ -91,6 +105,7 @@ static const struct sysinfo_ops sysinfo_sandbox_ops = {
 	.get_bool = sysinfo_sandbox_get_bool,
 	.get_int = sysinfo_sandbox_get_int,
 	.get_str = sysinfo_sandbox_get_str,
+	.get_str_list = sysinfo_sandbox_get_str_list,
 };
 
 int sysinfo_sandbox_probe(struct udevice *dev)
diff --git a/drivers/sysinfo/sysinfo-uclass.c b/drivers/sysinfo/sysinfo-uclass.c
index c5cc3cb959..71f9ad105a 100644
--- a/drivers/sysinfo/sysinfo-uclass.c
+++ b/drivers/sysinfo/sysinfo-uclass.c
@@ -92,6 +92,21 @@ int sysinfo_get_str(struct udevice *dev, int id, size_t size, char *val)
 	return ops->get_str(dev, id, size, val);
 }
 
+int sysinfo_get_str_list(struct udevice *dev, int id, unsigned idx, size_t size,
+			 char *val)
+{
+	struct sysinfo_priv *priv = dev_get_uclass_priv(dev);
+	struct sysinfo_ops *ops = sysinfo_get_ops(dev);
+
+	if (!priv->detected)
+		return -EPERM;
+
+	if (!ops->get_str_list)
+		return -ENOSYS;
+
+	return ops->get_str_list(dev, id, idx, size, val);
+}
+
 UCLASS_DRIVER(sysinfo) = {
 	.id		= UCLASS_SYSINFO,
 	.name		= "sysinfo",
diff --git a/include/sysinfo.h b/include/sysinfo.h
index 6031aec578..0d8a2d1676 100644
--- a/include/sysinfo.h
+++ b/include/sysinfo.h
@@ -106,6 +106,25 @@ struct sysinfo_ops {
 
 	int (*get_str)(struct udevice *dev, int id, size_t size, char *val);
 
+	/**
+	 * get_str_list() - Read a specific string data value from a string list
+	 *		    that describes hardware setup.
+	 * @dev:	The sysinfo instance to gather the data.
+	 * @id:		A unique identifier for the string list to read from.
+	 * @idx:	The index of the string in the string list.
+	 * @size:	The size of the buffer to receive the string data.
+	 *		If the buffer is not large enough to contain the whole
+	 *		string, the string must be trimmed to fit in the
+	 *		buffer including the terminating NULL-byte.
+	 * @val:	Pointer to a buffer that receives the value read.
+	 *
+	 * Return: Actual length of the string excluding the terminating
+	 *	   NULL-byte if OK, -ENOENT if list with ID @id does not exist,
+	 *	   -ERANGE if @idx is invalid or -ve on error.
+	 */
+	int (*get_str_list)(struct udevice *dev, int id, unsigned idx,
+			    size_t size, char *val);
+
 	/**
 	 * get_fit_loadable - Get the name of an image to load from FIT
 	 * This function can be used to provide the image names based on runtime
@@ -180,6 +199,25 @@ int sysinfo_get_int(struct udevice *dev, int id, int *val);
  */
 int sysinfo_get_str(struct udevice *dev, int id, size_t size, char *val);
 
+/**
+ * sysinfo_get_str_list() - Read a specific string data value from a string list
+ *			    that describes hardware setup.
+ * @dev:	The sysinfo instance to gather the data.
+ * @id:		A unique identifier for the string list to read from.
+ * @idx:	The index of the string in the string list.
+ * @size:	The size of the buffer to receive the string data. If the buffer
+ *		is not large enough to contain the whole string, the string will
+ *		be trimmed to fit in the buffer including the terminating
+ *		NULL-byte.
+ * @val:	Pointer to a buffer that receives the value read.
+ *
+ * Return: Actual length of the string excluding the terminating NULL-byte if
+ *	   OK, -ENOENT if list with ID @id does not exist, -ERANGE if @idx is
+ *	   invalid, -EPERM if called before sysinfo_detect(), else -ve on error.
+ */
+int sysinfo_get_str_list(struct udevice *dev, int id, unsigned idx, size_t size,
+			 char *val);
+
 /**
  * sysinfo_get() - Return the sysinfo device for the sysinfo in question.
  * @devp: Pointer to structure to receive the sysinfo device.
@@ -235,6 +273,12 @@ static inline int sysinfo_get_str(struct udevice *dev, int id, size_t size,
 	return -ENOSYS;
 }
 
+static inline int sysinfo_get_str_list(struct udevice *dev, int id,
+				       unsigned idx, size_t size, char *val)
+{
+	return -ENOSYS;
+}
+
 static inline int sysinfo_get(struct udevice **devp)
 {
 	return -ENOSYS;
diff --git a/test/dm/sysinfo.c b/test/dm/sysinfo.c
index 2c1bd1ce40..a6b246f2df 100644
--- a/test/dm/sysinfo.c
+++ b/test/dm/sysinfo.c
@@ -58,6 +58,19 @@ static int dm_test_sysinfo(struct unit_test_state *uts)
 				       str));
 	ut_asserteq_str(str, "Yuggoth");
 
+	ut_asserteq(6, sysinfo_get_str_list(sysinfo, STR_VACATIONSPOT, 0,
+					    sizeof(str), str));
+	ut_asserteq_str(str, "R'lyeh");
+
+	ut_asserteq(17, sysinfo_get_str_list(sysinfo, STR_VACATIONSPOT, 5, 6,
+					     str));
+	ut_asserteq_str(str, "The N");
+
+	ut_asserteq(-ENOENT, sysinfo_get_str_list(sysinfo, INT_TEST1, 0,
+						  sizeof(str), str));
+	ut_asserteq(-ERANGE, sysinfo_get_str_list(sysinfo, STR_VACATIONSPOT, 10,
+						  sizeof(str), str));
+
 	return 0;
 }
 
-- 
2.32.0

Re: [PATCH v2 06/12] sysinfo: Add get_str_list() method

[Simon Glass]

On Wed, 3 Nov 2021 at 17:23, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> Add get_str_list() method to sysinfo operations.
>
> The get_str_list() method is similar to get_str(), but receives one
> additional argument, @idx, and fills in the @idx-th string from a given
> list.
>
> Add sandbox implementation together with a unittest.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  drivers/sysinfo/sandbox.c        | 15 +++++++++++
>  drivers/sysinfo/sysinfo-uclass.c | 15 +++++++++++
>  include/sysinfo.h                | 44 ++++++++++++++++++++++++++++++++
>  test/dm/sysinfo.c                | 13 ++++++++++
>  4 files changed, 87 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Except I think it should return -NOSPC if the buffer is too small.

Re: [PATCH v2 06/12] sysinfo: Add get_str_list() method

[Marek Behún]

On Thu, 4 Nov 2021 20:02:27 -0600
Simon Glass <sjg@chromium.org> wrote:

> On Wed, 3 Nov 2021 at 17:23, Marek Behún <kabel@kernel.org> wrote:
> >
> > From: Marek Behún <marek.behun@nic.cz>
> >
> > Add get_str_list() method to sysinfo operations.
> >
> > The get_str_list() method is similar to get_str(), but receives one
> > additional argument, @idx, and fills in the @idx-th string from a given
> > list.
> >
> > Add sandbox implementation together with a unittest.
> >
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > ---
> >  drivers/sysinfo/sandbox.c        | 15 +++++++++++
> >  drivers/sysinfo/sysinfo-uclass.c | 15 +++++++++++
> >  include/sysinfo.h                | 44 ++++++++++++++++++++++++++++++++
> >  test/dm/sysinfo.c                | 13 ++++++++++
> >  4 files changed, 87 insertions(+)
> >  
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Except I think it should return -NOSPC if the buffer is too small.

No because then you don't know how large buffer you actually need and
have to guess (or grow by a factor of two until it fits).

This way you can call with size=0, get the length, allocate buffer and
call again.

Marek

Re: [PATCH v2 06/12] sysinfo: Add get_str_list() method

[Simon Glass]

Hi Marek,

On Fri, 5 Nov 2021 at 05:20, Marek Behún <kabel@kernel.org> wrote:
>
> On Thu, 4 Nov 2021 20:02:27 -0600
> Simon Glass <sjg@chromium.org> wrote:
>
> > On Wed, 3 Nov 2021 at 17:23, Marek Behún <kabel@kernel.org> wrote:
> > >
> > > From: Marek Behún <marek.behun@nic.cz>
> > >
> > > Add get_str_list() method to sysinfo operations.
> > >
> > > The get_str_list() method is similar to get_str(), but receives one
> > > additional argument, @idx, and fills in the @idx-th string from a given
> > > list.
> > >
> > > Add sandbox implementation together with a unittest.
> > >
> > > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > > ---
> > >  drivers/sysinfo/sandbox.c        | 15 +++++++++++
> > >  drivers/sysinfo/sysinfo-uclass.c | 15 +++++++++++
> > >  include/sysinfo.h                | 44 ++++++++++++++++++++++++++++++++
> > >  test/dm/sysinfo.c                | 13 ++++++++++
> > >  4 files changed, 87 insertions(+)
> > >
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > Except I think it should return -NOSPC if the buffer is too small.
>
> No because then you don't know how large buffer you actually need and
> have to guess (or grow by a factor of two until it fits).
>
> This way you can call with size=0, get the length, allocate buffer and
> call again.

OK, I completely missed that. Can you mention this approach in the comments?

"Actual length of the string" - does that mean the length that would
have been returned if there were enough space? If so, please make that
clear.

Regards,
Simon

>
> Marek

[PATCH v2 07/12] sysinfo: Make .detect() non-mandatory

[Marek Behún]

From: Marek Behún <marek.behun@nic.cz>

Make it so that if the .detect() method is not implemented, the sysinfo
is considered to be present.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/sysinfo/sysinfo-uclass.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/sysinfo/sysinfo-uclass.c b/drivers/sysinfo/sysinfo-uclass.c
index 71f9ad105a..d945f073c5 100644
--- a/drivers/sysinfo/sysinfo-uclass.c
+++ b/drivers/sysinfo/sysinfo-uclass.c
@@ -25,10 +25,7 @@ int sysinfo_detect(struct udevice *dev)
 	struct sysinfo_priv *priv = dev_get_uclass_priv(dev);
 	struct sysinfo_ops *ops = sysinfo_get_ops(dev);
 
-	if (!ops->detect)
-		return -ENOSYS;
-
-	ret = ops->detect(dev);
+	ret = ops->detect ? ops->detect(dev) : 0;
 	if (!ret)
 		priv->detected = true;
 
-- 
2.32.0

Re: [PATCH v2 07/12] sysinfo: Make .detect() non-mandatory

[Simon Glass]

On Wed, 3 Nov 2021 at 17:23, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> Make it so that if the .detect() method is not implemented, the sysinfo
> is considered to be present.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  drivers/sysinfo/sysinfo-uclass.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

Need to update sysinfo.h docs for the detect() method, I think.


>
> diff --git a/drivers/sysinfo/sysinfo-uclass.c b/drivers/sysinfo/sysinfo-uclass.c
> index 71f9ad105a..d945f073c5 100644
> --- a/drivers/sysinfo/sysinfo-uclass.c
> +++ b/drivers/sysinfo/sysinfo-uclass.c
> @@ -25,10 +25,7 @@ int sysinfo_detect(struct udevice *dev)
>         struct sysinfo_priv *priv = dev_get_uclass_priv(dev);
>         struct sysinfo_ops *ops = sysinfo_get_ops(dev);
>
> -       if (!ops->detect)
> -               return -ENOSYS;
> -
> -       ret = ops->detect(dev);
> +       ret = ops->detect ? ops->detect(dev) : 0;
>         if (!ret)
>                 priv->detected = true;
>
> --
> 2.32.0
>

[PATCH v2 08/12] sysinfo: Add support for iterating string list

[Marek Behún]

From: Marek Behún <marek.behun@nic.cz>

Add function
  sysinfo_get_str_list_max_len()
to determine length of the longest string in a string list, functions
  sysinfo_str_list_first() and
  sysinfo_str_list_next()
to support iteration over string list elements and macro
  for_each_sysinfo_str_list()
to make the iteration simple to use.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/sysinfo/sysinfo-uclass.c | 79 +++++++++++++++++++++++++
 include/sysinfo.h                | 99 ++++++++++++++++++++++++++++++++
 test/dm/sysinfo.c                | 35 +++++++++++
 3 files changed, 213 insertions(+)

diff --git a/drivers/sysinfo/sysinfo-uclass.c b/drivers/sysinfo/sysinfo-uclass.c
index d945f073c5..78035a95aa 100644
--- a/drivers/sysinfo/sysinfo-uclass.c
+++ b/drivers/sysinfo/sysinfo-uclass.c
@@ -8,6 +8,7 @@
 
 #include <common.h>
 #include <dm.h>
+#include <malloc.h>
 #include <sysinfo.h>
 
 struct sysinfo_priv {
@@ -104,6 +105,84 @@ int sysinfo_get_str_list(struct udevice *dev, int id, unsigned idx, size_t size,
 	return ops->get_str_list(dev, id, idx, size, val);
 }
 
+int sysinfo_get_str_list_max_len(struct udevice *dev, int id)
+{
+	int maxlen = 0;
+	unsigned i;
+
+	/* first find out length of the longest string in the list */
+	for (i = 0; ; ++i) {
+		char dummy[1];
+		int len;
+
+		len = sysinfo_get_str_list(dev, id, i, 0, dummy);
+		if (len == -ERANGE)
+			break;
+		else if (len < 0)
+			return len;
+		else if (len > maxlen)
+			maxlen = len;
+	}
+
+	return maxlen;
+}
+
+struct sysinfo_str_list_iter {
+	struct udevice *dev;
+	int id;
+	size_t size;
+	unsigned idx;
+	char value[];
+};
+
+char *sysinfo_str_list_first(struct udevice *dev, int id, void *_iter)
+{
+	struct sysinfo_str_list_iter *iter, **iterp = _iter;
+	int maxlen, res;
+
+	maxlen = sysinfo_get_str_list_max_len(dev, id);
+	if (maxlen < 0)
+		return NULL;
+
+	iter = malloc(sizeof(struct sysinfo_str_list_iter) + maxlen + 1);
+	if (!iter) {
+		printf("No memory for sysinfo string list iterator\n");
+		return NULL;
+	}
+
+	iter->dev = dev;
+	iter->id = id;
+	iter->size = maxlen + 1;
+	iter->idx = 0;
+
+	res = sysinfo_get_str_list(dev, id, 0, iter->size, iter->value);
+	if (res < 0) {
+		*iterp = NULL;
+		free(iter);
+		return NULL;
+	}
+
+	*iterp = iter;
+
+	return iter->value;
+}
+
+char *sysinfo_str_list_next(void *_iter)
+{
+	struct sysinfo_str_list_iter **iterp = _iter, *iter = *iterp;
+	int res;
+
+	res = sysinfo_get_str_list(iter->dev, iter->id, ++iter->idx, iter->size,
+				   iter->value);
+	if (res < 0) {
+		*iterp = NULL;
+		free(iter);
+		return NULL;
+	}
+
+	return iter->value;
+}
+
 UCLASS_DRIVER(sysinfo) = {
 	.id		= UCLASS_SYSINFO,
 	.name		= "sysinfo",
diff --git a/include/sysinfo.h b/include/sysinfo.h
index 0d8a2d1676..d32bf3e808 100644
--- a/include/sysinfo.h
+++ b/include/sysinfo.h
@@ -218,6 +218,61 @@ int sysinfo_get_str(struct udevice *dev, int id, size_t size, char *val);
 int sysinfo_get_str_list(struct udevice *dev, int id, unsigned idx, size_t size,
 			 char *val);
 
+/**
+ * sysinfo_get_str_list_max_len() - Get length of longest string in a string
+ *				    list that describes hardware setup.
+ * @dev:	The sysinfo instance to gather the data.
+ * @id:		A unique identifier for the string list to read from.
+ *
+ * Return: Length (excluding the terminating NULL-byte) of the longest string in
+ *	   the string list, or -ve on error.
+ */
+int sysinfo_get_str_list_max_len(struct udevice *dev, int id);
+
+/**
+ * sysinfo_str_list_first() - Start iterating a string list.
+ * @dev:	The sysinfo instance to gather the data.
+ * @id:		A unique identifier for the string list to read from.
+ * @_iter:	Pointer where iterator data will be stored.
+ *
+ * Pass a reference to a void * pointer as @_iter, i.e.
+ *	void *iter;
+ *	first = sysinfo_str_list_first(dev, id, &iter);
+ *
+ * The function will allocate space for the value. You need to iterate all
+ * elements with sysinfo_str_list_next() for the space to be freed, or free
+ * the pointer stored in @_iter, i.e.
+ *	void *iter;
+ *	first = sysinfo_str_list_first(dev, id, &iter);
+ *	if (first)
+ *		free(iter);
+ *
+ * Return: First string in the string list, or NULL on error.
+ */
+char *sysinfo_str_list_first(struct udevice *dev, int id, void *_iter);
+
+/**
+ * sysinfo_str_list_next() - Get next string in the string string list.
+ * @_iter:	Pointer to iterator, filled in by sysinfo_str_list_first().
+ *
+ * Pass a reference to a void * pointer as @_iter, i.e.
+ *	void *iter;
+ *	first = sysinfo_str_list_first(dev, id, &iter);
+ *	next = sysinfo_str_list_next(&iter);
+ *
+ * All elements must be iterated until the function returns NULL for the
+ * resources allocated for the iteration to be freed, or pointer stored in
+ * @_iter must be freed, i.e.:
+ *	void *iter;
+ *	first = sysinfo_str_list_first(dev, id, &iter);
+ *	next = sysinfo_str_list_next(&iter);
+ *	if (next)
+ *		free(iter);
+ *
+ * Return: Next string in the string list, NULL on end of list or NULL on error.
+ */
+char *sysinfo_str_list_next(void *_iter);
+
 /**
  * sysinfo_get() - Return the sysinfo device for the sysinfo in question.
  * @devp: Pointer to structure to receive the sysinfo device.
@@ -279,6 +334,22 @@ static inline int sysinfo_get_str_list(struct udevice *dev, int id,
 	return -ENOSYS;
 }
 
+static inline int sysinfo_get_str_list_max_len(struct udevice *dev, int id)
+{
+	return -ENOSYS;
+}
+
+static inline char *sysinfo_str_list_first(struct udevice *dev, int id,
+					   void *_iter)
+{
+	return NULL;
+}
+
+static inline char *sysinfo_str_list_next(void *_iter)
+{
+	return NULL;
+}
+
 static inline int sysinfo_get(struct udevice **devp)
 {
 	return -ENOSYS;
@@ -291,4 +362,32 @@ static inline int sysinfo_get_fit_loadable(struct udevice *dev, int index,
 }
 
 #endif
+
+/**
+ * for_each_sysinfo_str_list - Iterate a sysinfo string list
+ * @dev:	The sysinfo instance to gather the data.
+ * @id:		A unique identifier for the string list to read from.
+ * @val:	String pointer for the value.
+ * @iter:	Pointer where iteration data are stored.
+ *
+ * Important: all elements of the list must be iterated for the iterator
+ * resources to be freed automatically. If you need to break from the for cycle,
+ * you need to free the iterator.
+ *
+ * Example:
+ *	char *value;
+ *	void *iter;
+ *	for_each_sysinfo_str_list(dev, MY_STR_LIST, value, iter) {
+ *		printf("Value: %s\n", value);
+ *		if (!strcmp(value, "needle")) {
+ *			free(iter);
+ *			break;
+ *		}
+ *	}
+ */
+#define for_each_sysinfo_str_list(dev, id, val, iter)			\
+	for ((val) = sysinfo_str_list_first((dev), (id), &(iter));	\
+	     (val);							\
+	     (val) = sysinfo_str_list_next(&(iter)))
+
 #endif
diff --git a/test/dm/sysinfo.c b/test/dm/sysinfo.c
index a6b246f2df..e9b70d8e1a 100644
--- a/test/dm/sysinfo.c
+++ b/test/dm/sysinfo.c
@@ -75,3 +75,38 @@ static int dm_test_sysinfo(struct unit_test_state *uts)
 }
 
 DM_TEST(dm_test_sysinfo, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
+
+static int dm_test_sysinfo_str_list_iter(struct unit_test_state *uts)
+{
+	struct udevice *sysinfo;
+	char *value;
+	void *iter;
+	int idx;
+
+	ut_assertok(sysinfo_get(&sysinfo));
+	ut_assert(sysinfo);
+
+	sysinfo_detect(sysinfo);
+
+	idx = 0;
+	for_each_sysinfo_str_list(sysinfo, STR_VACATIONSPOT, value, iter) {
+		switch (idx) {
+		case 0:
+			ut_asserteq_str(value, "R'lyeh");
+			break;
+		case 2:
+			ut_asserteq_str(value, "Plateau of Leng");
+			break;
+		case 3:
+			ut_asserteq_str(value, "Carcosa");
+			break;
+		}
+		++idx;
+	}
+
+	ut_assert(NULL == iter);
+
+	return 0;
+}
+
+DM_TEST(dm_test_sysinfo_str_list_iter, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
-- 
2.32.0

Re: [PATCH v2 08/12] sysinfo: Add support for iterating string list

[Simon Glass]

Hi Marek,

On Wed, 3 Nov 2021 at 17:23, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> Add function
>   sysinfo_get_str_list_max_len()
> to determine length of the longest string in a string list, functions
>   sysinfo_str_list_first() and
>   sysinfo_str_list_next()
> to support iteration over string list elements and macro
>   for_each_sysinfo_str_list()
> to make the iteration simple to use.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  drivers/sysinfo/sysinfo-uclass.c | 79 +++++++++++++++++++++++++
>  include/sysinfo.h                | 99 ++++++++++++++++++++++++++++++++
>  test/dm/sysinfo.c                | 35 +++++++++++
>  3 files changed, 213 insertions(+)
>
> diff --git a/drivers/sysinfo/sysinfo-uclass.c b/drivers/sysinfo/sysinfo-uclass.c
> index d945f073c5..78035a95aa 100644
> --- a/drivers/sysinfo/sysinfo-uclass.c
> +++ b/drivers/sysinfo/sysinfo-uclass.c
> @@ -8,6 +8,7 @@
>
>  #include <common.h>
>  #include <dm.h>
> +#include <malloc.h>
>  #include <sysinfo.h>
>
>  struct sysinfo_priv {
> @@ -104,6 +105,84 @@ int sysinfo_get_str_list(struct udevice *dev, int id, unsigned idx, size_t size,
>         return ops->get_str_list(dev, id, idx, size, val);
>  }
>
> +int sysinfo_get_str_list_max_len(struct udevice *dev, int id)
> +{
> +       int maxlen = 0;
> +       unsigned i;
> +
> +       /* first find out length of the longest string in the list */
> +       for (i = 0; ; ++i) {
> +               char dummy[1];
> +               int len;
> +
> +               len = sysinfo_get_str_list(dev, id, i, 0, dummy);
> +               if (len == -ERANGE)
> +                       break;
> +               else if (len < 0)
> +                       return len;
> +               else if (len > maxlen)
> +                       maxlen = len;
> +       }
> +
> +       return maxlen;
> +}
> +
> +struct sysinfo_str_list_iter {
> +       struct udevice *dev;
> +       int id;
> +       size_t size;
> +       unsigned idx;
> +       char value[];
> +};
> +
> +char *sysinfo_str_list_first(struct udevice *dev, int id, void *_iter)

Better to make iter a struct sysinfo_str_list_iter, I think and
require the caller to declare it:

sysinfo_str_list_iter iter;
char str[80]'

p = sysinfo_str_list_first(dev, i, &str, sizeof(str), &iter);
...

Do you need the iter?

If you want to support arbitratry length, I suppose that is OK?? But I
don't like allocating memory unless it is needed.

> +{
> +       struct sysinfo_str_list_iter *iter, **iterp = _iter;
> +       int maxlen, res;
> +
> +       maxlen = sysinfo_get_str_list_max_len(dev, id);
> +       if (maxlen < 0)
> +               return NULL;
> +
> +       iter = malloc(sizeof(struct sysinfo_str_list_iter) + maxlen + 1);
> +       if (!iter) {
> +               printf("No memory for sysinfo string list iterator\n");
> +               return NULL;
> +       }
> +
> +       iter->dev = dev;
> +       iter->id = id;
> +       iter->size = maxlen + 1;
> +       iter->idx = 0;
> +
> +       res = sysinfo_get_str_list(dev, id, 0, iter->size, iter->value);
> +       if (res < 0) {
> +               *iterp = NULL;
> +               free(iter);
> +               return NULL;
> +       }
> +
> +       *iterp = iter;
> +
> +       return iter->value;
> +}
> +
> +char *sysinfo_str_list_next(void *_iter)
> +{
> +       struct sysinfo_str_list_iter **iterp = _iter, *iter = *iterp;
> +       int res;
> +
> +       res = sysinfo_get_str_list(iter->dev, iter->id, ++iter->idx, iter->size,
> +                                  iter->value);
> +       if (res < 0) {
> +               *iterp = NULL;
> +               free(iter);
> +               return NULL;
> +       }
> +
> +       return iter->value;
> +}
> +
>  UCLASS_DRIVER(sysinfo) = {
>         .id             = UCLASS_SYSINFO,
>         .name           = "sysinfo",
> diff --git a/include/sysinfo.h b/include/sysinfo.h
> index 0d8a2d1676..d32bf3e808 100644
> --- a/include/sysinfo.h
> +++ b/include/sysinfo.h
> @@ -218,6 +218,61 @@ int sysinfo_get_str(struct udevice *dev, int id, size_t size, char *val);
>  int sysinfo_get_str_list(struct udevice *dev, int id, unsigned idx, size_t size,
>                          char *val);
>
> +/**
> + * sysinfo_get_str_list_max_len() - Get length of longest string in a string
> + *                                 list that describes hardware setup.
> + * @dev:       The sysinfo instance to gather the data.
> + * @id:                A unique identifier for the string list to read from.
> + *
> + * Return: Length (excluding the terminating NULL-byte) of the longest string in
> + *        the string list, or -ve on error.
> + */
> +int sysinfo_get_str_list_max_len(struct udevice *dev, int id);
> +
> +/**
> + * sysinfo_str_list_first() - Start iterating a string list.
> + * @dev:       The sysinfo instance to gather the data.
> + * @id:                A unique identifier for the string list to read from.
> + * @_iter:     Pointer where iterator data will be stored.
> + *
> + * Pass a reference to a void * pointer as @_iter, i.e.
> + *     void *iter;
> + *     first = sysinfo_str_list_first(dev, id, &iter);
> + *
> + * The function will allocate space for the value. You need to iterate all
> + * elements with sysinfo_str_list_next() for the space to be freed, or free
> + * the pointer stored in @_iter, i.e.
> + *     void *iter;
> + *     first = sysinfo_str_list_first(dev, id, &iter);
> + *     if (first)
> + *             free(iter);
> + *
> + * Return: First string in the string list, or NULL on error.
> + */
> +char *sysinfo_str_list_first(struct udevice *dev, int id, void *_iter);
> +
> +/**
> + * sysinfo_str_list_next() - Get next string in the string string list.
> + * @_iter:     Pointer to iterator, filled in by sysinfo_str_list_first().
> + *
> + * Pass a reference to a void * pointer as @_iter, i.e.
> + *     void *iter;
> + *     first = sysinfo_str_list_first(dev, id, &iter);
> + *     next = sysinfo_str_list_next(&iter);
> + *
> + * All elements must be iterated until the function returns NULL for the
> + * resources allocated for the iteration to be freed, or pointer stored in
> + * @_iter must be freed, i.e.:
> + *     void *iter;
> + *     first = sysinfo_str_list_first(dev, id, &iter);
> + *     next = sysinfo_str_list_next(&iter);
> + *     if (next)
> + *             free(iter);
> + *
> + * Return: Next string in the string list, NULL on end of list or NULL on error.
> + */
> +char *sysinfo_str_list_next(void *_iter);
> +
>  /**
>   * sysinfo_get() - Return the sysinfo device for the sysinfo in question.
>   * @devp: Pointer to structure to receive the sysinfo device.
> @@ -279,6 +334,22 @@ static inline int sysinfo_get_str_list(struct udevice *dev, int id,
>         return -ENOSYS;
>  }
>
> +static inline int sysinfo_get_str_list_max_len(struct udevice *dev, int id)
> +{
> +       return -ENOSYS;
> +}
> +
> +static inline char *sysinfo_str_list_first(struct udevice *dev, int id,
> +                                          void *_iter)
> +{
> +       return NULL;
> +}
> +
> +static inline char *sysinfo_str_list_next(void *_iter)
> +{
> +       return NULL;
> +}
> +
>  static inline int sysinfo_get(struct udevice **devp)
>  {
>         return -ENOSYS;
> @@ -291,4 +362,32 @@ static inline int sysinfo_get_fit_loadable(struct udevice *dev, int index,
>  }
>
>  #endif
> +
> +/**
> + * for_each_sysinfo_str_list - Iterate a sysinfo string list
> + * @dev:       The sysinfo instance to gather the data.
> + * @id:                A unique identifier for the string list to read from.
> + * @val:       String pointer for the value.
> + * @iter:      Pointer where iteration data are stored.
> + *
> + * Important: all elements of the list must be iterated for the iterator
> + * resources to be freed automatically. If you need to break from the for cycle,
> + * you need to free the iterator.
> + *
> + * Example:
> + *     char *value;
> + *     void *iter;
> + *     for_each_sysinfo_str_list(dev, MY_STR_LIST, value, iter) {
> + *             printf("Value: %s\n", value);
> + *             if (!strcmp(value, "needle")) {
> + *                     free(iter);
> + *                     break;
> + *             }
> + *     }
> + */
> +#define for_each_sysinfo_str_list(dev, id, val, iter)                  \
> +       for ((val) = sysinfo_str_list_first((dev), (id), &(iter));      \
> +            (val);                                                     \
> +            (val) = sysinfo_str_list_next(&(iter)))
> +
>  #endif
> diff --git a/test/dm/sysinfo.c b/test/dm/sysinfo.c
> index a6b246f2df..e9b70d8e1a 100644
> --- a/test/dm/sysinfo.c
> +++ b/test/dm/sysinfo.c
> @@ -75,3 +75,38 @@ static int dm_test_sysinfo(struct unit_test_state *uts)
>  }
>
>  DM_TEST(dm_test_sysinfo, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
> +
> +static int dm_test_sysinfo_str_list_iter(struct unit_test_state *uts)
> +{
> +       struct udevice *sysinfo;
> +       char *value;
> +       void *iter;
> +       int idx;
> +
> +       ut_assertok(sysinfo_get(&sysinfo));
> +       ut_assert(sysinfo);
> +
> +       sysinfo_detect(sysinfo);
> +
> +       idx = 0;
> +       for_each_sysinfo_str_list(sysinfo, STR_VACATIONSPOT, value, iter) {
> +               switch (idx) {
> +               case 0:
> +                       ut_asserteq_str(value, "R'lyeh");
> +                       break;
> +               case 2:
> +                       ut_asserteq_str(value, "Plateau of Leng");
> +                       break;
> +               case 3:
> +                       ut_asserteq_str(value, "Carcosa");
> +                       break;
> +               }
> +               ++idx;
> +       }
> +
> +       ut_assert(NULL == iter);
> +
> +       return 0;
> +}
> +
> +DM_TEST(dm_test_sysinfo_str_list_iter, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
> --
> 2.32.0
>

Regards,
Simon

Re: [PATCH v2 08/12] sysinfo: Add support for iterating string list

[Marek Behún]

On Thu, 4 Nov 2021 20:02:29 -0600
Simon Glass <sjg@chromium.org> wrote:

> Better to make iter a struct sysinfo_str_list_iter, I think and
> require the caller to declare it:
> 
> sysinfo_str_list_iter iter;
> char str[80]'
> 
> p = sysinfo_str_list_first(dev, i, &str, sizeof(str), &iter);
> ...
> 
> Do you need the iter?
> 
> If you want to support arbitratry length, I suppose that is OK?? But I
> don't like allocating memory unless it is needed.

Well if I am iterating through default environment variables
overwrites, they can be basically up to ENV_SIZE long. There may be
some long commands stored there.

Another solution would be to redesign sysinfo_get_str() and introduce
sysinfo_get_str_list() so that they won't fill a buffer given by user,
but instead have their own buffer in implementation and return const
char * pointer.

Marek

Re: [PATCH v2 08/12] sysinfo: Add support for iterating string list

[Simon Glass]

Hi Marek,

On Fri, 5 Nov 2021 at 05:24, Marek Behún <kabel@kernel.org> wrote:
>
> On Thu, 4 Nov 2021 20:02:29 -0600
> Simon Glass <sjg@chromium.org> wrote:
>
> > Better to make iter a struct sysinfo_str_list_iter, I think and
> > require the caller to declare it:
> >
> > sysinfo_str_list_iter iter;
> > char str[80]'
> >
> > p = sysinfo_str_list_first(dev, i, &str, sizeof(str), &iter);
> > ...
> >
> > Do you need the iter?
> >
> > If you want to support arbitratry length, I suppose that is OK?? But I
> > don't like allocating memory unless it is needed.
>
> Well if I am iterating through default environment variables
> overwrites, they can be basically up to ENV_SIZE long. There may be
> some long commands stored there.

OK.

>
> Another solution would be to redesign sysinfo_get_str() and introduce
> sysinfo_get_str_list() so that they won't fill a buffer given by user,
> but instead have their own buffer in implementation and return const
> char * pointer.

Yes that was a design idea at the start...but I think at present I
like the current API. I just didn't understand your intent properly.

My new thoughts:
- pass in the iter so malloc() is not needed there (change str to a char *)
- return an int from your iterator functions, so you can tell when you
run out of memory and need to die
- comment struct sysinfo_str_list_iter
- use log_debug() instead of printf(), on error

Also I wonder about this:
- get the caller to provide the str buffer, and maxsize, so malloc()
is not needed

I can see the advantage of allocating though. I wonder if you might
keep track of the current buffer length and do a realloc() if it is
too small, each time? Scanning through to find the max length might be
slower? Some drivers may read from an I2C device, for example.

Regards,
Simon

[PATCH v2 09/12] env: Change return behaviour of env_set_default_vars()

[Marek Behún]

From: Marek Behún <marek.behun@nic.cz>

The env_set_default_vars() function does not document return value and
behaves differently from other env_* functions.

Change the return value to return 0 on success and -ve on error.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 env/common.c  | 9 ++++++---
 include/env.h | 1 +
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/env/common.c b/env/common.c
index 208e2adaa0..cefe58561b 100644
--- a/env/common.c
+++ b/env/common.c
@@ -283,9 +283,12 @@ int env_set_default_vars(int nvars, char * const vars[], int flags)
 	 * (and use \0 as a separator)
 	 */
 	flags |= H_NOCLEAR | H_DEFAULT;
-	return himport_r(&env_htab, default_environment,
-				sizeof(default_environment), '\0',
-				flags, 0, nvars, vars);
+	if (!himport_r(&env_htab, default_environment,
+		       sizeof(default_environment), '\0', flags, 0, nvars,
+		       vars))
+		return -errno;
+
+	return 0;
 }
 
 /*
diff --git a/include/env.h b/include/env.h
index ee5e30d036..d0b95a498d 100644
--- a/include/env.h
+++ b/include/env.h
@@ -245,6 +245,7 @@ void env_fix_drivers(void);
  * @nvars: Number of variables to set/reset
  * @vars: List of variables to set/reset
  * @flags: Flags controlling matching (H_... - see search.h)
+ * @return 0 if OK, -ve on error
  */
 int env_set_default_vars(int nvars, char *const vars[], int flags);
 
-- 
2.32.0

Re: [PATCH v2 09/12] env: Change return behaviour of env_set_default_vars()

[Simon Glass]

On Wed, 3 Nov 2021 at 17:23, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> The env_set_default_vars() function does not document return value and
> behaves differently from other env_* functions.
>
> Change the return value to return 0 on success and -ve on error.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  env/common.c  | 9 ++++++---
>  include/env.h | 1 +
>  2 files changed, 7 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 10/12] env: Add support for overwriting default environment via sysinfo

[Marek Behún]

From: Marek Behún <marek.behun@nic.cz>

The default_environment[] buffer is built at compile time, but sometimes
it makes sense for some default environment variables to be determined
at runtime, for example:
- one board code may support different boards, and needs that
    fdtfile, board, board_name
  are set appropriately when command
    env default -a
  is executed
- some boards may want to prohibit the
    env default -a
  command to remove device MAC addresses stored in
    ethaddr, ethNaddr.
  This is the case for the ESPRESSObin board code, for example, where
  currently the board_late_init() function rewrites the default
  environment array to achieve this.

Add new sysinfo IDs
  SYSINFO_ID_DEF_ENV_NAMES
  SYSINFO_ID_DEF_ENV_VALUES
corresponding to sysinfo string list of names and values of default
environment variables that are to take precedence over the
default_environment[] buffer.

Add code to default environemnt handlers in env/common.c, which iterate
these sysinfo string lists correspondingly.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 env/common.c      | 107 +++++++++++++++++++++++++++++++++++++++++++++-
 include/sysinfo.h |   4 ++
 2 files changed, 110 insertions(+), 1 deletion(-)

diff --git a/env/common.c b/env/common.c
index cefe58561b..8ac287beae 100644
--- a/env/common.c
+++ b/env/common.c
@@ -22,6 +22,7 @@
 #include <u-boot/crc.h>
 #include <dm/ofnode.h>
 #include <net.h>
+#include <sysinfo.h>
 #include <watchdog.h>
 
 DECLARE_GLOBAL_DATA_PTR;
@@ -148,6 +149,31 @@ char *from_env(const char *envvar)
 	return ret;
 }
 
+static int sysinfo_default_env_get(const char *var, char *buf, unsigned len)
+{
+	struct udevice *dev;
+	unsigned idx;
+	char *name;
+	void *iter;
+
+	if (sysinfo_get(&dev) < 0 || sysinfo_detect(dev) < 0)
+		return -ENODEV;
+
+	idx = 0;
+	for_each_sysinfo_str_list(dev, SYSINFO_ID_DEF_ENV_NAMES, name, iter) {
+		if (strcmp(var, name)) {
+			++idx;
+			continue;
+		}
+
+		free(iter);
+		return sysinfo_get_str_list(dev, SYSINFO_ID_DEF_ENV_VALUES, idx,
+					    len, buf);
+	}
+
+	return -ENOENT;
+}
+
 static int env_get_from_linear(const char *env, const char *name, char *buf,
 			       unsigned len)
 {
@@ -157,6 +183,17 @@ static int env_get_from_linear(const char *env, const char *name, char *buf,
 	if (name == NULL || *name == '\0')
 		return -1;
 
+	if (env == default_environment) {
+		int res = sysinfo_default_env_get(name, buf, len);
+
+		/*
+		 * Board special default envs take precedence over the
+		 * default_environment[] array.
+		 */
+		if (res >= 0)
+			return res;
+	}
+
 	name_len = strlen(name);
 
 	for (p = env; *p != '\0'; p = end + 1) {
@@ -248,8 +285,69 @@ char *env_get_default(const char *name)
 	return NULL;
 }
 
+static int sysinfo_import_default_envs(bool all, int nvars, char * const vars[],
+				       int flags)
+{
+	struct udevice *dev;
+	char *name, *value;
+	unsigned idx;
+	void *iter;
+	int len;
+
+	if (sysinfo_get(&dev) < 0 || sysinfo_detect(dev) < 0)
+		return 0;
+
+	len = sysinfo_get_str_list_max_len(dev, SYSINFO_ID_DEF_ENV_VALUES);
+	if (len == -ENOSYS || len == -ENOENT || len == -ERANGE)
+		return 0;
+	else if (len < 0)
+		return len;
+
+	value = malloc(len + 1);
+	if (!value)
+		return -ENOMEM;
+
+	idx = 0;
+	for_each_sysinfo_str_list(dev, SYSINFO_ID_DEF_ENV_NAMES, name, iter) {
+		struct env_entry e, *ep;
+
+		if (!all) {
+			int j;
+
+			/* If name is not in vars, skip */
+			for (j = 0; j < nvars; ++j)
+				if (!strcmp(name, vars[j]))
+					break;
+			if (j == nvars) {
+				idx++;
+				continue;
+			}
+		}
+
+		len = sysinfo_get_str_list(dev, SYSINFO_ID_DEF_ENV_VALUES,
+					   idx++, ENV_SIZE, value);
+		if (len < 0)
+			continue;
+
+		e.key = name;
+		e.data = value;
+		if (!hsearch_r(e, ENV_ENTER, &ep, &env_htab, flags)) {
+			int res = -errno;
+			free(iter);
+			free(value);
+			return res;
+		}
+	}
+
+	free(value);
+
+	return 0;
+}
+
 void env_set_default(const char *s, int flags)
 {
+	int res;
+
 	if (s) {
 		if ((flags & H_INTERACTIVE) == 0) {
 			printf("*** Warning - %s, "
@@ -270,6 +368,13 @@ void env_set_default(const char *s, int flags)
 		return;
 	}
 
+	res = sysinfo_import_default_envs(true, 0, NULL, flags);
+	if (res < 0) {
+		pr_err("## Error: Board special default environment import failed: %d\n",
+		       res);
+		return;
+	}
+
 	gd->flags |= GD_FLG_ENV_READY;
 	gd->flags |= GD_FLG_ENV_DEFAULT;
 }
@@ -288,7 +393,7 @@ int env_set_default_vars(int nvars, char * const vars[], int flags)
 		       vars))
 		return -errno;
 
-	return 0;
+	return sysinfo_import_default_envs(false, nvars, vars, flags);
 }
 
 /*
diff --git a/include/sysinfo.h b/include/sysinfo.h
index d32bf3e808..587ad0dc75 100644
--- a/include/sysinfo.h
+++ b/include/sysinfo.h
@@ -47,6 +47,10 @@ enum sysinfo_id {
 	/* For show_board_info() */
 	SYSINFO_ID_BOARD_MODEL,
 
+	/* For overwriting default environment variables */
+	SYSINFO_ID_DEF_ENV_NAMES,
+	SYSINFO_ID_DEF_ENV_VALUES,
+
 	/* First value available for downstream/board used */
 	SYSINFO_ID_USER = 0x1000,
 };
-- 
2.32.0

Re: [PATCH v2 10/12] env: Add support for overwriting default environment via sysinfo

[Simon Glass]

On Wed, 3 Nov 2021 at 17:23, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> The default_environment[] buffer is built at compile time, but sometimes
> it makes sense for some default environment variables to be determined
> at runtime, for example:
> - one board code may support different boards, and needs that
>     fdtfile, board, board_name
>   are set appropriately when command
>     env default -a
>   is executed
> - some boards may want to prohibit the
>     env default -a
>   command to remove device MAC addresses stored in
>     ethaddr, ethNaddr.
>   This is the case for the ESPRESSObin board code, for example, where
>   currently the board_late_init() function rewrites the default
>   environment array to achieve this.
>
> Add new sysinfo IDs
>   SYSINFO_ID_DEF_ENV_NAMES
>   SYSINFO_ID_DEF_ENV_VALUES
> corresponding to sysinfo string list of names and values of default
> environment variables that are to take precedence over the
> default_environment[] buffer.
>
> Add code to default environemnt handlers in env/common.c, which iterate
> these sysinfo string lists correspondingly.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  env/common.c      | 107 +++++++++++++++++++++++++++++++++++++++++++++-
>  include/sysinfo.h |   4 ++
>  2 files changed, 110 insertions(+), 1 deletion(-)
>

Seems OK to me, but see my earlier request to simplify the API and
reduce flexibility.

Regards,
Simon

[PATCH v2 11/12] arm: mvebu: Espressobin: Use new API for setting default env at runtime

[Marek Behún]

From: Marek Behún <marek.behun@nic.cz>

ESPRESSObin's board code uses an ad-hoc solution for ensuring that
ethaddrs are not overwritten by `env default -a` command and that the
`fdtfile` is set to correct value when `env default -a` is called.

This ad-hoc solution is overwriting the default_environment[] buffer in
board_late_init().

Since now we have a specific API for overwriting default environment,
convert this ad-hoc code to this new API.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 board/Marvell/mvebu_armada-37xx/board.c     | 135 +++++++++++++-------
 configs/mvebu_espressobin-88f3720_defconfig |   1 +
 include/configs/mvebu_armada-37xx.h         |  17 +--
 3 files changed, 90 insertions(+), 63 deletions(-)

diff --git a/board/Marvell/mvebu_armada-37xx/board.c b/board/Marvell/mvebu_armada-37xx/board.c
index d7b6ecafbf..d36620c37a 100644
--- a/board/Marvell/mvebu_armada-37xx/board.c
+++ b/board/Marvell/mvebu_armada-37xx/board.c
@@ -6,17 +6,20 @@
 #include <common.h>
 #include <dm.h>
 #include <dm/device-internal.h>
+#include <dm/lists.h>
 #include <env.h>
 #include <env_internal.h>
 #include <i2c.h>
 #include <init.h>
 #include <mmc.h>
+#include <net.h>
 #include <phy.h>
 #include <asm/global_data.h>
 #include <asm/io.h>
 #include <asm/arch/cpu.h>
 #include <asm/arch/soc.h>
 #include <linux/delay.h>
+#include <sysinfo.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -85,66 +88,104 @@ int board_init(void)
 }
 
 #ifdef CONFIG_BOARD_LATE_INIT
-int board_late_init(void)
-{
-	char *ptr = &default_environment[0];
-	struct udevice *dev;
-	struct mmc *mmc_dev;
+struct ebin_sysinfo {
 	bool ddr4, emmc;
-	const char *mac;
-	char eth[10];
-	int i;
-
-	if (!of_machine_is_compatible("globalscale,espressobin"))
-		return 0;
-
-	/* Find free buffer in default_environment[] for new variables */
-	while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
-	ptr += 2;
-
-	/*
-	 * Ensure that 'env default -a' does not erase permanent MAC addresses
-	 * stored in env variables: $ethaddr, $eth1addr, $eth2addr and $eth3addr
-	 */
-
-	mac = env_get("ethaddr");
-	if (mac && strlen(mac) <= 17)
-		ptr += sprintf(ptr, "ethaddr=%s", mac) + 1;
+	char vars[5][9];
+	u8 nvars;
+	u8 macs[4][6];
+};
 
-	for (i = 1; i <= 3; i++) {
-		sprintf(eth, "eth%daddr", i);
-		mac = env_get(eth);
-		if (mac && strlen(mac) <= 17)
-			ptr += sprintf(ptr, "%s=%s", eth, mac) + 1;
-	}
+/*
+ * We must do this in probe() instead of detect(), because we call
+ * eth_env_get_enetaddr_by_index(), which may try to read default environment,
+ * which may call detect() again from itself.
+ */
+static int ebin_probe(struct udevice *dev)
+{
+	struct ebin_sysinfo *priv = dev_get_priv(dev);
+	struct mmc *mmc;
+	int i;
 
 	/* If the memory controller has been configured for DDR4, we're running on v7 */
-	ddr4 = ((readl(A3700_CH0_MC_CTRL2_REG) >> A3700_MC_CTRL2_SDRAM_TYPE_OFFS)
-		& A3700_MC_CTRL2_SDRAM_TYPE_MASK) == A3700_MC_CTRL2_SDRAM_TYPE_DDR4;
+	priv->ddr4 = ((readl(A3700_CH0_MC_CTRL2_REG) >> A3700_MC_CTRL2_SDRAM_TYPE_OFFS)
+		      & A3700_MC_CTRL2_SDRAM_TYPE_MASK) == A3700_MC_CTRL2_SDRAM_TYPE_DDR4;
 
 	/* eMMC is mmc dev num 1 */
-	mmc_dev = find_mmc_device(1);
-	emmc = (mmc_dev && mmc_get_op_cond(mmc_dev, true) == 0);
+	mmc = find_mmc_device(1);
+	priv->emmc = (mmc && mmc_get_op_cond(mmc, true) == 0);
 
 	/* if eMMC is not present then remove it from DM */
-	if (!emmc && mmc_dev) {
-		dev = mmc_dev->dev;
-		device_remove(dev, DM_REMOVE_NORMAL);
-		device_unbind(dev);
+	if (!priv->emmc && mmc) {
+		struct udevice *mmc_dev = mmc->dev;
+		device_remove(mmc_dev, DM_REMOVE_NORMAL);
+		device_unbind(mmc_dev);
 	}
 
-	/* Ensure that 'env default -a' set correct value to $fdtfile */
-	if (ddr4 && emmc)
-		strcpy(ptr, "fdtfile=marvell/armada-3720-espressobin-v7-emmc.dtb");
-	else if (ddr4)
-		strcpy(ptr, "fdtfile=marvell/armada-3720-espressobin-v7.dtb");
-	else if (emmc)
-		strcpy(ptr, "fdtfile=marvell/armada-3720-espressobin-emmc.dtb");
-	else
-		strcpy(ptr, "fdtfile=marvell/armada-3720-espressobin.dtb");
+	strcpy(priv->vars[priv->nvars++], "fdtfile");
+
+	for (i = 0; i < 4; ++i)
+		if (eth_env_get_enetaddr_by_index("eth", i,
+						  priv->macs[priv->nvars - 1]))
+			sprintf(priv->vars[priv->nvars++],
+				i ? "eth%daddr" : "ethaddr", i);
 
 	return 0;
 }
+
+static int ebin_get_str_list(struct udevice *dev, int id, unsigned idx,
+			     size_t size, char *val)
+{
+	struct ebin_sysinfo *priv = dev_get_priv(dev);
+
+	if (id != SYSINFO_ID_DEF_ENV_NAMES && id != SYSINFO_ID_DEF_ENV_VALUES)
+		return -ENOENT;
+
+	if (idx >= priv->nvars)
+		return -ERANGE;
+
+	if (id == SYSINFO_ID_DEF_ENV_NAMES)
+		return snprintf(val, size, "%s", priv->vars[idx]);
+
+	if (idx == 0)
+		return snprintf(val, size,
+				"marvell/armada-3720-espressobin%s%s.dtb",
+				priv->ddr4 ? "-v7" : "",
+				priv->emmc ? "-emmc" : "");
+
+	return snprintf(val, size, "%pM", priv->macs[idx - 1]);
+}
+
+static struct sysinfo_ops ebin_sysinfo_ops = {
+	.get_str_list = ebin_get_str_list,
+};
+
+U_BOOT_DRIVER(ebin_sysinfo) = {
+	.name = "espressobin-sysinfo",
+	.id = UCLASS_SYSINFO,
+	.ops = &ebin_sysinfo_ops,
+	.priv_auto = sizeof(struct ebin_sysinfo),
+	.probe = ebin_probe,
+};
+
+int board_late_init(void)
+{
+	struct udevice *dev;
+	int res;
+
+	if (!of_machine_is_compatible("globalscale,espressobin"))
+		return 0;
+
+	res = device_bind_driver(gd->dm_root, "espressobin-sysinfo",
+				 "espressobin-sysinfo", &dev);
+	if (res < 0)
+		return res;
+
+	res = device_probe(dev);
+	if (res < 0)
+		return res;
+
+	return sysinfo_detect(dev);
+}
 #endif
 
 /* Board specific AHCI / SATA enable code */
diff --git a/configs/mvebu_espressobin-88f3720_defconfig b/configs/mvebu_espressobin-88f3720_defconfig
index 3a69954fcd..6e15d9b4b1 100644
--- a/configs/mvebu_espressobin-88f3720_defconfig
+++ b/configs/mvebu_espressobin-88f3720_defconfig
@@ -84,6 +84,7 @@ CONFIG_DM_REGULATOR_GPIO=y
 CONFIG_DEBUG_UART_ANNOUNCE=y
 CONFIG_MVEBU_A3700_UART=y
 CONFIG_MVEBU_A3700_SPI=y
+CONFIG_SYSINFO=y
 CONFIG_USB=y
 CONFIG_USB_XHCI_HCD=y
 CONFIG_USB_EHCI_HCD=y
diff --git a/include/configs/mvebu_armada-37xx.h b/include/configs/mvebu_armada-37xx.h
index e7f7e772fc..1669eaf715 100644
--- a/include/configs/mvebu_armada-37xx.h
+++ b/include/configs/mvebu_armada-37xx.h
@@ -35,11 +35,6 @@
 /* End of 16M scrubbed by training in bootrom */
 #define CONFIG_SYS_INIT_SP_ADDR         (CONFIG_SYS_TEXT_BASE + 0xFF0000)
 
-/*
- * Environment
- */
-#define DEFAULT_ENV_IS_RW		/* required for configuring default fdtfile= */
-
 /*
  * Ethernet Driver configuration
  */
@@ -70,15 +65,6 @@
 
 #include <config_distro_bootcmd.h>
 
-/* filler for default values filled by board_early_init_f() */
-#define ENV_RW_FILLER \
-	"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" /* for ethaddr= */ \
-	"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" /* for eth1addr= */ \
-	"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" /* for eth2addr= */ \
-	"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" /* for eth3addr= */ \
-	"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" /* for fdtfile= */ \
-	""
-
 /* fdt_addr and kernel_addr are needed for existing distribution boot scripts */
 #define CONFIG_EXTRA_ENV_SETTINGS	\
 	"scriptaddr=0x6d00000\0"	\
@@ -88,7 +74,6 @@
 	"kernel_addr=0x7000000\0"	\
 	"kernel_addr_r=0x7000000\0"	\
 	"ramdisk_addr_r=0xa000000\0"	\
-	BOOTENV \
-	ENV_RW_FILLER
+	BOOTENV
 
 #endif /* _CONFIG_MVEBU_ARMADA_37XX_H */
-- 
2.32.0

Re: [PATCH v2 11/12] arm: mvebu: Espressobin: Use new API for setting default env at runtime

[Simon Glass]

On Wed, 3 Nov 2021 at 17:23, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> ESPRESSObin's board code uses an ad-hoc solution for ensuring that
> ethaddrs are not overwritten by `env default -a` command and that the
> `fdtfile` is set to correct value when `env default -a` is called.
>
> This ad-hoc solution is overwriting the default_environment[] buffer in
> board_late_init().
>
> Since now we have a specific API for overwriting default environment,
> convert this ad-hoc code to this new API.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  board/Marvell/mvebu_armada-37xx/board.c     | 135 +++++++++++++-------
>  configs/mvebu_espressobin-88f3720_defconfig |   1 +
>  include/configs/mvebu_armada-37xx.h         |  17 +--
>  3 files changed, 90 insertions(+), 63 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 11/12] arm: mvebu: Espressobin: Use new API for setting default env at runtime

[Pali Rohár]

On Thursday 04 November 2021 20:02:33 Simon Glass wrote:
> On Wed, 3 Nov 2021 at 17:23, Marek Behún <kabel@kernel.org> wrote:
> >
> > From: Marek Behún <marek.behun@nic.cz>
> >
> > ESPRESSObin's board code uses an ad-hoc solution for ensuring that
> > ethaddrs are not overwritten by `env default -a` command and that the
> > `fdtfile` is set to correct value when `env default -a` is called.
> >
> > This ad-hoc solution is overwriting the default_environment[] buffer in
> > board_late_init().
> >
> > Since now we have a specific API for overwriting default environment,
> > convert this ad-hoc code to this new API.
> >
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > ---
> >  board/Marvell/mvebu_armada-37xx/board.c     | 135 +++++++++++++-------
> >  configs/mvebu_espressobin-88f3720_defconfig |   1 +
> >  include/configs/mvebu_armada-37xx.h         |  17 +--
> >  3 files changed, 90 insertions(+), 63 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>

Hello Simon! This change needs to be properly tested to ensure that
detection of eMMC is still correctly working and erase of MAC addresses
does not happen again. I will try to find some time during next week to
test this patch series on Espressobin board.

Re: [PATCH v2 11/12] arm: mvebu: Espressobin: Use new API for setting default env at runtime

[Pali Rohár]

On Friday 05 November 2021 09:50:42 Pali Rohár wrote:
> On Thursday 04 November 2021 20:02:33 Simon Glass wrote:
> > On Wed, 3 Nov 2021 at 17:23, Marek Behún <kabel@kernel.org> wrote:
> > >
> > > From: Marek Behún <marek.behun@nic.cz>
> > >
> > > ESPRESSObin's board code uses an ad-hoc solution for ensuring that
> > > ethaddrs are not overwritten by `env default -a` command and that the
> > > `fdtfile` is set to correct value when `env default -a` is called.
> > >
> > > This ad-hoc solution is overwriting the default_environment[] buffer in
> > > board_late_init().
> > >
> > > Since now we have a specific API for overwriting default environment,
> > > convert this ad-hoc code to this new API.
> > >
> > > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > > ---
> > >  board/Marvell/mvebu_armada-37xx/board.c     | 135 +++++++++++++-------
> > >  configs/mvebu_espressobin-88f3720_defconfig |   1 +
> > >  include/configs/mvebu_armada-37xx.h         |  17 +--
> > >  3 files changed, 90 insertions(+), 63 deletions(-)
> > 
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Hello Simon! This change needs to be properly tested to ensure that
> detection of eMMC is still correctly working and erase of MAC addresses
> does not happen again. I will try to find some time during next week to
> test this patch series on Espressobin board.

Hello! I have tested this change and it does NOT work.

Loading Environment from SPIFlash... SF: Detected w25q64dw with page size 256 Bytes, erase size 4 KiB, total 8 MiB
*** Warning - bad CRC, using default environment

=> echo $fdtfile

=>

Seems that $fdtfile is not set to default value when using default
environment. Calling 'env default -a' fixes it:

=> env default -a
## Resetting to default environment
=> echo $fdtfile 
marvell/armada-3720-espressobin.dtb
=> 

[PATCH v2 12/12] env: Remove support for read-write default_environment[]

[Marek Behún]

From: Marek Behún <marek.behun@nic.cz>

Since the default_environment[] buffer is not overwritten anymore by any
board, remove support for read-write default_environment[].

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 include/env_default.h  | 2 --
 include/env_internal.h | 4 ----
 2 files changed, 6 deletions(-)

diff --git a/include/env_default.h b/include/env_default.h
index 23430dc70d..a1bdf97d38 100644
--- a/include/env_default.h
+++ b/include/env_default.h
@@ -19,8 +19,6 @@ env_t embedded_environment __UBOOT_ENV_SECTION__(environment) = {
 	{
 #elif defined(DEFAULT_ENV_INSTANCE_STATIC)
 static char default_environment[] = {
-#elif defined(DEFAULT_ENV_IS_RW)
-char default_environment[] = {
 #else
 const char default_environment[] = {
 #endif
diff --git a/include/env_internal.h b/include/env_internal.h
index f74927cd64..e0fc3e0070 100644
--- a/include/env_internal.h
+++ b/include/env_internal.h
@@ -111,11 +111,7 @@ typedef struct environment_s {
 extern env_t embedded_environment;
 #endif /* ENV_IS_EMBEDDED */
 
-#ifdef DEFAULT_ENV_IS_RW
-extern char default_environment[];
-#else
 extern const char default_environment[];
-#endif
 
 #ifndef DO_DEPS_ONLY
 
-- 
2.32.0

Re: [PATCH v2 12/12] env: Remove support for read-write default_environment[]

[Simon Glass]

On Wed, 3 Nov 2021 at 17:23, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> Since the default_environment[] buffer is not overwritten anymore by any
> board, remove support for read-write default_environment[].
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  include/env_default.h  | 2 --
>  include/env_internal.h | 4 ----
>  2 files changed, 6 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 03/12] env: Simplify env_get_default()

[Simon Glass]

From: Marek Behún <marek.behun@nic.cz>

Instead of pretending that we don't have environment to force searching
default environment in env_get_default(), get the data from the
default_environment[] buffer directly.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 env/common.c | 45 ++++++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

Applied to u-boot-dm, thanks!

Re: [PATCH v2 02/12] env: Fix env_get() when returning empty string using env_get_f()

[Simon Glass]

From: Marek Behún <marek.behun@nic.cz>

The env_get_f() function returns -1 on failure. Returning 0 means that
the variable exists, and is empty string.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 env/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Applied to u-boot-dm, thanks!

Re: [PATCH v2 01/12] env: Don't set ready flag if import failed in env_set_default()

[Simon Glass]

From: Marek Behún <marek.behun@nic.cz>

Do not set GD_FLG_ENV_READY nor GD_FLG_ENV_DEFAULT if failed importing
in env_set_default().

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 env/common.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Applied to u-boot-dm, thanks!