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

[Priyank Rathod]

On Tue, 1 Sep 2026, Ilpo Järvinen wrote:
> On Mon, 31 Aug 2026, Priyank Rathod wrote:
>
> >   - 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).

Hi Ilpo,

Sorry for the slow reply, and for posting LMR v3 before answering this.

You're right. The helper only returns Function 0, which isn't what the
ASPM driver needs, so I shouldn't have listed it as a user.

> >   - 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.

Agreed, PTM comes off the list.

> >   - 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.

Agreed, that one goes too.

> >   - 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.

The helper doesn't handle RCiEPs, and I don't have a patch converting
err.c, so I'll drop it as well.

That leaves LMR as the only user, so for v4 I dropped the exported
helper and kept the lookup private to margin.c. LMR only needs
Function 0 on the Upstream Port side (pcilmr makes the same
assumption).

On v7 you said you'd like to avoid duplicating what the ASPM driver
does. If you'd rather have a shared helper in the PCI core, I can do
that as a separate series that also converts aspm.c, so it has a
second user from the start. Let me know which you prefer.

> > +		*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.
>
> > +	} else {
> > +		*downstream_port = pci_upstream_bridge(pdev);
> > +		*upstream_port = pdev;
>
> Why complicate things with the unbalance?

Both were fixed in the copy in LMR v3 (patch 1/3), which takes a
reference on both ends. This code goes away in v4 anyway.

On ASPM, since you asked about it on v6 and v7: v4 drops
pci_aspm_inhibit() and doesn't change aspm.c. It uses the existing
API the way ath10k, ath11k and ath12k do: save with
pcie_aspm_enabled(), call pci_disable_link_state() before margining
and pci_force_enable_link_state() afterwards. Reading aspm.c, that
round trip doesn't restore everything, so v4 adds a few things in
margin.c:

 - If pcie_aspm_enabled() returns 0 (firmware left ASPM off,
   pcie_aspm=off, or the "performance" policy), v4 calls neither
   function.
 - It disables only the states pcie_aspm_enabled() reported, not
   PCIE_LINK_STATE_ASPM_ALL, so the restore clears the same
   aspm_disable bits (except the L1 substates, see below).
 - __pci_enable_link_state() sets clkpm_default from
   PCIE_LINK_STATE_CLKPM, which pcie_aspm_enabled() never reports, so
   with the default policy the restore would turn Clock PM off. v4
   adds PCIE_LINK_STATE_CLKPM to the restore mask if CLKREQ_EN was
   set at the start. The ath drivers hit this too.
 - It reads ASPM Control in Link Control on both ends (read-only) at
   the start and before each margining command, and fails with -EBUSY
   if it's set. That covers firmware-enabled ASPM with pcie_aspm=off,
   and ASPM coming back on mid-session through the link/*_aspm sysfs
   files or another driver calling pci_force_enable_link_state().

The rest is documented in pcie-lmr.rst:

 - If another driver disables a state that LMR also disabled, the
   restore turns it back on.
 - aspm_default ends up as the saved set.
 - Disabling L1 also disables the L1 substates, so substates that
   were off at the start stay disabled.
 - Without OS control of ASPM, the disable works but the restore
   returns -EPERM, so ASPM stays off until reboot.

Counting disables per state in aspm.c, as you suggested on v7, would
fix most of these, but v4 doesn't depend on it.

v3 also brought back the MSampleMultipleReceivers bit that you couldn't
find in r7.0 on v5. That was my mistake, and v4 removes it. Some
section numbers in v3 were wrong too (Link Control 2, for one), so v4
refers to registers and fields by name instead of section numbers, and
keeps comments within 80 columns.

Thanks,
Priyank