diff mbox series

[1/2] PCI/ASPM: Add ASPM BIOS override function

Message ID 20211120015756.1396263-1-david.e.box@linux.intel.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series [1/2] PCI/ASPM: Add ASPM BIOS override function | expand

Commit Message

David E. Box Nov. 20, 2021, 1:57 a.m. UTC 
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. 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.

Signed-off-by: Michael Bottini <michael.a.bottini@linux.intel.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/pci/pci.h       |  2 ++
 drivers/pci/pcie/aspm.c | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+)

Comments

Krzysztof Wilczyński Nov. 30, 2021, 8:03 a.m. UTC | #1 
Hi David,

[...]
> @@ -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);
> +void 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 void pcie_aspm_policy_override(struct pci_dev *dev) {}
>  #endif

A small nitpick, and a slight OCD on my part, so feel free to ignore this,
of course: a missing space between curly brackets, to keep things aligned
with previous definitions.

> @@ -1140,6 +1140,24 @@ int pci_disable_link_state(struct pci_dev *pdev, int state)
>  }
>  EXPORT_SYMBOL(pci_disable_link_state);
>  
> +void pcie_aspm_policy_override(struct pci_dev *pdev)
> +{
> +	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
> +
> +	down_read(&pci_bus_sem);
> +	mutex_lock(&aspm_lock);
> +
> +	if (link) {
> +		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);
> +}
> +EXPORT_SYMBOL(pcie_aspm_policy_override);

What about the following version where if we have no link (albeit, I am not
sure how often this is going to be the case?) then we don't even attempt to
get a hold on the lock and such:

	void pcie_aspm_policy_override(struct pci_dev *pdev)
	{
		struct pcie_link_state *link = pcie_aspm_get_link(pdev);
	
		if (!link)
			return;
	
		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);
	}

What do you think?  Would this make sense?

	Krzysztof
diff mbox series

Patch

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 3d60cabde1a1..172ec914e988 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);
+void 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 void pcie_aspm_policy_override(struct pci_dev *dev) {}
 #endif
 
 #ifdef CONFIG_PCIE_ECRC
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 52c74682601a..ccb98586bf0d 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1140,6 +1140,24 @@  int pci_disable_link_state(struct pci_dev *pdev, int state)
 }
 EXPORT_SYMBOL(pci_disable_link_state);
 
+void pcie_aspm_policy_override(struct pci_dev *pdev)
+{
+	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
+
+	down_read(&pci_bus_sem);
+	mutex_lock(&aspm_lock);
+
+	if (link) {
+		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);
+}
+EXPORT_SYMBOL(pcie_aspm_policy_override);
+
 static int pcie_aspm_set_policy(const char *val,
 				const struct kernel_param *kp)
 {