| Message ID | 20240422093619.118278-1-xiongxin@kylinos.cn (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Revert "include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume" | expand |
On 4/22/2024 04:36, xiongxin wrote: > This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce. > > 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. As an alternative, how about exporting and renaming the variable in_suspend in kernel/power/hibernate.c and considering that to tell if the hibernate process is going on? Then it should work just the same as it does at suspend. > > Signed-off-by: xiongxin <xiongxin@kylinos.cn> > > diff --git a/include/linux/suspend.h b/include/linux/suspend.h > index da6ebca3ff77..415483b89b11 100644 > --- a/include/linux/suspend.h > +++ b/include/linux/suspend.h > @@ -503,7 +503,6 @@ static inline void unlock_system_sleep(unsigned int flags) {} > #ifdef CONFIG_PM_SLEEP_DEBUG > extern bool pm_print_times_enabled; > extern bool pm_debug_messages_on; > -extern bool pm_debug_messages_should_print(void); > static inline int pm_dyn_debug_messages_on(void) > { > #ifdef CONFIG_DYNAMIC_DEBUG > @@ -517,14 +516,14 @@ static inline int pm_dyn_debug_messages_on(void) > #endif > #define __pm_pr_dbg(fmt, ...) \ > do { \ > - if (pm_debug_messages_should_print()) \ > + if (pm_debug_messages_on) \ > printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \ > else if (pm_dyn_debug_messages_on()) \ > pr_debug(fmt, ##__VA_ARGS__); \ > } while (0) > #define __pm_deferred_pr_dbg(fmt, ...) \ > do { \ > - if (pm_debug_messages_should_print()) \ > + if (pm_debug_messages_on) \ > printk_deferred(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \ > } while (0) > #else > @@ -542,8 +541,7 @@ static inline int pm_dyn_debug_messages_on(void) > /** > * pm_pr_dbg - print pm sleep debug messages > * > - * If pm_debug_messages_on is enabled and the system is entering/leaving > - * suspend, print message. > + * If pm_debug_messages_on is enabled, print message. > * If pm_debug_messages_on is disabled and CONFIG_DYNAMIC_DEBUG is enabled, > * print message only from instances explicitly enabled on dynamic debug's > * control. > diff --git a/kernel/power/main.c b/kernel/power/main.c > index a9e0693aaf69..aa754241aaa6 100644 > --- a/kernel/power/main.c > +++ b/kernel/power/main.c > @@ -611,12 +611,6 @@ power_attr_ro(pm_wakeup_irq); > > 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; > -} > -EXPORT_SYMBOL_GPL(pm_debug_messages_should_print); > - > static ssize_t pm_debug_messages_show(struct kobject *kobj, > struct kobj_attribute *attr, char *buf) > {
On Mon, Apr 22, 2024 at 4:33 PM Mario Limonciello <mario.limonciello@amd.com> wrote: > > On 4/22/2024 04:36, xiongxin wrote: > > This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce. > > > > 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. > > As an alternative, how about exporting and renaming the variable > in_suspend in kernel/power/hibernate.c and considering that to tell if > the hibernate process is going on? > > Then it should work just the same as it does at suspend. Well, this is not the only part that stopped working AFAICS. I'll queue up the revert. Thanks! > > > > Signed-off-by: xiongxin <xiongxin@kylinos.cn> > > > > diff --git a/include/linux/suspend.h b/include/linux/suspend.h > > index da6ebca3ff77..415483b89b11 100644 > > --- a/include/linux/suspend.h > > +++ b/include/linux/suspend.h > > @@ -503,7 +503,6 @@ static inline void unlock_system_sleep(unsigned int flags) {} > > #ifdef CONFIG_PM_SLEEP_DEBUG > > extern bool pm_print_times_enabled; > > extern bool pm_debug_messages_on; > > -extern bool pm_debug_messages_should_print(void); > > static inline int pm_dyn_debug_messages_on(void) > > { > > #ifdef CONFIG_DYNAMIC_DEBUG > > @@ -517,14 +516,14 @@ static inline int pm_dyn_debug_messages_on(void) > > #endif > > #define __pm_pr_dbg(fmt, ...) \ > > do { \ > > - if (pm_debug_messages_should_print()) \ > > + if (pm_debug_messages_on) \ > > printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \ > > else if (pm_dyn_debug_messages_on()) \ > > pr_debug(fmt, ##__VA_ARGS__); \ > > } while (0) > > #define __pm_deferred_pr_dbg(fmt, ...) \ > > do { \ > > - if (pm_debug_messages_should_print()) \ > > + if (pm_debug_messages_on) \ > > printk_deferred(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \ > > } while (0) > > #else > > @@ -542,8 +541,7 @@ static inline int pm_dyn_debug_messages_on(void) > > /** > > * pm_pr_dbg - print pm sleep debug messages > > * > > - * If pm_debug_messages_on is enabled and the system is entering/leaving > > - * suspend, print message. > > + * If pm_debug_messages_on is enabled, print message. > > * If pm_debug_messages_on is disabled and CONFIG_DYNAMIC_DEBUG is enabled, > > * print message only from instances explicitly enabled on dynamic debug's > > * control. > > diff --git a/kernel/power/main.c b/kernel/power/main.c > > index a9e0693aaf69..aa754241aaa6 100644 > > --- a/kernel/power/main.c > > +++ b/kernel/power/main.c > > @@ -611,12 +611,6 @@ power_attr_ro(pm_wakeup_irq); > > > > 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; > > -} > > -EXPORT_SYMBOL_GPL(pm_debug_messages_should_print); > > - > > static ssize_t pm_debug_messages_show(struct kobject *kobj, > > struct kobj_attribute *attr, char *buf) > > { >
On 4/22/2024 09:45, Rafael J. Wysocki wrote: > On Mon, Apr 22, 2024 at 4:33 PM Mario Limonciello > <mario.limonciello@amd.com> wrote: >> >> On 4/22/2024 04:36, xiongxin wrote: >>> This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce. >>> >>> 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. >> >> As an alternative, how about exporting and renaming the variable >> in_suspend in kernel/power/hibernate.c and considering that to tell if >> the hibernate process is going on? >> >> Then it should work just the same as it does at suspend. > > Well, this is not the only part that stopped working AFAICS. I'll > queue up the revert. I just tested the revert to see what happens to other drivers but it's going to have more collateral damage. ERROR: modpost: "pm_debug_messages_on" [drivers/platform/x86/amd/pmc/amd-pmc.ko] undefined! > > Thanks! > >>> >>> Signed-off-by: xiongxin <xiongxin@kylinos.cn> >>> >>> diff --git a/include/linux/suspend.h b/include/linux/suspend.h >>> index da6ebca3ff77..415483b89b11 100644 >>> --- a/include/linux/suspend.h >>> +++ b/include/linux/suspend.h >>> @@ -503,7 +503,6 @@ static inline void unlock_system_sleep(unsigned int flags) {} >>> #ifdef CONFIG_PM_SLEEP_DEBUG >>> extern bool pm_print_times_enabled; >>> extern bool pm_debug_messages_on; >>> -extern bool pm_debug_messages_should_print(void); >>> static inline int pm_dyn_debug_messages_on(void) >>> { >>> #ifdef CONFIG_DYNAMIC_DEBUG >>> @@ -517,14 +516,14 @@ static inline int pm_dyn_debug_messages_on(void) >>> #endif >>> #define __pm_pr_dbg(fmt, ...) \ >>> do { \ >>> - if (pm_debug_messages_should_print()) \ >>> + if (pm_debug_messages_on) \ >>> printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \ >>> else if (pm_dyn_debug_messages_on()) \ >>> pr_debug(fmt, ##__VA_ARGS__); \ >>> } while (0) >>> #define __pm_deferred_pr_dbg(fmt, ...) \ >>> do { \ >>> - if (pm_debug_messages_should_print()) \ >>> + if (pm_debug_messages_on) \ >>> printk_deferred(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \ >>> } while (0) >>> #else >>> @@ -542,8 +541,7 @@ static inline int pm_dyn_debug_messages_on(void) >>> /** >>> * pm_pr_dbg - print pm sleep debug messages >>> * >>> - * If pm_debug_messages_on is enabled and the system is entering/leaving >>> - * suspend, print message. >>> + * If pm_debug_messages_on is enabled, print message. >>> * If pm_debug_messages_on is disabled and CONFIG_DYNAMIC_DEBUG is enabled, >>> * print message only from instances explicitly enabled on dynamic debug's >>> * control. >>> diff --git a/kernel/power/main.c b/kernel/power/main.c >>> index a9e0693aaf69..aa754241aaa6 100644 >>> --- a/kernel/power/main.c >>> +++ b/kernel/power/main.c >>> @@ -611,12 +611,6 @@ power_attr_ro(pm_wakeup_irq); >>> >>> 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; >>> -} >>> -EXPORT_SYMBOL_GPL(pm_debug_messages_should_print); >>> - >>> static ssize_t pm_debug_messages_show(struct kobject *kobj, >>> struct kobj_attribute *attr, char *buf) >>> { >>
On Mon, Apr 22, 2024 at 5:02 PM Mario Limonciello <mario.limonciello@amd.com> wrote: > > On 4/22/2024 09:45, Rafael J. Wysocki wrote: > > On Mon, Apr 22, 2024 at 4:33 PM Mario Limonciello > > <mario.limonciello@amd.com> wrote: > >> > >> On 4/22/2024 04:36, xiongxin wrote: > >>> This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce. > >>> > >>> 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. > >> > >> As an alternative, how about exporting and renaming the variable > >> in_suspend in kernel/power/hibernate.c and considering that to tell if > >> the hibernate process is going on? > >> > >> Then it should work just the same as it does at suspend. > > > > Well, this is not the only part that stopped working AFAICS. I'll > > queue up the revert. > > I just tested the revert to see what happens to other drivers but it's > going to have more collateral damage. > > ERROR: modpost: "pm_debug_messages_on" > [drivers/platform/x86/amd/pmc/amd-pmc.ko] undefined! What about removing the "pm_suspend_target_state != PM_SUSPEND_ON" part from pm_debug_messages_should_print()? This should be as good as the revert from the POV of restoring the previous functionality.
On 4/22/2024 10:18, Rafael J. Wysocki wrote: > On Mon, Apr 22, 2024 at 5:02 PM Mario Limonciello > <mario.limonciello@amd.com> wrote: >> >> On 4/22/2024 09:45, Rafael J. Wysocki wrote: >>> On Mon, Apr 22, 2024 at 4:33 PM Mario Limonciello >>> <mario.limonciello@amd.com> wrote: >>>> >>>> On 4/22/2024 04:36, xiongxin wrote: >>>>> This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce. >>>>> >>>>> 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. >>>> >>>> As an alternative, how about exporting and renaming the variable >>>> in_suspend in kernel/power/hibernate.c and considering that to tell if >>>> the hibernate process is going on? >>>> >>>> Then it should work just the same as it does at suspend. >>> >>> Well, this is not the only part that stopped working AFAICS. I'll >>> queue up the revert. >> >> I just tested the revert to see what happens to other drivers but it's >> going to have more collateral damage. >> >> ERROR: modpost: "pm_debug_messages_on" >> [drivers/platform/x86/amd/pmc/amd-pmc.ko] undefined! > > What about removing the "pm_suspend_target_state != PM_SUSPEND_ON" > part from pm_debug_messages_should_print()? > > This should be as good as the revert from the POV of restoring the > previous functionality. That would probably help this reported issue but it's going to be REALLY noisy for the pinctrl-amd driver for anyone that sets /sys/power/pm_debug_messages. There is a message in that driver that is emitted whenever a GPIO is active and pm_debug_messages is set. It's a really useful message for tracking down which GPIO woke the system up as the IRQ that is active is the GPIO controller master IRQ not an IRQ for the GPIO. But if that change is made anyone who sets /sys/power/pm_debug_messages is going to see their kernel ring buffer flooded with every since interrupt associated with an I2C touchpad attention pin (for example). So if the desire really is to back all this out, I think we need to also back out other users of pm_pr_dbg() too.
On Mon, Apr 22, 2024 at 5:25 PM Mario Limonciello <mario.limonciello@amd.com> wrote: > > On 4/22/2024 10:18, Rafael J. Wysocki wrote: > > On Mon, Apr 22, 2024 at 5:02 PM Mario Limonciello > > <mario.limonciello@amd.com> wrote: > >> > >> On 4/22/2024 09:45, Rafael J. Wysocki wrote: > >>> On Mon, Apr 22, 2024 at 4:33 PM Mario Limonciello > >>> <mario.limonciello@amd.com> wrote: > >>>> > >>>> On 4/22/2024 04:36, xiongxin wrote: > >>>>> This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce. > >>>>> > >>>>> 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. > >>>> > >>>> As an alternative, how about exporting and renaming the variable > >>>> in_suspend in kernel/power/hibernate.c and considering that to tell if > >>>> the hibernate process is going on? > >>>> > >>>> Then it should work just the same as it does at suspend. > >>> > >>> Well, this is not the only part that stopped working AFAICS. I'll > >>> queue up the revert. > >> > >> I just tested the revert to see what happens to other drivers but it's > >> going to have more collateral damage. > >> > >> ERROR: modpost: "pm_debug_messages_on" > >> [drivers/platform/x86/amd/pmc/amd-pmc.ko] undefined! > > > > What about removing the "pm_suspend_target_state != PM_SUSPEND_ON" > > part from pm_debug_messages_should_print()? > > > > This should be as good as the revert from the POV of restoring the > > previous functionality. > > That would probably help this reported issue but it's going to be REALLY > noisy for the pinctrl-amd driver for anyone that sets > /sys/power/pm_debug_messages. > > There is a message in that driver that is emitted whenever a GPIO is > active and pm_debug_messages is set. > > It's a really useful message for tracking down which GPIO woke the > system up as the IRQ that is active is the GPIO controller master IRQ > not an IRQ for the GPIO. > > But if that change is made anyone who sets /sys/power/pm_debug_messages > is going to see their kernel ring buffer flooded with every since > interrupt associated with an I2C touchpad attention pin (for example). > > So if the desire really is to back all this out, I think we need to also > back out other users of pm_pr_dbg() too. OK, so it needs to check hibernate_atomic in addition to pm_suspend_target_state.
On 4/22/2024 10:43, Rafael J. Wysocki wrote: > On Mon, Apr 22, 2024 at 5:25 PM Mario Limonciello > <mario.limonciello@amd.com> wrote: >> >> On 4/22/2024 10:18, Rafael J. Wysocki wrote: >>> On Mon, Apr 22, 2024 at 5:02 PM Mario Limonciello >>> <mario.limonciello@amd.com> wrote: >>>> >>>> On 4/22/2024 09:45, Rafael J. Wysocki wrote: >>>>> On Mon, Apr 22, 2024 at 4:33 PM Mario Limonciello >>>>> <mario.limonciello@amd.com> wrote: >>>>>> >>>>>> On 4/22/2024 04:36, xiongxin wrote: >>>>>>> This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce. >>>>>>> >>>>>>> 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. >>>>>> >>>>>> As an alternative, how about exporting and renaming the variable >>>>>> in_suspend in kernel/power/hibernate.c and considering that to tell if >>>>>> the hibernate process is going on? >>>>>> >>>>>> Then it should work just the same as it does at suspend. >>>>> >>>>> Well, this is not the only part that stopped working AFAICS. I'll >>>>> queue up the revert. >>>> >>>> I just tested the revert to see what happens to other drivers but it's >>>> going to have more collateral damage. >>>> >>>> ERROR: modpost: "pm_debug_messages_on" >>>> [drivers/platform/x86/amd/pmc/amd-pmc.ko] undefined! >>> >>> What about removing the "pm_suspend_target_state != PM_SUSPEND_ON" >>> part from pm_debug_messages_should_print()? >>> >>> This should be as good as the revert from the POV of restoring the >>> previous functionality. >> >> That would probably help this reported issue but it's going to be REALLY >> noisy for the pinctrl-amd driver for anyone that sets >> /sys/power/pm_debug_messages. >> >> There is a message in that driver that is emitted whenever a GPIO is >> active and pm_debug_messages is set. >> >> It's a really useful message for tracking down which GPIO woke the >> system up as the IRQ that is active is the GPIO controller master IRQ >> not an IRQ for the GPIO. >> >> But if that change is made anyone who sets /sys/power/pm_debug_messages >> is going to see their kernel ring buffer flooded with every since >> interrupt associated with an I2C touchpad attention pin (for example). >> >> So if the desire really is to back all this out, I think we need to also >> back out other users of pm_pr_dbg() too. > > OK, so it needs to check hibernate_atomic in addition to > pm_suspend_target_state. Yeah, that sounds great to me. Tangentially related to the discussion; how would you feel about a /sys/power/pm_wakeup_gpio? Or /sys/power/pm_wakeup_gpios? If we did the plural and used a comma separated list we could probably axe the message I mentioned above from pinctrl-amd all together. Thanks,
On Mon, Apr 22, 2024 at 5:54 PM Mario Limonciello <mario.limonciello@amd.com> wrote: > > On 4/22/2024 10:43, Rafael J. Wysocki wrote: > > On Mon, Apr 22, 2024 at 5:25 PM Mario Limonciello > > <mario.limonciello@amd.com> wrote: > >> > >> On 4/22/2024 10:18, Rafael J. Wysocki wrote: > >>> On Mon, Apr 22, 2024 at 5:02 PM Mario Limonciello > >>> <mario.limonciello@amd.com> wrote: > >>>> > >>>> On 4/22/2024 09:45, Rafael J. Wysocki wrote: > >>>>> On Mon, Apr 22, 2024 at 4:33 PM Mario Limonciello > >>>>> <mario.limonciello@amd.com> wrote: > >>>>>> > >>>>>> On 4/22/2024 04:36, xiongxin wrote: > >>>>>>> This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce. > >>>>>>> > >>>>>>> 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. > >>>>>> > >>>>>> As an alternative, how about exporting and renaming the variable > >>>>>> in_suspend in kernel/power/hibernate.c and considering that to tell if > >>>>>> the hibernate process is going on? > >>>>>> > >>>>>> Then it should work just the same as it does at suspend. > >>>>> > >>>>> Well, this is not the only part that stopped working AFAICS. I'll > >>>>> queue up the revert. > >>>> > >>>> I just tested the revert to see what happens to other drivers but it's > >>>> going to have more collateral damage. > >>>> > >>>> ERROR: modpost: "pm_debug_messages_on" > >>>> [drivers/platform/x86/amd/pmc/amd-pmc.ko] undefined! > >>> > >>> What about removing the "pm_suspend_target_state != PM_SUSPEND_ON" > >>> part from pm_debug_messages_should_print()? > >>> > >>> This should be as good as the revert from the POV of restoring the > >>> previous functionality. > >> > >> That would probably help this reported issue but it's going to be REALLY > >> noisy for the pinctrl-amd driver for anyone that sets > >> /sys/power/pm_debug_messages. > >> > >> There is a message in that driver that is emitted whenever a GPIO is > >> active and pm_debug_messages is set. > >> > >> It's a really useful message for tracking down which GPIO woke the > >> system up as the IRQ that is active is the GPIO controller master IRQ > >> not an IRQ for the GPIO. > >> > >> But if that change is made anyone who sets /sys/power/pm_debug_messages > >> is going to see their kernel ring buffer flooded with every since > >> interrupt associated with an I2C touchpad attention pin (for example). > >> > >> So if the desire really is to back all this out, I think we need to also > >> back out other users of pm_pr_dbg() too. > > > > OK, so it needs to check hibernate_atomic in addition to > > pm_suspend_target_state. > > Yeah, that sounds great to me. OK > Tangentially related to the discussion; how would you feel about a > /sys/power/pm_wakeup_gpio? Or /sys/power/pm_wakeup_gpios? > > If we did the plural and used a comma separated list we could probably > axe the message I mentioned above from pinctrl-amd all together. That would be too specific IMV. The whole idea with pm_debug_messages is to switch them all on or off in one go.
在 2024/4/23 00:04, Rafael J. Wysocki 写道: > On Mon, Apr 22, 2024 at 5:54 PM Mario Limonciello > <mario.limonciello@amd.com> wrote: >> On 4/22/2024 10:43, Rafael J. Wysocki wrote: >>> On Mon, Apr 22, 2024 at 5:25 PM Mario Limonciello >>> <mario.limonciello@amd.com> wrote: >>>> On 4/22/2024 10:18, Rafael J. Wysocki wrote: >>>>> On Mon, Apr 22, 2024 at 5:02 PM Mario Limonciello >>>>> <mario.limonciello@amd.com> wrote: >>>>>> On 4/22/2024 09:45, Rafael J. Wysocki wrote: >>>>>>> On Mon, Apr 22, 2024 at 4:33 PM Mario Limonciello >>>>>>> <mario.limonciello@amd.com> wrote: >>>>>>>> On 4/22/2024 04:36, xiongxin wrote: >>>>>>>>> This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce. >>>>>>>>> >>>>>>>>> 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. >>>>>>>> As an alternative, how about exporting and renaming the variable >>>>>>>> in_suspend in kernel/power/hibernate.c and considering that to tell if >>>>>>>> the hibernate process is going on? >>>>>>>> >>>>>>>> Then it should work just the same as it does at suspend. >>>>>>> Well, this is not the only part that stopped working AFAICS. I'll >>>>>>> queue up the revert. >>>>>> I just tested the revert to see what happens to other drivers but it's >>>>>> going to have more collateral damage. >>>>>> >>>>>> ERROR: modpost: "pm_debug_messages_on" >>>>>> [drivers/platform/x86/amd/pmc/amd-pmc.ko] undefined! The revert has simply removed the pm_debug_messages_should_print() func, there is no reference to this function anywhere else in the source code, and drivers/platform/x86/amd/pmc/ path does not reference pm_debug_messages_on or this function. >>>>> What about removing the "pm_suspend_target_state != PM_SUSPEND_ON" >>>>> part from pm_debug_messages_should_print()? >>>>> >>>>> This should be as good as the revert from the POV of restoring the >>>>> previous functionality. >>>> That would probably help this reported issue but it's going to be REALLY >>>> noisy for the pinctrl-amd driver for anyone that sets >>>> /sys/power/pm_debug_messages. >>>> >>>> There is a message in that driver that is emitted whenever a GPIO is >>>> active and pm_debug_messages is set. >>>> >>>> It's a really useful message for tracking down which GPIO woke the >>>> system up as the IRQ that is active is the GPIO controller master IRQ >>>> not an IRQ for the GPIO. >>>> >>>> But if that change is made anyone who sets /sys/power/pm_debug_messages >>>> is going to see their kernel ring buffer flooded with every since >>>> interrupt associated with an I2C touchpad attention pin (for example). >>>> >>>> So if the desire really is to back all this out, I think we need to also >>>> back out other users of pm_pr_dbg() too. >>> OK, so it needs to check hibernate_atomic in addition to >>> pm_suspend_target_state. >> Yeah, that sounds great to me. > OK > >> Tangentially related to the discussion; how would you feel about a >> /sys/power/pm_wakeup_gpio? Or /sys/power/pm_wakeup_gpios? >> >> If we did the plural and used a comma separated list we could probably >> axe the message I mentioned above from pinctrl-amd all together. > That would be too specific IMV. > > The whole idea with pm_debug_messages is to switch them all on or off in one go.
On 4/22/2024 19:59, xiongxin wrote: > 在 2024/4/23 00:04, Rafael J. Wysocki 写道: >> On Mon, Apr 22, 2024 at 5:54 PM Mario Limonciello >> <mario.limonciello@amd.com> wrote: >>> On 4/22/2024 10:43, Rafael J. Wysocki wrote: >>>> On Mon, Apr 22, 2024 at 5:25 PM Mario Limonciello >>>> <mario.limonciello@amd.com> wrote: >>>>> On 4/22/2024 10:18, Rafael J. Wysocki wrote: >>>>>> On Mon, Apr 22, 2024 at 5:02 PM Mario Limonciello >>>>>> <mario.limonciello@amd.com> wrote: >>>>>>> On 4/22/2024 09:45, Rafael J. Wysocki wrote: >>>>>>>> On Mon, Apr 22, 2024 at 4:33 PM Mario Limonciello >>>>>>>> <mario.limonciello@amd.com> wrote: >>>>>>>>> On 4/22/2024 04:36, xiongxin wrote: >>>>>>>>>> This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce. >>>>>>>>>> >>>>>>>>>> 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. >>>>>>>>> As an alternative, how about exporting and renaming the variable >>>>>>>>> in_suspend in kernel/power/hibernate.c and considering that to >>>>>>>>> tell if >>>>>>>>> the hibernate process is going on? >>>>>>>>> >>>>>>>>> Then it should work just the same as it does at suspend. >>>>>>>> Well, this is not the only part that stopped working AFAICS. I'll >>>>>>>> queue up the revert. >>>>>>> I just tested the revert to see what happens to other drivers but >>>>>>> it's >>>>>>> going to have more collateral damage. >>>>>>> >>>>>>> ERROR: modpost: "pm_debug_messages_on" >>>>>>> [drivers/platform/x86/amd/pmc/amd-pmc.ko] undefined! > > The revert has simply removed the pm_debug_messages_should_print() func, > there is no reference to > > this function anywhere else in the source code, and > drivers/platform/x86/amd/pmc/ path does not > > reference pm_debug_messages_on or this function. amd_pmc_idlemask_read() uses the pm_pr_dbg() macro which uses the __pm_pr_dbg() macro. In your revert the pm_debug_messages_on variable is not exported like pm_debug_messages_should_print() was in the original commit. As amd-pmc is compiled as a module it won't be able to access that variable. > >>>>>> What about removing the "pm_suspend_target_state != PM_SUSPEND_ON" >>>>>> part from pm_debug_messages_should_print()? >>>>>> >>>>>> This should be as good as the revert from the POV of restoring the >>>>>> previous functionality. >>>>> That would probably help this reported issue but it's going to be >>>>> REALLY >>>>> noisy for the pinctrl-amd driver for anyone that sets >>>>> /sys/power/pm_debug_messages. >>>>> >>>>> There is a message in that driver that is emitted whenever a GPIO is >>>>> active and pm_debug_messages is set. >>>>> >>>>> It's a really useful message for tracking down which GPIO woke the >>>>> system up as the IRQ that is active is the GPIO controller master IRQ >>>>> not an IRQ for the GPIO. >>>>> >>>>> But if that change is made anyone who sets >>>>> /sys/power/pm_debug_messages >>>>> is going to see their kernel ring buffer flooded with every since >>>>> interrupt associated with an I2C touchpad attention pin (for example). >>>>> >>>>> So if the desire really is to back all this out, I think we need to >>>>> also >>>>> back out other users of pm_pr_dbg() too. >>>> OK, so it needs to check hibernate_atomic in addition to >>>> pm_suspend_target_state. >>> Yeah, that sounds great to me. >> OK >> >>> Tangentially related to the discussion; how would you feel about a >>> /sys/power/pm_wakeup_gpio? Or /sys/power/pm_wakeup_gpios? >>> >>> If we did the plural and used a comma separated list we could probably >>> axe the message I mentioned above from pinctrl-amd all together. >> That would be too specific IMV. >> >> The whole idea with pm_debug_messages is to switch them all on or off >> in one go. > >
diff --git a/include/linux/suspend.h b/include/linux/suspend.h index da6ebca3ff77..415483b89b11 100644 --- a/include/linux/suspend.h +++ b/include/linux/suspend.h @@ -503,7 +503,6 @@ static inline void unlock_system_sleep(unsigned int flags) {} #ifdef CONFIG_PM_SLEEP_DEBUG extern bool pm_print_times_enabled; extern bool pm_debug_messages_on; -extern bool pm_debug_messages_should_print(void); static inline int pm_dyn_debug_messages_on(void) { #ifdef CONFIG_DYNAMIC_DEBUG @@ -517,14 +516,14 @@ static inline int pm_dyn_debug_messages_on(void) #endif #define __pm_pr_dbg(fmt, ...) \ do { \ - if (pm_debug_messages_should_print()) \ + if (pm_debug_messages_on) \ printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \ else if (pm_dyn_debug_messages_on()) \ pr_debug(fmt, ##__VA_ARGS__); \ } while (0) #define __pm_deferred_pr_dbg(fmt, ...) \ do { \ - if (pm_debug_messages_should_print()) \ + if (pm_debug_messages_on) \ printk_deferred(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \ } while (0) #else @@ -542,8 +541,7 @@ static inline int pm_dyn_debug_messages_on(void) /** * pm_pr_dbg - print pm sleep debug messages * - * If pm_debug_messages_on is enabled and the system is entering/leaving - * suspend, print message. + * If pm_debug_messages_on is enabled, print message. * If pm_debug_messages_on is disabled and CONFIG_DYNAMIC_DEBUG is enabled, * print message only from instances explicitly enabled on dynamic debug's * control. diff --git a/kernel/power/main.c b/kernel/power/main.c index a9e0693aaf69..aa754241aaa6 100644 --- a/kernel/power/main.c +++ b/kernel/power/main.c @@ -611,12 +611,6 @@ power_attr_ro(pm_wakeup_irq); 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; -} -EXPORT_SYMBOL_GPL(pm_debug_messages_should_print); - static ssize_t pm_debug_messages_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) {
This reverts commit cdb8c100d8a4b4e31c829724e40b4fdf32977cce. 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. Signed-off-by: xiongxin <xiongxin@kylinos.cn>