Re: [PATCH 06/14] fastboot: Change fastboot_buf_addr to an address

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Simon,

Thank you for your patch.

On dim., déc. 03, 2023 at 17:31, Simon Glass <sjg@chromium.org> wrote:

> Given the name of this variable, it should be an address, not a
> pointer. Update this, to make it easier to use with sandbox.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Tested that I could reflash AOSP on the Khadas VIM 3 board.

Tested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> # on vim3

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

Some small nit/question below.

> ---
>
>  cmd/fastboot.c                |  2 +-
>  drivers/fastboot/fb_command.c | 13 ++++++++-----
>  drivers/fastboot/fb_common.c  | 15 ++++-----------
>  include/fastboot-internal.h   |  2 +-
>  include/fastboot.h            |  6 +++---
>  5 files changed, 17 insertions(+), 21 deletions(-)
>
> diff --git a/cmd/fastboot.c b/cmd/fastboot.c
> index c3c19231c988..792e83d372c3 100644
> --- a/cmd/fastboot.c
> +++ b/cmd/fastboot.c
> @@ -159,7 +159,7 @@ NXTARG:
>  		return CMD_RET_USAGE;
>  	}
>  
> -	fastboot_init((void *)buf_addr, buf_size);
> +	fastboot_init(buf_addr, buf_size);
>  
>  	if (!strcmp(argv[1], "udp"))
>  		return do_fastboot_udp(argc, argv, buf_addr, buf_size);
> diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c
> index 5fcadcdf503d..ec030886dbb8 100644
> --- a/drivers/fastboot/fb_command.c
> +++ b/drivers/fastboot/fb_command.c
> @@ -10,6 +10,7 @@
>  #include <fastboot-internal.h>
>  #include <fb_mmc.h>
>  #include <fb_nand.h>
> +#include <mapmem.h>
>  #include <part.h>
>  #include <stdlib.h>
>  #include <linux/printk.h>
> @@ -252,7 +253,7 @@ void fastboot_data_download(const void *fastboot_data,
>  		return;
>  	}
>  	/* Download data to fastboot_buf_addr */
> -	memcpy(fastboot_buf_addr + fastboot_bytes_received,
> +	memcpy(map_sysmem(fastboot_buf_addr, 0) + fastboot_bytes_received,
>  	       fastboot_data, fastboot_data_len);

I'm a little confused on why there is no call of unmap_sysmem() here but
that seems to be the case for plenty other callers of map_sysmem().

From what I've seen, in the sandbox unmap_sysmem() is a no-oop, but what
about other architectures ?

>  
>  	pre_dot_num = fastboot_bytes_received / BYTES_PER_DOT;
> @@ -296,13 +297,15 @@ void fastboot_data_complete(char *response)
>   */
>  static void __maybe_unused flash(char *cmd_parameter, char *response)
>  {
> +	void *buf = map_sysmem(fastboot_buf_addr, 0);
> +
>  	if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_MMC))
> -		fastboot_mmc_flash_write(cmd_parameter, fastboot_buf_addr,
> -					 image_size, response);
> +		fastboot_mmc_flash_write(cmd_parameter, buf, image_size,
> +					 response);
>  
>  	if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_NAND))
> -		fastboot_nand_flash_write(cmd_parameter, fastboot_buf_addr,
> -					  image_size, response);
> +		fastboot_nand_flash_write(cmd_parameter, buf, image_size,
> +					  response);

Same here.

>  }
>  
>  /**
> diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c
> index 3576b0677299..07f5946d9ed1 100644
> --- a/drivers/fastboot/fb_common.c
> +++ b/drivers/fastboot/fb_common.c
> @@ -20,7 +20,7 @@
>  /**
>   * fastboot_buf_addr - base address of the fastboot download buffer
>   */
> -void *fastboot_buf_addr;
> +ulong fastboot_buf_addr;
>  
>  /**
>   * fastboot_buf_size - size of the fastboot download buffer
> @@ -154,7 +154,7 @@ void fastboot_boot(void)
>  		};
>  
>  		snprintf(boot_addr_start, sizeof(boot_addr_start) - 1,
> -			 "0x%p", fastboot_buf_addr);
> +			 "%lx", fastboot_buf_addr);
>  		printf("Booting kernel at %s...\n\n\n", boot_addr_start);
>  
>  		do_bootm(NULL, 0, 2, bootm_args);
> @@ -214,16 +214,9 @@ void fastboot_set_progress_callback(void (*progress)(const char *msg))
>  	fastboot_progress_callback = progress;
>  }
>  
> -/*
> - * fastboot_init() - initialise new fastboot protocol session
> - *
> - * @buf_addr: Pointer to download buffer, or NULL for default
> - * @buf_size: Size of download buffer, or zero for default
> - */
> -void fastboot_init(void *buf_addr, u32 buf_size)
> +void fastboot_init(ulong buf_addr, u32 buf_size)
>  {
> -	fastboot_buf_addr = buf_addr ? buf_addr :
> -				       (void *)CONFIG_FASTBOOT_BUF_ADDR;
> +	fastboot_buf_addr = buf_addr ? buf_addr : CONFIG_FASTBOOT_BUF_ADDR;
>  	fastboot_buf_size = buf_size ? buf_size : CONFIG_FASTBOOT_BUF_SIZE;
>  	fastboot_set_progress_callback(NULL);
>  }
> diff --git a/include/fastboot-internal.h b/include/fastboot-internal.h
> index bf2f2b3c8914..d3e1c106e23f 100644
> --- a/include/fastboot-internal.h
> +++ b/include/fastboot-internal.h
> @@ -6,7 +6,7 @@
>  /**
>   * fastboot_buf_addr - base address of the fastboot download buffer
>   */
> -extern void *fastboot_buf_addr;
> +extern ulong fastboot_buf_addr;
>  
>  /**
>   * fastboot_buf_size - size of the fastboot download buffer
> diff --git a/include/fastboot.h b/include/fastboot.h
> index 296451f89d44..744ab61cc18a 100644
> --- a/include/fastboot.h
> +++ b/include/fastboot.h
> @@ -103,13 +103,13 @@ int fastboot_set_reboot_flag(enum fastboot_reboot_reason reason);
>   */
>  void fastboot_set_progress_callback(void (*progress)(const char *msg));
>  
> -/*
> +/**
>   * fastboot_init() - initialise new fastboot protocol session
>   *
> - * @buf_addr: Pointer to download buffer, or NULL for default
> + * @buf_addr: Address of download buffer, or 0 for default
>   * @buf_size: Size of download buffer, or zero for default
>   */
> -void fastboot_init(void *buf_addr, u32 buf_size);
> +void fastboot_init(ulong buf_addr, u32 buf_size);
>  
>  /**
>   * fastboot_boot() - Execute fastboot boot command
> -- 
> 2.43.0.rc2.451.g8631bc7472-goog