| Message ID | 20211216055600.2425362-1-david.e.box@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [V4,1/2] PCI/ASPM: Add ASPM BIOS override function | expand |
On Wed, Dec 15, 2021 at 09:55:59PM -0800, David E. Box wrote: > From: Michael Bottini <michael.a.bottini@linux.intel.com> > > 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@linux.intel.com> > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > --- > 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@linux.com. > - 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 <michael.a.bottini@linux.intel.com> > > > > 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 <michael.a.bottini@linux.intel.com> > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > > --- > > 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@linux.com. > > - 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 > >
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) {