Re: [PATCH v3 6/6] MdePkg/Pci22.h: Deprecate out-of-Spec IncompatiblePciDevice macros

[Laszlo Ersek <lersek@redhat.com>]

On 02/08/17 01:58, Ni, Ruiyu wrote:
> These macros comply with the spec so I had no reason to deprecate them.
> 
> But I did want to avoid using them in drivers.
> 
> Maybe I could just deprecate the two macros PCI_ACPI_UNUSED and
> PCI_BAR_NOCHANGE.
> 
> What do you think?

I'm fine either way:
- deprecating these two macros (and keeping patch #5 as-is, modulo the
  whitespace fix) sounds good,
- not deprecating these two macros (and then preserving them in OVMF
  too) sounds good as well.

What I don't really understand is, what speaks against using the two
macros if they are not deprecated / against the spec?

But, again, if you deprecate these two as well, that's totally fine by me.

Thanks!
Laszlo

> 
>  
> 
> Regards,
> 
> Ray
> 
>  
> 
> *From:*edk2-devel [mailto:edk2-devel-bounces@lists.01.org] *On Behalf Of
> *Laszlo Ersek
> *Sent:* Wednesday, February 8, 2017 1:43 AM
> *To:* Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@ml01.01.org
> *Cc:* Fan, Jeff <jeff.fan@intel.com>
> *Subject:* Re: [edk2] [PATCH v3 6/6] MdePkg/Pci22.h: Deprecate
> out-of-Spec IncompatiblePciDevice macros
> 
>  
> 
> On 02/07/17 04:33, Ruiyu Ni wrote:
>> DEVICE_ID_NOCARE is defined as 0xFFFF but Spec says (UINT64) -1
>> should be used to match any VendorId/DeviceId/RevisionId/
>> SubsystemVendorId/SubsystemDeviceId.
>> 
>> PCI_BAR_OLD_ALIGN/PCI_BAR_EVEN_ALIGN/PCI_BAR_SQUAD_ALIGN/
>> PCI_BAR_DQUAD_ALIGN are defined but Spec doesn't have such
>> definitions.
>> 
>> PCI_BAR_ALL is defined as 0xFF but Spec says (UINT64) -1 should be
>> used to match all BARs.
>> 
>> All of the above macros are marked as deprecated.
>> 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com <mailto:ruiyu.ni@intel.com>>
>> Reviewed-by: Liming Gao <liming.gao@intel.com <mailto:liming.gao@intel.com>>
>> Cc: Jeff Fan <jeff.fan@intel.com <mailto:jeff.fan@intel.com>>
>> ---
>>  MdePkg/Include/IndustryStandard/Pci22.h | 24 +++++++++++++++++-------
>>  1 file changed, 17 insertions(+), 7 deletions(-)
>> 
>> diff --git a/MdePkg/Include/IndustryStandard/Pci22.h b/MdePkg/Include/IndustryStandard/Pci22.h
>> index 4cf8389..5dec65d 100644
>> --- a/MdePkg/Include/IndustryStandard/Pci22.h
>> +++ b/MdePkg/Include/IndustryStandard/Pci22.h
>> @@ -7,7 +7,7 @@
>>      PC Card Standard, 8.0
>>      PCI Power Management Interface Specifiction, Revision 1.2
>>  
>> -  Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
>> +  Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
>>    Copyright (c) 2014 - 2015, Hewlett-Packard Development Company, L.P.<BR>
>>    This program and the accompanying materials                          
>>    are licensed and made available under the terms and conditions of the BSD License         
>> @@ -780,14 +780,25 @@ typedef struct {
>>    ///
>>  } EFI_PCI_CAPABILITY_HOTPLUG;
>>  
>> -#define DEVICE_ID_NOCARE    0xFFFF
>> +///
>> +/// Below macros (till PCI_BAR_NOCHANGE) were used by EfiIncompatiblePciDeviceSupport Protocol.
>> +///
>> +#ifndef DISABLE_NEW_DEPRECATED_INTERFACES
>> +
>> +///
>> +/// [ATTENTION] These macros are deprecated because they don't match Spec or not defined in Spec.
>> +///
>> +#define DEVICE_ID_NOCARE    0xFFFF                 ///< Deprecated. Value doesn't match Spec.
>> +#define PCI_BAR_OLD_ALIGN   0xFFFFFFFFFFFFFFFFULL  ///< Deprecated. Value isn't defined in Spec.
>> +#define PCI_BAR_EVEN_ALIGN  0xFFFFFFFFFFFFFFFEULL  ///< Deprecated. Value isn't defined in Spec.
>> +#define PCI_BAR_SQUAD_ALIGN 0xFFFFFFFFFFFFFFFDULL  ///< Deprecated. Value isn't defined in Spec.
>> +#define PCI_BAR_DQUAD_ALIGN 0xFFFFFFFFFFFFFFFCULL  ///< Deprecated. Value isn't defined in Spec.
>> +#define PCI_BAR_ALL         0xFF                   ///< Deprecated. Value doesn't match Spec.
>> +
>> +#endif
>>  
>>  #define PCI_ACPI_UNUSED     0
>>  #define PCI_BAR_NOCHANGE    0
> 
> Any particular reason for not deprecating these two macros? Is it an
> oversight, or is it intentional to keep them?
> 
> (If they comply with the spec, then replacing them in patch #5 seems
> unwarranted.)
> 
> Thanks,
> Laszlo
> 
>> -#define PCI_BAR_OLD_ALIGN   0xFFFFFFFFFFFFFFFFULL
>> -#define PCI_BAR_EVEN_ALIGN  0xFFFFFFFFFFFFFFFEULL
>> -#define PCI_BAR_SQUAD_ALIGN 0xFFFFFFFFFFFFFFFDULL
>> -#define PCI_BAR_DQUAD_ALIGN 0xFFFFFFFFFFFFFFFCULL
>>  
>>  #define PCI_BAR_IDX0        0x00
>>  #define PCI_BAR_IDX1        0x01
>> @@ -795,7 +806,6 @@ typedef struct {
>>  #define PCI_BAR_IDX3        0x03
>>  #define PCI_BAR_IDX4        0x04
>>  #define PCI_BAR_IDX5        0x05
>> -#define PCI_BAR_ALL         0xFF
>>  
>>  ///
>>  /// EFI PCI Option ROM definitions
>> 
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel
>