[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200529202135.GA461617@bjorn-Precision-5520>
Date: Fri, 29 May 2020 15:21:35 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Heiner Kallweit <hkallweit1@...il.com>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>,
Bjorn Helgaas <bhelgaas@...gle.com>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
Matthew Garrett <mjg59@...f.ucam.org>
Subject: Re: Lost PCIe PME after a914ff2d78ce ("PCI/ASPM: Don't select
CONFIG_PCIEASPM by default")
[+cc Matthew]
On Fri, May 29, 2020 at 10:09:08PM +0200, Heiner Kallweit wrote:
> On 29.05.2020 21:40, Heiner Kallweit wrote:
> > On 29.05.2020 21:21, Bjorn Helgaas wrote:
> >> On Fri, May 29, 2020 at 08:50:46PM +0200, Heiner Kallweit wrote:
> >>> On 28.05.2020 23:44, Heiner Kallweit wrote:
> >>>> For whatever reason with this change (and losing ASPM control) I also
> >>>> loose the PCIe PME interrupts. This prevents my network card from
> >>>> resuming from runtime-suspend.
> >>>> Reverting the change brings back ASPM control and the PCIe PME irq's.
> >>>>
> >>>> Affected system is a Zotac MiniPC with a N3450 CPU:
> >>>> PCI bridge: Intel Corporation Celeron N3350/Pentium N4200/Atom E3900 Series PCI Express Port A #1 (rev fb)
> >>>>
> >>> I checked a little bit further and w/o ASPM control the root ports
> >>> don't have the PME service bit set in their capabilities.
> >>> Not sure whether this is a chipset bug or whether there's a better
> >>> explanation. However more chipsets may have such a behavior.
> >>
> >> Hmm. Is the difference simply changing the PCIEASPM config symbol, or
> >> are you booting with command-line arguments like "pcie_aspm=off"?
> >>
> > Only difference is the config symbol. My command line is plain and simple:
> >
> > Command line: initrd=\intel-ucode.img initrd=\initramfs-linux.img root=/dev/sda2 rw
> >
> >> What's the specific PME bit that changes in the root ports? Can you
> >> collect the "sudo lspci -vvxxxx" output with and without ASPM?
> >>
> >> The capability bits are generally read-only as far as the PCI spec is
> >> concerned, but devices have implementation-specific knobs that the
> >> BIOS may use to change things. Without CONFIG_PCIEASPM, Linux will
> >> not request control of LTR, and that could cause the BIOS to change
> >> something. You should be able to see the LTR control difference in
> >> the dmesg logging about _OSC.
> >>
> >>> W/o the "default y" for ASPM control we also have the situation now
> >>> that the config option description says "When in doubt, say Y."
> >>> but it takes the EXPERT mode to enable it. This seems to be a little
> >>> bit inconsistent.
> >>
> >> We should probably remove the "if EXPERT" from the PCIEASPM kconfig.
> >> But I would expect PME to work correctly regardless of PCIEASPM, so
> >> removing "if EXPERT" doesn't solve the underlying problem.
> >>
> >> Rafael, does this ring any bells for you? I don't remember a
> >> connection between PME and ASPM, but maybe there is one.
> >>
> >>> To cut a long story short:
> >>> At least on some systems this change has unwanted side effects.
> >
> > lspci output w/ and w/o ASPM is attached incl. a diff.
> > Here comes the _OSC difference.
> >
> > w/o ASPM
> >
> > [ 0.386063] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig Segments MSI HPX-Type3]
> > [ 0.386918] acpi PNP0A08:00: _OSC: not requesting OS control; OS requires [ExtendedConfig ASPM ClockPM MSI]
> >
> > w/ ASPM
> > [ 0.388141] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI HPX-Type3]
> > [ 0.393648] acpi PNP0A08:00: _OSC: OS now controls [PME AER PCIeCapability LTR]
> >
> > It's at least interesting that w/o ASPM OS doesn't control PME and AER.
> >
>
> This was the right entry point, also w/o ASPM control OS states to ACPI that it
> needs ASPM and ClockPM. The following patch fixes the PME issue for me.
> See also the _OSC part below.
>
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 9e235c1a7..8df1fa728 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -38,10 +38,15 @@ static int acpi_pci_root_scan_dependent(struct acpi_device *adev)
> return 0;
> }
>
> +#ifdef CONFIG_PCIEASPM
> #define ACPI_PCIE_REQ_SUPPORT (OSC_PCI_EXT_CONFIG_SUPPORT \
> | OSC_PCI_ASPM_SUPPORT \
> | OSC_PCI_CLOCK_PM_SUPPORT \
> | OSC_PCI_MSI_SUPPORT)
> +#else
> +#define ACPI_PCIE_REQ_SUPPORT (OSC_PCI_EXT_CONFIG_SUPPORT \
> + | OSC_PCI_MSI_SUPPORT)
> +#endif
Yeah, that makes sense. I can't remember the details, but I'm pretty
sure there's a reason why we ask for the whole set of things. Seems
like it solved some problem. I think Matthew Garrett might have been
involved in that.
Powered by blists - more mailing lists