[PATCH] RFC: spl: fit: Use libfdt functions to read stringlist

[PATCH] RFC: spl: fit: Use libfdt functions to read stringlist

[Simon Glass]

At present the code here reimplements a few libfdt functions and does not
always respect the property length. The !str check is unlikely to fire
since 1 is added to the string address. If strchr() returns NULL then the
code produces (void*)1 instead. Also it might extend beyond the property
value since strchr() does not have a maximum length.

In any case it does not seem worthwhile to implement the libfdt functions
again, despite small code-size advantages. There is no function to return
the count after a failed get, but we can call two functions. We could add
one if code size is considered critical here.

Update the code to use libfdt directly.

For lion-rk3368 (aarch64) this adds 68 bytes of code.
For am57xx_hs_evm (arm) it adds 134 bytes.

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

 common/spl/spl_fit.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 75c8ff065bb..3b5307e4b2d 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -83,33 +83,24 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
 				  const char **outname)
 {
 	struct udevice *sysinfo;
-	const char *name, *str;
-	__maybe_unused int node;
-	int len, i;
-	bool found = true;
-
-	name = fdt_getprop(ctx->fit, ctx->conf_node, type, &len);
-	if (!name) {
-		debug("cannot find property '%s': %d\n", type, len);
-		return -EINVAL;
-	}
+	const char *str;
+	int count;
+	bool found;
 
-	str = name;
-	for (i = 0; i < index; i++) {
-		str = strchr(str, '\0') + 1;
-		if (!str || (str - name >= len)) {
-			found = false;
-			break;
-		}
-	}
+	count = fdt_stringlist_count(ctx->fit, ctx->conf_node, type);
+	str = fdt_stringlist_get(ctx->fit, ctx->conf_node, type, index, NULL);
+	found = str;
 
 	if (!found && CONFIG_IS_ENABLED(SYSINFO) && !sysinfo_get(&sysinfo)) {
 		int rc;
 		/*
 		 * no string in the property for this index. Check if the
-		 * sysinfo-level code can supply one.
+		 * sysinfo-level code can supply one. Subtract the number of
+		 * strings found in the devicetre node, so that @index numbers
+		 * the options in order from 0, starting with the devicetree
+		 * property and ending with sysinfo.
 		 */
-		rc = sysinfo_get_fit_loadable(sysinfo, index - i - 1, type,
+		rc = sysinfo_get_fit_loadable(sysinfo, index - count, type,
 					      &str);
 		if (rc && rc != -ENOENT)
 			return rc;
-- 
2.30.1.766.gb4fecdf3b7-goog

[PATCH] RFC: spl: fit: Use libfdt functions to read stringlist

[Tom Rini]

On Thu, Feb 25, 2021 at 02:31:23PM -0500, Simon Glass wrote:

> At present the code here reimplements a few libfdt functions and does not
> always respect the property length. The !str check is unlikely to fire
> since 1 is added to the string address. If strchr() returns NULL then the
> code produces (void*)1 instead. Also it might extend beyond the property
> value since strchr() does not have a maximum length.
> 
> In any case it does not seem worthwhile to implement the libfdt functions
> again, despite small code-size advantages. There is no function to return
> the count after a failed get, but we can call two functions. We could add
> one if code size is considered critical here.
> 
> Update the code to use libfdt directly.
> 
> For lion-rk3368 (aarch64) this adds 68 bytes of code.
> For am57xx_hs_evm (arm) it adds 134 bytes.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210226/28ed1538/attachment.sig>

[PATCH] RFC: spl: fit: Use libfdt functions to read stringlist

[Andre Przywara]

On Thu, 25 Feb 2021 14:31:23 -0500
Simon Glass <sjg@chromium.org> wrote:

Hi Simon,

> At present the code here reimplements a few libfdt functions and does not
> always respect the property length. The !str check is unlikely to fire
> since 1 is added to the string address. If strchr() returns NULL then the
> code produces (void*)1 instead. Also it might extend beyond the property
> value since strchr() does not have a maximum length.
> 
> In any case it does not seem worthwhile to implement the libfdt functions
> again, despite small code-size advantages. There is no function to return
> the count after a failed get, but we can call two functions. We could add
> one if code size is considered critical here.
> 
> Update the code to use libfdt directly.
> 
> For lion-rk3368 (aarch64) this adds 68 bytes of code.
> For am57xx_hs_evm (arm) it adds 134 bytes.

So for the 64-bit sunxi boards I get 254 bytes more, tested with
various boards, on GCC 7.5.0, GCC 9.2.0 and GCC 10.2.0.
So it's not a dealbreaker yet, but get it's a lot closer to the
limit: for the Pine H64 it's 31240 bytes now. I think we need some
slack before 32KB (for the stack? some buffer?), need to look up the
details.

So I agree with your rationale of not reinventing the wheel and fixing
the bugs on the way, but can you elaborate on your suggestion in the
last paragraph? Do you mean to add a function to libfdt, that combines
count&get, and somehow returns the count even when no string is found?

Don't we need the count just in the if (CONFIG_SYSINFO) clause, which
sunxi doesn't define? So could move the call into there? That seems to
cut 150 bytes off, if I am not mistaken?

Cheers,
Andre
 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  common/spl/spl_fit.c | 31 +++++++++++--------------------
>  1 file changed, 11 insertions(+), 20 deletions(-)
> 
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index 75c8ff065bb..3b5307e4b2d 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -83,33 +83,24 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
>  				  const char **outname)
>  {
>  	struct udevice *sysinfo;
> -	const char *name, *str;
> -	__maybe_unused int node;
> -	int len, i;
> -	bool found = true;
> -
> -	name = fdt_getprop(ctx->fit, ctx->conf_node, type, &len);
> -	if (!name) {
> -		debug("cannot find property '%s': %d\n", type, len);
> -		return -EINVAL;
> -	}
> +	const char *str;
> +	int count;
> +	bool found;
>  
> -	str = name;
> -	for (i = 0; i < index; i++) {
> -		str = strchr(str, '\0') + 1;
> -		if (!str || (str - name >= len)) {
> -			found = false;
> -			break;
> -		}
> -	}
> +	count = fdt_stringlist_count(ctx->fit, ctx->conf_node, type);
> +	str = fdt_stringlist_get(ctx->fit, ctx->conf_node, type, index, NULL);
> +	found = str;
>  
>  	if (!found && CONFIG_IS_ENABLED(SYSINFO) && !sysinfo_get(&sysinfo)) {
>  		int rc;
>  		/*
>  		 * no string in the property for this index. Check if the
> -		 * sysinfo-level code can supply one.
> +		 * sysinfo-level code can supply one. Subtract the number of
> +		 * strings found in the devicetre node, so that @index numbers
> +		 * the options in order from 0, starting with the devicetree
> +		 * property and ending with sysinfo.
>  		 */
> -		rc = sysinfo_get_fit_loadable(sysinfo, index - i - 1, type,
> +		rc = sysinfo_get_fit_loadable(sysinfo, index - count, type,
>  					      &str);
>  		if (rc && rc != -ENOENT)
>  			return rc;