| Message ID | 20210225143118.1.Ib1a20614bbab61d1c9ef8a1df528f10ce98c5bcc@changeid |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | RFC: spl: fit: Use libfdt functions to read stringlist | expand |
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>
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;
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;
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(-)