Re: Lost PCIe PME after a914ff2d78ce ("PCI/ASPM: Don't select CONFIG_PCIEASPM by default")

[Bjorn Helgaas <helgaas@kernel.org>]

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