Re: [PATCH] ArmPlatformPkg/PlatformIntelBdsLib: don't clobber ConSplitter handle

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 3 March 2017 at 21:23, Leif Lindholm <leif.lindholm@linaro.org> 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" <terry.lee@hpe.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> 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 <leif.lindholm@linaro.org>
>
>> ---
>>
>> 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.