From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c09::22e; helo=mail-wm0-x22e.google.com; envelope-from=haojian.zhuang@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x22e.google.com (mail-wm0-x22e.google.com [IPv6:2a00:1450:400c:c09::22e]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 205A3202E5F0E for ; Wed, 25 Apr 2018 20:12:28 -0700 (PDT) Received: by mail-wm0-x22e.google.com with SMTP id o78so10251187wmg.0 for ; Wed, 25 Apr 2018 20:12:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=4ThXi6PtwpoYpVkRjDSjziB/tkHTjOM02XmBDqMPkXY=; b=kTsqVOig4dhj0XVoFchMmRLm/99kJFPt5esaGxayht37yKjlVrcggjyeIa7gd+tpvv DZTXYWqtoTMUrICdbh7s097JMiCdK+6klSwxf28HCXUI5aY9HTwiLlDmz2v48BhH3iGy i7L5DtOzxtz8LSZup6qUJPe1pZmwv3oIxTqok= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=4ThXi6PtwpoYpVkRjDSjziB/tkHTjOM02XmBDqMPkXY=; b=rFOJw2hvIN/QTrgMI+tg5MNLBrVKmWcp2clAzmIkDLyeXXj4NC60KDDr2Fy99XL84w laBJFr1W6ztYRqUa9xEXrabvdDclPZF9jfRKzlI8JI497G24yPIbtvXjlyo2geZC5KWF d/O3VQqRLZBtinekF4BAArZ9LqdDIpv0PKIkD564S31V1us0lMejC88ekiHRd26xxtE8 VAUJvsBMbu9hyQXcjKaXiS6Haj0DRt4tjmxeuVgnAE6VDQfbqxW9fsTSFGfxXnJZ+2Rb Dj014DGihLQwLpF9+8TfzTlC2H7WW/s4Kq+aR88kersIcSoz9FizHk3mvY18vh+svrBF +VWw== X-Gm-Message-State: ALQs6tCNFhK+soOmbf9P8y+c40iMmQkW/YV+bFE+59pxu+cDOcaYZNyb F1dVWPWFMThHdop5uY1r6xaCcJpxuPWZ4p1N0ScCsw== X-Google-Smtp-Source: AB8JxZpovHYOmYO9RfupU8IzF0JhWBti1L8QmSlRtybsLyaihXTDCo81aZtM3V5++D4dM68OrQhWO1bqJBF7WmrGkk4= X-Received: by 10.28.110.30 with SMTP id j30mr3595346wmc.62.1524712346767; Wed, 25 Apr 2018 20:12:26 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.150.22 with HTTP; Wed, 25 Apr 2018 20:12:26 -0700 (PDT) In-Reply-To: <0a629a97-9b8b-08a5-c04f-5bba40a1a5ff@redhat.com> References: <1524632376-23501-1-git-send-email-haojian.zhuang@linaro.org> <0a629a97-9b8b-08a5-c04f-5bba40a1a5ff@redhat.com> From: Haojian Zhuang Date: Thu, 26 Apr 2018 11:12:26 +0800 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , "Leif Lindholm (Linaro address)" , Ard Biesheuvel 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: Thu, 26 Apr 2018 03:12:29 -0000 Content-Type: text/plain; charset="UTF-8" On 26 April 2018 at 00:10, Laszlo Ersek wrote: > 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. > OK > > (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. > Yes, it's depend on another patch series on virtual keyboard. > > (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. > OK. I'll remove it. > > (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. > For this one, I have a different opinion. EFI_STATUS EFIAPI EfiBootManagerInitializeLoadOption ( ... ) { if ((Option == NULL) || (Description == NULL) || (FilePath == NULL)) { return EFI_INVALID_PARAMETER; } if (((OptionalData != NULL) && (OptionalDataSize == 0)) || ((OptionalData == NULL) && (OptionalDataSize != 0))) { return EFI_INVALID_PARAMETER; } if ((UINT32) OptionType >= LoadOptionTypeMax) { return EFI_INVALID_PARAMETER; } ZeroMem (Option, sizeof (EFI_BOOT_MANAGER_LOAD_OPTION)); Option->OptionNumber = OptionNumber; Option->OptionType = OptionType; Option->Attributes = Attributes; Option->Description = AllocateCopyPool (StrSize (Description), Description); Option->FilePath = DuplicateDevicePath (FilePath); if (OptionalData != NULL) { Option->OptionalData = AllocateCopyPool (OptionalDataSize, OptionalData); Option->OptionalDataSize = OptionalDataSize; } return EFI_SUCCESS; } We can find that no memory is allocated to "DevicePath" if it returns with error. So we shouldn't free memory on "DevicePath" if "Status" is error code. Other issues will be fixed in v6. > > With (3) and (4) addressed, for the series: > > Acked-by: Laszlo Ersek > Thanks a lot. Best Regards Haojian