All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler
@ 2022-03-14  5:03 Mario Limonciello
  2022-03-14  5:03 ` [PATCH v3 2/5] ACPI / x86: Pass the constraints checking result to LPS0 callback Mario Limonciello
                   ` (5 more replies)
  0 siblings, 6 replies; 17+ messages in thread
From: Mario Limonciello @ 2022-03-14  5:03 UTC (permalink / raw)
  To: Hans de Goede, Mark Gross, Rafael J . Wysocki
  Cc: open list:X86 PLATFORM DRIVERS, linux-acpi, Shyam Sundar S K,
	Goswami Sanket, Mario Limonciello

Currenty the latest thing run during a suspend to idle attempt is
the LPS0 `prepare_late` callback and the earliest thing is the
`resume_early` callback.

There is a desire for the `amd-pmc` driver to suspend later in the
suspend process (ideally the very last thing), so create a callback
that it or any other driver can hook into to do this.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
changes from v2->v3:
 * Check that callbacks exist before calling
changes from v1->v2:
 * Change register/unregister arguments to be struct

 drivers/acpi/x86/s2idle.c | 68 ++++++++++++++++++++++++++++++++++++++-
 include/linux/acpi.h      | 11 ++++++-
 2 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index abc06e7f89d8..69008c4a86ea 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -86,6 +86,9 @@ struct lpi_device_constraint_amd {
 	int min_dstate;
 };
 
+static LIST_HEAD(lps0_callback_handler_head);
+static DEFINE_MUTEX(lps0_callback_handler_mutex);
+
 static struct lpi_constraints *lpi_constraints_table;
 static int lpi_constraints_table_size;
 static int rev_id;
@@ -444,6 +447,9 @@ static struct acpi_scan_handler lps0_handler = {
 
 int acpi_s2idle_prepare_late(void)
 {
+	struct lps0_callback_handler *handler;
+	int rc = 0;
+
 	if (!lps0_device_handle || sleep_no_lps0)
 		return 0;
 
@@ -474,14 +480,34 @@ int acpi_s2idle_prepare_late(void)
 		acpi_sleep_run_lps0_dsm(ACPI_LPS0_MS_ENTRY,
 				lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
 	}
-	return 0;
+
+	mutex_lock(&lps0_callback_handler_mutex);
+	list_for_each_entry(handler, &lps0_callback_handler_head, list_node) {
+		if (handler->prepare_late_callback) {
+			rc = handler->prepare_late_callback(handler->context);
+			if (rc)
+				goto out;
+		}
+	}
+out:
+	mutex_unlock(&lps0_callback_handler_mutex);
+
+	return rc;
 }
 
 void acpi_s2idle_restore_early(void)
 {
+	struct lps0_callback_handler *handler;
+
 	if (!lps0_device_handle || sleep_no_lps0)
 		return;
 
+	mutex_lock(&lps0_callback_handler_mutex);
+	list_for_each_entry(handler, &lps0_callback_handler_head, list_node)
+		if (handler->restore_early_callback)
+			handler->restore_early_callback(handler->context);
+	mutex_unlock(&lps0_callback_handler_mutex);
+
 	/* Modern standby exit */
 	if (lps0_dsm_func_mask_microsoft > 0)
 		acpi_sleep_run_lps0_dsm(ACPI_LPS0_MS_EXIT,
@@ -524,4 +550,44 @@ void acpi_s2idle_setup(void)
 	s2idle_set_ops(&acpi_s2idle_ops_lps0);
 }
 
+int acpi_register_lps0_callbacks(struct lps0_callback_handler *arg)
+{
+	struct lps0_callback_handler *handler;
+
+	if (!lps0_device_handle || sleep_no_lps0)
+		return -ENODEV;
+
+	handler = kmalloc(sizeof(*handler), GFP_KERNEL);
+	if (!handler)
+		return -ENOMEM;
+	handler->prepare_late_callback = arg->prepare_late_callback;
+	handler->restore_early_callback = arg->restore_early_callback;
+	handler->context = arg->context;
+
+	mutex_lock(&lps0_callback_handler_mutex);
+	list_add(&handler->list_node, &lps0_callback_handler_head);
+	mutex_unlock(&lps0_callback_handler_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_register_lps0_callbacks);
+
+void acpi_unregister_lps0_callbacks(struct lps0_callback_handler *arg)
+{
+	struct lps0_callback_handler *handler;
+
+	mutex_lock(&lps0_callback_handler_mutex);
+	list_for_each_entry(handler, &lps0_callback_handler_head, list_node) {
+		if (handler->prepare_late_callback == arg->prepare_late_callback &&
+		    handler->restore_early_callback == arg->restore_early_callback &&
+		    handler->context == arg->context) {
+			list_del(&handler->list_node);
+			kfree(handler);
+			break;
+		}
+	}
+	mutex_unlock(&lps0_callback_handler_mutex);
+}
+EXPORT_SYMBOL_GPL(acpi_unregister_lps0_callbacks);
+
 #endif /* CONFIG_SUSPEND */
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 6274758648e3..df105f5e03e5 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1023,7 +1023,16 @@ void acpi_os_set_prepare_extended_sleep(int (*func)(u8 sleep_state,
 
 acpi_status acpi_os_prepare_extended_sleep(u8 sleep_state,
 					   u32 val_a, u32 val_b);
-
+#ifdef CONFIG_X86
+struct lps0_callback_handler {
+	struct list_head list_node;
+	int (*prepare_late_callback)(void *context);
+	void (*restore_early_callback)(void *context);
+	void *context;
+};
+int acpi_register_lps0_callbacks(struct lps0_callback_handler *arg);
+void acpi_unregister_lps0_callbacks(struct lps0_callback_handler *arg);
+#endif /* CONFIG_X86 */
 #ifndef CONFIG_IA64
 void arch_reserve_mem_area(acpi_physical_address addr, size_t size);
 #else
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v3 2/5] ACPI / x86: Pass the constraints checking result to LPS0 callback
  2022-03-14  5:03 [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler Mario Limonciello
@ 2022-03-14  5:03 ` Mario Limonciello
  2022-03-14  5:03 ` [PATCH v3 3/5] ACPI / x86: Check LPI constraints by default Mario Limonciello
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 17+ messages in thread
From: Mario Limonciello @ 2022-03-14  5:03 UTC (permalink / raw)
  To: Hans de Goede, Mark Gross, Rafael J . Wysocki
  Cc: open list:X86 PLATFORM DRIVERS, linux-acpi, Shyam Sundar S K,
	Goswami Sanket, Mario Limonciello

If constraints checking has been enabled by the LPS0 code, it may
also be useful for drivers using the callback to make a decision what
to do.

For example this may in the future allow a failing constraints check
preventing another driver from notifying firmware that all required
devices have entered the deepest state.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
Changes from v2->v3:
 * rebase on top of changes in first patch
Changes from v1->v2:
 * rebase on top of changes in first patch

 drivers/acpi/x86/s2idle.c | 11 +++++++----
 include/linux/acpi.h      |  2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index 69008c4a86ea..646faa117c70 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -290,7 +290,7 @@ static void lpi_device_get_constraints(void)
 	ACPI_FREE(out_obj);
 }
 
-static void lpi_check_constraints(void)
+static void lpi_check_constraints(bool *met)
 {
 	int i;
 
@@ -312,11 +312,13 @@ static void lpi_check_constraints(void)
 			continue;
 		}
 
-		if (adev->power.state < lpi_constraints_table[i].min_dstate)
+		if (adev->power.state < lpi_constraints_table[i].min_dstate) {
 			acpi_handle_info(handle,
 				"LPI: Constraint not met; min power state:%s current power state:%s\n",
 				acpi_power_state_string(lpi_constraints_table[i].min_dstate),
 				acpi_power_state_string(adev->power.state));
+			*met = false;
+		}
 	}
 }
 
@@ -448,13 +450,14 @@ static struct acpi_scan_handler lps0_handler = {
 int acpi_s2idle_prepare_late(void)
 {
 	struct lps0_callback_handler *handler;
+	bool constraints_met = true;
 	int rc = 0;
 
 	if (!lps0_device_handle || sleep_no_lps0)
 		return 0;
 
 	if (pm_debug_messages_on)
-		lpi_check_constraints();
+		lpi_check_constraints(&constraints_met);
 
 	/* Screen off */
 	if (lps0_dsm_func_mask > 0)
@@ -484,7 +487,7 @@ int acpi_s2idle_prepare_late(void)
 	mutex_lock(&lps0_callback_handler_mutex);
 	list_for_each_entry(handler, &lps0_callback_handler_head, list_node) {
 		if (handler->prepare_late_callback) {
-			rc = handler->prepare_late_callback(handler->context);
+			rc = handler->prepare_late_callback(handler->context, constraints_met);
 			if (rc)
 				goto out;
 		}
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index df105f5e03e5..4906db854554 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1026,7 +1026,7 @@ acpi_status acpi_os_prepare_extended_sleep(u8 sleep_state,
 #ifdef CONFIG_X86
 struct lps0_callback_handler {
 	struct list_head list_node;
-	int (*prepare_late_callback)(void *context);
+	int (*prepare_late_callback)(void *context, bool constraints);
 	void (*restore_early_callback)(void *context);
 	void *context;
 };
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v3 3/5] ACPI / x86: Check LPI constraints by default
  2022-03-14  5:03 [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler Mario Limonciello
  2022-03-14  5:03 ` [PATCH v3 2/5] ACPI / x86: Pass the constraints checking result to LPS0 callback Mario Limonciello
@ 2022-03-14  5:03 ` Mario Limonciello
  2022-03-14  5:03 ` [PATCH v3 4/5] platform/x86: amd-pmc: Move to later in the suspend process Mario Limonciello
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 17+ messages in thread
From: Mario Limonciello @ 2022-03-14  5:03 UTC (permalink / raw)
  To: Hans de Goede, Mark Gross, Rafael J . Wysocki
  Cc: open list:X86 PLATFORM DRIVERS, linux-acpi, Shyam Sundar S K,
	Goswami Sanket, Mario Limonciello, David E . Box

Currently LPI constraints are only checked when `pm_debug_messages` has
been set. They are mostly used as a debugging tactic.

As the results of constraint checking will be passed to consumers of
the LPS0 callback, ensure that constraints are checked by default so this
value can be trusted.

Suggested-by: David E. Box <david.e.box@linux.intel.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
changes from v2->v3:
 * no changes
changes from v1->v2:
 * New patch
 drivers/acpi/x86/s2idle.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index 646faa117c70..2a15e61406a3 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -307,16 +307,19 @@ static void lpi_check_constraints(bool *met)
 			acpi_power_state_string(adev->power.state));
 
 		if (!adev->flags.power_manageable) {
-			acpi_handle_info(handle, "LPI: Device not power manageable\n");
+			if (pm_debug_messages_on)
+				acpi_handle_info(handle, "LPI: Device not power manageable\n");
 			lpi_constraints_table[i].handle = NULL;
 			continue;
 		}
 
 		if (adev->power.state < lpi_constraints_table[i].min_dstate) {
-			acpi_handle_info(handle,
-				"LPI: Constraint not met; min power state:%s current power state:%s\n",
-				acpi_power_state_string(lpi_constraints_table[i].min_dstate),
-				acpi_power_state_string(adev->power.state));
+			if (pm_debug_messages_on) {
+				acpi_handle_info(handle,
+					"LPI: Constraint not met; min power state:%s current power state:%s\n",
+					acpi_power_state_string(lpi_constraints_table[i].min_dstate),
+					acpi_power_state_string(adev->power.state));
+			}
 			*met = false;
 		}
 	}
@@ -456,8 +459,7 @@ int acpi_s2idle_prepare_late(void)
 	if (!lps0_device_handle || sleep_no_lps0)
 		return 0;
 
-	if (pm_debug_messages_on)
-		lpi_check_constraints(&constraints_met);
+	lpi_check_constraints(&constraints_met);
 
 	/* Screen off */
 	if (lps0_dsm_func_mask > 0)
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v3 4/5] platform/x86: amd-pmc: Move to later in the suspend process
  2022-03-14  5:03 [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler Mario Limonciello
  2022-03-14  5:03 ` [PATCH v3 2/5] ACPI / x86: Pass the constraints checking result to LPS0 callback Mario Limonciello
  2022-03-14  5:03 ` [PATCH v3 3/5] ACPI / x86: Check LPI constraints by default Mario Limonciello
@ 2022-03-14  5:03 ` Mario Limonciello
  2022-03-14  5:03 ` [PATCH v3 5/5] platform/x86: amd-pmc: Drop CPU QoS workaround Mario Limonciello
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 17+ messages in thread
From: Mario Limonciello @ 2022-03-14  5:03 UTC (permalink / raw)
  To: Hans de Goede, Mark Gross, Rafael J . Wysocki
  Cc: open list:X86 PLATFORM DRIVERS, linux-acpi, Shyam Sundar S K,
	Goswami Sanket, Mario Limonciello

The `OS_HINT` message is supposed to indicate that everything else
that is supposed to go into the deepest state has done so.

This assumption is invalid as:
1) The CPUs will still go in and out of the deepest state
2) Other devices may still run their `noirq` suspend routines
3) The LPS0 ACPI device will still run

To more closely mirror how this works on other operating systems,
move the `amd-pmc` suspend to the very last thing before the s2idle
loop via an lps0 callback.

Fixes: 8d89835b0467 ("PM: suspend: Do not pause cpuidle in the suspend-to-idle path")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
changes from v2->v3:
 * no changes
changes from v1->v2:
 * adjust for changes in previous patches
 * display a debugging message for constraints

 drivers/platform/x86/amd-pmc.c | 42 +++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index 971aaabaa9c8..752a99d759d2 100644
--- a/drivers/platform/x86/amd-pmc.c
+++ b/drivers/platform/x86/amd-pmc.c
@@ -639,13 +639,16 @@ static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
 	return rc;
 }
 
-static int __maybe_unused amd_pmc_suspend(struct device *dev)
+static int amd_pmc_suspend(void *context, bool constraints)
 {
-	struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
+	struct amd_pmc_dev *pdev = dev_get_drvdata((struct device *)context);
 	int rc;
 	u8 msg;
 	u32 arg = 1;
 
+	/* for enabling constraints checking in the future */
+	dev_dbg(pdev->dev, "LPI constraints were%smet.\n", constraints ? " " : " not ");
+
 	/* Reset and Start SMU logging - to monitor the s0i3 stats */
 	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_RESET, 0);
 	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_START, 0);
@@ -658,7 +661,7 @@ static int __maybe_unused amd_pmc_suspend(struct device *dev)
 	}
 
 	/* Dump the IdleMask before we send hint to SMU */
-	amd_pmc_idlemask_read(pdev, dev, NULL);
+	amd_pmc_idlemask_read(pdev, pdev->dev, NULL);
 	msg = amd_pmc_get_os_hint(pdev);
 	rc = amd_pmc_send_cmd(pdev, arg, NULL, msg, 0);
 	if (rc) {
@@ -681,28 +684,28 @@ static int __maybe_unused amd_pmc_suspend(struct device *dev)
 	return rc;
 }
 
-static int __maybe_unused amd_pmc_resume(struct device *dev)
+static void amd_pmc_resume(void *context)
 {
-	struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
+	struct amd_pmc_dev *pdev = dev_get_drvdata((struct device *)context);
 	int rc;
 	u8 msg;
 
 	msg = amd_pmc_get_os_hint(pdev);
 	rc = amd_pmc_send_cmd(pdev, 0, NULL, msg, 0);
 	if (rc)
-		dev_err(pdev->dev, "resume failed\n");
+		dev_err(pdev->dev, "resume failed: %d\n", rc);
 
 	/* Let SMU know that we are looking for stats */
 	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_DUMP_DATA, 0);
 
 	/* Dump the IdleMask to see the blockers */
-	amd_pmc_idlemask_read(pdev, dev, NULL);
+	amd_pmc_idlemask_read(pdev, pdev->dev, NULL);
 
 	/* Write data incremented by 1 to distinguish in stb_read */
 	if (enable_stb)
 		rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_PREDEF + 1);
 	if (rc)
-		dev_err(pdev->dev, "error writing to STB\n");
+		dev_err(pdev->dev, "error writing to STB: %d\n", rc);
 
 	/* Restore the QoS request back to defaults if it was set */
 	if (pdev->cpu_id == AMD_CPU_ID_CZN)
@@ -711,15 +714,8 @@ static int __maybe_unused amd_pmc_resume(struct device *dev)
 
 	/* Notify on failed entry */
 	amd_pmc_validate_deepest(pdev);
-
-	return rc;
 }
 
-static const struct dev_pm_ops amd_pmc_pm_ops = {
-	.suspend_noirq = amd_pmc_suspend,
-	.resume_noirq = amd_pmc_resume,
-};
-
 static const struct pci_device_id pmc_pci_ids[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_CPU_ID_YC) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_CPU_ID_CZN) },
@@ -805,6 +801,11 @@ static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf)
 static int amd_pmc_probe(struct platform_device *pdev)
 {
 	struct amd_pmc_dev *dev = &pmc;
+	struct lps0_callback_handler lps0_handler = {
+		.prepare_late_callback = amd_pmc_suspend,
+		.restore_early_callback = amd_pmc_resume,
+		.context = &pdev->dev,
+	};
 	struct pci_dev *rdev;
 	u32 base_addr_lo, base_addr_hi;
 	u64 base_addr, fch_phys_addr;
@@ -884,6 +885,10 @@ static int amd_pmc_probe(struct platform_device *pdev)
 
 	amd_pmc_get_smu_version(dev);
 	platform_set_drvdata(pdev, dev);
+	err = acpi_register_lps0_callbacks(&lps0_handler);
+	if (err)
+		goto err_pci_dev_put;
+
 	amd_pmc_dbgfs_register(dev);
 	cpu_latency_qos_add_request(&dev->amd_pmc_pm_qos_req, PM_QOS_DEFAULT_VALUE);
 	return 0;
@@ -896,7 +901,13 @@ static int amd_pmc_probe(struct platform_device *pdev)
 static int amd_pmc_remove(struct platform_device *pdev)
 {
 	struct amd_pmc_dev *dev = platform_get_drvdata(pdev);
+	struct lps0_callback_handler lps0_handler = {
+		.prepare_late_callback = amd_pmc_suspend,
+		.restore_early_callback = amd_pmc_resume,
+		.context = &pdev->dev,
+	};
 
+	acpi_unregister_lps0_callbacks(&lps0_handler);
 	amd_pmc_dbgfs_unregister(dev);
 	pci_dev_put(dev->rdev);
 	mutex_destroy(&dev->lock);
@@ -917,7 +928,6 @@ static struct platform_driver amd_pmc_driver = {
 	.driver = {
 		.name = "amd_pmc",
 		.acpi_match_table = amd_pmc_acpi_ids,
-		.pm = &amd_pmc_pm_ops,
 	},
 	.probe = amd_pmc_probe,
 	.remove = amd_pmc_remove,
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v3 5/5] platform/x86: amd-pmc: Drop CPU QoS workaround
  2022-03-14  5:03 [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler Mario Limonciello
                   ` (2 preceding siblings ...)
  2022-03-14  5:03 ` [PATCH v3 4/5] platform/x86: amd-pmc: Move to later in the suspend process Mario Limonciello
@ 2022-03-14  5:03 ` Mario Limonciello
  2022-03-14  9:05 ` [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler Hans de Goede
  2022-03-14  9:12 ` Lukas Wunner
  5 siblings, 0 replies; 17+ messages in thread
From: Mario Limonciello @ 2022-03-14  5:03 UTC (permalink / raw)
  To: Hans de Goede, Mark Gross, Rafael J . Wysocki
  Cc: open list:X86 PLATFORM DRIVERS, linux-acpi, Shyam Sundar S K,
	Goswami Sanket, Mario Limonciello

A workaround was previously introduced as part of commit 646f429ec2de
("platform/x86: amd-pmc: Set QOS during suspend on CZN w/ timer wakeup")
to prevent CPUs from going into the deepest state during the execution
of the `noirq` stage of `amd_pmc`.  As `amd_pmc` has been pushed to the
last step for suspend on AMD platforms, this workaround is no longer
necessary.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
changes from v2->v3:
 * No changes
changes from v1->v2:
 * No changes

 drivers/platform/x86/amd-pmc.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index 752a99d759d2..b3c6b5cc83c1 100644
--- a/drivers/platform/x86/amd-pmc.c
+++ b/drivers/platform/x86/amd-pmc.c
@@ -21,7 +21,6 @@
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
-#include <linux/pm_qos.h>
 #include <linux/rtc.h>
 #include <linux/suspend.h>
 #include <linux/seq_file.h>
@@ -96,9 +95,6 @@
 #define PMC_MSG_DELAY_MIN_US		50
 #define RESPONSE_REGISTER_LOOP_MAX	20000
 
-/* QoS request for letting CPUs in idle states, but not the deepest */
-#define AMD_PMC_MAX_IDLE_STATE_LATENCY	3
-
 #define SOC_SUBSYSTEM_IP_MAX	12
 #define DELAY_MIN_US		2000
 #define DELAY_MAX_US		3000
@@ -153,7 +149,6 @@ struct amd_pmc_dev {
 	struct device *dev;
 	struct pci_dev *rdev;
 	struct mutex lock; /* generic mutex lock */
-	struct pm_qos_request amd_pmc_pm_qos_req;
 #if IS_ENABLED(CONFIG_DEBUG_FS)
 	struct dentry *dbgfs_dir;
 #endif /* CONFIG_DEBUG_FS */
@@ -628,14 +623,6 @@ static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
 	rc = rtc_alarm_irq_enable(rtc_device, 0);
 	dev_dbg(pdev->dev, "wakeup timer programmed for %lld seconds\n", duration);
 
-	/*
-	 * Prevent CPUs from getting into deep idle states while sending OS_HINT
-	 * which is otherwise generally safe to send when at least one of the CPUs
-	 * is not in deep idle states.
-	 */
-	cpu_latency_qos_update_request(&pdev->amd_pmc_pm_qos_req, AMD_PMC_MAX_IDLE_STATE_LATENCY);
-	wake_up_all_idle_cpus();
-
 	return rc;
 }
 
@@ -678,9 +665,6 @@ static int amd_pmc_suspend(void *context, bool constraints)
 
 	return 0;
 fail:
-	if (pdev->cpu_id == AMD_CPU_ID_CZN)
-		cpu_latency_qos_update_request(&pdev->amd_pmc_pm_qos_req,
-						PM_QOS_DEFAULT_VALUE);
 	return rc;
 }
 
@@ -707,11 +691,6 @@ static void amd_pmc_resume(void *context)
 	if (rc)
 		dev_err(pdev->dev, "error writing to STB: %d\n", rc);
 
-	/* Restore the QoS request back to defaults if it was set */
-	if (pdev->cpu_id == AMD_CPU_ID_CZN)
-		cpu_latency_qos_update_request(&pdev->amd_pmc_pm_qos_req,
-						PM_QOS_DEFAULT_VALUE);
-
 	/* Notify on failed entry */
 	amd_pmc_validate_deepest(pdev);
 }
@@ -890,7 +869,6 @@ static int amd_pmc_probe(struct platform_device *pdev)
 		goto err_pci_dev_put;
 
 	amd_pmc_dbgfs_register(dev);
-	cpu_latency_qos_add_request(&dev->amd_pmc_pm_qos_req, PM_QOS_DEFAULT_VALUE);
 	return 0;
 
 err_pci_dev_put:
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler
  2022-03-14  5:03 [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler Mario Limonciello
                   ` (3 preceding siblings ...)
  2022-03-14  5:03 ` [PATCH v3 5/5] platform/x86: amd-pmc: Drop CPU QoS workaround Mario Limonciello
@ 2022-03-14  9:05 ` Hans de Goede
  2022-03-14 13:32   ` Limonciello, Mario
  2022-03-14  9:12 ` Lukas Wunner
  5 siblings, 1 reply; 17+ messages in thread
From: Hans de Goede @ 2022-03-14  9:05 UTC (permalink / raw)
  To: Mario Limonciello, Mark Gross, Rafael J . Wysocki
  Cc: open list:X86 PLATFORM DRIVERS, linux-acpi, Shyam Sundar S K,
	Goswami Sanket

Hi Mario,

On 3/14/22 06:03, Mario Limonciello wrote:
> Currenty the latest thing run during a suspend to idle attempt is
> the LPS0 `prepare_late` callback and the earliest thing is the
> `resume_early` callback.
> 
> There is a desire for the `amd-pmc` driver to suspend later in the
> suspend process (ideally the very last thing), so create a callback
> that it or any other driver can hook into to do this.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> changes from v2->v3:
>  * Check that callbacks exist before calling
> changes from v1->v2:
>  * Change register/unregister arguments to be struct
> 
>  drivers/acpi/x86/s2idle.c | 68 ++++++++++++++++++++++++++++++++++++++-
>  include/linux/acpi.h      | 11 ++++++-
>  2 files changed, 77 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> index abc06e7f89d8..69008c4a86ea 100644
> --- a/drivers/acpi/x86/s2idle.c
> +++ b/drivers/acpi/x86/s2idle.c
> @@ -86,6 +86,9 @@ struct lpi_device_constraint_amd {
>  	int min_dstate;
>  };
>  
> +static LIST_HEAD(lps0_callback_handler_head);
> +static DEFINE_MUTEX(lps0_callback_handler_mutex);
> +
>  static struct lpi_constraints *lpi_constraints_table;
>  static int lpi_constraints_table_size;
>  static int rev_id;
> @@ -444,6 +447,9 @@ static struct acpi_scan_handler lps0_handler = {
>  
>  int acpi_s2idle_prepare_late(void)
>  {
> +	struct lps0_callback_handler *handler;
> +	int rc = 0;
> +
>  	if (!lps0_device_handle || sleep_no_lps0)
>  		return 0;
>  
> @@ -474,14 +480,34 @@ int acpi_s2idle_prepare_late(void)
>  		acpi_sleep_run_lps0_dsm(ACPI_LPS0_MS_ENTRY,
>  				lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
>  	}
> -	return 0;
> +
> +	mutex_lock(&lps0_callback_handler_mutex);
> +	list_for_each_entry(handler, &lps0_callback_handler_head, list_node) {
> +		if (handler->prepare_late_callback) {
> +			rc = handler->prepare_late_callback(handler->context);
> +			if (rc)
> +				goto out;
> +		}
> +	}
> +out:
> +	mutex_unlock(&lps0_callback_handler_mutex);
> +
> +	return rc;
>  }
>  
>  void acpi_s2idle_restore_early(void)
>  {
> +	struct lps0_callback_handler *handler;
> +
>  	if (!lps0_device_handle || sleep_no_lps0)
>  		return;
>  
> +	mutex_lock(&lps0_callback_handler_mutex);
> +	list_for_each_entry(handler, &lps0_callback_handler_head, list_node)
> +		if (handler->restore_early_callback)
> +			handler->restore_early_callback(handler->context);
> +	mutex_unlock(&lps0_callback_handler_mutex);
> +
>  	/* Modern standby exit */
>  	if (lps0_dsm_func_mask_microsoft > 0)
>  		acpi_sleep_run_lps0_dsm(ACPI_LPS0_MS_EXIT,
> @@ -524,4 +550,44 @@ void acpi_s2idle_setup(void)
>  	s2idle_set_ops(&acpi_s2idle_ops_lps0);
>  }
>  
> +int acpi_register_lps0_callbacks(struct lps0_callback_handler *arg)
> +{
> +	struct lps0_callback_handler *handler;
> +
> +	if (!lps0_device_handle || sleep_no_lps0)
> +		return -ENODEV;
> +
> +	handler = kmalloc(sizeof(*handler), GFP_KERNEL);
> +	if (!handler)
> +		return -ENOMEM;
> +	handler->prepare_late_callback = arg->prepare_late_callback;
> +	handler->restore_early_callback = arg->restore_early_callback;
> +	handler->context = arg->context;
> +
> +	mutex_lock(&lps0_callback_handler_mutex);
> +	list_add(&handler->list_node, &lps0_callback_handler_head);
> +	mutex_unlock(&lps0_callback_handler_mutex);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(acpi_register_lps0_callbacks);

Typically with calls like these we simply let the caller own the struct lps0_callback_handler
and only make the list_add() call here. Typically the struct lps0_callback_handler will
be embedded in the driver_data of the driver registering the handler and it will
call the unregister function before free-ing its driver_data.

> +
> +void acpi_unregister_lps0_callbacks(struct lps0_callback_handler *arg)
> +{
> +	struct lps0_callback_handler *handler;
> +
> +	mutex_lock(&lps0_callback_handler_mutex);
> +	list_for_each_entry(handler, &lps0_callback_handler_head, list_node) {
> +		if (handler->prepare_late_callback == arg->prepare_late_callback &&
> +		    handler->restore_early_callback == arg->restore_early_callback &&
> +		    handler->context == arg->context) {
> +			list_del(&handler->list_node);
> +			kfree(handler);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&lps0_callback_handler_mutex);
> +}
> +EXPORT_SYMBOL_GPL(acpi_unregister_lps0_callbacks);

And this then becomes just lock, list_del, unlock.

Regards,

Hans



> +
>  #endif /* CONFIG_SUSPEND */
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 6274758648e3..df105f5e03e5 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1023,7 +1023,16 @@ void acpi_os_set_prepare_extended_sleep(int (*func)(u8 sleep_state,
>  
>  acpi_status acpi_os_prepare_extended_sleep(u8 sleep_state,
>  					   u32 val_a, u32 val_b);
> -
> +#ifdef CONFIG_X86
> +struct lps0_callback_handler {
> +	struct list_head list_node;
> +	int (*prepare_late_callback)(void *context);
> +	void (*restore_early_callback)(void *context);
> +	void *context;
> +};
> +int acpi_register_lps0_callbacks(struct lps0_callback_handler *arg);
> +void acpi_unregister_lps0_callbacks(struct lps0_callback_handler *arg);
> +#endif /* CONFIG_X86 */
>  #ifndef CONFIG_IA64
>  void arch_reserve_mem_area(acpi_physical_address addr, size_t size);
>  #else


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler
  2022-03-14  5:03 [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler Mario Limonciello
                   ` (4 preceding siblings ...)
  2022-03-14  9:05 ` [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler Hans de Goede
@ 2022-03-14  9:12 ` Lukas Wunner
  2022-03-14 13:28   ` Limonciello, Mario
  5 siblings, 1 reply; 17+ messages in thread
From: Lukas Wunner @ 2022-03-14  9:12 UTC (permalink / raw)
  To: Mario Limonciello
  Cc: Hans de Goede, Mark Gross, Rafael J . Wysocki,
	open list:X86 PLATFORM DRIVERS, linux-acpi, Shyam Sundar S K,
	Goswami Sanket

On Mon, Mar 14, 2022 at 12:03:35AM -0500, Mario Limonciello wrote:
> Currenty the latest thing run during a suspend to idle attempt is
> the LPS0 `prepare_late` callback and the earliest thing is the
> `resume_early` callback.
> 
> There is a desire for the `amd-pmc` driver to suspend later in the
> suspend process (ideally the very last thing), so create a callback
> that it or any other driver can hook into to do this.

I'm wondering if this can be solved with much less code by either
using device links (a device link to amd-pmc from everything that
needs to be suspended before it), or with a notifier chain?

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler
  2022-03-14  9:12 ` Lukas Wunner
@ 2022-03-14 13:28   ` Limonciello, Mario
  0 siblings, 0 replies; 17+ messages in thread
From: Limonciello, Mario @ 2022-03-14 13:28 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: Hans de Goede, Mark Gross, Rafael J . Wysocki,
	open list:X86 PLATFORM DRIVERS, linux-acpi, S-k, Shyam-sundar,
	Goswami, Sanket

[Public]



> -----Original Message-----
> From: Lukas Wunner <lukas@wunner.de>
> Sent: Monday, March 14, 2022 04:13
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: Hans de Goede <hdegoede@redhat.com>; Mark Gross
> <mgross@linux.intel.com>; Rafael J . Wysocki <rjw@rjwysocki.net>; open
> list:X86 PLATFORM DRIVERS <platform-driver-x86@vger.kernel.org>; linux-
> acpi@vger.kernel.org; S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>;
> Goswami, Sanket <Sanket.Goswami@amd.com>
> Subject: Re: [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler
> 
> On Mon, Mar 14, 2022 at 12:03:35AM -0500, Mario Limonciello wrote:
> > Currenty the latest thing run during a suspend to idle attempt is
> > the LPS0 `prepare_late` callback and the earliest thing is the
> > `resume_early` callback.
> >
> > There is a desire for the `amd-pmc` driver to suspend later in the
> > suspend process (ideally the very last thing), so create a callback
> > that it or any other driver can hook into to do this.
> 
> I'm wondering if this can be solved with much less code by either
> using device links (a device link to amd-pmc from everything that
> needs to be suspended before it), or with a notifier chain?
> 

I don't believe that device links will work well here - the LPS0 "prepare_late"
happens after all device suspend routines are already done.

Notifier chains would probably work, but I'm not convinced they will be much
less code.  As I already have everything working with the current patch series
before I try I'd like to know what Rafael prefers that for this purpose.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler
  2022-03-14  9:05 ` [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler Hans de Goede
@ 2022-03-14 13:32   ` Limonciello, Mario
  2022-03-14 13:37     ` Hans de Goede
  2022-03-15  1:01     ` David E. Box
  0 siblings, 2 replies; 17+ messages in thread
From: Limonciello, Mario @ 2022-03-14 13:32 UTC (permalink / raw)
  To: Hans de Goede, Mark Gross, Rafael J . Wysocki
  Cc: open list:X86 PLATFORM DRIVERS, linux-acpi, S-k, Shyam-sundar,
	Goswami, Sanket

[Public]

> > +int acpi_register_lps0_callbacks(struct lps0_callback_handler *arg)
> > +{
> > +	struct lps0_callback_handler *handler;
> > +
> > +	if (!lps0_device_handle || sleep_no_lps0)
> > +		return -ENODEV;
> > +
> > +	handler = kmalloc(sizeof(*handler), GFP_KERNEL);
> > +	if (!handler)
> > +		return -ENOMEM;
> > +	handler->prepare_late_callback = arg->prepare_late_callback;
> > +	handler->restore_early_callback = arg->restore_early_callback;
> > +	handler->context = arg->context;
> > +
> > +	mutex_lock(&lps0_callback_handler_mutex);
> > +	list_add(&handler->list_node, &lps0_callback_handler_head);
> > +	mutex_unlock(&lps0_callback_handler_mutex);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(acpi_register_lps0_callbacks);
> 
> Typically with calls like these we simply let the caller own the struct
> lps0_callback_handler
> and only make the list_add() call here. Typically the struct
> lps0_callback_handler will
> be embedded in the driver_data of the driver registering the handler and it
> will
> call the unregister function before free-ing its driver_data.
> 

When I put this together I was modeling it off of `struct acpi_wakeup_handler`
which the handling and the use in the kernel doesn't seem to follow the design pattern
you describe.

Rafael - can you please confirm which direction you want to see here for this?

> > +
> > +void acpi_unregister_lps0_callbacks(struct lps0_callback_handler *arg)
> > +{
> > +	struct lps0_callback_handler *handler;
> > +
> > +	mutex_lock(&lps0_callback_handler_mutex);
> > +	list_for_each_entry(handler, &lps0_callback_handler_head,
> list_node) {
> > +		if (handler->prepare_late_callback == arg-
> >prepare_late_callback &&
> > +		    handler->restore_early_callback == arg-
> >restore_early_callback &&
> > +		    handler->context == arg->context) {
> > +			list_del(&handler->list_node);
> > +			kfree(handler);
> > +			break;
> > +		}
> > +	}
> > +	mutex_unlock(&lps0_callback_handler_mutex);
> > +}
> > +EXPORT_SYMBOL_GPL(acpi_unregister_lps0_callbacks);
> 
> And this then becomes just lock, list_del, unlock.
> 
> Regards,
> 
> Hans

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler
  2022-03-14 13:32   ` Limonciello, Mario
@ 2022-03-14 13:37     ` Hans de Goede
  2022-03-16 15:02       ` Rafael J. Wysocki
  2022-03-15  1:01     ` David E. Box
  1 sibling, 1 reply; 17+ messages in thread
From: Hans de Goede @ 2022-03-14 13:37 UTC (permalink / raw)
  To: Limonciello, Mario, Mark Gross, Rafael J . Wysocki
  Cc: open list:X86 PLATFORM DRIVERS, linux-acpi, S-k, Shyam-sundar,
	Goswami, Sanket

Hi,

On 3/14/22 14:32, Limonciello, Mario wrote:
> [Public]
> 
>>> +int acpi_register_lps0_callbacks(struct lps0_callback_handler *arg)
>>> +{
>>> +	struct lps0_callback_handler *handler;
>>> +
>>> +	if (!lps0_device_handle || sleep_no_lps0)
>>> +		return -ENODEV;
>>> +
>>> +	handler = kmalloc(sizeof(*handler), GFP_KERNEL);
>>> +	if (!handler)
>>> +		return -ENOMEM;
>>> +	handler->prepare_late_callback = arg->prepare_late_callback;
>>> +	handler->restore_early_callback = arg->restore_early_callback;
>>> +	handler->context = arg->context;
>>> +
>>> +	mutex_lock(&lps0_callback_handler_mutex);
>>> +	list_add(&handler->list_node, &lps0_callback_handler_head);
>>> +	mutex_unlock(&lps0_callback_handler_mutex);
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(acpi_register_lps0_callbacks);
>>
>> Typically with calls like these we simply let the caller own the struct
>> lps0_callback_handler
>> and only make the list_add() call here. Typically the struct
>> lps0_callback_handler will
>> be embedded in the driver_data of the driver registering the handler and it
>> will
>> call the unregister function before free-ing its driver_data.
>>
> 
> When I put this together I was modeling it off of `struct acpi_wakeup_handler`
> which the handling and the use in the kernel doesn't seem to follow the design pattern
> you describe.

Ah, fair enough. Whatever Rafael prefers works for me.

I pointed this out, because making this change would also make 4/5 a bit
cleaner. You are recreating the same struct lps0_callback_handler on
stack twice there, which looked weird to me.

Note if Rafael wants to stick with the approach from this v3, then
I guess that the approach in 4/5 is fine. 
> Rafael - can you please confirm which direction you want to see here for this?

Regards,

Hans



> 
>>> +
>>> +void acpi_unregister_lps0_callbacks(struct lps0_callback_handler *arg)
>>> +{
>>> +	struct lps0_callback_handler *handler;
>>> +
>>> +	mutex_lock(&lps0_callback_handler_mutex);
>>> +	list_for_each_entry(handler, &lps0_callback_handler_head,
>> list_node) {
>>> +		if (handler->prepare_late_callback == arg-
>>> prepare_late_callback &&
>>> +		    handler->restore_early_callback == arg-
>>> restore_early_callback &&
>>> +		    handler->context == arg->context) {
>>> +			list_del(&handler->list_node);
>>> +			kfree(handler);
>>> +			break;
>>> +		}
>>> +	}
>>> +	mutex_unlock(&lps0_callback_handler_mutex);
>>> +}
>>> +EXPORT_SYMBOL_GPL(acpi_unregister_lps0_callbacks);
>>
>> And this then becomes just lock, list_del, unlock.
>>
>> Regards,
>>
>> Hans


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler
  2022-03-14 13:32   ` Limonciello, Mario
  2022-03-14 13:37     ` Hans de Goede
@ 2022-03-15  1:01     ` David E. Box
  1 sibling, 0 replies; 17+ messages in thread
From: David E. Box @ 2022-03-15  1:01 UTC (permalink / raw)
  To: Limonciello, Mario, Hans de Goede, Mark Gross, Rafael J . Wysocki
  Cc: open list:X86 PLATFORM DRIVERS, linux-acpi, S-k, Shyam-sundar,
	Goswami, Sanket

On Mon, 2022-03-14 at 13:32 +0000, Limonciello, Mario wrote:
> [Public]
> 
> > > +int acpi_register_lps0_callbacks(struct lps0_callback_handler *arg)
> > > +{
> > > +	struct lps0_callback_handler *handler;
> > > +
> > > +	if (!lps0_device_handle || sleep_no_lps0)
> > > +		return -ENODEV;
> > > +
> > > +	handler = kmalloc(sizeof(*handler), GFP_KERNEL);
> > > +	if (!handler)
> > > +		return -ENOMEM;
> > > +	handler->prepare_late_callback = arg->prepare_late_callback;
> > > +	handler->restore_early_callback = arg->restore_early_callback;
> > > +	handler->context = arg->context;
> > > +
> > > +	mutex_lock(&lps0_callback_handler_mutex);
> > > +	list_add(&handler->list_node, &lps0_callback_handler_head);
> > > +	mutex_unlock(&lps0_callback_handler_mutex);
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(acpi_register_lps0_callbacks);
> > 
> > Typically with calls like these we simply let the caller own the struct
> > lps0_callback_handler
> > and only make the list_add() call here. Typically the struct
> > lps0_callback_handler will
> > be embedded in the driver_data of the driver registering the handler and it
> > will
> > call the unregister function before free-ing its driver_data.
> > 
> 
> When I put this together I was modeling it off of `struct acpi_wakeup_handler`
> which the handling and the use in the kernel doesn't seem to follow the design
> pattern
> you describe.
> 
> Rafael - can you please confirm which direction you want to see here for this?
> 
> > > +
> > > +void acpi_unregister_lps0_callbacks(struct lps0_callback_handler *arg)
> > > +{
> > > +	struct lps0_callback_handler *handler;
> > > +
> > > +	mutex_lock(&lps0_callback_handler_mutex);
> > > +	list_for_each_entry(handler, &lps0_callback_handler_head,
> > list_node) {
> > > +		if (handler->prepare_late_callback == arg-
> > > prepare_late_callback &&
> > > +		    handler->restore_early_callback == arg-
> > > restore_early_callback &&
> > > +		    handler->context == arg->context) {
> > > +			list_del(&handler->list_node);
> > > +			kfree(handler);
> > > +			break;
> > > +		}
> > > +	}
> > > +	mutex_unlock(&lps0_callback_handler_mutex);
> > > +}
> > > +EXPORT_SYMBOL_GPL(acpi_unregister_lps0_callbacks);
> > 
> > And this then becomes just lock, list_del, unlock.
> > 
> > Regards,
> > 
> > Hans

If you keep v3,

Reviewed-by: David E. Box <david.e.box@linux.intel.com>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler
  2022-03-14 13:37     ` Hans de Goede
@ 2022-03-16 15:02       ` Rafael J. Wysocki
  2022-03-16 15:34         ` Rafael J. Wysocki
  0 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2022-03-16 15:02 UTC (permalink / raw)
  To: Hans de Goede, Limonciello, Mario
  Cc: Mark Gross, Rafael J . Wysocki, open list:X86 PLATFORM DRIVERS,
	linux-acpi, S-k, Shyam-sundar, Goswami, Sanket

On Mon, Mar 14, 2022 at 2:37 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 3/14/22 14:32, Limonciello, Mario wrote:
> > [Public]
> >
> >>> +int acpi_register_lps0_callbacks(struct lps0_callback_handler *arg)
> >>> +{
> >>> +   struct lps0_callback_handler *handler;
> >>> +
> >>> +   if (!lps0_device_handle || sleep_no_lps0)
> >>> +           return -ENODEV;
> >>> +
> >>> +   handler = kmalloc(sizeof(*handler), GFP_KERNEL);
> >>> +   if (!handler)
> >>> +           return -ENOMEM;
> >>> +   handler->prepare_late_callback = arg->prepare_late_callback;
> >>> +   handler->restore_early_callback = arg->restore_early_callback;
> >>> +   handler->context = arg->context;
> >>> +
> >>> +   mutex_lock(&lps0_callback_handler_mutex);
> >>> +   list_add(&handler->list_node, &lps0_callback_handler_head);
> >>> +   mutex_unlock(&lps0_callback_handler_mutex);
> >>> +
> >>> +   return 0;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(acpi_register_lps0_callbacks);
> >>
> >> Typically with calls like these we simply let the caller own the struct
> >> lps0_callback_handler
> >> and only make the list_add() call here. Typically the struct
> >> lps0_callback_handler will
> >> be embedded in the driver_data of the driver registering the handler and it
> >> will
> >> call the unregister function before free-ing its driver_data.
> >>
> >
> > When I put this together I was modeling it off of `struct acpi_wakeup_handler`

The structure added by this patch is more like struct dev_pm_ops, though.

> > which the handling and the use in the kernel doesn't seem to follow the design pattern
> > you describe.
>
> Ah, fair enough. Whatever Rafael prefers works for me.

My preference at this point would be to use a notifier chain, unless
that's not sufficient for some reason, because it appears to match the
notifier usage model.

> I pointed this out, because making this change would also make 4/5 a bit
> cleaner. You are recreating the same struct lps0_callback_handler on
> stack twice there, which looked weird to me.
>
> Note if Rafael wants to stick with the approach from this v3, then
> I guess that the approach in 4/5 is fine.
> > Rafael - can you please confirm which direction you want to see here for this?

Done above.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler
  2022-03-16 15:02       ` Rafael J. Wysocki
@ 2022-03-16 15:34         ` Rafael J. Wysocki
  2022-03-16 15:43           ` Limonciello, Mario
  0 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2022-03-16 15:34 UTC (permalink / raw)
  To: Hans de Goede, Limonciello, Mario
  Cc: Mark Gross, Rafael J . Wysocki, open list:X86 PLATFORM DRIVERS,
	linux-acpi, S-k, Shyam-sundar, Goswami, Sanket

On Wed, Mar 16, 2022 at 4:02 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Mar 14, 2022 at 2:37 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Hi,
> >
> > On 3/14/22 14:32, Limonciello, Mario wrote:
> > > [Public]
> > >
> > >>> +int acpi_register_lps0_callbacks(struct lps0_callback_handler *arg)
> > >>> +{
> > >>> +   struct lps0_callback_handler *handler;
> > >>> +
> > >>> +   if (!lps0_device_handle || sleep_no_lps0)
> > >>> +           return -ENODEV;
> > >>> +
> > >>> +   handler = kmalloc(sizeof(*handler), GFP_KERNEL);
> > >>> +   if (!handler)
> > >>> +           return -ENOMEM;
> > >>> +   handler->prepare_late_callback = arg->prepare_late_callback;
> > >>> +   handler->restore_early_callback = arg->restore_early_callback;
> > >>> +   handler->context = arg->context;
> > >>> +
> > >>> +   mutex_lock(&lps0_callback_handler_mutex);
> > >>> +   list_add(&handler->list_node, &lps0_callback_handler_head);
> > >>> +   mutex_unlock(&lps0_callback_handler_mutex);
> > >>> +
> > >>> +   return 0;
> > >>> +}
> > >>> +EXPORT_SYMBOL_GPL(acpi_register_lps0_callbacks);
> > >>
> > >> Typically with calls like these we simply let the caller own the struct
> > >> lps0_callback_handler
> > >> and only make the list_add() call here. Typically the struct
> > >> lps0_callback_handler will
> > >> be embedded in the driver_data of the driver registering the handler and it
> > >> will
> > >> call the unregister function before free-ing its driver_data.
> > >>
> > >
> > > When I put this together I was modeling it off of `struct acpi_wakeup_handler`
>
> The structure added by this patch is more like struct dev_pm_ops, though.
>
> > > which the handling and the use in the kernel doesn't seem to follow the design pattern
> > > you describe.
> >
> > Ah, fair enough. Whatever Rafael prefers works for me.
>
> My preference at this point would be to use a notifier chain, unless
> that's not sufficient for some reason, because it appears to match the
> notifier usage model.

Well, I'm actually not sure about that.

> > I pointed this out, because making this change would also make 4/5 a bit
> > cleaner. You are recreating the same struct lps0_callback_handler on
> > stack twice there, which looked weird to me.
> >
> > Note if Rafael wants to stick with the approach from this v3, then
> > I guess that the approach in 4/5 is fine.
> > > Rafael - can you please confirm which direction you want to see here for this?

So the idea is that the PMC driver's "suspend" method needs to be
invoked from acpi_s2idle_prepare_late(), so it doesn't interfere with
the suspend of the other devices in the system and so it can take the
constraints into account.

What is it going to do, in the future, depending on whether or not the
constraints are met?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler
  2022-03-16 15:34         ` Rafael J. Wysocki
@ 2022-03-16 15:43           ` Limonciello, Mario
  2022-03-16 15:52             ` Rafael J. Wysocki
  0 siblings, 1 reply; 17+ messages in thread
From: Limonciello, Mario @ 2022-03-16 15:43 UTC (permalink / raw)
  To: Rafael J. Wysocki, Hans de Goede
  Cc: Mark Gross, Rafael J . Wysocki, open list:X86 PLATFORM DRIVERS,
	linux-acpi, S-k, Shyam-sundar, Goswami, Sanket

[Public]



> -----Original Message-----
> From: Rafael J. Wysocki <rafael@kernel.org>
> Sent: Wednesday, March 16, 2022 10:35
> To: Hans de Goede <hdegoede@redhat.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>
> Cc: Mark Gross <mgross@linux.intel.com>; Rafael J . Wysocki
> <rjw@rjwysocki.net>; open list:X86 PLATFORM DRIVERS <platform-driver-
> x86@vger.kernel.org>; linux-acpi@vger.kernel.org; S-k, Shyam-sundar
> <Shyam-sundar.S-k@amd.com>; Goswami, Sanket
> <Sanket.Goswami@amd.com>
> Subject: Re: [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler
> 
> On Wed, Mar 16, 2022 at 4:02 PM Rafael J. Wysocki <rafael@kernel.org>
> wrote:
> >
> > On Mon, Mar 14, 2022 at 2:37 PM Hans de Goede
> <hdegoede@redhat.com> wrote:
> > >
> > > Hi,
> > >
> > > On 3/14/22 14:32, Limonciello, Mario wrote:
> > > > [Public]
> > > >
> > > >>> +int acpi_register_lps0_callbacks(struct lps0_callback_handler *arg)
> > > >>> +{
> > > >>> +   struct lps0_callback_handler *handler;
> > > >>> +
> > > >>> +   if (!lps0_device_handle || sleep_no_lps0)
> > > >>> +           return -ENODEV;
> > > >>> +
> > > >>> +   handler = kmalloc(sizeof(*handler), GFP_KERNEL);
> > > >>> +   if (!handler)
> > > >>> +           return -ENOMEM;
> > > >>> +   handler->prepare_late_callback = arg->prepare_late_callback;
> > > >>> +   handler->restore_early_callback = arg->restore_early_callback;
> > > >>> +   handler->context = arg->context;
> > > >>> +
> > > >>> +   mutex_lock(&lps0_callback_handler_mutex);
> > > >>> +   list_add(&handler->list_node, &lps0_callback_handler_head);
> > > >>> +   mutex_unlock(&lps0_callback_handler_mutex);
> > > >>> +
> > > >>> +   return 0;
> > > >>> +}
> > > >>> +EXPORT_SYMBOL_GPL(acpi_register_lps0_callbacks);
> > > >>
> > > >> Typically with calls like these we simply let the caller own the struct
> > > >> lps0_callback_handler
> > > >> and only make the list_add() call here. Typically the struct
> > > >> lps0_callback_handler will
> > > >> be embedded in the driver_data of the driver registering the handler
> and it
> > > >> will
> > > >> call the unregister function before free-ing its driver_data.
> > > >>
> > > >
> > > > When I put this together I was modeling it off of `struct
> acpi_wakeup_handler`
> >
> > The structure added by this patch is more like struct dev_pm_ops, though.
> >
> > > > which the handling and the use in the kernel doesn't seem to follow the
> design pattern
> > > > you describe.
> > >
> > > Ah, fair enough. Whatever Rafael prefers works for me.
> >
> > My preference at this point would be to use a notifier chain, unless
> > that's not sufficient for some reason, because it appears to match the
> > notifier usage model.
> 
> Well, I'm actually not sure about that.
> 
> > > I pointed this out, because making this change would also make 4/5 a bit
> > > cleaner. You are recreating the same struct lps0_callback_handler on
> > > stack twice there, which looked weird to me.
> > >
> > > Note if Rafael wants to stick with the approach from this v3, then
> > > I guess that the approach in 4/5 is fine.
> > > > Rafael - can you please confirm which direction you want to see here
> for this?
> 
> So the idea is that the PMC driver's "suspend" method needs to be
> invoked from acpi_s2idle_prepare_late(), so it doesn't interfere with
> the suspend of the other devices in the system and so it can take the
> constraints into account.

The reason to do nothing (besides a debug level message right now) with the constraints
information is that at least on today's OEM platforms there are some instances constraints
aren't met on Linux that need to be investigated and root caused.  These particular constraints
don't actually cause problems reaching s0ix residency though.

> 
> What is it going to do, in the future, depending on whether or not the
> constraints are met?

The idea was that if constraints were met that it would send the OS_HINT as part of
amd_pmc_suspend/amd_pmc_resume, and if they aren't met then skip this step.

It would effectively block the system from getting s0ix residency unless the constraints
are all met.  Given we know some OEM platforms have problems in current generations
with constraints it would probably need to be restricted to this behavior only on a future
SOC that we are confident of all drivers and firmware are doing the right thing.

By passing the information to amd_pmc we can keep that logic restricting the behavior to
only those platforms that we're confident on that behavior.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler
  2022-03-16 15:43           ` Limonciello, Mario
@ 2022-03-16 15:52             ` Rafael J. Wysocki
  2022-03-16 16:43               ` Limonciello, Mario
  0 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2022-03-16 15:52 UTC (permalink / raw)
  To: Limonciello, Mario
  Cc: Rafael J. Wysocki, Hans de Goede, Mark Gross, Rafael J . Wysocki,
	open list:X86 PLATFORM DRIVERS, linux-acpi, S-k, Shyam-sundar,
	Goswami, Sanket

On Wed, Mar 16, 2022 at 4:43 PM Limonciello, Mario
<Mario.Limonciello@amd.com> wrote:
>
> [Public]
>
>
>
> > -----Original Message-----
> > From: Rafael J. Wysocki <rafael@kernel.org>
> > Sent: Wednesday, March 16, 2022 10:35
> > To: Hans de Goede <hdegoede@redhat.com>; Limonciello, Mario
> > <Mario.Limonciello@amd.com>
> > Cc: Mark Gross <mgross@linux.intel.com>; Rafael J . Wysocki
> > <rjw@rjwysocki.net>; open list:X86 PLATFORM DRIVERS <platform-driver-
> > x86@vger.kernel.org>; linux-acpi@vger.kernel.org; S-k, Shyam-sundar
> > <Shyam-sundar.S-k@amd.com>; Goswami, Sanket
> > <Sanket.Goswami@amd.com>
> > Subject: Re: [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler
> >
> > On Wed, Mar 16, 2022 at 4:02 PM Rafael J. Wysocki <rafael@kernel.org>
> > wrote:
> > >
> > > On Mon, Mar 14, 2022 at 2:37 PM Hans de Goede
> > <hdegoede@redhat.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On 3/14/22 14:32, Limonciello, Mario wrote:
> > > > > [Public]
> > > > >
> > > > >>> +int acpi_register_lps0_callbacks(struct lps0_callback_handler *arg)
> > > > >>> +{
> > > > >>> +   struct lps0_callback_handler *handler;
> > > > >>> +
> > > > >>> +   if (!lps0_device_handle || sleep_no_lps0)
> > > > >>> +           return -ENODEV;
> > > > >>> +
> > > > >>> +   handler = kmalloc(sizeof(*handler), GFP_KERNEL);
> > > > >>> +   if (!handler)
> > > > >>> +           return -ENOMEM;
> > > > >>> +   handler->prepare_late_callback = arg->prepare_late_callback;
> > > > >>> +   handler->restore_early_callback = arg->restore_early_callback;
> > > > >>> +   handler->context = arg->context;
> > > > >>> +
> > > > >>> +   mutex_lock(&lps0_callback_handler_mutex);
> > > > >>> +   list_add(&handler->list_node, &lps0_callback_handler_head);
> > > > >>> +   mutex_unlock(&lps0_callback_handler_mutex);
> > > > >>> +
> > > > >>> +   return 0;
> > > > >>> +}
> > > > >>> +EXPORT_SYMBOL_GPL(acpi_register_lps0_callbacks);
> > > > >>
> > > > >> Typically with calls like these we simply let the caller own the struct
> > > > >> lps0_callback_handler
> > > > >> and only make the list_add() call here. Typically the struct
> > > > >> lps0_callback_handler will
> > > > >> be embedded in the driver_data of the driver registering the handler
> > and it
> > > > >> will
> > > > >> call the unregister function before free-ing its driver_data.
> > > > >>
> > > > >
> > > > > When I put this together I was modeling it off of `struct
> > acpi_wakeup_handler`
> > >
> > > The structure added by this patch is more like struct dev_pm_ops, though.
> > >
> > > > > which the handling and the use in the kernel doesn't seem to follow the
> > design pattern
> > > > > you describe.
> > > >
> > > > Ah, fair enough. Whatever Rafael prefers works for me.
> > >
> > > My preference at this point would be to use a notifier chain, unless
> > > that's not sufficient for some reason, because it appears to match the
> > > notifier usage model.
> >
> > Well, I'm actually not sure about that.
> >
> > > > I pointed this out, because making this change would also make 4/5 a bit
> > > > cleaner. You are recreating the same struct lps0_callback_handler on
> > > > stack twice there, which looked weird to me.
> > > >
> > > > Note if Rafael wants to stick with the approach from this v3, then
> > > > I guess that the approach in 4/5 is fine.
> > > > > Rafael - can you please confirm which direction you want to see here
> > for this?
> >
> > So the idea is that the PMC driver's "suspend" method needs to be
> > invoked from acpi_s2idle_prepare_late(), so it doesn't interfere with
> > the suspend of the other devices in the system and so it can take the
> > constraints into account.
>
> The reason to do nothing (besides a debug level message right now) with the constraints
> information is that at least on today's OEM platforms there are some instances constraints
> aren't met on Linux that need to be investigated and root caused.  These particular constraints
> don't actually cause problems reaching s0ix residency though.

Why are they useful at all, then?

> >
> > What is it going to do, in the future, depending on whether or not the
> > constraints are met?
>
> The idea was that if constraints were met that it would send the OS_HINT as part of
> amd_pmc_suspend/amd_pmc_resume, and if they aren't met then skip this step.
>
> It would effectively block the system from getting s0ix residency unless the constraints
> are all met.

Why do that?

> Given we know some OEM platforms have problems in current generations
> with constraints it would probably need to be restricted to this behavior only on a future
> SOC that we are confident of all drivers and firmware are doing the right thing.
>
> By passing the information to amd_pmc we can keep that logic restricting the behavior to
> only those platforms that we're confident on that behavior.

Honestly, I'm not quite sure why it is a good idea to prevent the
platform from attempting to get into S0ix via suspend-to-idle in any
case.

You know you have to suspend.  You don't know how much time you will
be suspended.  The constraints can only tell you what's the
lowest-power state you can achieve at this point, but why is it
relevant?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler
  2022-03-16 15:52             ` Rafael J. Wysocki
@ 2022-03-16 16:43               ` Limonciello, Mario
  2022-03-16 17:27                 ` Rafael J. Wysocki
  0 siblings, 1 reply; 17+ messages in thread
From: Limonciello, Mario @ 2022-03-16 16:43 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Hans de Goede, Mark Gross, Rafael J . Wysocki,
	open list:X86 PLATFORM DRIVERS, linux-acpi, S-k, Shyam-sundar,
	Goswami, Sanket

>>>> My preference at this point would be to use a notifier chain, unless
>>>> that's not sufficient for some reason, because it appears to match the
>>>> notifier usage model.
>>>
>>> Well, I'm actually not sure about that.
>>>
>>>>> I pointed this out, because making this change would also make 4/5 a bit
>>>>> cleaner. You are recreating the same struct lps0_callback_handler on
>>>>> stack twice there, which looked weird to me.
>>>>>
>>>>> Note if Rafael wants to stick with the approach from this v3, then
>>>>> I guess that the approach in 4/5 is fine.
>>>>>> Rafael - can you please confirm which direction you want to see here
>>> for this?
>>>
>>> So the idea is that the PMC driver's "suspend" method needs to be
>>> invoked from acpi_s2idle_prepare_late(), so it doesn't interfere with
>>> the suspend of the other devices in the system and so it can take the
>>> constraints into account.
>>
>> The reason to do nothing (besides a debug level message right now) with the constraints
>> information is that at least on today's OEM platforms there are some instances constraints
>> aren't met on Linux that need to be investigated and root caused.  These particular constraints
>> don't actually cause problems reaching s0ix residency though.
> 
> Why are they useful at all, then?
> 
>>>
>>> What is it going to do, in the future, depending on whether or not the
>>> constraints are met?
>>
>> The idea was that if constraints were met that it would send the OS_HINT as part of
>> amd_pmc_suspend/amd_pmc_resume, and if they aren't met then skip this step.
>>
>> It would effectively block the system from getting s0ix residency unless the constraints
>> are all met.
> 
> Why do that?

I guess to both of your above questions this begs a comparison of how 
things work in Windows versus Linux.

Windows Modern Standby has this concept of "SW DRIPS" vs "HW DRIPS". 
 From an end user perspective you close your lid or you click Start 
button and hit sleep and the machine is in sleep.  Whether it's in the 
deepest state is "invisible" to you unless you're running a sleep study.

Windows will at this time requests devices to go into their deepest 
states and will continue to monitor them against the constraints table.
When the constraints table is matched a uPEP driver is notified (this is 
the _DSM stuff we have in Linux too for "deepest state") and then it can 
do as it pleases.   ON AMD's platform this sends OS_HINT.  OS_HINT is 
meant to be indicate that the OS is done with all it's suspend actions 
(caches flushed, devices in D3 etc) and the system can "try" to enter s0ix.

Windows will then also distinguish between different types of wakeups 
and have different behaviors for them.   There are wakeups that can be 
treated as keep screen off, and then possibly go back into deepest state.

"As soon as the SoC wakes and the platform exits the DRIPS state, the 
CPUs start running code again. However, the screen stays powered off 
unless the interrupt was a result of user input or connecting to a power 
source"
https://docs.microsoft.com/en-us/windows-hardware/design/device-experiences/transitioning-between-idle-and-active-states

"During the Sleep state, specific value-adding SW activity may run"
https://docs.microsoft.com/en-us/windows-hardware/design/device-experiences/modern-standby-states

So from the time that I clicked sleep in the OS, I might NOT get into 
the deepest state (HW DRIPS), or I might have gotten in and out several 
times.  If a device driver failed to put a PCI device in D3 for example 
I would not be able to enter HW DRIPS, but the suspend wouldn't "fail".

Now to contrast this to Linux when I enter suspend all drivers will run 
their various PM callbacks and devices will go into their deepest states.
* If a driver fails, the suspend actually fails and you get an error 
code to go investigate what happened.
* If the devices don't get into their deepest state by the time you get 
to the s2idle loop you don't get s0ix residency.

As you can see at least for AMD's platforms OS_HINT is sent too "early". 
  That's why this series exists in the first place.

So with all that said; why look at constraints at all if stuff is working?
 From this design at least on Windows constraints are supposed to be a 
safety guard that you don't start the HW process for s0i3 process "too 
early".

The last commit that is getting reverted in this series is an example of 
what could happen if the process is started prematurely.

> 
>> Given we know some OEM platforms have problems in current generations
>> with constraints it would probably need to be restricted to this behavior only on a future
>> SOC that we are confident of all drivers and firmware are doing the right thing.
>>
>> By passing the information to amd_pmc we can keep that logic restricting the behavior to
>> only those platforms that we're confident on that behavior.
> 
> Honestly, I'm not quite sure why it is a good idea to prevent the
> platform from attempting to get into S0ix via suspend-to-idle in any
> case.
> 
> You know you have to suspend.  You don't know how much time you will
> be suspended.  The constraints can only tell you what's the
> lowest-power state you can achieve at this point, but why is it
> relevant?

Having thought through and said all I did above, I do concede you're 
right with the Linux approach to sleep the constraints really don't add 
a lot of value.  If a device fails to enter it's intended sleep states 
the suspend will "fail".  If the suspend succeeds but the constraints 
table doesn't match, it's just a hint where to focus on problems.

I appreciate your thoughts and I will drop the constraints passing patch 
in this series.

With that intent of dropping that would you still like this reworked as 
a notifier chain or keep it as this design?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler
  2022-03-16 16:43               ` Limonciello, Mario
@ 2022-03-16 17:27                 ` Rafael J. Wysocki
  0 siblings, 0 replies; 17+ messages in thread
From: Rafael J. Wysocki @ 2022-03-16 17:27 UTC (permalink / raw)
  To: Limonciello, Mario
  Cc: Rafael J. Wysocki, Hans de Goede, Mark Gross, Rafael J . Wysocki,
	open list:X86 PLATFORM DRIVERS, linux-acpi, S-k, Shyam-sundar,
	Goswami, Sanket

On Wed, Mar 16, 2022 at 5:43 PM Limonciello, Mario
<mario.limonciello@amd.com> wrote:
>
> >>>> My preference at this point would be to use a notifier chain, unless
> >>>> that's not sufficient for some reason, because it appears to match the
> >>>> notifier usage model.
> >>>
> >>> Well, I'm actually not sure about that.
> >>>
> >>>>> I pointed this out, because making this change would also make 4/5 a bit
> >>>>> cleaner. You are recreating the same struct lps0_callback_handler on
> >>>>> stack twice there, which looked weird to me.
> >>>>>
> >>>>> Note if Rafael wants to stick with the approach from this v3, then
> >>>>> I guess that the approach in 4/5 is fine.
> >>>>>> Rafael - can you please confirm which direction you want to see here
> >>> for this?
> >>>
> >>> So the idea is that the PMC driver's "suspend" method needs to be
> >>> invoked from acpi_s2idle_prepare_late(), so it doesn't interfere with
> >>> the suspend of the other devices in the system and so it can take the
> >>> constraints into account.
> >>
> >> The reason to do nothing (besides a debug level message right now) with the constraints
> >> information is that at least on today's OEM platforms there are some instances constraints
> >> aren't met on Linux that need to be investigated and root caused.  These particular constraints
> >> don't actually cause problems reaching s0ix residency though.
> >
> > Why are they useful at all, then?
> >
> >>>
> >>> What is it going to do, in the future, depending on whether or not the
> >>> constraints are met?
> >>
> >> The idea was that if constraints were met that it would send the OS_HINT as part of
> >> amd_pmc_suspend/amd_pmc_resume, and if they aren't met then skip this step.
> >>
> >> It would effectively block the system from getting s0ix residency unless the constraints
> >> are all met.
> >
> > Why do that?
>
> I guess to both of your above questions this begs a comparison of how
> things work in Windows versus Linux.
>
> Windows Modern Standby has this concept of "SW DRIPS" vs "HW DRIPS".
>  From an end user perspective you close your lid or you click Start
> button and hit sleep and the machine is in sleep.  Whether it's in the
> deepest state is "invisible" to you unless you're running a sleep study.
>
> Windows will at this time requests devices to go into their deepest
> states and will continue to monitor them against the constraints table.
> When the constraints table is matched a uPEP driver is notified (this is
> the _DSM stuff we have in Linux too for "deepest state") and then it can
> do as it pleases.   ON AMD's platform this sends OS_HINT.  OS_HINT is
> meant to be indicate that the OS is done with all it's suspend actions
> (caches flushed, devices in D3 etc) and the system can "try" to enter s0ix.
>
> Windows will then also distinguish between different types of wakeups
> and have different behaviors for them.   There are wakeups that can be
> treated as keep screen off, and then possibly go back into deepest state.
>
> "As soon as the SoC wakes and the platform exits the DRIPS state, the
> CPUs start running code again. However, the screen stays powered off
> unless the interrupt was a result of user input or connecting to a power
> source"
> https://docs.microsoft.com/en-us/windows-hardware/design/device-experiences/transitioning-between-idle-and-active-states
>
> "During the Sleep state, specific value-adding SW activity may run"
> https://docs.microsoft.com/en-us/windows-hardware/design/device-experiences/modern-standby-states
>
> So from the time that I clicked sleep in the OS, I might NOT get into
> the deepest state (HW DRIPS), or I might have gotten in and out several
> times.  If a device driver failed to put a PCI device in D3 for example
> I would not be able to enter HW DRIPS, but the suspend wouldn't "fail".

Right.  I'm aware of how this is expected to work in the Windows
Modern Standby context.

> Now to contrast this to Linux when I enter suspend all drivers will run
> their various PM callbacks and devices will go into their deepest states.
> * If a driver fails, the suspend actually fails and you get an error
> code to go investigate what happened.
> * If the devices don't get into their deepest state by the time you get
> to the s2idle loop you don't get s0ix residency.
>
> As you can see at least for AMD's platforms OS_HINT is sent too "early".
>   That's why this series exists in the first place.

Right.

> So with all that said; why look at constraints at all if stuff is working?
>  From this design at least on Windows constraints are supposed to be a
> safety guard that you don't start the HW process for s0i3 process "too
> early".
>
> The last commit that is getting reverted in this series is an example of
> what could happen if the process is started prematurely.

Well, it is not "the process started prematurely", but an ordering
issue in a process that's already under way.

The real difference is that Modern Standby is designed as an extension
of the system working state and things can go from "working" to
"sleeping" in a kind of transparent fashion, whereas in Linux the
system is either suspended or working, or going from one of these
states to the other.

From the Modern Standby POV it only makes sense to attempt to enter HW
DRIPS if all of the potentially involved entities are ready, and hence
the constraints.  In Linux, they all are expected to get ready during
transitions from "working" to "suspended".

> >
> >> Given we know some OEM platforms have problems in current generations
> >> with constraints it would probably need to be restricted to this behavior only on a future
> >> SOC that we are confident of all drivers and firmware are doing the right thing.
> >>
> >> By passing the information to amd_pmc we can keep that logic restricting the behavior to
> >> only those platforms that we're confident on that behavior.
> >
> > Honestly, I'm not quite sure why it is a good idea to prevent the
> > platform from attempting to get into S0ix via suspend-to-idle in any
> > case.
> >
> > You know you have to suspend.  You don't know how much time you will
> > be suspended.  The constraints can only tell you what's the
> > lowest-power state you can achieve at this point, but why is it
> > relevant?
>
> Having thought through and said all I did above, I do concede you're
> right with the Linux approach to sleep the constraints really don't add
> a lot of value.  If a device fails to enter it's intended sleep states
> the suspend will "fail".  If the suspend succeeds but the constraints
> table doesn't match, it's just a hint where to focus on problems.

Exactly.

> I appreciate your thoughts and I will drop the constraints passing patch
> in this series.

Thanks!

> With that intent of dropping that would you still like this reworked as
> a notifier chain or keep it as this design?

I would introduce something like

struct acpi_s2idle_dev_ops {
        struct list_head list_node;
        void (*prepare)(struct device *dev);
        void (*restore)(struct device *dev);
};

and let whoever wants to use one of these pass the pointer to it to a
"register" function (that will only do a "list add").  The reason why
I wouldn't allow ->prepare() to fail is that failing suspend at this
point isn't particularly useful (and arguably it isn't really useful
at all, but that's a whole different topic).

This can be a const struct in a driver, so it cannot be modified even
by mistake which reduces the attack surface a bit.

Then, make changes to acpi_s2idle_prepare_late() and
acpi_s2idle_restore_early() like in the $subject patch, except that
the extra locking may not be needed if the "register" function uses
lock_system_sleep() for mutual exclusion.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2022-03-16 17:27 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-14  5:03 [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler Mario Limonciello
2022-03-14  5:03 ` [PATCH v3 2/5] ACPI / x86: Pass the constraints checking result to LPS0 callback Mario Limonciello
2022-03-14  5:03 ` [PATCH v3 3/5] ACPI / x86: Check LPI constraints by default Mario Limonciello
2022-03-14  5:03 ` [PATCH v3 4/5] platform/x86: amd-pmc: Move to later in the suspend process Mario Limonciello
2022-03-14  5:03 ` [PATCH v3 5/5] platform/x86: amd-pmc: Drop CPU QoS workaround Mario Limonciello
2022-03-14  9:05 ` [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler Hans de Goede
2022-03-14 13:32   ` Limonciello, Mario
2022-03-14 13:37     ` Hans de Goede
2022-03-16 15:02       ` Rafael J. Wysocki
2022-03-16 15:34         ` Rafael J. Wysocki
2022-03-16 15:43           ` Limonciello, Mario
2022-03-16 15:52             ` Rafael J. Wysocki
2022-03-16 16:43               ` Limonciello, Mario
2022-03-16 17:27                 ` Rafael J. Wysocki
2022-03-15  1:01     ` David E. Box
2022-03-14  9:12 ` Lukas Wunner
2022-03-14 13:28   ` Limonciello, Mario

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.