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

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

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.

> 
> > > +
> > > +static void pci_configure_rcb(struct pci_dev *dev)
> 
--- snip ---
> > > +
> > > +	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).

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.

> 
> > > +		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,
Niklas