Re: [PATCH v10] PCI: Add pcie_link_is_active() function

[Krishna Chaitanya Chundru <krishna.chundru@xxxxxxxxxxxxxxxx>]

On 6/17/2025 9:11 PM, Timothy Pearson wrote:
> Add pcie_link_is_active() function to check if the physical PCIe link is
> active, replacing duplicate code in multiple locations.
>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@xxxxxxxxxxxxxxxx>
> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Timothy Pearson <tpearson@xxxxxxxxxxxxxxxxxxxxx>
> ---
Hi Timothy,

Are you going to respin this patch.
If not I can take this patch in my series which I am going to post next
week.

- Krishna Chaitanya.
>   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 ++++++++++++++++++++++++++---
>   drivers/pci/pci.h                 |  1 +
>   5 files changed, 33 insertions(+), 35 deletions(-)
>
> 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..4e96ff8ee5ec 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 = %#06x\n", lnk_status);
> +	return !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
> +}
> +EXPORT_SYMBOL(pcie_link_is_active);
> +
>   /**
>    * pci_select_bars - Make BAR mask from the type of resource
>    * @dev: the PCI device for which BAR mask is made
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 12215ee72afb..cf1afb718f8a 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -231,6 +231,7 @@ static inline int pci_proc_detach_bus(struct pci_bus *bus) { return 0; }
>   /* Functions for PCI Hotplug drivers to use */
>   int pci_hp_add_bridge(struct pci_dev *dev);
>   bool pci_hp_spurious_link_change(struct pci_dev *pdev);
> +int pcie_link_is_active(struct pci_dev *dev);
>   
>   #if defined(CONFIG_SYSFS) && defined(HAVE_PCI_LEGACY)
>   void pci_create_legacy_files(struct pci_bus *bus);