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

[Haakon Bugge <haakon.bugge@oracle.com>]

> On 22 Jan 2026, at 14:45, Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> 
> 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.

Well, not exactly, as there will be no writes unless required. This was discussed here [1].

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

No other reasons but performance.

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

That may be a good idea, but for sure, outside the scope of this series.


Thxs, Håkon

[1] https://lore.kernel.org/linux-pci/ECE29E32-7925-44C3-BAAA-B16003E9E997@oracle.com/