Sorry I didn't see this before my comments yesterday For either individual or split set, Reviewed-by: Jon Derrick <jonathan.derrick@xxxxxxxxx> Thanks Kuppuswamy On Mon, 2020-04-27 at 19:02 -0500, Bjorn Helgaas wrote: > [+cc Jon, Alexandru] > > On Sun, Apr 26, 2020 at 11:30:06AM -0700, sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx wrote: > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> > > > > Currently PCIe AER driver uses HEST FIRMWARE_FIRST bit to > > determine the PCIe AER Capability ownership between OS and > > firmware. This support is added based on following spec > > reference. > > > > Per ACPI spec r6.3, table 18-387, 18-388, 18-389, HEST table > > flags field BIT-0 and BIT-1 can be used to expose the > > ownership of error source between firmware and OSPM. > > > > Bit [0] - FIRMWARE_FIRST: If set, indicates that system > > firmware will handle errors from this source > > first. > > Bit [1] – GLOBAL: If set, indicates that the settings > > contained in this structure apply globally to all > > PCI Express Bridges. > > > > Although above spec reference states that setting > > FIRMWARE_FIRST bit means firmware will handle the error source > > first, it does not explicitly state anything about AER > > ownership. So using HEST to determine AER ownership is > > incorrect. > > > > Also, as per following specification references, _OSC can be > > used to negotiate the AER ownership between firmware and OS. > > Details are, > > > > Per ACPI spec r6.3, sec 6.2.11.3, table titled “Interpretation > > of _OSC Control Field” and as per PCI firmware specification r3.2, > > sec 4.5.1, table 4-5, OS can set bit 3 of _OSC control field > > to request control over PCI Express AER. If the OS successfully > > receives control of this feature, it must handle error reporting > > through the AER Capability as described in the PCI Express Base > > Specification. > > > > Since above spec references clearly states _OSC can be used to > > determine AER ownership, don't depend on HEST FIRMWARE_FIRST bit. > > I split this up a bit and applied the first part to pci/error to get > it into -next so we can start seeing what breaks. I won't be too > surprised if we trip over something. > > Here's the first part (entire original patch is at > https://lore.kernel.org/r/67af2931705bed9a588b5a39d369cb70b9942190.1587925636.git.sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx). > > commit 8f8e42e7c2dd ("PCI/AER: Use only _OSC to determine AER ownership") > Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> > Date: Mon Apr 27 18:25:13 2020 -0500 > > PCI/AER: Use only _OSC to determine AER ownership > > Per the PCI Firmware spec, r3.2, sec 4.5.1, the OS can request control of > AER via bit 3 of the _OSC Control Field. In the returned value of the > Control Field: > > The firmware sets [bit 3] to 1 to grant control over PCI Express Advanced > Error Reporting. ... after control is transferred to the operating > system, firmware must not modify the Advanced Error Reporting Capability. > If control of this feature was requested and denied or was not requested, > firmware returns this bit set to 0. > > Previously the pci_root driver looked at the HEST FIRMWARE_FIRST bit to > determine whether to request ownership of the AER Capability. This was > based on ACPI spec v6.3, sec 18.3.2.4, and similar sections, which say > things like: > > Bit [0] - FIRMWARE_FIRST: If set, indicates that system firmware will > handle errors from this source first. > > Bit [1] - GLOBAL: If set, indicates that the settings contained in this > structure apply globally to all PCI Express Devices. > > These ACPI references don't say anything about ownership of the AER > Capability. > > Remove use of the FIRMWARE_FIRST bit and rely only on the _OSC bit to > determine whether we have control of the AER Capability. > > Link: https://lore.kernel.org/r/67af2931705bed9a588b5a39d369cb70b9942190.1587925636.git.sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx > [bhelgaas: commit log, split patches] > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> > Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index ac8ad6cb82aa..9e235c1a75ff 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -483,13 +483,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm, > if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC)) > control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL; > > - if (pci_aer_available()) { > - if (aer_acpi_firmware_first()) > - dev_info(&device->dev, > - "PCIe AER handled by firmware\n"); > - else > - control |= OSC_PCI_EXPRESS_AER_CONTROL; > - } > + if (pci_aer_available()) > + control |= OSC_PCI_EXPRESS_AER_CONTROL; > > /* > * Per the Downstream Port Containment Related Enhancements ECN to > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > index f4274d301235..efc26773cc6d 100644 > --- a/drivers/pci/pcie/aer.c > +++ b/drivers/pci/pcie/aer.c > @@ -318,30 +318,6 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev) > aer_set_firmware_first(dev); > return dev->__aer_firmware_first; > } > - > -static bool aer_firmware_first; > - > -/** > - * aer_acpi_firmware_first - Check if APEI should control AER. > - */ > -bool aer_acpi_firmware_first(void) > -{ > - static bool parsed = false; > - struct aer_hest_parse_info info = { > - .pci_dev = NULL, /* Check all PCIe devices */ > - .firmware_first = 0, > - }; > - > - if (pcie_ports_native) > - return false; > - > - if (!parsed) { > - apei_hest_parse(aer_hest_parse, &info); > - aer_firmware_first = info.firmware_first; > - parsed = true; > - } > - return aer_firmware_first; > -} > #endif > > #define PCI_EXP_AER_FLAGS (PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \ > @@ -1523,7 +1499,7 @@ static struct pcie_port_service_driver aerdriver = { > */ > int __init pcie_aer_init(void) > { > - if (!pci_aer_available() || aer_acpi_firmware_first()) > + if (!pci_aer_available()) > return -ENXIO; > return pcie_port_service_register(&aerdriver); > } > diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h > index 2d155bfb8fbf..11c98875538a 100644 > --- a/include/linux/pci-acpi.h > +++ b/include/linux/pci-acpi.h > @@ -125,10 +125,4 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { } > static inline void acpi_pci_remove_bus(struct pci_bus *bus) { } > #endif /* CONFIG_ACPI */ > > -#ifdef CONFIG_ACPI_APEI > -extern bool aer_acpi_firmware_first(void); > -#else > -static inline bool aer_acpi_firmware_first(void) { return false; } > -#endif > - > #endif /* _PCI_ACPI_H_ */
