| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJZ5v0jbc0xdwBkxa3uHmTmvpnBp+qcWre26-Q-Nd2UVa_=v5Q@mail.gmail.com>
Date: Fri, 22 Aug 2025 14:12:20 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: "David E. Box" <david.e.box@...ux.intel.com>
Cc: rafael@...nel.org, bhelgaas@...gle.com, vicamo.yang@...onical.com,
kenny@...ix.com, ilpo.jarvinen@...ux.intel.com, nirmal.patel@...ux.intel.com,
mani@...nel.org, linux-pm@...r.kernel.org, linux-pci@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2 1/2] PCI/ASPM: Add host-bridge API to override default
ASPM/CLKPM link state
On Fri, Aug 22, 2025 at 5:12 AM David E. Box
<david.e.box@...ux.intel.com> wrote:
>
> Synthetic PCIe hierarchies, such as those created by Intel VMD, are not
> enumerated by firmware and do not receive BIOS-provided ASPM or CLKPM
> defaults. Devices in such domains may therefore run without the intended
> power management.
>
> Add a host-bridge mechanism that lets controller drivers supply their own
> defaults. A new aspm_default_link_state field in struct pci_host_bridge is
> set via pci_host_set_default_pcie_link_state(). During link initialization,
> if this field is non-zero, ASPM and CLKPM defaults come from it instead of
> BIOS.
>
> This enables drivers like VMD to align link power management with platform
> expectations and avoids embedding controller-specific quirks in ASPM core
> logic.
>
> Link: https://patchwork.ozlabs.org/project/linux-pci/patch/20250720190140.2639200-1-david.e.box%40linux.intel.com/
> Signed-off-by: David E. Box <david.e.box@...ux.intel.com>
No issues found, so
Reviewed-by: Rafael J. Wysocki (Intel) <rafael@...nel.org>
There is a very minor nit below, but it has no bearing on the above.
> ---
>
> Changes in V1 from RFC:
>
> -- Rename field to aspm_dflt_link_state since it stores
> PCIE_LINK_STATE_XXX flags, not a policy enum.
> -- Move the field to struct pci_host_bridge since it's being applied to
> the entire host bridge per Mani's suggestion.
> -- During testing noticed that clkpm remained disabled and this was
> also handled by the formerly used pci_enable_link_state(). Add a
> check in pcie_clkpm_cap_init() as well to enable clkpm during init.
>
> Changes in V2:
>
> -- Host field name changed to aspm_default_link_state.
> -- Added get/set functions for aspm_default_link_state. Only the
> setter is exported. Added a kernel-doc describing usage and
> particulars around meaning of 0.
>
> drivers/pci/pcie/aspm.c | 42 +++++++++++++++++++++++++++++++++++++++--
> include/linux/pci.h | 9 +++++++++
> 2 files changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 919a05b97647..b4f0b4805a35 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -373,6 +373,39 @@ static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
> pcie_set_clkpm_nocheck(link, enable);
> }
>
> +/**
> + * pci_host_set_default_pcie_link_state - set controller-provided default ASPM/CLKPM mask
> + * @host: host bridge on which to apply the defaults
> + * @state: PCIE_LINK_STATE_XXX flags
> + *
> + * Allows a PCIe controller driver to specify the default ASPM and/or
> + * Clock Power Management (CLKPM) link state mask that will be used
> + * for links under this host bridge during ASPM/CLKPM capability init.
> + *
> + * The value is consumed in pcie_aspm_cap_init() and pcie_clkpm_cap_init()
> + * to override the firmware-discovered defaults.
> + *
> + * Interpretation of aspm_default_link_state:
> + * - Nonzero: bitmask of PCIE_LINK_STATE_* values to be used as defaults
> + * - Zero: no override provided; ASPM/CLKPM defaults fall back to
> + * values discovered in hardware/firmware
> + *
> + * Note: zero is always treated as "unset", not as "force ASPM/CLKPM off".
> + */
> +void pci_host_set_default_pcie_link_state(struct pci_host_bridge *host,
> + unsigned int state)
> +{
> + host->aspm_default_link_state = state;
> +}
> +EXPORT_SYMBOL_GPL(pci_host_set_default_pcie_link_state);
> +
> +static u32 pci_host_get_default_pcie_link_state(struct pci_dev *parent)
The parameter need not be called "parent".
> +{
> + struct pci_host_bridge *host = pci_find_host_bridge(parent->bus);
> +
> + return host ? host->aspm_default_link_state : 0;
> +}
> +
> static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
> {
> int capable = 1, enabled = 1;
> @@ -394,7 +427,10 @@ static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
> enabled = 0;
> }
> link->clkpm_enabled = enabled;
> - link->clkpm_default = enabled;
> + if (pci_host_get_default_pcie_link_state(link->pdev) & PCIE_LINK_STATE_CLKPM)
> + link->clkpm_default = 1;
> + else
> + link->clkpm_default = enabled;
> link->clkpm_capable = capable;
> link->clkpm_disable = blacklist ? 1 : 0;
> }
> @@ -866,7 +902,9 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
> }
>
> /* Save default state */
> - link->aspm_default = link->aspm_enabled;
> + link->aspm_default = pci_host_get_default_pcie_link_state(parent);
> + if (!link->aspm_default)
> + link->aspm_default = link->aspm_enabled;
>
> /* Setup initial capable state. Will be updated later */
> link->aspm_capable = link->aspm_support;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 59876de13860..8947cbaf9fa6 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -620,6 +620,10 @@ struct pci_host_bridge {
> unsigned int size_windows:1; /* Enable root bus sizing */
> unsigned int msi_domain:1; /* Bridge wants MSI domain */
>
> +#ifdef CONFIG_PCIEASPM
> + unsigned int aspm_default_link_state; /* Controller-provided default */
> +#endif
> +
> /* Resource alignment requirements */
> resource_size_t (*align_resource)(struct pci_dev *dev,
> const struct resource *res,
> @@ -1849,6 +1853,8 @@ int pci_disable_link_state(struct pci_dev *pdev, int state);
> int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
> int pci_enable_link_state(struct pci_dev *pdev, int state);
> int pci_enable_link_state_locked(struct pci_dev *pdev, int state);
> +void pci_host_set_default_pcie_link_state(struct pci_host_bridge *host,
> + unsigned int state);
> void pcie_no_aspm(void);
> bool pcie_aspm_support_enabled(void);
> bool pcie_aspm_enabled(struct pci_dev *pdev);
> @@ -1861,6 +1867,9 @@ static inline int pci_enable_link_state(struct pci_dev *pdev, int state)
> { return 0; }
> static inline int pci_enable_link_state_locked(struct pci_dev *pdev, int state)
> { return 0; }
> +static inline void
> +pci_host_set_default_pcie_link_state(struct pci_host_bridge *host,
> + unsigned int state) { }
> static inline void pcie_no_aspm(void) { }
> static inline bool pcie_aspm_support_enabled(void) { return false; }
> static inline bool pcie_aspm_enabled(struct pci_dev *pdev) { return false; }
>
> base-commit: c17b750b3ad9f45f2b6f7e6f7f4679844244f0b9
> --
> 2.43.0
>
Powered by blists - more mailing lists