> On Thu, Jan 22, 2026 at 02:09:54PM +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:
>>
>> OSPM [1] will only evaluate _HPX with Setting Record – Type 2 if OSPM
>> is not controlling the PCI Express Advanced Error Reporting
>> capability.
>>
>> Hence, we only call program_hpx_type2() when the OSPM owns the PCIe
>> hotplug capability but not the AER.
>>
>> The Advanced Configuration and Power Interface (ACPI) Specification
>> version 6.6 has a provision that gives the OSPM the ability to control
>> the other PCIe Device Control bits any way. In a note in section
>> 6.2.9, it is stated:
>>
>> "OSPM may override the settings provided by the _HPX object's Type2
>> record (PCI Express Settings) or Type3 record (PCI Express Descriptor
>> Settings) when OSPM has assumed native control of the corresponding
>> feature."
>>
>> So, in order to preserve the non-AER bits in PCIe Device Control, in
>> particular the performance sensitive ExtTag and RO, we make sure
>> program_hpx_type2() if called, doesn't modify any non-AER bits.
>>
>> Also, when program_hpx_type2() is called, we completely avoid
>> modifying any bits in the Link Control register. However, if the _HPX
>> type 2 records contains bits indicating such modifications, we print
>> an info message.
>>
>> [1] Operating System-directed configuration and Power Management
>
> I looked at the specs again and pulled out some more details. Here's
> what seemed relevant to me:
>
> PCI/ACPI: Restrict program_hpx_type2() to AER bits
>
> Previously program_hpx_type2() applied PCIe settings unconditionally, which
> could incorrectly change bits like Extended Tag Field Enable and Enable
> Relaxed Ordering.
>
> When _HPX was added to ACPI r3.0, the intent of the PCIe Setting Record
> (Type 2) in sec 6.2.7.3 was to configure AER registers when the OS does not
> own the AER Capability:
>
> The PCI Express setting record contains ... [the AER] Uncorrectable Error
> Mask, Uncorrectable Error Severity, Correctable Error Mask ... to be used
> when configuring registers in the Advanced Error Reporting Extended
> Capability Structure ...
>
> OSPM will only evaluate _HPX with Setting Record – Type 2 if OSPM is not
> controlling the PCI Express Advanced Error Reporting capability.
>
> ACPI r3.0b, sec 6.2.7.3, added more AER registers, including registers in
> the PCIe Capability with AER-related bits, and the restriction that the OS
> use this only when it owns PCIe native hotplug:
>
> ... when configuring PCI Express registers in the Advanced Error
> Reporting Extended Capability Structure *or PCI Express Capability
> Structure* ...
>
> An OS that has assumed ownership of native hot plug but does not ... have
> ownership of the AER register set must use ... the Type 2 record to
> program the AER registers ...
>
> However, since the Type 2 record also includes register bits that have
> functions other than AER, the OS must ignore values ... that are not
> applicable.
>
> Restrict program_hpx_type2() to only the intended purpose:
>
> - Apply settings only when OS owns PCIe native hotplug but not AER,
>
> - Only touch the AER-related bits (Error Reporting Enables) in Device
> Control
>
> - Don't touch Link Control at all, since nothing there seems AER-related,
> but log _HPX settings for debugging purposes
>
> Note that Read Completion Boundary is now configured elsewhere, since it is
> unrelated to _HPX.
Thanks for the word-smithing and improved accuracy!
>> + /* Log if _HPX attempts to modify PCIe Link Control register */
>> if (pcie_cap_has_lnkctl(dev)) {
>> -
>> - /*
>> - * 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);
This was what confused me a lot, the bit-wise NOT above. That must be wrong, as
pcie_capability_clear_and_set_word() inverts the "clear" argument.
>> + if (hpx->pci_exp_lnkctl_and)
>> + pci_info(dev,
>> + "_HPX attempts to reset the following bits in
>> PCIe Link Control: 0x%04x\n",
>> + hpx->pci_exp_lnkctl_and);
>> + if (hpx->pci_exp_lnkctl_or)
>> + pci_info(dev,
>> + "_HPX attempts to set the following bits in
>> PCIe Link Control: 0x%04x\n",
>> + hpx->pci_exp_lnkctl_or);
>
> Again I'm afraid I suggested some over-engineering, and even worse, I
> misinterpreted the pci_exp_lnkctl_and and pci_exp_lnkctl_or usage when
> I said "if pci_exp_lnkctl_and or pci_exp_lnkctl_or are *non-zero*".
>
> Per spec, we are supposed to AND the register with pci_exp_lnkctl_and,
> so the interesting value would be anything other than 0xffff. Since
> we OR it with pci_exp_lnkctl_or, the interesting values there would be
> anything non-zero. So I think what we would want is something like
> this:
>
> + /* Log if _HPX attempts to modify PCIe Link Control register */
> if (pcie_cap_has_lnkctl(dev)) {
> -
> - /*
> - * 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);
> + if (hpx->pci_exp_lnkctl_and != 0xffff ||
> + hpx->pci_exp_lnkctl_or != 0)
> + pci_info(dev, "_HPX attempts Link Control setting (AND
> %#06x OR %#06x)\n",
> + hpx->pci_exp_lnkctl_and,
> + hpx->pci_exp_lnkctl_or);
> }
Since we do not want to fix incorrect firmware in this respect, the above makes
perfect sense to me. A v4 is on its way.
Thxs, Håkon
