[PATCH 0/4] PCI: PCIe capability structure related cleanup/fixes

[PATCH 0/4] PCI: PCIe capability structure related cleanup/fixes

[Myron Stowe]

The following series introduces PCI Express 'capability structure'
related cleanup, fixes, and optimizations.

Patch 1/4 changes pci_ltr_supported() to a static routine.

Patch 2/4 removes redundant checking in various PCI Express features as
suggested by Bjorn Helgaas in
http://marc.info/?l=linux-pci&m=130463494319762&w=2

There is a similar idiom in use that could be similarly be re-factored:
    if (!pci_is_pcie(dev))
	return;

    pos = pci_find_ext_capability(dev, ...);
    if (!pos)
	return;

At first it seemed incorrect to remove the redundant call of
pci_is_pcie() in these cases as a PCI or PCI-X (< 2.0) device may be
involved.  In such cases an "extended capability" list would not exist,
as it was not introduced until PCI-X 2.0, and accesses outside of the
device's configuration space would be attempted.  However, upon further
review of pci_find_ext_capability() it looks as if such accesses would
be handled correctly due to the short-circuiting logic involved -

    if (pci_read_config_dword(dev, pos, &header) != PCIBIOS_SUCCESSFUL)
	return 0;

As such, I'll entertain comments as to whether or not we should also
make similar removals of pci_is_pcie() in these cases.

Patch 3/4 introduces pci_pcie_cap2() for use in v2 capability related
feature code.  The makeup of Express' capability structure varies
substantially between v1 and v2.

There is still some redundancy in PCIe v2 capabilities checking related
to the Latency Tolerance Reporting (LTR) feature routines that likely
could be re-factored further; please feel free to respond with ideas.

Patch 4/4 makes a minor optimization to the saving and restoring of
PCI Express capability structures.

Seems like the same type of optimization could be done to remove the
'if (pcie_cap_has_lnkctl(dev->pcie_type, flags))' check.  According to
section 7.8 "PCI Express Capability Structure" of the PCI Express 1.0a
specification:

    "Figure 7-10 details allocation of register fields in the PCI
     Express Capability structure. The PCI Express Capabilities,
     Device Capabilities, Device Status/Control, Link Capabilities,
     and Link Status/Control registers are required for all PCI
     Express devices. Endpoints are not required to implement
     registers other than those listed above and terminate the
     capability structure."

There may have been some early Express devices that did not properly
follow the specification which required the introduction of
'pcie_cap_has_lnkctl()' so I did not make the additional optimization.
---

Myron Stowe (4):
      PCI: Remove redundant capabilities checking in pci_{save,restore}_pcie_state
      PCI: Add pci_pcie_cap2() check for PCIe feature capabilities >= v2
      PCI: Remove redundant checking in PCI Express capability routines
      PCI: make pci_ltr_supported static.


 drivers/pci/pci.c        |   93 ++++++++++++++++++++++++++++------------------
 include/linux/pci.h      |    1 
 include/linux/pci_regs.h |    7 +++
 3 files changed, 64 insertions(+), 37 deletions(-)

-- 

[PATCH 1/4] PCI: make pci_ltr_supported static.

[Myron Stowe]

The PCI Express Latency Tolerance Reporting (LTR) feature's
pci_ltr_supported() routine is currently only used within
drivers/pci/pci.c so make it static.

Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
---

 drivers/pci/pci.c   |    4 ++--
 include/linux/pci.h |    1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 447e834..64471b1 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -39,6 +39,7 @@ EXPORT_SYMBOL(pci_pci_problems);
 
 unsigned int pci_pm_d3_delay;
 
+static bool pci_ltr_supported(struct pci_dev *dev);
 static void pci_pme_list_scan(struct work_struct *work);
 
 static LIST_HEAD(pci_pme_list);
@@ -2169,7 +2170,7 @@ EXPORT_SYMBOL(pci_disable_obff);
  * RETURNS:
  * True if @dev supports latency tolerance reporting, false otherwise.
  */
-bool pci_ltr_supported(struct pci_dev *dev)
+static bool pci_ltr_supported(struct pci_dev *dev)
 {
 	int pos;
 	u32 cap;
@@ -2185,7 +2186,6 @@ bool pci_ltr_supported(struct pci_dev *dev)
 
 	return cap & PCI_EXP_DEVCAP2_LTR;
 }
-EXPORT_SYMBOL(pci_ltr_supported);
 
 /**
  * pci_enable_ltr - enable latency tolerance reporting
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d8c379d..b2bec26 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -875,7 +875,6 @@ enum pci_obff_signal_type {
 int pci_enable_obff(struct pci_dev *dev, enum pci_obff_signal_type);
 void pci_disable_obff(struct pci_dev *dev);
 
-bool pci_ltr_supported(struct pci_dev *dev);
 int pci_enable_ltr(struct pci_dev *dev);
 void pci_disable_ltr(struct pci_dev *dev);
 int pci_set_ltr(struct pci_dev *dev, int snoop_lat_ns, int nosnoop_lat_ns);

[PATCH 2/4] PCI: Remove redundant checking in PCI Express capability routines

[Myron Stowe]

There are a number of redundant pci_is_pcie() checks in various PCI
Express capabilities related routines like the following:

    if (!pci_is_pcie(dev))
	return false;

    pos = pci_pcie_cap(dev);
    if (!pos)
	return false;

The current pci_is_pcie() implementation is merely:

    static inline bool pci_is_pcie(struct pci_dev *dev)
    {
        return !!pci_pcie_cap(dev);
    }

so we can just drop the pci_is_pcie() test in such cases.

Suggested by Bjorn Helgaas in -
http://marc.info/?l=linux-pci&m=130463494319762&w=2

Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
---

 drivers/pci/pci.c |   14 +-------------
 1 files changed, 1 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 64471b1..ff0beb0 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1995,7 +1995,7 @@ void pci_enable_ari(struct pci_dev *dev)
 		return;
 
 	bridge = dev->bus->self;
-	if (!bridge || !pci_is_pcie(bridge))
+	if (!bridge)
 		return;
 
 	pos = pci_pcie_cap(bridge);
@@ -2055,9 +2055,6 @@ void pci_disable_ido(struct pci_dev *dev, unsigned long type)
 	int pos;
 	u16 ctrl;
 
-	if (!pci_is_pcie(dev))
-		return;
-
 	pos = pci_pcie_cap(dev);
 	if (!pos)
 		return;
@@ -2097,9 +2094,6 @@ int pci_enable_obff(struct pci_dev *dev, enum pci_obff_signal_type type)
 	u16 ctrl;
 	int ret;
 
-	if (!pci_is_pcie(dev))
-		return -ENOTSUPP;
-
 	pos = pci_pcie_cap(dev);
 	if (!pos)
 		return -ENOTSUPP;
@@ -2150,9 +2144,6 @@ void pci_disable_obff(struct pci_dev *dev)
 	int pos;
 	u16 ctrl;
 
-	if (!pci_is_pcie(dev))
-		return;
-
 	pos = pci_pcie_cap(dev);
 	if (!pos)
 		return;
@@ -2175,9 +2166,6 @@ static bool pci_ltr_supported(struct pci_dev *dev)
 	int pos;
 	u32 cap;
 
-	if (!pci_is_pcie(dev))
-		return false;
-
 	pos = pci_pcie_cap(dev);
 	if (!pos)
 		return false;

[PATCH 3/4] PCI: Add pci_pcie_cap2() check for PCIe feature capabilities >= v2

[Myron Stowe]

This patch resolves potential issues when accessing PCI Express
capability structures.  The makeup of Express' capability structure
varies substantially between v1 and v2:

    PCI Express 1.0 and 1.1 base neither requires the endpoint to
    implement the entire PCIe capability structure nor specifies
    default values of registers that are not implemented by the
    device.  Its capability version is 1.

    PCIe 1.1 Capability Structure Expansion ECN, PCIe 2.0, 2.1, and
    3.0 added additional registers to the structure and require all
    registers to be either implemented or hardwired to 0.  Their PCIe
    capability version is 2.

Due to the differences in the capability structures, code dealing with
capability features must be careful not to access the additional
registers introduced with v2 unless the device is specifically known to
be a v2 capable device.  Otherwise, attempts to access non-existant
registers will occur.  This is a subtle issue that is hard to track down
when it occurs (and it has - see commit 864d296cf94).

To try and help mitigate such occurrances, this patch introduces
pci_pcie_cap2() which is similar to pci_pcie_cap() but also checks
that the PCIe capability version is >= 2.  pci_pcie_cap2() should be
used for qualifying PCIe capability features introduced after v1.

Suggested by Don Dutile.

Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
---

 drivers/pci/pci.c        |   65 +++++++++++++++++++++++++++++++++++-----------
 include/linux/pci_regs.h |    7 +++++
 2 files changed, 57 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ff0beb0..26933ff 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -279,6 +279,38 @@ int pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap)
 }
 
 /**
+ * pci_pcie_cap2 - query for devices' PCI_CAP_ID_EXP v2 capability structure
+ * @dev: PCI device to check
+ *
+ * Like pci_pcie_cap() but also checks that the PCIe capability version is
+ * >= 2.  Note that v1 capability structures could be sparse in that not
+ * all register fields were required.  v2 requires the entire structure to
+ * be present size wise, while still allowing for non-implemented registers
+ * to exist but they must be hardwired to 0.
+ *
+ * Due to the differences in the versions of capability structures, one
+ * must be careful not to try and access non-existant registers that may
+ * exist in early versions - v1 - of Express devices.
+ *
+ * Returns the offset of the PCIe capability structure as long as the
+ * capability version is >= 2; otherwise 0 is returned.
+ */
+static int pci_pcie_cap2(struct pci_dev *dev)
+{
+	u16 flags;
+	int pos;
+
+	pos = pci_pcie_cap(dev);
+	if (pos) {
+		pci_read_config_word(dev, pos + PCI_EXP_FLAGS, &flags);
+		if ((flags & PCI_EXP_FLAGS_VERS) < 2)
+			pos = 0;
+	}
+
+	return pos;
+}
+
+/**
  * pci_find_ext_capability - Find an extended capability
  * @dev: PCI device to query
  * @cap: capability code
@@ -1984,7 +2016,7 @@ void pci_enable_ari(struct pci_dev *dev)
 {
 	int pos;
 	u32 cap;
-	u16 flags, ctrl;
+	u16 ctrl;
 	struct pci_dev *bridge;
 
 	if (pcie_ari_disabled || !pci_is_pcie(dev) || dev->devfn)
@@ -1998,13 +2030,9 @@ void pci_enable_ari(struct pci_dev *dev)
 	if (!bridge)
 		return;
 
-	pos = pci_pcie_cap(bridge);
-	if (!pos)
-		return;
-
 	/* ARI is a PCIe v2 feature */
-	pci_read_config_word(bridge, pos + PCI_EXP_FLAGS, &flags);
-	if ((flags & PCI_EXP_FLAGS_VERS) < 2)
+	pos = pci_pcie_cap2(bridge);
+	if (!pos)
 		return;
 
 	pci_read_config_dword(bridge, pos + PCI_EXP_DEVCAP2, &cap);
@@ -2019,7 +2047,7 @@ void pci_enable_ari(struct pci_dev *dev)
 }
 
 /**
- * pci_enable_ido - enable ID-based ordering on a device
+ * pci_enable_ido - enable ID-based Ordering on a device
  * @dev: the PCI device
  * @type: which types of IDO to enable
  *
@@ -2032,7 +2060,8 @@ void pci_enable_ido(struct pci_dev *dev, unsigned long type)
 	int pos;
 	u16 ctrl;
 
-	pos = pci_pcie_cap(dev);
+	/* ID-based Ordering is a PCIe v2 feature */
+	pos = pci_pcie_cap2(dev);
 	if (!pos)
 		return;
 
@@ -2055,7 +2084,8 @@ void pci_disable_ido(struct pci_dev *dev, unsigned long type)
 	int pos;
 	u16 ctrl;
 
-	pos = pci_pcie_cap(dev);
+	/* ID-based Ordering is a PCIe v2 feature */
+	pos = pci_pcie_cap2(dev);
 	if (!pos)
 		return;
 
@@ -2094,7 +2124,8 @@ int pci_enable_obff(struct pci_dev *dev, enum pci_obff_signal_type type)
 	u16 ctrl;
 	int ret;
 
-	pos = pci_pcie_cap(dev);
+	/* OBFF is a PCIe v2 feature */
+	pos = pci_pcie_cap2(dev);
 	if (!pos)
 		return -ENOTSUPP;
 
@@ -2144,7 +2175,8 @@ void pci_disable_obff(struct pci_dev *dev)
 	int pos;
 	u16 ctrl;
 
-	pos = pci_pcie_cap(dev);
+	/* OBFF is a PCIe v2 feature */
+	pos = pci_pcie_cap2(dev);
 	if (!pos)
 		return;
 
@@ -2166,7 +2198,8 @@ static bool pci_ltr_supported(struct pci_dev *dev)
 	int pos;
 	u32 cap;
 
-	pos = pci_pcie_cap(dev);
+	/* LTR is a PCIe v2 feature */
+	pos = pci_pcie_cap2(dev);
 	if (!pos)
 		return false;
 
@@ -2194,7 +2227,8 @@ int pci_enable_ltr(struct pci_dev *dev)
 	if (!pci_ltr_supported(dev))
 		return -ENOTSUPP;
 
-	pos = pci_pcie_cap(dev);
+	/* LTR is a PCIe v2 feature */
+	pos = pci_pcie_cap2(dev);
 	if (!pos)
 		return -ENOTSUPP;
 
@@ -2229,7 +2263,8 @@ void pci_disable_ltr(struct pci_dev *dev)
 	if (!pci_ltr_supported(dev))
 		return;
 
-	pos = pci_pcie_cap(dev);
+	/* LTR is a PCIe v2 feature */
+	pos = pci_pcie_cap2(dev);
 	if (!pos)
 		return;
 
diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
index 4b608f5..bc1b54d 100644
--- a/include/linux/pci_regs.h
+++ b/include/linux/pci_regs.h
@@ -507,6 +507,13 @@
 #define PCI_EXP_RTSTA		32	/* Root Status */
 #define PCI_EXP_RTSTA_PME	0x10000 /* PME status */
 #define PCI_EXP_RTSTA_PENDING	0x20000 /* PME pending */
+/*
+ * Note that the following PCI Express 'Capability Structure' registers
+ * were introduced with 'Capability Version' 0x2 (v2).  These registers
+ * do not exist, and thus should not be attempted to be accesses, by
+ * PCI Express v1 devices (see pci_pcie_cap2() for use in qualifying
+ * PCI Express v2 capability related features).
+ */
 #define PCI_EXP_DEVCAP2		36	/* Device Capabilities 2 */
 #define  PCI_EXP_DEVCAP2_ARI	0x20	/* Alternative Routing-ID */
 #define  PCI_EXP_DEVCAP2_LTR	0x800	/* Latency tolerance reporting */

[PATCH 4/4] PCI: Remove redundant capabilities checking in pci_{save, restore}_pcie_state

[Myron Stowe]

Unlike PCI Express v1's Capabilities Structure, v2's requires the entire
structure to be implemented.  In v2 structures, register fields that are
not necessarly implemented, are present but hardwired to 0x0.  These may
include: Link Capabilities, Status, and Control; Slot Capabilities,
Status, and Control; Root Capabilities, Status, and Control; and all of
the '2' (Device, Link, and Slot) Capabilities, Status, and Control
registers.

This patch removes the redundant capability checks corresponding to the
Link 2's and Slot 2's, Capabilities, Status, and Control registers as they
will be present if Device Capabilities 2's registers are (which explains
why the macros for each of the three are identical).

Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
---

 drivers/pci/pci.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 26933ff..f9f8036 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -903,12 +903,11 @@ static int pci_save_pcie_state(struct pci_dev *dev)
 		pci_read_config_word(dev, pos + PCI_EXP_SLTCTL, &cap[i++]);
 	if (pcie_cap_has_rtctl(dev->pcie_type, flags))
 		pci_read_config_word(dev, pos + PCI_EXP_RTCTL, &cap[i++]);
-	if (pcie_cap_has_devctl2(dev->pcie_type, flags))
+	if (pcie_cap_has_devctl2(dev->pcie_type, flags)) {
 		pci_read_config_word(dev, pos + PCI_EXP_DEVCTL2, &cap[i++]);
-	if (pcie_cap_has_lnkctl2(dev->pcie_type, flags))
 		pci_read_config_word(dev, pos + PCI_EXP_LNKCTL2, &cap[i++]);
-	if (pcie_cap_has_sltctl2(dev->pcie_type, flags))
 		pci_read_config_word(dev, pos + PCI_EXP_SLTCTL2, &cap[i++]);
+	}
 
 	return 0;
 }
@@ -936,12 +935,11 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
 		pci_write_config_word(dev, pos + PCI_EXP_SLTCTL, cap[i++]);
 	if (pcie_cap_has_rtctl(dev->pcie_type, flags))
 		pci_write_config_word(dev, pos + PCI_EXP_RTCTL, cap[i++]);
-	if (pcie_cap_has_devctl2(dev->pcie_type, flags))
+	if (pcie_cap_has_devctl2(dev->pcie_type, flags)) {
 		pci_write_config_word(dev, pos + PCI_EXP_DEVCTL2, cap[i++]);
-	if (pcie_cap_has_lnkctl2(dev->pcie_type, flags))
 		pci_write_config_word(dev, pos + PCI_EXP_LNKCTL2, cap[i++]);
-	if (pcie_cap_has_sltctl2(dev->pcie_type, flags))
 		pci_write_config_word(dev, pos + PCI_EXP_SLTCTL2, cap[i++]);
+	}
 }
 
 

Re: [PATCH 3/4] PCI: Add pci_pcie_cap2() check for PCIe feature capabilities >= v2

[Don Dutile]

On 06/01/2012 05:16 PM, Myron Stowe wrote:
> This patch resolves potential issues when accessing PCI Express
> capability structures.  The makeup of Express' capability structure
> varies substantially between v1 and v2:
>
>      PCI Express 1.0 and 1.1 base neither requires the endpoint to
>      implement the entire PCIe capability structure nor specifies
>      default values of registers that are not implemented by the
>      device.  Its capability version is 1.
>
>      PCIe 1.1 Capability Structure Expansion ECN, PCIe 2.0, 2.1, and
>      3.0 added additional registers to the structure and require all
>      registers to be either implemented or hardwired to 0.  Their PCIe
>      capability version is 2.
>
> Due to the differences in the capability structures, code dealing with
> capability features must be careful not to access the additional
> registers introduced with v2 unless the device is specifically known to
> be a v2 capable device.  Otherwise, attempts to access non-existant
> registers will occur.  This is a subtle issue that is hard to track down
> when it occurs (and it has - see commit 864d296cf94).
>
> To try and help mitigate such occurrances, this patch introduces
> pci_pcie_cap2() which is similar to pci_pcie_cap() but also checks
> that the PCIe capability version is>= 2.  pci_pcie_cap2() should be
> used for qualifying PCIe capability features introduced after v1.
>
> Suggested by Don Dutile.
>
Acked by him too!
... except one nit...

I would change the comment "is a PCIe v2 feature" to "is a PCIe cap v2 list feature"
so no one confuses spec version with a specific cap-struct version.

but I wouldn't hold up this patch for this nit.

> Signed-off-by: Myron Stowe<myron.stowe@redhat.com>
> ---
>
>   drivers/pci/pci.c        |   65 +++++++++++++++++++++++++++++++++++-----------
>   include/linux/pci_regs.h |    7 +++++
>   2 files changed, 57 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index ff0beb0..26933ff 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -279,6 +279,38 @@ int pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap)
>   }
>
>   /**
> + * pci_pcie_cap2 - query for devices' PCI_CAP_ID_EXP v2 capability structure
> + * @dev: PCI device to check
> + *
> + * Like pci_pcie_cap() but also checks that the PCIe capability version is
> + *>= 2.  Note that v1 capability structures could be sparse in that not
> + * all register fields were required.  v2 requires the entire structure to
> + * be present size wise, while still allowing for non-implemented registers
> + * to exist but they must be hardwired to 0.
> + *
> + * Due to the differences in the versions of capability structures, one
> + * must be careful not to try and access non-existant registers that may
> + * exist in early versions - v1 - of Express devices.
> + *
> + * Returns the offset of the PCIe capability structure as long as the
> + * capability version is>= 2; otherwise 0 is returned.
> + */
> +static int pci_pcie_cap2(struct pci_dev *dev)
> +{
> +	u16 flags;
> +	int pos;
> +
> +	pos = pci_pcie_cap(dev);
> +	if (pos) {
> +		pci_read_config_word(dev, pos + PCI_EXP_FLAGS,&flags);
> +		if ((flags&  PCI_EXP_FLAGS_VERS)<  2)
> +			pos = 0;
> +	}
> +
> +	return pos;
> +}
> +
> +/**
>    * pci_find_ext_capability - Find an extended capability
>    * @dev: PCI device to query
>    * @cap: capability code
> @@ -1984,7 +2016,7 @@ void pci_enable_ari(struct pci_dev *dev)
>   {
>   	int pos;
>   	u32 cap;
> -	u16 flags, ctrl;
> +	u16 ctrl;
>   	struct pci_dev *bridge;
>
>   	if (pcie_ari_disabled || !pci_is_pcie(dev) || dev->devfn)
> @@ -1998,13 +2030,9 @@ void pci_enable_ari(struct pci_dev *dev)
>   	if (!bridge)
>   		return;
>
> -	pos = pci_pcie_cap(bridge);
> -	if (!pos)
> -		return;
> -
>   	/* ARI is a PCIe v2 feature */
> -	pci_read_config_word(bridge, pos + PCI_EXP_FLAGS,&flags);
> -	if ((flags&  PCI_EXP_FLAGS_VERS)<  2)
> +	pos = pci_pcie_cap2(bridge);
> +	if (!pos)
>   		return;
>
>   	pci_read_config_dword(bridge, pos + PCI_EXP_DEVCAP2,&cap);
> @@ -2019,7 +2047,7 @@ void pci_enable_ari(struct pci_dev *dev)
>   }
>
>   /**
> - * pci_enable_ido - enable ID-based ordering on a device
> + * pci_enable_ido - enable ID-based Ordering on a device
>    * @dev: the PCI device
>    * @type: which types of IDO to enable
>    *
> @@ -2032,7 +2060,8 @@ void pci_enable_ido(struct pci_dev *dev, unsigned long type)
>   	int pos;
>   	u16 ctrl;
>
> -	pos = pci_pcie_cap(dev);
> +	/* ID-based Ordering is a PCIe v2 feature */
> +	pos = pci_pcie_cap2(dev);
>   	if (!pos)
>   		return;
>
> @@ -2055,7 +2084,8 @@ void pci_disable_ido(struct pci_dev *dev, unsigned long type)
>   	int pos;
>   	u16 ctrl;
>
> -	pos = pci_pcie_cap(dev);
> +	/* ID-based Ordering is a PCIe v2 feature */
> +	pos = pci_pcie_cap2(dev);
>   	if (!pos)
>   		return;
>
> @@ -2094,7 +2124,8 @@ int pci_enable_obff(struct pci_dev *dev, enum pci_obff_signal_type type)
>   	u16 ctrl;
>   	int ret;
>
> -	pos = pci_pcie_cap(dev);
> +	/* OBFF is a PCIe v2 feature */
> +	pos = pci_pcie_cap2(dev);
>   	if (!pos)
>   		return -ENOTSUPP;
>
> @@ -2144,7 +2175,8 @@ void pci_disable_obff(struct pci_dev *dev)
>   	int pos;
>   	u16 ctrl;
>
> -	pos = pci_pcie_cap(dev);
> +	/* OBFF is a PCIe v2 feature */
> +	pos = pci_pcie_cap2(dev);
>   	if (!pos)
>   		return;
>
> @@ -2166,7 +2198,8 @@ static bool pci_ltr_supported(struct pci_dev *dev)
>   	int pos;
>   	u32 cap;
>
> -	pos = pci_pcie_cap(dev);
> +	/* LTR is a PCIe v2 feature */
> +	pos = pci_pcie_cap2(dev);
>   	if (!pos)
>   		return false;
>
> @@ -2194,7 +2227,8 @@ int pci_enable_ltr(struct pci_dev *dev)
>   	if (!pci_ltr_supported(dev))
>   		return -ENOTSUPP;
>
> -	pos = pci_pcie_cap(dev);
> +	/* LTR is a PCIe v2 feature */
> +	pos = pci_pcie_cap2(dev);
>   	if (!pos)
>   		return -ENOTSUPP;
>
> @@ -2229,7 +2263,8 @@ void pci_disable_ltr(struct pci_dev *dev)
>   	if (!pci_ltr_supported(dev))
>   		return;
>
> -	pos = pci_pcie_cap(dev);
> +	/* LTR is a PCIe v2 feature */
> +	pos = pci_pcie_cap2(dev);
>   	if (!pos)
>   		return;
>
> diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
> index 4b608f5..bc1b54d 100644
> --- a/include/linux/pci_regs.h
> +++ b/include/linux/pci_regs.h
> @@ -507,6 +507,13 @@
>   #define PCI_EXP_RTSTA		32	/* Root Status */
>   #define PCI_EXP_RTSTA_PME	0x10000 /* PME status */
>   #define PCI_EXP_RTSTA_PENDING	0x20000 /* PME pending */
> +/*
> + * Note that the following PCI Express 'Capability Structure' registers
> + * were introduced with 'Capability Version' 0x2 (v2).  These registers
> + * do not exist, and thus should not be attempted to be accesses, by
> + * PCI Express v1 devices (see pci_pcie_cap2() for use in qualifying
> + * PCI Express v2 capability related features).
> + */
>   #define PCI_EXP_DEVCAP2		36	/* Device Capabilities 2 */
>   #define  PCI_EXP_DEVCAP2_ARI	0x20	/* Alternative Routing-ID */
>   #define  PCI_EXP_DEVCAP2_LTR	0x800	/* Latency tolerance reporting */
>

Re: [PATCH 4/4] PCI: Remove redundant capabilities checking in pci_{save, restore}_pcie_state

[Don Dutile]

On 06/01/2012 05:16 PM, Myron Stowe wrote:
> Unlike PCI Express v1's Capabilities Structure, v2's requires the entire
> structure to be implemented.  In v2 structures, register fields that are
> not necessarly implemented, are present but hardwired to 0x0.  These may
> include: Link Capabilities, Status, and Control; Slot Capabilities,
> Status, and Control; Root Capabilities, Status, and Control; and all of
> the '2' (Device, Link, and Slot) Capabilities, Status, and Control
> registers.
>
> This patch removes the redundant capability checks corresponding to the
> Link 2's and Slot 2's, Capabilities, Status, and Control registers as they
> will be present if Device Capabilities 2's registers are (which explains
> why the macros for each of the three are identical).
>
> Signed-off-by: Myron Stowe<myron.stowe@redhat.com>
> ---
>
>   drivers/pci/pci.c |   10 ++++------
>   1 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 26933ff..f9f8036 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -903,12 +903,11 @@ static int pci_save_pcie_state(struct pci_dev *dev)
>   		pci_read_config_word(dev, pos + PCI_EXP_SLTCTL,&cap[i++]);
>   	if (pcie_cap_has_rtctl(dev->pcie_type, flags))
>   		pci_read_config_word(dev, pos + PCI_EXP_RTCTL,&cap[i++]);
> -	if (pcie_cap_has_devctl2(dev->pcie_type, flags))
> +	if (pcie_cap_has_devctl2(dev->pcie_type, flags)) {
and why not use your new function:
+	pos = pci_pcie_cap2(bridge);
+	if (!pos)
instead of this devctl2-specific check, so it's obvious it's the whole cap-struct (additional regs)?
  
>   		pci_read_config_word(dev, pos + PCI_EXP_DEVCTL2,&cap[i++]);
> -	if (pcie_cap_has_lnkctl2(dev->pcie_type, flags))
>   		pci_read_config_word(dev, pos + PCI_EXP_LNKCTL2,&cap[i++]);
> -	if (pcie_cap_has_sltctl2(dev->pcie_type, flags))
>   		pci_read_config_word(dev, pos + PCI_EXP_SLTCTL2,&cap[i++]);
> +	}
>
>   	return 0;
>   }
> @@ -936,12 +935,11 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
>   		pci_write_config_word(dev, pos + PCI_EXP_SLTCTL, cap[i++]);
>   	if (pcie_cap_has_rtctl(dev->pcie_type, flags))
>   		pci_write_config_word(dev, pos + PCI_EXP_RTCTL, cap[i++]);
> -	if (pcie_cap_has_devctl2(dev->pcie_type, flags))
> +	if (pcie_cap_has_devctl2(dev->pcie_type, flags)) {
ditto.

>   		pci_write_config_word(dev, pos + PCI_EXP_DEVCTL2, cap[i++]);
> -	if (pcie_cap_has_lnkctl2(dev->pcie_type, flags))
>   		pci_write_config_word(dev, pos + PCI_EXP_LNKCTL2, cap[i++]);
> -	if (pcie_cap_has_sltctl2(dev->pcie_type, flags))
>   		pci_write_config_word(dev, pos + PCI_EXP_SLTCTL2, cap[i++]);
> +	}
>   }
>
>
>

Re: [PATCH 2/4] PCI: Remove redundant checking in PCI Express capability routines

[Don Dutile]

On 06/01/2012 05:16 PM, Myron Stowe wrote:
> There are a number of redundant pci_is_pcie() checks in various PCI
> Express capabilities related routines like the following:
>
>      if (!pci_is_pcie(dev))
> 	return false;
>
>      pos = pci_pcie_cap(dev);
>      if (!pos)
> 	return false;
>
> The current pci_is_pcie() implementation is merely:
>
>      static inline bool pci_is_pcie(struct pci_dev *dev)
>      {
>          return !!pci_pcie_cap(dev);
>      }
>
> so we can just drop the pci_is_pcie() test in such cases.
>
> Suggested by Bjorn Helgaas in -
> http://marc.info/?l=linux-pci&m=130463494319762&w=2
>
> Signed-off-by: Myron Stowe<myron.stowe@redhat.com>
> ---
>
you can add my ack to this patch.

>   drivers/pci/pci.c |   14 +-------------
>   1 files changed, 1 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 64471b1..ff0beb0 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1995,7 +1995,7 @@ void pci_enable_ari(struct pci_dev *dev)
>   		return;
>
>   	bridge = dev->bus->self;
> -	if (!bridge || !pci_is_pcie(bridge))
> +	if (!bridge)
>   		return;
>
>   	pos = pci_pcie_cap(bridge);
> @@ -2055,9 +2055,6 @@ void pci_disable_ido(struct pci_dev *dev, unsigned long type)
>   	int pos;
>   	u16 ctrl;
>
> -	if (!pci_is_pcie(dev))
> -		return;
> -
>   	pos = pci_pcie_cap(dev);
>   	if (!pos)
>   		return;
> @@ -2097,9 +2094,6 @@ int pci_enable_obff(struct pci_dev *dev, enum pci_obff_signal_type type)
>   	u16 ctrl;
>   	int ret;
>
> -	if (!pci_is_pcie(dev))
> -		return -ENOTSUPP;
> -
>   	pos = pci_pcie_cap(dev);
>   	if (!pos)
>   		return -ENOTSUPP;
> @@ -2150,9 +2144,6 @@ void pci_disable_obff(struct pci_dev *dev)
>   	int pos;
>   	u16 ctrl;
>
> -	if (!pci_is_pcie(dev))
> -		return;
> -
>   	pos = pci_pcie_cap(dev);
>   	if (!pos)
>   		return;
> @@ -2175,9 +2166,6 @@ static bool pci_ltr_supported(struct pci_dev *dev)
>   	int pos;
>   	u32 cap;
>
> -	if (!pci_is_pcie(dev))
> -		return false;
> -
>   	pos = pci_pcie_cap(dev);
>   	if (!pos)
>   		return false;
>

Re: [PATCH 1/4] PCI: make pci_ltr_supported static.

[Don Dutile]

On 06/01/2012 05:16 PM, Myron Stowe wrote:
> The PCI Express Latency Tolerance Reporting (LTR) feature's
> pci_ltr_supported() routine is currently only used within
> drivers/pci/pci.c so make it static.
>
> Signed-off-by: Myron Stowe<myron.stowe@redhat.com>
> ---
>
Acked-by: Donald Dutile <ddutile@redhat.com>

>   drivers/pci/pci.c   |    4 ++--
>   include/linux/pci.h |    1 -
>   2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 447e834..64471b1 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -39,6 +39,7 @@ EXPORT_SYMBOL(pci_pci_problems);
>
>   unsigned int pci_pm_d3_delay;
>
> +static bool pci_ltr_supported(struct pci_dev *dev);
>   static void pci_pme_list_scan(struct work_struct *work);
>
>   static LIST_HEAD(pci_pme_list);
> @@ -2169,7 +2170,7 @@ EXPORT_SYMBOL(pci_disable_obff);
>    * RETURNS:
>    * True if @dev supports latency tolerance reporting, false otherwise.
>    */
> -bool pci_ltr_supported(struct pci_dev *dev)
> +static bool pci_ltr_supported(struct pci_dev *dev)
>   {
>   	int pos;
>   	u32 cap;
> @@ -2185,7 +2186,6 @@ bool pci_ltr_supported(struct pci_dev *dev)
>
>   	return cap&  PCI_EXP_DEVCAP2_LTR;
>   }
> -EXPORT_SYMBOL(pci_ltr_supported);
>
>   /**
>    * pci_enable_ltr - enable latency tolerance reporting
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index d8c379d..b2bec26 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -875,7 +875,6 @@ enum pci_obff_signal_type {
>   int pci_enable_obff(struct pci_dev *dev, enum pci_obff_signal_type);
>   void pci_disable_obff(struct pci_dev *dev);
>
> -bool pci_ltr_supported(struct pci_dev *dev);
>   int pci_enable_ltr(struct pci_dev *dev);
>   void pci_disable_ltr(struct pci_dev *dev);
>   int pci_set_ltr(struct pci_dev *dev, int snoop_lat_ns, int nosnoop_lat_ns);
>

Re: [PATCH 0/4] PCI: PCIe capability structure related cleanup/fixes

[Bjorn Helgaas]

On Fri, Jun 1, 2012 at 2:16 PM, Myron Stowe <myron.stowe@redhat.com> wrote:
> The following series introduces PCI Express 'capability structure'
> related cleanup, fixes, and optimizations.
>
> Patch 1/4 changes pci_ltr_supported() to a static routine.
>
> Patch 2/4 removes redundant checking in various PCI Express features as
> suggested by Bjorn Helgaas in
> http://marc.info/?l=linux-pci&m=130463494319762&w=2
>
> There is a similar idiom in use that could be similarly be re-factored:
>    if (!pci_is_pcie(dev))
>        return;
>
>    pos = pci_find_ext_capability(dev, ...);
>    if (!pos)
>        return;
>
> At first it seemed incorrect to remove the redundant call of
> pci_is_pcie() in these cases as a PCI or PCI-X (< 2.0) device may be
> involved.  In such cases an "extended capability" list would not exist,
> as it was not introduced until PCI-X 2.0, and accesses outside of the
> device's configuration space would be attempted.  However, upon further
> review of pci_find_ext_capability() it looks as if such accesses would
> be handled correctly due to the short-circuiting logic involved -
>
>    if (pci_read_config_dword(dev, pos, &header) != PCIBIOS_SUCCESSFUL)
>        return 0;
>
> As such, I'll entertain comments as to whether or not we should also
> make similar removals of pci_is_pcie() in these cases.
>
> Patch 3/4 introduces pci_pcie_cap2() for use in v2 capability related
> feature code.  The makeup of Express' capability structure varies
> substantially between v1 and v2.
>
> There is still some redundancy in PCIe v2 capabilities checking related
> to the Latency Tolerance Reporting (LTR) feature routines that likely
> could be re-factored further; please feel free to respond with ideas.
>
> Patch 4/4 makes a minor optimization to the saving and restoring of
> PCI Express capability structures.
>
> Seems like the same type of optimization could be done to remove the
> 'if (pcie_cap_has_lnkctl(dev->pcie_type, flags))' check.  According to
> section 7.8 "PCI Express Capability Structure" of the PCI Express 1.0a
> specification:
>
>    "Figure 7-10 details allocation of register fields in the PCI
>     Express Capability structure. The PCI Express Capabilities,
>     Device Capabilities, Device Status/Control, Link Capabilities,
>     and Link Status/Control registers are required for all PCI
>     Express devices. Endpoints are not required to implement
>     registers other than those listed above and terminate the
>     capability structure."
>
> There may have been some early Express devices that did not properly
> follow the specification which required the introduction of
> 'pcie_cap_has_lnkctl()' so I did not make the additional optimization.
> ---
>
> Myron Stowe (4):
>      PCI: Remove redundant capabilities checking in pci_{save,restore}_pcie_state
>      PCI: Add pci_pcie_cap2() check for PCIe feature capabilities >= v2
>      PCI: Remove redundant checking in PCI Express capability routines
>      PCI: make pci_ltr_supported static.

I added Don's acks, made a couple minor changes he suggested, removed
the static pci_ltr_supported() function declaration (unnecessary,
AFAICS), and pushed these to:

  http://git.kernel.org/?p=linux/kernel/git/helgaas/pci.git;a=shortlog;h=refs/heads/topic/stowe-cap-cleanup

If everything looks right to you, I'll merge it into "next" tomorrow.
Thanks for doing this; I think it's some nice cleanup and will make
things safer and easier to understand.

Bjorn

Re: [PATCH 0/4] PCI: PCIe capability structure related cleanup/fixes

[Myron Stowe]

On Mon, 2012-06-11 at 19:52 -0700, Bjorn Helgaas wrote:
> On Fri, Jun 1, 2012 at 2:16 PM, Myron Stowe <myron.stowe@redhat.com> wrote:
> > The following series introduces PCI Express 'capability structure'
> > related cleanup, fixes, and optimizations.
> >
> > Patch 1/4 changes pci_ltr_supported() to a static routine.
> >
> > Patch 2/4 removes redundant checking in various PCI Express features as
> > suggested by Bjorn Helgaas in
> > http://marc.info/?l=linux-pci&m=130463494319762&w=2
> >
> > There is a similar idiom in use that could be similarly be re-factored:
> >    if (!pci_is_pcie(dev))
> >        return;
> >
> >    pos = pci_find_ext_capability(dev, ...);
> >    if (!pos)
> >        return;
> >
> > At first it seemed incorrect to remove the redundant call of
> > pci_is_pcie() in these cases as a PCI or PCI-X (< 2.0) device may be
> > involved.  In such cases an "extended capability" list would not exist,
> > as it was not introduced until PCI-X 2.0, and accesses outside of the
> > device's configuration space would be attempted.  However, upon further
> > review of pci_find_ext_capability() it looks as if such accesses would
> > be handled correctly due to the short-circuiting logic involved -
> >
> >    if (pci_read_config_dword(dev, pos, &header) != PCIBIOS_SUCCESSFUL)
> >        return 0;
> >
> > As such, I'll entertain comments as to whether or not we should also
> > make similar removals of pci_is_pcie() in these cases.
> >
> > Patch 3/4 introduces pci_pcie_cap2() for use in v2 capability related
> > feature code.  The makeup of Express' capability structure varies
> > substantially between v1 and v2.
> >
> > There is still some redundancy in PCIe v2 capabilities checking related
> > to the Latency Tolerance Reporting (LTR) feature routines that likely
> > could be re-factored further; please feel free to respond with ideas.
> >
> > Patch 4/4 makes a minor optimization to the saving and restoring of
> > PCI Express capability structures.
> >
> > Seems like the same type of optimization could be done to remove the
> > 'if (pcie_cap_has_lnkctl(dev->pcie_type, flags))' check.  According to
> > section 7.8 "PCI Express Capability Structure" of the PCI Express 1.0a
> > specification:
> >
> >    "Figure 7-10 details allocation of register fields in the PCI
> >     Express Capability structure. The PCI Express Capabilities,
> >     Device Capabilities, Device Status/Control, Link Capabilities,
> >     and Link Status/Control registers are required for all PCI
> >     Express devices. Endpoints are not required to implement
> >     registers other than those listed above and terminate the
> >     capability structure."
> >
> > There may have been some early Express devices that did not properly
> > follow the specification which required the introduction of
> > 'pcie_cap_has_lnkctl()' so I did not make the additional optimization.
> > ---
> >
> > Myron Stowe (4):
> >      PCI: Remove redundant capabilities checking in pci_{save,restore}_pcie_state
> >      PCI: Add pci_pcie_cap2() check for PCIe feature capabilities >= v2
> >      PCI: Remove redundant checking in PCI Express capability routines
> >      PCI: make pci_ltr_supported static.
> 
> I added Don's acks, made a couple minor changes he suggested, removed
> the static pci_ltr_supported() function declaration (unnecessary,
> AFAICS), and pushed these to:
> 
>   http://git.kernel.org/?p=linux/kernel/git/helgaas/pci.git;a=shortlog;h=refs/heads/topic/stowe-cap-cleanup
> 
> If everything looks right to you, I'll merge it into "next" tomorrow.
> Thanks for doing this; I think it's some nice cleanup and will make
> things safer and easier to understand.

Looks good - thanks to both Don and yourself for the suggestions and
changes to make the patch headers more comprehensible with respect to
the capabilities structure versions.

Myron
> 
> Bjorn

Re: [PATCH 0/4] PCI: PCIe capability structure related cleanup/fixes

[Bjorn Helgaas]

On Tue, Jun 12, 2012 at 10:34 AM, Myron Stowe <mstowe@redhat.com> wrote:
> On Mon, 2012-06-11 at 19:52 -0700, Bjorn Helgaas wrote:
>> On Fri, Jun 1, 2012 at 2:16 PM, Myron Stowe <myron.stowe@redhat.com> wrote:
>> > The following series introduces PCI Express 'capability structure'
>> > related cleanup, fixes, and optimizations.
>> >
>> > Patch 1/4 changes pci_ltr_supported() to a static routine.
>> >
>> > Patch 2/4 removes redundant checking in various PCI Express features as
>> > suggested by Bjorn Helgaas in
>> > http://marc.info/?l=linux-pci&m=130463494319762&w=2
>> >
>> > There is a similar idiom in use that could be similarly be re-factored:
>> >    if (!pci_is_pcie(dev))
>> >        return;
>> >
>> >    pos = pci_find_ext_capability(dev, ...);
>> >    if (!pos)
>> >        return;
>> >
>> > At first it seemed incorrect to remove the redundant call of
>> > pci_is_pcie() in these cases as a PCI or PCI-X (< 2.0) device may be
>> > involved.  In such cases an "extended capability" list would not exist,
>> > as it was not introduced until PCI-X 2.0, and accesses outside of the
>> > device's configuration space would be attempted.  However, upon further
>> > review of pci_find_ext_capability() it looks as if such accesses would
>> > be handled correctly due to the short-circuiting logic involved -
>> >
>> >    if (pci_read_config_dword(dev, pos, &header) != PCIBIOS_SUCCESSFUL)
>> >        return 0;
>> >
>> > As such, I'll entertain comments as to whether or not we should also
>> > make similar removals of pci_is_pcie() in these cases.
>> >
>> > Patch 3/4 introduces pci_pcie_cap2() for use in v2 capability related
>> > feature code.  The makeup of Express' capability structure varies
>> > substantially between v1 and v2.
>> >
>> > There is still some redundancy in PCIe v2 capabilities checking related
>> > to the Latency Tolerance Reporting (LTR) feature routines that likely
>> > could be re-factored further; please feel free to respond with ideas.
>> >
>> > Patch 4/4 makes a minor optimization to the saving and restoring of
>> > PCI Express capability structures.
>> >
>> > Seems like the same type of optimization could be done to remove the
>> > 'if (pcie_cap_has_lnkctl(dev->pcie_type, flags))' check.  According to
>> > section 7.8 "PCI Express Capability Structure" of the PCI Express 1.0a
>> > specification:
>> >
>> >    "Figure 7-10 details allocation of register fields in the PCI
>> >     Express Capability structure. The PCI Express Capabilities,
>> >     Device Capabilities, Device Status/Control, Link Capabilities,
>> >     and Link Status/Control registers are required for all PCI
>> >     Express devices. Endpoints are not required to implement
>> >     registers other than those listed above and terminate the
>> >     capability structure."
>> >
>> > There may have been some early Express devices that did not properly
>> > follow the specification which required the introduction of
>> > 'pcie_cap_has_lnkctl()' so I did not make the additional optimization.
>> > ---
>> >
>> > Myron Stowe (4):
>> >      PCI: Remove redundant capabilities checking in pci_{save,restore}_pcie_state
>> >      PCI: Add pci_pcie_cap2() check for PCIe feature capabilities >= v2
>> >      PCI: Remove redundant checking in PCI Express capability routines
>> >      PCI: make pci_ltr_supported static.
>>
>> I added Don's acks, made a couple minor changes he suggested, removed
>> the static pci_ltr_supported() function declaration (unnecessary,
>> AFAICS), and pushed these to:
>>
>>   http://git.kernel.org/?p=linux/kernel/git/helgaas/pci.git;a=shortlog;h=refs/heads/topic/stowe-cap-cleanup
>>
>> If everything looks right to you, I'll merge it into "next" tomorrow.
>> Thanks for doing this; I think it's some nice cleanup and will make
>> things safer and easier to understand.
>
> Looks good - thanks to both Don and yourself for the suggestions and
> changes to make the patch headers more comprehensible with respect to
> the capabilities structure versions.

Great, I merged that topic branch to "next" and pushed it.

Bjorn