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

xiongxin
2024-05-01T04:45:30Z

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