linux-kernel - Re: [PATCH] PCI/ACPI: Confine program_hpx

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: <52ED30CB-08FB-44AD-B366-AA3263236FA5@oracle.com>
Date: Thu, 15 Jan 2026 15:39:21 +0000
From: Haakon Bugge <haakon.bugge@...cle.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: "Rafael J. Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Greg Kroah-Hartman <gregkh@...e.de>,
        Kenji Kaneshige <kaneshige.kenji@...fujitsu.com>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] PCI/ACPI: Confine program_hpx_type2 to the AER bits

Thanks for the review, Bjørn!

>> On Tue, Jan 13, 2026 at 06:15:20PM +0100, Håkon Bugge wrote:
>> program_hpx_type2() is today unconditionally called, despite the fact
>> that when the _HPX was added to the ACPI spec. v3.0, the description
>> stated:
>> 
>> +	/* We only do the HP programming if we own the PCIe native
>> +	 * hotplug and not the AER ownership
>> +	 */
>> 
>> Conventional multi-line comments are:
> 
>  /*
>   * We ...
>   */

The net convention lurked in ;-) 

>> +	if (!host->native_pcie_hotplug || host->native_aer)
>> +		return;
>> +
>> 	if (hpx->revision > 1) {
>> 		pci_warn(dev, "PCIe settings rev %d not supported\n",
>> 			 hpx->revision);
>> 		return;
>> 	}
>> 
>> -	/*
>> -	 * Don't allow _HPX to change MPS or MRRS settings.  We manage
>> -	 * those to make sure they're consistent with the rest of the
>> +	/* We only allow _HPX to program the AER registers, namely
>> +	 * PCI_EXP_DEVCTL_CERE, PCI_EXP_DEVCTL_NFERE,
>> +	 * PCI_EXP_DEVCTL_FERE, and PCI_EXP_DEVCTL_URRE.
>> +	 *
>> +	 * The other settings in PCIe DEVCTL are managed by OS in
>> +	 * order to make sure they're consistent with the rest of the
>> 	 * platform.
>> 	 */
>> -	hpx->pci_exp_devctl_and |= PCI_EXP_DEVCTL_PAYLOAD |
>> -				    PCI_EXP_DEVCTL_READRQ;
>> -	hpx->pci_exp_devctl_or &= ~(PCI_EXP_DEVCTL_PAYLOAD |
>> -				    PCI_EXP_DEVCTL_READRQ);
>> +	hpx->pci_exp_devctl_and |= PCI_EXP_DEVCTL_RELAX_EN   |
>> +				   PCI_EXP_DEVCTL_PAYLOAD    |
>> +				   PCI_EXP_DEVCTL_EXT_TAG    |
>> +				   PCI_EXP_DEVCTL_PHANTOM    |
>> +				   PCI_EXP_DEVCTL_AUX_PME    |
>> +				   PCI_EXP_DEVCTL_NOSNOOP_EN |
>> +				   PCI_EXP_DEVCTL_READRQ     |
>> +				   PCI_EXP_DEVCTL_BCR_FLR;
>> +	hpx->pci_exp_devctl_or &= ~(PCI_EXP_DEVCTL_RELAX_EN   |
>> +				    PCI_EXP_DEVCTL_PAYLOAD    |
>> +				    PCI_EXP_DEVCTL_EXT_TAG    |
>> +				    PCI_EXP_DEVCTL_PHANTOM    |
>> +				    PCI_EXP_DEVCTL_AUX_PME    |
>> +				    PCI_EXP_DEVCTL_NOSNOOP_EN |
>> +				    PCI_EXP_DEVCTL_READRQ     |
>> +				    PCI_EXP_DEVCTL_BCR_FLR);
>> 
> Instead of listing the bits we *don't* want to touch, I think we
> should explicitly *include* CERE, NFERE, FERE, URRE.  Maybe we should
> move the PCI_EXP_AER_FLAGS #define to drivers/pci/pci.h so we could
> use it directly, e.g.,
> 
>  hpx->pci_exp_devctl_and |= ~PCI_EXP_AER_FLAGS;
>  hpx->pci_exp_devctl_or &= PCI_EXP_AER_FLAGS;

Good idea. But what about moving it to include/uapi/linux/pci_regs.h and also rename it from PCI_EXP_AER_FLAGS to PCI_EXP_DEVCTL_AER, to match the convention for DEVCTL in pci_regs.h?


>> 	/* Initialize Device Control Register */
>> 	pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
>> 			~hpx->pci_exp_devctl_and, hpx->pci_exp_devctl_or);
>> 
>> -	/* Initialize Link Control Register */
>> +	/* Log the Link Control Register if any bits are set */
>> 	if (pcie_cap_has_lnkctl(dev)) {
>> +		u16 lnkctl;
>> 
>> -		/*
>> -		 * If the Root Port supports Read Completion Boundary of
>> -		 * 128, set RCB to 128.  Otherwise, clear it.
>> -		 */
>> -		hpx->pci_exp_lnkctl_and |= PCI_EXP_LNKCTL_RCB;
>> -		hpx->pci_exp_lnkctl_or &= ~PCI_EXP_LNKCTL_RCB;
>> -		if (pcie_root_rcb_set(dev))
>> -			hpx->pci_exp_lnkctl_or |= PCI_EXP_LNKCTL_RCB;
>> -
>> -		pcie_capability_clear_and_set_word(dev, PCI_EXP_LNKCTL,
>> -			~hpx->pci_exp_lnkctl_and, hpx->pci_exp_lnkctl_or);
>> +		pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnkctl);
>> +		if (lnkctl)
>> +			pci_warn(dev, "Some bits in PCIe Link Control are set: 0x%04x\n",
>> +				 lnkctl);
>> 
> Sorry, I wasn't clear about this.  I meant that we could log the
> LNKCTL AND/OR values from _HPX, not the values from PCI_EXP_LNKCTL
> itself.  There will definitely be bits set in PCI_EXP_LNKCTL in normal
> operation, which is perfectly fine.
> 
> But if pci_exp_lnkctl_and or pci_exp_lnkctl_or are non-zero, the
> platform is telling us to do something, and we're ignoring it.
> *That's* what I think we might want to know about.  pci_info() is
> probably sufficient; the user doesn't need to *do* anything with it, I
> just want it in case we need to debug an issue.

My bad, Yes, that makes more sense to me. And, you're OK with removing the RCB tweaking as well?


Thxs, Håkon