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

[Leif Lindholm <leif.lindholm@linaro.org>]

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.

> +      if (*Interface == NULL) {
> +        //
> +        // 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);
> +
> +        gBS->HandleProtocol (*Handle, Protocol, Interface);
> +      }
>      }
>      DEBUG_CODE_BEGIN();
>        if (EFI_ERROR(Status)) {
> @@ -172,12 +179,6 @@ InitializeConsolePipe (
>          }
>        }
>      DEBUG_CODE_END();
> -
> -    // If the console splitter driver is not supported by the platform then use the first Device Path
> -    // instance for the console interface.
> -    if (!EFI_ERROR(Status) && (*Interface == NULL)) {
> -      Status = gBS->HandleProtocol (*Handle, Protocol, Interface);
> -    }
>    }
>  
>    // No Device Path has been defined for this console interface. We take the first protocol implementation
> -- 
> 2.7.4
>