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

[Mario Limonciello <mario.limonciello@amd.com>]

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.