Re: [PATCH v3 1/2] PCI: Initialize RCB from pci_configure_device

[Niklas Schnelle <schnelle@linux.ibm.com>]

On Thu, 2026-01-22 at 14:09 +0100, Håkon Bugge wrote:
> Commit e42010d8207f ("PCI: Set Read Completion Boundary to 128 iff
> Root Port supports it (_HPX)") fixed a bogus _HPX type 2 record, which
> instructed program_hpx_type2() to set the RCB in an endpoint,
> although it's RC did not have the RCB bit set.
> 
--- snip ---
>  
> +static bool pcie_read_root_rcb(struct pci_dev *dev, bool *rcb)
> +{
> +	struct pci_dev *rp = pcie_find_root_port(dev);
> +	u16 lnkctl;
> +
> +	if (!rp)
> +		return false;
> +
> +	pcie_capability_read_word(rp, PCI_EXP_LNKCTL, &lnkctl);
> +
> +	*rcb = !!(lnkctl & PCI_EXP_LNKCTL_RCB);
> +	return true;
> +}

In principle this is one of these things where platforms like s390
where the root port is hidden (only s390?) might want a hook to use
firmware supplied information to determine if the hidden root port
supports the setting. I wonder if this would make sense as a __weak
pcibios_read_rcb_supported() function. Not saying we need this now,
just thinking out loud.

> +
> +static void pci_configure_rcb(struct pci_dev *dev)
> +{
> +	u16 lnkctl;
> +	bool rcb;
> +
> +	/*
> +	 * Per PCIe r7.0, sec 7.5.3.7, RCB is only meaningful in Root
> +	 * Ports (where it is read-only), Endpoints, and Bridges.  It
> +	 * may only be set for Endpoints and Bridges if it is set in
> +	 * the Root Port. For Endpoints, it is 'RsvdP' for Virtual
> +	 * Functions. If the Root Port's RCB cannot be determined, we
> +	 * bail out.
> +	 */
> +	if (!pci_is_pcie(dev) ||
> +	    pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> +	    pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM ||
> +	    pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
> +	    pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC ||
> +	    dev->is_virtfn || !pcie_read_root_rcb(dev, &rcb))
> +		return;

This solves the concern Bjorn had for hidden root ports in VMs and I
confirmed that the check correctly bails on s390 even for PFs. Thanks!

> +
> +	pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnkctl);
> +	if (rcb) {
> +		if (lnkctl & PCI_EXP_LNKCTL_RCB)
> +			return;
> +
> +		lnkctl |= PCI_EXP_LNKCTL_RCB;
> +	} else {
> +		if (!(lnkctl & PCI_EXP_LNKCTL_RCB))
> +			return;
> +
> +		pci_info(dev, FW_INFO "clearing RCB (RCB not set in Root Port)\n");

The FW_INFO in here seems to be a remnant from ACPI code. As far as I
know this isn't usually used in core PCI code and seems conceptually
misleading to me since this doesn't come out of ACPI or other firmware
code.

> +		lnkctl &= ~PCI_EXP_LNKCTL_RCB;
> +	}
> +
> +	pcie_capability_write_word(dev, PCI_EXP_LNKCTL, lnkctl);

I do see Ilpo's point about pcie_capability_clear_and_set_word() and
agree that it would look cleaner. For good measure I tested with that
variant too and do prefer it, especially since it also gets rid of the
lnkctl variable. On ther other hand the message might help identify
weird firmware behavior. So no strong preference from my side.

> +}
> +
>  static void pci_configure_device(struct pci_dev *dev)
>  {
>  	pci_configure_mps(dev);
> @@ -2419,6 +2471,7 @@ static void pci_configure_device(struct pci_dev *dev)
>  	pci_configure_aspm_l1ss(dev);
>  	pci_configure_eetlp_prefix(dev);
>  	pci_configure_serr(dev);
> +	pci_configure_rcb(dev);
>  
>  	pci_acpi_program_hp_params(dev);
>  }

I tested that this patch series does not create problems on s390 and
would leave the RCB setting as is if our firmware set it. Since the
second patch doesn't touch code that is build on s390 I think the
Tested-by only makes sense for this one.

So feel free to add my:

Tested-by: Niklas Schnelle <schnelle@linux.ibm.com>

Furthermore with either the FW_INFO dropped or Ilpo's suggestion
incorporated feel free to also add:

Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>