In order to solve a problem on a system where PCIe switch upstream port
incorrectly identifies itself as downstream port commit d0751b98dfa3
("PCI: Add dev->has_secondary_link to track downstream PCIe links")
introduced a flag to struct pci_dev that can be used to determine which
side of the port the link is.
The problem with this approach is that each time caller wants to
determine whether the port is upstream or downstream, it may need to
look at the dev->has_secondary_link as well. Making the calling code
unnecessary complicated.
Instead of this, we can correct the type of the port itself so that
pci_pcie_type() returns the actual type regardless of what the port
claims it is. Then update the users to call pci_pcie_type() and
pcie_downstream_port() accordingly.
This allows us to remove dev->has_secondary_flag completely.
Link: https://lore.kernel.org/linux-pci/20190703133953.GK128603@xxxxxxxxxx/
Suggested-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
---
I haven't been able to test this properly since I don't have access to any
system having the issue. I tried on a couple of systems without the issue
and did not see any problems, though.
@Robert, as the reporter of the original issue if you still have access to
any of these systems I would appreciate if you could give this patch series
a try.
drivers/pci/pci.c | 2 +-
drivers/pci/pcie/aspm.c | 8 +++----
drivers/pci/pcie/err.c | 2 +-
drivers/pci/probe.c | 48 ++++++++++++++++++++++++-----------------
drivers/pci/vc.c | 4 +++-
include/linux/pci.h | 1 -
6 files changed, 37 insertions(+), 28 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 9ac50710f1d4..3c0672f1dfe7 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3577,7 +3577,7 @@ int pci_enable_atomic_ops_to_root(struct pci_dev *dev, u32 cap_mask)
}
/* Ensure upstream ports don't block AtomicOps on egress */
- if (!bridge->has_secondary_link) {
+ if (pci_pcie_type(bridge) == PCI_EXP_TYPE_UPSTREAM) {
pcie_capability_read_dword(bridge, PCI_EXP_DEVCTL2,
&ctl2);
if (ctl2 & PCI_EXP_DEVCTL2_ATOMIC_EGRESS_BLOCK)
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 464f8f92653f..2776d34be693 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -913,10 +913,10 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
/*
* We allocate pcie_link_state for the component on the upstream
- * end of a Link, so there's nothing to do unless this device has a
- * Link on its secondary side.
+ * end of a Link, so there's nothing to do unless this device is
+ * downstream port.
*/
- if (!pdev->has_secondary_link)
+ if (!pcie_downstream_port(pdev))
return;
/* VIA has a strange chipset, root port is under a bridge */
@@ -1070,7 +1070,7 @@ static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
if (!pci_is_pcie(pdev))
return 0;
- if (pdev->has_secondary_link)
+ if (pcie_downstream_port(pdev))
parent = pdev;
if (!parent || !parent->link_state)
return -EINVAL;
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 773197a12568..b0e6048a9208 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -166,7 +166,7 @@ static pci_ers_result_t reset_link(struct pci_dev *dev, u32 service)
driver = pcie_port_find_service(dev, service);
if (driver && driver->reset_link) {
status = driver->reset_link(dev);
- } else if (dev->has_secondary_link) {
+ } else if (pcie_downstream_port(dev)) {
status = default_reset_link(dev);
} else {
pci_printk(KERN_DEBUG, dev, "no link-reset support at upstream device %s\n",
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index a3c7338fad86..01752a63cd15 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1431,26 +1431,38 @@ void set_pcie_port_type(struct pci_dev *pdev)
pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, ®16);
pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD;
+ parent = pci_upstream_bridge(pdev);
+ if (!parent)
+ return;
+
/*
- * A Root Port or a PCI-to-PCIe bridge is always the upstream end
- * of a Link. No PCIe component has two Links. Two Links are
- * connected by a Switch that has a Port on each Link and internal
- * logic to connect the two Ports.
+ * Some systems do not identify their upstream/downstream ports
+ * correctly so detect impossible configurations here and correct
+ * the port type accordingly.
*/
type = pci_pcie_type(pdev);
- if (type == PCI_EXP_TYPE_ROOT_PORT ||
- type == PCI_EXP_TYPE_PCIE_BRIDGE)
- pdev->has_secondary_link = 1;
- else if (type == PCI_EXP_TYPE_UPSTREAM ||
- type == PCI_EXP_TYPE_DOWNSTREAM) {
- parent = pci_upstream_bridge(pdev);
-
+ if (type == PCI_EXP_TYPE_DOWNSTREAM) {
/*
- * Usually there's an upstream device (Root Port or Switch
- * Downstream Port), but we can't assume one exists.
+ * If pdev claims to be downstream port but the parent
+ * device is also downstream port assume pdev is actually
+ * upstream port.
*/
- if (parent && !parent->has_secondary_link)
- pdev->has_secondary_link = 1;
+ if (pcie_downstream_port(parent)) {
+ pci_info(pdev, "claims to be downstream port but is acting as upstream port, correcting type\n");
+ pdev->pcie_flags_reg &= ~PCI_EXP_FLAGS_TYPE;
+ pdev->pcie_flags_reg |= PCI_EXP_TYPE_UPSTREAM;
+ }
+ } else if (type == PCI_EXP_TYPE_UPSTREAM) {
+ /*
+ * If pdev claims to be upstream port but the parent
+ * device is also upstream port assume pdev is actually
+ * downstream port.
+ */
+ if (pci_pcie_type(parent) == PCI_EXP_TYPE_UPSTREAM) {
+ pci_info(pdev, "claims to be upstream port but is acting as downstream port, correcting type\n");
+ pdev->pcie_flags_reg &= ~PCI_EXP_FLAGS_TYPE;
+ pdev->pcie_flags_reg |= PCI_EXP_TYPE_DOWNSTREAM;
+ }
}
}
@@ -2764,12 +2776,8 @@ static int only_one_child(struct pci_bus *bus)
* A PCIe Downstream Port normally leads to a Link with only Device
* 0 on it (PCIe spec r3.1, sec 7.3.1). As an optimization, scan
* only for Device 0 in that situation.
- *
- * Checking has_secondary_link is a hack to identify Downstream
- * Ports because sometimes Switches are configured such that the
- * PCIe Port Type labels are backwards.
*/
- if (bridge && pci_is_pcie(bridge) && bridge->has_secondary_link)
+ if (bridge && pci_is_pcie(bridge) && pcie_downstream_port(bridge))
return 1;
return 0;
diff --git a/drivers/pci/vc.c b/drivers/pci/vc.c
index 5acd9c02683a..9ae9fb9339e8 100644
--- a/drivers/pci/vc.c
+++ b/drivers/pci/vc.c
@@ -13,6 +13,8 @@
#include <linux/pci_regs.h>
#include <linux/types.h>
+#include "pci.h"
+
/**
* pci_vc_save_restore_dwords - Save or restore a series of dwords
* @dev: device
@@ -105,7 +107,7 @@ static void pci_vc_enable(struct pci_dev *dev, int pos, int res)
struct pci_dev *link = NULL;
/* Enable VCs from the downstream device */
- if (!dev->has_secondary_link)
+ if (!pci_is_pcie(dev) || !pcie_downstream_port(dev))
return;
ctrl_pos = pos + PCI_VC_RES_CTRL + (res * PCI_CAP_VC_PER_VC_SIZEOF);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 82e4cd1b7ac3..2f8990246316 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -418,7 +418,6 @@ struct pci_dev {
unsigned int broken_intx_masking:1; /* INTx masking can't be used */
unsigned int io_window_1k:1; /* Intel bridge 1K I/O windows */
unsigned int irq_managed:1;
- unsigned int has_secondary_link:1;
unsigned int non_compliant_bars:1; /* Broken BARs; ignore them */
unsigned int is_probed:1; /* Device probing in progress */
unsigned int link_active_reporting:1;/* Device capable of reporting link active */
--
2.23.0.rc1
