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.h | 1 -
drivers/pci/hotplug/pciehp_ctrl.c | 7 ++++---
drivers/pci/hotplug/pciehp_hpc.c | 33 +++------------------------------
drivers/pci/pci.c | 26 +++++++++++++++++++++++---
include/linux/pci.h | 4 ++++
5 files changed, 34 insertions(+), 37 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 273dd8c66f4eff8b62ab065cebf97db3c343977d..acef728530e36d6ea4d7db3afe97ed31b85be064 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);
void pciehp_release_ctrl(struct controller *ctrl);
int pciehp_sysfs_enable_slot(struct hotplug_slot *hotplug_slot);
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index d603a7aa74838c748f6ac2d22ffb8b8cfe64e469..36468a9c31d669ec916e867ecfb7a8220cfab157 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -230,7 +230,8 @@ void pciehp_handle_disable_request(struct controller *ctrl)
void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
{
- int present, link_active;
+ bool link_active;
+ int present;
/*
* If the slot is on and presence or link has changed, turn it off.
@@ -260,8 +261,8 @@ 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);
- if (present <= 0 && link_active <= 0) {
+ link_active = pcie_link_is_active(ctrl->pcie->port);
+ if (present <= 0 && !link_active) {
if (ctrl->state == BLINKINGON_STATE) {
ctrl->state = OFF_STATE;
cancel_delayed_work(&ctrl->button_work);
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 8a09fb6083e27669a12f1a3bb2a550369d471d16..278bc21d531dd20a38e06e5d33f5ccd18131c2c3 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)
@@ -584,7 +557,7 @@ 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))
+ if (!pcie_link_is_active(ctrl_dev(ctrl)))
pciehp_request(ctrl, PCI_EXP_SLTSTA_DLLSC);
up_read(&ctrl->reset_lock);
}
@@ -884,7 +857,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 4d7c9f64ea24ec754a135a2585c99489cfa641a9..d14cd6843a020f2cec3e4cc36522526cf1faf0ba 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4926,7 +4926,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);
@@ -4942,8 +4941,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))
return -ENOTTY;
return pci_dev_wait(child, reset_type,
@@ -6251,6 +6249,28 @@ 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 link is active or not.
+ *
+ * Return: true if link is active.
+ */
+bool 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 false;
+
+ pci_dbg(pdev, "lnk_status = %x\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/include/linux/pci.h b/include/linux/pci.h
index 09cda518350c8ea86bf1c6bd64ed8d67e774c8df..2c34302dc5bb73aa2f9e3bd02c12684d8b6856d9 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1945,6 +1945,7 @@ pci_release_mem_regions(struct pci_dev *pdev)
pci_select_bars(pdev, IORESOURCE_MEM));
}
+bool pcie_link_is_active(struct pci_dev *dev);
#else /* CONFIG_PCI is not enabled */
static inline void pci_set_flags(int flags) { }
@@ -2093,6 +2094,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; }
#endif /* CONFIG_PCI */
/* Include architecture-dependent settings and functions */
--
2.34.1On Sat, Apr 12, 2025 at 07:19:56AM +0530, Krishna Chaitanya Chundru wrote:
> 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.h | 1 -
> drivers/pci/hotplug/pciehp_ctrl.c | 7 ++++---
> drivers/pci/hotplug/pciehp_hpc.c | 33 +++------------------------------
> drivers/pci/pci.c | 26 +++++++++++++++++++++++---
> include/linux/pci.h | 4 ++++
> 5 files changed, 34 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index 273dd8c66f4eff8b62ab065cebf97db3c343977d..acef728530e36d6ea4d7db3afe97ed31b85be064 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);
> void pciehp_release_ctrl(struct controller *ctrl);
>
> int pciehp_sysfs_enable_slot(struct hotplug_slot *hotplug_slot);
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index d603a7aa74838c748f6ac2d22ffb8b8cfe64e469..36468a9c31d669ec916e867ecfb7a8220cfab157 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -230,7 +230,8 @@ void pciehp_handle_disable_request(struct controller *ctrl)
>
> void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
> {
> - int present, link_active;
> + bool link_active;
> + int present;
>
> /*
> * If the slot is on and presence or link has changed, turn it off.
> @@ -260,8 +261,8 @@ 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);
> - if (present <= 0 && link_active <= 0) {
> + link_active = pcie_link_is_active(ctrl->pcie->port);
> + if (present <= 0 && !link_active) {
> if (ctrl->state == BLINKINGON_STATE) {
> ctrl->state = OFF_STATE;
> cancel_delayed_work(&ctrl->button_work);
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 8a09fb6083e27669a12f1a3bb2a550369d471d16..278bc21d531dd20a38e06e5d33f5ccd18131c2c3 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.
You've lost this somewhat important comment that still exists after this
patch.
Rob
On Sat, Apr 12, 2025 at 07:19:56AM +0530, Krishna Chaitanya Chundru wrote:
> 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>
Reviewed-by: Lukas Wunner <lukas@wunner.de>
One heads-up and one nit:
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -584,7 +557,7 @@ 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))
> + if (!pcie_link_is_active(ctrl_dev(ctrl)))
> pciehp_request(ctrl, PCI_EXP_SLTSTA_DLLSC);
> up_read(&ctrl->reset_lock);
> }
Heads-up: There's a trivial merge conflict here with what's queued on
pci.git/hotplug. No need for you to respin because I expect this will be
merged through a different topic branch anyway, but Bjorn and the other
maintainers will see the merge conflict when generating the next branch.
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1945,6 +1945,7 @@ pci_release_mem_regions(struct pci_dev *pdev)
> pci_select_bars(pdev, IORESOURCE_MEM));
> }
>
> +bool pcie_link_is_active(struct pci_dev *dev);
> #else /* CONFIG_PCI is not enabled */
>
> static inline void pci_set_flags(int flags) { }
> @@ -2093,6 +2094,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; }
> #endif /* CONFIG_PCI */
Nit: Seems like this would still fit within 80 chars:
static inline bool pcie_link_is_active(struct pci_dev *dev) { return false; }
That said, all existing callers of this function as well as the new one
introduced by this series are only compiled in the CONFIG_PCI=y case,
so I'm not sure the inline stub is necessary at all?
Thanks,
Lukas
On 4/12/2025 9:22 AM, Lukas Wunner wrote:
> On Sat, Apr 12, 2025 at 07:19:56AM +0530, Krishna Chaitanya Chundru wrote:
>> 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>
>
> Reviewed-by: Lukas Wunner <lukas@wunner.de>
>
> One heads-up and one nit:
>
>> --- a/drivers/pci/hotplug/pciehp_hpc.c
>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
>> @@ -584,7 +557,7 @@ 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))
>> + if (!pcie_link_is_active(ctrl_dev(ctrl)))
>> pciehp_request(ctrl, PCI_EXP_SLTSTA_DLLSC);
>> up_read(&ctrl->reset_lock);
>> }
>
> Heads-up: There's a trivial merge conflict here with what's queued on
> pci.git/hotplug. No need for you to respin because I expect this will be
> merged through a different topic branch anyway, but Bjorn and the other
> maintainers will see the merge conflict when generating the next branch.
>
>
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1945,6 +1945,7 @@ pci_release_mem_regions(struct pci_dev *pdev)
>> pci_select_bars(pdev, IORESOURCE_MEM));
>> }
>>
>> +bool pcie_link_is_active(struct pci_dev *dev);
>> #else /* CONFIG_PCI is not enabled */
>>
>> static inline void pci_set_flags(int flags) { }
>> @@ -2093,6 +2094,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; }
>> #endif /* CONFIG_PCI */
>
> Nit: Seems like this would still fit within 80 chars:
>
> static inline bool pcie_link_is_active(struct pci_dev *dev) { return false; }
>
> That said, all existing callers of this function as well as the new one
> introduced by this series are only compiled in the CONFIG_PCI=y case,
> so I'm not sure the inline stub is necessary at all?
>
If any other driver other than these two drivers in future wants to use
this API, it can be useful in that case.
- Krishna Chaitanya.
> Thanks,
>
> Lukas
On Sat, Apr 12, 2025 at 05:52:56AM +0200, Lukas Wunner wrote: > On Sat, Apr 12, 2025 at 07:19:56AM +0530, Krishna Chaitanya Chundru wrote: > > 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> > > Reviewed-by: Lukas Wunner <lukas@wunner.de> Looking at this with a fresh pair of eyeballs, I realize there's an issue here, so unfortunately I have to retract the Reviewed-by: pcie_link_is_active() differs from the existing pciehp_check_link_active() in that it returns 0 not only if the link is down, but also if the Config Space read returns with an error. In particular, if Config Space of a hotplug bridge is inaccessible, 0 is returned instead of -ENODEV with this patch. That can happen if the hotplug bridge itself has been hot-removed, which is common with Thunderbolt, but also on servers with nested PCIe switches. The existing invocations of pciehp_check_link_active() do the right thing if the hotplug bridge has been hot-removed, but after this patch they no longer do. For example in this hunk ... > > --- a/drivers/pci/hotplug/pciehp_hpc.c > > +++ b/drivers/pci/hotplug/pciehp_hpc.c > > @@ -584,7 +557,7 @@ 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)) > > + if (!pcie_link_is_active(ctrl_dev(ctrl))) > > pciehp_request(ctrl, PCI_EXP_SLTSTA_DLLSC); > > up_read(&ctrl->reset_lock); > > } ... pciehp_request() will be called if the hotplug bridge was hot-removed, which isn't the right thing to do. The current behavior is to do nothing. I realize I steered you in the wrong direction because in my review of your v4 I asked why pcie_link_is_active() doesn't return a bool: https://lore.kernel.org/all/Z72TRBvpzizcgm9S@wunner.de/ So I sincerely apologize for that! You actually did the right thing in v4 by returning a negative int if the Config Space read returned an error. Thanks, Lukas
On 4/13/2025 10:44 PM, Lukas Wunner wrote: > On Sat, Apr 12, 2025 at 05:52:56AM +0200, Lukas Wunner wrote: >> On Sat, Apr 12, 2025 at 07:19:56AM +0530, Krishna Chaitanya Chundru wrote: >>> 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> >> >> Reviewed-by: Lukas Wunner <lukas@wunner.de> > > Looking at this with a fresh pair of eyeballs, I realize there's an issue > here, so unfortunately I have to retract the Reviewed-by: > > pcie_link_is_active() differs from the existing pciehp_check_link_active() > in that it returns 0 not only if the link is down, but also if the > Config Space read returns with an error. > > In particular, if Config Space of a hotplug bridge is inaccessible, > 0 is returned instead of -ENODEV with this patch. That can happen if > the hotplug bridge itself has been hot-removed, which is common with > Thunderbolt, but also on servers with nested PCIe switches. > > The existing invocations of pciehp_check_link_active() do the right > thing if the hotplug bridge has been hot-removed, but after this patch > they no longer do. For example in this hunk ... > >>> --- a/drivers/pci/hotplug/pciehp_hpc.c >>> +++ b/drivers/pci/hotplug/pciehp_hpc.c >>> @@ -584,7 +557,7 @@ 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)) >>> + if (!pcie_link_is_active(ctrl_dev(ctrl))) >>> pciehp_request(ctrl, PCI_EXP_SLTSTA_DLLSC); >>> up_read(&ctrl->reset_lock); >>> } > > ... pciehp_request() will be called if the hotplug bridge was > hot-removed, which isn't the right thing to do. The current > behavior is to do nothing. > > I realize I steered you in the wrong direction because in my > review of your v4 I asked why pcie_link_is_active() doesn't > return a bool: > > https://lore.kernel.org/all/Z72TRBvpzizcgm9S@wunner.de/ > > So I sincerely apologize for that! You actually did the right > thing in v4 by returning a negative int if the Config Space read > returned an error. > Ok, No problem. I will revert v4 patch i.e returning int instead of bool. I will wait for few days for any other new review comments. - Krishna Chaitanya. > Thanks, > > Lukas
© 2016 - 2026 Red Hat, Inc.