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

[Bjorn Helgaas]

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@xxxxxxxxxxxxxxx
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Stefan Roese <stefan.roese@xxxxxxxxxxx>
> ---
>  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.