From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x234.google.com (mail-it0-x234.google.com [IPv6:2607:f8b0:4001:c0b::234]) (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 27EF6802BD for ; Fri, 3 Mar 2017 13:37:32 -0800 (PST) Received: by mail-it0-x234.google.com with SMTP id 203so20919387ith.0 for ; Fri, 03 Mar 2017 13:37:32 -0800 (PST) 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=Ghy1s+se7+IocR00Aohb6OuOoxOxngW5Nd45ZS4Vb4k=; b=T6uy7XS5RP50JWlWLOFkL2m8BZiozlSK+mcCX6D6Z/NMqwXO2rqGkCK3c0NEfKkDgf QFSnO5fTfyo3CFo67bHdhcaj1ZOZcjUu5M1fgAj0VKyQ9oaSlja0BVTMGP0vSEOBahWC Jxp2eBrwXhSfoSH62/r34PiLPfEwXFGE4gtsQ= 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=Ghy1s+se7+IocR00Aohb6OuOoxOxngW5Nd45ZS4Vb4k=; b=khq0nJgqyKQASlU5gHavuyzKXGsITIIZJRlsmNXjIb4vxX/UGrvvKBFbuH9qk9yPkn CoxbWiCOy2UwNoDtNiQ36mgzqfUpKc2WvAY12rTzSr5z/f4xQq28JBawUtzgb6c8h/7v 6X7lTu1bBkOfM5TFKhrDMNs7GBKhQDSEEhtQQTYJ1cOR6Ezj4u7w+LZU2reSGqhpR6dO kGC3hP6SA+W7ch62wfuvOxPUabxF7MShTMue+IpPkIAaDfdkHHj6l73H9zp1g4orem3L lo6yRFqgcYgN0wLnESlQi94WOtXUpYpDTJPMnS3GIbs+VVEC4rw2VV2ftZuTkiI6MC2j Ogmg== X-Gm-Message-State: AMke39kUTfhXXW1lVawhLQhcOvudWWzpEsv4CzaYnOFhesCckGoQPc91VvPRHbg32AwT8LDmqfKxGP8MuDhRM94q X-Received: by 10.36.77.10 with SMTP id l10mr4756475itb.59.1488577051368; Fri, 03 Mar 2017 13:37:31 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Fri, 3 Mar 2017 13:37:30 -0800 (PST) In-Reply-To: References: <1488393273-27283-1-git-send-email-ard.biesheuvel@linaro.org> <20170303212316.GS16034@bivouac.eciton.net> From: Ard Biesheuvel Date: Fri, 3 Mar 2017 21:37:30 +0000 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" , "Lee, Terry Ping-Chung" Subject: Re: [PATCH] ArmPlatformPkg/PlatformIntelBdsLib: don't clobber ConSplitter handle X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 03 Mar 2017 21:37:32 -0000 Content-Type: text/plain; charset=UTF-8 On 3 March 2017 at 21:32, Ard Biesheuvel wrote: > On 3 March 2017 at 21:23, Leif Lindholm wrote: >> On Wed, Mar 01, 2017 at 06:34:33PM +0000, Ard Biesheuvel wrote: >>> The InitializeConsolePipe() routine takes care to only set its output >>> argument *Interface if it is not already set, to prevent overwriting >>> the ConSplitter interface pointer that may have already been assigned. >>> >>> However, the associated OUT argument 'Handle' is clobbered by the >>> subsequent unnecessary LocateDevicePath() invocation, which should >>> similarly be made dependent on whether *Interface has been set >>> already. >>> >>> Reported-by: "Lee, Terry Ping-Chung" >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Ard Biesheuvel >> >> This looks good. I'll make one non-functional ecomment, and you can >> decide whether to incorporate it before pushing or not. >> >> Reviewed-by: Leif Lindholm >> >>> --- >>> >>> Terry: does this work for you? Thanks. >>> >>> ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c | 25 ++++++++++---------- >>> 1 file changed, 13 insertions(+), 12 deletions(-) >>> >>> diff --git a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c >>> index 885866854329..f8e04efea63d 100644 >>> --- a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c >>> +++ b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c >>> @@ -148,12 +148,19 @@ InitializeConsolePipe ( >>> >>> Status = BdsLibConnectDevicePath (DevicePath); >>> if (!EFI_ERROR (Status)) { >>> - // >>> - // If BdsLibConnectDevicePath () succeeded, *Handle must have a non-NULL >>> - // value. So ASSERT that this is the case. >>> - // >>> - gBS->LocateDevicePath (&gEfiDevicePathProtocolGuid, &DevicePath, Handle); >>> - ASSERT (*Handle != NULL); >>> + >>> + // If the console splitter driver is not supported by the platform then >>> + // use the first Device Path instance for the console interface. >> >> I realise this comment is just being moved, but it's confusing. It >> refers to a state that is only visible if you're already aware of 2 or >> 3 facts not visible at this point. >> >> If I understand the situation correctly, the following would be a more >> helpful description: >> >> // If the console splitter driver is supported by the platform, then >> // *Interface is already initialized. If it is not, use the first >> // successfully connected device as the console. >> > > Not entirely. The consplitter is one example of where multiple > consoles may be available, but it really applies to any situation > where multiple device paths are provided. So it is not that the > function is *entered* with *Interface non-null, but that any > additional iterations of the loop will clobber *Handle whereas > *Interface will only be set in the first iteration. > Actually, I suppose it may be entered with *Interface non-NULL, but even if it is NULL we should take care to update *Handle and *interface at the same time only.