Hi Sughosh
On Fri, 1 Apr 2022 at 09:58, Sughosh Ganu sughosh.ganu@linaro.org wrote:
hi Ilias,
On Fri, 1 Apr 2022 at 01:05, Ilias Apalodimas ilias.apalodimas@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@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 */
[...]