Introduce a common API to check if the PCIe link is active, replacing
duplicate code in multiple locations.
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
drivers/pci/hotplug/pciehp_hpc.c | 13 +------------
drivers/pci/pci.c | 26 +++++++++++++++++++++++---
include/linux/pci.h | 5 +++++
3 files changed, 29 insertions(+), 15 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index bb5a8d9f03ad..d0a2efebb519 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -234,18 +234,7 @@ static void pcie_write_cmd_nowait(struct controller *ctrl, u16 cmd, u16 mask)
*/
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;
+ return pcie_is_link_active(ctrl_dev(ctrl));
}
static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 869d204a70a3..3d4fe6fefa13 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4907,7 +4907,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);
@@ -4923,8 +4922,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_is_link_active(dev))
return -ENOTTY;
return pci_dev_wait(child, reset_type,
@@ -6219,6 +6217,28 @@ void pcie_print_link_status(struct pci_dev *dev)
}
EXPORT_SYMBOL(pcie_print_link_status);
+/**
+ * pcie_is_link_active() - Checks if the link is active or not
+ * @pdev: PCI device to query
+ *
+ * Check whether the link is active or not.
+ *
+ * If the config read returns error then return -ENODEV.
+ */
+int pcie_is_link_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);
+ return !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
+}
+EXPORT_SYMBOL(pcie_is_link_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/include/linux/pci.h b/include/linux/pci.h
index bbec32be668b..84bb98e61e8a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1946,6 +1946,7 @@ pci_release_mem_regions(struct pci_dev *pdev)
pci_select_bars(pdev, IORESOURCE_MEM));
}
+int pcie_is_link_active(struct pci_dev *dev);
#else /* CONFIG_PCI is not enabled */
static inline void pci_set_flags(int flags) { }
@@ -2094,6 +2095,10 @@ pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
{
return -ENOSPC;
}
+
+static inline int pcie_is_link_active(struct pci_dev *dev)
+{ return -ENODEV; }
+
#endif /* CONFIG_PCI */
/* Include architecture-dependent settings and functions */
--
2.34.1On Tue, Feb 25, 2025 at 03:04:04PM +0530, Krishna Chaitanya Chundru wrote:
> Introduce a common API to check if the PCIe link is active, replacing
> duplicate code in multiple locations.
[...]
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -234,18 +234,7 @@ static void pcie_write_cmd_nowait(struct controller *ctrl, u16 cmd, u16 mask)
> */
> 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;
> + return pcie_is_link_active(ctrl_dev(ctrl));
> }
Please replace all call sites of pciehp_check_link_active() with a call
to the new function.
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4923,8 +4922,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_is_link_active(dev))
> return -ENOTTY;
Missing negation.
> +/**
> + * pcie_is_link_active() - Checks if the link is active or not
> + * @pdev: PCI device to query
> + *
> + * Check whether the link is active or not.
> + *
> + * If the config read returns error then return -ENODEV.
> + */
> +int pcie_is_link_active(struct pci_dev *pdev)
Why not return bool?
I don't quite like the function name because in English the correct word
order is subject - predicate - object, i.e. pcie_link_is_active() or
even shorter, pcie_link_active().
> @@ -2094,6 +2095,10 @@ pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
> {
> return -ENOSPC;
> }
> +
> +static inline int pcie_is_link_active(struct pci_dev *dev)
> +{ return -ENODEV; }
> +
> #endif /* CONFIG_PCI */
Is the empty inline really necessary? What breaks if you leave it out?
Thanks,
Lukas
On 2/25/2025 3:24 PM, Lukas Wunner wrote:
> On Tue, Feb 25, 2025 at 03:04:04PM +0530, Krishna Chaitanya Chundru wrote:
>> Introduce a common API to check if the PCIe link is active, replacing
>> duplicate code in multiple locations.
> [...]
>> --- a/drivers/pci/hotplug/pciehp_hpc.c
>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
>> @@ -234,18 +234,7 @@ static void pcie_write_cmd_nowait(struct controller *ctrl, u16 cmd, u16 mask)
>> */
>> 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;
>> + return pcie_is_link_active(ctrl_dev(ctrl));
>> }
>
> Please replace all call sites of pciehp_check_link_active() with a call
> to the new function.
>
>
ack
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -4923,8 +4922,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_is_link_active(dev))
>> return -ENOTTY;
>
> Missing negation.
>
>
ack
>> +/**
>> + * pcie_is_link_active() - Checks if the link is active or not
>> + * @pdev: PCI device to query
>> + *
>> + * Check whether the link is active or not.
>> + *
>> + * If the config read returns error then return -ENODEV.
>> + */
>> +int pcie_is_link_active(struct pci_dev *pdev)
>
> Why not return bool?
>
pciehp_check_link_active is expecting int to make sure this new function
not disturbing the hotplug driver I added return type as int, I can
change it to bool if it fine with hotplug drivers.
> I don't quite like the function name because in English the correct word
> order is subject - predicate - object, i.e. pcie_link_is_active() or
> even shorter, pcie_link_active().
>
ack
>
>> @@ -2094,6 +2095,10 @@ pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
>> {
>> return -ENOSPC;
>> }
>> +
>> +static inline int pcie_is_link_active(struct pci_dev *dev)
>> +{ return -ENODEV; }
>> +
>> #endif /* CONFIG_PCI */
>
> Is the empty inline really necessary? What breaks if you leave it out?
>
ack I will remove it.
- Krishna Chaitanya.
> Thanks,
>
> Lukas
© 2016 - 2026 Red Hat, Inc.