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

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Jan 15, 2026 at 03:39:21PM +0000, Haakon Bugge wrote:
> Thanks for the review, Bjørn!
> ...

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

I suggested drivers/pci/pci.h because (so far) the only need for
PCI_EXP_AER_FLAGS is in drivers/pci, and that set of flags seems like
an OS policy.  Most of pci_regs.h is basically translating the PCI
spec into #defines, without any real usage or policy parts.  I'm not
sure whether PCI_EXP_AER_FLAGS would be useful to userspace.

> >> 	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?

Good question.  My hope is that the code here is just to make sure
that we don't *clear* PCI_EXP_LNKCTL_RCB when we want it set but a
type 2 record might clear it by mistake.

We should audit PCI_EXP_LNKCTL_RCB usage to be sure that if we remove
this code, PCI_EXP_LNKCTL_RCB will still be set whenever it needs to
be set.  If we rely on the existence of an _HPX type 2 record for it
to be set, that would be completely wrong.

Bjorn