Re: [PATCH] EmbeddedPkg/DtPlatformDxe: remove /chosen/stdout-path on GOP registration

[Laszlo Ersek <lersek@redhat.com>]

On 10/19/17 21:21, Ard Biesheuvel wrote:
> The presence of a /chosen/stdout-path property will force Linux to use
> the serial port as the primary console, even if a graphical console is
> available as well. But the presence of the Graphics Output Protocol (GOP)
> is a strong indication that the user may prefer to use his keyboard and
> mouse rather than his terminal emulator to interact with the system, so
> let's remove /chosen/stdout-path as soon as a GOP instance is registered.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c   | 41 ++++++++++++++++++++
>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf |  4 ++
>  2 files changed, 45 insertions(+)

Some general notes (I understand you might not proceed with this
direction after all):

> 
> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
> index 1014be2281d4..4f9ac8090fac 100644
> --- a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
> @@ -12,6 +12,7 @@
>  *
>  **/
>  
> +#include <libfdt.h>
>  #include <Library/BaseLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/DevicePathLib.h>
> @@ -54,6 +55,9 @@ STATIC HII_VENDOR_DEVICE_PATH     mDtPlatformDxeVendorDevicePath = {
>    }
>  };
>  
> +STATIC EFI_EVENT                  mRegisterGopEvent;
> +STATIC VOID                       *mGopRegistration;
> +
>  STATIC
>  EFI_STATUS
>  InstallHiiPages (
> @@ -89,6 +93,32 @@ InstallHiiPages (
>    return EFI_SUCCESS;
>  }
>  
> +STATIC
> +VOID
> +OnRegisterGop (
> +  IN EFI_EVENT  Event,
> +  IN VOID       *Dtb
> +  )
> +{
> +  INT32     Node;
> +  INT32     Error;
> +
> +  DEBUG ((DEBUG_INFO,
> +    "%a: a GOP has been registered, removing /chosen/stdout-path from DT\n",
> +    __FUNCTION__));
> +
> +  Node = fdt_path_offset (Dtb, "/chosen");
> +  if (Node < 0) {
> +    return;
> +  }
> +
> +  Error = fdt_delprop (Dtb, Node, "stdout-path");
> +  if (Error) {
> +    DEBUG ((DEBUG_INFO, "%a: Failed to delete 'stdout-path' property: %a\n",
> +      __FUNCTION__, fdt_strerror (Error)));
> +  }
> +}

In theory multiple GOPs can be installed. The last DEBUG message seems
to imply that the node removal is expected to succeed on the first call,
and then shouldn't be attempted. For this (i.e., for "one-shot" protocol
notifies), I suggest closing the event in the callback. That covers the
de-registration automatically.

> +
>  /**
>    The entry point for DtPlatformDxe driver.
>  
> @@ -181,6 +211,17 @@ DtPlatformDxeEntryPoint (
>          __FUNCTION__));
>        goto FreeDtb;
>      }
> +
> +    Status = gBS->CreateEvent (EVT_NOTIFY_SIGNAL, TPL_CALLBACK,
> +                    OnRegisterGop, Dtb, &mRegisterGopEvent);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "%a: failed to create event - %r\n",
> +        __FUNCTION__, Status));
> +    }
> +
> +    Status = gBS->RegisterProtocolNotify (&gEfiGraphicsOutputProtocolGuid,
> +                    mRegisterGopEvent, &mGopRegistration);

If CreateEvent() fails, RegisterProtocolNotify() should not be called on
mRegisterGopEvent.

> +    ASSERT_EFI_ERROR (Status);
>    } else {
>      ASSERT (FALSE);
>    }

This function seems intent on releasing resources on error exit (see the
"FreeDtb" label and the many jumps to it). In that spirit, I suggest
replacing the ASSERT_EFI_ERROR() with a jump to a new error label, for
closing mRegisterGopEvent.

(Again, I know this could be total moot after Leif's feedback; I was a
bit curious nonetheless.)

Thanks
Laszlo

> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
> index 45dfd9088bf0..6b331294df8a 100644
> --- a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
> @@ -41,6 +41,7 @@ [LibraryClasses]
>    BaseLib
>    DebugLib
>    DtPlatformDtbLoaderLib
> +  FdtLib
>    HiiLib
>    MemoryAllocationLib
>    UefiBootServicesTableLib
> @@ -53,6 +54,9 @@ [Guids]
>    gEdkiiPlatformHasAcpiGuid
>    gFdtTableGuid
>  
> +[Protocols]
> +  gEfiGraphicsOutputProtocolGuid
> +
>  [Depex]
>    gEfiVariableArchProtocolGuid        AND
>    gEfiVariableWriteArchProtocolGuid
>