* [PATCH v2 00/12] Board specific runtime determined default env
@ 2021-11-03 23:23 Marek Behún
2021-11-03 23:23 ` [PATCH v2 01/12] env: Don't set ready flag if import failed in env_set_default() Marek Behún
` (14 more replies)
0 siblings, 15 replies; 33+ messages in thread
From: Marek Behún @ 2021-11-03 23:23 UTC (permalink / raw)
To: Simon Glass, Pali Rohár; +Cc: u-boot, 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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 01/12] env: Don't set ready flag if import failed in env_set_default()
2021-11-03 23:23 [PATCH v2 00/12] Board specific runtime determined default env Marek Behún
@ 2021-11-03 23:23 ` Marek Behún
2021-11-03 23:23 ` [PATCH v2 02/12] env: Fix env_get() when returning empty string using env_get_f() Marek Behún
` (13 subsequent siblings)
14 siblings, 0 replies; 33+ messages in thread
From: Marek Behún @ 2021-11-03 23:23 UTC (permalink / raw)
To: Simon Glass, Pali Rohár; +Cc: u-boot, 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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 02/12] env: Fix env_get() when returning empty string using env_get_f()
2021-11-03 23:23 [PATCH v2 00/12] Board specific runtime determined default env Marek Behún
2021-11-03 23:23 ` [PATCH v2 01/12] env: Don't set ready flag if import failed in env_set_default() Marek Behún
@ 2021-11-03 23:23 ` Marek Behún
2021-11-03 23:23 ` [PATCH v2 03/12] env: Simplify env_get_default() Marek Behún
` (12 subsequent siblings)
14 siblings, 0 replies; 33+ messages in thread
From: Marek Behún @ 2021-11-03 23:23 UTC (permalink / raw)
To: Simon Glass, Pali Rohár; +Cc: u-boot, 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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 03/12] env: Simplify env_get_default()
2021-11-03 23:23 [PATCH v2 00/12] Board specific runtime determined default env Marek Behún
2021-11-03 23:23 ` [PATCH v2 01/12] env: Don't set ready flag if import failed in env_set_default() Marek Behún
2021-11-03 23:23 ` [PATCH v2 02/12] env: Fix env_get() when returning empty string using env_get_f() Marek Behún
@ 2021-11-03 23:23 ` Marek Behún
2021-11-03 23:23 ` [PATCH v2 04/12] sysinfo: Make sysinfo_get_str() behave like snprintf() Marek Behún
` (11 subsequent siblings)
14 siblings, 0 replies; 33+ messages in thread
From: Marek Behún @ 2021-11-03 23:23 UTC (permalink / raw)
To: Simon Glass, Pali Rohár; +Cc: u-boot, 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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 04/12] sysinfo: Make sysinfo_get_str() behave like snprintf()
2021-11-03 23:23 [PATCH v2 00/12] Board specific runtime determined default env Marek Behún
` (2 preceding siblings ...)
2021-11-03 23:23 ` [PATCH v2 03/12] env: Simplify env_get_default() Marek Behún
@ 2021-11-03 23:23 ` Marek Behún
2021-11-05 2:02 ` Simon Glass
2021-11-03 23:23 ` [PATCH v2 05/12] test: Use ut_asserteq_str() instead of ut_assertok(strcmp()) Marek Behún
` (10 subsequent siblings)
14 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-11-03 23:23 UTC (permalink / raw)
To: Simon Glass, Pali Rohár; +Cc: u-boot, 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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 05/12] test: Use ut_asserteq_str() instead of ut_assertok(strcmp())
2021-11-03 23:23 [PATCH v2 00/12] Board specific runtime determined default env Marek Behún
` (3 preceding siblings ...)
2021-11-03 23:23 ` [PATCH v2 04/12] sysinfo: Make sysinfo_get_str() behave like snprintf() Marek Behún
@ 2021-11-03 23:23 ` Marek Behún
2021-11-05 2:02 ` Simon Glass
2021-11-03 23:23 ` [PATCH v2 06/12] sysinfo: Add get_str_list() method Marek Behún
` (9 subsequent siblings)
14 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-11-03 23:23 UTC (permalink / raw)
To: Simon Glass, Pali Rohár; +Cc: u-boot, 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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 06/12] sysinfo: Add get_str_list() method
2021-11-03 23:23 [PATCH v2 00/12] Board specific runtime determined default env Marek Behún
` (4 preceding siblings ...)
2021-11-03 23:23 ` [PATCH v2 05/12] test: Use ut_asserteq_str() instead of ut_assertok(strcmp()) Marek Behún
@ 2021-11-03 23:23 ` Marek Behún
2021-11-05 2:02 ` Simon Glass
2021-11-03 23:23 ` [PATCH v2 07/12] sysinfo: Make .detect() non-mandatory Marek Behún
` (8 subsequent siblings)
14 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-11-03 23:23 UTC (permalink / raw)
To: Simon Glass, Pali Rohár; +Cc: u-boot, 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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 07/12] sysinfo: Make .detect() non-mandatory
2021-11-03 23:23 [PATCH v2 00/12] Board specific runtime determined default env Marek Behún
` (5 preceding siblings ...)
2021-11-03 23:23 ` [PATCH v2 06/12] sysinfo: Add get_str_list() method Marek Behún
@ 2021-11-03 23:23 ` Marek Behún
2021-11-05 2:02 ` Simon Glass
2021-11-03 23:23 ` [PATCH v2 08/12] sysinfo: Add support for iterating string list Marek Behún
` (7 subsequent siblings)
14 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-11-03 23:23 UTC (permalink / raw)
To: Simon Glass, Pali Rohár; +Cc: u-boot, 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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 08/12] sysinfo: Add support for iterating string list
2021-11-03 23:23 [PATCH v2 00/12] Board specific runtime determined default env Marek Behún
` (6 preceding siblings ...)
2021-11-03 23:23 ` [PATCH v2 07/12] sysinfo: Make .detect() non-mandatory Marek Behún
@ 2021-11-03 23:23 ` Marek Behún
2021-11-05 2:02 ` Simon Glass
2021-11-03 23:23 ` [PATCH v2 09/12] env: Change return behaviour of env_set_default_vars() Marek Behún
` (6 subsequent siblings)
14 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-11-03 23:23 UTC (permalink / raw)
To: Simon Glass, Pali Rohár; +Cc: u-boot, 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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 09/12] env: Change return behaviour of env_set_default_vars()
2021-11-03 23:23 [PATCH v2 00/12] Board specific runtime determined default env Marek Behún
` (7 preceding siblings ...)
2021-11-03 23:23 ` [PATCH v2 08/12] sysinfo: Add support for iterating string list Marek Behún
@ 2021-11-03 23:23 ` Marek Behún
2021-11-05 2:02 ` Simon Glass
2021-11-03 23:23 ` [PATCH v2 10/12] env: Add support for overwriting default environment via sysinfo Marek Behún
` (5 subsequent siblings)
14 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-11-03 23:23 UTC (permalink / raw)
To: Simon Glass, Pali Rohár; +Cc: u-boot, 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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 10/12] env: Add support for overwriting default environment via sysinfo
2021-11-03 23:23 [PATCH v2 00/12] Board specific runtime determined default env Marek Behún
` (8 preceding siblings ...)
2021-11-03 23:23 ` [PATCH v2 09/12] env: Change return behaviour of env_set_default_vars() Marek Behún
@ 2021-11-03 23:23 ` Marek Behún
2021-11-05 2:02 ` Simon Glass
2021-11-03 23:23 ` [PATCH v2 11/12] arm: mvebu: Espressobin: Use new API for setting default env at runtime Marek Behún
` (4 subsequent siblings)
14 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-11-03 23:23 UTC (permalink / raw)
To: Simon Glass, Pali Rohár; +Cc: u-boot, 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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 11/12] arm: mvebu: Espressobin: Use new API for setting default env at runtime
2021-11-03 23:23 [PATCH v2 00/12] Board specific runtime determined default env Marek Behún
` (9 preceding siblings ...)
2021-11-03 23:23 ` [PATCH v2 10/12] env: Add support for overwriting default environment via sysinfo Marek Behún
@ 2021-11-03 23:23 ` Marek Behún
2021-11-05 2:02 ` Simon Glass
2021-11-03 23:23 ` [PATCH v2 12/12] env: Remove support for read-write default_environment[] Marek Behún
` (3 subsequent siblings)
14 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-11-03 23:23 UTC (permalink / raw)
To: Simon Glass, Pali Rohár; +Cc: u-boot, 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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 12/12] env: Remove support for read-write default_environment[]
2021-11-03 23:23 [PATCH v2 00/12] Board specific runtime determined default env Marek Behún
` (10 preceding siblings ...)
2021-11-03 23:23 ` [PATCH v2 11/12] arm: mvebu: Espressobin: Use new API for setting default env at runtime Marek Behún
@ 2021-11-03 23:23 ` Marek Behún
2021-11-05 2:02 ` Simon Glass
2021-11-14 0:34 ` [PATCH v2 03/12] env: Simplify env_get_default() Simon Glass
` (2 subsequent siblings)
14 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-11-03 23:23 UTC (permalink / raw)
To: Simon Glass, Pali Rohár; +Cc: u-boot, 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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v2 04/12] sysinfo: Make sysinfo_get_str() behave like snprintf()
2021-11-03 23:23 ` [PATCH v2 04/12] sysinfo: Make sysinfo_get_str() behave like snprintf() Marek Behún
@ 2021-11-05 2:02 ` Simon Glass
2021-11-05 11:19 ` Marek Behún
0 siblings, 1 reply; 33+ messages in thread
From: Simon Glass @ 2021-11-05 2:02 UTC (permalink / raw)
To: Marek Behún; +Cc: Pali Rohár, u-boot, Marek Behún
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 05/12] test: Use ut_asserteq_str() instead of ut_assertok(strcmp())
2021-11-03 23:23 ` [PATCH v2 05/12] test: Use ut_asserteq_str() instead of ut_assertok(strcmp()) Marek Behún
@ 2021-11-05 2:02 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-11-05 2:02 UTC (permalink / raw)
To: Marek Behún; +Cc: Pali Rohár, u-boot, Marek Behún
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>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 06/12] sysinfo: Add get_str_list() method
2021-11-03 23:23 ` [PATCH v2 06/12] sysinfo: Add get_str_list() method Marek Behún
@ 2021-11-05 2:02 ` Simon Glass
2021-11-05 11:20 ` Marek Behún
0 siblings, 1 reply; 33+ messages in thread
From: Simon Glass @ 2021-11-05 2:02 UTC (permalink / raw)
To: Marek Behún; +Cc: Pali Rohár, u-boot, Marek Behún
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.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 07/12] sysinfo: Make .detect() non-mandatory
2021-11-03 23:23 ` [PATCH v2 07/12] sysinfo: Make .detect() non-mandatory Marek Behún
@ 2021-11-05 2:02 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-11-05 2:02 UTC (permalink / raw)
To: Marek Behún; +Cc: Pali Rohár, u-boot, Marek Behún
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
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 08/12] sysinfo: Add support for iterating string list
2021-11-03 23:23 ` [PATCH v2 08/12] sysinfo: Add support for iterating string list Marek Behún
@ 2021-11-05 2:02 ` Simon Glass
2021-11-05 11:24 ` Marek Behún
0 siblings, 1 reply; 33+ messages in thread
From: Simon Glass @ 2021-11-05 2:02 UTC (permalink / raw)
To: Marek Behún; +Cc: Pali Rohár, u-boot, Marek Behún
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 09/12] env: Change return behaviour of env_set_default_vars()
2021-11-03 23:23 ` [PATCH v2 09/12] env: Change return behaviour of env_set_default_vars() Marek Behún
@ 2021-11-05 2:02 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-11-05 2:02 UTC (permalink / raw)
To: Marek Behún; +Cc: Pali Rohár, u-boot, Marek Behún
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>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 10/12] env: Add support for overwriting default environment via sysinfo
2021-11-03 23:23 ` [PATCH v2 10/12] env: Add support for overwriting default environment via sysinfo Marek Behún
@ 2021-11-05 2:02 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-11-05 2:02 UTC (permalink / raw)
To: Marek Behún; +Cc: Pali Rohár, u-boot, Marek Behún
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 11/12] arm: mvebu: Espressobin: Use new API for setting default env at runtime
2021-11-03 23:23 ` [PATCH v2 11/12] arm: mvebu: Espressobin: Use new API for setting default env at runtime Marek Behún
@ 2021-11-05 2:02 ` Simon Glass
2021-11-05 8:50 ` Pali Rohár
0 siblings, 1 reply; 33+ messages in thread
From: Simon Glass @ 2021-11-05 2:02 UTC (permalink / raw)
To: Marek Behún; +Cc: Pali Rohár, u-boot, Marek Behún
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>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 12/12] env: Remove support for read-write default_environment[]
2021-11-03 23:23 ` [PATCH v2 12/12] env: Remove support for read-write default_environment[] Marek Behún
@ 2021-11-05 2:02 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-11-05 2:02 UTC (permalink / raw)
To: Marek Behún; +Cc: Pali Rohár, u-boot, Marek Behún
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>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 11/12] arm: mvebu: Espressobin: Use new API for setting default env at runtime
2021-11-05 2:02 ` Simon Glass
@ 2021-11-05 8:50 ` Pali Rohár
2021-11-09 15:37 ` Pali Rohár
0 siblings, 1 reply; 33+ messages in thread
From: Pali Rohár @ 2021-11-05 8:50 UTC (permalink / raw)
To: Simon Glass; +Cc: Marek Behún, u-boot, Marek Behún
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.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 04/12] sysinfo: Make sysinfo_get_str() behave like snprintf()
2021-11-05 2:02 ` Simon Glass
@ 2021-11-05 11:19 ` Marek Behún
2021-11-05 16:12 ` Simon Glass
0 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-11-05 11:19 UTC (permalink / raw)
To: Simon Glass; +Cc: Pali Rohár, u-boot, 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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 06/12] sysinfo: Add get_str_list() method
2021-11-05 2:02 ` Simon Glass
@ 2021-11-05 11:20 ` Marek Behún
2021-11-05 16:12 ` Simon Glass
0 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-11-05 11:20 UTC (permalink / raw)
To: Simon Glass; +Cc: Pali Rohár, u-boot, 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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 08/12] sysinfo: Add support for iterating string list
2021-11-05 2:02 ` Simon Glass
@ 2021-11-05 11:24 ` Marek Behún
2021-11-05 16:12 ` Simon Glass
0 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-11-05 11:24 UTC (permalink / raw)
To: Simon Glass; +Cc: Pali Rohár, u-boot, 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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 06/12] sysinfo: Add get_str_list() method
2021-11-05 11:20 ` Marek Behún
@ 2021-11-05 16:12 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-11-05 16:12 UTC (permalink / raw)
To: Marek Behún; +Cc: Pali Rohár, u-boot, Marek Behún
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 04/12] sysinfo: Make sysinfo_get_str() behave like snprintf()
2021-11-05 11:19 ` Marek Behún
@ 2021-11-05 16:12 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-11-05 16:12 UTC (permalink / raw)
To: Marek Behún; +Cc: Pali Rohár, u-boot, Marek Behún
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 08/12] sysinfo: Add support for iterating string list
2021-11-05 11:24 ` Marek Behún
@ 2021-11-05 16:12 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-11-05 16:12 UTC (permalink / raw)
To: Marek Behún; +Cc: Pali Rohár, u-boot, Marek Behún
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 11/12] arm: mvebu: Espressobin: Use new API for setting default env at runtime
2021-11-05 8:50 ` Pali Rohár
@ 2021-11-09 15:37 ` Pali Rohár
0 siblings, 0 replies; 33+ messages in thread
From: Pali Rohár @ 2021-11-09 15:37 UTC (permalink / raw)
To: Simon Glass; +Cc: Marek Behún, u-boot, Marek Behún
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
=>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 03/12] env: Simplify env_get_default()
2021-11-03 23:23 [PATCH v2 00/12] Board specific runtime determined default env Marek Behún
` (11 preceding siblings ...)
2021-11-03 23:23 ` [PATCH v2 12/12] env: Remove support for read-write default_environment[] Marek Behún
@ 2021-11-14 0:34 ` Simon Glass
2021-11-14 0:34 ` [PATCH v2 02/12] env: Fix env_get() when returning empty string using env_get_f() Simon Glass
2021-11-14 0:34 ` [PATCH v2 01/12] env: Don't set ready flag if import failed in env_set_default() Simon Glass
14 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-11-14 0:34 UTC (permalink / raw)
To: Marek Behún; +Cc: u-boot, Marek Behún, Simon Glass, Pali Rohár
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!
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 02/12] env: Fix env_get() when returning empty string using env_get_f()
2021-11-03 23:23 [PATCH v2 00/12] Board specific runtime determined default env Marek Behún
` (12 preceding siblings ...)
2021-11-14 0:34 ` [PATCH v2 03/12] env: Simplify env_get_default() Simon Glass
@ 2021-11-14 0:34 ` Simon Glass
2021-11-14 0:34 ` [PATCH v2 01/12] env: Don't set ready flag if import failed in env_set_default() Simon Glass
14 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-11-14 0:34 UTC (permalink / raw)
To: Marek Behún; +Cc: u-boot, Marek Behún, Simon Glass, Pali Rohár
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!
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 01/12] env: Don't set ready flag if import failed in env_set_default()
2021-11-03 23:23 [PATCH v2 00/12] Board specific runtime determined default env Marek Behún
` (13 preceding siblings ...)
2021-11-14 0:34 ` [PATCH v2 02/12] env: Fix env_get() when returning empty string using env_get_f() Simon Glass
@ 2021-11-14 0:34 ` Simon Glass
14 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-11-14 0:34 UTC (permalink / raw)
To: Marek Behún; +Cc: u-boot, Marek Behún, Simon Glass, Pali Rohár
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!
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2021-11-14 0:35 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-03 23:23 [PATCH v2 00/12] Board specific runtime determined default env Marek Behún
2021-11-03 23:23 ` [PATCH v2 01/12] env: Don't set ready flag if import failed in env_set_default() Marek Behún
2021-11-03 23:23 ` [PATCH v2 02/12] env: Fix env_get() when returning empty string using env_get_f() Marek Behún
2021-11-03 23:23 ` [PATCH v2 03/12] env: Simplify env_get_default() Marek Behún
2021-11-03 23:23 ` [PATCH v2 04/12] sysinfo: Make sysinfo_get_str() behave like snprintf() Marek Behún
2021-11-05 2:02 ` Simon Glass
2021-11-05 11:19 ` Marek Behún
2021-11-05 16:12 ` Simon Glass
2021-11-03 23:23 ` [PATCH v2 05/12] test: Use ut_asserteq_str() instead of ut_assertok(strcmp()) Marek Behún
2021-11-05 2:02 ` Simon Glass
2021-11-03 23:23 ` [PATCH v2 06/12] sysinfo: Add get_str_list() method Marek Behún
2021-11-05 2:02 ` Simon Glass
2021-11-05 11:20 ` Marek Behún
2021-11-05 16:12 ` Simon Glass
2021-11-03 23:23 ` [PATCH v2 07/12] sysinfo: Make .detect() non-mandatory Marek Behún
2021-11-05 2:02 ` Simon Glass
2021-11-03 23:23 ` [PATCH v2 08/12] sysinfo: Add support for iterating string list Marek Behún
2021-11-05 2:02 ` Simon Glass
2021-11-05 11:24 ` Marek Behún
2021-11-05 16:12 ` Simon Glass
2021-11-03 23:23 ` [PATCH v2 09/12] env: Change return behaviour of env_set_default_vars() Marek Behún
2021-11-05 2:02 ` Simon Glass
2021-11-03 23:23 ` [PATCH v2 10/12] env: Add support for overwriting default environment via sysinfo Marek Behún
2021-11-05 2:02 ` Simon Glass
2021-11-03 23:23 ` [PATCH v2 11/12] arm: mvebu: Espressobin: Use new API for setting default env at runtime Marek Behún
2021-11-05 2:02 ` Simon Glass
2021-11-05 8:50 ` Pali Rohár
2021-11-09 15:37 ` Pali Rohár
2021-11-03 23:23 ` [PATCH v2 12/12] env: Remove support for read-write default_environment[] Marek Behún
2021-11-05 2:02 ` Simon Glass
2021-11-14 0:34 ` [PATCH v2 03/12] env: Simplify env_get_default() Simon Glass
2021-11-14 0:34 ` [PATCH v2 02/12] env: Fix env_get() when returning empty string using env_get_f() Simon Glass
2021-11-14 0:34 ` [PATCH v2 01/12] env: Don't set ready flag if import failed in env_set_default() Simon Glass
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.