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 A5E0EC636CC for ; Wed, 15 Feb 2023 18:25:56 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 9310985A53; Wed, 15 Feb 2023 19:25:54 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=reject dis=none) header.from=kwiboo.se Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=kwiboo.se header.i=@kwiboo.se header.b="m1pqwY+o"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 8A89685A54; Wed, 15 Feb 2023 19:25:51 +0100 (CET) Received: from wrqvxttq.outbound-mail.sendgrid.net (wrqvxttq.outbound-mail.sendgrid.net [149.72.167.116]) (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 EEF3385A5A for ; Wed, 15 Feb 2023 19:25:41 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=reject dis=none) header.from=kwiboo.se Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=bounces+31435339-7456-u-boot=lists.denx.de@em2124.kwiboo.se DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kwiboo.se; h=mime-version:subject:references:from:in-reply-to:to:cc:content-type: content-transfer-encoding:cc:content-type:from:subject:to; s=s1; bh=guX4ldNzrph858sIYeGX1hbU2TMNTRSp998UaF09jw0=; b=m1pqwY+o0kd5olkRbkbak8CYPtQKeICesrvPpUmiG+XiltCQnXGKBPsLb31rgzm4M6gO AH1SRTQOhPu4fpO87NauUSZkNrI/5ZZwfkBjilzNwTfJ8oHVCSYrwylv/9AMwFnk3GGKeg KdNTx1Nnsxlg4gnWWGKAO/l+xJogUvbrXQFREYAMazHrk/t299BjuOKTOcDPn4ZhSUA9v5 hoLY9jpc1WS/nmlG/FtXMpQ8yqRDuNcorvesCFinKyA9S6TEISBw5+ecnsQVtv/P1pCSPQ bapfAmsvpkbrjgDB7dyrG6VuU5czDxLd3BusMQKP8HFKOE6/AOAYkFERbhLYE2Xg== Received: by filterdrecv-7765ff4f7d-686qh with SMTP id filterdrecv-7765ff4f7d-686qh-1-63ED2399-3D 2023-02-15 18:25:30.195005723 +0000 UTC m=+430085.182996712 Received: from [192.168.1.50] (unknown) by geopod-ismtpd-3-2 (SG) with ESMTP id zEPHZgwDQr2HL6RLqzTkwA Wed, 15 Feb 2023 18:25:29.465 +0000 (UTC) Message-ID: <3af1296f-b50b-7c51-a20c-a3c4815c6161@kwiboo.se> Date: Wed, 15 Feb 2023 18:25:30 +0000 (UTC) MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.7.2 Subject: Re: [PATCH v2 6/6] RFC: binman: Improve allow missing for mkimage entry Content-Language: en-US References: <20230205202116.2891673-1-jonas@kwiboo.se> <20230214103300.690542-1-jonas@kwiboo.se> <20230214103300.690542-7-jonas@kwiboo.se> From: Jonas Karlman In-Reply-To: X-SG-EID: =?us-ascii?Q?TdbjyGynYnRZWhH+7lKUQJL+ZxmxpowvO2O9SQF5CwCVrYgcwUXgU5DKUU3QxA?= =?us-ascii?Q?fZekEeQsTe+RrMu3cja6a0hx6Molo9c5c+WanqV?= =?us-ascii?Q?ahF7M35ejKzf7W2TqKQEqXQ85mKJwf90m+uH01u?= =?us-ascii?Q?5tdYShX6URuAP7UM6LV1rst6mpeyJTYZ6Z68nwO?= =?us-ascii?Q?G7zlTB+73PudE5WQlZdL04svbbO5iisESsiBMsC?= =?us-ascii?Q?JA7r0KlHPnEHy=2FtMJNO5ceFPOGnlUvjpnFz5yx?= To: Simon Glass Cc: Philipp Tomsich , Kever Yang , Joseph Chen , Alper Nebi Yasak , Quentin Schulz , Jagan Teki , Heinrich Schuchardt , u-boot@lists.denx.de X-Entity-ID: P7KYpSJvGCELWjBME/J5tg== Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit 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 Hi Simon, On 2023-02-14 20:48, Simon Glass wrote: > Hi Jonas, > > On Tue, 14 Feb 2023 at 03:34, Jonas Karlman wrote: >> >> Implement CheckMissing and CheckOptional methods that is adapted to >> Entry_mkimage in order to improve support for allow missing flag. >> >> Use collect_contents_to_file in multiple-data-files handling to improve >> support for allow missing and fake blobs handling. >> >> Signed-off-by: Jonas Karlman >> --- >> Building for RK3568 without ROCKCHIP_TPL will result in the following >> error message, regardless if BINMAN_ALLOW_MISSING is used or not. >> >> binman: Filename 'rockchip-tpl' not found in input path (...) >> >> With this patch and using BINMAN_ALLOW_MISSING=1 a new external blob >> with no content is created and then passed to mkimage, resulting in an >> error from the mkimage command. >> >> binman: Error 255 running 'mkimage -d ./mkimage-0.simple-bin.mkimage:./mkimage-1.simple-bin.mkimage -n rk3568 -T rksd ./idbloader.img': mkimage: Can't read ./mkimage-0.simple-bin.mkimage: Invalid argument >> >> If the --fake-ext-blobs argument is also used I get the message I was >> expecting to see from the beginning. >> >> Image 'main-section' is missing external blobs and is non-functional: rockchip-tpl >> >> /binman/simple-bin/mkimage/rockchip-tpl: >> An external TPL is required to initialize DRAM. Get the external TPL >> binary and build with ROCKCHIP_TPL=/path/to/ddr.bin. One possible source >> for the external TPL binary is https://github.com/rockchip-linux/rkbin>>> Image 'main-section' has faked external blobs and is non-functional: rockchip-tpl >> >> Some images are invalid >> >> Not sure how this should work, but I was expecting to see the message >> about the missing rockchip-tpl from the beginning instead of the generic >> "not found in input path" message. At leas with BINMAN_ALLOW_MISSING=1. >> >> tools/binman/etype/mkimage.py | 37 +++++++++++++++++++++++++++++++---- >> 1 file changed, 33 insertions(+), 4 deletions(-) >> >> diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py >> index cb264c3cad0b..44510a8c40ba 100644 >> --- a/tools/binman/etype/mkimage.py >> +++ b/tools/binman/etype/mkimage.py >> @@ -153,10 +153,12 @@ class Entry_mkimage(Entry): >> if self._multiple_data_files: >> fnames = [] >> uniq = self.GetUniqueName() >> - for entry in self._mkimage_entries.values(): >> - if not entry.ObtainContents(fake_size=fake_size): >> + for idx, entry in enumerate(self._mkimage_entries.values()): >> + entry_data, entry_fname, _ = self.collect_contents_to_file( >> + [entry], 'mkimage-%s' % idx, fake_size) >> + if entry_data is None: > > This is OK, I suppose, but I'm not quite sure what actually changes > here, other than writing each entry to a file? The collect_contents_to_file function seemed to handle the external/missing/optional/faked entry flags. So I changed to use that function in order to see if that would change anything, see below. Use of this function does make it possible to use entry type other then external blobs, not sure if that would ever be needed/useful. > > Also, if you do this, please add / extend a test that checks that the > output files are written, since there is otherwise no coverage here. > > What test uses these optional mkimage pieces? Sorry, I was not clear enough about the reason for these changes in my message above. Building with --rockchip-tpl-path=/path/to/existing/tpl works as expected and generates a working image. I was expecting that the missing-blob-help message added in patch 1 would be shown by binman when rockchip-tpl-path was empty/not-existing, or at least together with the allow-missing flag. However, whatever I tested, empty/none-existing rockchip-tpl-path, allow-missing, fake-ext-blobs, I was not able to see this message. Instead, all I could get from binman was this "Filename 'rockchip-tpl' not found in input path" message. Maybe my assumptions about when these missing messages should be shown is wrong? Trying binman with the following two dts and --allow-missing gives different results. First one shows the missing message, second one show filename not found. binman { rockchip-tpl { }; }; binman { mkimage { args = "-n rk3568 -T rksd"; multiple-data-files; rockchip-tpl { }; }; }; With the changes in this patch I instead get the missing message when I also add the --fake-ext-blobs flag, so it clearly needs more work or a completely different approach if we want to be able to see the missing message added in patch 1. Adding a message that never will be displayed annoys me :-) Maybe I should just drop this rfc/patch for a v3 of this series? Regards, Jonas > >> return False >> - fnames.append(tools.get_input_filename(entry.GetDefaultFilename())) >> + fnames.append(entry_fname) >> input_fname = ":".join(fnames) >> else: >> data, input_fname, uniq = self.collect_contents_to_file( >> @@ -165,7 +167,7 @@ class Entry_mkimage(Entry): >> return False >> if self._imagename: >> image_data, imagename_fname, _ = self.collect_contents_to_file( >> - [self._imagename], 'mkimage-n', 1024) >> + [self._imagename], 'mkimage-n', fake_size) >> if image_data is None: >> return False >> outfile = self._filename if self._filename else 'mkimage-out.%s' % uniq >> @@ -216,6 +218,20 @@ class Entry_mkimage(Entry): >> if self._imagename: >> self._imagename.SetAllowFakeBlob(allow_fake) >> >> + def CheckMissing(self, missing_list): >> + """Check if any entries in this section have missing external blobs >> + >> + If there are missing (non-optional) blobs, the entries are added to the >> + list >> + >> + Args: >> + missing_list: List of Entry objects to be added to >> + """ >> + for entry in self._mkimage_entries.values(): >> + entry.CheckMissing(missing_list) >> + if self._imagename: >> + self._imagename.CheckMissing(missing_list) >> + >> def CheckFakedBlobs(self, faked_blobs_list): >> """Check if any entries in this section have faked external blobs >> >> @@ -229,6 +245,19 @@ class Entry_mkimage(Entry): >> if self._imagename: >> self._imagename.CheckFakedBlobs(faked_blobs_list) >> >> + def CheckOptional(self, optional_list): >> + """Check the section for missing but optional external blobs >> + >> + If there are missing (optional) blobs, the entries are added to the list >> + >> + Args: >> + optional_list (list): List of Entry objects to be added to >> + """ >> + for entry in self._mkimage_entries.values(): >> + entry.CheckOptional(optional_list) >> + if self._imagename: >> + self._imagename.CheckOptional(optional_list) >> + >> def AddBintools(self, btools): >> super().AddBintools(btools) >> self.mkimage = self.AddBintool(btools, 'mkimage') >> -- >> 2.39.1 >> > > Regards, > Simon