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

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

On 01/13/21 07:00, Jiahui Cen wrote:
> 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?

If you can add a separate patch for that, it's appreciated. It's not
required though -- up to you.

Thanks
Laszlo