Re: [edk2-devel] API breakages and their implications. Was: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support

["Laszlo Ersek" <lersek@redhat.com>]

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 <Library/PcdLib.h>
#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 <Library/SerialUartClockLib.h>
#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