Re: [PATCH v1 1/1] PCI/AER: Use _OSC negotiation to determine AER ownership

["Derrick, Jonathan" <jonathan.derrick@intel.com>]

Sorry I didn't see this before my comments yesterday

For either individual or split set,
Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>

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@linux.intel.com wrote:
> > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > 
> > 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@linux.intel.com).
> 
> commit 8f8e42e7c2dd ("PCI/AER: Use only _OSC to determine AER ownership")
> Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 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@linux.intel.com
>     [bhelgaas: commit log, split patches]
>     Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> 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_ */