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

xiongxin
2024-04-23T01:10:25Z

在 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.