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@xxxxxxxxx>
>> ---
>> 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
>>
>>
>>
>>
