On Tue, Oct 13, 2026 at 15:59, phodina <[email protected]> wrote:
> Hi Mattijs, > > > On Tuesday, October 13th, 2026 at 4:20 PM, Mattijs Korpershoek > <[email protected]> wrote: > >> Hi Petr, >> >> Thank you for the patch. >> >> On Sun, Oct 04, 2026 at 22:48, Petr Hodina via B4 Relay >> <[email protected]> wrote: >> >> > From: Petr Hodina <[email protected]> >> > >> > As dfu_alt_info is read via env_get() it needs to expand the ${var} >> > references using cli_simple_process_macros. >> > >> > Signed-off-by: Petr Hodina <[email protected]> >> > --- >> > drivers/dfu/dfu.c | 16 +++++++++++++++- >> > 1 file changed, 15 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c >> > index 90d8b45ce8a..fa074db242c 100644 >> > --- a/drivers/dfu/dfu.c >> > +++ b/drivers/dfu/dfu.c >> > @@ -6,6 +6,7 @@ >> > * author: Lukasz Majewski <[email protected]> >> > */ >> > >> > +#include <cli.h> >> > #include <env.h> >> > #include <errno.h> >> > #include <log.h> >> > @@ -178,7 +179,20 @@ int dfu_init_env_entities(char *interface, char >> > *devstr) >> > return -EINVAL; >> > } >> > >> > - env_bkp = strdup(str_env); >> > + /* >> > + * Expand the dfu_alt_info here as it's read via env_get() which >> > + * does not expand references the way hush does for a command line >> > + */ >> > + env_bkp = malloc(strlen(str_env) * 2 + 1); >> >> Can we explain why we picked strlen(str_env) * 2 + 1? >> >> Is it an arbitrary choice or is there any justification? >> > > It's just good enough solution to work the problem of flashing the pinephone > pro and skipping the USB3 phy enumeration issue for the maskROM and > rkdeveloptool commands :) I see. It's really nice that the pinephone pro can be flashed with this! > > Then again you are right here to ask about the implementation. > > Looked around and there are two more consumers in 'boot/pxe_utils.c' and > 'boot/bootm.c'. > > My take on this expansion size problem would be to add a helper function that > runs the expansion first just counting the size needed. Then allocate the > buffer. Say implemented as such: > > - env_bkp = malloc(strlen(str_env) * 2 + 1); > + size = cli_simple_macros_size(str_env); > + env_bkp = malloc(size); I like this idea, but looking at the code in common/cli_simple.c this does not seems easy to implement (at least to me) Looking a bit more at this, it seems that a lot of boards have a local define DFU_ALT_BUF_LEN set to SZ_1K: $ rg '#define DFU_ALT_BUF_LEN' board/xilinx/zynqmp/zynqmp.c 639:#define DFU_ALT_BUF_LEN SZ_1K board/xilinx/zynq/board.c 172:#define DFU_ALT_BUF_LEN SZ_1K board/xilinx/versal/board.c 314:#define DFU_ALT_BUF_LEN SZ_1K board/xilinx/common/board.c 72:#define DFU_ALT_BUF_LEN SZ_1K board/amd/versal2/board.c 363:#define DFU_ALT_BUF_LEN SZ_1K board/emulation/common/qemu_dfu.c 12:#define DFU_ALT_BUF_LEN SZ_1K board/st/common/stm32mp_dfu.c 18:#define DFU_ALT_BUF_LEN SZ_1K board/socionext/developerbox/fwu_plat.c 13:#define DFU_ALT_BUF_LEN 256 arch/arm/mach-rockchip/board.c 41:#define DFU_ALT_BUF_LEN SZ_1K In my opinion, it's good enough to use SZ_1K for the buffer length here, documenting with a comment that it's based on DFU_ALT_BUF_LEN. Does that work for you? If you want to attempt cli_simple_macros_size() instead, feel free to do so, of course. > > Alternatively we can preallocate some buffer size and attempt to increase it > in a loop (bounded to some max size). > > What do you think? > > I'd later also send another patch series that would fix the expansion in the > 'boot/pxe_utils.c' as it's fixed to CONFIG_SYS_CBSIZE and does not check the > return value. Matching the implementation we deem as a correct one. I agree that we should check the return value, good catch! Thanks Mattijs > > The reason is we'd like to use the PXE to boot the CI images later as > alternative means to the DFU. > > ''' > if (label->append) { > char bootargs[CONFIG_SYS_CBSIZE]; > > cli_simple_process_macros(label->append, bootargs, > sizeof(bootargs)); > env_set("bootargs", bootargs); > } > ''' > >
