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

Rafael J. Wysocki
2024-04-22T16:04:16Z

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.