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

[Sughosh Ganu]

hi Ilias,

On Fri, 1 Apr 2022 at 01:05, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> 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 at 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() '

Okay

> > +
> > +     memset(buf, 0, DFU_ALT_BUF_LEN);
>
> I'd prefer sizeof(buf) instead of explicitly calling the length.

So the current boards are using this wrong. The
ALLOC_CACHE_ALIGN_BUFFER actually defines an array named __buf, and a
char *buf which points to the array. So the existing code in some
boards was zeroing out only the sizeof(char *) number of bytes. The
sandbox clang build throws up this warning, which is how I found it.

-sughosh

>
> 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 */
>
> [...]