All uses in the kernel are currently already oriented around
suspend/resume. As some other parts of the kernel may also use these
messages in functions that could also be used outside of
suspend/resume, only enable in suspend/resume path.
Signed-off-by: Mario Limonciello <[email protected]>
---
v3->v4:
* add back do/while as it wasn't pointless. It fixes a warning.
---
include/linux/suspend.h | 8 +++++---
kernel/power/main.c | 6 ++++++
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 1a0426e6761c..74f406c53ac0 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -555,6 +555,7 @@ static inline void unlock_system_sleep(unsigned int flags) {}
#ifdef CONFIG_PM_SLEEP_DEBUG
extern bool pm_print_times_enabled;
extern bool pm_debug_messages_on;
+extern bool pm_debug_messages_should_print(void);
static inline int pm_dyn_debug_messages_on(void)
{
#ifdef CONFIG_DYNAMIC_DEBUG
@@ -568,14 +569,14 @@ static inline int pm_dyn_debug_messages_on(void)
#endif
#define __pm_pr_dbg(fmt, ...) \
do { \
- if (pm_debug_messages_on) \
+ if (pm_debug_messages_should_print()) \
printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \
else if (pm_dyn_debug_messages_on()) \
pr_debug(fmt, ##__VA_ARGS__); \
} while (0)
#define __pm_deferred_pr_dbg(fmt, ...) \
do { \
- if (pm_debug_messages_on) \
+ if (pm_debug_messages_should_print()) \
printk_deferred(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \
} while (0)
#else
@@ -593,7 +594,8 @@ static inline int pm_dyn_debug_messages_on(void)
/**
* pm_pr_dbg - print pm sleep debug messages
*
- * If pm_debug_messages_on is enabled, print message.
+ * If pm_debug_messages_on is enabled and the system is entering/leaving
+ * suspend, print message.
* If pm_debug_messages_on is disabled and CONFIG_DYNAMIC_DEBUG is enabled,
* print message only from instances explicitly enabled on dynamic debug's
* control.
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 3113ec2f1db4..daa535012e51 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -556,6 +556,12 @@ power_attr_ro(pm_wakeup_irq);
bool pm_debug_messages_on __read_mostly;
+bool pm_debug_messages_should_print(void)
+{
+ return pm_debug_messages_on && pm_suspend_target_state != PM_SUSPEND_ON;
+}
+EXPORT_SYMBOL_GPL(pm_debug_messages_should_print);
+
static ssize_t pm_debug_messages_show(struct kobject *kobj,
struct kobj_attribute *attr, char *buf)
{
--
2.34.1
Enabling debugging messages for the state requires turning on dynamic
debugging for the file. To make it more accessible, use
`pm_debug_messages` and clearer strings for what is happening.
Signed-off-by: Mario Limonciello <[email protected]>
---
drivers/acpi/x86/s2idle.c | 52 ++++++++++++++++++++++++++++++++++-----
1 file changed, 46 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index e499c60c4579..7681f6ecab67 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -59,6 +59,7 @@ static int lps0_dsm_func_mask;
static guid_t lps0_dsm_guid_microsoft;
static int lps0_dsm_func_mask_microsoft;
+static int lps0_dsm_state;
/* Device constraint entry structure */
struct lpi_device_info {
@@ -320,6 +321,44 @@ static void lpi_check_constraints(void)
}
}
+static bool acpi_s2idle_vendor_amd(void)
+{
+ return boot_cpu_data.x86_vendor == X86_VENDOR_AMD;
+}
+
+static const char *acpi_sleep_dsm_state_to_str(unsigned int state)
+{
+ if (lps0_dsm_func_mask_microsoft || !acpi_s2idle_vendor_amd()) {
+ switch (state) {
+ case ACPI_LPS0_SCREEN_OFF:
+ return "screen off";
+ case ACPI_LPS0_SCREEN_ON:
+ return "screen on";
+ case ACPI_LPS0_ENTRY:
+ return "lps0 entry";
+ case ACPI_LPS0_EXIT:
+ return "lps0 exit";
+ case ACPI_LPS0_MS_ENTRY:
+ return "lps0 ms entry";
+ case ACPI_LPS0_MS_EXIT:
+ return "lps0 ms exit";
+ }
+ } else {
+ switch (state) {
+ case ACPI_LPS0_SCREEN_ON_AMD:
+ return "screen on";
+ case ACPI_LPS0_SCREEN_OFF_AMD:
+ return "screen off";
+ case ACPI_LPS0_ENTRY_AMD:
+ return "lps0 entry";
+ case ACPI_LPS0_EXIT_AMD:
+ return "lps0 exit";
+ }
+ }
+
+ return "unknown";
+}
+
static void acpi_sleep_run_lps0_dsm(unsigned int func, unsigned int func_mask, guid_t dsm_guid)
{
union acpi_object *out_obj;
@@ -331,14 +370,15 @@ static void acpi_sleep_run_lps0_dsm(unsigned int func, unsigned int func_mask, g
rev_id, func, NULL);
ACPI_FREE(out_obj);
- acpi_handle_debug(lps0_device_handle, "_DSM function %u evaluation %s\n",
- func, out_obj ? "successful" : "failed");
+ lps0_dsm_state = func;
+ if (pm_debug_messages_on) {
+ acpi_handle_info(lps0_device_handle,
+ "%s transitioned to state %s\n",
+ out_obj ? "Successfully" : "Failed to",
+ acpi_sleep_dsm_state_to_str(lps0_dsm_state));
+ }
}
-static bool acpi_s2idle_vendor_amd(void)
-{
- return boot_cpu_data.x86_vendor == X86_VENDOR_AMD;
-}
static int validate_dsm(acpi_handle handle, const char *uuid, int rev, guid_t *dsm_guid)
{
--
2.34.1
Using pm_pr_dbg() allows users to toggle `/sys/power/pm_debug_messages`
as a single knob to turn on messages that amd-pmc can emit to aid in
any s2idle debugging.
Signed-off-by: Mario Limonciello <[email protected]>
---
drivers/platform/x86/amd/pmc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
index 427905714f79..1304cd6f13f6 100644
--- a/drivers/platform/x86/amd/pmc.c
+++ b/drivers/platform/x86/amd/pmc.c
@@ -543,7 +543,7 @@ static int amd_pmc_idlemask_read(struct amd_pmc_dev *pdev, struct device *dev,
}
if (dev)
- dev_dbg(pdev->dev, "SMU idlemask s0i3: 0x%x\n", val);
+ pm_pr_dbg("SMU idlemask s0i3: 0x%x\n", val);
if (s)
seq_printf(s, "SMU idlemask : 0x%x\n", val);
@@ -769,7 +769,7 @@ static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
*arg |= (duration << 16);
rc = rtc_alarm_irq_enable(rtc_device, 0);
- dev_dbg(pdev->dev, "wakeup timer programmed for %lld seconds\n", duration);
+ pm_pr_dbg("wakeup timer programmed for %lld seconds\n", duration);
return rc;
}
--
2.34.1
To make the GPIO tracking around suspend easier for end users to
use, link it with pm_debug_messages. This will make discovering
sources of spurious GPIOs around suspend easier.
Signed-off-by: Mario Limonciello <[email protected]>
---
drivers/pinctrl/pinctrl-amd.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index f279b360c20d..43d3530bab48 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -30,6 +30,7 @@
#include <linux/pinctrl/pinconf.h>
#include <linux/pinctrl/pinconf-generic.h>
#include <linux/pinctrl/pinmux.h>
+#include <linux/suspend.h>
#include "core.h"
#include "pinctrl-utils.h"
@@ -636,9 +637,8 @@ static bool do_amd_gpio_irq_handler(int irq, void *dev_id)
regval = readl(regs + i);
if (regval & PIN_IRQ_PENDING)
- dev_dbg(&gpio_dev->pdev->dev,
- "GPIO %d is active: 0x%x",
- irqnr + i, regval);
+ pm_pr_dbg("GPIO %d is active: 0x%x",
+ irqnr + i, regval);
/* caused wake on resume context for shared IRQ */
if (irq < 0 && (regval & BIT(WAKE_STS_OFF)))
--
2.34.1
On Fri, Jun 2, 2023 at 9:32 PM Mario Limonciello
<[email protected]> wrote:
>
> Enabling debugging messages for the state requires turning on dynamic
> debugging for the file. To make it more accessible, use
> `pm_debug_messages` and clearer strings for what is happening.
>
> Signed-off-by: Mario Limonciello <[email protected]>
I'm inclined to apply this one and the [1/4] at this point.
I can also apply the 2 remaining patches in this series if I get ACKs
for them from the respective subsystem maintainers.
> ---
> drivers/acpi/x86/s2idle.c | 52 ++++++++++++++++++++++++++++++++++-----
> 1 file changed, 46 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> index e499c60c4579..7681f6ecab67 100644
> --- a/drivers/acpi/x86/s2idle.c
> +++ b/drivers/acpi/x86/s2idle.c
> @@ -59,6 +59,7 @@ static int lps0_dsm_func_mask;
>
> static guid_t lps0_dsm_guid_microsoft;
> static int lps0_dsm_func_mask_microsoft;
> +static int lps0_dsm_state;
>
> /* Device constraint entry structure */
> struct lpi_device_info {
> @@ -320,6 +321,44 @@ static void lpi_check_constraints(void)
> }
> }
>
> +static bool acpi_s2idle_vendor_amd(void)
> +{
> + return boot_cpu_data.x86_vendor == X86_VENDOR_AMD;
> +}
> +
> +static const char *acpi_sleep_dsm_state_to_str(unsigned int state)
> +{
> + if (lps0_dsm_func_mask_microsoft || !acpi_s2idle_vendor_amd()) {
> + switch (state) {
> + case ACPI_LPS0_SCREEN_OFF:
> + return "screen off";
> + case ACPI_LPS0_SCREEN_ON:
> + return "screen on";
> + case ACPI_LPS0_ENTRY:
> + return "lps0 entry";
> + case ACPI_LPS0_EXIT:
> + return "lps0 exit";
> + case ACPI_LPS0_MS_ENTRY:
> + return "lps0 ms entry";
> + case ACPI_LPS0_MS_EXIT:
> + return "lps0 ms exit";
> + }
> + } else {
> + switch (state) {
> + case ACPI_LPS0_SCREEN_ON_AMD:
> + return "screen on";
> + case ACPI_LPS0_SCREEN_OFF_AMD:
> + return "screen off";
> + case ACPI_LPS0_ENTRY_AMD:
> + return "lps0 entry";
> + case ACPI_LPS0_EXIT_AMD:
> + return "lps0 exit";
> + }
> + }
> +
> + return "unknown";
> +}
> +
> static void acpi_sleep_run_lps0_dsm(unsigned int func, unsigned int func_mask, guid_t dsm_guid)
> {
> union acpi_object *out_obj;
> @@ -331,14 +370,15 @@ static void acpi_sleep_run_lps0_dsm(unsigned int func, unsigned int func_mask, g
> rev_id, func, NULL);
> ACPI_FREE(out_obj);
>
> - acpi_handle_debug(lps0_device_handle, "_DSM function %u evaluation %s\n",
> - func, out_obj ? "successful" : "failed");
> + lps0_dsm_state = func;
> + if (pm_debug_messages_on) {
> + acpi_handle_info(lps0_device_handle,
> + "%s transitioned to state %s\n",
> + out_obj ? "Successfully" : "Failed to",
> + acpi_sleep_dsm_state_to_str(lps0_dsm_state));
> + }
> }
>
> -static bool acpi_s2idle_vendor_amd(void)
> -{
> - return boot_cpu_data.x86_vendor == X86_VENDOR_AMD;
> -}
>
> static int validate_dsm(acpi_handle handle, const char *uuid, int rev, guid_t *dsm_guid)
> {
> --
> 2.34.1
>
Hi,
On 6/2/23 09:30, Mario Limonciello wrote:
> Using pm_pr_dbg() allows users to toggle `/sys/power/pm_debug_messages`
> as a single knob to turn on messages that amd-pmc can emit to aid in
> any s2idle debugging.
>
> Signed-off-by: Mario Limonciello <[email protected]>
Thanks, patch looks good to me.
Here is my ack for merging this through the linux-pm tree:
Acked-by: Hans de Goede <[email protected]>
Regards,
Hans
> ---
> drivers/platform/x86/amd/pmc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
> index 427905714f79..1304cd6f13f6 100644
> --- a/drivers/platform/x86/amd/pmc.c
> +++ b/drivers/platform/x86/amd/pmc.c
> @@ -543,7 +543,7 @@ static int amd_pmc_idlemask_read(struct amd_pmc_dev *pdev, struct device *dev,
> }
>
> if (dev)
> - dev_dbg(pdev->dev, "SMU idlemask s0i3: 0x%x\n", val);
> + pm_pr_dbg("SMU idlemask s0i3: 0x%x\n", val);
>
> if (s)
> seq_printf(s, "SMU idlemask : 0x%x\n", val);
> @@ -769,7 +769,7 @@ static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
>
> *arg |= (duration << 16);
> rc = rtc_alarm_irq_enable(rtc_device, 0);
> - dev_dbg(pdev->dev, "wakeup timer programmed for %lld seconds\n", duration);
> + pm_pr_dbg("wakeup timer programmed for %lld seconds\n", duration);
>
> return rc;
> }
On Fri, Jun 2, 2023 at 9:32 PM Mario Limonciello
<[email protected]> wrote:
> To make the GPIO tracking around suspend easier for end users to
> use, link it with pm_debug_messages. This will make discovering
> sources of spurious GPIOs around suspend easier.
>
> Signed-off-by: Mario Limonciello <[email protected]>
Acked-by: Linus Walleij <[email protected]>
If the PM people merge the other patches they can take this too because
of the dependency.
Yours,
Linus Walleij
On Sun, Jun 4, 2023 at 6:04 PM Rafael J. Wysocki <[email protected]> wrote:
>
> On Fri, Jun 2, 2023 at 9:32 PM Mario Limonciello
> <[email protected]> wrote:
> >
> > Enabling debugging messages for the state requires turning on dynamic
> > debugging for the file. To make it more accessible, use
> > `pm_debug_messages` and clearer strings for what is happening.
> >
> > Signed-off-by: Mario Limonciello <[email protected]>
>
> I'm inclined to apply this one and the [1/4] at this point.
>
> I can also apply the 2 remaining patches in this series if I get ACKs
> for them from the respective subsystem maintainers.
The ACKs were provided, so the entire series has been applied as 6.5
material, thanks!
On 4/7/24 5:49 AM, xiongxin wrote:
> From: Mario Limonciello <[email protected]>
>
> All uses in the kernel are currently already oriented around
> suspend/resume. As some other parts of the kernel may also use these
> messages in functions that could also be used outside of
> suspend/resume, only enable in suspend/resume path.
>
> Signed-off-by: Mario Limonciello <[email protected]>
> ---
> v3->v4:
> * add back do/while as it wasn't pointless. It fixes a warning.
> ---
> include/linux/suspend.h | 8 +++++---
> kernel/power/main.c | 6 ++++++
> 2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 1a0426e6761c..74f406c53ac0 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -555,6 +555,7 @@ static inline void unlock_system_sleep(unsigned int flags) {}
> #ifdef CONFIG_PM_SLEEP_DEBUG
> extern bool pm_print_times_enabled;
> extern bool pm_debug_messages_on;
> +extern bool pm_debug_messages_should_print(void);
> static inline int pm_dyn_debug_messages_on(void)
> {
> #ifdef CONFIG_DYNAMIC_DEBUG
> @@ -568,14 +569,14 @@ static inline int pm_dyn_debug_messages_on(void)
> #endif
> #define __pm_pr_dbg(fmt, ...) \
> do { \
> - if (pm_debug_messages_on) \
> + if (pm_debug_messages_should_print()) \
> printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \
> else if (pm_dyn_debug_messages_on()) \
> pr_debug(fmt, ##__VA_ARGS__); \
> } while (0)
> #define __pm_deferred_pr_dbg(fmt, ...) \
> do { \
> - if (pm_debug_messages_on) \
> + if (pm_debug_messages_should_print()) \
> printk_deferred(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \
> } while (0)
> #else
> @@ -593,7 +594,8 @@ static inline int pm_dyn_debug_messages_on(void)
> /**
> * pm_pr_dbg - print pm sleep debug messages
> *
> - * If pm_debug_messages_on is enabled, print message.
> + * If pm_debug_messages_on is enabled and the system is entering/leaving
> + * suspend, print message.
> * If pm_debug_messages_on is disabled and CONFIG_DYNAMIC_DEBUG is enabled,
> * print message only from instances explicitly enabled on dynamic debug's
> * control.
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 3113ec2f1db4..daa535012e51 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -556,6 +556,12 @@ power_attr_ro(pm_wakeup_irq);
>
> bool pm_debug_messages_on __read_mostly;
>
> +bool pm_debug_messages_should_print(void)
> +{
> + return pm_debug_messages_on && pm_suspend_target_state != PM_SUSPEND_ON;
>
>> hibernate processes also mostly use the pm_pr_dbg() function, which
>> results in hibernate processes only being able to output such logs
>> through dynamic debug, which is unfriendly to kernels without
>> CONFIG_DYNAMIC_DEBUG configuration.
This part of the patch doesn't look so good. ^^^^^^^^^^^^^^^^^^^^
>
> +}
> +EXPORT_SYMBOL_GPL(pm_debug_messages_should_print);
> +
> static ssize_t pm_debug_messages_show(struct kobject *kobj,
> struct kobj_attribute *attr, char *buf)
> {
>
--
#Randy