Re: [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

On 9/25/2018 10:35 AM, Zeng, Star wrote:
> On 2018/9/21 19:12, Laszlo Ersek wrote:
>> On 09/21/18 09:25, Ruiyu Ni wrote:
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>>> Cc: Star Zeng <star.zeng@intel.com>
>>> ---
>>>   .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c     | 26 
>>> ++++++++++------------
>>>   1 file changed, 12 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c 
>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> index f6234b5d11..916709e276 100644
>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> @@ -21,6 +21,8 @@ extern EDKII_IOMMU_PROTOCOL        *mIoMmuProtocol;
>>>   #define NO_MAPPING  (VOID *) (UINTN) -1
>>> +#define RESOURCE_VALID(R) ((R).Base <= (R).Limit)
>>> +
>>>   //
>>>   // Lookup table for increment values based on transfer widths
>>>   //
>>> @@ -122,25 +124,25 @@ CreateRootBridge (
>>>     //
>>>     // Make sure Mem and MemAbove4G apertures are valid
>>>     //
>>> -  if (Bridge->Mem.Base <= Bridge->Mem.Limit) {
>>> +  if (RESOURCE_VALID (Bridge->Mem)) {
>>>       ASSERT (Bridge->Mem.Limit < SIZE_4GB);
>>>       if (Bridge->Mem.Limit >= SIZE_4GB) {
>>>         return NULL;
>>>       }
>>>     }
>>> -  if (Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) {
>>> +  if (RESOURCE_VALID (Bridge->MemAbove4G)) {
>>>       ASSERT (Bridge->MemAbove4G.Base >= SIZE_4GB);
>>>       if (Bridge->MemAbove4G.Base < SIZE_4GB) {
>>>         return NULL;
>>>       }
>>>     }
>>> -  if (Bridge->PMem.Base <= Bridge->PMem.Limit) {
>>> +  if (RESOURCE_VALID (Bridge->PMem)) {
>>>       ASSERT (Bridge->PMem.Limit < SIZE_4GB);
>>>       if (Bridge->PMem.Limit >= SIZE_4GB) {
>>>         return NULL;
>>>       }
>>>     }
>>> -  if (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit) {
>>> +  if (RESOURCE_VALID (Bridge->PMemAbove4G)) {
>>>       ASSERT (Bridge->PMemAbove4G.Base >= SIZE_4GB);
>>>       if (Bridge->PMemAbove4G.Base < SIZE_4GB) {
>>>         return NULL;
>>> @@ -157,11 +159,9 @@ CreateRootBridge (
>>>         // support separate windows for Non-prefetchable and 
>>> Prefetchable
>>>         // memory.
>>>         //
>>> -      ASSERT (Bridge->PMem.Base > Bridge->PMem.Limit);
>>> -      ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
>>> -      if ((Bridge->PMem.Base <= Bridge->PMem.Limit) ||
>>> -          (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
>>> -          ) {
>>> +      ASSERT (!RESOURCE_VALID (Bridge->PMem));
>>> +      ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
>>> +      if (RESOURCE_VALID (Bridge->PMem) || RESOURCE_VALID 
>>> (Bridge->PMemAbove4G)) {
>>>           return NULL;
>>>         }
>>>       }
>>> @@ -171,11 +171,9 @@ CreateRootBridge (
>>>         // If this bit is not set, then the PCI Root Bridge does not 
>>> support
>>>         // 64 bit memory windows.
>>>         //
>>> -      ASSERT (Bridge->MemAbove4G.Base > Bridge->MemAbove4G.Limit);
>>> -      ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
>>> -      if ((Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) ||
>>> -          (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
>>> -          ) {
>>> +      ASSERT (!RESOURCE_VALID (Bridge->MemAbove4G));
>>> +      ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
>>> +      if (RESOURCE_VALID (Bridge->MemAbove4G) || RESOURCE_VALID 
>>> (Bridge->PMemAbove4G)) {
>>>           return NULL;
>>>         }
>>>       }
>>>
>>
>> Two superficial comments:
>>
>> - edk2 prefers long parameter names, so I suggest replacing "R" in the
>> macro definition with "Resource"
>>
>> - taking the parameter as a pointer is frequently considered more 
>> flexible.
>>
>> #define RESOURCE_VALID(Resource) ((Resource)->Base <= (Resource)->Limit)
>> ....
>> if (RESOURCE_VALID (&Bridge->Mem)) {
>> ....
>>
>> Up to you -- if you like these, feel free to update the patch before
>> pushing it (from my side anyway; you do need MdeModulePkg maintainer
>> review as well).
> 
> I have no strong preference here. Let Ray to make the choice.
> I have another very small comment.
> Is it better to add "#define RESOURCE_VALID(R) ((R).Base <= (R).Limit)" 
> in PciRootBridge.h?
> Also move "#define NO_MAPPING  (VOID *) (UINTN) -1" into PciRootBridge.h?
> And also move "extern EDKII_IOMMU_PROTOCOL        *mIoMmuProtocol;" into 
> PciHostBridge.h?

The NO_MAPPING, RESOURCE_VALID macros are only used in this C file.
I prefer to put them in PciRootBridge.h only when another C file needs 
to reference these macros.

I agree moving mIoMmuProtocol to PciHostBridge.h.
I am happy to do that in a separate patch in V2.

Agree?

> 
> Thanks,
> Star
> 
>>
>> With or without changes:
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Thanks
>> Laszlo
>>
> 


-- 
Thanks,
Ray