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

Mario Limonciello
2024-04-22T15:54:23Z

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,