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

[Johannes Thumshirn <morbidrsa@gmail.com>]

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@oracle.com]
>
> 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.