[PATCH v2 00/12] Board specific runtime determined default env
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(-)
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;
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!
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;
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!
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)
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!
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;
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
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
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
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; }
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
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; }
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.
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
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
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;
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
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);
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
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
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
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);
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
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, };
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
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 */
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
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.
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 =>
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
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
participants (3)
-
Marek Behún -
Pali Rohár -
Simon Glass