[PATCH 1/2] cmd: fdt: Fix iteration over elements above index 2 in fdt get

[PATCH 1/2] cmd: fdt: Fix iteration over elements above index 2 in fdt get

[Marek Vasut]

Always increment both the iterator and pointer into the string
property value by length of the current element + 1 (to cater
for the string delimiter), otherwise the element extracted from
the string property value would be extracted from an offset that
is multiple of the length of the first element, instead of sum
of element lengths until select index.

This fixes 'fdt get value' operation for index above 2.

Fixes: 13982ced2cc ("cmd: fdt: Add support for reading stringlist property values")
Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
 cmd/fdt.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/cmd/fdt.c b/cmd/fdt.c
index 4b2dcfec863..8e51a431261 100644
--- a/cmd/fdt.c
+++ b/cmd/fdt.c
@@ -60,11 +60,14 @@ static int fdt_value_env_set(const void *nodep, int len,
 		 * Iterate over all members in stringlist and find the one at
 		 * offset $index. If no such index exists, indicate failure.
 		 */
-		for (i = 0; i < len; i += strlen(nodec) + 1) {
-			if (index-- > 0)
+		for (i = 0; i < len; ) {
+			if (index-- > 0) {
+				i += strlen(nodec) + 1;
+				nodec += strlen(nodec) + 1;
 				continue;
+			}
 
-			env_set(var, nodec + i);
+			env_set(var, nodec);
 			return 0;
 		}
 
-- 
2.35.1

[PATCH 2/2] test: cmd: fdt: Add fdt get value test case

[Marek Vasut]

Add test case for 'fdt get value' sub command.

The test case can be triggered using:
"
./u-boot -d u-boot.dtb -c 'ut fdt'
"

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
 test/cmd/fdt.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/test/cmd/fdt.c b/test/cmd/fdt.c
index ba9eaa42c14..7974c88c0d6 100644
--- a/test/cmd/fdt.c
+++ b/test/cmd/fdt.c
@@ -142,6 +142,59 @@ static int fdt_test_resize(struct unit_test_state *uts)
 }
 FDT_TEST(fdt_test_resize, UT_TESTF_CONSOLE_REC);
 
+/* Test 'fdt get' reading an fdt */
+static int fdt_test_get(struct unit_test_state *uts)
+{
+	ulong addr;
+
+	addr = map_to_sysmem(gd->fdt_blob);
+	set_working_fdt_addr(addr);
+
+	/* Test getting default element of /clk-test node clock-names property */
+	ut_assertok(console_record_reset_enable());
+	ut_assertok(run_command("fdt get value fdflt /clk-test clock-names", 0));
+	ut_asserteq_str("fixed", env_get("fdflt"));
+	ut_assertok(ut_check_console_end(uts));
+
+	/* Test getting 0th element of /clk-test node clock-names property */
+	ut_assertok(console_record_reset_enable());
+	ut_assertok(run_command("fdt get value fzero /clk-test clock-names 0", 0));
+	ut_asserteq_str("fixed", env_get("fzero"));
+	ut_assertok(ut_check_console_end(uts));
+
+	/* Test getting 1st element of /clk-test node clock-names property */
+	ut_assertok(console_record_reset_enable());
+	ut_assertok(run_command("fdt get value fone /clk-test clock-names 1", 0));
+	ut_asserteq_str("i2c", env_get("fone"));
+	ut_assertok(ut_check_console_end(uts));
+
+	/* Test getting 2nd element of /clk-test node clock-names property */
+	ut_assertok(console_record_reset_enable());
+	ut_assertok(run_command("fdt get value ftwo /clk-test clock-names 2", 0));
+	ut_asserteq_str("spi", env_get("ftwo"));
+	ut_assertok(ut_check_console_end(uts));
+
+	/* Test missing 10th element of /clk-test node clock-names property */
+	ut_assertok(console_record_reset_enable());
+	ut_asserteq(1, run_command("fdt get value ftwo /clk-test clock-names 10", 0));
+	ut_assertok(ut_check_console_end(uts));
+
+	/* Test getting default element of /clk-test node nonexistent property */
+	ut_assertok(console_record_reset_enable());
+	ut_asserteq(1, run_command("fdt get value fnone /clk-test nonexistent", 1));
+	ut_assert_nextline("libfdt fdt_getprop(): FDT_ERR_NOTFOUND");
+	ut_assertok(ut_check_console_end(uts));
+
+	/* Test getting default element of /nonexistent node */
+	ut_assertok(console_record_reset_enable());
+	ut_asserteq(1, run_command("fdt get value fnode /nonexistent nonexistent", 1));
+	ut_assert_nextline("libfdt fdt_path_offset() returned FDT_ERR_NOTFOUND");
+	ut_assertok(ut_check_console_end(uts));
+
+	return 0;
+}
+FDT_TEST(fdt_test_get, UT_TESTF_CONSOLE_REC);
+
 int do_ut_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	struct unit_test *tests = UNIT_TEST_SUITE_START(fdt_test);
-- 
2.35.1

Re: [PATCH 1/2] cmd: fdt: Fix iteration over elements above index 2 in fdt get

[Heinrich Schuchardt]

On 11/13/22 21:45, Marek Vasut wrote:
> Always increment both the iterator and pointer into the string
> property value by length of the current element + 1 (to cater
> for the string delimiter), otherwise the element extracted from
> the string property value would be extracted from an offset that
> is multiple of the length of the first element, instead of sum
> of element lengths until select index.
> 
> This fixes 'fdt get value' operation for index above 2.

As '0' is the first index: %s/above 2/above 1/

Acked-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

> 
> Fixes: 13982ced2cc ("cmd: fdt: Add support for reading stringlist property values")
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> ---
>   cmd/fdt.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/cmd/fdt.c b/cmd/fdt.c
> index 4b2dcfec863..8e51a431261 100644
> --- a/cmd/fdt.c
> +++ b/cmd/fdt.c
> @@ -60,11 +60,14 @@ static int fdt_value_env_set(const void *nodep, int len,
>   		 * Iterate over all members in stringlist and find the one at
>   		 * offset $index. If no such index exists, indicate failure.
>   		 */
> -		for (i = 0; i < len; i += strlen(nodec) + 1) {
> -			if (index-- > 0)
> +		for (i = 0; i < len; ) {
> +			if (index-- > 0) {
> +				i += strlen(nodec) + 1;
> +				nodec += strlen(nodec) + 1;
>   				continue;
> +			}
>   
> -			env_set(var, nodec + i);
> +			env_set(var, nodec);
>   			return 0;
>   		}
>   

Re: [PATCH 2/2] test: cmd: fdt: Add fdt get value test case

[Heinrich Schuchardt]

On 11/13/22 21:45, Marek Vasut wrote:
> Add test case for 'fdt get value' sub command.
> 
> The test case can be triggered using:
> "
> ./u-boot -d u-boot.dtb -c 'ut fdt'
> "
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> ---
>   test/cmd/fdt.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 53 insertions(+)
> 
> diff --git a/test/cmd/fdt.c b/test/cmd/fdt.c
> index ba9eaa42c14..7974c88c0d6 100644
> --- a/test/cmd/fdt.c
> +++ b/test/cmd/fdt.c
> @@ -142,6 +142,59 @@ static int fdt_test_resize(struct unit_test_state *uts)
>   }
>   FDT_TEST(fdt_test_resize, UT_TESTF_CONSOLE_REC);
>   
> +/* Test 'fdt get' reading an fdt */
> +static int fdt_test_get(struct unit_test_state *uts)
> +{
> +	ulong addr;
> +
> +	addr = map_to_sysmem(gd->fdt_blob);
> +	set_working_fdt_addr(addr);
> +
> +	/* Test getting default element of /clk-test node clock-names property */
> +	ut_assertok(console_record_reset_enable());
> +	ut_assertok(run_command("fdt get value fdflt /clk-test clock-names", 0));

The command 'fdt get value' is missing in doc/usage/cmd/fdt.rst.

Acked-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

> +	ut_asserteq_str("fixed", env_get("fdflt"));
> +	ut_assertok(ut_check_console_end(uts));
> +
> +	/* Test getting 0th element of /clk-test node clock-names property */
> +	ut_assertok(console_record_reset_enable());
> +	ut_assertok(run_command("fdt get value fzero /clk-test clock-names 0", 0));
> +	ut_asserteq_str("fixed", env_get("fzero"));
> +	ut_assertok(ut_check_console_end(uts));
> +
> +	/* Test getting 1st element of /clk-test node clock-names property */
> +	ut_assertok(console_record_reset_enable());
> +	ut_assertok(run_command("fdt get value fone /clk-test clock-names 1", 0));
> +	ut_asserteq_str("i2c", env_get("fone"));
> +	ut_assertok(ut_check_console_end(uts));
> +
> +	/* Test getting 2nd element of /clk-test node clock-names property */
> +	ut_assertok(console_record_reset_enable());
> +	ut_assertok(run_command("fdt get value ftwo /clk-test clock-names 2", 0));
> +	ut_asserteq_str("spi", env_get("ftwo"));
> +	ut_assertok(ut_check_console_end(uts));
> +
> +	/* Test missing 10th element of /clk-test node clock-names property */
> +	ut_assertok(console_record_reset_enable());
> +	ut_asserteq(1, run_command("fdt get value ftwo /clk-test clock-names 10", 0));
> +	ut_assertok(ut_check_console_end(uts));
> +
> +	/* Test getting default element of /clk-test node nonexistent property */
> +	ut_assertok(console_record_reset_enable());
> +	ut_asserteq(1, run_command("fdt get value fnone /clk-test nonexistent", 1));
> +	ut_assert_nextline("libfdt fdt_getprop(): FDT_ERR_NOTFOUND");
> +	ut_assertok(ut_check_console_end(uts));
> +
> +	/* Test getting default element of /nonexistent node */
> +	ut_assertok(console_record_reset_enable());
> +	ut_asserteq(1, run_command("fdt get value fnode /nonexistent nonexistent", 1));
> +	ut_assert_nextline("libfdt fdt_path_offset() returned FDT_ERR_NOTFOUND");
> +	ut_assertok(ut_check_console_end(uts));
> +
> +	return 0;
> +}
> +FDT_TEST(fdt_test_get, UT_TESTF_CONSOLE_REC);
> +
>   int do_ut_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>   {
>   	struct unit_test *tests = UNIT_TEST_SUITE_START(fdt_test);

Re: [PATCH 2/2] test: cmd: fdt: Add fdt get value test case

[Marek Vasut]

On 11/14/22 09:30, Heinrich Schuchardt wrote:
> On 11/13/22 21:45, Marek Vasut wrote:
>> Add test case for 'fdt get value' sub command.
>>
>> The test case can be triggered using:
>> "
>> ./u-boot -d u-boot.dtb -c 'ut fdt'
>> "
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Tom Rini <trini@konsulko.com>
>> ---
>>   test/cmd/fdt.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 53 insertions(+)
>>
>> diff --git a/test/cmd/fdt.c b/test/cmd/fdt.c
>> index ba9eaa42c14..7974c88c0d6 100644
>> --- a/test/cmd/fdt.c
>> +++ b/test/cmd/fdt.c
>> @@ -142,6 +142,59 @@ static int fdt_test_resize(struct unit_test_state 
>> *uts)
>>   }
>>   FDT_TEST(fdt_test_resize, UT_TESTF_CONSOLE_REC);
>> +/* Test 'fdt get' reading an fdt */
>> +static int fdt_test_get(struct unit_test_state *uts)
>> +{
>> +    ulong addr;
>> +
>> +    addr = map_to_sysmem(gd->fdt_blob);
>> +    set_working_fdt_addr(addr);
>> +
>> +    /* Test getting default element of /clk-test node clock-names 
>> property */
>> +    ut_assertok(console_record_reset_enable());
>> +    ut_assertok(run_command("fdt get value fdflt /clk-test 
>> clock-names", 0));
> 
> The command 'fdt get value' is missing in doc/usage/cmd/fdt.rst.

Ugh ... the entire 'fdt' command documentation except for 'fdt addr' is 
missing in that file.