| Message ID | 20240423081723.412237-1-xiongxin@kylinos.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PM: sleep: Optimize the pm_debug_messages_should_print() function | expand |
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?
在 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.
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.
在 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);
>>> 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.
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.
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);
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"). ---