Re: [PATCH] PCI/ACPI: Confine program_hpx_type2 to the AER bits

[Bjorn Helgaas <helgaas@kernel.org>]

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 ...
   */

> +	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;

>  	/* 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.

Bjorn