linux-acpi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 1/4] ACPI / x86: Add support for LPS0 callback handler
@ 2022-03-17 14:14 Mario Limonciello
  2022-03-17 14:14 ` [PATCH v5 2/4] platform/x86: amd-pmc: Move to later in the suspend process Mario Limonciello
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Mario Limonciello @ 2022-03-17 14:14 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 v4->v5:
* fix a check for handler->prepare
  Reported by Dan Carpenter <dan.carpenter@oracle.com>
changes from v3->v4:
* drop use of mutex, use lock_system_sleep instead
* don't pass argument of context, driver will maintain this internally
* don't allow failing prepare stage
* don't allow unregistering if sleep_no_lps0 is set
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 | 40 +++++++++++++++++++++++++++++++++++++++
 include/linux/acpi.h      | 10 +++++++++-
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index abc06e7f89d8..031b20a547f9 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -86,6 +86,8 @@ struct lpi_device_constraint_amd {
 	int min_dstate;
 };
 
+static LIST_HEAD(lps0_s2idle_devops_head);
+
 static struct lpi_constraints *lpi_constraints_table;
 static int lpi_constraints_table_size;
 static int rev_id;
@@ -444,6 +446,8 @@ static struct acpi_scan_handler lps0_handler = {
 
 int acpi_s2idle_prepare_late(void)
 {
+	struct acpi_s2idle_dev_ops *handler;
+
 	if (!lps0_device_handle || sleep_no_lps0)
 		return 0;
 
@@ -474,14 +478,26 @@ int acpi_s2idle_prepare_late(void)
 		acpi_sleep_run_lps0_dsm(ACPI_LPS0_MS_ENTRY,
 				lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
 	}
+
+	list_for_each_entry(handler, &lps0_s2idle_devops_head, list_node) {
+		if (handler->prepare)
+			handler->prepare();
+	}
+
 	return 0;
 }
 
 void acpi_s2idle_restore_early(void)
 {
+	struct acpi_s2idle_dev_ops *handler;
+
 	if (!lps0_device_handle || sleep_no_lps0)
 		return;
 
+	list_for_each_entry(handler, &lps0_s2idle_devops_head, list_node)
+		if (handler->restore)
+			handler->restore();
+
 	/* Modern standby exit */
 	if (lps0_dsm_func_mask_microsoft > 0)
 		acpi_sleep_run_lps0_dsm(ACPI_LPS0_MS_EXIT,
@@ -524,4 +540,28 @@ void acpi_s2idle_setup(void)
 	s2idle_set_ops(&acpi_s2idle_ops_lps0);
 }
 
+int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg)
+{
+	if (!lps0_device_handle || sleep_no_lps0)
+		return -ENODEV;
+
+	lock_system_sleep();
+	list_add(&arg->list_node, &lps0_s2idle_devops_head);
+	unlock_system_sleep();
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_register_lps0_dev);
+
+void acpi_unregister_lps0_dev(struct acpi_s2idle_dev_ops *arg)
+{
+	if (!lps0_device_handle || sleep_no_lps0)
+		return;
+
+	lock_system_sleep();
+	list_del(&arg->list_node);
+	unlock_system_sleep();
+}
+EXPORT_SYMBOL_GPL(acpi_unregister_lps0_dev);
+
 #endif /* CONFIG_SUSPEND */
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 6274758648e3..47c16cdc8e0e 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1023,7 +1023,15 @@ 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 acpi_s2idle_dev_ops {
+	struct list_head list_node;
+	void (*prepare)(void);
+	void (*restore)(void);
+};
+int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg);
+void acpi_unregister_lps0_dev(struct acpi_s2idle_dev_ops *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] 8+ messages in thread

* [PATCH v5 2/4] platform/x86: amd-pmc: Move to later in the suspend process
  2022-03-17 14:14 [PATCH v5 1/4] ACPI / x86: Add support for LPS0 callback handler Mario Limonciello
@ 2022-03-17 14:14 ` Mario Limonciello
  2022-03-17 14:14 ` [PATCH v5 3/4] platform/x86: amd-pmc: Output error codes in messages Mario Limonciello
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Mario Limonciello @ 2022-03-17 14:14 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 v4->v5:
* split into two patches
changes from v3->v4:
* Drop constraints messaging
* Adjust for changes on earlier patches
* Don't fail init if sleep_no_lps0 is set
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 | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index 971aaabaa9c8..2736ab587f2a 100644
--- a/drivers/platform/x86/amd-pmc.c
+++ b/drivers/platform/x86/amd-pmc.c
@@ -639,9 +639,9 @@ 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 void amd_pmc_s2idle_prepare(void)
 {
-	struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
+	struct amd_pmc_dev *pdev = &pmc;
 	int rc;
 	u8 msg;
 	u32 arg = 1;
@@ -658,7 +658,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) {
@@ -672,18 +672,16 @@ static int __maybe_unused amd_pmc_suspend(struct device *dev)
 		dev_err(pdev->dev, "error writing to STB\n");
 		goto fail;
 	}
-
-	return 0;
+	return;
 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;
 }
 
-static int __maybe_unused amd_pmc_resume(struct device *dev)
+static void amd_pmc_s2idle_restore(void)
 {
-	struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
+	struct amd_pmc_dev *pdev = &pmc;
 	int rc;
 	u8 msg;
 
@@ -696,7 +694,7 @@ static int __maybe_unused amd_pmc_resume(struct device *dev)
 	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)
@@ -711,13 +709,11 @@ 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 struct acpi_s2idle_dev_ops amd_pmc_s2idle_dev_ops = {
+	.prepare = amd_pmc_s2idle_prepare,
+	.restore = amd_pmc_s2idle_restore,
 };
 
 static const struct pci_device_id pmc_pci_ids[] = {
@@ -884,6 +880,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_dev(&amd_pmc_s2idle_dev_ops);
+	if (err)
+		dev_warn(dev->dev, "failed to register LPS0 sleep handler, expect increased power consumption\n");
+
 	amd_pmc_dbgfs_register(dev);
 	cpu_latency_qos_add_request(&dev->amd_pmc_pm_qos_req, PM_QOS_DEFAULT_VALUE);
 	return 0;
@@ -897,6 +897,7 @@ static int amd_pmc_remove(struct platform_device *pdev)
 {
 	struct amd_pmc_dev *dev = platform_get_drvdata(pdev);
 
+	acpi_unregister_lps0_dev(&amd_pmc_s2idle_dev_ops);
 	amd_pmc_dbgfs_unregister(dev);
 	pci_dev_put(dev->rdev);
 	mutex_destroy(&dev->lock);
@@ -917,7 +918,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] 8+ messages in thread

* [PATCH v5 3/4] platform/x86: amd-pmc: Output error codes in messages
  2022-03-17 14:14 [PATCH v5 1/4] ACPI / x86: Add support for LPS0 callback handler Mario Limonciello
  2022-03-17 14:14 ` [PATCH v5 2/4] platform/x86: amd-pmc: Move to later in the suspend process Mario Limonciello
@ 2022-03-17 14:14 ` Mario Limonciello
  2022-03-17 14:14 ` [PATCH v5 4/4] platform/x86: amd-pmc: Drop CPU QoS workaround Mario Limonciello
  2022-03-17 16:45 ` [PATCH v5 1/4] ACPI / x86: Add support for LPS0 callback handler Rafael J. Wysocki
  3 siblings, 0 replies; 8+ messages in thread
From: Mario Limonciello @ 2022-03-17 14:14 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 return type for the s2idle callbacks is 'void', so any errors that
occur will no longer be displayed.

Output these error codes in the messages in case of any failures.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
changes from v4->v5:
 * split from previous patch

 drivers/platform/x86/amd-pmc.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index 2736ab587f2a..f36cf125b284 100644
--- a/drivers/platform/x86/amd-pmc.c
+++ b/drivers/platform/x86/amd-pmc.c
@@ -653,8 +653,10 @@ static void amd_pmc_s2idle_prepare(void)
 	/* Activate CZN specific RTC functionality */
 	if (pdev->cpu_id == AMD_CPU_ID_CZN) {
 		rc = amd_pmc_verify_czn_rtc(pdev, &arg);
-		if (rc)
+		if (rc) {
+			dev_err(pdev->dev, "failed to set RTC: %d\n", rc);
 			goto fail;
+		}
 	}
 
 	/* Dump the IdleMask before we send hint to SMU */
@@ -662,14 +664,14 @@ static void amd_pmc_s2idle_prepare(void)
 	msg = amd_pmc_get_os_hint(pdev);
 	rc = amd_pmc_send_cmd(pdev, arg, NULL, msg, 0);
 	if (rc) {
-		dev_err(pdev->dev, "suspend failed\n");
+		dev_err(pdev->dev, "suspend failed: %d\n", rc);
 		goto fail;
 	}
 
 	if (enable_stb)
 		rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_PREDEF);
 	if (rc) {
-		dev_err(pdev->dev, "error writing to STB\n");
+		dev_err(pdev->dev, "error writing to STB: %d\n", rc);
 		goto fail;
 	}
 	return;
@@ -688,7 +690,7 @@ static void amd_pmc_s2idle_restore(void)
 	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);
@@ -700,7 +702,7 @@ static void amd_pmc_s2idle_restore(void)
 	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)
-- 
2.34.1


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

* [PATCH v5 4/4] platform/x86: amd-pmc: Drop CPU QoS workaround
  2022-03-17 14:14 [PATCH v5 1/4] ACPI / x86: Add support for LPS0 callback handler Mario Limonciello
  2022-03-17 14:14 ` [PATCH v5 2/4] platform/x86: amd-pmc: Move to later in the suspend process Mario Limonciello
  2022-03-17 14:14 ` [PATCH v5 3/4] platform/x86: amd-pmc: Output error codes in messages Mario Limonciello
@ 2022-03-17 14:14 ` Mario Limonciello
  2022-03-17 18:54   ` Hans de Goede
  2022-03-17 16:45 ` [PATCH v5 1/4] ACPI / x86: Add support for LPS0 callback handler Rafael J. Wysocki
  3 siblings, 1 reply; 8+ messages in thread
From: Mario Limonciello @ 2022-03-17 14:14 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 v4->v5:
 * rebase on earlier patches
changes from v3->v4:
 * rebase on earlier patches
changes from v2->v3:
 * No changes
changes from v1->v2:
 * No changes

 drivers/platform/x86/amd-pmc.c | 32 +++-----------------------------
 1 file changed, 3 insertions(+), 29 deletions(-)

diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index f36cf125b284..7317993cd91b 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;
 }
 
@@ -655,7 +642,7 @@ static void amd_pmc_s2idle_prepare(void)
 		rc = amd_pmc_verify_czn_rtc(pdev, &arg);
 		if (rc) {
 			dev_err(pdev->dev, "failed to set RTC: %d\n", rc);
-			goto fail;
+			return;
 		}
 	}
 
@@ -665,20 +652,13 @@ static void amd_pmc_s2idle_prepare(void)
 	rc = amd_pmc_send_cmd(pdev, arg, NULL, msg, 0);
 	if (rc) {
 		dev_err(pdev->dev, "suspend failed: %d\n", rc);
-		goto fail;
+		return;
 	}
 
 	if (enable_stb)
 		rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_PREDEF);
-	if (rc) {
+	if (rc)
 		dev_err(pdev->dev, "error writing to STB: %d\n", rc);
-		goto fail;
-	}
-	return;
-fail:
-	if (pdev->cpu_id == AMD_CPU_ID_CZN)
-		cpu_latency_qos_update_request(&pdev->amd_pmc_pm_qos_req,
-						PM_QOS_DEFAULT_VALUE);
 }
 
 static void amd_pmc_s2idle_restore(void)
@@ -704,11 +684,6 @@ static void amd_pmc_s2idle_restore(void)
 	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);
 }
@@ -887,7 +862,6 @@ static int amd_pmc_probe(struct platform_device *pdev)
 		dev_warn(dev->dev, "failed to register LPS0 sleep handler, expect increased power consumption\n");
 
 	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] 8+ messages in thread

* Re: [PATCH v5 1/4] ACPI / x86: Add support for LPS0 callback handler
  2022-03-17 14:14 [PATCH v5 1/4] ACPI / x86: Add support for LPS0 callback handler Mario Limonciello
                   ` (2 preceding siblings ...)
  2022-03-17 14:14 ` [PATCH v5 4/4] platform/x86: amd-pmc: Drop CPU QoS workaround Mario Limonciello
@ 2022-03-17 16:45 ` Rafael J. Wysocki
  2022-03-17 16:49   ` Limonciello, Mario
  3 siblings, 1 reply; 8+ messages in thread
From: Rafael J. Wysocki @ 2022-03-17 16:45 UTC (permalink / raw)
  To: Mario Limonciello
  Cc: Hans de Goede, Mark Gross, Rafael J . Wysocki,
	open list:X86 PLATFORM DRIVERS, ACPI Devel Maling List,
	Shyam Sundar S K, Goswami Sanket

On Thu, Mar 17, 2022 at 3:15 PM Mario Limonciello
<mario.limonciello@amd.com> 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>

This looks good to me, so:

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

and I don't have any conflicting changes queued up for 5.17, so from
my POV it can go in via the x86 platform drivers tree along with the
other patches in the series.

> ---
> changes from v4->v5:
> * fix a check for handler->prepare
>   Reported by Dan Carpenter <dan.carpenter@oracle.com>
> changes from v3->v4:
> * drop use of mutex, use lock_system_sleep instead
> * don't pass argument of context, driver will maintain this internally
> * don't allow failing prepare stage
> * don't allow unregistering if sleep_no_lps0 is set
> 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 | 40 +++++++++++++++++++++++++++++++++++++++
>  include/linux/acpi.h      | 10 +++++++++-
>  2 files changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> index abc06e7f89d8..031b20a547f9 100644
> --- a/drivers/acpi/x86/s2idle.c
> +++ b/drivers/acpi/x86/s2idle.c
> @@ -86,6 +86,8 @@ struct lpi_device_constraint_amd {
>         int min_dstate;
>  };
>
> +static LIST_HEAD(lps0_s2idle_devops_head);
> +
>  static struct lpi_constraints *lpi_constraints_table;
>  static int lpi_constraints_table_size;
>  static int rev_id;
> @@ -444,6 +446,8 @@ static struct acpi_scan_handler lps0_handler = {
>
>  int acpi_s2idle_prepare_late(void)
>  {
> +       struct acpi_s2idle_dev_ops *handler;
> +
>         if (!lps0_device_handle || sleep_no_lps0)
>                 return 0;
>
> @@ -474,14 +478,26 @@ int acpi_s2idle_prepare_late(void)
>                 acpi_sleep_run_lps0_dsm(ACPI_LPS0_MS_ENTRY,
>                                 lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
>         }
> +
> +       list_for_each_entry(handler, &lps0_s2idle_devops_head, list_node) {
> +               if (handler->prepare)
> +                       handler->prepare();
> +       }
> +
>         return 0;
>  }
>
>  void acpi_s2idle_restore_early(void)
>  {
> +       struct acpi_s2idle_dev_ops *handler;
> +
>         if (!lps0_device_handle || sleep_no_lps0)
>                 return;
>
> +       list_for_each_entry(handler, &lps0_s2idle_devops_head, list_node)
> +               if (handler->restore)
> +                       handler->restore();
> +
>         /* Modern standby exit */
>         if (lps0_dsm_func_mask_microsoft > 0)
>                 acpi_sleep_run_lps0_dsm(ACPI_LPS0_MS_EXIT,
> @@ -524,4 +540,28 @@ void acpi_s2idle_setup(void)
>         s2idle_set_ops(&acpi_s2idle_ops_lps0);
>  }
>
> +int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg)
> +{
> +       if (!lps0_device_handle || sleep_no_lps0)
> +               return -ENODEV;
> +
> +       lock_system_sleep();
> +       list_add(&arg->list_node, &lps0_s2idle_devops_head);
> +       unlock_system_sleep();
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(acpi_register_lps0_dev);
> +
> +void acpi_unregister_lps0_dev(struct acpi_s2idle_dev_ops *arg)
> +{
> +       if (!lps0_device_handle || sleep_no_lps0)
> +               return;
> +
> +       lock_system_sleep();
> +       list_del(&arg->list_node);
> +       unlock_system_sleep();
> +}
> +EXPORT_SYMBOL_GPL(acpi_unregister_lps0_dev);
> +
>  #endif /* CONFIG_SUSPEND */
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 6274758648e3..47c16cdc8e0e 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1023,7 +1023,15 @@ 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 acpi_s2idle_dev_ops {
> +       struct list_head list_node;
> +       void (*prepare)(void);
> +       void (*restore)(void);
> +};
> +int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg);
> +void acpi_unregister_lps0_dev(struct acpi_s2idle_dev_ops *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	[flat|nested] 8+ messages in thread

* RE: [PATCH v5 1/4] ACPI / x86: Add support for LPS0 callback handler
  2022-03-17 16:45 ` [PATCH v5 1/4] ACPI / x86: Add support for LPS0 callback handler Rafael J. Wysocki
@ 2022-03-17 16:49   ` Limonciello, Mario
  2022-03-17 16:55     ` Hans de Goede
  0 siblings, 1 reply; 8+ messages in thread
From: Limonciello, Mario @ 2022-03-17 16:49 UTC (permalink / raw)
  To: Rafael J. Wysocki, Hans de Goede
  Cc: Mark Gross, Rafael J . Wysocki, open list:X86 PLATFORM DRIVERS,
	ACPI Devel Maling List, S-k, Shyam-sundar, Goswami, Sanket

[Public]



> -----Original Message-----
> From: Rafael J. Wysocki <rafael@kernel.org>
> Sent: Thursday, March 17, 2022 11:46
> 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>; ACPI
> Devel Maling List <linux-acpi@vger.kernel.org>; S-k, Shyam-sundar <Shyam-
> sundar.S-k@amd.com>; Goswami, Sanket <Sanket.Goswami@amd.com>
> Subject: Re: [PATCH v5 1/4] ACPI / x86: Add support for LPS0 callback handler
> 
> On Thu, Mar 17, 2022 at 3:15 PM Mario Limonciello
> <mario.limonciello@amd.com> 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>
> 
> This looks good to me, so:
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 

Thanks!

> and I don't have any conflicting changes queued up for 5.17, so from
> my POV it can go in via the x86 platform drivers tree along with the
> other patches in the series.

Hans,

FYI the series is based off of platform-x86/for-next also with changes to amd-pmc
Rafael doesn't have in his tree, so I also agree it's better to go through x86 platform
drivers.

> 
> > ---
> > changes from v4->v5:
> > * fix a check for handler->prepare
> >   Reported by Dan Carpenter <dan.carpenter@oracle.com>
> > changes from v3->v4:
> > * drop use of mutex, use lock_system_sleep instead
> > * don't pass argument of context, driver will maintain this internally
> > * don't allow failing prepare stage
> > * don't allow unregistering if sleep_no_lps0 is set
> > 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 | 40
> +++++++++++++++++++++++++++++++++++++++
> >  include/linux/acpi.h      | 10 +++++++++-
> >  2 files changed, 49 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> > index abc06e7f89d8..031b20a547f9 100644
> > --- a/drivers/acpi/x86/s2idle.c
> > +++ b/drivers/acpi/x86/s2idle.c
> > @@ -86,6 +86,8 @@ struct lpi_device_constraint_amd {
> >         int min_dstate;
> >  };
> >
> > +static LIST_HEAD(lps0_s2idle_devops_head);
> > +
> >  static struct lpi_constraints *lpi_constraints_table;
> >  static int lpi_constraints_table_size;
> >  static int rev_id;
> > @@ -444,6 +446,8 @@ static struct acpi_scan_handler lps0_handler = {
> >
> >  int acpi_s2idle_prepare_late(void)
> >  {
> > +       struct acpi_s2idle_dev_ops *handler;
> > +
> >         if (!lps0_device_handle || sleep_no_lps0)
> >                 return 0;
> >
> > @@ -474,14 +478,26 @@ int acpi_s2idle_prepare_late(void)
> >                 acpi_sleep_run_lps0_dsm(ACPI_LPS0_MS_ENTRY,
> >                                 lps0_dsm_func_mask_microsoft,
> lps0_dsm_guid_microsoft);
> >         }
> > +
> > +       list_for_each_entry(handler, &lps0_s2idle_devops_head, list_node) {
> > +               if (handler->prepare)
> > +                       handler->prepare();
> > +       }
> > +
> >         return 0;
> >  }
> >
> >  void acpi_s2idle_restore_early(void)
> >  {
> > +       struct acpi_s2idle_dev_ops *handler;
> > +
> >         if (!lps0_device_handle || sleep_no_lps0)
> >                 return;
> >
> > +       list_for_each_entry(handler, &lps0_s2idle_devops_head, list_node)
> > +               if (handler->restore)
> > +                       handler->restore();
> > +
> >         /* Modern standby exit */
> >         if (lps0_dsm_func_mask_microsoft > 0)
> >                 acpi_sleep_run_lps0_dsm(ACPI_LPS0_MS_EXIT,
> > @@ -524,4 +540,28 @@ void acpi_s2idle_setup(void)
> >         s2idle_set_ops(&acpi_s2idle_ops_lps0);
> >  }
> >
> > +int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg)
> > +{
> > +       if (!lps0_device_handle || sleep_no_lps0)
> > +               return -ENODEV;
> > +
> > +       lock_system_sleep();
> > +       list_add(&arg->list_node, &lps0_s2idle_devops_head);
> > +       unlock_system_sleep();
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(acpi_register_lps0_dev);
> > +
> > +void acpi_unregister_lps0_dev(struct acpi_s2idle_dev_ops *arg)
> > +{
> > +       if (!lps0_device_handle || sleep_no_lps0)
> > +               return;
> > +
> > +       lock_system_sleep();
> > +       list_del(&arg->list_node);
> > +       unlock_system_sleep();
> > +}
> > +EXPORT_SYMBOL_GPL(acpi_unregister_lps0_dev);
> > +
> >  #endif /* CONFIG_SUSPEND */
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index 6274758648e3..47c16cdc8e0e 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -1023,7 +1023,15 @@ 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 acpi_s2idle_dev_ops {
> > +       struct list_head list_node;
> > +       void (*prepare)(void);
> > +       void (*restore)(void);
> > +};
> > +int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg);
> > +void acpi_unregister_lps0_dev(struct acpi_s2idle_dev_ops *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	[flat|nested] 8+ messages in thread

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

Hi All,

On 3/17/22 17:49, Limonciello, Mario wrote:
> [Public]
> 
> 
> 
>> -----Original Message-----
>> From: Rafael J. Wysocki <rafael@kernel.org>
>> Sent: Thursday, March 17, 2022 11:46
>> 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>; ACPI
>> Devel Maling List <linux-acpi@vger.kernel.org>; S-k, Shyam-sundar <Shyam-
>> sundar.S-k@amd.com>; Goswami, Sanket <Sanket.Goswami@amd.com>
>> Subject: Re: [PATCH v5 1/4] ACPI / x86: Add support for LPS0 callback handler
>>
>> On Thu, Mar 17, 2022 at 3:15 PM Mario Limonciello
>> <mario.limonciello@amd.com> 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>
>>
>> This looks good to me, so:
>>
>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
> 
> Thanks!
> 
>> and I don't have any conflicting changes queued up for 5.17, so from
>> my POV it can go in via the x86 platform drivers tree along with the
>> other patches in the series.
> 
> Hans,
> 
> FYI the series is based off of platform-x86/for-next also with changes to amd-pmc
> Rafael doesn't have in his tree, so I also agree it's better to go through x86 platform
> drivers.

Ok, the entire series looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

So I'll go and merge this into review-hans, do some compile
tests and then push this directly to for-next to give it some
minimum baking time there before the 5.18 merge-window opens.

Regards,

Hans



> 
>>
>>> ---
>>> changes from v4->v5:
>>> * fix a check for handler->prepare
>>>   Reported by Dan Carpenter <dan.carpenter@oracle.com>
>>> changes from v3->v4:
>>> * drop use of mutex, use lock_system_sleep instead
>>> * don't pass argument of context, driver will maintain this internally
>>> * don't allow failing prepare stage
>>> * don't allow unregistering if sleep_no_lps0 is set
>>> 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 | 40
>> +++++++++++++++++++++++++++++++++++++++
>>>  include/linux/acpi.h      | 10 +++++++++-
>>>  2 files changed, 49 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
>>> index abc06e7f89d8..031b20a547f9 100644
>>> --- a/drivers/acpi/x86/s2idle.c
>>> +++ b/drivers/acpi/x86/s2idle.c
>>> @@ -86,6 +86,8 @@ struct lpi_device_constraint_amd {
>>>         int min_dstate;
>>>  };
>>>
>>> +static LIST_HEAD(lps0_s2idle_devops_head);
>>> +
>>>  static struct lpi_constraints *lpi_constraints_table;
>>>  static int lpi_constraints_table_size;
>>>  static int rev_id;
>>> @@ -444,6 +446,8 @@ static struct acpi_scan_handler lps0_handler = {
>>>
>>>  int acpi_s2idle_prepare_late(void)
>>>  {
>>> +       struct acpi_s2idle_dev_ops *handler;
>>> +
>>>         if (!lps0_device_handle || sleep_no_lps0)
>>>                 return 0;
>>>
>>> @@ -474,14 +478,26 @@ int acpi_s2idle_prepare_late(void)
>>>                 acpi_sleep_run_lps0_dsm(ACPI_LPS0_MS_ENTRY,
>>>                                 lps0_dsm_func_mask_microsoft,
>> lps0_dsm_guid_microsoft);
>>>         }
>>> +
>>> +       list_for_each_entry(handler, &lps0_s2idle_devops_head, list_node) {
>>> +               if (handler->prepare)
>>> +                       handler->prepare();
>>> +       }
>>> +
>>>         return 0;
>>>  }
>>>
>>>  void acpi_s2idle_restore_early(void)
>>>  {
>>> +       struct acpi_s2idle_dev_ops *handler;
>>> +
>>>         if (!lps0_device_handle || sleep_no_lps0)
>>>                 return;
>>>
>>> +       list_for_each_entry(handler, &lps0_s2idle_devops_head, list_node)
>>> +               if (handler->restore)
>>> +                       handler->restore();
>>> +
>>>         /* Modern standby exit */
>>>         if (lps0_dsm_func_mask_microsoft > 0)
>>>                 acpi_sleep_run_lps0_dsm(ACPI_LPS0_MS_EXIT,
>>> @@ -524,4 +540,28 @@ void acpi_s2idle_setup(void)
>>>         s2idle_set_ops(&acpi_s2idle_ops_lps0);
>>>  }
>>>
>>> +int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg)
>>> +{
>>> +       if (!lps0_device_handle || sleep_no_lps0)
>>> +               return -ENODEV;
>>> +
>>> +       lock_system_sleep();
>>> +       list_add(&arg->list_node, &lps0_s2idle_devops_head);
>>> +       unlock_system_sleep();
>>> +
>>> +       return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(acpi_register_lps0_dev);
>>> +
>>> +void acpi_unregister_lps0_dev(struct acpi_s2idle_dev_ops *arg)
>>> +{
>>> +       if (!lps0_device_handle || sleep_no_lps0)
>>> +               return;
>>> +
>>> +       lock_system_sleep();
>>> +       list_del(&arg->list_node);
>>> +       unlock_system_sleep();
>>> +}
>>> +EXPORT_SYMBOL_GPL(acpi_unregister_lps0_dev);
>>> +
>>>  #endif /* CONFIG_SUSPEND */
>>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>>> index 6274758648e3..47c16cdc8e0e 100644
>>> --- a/include/linux/acpi.h
>>> +++ b/include/linux/acpi.h
>>> @@ -1023,7 +1023,15 @@ 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 acpi_s2idle_dev_ops {
>>> +       struct list_head list_node;
>>> +       void (*prepare)(void);
>>> +       void (*restore)(void);
>>> +};
>>> +int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg);
>>> +void acpi_unregister_lps0_dev(struct acpi_s2idle_dev_ops *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	[flat|nested] 8+ messages in thread

* Re: [PATCH v5 4/4] platform/x86: amd-pmc: Drop CPU QoS workaround
  2022-03-17 14:14 ` [PATCH v5 4/4] platform/x86: amd-pmc: Drop CPU QoS workaround Mario Limonciello
@ 2022-03-17 18:54   ` Hans de Goede
  0 siblings, 0 replies; 8+ messages in thread
From: Hans de Goede @ 2022-03-17 18:54 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,

On 3/17/22 15:14, Mario Limonciello wrote:
> 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 v4->v5:
>  * rebase on earlier patches
> changes from v3->v4:
>  * rebase on earlier patches
> changes from v2->v3:
>  * No changes
> changes from v1->v2:
>  * No changes
> 
>  drivers/platform/x86/amd-pmc.c | 32 +++-----------------------------
>  1 file changed, 3 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
> index f36cf125b284..7317993cd91b 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;
>  }
>  
> @@ -655,7 +642,7 @@ static void amd_pmc_s2idle_prepare(void)
>  		rc = amd_pmc_verify_czn_rtc(pdev, &arg);
>  		if (rc) {
>  			dev_err(pdev->dev, "failed to set RTC: %d\n", rc);
> -			goto fail;
> +			return;
>  		}
>  	}
>  
> @@ -665,20 +652,13 @@ static void amd_pmc_s2idle_prepare(void)
>  	rc = amd_pmc_send_cmd(pdev, arg, NULL, msg, 0);
>  	if (rc) {
>  		dev_err(pdev->dev, "suspend failed: %d\n", rc);
> -		goto fail;
> +		return;
>  	}
>  
>  	if (enable_stb)
>  		rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_PREDEF);
> -	if (rc) {
> +	if (rc)
>  		dev_err(pdev->dev, "error writing to STB: %d\n", rc);
> -		goto fail;
> -	}

Ok, this entire series has been merged here now:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=for-next

now lets hope none of the various build tests become unhappy because of this.

Mario, one request, the:

	if (rc)
		dev_err(pdev->dev, "error writing to STB: %d\n", rc);

above looks weird, can you do a follow-up patch moving that to
inside/under the "if (enable_stb)" check ?

Currently it also rechecks the amd_pmc_verify_czn_rtc() when
enable_stb is false, which is weird.

Regards,

Hans





> -	return;
> -fail:
> -	if (pdev->cpu_id == AMD_CPU_ID_CZN)
> -		cpu_latency_qos_update_request(&pdev->amd_pmc_pm_qos_req,
> -						PM_QOS_DEFAULT_VALUE);
>  }
>  
>  static void amd_pmc_s2idle_restore(void)



> @@ -704,11 +684,6 @@ static void amd_pmc_s2idle_restore(void)
>  	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);
>  }
> @@ -887,7 +862,6 @@ static int amd_pmc_probe(struct platform_device *pdev)
>  		dev_warn(dev->dev, "failed to register LPS0 sleep handler, expect increased power consumption\n");
>  
>  	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:


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

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

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-17 14:14 [PATCH v5 1/4] ACPI / x86: Add support for LPS0 callback handler Mario Limonciello
2022-03-17 14:14 ` [PATCH v5 2/4] platform/x86: amd-pmc: Move to later in the suspend process Mario Limonciello
2022-03-17 14:14 ` [PATCH v5 3/4] platform/x86: amd-pmc: Output error codes in messages Mario Limonciello
2022-03-17 14:14 ` [PATCH v5 4/4] platform/x86: amd-pmc: Drop CPU QoS workaround Mario Limonciello
2022-03-17 18:54   ` Hans de Goede
2022-03-17 16:45 ` [PATCH v5 1/4] ACPI / x86: Add support for LPS0 callback handler Rafael J. Wysocki
2022-03-17 16:49   ` Limonciello, Mario
2022-03-17 16:55     ` Hans de Goede

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).