Re: [PATCH] PCI/ACPI: Always advertise ASPM support if CONFIG_PCIEASPM=y

From: Bjorn Helgaas <helgaas@kernel.org>
To: Yicong Yang <yangyicong@hisilicon.com>
Cc: bhelgaas@google.com, rafael@kernel.org,
	linux-pci@vger.kernel.org, linux-acpi@vger.kernel.org,
	lenb@kernel.org, linux-kernel@vger.kernel.org,
	linuxarm@huawei.com
Subject: Re: [PATCH] PCI/ACPI: Always advertise ASPM support if CONFIG_PCIEASPM=y
Date: Tue, 3 May 2022 17:38:57 -0500	[thread overview]
Message-ID: <20220503223857.GA414278@bhelgaas> (raw)
In-Reply-To: <20220425070634.28227-1-yangyicong@hisilicon.com>

On Mon, Apr 25, 2022 at 03:06:34PM +0800, Yicong Yang wrote:
> When we have CONFIG_PCIEASPM enabled it means OS can always support ASPM no
> matter user have disabled it through pcie_aspm=off or not. But currently we
> won't advertise ASPM support in _OSC negotiation if user disables it, which
> doesn't match the fact. This will also have side effects that other PCIe
> services like AER and hotplug will be disabled as ASPM support is required
> and we won't negotiate other services if ASPM support is absent.
> 
> So this patch makes OS always advertising ASPM support if CONFIG_PCIEASPM=y.
> It intends no functional change to pcie_aspm=off as it will still mark
> aspm_disabled=1 and aspm_support_enabled=false, driver will check these
> status before configuring ASPM.
>
> Tested this patch with pcie_aspm=off:
> estuary:/$ dmesg | egrep -i "aspm|osc"
> [    0.000000] PCIe ASPM is disabled
> [    8.706961] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM
> ClockPM Segments MSI EDR HPX-Type3]
> [    8.726032] acpi PNP0A08:00: _OSC: platform does not support [LTR]
> [    8.742818] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME
> AER PCIeCapability DPC]
> estuary:/sys/module/pcie_aspm/parameters$ cat policy
> [default] performance powersave powersupersave
> estuary:/sys/module/pcie_aspm/parameters$ echo powersave > policy
> bash: echo: write error: Operation not permitted
> 
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> [https://lore.kernel.org/linux-pci/20220407154257.GA235990@bhelgaas/]
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/acpi/pci_root.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 6f9e75d14808..17e78582e633 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -393,7 +393,7 @@ static u32 calculate_support(void)
>  	support |= OSC_PCI_HPX_TYPE_3_SUPPORT;
>  	if (pci_ext_cfg_avail())
>  		support |= OSC_PCI_EXT_CONFIG_SUPPORT;
> -	if (pcie_aspm_support_enabled())
> +	if (IS_ENABLED(CONFIG_PCIEASPM))

Is there any way firmware could tell the difference between
"CONFIG_PCIEASPM not set" and "CONFIG_PCIEASPM=y and booted with
'pcie_aspm=off'"?

If not, why would we even check whether CONFIG_PCIEASPM is set?

>  		support |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
>  	if (pci_msi_enabled())
>  		support |= OSC_PCI_MSI_SUPPORT;
> -- 
> 2.24.0
> 