Re: [PATCH 1/2] PCI: ASPM: Allow OS to configure ASPM where BIOS is incapable of

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, May 30, 2024 at 04:52:26PM +0800, Kai-Heng Feng wrote:
> Since commit f492edb40b54 ("PCI: vmd: Add quirk to configure PCIe ASPM
> and LTR"), ASPM is configured for NVMe devices enabled in VMD domain.
> 
> However, that doesn't cover the case when FADT has ACPI_FADT_NO_ASPM
> set.
> 
> So add a new attribute to bypass aspm_disabled so OS can configure ASPM.
> 
> Fixes: f492edb40b54 ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR")
> Link: https://lore.kernel.org/linux-pm/218aa81f-9c6-5929-578d-8dc15f83dd48@panix.com/
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/pci/pcie/aspm.c | 8 ++++++--
>  include/linux/pci.h     | 1 +
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index cee2365e54b8..e719605857b1 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1416,8 +1416,12 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
>  	 * the _OSC method), we can't honor that request.
>  	 */
>  	if (aspm_disabled) {
> -		pci_warn(pdev, "can't override BIOS ASPM; OS doesn't have ASPM control\n");
> -		return -EPERM;
> +		if (aspm_support_enabled && pdev->aspm_os_control)
> +			pci_info(pdev, "BIOS can't program ASPM, let OS control it\n");
> +		else {
> +			pci_warn(pdev, "can't override BIOS ASPM; OS doesn't have ASPM control\n");
> +			return -EPERM;

1) I dislike having this VMD-specific special case in the generic
code.

2) I think the "BIOS can't program ASPM ..." message is a little bit
misleading.  We're making the assumption that BIOS doesn't know about
devices below the VMD bridge, but we really don't know that.  BIOS
*could* have a VMD driver, and it could configure ASPM below the VMD.
We're just assuming that it doesn't.

It's also a little bit too verbose -- I think we get this message for
*every* device below VMD?  Maybe the vmd driver could print something
about ignoring the ACPI FADT "PCIe ASPM Controls" bit once per VMD?
Then it's clearly connected to something firmware folks know about.

3) The code ends up looking like this:

  if (aspm_disabled) {
    if (aspm_support_enabled && pdev->aspm_os_control)
      pci_info(pdev, "BIOS can't program ASPM, let OS control it\n");
    else {
      pci_warn(pdev, "can't override BIOS ASPM; OS doesn't have ASPM control\n");
      return -EPERM;
    }
  }

and I think it's confusing to check "aspm_support_enabled" and
"pdev->aspm_os_control" after we've already decided that ASPM is
sort of disabled by "aspm_disabled".

Plus, we're left with questions about all the *other* tests of
"aspm_disabled" in pcie_aspm_sanity_check(),
pcie_aspm_pm_state_change(), pcie_aspm_powersave_config_link(),
__pci_disable_link_state(), etc.  Why do they *not* need this change?

And what about pcie_aspm_init_link_state()?  Why doesn't *it* pay
attention to "aspm_disabled"?  It's all very complicated.

This is similar in some ways to native_aer, native_pme, etc., which we
negotiate with _OSC.  I wonder if we could make something similar for
this, since it's another case where we want to make something specific
to a host bridge instead of global.

> +		}
>  	}
>  
>  	if (!locked)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index fb004fd4e889..58cbd4bea320 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -467,6 +467,7 @@ struct pci_dev {
>  	unsigned int	no_command_memory:1;	/* No PCI_COMMAND_MEMORY */
>  	unsigned int	rom_bar_overlap:1;	/* ROM BAR disable broken */
>  	unsigned int	rom_attr_enabled:1;	/* Display of ROM attribute enabled? */
> +	unsigned int	aspm_os_control:1;	/* Display of ROM attribute enabled? */

Comment is wrong (but I hope we can avoid a per-device bit anyway).

>  	pci_dev_flags_t dev_flags;
>  	atomic_t	enable_cnt;	/* pci_enable_device has been called */
>  
> -- 
> 2.43.0
>