Message ID | 20250225-qps615_v4_1-v4-7-e08633a7bdf8@oss.qualcomm.com (mailing list archive) |
---|---|
State | New |
Delegated to: | Krzysztof WilczyĆski |
Headers | show |
Series | PCI: Enable Power and configure the TC956x PCIe switch | expand |
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. > --- 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
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 */
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(-)