hi Ilias, On Fri, 1 Apr 2022 at 01:05, Ilias Apalodimas <ilias.apalodi...@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.g...@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 */ > > [...]