On Thu, 10 Apr 2025, Lukas Wunner wrote:
> 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@xxxxxxxxxx>
> Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
--
i.
> ---
> drivers/pci/hotplug/pciehp.h | 1 +
> drivers/pci/hotplug/pciehp_core.c | 29 -----------------------
> drivers/pci/hotplug/pciehp_hpc.c | 49 +++++++++++++++++++++++++++++++++------
> 3 files changed, 43 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index 273dd8c..debc79b0 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -187,6 +187,7 @@ struct controller {
> 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 997841c..f59baa9 100644
> --- 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 != (pdev->vendor | (pdev->device << 16)) ||
> - pci_read_config_dword(pdev, PCI_CLASS_REVISION, ®) ||
> - 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 != (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 8a09fb6..388fbed 100644
> --- 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 != (pdev->vendor | (pdev->device << 16)) ||
> + pci_read_config_dword(pdev, PCI_CLASS_REVISION, ®) ||
> + 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 != (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);
> }
>
> /*
>
