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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <B6127BDA-03D1-4BFB-BA69-A91DC452BE9D@oracle.com>
Date: Wed, 28 Jan 2026 17:08:23 +0000
From: Haakon Bugge <haakon.bugge@...cle.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: Bjorn Helgaas <bhelgaas@...gle.com>,
        Niklas Schnelle
	<schnelle@...ux.ibm.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, Jan 22, 2026 at 02:09:53PM +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.
>> ...
>> 
>> +	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");
> 
> I know I suggested all this code and the message, but I'm not sure
> it's worth it.  If the device doesn't work,

I see your point. If the situation my commit is fixing has been there, the system would have failed, and a fix to the firmware must have been applied. Hence, so need to fix it in the OS.

> that will be obvious
> anyway, so this all feels over-engineered.
> 
>> +		lnkctl &= ~PCI_EXP_LNKCTL_RCB;
>> +	}
>> +
>> +	pcie_capability_write_word(dev, PCI_EXP_LNKCTL, lnkctl);
>> +}
> 
> What do you think about this?
> 
>    PCI: Initialize RCB from pci_configure_device()
> 
>    Commit e42010d8207f ("PCI: Set Read Completion Boundary to 128 iff Root
>    Port supports it (_HPX)") worked around a bogus _HPX type 2 record, which
>    caused program_hpx_type2() to set the RCB in an endpoint even though the
>    Root Port did not have the RCB bit set.
> 
>    e42010d8207f fixed that by setting the RCB in the endpoint only when it was
>    set in the Root Port.
> 
>    In retrospect, program_hpx_type2() is intended for AER-related settings,
>    and the RCB should be configured elsewhere so it doesn't depend on the
>    presence or contents of an _HPX record.
> 
>    Explicitly program the RCB from pci_configure_device() so it matches the
>    Root Port's RCB.  The Root Port may not be visible to virtualized guests;
>    in that case, leave RCB alone.
> 
>    Fixes: e42010d8207f ("PCI: Set Read Completion Boundary to 128 iff Root Port supports it (_HPX)")

Crisp and clear. For this and the other commit, is it OK that I add you as a co-developer? Aka:

Co-developed-by: Bjorn Helgaas <helgaas@...nel.org>
Signed-off-by: Bjorn Helgaas <helgaas@...nel.org>
Signed-off-by: Håkon Bugge <haakon.bugge@...cle.com>

?


> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 41183aed8f5d..8571c7c6e1a0 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2410,6 +2410,37 @@ static void pci_configure_serr(struct pci_dev *dev)
> 	}
> }
> 
> +static void pci_configure_rcb(struct pci_dev *dev)
> +{
> +	struct pci_dev *rp;
> +	u16 rp_lnkctl;
> +
> +	/*
> +	 * 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 (!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)
> +		return;
> +
> +	/* Root Port often not visible to virtualized guests */
> +	rp = pcie_find_root_port(dev);
> +	if (!rp)
> +		return;
> +
> +	pcie_capability_read_word(rp, PCI_EXP_LNKCTL, &rp_lnkctl);
> +	pcie_capability_clear_and_set_word(dev, PCI_EXP_LNKCTL,
> +					   PCI_EXP_LNKCTL_RCB,
> +					   (rp_lnkctl & PCI_EXP_LNKCTL_RCB) ?
> +					   PCI_EXP_LNKCTL_RCB : 0);

Looks good to me! This will enforce the locked flavour of pcie_capability_clear_and_set_word(). Is that an overkill?

Again, thank for the effort you put into this, ,Bjorn!


Thxs, Håkon



> +}
> +
> static void pci_configure_device(struct pci_dev *dev)
> {
> 	pci_configure_mps(dev);
> @@ -2419,6 +2450,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);
> }