From: Michael Bottini <[email protected]>
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. 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.
Signed-off-by: Michael Bottini <[email protected]>
Signed-off-by: David E. Box <[email protected]>
---
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 [email protected].
- 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);
+
+ 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
On Wed, Dec 15, 2021 at 09:55:59PM -0800, David E. Box wrote:
> From: Michael Bottini <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: David E. Box <[email protected]>
> ---
> 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 [email protected].
> - 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
>
On Thu, 2021-12-16 at 11:06 -0600, Bjorn Helgaas wrote:
> On Wed, Dec 15, 2021 at 09:55:59PM -0800, David E. Box wrote:
> > From: Michael Bottini <[email protected]>
> >
> > 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.
Okay, but shouldn't something be said here as to why it's needed? Maybe a more general description?
>
> > 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.
Yes. I'll clean up the wording.
>
> > Signed-off-by: Michael Bottini <[email protected]>
> > Signed-off-by: David E. Box <[email protected]>
> > ---
> > 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 [email protected].
> > - 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.
Indeed it is the inverse. Will implement as pci_enable_link_state().
Thanks.
>
> > + 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
> >