[PATCH 1/7] spl: fit: Don't overwrite previous loadable if "load" is missing

[Simon Glass <sjg@chromium.org>]

Hi Alexandru,

On Wed, 10 Mar 2021 at 11:04, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> spl_load_fit_image() will try to load an image at the address given
> in the "load" property. Absent such property, it uses
>
>         image_info->load_addr
>
> Correct use of this is demonstrated in spl_fit_append_fdt(), which
> resets the 'load_addr' before each spl_load_fit_image() call.
>
> On the other hand loading "loadables" loop in spl_load_simple_fit()
> completely ignores this. It re-uses the same structure, but doesn't
> reset load_addr. If loadable [i] does not have a "load" property, its
> load address defaults to load_addr, which still contains the address
> of loadable [i - 1].
>
> A simple solution is to treat NULL as an invalid load address. The
> caller can set load_addr = 0 to request an abort if the "load"
> property is absent.

Another way would be to add a load_valid bool or flag to the struct.

>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  common/spl/spl_fit.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index 75c8ff065b..43fc43cf2b 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -223,7 +223,7 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size,
>   * @image_info:        will be filled with information about the loaded image
>   *             If the FIT node does not contain a "load" (address) property,
>   *             the image gets loaded to the address pointed to by the
> - *             load_addr member in this struct.
> + *             load_addr member in this struct, if load_addr is not NULL

s/NULL/0/

This is not a pointer, but an address.

>   *
>   * Return:     0 on success or a negative error number.
>   */
> @@ -258,8 +258,14 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
>                 debug("%s ", genimg_get_comp_name(image_comp));
>         }
>
> -       if (fit_image_get_load(fit, node, &load_addr))
> +       if (fit_image_get_load(fit, node, &load_addr)) {
> +               if (!image_info->load_addr) {
> +                       printf("Can't load %s: No load address and no buffer\n",
> +                              fit_get_name(fit, node, NULL));
> +                       return -ENOBUFS;
> +               }
>                 load_addr = image_info->load_addr;
> +       }
>
>         if (!fit_image_get_data_position(fit, node, &offset)) {
>                 external_data = true;
> @@ -697,6 +703,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>                 if (firmware_node == node)
>                         continue;
>
> +               image_info.load_addr = 0;
>                 ret = spl_load_fit_image(info, sector, &ctx, node, &image_info);
>                 if (ret < 0) {
>                         printf("%s: can't load image loadables index %d (ret = %d)\n",
> --
> 2.26.2
>

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

Regards,
Simon