Re: [u-boot][PATCH] spl: spl_legacy: Fix NAND boot on OMAP3 BeagleBoard

[Roger Quadros <rogerq@kernel.org>]

+Dario

On 29/09/2022 13:11, Roger Quadros wrote:
> OMAP3 BeagleBoard NAND boot hangs when spl_load_legacy_img() tries
> to read the header into 'struct hdr' which is allocated on the
> stack.
> 
> As the header has already been read once before by spl_nand.c,
> we can avoid the extra header read here by simply passing around
> the pointer to the header.
> 
> This fixes NAND boot on OMAP3 BeagleBoard.
> 
> Signed-off-by: Roger Quadros <rogerq@kernel.org>
> ---
>  common/spl/spl_legacy.c | 19 ++++++++-----------
>  common/spl/spl_nand.c   |  2 +-
>  common/spl/spl_nor.c    |  6 +++++-
>  include/spl.h           |  7 +++++--
>  4 files changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c
> index ae8731c782..2e9226c990 100644
> --- a/common/spl/spl_legacy.c
> +++ b/common/spl/spl_legacy.c
> @@ -77,32 +77,29 @@ static inline int spl_image_get_comp(const struct image_header *hdr)
>  
>  int spl_load_legacy_img(struct spl_image_info *spl_image,
>  			struct spl_boot_device *bootdev,
> -			struct spl_load_info *load, ulong header)
> +			struct spl_load_info *load, ulong offset,
> +			struct image_header *hdr)
>  {
>  	__maybe_unused SizeT lzma_len;
>  	__maybe_unused void *src;
> -	struct image_header hdr;
>  	ulong dataptr;
>  	int ret;
>  
> -	/* Read header into local struct */
> -	load->read(load, header, sizeof(hdr), &hdr);
> -
>  	/*
>  	 * If the payload is compressed, the decompressed data should be
>  	 * directly write to its load address.
>  	 */
> -	if (spl_image_get_comp(&hdr) != IH_COMP_NONE)
> +	if (spl_image_get_comp(hdr) != IH_COMP_NONE)
>  		spl_image->flags |= SPL_COPY_PAYLOAD_ONLY;
>  
> -	ret = spl_parse_image_header(spl_image, bootdev, &hdr);
> +	ret = spl_parse_image_header(spl_image, bootdev, hdr);
>  	if (ret)
>  		return ret;
>  
>  	/* Read image */
> -	switch (spl_image_get_comp(&hdr)) {
> +	switch (spl_image_get_comp(hdr)) {
>  	case IH_COMP_NONE:
> -		dataptr = header;
> +		dataptr = offset;
>  
>  		/*
>  		 * Image header will be skipped only if SPL_COPY_PAYLOAD_ONLY
> @@ -119,7 +116,7 @@ int spl_load_legacy_img(struct spl_image_info *spl_image,
>  		lzma_len = LZMA_LEN;
>  
>  		/* dataptr points to compressed payload  */
> -		dataptr = header + sizeof(hdr);
> +		dataptr = offset + sizeof(hdr);
>  
>  		debug("LZMA: Decompressing %08lx to %08lx\n",
>  		      dataptr, spl_image->load_addr);
> @@ -143,7 +140,7 @@ int spl_load_legacy_img(struct spl_image_info *spl_image,
>  
>  	default:
>  		debug("Compression method %s is not supported\n",
> -		      genimg_get_comp_short_name(image_get_comp(&hdr)));
> +		      genimg_get_comp_short_name(image_get_comp(hdr)));
>  		return -EINVAL;
>  	}
>  
> diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
> index 7b7579a2df..5eb67b5468 100644
> --- a/common/spl/spl_nand.c
> +++ b/common/spl/spl_nand.c
> @@ -119,7 +119,7 @@ static int spl_nand_load_element(struct spl_image_info *spl_image,
>  		load.bl_len = 1;
>  		load.read = spl_nand_legacy_read;
>  
> -		return spl_load_legacy_img(spl_image, bootdev, &load, offset);
> +		return spl_load_legacy_img(spl_image, bootdev, &load, offset, header);
>  	} else {
>  		err = spl_parse_image_header(spl_image, bootdev, header);
>  		if (err)
> diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
> index 7986e930d2..f00a5c395b 100644
> --- a/common/spl/spl_nor.c
> +++ b/common/spl/spl_nor.c
> @@ -111,10 +111,14 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
>  
>  	/* Legacy image handling */
>  	if (IS_ENABLED(CONFIG_SPL_LEGACY_IMAGE_FORMAT)) {
> +		struct image_header hdr;
> +
>  		load.bl_len = 1;
>  		load.read = spl_nor_load_read;
> +		spl_nor_load_read(&load, spl_nor_get_uboot_base(), sizeof(hdr), &hdr);
>  		return spl_load_legacy_img(spl_image, bootdev, &load,
> -					   spl_nor_get_uboot_base());
> +					   spl_nor_get_uboot_base(),
> +					   &hdr);
>  	}
>  
>  	return 0;
> diff --git a/include/spl.h b/include/spl.h
> index aac6648f94..7fa5e51c39 100644
> --- a/include/spl.h
> +++ b/include/spl.h
> @@ -353,7 +353,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>   * spl_load_legacy_img() - Loads a legacy image from a device.
>   * @spl_image:	Image description to set up
>   * @load:	Structure containing the information required to load data.
> - * @header:	Pointer to image header (including appended image)
> + * @offset:	Pointer to image
> + * @hdr:	Pointer to image header
>   *
>   * Reads an legacy image from the device. Loads u-boot image to
>   * specified load address.
> @@ -361,7 +362,9 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>   */
>  int spl_load_legacy_img(struct spl_image_info *spl_image,
>  			struct spl_boot_device *bootdev,
> -			struct spl_load_info *load, ulong header);
> +			struct spl_load_info *load, ulong offset,
> +			struct image_header *hdr);
> +
>  
>  /**
>   * spl_load_imx_container() - Loads a imx container image from a device.