diff mbox series

[RFC,v1,2/4] PCI: Add support for drivers to decide bridge D3 policy

Message ID 20231009225653.36030-3-mario.limonciello@amd.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series Add support for drivers to decide bridge D3 policy | expand

Commit Message

Mario Limonciello Oct. 9, 2023, 10:56 p.m. UTC
commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
changed pci_bridge_d3_possible() so that any vendor's PCIe ports
from modern machines (>=2015) are allowed to be put into D3.

This policy change has worked for most machines, but the behavior
is improved with `pcie_port_pm=off` on some others.

Add support for drivers to register a callback that they can optionally
use to decide the policy on supported machines.

A priority is used so that if multiple drivers register a callback and
support deciding for a device the highest priority driver will return
the value.

Suggested-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pci/pci.c   | 119 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h |  54 ++++++++++++++++++++
 2 files changed, 173 insertions(+)

Comments

Lukas Wunner Oct. 14, 2023, 10:53 a.m. UTC | #1
On Mon, Oct 09, 2023 at 05:56:51PM -0500, Mario Limonciello wrote:
> commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> changed pci_bridge_d3_possible() so that any vendor's PCIe ports
> from modern machines (>=2015) are allowed to be put into D3.
> 
> This policy change has worked for most machines, but the behavior
> is improved with `pcie_port_pm=off` on some others.
> 
> Add support for drivers to register a callback that they can optionally
> use to decide the policy on supported machines.

I would assume that drivers can decide the policy already today through
pci_d3cold_enable() / pci_d3cold_disable().

Why is this not sufficient and what's the benefit of the more complex
approach proposed here?

Thanks,

Lukas
Mario Limonciello Oct. 15, 2023, 6:55 p.m. UTC | #2
On 10/14/2023 05:53, Lukas Wunner wrote:
> On Mon, Oct 09, 2023 at 05:56:51PM -0500, Mario Limonciello wrote:
>> commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
>> changed pci_bridge_d3_possible() so that any vendor's PCIe ports
>> from modern machines (>=2015) are allowed to be put into D3.
>>
>> This policy change has worked for most machines, but the behavior
>> is improved with `pcie_port_pm=off` on some others.
>>
>> Add support for drivers to register a callback that they can optionally
>> use to decide the policy on supported machines.
> 
> I would assume that drivers can decide the policy already today through
> pci_d3cold_enable() / pci_d3cold_disable().
> 
> Why is this not sufficient and what's the benefit of the more complex
> approach proposed here?
> 

This approach allows multiple drivers to participate.  Realistically 
however I don't know "many" drivers will participate in the first place.

 From our earlier conversations on the quirk thread I think your 
proposal will work as well for this, I'll try it.  Thanks.
diff mbox series

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 59c01d68c6d5..3b8e7b936908 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -21,6 +21,7 @@ 
 #include <linux/module.h>
 #include <linux/spinlock.h>
 #include <linux/string.h>
+#include <linux/list_sort.h>
 #include <linux/log2.h>
 #include <linux/logic_pio.h>
 #include <linux/pm_wakeup.h>
@@ -54,6 +55,8 @@  unsigned int pci_pm_d3hot_delay;
 static void pci_pme_list_scan(struct work_struct *work);
 
 static LIST_HEAD(pci_pme_list);
+static LIST_HEAD(d3_possible_list);
+static DEFINE_MUTEX(d3_possible_list_mutex);
 static DEFINE_MUTEX(pci_pme_list_mutex);
 static DECLARE_DELAYED_WORK(pci_pme_work, pci_pme_list_scan);
 
@@ -3031,6 +3034,16 @@  bool pci_bridge_d3_possible(struct pci_dev *bridge)
 		if (pci_bridge_d3_force)
 			return true;
 
+		/* check for any preferences for the bridge */
+		switch (bridge->driver_d3) {
+		case BRIDGE_PREF_DRIVER_D3:
+			return true;
+		case BRIDGE_PREF_DRIVER_D0:
+			return false;
+		default:
+			break;
+		}
+
 		/* Even the oldest 2010 Thunderbolt controller supports D3. */
 		if (bridge->is_thunderbolt)
 			return true;
@@ -3168,6 +3181,112 @@  void pci_d3cold_disable(struct pci_dev *dev)
 }
 EXPORT_SYMBOL_GPL(pci_d3cold_disable);
 
+static struct pci_dev *pci_get_upstream_port(struct pci_dev *pci_dev)
+{
+	struct pci_dev *bridge;
+
+	bridge = pci_upstream_bridge(pci_dev);
+	if (!bridge)
+		return NULL;
+
+	if (!pci_is_pcie(bridge))
+		return NULL;
+
+	switch (pci_pcie_type(bridge)) {
+	case PCI_EXP_TYPE_ROOT_PORT:
+	case PCI_EXP_TYPE_UPSTREAM:
+	case PCI_EXP_TYPE_DOWNSTREAM:
+		return bridge;
+	default:
+		break;
+	};
+
+	return NULL;
+}
+
+static void pci_refresh_driver_d3(void)
+{
+	struct pci_d3_driver_ops *driver;
+	struct pci_dev *pci_dev = NULL;
+	struct pci_dev *bridge;
+
+	/* 1st pass: unset any preferences set a previous invocation */
+	while ((pci_dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, pci_dev))) {
+		bridge = pci_get_upstream_port(pci_dev);
+		if (!bridge)
+			continue;
+
+		if (bridge->driver_d3 != BRIDGE_PREF_UNSET)
+			bridge->driver_d3 = BRIDGE_PREF_UNSET;
+	}
+
+	pci_dev = NULL;
+
+	/* 2nd pass: update the preference and refresh bridge_d3 */
+	while ((pci_dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, pci_dev))) {
+		bridge = pci_get_upstream_port(pci_dev);
+		if (!bridge)
+			continue;
+
+		/* don't make multiple passes on the same device */
+		if (bridge->driver_d3 != BRIDGE_PREF_UNSET)
+			continue;
+
+		/* the list is pre-sorted by highest priority */
+		mutex_lock(&d3_possible_list_mutex);
+		list_for_each_entry(driver, &d3_possible_list, list_node) {
+			/* another higher priority driver already set preference */
+			if (bridge->driver_d3 != BRIDGE_PREF_UNSET)
+				break;
+			if (!driver->check)
+				continue;
+			bridge->driver_d3 = driver->check(bridge);
+		}
+		mutex_unlock(&d3_possible_list_mutex);
+
+		/* no driver set a preference */
+		if (bridge->driver_d3 == BRIDGE_PREF_UNSET)
+			bridge->driver_d3 = BRIDGE_PREF_NONE;
+
+		/* update bridge_d3 */
+		pci_bridge_d3_update(pci_dev);
+	}
+}
+
+static int pci_d3_driver_cmp(void *priv, const struct list_head *_a,
+			   const struct list_head *_b)
+{
+	struct pci_d3_driver_ops *a = container_of(_a, typeof(*a), list_node);
+	struct pci_d3_driver_ops *b = container_of(_b, typeof(*b), list_node);
+
+	if (a->priority < b->priority)
+		return -1;
+	else if (a->priority > b->priority)
+		return 1;
+	return 0;
+}
+
+int pci_register_driver_d3_policy_cb(struct pci_d3_driver_ops *arg)
+{
+	mutex_lock(&d3_possible_list_mutex);
+	list_add(&arg->list_node, &d3_possible_list);
+	list_sort(NULL, &d3_possible_list, pci_d3_driver_cmp);
+	mutex_unlock(&d3_possible_list_mutex);
+	pci_refresh_driver_d3();
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_register_driver_d3_policy_cb);
+
+void pci_unregister_driver_d3_policy_cb(struct pci_d3_driver_ops *arg)
+{
+	mutex_lock(&d3_possible_list_mutex);
+	list_del(&arg->list_node);
+	list_sort(NULL, &d3_possible_list, pci_d3_driver_cmp);
+	mutex_unlock(&d3_possible_list_mutex);
+	pci_refresh_driver_d3();
+}
+EXPORT_SYMBOL_GPL(pci_unregister_driver_d3_policy_cb);
+
 /**
  * pci_pm_init - Initialize PM functions of given PCI device
  * @dev: PCI device to handle.
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8c7c2c3c6c65..c36903a4e351 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -389,6 +389,7 @@  struct pci_dev {
 						   bit manually */
 	unsigned int	d3hot_delay;	/* D3hot->D0 transition time in ms */
 	unsigned int	d3cold_delay;	/* D3cold->D0 transition time in ms */
+	unsigned int	driver_d3;	/* Driver D3 request preference */
 
 #ifdef CONFIG_PCIEASPM
 	struct pcie_link_state	*link_state;	/* ASPM link state */
@@ -1405,6 +1406,59 @@  bool pcie_relaxed_ordering_enabled(struct pci_dev *dev);
 void pci_resume_bus(struct pci_bus *bus);
 void pci_bus_set_current_state(struct pci_bus *bus, pci_power_t state);
 
+/**
+ * enum bridge_d3_pref - D3 preference of a bridge
+ * @BRIDGE_PREF_UNSET: Not configured by driver
+ * @BRIDGE_PREF_NONE: Driver does not care
+ * @BRIDGE_PREF_DRIVER_D3: Driver prefers D3
+ * @BRIDGE_PREF_DRIVER_D0: Driver prefers D0
+ */
+enum bridge_d3_pref {
+	BRIDGE_PREF_UNSET,
+	BRIDGE_PREF_NONE,
+	BRIDGE_PREF_DRIVER_D3,
+	BRIDGE_PREF_DRIVER_D0,
+};
+
+/**
+ * pci_d3_driver_ops - Operations provided by a driver to evaluate D3 policy
+ * @list_node: the linked list node
+ * @priority: the priority of the callback
+ * @check: the callback to evaluate D3 policy
+ *
+ * For the callback drivers are expected to return:
+ *  BRIDGE_PREF_NONE if they can't evaluate the decision whether to support D3
+ *  BRIDGE_PREF_DRIVER_D0 if they decide the device should not support D3
+ *  BRIDGE_PREF_DRIVER_D3 if they decide the device should support D3
+ */
+struct pci_d3_driver_ops {
+	struct list_head list_node;
+	int priority;
+	enum bridge_d3_pref (*check)(struct pci_dev *pdev);
+};
+
+/**
+ * pci_register_driver_d3_policy_cb - Register a driver callback for configuring D3 policy
+ * @arg: driver provided structure with function pointer and priority
+ *
+ * This function can be used by drivers to register a callback that can be used
+ * to delegate the decision of whether a device should be used for D3 to that
+ * driver.
+ *
+ * Returns 0 on success, error code on failure.
+ */
+int pci_register_driver_d3_policy_cb(struct pci_d3_driver_ops *arg);
+
+/**
+ * pci_unregister_driver_d3_policy_cb - Unregister a driver callback for configuring D3 policy
+ * @arg: driver provided structure with function pointer and priority
+ *
+ * This function can be used by drivers to unregister a callback that can be used
+ * to delegate the decision of whether a device should be used for D3 to that
+ * driver.
+ */
+void pci_unregister_driver_d3_policy_cb(struct pci_d3_driver_ops *arg);
+
 /* For use by arch with custom probe code */
 void set_pcie_port_type(struct pci_dev *pdev);
 void set_pcie_hotplug_bridge(struct pci_dev *pdev);