From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web11.8643.1585661013701902544 for ; Tue, 31 Mar 2020 06:23:33 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=FrfjRQ74; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585661012; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=8N96cABRdD9I45561+ZrYu5yrTeNxO5L0Vlp65Jjtv4=; b=FrfjRQ74A/W9M7G1fO6Cvnt8gXhIA7BMa4hZbMuPSYWChqOghU0tUf5XA3h3FjUdbPc3YI c0ok+HZGZo6/y7TaoZUC4hrt+9yT6ZLBp3fikJRTQxuEsCszghF+oh0LyNHSvKFS4lXdyz Mp4bSB51PjjyHvQL+BmkDBiAKmmK8s0= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-422-svkEuWq2OKC1uQZzIVKaPQ-1; Tue, 31 Mar 2020 09:23:28 -0400 X-MC-Unique: svkEuWq2OKC1uQZzIVKaPQ-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 5CF59107B277; Tue, 31 Mar 2020 13:23:27 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-131.ams2.redhat.com [10.36.115.131]) by smtp.corp.redhat.com (Postfix) with ESMTP id 940B850BEE; Tue, 31 Mar 2020 13:23:25 +0000 (UTC) Subject: Re: [edk2-devel] API breakages and their implications. Was: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support To: Leif Lindholm , "Ni, Ray" Cc: Ard Biesheuvel , "Jiang, Guomin" , "devel@edk2.groups.io" , Andrew Fish , Michael D Kinney References: <20200219133135.10407-2-pankaj.bansal@oss.nxp.com> <734D49CCEBEEF84792F5B80ED585239D5C4A0C2C@SHSMSX104.ccr.corp.intel.com> <20200319153057.GV23627@bivouac.eciton.net> <20200330073518.GU22097@bivouac.eciton.net> <734D49CCEBEEF84792F5B80ED585239D5C4CD51C@SHSMSX104.ccr.corp.intel.com> <20200331092226.GB7468@vanye> From: "Laszlo Ersek" Message-ID: <2bb24cc3-96d1-5e37-b069-f009cf824aef@redhat.com> Date: Tue, 31 Mar 2020 15:23:24 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200331092226.GB7468@vanye> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/31/20 11:22, Leif Lindholm wrote: > On Tue, Mar 31, 2020 at 01:53:21 +0000, Ni, Ray wrote: >> Leif, >> Please understand that the concern of this change is all the platforms that uses >> this serial port lib must be changed otherwise build breaks. > > Yes. This is the nature of collaborative development. > This is something we on the ARM side have lived with since we got > involved with EDK2, being the less-deployed user of the codebase, and > that is fine. > > TianoCore specifically produced the UDK to make life easier for those > who are unable to track upstream, and we have followed that up with > stable tags. I would highly recommend that any team that feels that > this change would be too much effort to move to edk2-stable202002. Of > course, they would then need to manually cherry-pick any improvements > and security fixes from master. That is their choice. > > Nevertheless, breaking existing platforms is a side effect, not a > goal. So if an alternative solution can be found (which is not a hack > that is going to affect the codebase negatively over time and simply > need to be fixed properly at a later date), then clearly that is > preferable. > > "This breaks many platforms" is a good argument for seeing if a > solution can be found that does not break (as) many platforms. It is > not an argument for duplicating drivers when the change needed for > those platforms is trivial. > > These days, Linux tends to deal with API breakages (and other things) > using a semantic patch tool called Coccinelle[1]. It would not be > unreasonable, and indeed it would be helpful to us on the non-x86 side > if something similar was adopted (and semantic patches mandated) for > API breakages in EDK2. > > [1] http://coccinelle.lip6.fr/sp.php Two comments: (1) One of the reasons why I would like to keep all platforms in a single tree is to deal with API changes like this. That way, someone proposing an API change would at least have the chance to fix up all the consumer sites. Of course it would require diligent review from the other pkg maintainers, but it could be implemented without any temprary breakage in the git history even. (2) Specifically about this problem. The vendor GUID approach is not a bad one. How about the following alternative: (2.1) Patch#1: in the "MdeModulePkg/Library/BaseSerialPortLib16550" directory, introduce a header file called "GetClock.h". This header file should declare: UINT32 GetClock ( VOID ); Note: EFIAPI not needed. The header file should be added to the existent "BaseSerialPortLib16550.inf" file, in the [Sources] section. Furthermore, in the same patch, introduce a new source file called "GetClockPcd.c". (Add this file to the INF file as well.) The source file should do: #include #include "GetClock.h" UINT32 GetClock ( VOID ) { return PcdGet32 (PcdSerialClockRate); } Finally, in the same patch#1, modify "BaseSerialPortLib16550.c" to #include "GetClock.h", and to call GetClock() in place of the current PcdGet32 (PcdSerialClockRate) calls. This patch basically splits the PcdGet32 (PcdSerialClockRate) call to a separate translation unit, hiding it behind the more abstract GetClock() API. (2.2) Patch#2: In the *same* "MdeModulePkg/Library/BaseSerialPortLib16550" directory, introduce three new files: - BaseSerialPortLibDynamicFrequency16550.inf - BaseSerialPortLibDynamicFrequency16550.uni - GetClockDynamicFrequency.c I think the contents of these files must be fairly obvious at this point, but anyway: (2.2.1) INF file: - modified copy of the INF file from (2.1) - Update file-top comment, copyright notice, BASE_NAME, MODULE_UNI_FILE, FILE_GUID. - [LibraryClasses]: add dependency on SerialUartClockLib - [Sources]: replace "GetClockPcd.c" with "GetClockDynamicFrequency.c" - [Pcd]: drop PcdSerialClockRate (2.2.2) UNI file: copy and modify the existent UNI file as needed. (2.2.3) GetClockDynamicFrequency.c: #include #include "GetClock.h" UINT32 GetClock ( VOID ) { return BaseSerialPortGetClock (); } This way, platforms currently consuming "MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf" will see no change whatsoever. Platforms needing the dynamic frequency can resolve SerialPortLib to "MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLibDynamicFrequency16550.inf", *and* also resolve SerialUartClockLib to an instance that matches their needs. All implementation except the GetClock() one will be shared between the two lib instances "BaseSerialPortLib16550" and "BaseSerialPortLibDynamicFrequency16550". This is a frequent pattern in edk2 -- refer to the various module directories that contain two or more INF files. For example: $ git ls-files -- \ MdePkg/*inf \ MdeModulePkg/*inf \ NetworkPkg/*inf \ PcAtChipsetPkg/*inf \ SecurityPkg/*inf \ ShellPkg/*inf \ UefiCpuPkg/*inf \ | rev \ | cut -f 2- -d / \ | rev \ | sort \ | uniq -d This will produce a list of directories where each directory contains at least two INF files: MdeModulePkg/Bus/I2c/I2cDxe MdeModulePkg/Core/PiSmmCore MdeModulePkg/Library/DxeCapsuleLibFmp MdeModulePkg/Library/DxeCoreMemoryAllocationLib MdeModulePkg/Library/DxeResetSystemLib/UnitTest MdeModulePkg/Library/LzmaCustomDecompressLib MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib MdeModulePkg/Library/SmmLockBoxLib MdeModulePkg/Logo MdeModulePkg/Universal/CapsulePei MdeModulePkg/Universal/EbcDxe MdeModulePkg/Universal/FaultTolerantWriteDxe MdeModulePkg/Universal/Variable/RuntimeDxe MdePkg/Library/BaseIoLibIntrinsic MdePkg/Library/BaseUefiDecompressLib MdePkg/Library/PciSegmentLibSegmentInfo MdePkg/Library/UefiDevicePathLib MdePkg/Test/UnitTest/Library/BaseLib MdePkg/Test/UnitTest/Library/BaseSafeIntLib PcAtChipsetPkg/Library/AcpiTimerLib SecurityPkg/HddPassword SecurityPkg/Library/HashLibBaseCryptoRouter SecurityPkg/Library/Tpm2DeviceLibDTpm SecurityPkg/Library/Tpm2DeviceLibRouter SecurityPkg/Tcg/Opal/OpalPassword SecurityPkg/Tcg/Tcg2Config ShellPkg/DynamicCommand/DpDynamicCommand ShellPkg/DynamicCommand/TftpDynamicCommand UefiCpuPkg/CpuFeatures UefiCpuPkg/Library/CpuExceptionHandlerLib UefiCpuPkg/Library/CpuTimerLib UefiCpuPkg/Library/MpInitLib UefiCpuPkg/Library/RegisterCpuFeaturesLib UefiCpuPkg/Library/SmmCpuFeaturesLib UefiCpuPkg/PiSmmCommunication If you look at INF files inside any given such directory, you'll find that the [Sources] sections between them have non-empty intersections, but also differences. That's the exact same pattern we should use here, IMO. (I intentionally used the core packages in this example.) Thanks Laszlo