| Message ID | 20180703163854.GA61685@bhelgaas-glaptop.roam.corp.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On 07/03/2018 11:38 AM, Bjorn Helgaas wrote: > On Mon, Jul 02, 2018 at 11:16:01AM -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. >> >> 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@gmail.com> >> --- >> drivers/pci/pcie/aer.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c >> index a2e88386af28..db2c01056dc7 100644 >> --- a/drivers/pci/pcie/aer.c >> +++ b/drivers/pci/pcie/aer.c >> @@ -283,13 +283,14 @@ static int aer_hest_parse(struct acpi_hest_header *hest_hdr, void *data) >> >> static void aer_set_firmware_first(struct pci_dev *pci_dev) >> { >> - int rc; >> + int rc = 0; >> struct aer_hest_parse_info info = { >> .pci_dev = pci_dev, >> .firmware_first = 0, >> }; >> >> - rc = apei_hest_parse(aer_hest_parse, &info); >> + if (!pcie_ports_native) >> + rc = apei_hest_parse(aer_hest_parse, &info); >> >> if (rc) >> pci_dev->__aer_firmware_first = 0; >> @@ -324,7 +325,9 @@ bool aer_acpi_firmware_first(void) >> }; >> >> if (!parsed) { >> - apei_hest_parse(aer_hest_parse, &info); >> + if (!pcie_ports_native) >> + apei_hest_parse(aer_hest_parse, &info); >> + >> aer_firmware_first = info.firmware_first; >> parsed = true; >> } > > I was thinking something along the lines of the patch below, so we > don't have to work through the settings of "rc" and "info". But maybe > I'm missing something subtle? > > One subtle thing that I didn't look into is the > pcie_aer_get_firmware_first() stub for the non-CONFIG_ACPI_APEI case. > > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > index b24f2d252180..5ccbd7635f33 100644 > --- a/drivers/pci/pcie/aer.c > +++ b/drivers/pci/pcie/aer.c > @@ -342,6 +342,9 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev) > if (!pci_is_pcie(dev)) > return 0; > > + if (pcie_ports_native) > + return 0; > + Here we're not setting dev->__aer_firmware_first_valid. Assuming we only check for it via pcie_aer_get_firmware_first(), we should be fine. THis field is used in portdrv.h, but its use seems safe. I think this approach can work. > if (!dev->__aer_firmware_first_valid) > aer_set_firmware_first(dev); > return dev->__aer_firmware_first; > @@ -362,6 +365,9 @@ bool aer_acpi_firmware_first(void) > .firmware_first = 0, > }; > > + if (pcie_ports_native) > + return false; > + This makes better sense. > if (!parsed) { > apei_hest_parse(aer_hest_parse, &info); > aer_firmware_first = info.firmware_first; >
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index b24f2d252180..5ccbd7635f33 100644 --- a/drivers/pci/pcie/aer.c +++ b/drivers/pci/pcie/aer.c @@ -342,6 +342,9 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev) if (!pci_is_pcie(dev)) return 0; + if (pcie_ports_native) + return 0; + if (!dev->__aer_firmware_first_valid) aer_set_firmware_first(dev); return dev->__aer_firmware_first; @@ -362,6 +365,9 @@ bool aer_acpi_firmware_first(void) .firmware_first = 0, }; + if (pcie_ports_native) + return false; + if (!parsed) { apei_hest_parse(aer_hest_parse, &info); aer_firmware_first = info.firmware_first;