Re: [PATCH v2] PCI/AER: Fix aerdrv loading with "pcie_ports=native" parameter

[Alex G]

On 06/30/2018 04:31 PM, Bjorn Helgaas wrote:
> [+cc Borislav, linux-acpi, since this involves APEI/HEST]

Borislav is not the relevant maintainer here, since we're not contingent 
on APEI handling. I think Keith has a lot more experience with this part 
of the kernel.

> On Tue, Jun 19, 2018 at 02:58:20PM -0500, Alexandru Gagniuc wrote:
> > According to the documentation, "pcie_ports=native", linux should use
> > native AER and DPC services. While that is true for the _OSC method
> > parsing, this is not the only place that is checked. Should the HEST
> > table list PCIe ports as firmware-first, linux will not use native
> > services.
>
> Nothing in ACPI-land looks at pcie_ports_native.  How should ACPI
> things work in the "pcie_ports=native" case?  I guess we still have to
> expect to receive error records from the firmware, because it may
> certainly send us non-PCI errors (machine checks, etc) and maybe even
> some PCI errors (even if the Linux AER driver claims AER interrupts,
> we don't know what notification mechanisms the firmware may be using).

I think ACPI land shouldn't care about this. We care about it from the 
PCIe stand point at the interface with ACPI. FW might see a delta in the 
sense that we request control of some features via _OSC, which we 
otherwise would not do without pcie_ports=native.

> I guess best-case, we'll get ACPI error records for all non-PCI
> things, and the Linux AER driver will see all the AER errors.

It might affect FW's ability to catch errors, but that's dependent on 
the root port implementation.

> Worst-case, I don't really know what to expect.  Duplicate reporting
> of AER errors via firmware and Linux AER driver?  Some kind of
> confusion about who acknowledges and clears them?

Once user enters pcie_ports=native, all bets are off: you broke the 
contract you have with the FW -- whether or not you have this patch.

> Out of curiosity, what is your use case for "pcie_ports=native"?
> Presumably there's something that works better when using it, and
> things work even *better* with this patch?

Corectness. It bothers me that actual behavior does not match the 
documentation:

 native  Use native PCIe services associated with PCIe ports
                        unconditionally.


> I know people do use it, because I often see it mentioned in forums
> and bug reports, but I really don't expect it to work very well
> because we're ignoring the usage model the firmware is designed
> around.  My unproven suspicion is that most uses are in the black
> magic category of "there's a bug here, and we don't know how to fix
> it, but pcie_ports=native makes it work better".

There exist cases that firmware didn't consider. I would not call them 
"firmware bugs", but there are cases where the user understands the 
platform better than firmware.
Example: on certain PCIe switches, a hardware PCIe error may bring the 
switch downstream ports into a state where they stop notifying hotplug 
events. Depending on the platform, firmware may or may not fix this 
condition, but "pcie_ports=native" enables DPC. DPC contains the error 
without the switch downstream port entering the weird error state in the 
first place.

All bets are off at this point.

> Obviously I would much rather find and fix those bugs so people
> wouldn't have to stumble over the problem in the first place.

Using native services when firmware asks us not to is a crapshoot every 
time. I don't condone the use of this feature, but if we do get a 
pcie_ports=native request, we should at least honor it.


> > This happens because aer_acpi_firmware_first() doesn't take
> > 'pcie_ports' into account. This is wrong. DPC uses the same logic when
> > it decides whether to load or not, so fixing this also fixes DPC not
> > loading.
> >
> > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@xxxxxxxxx>
> > ---
> >   drivers/pci/pcie/aer.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > Changes since v1:
> >   - Re-tested with latest and greatest (v4.18-rc1) -- works great
> >
> > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> > index a2e88386af28..98ced0f7c850 100644
> > --- a/drivers/pci/pcie/aer.c
> > +++ b/drivers/pci/pcie/aer.c
> > @@ -291,7 +291,7 @@ static void aer_set_firmware_first(struct pci_dev *pci_dev)
> >   
> >   	rc = apei_hest_parse(aer_hest_parse, &info);
> >   
> > -	if (rc)
> > +	if (rc || pcie_ports_native)
> >   		pci_dev->__aer_firmware_first = 0;
> >   	else
> >   		pci_dev->__aer_firmware_first = info.firmware_first;
> > @@ -327,6 +327,9 @@ bool aer_acpi_firmware_first(void)
> >   		apei_hest_parse(aer_hest_parse, &info);
> >   		aer_firmware_first = info.firmware_first;
> >   		parsed = true;
> > +		if (pcie_ports_native)
> > +			aer_firmware_first = 0;
> > +
> >   	}
> >   	return aer_firmware_first;
> >   }
> > --
> > 2.14.3