PCI: pciehp: Ignore Presence Detect Changed caused by DPC

Commit a97396c6eb13 ("PCI: pciehp: Ignore Link Down/Up caused by DPC")
amended PCIe hotplug to not bring down the slot upon Data Link Layer State
Changed events caused by Downstream Port Containment.

However Keith reports off-list that if the slot uses in-band presence
detect (i.e. Presence Detect State is derived from Data Link Layer Link
Active), DPC also causes a spurious Presence Detect Changed event.

This needs to be ignored as well.

Unfortunately there's no register indicating that in-band presence detect
is used.  PCIe r5.0 sec 7.5.3.10 introduced the In-Band PD Disable bit in
the Slot Control Register.  The PCIe hotplug driver sets this bit on
ports supporting it.  But older ports may still use in-band presence
detect.

If in-band presence detect can be disabled, Presence Detect Changed events
occurring during DPC must not be ignored because they signal device
replacement.  On all other ports, device replacement cannot be detected
reliably because the Presence Detect Changed event could be a side effect
of DPC.  On those (older) ports, perform a best-effort device replacement
check by comparing the Vendor ID, Device ID and other data in Config Space
with the values cached in struct pci_dev.  Use the existing helper
pciehp_device_replaced() to accomplish this.  It is currently #ifdef'ed to
CONFIG_PM_SLEEP in pciehp_core.c, so move it to pciehp_hpc.c where most
other functions accessing config space reside.

Reported-by: Keith Busch <kbusch@kernel.org>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Link: https://patch.msgid.link/fa264ff71952915c4e35a53c89eb0cde8455a5c5.1744298239.git.lukas@wunner.de

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 273dd8c66f4eff8b62ab065cebf97db3c343977d..debc79b0adfb2c8e06aabb765e1741572685100b 100644 (file)
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -187,6 +187,7 @@ int pciehp_card_present(struct controller *ctrl);
 int pciehp_card_present_or_link_active(struct controller *ctrl);
 int pciehp_check_link_status(struct controller *ctrl);
 int pciehp_check_link_active(struct controller *ctrl);
+bool pciehp_device_replaced(struct controller *ctrl);
 void pciehp_release_ctrl(struct controller *ctrl);
 
 int pciehp_sysfs_enable_slot(struct hotplug_slot *hotplug_slot);

diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 997841c6989359e5834d5946d25096ce3689782d..f59baa91297099ed194ce255c98e01d14a4e4241 100644 (file)
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -284,35 +284,6 @@ static int pciehp_suspend(struct pcie_device *dev)
        return 0;
 }
 
-static bool pciehp_device_replaced(struct controller *ctrl)
-{
-       struct pci_dev *pdev __free(pci_dev_put) = NULL;
-       u32 reg;
-
-       if (pci_dev_is_disconnected(ctrl->pcie->port))
-               return false;
-
-       pdev = pci_get_slot(ctrl->pcie->port->subordinate, PCI_DEVFN(0, 0));
-       if (!pdev)
-               return true;
-
-       if (pci_read_config_dword(pdev, PCI_VENDOR_ID, &reg) ||
-           reg != (pdev->vendor | (pdev->device << 16)) ||
-           pci_read_config_dword(pdev, PCI_CLASS_REVISION, &reg) ||
-           reg != (pdev->revision | (pdev->class << 8)))
-               return true;
-
-       if (pdev->hdr_type == PCI_HEADER_TYPE_NORMAL &&
-           (pci_read_config_dword(pdev, PCI_SUBSYSTEM_VENDOR_ID, &reg) ||
-            reg != (pdev->subsystem_vendor | (pdev->subsystem_device << 16))))
-               return true;
-
-       if (pci_get_dsn(pdev) != ctrl->dsn)
-               return true;
-
-       return false;
-}
-
 static int pciehp_resume_noirq(struct pcie_device *dev)
 {
        struct controller *ctrl = get_service_data(dev);

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 8a09fb6083e27669a12f1a3bb2a550369d471d16..388fbed25402b5dcd110e64461fb5f83e09e27d3 100644 (file)
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -563,18 +563,48 @@ void pciehp_power_off_slot(struct controller *ctrl)
                 PCI_EXP_SLTCTL_PWR_OFF);
 }
 
+bool pciehp_device_replaced(struct controller *ctrl)
+{
+       struct pci_dev *pdev __free(pci_dev_put) = NULL;
+       u32 reg;
+
+       if (pci_dev_is_disconnected(ctrl->pcie->port))
+               return false;
+
+       pdev = pci_get_slot(ctrl->pcie->port->subordinate, PCI_DEVFN(0, 0));
+       if (!pdev)
+               return true;
+
+       if (pci_read_config_dword(pdev, PCI_VENDOR_ID, &reg) ||
+           reg != (pdev->vendor | (pdev->device << 16)) ||
+           pci_read_config_dword(pdev, PCI_CLASS_REVISION, &reg) ||
+           reg != (pdev->revision | (pdev->class << 8)))
+               return true;
+
+       if (pdev->hdr_type == PCI_HEADER_TYPE_NORMAL &&
+           (pci_read_config_dword(pdev, PCI_SUBSYSTEM_VENDOR_ID, &reg) ||
+            reg != (pdev->subsystem_vendor | (pdev->subsystem_device << 16))))
+               return true;
+
+       if (pci_get_dsn(pdev) != ctrl->dsn)
+               return true;
+
+       return false;
+}
+
 static void pciehp_ignore_dpc_link_change(struct controller *ctrl,
-                                         struct pci_dev *pdev, int irq)
+                                         struct pci_dev *pdev, int irq,
+                                         u16 ignored_events)
 {
        /*
         * Ignore link changes which occurred while waiting for DPC recovery.
         * Could be several if DPC triggered multiple times consecutively.
         */
        synchronize_hardirq(irq);
-       atomic_and(~PCI_EXP_SLTSTA_DLLSC, &ctrl->pending_events);
+       atomic_and(~ignored_events, &ctrl->pending_events);
        if (pciehp_poll_mode)
                pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
-                                          PCI_EXP_SLTSTA_DLLSC);
+                                          ignored_events);
        ctrl_info(ctrl, "Slot(%s): Link Down/Up ignored (recovered by DPC)\n",
                  slot_name(ctrl));
 
@@ -584,8 +614,8 @@ static void pciehp_ignore_dpc_link_change(struct controller *ctrl,
         * Synthesize it to ensure that it is acted on.
         */
        down_read_nested(&ctrl->reset_lock, ctrl->depth);
-       if (!pciehp_check_link_active(ctrl))
-               pciehp_request(ctrl, PCI_EXP_SLTSTA_DLLSC);
+       if (!pciehp_check_link_active(ctrl) || pciehp_device_replaced(ctrl))
+               pciehp_request(ctrl, ignored_events);
        up_read(&ctrl->reset_lock);
 }
 
@@ -736,8 +766,13 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
         */
        if ((events & PCI_EXP_SLTSTA_DLLSC) && pci_dpc_recovered(pdev) &&
            ctrl->state == ON_STATE) {
-               events &= ~PCI_EXP_SLTSTA_DLLSC;
-               pciehp_ignore_dpc_link_change(ctrl, pdev, irq);
+               u16 ignored_events = PCI_EXP_SLTSTA_DLLSC;
+
+               if (!ctrl->inband_presence_disabled)
+                       ignored_events |= events & PCI_EXP_SLTSTA_PDC;
+
+               events &= ~ignored_events;
+               pciehp_ignore_dpc_link_change(ctrl, pdev, irq, ignored_events);
        }
 
        /*