[1/2] PCI: Write RCB only when it changes

pci_configure_rcb() does a read-modify-write of the Link Control
register of every endpoint at enumeration, even when RCB already has
the right value. Some devices react to any write of this register.

The Renesas uPD720201 xHCI (1912:0014) comes out of reset with ASPM L0s
and L1 enabled in Link Control. On a link whose Root Port supports no
ASPM, nothing else writes that register before the driver loads, and
the chip clears ASPM Control itself during the firmware download. After
a host write, even of the unchanged value 0x0003, it no longer does
so. ASPM stays enabled, and the first access to the xHCI BAR runs into
PCIe completion timeouts that hang the system.

Seen on an AMD Versal board (CPM Root Port without ASPM support): the
hang bisects to this commit, reverting it fixes it, and on a kernel
without it a single setpci write of the unchanged value reproduces it.

Read Link Control first and write it only when RCB has to change.

Fixes: 1a6845aaa6de ("PCI: Initialize RCB from pci_configure_device()")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Stefan Roese <stefan.roese@mailbox.org>
---
 drivers/pci/probe.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

> On 30 Sep 2026, at 16:46, Stefan Roese <stefan.roese@mailbox.org> wrote:
> 
> pci_configure_rcb() does a read-modify-write of the Link Control
> register of every endpoint at enumeration, even when RCB already has
> the right value. Some devices react to any write of this register.
> 
> The Renesas uPD720201 xHCI (1912:0014) comes out of reset with ASPM L0s
> and L1 enabled in Link Control. On a link whose Root Port supports no
> ASPM, nothing else writes that register before the driver loads, and
> the chip clears ASPM Control itself during the firmware download. After
> a host write, even of the unchanged value 0x0003, it no longer does
> so. ASPM stays enabled, and the first access to the xHCI BAR runs into
> PCIe completion timeouts that hang the system.
> 
> Seen on an AMD Versal board (CPM Root Port without ASPM support): the
> hang bisects to this commit, reverting it fixes it, and on a kernel
> without it a single setpci write of the unchanged value reproduces it.
> 
> Read Link Control first and write it only when RCB has to change.

This was debated during the review of 1a6845aaa6de. The conditional
logic to avoid a Link Control write when RCB already matched was
considered “over-engineered”:
https://lore.kernel.org/lkml/20260122130957.68757-2-haakon.bugge@oracle.com/

Since an unconditional write to Link Control while programming RCB
does not violate the PCIe specification, this appears to be
device-specific behavior and should be handled with a device quirk
instead.


Thxs, Håkon

> 
> Fixes: 1a6845aaa6de ("PCI: Initialize RCB from pci_configure_device()")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Stefan Roese <stefan.roese@mailbox.org>
> ---
> drivers/pci/probe.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 721daf5c5184..35e794df3be4 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2426,7 +2426,7 @@ static void pci_configure_serr(struct pci_dev *dev)
> static void pci_configure_rcb(struct pci_dev *dev)
> {
> struct pci_dev *rp;
> - u16 rp_lnkctl;
> + u16 rp_lnkctl, lnkctl, rcb;
> 
> /*
> * Per PCIe r7.0, sec 7.5.3.7, RCB is only meaningful in Root Ports
> @@ -2448,10 +2448,17 @@ static void pci_configure_rcb(struct pci_dev *dev)
> return;
> 
> pcie_capability_read_word(rp, PCI_EXP_LNKCTL, &rp_lnkctl);
> - pcie_capability_clear_and_set_word(dev, PCI_EXP_LNKCTL,
> - PCI_EXP_LNKCTL_RCB,
> - (rp_lnkctl & PCI_EXP_LNKCTL_RCB) ?
> - PCI_EXP_LNKCTL_RCB : 0);
> + rcb = rp_lnkctl & PCI_EXP_LNKCTL_RCB;
> +
> + /*
> + * Write Link Control only when RCB actually changes. Some devices
> + * react to any write of this register, even one with an unchanged
> + * value.
> + */
> + pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnkctl);
> + if ((lnkctl & PCI_EXP_LNKCTL_RCB) != rcb)
> + pcie_capability_clear_and_set_word(dev, PCI_EXP_LNKCTL,
> + PCI_EXP_LNKCTL_RCB, rcb);
> }
> 
> static void pci_configure_device(struct pci_dev *dev)
> -- 
> 2.56.0
> 
> 
>

On Wed, Sep 30, 2026 at 04:46:49PM +0200, Stefan Roese wrote:
> pci_configure_rcb() does a read-modify-write of the Link Control
> register of every endpoint at enumeration, even when RCB already has
> the right value. Some devices react to any write of this register.
> 
> The Renesas uPD720201 xHCI (1912:0014) comes out of reset with ASPM L0s
> and L1 enabled in Link Control. On a link whose Root Port supports no
> ASPM, nothing else writes that register before the driver loads, and
> the chip clears ASPM Control itself during the firmware download. After
> a host write, even of the unchanged value 0x0003, it no longer does
> so. ASPM stays enabled, and the first access to the xHCI BAR runs into
> PCIe completion timeouts that hang the system.

In "After a host write, even of the unchanged value 0x0003, it no
longer does so", what are you saying it no longer does?

Are you saying the chip no longer clears ASPM Control during firmware
download?

The PCIe Mini Card CEM and M.2 specs both say L0s and L1 should be
enabled by default, so I guess it makes sense that they're set when
coming out of reset.

But PCIe r7.0, sec 5.4.1.4, says the result is undefined if software
enables L0s when the other end of the link doesn't support it, and I
guess writing 0x0003 (ASPM L0s and L1 enabled) counts as enabling L0s,
and we certainly got undefined results.

> Seen on an AMD Versal board (CPM Root Port without ASPM support): the
> hang bisects to this commit, reverting it fixes it, and on a kernel
> without it a single setpci write of the unchanged value reproduces it.
> 
> Read Link Control first and write it only when RCB has to change.
> 
> Fixes: 1a6845aaa6de ("PCI: Initialize RCB from pci_configure_device()")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Stefan Roese <stefan.roese@mailbox.org>
> ---
>  drivers/pci/probe.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 721daf5c5184..35e794df3be4 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2426,7 +2426,7 @@ static void pci_configure_serr(struct pci_dev *dev)
>  static void pci_configure_rcb(struct pci_dev *dev)
>  {
>  	struct pci_dev *rp;
> -	u16 rp_lnkctl;
> +	u16 rp_lnkctl, lnkctl, rcb;
>  
>  	/*
>  	 * Per PCIe r7.0, sec 7.5.3.7, RCB is only meaningful in Root Ports
> @@ -2448,10 +2448,17 @@ static void pci_configure_rcb(struct pci_dev *dev)
>  		return;
>  
>  	pcie_capability_read_word(rp, PCI_EXP_LNKCTL, &rp_lnkctl);
> -	pcie_capability_clear_and_set_word(dev, PCI_EXP_LNKCTL,
> -					   PCI_EXP_LNKCTL_RCB,
> -					   (rp_lnkctl & PCI_EXP_LNKCTL_RCB) ?
> -					   PCI_EXP_LNKCTL_RCB : 0);
> +	rcb = rp_lnkctl & PCI_EXP_LNKCTL_RCB;
> +
> +	/*
> +	 * Write Link Control only when RCB actually changes. Some devices
> +	 * react to any write of this register, even one with an unchanged
> +	 * value.
> +	 */
> +	pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnkctl);
> +	if ((lnkctl & PCI_EXP_LNKCTL_RCB) != rcb)
> +		pcie_capability_clear_and_set_word(dev, PCI_EXP_LNKCTL,
> +						   PCI_EXP_LNKCTL_RCB, rcb);

What if we just did this:

  if (rp_lnkctl & PCI_EXP_LNKCTL_RCB)
    pcie_capability_set_word(dev, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_RCB);

I don't know if it's ever necessary to *clear* RCB.  If RCB is set in
an Endpoint when it's not set in the Root Port, that would be a
firmware configuration error.

Either way, it's ugly magic to avoid the ASPM Control write here based
on the unrelated RCB settings.  But avoiding the read/modify/write is
probably worth doing just from a performance point of view.

On Wed, Sep 30, 2026 at 06:22:14PM -0500, Bjorn Helgaas wrote:
> In "After a host write, even of the unchanged value 0x0003, it no
> longer does so", what are you saying it no longer does?
>
> Are you saying the chip no longer clears ASPM Control during firmware
> download?

Yes, exactly. I watched Link Control with a kprobe on the config
write path. Without any host write to Link Control, it changes from
0x0003 to 0x0000 about 11-19 ms into the firmware download by
xhci-pci-renesas, between two config writes of the driver to its
download registers. Linux does not write Link Control there, so the
chip does it itself.

After a single host write of the unchanged value 0x0003, either from
pci_configure_rcb() or from a manual setpci on a kernel without
1a6845aaa6de, Link Control stays 0x0003 through the whole download.
The first MMIO access to the xHCI BAR afterwards ends in completion
timeouts, an AER error and link loss.

> But PCIe r7.0, sec 5.4.1.4, says the result is undefined if software
> enables L0s when the other end of the link doesn't support it, and I
> guess writing 0x0003 (ASPM L0s and L1 enabled) counts as enabling L0s,
> and we certainly got undefined results.

Agreed. The chip's own clearing has been hiding that ASPM is enabled
against a Root Port that supports no ASPM. So the real fix is in
aspm.c (v1 2/2, now 1/2), and the RCB write is only what exposed it.

> What if we just did this:
>
>   if (rp_lnkctl & PCI_EXP_LNKCTL_RCB)
>     pcie_capability_set_word(dev, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_RCB);

Fine with me. On this board RCB is clear in the Root Port, so this
also avoids the write here.

v2 makes the aspm.c change the first patch (with Fixes: and stable),
adds the saved state update that sashiko pointed out, and uses your
set-only variant as the second patch. I tested the combinations on the
board, v6.18.40 with the patches backported, 3 cold boots each:

- aspm.c patch alone: good, the "clearing ASPM Control" message shows
  up for the xHCI, after the RCB write.
- both patches: good.
- set-only patch alone, booted with pcie_aspm=off: good, the chip
  clears ASPM Control itself again.
- aspm.c patch alone, booted with pcie_aspm=off: bad, completion
  timeouts as before.

The last case is why the set-only patch now carries Fixes: and stable
as well: with pcie_aspm=off the aspm.c change never runs, and only
avoiding the Link Control write keeps the system alive. The same holds
for CONFIG_PCIEASPM=n. If you would rather take the set-only patch
without the stable tag, that is fine with me.

v2 follows.

Thanks,
Stefan

Hi Håkon,

On 30 Sep 2026, at 16:41, Haakon Bugge wrote:
> Since an unconditional write to Link Control while programming RCB
> does not violate the PCIe specification, this appears to be
> device-specific behavior and should be handled with a device quirk
> instead.

You're right that the RCB write itself is legal. What is not legal is
the state it leaves behind: ASPM L0s/L1 enabled on a link whose Root
Port supports no ASPM (r7.0, sec 5.4.1.4, as Bjorn pointed out).
pcie_aspm_cap_init() returns early for such a link and never clears
ASPM Control, so this is not specific to one device.

I did look at a quirk. The existing helper, pcie_aspm_remove_cap(),
only clears the support flags, which leads into the same early return
and leaves ASPM Control set. A quirk that clears it would have to
repeat the aspm.c link checks. So v2 will fix this in aspm.c, and
keep only Bjorn's set-only RCB change in probe.c.

Thanks,
Stefan