| Message ID | 73dd692e-bbce-35f5-88e9-417fb0f7229e@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: add and use constant PCI_STATUS_ERROR_BITS and helper pci_status_get_and_clear_errors | expand |
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.
> 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.
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.
> +
> #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
>
>
>
>
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 >> >> >> >>
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) + #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 */
This constant is used (with different names) in more than one driver, so move it to the PCI core. 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(-)