diff mbox series

Revert "mksunxi_fit_atf.sh: Allow for this to complete when bl31.bin is missing"

Message ID 20200317131255.26856-1-ynezz@true.cz
State New
Headers show
Series Revert "mksunxi_fit_atf.sh: Allow for this to complete when bl31.bin is missing" | expand

Commit Message

Petr Štetiar March 17, 2020, 1:12 p.m. UTC
This reverts commit 4c78028737c3185f49f5691183aeac3478b5f699.

bl31.bin file is mandatory for functional, usable and bootable binaries,
thus it should be hard error if bl31.bin is missing. It doesn't matter
if I'm building on autobuilder or locally, the resulting binaries should
be usable in both cases.

Cc: Simon Glass <sjg at chromium.org>
Cc: Tom Rini <trini at konsulko.com>
Cc: Andre Przywara <andre.przywara at arm.com>
Cc: Maxime Ripard <maxime.ripard at free-electrons.com>
Signed-off-by: Petr ?tetiar <ynezz at true.cz>
---

I've just spent some time hunting eMMC boot issue on a64-olinuxino. It's
really easy to miss that warning message on fast build hosts as the message is
scrolled out very quickly out of the screen and thus using the broken images,
which would get stuck at `Trying to boot from MMC2`.

I think, that if it's desired to have broken images on the output, then it
should be handled on the autobuilder itself before build of sunxi target.

Another option is probably adding AUTOBUILDER_ALLOW_BROKEN_IMAGES config
option, which would make it clear for everybody.

 board/sunxi/mksunxi_fit_atf.sh | 6 ------
 1 file changed, 6 deletions(-)

Comments

Tom Rini March 17, 2020, 1:20 p.m. UTC | #1
On Tue, Mar 17, 2020 at 02:12:55PM +0100, Petr ?tetiar wrote:

> This reverts commit 4c78028737c3185f49f5691183aeac3478b5f699.
> 
> bl31.bin file is mandatory for functional, usable and bootable binaries,
> thus it should be hard error if bl31.bin is missing. It doesn't matter
> if I'm building on autobuilder or locally, the resulting binaries should
> be usable in both cases.
> 
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Tom Rini <trini at konsulko.com>
> Cc: Andre Przywara <andre.przywara at arm.com>
> Cc: Maxime Ripard <maxime.ripard at free-electrons.com>
> Signed-off-by: Petr ?tetiar <ynezz at true.cz>
> ---
> 
> I've just spent some time hunting eMMC boot issue on a64-olinuxino. It's
> really easy to miss that warning message on fast build hosts as the message is
> scrolled out very quickly out of the screen and thus using the broken images,
> which would get stuck at `Trying to boot from MMC2`.
> 
> I think, that if it's desired to have broken images on the output, then it
> should be handled on the autobuilder itself before build of sunxi target.
> 
> Another option is probably adding AUTOBUILDER_ALLOW_BROKEN_IMAGES config
> option, which would make it clear for everybody.
> 
>  board/sunxi/mksunxi_fit_atf.sh | 6 ------
>  1 file changed, 6 deletions(-)

It's the case that the vast majority of aarch64 platforms only function
when we have other binary files available for the final link and so have
a similar we had hoped loud enough WARNING message.  And yes, this is so
that all of the different CI systems can complete build testing.

So, can you please RFC some patches such that we can still run CI but we
drop all of the "non-functional" WARNING messages we have?  Thanks!
Andre Przywara March 17, 2020, 1:41 p.m. UTC | #2
On 17/03/2020 13:12, Petr ?tetiar wrote:

Hi,

> This reverts commit 4c78028737c3185f49f5691183aeac3478b5f699.
> 
> bl31.bin file is mandatory for functional, usable and bootable binaries,
> thus it should be hard error if bl31.bin is missing. It doesn't matter
> if I'm building on autobuilder or locally, the resulting binaries should
> be usable in both cases.
> 
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Tom Rini <trini at konsulko.com>
> Cc: Andre Przywara <andre.przywara at arm.com>
> Cc: Maxime Ripard <maxime.ripard at free-electrons.com>
> Signed-off-by: Petr ?tetiar <ynezz at true.cz>
> ---
> 
> I've just spent some time hunting eMMC boot issue on a64-olinuxino. It's
> really easy to miss that warning message on fast build hosts as the message is
> scrolled out very quickly out of the screen

For this reason I tend to use make -s on everything, this reduces the
output to the really important information.
If you need a notion of whether the build did something useful (and how
much), prepend "time" to the make command.

> and thus using the broken images,
> which would get stuck at `Trying to boot from MMC2`.
> 
> I think, that if it's desired to have broken images on the output, then it
> should be handled on the autobuilder itself before build of sunxi target.
> 
> Another option is probably adding AUTOBUILDER_ALLOW_BROKEN_IMAGES config
> option, which would make it clear for everybody.
> 
>  board/sunxi/mksunxi_fit_atf.sh | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/board/sunxi/mksunxi_fit_atf.sh b/board/sunxi/mksunxi_fit_atf.sh
> index 88ad71974706..dea9aaa5691d 100755
> --- a/board/sunxi/mksunxi_fit_atf.sh
> +++ b/board/sunxi/mksunxi_fit_atf.sh
> @@ -7,12 +7,6 @@
>  
>  [ -z "$BL31" ] && BL31="bl31.bin"
>  
> -if [ ! -f $BL31 ]; then
> -	echo "WARNING: BL31 file $BL31 NOT found, resulting binary is non-functional" >&2
> -	echo "Please read the section on ARM Trusted Firmware (ATF) in board/sunxi/README.sunxi64" >&2
> -	BL31=/dev/null

So this message has some good info in it. What about we leave that in,
slightly reworded? And just remove the BL31=/dev/null line, so that the
build will actually fail?

Cheers,
Andre



> -fi
> -
>  if grep -q "^CONFIG_MACH_SUN50I_H6=y" .config; then
>  	BL31_ADDR=0x104000
>  else
>
Petr Štetiar March 17, 2020, 1:54 p.m. UTC | #3
Tom Rini <trini at konsulko.com> [2020-03-17 09:20:38]:

Hi,

> On Tue, Mar 17, 2020 at 02:12:55PM +0100, Petr ?tetiar wrote:
> 
> > This reverts commit 4c78028737c3185f49f5691183aeac3478b5f699.
> > 
> > bl31.bin file is mandatory for functional, usable and bootable binaries,
> > thus it should be hard error if bl31.bin is missing. It doesn't matter
> > if I'm building on autobuilder or locally, the resulting binaries should
> > be usable in both cases.
> > 
> > Cc: Simon Glass <sjg at chromium.org>
> > Cc: Tom Rini <trini at konsulko.com>
> > Cc: Andre Przywara <andre.przywara at arm.com>
> > Cc: Maxime Ripard <maxime.ripard at free-electrons.com>
> > Signed-off-by: Petr ?tetiar <ynezz at true.cz>
> > ---
> > 
> > I've just spent some time hunting eMMC boot issue on a64-olinuxino. It's
> > really easy to miss that warning message on fast build hosts as the message is
> > scrolled out very quickly out of the screen and thus using the broken images,
> > which would get stuck at `Trying to boot from MMC2`.
> > 
> > I think, that if it's desired to have broken images on the output, then it
> > should be handled on the autobuilder itself before build of sunxi target.
> > 
> > Another option is probably adding AUTOBUILDER_ALLOW_BROKEN_IMAGES config
> > option, which would make it clear for everybody.
> > 
> >  board/sunxi/mksunxi_fit_atf.sh | 6 ------
> >  1 file changed, 6 deletions(-)
> 
> It's the case that the vast majority of aarch64 platforms only function
> when we have other binary files available for the final link and so have
> a similar we had hoped loud enough WARNING message.  And yes, this is so
> that all of the different CI systems can complete build testing.

so there's some requirement, that the CI systems should always build with
simple `make` call? You know, `make BL31=/dev/null` works just fine if having a
green build, but unusable binaries is desired.

> So, can you please RFC some patches such that we can still run CI but we
> drop all of the "non-functional" WARNING messages we have?  Thanks!

If it's desired to bloat the codebase with config options and ifdefs just to
adapt for unflexible CI systems, fine with me, I'll do so.

Cheers,

Petr
Tom Rini March 17, 2020, 3:24 p.m. UTC | #4
On Tue, Mar 17, 2020 at 02:54:40PM +0100, Petr ?tetiar wrote:
> Tom Rini <trini at konsulko.com> [2020-03-17 09:20:38]:
> 
> Hi,
> 
> > On Tue, Mar 17, 2020 at 02:12:55PM +0100, Petr ?tetiar wrote:
> > 
> > > This reverts commit 4c78028737c3185f49f5691183aeac3478b5f699.
> > > 
> > > bl31.bin file is mandatory for functional, usable and bootable binaries,
> > > thus it should be hard error if bl31.bin is missing. It doesn't matter
> > > if I'm building on autobuilder or locally, the resulting binaries should
> > > be usable in both cases.
> > > 
> > > Cc: Simon Glass <sjg at chromium.org>
> > > Cc: Tom Rini <trini at konsulko.com>
> > > Cc: Andre Przywara <andre.przywara at arm.com>
> > > Cc: Maxime Ripard <maxime.ripard at free-electrons.com>
> > > Signed-off-by: Petr ?tetiar <ynezz at true.cz>
> > > ---
> > > 
> > > I've just spent some time hunting eMMC boot issue on a64-olinuxino. It's
> > > really easy to miss that warning message on fast build hosts as the message is
> > > scrolled out very quickly out of the screen and thus using the broken images,
> > > which would get stuck at `Trying to boot from MMC2`.
> > > 
> > > I think, that if it's desired to have broken images on the output, then it
> > > should be handled on the autobuilder itself before build of sunxi target.
> > > 
> > > Another option is probably adding AUTOBUILDER_ALLOW_BROKEN_IMAGES config
> > > option, which would make it clear for everybody.
> > > 
> > >  board/sunxi/mksunxi_fit_atf.sh | 6 ------
> > >  1 file changed, 6 deletions(-)
> > 
> > It's the case that the vast majority of aarch64 platforms only function
> > when we have other binary files available for the final link and so have
> > a similar we had hoped loud enough WARNING message.  And yes, this is so
> > that all of the different CI systems can complete build testing.
> 
> so there's some requirement, that the CI systems should always build with
> simple `make` call? You know, `make BL31=/dev/null` works just fine if having a
> green build, but unusable binaries is desired.

Yes, it's required that CI continue to pass, otherwise we run into the
case where you can't build the platform at all.  And no, it's not just a
simple 'make' call, please take a look at one or more of the CI scripts.

> > So, can you please RFC some patches such that we can still run CI but we
> > drop all of the "non-functional" WARNING messages we have?  Thanks!
> 
> If it's desired to bloat the codebase with config options and ifdefs just to
> adapt for unflexible CI systems, fine with me, I'll do so.

Everything is as (un)flexible as we make it.  The current approach, for
all the platforms that require various third-party binaries, is to shout
at the user they're making a binary that won't work.  Before this, we
had either silently making binaries that didn't work or defaulting to
not trying to build a final image format (and then problems introduced
in that last phase not being detected).

So if you can think of a cleaner approach that doesn't further
complicate CI nor end-users, please RFC some patches.  Thanks!
Tom Rini March 17, 2020, 4:24 p.m. UTC | #5
On Tue, Mar 17, 2020 at 11:24:23AM -0400, Tom Rini wrote:
> On Tue, Mar 17, 2020 at 02:54:40PM +0100, Petr ?tetiar wrote:
> > Tom Rini <trini at konsulko.com> [2020-03-17 09:20:38]:
> > 
> > Hi,
> > 
> > > On Tue, Mar 17, 2020 at 02:12:55PM +0100, Petr ?tetiar wrote:
> > > 
> > > > This reverts commit 4c78028737c3185f49f5691183aeac3478b5f699.
> > > > 
> > > > bl31.bin file is mandatory for functional, usable and bootable binaries,
> > > > thus it should be hard error if bl31.bin is missing. It doesn't matter
> > > > if I'm building on autobuilder or locally, the resulting binaries should
> > > > be usable in both cases.
> > > > 
> > > > Cc: Simon Glass <sjg at chromium.org>
> > > > Cc: Tom Rini <trini at konsulko.com>
> > > > Cc: Andre Przywara <andre.przywara at arm.com>
> > > > Cc: Maxime Ripard <maxime.ripard at free-electrons.com>
> > > > Signed-off-by: Petr ?tetiar <ynezz at true.cz>
> > > > ---
> > > > 
> > > > I've just spent some time hunting eMMC boot issue on a64-olinuxino. It's
> > > > really easy to miss that warning message on fast build hosts as the message is
> > > > scrolled out very quickly out of the screen and thus using the broken images,
> > > > which would get stuck at `Trying to boot from MMC2`.
> > > > 
> > > > I think, that if it's desired to have broken images on the output, then it
> > > > should be handled on the autobuilder itself before build of sunxi target.
> > > > 
> > > > Another option is probably adding AUTOBUILDER_ALLOW_BROKEN_IMAGES config
> > > > option, which would make it clear for everybody.
> > > > 
> > > >  board/sunxi/mksunxi_fit_atf.sh | 6 ------
> > > >  1 file changed, 6 deletions(-)
> > > 
> > > It's the case that the vast majority of aarch64 platforms only function
> > > when we have other binary files available for the final link and so have
> > > a similar we had hoped loud enough WARNING message.  And yes, this is so
> > > that all of the different CI systems can complete build testing.
> > 
> > so there's some requirement, that the CI systems should always build with
> > simple `make` call? You know, `make BL31=/dev/null` works just fine if having a
> > green build, but unusable binaries is desired.
> 
> Yes, it's required that CI continue to pass, otherwise we run into the
> case where you can't build the platform at all.  And no, it's not just a
> simple 'make' call, please take a look at one or more of the CI scripts.
> 
> > > So, can you please RFC some patches such that we can still run CI but we
> > > drop all of the "non-functional" WARNING messages we have?  Thanks!
> > 
> > If it's desired to bloat the codebase with config options and ifdefs just to
> > adapt for unflexible CI systems, fine with me, I'll do so.
> 
> Everything is as (un)flexible as we make it.  The current approach, for
> all the platforms that require various third-party binaries, is to shout
> at the user they're making a binary that won't work.  Before this, we
> had either silently making binaries that didn't work or defaulting to
> not trying to build a final image format (and then problems introduced
> in that last phase not being detected).
> 
> So if you can think of a cleaner approach that doesn't further
> complicate CI nor end-users, please RFC some patches.  Thanks!

Part of the issue in fact is that "simple make" isn't the best example
of how to build U-Boot:
$ ./tools/buildman/buildman -o /tmp/pinebook pinebook;echo $?
Building current source for 1 boards (1 thread, 16 jobs per thread)
   aarch64:  w+   pinebook
+WARNING: BL31 file bl31.bin NOT found, resulting binary is non-functional
+Please read the section on ARM Trusted Firmware (ATF) in board/sunxi/README.sunxi64
    0    1    0 /1      pinebook
129
$
diff mbox series

Patch

diff --git a/board/sunxi/mksunxi_fit_atf.sh b/board/sunxi/mksunxi_fit_atf.sh
index 88ad71974706..dea9aaa5691d 100755
--- a/board/sunxi/mksunxi_fit_atf.sh
+++ b/board/sunxi/mksunxi_fit_atf.sh
@@ -7,12 +7,6 @@ 
 
 [ -z "$BL31" ] && BL31="bl31.bin"
 
-if [ ! -f $BL31 ]; then
-	echo "WARNING: BL31 file $BL31 NOT found, resulting binary is non-functional" >&2
-	echo "Please read the section on ARM Trusted Firmware (ATF) in board/sunxi/README.sunxi64" >&2
-	BL31=/dev/null
-fi
-
 if grep -q "^CONFIG_MACH_SUN50I_H6=y" .config; then
 	BL31_ADDR=0x104000
 else