| Message ID | 20250412-qps615_v4_1-v5-7-5b6a06132fec@oss.qualcomm.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Enable Power and configure the TC9563 PCIe switch | expand |
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 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 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/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
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 */
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(-)