----- Original Message -----
> From: "Bjorn Helgaas" <helgaas@xxxxxxxxxx>
> To: "Timothy Pearson" <tpearson@xxxxxxxxxxxxxxxxxxxxx>
> Cc: "linux-pci" <linux-pci@xxxxxxxxxxxxxxx>, "mahesh" <mahesh@xxxxxxxxxxxxx>, "Oliver" <oohall@xxxxxxxxx>, "Madhavan
> Srinivasan" <maddy@xxxxxxxxxxxxx>, "Michael Ellerman" <mpe@xxxxxxxxxxxxxx>, "Lukas Wunner" <lukas@xxxxxxxxx>
> Sent: Tuesday, June 17, 2025 10:03:01 AM
> Subject: Re: [PATCH v7] PCI: Add pcie_link_is_active() to determine if the PCIe link
> [+cc ppc folks, Lukas]
>
> On Tue, Jun 17, 2025 at 09:25:24AM -0500, Timothy Pearson wrote:
>> is active
>
> Subject line needs to be complete in itself. No need to include
> "PCIe" in the "PCIe link" part. We already say PCI or PCIe twice even
> without that.
>
> Commit log needs to be complete in itself also. It's OK to repeat the
> subject line in the commit log, but definitely not to split a sentence
> across them.
Agreed. Will fix in v8.
>> Introduce a common API to check if the PCIe link is active, replacing
>> duplicate code in multiple locations.
>
> Mention the actual function name here, not just "a common API", e.g.,
> "Add pcie_link_is_active() ..."
>
>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@xxxxxxxxxxxxxxxx>
>> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Timothy Pearson <tpearson@xxxxxxxxxxxxxxxxxxxxx>
>> ---
>> arch/powerpc/kernel/eeh_driver.c | 8 +++++++-
>> 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 ++++++++++++++++++++++++++---
>> include/linux/pci.h | 4 ++++
>> 6 files changed, 43 insertions(+), 36 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
>> index 441a3562bddd..4fdd62432f2c 100644
>> --- a/arch/powerpc/kernel/eeh_driver.c
>> +++ b/arch/powerpc/kernel/eeh_driver.c
>> @@ -1097,8 +1097,14 @@ void eeh_handle_normal_event(struct eeh_pe *pe)
>> eeh_pe_dev_mode_mark(pe, EEH_DEV_REMOVED);
>>
>> pci_lock_rescan_remove();
>> - pci_hp_remove_devices(bus);
>> + bus = eeh_pe_bus_get(pe);
>> + if (bus)
>> + pci_hp_remove_devices(bus);
>> + else
>> + pr_err("%s: PCI bus for PHB#%x-PE#%x disappeared\n",
>> + __func__, pe->phb->global_number, pe->addr);
>> pci_unlock_rescan_remove();
>
> Is this an unrelated change included here by mistake? I don't see the
> connection with pcie_link_is_active().
Yes, this was an inadvertent inclusion. I have a large ppc hotplug patch series in the works that depends on this functionality, am testing the entire series on ppc hardware, and inadvertently included that line.
>
>> /* The passed PE should no longer be used */
>> return;
>> }
>> 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..ad639e60f3bd 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 = %x\n", lnk_status);
>
> I know this is just carried over from the original definition, but
> "%#06x" would remove the ambiguity about whether the value is in
> decimal or hex.
Agreed. Will fix in v8.
>> + return !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
>> +}
>> +EXPORT_SYMBOL(pcie_link_is_active);
>
> Unless something outside drivers/pci/ actually needs this, it should
> not be exported and it should be declared in drivers/pci/pci.h
> instead.
Agreed. Will fix in v8.
>> /**
>> * 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 05e68f35f392..5d1c9f718ac8 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1993,6 +1993,7 @@ pci_release_mem_regions(struct pci_dev *pdev)
>> pci_select_bars(pdev, IORESOURCE_MEM));
>> }
>>
>> +int pcie_link_is_active(struct pci_dev *dev);
>> #else /* CONFIG_PCI is not enabled */
>>
>> static inline void pci_set_flags(int flags) { }
>> @@ -2141,6 +2142,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; }
>
> I doubt we would need this stub.
Agreed. Will fix in v8.
> > #endif /* CONFIG_PCI */
