Re: [PATCH v3 1/8] PCI: add constant PCI_STATUS_ERROR_BITS

[Heiner Kallweit <hkallweit1@gmail.com>]

On 25.02.2020 21:50, Bjorn Helgaas wrote:
> On Tue, Feb 25, 2020 at 03:03:44PM +0100, Heiner Kallweit wrote:
> 
> Run "git log --oneline drivers/pci" and make yours match.  In
> particular, capitalize the first word ("Add").  Same for the other PCI
> patches.  I don't know the drivers/net convention, but please find and
> follow that as well.
> 
>> This constant is used (with different names) in more than one driver,
>> so move it to the PCI core.
> 
> The driver constants in *this* patch at least use the same name.
> 
Right, I have to fix the description.

>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/ethernet/marvell/skge.h | 6 ------
>>  drivers/net/ethernet/marvell/sky2.h | 6 ------
>>  include/uapi/linux/pci_regs.h       | 7 +++++++
>>  3 files changed, 7 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/marvell/skge.h b/drivers/net/ethernet/marvell/skge.h
>> index 6fa7b6a34..e149bdfe1 100644
>> --- a/drivers/net/ethernet/marvell/skge.h
>> +++ b/drivers/net/ethernet/marvell/skge.h
>> @@ -15,12 +15,6 @@
>>  #define  PCI_VPD_ROM_SZ	7L<<14	/* VPD ROM size 0=256, 1=512, ... */
>>  #define  PCI_REV_DESC	1<<2	/* Reverse Descriptor bytes */
>>  
>> -#define PCI_STATUS_ERROR_BITS (PCI_STATUS_DETECTED_PARITY | \
>> -			       PCI_STATUS_SIG_SYSTEM_ERROR | \
>> -			       PCI_STATUS_REC_MASTER_ABORT | \
>> -			       PCI_STATUS_REC_TARGET_ABORT | \
>> -			       PCI_STATUS_PARITY)
>> -
>>  enum csr_regs {
>>  	B0_RAP	= 0x0000,
>>  	B0_CTST	= 0x0004,
>> diff --git a/drivers/net/ethernet/marvell/sky2.h b/drivers/net/ethernet/marvell/sky2.h
>> index b02b65230..851d8ed34 100644
>> --- a/drivers/net/ethernet/marvell/sky2.h
>> +++ b/drivers/net/ethernet/marvell/sky2.h
>> @@ -252,12 +252,6 @@ enum {
>>  };
>>  
>>  
>> -#define PCI_STATUS_ERROR_BITS (PCI_STATUS_DETECTED_PARITY | \
>> -			       PCI_STATUS_SIG_SYSTEM_ERROR | \
>> -			       PCI_STATUS_REC_MASTER_ABORT | \
>> -			       PCI_STATUS_REC_TARGET_ABORT | \
>> -			       PCI_STATUS_PARITY)
>> -
>>  enum csr_regs {
>>  	B0_RAP		= 0x0000,
>>  	B0_CTST		= 0x0004,
>> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
>> index 543769048..9b84a1278 100644
>> --- a/include/uapi/linux/pci_regs.h
>> +++ b/include/uapi/linux/pci_regs.h
>> @@ -68,6 +68,13 @@
>>  #define  PCI_STATUS_SIG_SYSTEM_ERROR	0x4000 /* Set when we drive SERR */
>>  #define  PCI_STATUS_DETECTED_PARITY	0x8000 /* Set on parity error */
>>  
>> +#define PCI_STATUS_ERROR_BITS (PCI_STATUS_DETECTED_PARITY  | \
>> +			       PCI_STATUS_SIG_SYSTEM_ERROR | \
>> +			       PCI_STATUS_REC_MASTER_ABORT | \
>> +			       PCI_STATUS_REC_TARGET_ABORT | \
>> +			       PCI_STATUS_SIG_TARGET_ABORT | \
>> +			       PCI_STATUS_PARITY)
> 
> This actually *adds* PCI_STATUS_SIG_TARGET_ABORT, which is not in the
> driver definitions.  At the very least that should be mentioned in the
> commit log.
> 
> Ideally the addition would be in its own patch so it's obvious and
> bisectable, but I see the problem -- the subsequent patches
> consolidate things that aren't really quite the same.  One alternative
> would be to have preliminary patches that change the drivers
> individually so they all use this new mask, then do the consolidation
> afterwards.
> 
I checked the other patches and we'd need such preliminary patches
for three of them:
marvell: misses PCI_STATUS_SIG_TARGET_ABORT
skfp: misses PCI_STATUS_REC_TARGET_ABORT
r8169: misses PCI_STATUS_PARITY

> There is pitifully little documentation about the use of include/uapi,
> but AFAICT that is for the user API, and this isn't part of that.  I
> think this #define could go in include/linux/pci.h instead.
> 
OK, then I'll change the series accordingly.

>> +
>>  #define PCI_CLASS_REVISION	0x08	/* High 24 bits are class, low 8 revision */
>>  #define PCI_REVISION_ID		0x08	/* Revision ID */
>>  #define PCI_CLASS_PROG		0x09	/* Reg. Level Programming Interface */
>> -- 
>> 2.25.1
>>
>>
>>
>>