On Tue, Feb 20, 2024 at 06:45:32PM -0800, Kuppuswamy Sathyanarayanan wrote:
On 2/20/24 3:55 PM, Bjorn Helgaas wrote:
From: Bjorn Helgaas bhelgaas@google.com
When booting with "pci=noaer", we don't request control of AER, but we previously *did* request control of DPC, as in the dmesg log attached at the bugzilla below:
Command line: ... pci=noaer acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI EDR HPX-Type3] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug SHPCHotplug PME PCIeCapability LTR DPC]
That's illegal per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, which says:
If the operating system sets this bit [OSC_PCI_EXPRESS_DPC_CONTROL], it must also set bit 7 of the Support field (indicating support for Error Disconnect Recover notifications) and bits 3 and 4 of the Control field (requesting control of PCI Express Advanced Error Reporting and the PCI Express Capability Structure).
Request DPC control only if we have also requested AER control.
Can you also add similar check in calculate_support call?
if (pci_aer_available() && IS_ENABLED(CONFIG_PCIE_EDR)) support |= OSC_PCI_EDR_SUPPORT;
That doesn't seem right to me. The implementation note in sec 4.6.12 suggests that EDR Notifications may be used even when the firmware maintains control of AER and DPC. Maybe that note is wrong or misleading, but as written, I interpret that as meaning that it may be useful for the platform to know that the OS supports EDR even if it AER control isn't requested or granted.
Fixes: ac1c8e35a326 ("PCI/DPC: Add Error Disconnect Recover (EDR) support") Link: https://bugzilla.kernel.org/show_bug.cgi?id=218491#c12 Signed-off-by: Bjorn Helgaas bhelgaas@google.com Cc: stable@vger.kernel.org # v5.7+ Cc: Kuppuswamy Sathyanarayanan sathyanarayanan.kuppuswamy@linux.intel.com Cc: Matthew W Carlis mattc@purestorage.com Cc: Keith Busch kbusch@kernel.org Cc: Lukas Wunner lukas@wunner.de Cc: Mika Westerberg mika.westerberg@linux.intel.com Cc: Jesse Brandeburg jesse.brandeburg@intel.com
drivers/acpi/pci_root.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 58b89b8d950e..1c16965427b3 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -518,17 +518,19 @@ static u32 calculate_control(void) if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC)) control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
- if (pci_aer_available())
- if (pci_aer_available()) { control |= OSC_PCI_EXPRESS_AER_CONTROL;
- /*
* Per the Downstream Port Containment Related Enhancements ECN to* the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,* OSC_PCI_EXPRESS_DPC_CONTROL indicates the OS supports both DPC* and EDR.*/- if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
control |= OSC_PCI_EXPRESS_DPC_CONTROL;
/** Per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, the* OS can request DPC control only if it has advertised* OSC_PCI_EDR_SUPPORT and requested both* OSC_PCI_EXPRESS_CAPABILITY_CONTROL andI think you mean OSC_PCI_EXPRESS_DPC_CONTROL.
No, I just tried to rephrase the text for _OSC Control, bit 7 (quoted in the commit log), so I think requesting control of bits 3 and 4 (AER and PCIe Capability) is right.
* OSC_PCI_EXPRESS_AER_CONTROL.*/if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))control |= OSC_PCI_EXPRESS_DPC_CONTROL;Since you are cleaning up this part, why not add a patch to remove CONFIG_PCIE_EDR?
Good idea, I'll do that, too.
Bjorn