[PATCH v5 05/11] spl: Convert mmc to spl_load

[Xavier Drudis Ferran]

El Mon, Jul 31, 2023 at 06:42:57PM -0400, Sean Anderson deia:
> This converts the mmc loader to spl_load. Legacy images are handled by
> spl_load (via spl_parse_image_header), so mmc_load_legacy can be
> omitted.
>

Yes. I haven't used the legacy case, but by looking at the code, it
seems to me that mmc_load_legacy left the image at some mapped memory
at [ spl_image->load_addr,   spl_image->load_addr + size )
and the new code does too, but when the image is not aligned, the
memory that gets written to
was at [ spl_image->load_addr,   spl_image->load_addr + size + spl_image->offset % mmc->read_bl_len )
and it will
be at [ spl_image->load_addr - spl_image->offset % mmc->read_bl_len,   spl_image->load_addr + size )
after this patch.

Meaning both with or without this patch some memory outside the image
gets written when the image is not aligned on media, but before it was
some part of a block at the end and now is that part before the
beginning.

Whether that's better or worse I don't know. It depends on whether
it's a problem to write in non mapped memory, whether there's
something worth preserving there, and whether some SOC has some sort
of special behaving memory just there, like it happened with the issue
Jerome Forissier found (note in this case it wasn't legacy, it was
simple fit)

https://patchwork.ozlabs.org/project/uboot/patch/c941d835a85255ff81a21c72069c3a9fc9a8a255.1656489154.git.jerome.forissier@linaro.org/
                                                                  
> Signed-off-by: Sean Anderson <sean.anderson at seco.com>
> ---
> 
> Changes in v5:
> - Rework to load header in spl_load
> 
>  common/spl/spl_mmc.c | 91 ++++----------------------------------------
>  1 file changed, 8 insertions(+), 83 deletions(-)
> 
> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> index a665091b00..c926a42c0f 100644
> --- a/common/spl/spl_mmc.c
> +++ b/common/spl/spl_mmc.c
> @@ -17,48 +17,6 @@
>  #include <mmc.h>
>  #include <image.h>
>  
> -static int mmc_load_legacy(struct spl_image_info *spl_image,
> -			   struct spl_boot_device *bootdev,
> -			   struct mmc *mmc,
> -			   ulong sector, struct legacy_img_hdr *header)
> -{
> -	u32 image_offset_sectors;
> -	u32 image_size_sectors;
> -	unsigned long count;
> -	u32 image_offset;
> -	int ret;
> -
> -	ret = spl_parse_image_header(spl_image, bootdev, header);
> -	if (ret)
> -		return ret;
> -
> -	/* convert offset to sectors - round down */
> -	image_offset_sectors = spl_image->offset / mmc->read_bl_len;
> -	/* calculate remaining offset */
> -	image_offset = spl_image->offset % mmc->read_bl_len;
> -
> -	/* convert size to sectors - round up */
> -	image_size_sectors = (spl_image->size + mmc->read_bl_len - 1) /
> -			     mmc->read_bl_len;
> -
> -	/* Read the header too to avoid extra memcpy */
> -	count = blk_dread(mmc_get_blk_desc(mmc),
> -			  sector + image_offset_sectors,
> -			  image_size_sectors,
> -			  (void *)(ulong)spl_image->load_addr);
> -	debug("read %x sectors to %lx\n", image_size_sectors,
> -	      spl_image->load_addr);
> -	if (count != image_size_sectors)
> -		return -EIO;
> -
> -	if (image_offset)
> -		memmove((void *)(ulong)spl_image->load_addr,
> -			(void *)(ulong)spl_image->load_addr + image_offset,
> -			spl_image->size);
> -
> -	return 0;
> -}
> -
>  static ulong h_spl_load_read(struct spl_load_info *load, ulong sector,
>  			     ulong count, void *buf)
>  {
> @@ -82,47 +40,14 @@ int mmc_load_image_raw_sector(struct spl_image_info *spl_image,
>  			      struct spl_boot_device *bootdev,
>  			      struct mmc *mmc, unsigned long sector)
>  {
> -	unsigned long count;
> -	struct legacy_img_hdr *header;
> -	struct blk_desc *bd = mmc_get_blk_desc(mmc);
> -	int ret = 0;
> -
> -	header = spl_get_load_buffer(-sizeof(*header), bd->blksz);
> -
> -	/* read image header to find the image size & load address */
> -	count = blk_dread(bd, sector, 1, header);
> -	debug("hdr read sector %lx, count=%lu\n", sector, count);
> -	if (count == 0) {
> -		ret = -EIO;
> -		goto end;
> -	}
> -
> -	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
> -	    image_get_magic(header) == FDT_MAGIC) {
> -		struct spl_load_info load;
> -
> -		debug("Found FIT\n");
> -		load.dev = mmc;
> -		load.priv = NULL;
> -		load.filename = NULL;
> -		load.bl_len = mmc->read_bl_len;
> -		load.read = h_spl_load_read;
> -		ret = spl_load_simple_fit(spl_image, &load, sector, header);
> -	} else if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
> -		struct spl_load_info load;
> -
> -		load.dev = mmc;
> -		load.priv = NULL;
> -		load.filename = NULL;
> -		load.bl_len = mmc->read_bl_len;
> -		load.read = h_spl_load_read;
> -
> -		ret = spl_load_imx_container(spl_image, &load, sector);
> -	} else {
> -		ret = mmc_load_legacy(spl_image, bootdev, mmc, sector, header);
> -	}
> -
> -end:
> +	int ret;
> +	struct spl_load_info load = {
> +		.dev = mmc,
> +		.bl_len = mmc->read_bl_len,
> +		.read = h_spl_load_read,
> +	};
> +
> +	ret = spl_load(spl_image, bootdev, &load, 0, sector);
>  	if (ret) {
>  #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>  		puts("mmc_load_image_raw_sector: mmc block read error\n");
> -- 
> 2.40.1
>