linux-kernel - Re: [PATCH v3 1/2] PCI: Initialize RCB from pci_configure

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7E4D523D-5105-4173-AC1A-8B7898DC48A8@oracle.com>
Date: Fri, 23 Jan 2026 17:38:19 +0000
From: Haakon Bugge <haakon.bugge@...cle.com>
To: Niklas Schnelle <schnelle@...ux.ibm.com>
CC: Bjorn Helgaas <bhelgaas@...gle.com>, Alex Williamson <alex@...zbot.org>,
        Johannes Thumshirn <morbidrsa@...il.com>,
        "linux-pci@...r.kernel.org"
	<linux-pci@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "linux-acpi@...r.kernel.org"
	<linux-acpi@...r.kernel.org>
Subject: Re: [PATCH v3 1/2] PCI: Initialize RCB from pci_configure_device

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