Re: [PATCH] dfu: define a callback function for the dfu_alt_info environment variable

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Rasmus,

Thank you for the patch.

On mer., sept. 11, 2024 at 15:39, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote:

> I'm trying to use dfu-util for bootstrapping an stm32mp board. It
> mostly works fine, but something goes horribly wrong as soon as I make
> use of the ability to run arbitrary u-boot shell commands. The shell
> commands themselves work fine, but the heuristic "dfu_alt_info may
> have changed, we have to reinit" seems to cause the board and/or my
> host machine to go into some bad state, and further dfu-util commands
> fail.
>
> U-Boot already has a mechanism whereby C code can be told about
> changes to specific environment variables. So instead of always doing
> re-init, add a hook to the dfu_alt_info variable so that we only do
> set dfu_reinit_needed if the commands actually did modify that
> variable.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

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

> ---
>  drivers/dfu/dfu.c      | 16 ++++++++++++++++
>  drivers/dfu/dfu_mmc.c  |  3 ++-
>  include/dfu.h          |  1 +
>  include/env_callback.h |  7 +++++++
>  4 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> index 540d48fab77..7a4d7ba2a7f 100644
> --- a/drivers/dfu/dfu.c
> +++ b/drivers/dfu/dfu.c
> @@ -27,6 +27,21 @@ static unsigned long dfu_timeout = 0;
>  #endif
>  
>  bool dfu_reinit_needed = false;
> +bool dfu_alt_info_changed = false;
> +
> +static int on_dfu_alt_info(const char *name, const char *value, enum env_op op,
> +			   int flags)
> +{
> +	switch (op) {
> +	case env_op_create:
> +	case env_op_overwrite:
> +	case env_op_delete:
> +		dfu_alt_info_changed = true;
> +		break;
> +	}
> +	return 0;
> +}
> +U_BOOT_ENV_CALLBACK(dfu_alt_info, on_dfu_alt_info);
>  
>  /*
>   * The purpose of the dfu_flush_callback() function is to
> @@ -152,6 +167,7 @@ int dfu_init_env_entities(char *interface, char *devstr)
>  	int ret = 0;
>  
>  	dfu_reinit_needed = false;
> +	dfu_alt_info_changed = false;
>  
>  #ifdef CONFIG_SET_DFU_ALT_INFO
>  	set_dfu_alt_info(interface, devstr);
> diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
> index 8f7ecfa8fc7..c19eb919388 100644
> --- a/drivers/dfu/dfu_mmc.c
> +++ b/drivers/dfu/dfu_mmc.c
> @@ -232,7 +232,8 @@ int dfu_flush_medium_mmc(struct dfu_entity *dfu)
>  		break;
>  	case DFU_SCRIPT:
>  		/* script may have changed the dfu_alt_info */
> -		dfu_reinit_needed = true;
> +		if (dfu_alt_info_changed)
> +			dfu_reinit_needed = true;
>  		break;
>  	case DFU_RAW_ADDR:
>  	case DFU_SKIP:
> diff --git a/include/dfu.h b/include/dfu.h
> index 6c5431b3948..e25588c33cb 100644
> --- a/include/dfu.h
> +++ b/include/dfu.h
> @@ -517,6 +517,7 @@ static inline int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr,
>  #endif
>  
>  extern bool dfu_reinit_needed;
> +extern bool dfu_alt_info_changed;
>  
>  #if CONFIG_IS_ENABLED(DFU_WRITE_ALT)
>  /**
> diff --git a/include/env_callback.h b/include/env_callback.h
> index 8e500aaaf80..66cc8309e71 100644
> --- a/include/env_callback.h
> +++ b/include/env_callback.h
> @@ -69,6 +69,12 @@
>  #define BOOTSTD_CALLBACK
>  #endif
>  
> +#ifdef CONFIG_DFU
> +#define DFU_CALLBACK "dfu_alt_info:dfu_alt_info,"
> +#else
> +#define DFU_CALLBACK
> +#endif
> +
>  /*
>   * This list of callback bindings is static, but may be overridden by defining
>   * a new association in the ".callbacks" environment variable.
> @@ -79,6 +85,7 @@
>  	NET_CALLBACKS \
>  	NET6_CALLBACKS \
>  	BOOTSTD_CALLBACK \
> +	DFU_CALLBACK \
>  	"loadaddr:loadaddr," \
>  	SILENT_CALLBACK \
>  	"stdin:console,stdout:console,stderr:console," \
> -- 
> 2.46.0