[+cc ppc folks, Lukas]
On Tue, Jun 17, 2025 at 09:25:24AM -0500, Timothy Pearson wrote:
> is active
Subject line needs to be complete in itself. No need to include
"PCIe" in the "PCIe link" part. We already say PCI or PCIe twice even
without that.
Commit log needs to be complete in itself also. It's OK to repeat the
subject line in the commit log, but definitely not to split a sentence
across them.
> Introduce a common API to check if the PCIe link is active, replacing
> duplicate code in multiple locations.
Mention the actual function name here, not just "a common API", e.g.,
"Add pcie_link_is_active() ..."
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@xxxxxxxxxxxxxxxx>
> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Timothy Pearson <tpearson@xxxxxxxxxxxxxxxxxxxxx>
> ---
> arch/powerpc/kernel/eeh_driver.c | 8 +++++++-
> drivers/pci/hotplug/pciehp.h | 1 -
> drivers/pci/hotplug/pciehp_ctrl.c | 2 +-
> drivers/pci/hotplug/pciehp_hpc.c | 33 +++----------------------------
> drivers/pci/pci.c | 31 ++++++++++++++++++++++++++---
> include/linux/pci.h | 4 ++++
> 6 files changed, 43 insertions(+), 36 deletions(-)
>
> diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
> index 441a3562bddd..4fdd62432f2c 100644
> --- a/arch/powerpc/kernel/eeh_driver.c
> +++ b/arch/powerpc/kernel/eeh_driver.c
> @@ -1097,8 +1097,14 @@ void eeh_handle_normal_event(struct eeh_pe *pe)
> eeh_pe_dev_mode_mark(pe, EEH_DEV_REMOVED);
>
> pci_lock_rescan_remove();
> - pci_hp_remove_devices(bus);
> + bus = eeh_pe_bus_get(pe);
> + if (bus)
> + pci_hp_remove_devices(bus);
> + else
> + pr_err("%s: PCI bus for PHB#%x-PE#%x disappeared\n",
> + __func__, pe->phb->global_number, pe->addr);
> pci_unlock_rescan_remove();
Is this an unrelated change included here by mistake? I don't see the
connection with pcie_link_is_active().
> /* The passed PE should no longer be used */
> return;
> }
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index debc79b0adfb..79df49cc9946 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -186,7 +186,6 @@ int pciehp_query_power_fault(struct controller *ctrl);
> 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);
>
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index bcc938d4420f..6cc1b27b3b11 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -260,7 +260,7 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
> /* Turn the slot on if it's occupied or link is up */
> mutex_lock(&ctrl->state_lock);
> present = pciehp_card_present(ctrl);
> - link_active = pciehp_check_link_active(ctrl);
> + link_active = pcie_link_is_active(ctrl->pcie->port);
> if (present <= 0 && link_active <= 0) {
> if (ctrl->state == BLINKINGON_STATE) {
> ctrl->state = OFF_STATE;
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index ebd342bda235..d29ce3715a44 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -221,33 +221,6 @@ static void pcie_write_cmd_nowait(struct controller *ctrl, u16 cmd, u16 mask)
> pcie_do_write_cmd(ctrl, cmd, mask, false);
> }
>
> -/**
> - * pciehp_check_link_active() - Is the link active
> - * @ctrl: PCIe hotplug controller
> - *
> - * Check whether the downstream link is currently active. Note it is
> - * possible that the card is removed immediately after this so the
> - * caller may need to take it into account.
> - *
> - * If the hotplug controller itself is not available anymore returns
> - * %-ENODEV.
> - */
> -int pciehp_check_link_active(struct controller *ctrl)
> -{
> - struct pci_dev *pdev = ctrl_dev(ctrl);
> - u16 lnk_status;
> - int ret;
> -
> - ret = pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
> - if (ret == PCIBIOS_DEVICE_NOT_FOUND || PCI_POSSIBLE_ERROR(lnk_status))
> - return -ENODEV;
> -
> - ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
> - ctrl_dbg(ctrl, "%s: lnk_status = %x\n", __func__, lnk_status);
> -
> - return ret;
> -}
> -
> static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
> {
> u32 l;
> @@ -467,7 +440,7 @@ int pciehp_card_present_or_link_active(struct controller *ctrl)
> if (ret)
> return ret;
>
> - return pciehp_check_link_active(ctrl);
> + return pcie_link_is_active(ctrl_dev(ctrl));
> }
>
> int pciehp_query_power_fault(struct controller *ctrl)
> @@ -614,7 +587,7 @@ static void pciehp_ignore_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_device_replaced(ctrl))
> + if (!pcie_link_is_active(ctrl_dev(ctrl)) || pciehp_device_replaced(ctrl))
> pciehp_request(ctrl, ignored_events);
> up_read(&ctrl->reset_lock);
> }
> @@ -921,7 +894,7 @@ int pciehp_slot_reset(struct pcie_device *dev)
> pcie_capability_write_word(dev->port, PCI_EXP_SLTSTA,
> PCI_EXP_SLTSTA_DLLSC);
>
> - if (!pciehp_check_link_active(ctrl))
> + if (!pcie_link_is_active(ctrl_dev(ctrl)))
> pciehp_request(ctrl, PCI_EXP_SLTSTA_DLLSC);
>
> return 0;
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e9448d55113b..ad639e60f3bd 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4908,7 +4908,6 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
> return 0;
>
> if (pcie_get_speed_cap(dev) <= PCIE_SPEED_5_0GT) {
> - u16 status;
>
> pci_dbg(dev, "waiting %d ms for downstream link\n", delay);
> msleep(delay);
> @@ -4924,8 +4923,7 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
> if (!dev->link_active_reporting)
> return -ENOTTY;
>
> - pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &status);
> - if (!(status & PCI_EXP_LNKSTA_DLLLA))
> + if (pcie_link_is_active(dev) <= 0)
> return -ENOTTY;
>
> return pci_dev_wait(child, reset_type,
> @@ -6230,6 +6228,33 @@ void pcie_print_link_status(struct pci_dev *dev)
> }
> EXPORT_SYMBOL(pcie_print_link_status);
>
> +/**
> + * pcie_link_is_active() - Checks if the link is active or not
> + * @pdev: PCI device to query
> + *
> + * Check whether the physical link is active or not. Note it is
> + * possible that the card is removed immediately after this so the
> + * caller may need to take it into account.
> + *
> + * If the PCI device itself is not available anymore returns
> + * %-ENODEV.
> + *
> + * Return: link state, or -ENODEV if the config read failes.
> + */
> +int pcie_link_is_active(struct pci_dev *pdev)
> +{
> + u16 lnk_status;
> + int ret;
> +
> + ret = pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
> + if (ret == PCIBIOS_DEVICE_NOT_FOUND || PCI_POSSIBLE_ERROR(lnk_status))
> + return -ENODEV;
> +
> + pci_dbg(pdev, "lnk_status = %x\n", lnk_status);
I know this is just carried over from the original definition, but
"%#06x" would remove the ambiguity about whether the value is in
decimal or hex.
> + return !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
> +}
> +EXPORT_SYMBOL(pcie_link_is_active);
Unless something outside drivers/pci/ actually needs this, it should
not be exported and it should be declared in drivers/pci/pci.h
instead.
> /**
> * pci_select_bars - Make BAR mask from the type of resource
> * @dev: the PCI device for which BAR mask is made
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 05e68f35f392..5d1c9f718ac8 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1993,6 +1993,7 @@ pci_release_mem_regions(struct pci_dev *pdev)
> pci_select_bars(pdev, IORESOURCE_MEM));
> }
>
> +int pcie_link_is_active(struct pci_dev *dev);
> #else /* CONFIG_PCI is not enabled */
>
> static inline void pci_set_flags(int flags) { }
> @@ -2141,6 +2142,9 @@ pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
> {
> return -ENOSPC;
> }
> +
> +static inline bool pcie_link_is_active(struct pci_dev *dev)
> +{ return false; }
I doubt we would need this stub.
> #endif /* CONFIG_PCI */
