[PATCH] Revert "include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume"

[PATCH] Revert "include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume"

[xiongxin]

This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce.

In the suspend process, pm_pr_dbg() is called before setting
pm_suspend_target_state. As a result, this part of the log cannot be
output.

pm_pr_dbg() also outputs debug logs for hibernate, but
pm_suspend_target_state is not set, resulting in hibernate debug logs
can only be output through dynamic debug, which is very inconvenient.

Signed-off-by: xiongxin <xiongxin@kylinos.cn>

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index da6ebca3ff77..415483b89b11 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -503,7 +503,6 @@ 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
@@ -517,14 +516,14 @@ static inline int pm_dyn_debug_messages_on(void)
 #endif
 #define __pm_pr_dbg(fmt, ...)					\
 	do {							\
-		if (pm_debug_messages_should_print())		\
+		if (pm_debug_messages_on)			\
 			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_should_print())		\
+		if (pm_debug_messages_on)			\
 			printk_deferred(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__);	\
 	} while (0)
 #else
@@ -542,8 +541,7 @@ static inline int pm_dyn_debug_messages_on(void)
 /**
  * pm_pr_dbg - print pm sleep debug messages
  *
- * If pm_debug_messages_on is enabled and the system is entering/leaving
- *      suspend, print message.
+ * If pm_debug_messages_on is enabled, 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 a9e0693aaf69..aa754241aaa6 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -611,12 +611,6 @@ 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

Re: [PATCH] Revert "include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume"

[Mario Limonciello]

On 4/22/2024 04:36, xiongxin wrote:
> This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce.
> 
> In the suspend process, pm_pr_dbg() is called before setting
> pm_suspend_target_state. As a result, this part of the log cannot be
> output.
> 
> pm_pr_dbg() also outputs debug logs for hibernate, but
> pm_suspend_target_state is not set, resulting in hibernate debug logs
> can only be output through dynamic debug, which is very inconvenient.

As an alternative, how about exporting and renaming the variable 
in_suspend in kernel/power/hibernate.c and considering that to tell if 
the hibernate process is going on?

Then it should work just the same as it does at suspend.

> 
> Signed-off-by: xiongxin <xiongxin@kylinos.cn>
> 
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index da6ebca3ff77..415483b89b11 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -503,7 +503,6 @@ 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
> @@ -517,14 +516,14 @@ static inline int pm_dyn_debug_messages_on(void)
>   #endif
>   #define __pm_pr_dbg(fmt, ...)					\
>   	do {							\
> -		if (pm_debug_messages_should_print())		\
> +		if (pm_debug_messages_on)			\
>   			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_should_print())		\
> +		if (pm_debug_messages_on)			\
>   			printk_deferred(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__);	\
>   	} while (0)
>   #else
> @@ -542,8 +541,7 @@ static inline int pm_dyn_debug_messages_on(void)
>   /**
>    * pm_pr_dbg - print pm sleep debug messages
>    *
> - * If pm_debug_messages_on is enabled and the system is entering/leaving
> - *      suspend, print message.
> + * If pm_debug_messages_on is enabled, 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 a9e0693aaf69..aa754241aaa6 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -611,12 +611,6 @@ 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)
>   {

Re: [PATCH] Revert "include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume"

[Rafael J. Wysocki]

On Mon, Apr 22, 2024 at 4:33 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 4/22/2024 04:36, xiongxin wrote:
> > This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce.
> >
> > In the suspend process, pm_pr_dbg() is called before setting
> > pm_suspend_target_state. As a result, this part of the log cannot be
> > output.
> >
> > pm_pr_dbg() also outputs debug logs for hibernate, but
> > pm_suspend_target_state is not set, resulting in hibernate debug logs
> > can only be output through dynamic debug, which is very inconvenient.
>
> As an alternative, how about exporting and renaming the variable
> in_suspend in kernel/power/hibernate.c and considering that to tell if
> the hibernate process is going on?
>
> Then it should work just the same as it does at suspend.

Well, this is not the only part that stopped working AFAICS.  I'll
queue up the revert.

Thanks!

> >
> > Signed-off-by: xiongxin <xiongxin@kylinos.cn>
> >
> > diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > index da6ebca3ff77..415483b89b11 100644
> > --- a/include/linux/suspend.h
> > +++ b/include/linux/suspend.h
> > @@ -503,7 +503,6 @@ 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
> > @@ -517,14 +516,14 @@ static inline int pm_dyn_debug_messages_on(void)
> >   #endif
> >   #define __pm_pr_dbg(fmt, ...)                                       \
> >       do {                                                    \
> > -             if (pm_debug_messages_should_print())           \
> > +             if (pm_debug_messages_on)                       \
> >                       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_should_print())           \
> > +             if (pm_debug_messages_on)                       \
> >                       printk_deferred(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \
> >       } while (0)
> >   #else
> > @@ -542,8 +541,7 @@ static inline int pm_dyn_debug_messages_on(void)
> >   /**
> >    * pm_pr_dbg - print pm sleep debug messages
> >    *
> > - * If pm_debug_messages_on is enabled and the system is entering/leaving
> > - *      suspend, print message.
> > + * If pm_debug_messages_on is enabled, 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 a9e0693aaf69..aa754241aaa6 100644
> > --- a/kernel/power/main.c
> > +++ b/kernel/power/main.c
> > @@ -611,12 +611,6 @@ 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)
> >   {
>

Re: [PATCH] Revert "include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume"

[Mario Limonciello]

On 4/22/2024 09:45, Rafael J. Wysocki wrote:
> On Mon, Apr 22, 2024 at 4:33 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> On 4/22/2024 04:36, xiongxin wrote:
>>> This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce.
>>>
>>> In the suspend process, pm_pr_dbg() is called before setting
>>> pm_suspend_target_state. As a result, this part of the log cannot be
>>> output.
>>>
>>> pm_pr_dbg() also outputs debug logs for hibernate, but
>>> pm_suspend_target_state is not set, resulting in hibernate debug logs
>>> can only be output through dynamic debug, which is very inconvenient.
>>
>> As an alternative, how about exporting and renaming the variable
>> in_suspend in kernel/power/hibernate.c and considering that to tell if
>> the hibernate process is going on?
>>
>> Then it should work just the same as it does at suspend.
> 
> Well, this is not the only part that stopped working AFAICS.  I'll
> queue up the revert.

I just tested the revert to see what happens to other drivers but it's 
going to have more collateral damage.

ERROR: modpost: "pm_debug_messages_on" 
[drivers/platform/x86/amd/pmc/amd-pmc.ko] undefined!

> 
> Thanks!
> 
>>>
>>> Signed-off-by: xiongxin <xiongxin@kylinos.cn>
>>>
>>> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
>>> index da6ebca3ff77..415483b89b11 100644
>>> --- a/include/linux/suspend.h
>>> +++ b/include/linux/suspend.h
>>> @@ -503,7 +503,6 @@ 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
>>> @@ -517,14 +516,14 @@ static inline int pm_dyn_debug_messages_on(void)
>>>    #endif
>>>    #define __pm_pr_dbg(fmt, ...)                                       \
>>>        do {                                                    \
>>> -             if (pm_debug_messages_should_print())           \
>>> +             if (pm_debug_messages_on)                       \
>>>                        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_should_print())           \
>>> +             if (pm_debug_messages_on)                       \
>>>                        printk_deferred(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \
>>>        } while (0)
>>>    #else
>>> @@ -542,8 +541,7 @@ static inline int pm_dyn_debug_messages_on(void)
>>>    /**
>>>     * pm_pr_dbg - print pm sleep debug messages
>>>     *
>>> - * If pm_debug_messages_on is enabled and the system is entering/leaving
>>> - *      suspend, print message.
>>> + * If pm_debug_messages_on is enabled, 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 a9e0693aaf69..aa754241aaa6 100644
>>> --- a/kernel/power/main.c
>>> +++ b/kernel/power/main.c
>>> @@ -611,12 +611,6 @@ 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)
>>>    {
>>

Re: [PATCH] Revert "include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume"

[Rafael J. Wysocki]

On Mon, Apr 22, 2024 at 5:02 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 4/22/2024 09:45, Rafael J. Wysocki wrote:
> > On Mon, Apr 22, 2024 at 4:33 PM Mario Limonciello
> > <mario.limonciello@amd.com> wrote:
> >>
> >> On 4/22/2024 04:36, xiongxin wrote:
> >>> This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce.
> >>>
> >>> In the suspend process, pm_pr_dbg() is called before setting
> >>> pm_suspend_target_state. As a result, this part of the log cannot be
> >>> output.
> >>>
> >>> pm_pr_dbg() also outputs debug logs for hibernate, but
> >>> pm_suspend_target_state is not set, resulting in hibernate debug logs
> >>> can only be output through dynamic debug, which is very inconvenient.
> >>
> >> As an alternative, how about exporting and renaming the variable
> >> in_suspend in kernel/power/hibernate.c and considering that to tell if
> >> the hibernate process is going on?
> >>
> >> Then it should work just the same as it does at suspend.
> >
> > Well, this is not the only part that stopped working AFAICS.  I'll
> > queue up the revert.
>
> I just tested the revert to see what happens to other drivers but it's
> going to have more collateral damage.
>
> ERROR: modpost: "pm_debug_messages_on"
> [drivers/platform/x86/amd/pmc/amd-pmc.ko] undefined!

What about removing the "pm_suspend_target_state != PM_SUSPEND_ON"
part from pm_debug_messages_should_print()?

This should be as good as the revert from the POV of restoring the
previous functionality.

Re: [PATCH] Revert "include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume"

[Mario Limonciello]

On 4/22/2024 10:18, Rafael J. Wysocki wrote:
> On Mon, Apr 22, 2024 at 5:02 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> On 4/22/2024 09:45, Rafael J. Wysocki wrote:
>>> On Mon, Apr 22, 2024 at 4:33 PM Mario Limonciello
>>> <mario.limonciello@amd.com> wrote:
>>>>
>>>> On 4/22/2024 04:36, xiongxin wrote:
>>>>> This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce.
>>>>>
>>>>> In the suspend process, pm_pr_dbg() is called before setting
>>>>> pm_suspend_target_state. As a result, this part of the log cannot be
>>>>> output.
>>>>>
>>>>> pm_pr_dbg() also outputs debug logs for hibernate, but
>>>>> pm_suspend_target_state is not set, resulting in hibernate debug logs
>>>>> can only be output through dynamic debug, which is very inconvenient.
>>>>
>>>> As an alternative, how about exporting and renaming the variable
>>>> in_suspend in kernel/power/hibernate.c and considering that to tell if
>>>> the hibernate process is going on?
>>>>
>>>> Then it should work just the same as it does at suspend.
>>>
>>> Well, this is not the only part that stopped working AFAICS.  I'll
>>> queue up the revert.
>>
>> I just tested the revert to see what happens to other drivers but it's
>> going to have more collateral damage.
>>
>> ERROR: modpost: "pm_debug_messages_on"
>> [drivers/platform/x86/amd/pmc/amd-pmc.ko] undefined!
> 
> What about removing the "pm_suspend_target_state != PM_SUSPEND_ON"
> part from pm_debug_messages_should_print()?
> 
> This should be as good as the revert from the POV of restoring the
> previous functionality.

That would probably help this reported issue but it's going to be REALLY 
noisy for the pinctrl-amd driver for anyone that sets 
/sys/power/pm_debug_messages.

There is a message in that driver that is emitted whenever a GPIO is 
active and pm_debug_messages is set.

It's a really useful message for tracking down which GPIO woke the 
system up as the IRQ that is active is the GPIO controller master IRQ 
not an IRQ for the GPIO.

But if that change is made anyone who sets /sys/power/pm_debug_messages 
is going to see their kernel ring buffer flooded with every since 
interrupt associated with an I2C touchpad attention pin (for example).

So if the desire really is to back all this out, I think we need to also 
back out other users of pm_pr_dbg() too.

Re: [PATCH] Revert "include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume"

[Rafael J. Wysocki]

On Mon, Apr 22, 2024 at 5:25 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 4/22/2024 10:18, Rafael J. Wysocki wrote:
> > On Mon, Apr 22, 2024 at 5:02 PM Mario Limonciello
> > <mario.limonciello@amd.com> wrote:
> >>
> >> On 4/22/2024 09:45, Rafael J. Wysocki wrote:
> >>> On Mon, Apr 22, 2024 at 4:33 PM Mario Limonciello
> >>> <mario.limonciello@amd.com> wrote:
> >>>>
> >>>> On 4/22/2024 04:36, xiongxin wrote:
> >>>>> This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce.
> >>>>>
> >>>>> In the suspend process, pm_pr_dbg() is called before setting
> >>>>> pm_suspend_target_state. As a result, this part of the log cannot be
> >>>>> output.
> >>>>>
> >>>>> pm_pr_dbg() also outputs debug logs for hibernate, but
> >>>>> pm_suspend_target_state is not set, resulting in hibernate debug logs
> >>>>> can only be output through dynamic debug, which is very inconvenient.
> >>>>
> >>>> As an alternative, how about exporting and renaming the variable
> >>>> in_suspend in kernel/power/hibernate.c and considering that to tell if
> >>>> the hibernate process is going on?
> >>>>
> >>>> Then it should work just the same as it does at suspend.
> >>>
> >>> Well, this is not the only part that stopped working AFAICS.  I'll
> >>> queue up the revert.
> >>
> >> I just tested the revert to see what happens to other drivers but it's
> >> going to have more collateral damage.
> >>
> >> ERROR: modpost: "pm_debug_messages_on"
> >> [drivers/platform/x86/amd/pmc/amd-pmc.ko] undefined!
> >
> > What about removing the "pm_suspend_target_state != PM_SUSPEND_ON"
> > part from pm_debug_messages_should_print()?
> >
> > This should be as good as the revert from the POV of restoring the
> > previous functionality.
>
> That would probably help this reported issue but it's going to be REALLY
> noisy for the pinctrl-amd driver for anyone that sets
> /sys/power/pm_debug_messages.
>
> There is a message in that driver that is emitted whenever a GPIO is
> active and pm_debug_messages is set.
>
> It's a really useful message for tracking down which GPIO woke the
> system up as the IRQ that is active is the GPIO controller master IRQ
> not an IRQ for the GPIO.
>
> But if that change is made anyone who sets /sys/power/pm_debug_messages
> is going to see their kernel ring buffer flooded with every since
> interrupt associated with an I2C touchpad attention pin (for example).
>
> So if the desire really is to back all this out, I think we need to also
> back out other users of pm_pr_dbg() too.

OK, so it needs to check hibernate_atomic in addition to
pm_suspend_target_state.

Re: [PATCH] Revert "include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume"

[Mario Limonciello]

On 4/22/2024 10:43, Rafael J. Wysocki wrote:
> On Mon, Apr 22, 2024 at 5:25 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> On 4/22/2024 10:18, Rafael J. Wysocki wrote:
>>> On Mon, Apr 22, 2024 at 5:02 PM Mario Limonciello
>>> <mario.limonciello@amd.com> wrote:
>>>>
>>>> On 4/22/2024 09:45, Rafael J. Wysocki wrote:
>>>>> On Mon, Apr 22, 2024 at 4:33 PM Mario Limonciello
>>>>> <mario.limonciello@amd.com> wrote:
>>>>>>
>>>>>> On 4/22/2024 04:36, xiongxin wrote:
>>>>>>> This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce.
>>>>>>>
>>>>>>> In the suspend process, pm_pr_dbg() is called before setting
>>>>>>> pm_suspend_target_state. As a result, this part of the log cannot be
>>>>>>> output.
>>>>>>>
>>>>>>> pm_pr_dbg() also outputs debug logs for hibernate, but
>>>>>>> pm_suspend_target_state is not set, resulting in hibernate debug logs
>>>>>>> can only be output through dynamic debug, which is very inconvenient.
>>>>>>
>>>>>> As an alternative, how about exporting and renaming the variable
>>>>>> in_suspend in kernel/power/hibernate.c and considering that to tell if
>>>>>> the hibernate process is going on?
>>>>>>
>>>>>> Then it should work just the same as it does at suspend.
>>>>>
>>>>> Well, this is not the only part that stopped working AFAICS.  I'll
>>>>> queue up the revert.
>>>>
>>>> I just tested the revert to see what happens to other drivers but it's
>>>> going to have more collateral damage.
>>>>
>>>> ERROR: modpost: "pm_debug_messages_on"
>>>> [drivers/platform/x86/amd/pmc/amd-pmc.ko] undefined!
>>>
>>> What about removing the "pm_suspend_target_state != PM_SUSPEND_ON"
>>> part from pm_debug_messages_should_print()?
>>>
>>> This should be as good as the revert from the POV of restoring the
>>> previous functionality.
>>
>> That would probably help this reported issue but it's going to be REALLY
>> noisy for the pinctrl-amd driver for anyone that sets
>> /sys/power/pm_debug_messages.
>>
>> There is a message in that driver that is emitted whenever a GPIO is
>> active and pm_debug_messages is set.
>>
>> It's a really useful message for tracking down which GPIO woke the
>> system up as the IRQ that is active is the GPIO controller master IRQ
>> not an IRQ for the GPIO.
>>
>> But if that change is made anyone who sets /sys/power/pm_debug_messages
>> is going to see their kernel ring buffer flooded with every since
>> interrupt associated with an I2C touchpad attention pin (for example).
>>
>> So if the desire really is to back all this out, I think we need to also
>> back out other users of pm_pr_dbg() too.
> 
> OK, so it needs to check hibernate_atomic in addition to
> pm_suspend_target_state.

Yeah, that sounds great to me.

Tangentially related to the discussion; how would you feel about a 
/sys/power/pm_wakeup_gpio?  Or /sys/power/pm_wakeup_gpios?

If we did the plural and used a comma separated list we could probably 
axe the message I mentioned above from pinctrl-amd all together.

Thanks,

Re: [PATCH] Revert "include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume"

[Rafael J. Wysocki]

On Mon, Apr 22, 2024 at 5:54 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 4/22/2024 10:43, Rafael J. Wysocki wrote:
> > On Mon, Apr 22, 2024 at 5:25 PM Mario Limonciello
> > <mario.limonciello@amd.com> wrote:
> >>
> >> On 4/22/2024 10:18, Rafael J. Wysocki wrote:
> >>> On Mon, Apr 22, 2024 at 5:02 PM Mario Limonciello
> >>> <mario.limonciello@amd.com> wrote:
> >>>>
> >>>> On 4/22/2024 09:45, Rafael J. Wysocki wrote:
> >>>>> On Mon, Apr 22, 2024 at 4:33 PM Mario Limonciello
> >>>>> <mario.limonciello@amd.com> wrote:
> >>>>>>
> >>>>>> On 4/22/2024 04:36, xiongxin wrote:
> >>>>>>> This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce.
> >>>>>>>
> >>>>>>> In the suspend process, pm_pr_dbg() is called before setting
> >>>>>>> pm_suspend_target_state. As a result, this part of the log cannot be
> >>>>>>> output.
> >>>>>>>
> >>>>>>> pm_pr_dbg() also outputs debug logs for hibernate, but
> >>>>>>> pm_suspend_target_state is not set, resulting in hibernate debug logs
> >>>>>>> can only be output through dynamic debug, which is very inconvenient.
> >>>>>>
> >>>>>> As an alternative, how about exporting and renaming the variable
> >>>>>> in_suspend in kernel/power/hibernate.c and considering that to tell if
> >>>>>> the hibernate process is going on?
> >>>>>>
> >>>>>> Then it should work just the same as it does at suspend.
> >>>>>
> >>>>> Well, this is not the only part that stopped working AFAICS.  I'll
> >>>>> queue up the revert.
> >>>>
> >>>> I just tested the revert to see what happens to other drivers but it's
> >>>> going to have more collateral damage.
> >>>>
> >>>> ERROR: modpost: "pm_debug_messages_on"
> >>>> [drivers/platform/x86/amd/pmc/amd-pmc.ko] undefined!
> >>>
> >>> What about removing the "pm_suspend_target_state != PM_SUSPEND_ON"
> >>> part from pm_debug_messages_should_print()?
> >>>
> >>> This should be as good as the revert from the POV of restoring the
> >>> previous functionality.
> >>
> >> That would probably help this reported issue but it's going to be REALLY
> >> noisy for the pinctrl-amd driver for anyone that sets
> >> /sys/power/pm_debug_messages.
> >>
> >> There is a message in that driver that is emitted whenever a GPIO is
> >> active and pm_debug_messages is set.
> >>
> >> It's a really useful message for tracking down which GPIO woke the
> >> system up as the IRQ that is active is the GPIO controller master IRQ
> >> not an IRQ for the GPIO.
> >>
> >> But if that change is made anyone who sets /sys/power/pm_debug_messages
> >> is going to see their kernel ring buffer flooded with every since
> >> interrupt associated with an I2C touchpad attention pin (for example).
> >>
> >> So if the desire really is to back all this out, I think we need to also
> >> back out other users of pm_pr_dbg() too.
> >
> > OK, so it needs to check hibernate_atomic in addition to
> > pm_suspend_target_state.
>
> Yeah, that sounds great to me.

OK

> Tangentially related to the discussion; how would you feel about a
> /sys/power/pm_wakeup_gpio?  Or /sys/power/pm_wakeup_gpios?
>
> If we did the plural and used a comma separated list we could probably
> axe the message I mentioned above from pinctrl-amd all together.

That would be too specific IMV.

The whole idea with pm_debug_messages is to switch them all on or off in one go.

Re: [PATCH] Revert "include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume"

[xiongxin]

在 2024/4/23 00:04, Rafael J. Wysocki 写道:
> On Mon, Apr 22, 2024 at 5:54 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>> On 4/22/2024 10:43, Rafael J. Wysocki wrote:
>>> On Mon, Apr 22, 2024 at 5:25 PM Mario Limonciello
>>> <mario.limonciello@amd.com> wrote:
>>>> On 4/22/2024 10:18, Rafael J. Wysocki wrote:
>>>>> On Mon, Apr 22, 2024 at 5:02 PM Mario Limonciello
>>>>> <mario.limonciello@amd.com> wrote:
>>>>>> On 4/22/2024 09:45, Rafael J. Wysocki wrote:
>>>>>>> On Mon, Apr 22, 2024 at 4:33 PM Mario Limonciello
>>>>>>> <mario.limonciello@amd.com> wrote:
>>>>>>>> On 4/22/2024 04:36, xiongxin wrote:
>>>>>>>>> This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce.
>>>>>>>>>
>>>>>>>>> In the suspend process, pm_pr_dbg() is called before setting
>>>>>>>>> pm_suspend_target_state. As a result, this part of the log cannot be
>>>>>>>>> output.
>>>>>>>>>
>>>>>>>>> pm_pr_dbg() also outputs debug logs for hibernate, but
>>>>>>>>> pm_suspend_target_state is not set, resulting in hibernate debug logs
>>>>>>>>> can only be output through dynamic debug, which is very inconvenient.
>>>>>>>> As an alternative, how about exporting and renaming the variable
>>>>>>>> in_suspend in kernel/power/hibernate.c and considering that to tell if
>>>>>>>> the hibernate process is going on?
>>>>>>>>
>>>>>>>> Then it should work just the same as it does at suspend.
>>>>>>> Well, this is not the only part that stopped working AFAICS.  I'll
>>>>>>> queue up the revert.
>>>>>> I just tested the revert to see what happens to other drivers but it's
>>>>>> going to have more collateral damage.
>>>>>>
>>>>>> ERROR: modpost: "pm_debug_messages_on"
>>>>>> [drivers/platform/x86/amd/pmc/amd-pmc.ko] undefined!

The revert has simply removed the pm_debug_messages_should_print() func, 
there is no reference to

this function anywhere else in the source code, and 
drivers/platform/x86/amd/pmc/ path does not

reference pm_debug_messages_on or this function.

>>>>> What about removing the "pm_suspend_target_state != PM_SUSPEND_ON"
>>>>> part from pm_debug_messages_should_print()?
>>>>>
>>>>> This should be as good as the revert from the POV of restoring the
>>>>> previous functionality.
>>>> That would probably help this reported issue but it's going to be REALLY
>>>> noisy for the pinctrl-amd driver for anyone that sets
>>>> /sys/power/pm_debug_messages.
>>>>
>>>> There is a message in that driver that is emitted whenever a GPIO is
>>>> active and pm_debug_messages is set.
>>>>
>>>> It's a really useful message for tracking down which GPIO woke the
>>>> system up as the IRQ that is active is the GPIO controller master IRQ
>>>> not an IRQ for the GPIO.
>>>>
>>>> But if that change is made anyone who sets /sys/power/pm_debug_messages
>>>> is going to see their kernel ring buffer flooded with every since
>>>> interrupt associated with an I2C touchpad attention pin (for example).
>>>>
>>>> So if the desire really is to back all this out, I think we need to also
>>>> back out other users of pm_pr_dbg() too.
>>> OK, so it needs to check hibernate_atomic in addition to
>>> pm_suspend_target_state.
>> Yeah, that sounds great to me.
> OK
>
>> Tangentially related to the discussion; how would you feel about a
>> /sys/power/pm_wakeup_gpio?  Or /sys/power/pm_wakeup_gpios?
>>
>> If we did the plural and used a comma separated list we could probably
>> axe the message I mentioned above from pinctrl-amd all together.
> That would be too specific IMV.
>
> The whole idea with pm_debug_messages is to switch them all on or off in one go.

Re: [PATCH] Revert "include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume"

[Mario Limonciello]

On 4/22/2024 19:59, xiongxin wrote:
> 在 2024/4/23 00:04, Rafael J. Wysocki 写道:
>> On Mon, Apr 22, 2024 at 5:54 PM Mario Limonciello
>> <mario.limonciello@amd.com> wrote:
>>> On 4/22/2024 10:43, Rafael J. Wysocki wrote:
>>>> On Mon, Apr 22, 2024 at 5:25 PM Mario Limonciello
>>>> <mario.limonciello@amd.com> wrote:
>>>>> On 4/22/2024 10:18, Rafael J. Wysocki wrote:
>>>>>> On Mon, Apr 22, 2024 at 5:02 PM Mario Limonciello
>>>>>> <mario.limonciello@amd.com> wrote:
>>>>>>> On 4/22/2024 09:45, Rafael J. Wysocki wrote:
>>>>>>>> On Mon, Apr 22, 2024 at 4:33 PM Mario Limonciello
>>>>>>>> <mario.limonciello@amd.com> wrote:
>>>>>>>>> On 4/22/2024 04:36, xiongxin wrote:
>>>>>>>>>> This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce.
>>>>>>>>>>
>>>>>>>>>> In the suspend process, pm_pr_dbg() is called before setting
>>>>>>>>>> pm_suspend_target_state. As a result, this part of the log 
>>>>>>>>>> cannot be
>>>>>>>>>> output.
>>>>>>>>>>
>>>>>>>>>> pm_pr_dbg() also outputs debug logs for hibernate, but
>>>>>>>>>> pm_suspend_target_state is not set, resulting in hibernate 
>>>>>>>>>> debug logs
>>>>>>>>>> can only be output through dynamic debug, which is very 
>>>>>>>>>> inconvenient.
>>>>>>>>> As an alternative, how about exporting and renaming the variable
>>>>>>>>> in_suspend in kernel/power/hibernate.c and considering that to 
>>>>>>>>> tell if
>>>>>>>>> the hibernate process is going on?
>>>>>>>>>
>>>>>>>>> Then it should work just the same as it does at suspend.
>>>>>>>> Well, this is not the only part that stopped working AFAICS.  I'll
>>>>>>>> queue up the revert.
>>>>>>> I just tested the revert to see what happens to other drivers but 
>>>>>>> it's
>>>>>>> going to have more collateral damage.
>>>>>>>
>>>>>>> ERROR: modpost: "pm_debug_messages_on"
>>>>>>> [drivers/platform/x86/amd/pmc/amd-pmc.ko] undefined!
> 
> The revert has simply removed the pm_debug_messages_should_print() func, 
> there is no reference to
> 
> this function anywhere else in the source code, and 
> drivers/platform/x86/amd/pmc/ path does not
> 
> reference pm_debug_messages_on or this function.

amd_pmc_idlemask_read() uses the pm_pr_dbg() macro which uses the 
__pm_pr_dbg() macro.

In your revert the pm_debug_messages_on variable is not exported like 
pm_debug_messages_should_print() was in the original commit.
As amd-pmc is compiled as a module it won't be able to access that variable.

> 
>>>>>> What about removing the "pm_suspend_target_state != PM_SUSPEND_ON"
>>>>>> part from pm_debug_messages_should_print()?
>>>>>>
>>>>>> This should be as good as the revert from the POV of restoring the
>>>>>> previous functionality.
>>>>> That would probably help this reported issue but it's going to be 
>>>>> REALLY
>>>>> noisy for the pinctrl-amd driver for anyone that sets
>>>>> /sys/power/pm_debug_messages.
>>>>>
>>>>> There is a message in that driver that is emitted whenever a GPIO is
>>>>> active and pm_debug_messages is set.
>>>>>
>>>>> It's a really useful message for tracking down which GPIO woke the
>>>>> system up as the IRQ that is active is the GPIO controller master IRQ
>>>>> not an IRQ for the GPIO.
>>>>>
>>>>> But if that change is made anyone who sets 
>>>>> /sys/power/pm_debug_messages
>>>>> is going to see their kernel ring buffer flooded with every since
>>>>> interrupt associated with an I2C touchpad attention pin (for example).
>>>>>
>>>>> So if the desire really is to back all this out, I think we need to 
>>>>> also
>>>>> back out other users of pm_pr_dbg() too.
>>>> OK, so it needs to check hibernate_atomic in addition to
>>>> pm_suspend_target_state.
>>> Yeah, that sounds great to me.
>> OK
>>
>>> Tangentially related to the discussion; how would you feel about a
>>> /sys/power/pm_wakeup_gpio?  Or /sys/power/pm_wakeup_gpios?
>>>
>>> If we did the plural and used a comma separated list we could probably
>>> axe the message I mentioned above from pinctrl-amd all together.
>> That would be too specific IMV.
>>
>> The whole idea with pm_debug_messages is to switch them all on or off 
>> in one go.
> 
> 

[PATCH] PM: sleep: Optimize the pm_debug_messages_should_print() function

[xiongxin]

commit cdb8c100d8a4 ("include/linux/suspend.h: Only show pm_pr_dbg
messages at suspend/resume"), pm_debug_messages_should_print() is
implemented to determine the output of pm_pr_dbg(), using
pm_suspend_target_state to identify the suspend process. However, this
limits the output range of pm_pr_dbg().

In the suspend process, pm_pr_dbg() is called before setting
pm_suspend_target_state. As a result, this part of the log cannot be
output.

pm_pr_dbg() also outputs debug logs for hibernate, but
pm_suspend_target_state is not set, resulting in hibernate debug logs
can only be output through dynamic debug, which is very inconvenient.

Currently, remove pm_suspend_target_state from
pm_debug_messages_should_print() to ensure that sleep and hibernate main
logic can output debug normally.

Fixes: cdb8c100d8a4 ("include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume").
Signed-off-by: xiongxin <xiongxin@kylinos.cn>
---
v2:
	* Resolve the compilation error and re-submit with the fix
	  patch.
v1:
	* Revert the commit cdb8c100d8a4 ("include/linux/suspend.h: Only
	  show pm_pr_dbg messages at suspend/resume").
---

diff --git a/kernel/power/main.c b/kernel/power/main.c
index a9e0693aaf69..24693599c0bc 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -613,7 +613,7 @@ 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;
+	return pm_debug_messages_on;
 }
 EXPORT_SYMBOL_GPL(pm_debug_messages_should_print);
 
-- 
2.34.1

Re: [PATCH] PM: sleep: Optimize the pm_debug_messages_should_print() function

[Mario Limonciello]

On 4/23/2024 03:17, xiongxin wrote:
> commit cdb8c100d8a4 ("include/linux/suspend.h: Only show pm_pr_dbg
> messages at suspend/resume"), pm_debug_messages_should_print() is
> implemented to determine the output of pm_pr_dbg(), using
> pm_suspend_target_state to identify the suspend process. However, this
> limits the output range of pm_pr_dbg().
> 
> In the suspend process, pm_pr_dbg() is called before setting
> pm_suspend_target_state. As a result, this part of the log cannot be
> output.
> 
> pm_pr_dbg() also outputs debug logs for hibernate, but
> pm_suspend_target_state is not set, resulting in hibernate debug logs
> can only be output through dynamic debug, which is very inconvenient.
> 
> Currently, remove pm_suspend_target_state from
> pm_debug_messages_should_print() to ensure that sleep and hibernate main
> logic can output debug normally.
> 
> Fixes: cdb8c100d8a4 ("include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume").
> Signed-off-by: xiongxin <xiongxin@kylinos.cn>
> ---
> v2:
> 	* Resolve the compilation error and re-submit with the fix
> 	  patch.
> v1:
> 	* Revert the commit cdb8c100d8a4 ("include/linux/suspend.h: Only
> 	  show pm_pr_dbg messages at suspend/resume").
> ---
> 
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index a9e0693aaf69..24693599c0bc 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -613,7 +613,7 @@ 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;
> +	return pm_debug_messages_on;
>   }
>   EXPORT_SYMBOL_GPL(pm_debug_messages_should_print);
>   

Did you miss the proposal for fixing this for hibernate by adding the 
extra variable to monitor?

Re: [PATCH] PM: sleep: Optimize the pm_debug_messages_should_print() function

[xiongxin]

在 2024/4/24 00:52, Mario Limonciello 写道:
> On 4/23/2024 03:17, xiongxin wrote:
>> commit cdb8c100d8a4 ("include/linux/suspend.h: Only show pm_pr_dbg
>> messages at suspend/resume"), pm_debug_messages_should_print() is
>> implemented to determine the output of pm_pr_dbg(), using
>> pm_suspend_target_state to identify the suspend process. However, this
>> limits the output range of pm_pr_dbg().
>>
>> In the suspend process, pm_pr_dbg() is called before setting
>> pm_suspend_target_state. As a result, this part of the log cannot be
>> output.
>>
>> pm_pr_dbg() also outputs debug logs for hibernate, but
>> pm_suspend_target_state is not set, resulting in hibernate debug logs
>> can only be output through dynamic debug, which is very inconvenient.
>>
>> Currently, remove pm_suspend_target_state from
>> pm_debug_messages_should_print() to ensure that sleep and hibernate main
>> logic can output debug normally.
>>
>> Fixes: cdb8c100d8a4 ("include/linux/suspend.h: Only show pm_pr_dbg 
>> messages at suspend/resume").
>> Signed-off-by: xiongxin <xiongxin@kylinos.cn>
>> ---
>> v2:
>>     * Resolve the compilation error and re-submit with the fix
>>       patch.
>> v1:
>>     * Revert the commit cdb8c100d8a4 ("include/linux/suspend.h: Only
>>       show pm_pr_dbg messages at suspend/resume").
>> ---
>>
>> diff --git a/kernel/power/main.c b/kernel/power/main.c
>> index a9e0693aaf69..24693599c0bc 100644
>> --- a/kernel/power/main.c
>> +++ b/kernel/power/main.c
>> @@ -613,7 +613,7 @@ 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;
>> +    return pm_debug_messages_on;
>>   }
>>   EXPORT_SYMBOL_GPL(pm_debug_messages_should_print);
>
> Did you miss the proposal for fixing this for hibernate by adding the 
> extra variable to monitor?

Can I change pm_pr_dbg() in amd_pmc_idlemask_read() to pr_debug() based on

pm_debug_messages_on condition?

I suggest not adding a new variable to this.

Re: [PATCH] PM: sleep: Optimize the pm_debug_messages_should_print() function

[Mario Limonciello]

On 4/30/2024 03:45, xiongxin wrote:
> 
> 在 2024/4/24 00:52, Mario Limonciello 写道:
>> On 4/23/2024 03:17, xiongxin wrote:
>>> commit cdb8c100d8a4 ("include/linux/suspend.h: Only show pm_pr_dbg
>>> messages at suspend/resume"), pm_debug_messages_should_print() is
>>> implemented to determine the output of pm_pr_dbg(), using
>>> pm_suspend_target_state to identify the suspend process. However, this
>>> limits the output range of pm_pr_dbg().
>>>
>>> In the suspend process, pm_pr_dbg() is called before setting
>>> pm_suspend_target_state. As a result, this part of the log cannot be
>>> output.
>>>
>>> pm_pr_dbg() also outputs debug logs for hibernate, but
>>> pm_suspend_target_state is not set, resulting in hibernate debug logs
>>> can only be output through dynamic debug, which is very inconvenient.
>>>
>>> Currently, remove pm_suspend_target_state from
>>> pm_debug_messages_should_print() to ensure that sleep and hibernate main
>>> logic can output debug normally.
>>>
>>> Fixes: cdb8c100d8a4 ("include/linux/suspend.h: Only show pm_pr_dbg 
>>> messages at suspend/resume").
>>> Signed-off-by: xiongxin <xiongxin@kylinos.cn>
>>> ---
>>> v2:
>>>     * Resolve the compilation error and re-submit with the fix
>>>       patch.
>>> v1:
>>>     * Revert the commit cdb8c100d8a4 ("include/linux/suspend.h: Only
>>>       show pm_pr_dbg messages at suspend/resume").
>>> ---
>>>
>>> diff --git a/kernel/power/main.c b/kernel/power/main.c
>>> index a9e0693aaf69..24693599c0bc 100644
>>> --- a/kernel/power/main.c
>>> +++ b/kernel/power/main.c
>>> @@ -613,7 +613,7 @@ 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;
>>> +    return pm_debug_messages_on;
>>>   }
>>>   EXPORT_SYMBOL_GPL(pm_debug_messages_should_print);
>>
>> Did you miss the proposal for fixing this for hibernate by adding the 
>> extra variable to monitor?
> 
> Can I change pm_pr_dbg() in amd_pmc_idlemask_read() to pr_debug() based on
> 
> pm_debug_messages_on condition?
> 
> I suggest not adding a new variable to this.
> 

I don't understand the opposition to the new variable.

The whole point of /sys/power/pm_debug_messages is so that it's a one 
stop shop to turn on power management related debugging at power state 
but nothing more.

You turn that on and you can get messages from the core and also any 
drivers that want to emit messages during that time.

If changing drivers back to pr_debug that means that users and software 
need to manually turn on BOTH /sys/power/pm_debug_messages as well as 
dynamic debug for any power management related messages.

Whereas if just adding another variable for a condition then just turn 
on the sysfs file for any hibernate or suspend debugging.

Re: [PATCH] PM: sleep: Optimize the pm_debug_messages_should_print() function

[xiongxin]

在 2024/4/30 22:36, Mario Limonciello 写道:
> On 4/30/2024 03:45, xiongxin wrote:
>>
>> 在 2024/4/24 00:52, Mario Limonciello 写道:
>>> On 4/23/2024 03:17, xiongxin wrote:
>>>> commit cdb8c100d8a4 ("include/linux/suspend.h: Only show pm_pr_dbg
>>>> messages at suspend/resume"), pm_debug_messages_should_print() is
>>>> implemented to determine the output of pm_pr_dbg(), using
>>>> pm_suspend_target_state to identify the suspend process. However, this
>>>> limits the output range of pm_pr_dbg().
>>>>
>>>> In the suspend process, pm_pr_dbg() is called before setting
>>>> pm_suspend_target_state. As a result, this part of the log cannot be
>>>> output.
>>>>
>>>> pm_pr_dbg() also outputs debug logs for hibernate, but
>>>> pm_suspend_target_state is not set, resulting in hibernate debug logs
>>>> can only be output through dynamic debug, which is very inconvenient.
>>>>
>>>> Currently, remove pm_suspend_target_state from
>>>> pm_debug_messages_should_print() to ensure that sleep and hibernate 
>>>> main
>>>> logic can output debug normally.
>>>>
>>>> Fixes: cdb8c100d8a4 ("include/linux/suspend.h: Only show pm_pr_dbg 
>>>> messages at suspend/resume").
>>>> Signed-off-by: xiongxin <xiongxin@kylinos.cn>
>>>> ---
>>>> v2:
>>>>     * Resolve the compilation error and re-submit with the fix
>>>>       patch.
>>>> v1:
>>>>     * Revert the commit cdb8c100d8a4 ("include/linux/suspend.h: Only
>>>>       show pm_pr_dbg messages at suspend/resume").
>>>> ---
>>>>
>>>> diff --git a/kernel/power/main.c b/kernel/power/main.c
>>>> index a9e0693aaf69..24693599c0bc 100644
>>>> --- a/kernel/power/main.c
>>>> +++ b/kernel/power/main.c
>>>> @@ -613,7 +613,7 @@ 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;
>>>> +    return pm_debug_messages_on;
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(pm_debug_messages_should_print);
>>>
>>> Did you miss the proposal for fixing this for hibernate by adding the 
>>> extra variable to monitor?
>>
>> Can I change pm_pr_dbg() in amd_pmc_idlemask_read() to pr_debug() 
>> based on
>>
>> pm_debug_messages_on condition?
>>
>> I suggest not adding a new variable to this.
>>
> 
> I don't understand the opposition to the new variable.
> 
> The whole point of /sys/power/pm_debug_messages is so that it's a one 
> stop shop to turn on power management related debugging at power state 
> but nothing more.
> 
> You turn that on and you can get messages from the core and also any 
> drivers that want to emit messages during that time.
> 
> If changing drivers back to pr_debug that means that users and software 
> need to manually turn on BOTH /sys/power/pm_debug_messages as well as 
> dynamic debug for any power management related messages.
> 
> Whereas if just adding another variable for a condition then just turn 
> on the sysfs file for any hibernate or suspend debugging.

Your patch makes the output of pm_pr_dbg() based on the values of 
pm_debug_messages_on and pm_suspend_target_state; However, 
pm_suspend_target_state's impact domain does not include enter_state() 
and hibernate processes;

The patch affects the output of the sleep mainline debug log, which is 
very unfriendly to others developers, and it is even more troublesome
to add a new variable based on your suggestion.

The kernel already has a log output solution based on the value of 
pm_suspend_target_state. I will issue a repair patch as follows in
amd_pmc_idlemask_read():

if (dev && pm_suspend_target_state != PM_SUSPEND_ON)
	pr_info("SMU idlemask s0i3: 0x%x\n", val);

Re: [PATCH] PM: sleep: Optimize the pm_debug_messages_should_print() function

[Mario Limonciello]

>>> Can I change pm_pr_dbg() in amd_pmc_idlemask_read() to pr_debug() 
>>> based on
>>>
>>> pm_debug_messages_on condition?
>>>
>>> I suggest not adding a new variable to this.
>>>
>>
>> I don't understand the opposition to the new variable.
>>
>> The whole point of /sys/power/pm_debug_messages is so that it's a one 
>> stop shop to turn on power management related debugging at power state 
>> but nothing more.
>>
>> You turn that on and you can get messages from the core and also any 
>> drivers that want to emit messages during that time.
>>
>> If changing drivers back to pr_debug that means that users and 
>> software need to manually turn on BOTH /sys/power/pm_debug_messages as 
>> well as dynamic debug for any power management related messages.
>>
>> Whereas if just adding another variable for a condition then just turn 
>> on the sysfs file for any hibernate or suspend debugging.
> 
> Your patch makes the output of pm_pr_dbg() based on the values of 
> pm_debug_messages_on and pm_suspend_target_state; However, 
> pm_suspend_target_state's impact domain does not include enter_state() 
> and hibernate processes;
> 
> The patch affects the output of the sleep mainline debug log, which is 
> very unfriendly to others developers, and it is even more troublesome
> to add a new variable based on your suggestion.

Why is adding a new variable more troublesome?  We're talking about a 
one line change and then it can run in more power management situations.

> 
> The kernel already has a log output solution based on the value of 
> pm_suspend_target_state. I will issue a repair patch as follows in
> amd_pmc_idlemask_read():
> 
> if (dev && pm_suspend_target_state != PM_SUSPEND_ON)
>      pr_info("SMU idlemask s0i3: 0x%x\n", val);

But then this is going to be really noisy still for the general purpose 
users.

The point of pm_pr_dbg() is that it only outputs the debugging message 
when /sys/power/pm_debug_messages is set.

99% of people don't need this message, but when someone comes to say "it 
doesn't work!" changing one sysfs file gets me a lot more data about 
/why/ it doesn't work without compromising everyone else's logs.

Re: [PATCH] PM: sleep: Optimize the pm_debug_messages_should_print() function

[xiongxin]

On 2024/5/3 03:04, Mario Limonciello wrote:
> 
>>>> Can I change pm_pr_dbg() in amd_pmc_idlemask_read() to pr_debug() 
>>>> based on
>>>>
>>>> pm_debug_messages_on condition?
>>>>
>>>> I suggest not adding a new variable to this.
>>>>
>>>
>>> I don't understand the opposition to the new variable.
>>>
>>> The whole point of /sys/power/pm_debug_messages is so that it's a one 
>>> stop shop to turn on power management related debugging at power 
>>> state but nothing more.
>>>
>>> You turn that on and you can get messages from the core and also any 
>>> drivers that want to emit messages during that time.
>>>
>>> If changing drivers back to pr_debug that means that users and 
>>> software need to manually turn on BOTH /sys/power/pm_debug_messages 
>>> as well as dynamic debug for any power management related messages.
>>>
>>> Whereas if just adding another variable for a condition then just 
>>> turn on the sysfs file for any hibernate or suspend debugging.
>>
>> Your patch makes the output of pm_pr_dbg() based on the values of 
>> pm_debug_messages_on and pm_suspend_target_state; However, 
>> pm_suspend_target_state's impact domain does not include enter_state() 
>> and hibernate processes;
>>
>> The patch affects the output of the sleep mainline debug log, which is 
>> very unfriendly to others developers, and it is even more troublesome
>> to add a new variable based on your suggestion.
> 
> Why is adding a new variable more troublesome?  We're talking about a 
> one line change and then it can run in more power management situations.

Please check the patch you submitted: cdb8c100d8a4 
(include/linux/suspend.h: Only show pm_pr_dbg messages at 
suspend/resume). The patch you submit and merge limits the scope of what 
pm_pr_dbg() can output, that is, you modify the original capability of 
pm_pr_dbg().

All I'm doing is trying to get pm_pr_dbg() back to its original output 
capacity.This is not an innovative technique, so why consider adding a 
variable to change the thinking of other developers who have already 
mastered pm_pr_dbg()?

> 
>>
>> The kernel already has a log output solution based on the value of 
>> pm_suspend_target_state. I will issue a repair patch as follows in
>> amd_pmc_idlemask_read():
>>
>> if (dev && pm_suspend_target_state != PM_SUSPEND_ON)
>>      pr_info("SMU idlemask s0i3: 0x%x\n", val);

My previous suggestion was not considered reasonable, so I slightly 
modified it as follows:

if (dev && pm_suspend_target_state != PM_SUSPEND_ON)
	pm_pr_dbg(SMU idlemask s0i3: 0x%x\n", val)

This is still based on /sys/power/pm_debug_messages, but does not change 
the original logic of pm_pr_dbg().

> 
> But then this is going to be really noisy still for the general purpose 
> users.
> 
> The point of pm_pr_dbg() is that it only outputs the debugging message 
> when /sys/power/pm_debug_messages is set.
> 
> 99% of people don't need this message, but when someone comes to say "it 
> doesn't work!" changing one sysfs file gets me a lot more data about 
> /why/ it doesn't work without compromising everyone else's logs.

Re: [PATCH] PM: sleep: Optimize the pm_debug_messages_should_print() function

[xiongxin]

On 2024/5/3 03:04, Mario Limonciello wrote:
> 
>>>> Can I change pm_pr_dbg() in amd_pmc_idlemask_read() to pr_debug() 
>>>> based on
>>>>
>>>> pm_debug_messages_on condition?
>>>>
>>>> I suggest not adding a new variable to this.
>>>>
>>>
>>> I don't understand the opposition to the new variable.
>>>
>>> The whole point of /sys/power/pm_debug_messages is so that it's a one 
>>> stop shop to turn on power management related debugging at power 
>>> state but nothing more.
>>>
>>> You turn that on and you can get messages from the core and also any 
>>> drivers that want to emit messages during that time.
>>>
>>> If changing drivers back to pr_debug that means that users and 
>>> software need to manually turn on BOTH /sys/power/pm_debug_messages 
>>> as well as dynamic debug for any power management related messages.
>>>
>>> Whereas if just adding another variable for a condition then just 
>>> turn on the sysfs file for any hibernate or suspend debugging.
>>
>> Your patch makes the output of pm_pr_dbg() based on the values of 
>> pm_debug_messages_on and pm_suspend_target_state; However, 
>> pm_suspend_target_state's impact domain does not include enter_state() 
>> and hibernate processes;
>>
>> The patch affects the output of the sleep mainline debug log, which is 
>> very unfriendly to others developers, and it is even more troublesome
>> to add a new variable based on your suggestion.
> 
> Why is adding a new variable more troublesome?  We're talking about a 
> one line change and then it can run in more power management situations.
> 
>>
>> The kernel already has a log output solution based on the value of 
>> pm_suspend_target_state. I will issue a repair patch as follows in
>> amd_pmc_idlemask_read():
>>
>> if (dev && pm_suspend_target_state != PM_SUSPEND_ON)
>>      pr_info("SMU idlemask s0i3: 0x%x\n", val);
> 
> But then this is going to be really noisy still for the general purpose 
> users.
> 
> The point of pm_pr_dbg() is that it only outputs the debugging message 
> when /sys/power/pm_debug_messages is set.
> 
> 99% of people don't need this message, but when someone comes to say "it 
> doesn't work!" changing one sysfs file gets me a lot more data about 
> /why/ it doesn't work without compromising everyone else's logs.

Hi Rafael.

Can pm_suspend_target_state be extended to the state_store() function? 
Change the value of the variable as soon as suspend or hibernate 
processes begin. For the invoke process in kernel/power/user.c, ensure 
that pm_suspend_target_state is set before invoking 
suspend_devices_and_enter().

However, many drivers do some operations based on the 
pm_suspend_target_state variable, and it is not clear whether expanding 
the scope of the variable will cause other exceptions.

Or re-implement a local variable based on state_store() and judge the 
variable in pm_debug_messages_should_print(), so that pm_pr_dbg() can be 
called directly by other drivers. And only suspend or hibernate 
processes print logs.