Re: [PATCH] PCI: Add pcie_get_link_endpoints() helper

[Ilpo Järvinen]

On Mon, 31 Aug 2026, Priyank Rathod wrote:

> In PCIe topologies, physical links are point-to-point connections
> between an Upstream Component (Downstream Port, such as a Root Port
> or Switch Downstream Port) and a Downstream Component (Upstream Port,
> such as an Endpoint or Switch Upstream Port), per PCIe Base Specification
> r7.0/r6.0 sec 1.3.1.
> 
> Currently, various drivers across drivers/pci/ independently infer the
> two ends of a PCIe link using varied ad-hoc methods:
>   - ASPM (drivers/pci/pcie/aspm.c): pcie_aspm_get_link(),
>     alloc_pcie_link_state(), and pci_configure_ltr() resolve parent
>     bridges and subordinate function 0.

This is not the full picture. The aspm driver has to write the same config 
to all functions (except L1SS that is only in func 0).

>   - Precision Time Measurement (drivers/pci/pcie/ptm.c):
>     pci_upstream_ptm() traverses pci_upstream_bridge() to validate
>     upstream link partner capabilities.

This looks something entirely different, and it's pure get (a certain) 
device upstream, not the get-my-link-partner query.

>   - PCIe Link Retrain (drivers/pci/pci.c): pcie_retrain_link() and
>     pcie_update_link_speed() coordinate link retraining from the
>     Downstream Port across the subordinate bus.

I don't entirely agree with this assessment. pcie_retrain_link() is given 
a downstream port, mere looking into ->subordinate doesn't constitute as a 
need for having access to the struct of the other end of the link.

>   - AER/DPC Recovery (drivers/pci/pcie/err.c): pcie_do_recovery()
>     identifies the parent bridge via pci_upstream_bridge() to trigger
>     secondary bus reset and broadcast driver recovery callbacks.

This seems to have some validity, though it must cover RCiEPs.

>   - Lane Margining at Receiver (LMR): margin_enable_write() requires
>     both link endpoints to establish hierarchical locking and runtime PM
>     pinning.
> 
> Ad-hoc subordinate bus iteration risks races with concurrent device
> removal or hotplug if pci_bus_sem is omitted.
> 
> Introduce pcie_get_link_endpoints() and pcie_put_link_endpoints() in the
> PCI core to provide a standardized, symmetric, and race-safe helper:
>   - For Endpoints: resolves the parent Downstream Port via
>     pci_upstream_bridge(pdev).
>   - For Downstream Ports: safely inspects the subordinate bus under
>     down_read(&pci_bus_sem) and acquires a reference via pci_dev_get()
>     on the child device.
> 
> Callers release the acquired reference using pcie_put_link_endpoints().
> 
> Signed-off-by: Priyank Rathod <rathodpriyank@xxxxxxxxxx>
> ---
> Hi Bjorn, Ilpo, and PCI maintainers,
> 
> During the review of the PCIe Lane Margining at Receiver (LMR) patch series
> (v7: https://lore.kernel.org/linux-pci/20260828-pcie-lmt-v7-1-6012e9e0940a@xxxxxxxxxx/),
> Ilpo Järvinen pointed out that feature drivers (such as LMR) resolving link
> partners duplicate link traversal logic that is already present in drivers such
> as ASPM:
> 
>   "This feels like duplicating similar functionality with the aspm driver
>    that also wants to infer ends of the link when giving a pci_dev in.
>    The aspm driver currently does that within, but it kind of duplicating
>    pci_bus. It would be nice to avoid the duplication and have something
>    similar for this in PCI core."
> 
> In PCIe topologies, physical links are point-to-point interconnects
> between an Upstream Component (Downstream Port, such as a Root Port or Switch
> Downstream Port) and a Downstream Component (Upstream Port, such as an Endpoint
> or Switch Upstream Port), per PCIe Base Specification Revision 7.0 / 6.0
> Section 1.3.1.
> 
> Several subsystems across drivers/pci/ independently infer and coordinate both
> ends of a PCIe link:
>   1. ASPM (drivers/pci/pcie/aspm.c): pcie_aspm_get_link(),
>      alloc_pcie_link_state(), and pci_configure_ltr() resolve parent bridges
>      and subordinate function 0 to configure ASPMC and L1SS.
>   2. Precision Time Measurement (drivers/pci/pcie/ptm.c): pci_upstream_ptm()
>      traverses pci_upstream_bridge() to validate upstream link partner
>      capabilities.
>   3. PCIe Link Retrain (drivers/pci/pci.c): pcie_retrain_link() and
>      pcie_update_link_speed() coordinate link retraining from the Downstream
>      Port across the subordinate bus.
>   4. AER & DPC Recovery (drivers/pci/pcie/err.c): pcie_do_recovery() identifies
>      the parent bridge via pci_upstream_bridge() to trigger secondary bus
>      resets and broadcast driver error callbacks.
>   5. Lane Margining at Receiver (LMR): margin_enable_write() requires both
>      link endpoints to establish hierarchical locking (pci_dev_lock) and
>      runtime PM pinning.
> 
> Currently, these drivers independently implement ad-hoc traversals via
> pci_upstream_bridge() or subordinate bus device iteration. Ad-hoc subordinate
> bus iteration is error-prone and risks races with concurrent hot-unplug or
> device removal if pci_bus_sem is omitted.
> 
> This patch introduces pcie_get_link_endpoints() and pcie_put_link_endpoints()
> in the PCI core (drivers/pci/pci.c and include/linux/pci.h) as a standalone
> helper:
>   - For Endpoints: resolves the parent Downstream Port via
>     pci_upstream_bridge(pdev).
>   - For Downstream Ports: safely inspects the subordinate bus under
>     down_read(&pci_bus_sem) and acquires a reference via pci_dev_get() on the
>     child device.
> 
> Callers release the acquired reference with pcie_put_link_endpoints().
> 
> Validation:
>   - Compiled clean on x86_64 defconfig (0 warnings, 0 errors).
>   - Multi-architecture build validated for ARM64 and x86_64 targets.
>   - Passed checkpatch.pl (0 warnings, 0 errors).
>   - Passed Sashiko pre-commit test runner (ID: 46b5f8a3da1ca0056407b3db6c82b001264f86b7).
> ---
>  drivers/pci/pci.c   | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h |  6 ++++++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 77b17b13ee61..7bdfe7e5ab3a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4618,6 +4618,56 @@ int pcie_retrain_link(struct pci_dev *pdev, bool use_lt)
>  	return rc;
>  }
>  
> +/**
> + * pcie_get_link_endpoints - Identify Upstream and Downstream ends of a PCIe link
> + * @pdev: Any PCIe device on the link (Downstream Port or Endpoint)
> + * @downstream_port: Output pointer to Downstream Port (Upstream Component)
> + * @upstream_port: Output pointer to Upstream Port (Downstream Component)
> + *
> + * Identifies both ends of a point-to-point PCIe link. Increments reference count
> + * on @upstream_port if dynamically discovered on a downstream port. Callers must
> + * release with pcie_put_link_endpoints().
> + *
> + * Return: 0 on success, or -EINVAL if @pdev is NULL or not PCIe.
> + */
> +int pcie_get_link_endpoints(struct pci_dev *pdev,
> +			    struct pci_dev **downstream_port,
> +			    struct pci_dev **upstream_port)
> +{
> +	if (!pdev || !pci_is_pcie(pdev))
> +		return -EINVAL;
> +
> +	if (pcie_downstream_port(pdev)) {
> +		*downstream_port = pdev;
> +		down_read(&pci_bus_sem);
> +		*upstream_port = pdev->subordinate ?
> +			pci_dev_get(list_first_entry_or_null(&pdev->subordinate->devices,
> +							     struct pci_dev, bus_list)) : NULL;

Too much is crammed into one statement.

> +		up_read(&pci_bus_sem);
> +	} else {
> +		*downstream_port = pci_upstream_bridge(pdev);
> +		*upstream_port = pdev;

Why complicate things with the unbalance?

> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pcie_get_link_endpoints);
> +
> +/**
> + * pcie_put_link_endpoints - Release references acquired by pcie_get_link_endpoints
> + * @pdev: Device passed to pcie_get_link_endpoints()
> + * @downstream_port: Downstream Port pointer
> + * @upstream_port: Upstream Port pointer
> + */
> +void pcie_put_link_endpoints(struct pci_dev *pdev,
> +			     struct pci_dev *downstream_port,
> +			     struct pci_dev *upstream_port)
> +{
> +	if (pdev && pcie_downstream_port(pdev) && upstream_port)
> +		pci_dev_put(upstream_port);
> +}
> +EXPORT_SYMBOL_GPL(pcie_put_link_endpoints);

--
 i.