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

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

> On 23 Jan 2026, at 19:54, Niklas Schnelle <schnelle@linux.ibm.com> wrote:
> 
> On Fri, 2026-01-23 at 17:38 +0000, Haakon Bugge wrote:
>>>> 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.
> 
> I only meant to have it as a function in a similar manner to e.g.
> pcibios_enable_device() that we could override. But I don't think that
> needs or even should be part of this patch as there wouldn't be a user
> of the override yet.

Understood.

[snip]

>>> 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).
> 
> Ah thanks for the pointer, I wasn't aware of this explicit "for
> reporting FW issues" use. Reading commit a0ad05c75aa3 ("Introduce
> FW_BUG, FW_WARN and FW_INFO to consistenly tell users about BIOS bugs")
> now I agree this makes sense after all.

I must admit, I wasn't either, before I read that commit :-)

>> 
>>>> + lnkctl &= ~PCI_EXP_LNKCTL_RCB;
>>>> + }
>>>> +
>>>> + pcie_capability_write_word(dev, PCI_EXP_LNKCTL, lnkctl);
>>> 
> --- snip ---
>>> 
>>> 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).
> 
> As stated above with the additional explanation this makes sense to me
> now so no need for the conditional anymore either ;)

Thanks for both your t-b and r-b on this commit!


Thxs, Håkon