Re: [PATCH] Revert "include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume"

Rafael J. Wysocki
2024-04-22T15:43:46Z

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.