[PATCH] dfu: Prevent set_dfu_alt_info() from overwriting a previous value

[Siddharth Vadapalli]

On Tue, Dec 17, 2024 at 02:48:35PM -0600, Jonathan Humphreys wrote:

Hello Jon,

Thank you for posting this patch. I will drop the equivalent of this
patch when I post the v2 series for:
https://patchwork.ozlabs.org/project/uboot/cover/20241217131658.2920799-1-s-vadapalli@ti.com/

> If CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment
> variable is dynamically set when initializing the DFU entities, which is
> done as part of normal flow of a DFU operation.
> 
> The USB DFU boot support will set it's own specific value for dfu_alt_info

nitpick: s/it's/its

> before performing the DFU operation. This means that if
> CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment variable
> that the USB DFU boot path had set is overwritten, causing USB DFU boot to
> fail.
> 
> Likewise, if the user sets their own value for dfu_alt_info, say at the
> U-Boot prompt, it get's overwritten if CONFIG_SET_DFU_ALT_INFO is enabled.

nitpick: s/get's/gets

> 
> This patch will first check that dfu_alt_info isn't already set before
> calling set_dfu_alt_info(), when CONFIG_SET_DFU_ALT_INFO is enabled.

Rather than referring to "patch", we could rephrase the above as:

Check that dfu_alt_info isn't already set before calling set_dfu_alt_info()
when CONFIG_SET_DFU_ALT_INFO is enabled, in order to avoid overwriting it.

> 
> Signed-off-by: Jonathan Humphreys <j-humphreys at ti.com>
> ---
>  drivers/dfu/dfu.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> index 756569217bb..ab8abae1d89 100644
> --- a/drivers/dfu/dfu.c
> +++ b/drivers/dfu/dfu.c
> @@ -169,10 +169,13 @@ int dfu_init_env_entities(char *interface, char *devstr)
>  	dfu_reinit_needed = false;
>  	dfu_alt_info_changed = false;
>  
> +	str_env = env_get("dfu_alt_info");
>  #ifdef CONFIG_SET_DFU_ALT_INFO
> -	set_dfu_alt_info(interface, devstr);
> +	if (!str_env) {
> +		set_dfu_alt_info(interface, devstr);
> +		str_env = env_get("dfu_alt_info");
> +	}
>  #endif
> -	str_env = env_get("dfu_alt_info");
>  	if (!str_env) {
>  		pr_err("\"dfu_alt_info\" env variable not defined!\n");
>  		return -EINVAL;

Regards,
Siddharth.