[+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.
