From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::241; helo=mail-it0-x241.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x241.google.com (mail-it0-x241.google.com [IPv6:2607:f8b0:4001:c0b::241]) (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 CC80B202E5CD3 for ; Fri, 20 Oct 2017 04:36:34 -0700 (PDT) Received: by mail-it0-x241.google.com with SMTP id o135so13099354itb.0 for ; Fri, 20 Oct 2017 04:40:14 -0700 (PDT) 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=XzVNLTV8lMML8legeQRelj270Sp+HLwhbG9Hll1cLYI=; b=fOtIDmBwDlW0DwpScqZ4eAPDqVMkmJudM6N1iimtGozmsPmCIHMboue+Jwciww/FPg G27kd08xlREL1cgCidEH8tgs/V29YmC//q+PTDIyO8t07lBwRIziy67NtEh7af5tyQL7 Hddndztsbl1JVPdwRQZAVJylTeLd9i++3apDY= 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=XzVNLTV8lMML8legeQRelj270Sp+HLwhbG9Hll1cLYI=; b=DTsp/TXwpgU/Cx8qK/1prGdC4WNgiRnPY991lZjagOQ+hd0GRrPJDhHvDa7ns6R3FO O6k3RC5fMNUsNynd/l/QZ7NHydB55nnPVNgIiiieBvPE6RVrv6Q1C35T9F8mS2T25FS1 pqD0tdDFbDemQb0ItxKPKmZK7V2VnCwf2/W0zB9pLeBaBQS4kxOKMrfZIkeHg1dDC3Jv mWJ3pw24rto+xC02FdFN3eaTOFWDkg9BBWj+2OJYdYCuRreFyruW0P4MtHOw3YSDsPVW sGQ1OM9CwQXXvAd+2tP/iJkEtbQgnHVh1Md6ARG2asSRpbE23YnVMr9cVzRwkMiIm3yr ycxA== X-Gm-Message-State: AMCzsaUfzPWBVdgHdw04c4lfcfOOhmvYdzkTTkHmmlDIgVgRs6mZ/4Jv dUxzhXxRccO61Sm7/10uTVayDtfRWiPbMlEFamMOTw== X-Google-Smtp-Source: ABhQp+SWoVEVxUPLFhPjyBtxacbEuXC6qZEtXHz+WJ5HiPCs6gpXt9zYx3C9GDnSpiimfmCUASdkwAe0ZhHjc1Um50g= X-Received: by 10.36.210.198 with SMTP id z189mr2003098itf.65.1508499613351; Fri, 20 Oct 2017 04:40:13 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.131.167 with HTTP; Fri, 20 Oct 2017 04:40:12 -0700 (PDT) In-Reply-To: References: <20171019192141.4782-1-ard.biesheuvel@linaro.org> From: Ard Biesheuvel Date: Fri, 20 Oct 2017 12:40:12 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Leif Lindholm Subject: Re: [PATCH] EmbeddedPkg/DtPlatformDxe: remove /chosen/stdout-path on GOP registration X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 20 Oct 2017 11:36:35 -0000 Content-Type: text/plain; charset="UTF-8" On 20 October 2017 at 11:44, Laszlo Ersek 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 >> --- >> 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 >> #include >> #include >> #include >> @@ -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.