| Message ID | 1484877211-19332-2-git-send-email-andre.przywara@arm.com |
|---|---|
| State | RFC |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
On Friday 20 January 2017 07:23 AM, Andre Przywara wrote: > Currently the SPL FIT loader uses the spl_fit_select_fdt() function to > find the offset to the right DTB within the FIT image. > For this it iterates over all subnodes of the /configuration node in > the FIT tree and compares all "description" strings therein using a > board specific matching function. > If that finds a match, it uses the string in the "fdt" property of that > subnode to locate the matching subnode in the /images node, which points > to the DTB data. > Now this works very well, but is quite specific to cover this particular > use case. To open up the door for a more generic usage, let's split this > function into: > 1) a function that just returns the node offset for the matching > configuration node (spl_fit_find_config_node()) > 2) a function that returns the image data any given property in a given > configuration node points to, additionally using a given index into > a possbile list of strings (spl_fit_select_index()) > This allows us to replace the specific function above by asking for the > image the _first string of the "fdt" property_ in the matching > configuration subnode points to. > > This patch introduces no functional changes, it just refactors the code > to allow reusing it later. > > (diff is overly clever here and produces a hard-to-read patch, so I > recommend to throw a look at the result instead). > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> Reviewed-by: Lokesh Vutla <lokeshvuta@ti.com> Thanks and regards, Lokesh
Hi Andre, On 19 January 2017 at 18:53, Andre Przywara <andre.przywara@arm.com> wrote: > > Currently the SPL FIT loader uses the spl_fit_select_fdt() function to > find the offset to the right DTB within the FIT image. > For this it iterates over all subnodes of the /configuration node in > the FIT tree and compares all "description" strings therein using a > board specific matching function. > If that finds a match, it uses the string in the "fdt" property of that > subnode to locate the matching subnode in the /images node, which points > to the DTB data. > Now this works very well, but is quite specific to cover this particular > use case. To open up the door for a more generic usage, let's split this > function into: > 1) a function that just returns the node offset for the matching > configuration node (spl_fit_find_config_node()) > 2) a function that returns the image data any given property in a given > configuration node points to, additionally using a given index into > a possbile list of strings (spl_fit_select_index()) > This allows us to replace the specific function above by asking for the > image the _first string of the "fdt" property_ in the matching > configuration subnode points to. > > This patch introduces no functional changes, it just refactors the code > to allow reusing it later. > > (diff is overly clever here and produces a hard-to-read patch, so I > recommend to throw a look at the result instead). > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > common/spl/spl_fit.c | 82 ++++++++++++++++++++++++++++++++-------------------- > 1 file changed, 51 insertions(+), 31 deletions(-) > Reviewed-by: Simon Glass <sjg@chromium.org> Please check below. > diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c > index aae556f..c4e2f02 100644 > --- a/common/spl/spl_fit.c > +++ b/common/spl/spl_fit.c > @@ -22,13 +22,11 @@ static ulong fdt_getprop_u32(const void *fdt, int node, const char *prop) > return fdt32_to_cpu(*cell); > } > > -static int spl_fit_select_fdt(const void *fdt, int images, int *fdt_offsetp) > +static int spl_fit_find_config_node(const void *fdt) > { > - const char *name, *fdt_name; > - int conf, node, fdt_node; > - int len; > + const char *name; > + int conf, node, len; > > - *fdt_offsetp = 0; > conf = fdt_path_offset(fdt, FIT_CONFS_PATH); > if (conf < 0) { > debug("%s: Cannot find /configurations node: %d\n", __func__, > @@ -50,39 +48,60 @@ static int spl_fit_select_fdt(const void *fdt, int images, int *fdt_offsetp) > continue; > > debug("Selecting config '%s'", name); > - fdt_name = fdt_getprop(fdt, node, FIT_FDT_PROP, &len); > - if (!fdt_name) { > - debug("%s: Cannot find fdt name property: %d\n", > - __func__, len); > - return -EINVAL; > - } > > - debug(", fdt '%s'\n", fdt_name); > - fdt_node = fdt_subnode_offset(fdt, images, fdt_name); > - if (fdt_node < 0) { > - debug("%s: Cannot find fdt node '%s': %d\n", > - __func__, fdt_name, fdt_node); > - return -EINVAL; > + return node; > + } > + > + return -1; > +} > + > +static int spl_fit_select_index(const void *fit, int images, int *offsetp, > + const char *type, int index) > +{ > + const char *name, *img_name; > + int node, conf_node; > + int len, i; > + > + *offsetp = 0; > + conf_node = spl_fit_find_config_node(fit); > + if (conf_node < 0) { > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT > + printf("No matching DT out of these options:\n"); > + for (node = fdt_first_subnode(fit, conf_node); > + node >= 0; > + node = fdt_next_subnode(fit, node)) { > + name = fdt_getprop(fit, node, "description", &len); > + printf(" %s\n", name); > } > +#endif > + return -ENOENT; > + } > > - *fdt_offsetp = fdt_getprop_u32(fdt, fdt_node, "data-offset"); > - len = fdt_getprop_u32(fdt, fdt_node, "data-size"); > - debug("FIT: Selected '%s'\n", name); > + img_name = fdt_getprop(fit, conf_node, type, &len); > + if (!img_name) { > + debug("cannot find property '%s': %d\n", type, len); > + return -EINVAL; > + } > > - return len; > + for (i = 0; i < index; i++) { > + img_name = strchr(img_name, '\0') + 1; Don't you need to check against strchr() returning NULL? > + if (*img_name == '\0') { > + debug("no string for index %d\n", index); > + return -E2BIG; > + } > } > > -#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT > - printf("No matching DT out of these options:\n"); > - for (node = fdt_first_subnode(fdt, conf); > - node >= 0; > - node = fdt_next_subnode(fdt, node)) { > - name = fdt_getprop(fdt, node, "description", &len); > - printf(" %s\n", name); > + debug("%s: '%s'\n", type, img_name); > + node = fdt_subnode_offset(fit, images, img_name); > + if (node < 0) { > + debug("cannot find image node '%s': %d\n", img_name, node); > + return -EINVAL; > } > -#endif > > - return -ENOENT; > + *offsetp = fdt_getprop_u32(fit, node, "data-offset"); > + len = fdt_getprop_u32(fit, node, "data-size"); > + > + return len; > } > > static int get_aligned_image_offset(struct spl_load_info *info, int offset) > @@ -218,7 +237,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, > memcpy(dst, src, data_size); > > /* Figure out which device tree the board wants to use */ > - fdt_len = spl_fit_select_fdt(fit, images, &fdt_offset); > + fdt_len = spl_fit_select_index(fit, images, &fdt_offset, > + FIT_FDT_PROP, 0); > if (fdt_len < 0) > return fdt_len; > > -- > 2.8.2 > Regards, Simon
On 27/01/17 21:29, Simon Glass wrote: > Hi Andre, > > On 19 January 2017 at 18:53, Andre Przywara <andre.przywara@arm.com> wrote: >> >> Currently the SPL FIT loader uses the spl_fit_select_fdt() function to >> find the offset to the right DTB within the FIT image. >> For this it iterates over all subnodes of the /configuration node in >> the FIT tree and compares all "description" strings therein using a >> board specific matching function. >> If that finds a match, it uses the string in the "fdt" property of that >> subnode to locate the matching subnode in the /images node, which points >> to the DTB data. >> Now this works very well, but is quite specific to cover this particular >> use case. To open up the door for a more generic usage, let's split this >> function into: >> 1) a function that just returns the node offset for the matching >> configuration node (spl_fit_find_config_node()) >> 2) a function that returns the image data any given property in a given >> configuration node points to, additionally using a given index into >> a possbile list of strings (spl_fit_select_index()) >> This allows us to replace the specific function above by asking for the >> image the _first string of the "fdt" property_ in the matching >> configuration subnode points to. >> >> This patch introduces no functional changes, it just refactors the code >> to allow reusing it later. >> >> (diff is overly clever here and produces a hard-to-read patch, so I >> recommend to throw a look at the result instead). >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> common/spl/spl_fit.c | 82 ++++++++++++++++++++++++++++++++-------------------- >> 1 file changed, 51 insertions(+), 31 deletions(-) >> > > Reviewed-by: Simon Glass <sjg@chromium.org> Thanks a lot! > > Please check below. .... >> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c >> index aae556f..c4e2f02 100644 >> --- a/common/spl/spl_fit.c >> +++ b/common/spl/spl_fit.c >> @@ -22,13 +22,11 @@ static ulong fdt_getprop_u32(const void *fdt, int node, const char *prop) >> return fdt32_to_cpu(*cell); >> } >> >> -static int spl_fit_select_fdt(const void *fdt, int images, int *fdt_offsetp) >> +static int spl_fit_find_config_node(const void *fdt) >> { >> - const char *name, *fdt_name; >> - int conf, node, fdt_node; >> - int len; >> + const char *name; >> + int conf, node, len; >> >> - *fdt_offsetp = 0; >> conf = fdt_path_offset(fdt, FIT_CONFS_PATH); >> if (conf < 0) { >> debug("%s: Cannot find /configurations node: %d\n", __func__, >> @@ -50,39 +48,60 @@ static int spl_fit_select_fdt(const void *fdt, int images, int *fdt_offsetp) >> continue; >> >> debug("Selecting config '%s'", name); >> - fdt_name = fdt_getprop(fdt, node, FIT_FDT_PROP, &len); >> - if (!fdt_name) { >> - debug("%s: Cannot find fdt name property: %d\n", >> - __func__, len); >> - return -EINVAL; >> - } >> >> - debug(", fdt '%s'\n", fdt_name); >> - fdt_node = fdt_subnode_offset(fdt, images, fdt_name); >> - if (fdt_node < 0) { >> - debug("%s: Cannot find fdt node '%s': %d\n", >> - __func__, fdt_name, fdt_node); >> - return -EINVAL; >> + return node; >> + } >> + >> + return -1; >> +} >> + >> +static int spl_fit_select_index(const void *fit, int images, int *offsetp, >> + const char *type, int index) >> +{ >> + const char *name, *img_name; >> + int node, conf_node; >> + int len, i; >> + >> + *offsetp = 0; >> + conf_node = spl_fit_find_config_node(fit); >> + if (conf_node < 0) { >> +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT >> + printf("No matching DT out of these options:\n"); >> + for (node = fdt_first_subnode(fit, conf_node); >> + node >= 0; >> + node = fdt_next_subnode(fit, node)) { >> + name = fdt_getprop(fit, node, "description", &len); >> + printf(" %s\n", name); >> } >> +#endif >> + return -ENOENT; >> + } >> >> - *fdt_offsetp = fdt_getprop_u32(fdt, fdt_node, "data-offset"); >> - len = fdt_getprop_u32(fdt, fdt_node, "data-size"); >> - debug("FIT: Selected '%s'\n", name); >> + img_name = fdt_getprop(fit, conf_node, type, &len); >> + if (!img_name) { >> + debug("cannot find property '%s': %d\n", type, len); >> + return -EINVAL; >> + } >> >> - return len; >> + for (i = 0; i < index; i++) { >> + img_name = strchr(img_name, '\0') + 1; > > Don't you need to check against strchr() returning NULL? I think in a valid DT a string is always terminated by a NUL, but you are right that this code is broken. There is no double NUL at the end as I erroneously thought, instead we have to compare against the "len" value from above. Thanks for spotting this! Cheers, Andre. >> + if (*img_name == '\0') { >> + debug("no string for index %d\n", index); >> + return -E2BIG; >> + } >> } >> >> -#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT >> - printf("No matching DT out of these options:\n"); >> - for (node = fdt_first_subnode(fdt, conf); >> - node >= 0; >> - node = fdt_next_subnode(fdt, node)) { >> - name = fdt_getprop(fdt, node, "description", &len); >> - printf(" %s\n", name); >> + debug("%s: '%s'\n", type, img_name); >> + node = fdt_subnode_offset(fit, images, img_name); >> + if (node < 0) { >> + debug("cannot find image node '%s': %d\n", img_name, node); >> + return -EINVAL; >> } >> -#endif >> >> - return -ENOENT; >> + *offsetp = fdt_getprop_u32(fit, node, "data-offset"); >> + len = fdt_getprop_u32(fit, node, "data-size"); >> + >> + return len; >> } >> >> static int get_aligned_image_offset(struct spl_load_info *info, int offset) >> @@ -218,7 +237,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, >> memcpy(dst, src, data_size); >> >> /* Figure out which device tree the board wants to use */ >> - fdt_len = spl_fit_select_fdt(fit, images, &fdt_offset); >> + fdt_len = spl_fit_select_index(fit, images, &fdt_offset, >> + FIT_FDT_PROP, 0); >> if (fdt_len < 0) >> return fdt_len; >> >> -- >> 2.8.2 >> > > Regards, > Simon >
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index aae556f..c4e2f02 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -22,13 +22,11 @@ static ulong fdt_getprop_u32(const void *fdt, int node, const char *prop) return fdt32_to_cpu(*cell); } -static int spl_fit_select_fdt(const void *fdt, int images, int *fdt_offsetp) +static int spl_fit_find_config_node(const void *fdt) { - const char *name, *fdt_name; - int conf, node, fdt_node; - int len; + const char *name; + int conf, node, len; - *fdt_offsetp = 0; conf = fdt_path_offset(fdt, FIT_CONFS_PATH); if (conf < 0) { debug("%s: Cannot find /configurations node: %d\n", __func__, @@ -50,39 +48,60 @@ static int spl_fit_select_fdt(const void *fdt, int images, int *fdt_offsetp) continue; debug("Selecting config '%s'", name); - fdt_name = fdt_getprop(fdt, node, FIT_FDT_PROP, &len); - if (!fdt_name) { - debug("%s: Cannot find fdt name property: %d\n", - __func__, len); - return -EINVAL; - } - debug(", fdt '%s'\n", fdt_name); - fdt_node = fdt_subnode_offset(fdt, images, fdt_name); - if (fdt_node < 0) { - debug("%s: Cannot find fdt node '%s': %d\n", - __func__, fdt_name, fdt_node); - return -EINVAL; + return node; + } + + return -1; +} + +static int spl_fit_select_index(const void *fit, int images, int *offsetp, + const char *type, int index) +{ + const char *name, *img_name; + int node, conf_node; + int len, i; + + *offsetp = 0; + conf_node = spl_fit_find_config_node(fit); + if (conf_node < 0) { +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT + printf("No matching DT out of these options:\n"); + for (node = fdt_first_subnode(fit, conf_node); + node >= 0; + node = fdt_next_subnode(fit, node)) { + name = fdt_getprop(fit, node, "description", &len); + printf(" %s\n", name); } +#endif + return -ENOENT; + } - *fdt_offsetp = fdt_getprop_u32(fdt, fdt_node, "data-offset"); - len = fdt_getprop_u32(fdt, fdt_node, "data-size"); - debug("FIT: Selected '%s'\n", name); + img_name = fdt_getprop(fit, conf_node, type, &len); + if (!img_name) { + debug("cannot find property '%s': %d\n", type, len); + return -EINVAL; + } - return len; + for (i = 0; i < index; i++) { + img_name = strchr(img_name, '\0') + 1; + if (*img_name == '\0') { + debug("no string for index %d\n", index); + return -E2BIG; + } } -#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT - printf("No matching DT out of these options:\n"); - for (node = fdt_first_subnode(fdt, conf); - node >= 0; - node = fdt_next_subnode(fdt, node)) { - name = fdt_getprop(fdt, node, "description", &len); - printf(" %s\n", name); + debug("%s: '%s'\n", type, img_name); + node = fdt_subnode_offset(fit, images, img_name); + if (node < 0) { + debug("cannot find image node '%s': %d\n", img_name, node); + return -EINVAL; } -#endif - return -ENOENT; + *offsetp = fdt_getprop_u32(fit, node, "data-offset"); + len = fdt_getprop_u32(fit, node, "data-size"); + + return len; } static int get_aligned_image_offset(struct spl_load_info *info, int offset) @@ -218,7 +237,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, memcpy(dst, src, data_size); /* Figure out which device tree the board wants to use */ - fdt_len = spl_fit_select_fdt(fit, images, &fdt_offset); + fdt_len = spl_fit_select_index(fit, images, &fdt_offset, + FIT_FDT_PROP, 0); if (fdt_len < 0) return fdt_len;
Currently the SPL FIT loader uses the spl_fit_select_fdt() function to find the offset to the right DTB within the FIT image. For this it iterates over all subnodes of the /configuration node in the FIT tree and compares all "description" strings therein using a board specific matching function. If that finds a match, it uses the string in the "fdt" property of that subnode to locate the matching subnode in the /images node, which points to the DTB data. Now this works very well, but is quite specific to cover this particular use case. To open up the door for a more generic usage, let's split this function into: 1) a function that just returns the node offset for the matching configuration node (spl_fit_find_config_node()) 2) a function that returns the image data any given property in a given configuration node points to, additionally using a given index into a possbile list of strings (spl_fit_select_index()) This allows us to replace the specific function above by asking for the image the _first string of the "fdt" property_ in the matching configuration subnode points to. This patch introduces no functional changes, it just refactors the code to allow reusing it later. (diff is overly clever here and produces a hard-to-read patch, so I recommend to throw a look at the result instead). Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- common/spl/spl_fit.c | 82 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 51 insertions(+), 31 deletions(-)