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

["Jiahui Cen" <cenjiahui@huawei.com>]

Hi Laszlo,

On 2021/1/13 9:28, Laszlo Ersek wrote:
> 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.
> 

Right. I will fix them.

> (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().)
> 

For the original PciHostBridgeLib, should I fix the dependency
in a new patch? Or just in this patch?

Thanks,
Jiahui

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