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@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.
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@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.
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
> >
