[PATCH 0/4] PCIe Capability accessor fix and cleanups

[PATCH 0/4] PCIe Capability accessor fix and cleanups

[Bjorn Helgaas]

The first patch fixes a Link Control issue reported by Yuval Mintz.
The rest simplify the code a bit.

---

Bjorn Helgaas (4):
      PCI: Allow PCIe Capability link-related register access for switches
      PCI: Remove PCIe Capability version checks
      PCI: Support PCIe Capability Slot registers only for ports with slots
      PCI: Remove pcie_cap_has_devctl()


 drivers/pci/access.c |   26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

[PATCH 1/4] PCI: Allow PCIe Capability link-related register access for switches

[Bjorn Helgaas]

Every PCIe device has a link, except Root Complex Integrated Endpoints
and Root Complex Event Collectors.  Previously we didn't give access
to PCIe capability link-related registers for Upstream Ports, Downstream
Ports, and Bridges, so attempts to read PCI_EXP_LNKCTL incorrectly
returned zero.  See PCIe spec r3.0, sec 7.8 and 1.3.2.3.

Reported-by: Yuval Mintz <yuvalmin@broadcom.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/access.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 1cc2366..e26c3bd 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -485,9 +485,13 @@ static inline bool pcie_cap_has_lnkctl(const struct pci_dev *dev)
 	int type = pci_pcie_type(dev);
 
 	return pcie_cap_version(dev) > 1 ||
-	       type == PCI_EXP_TYPE_ROOT_PORT ||
 	       type == PCI_EXP_TYPE_ENDPOINT ||
-	       type == PCI_EXP_TYPE_LEG_END;
+	       type == PCI_EXP_TYPE_LEG_END ||
+	       type == PCI_EXP_TYPE_ROOT_PORT ||
+	       type == PCI_EXP_TYPE_UPSTREAM ||
+	       type == PCI_EXP_TYPE_DOWNSTREAM ||
+	       type == PCI_EXP_TYPE_PCI_BRIDGE ||
+	       type == PCI_EXP_TYPE_PCIE_BRIDGE;
 }
 
 static inline bool pcie_cap_has_sltctl(const struct pci_dev *dev)

[PATCH 2/4] PCI: Remove PCIe Capability version checks

[Bjorn Helgaas]

Previously we relied on the PCIe r3.0, sec 7.8, spec language that says
"For Functions that do not implement the [Link, Slot, Root] registers,
these spaces must be hardwired to 0b," which means that for v2 PCIe
capabilities, we don't need to check the device type at all.

But it's simpler if we don't need to check the capability version at all,
and I think the spec is explicit enough about which registers are required
for which types that we can remove the version checks.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/access.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index e26c3bd..9a46fa9 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -484,8 +484,7 @@ static inline bool pcie_cap_has_lnkctl(const struct pci_dev *dev)
 {
 	int type = pci_pcie_type(dev);
 
-	return pcie_cap_version(dev) > 1 ||
-	       type == PCI_EXP_TYPE_ENDPOINT ||
+	return type == PCI_EXP_TYPE_ENDPOINT ||
 	       type == PCI_EXP_TYPE_LEG_END ||
 	       type == PCI_EXP_TYPE_ROOT_PORT ||
 	       type == PCI_EXP_TYPE_UPSTREAM ||
@@ -498,8 +497,7 @@ static inline bool pcie_cap_has_sltctl(const struct pci_dev *dev)
 {
 	int type = pci_pcie_type(dev);
 
-	return pcie_cap_version(dev) > 1 ||
-	       type == PCI_EXP_TYPE_ROOT_PORT ||
+	return type == PCI_EXP_TYPE_ROOT_PORT ||
 	       (type == PCI_EXP_TYPE_DOWNSTREAM &&
 		pcie_caps_reg(dev) & PCI_EXP_FLAGS_SLOT);
 }
@@ -508,8 +506,7 @@ static inline bool pcie_cap_has_rtctl(const struct pci_dev *dev)
 {
 	int type = pci_pcie_type(dev);
 
-	return pcie_cap_version(dev) > 1 ||
-	       type == PCI_EXP_TYPE_ROOT_PORT ||
+	return type == PCI_EXP_TYPE_ROOT_PORT ||
 	       type == PCI_EXP_TYPE_RC_EC;
 }
 

[PATCH 3/4] PCI: Support PCIe Capability Slot registers only for ports with slots

[Bjorn Helgaas]

Previously we allowed callers to access Slot Capabilities, Status, and
Control for Root Ports even if the Root Port did not implement a slot.
This seems dubious because the spec only requires these registers if a
slot is implemented.

It's true that even Root Ports without slots must have *space* for these
slot registers, because the Root Capabilities, Status, and Control
registers are after the slot registers in the capability.  However,
for a v1 PCIe Capability, the *semantics* of the slot registers are
undefined unless a slot is implemented.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/access.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 9a46fa9..061da8c 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -497,9 +497,9 @@ static inline bool pcie_cap_has_sltctl(const struct pci_dev *dev)
 {
 	int type = pci_pcie_type(dev);
 
-	return type == PCI_EXP_TYPE_ROOT_PORT ||
-	       (type == PCI_EXP_TYPE_DOWNSTREAM &&
-		pcie_caps_reg(dev) & PCI_EXP_FLAGS_SLOT);
+	return (type == PCI_EXP_TYPE_ROOT_PORT ||
+		type == PCI_EXP_TYPE_DOWNSTREAM) &&
+	       pcie_caps_reg(dev) & PCI_EXP_FLAGS_SLOT;
 }
 
 static inline bool pcie_cap_has_rtctl(const struct pci_dev *dev)

[PATCH 4/4] PCI: Remove pcie_cap_has_devctl()

[Bjorn Helgaas]

pcie_cap_has_devctl() does nothing, so remove it.  Simplicity over
consistency in this case.  No functional change.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/access.c |    7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 061da8c..0857ca9 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -475,11 +475,6 @@ static inline int pcie_cap_version(const struct pci_dev *dev)
 	return pcie_caps_reg(dev) & PCI_EXP_FLAGS_VERS;
 }
 
-static inline bool pcie_cap_has_devctl(const struct pci_dev *dev)
-{
-	return true;
-}
-
 static inline bool pcie_cap_has_lnkctl(const struct pci_dev *dev)
 {
 	int type = pci_pcie_type(dev);
@@ -521,7 +516,7 @@ static bool pcie_capability_reg_implemented(struct pci_dev *dev, int pos)
 	case PCI_EXP_DEVCAP:
 	case PCI_EXP_DEVCTL:
 	case PCI_EXP_DEVSTA:
-		return pcie_cap_has_devctl(dev);
+		return true;
 	case PCI_EXP_LNKCAP:
 	case PCI_EXP_LNKCTL:
 	case PCI_EXP_LNKSTA:

Re: [PATCH 0/4] PCIe Capability accessor fix and cleanups

[Keller, Jacob E]

T24gV2VkLCAyMDEzLTA4LTI4IGF0IDEzOjI2IC0wNjAwLCBCam9ybiBIZWxnYWFzIHdyb3RlOg0K
PiBUaGUgZmlyc3QgcGF0Y2ggZml4ZXMgYSBMaW5rIENvbnRyb2wgaXNzdWUgcmVwb3J0ZWQgYnkg
WXV2YWwgTWludHouDQo+IFRoZSByZXN0IHNpbXBsaWZ5IHRoZSBjb2RlIGEgYml0Lg0KPiANCj4g
LS0tDQo+IA0KPiBCam9ybiBIZWxnYWFzICg0KToNCj4gICAgICAgUENJOiBBbGxvdyBQQ0llIENh
cGFiaWxpdHkgbGluay1yZWxhdGVkIHJlZ2lzdGVyIGFjY2VzcyBmb3Igc3dpdGNoZXMNCj4gICAg
ICAgUENJOiBSZW1vdmUgUENJZSBDYXBhYmlsaXR5IHZlcnNpb24gY2hlY2tzDQo+ICAgICAgIFBD
STogU3VwcG9ydCBQQ0llIENhcGFiaWxpdHkgU2xvdCByZWdpc3RlcnMgb25seSBmb3IgcG9ydHMg
d2l0aCBzbG90cw0KPiAgICAgICBQQ0k6IFJlbW92ZSBwY2llX2NhcF9oYXNfZGV2Y3RsKCkNCj4g
DQo+IA0KPiAgZHJpdmVycy9wY2kvYWNjZXNzLmMgfCAgIDI2ICsrKysrKysrKysrLS0tLS0tLS0t
LS0tLS0tDQo+ICAxIGZpbGUgY2hhbmdlZCwgMTEgaW5zZXJ0aW9ucygrKSwgMTUgZGVsZXRpb25z
KC0pDQoNCkkgbGlrZSB0aGUgY2hhbmdlcyBoZXJlLCBtYWtlcyBpdCBhIGxvdCBtb3JlIHJlYWRh
YmxlLg0KDQpUaGFua3MsDQpKYWtlDQo=

Re: [PATCH 0/4] PCIe Capability accessor fix and cleanups

[Jiang Liu]

On 08/29/2013 03:26 AM, Bjorn Helgaas wrote:
> The first patch fixes a Link Control issue reported by Yuval Mintz.
> The rest simplify the code a bit.
> 
> ---
> 
> Bjorn Helgaas (4):
>       PCI: Allow PCIe Capability link-related register access for switches
>       PCI: Remove PCIe Capability version checks
>       PCI: Support PCIe Capability Slot registers only for ports with slots
>       PCI: Remove pcie_cap_has_devctl()
> 
> 
>  drivers/pci/access.c |   26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
> 
Hi Bjorn,
	Good changes! We avoid some unnecessary hardware accesses for
unavailable registers.

Reviewed-By: Jiang Liu <jiang.liu@huawei.com>

Re: [PATCH 0/4] PCIe Capability accessor fix and cleanups

[Bjorn Helgaas]

On Wed, Aug 28, 2013 at 6:41 PM, Jiang Liu <liuj97@gmail.com> wrote:
> On 08/29/2013 03:26 AM, Bjorn Helgaas wrote:
>> The first patch fixes a Link Control issue reported by Yuval Mintz.
>> The rest simplify the code a bit.
>>
>> ---
>>
>> Bjorn Helgaas (4):
>>       PCI: Allow PCIe Capability link-related register access for switches
>>       PCI: Remove PCIe Capability version checks
>>       PCI: Support PCIe Capability Slot registers only for ports with slots
>>       PCI: Remove pcie_cap_has_devctl()
>>
>>
>>  drivers/pci/access.c |   26 +++++++++++---------------
>>  1 file changed, 11 insertions(+), 15 deletions(-)
>>
> Hi Bjorn,
>         Good changes! We avoid some unnecessary hardware accesses for
> unavailable registers.
>
> Reviewed-By: Jiang Liu <jiang.liu@huawei.com>

I applied these to pci/misc and put them in "next" for v3.12.