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

[xiongxin <xiongxin@kylinos.cn>]

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.