Re: [PATCH] MdeModulePkg/NonDiscoverable: Compare SIZE_4GB with address type

[Laszlo Ersek <lersek@redhat.com>]

On 01/11/17 11:59, Wu, Hao A wrote:
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Wednesday, January 11, 2017 3:55 PM
>> To: Ni, Ruiyu; Laszlo Ersek
>> Cc: Wu, Hao A; edk2-devel@lists.01.org
>> Subject: Re: [edk2] [PATCH] MdeModulePkg/NonDiscoverable: Compare
>> SIZE_4GB with address type
>>
>> On 11 January 2017 at 02:05, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
>>> One minor comments: put one space after (EFI_PHYSICAL_ADDRESS) and
>> (UINTN) as below.
>>>> +            (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress + *NumberOfBytes >
>> SIZE_4GB
>>>
>>
>> I disagree. The EDK2 coding style does not mandate a space after a
>> cast, and given the tight binding of the () cast operator in C, it is
>> counterinituitive.
> 
> Thanks Ard, I agree with your point. I will leave the patch unchanged.

Thanks! I agree those spaces are super counter-intuitive; in the past
we've had bugs due to them. It's for best to reflect the tight binding
of the cast operator in the source code, by keeping the operand directly
adjacent.

Cheers
Laszlo

> Best Regards,
> Hao Wu
> 
>>
>>> Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
>>>
>>>
>>>> -----Original Message-----
>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>>>> Hao Wu
>>>> Sent: Wednesday, January 11, 2017 10:01 AM
>>>> To: edk2-devel@lists.01.org
>>>> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Ard
>>>> Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Subject: [edk2] [PATCH] MdeModulePkg/NonDiscoverable: Compare
>>>> SIZE_4GB with address type
>>>>
>>>> Refine the codes to compare the definition 'SIZE_4GB' with type
>>>> EFI_PHYSICAL_ADDRESS.
>>>>
>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Hao Wu <hao.a.wu@intel.com>
>>>> ---
>>>>  .../Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c  |
>>>> 4 ++--
>>>>  .../NonDiscoverableDeviceRegistrationLib.c                            | 2 +-
>>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git
>>>> a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
>>>> PciDeviceIo.c
>>>> b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
>>>> PciDeviceIo.c
>>>> index b07c129..c836ad6 100644
>>>> ---
>>>> a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
>>>> PciDeviceIo.c
>>>> +++
>>>> b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
>>>> Pc
>>>> +++ iDeviceIo.c
>>>> @@ -598,7 +598,7 @@ CoherentPciIoMap (
>>>>    //
>>>>    Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This);
>>>>    if ((Dev->Attributes & EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE) ==
>>>> 0 &&
>>>> -      (UINTN)HostAddress + *NumberOfBytes > SIZE_4GB) {
>>>> +      (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress + *NumberOfBytes >
>>>> + SIZE_4GB) {
>>>>
>>>>      //
>>>>      // Bounce buffering is not possible for consistent mappings @@ -1006,7
>>>> +1006,7 @@ NonCoherentPciIoMap (
>>>>    // a bounce buffer and copy over the data in case HostAddress >= 4 GB.
>>>>    //
>>>>    Bounce = ((Dev->Attributes &
>>>> EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE) == 0 &&
>>>> -            (UINTN)HostAddress + *NumberOfBytes > SIZE_4GB);
>>>> +            (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress + *NumberOfBytes >
>>>> + SIZE_4GB);
>>>>
>>>>    if (!Bounce) {
>>>>      switch (Operation) {
>>>> diff --git
>>>> a/MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDisco
>>>> verableDeviceRegistrationLib.c
>>>> b/MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDisco
>>>> verableDeviceRegistrationLib.c
>>>> index 6f46dfa..4180b0a 100644
>>>> ---
>>>> a/MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDisco
>>>> verableDeviceRegistrationLib.c
>>>> +++
>>>> b/MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDisco
>>>> +++ verableDeviceRegistrationLib.c
>>>> @@ -163,7 +163,7 @@ RegisterNonDiscoverableMmioDevice (
>>>>      Desc->AddrLen               = Size;
>>>>      Desc->AddrRangeMax          = Base + Size - 1;
>>>>      Desc->ResType               = ACPI_ADDRESS_SPACE_TYPE_MEM;
>>>> -    Desc->AddrSpaceGranularity  = (Base + Size > SIZE_4GB) ? 64 : 32;
>>>> +    Desc->AddrSpaceGranularity  = ((EFI_PHYSICAL_ADDRESS) Base + Size >
>>>> + SIZE_4GB) ? 64 : 32;
>>>>      Desc->AddrTranslationOffset = 0;
>>>>    }
>>>>    VA_END (Args);
>>>> --
>>>> 1.9.5.msysgit.0
>>>>
>>>> _______________________________________________
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.01.org
>>>> https://lists.01.org/mailman/listinfo/edk2-devel