Re: [edk2-devel] [PATCH v4 3/9] OvmfPkg/PciHostBridgeLib: Extract InitRootBridge/UninitRootBridge

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

On 01/12/21 10:45, Jiahui Cen via groups.io wrote:
> Extract InitRootBridge/UninitRootBridge to PciHostBridgeUtilityLib as
> common utility functions. No change of functionality.
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien@xen.org>
> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
> Signed-off-by: Yubo Miao <miaoyubo@huawei.com>
> ---
>  OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf               |   2 -
>  OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf |   7 +
>  OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h                   |  75 ++++++++++
>  OvmfPkg/Library/PciHostBridgeLib/PciHostBridge.h                    |  56 -------
>  OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c                 | 158 +-------------------
>  OvmfPkg/Library/PciHostBridgeLib/XenSupport.c                       |   3 +-
>  OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c   | 155 +++++++++++++++++++
>  7 files changed, 243 insertions(+), 213 deletions(-)

(1) Renaming OVMF_PCI_ROOT_BRIDGE_DEVICE_PATH to
PCI_ROOT_BRIDGE_DEVICE_PATH is not only unnecessary, it carries a risk
of conflicting with something that PciHostBridgeLib in MdeModulePkg
might introduce in the future. Please undo this rename.

(2) Renaming "mRootBridgeDevicePathTemplate" to "mRootBridgeDevicePath"
is not necessary. Although this rename is not risky, it makes the review
(before/after comparison) of this patch more difficult than needed.
Please undo this rename. The word "template" is correct BTW, because we
modify the device path after we allocate & copy it from the template --
see UID.

(3) Unfortunately, the original PciHostBridgeLib instance fails to list
its PcdLib dependency, both between the #include directives, and in the
INF file. I'm not asking you to fix that up, but please do spell out the
PcdLib #include and INF file depencency in the new
PciHostBridgeUtilityLib instance. (See the PcdGet16() call in
PciHostBridgeUtilityInitRootBridge().)


With the above small warts fixed, I'm ready to give R-b.

Thanks
Laszlo