| Message ID | 20200317131255.26856-1-ynezz@true.cz |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Revert "mksunxi_fit_atf.sh: Allow for this to complete when bl31.bin is missing" | expand |
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@chromium.org> > Cc: Tom Rini <trini@konsulko.com> > Cc: Andre Przywara <andre.przywara@arm.com> > Cc: Maxime Ripard <maxime.ripard@free-electrons.com> > Signed-off-by: Petr Štetiar <ynezz@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!
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@chromium.org> > Cc: Tom Rini <trini@konsulko.com> > Cc: Andre Przywara <andre.przywara@arm.com> > Cc: Maxime Ripard <maxime.ripard@free-electrons.com> > Signed-off-by: Petr Štetiar <ynezz@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 >
Tom Rini <trini@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@chromium.org> > > Cc: Tom Rini <trini@konsulko.com> > > Cc: Andre Przywara <andre.przywara@arm.com> > > Cc: Maxime Ripard <maxime.ripard@free-electrons.com> > > Signed-off-by: Petr Štetiar <ynezz@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
On Tue, Mar 17, 2020 at 02:54:40PM +0100, Petr Štetiar wrote: > Tom Rini <trini@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@chromium.org> > > > Cc: Tom Rini <trini@konsulko.com> > > > Cc: Andre Przywara <andre.przywara@arm.com> > > > Cc: Maxime Ripard <maxime.ripard@free-electrons.com> > > > Signed-off-by: Petr Štetiar <ynezz@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!
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@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@chromium.org> > > > > Cc: Tom Rini <trini@konsulko.com> > > > > Cc: Andre Przywara <andre.przywara@arm.com> > > > > Cc: Maxime Ripard <maxime.ripard@free-electrons.com> > > > > Signed-off-by: Petr Štetiar <ynezz@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 --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
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@chromium.org> Cc: Tom Rini <trini@konsulko.com> Cc: Andre Przywara <andre.przywara@arm.com> Cc: Maxime Ripard <maxime.ripard@free-electrons.com> Signed-off-by: Petr Štetiar <ynezz@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(-)