From: xiongxin <xiongxin@kylinos.cn>
To: Mario Limonciello <mario.limonciello@amd.com>, rafael@kernel.org
Cc: linux-pm@vger.kernel.org
Subject: Re: [PATCH] PM: sleep: Optimize the pm_debug_messages_should_print() function
Date: Fri, 3 May 2024 09:29:34 +0800 [thread overview]
Message-ID: <4b077ec7-d9da-44fc-a083-1c27afff7e72@kylinos.cn> (raw)
In-Reply-To: <1714698149088349.0.seg@mailgw.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.
Please check the patch you submitted: cdb8c100d8a4
(include/linux/suspend.h: Only show pm_pr_dbg messages at
suspend/resume). The patch you submit and merge limits the scope of what
pm_pr_dbg() can output, that is, you modify the original capability of
pm_pr_dbg().
All I'm doing is trying to get pm_pr_dbg() back to its original output
capacity.This is not an innovative technique, so why consider adding a
variable to change the thinking of other developers who have already
mastered pm_pr_dbg()?
>
>>
>> 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);
My previous suggestion was not considered reasonable, so I slightly
modified it as follows:
if (dev && pm_suspend_target_state != PM_SUSPEND_ON)
pm_pr_dbg(SMU idlemask s0i3: 0x%x\n", val)
This is still based on /sys/power/pm_debug_messages, but does not change
the original logic of pm_pr_dbg().
>
> 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.
prev parent reply other threads:[~2024-05-03 1:29 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-22 9:36 [PATCH] Revert "include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume" xiongxin
2024-04-22 14:33 ` Mario Limonciello
2024-04-22 14:45 ` Rafael J. Wysocki
2024-04-22 15:01 ` Mario Limonciello
2024-04-22 15:18 ` Rafael J. Wysocki
2024-04-22 15:25 ` Mario Limonciello
2024-04-22 15:43 ` Rafael J. Wysocki
2024-04-22 15:54 ` Mario Limonciello
2024-04-22 16:04 ` Rafael J. Wysocki
2024-04-23 0:59 ` xiongxin
2024-04-23 3:42 ` Mario Limonciello
2024-04-23 8:17 ` [PATCH] PM: sleep: Optimize the pm_debug_messages_should_print() function xiongxin
2024-04-23 16:52 ` Mario Limonciello
2024-04-30 8:45 ` xiongxin
2024-04-30 14:36 ` Mario Limonciello
2024-05-01 4:45 ` xiongxin
2024-05-02 19:04 ` Mario Limonciello
[not found] ` <1714698149088349.0.seg@mailgw.kylinos.cn>
2024-05-03 1:29 ` xiongxin [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4b077ec7-d9da-44fc-a083-1c27afff7e72@kylinos.cn \
--to=xiongxin@kylinos.cn \
--cc=linux-pm@vger.kernel.org \
--cc=mario.limonciello@amd.com \
--cc=rafael@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).