From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 85BBB2034715B for ; Fri, 20 Oct 2017 03:40:30 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E03CC883D9; Fri, 20 Oct 2017 10:44:08 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com E03CC883D9 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-150.rdu2.redhat.com [10.10.120.150]) by smtp.corp.redhat.com (Postfix) with ESMTP id C999C60E37; Fri, 20 Oct 2017 10:44:07 +0000 (UTC) To: Ard Biesheuvel , edk2-devel@lists.01.org Cc: leif.lindholm@linaro.org References: <20171019192141.4782-1-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: Date: Fri, 20 Oct 2017 12:44:06 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171019192141.4782-1-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Fri, 20 Oct 2017 10:44:09 +0000 (UTC) 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 10:40:30 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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. > + > /** > 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 >