From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 11497C19F2D for ; Sat, 13 Aug 2022 12:36:59 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id A1E9B843FB; Sat, 13 Aug 2022 14:36:57 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=konsulko.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=konsulko.com header.i=@konsulko.com header.b="WGTGoGbT"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 3F70484409; Sat, 13 Aug 2022 14:36:56 +0200 (CEST) Received: from mail-qt1-x829.google.com (mail-qt1-x829.google.com [IPv6:2607:f8b0:4864:20::829]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id AFE3284180 for ; Sat, 13 Aug 2022 14:36:52 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=konsulko.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=trini@konsulko.com Received: by mail-qt1-x829.google.com with SMTP id h21so2595779qta.3 for ; Sat, 13 Aug 2022 05:36:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=konsulko.com; s=google; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc; bh=8q9JwYSsuxLQjhufTBNBYbq432nWKf9+uTxCWnPqv88=; b=WGTGoGbT6IWyrQ1TVfm+b401bvWydgH3pHss+bo52eaI3WTiSK3VZOwXDK15uELg8V nqHTZEktu4nhmh4kSByFLcmuSj8Dv7p7PzJO338V86maC8RKtTj6gJiS+CjvIwAbxsP5 +14MP2Dz4etVEn7rrWLbP2lp9HRh9265yaUAM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc; bh=8q9JwYSsuxLQjhufTBNBYbq432nWKf9+uTxCWnPqv88=; b=DcqDytM2x5V6hD+Nvy9+g4A3aFqosfllqOT8eMvn9ye+1B0IrurTpb8Dp+rR41HacD xuoaTf9aya+U+SeHgj6WUMJzz+h8858/WkwXXRH2twKo2xQMXXnf2vZJpc6zp+puVdww nMU2jTdzfSGYQr/Z8IC84S9lMvYbvz6toch3WT6xZ6ULuC6HGYypIakkGcATi54F9hiZ qwMetRWYeirkACF0MVDFF2kfwnY/SGAQYwOW3Oh87aIoXYwwfQmA6jHFMdQGSQ72tcR1 An7WrjYAyVWg0+h4/9+wevFS5gADiYXOGLnrUeyTvY9AxAat7zYEsmdVQPaS3h0/U5mT 9+/g== X-Gm-Message-State: ACgBeo3bugtsWcEa6aMkA9+KzCs1kiIsbDjJswzfJaFXODQC18fLQF+x a/I9r8BIF6AzRDGbRAgT+m3zHQ== X-Google-Smtp-Source: AA6agR7NZyWoAUdOPPzKWNpMgoDsuPIOyMei9ZTtXGtFaI2cyY9/dZX7K9UODW0yRQth3WJTbFrQ3A== X-Received: by 2002:ac8:5f47:0:b0:31f:1087:525a with SMTP id y7-20020ac85f47000000b0031f1087525amr7113495qta.619.1660394211311; Sat, 13 Aug 2022 05:36:51 -0700 (PDT) Received: from bill-the-cat (cpe-65-184-195-139.ec.res.rr.com. [65.184.195.139]) by smtp.gmail.com with ESMTPSA id u11-20020a05620a430b00b006b99b78751csm4203948qko.112.2022.08.13.05.36.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 13 Aug 2022 05:36:50 -0700 (PDT) Date: Sat, 13 Aug 2022 08:36:48 -0400 From: Tom Rini To: Simon Glass Cc: Tim Harvey , u-boot Subject: Re: binman warning/failure when blobs are missing Message-ID: <20220813123648.GZ1146598@bill-the-cat> References: <20220811164850.GY1146598@bill-the-cat> <20220811174356.GB1146598@bill-the-cat> <20220812010350.GD1146598@bill-the-cat> <20220812161109.GF1146598@bill-the-cat> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="NNTAO55NBBh1bmhY" Content-Disposition: inline In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean --NNTAO55NBBh1bmhY Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Aug 12, 2022 at 08:21:05PM -0600, Simon Glass wrote: > Hi Tom, >=20 > On Fri, 12 Aug 2022 at 10:11, Tom Rini wrote: > > > > On Fri, Aug 12, 2022 at 09:11:11AM -0600, Simon Glass wrote: > > > Hi Tom, > > > > > > On Thu, 11 Aug 2022 at 19:03, Tom Rini wrote: > > > > > > > > On Thu, Aug 11, 2022 at 06:08:51PM -0600, Simon Glass wrote: > > > > > Hi, > > > > > > > > > > On Thu, 11 Aug 2022 at 11:44, Tom Rini wrote: > > > > > > > > > > > > On Thu, Aug 11, 2022 at 09:59:05AM -0700, Tim Harvey wrote: > > > > > > > On Thu, Aug 11, 2022 at 9:48 AM Tom Rini = wrote: > > > > > > > > > > > > > > > > On Thu, Aug 11, 2022 at 09:27:44AM -0700, Tim Harvey wrote: > > > > > > > > > > > > > > > > > Greetings, > > > > > > > > > > > > > > > > > > After a couple of hours troubleshooting a bad boot image = today I > > > > > > > > > realized the issue was that I had some 0 byte files for t= he lpddr4 > > > > > > > > > training blobs that are part of the imx8mp binman created= image. > > > > > > > > > > > > > > > > > > Digging in I found that if a blob referenced in the binma= n node is > > > > > > > > > missing a warning will be output but the missing files wi= ll be > > > > > > > > > 'created' as 0 byte files such that the next time you bui= ld you will > > > > > > > > > get no warning (but will have a non-working image). Addit= ionally the > > > > > > > > > error does not cause a non-zero exit code. > > > > > > > > > > > > > > > > > > I'm not that fluent in python these days, and don't have = the time for > > > > > > > > > a while to try and fix this but I figured I would at leas= t send this > > > > > > > > > email in case someone else does. > > > > > > > > > > > > > > > > > > Example: > > > > > > > > > # rm *lpddr4*.bin # make sure lpddr4*.bin files reference= d in binman > > > > > > > > > nodes are missing > > > > > > > > > # make distclean imx8mp_venice_defconfig flash.bin && ech= o "build ok" > > > > > > > > > ... > > > > > > > > > BINMAN flash.bin > > > > > > > > > Image 'main-section' is missing external blobs and is non= -functional: ddr-1d-ime > > > > > > > > > m-fw ddr-1d-dmem-fw ddr-2d-imem-fw ddr-2d-dmem-fw > > > > > > > > > Image 'main-section' has faked external blobs and is non-= functional: lpddr4_pmu_ > > > > > > > > > train_1d_imem_202006.bin lpddr4_pmu_train_1d_dmem_202006.= bin lpddr4_pmu_train_2d > > > > > > > > > _imem_202006.bin lpddr4_pmu_train_2d_dmem_202006.bin > > > > > > > > > > > > > > > > > > Some images are invalid > > > > > > > > > ^^^ excellent warning > > > > > > > > > build ok > > > > > > > > > ^^^ not so great that there is a successful exit code > > > > > > > > > # make flash.bin && echo "build ok" > > > > > > > > > ... > > > > > > > > > BINMAN flash.bin > > > > > > > > > build ok > > > > > > > > > ^^^ absolutely horrible that 0 byte files were created an= d thus > > > > > > > > > everything looks good this time around! > > > > > > > > > # stat -c "%s %n" lpddr4*.bin > > > > > > > > > 0 lpddr4_pmu_train_1d_dmem_202006.bin > > > > > > > > > 0 lpddr4_pmu_train_1d_imem_202006.bin > > > > > > > > > 0 lpddr4_pmu_train_2d_dmem_202006.bin > > > > > > > > > 0 lpddr4_pmu_train_2d_imem_202006.bin > > > > > > > > > > > > > > > > So, this isn't the first time someone has had this problem.= On the one > > > > > > > > hand, we need CI to pass, and not require fetching of arbit= rary further > > > > > > > > images to assemble the binary. On the other hand, we don't = want users > > > > > > > > spending a bunch of time because something didn't work and = the normal > > > > > > > > way of conveying THIS WON'T WORK is a non-zero exit status.= Can we > > > > > > > > easily make some flag for buildman or binman that we do set= in CI but > > > > > > > > won't be set by users? > > > > > > > > > > > > > > > > > > > > > > Tom, > > > > > > > > > > > > > > Ok - that makes sense as far as the exit status goes. It woul= d be MUCH > > > > > > > easier to catch an error like this if binman didn't create 0 = byte > > > > > > > files for missing files so that you at least get the (ascii c= olored) > > > > > > > message indicating the image is wrong. > > > > > > > > > > Please see this: > > > > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20220807154708.1= 418967-2-sjg@chromium.org/ > > > > > > > > > > > > > > > > > > > I do love the idea of a flag for CI also! > > > > > > > > > > > > So, that's part of the root of the problem here. We're passing > > > > > > --fake-ext-blobs to binman so that it will create those 0 byte = files > > > > > > and it in turn complain. I think we didn't figure out a good wa= y to > > > > > > tell buildman to pass that to binman tho? > > > > > > > > > > I hope the above patch will fix this part of the problem. > > > > > > > > I'm not sure. What I really want to see is something like buildman > > > > --fake-ext-blobs, which in turn does 'make BINMANFLAGS=3D--fake-ext= -blobs' > > > > or 'make BINMAN_FAKE_EXT_BLOBS=3D1' and that is what sets --fake-ex= t-blobs > > > > to be passed inside of cmd_binman, so that we can tell CI to do that > > > > (just like we do -E today, to ensure -Werror) but regular users get > > > > build failures. > > > > > > IMO we should add my patch as well as what you are saying here. They > > > are handling slightly different issues. > > > > Yes, the above is useful, to fix a different issue (and should fix some > > issues in my before/after world builds). >=20 > OK good. Yes, a different issue as you say. >=20 > > > > > As to this one, I can make binman return an error result if any images > > > are invalid, controlled with a -W flag, perhaps, as with buildman. > > > > > > It is not just faked blobs - we need to raise a warning/error for > > > missing ones too. > > > > > > Then we could pass -W from CI using an env var picked up by the > > > Makefile as you say. It could control whether --allow-missing and > > > --fake-ext-blobs are passed to binman. > > > > > > Then a normal build will produce errors. > > > > > > We still need buildman to work locally though, for build testing. I > > > think returning exit code 101 could be good enough for that. > > > > > > Anyway, as I say, these are things for future improvement in addition > > > to my patch, IMO. > > > > I really think this needs to be a fails by default situation, and a new > > flag to buildman to say allow for CI and similar test builds to > > complete (that in turn passes the right flag(s) to binman), and complain > > without non-zero exit status. But the default should be visible / > > descriptive failure with a normal non-zero exit status. >=20 > I've been thinking about this. In practice there is no way that a > 'buildman -b' build can have the right binary blobs in place. Where > would they come from? So it would always fail. I don't think that is > really what we want? I use buildman in this way all the time. I also > hate binary blobs and don't want them to interrupt my workflow. As you > know the LTO issue has more than doubled my incremental build times > for sandbox and some other boards and I've been in this situation for > a long time (please can you apply that fix? :-) >=20 > I agree that the default should be a visible / descriptive failure, > but perhaps we can start with sorting out Makefile, so that by default > it fails and you have to pass an arg to get it to handle missing or > fake blobs. I should have done that at the start. That should fix most > people. This is your env var idea, and I agree with that. I can do a > patch. >=20 > Then we get to the people who use buildman for their builds instead of > make. That has recently included me on some machines, now we have the > --ide and -w flags, but I'm not sure how common it is. We don't > document buildman as a way of doing development. Anyway, we need this > to fail too, by default. In this case it is normally building a single > board and using the -w flag. >=20 > So perhaps we could have a different rule for -b or multiple boards > than for a 'current source' or '-w' build? I am not sure how we could > do this cleanly, but we could perhaps have a flag like --blobs with > several options: >=20 > --blobs fail | allow | fake >=20 > where the default value (if no --blobs flag is provided) varies > depending on other flags? >=20 > As I read this I suppose you will think this is overkill. But I am > really thinking about how this would affect development and there are > so many things already in the way. I think we're almost on the same page here. What I want is for: tools/buildman/buildman --fake-ext-blobs to do 'make BINMAN_FAKE_BLOBS=3D1' and so this then works: diff --git a/Makefile b/Makefile index 1a66f69a4b14..6c406735564c 100644 --- a/Makefile +++ b/Makefile @@ -1346,8 +1346,8 @@ cmd_binman =3D $(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 u-boot.dtb -O . -m --allow-missing \ - --fake-ext-blobs \ + build -u -d u-boot.dtb -O . -m \ + $(if $(BINMAN_FAKE_BLOBS),--allow-missing --fake-ext-blobs) \ -I . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \ -I arch/$(ARCH)/dts -a of-list=3D$(CONFIG_OF_LIST) \ $(foreach f,$(BINMAN_INDIRS),-I $(f)) \ So that then you're either using buildman and can pass the new flag to get binman to act like before, or you're going to see a more visible failure when using make and not do what needs to be done (which isn't always consistent, sigh) to say where to find what blobs. --=20 Tom --NNTAO55NBBh1bmhY Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQGzBAABCgAdFiEEGjx/cOCPqxcHgJu/FHw5/5Y0tywFAmL3mtkACgkQFHw5/5Y0 tyyHsgwAm5kJrf900Hv6n+xGeHJzYZnkm0ZATzy6xJLlB7ziVbRlgpgQyF39llB7 EEsfDEfyJXytmfn51K/al8s7WySljcdwbUi2CSeGdn0tN/Upy9czOW26E5+idwyL Dhm8Ge6w4JLbIE13Y/3OGcVD/+ekS0cfMsG1GWkJu/U4hfkVBicOXBoKPJdGB0bv /mL3lbYqcfNuY85f7F+4Yz7qAhhvKmVswHfCxWZKhuEmo0xouLueuY+bkaS5XOR8 uj865yfIwGvhBGGSqyglL0BBEHQ3xUZKAwmPpaip51HFKrYGXOhf2vqUaUBe8IfB 6U7o+JVb+EK+vEZaR8nGtzJpxf814CI5/Ohg81Adgv2l23aiD23foGu7a26S5NC9 hgqfVn1dvbXMy8TtyVzX7Cwsor7ifG94J/WnfIUqQUwQLDNw+520rTi8Pf5yHQi7 33DGhYpZ26283mamq1dR1L5dCualX+k0GyMF6c4WgAG7cxzxzckXInSl1rRiFohu WacNghqi =CGhR -----END PGP SIGNATURE----- --NNTAO55NBBh1bmhY--