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

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Apr 30, 2020 at 05:35:34PM -0700, Kuppuswamy, Sathyanarayanan wrote:
> On 4/30/2020 3:40 PM, Bjorn Helgaas wrote:
> > 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 pulled out the _OSC part of this to a separate patch.  What's left
> > is below, and is essentially equivalent to Alex's patch:
> > 
> >    https://lore.kernel.org/r/20190326172343.28946-3-mr.nuke.me@gmail.com/
> > 
> > I like what this does, but what I don't like is the fact that we now
> > have this thing called pcie_aer_get_firmware_first() that is not
> > connected with the ACPI FIRMWARE_FIRST bit at all.
>
> I agree. Since the current function has nothing to do with firmware
> first check, we can rename it. May be pci_aer_is_native() ?
>
> > I think the end result will be more readable if we get rid of the
> > "firmware_first" completely.  I don't know if we need a wrapper for it
> > at all, or if we should just open-code:
>
> Since pcie_aer_get_firmware_first() is used in following exported functions,
> I think we still need to check for "pcie_ports_native"
> and "pci_is_pcie()"

We *could* check pci_is_pcie(), but I don't think it's strictly
necessary because we really just depend on the AER capability, and we
already check for dev->aer_cap, which will only be set if we find one.

Good point about "pcie_ports_native".  I think I implemented
host->native_aer in a sub-optimal way: instead of sprinkling tests for
pcie_ports_native around, I think I should have done something like
this in acpi_pci_root_create():

  if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL) &&
      !pcie_ports_native)
        host_bridge->native_aer = 0;

That way (1) it's more obvious that the point of pcie_ports_native is
to override _OSC, and (2) we only need to check native_aer elsewhere.

If we refactored like that, the following should be sufficient:

> >    int pci_enable_pcie_error_reporting(struct pci_dev *dev)
> >    {
> >      struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
> > 
> >      if (!host->native_aer)
> >        return -EIO;