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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4155 bytes --]

On Thu, 22 Jan 2026, 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.
> 
> e42010d8207f fixed that by qualifying the setting of the RCB in the
> endpoint with the RC supporting an 128 byte RCB.
> 
> In retrospect, the program_hpx_type2() should only modify the AER
> bits, and stay away from fiddling with the Link Control Register.
> 
> Hence, we explicitly program the RCB from pci_configure_device().
> 
> According to PCIe r7.0, sec 7.5.3.7, RCB is only valid for Root Ports
> (where it is Read-Only), Bridges, and Endpoints. The bit is 'RsvdP'
> for Virtual Functions. Hence, for other cases than Bridges and Physical
> Endpoints, we bail out early from pci_configure_rcb().
> 
> If the Root Port's RCB cannot be determined, we do nothing.
> 
> If RCB is set in the Root Port and not in the device, we set it. If it
> is set in the device but not in the Root Port, we print an info
> message and reset it.
> 
> Fixes: Commit e42010d8207f ("PCI: Set Read Completion Boundary to 128 iff Root Port supports it (_HPX)")
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> 
> ---
> 
> Note, that the current duplication of pcie_root_rcb_set() will be
> removed in the next commit.
> 
> v2 -> v3:
>    * Qualified the device types more strictly
>    * s/pcie_root_rcb_set/pcie_read_root_rcb/ and changed signature
>    * Do nothing if the RP's RCB cannot be determined
>    * Reset the device's RCB if not set in the RP
> ---
>  drivers/pci/probe.c | 53 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 41183aed8f5d9..7165ac4065c97 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2410,6 +2410,58 @@ static void pci_configure_serr(struct pci_dev *dev)
>  	}
>  }
>  
> +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;
> +}
> +
> +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;
> +
> +	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");
> +		lnkctl &= ~PCI_EXP_LNKCTL_RCB;
> +	}
> +
> +	pcie_capability_write_word(dev, PCI_EXP_LNKCTL, lnkctl);

So this sequence is effectively implementing this simple statement:

	pcie_capability_clear_and_set_word(dev, PCI_EXP_LNKCTL,
					   PCI_EXP_LNKCTL_RCB,
					   rcb ? PCI_EXP_LNKCTL_RCB : 0);

+ the print.

Is there a good reason why you want to avoid the write by using early 
returns?


I also wonder if those clear & set & clean_and_set interfaces should 
implement the write avoidance if it's an useful thing (callers should be 
checked they're not used for RW1C bits if that's implemented though).

--
 i.