From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 906672282E592 for ; Wed, 25 Apr 2018 09:10:12 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9225181A8A12; Wed, 25 Apr 2018 16:10:11 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-20.rdu2.redhat.com [10.10.121.20]) by smtp.corp.redhat.com (Postfix) with ESMTP id C4AD22166BC6; Wed, 25 Apr 2018 16:10:10 +0000 (UTC) To: Haojian Zhuang References: <1524632376-23501-1-git-send-email-haojian.zhuang@linaro.org> Cc: edk2-devel@lists.01.org, "Leif Lindholm (Linaro address)" , Ard Biesheuvel From: Laszlo Ersek Message-ID: <0a629a97-9b8b-08a5-c04f-5bba40a1a5ff@redhat.com> Date: Wed, 25 Apr 2018 18:10:10 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <1524632376-23501-1-git-send-email-haojian.zhuang@linaro.org> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Wed, 25 Apr 2018 16:10:11 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Wed, 25 Apr 2018 16:10:11 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH edk2-platforms v5 0/2] add platform boot options X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Apr 2018 16:10:12 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hello Haojian, On 04/25/18 06:59, Haojian Zhuang wrote: > Changelog: > v5: > * Avoid to merge device path and grub's file path in driver. > Merge them directly in DSC file. > * Avoid duplicated code to create boot options. > * Use goto to handle error condition. > v4: > * Add BootCount parameter in the interface. > v3: > * Move in initilization of boot entry. > * Update the name of interface in platform boot manager protocol. > v2: > * Update with platform boot manager protocol. > > Haojian Zhuang (2): > Platform/HiKey960: register predefined boot options > Platform/HiKey: create 4 boot options > > Platform/Hisilicon/HiKey/HiKey.dec | 8 +- > Platform/Hisilicon/HiKey/HiKey.dsc | 7 + > Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c | 171 ++++++++++++++++++++ > Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf | 11 ++ > .../{HiKey/HiKey.dec => HiKey960/HiKey960.dec} | 17 +- > Platform/Hisilicon/HiKey960/HiKey960.dsc | 6 + > .../Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c | 172 +++++++++++++++++++++ > .../Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf | 11 ++ > 8 files changed, 391 insertions(+), 12 deletions(-) > copy Platform/Hisilicon/{HiKey/HiKey.dec => HiKey960/HiKey960.dec} (56%) > (1) in the future, please CC the blurb -- patch 0/xxx -- to everyone who are CC'd on any of the patches. What I usually do is: - format all the patches - grep them for '^Cc:' - sort the resultant list uniquely - paste the output into the blurb message This way, if a person is CC'd at least on one of the patches in the series, they will receive a copy of the cover letter too. (2) As of commit b581a4934d8f ("ARM/JunoPkg: Add HDLCD platform library", 2018-04-23), I cannot find HiKeyDxe or HiKey960Dxe in the edk2-platforms tree. That's not a problem, it just means that I cannot verify the resource management / error handling in the HiKey960EntryPoint() and HiKeyEntryPoint() functions. For this reason, I cannot give R-b, just A-b. I hope that will suffice -- please do not repost because of this. (3) The GRUB_FILE_NAME macro is now superfluous in both of the patches. Again, it's not necessary for me to review the series just because of such an update. Perhaps Leif or Ard can remove the macro defs for you when they push the patches. (4) In both patches, in both of the CreatePlatformBootOptionFromPath() and CreatePlatformBootOptionFromGuid() helper functions, the "DevicePath" variable is leaked when EfiBootManagerInitializeLoadOption() fails. "DevicePath" should be freed unconditionally at that point, in both patches and in both helper functions (4 instances in total). Simply eliminate the following: if (EFI_ERROR (Status)) { return Status; } and you will be left with: Status = EfiBootManagerInitializeLoadOption ( ... ); FreePool (DevicePath); return Status; I believe I need not separately review this update either. With (3) and (4) addressed, for the series: Acked-by: Laszlo Ersek Thanks Laszlo