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

[Johannes Thumshirn]

On 1/16/26 10:11 PM, Bjorn Helgaas wrote:
> [+cc Johannes (author of e42010d8207f ("PCI: Set Read Completion
> Boundary to 128 iff Root Port supports it (_HPX)"), Myron; start of
> thread:
> https://lore.kernel.org/r/20260113171522.3446407-1-haakon.bugge@xxxxxxxxxx]
>
> On Fri, Jan 16, 2026 at 10:10:43AM +0000, Haakon Bugge wrote:
> > > On Thu, Jan 15, 2026 at 03:39:21PM +0000, Haakon Bugge wrote:
> > > > Thanks for the review, Bjørn!
> > > > ...
> I should have mentioned this earlier, but I think the commit log
> should include something about the problem this change fixes.  I
> assume that the current code changes ExtTag and/or RO, and that causes
> something bad.  That's what is motivating this change.
>
> > > > > > 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.
> > Commit e42010d8207f ("PCI: Set Read Completion Boundary to 128 iff
> > Root Port supports it (_HPX)") fixes the "opposite" case, where _HPX
> > sets the RCB even though the RC does not support it. That commit
> > removes any RCB setting from the type 2 record from the equation,
> > and sets RCB if the RC has the bit set. And to me, that seems to be
> > the correct behaviour.
> Thanks for digging into that.  You're right that it looks like
> e42010d8207f ("PCI: Set Read Completion Boundary to 128 iff Root Port
> supports it (_HPX)") was motivated by a machine with a Root Port with
> PCI_EXP_LNKCTL_RCB cleared, but an _HPX record telling us to set
> PCI_EXP_LNKCTL_RCB.

IIRC (this is nearly 10 years old) that's been the case. But back then 
it clearly was a bios issue, but we decided to fix it in the kernel if 
my memory serves me well.