On Wed, Dec 15, 2021 at 09:55:59PM -0800, David E. Box wrote:
> From: Michael Bottini <michael.a.bottini@xxxxxxxxxxxxxxx>
>
> Devices that appear under the Intel VMD host bridge are not visible to BIOS
> and therefore not programmed by BIOS with ASPM settings. For these devices,
> it is necessary for the driver to configure ASPM.
The VMD-related parts of this commit log belong in the next patch,
because this patch has nothing in particular to do with VMD.
> Since ASPM settings are adjustable at runtime by module parameter,
> use the same mechanism to allow drivers to override the default (in
> this case never configured) BIOS policy to ASPM_STATE_ALL. Then,
> reconfigure ASPM on the link. Do not override if ASPM control is
> disabled.
The module parameter ("policy") has global effect: it runs
pcie_aspm_set_policy(), which assigns the global "aspm_policy" and
then reconfigures all links in the system.
This is not that; it's a link-based thing that doesn't change
"aspm_policy" and only affects a single link. This is more like
aspm_attr_store_common() for the sysfs "l0s_aspm" and similar
attributes, or the pci_disable_link_state() interface for drivers.
> Signed-off-by: Michael Bottini <michael.a.bottini@xxxxxxxxxxxxxxx>
> Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
> ---
> V4
> - No changes.
> V3
> - Fix missing semicolon in static inline function.
>
> V2
> - Change return type to int so caller can determine if override was
> successful.
> - Return immediately if link is not found so that lock it not
> unecessarily taken, suggested by kw@xxxxxxxxx.
> - Don't override if aspm_disabled is true.
>
> drivers/pci/pci.h | 2 ++
> drivers/pci/pcie/aspm.c | 19 +++++++++++++++++++
> 2 files changed, 21 insertions(+)
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 3d60cabde1a1..c9c55d43cd8a 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -562,11 +562,13 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev);
> void pcie_aspm_exit_link_state(struct pci_dev *pdev);
> void pcie_aspm_pm_state_change(struct pci_dev *pdev);
> void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
> +int pcie_aspm_policy_override(struct pci_dev *dev);
> #else
> static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
> static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { }
> static inline void pcie_aspm_pm_state_change(struct pci_dev *pdev) { }
> static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { }
> +static inline int pcie_aspm_policy_override(struct pci_dev *dev) { return -EINVAL; }
> #endif
>
> #ifdef CONFIG_PCIE_ECRC
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 52c74682601a..e2c61e14e724 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1140,6 +1140,25 @@ int pci_disable_link_state(struct pci_dev *pdev, int state)
> }
> EXPORT_SYMBOL(pci_disable_link_state);
>
> +int pcie_aspm_policy_override(struct pci_dev *pdev)
> +{
> + struct pcie_link_state *link = pcie_aspm_get_link(pdev);
> +
> + if (!link || aspm_disabled)
> + return -EINVAL;
> +
> + down_read(&pci_bus_sem);
> + mutex_lock(&aspm_lock);
> + link->aspm_default = ASPM_STATE_ALL;
> + pcie_config_aspm_link(link, policy_to_aspm_state(link));
> + pcie_set_clkpm(link, policy_to_clkpm_state(link));
> + mutex_unlock(&aspm_lock);
> + up_read(&pci_bus_sem);
This is essentially the inverse of pci_disable_link_state(). Why not
name it so the connection is obvious? Probably also make the
signature ("int state") similar.
> + return 0;
> +}
> +EXPORT_SYMBOL(pcie_aspm_policy_override);
> +
> static int pcie_aspm_set_policy(const char *val,
> const struct kernel_param *kp)
> {
> --
> 2.25.1
>
