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

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

On 20 October 2017 at 11:44, Laszlo Ersek <lersek@redhat.com> wrote:
> 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.
>

Good point, thanks. I realised failure was possible here if the
property does not exist to begin with (put perhaps the notify callback
should not be registered in the first place) hence the _INFO

>> +
>>  /**
>>    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.
>

Another good point.

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

I think the best way forward is to implement a separate driver with
its own HII form, that removes /chosen/stdout-path and uninstalls the
SPCR table if either or both are present. I couldn't figure out how to
uninstall a ACPI table though, so I may need to move the SPCR
installation into that driver as well.