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

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>

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)
>   {

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.

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.

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.

在 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.

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.
> 
>

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?

在 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.

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.

在 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);

>>> 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.