[PATCH 0/2] PCI: Comment and PCIe capability accessor fixes

[PATCH 0/2] PCI: Comment and PCIe capability accessor fixes

[Bjorn Helgaas]

These are a trivial comment fix and an almost as trivial accessor fix.

Arcanum for the day: when the PCIe spec says "Downstream Port," it means
both Root Ports and Switch Downstream Ports.

---

Bjorn Helgaas (2):
      PCI: Fix pcie_port_device_resume() comment
      PCI: Add pcie_downstream_port() (true for Root and Switch Downstream Ports)


 drivers/pci/access.c            |   23 +++++++++++++----------
 drivers/pci/pcie/portdrv_core.c |    2 +-
 2 files changed, 14 insertions(+), 11 deletions(-)

[PATCH 1/2] PCI: Fix pcie_port_device_resume() comment

[Bjorn Helgaas]

The function comment claimed this was pcie_port_device_suspend(), but it's
really pcie_port_device_resume().  Perils of cut and paste.

Use the correct function name in the comment.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/portdrv_core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 2f0ce66..88122dc 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -448,7 +448,7 @@ static int resume_iter(struct device *dev, void *data)
 }
 
 /**
- * pcie_port_device_suspend - resume port services associated with a PCIe port
+ * pcie_port_device_resume - resume port services associated with a PCIe port
  * @dev: PCI Express port to handle
  */
 int pcie_port_device_resume(struct device *dev)

[PATCH 2/2] PCI: Add pcie_downstream_port() (true for Root and Switch Downstream Ports)

[Bjorn Helgaas]

As used in the PCIe spec, "Downstream Port" includes both Root Ports and
Switch Downstream Ports.  We sometimes checked for PCI_EXP_TYPE_DOWNSTREAM
when we should have checked for PCI_EXP_TYPE_ROOT_PORT or
PCI_EXP_TYPE_DOWNSTREAM.

For a Root Port without a slot, the effect of this was that using
pcie_capability_read_word() to read PCI_EXP_SLTSTA returned zero instead of
showing the Presence Detect State bit hardwired to one as the PCIe Spec,
r3.0, sec 7.8, requires.  (This read is completed in software because
previous PCIe spec versions didn't require PCI_EXP_SLTSTA to exist at all.)

Nothing in the kernel currently depends on this (pciehp only reads
PCI_EXP_SLTSTA on ports with slots), so this is a cleanup and not a
functional change.

Add a pcie_downstream_port() helper function and use it.

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

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index d9b64a1..79f0102 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -531,6 +531,14 @@ static inline int pcie_cap_version(const struct pci_dev *dev)
 	return pcie_caps_reg(dev) & PCI_EXP_FLAGS_VERS;
 }
 
+static bool pcie_downstream_port(constr struct pci_dev *dev)
+{
+	int type = pci_pcie_type(dev);
+
+	return type == PCI_EXP_TYPE_ROOT_PORT ||
+	       type == PCI_EXP_TYPE_DOWNSTREAM;
+}
+
 bool pcie_cap_has_lnkctl(const struct pci_dev *dev)
 {
 	int type = pci_pcie_type(dev);
@@ -546,10 +554,7 @@ bool pcie_cap_has_lnkctl(const struct pci_dev *dev)
 
 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) &&
+	return pcie_downstream_port(dev) &&
 	       pcie_caps_reg(dev) & PCI_EXP_FLAGS_SLOT;
 }
 
@@ -628,10 +633,9 @@ int pcie_capability_read_word(struct pci_dev *dev, int pos, u16 *val)
 	 * State bit in the Slot Status register of Downstream Ports,
 	 * which must be hardwired to 1b.  (PCIe Base Spec 3.0, sec 7.8)
 	 */
-	if (pci_is_pcie(dev) && pos == PCI_EXP_SLTSTA &&
-		 pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM) {
+	if (pci_is_pcie(dev) && pcie_downstream_port(dev) &&
+	    pos == PCI_EXP_SLTSTA)
 		*val = PCI_EXP_SLTSTA_PDS;
-	}
 
 	return 0;
 }
@@ -657,10 +661,9 @@ int pcie_capability_read_dword(struct pci_dev *dev, int pos, u32 *val)
 		return ret;
 	}
 
-	if (pci_is_pcie(dev) && pos == PCI_EXP_SLTCTL &&
-		 pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM) {
+	if (pci_is_pcie(dev) && pcie_downstream_port(dev) &&
+	    pos == PCI_EXP_SLTSTA)
 		*val = PCI_EXP_SLTSTA_PDS;
-	}
 
 	return 0;
 }

Re: [PATCH 1/2] PCI: Fix pcie_port_device_resume() comment

[Rafael J. Wysocki]

On 7/14/2015 9:45 PM, Bjorn Helgaas wrote:
> The function comment claimed this was pcie_port_device_suspend(), but it's
> really pcie_port_device_resume().  Perils of cut and paste.
>
> Use the correct function name in the comment.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

ACK

> ---
>   drivers/pci/pcie/portdrv_core.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 2f0ce66..88122dc 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -448,7 +448,7 @@ static int resume_iter(struct device *dev, void *data)
>   }
>   
>   /**
> - * pcie_port_device_suspend - resume port services associated with a PCIe port
> + * pcie_port_device_resume - resume port services associated with a PCIe port
>    * @dev: PCI Express port to handle
>    */
>   int pcie_port_device_resume(struct device *dev)
>

Re: [PATCH 2/2] PCI: Add pcie_downstream_port() (true for Root and Switch Downstream Ports)

[Rafael J. Wysocki]

On 7/14/2015 9:45 PM, Bjorn Helgaas wrote:
> As used in the PCIe spec, "Downstream Port" includes both Root Ports and
> Switch Downstream Ports.  We sometimes checked for PCI_EXP_TYPE_DOWNSTREAM
> when we should have checked for PCI_EXP_TYPE_ROOT_PORT or
> PCI_EXP_TYPE_DOWNSTREAM.
>
> For a Root Port without a slot, the effect of this was that using
> pcie_capability_read_word() to read PCI_EXP_SLTSTA returned zero instead of
> showing the Presence Detect State bit hardwired to one as the PCIe Spec,
> r3.0, sec 7.8, requires.  (This read is completed in software because
> previous PCIe spec versions didn't require PCI_EXP_SLTSTA to exist at all.)
>
> Nothing in the kernel currently depends on this (pciehp only reads
> PCI_EXP_SLTSTA on ports with slots), so this is a cleanup and not a
> functional change.
>
> Add a pcie_downstream_port() helper function and use it.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

ACK

> ---
>   drivers/pci/access.c |   23 +++++++++++++----------
>   1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index d9b64a1..79f0102 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -531,6 +531,14 @@ static inline int pcie_cap_version(const struct pci_dev *dev)
>   	return pcie_caps_reg(dev) & PCI_EXP_FLAGS_VERS;
>   }
>   
> +static bool pcie_downstream_port(constr struct pci_dev *dev)
> +{
> +	int type = pci_pcie_type(dev);
> +
> +	return type == PCI_EXP_TYPE_ROOT_PORT ||
> +	       type == PCI_EXP_TYPE_DOWNSTREAM;
> +}
> +
>   bool pcie_cap_has_lnkctl(const struct pci_dev *dev)
>   {
>   	int type = pci_pcie_type(dev);
> @@ -546,10 +554,7 @@ bool pcie_cap_has_lnkctl(const struct pci_dev *dev)
>   
>   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) &&
> +	return pcie_downstream_port(dev) &&
>   	       pcie_caps_reg(dev) & PCI_EXP_FLAGS_SLOT;
>   }
>   
> @@ -628,10 +633,9 @@ int pcie_capability_read_word(struct pci_dev *dev, int pos, u16 *val)
>   	 * State bit in the Slot Status register of Downstream Ports,
>   	 * which must be hardwired to 1b.  (PCIe Base Spec 3.0, sec 7.8)
>   	 */
> -	if (pci_is_pcie(dev) && pos == PCI_EXP_SLTSTA &&
> -		 pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM) {
> +	if (pci_is_pcie(dev) && pcie_downstream_port(dev) &&
> +	    pos == PCI_EXP_SLTSTA)
>   		*val = PCI_EXP_SLTSTA_PDS;
> -	}
>   
>   	return 0;
>   }
> @@ -657,10 +661,9 @@ int pcie_capability_read_dword(struct pci_dev *dev, int pos, u32 *val)
>   		return ret;
>   	}
>   
> -	if (pci_is_pcie(dev) && pos == PCI_EXP_SLTCTL &&
> -		 pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM) {
> +	if (pci_is_pcie(dev) && pcie_downstream_port(dev) &&
> +	    pos == PCI_EXP_SLTSTA)
>   		*val = PCI_EXP_SLTSTA_PDS;
> -	}
>   
>   	return 0;
>   }
>