From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x231.google.com (mail-io0-x231.google.com [IPv6:2607:f8b0:4001:c06::231]) (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 21602802BC for ; Fri, 3 Mar 2017 13:32:43 -0800 (PST) Received: by mail-io0-x231.google.com with SMTP id l7so83363019ioe.3 for ; Fri, 03 Mar 2017 13:32:43 -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=dvmtuZ0yqfsBxCdmHk7vhiWsDZNDAuUOuu7ABuQli7A=; b=HRnayCq0/L0nPMuvifXGaD9Ug8LrRzhqoURWAvFsbMbpqNFSFlNbz2Q9b40Ap77RQN Oq/lEss/14/05QryaZYgVjug1N61yaNraXTffrjjOKm07P3M5ATkn/LL4m5WIubkm56a qK/VB1JbRubleLjjGEGaaQOmxG9K4DIxpOHSQ= 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=dvmtuZ0yqfsBxCdmHk7vhiWsDZNDAuUOuu7ABuQli7A=; b=C/rLgYrF+olIGWSYXZaASvSuosN/2n2Hu+//+1qgWIKZ3LuA9qX7+gAe1LQtYhixTq tLMFKzhCwe7TwnKCOTYAyvtyKGYr3Sv5fO4x/iGAqHiHP2r+dhHmZKG7gUQILc0JJxWi kyH/FeTFlAnwIDGrxcix6a7QgtQl6Rue0/9Nbg+KpKvk2y6cGrlTsglWznKrVH494AmG m1E50rpdVmfOTyYa9XIV3VVf3/WolCS24DQ0b3mrNtY35n52e6hPCSyZcUp82p21Ey5E sCHgXhzDh8FF03vo8zNv1E0rJJodvKUVtbLC4SexJJ4+luczNTlR8rs2FUDsG/GZ//QI zb1A== X-Gm-Message-State: AMke39k7C9MwcHzzgjHWgYPI+CutFKrmU7lSP4pgm+EPVr+NpQIxUTY3Mp2qOPGP/VbB53zrC3wg6iRdLl1FNqv9 X-Received: by 10.107.168.21 with SMTP id r21mr4507764ioe.45.1488576762406; Fri, 03 Mar 2017 13:32:42 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Fri, 3 Mar 2017 13:32:42 -0800 (PST) In-Reply-To: <20170303212316.GS16034@bivouac.eciton.net> 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:32:42 +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:32:43 -0000 Content-Type: text/plain; charset=UTF-8 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. I will go ahead and apply this with a clarified comment.