Re: [PATCH v4 4/8] board: Define set_dfu_alt_info() for boards with UEFI capsule update enabled

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Sughosh, 

Some nots below

On Thu, Mar 31, 2022 at 06:57:46PM +0530, Sughosh Ganu wrote:
> Currently, there are a bunch of boards which enable the UEFI capsule
> update feature. The actual update of the firmware images is done
> through the dfu framework which uses the dfu_alt_info environment
> variable for getting information on the update, like device, partition
> number/address etc. Currently, these boards define the dfu_alt_info
> variable in the board config header, as an environment variable. With
> this, the variable can be modified from the u-boot command line and
> this can cause an incorrect update.
> 
> To prevent this from happening, define the set_dfu_alt_info function
> in the board file, and select SET_DFU_ALT_INFO for all platforms which
> enable the capsule update feature. With the function defined, the dfu
> framework populates the dfu_alt_info variable through the board file,
> instead of fetching the variable from the environment, thus making the
> update more robust.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
> 
> Changes since V3:
> * Do not remove the existing dfu_alt_info definitions made by
>   platforms in the config files, as discussed with Masami.
> * Squash the selection of the SET_DFU_ALT_INFO config symbol for
>   capsule update feature as part of this patch.
> 
> 
>  .../imx8mp_rsb3720a1/imx8mp_rsb3720a1.c       | 24 +++++++++++++++++
>  .../imx8mm-cl-iot-gate/imx8mm-cl-iot-gate.c   | 24 +++++++++++++++++
>  board/emulation/common/qemu_dfu.c             |  6 ++---
>  board/kontron/pitx_imx8m/pitx_imx8m.c         | 24 +++++++++++++++++
>  board/kontron/sl-mx8mm/sl-mx8mm.c             | 24 +++++++++++++++++
>  board/kontron/sl28/sl28.c                     | 25 ++++++++++++++++++
>  board/sandbox/sandbox.c                       | 26 +++++++++++++++++++
>  board/socionext/developerbox/developerbox.c   | 26 +++++++++++++++++++
>  board/xilinx/zynq/board.c                     |  5 ++--
>  board/xilinx/zynqmp/zynqmp.c                  |  5 ++--
>  lib/efi_loader/Kconfig                        |  2 ++
>  11 files changed, 184 insertions(+), 7 deletions(-)
> 
> diff --git a/board/advantech/imx8mp_rsb3720a1/imx8mp_rsb3720a1.c b/board/advantech/imx8mp_rsb3720a1/imx8mp_rsb3720a1.c
> index 1c953ba195..41154ca9f3 100644
> --- a/board/advantech/imx8mp_rsb3720a1/imx8mp_rsb3720a1.c
> +++ b/board/advantech/imx8mp_rsb3720a1/imx8mp_rsb3720a1.c
> @@ -5,10 +5,12 @@
>   */
>  
>  #include <common.h>
> +#include <dfu.h>
>  #include <dwc3-uboot.h>
>  #include <efi.h>
>  #include <efi_loader.h>
>  #include <errno.h>
> +#include <memalign.h>
>  #include <miiphy.h>
>  #include <netdev.h>
>  #include <spl.h>
> @@ -24,6 +26,7 @@
>  #include <asm/mach-imx/dma.h>
>  #include <linux/delay.h>
>  #include <linux/kernel.h>
> +#include <linux/sizes.h>
>  #include <power/pmic.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
> @@ -231,3 +234,24 @@ unsigned long spl_mmc_get_uboot_raw_sector(struct mmc *mmc)
>  	}
>  }
>  #endif /* CONFIG_SPL_MMC_SUPPORT */
> +
> +#if defined(CONFIG_SET_DFU_ALT_INFO)
> +
> +#define DFU_ALT_BUF_LEN		SZ_1K
> +
> +void set_dfu_alt_info(char *interface, char *devstr)
> +{
> +	ALLOC_CACHE_ALIGN_BUFFER(char, buf, DFU_ALT_BUF_LEN);
> +
> +	if (!CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT) &&
> +	    env_get("dfu_alt_info"))
> +		return;

Just add a helper function with this since we need to repeat it for every
board. Something like 'bool needs_runtime_dfu_alt_info() '
> +
> +	memset(buf, 0, DFU_ALT_BUF_LEN);

I'd prefer sizeof(buf) instead of explicitly calling the length.

Otherwise LGTM, but I'd prefer if board maintainer had a look as well 

Thanks
/Ilias

> +
> +	snprintf(buf, DFU_ALT_BUF_LEN,
> +		 "mmc 2=flash-bin raw 0 0x1B00 mmcpart 1");
> +
> +	env_set("dfu_alt_info", buf);
> +}
> +#endif /* CONFIG_SET_DFU_ALT_INFO */

[...]