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

Mario Limonciello
2024-04-23T03:42:12Z

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