diff mbox series

[1/1] boot/uboot: fix binman failure

Message ID 20250207231203.3952765-1-dario.binacchi@amarulasolutions.com
State New
Headers
Series [1/1] boot/uboot: fix binman failure |

Commit Message

Dario Binacchi Feb. 7, 2025, 11:12 p.m. UTC 
The commit 4bce3270d680 ("Config.in: timeout earlier when connecting to
download servers"), among other things, adds the connection timeout
parameter to the default scp command:

config BR2_SCP
       string "Secure copy (scp) command"
       default "scp -o ConnectTimeout=10"

Since the package/pkg-download.mk file exports this command using the
SCP variable:

export SCP := $(call qstrip,$(BR2_SCP))

and the U-Boot Makefile uses this variable as the path for the scp
command (rather than the command itself):

cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \
		$(foreach f,$(BINMAN_TOOLPATHS),--toolpath $(f)) \
                --toolpath $(objtree)/tools \
		$(if $(BINMAN_VERBOSE),-v$(BINMAN_VERBOSE)) \
		build -u -d $(binman_dtb) -O . -m \
		--allow-missing --fake-ext-blobs \
		$(if $(BINMAN_ALLOW_MISSING),--ignore-missing) \
		-I . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \
		$(foreach f,$(of_list_dirs),-I $(f)) -a of-list=$(of_list) \
		$(foreach f,$(BINMAN_INDIRS),-I $(f)) \
		-a atf-bl31-path=${BL31} \
		-a tee-os-path=${TEE} \
		-a ti-dm-path=${TI_DM} \
		-a opensbi-path=${OPENSBI} \
		-a default-dt=$(default_dt) \
		-a scp-path=$(SCP) \

the following error occurs:

  BINMAN  .binman_stamp
usage: binman [-h] [-B BUILD_DIR] [-D] [-H] [--tooldir TOOLDIR] [--toolpath TOOLPATH] [-T THREADS] [--test-section-timeout] [-v VERBOSITY] [-V]
              {build,bintool-docs,entry-docs,ls,extract,replace,sign,test,tool} ...
binman: error: unrecognized arguments: -o ConnectTimeout=10
make[2]: *** [Makefile:1126: .binman_stamp] Error 2

The patch fixes the SCP setting passed to U-Boot to ensure that its
parameters are not included.

Fixes: 4bce3270d680 ("Config.in: timeout earlier when connecting to download servers")
Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
---
 boot/uboot/uboot.mk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Sergey Matyukevich Feb. 8, 2025, 3:21 p.m. UTC | #1 
Hi Dario,

On Sat, Feb 08, 2025 at 12:12:03AM +0100, Dario Binacchi wrote:
> The commit 4bce3270d680 ("Config.in: timeout earlier when connecting to
> download servers"), among other things, adds the connection timeout
> parameter to the default scp command:

...

> Fixes: 4bce3270d680 ("Config.in: timeout earlier when connecting to download servers")
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> ---
>  boot/uboot/uboot.mk | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
> index 2796b0a31010..6bbdc4151fac 100644
> --- a/boot/uboot/uboot.mk
> +++ b/boot/uboot/uboot.mk
> @@ -176,7 +176,8 @@ UBOOT_MAKE_OPTS += \
>  	ARCH=$(UBOOT_ARCH) \
>  	HOSTCC="$(HOSTCC) $(subst -I/,-isystem /,$(subst -I /,-isystem /,$(HOST_CFLAGS)))" \
>  	HOSTLDFLAGS="$(HOST_LDFLAGS)" \
> -	$(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS))
> +	$(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS)) \
> +	SCP=$(word 1, $(SCP))
>  
>  # Disable FDPIC if enabled by default in toolchain
>  ifeq ($(BR2_BINFMT_FDPIC),y)
> -- 
> 2.43.0

Thanks for the fix ! I received several autobuild reports for OrangePi
boards today and tested your fix for some of them. Here are the links:

Fixes:
https://gitlab.com/buildroot.org/buildroot/-/jobs/9054932579
https://gitlab.com/buildroot.org/buildroot/-/jobs/9054932525

Tested-by: Sergey Matyukevich <geomatsi@gmail.com>

Regards,
Sergey

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
'Abel Vesa' via Amarula Linux Feb. 10, 2025, 11:53 a.m. UTC | #2 
On Sat, Feb 08, 2025 at 06:21:15PM +0300, Sergey Matyukevich wrote:
> Hi Dario,
> 
> On Sat, Feb 08, 2025 at 12:12:03AM +0100, Dario Binacchi wrote:
> > The commit 4bce3270d680 ("Config.in: timeout earlier when connecting to
> > download servers"), among other things, adds the connection timeout
> > parameter to the default scp command:
> 
> ...
> 
> > Fixes: 4bce3270d680 ("Config.in: timeout earlier when connecting to download servers")
> > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> > ---
> >  boot/uboot/uboot.mk | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
> > index 2796b0a31010..6bbdc4151fac 100644
> > --- a/boot/uboot/uboot.mk
> > +++ b/boot/uboot/uboot.mk
> > @@ -176,7 +176,8 @@ UBOOT_MAKE_OPTS += \
> >  	ARCH=$(UBOOT_ARCH) \
> >  	HOSTCC="$(HOSTCC) $(subst -I/,-isystem /,$(subst -I /,-isystem /,$(HOST_CFLAGS)))" \
> >  	HOSTLDFLAGS="$(HOST_LDFLAGS)" \
> > -	$(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS))
> > +	$(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS)) \
> > +	SCP=$(word 1, $(SCP))
> >  
> >  # Disable FDPIC if enabled by default in toolchain
> >  ifeq ($(BR2_BINFMT_FDPIC),y)
> > -- 
> > 2.43.0
> 
> Thanks for the fix ! I received several autobuild reports for OrangePi
> boards today and tested your fix for some of them. Here are the links:
> 
> Fixes:
> https://gitlab.com/buildroot.org/buildroot/-/jobs/9054932579
> https://gitlab.com/buildroot.org/buildroot/-/jobs/9054932525
> 
> Tested-by: Sergey Matyukevich <geomatsi@gmail.com>

Tested-by: Niklas Cassel <cassel@kernel.org>


Could this please be picked up?

The buildroot master branch currently fails to build without this patch.


Kind regards,
Niklas

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
Julien Olivain Feb. 10, 2025, 7:33 p.m. UTC | #3 
Hi Dario,

Thanks for the patch.

On 08/02/2025 00:12, Dario Binacchi wrote:
> The commit 4bce3270d680 ("Config.in: timeout earlier when connecting to
> download servers"), among other things, adds the connection timeout
> parameter to the default scp command:
> 
> config BR2_SCP
>        string "Secure copy (scp) command"
>        default "scp -o ConnectTimeout=10"
> 
> Since the package/pkg-download.mk file exports this command using the
> SCP variable:
> 
> export SCP := $(call qstrip,$(BR2_SCP))
> 
> and the U-Boot Makefile uses this variable as the path for the scp
> command (rather than the command itself):
> 
> cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \
> 		$(foreach f,$(BINMAN_TOOLPATHS),--toolpath $(f)) \
>                 --toolpath $(objtree)/tools \
> 		$(if $(BINMAN_VERBOSE),-v$(BINMAN_VERBOSE)) \
> 		build -u -d $(binman_dtb) -O . -m \
> 		--allow-missing --fake-ext-blobs \
> 		$(if $(BINMAN_ALLOW_MISSING),--ignore-missing) \
> 		-I . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \
> 		$(foreach f,$(of_list_dirs),-I $(f)) -a of-list=$(of_list) \
> 		$(foreach f,$(BINMAN_INDIRS),-I $(f)) \
> 		-a atf-bl31-path=${BL31} \
> 		-a tee-os-path=${TEE} \
> 		-a ti-dm-path=${TI_DM} \
> 		-a opensbi-path=${OPENSBI} \
> 		-a default-dt=$(default_dt) \
> 		-a scp-path=$(SCP) \
> 
> the following error occurs:
> 
>   BINMAN  .binman_stamp
> usage: binman [-h] [-B BUILD_DIR] [-D] [-H] [--tooldir TOOLDIR] 
> [--toolpath TOOLPATH] [-T THREADS] [--test-section-timeout] [-v 
> VERBOSITY] [-V]
>               
> {build,bintool-docs,entry-docs,ls,extract,replace,sign,test,tool} ...
> binman: error: unrecognized arguments: -o ConnectTimeout=10
> make[2]: *** [Makefile:1126: .binman_stamp] Error 2
> 
> The patch fixes the SCP setting passed to U-Boot to ensure that its
> parameters are not included.
> 
> Fixes: 4bce3270d680 ("Config.in: timeout earlier when connecting to 
> download servers")
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> ---
>  boot/uboot/uboot.mk | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
> index 2796b0a31010..6bbdc4151fac 100644
> --- a/boot/uboot/uboot.mk
> +++ b/boot/uboot/uboot.mk
> @@ -176,7 +176,8 @@ UBOOT_MAKE_OPTS += \
>  	ARCH=$(UBOOT_ARCH) \
>  	HOSTCC="$(HOSTCC) $(subst -I/,-isystem /,$(subst -I /,-isystem 
> /,$(HOST_CFLAGS)))" \
>  	HOSTLDFLAGS="$(HOST_LDFLAGS)" \
> -	$(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS))
> +	$(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS)) \
> +	SCP=$(word 1, $(SCP))

I don't think this is the right fix for this issue.

U-Boot Makefile is using the "SCP" variable here:
https://source.denx.de/u-boot/u-boot/-/blob/v2025.01/Makefile#L1411

According the the "binman" documentation:
https://source.denx.de/u-boot/u-boot/-/blob/v2025.01/tools/binman/entries.rst#L1727

This SCP is the "System Control Processor firmware blob".

Buildroot exports a SCP variable here:
https://gitlab.com/buildroot.org/buildroot/-/blob/2024.11.1/package/pkg-download.mk#L18

The value is set here:
https://gitlab.com/buildroot.org/buildroot/-/blob/2024.11.1/Config.in#L134

BR2_SCP corresponds to the SSH Secure File Copy command:
https://man.openbsd.org/scp

which has nothing to do with the System Control Processor firmware
binman is expecting. So this is an unfortunate name clash.

There is few defconfigs using BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS to
workaround this. See for example:
https://gitlab.com/buildroot.org/buildroot/-/blob/2024.11.1/configs/orangepi_zero_plus2_defconfig#L45

There is no defconfig in Buildroot setting an actual file for u-boot 
SCP.

Maybe we could introduce a new u-boot Kconfig option string such as:
BR2_TARGET_UBOOT_SCP_FIRMWARE and properly set the u-boot SCP
variable to this value (and set it to /dev/null if empty).

What do you think?

>  # Disable FDPIC if enabled by default in toolchain
>  ifeq ($(BR2_BINFMT_FDPIC),y)
> --
> 2.43.0

Best regards,

Julien.

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
Julien Olivain Feb. 10, 2025, 9:16 p.m. UTC | #4 
Hi Dario, All,

On 10/02/2025 20:33, Julien Olivain wrote:
> Maybe we could introduce a new u-boot Kconfig option string such as:
> BR2_TARGET_UBOOT_SCP_FIRMWARE and properly set the u-boot SCP
> variable to this value (and set it to /dev/null if empty).

After quickly testing, it seems u-boot and binman can take
an empty string without failing. So there is no need to
set it to /dev/null if empty. So if a new Kconfig is added
with an empty default value, just adding
"SCP=$(BR2_TARGET_UBOOT_SCP_FIRMWARE)" in UBOOT_MAKE_OPTS
should be fine.

Best regards,

Julien.

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
'Abel Vesa' via Amarula Linux Feb. 12, 2025, 9:24 p.m. UTC | #5 
Hi Julien, Dario,

Julien Olivain <ju.o@free.fr> writes:
> I don't think this is the right fix for this issue.
>
> U-Boot Makefile is using the "SCP" variable here:
> https://source.denx.de/u-boot/u-boot/-/blob/v2025.01/Makefile#L1411
>
> According the the "binman" documentation:
> https://source.denx.de/u-boot/u-boot/-/blob/v2025.01/tools/binman/entries.rst#L1727
>
> This SCP is the "System Control Processor firmware blob".
>
> Buildroot exports a SCP variable here:
> https://gitlab.com/buildroot.org/buildroot/-/blob/2024.11.1/package/pkg-download.mk#L18
>
> The value is set here:
> https://gitlab.com/buildroot.org/buildroot/-/blob/2024.11.1/Config.in#L134
>
> BR2_SCP corresponds to the SSH Secure File Copy command:
> https://man.openbsd.org/scp
>
> which has nothing to do with the System Control Processor firmware
> binman is expecting. So this is an unfortunate name clash.
>
> There is few defconfigs using BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS to
> workaround this. See for example:
> https://gitlab.com/buildroot.org/buildroot/-/blob/2024.11.1/configs/orangepi_zero_plus2_defconfig#L45

This worked to fix the issue for me on bananapi m3:

# https://lore.kernel.org/buildroot/96b69abb843dc7292777c64f6787f134@free.fr/
# https://gitlab.com/buildroot.org/buildroot/-/blob/2024.11.1/configs/orangepi_zero_plus2_defconfig#L45
# https://github.com/skiffos/SkiffOS/issues/325
BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS="SCP=/dev/null"

Thanks,
Christian Stewart

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
Sergey Matyukevich Feb. 13, 2025, 8:50 a.m. UTC | #6 
Hi Julien and all,

On Mon, Feb 10, 2025 at 10:16:05PM +0100, Julien Olivain wrote:
> Hi Dario, All,
> 
> On 10/02/2025 20:33, Julien Olivain wrote:
> > Maybe we could introduce a new u-boot Kconfig option string such as:
> > BR2_TARGET_UBOOT_SCP_FIRMWARE and properly set the u-boot SCP
> > variable to this value (and set it to /dev/null if empty).
> 
> After quickly testing, it seems u-boot and binman can take
> an empty string without failing. So there is no need to
> set it to /dev/null if empty. So if a new Kconfig is added
> with an empty default value, just adding
> "SCP=$(BR2_TARGET_UBOOT_SCP_FIRMWARE)" in UBOOT_MAKE_OPTS
> should be fine.

Am I correct assuming that there will be no generic fix and
the recommended solution is to add the following line to all
the impacted configs:
- BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS="SCP=/dev/null"

Regards,
Sergey

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
'Abel Vesa' via Amarula Linux Feb. 13, 2025, 1:21 p.m. UTC | #7 
Hello Sergey,

On Thu, Feb 13, 2025 at 11:50:35AM +0300, Sergey Matyukevich wrote:
> Hi Julien and all,
> 
> On Mon, Feb 10, 2025 at 10:16:05PM +0100, Julien Olivain wrote:
> > Hi Dario, All,
> > 
> > On 10/02/2025 20:33, Julien Olivain wrote:
> > > Maybe we could introduce a new u-boot Kconfig option string such as:
> > > BR2_TARGET_UBOOT_SCP_FIRMWARE and properly set the u-boot SCP
> > > variable to this value (and set it to /dev/null if empty).
> > 
> > After quickly testing, it seems u-boot and binman can take
> > an empty string without failing. So there is no need to
> > set it to /dev/null if empty. So if a new Kconfig is added
> > with an empty default value, just adding
> > "SCP=$(BR2_TARGET_UBOOT_SCP_FIRMWARE)" in UBOOT_MAKE_OPTS
> > should be fine.
> 
> Am I correct assuming that there will be no generic fix and
> the recommended solution is to add the following line to all
> the impacted configs:
> - BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS="SCP=/dev/null"

I think that Dario is working on a patch:
https://lore.kernel.org/buildroot/87frkjyma6.fsf@dell.be.48ers.dk/T/#m7f5f85f0cdb2468c78512d34ade8703595921047

However, Peter had some comments, so maybe Dario will send a v2?


Kind regards,
Niklas

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
diff mbox series

Patch

diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
index 2796b0a31010..6bbdc4151fac 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -176,7 +176,8 @@  UBOOT_MAKE_OPTS += \
 	ARCH=$(UBOOT_ARCH) \
 	HOSTCC="$(HOSTCC) $(subst -I/,-isystem /,$(subst -I /,-isystem /,$(HOST_CFLAGS)))" \
 	HOSTLDFLAGS="$(HOST_LDFLAGS)" \
-	$(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS))
+	$(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS)) \
+	SCP=$(word 1, $(SCP))
 
 # Disable FDPIC if enabled by default in toolchain
 ifeq ($(BR2_BINFMT_FDPIC),y)