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

Mario Limonciello
2024-04-30T14:36:14Z

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.