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

[Ilias Apalodimas]

Hi Sughosh

On Fri, 1 Apr 2022 at 09:58, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> 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

Ah right, then that's fine.

Thinking about the common function a bit more can we slightly change
what you have?
On the fw_images struct add a const char* with the dfu_alt_str.
Instead of defining a string literal and snprintf it, use the per
board struct definition and get the string you need.  So the common
case can be abstracted in a __weak function for all the boards to use
and if some platform needs something more it can replace it with it's
own function at link time.

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