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

Haakon Bugge
2026-01-23T17:38:45Z

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

That may be a good idea. But I am not quite sure how we can find the Root Port from an "orphan" bridge or endpoint through the pci_bios_read set of interfaces.

>> +
>> +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!

Thanks for confirming!

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

I humbly disagree. As per PCIe r7.0, sec 7.5.3.7: "Default value of this bit is 0b". So, if we find it set, and it is not set in the Root Port, to me, it must be a firmware bug. And that is exactly what FW_INFO is intended for: "Introduce FW_BUG, FW_WARN and FW_INFO to consistenly tell users about BIOS bugs" (commit a0ad05c75aa3).

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

OK.

>> +}
>> +
>> 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>

Thanks a lot for the testing!

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

Thanks for the conditional r-b. I'll like Bjorn to chime in here, as he introduced FW_INFO and did not object to the existing (non Ilpo variant).

Thxs, Håkon