[PATCH v2 1/4] include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume

[PATCH v2 1/4] include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume

[Mario Limonciello]

All uses in the kernel are currently already oriented around
suspend/resume. As some other parts of the kernel may also use these
messages in functions that could also be used outside of
suspend/resume, only enable in suspend/resume path.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 include/linux/suspend.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index d0d4598a7b3f..a40f2e667e09 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -564,7 +564,8 @@ static inline int pm_dyn_debug_messages_on(void)
 #endif
 #define __pm_pr_dbg(fmt, ...)					\
 	do {							\
-		if (pm_debug_messages_on)			\
+		if (pm_debug_messages_on &&			\
+		    pm_suspend_target_state != PM_SUSPEND_ON)	\
 			printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__);	\
 		else if (pm_dyn_debug_messages_on())		\
 			pr_debug(fmt, ##__VA_ARGS__);	\
@@ -589,7 +590,8 @@ static inline int pm_dyn_debug_messages_on(void)
 /**
  * pm_pr_dbg - print pm sleep debug messages
  *
- * If pm_debug_messages_on is enabled, print message.
+ * If pm_debug_messages_on is enabled and the system is entering/leaving
+ *      suspend, print message.
  * If pm_debug_messages_on is disabled and CONFIG_DYNAMIC_DEBUG is enabled,
  *	print message only from instances explicitly enabled on dynamic debug's
  *	control.

base-commit: 42dfdd08422dec99bfe526072063f65c0b9fb7d2
-- 
2.34.1

[PATCH v2 2/4] ACPI: x86: Add pm_debug_messages for LPS0 _DSM state tracking

[Mario Limonciello]

Enabling debugging messages for the state requires turning on dynamic
debugging for the file. To make it more accessible, use
`pm_debug_messages` and clearer strings for what is happening.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v1->v2:
 * Remove an unnecessary stray semicolon

 drivers/acpi/x86/s2idle.c | 52 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 46 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index e499c60c4579..7681f6ecab67 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -59,6 +59,7 @@ static int lps0_dsm_func_mask;
 
 static guid_t lps0_dsm_guid_microsoft;
 static int lps0_dsm_func_mask_microsoft;
+static int lps0_dsm_state;
 
 /* Device constraint entry structure */
 struct lpi_device_info {
@@ -320,6 +321,44 @@ static void lpi_check_constraints(void)
 	}
 }
 
+static bool acpi_s2idle_vendor_amd(void)
+{
+	return boot_cpu_data.x86_vendor == X86_VENDOR_AMD;
+}
+
+static const char *acpi_sleep_dsm_state_to_str(unsigned int state)
+{
+	if (lps0_dsm_func_mask_microsoft || !acpi_s2idle_vendor_amd()) {
+		switch (state) {
+		case ACPI_LPS0_SCREEN_OFF:
+			return "screen off";
+		case ACPI_LPS0_SCREEN_ON:
+			return "screen on";
+		case ACPI_LPS0_ENTRY:
+			return "lps0 entry";
+		case ACPI_LPS0_EXIT:
+			return "lps0 exit";
+		case ACPI_LPS0_MS_ENTRY:
+			return "lps0 ms entry";
+		case ACPI_LPS0_MS_EXIT:
+			return "lps0 ms exit";
+		}
+	} else {
+		switch (state) {
+		case ACPI_LPS0_SCREEN_ON_AMD:
+			return "screen on";
+		case ACPI_LPS0_SCREEN_OFF_AMD:
+			return "screen off";
+		case ACPI_LPS0_ENTRY_AMD:
+			return "lps0 entry";
+		case ACPI_LPS0_EXIT_AMD:
+			return "lps0 exit";
+		}
+	}
+
+	return "unknown";
+}
+
 static void acpi_sleep_run_lps0_dsm(unsigned int func, unsigned int func_mask, guid_t dsm_guid)
 {
 	union acpi_object *out_obj;
@@ -331,14 +370,15 @@ static void acpi_sleep_run_lps0_dsm(unsigned int func, unsigned int func_mask, g
 					rev_id, func, NULL);
 	ACPI_FREE(out_obj);
 
-	acpi_handle_debug(lps0_device_handle, "_DSM function %u evaluation %s\n",
-			  func, out_obj ? "successful" : "failed");
+	lps0_dsm_state = func;
+	if (pm_debug_messages_on) {
+		acpi_handle_info(lps0_device_handle,
+				"%s transitioned to state %s\n",
+				 out_obj ? "Successfully" : "Failed to",
+				 acpi_sleep_dsm_state_to_str(lps0_dsm_state));
+	}
 }
 
-static bool acpi_s2idle_vendor_amd(void)
-{
-	return boot_cpu_data.x86_vendor == X86_VENDOR_AMD;
-}
 
 static int validate_dsm(acpi_handle handle, const char *uuid, int rev, guid_t *dsm_guid)
 {
-- 
2.34.1

Re: [PATCH v2 2/4] ACPI: x86: Add pm_debug_messages for LPS0 _DSM state tracking

[andy.shevchenko]

Mon, May 22, 2023 at 03:00:31PM -0500, Mario Limonciello kirjoitti:
> Enabling debugging messages for the state requires turning on dynamic
> debugging for the file. To make it more accessible, use
> `pm_debug_messages` and clearer strings for what is happening.

...

> +		switch (state) {
> +		case ACPI_LPS0_SCREEN_OFF:
> +			return "screen off";
> +		case ACPI_LPS0_SCREEN_ON:
> +			return "screen on";
> +		case ACPI_LPS0_ENTRY:
> +			return "lps0 entry";
> +		case ACPI_LPS0_EXIT:
> +			return "lps0 exit";
> +		case ACPI_LPS0_MS_ENTRY:
> +			return "lps0 ms entry";
> +		case ACPI_LPS0_MS_EXIT:
> +			return "lps0 ms exit";

No default?

> +		}

...

> +		switch (state) {
> +		case ACPI_LPS0_SCREEN_ON_AMD:
> +			return "screen on";
> +		case ACPI_LPS0_SCREEN_OFF_AMD:
> +			return "screen off";
> +		case ACPI_LPS0_ENTRY_AMD:
> +			return "lps0 entry";
> +		case ACPI_LPS0_EXIT_AMD:
> +			return "lps0 exit";
> +		}
> +	}
> +
> +	return "unknown";

Make it default in each switch-case. That way we might have an option to alter
them if needed.

...

> -	acpi_handle_debug(lps0_device_handle, "_DSM function %u evaluation %s\n",
> -			  func, out_obj ? "successful" : "failed");
> +	lps0_dsm_state = func;
> +	if (pm_debug_messages_on) {
> +		acpi_handle_info(lps0_device_handle,
> +				"%s transitioned to state %s\n",
> +				 out_obj ? "Successfully" : "Failed to",
> +				 acpi_sleep_dsm_state_to_str(lps0_dsm_state));
> +	}

Can we keep the original choice (i.e. 

	? "successful" : "failed");

) unmodified? The rationale is that we migh add something like
str_successful_failed() to the string_helpers.h for wider use and common
standardization.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 3/4] pinctrl: amd: Use pm_pr_dbg to show debugging messages

[Mario Limonciello]

To make the GPIO tracking around suspend easier for end users to
use, link it with pm_debug_messages.  This will make discovering
sources of spurious GPIOs around suspend easier.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pinctrl/pinctrl-amd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index f279b360c20d..43d3530bab48 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -30,6 +30,7 @@
 #include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinconf-generic.h>
 #include <linux/pinctrl/pinmux.h>
+#include <linux/suspend.h>
 
 #include "core.h"
 #include "pinctrl-utils.h"
@@ -636,9 +637,8 @@ static bool do_amd_gpio_irq_handler(int irq, void *dev_id)
 			regval = readl(regs + i);
 
 			if (regval & PIN_IRQ_PENDING)
-				dev_dbg(&gpio_dev->pdev->dev,
-					"GPIO %d is active: 0x%x",
-					irqnr + i, regval);
+				pm_pr_dbg("GPIO %d is active: 0x%x",
+					  irqnr + i, regval);
 
 			/* caused wake on resume context for shared IRQ */
 			if (irq < 0 && (regval & BIT(WAKE_STS_OFF)))
-- 
2.34.1

Re: [PATCH v2 3/4] pinctrl: amd: Use pm_pr_dbg to show debugging messages

[andy.shevchenko]

Mon, May 22, 2023 at 03:00:32PM -0500, Mario Limonciello kirjoitti:
> To make the GPIO tracking around suspend easier for end users to
> use, link it with pm_debug_messages.  This will make discovering
> sources of spurious GPIOs around suspend easier.

Unfortunatelly this has two regressions.

...

> -				dev_dbg(&gpio_dev->pdev->dev,
> -					"GPIO %d is active: 0x%x",
> -					irqnr + i, regval);
> +				pm_pr_dbg("GPIO %d is active: 0x%x",
> +					  irqnr + i, regval);

Regression 1: The device is now omitted from the output.
Regression 2: See https://stackoverflow.com/a/43957671/2511795

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] pinctrl: amd: Use pm_pr_dbg to show debugging messages

[Rafael J. Wysocki]

On Tue, May 23, 2023 at 6:55 PM <andy.shevchenko@gmail.com> wrote:
>
> Mon, May 22, 2023 at 03:00:32PM -0500, Mario Limonciello kirjoitti:
> > To make the GPIO tracking around suspend easier for end users to
> > use, link it with pm_debug_messages.  This will make discovering
> > sources of spurious GPIOs around suspend easier.
>
> Unfortunatelly this has two regressions.
>
> ...
>
> > -                             dev_dbg(&gpio_dev->pdev->dev,
> > -                                     "GPIO %d is active: 0x%x",
> > -                                     irqnr + i, regval);
> > +                             pm_pr_dbg("GPIO %d is active: 0x%x",
> > +                                       irqnr + i, regval);
>
> Regression 1: The device is now omitted from the output.

Right.

> Regression 2: See https://stackoverflow.com/a/43957671/2511795

Care to elaborate?  I'm not sure what you mean exactly.

Re: [PATCH v2 3/4] pinctrl: amd: Use pm_pr_dbg to show debugging messages

[Andy Shevchenko]

On Wed, May 24, 2023 at 9:28 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, May 23, 2023 at 6:55 PM <andy.shevchenko@gmail.com> wrote:
> > Mon, May 22, 2023 at 03:00:32PM -0500, Mario Limonciello kirjoitti:

...

> > > -                             dev_dbg(&gpio_dev->pdev->dev,
> > > -                                     "GPIO %d is active: 0x%x",
> > > -                                     irqnr + i, regval);
> > > +                             pm_pr_dbg("GPIO %d is active: 0x%x",
> > > +                                       irqnr + i, regval);
> >
> > Regression 1: The device is now omitted from the output.
>
> Right.
>
> > Regression 2: See https://stackoverflow.com/a/43957671/2511795
>
> Care to elaborate?  I'm not sure what you mean exactly.

dev_dbg has 3 cases how it prints its content:
1/ With dynamic debug when it's enabled.
2/ With -DDEBUG if it's defined for the certain file(s) in the Makefile.
3/ No print.

pm_pr_dbg relies on CONFIG_PM_SLEEP_DEBUG, pm_debug_messages_on and
not on -DDEBUG. I haven't checked all relations between those 3, but
it seems to me that DEBUG is not equivalent to the others.
CONFIG_PM_SLEEP_DEBUG=n prevents printing with the dynamic debug on.

OTOH I dunno how this is relevant to the functionality of the driver
in question. Maybe it's okay to have such changes.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] pinctrl: amd: Use pm_pr_dbg to show debugging messages

[Limonciello, Mario]

On 5/24/2023 2:57 PM, Andy Shevchenko wrote:
> On Wed, May 24, 2023 at 9:28 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Tue, May 23, 2023 at 6:55 PM <andy.shevchenko@gmail.com> wrote:
>>> Mon, May 22, 2023 at 03:00:32PM -0500, Mario Limonciello kirjoitti:
> ...
>
>>>> -                             dev_dbg(&gpio_dev->pdev->dev,
>>>> -                                     "GPIO %d is active: 0x%x",
>>>> -                                     irqnr + i, regval);
>>>> +                             pm_pr_dbg("GPIO %d is active: 0x%x",
>>>> +                                       irqnr + i, regval);
>>> Regression 1: The device is now omitted from the output.
>> Right.
>>
>>> Regression 2: See https://stackoverflow.com/a/43957671/2511795
>> Care to elaborate?  I'm not sure what you mean exactly.
> dev_dbg has 3 cases how it prints its content:
> 1/ With dynamic debug when it's enabled.
> 2/ With -DDEBUG if it's defined for the certain file(s) in the Makefile.
> 3/ No print.
>
> pm_pr_dbg relies on CONFIG_PM_SLEEP_DEBUG, pm_debug_messages_on and
> not on -DDEBUG. I haven't checked all relations between those 3, but
> it seems to me that DEBUG is not equivalent to the others.
> CONFIG_PM_SLEEP_DEBUG=n prevents printing with the dynamic debug on.
>
> OTOH I dunno how this is relevant to the functionality of the driver
> in question. Maybe it's okay to have such changes.

The main reason for this debug statement in the first
place was for debugging sources of spurious wakeups.

As the statement is in the interrupt handler, turning
it on at "runtime" usually makes for a very noisy kernel
log because things like I2C touchpad will fire interrupts
constantly.

[PATCH v2 4/4] platform/x86/amd: pmc: Use pm_pr_dbg() for suspend related messages

[Mario Limonciello]

Using pm_pr_dbg() allows users to toggle `/sys/power/pm_debug_messages`
as a single knob to turn on messages that amd-pmc can emit to aid in
any s2idle debugging.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/amd/pmc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
index 427905714f79..1304cd6f13f6 100644
--- a/drivers/platform/x86/amd/pmc.c
+++ b/drivers/platform/x86/amd/pmc.c
@@ -543,7 +543,7 @@ static int amd_pmc_idlemask_read(struct amd_pmc_dev *pdev, struct device *dev,
 	}
 
 	if (dev)
-		dev_dbg(pdev->dev, "SMU idlemask s0i3: 0x%x\n", val);
+		pm_pr_dbg("SMU idlemask s0i3: 0x%x\n", val);
 
 	if (s)
 		seq_printf(s, "SMU idlemask : 0x%x\n", val);
@@ -769,7 +769,7 @@ static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
 
 	*arg |= (duration << 16);
 	rc = rtc_alarm_irq_enable(rtc_device, 0);
-	dev_dbg(pdev->dev, "wakeup timer programmed for %lld seconds\n", duration);
+	pm_pr_dbg("wakeup timer programmed for %lld seconds\n", duration);
 
 	return rc;
 }
-- 
2.34.1

Re: [PATCH v2 4/4] platform/x86/amd: pmc: Use pm_pr_dbg() for suspend related messages

[Hans de Goede]

Hi Mario,

On 5/22/23 22:00, Mario Limonciello wrote:
> Using pm_pr_dbg() allows users to toggle `/sys/power/pm_debug_messages`
> as a single knob to turn on messages that amd-pmc can emit to aid in
> any s2idle debugging.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/platform/x86/amd/pmc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
> index 427905714f79..1304cd6f13f6 100644
> --- a/drivers/platform/x86/amd/pmc.c
> +++ b/drivers/platform/x86/amd/pmc.c
> @@ -543,7 +543,7 @@ static int amd_pmc_idlemask_read(struct amd_pmc_dev *pdev, struct device *dev,
>  	}
>  
>  	if (dev)
> -		dev_dbg(pdev->dev, "SMU idlemask s0i3: 0x%x\n", val);
> +		pm_pr_dbg("SMU idlemask s0i3: 0x%x\n", val);
>  
>  	if (s)
>  		seq_printf(s, "SMU idlemask : 0x%x\n", val);

This does not compile, amd/pmc.c may be build as an amd-pmc.ko module
and currently the pm_debug_messages_on flag used by pm_pr_dbg()
is not exported to modules:

  CC [M]  drivers/platform/x86/amd/pmc.o
  LD [M]  drivers/platform/x86/amd/amd-pmc.o
  MODPOST Module.symvers
ERROR: modpost: "pm_debug_messages_on" [drivers/platform/x86/amd/amd-pmc.ko] undefined!
make[1]: *** [scripts/Makefile.modpost:136: Module.symvers] Error 1
make: *** [Makefile:1978: modpost] Error 2

Regards,

Hans

RE: [PATCH v2 4/4] platform/x86/amd: pmc: Use pm_pr_dbg() for suspend related messages

[Limonciello, Mario]

[AMD Official Use Only - General]

> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: Tuesday, May 23, 2023 6:08 AM
> To: Limonciello, Mario <Mario.Limonciello@amd.com>; rafael@kernel.org;
> linus.walleij@linaro.org
> Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> gpio@vger.kernel.org; platform-driver-x86@vger.kernel.org; linux-
> pm@vger.kernel.org; S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>;
> Natikar, Basavaraj <Basavaraj.Natikar@amd.com>
> Subject: Re: [PATCH v2 4/4] platform/x86/amd: pmc: Use pm_pr_dbg() for
> suspend related messages
>
> Hi Mario,
>
> On 5/22/23 22:00, Mario Limonciello wrote:
> > Using pm_pr_dbg() allows users to toggle
> `/sys/power/pm_debug_messages`
> > as a single knob to turn on messages that amd-pmc can emit to aid in
> > any s2idle debugging.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > ---
> >  drivers/platform/x86/amd/pmc.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/platform/x86/amd/pmc.c
> b/drivers/platform/x86/amd/pmc.c
> > index 427905714f79..1304cd6f13f6 100644
> > --- a/drivers/platform/x86/amd/pmc.c
> > +++ b/drivers/platform/x86/amd/pmc.c
> > @@ -543,7 +543,7 @@ static int amd_pmc_idlemask_read(struct
> amd_pmc_dev *pdev, struct device *dev,
> >     }
> >
> >     if (dev)
> > -           dev_dbg(pdev->dev, "SMU idlemask s0i3: 0x%x\n", val);
> > +           pm_pr_dbg("SMU idlemask s0i3: 0x%x\n", val);
> >
> >     if (s)
> >             seq_printf(s, "SMU idlemask : 0x%x\n", val);
>
> This does not compile, amd/pmc.c may be build as an amd-pmc.ko module
> and currently the pm_debug_messages_on flag used by pm_pr_dbg()
> is not exported to modules:
>
>   CC [M]  drivers/platform/x86/amd/pmc.o
>   LD [M]  drivers/platform/x86/amd/amd-pmc.o
>   MODPOST Module.symvers
> ERROR: modpost: "pm_debug_messages_on"
> [drivers/platform/x86/amd/amd-pmc.ko] undefined!
> make[1]: *** [scripts/Makefile.modpost:136: Module.symvers] Error 1
> make: *** [Makefile:1978: modpost] Error 2
>
> Regards,
>
> Hans
>

My apologies, yes I was compiling in when testing.  Let me ask if this
series makes sense and is "generally" agreeable though.

If it is I'll adjust it so that exports to modules.  If the preference is
to keep /sys/power/pm_debug_messages only for core PM stuff
then I'll just send the one patch improvement for s2idle.c logging.

Re: [PATCH v2 4/4] platform/x86/amd: pmc: Use pm_pr_dbg() for suspend related messages

[Hans de Goede]

Hi Mario,

On 5/23/23 18:21, Limonciello, Mario wrote:
> [AMD Official Use Only - General]
> 
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@redhat.com>
>> Sent: Tuesday, May 23, 2023 6:08 AM
>> To: Limonciello, Mario <Mario.Limonciello@amd.com>; rafael@kernel.org;
>> linus.walleij@linaro.org
>> Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
>> gpio@vger.kernel.org; platform-driver-x86@vger.kernel.org; linux-
>> pm@vger.kernel.org; S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>;
>> Natikar, Basavaraj <Basavaraj.Natikar@amd.com>
>> Subject: Re: [PATCH v2 4/4] platform/x86/amd: pmc: Use pm_pr_dbg() for
>> suspend related messages
>>
>> Hi Mario,
>>
>> On 5/22/23 22:00, Mario Limonciello wrote:
>>> Using pm_pr_dbg() allows users to toggle
>> `/sys/power/pm_debug_messages`
>>> as a single knob to turn on messages that amd-pmc can emit to aid in
>>> any s2idle debugging.
>>>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>> ---
>>>  drivers/platform/x86/amd/pmc.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/amd/pmc.c
>> b/drivers/platform/x86/amd/pmc.c
>>> index 427905714f79..1304cd6f13f6 100644
>>> --- a/drivers/platform/x86/amd/pmc.c
>>> +++ b/drivers/platform/x86/amd/pmc.c
>>> @@ -543,7 +543,7 @@ static int amd_pmc_idlemask_read(struct
>> amd_pmc_dev *pdev, struct device *dev,
>>>     }
>>>
>>>     if (dev)
>>> -           dev_dbg(pdev->dev, "SMU idlemask s0i3: 0x%x\n", val);
>>> +           pm_pr_dbg("SMU idlemask s0i3: 0x%x\n", val);
>>>
>>>     if (s)
>>>             seq_printf(s, "SMU idlemask : 0x%x\n", val);
>>
>> This does not compile, amd/pmc.c may be build as an amd-pmc.ko module
>> and currently the pm_debug_messages_on flag used by pm_pr_dbg()
>> is not exported to modules:
>>
>>   CC [M]  drivers/platform/x86/amd/pmc.o
>>   LD [M]  drivers/platform/x86/amd/amd-pmc.o
>>   MODPOST Module.symvers
>> ERROR: modpost: "pm_debug_messages_on"
>> [drivers/platform/x86/amd/amd-pmc.ko] undefined!
>> make[1]: *** [scripts/Makefile.modpost:136: Module.symvers] Error 1
>> make: *** [Makefile:1978: modpost] Error 2
>>
>> Regards,
>>
>> Hans
>>
> 
> My apologies, yes I was compiling in when testing.  Let me ask if this
> series makes sense and is "generally" agreeable though.

I have no objections against this series, otherwise I don't really
have a strong opinion on this series.

If this makes sense and if exporting pm_debug_messages_on is ok
is Rafael's call to make IMHO.

Rafael ?

Regards,

Hans

Re: [PATCH v2 4/4] platform/x86/amd: pmc: Use pm_pr_dbg() for suspend related messages

[Rafael J. Wysocki]

On Thu, May 25, 2023 at 12:13 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Mario,
>
> On 5/23/23 18:21, Limonciello, Mario wrote:
> > [AMD Official Use Only - General]
> >
> >> -----Original Message-----
> >> From: Hans de Goede <hdegoede@redhat.com>
> >> Sent: Tuesday, May 23, 2023 6:08 AM
> >> To: Limonciello, Mario <Mario.Limonciello@amd.com>; rafael@kernel.org;
> >> linus.walleij@linaro.org
> >> Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> >> gpio@vger.kernel.org; platform-driver-x86@vger.kernel.org; linux-
> >> pm@vger.kernel.org; S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>;
> >> Natikar, Basavaraj <Basavaraj.Natikar@amd.com>
> >> Subject: Re: [PATCH v2 4/4] platform/x86/amd: pmc: Use pm_pr_dbg() for
> >> suspend related messages
> >>
> >> Hi Mario,
> >>
> >> On 5/22/23 22:00, Mario Limonciello wrote:
> >>> Using pm_pr_dbg() allows users to toggle
> >> `/sys/power/pm_debug_messages`
> >>> as a single knob to turn on messages that amd-pmc can emit to aid in
> >>> any s2idle debugging.
> >>>
> >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >>> ---
> >>>  drivers/platform/x86/amd/pmc.c | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/platform/x86/amd/pmc.c
> >> b/drivers/platform/x86/amd/pmc.c
> >>> index 427905714f79..1304cd6f13f6 100644
> >>> --- a/drivers/platform/x86/amd/pmc.c
> >>> +++ b/drivers/platform/x86/amd/pmc.c
> >>> @@ -543,7 +543,7 @@ static int amd_pmc_idlemask_read(struct
> >> amd_pmc_dev *pdev, struct device *dev,
> >>>     }
> >>>
> >>>     if (dev)
> >>> -           dev_dbg(pdev->dev, "SMU idlemask s0i3: 0x%x\n", val);
> >>> +           pm_pr_dbg("SMU idlemask s0i3: 0x%x\n", val);
> >>>
> >>>     if (s)
> >>>             seq_printf(s, "SMU idlemask : 0x%x\n", val);
> >>
> >> This does not compile, amd/pmc.c may be build as an amd-pmc.ko module
> >> and currently the pm_debug_messages_on flag used by pm_pr_dbg()
> >> is not exported to modules:
> >>
> >>   CC [M]  drivers/platform/x86/amd/pmc.o
> >>   LD [M]  drivers/platform/x86/amd/amd-pmc.o
> >>   MODPOST Module.symvers
> >> ERROR: modpost: "pm_debug_messages_on"
> >> [drivers/platform/x86/amd/amd-pmc.ko] undefined!
> >> make[1]: *** [scripts/Makefile.modpost:136: Module.symvers] Error 1
> >> make: *** [Makefile:1978: modpost] Error 2
> >>
> >> Regards,
> >>
> >> Hans
> >>
> >
> > My apologies, yes I was compiling in when testing.  Let me ask if this
> > series makes sense and is "generally" agreeable though.
>
> I have no objections against this series, otherwise I don't really
> have a strong opinion on this series.
>
> If this makes sense and if exporting pm_debug_messages_on is ok
> is Rafael's call to make IMHO.
>
> Rafael ?

I have no strong opinion.

I would do it slightly differently as mentioned in my reply to patch
[1/4] (and then the new function could be used in patch [2/4] I
think).

Otherwise this is fine with me if it helps to debug failures in the field.

Re: [PATCH v2 4/4] platform/x86/amd: pmc: Use pm_pr_dbg() for suspend related messages

[andy.shevchenko]

Mon, May 22, 2023 at 03:00:33PM -0500, Mario Limonciello kirjoitti:
> Using pm_pr_dbg() allows users to toggle `/sys/power/pm_debug_messages`
> as a single knob to turn on messages that amd-pmc can emit to aid in
> any s2idle debugging.

It has the same two regressions as I pointed out in previous reply.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/4] include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume

[Rafael J. Wysocki]

On Mon, May 22, 2023 at 10:01 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> All uses in the kernel are currently already oriented around
> suspend/resume. As some other parts of the kernel may also use these
> messages in functions that could also be used outside of
> suspend/resume, only enable in suspend/resume path.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  include/linux/suspend.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index d0d4598a7b3f..a40f2e667e09 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -564,7 +564,8 @@ static inline int pm_dyn_debug_messages_on(void)
>  #endif
>  #define __pm_pr_dbg(fmt, ...)                                  \
>         do {                                                    \
> -               if (pm_debug_messages_on)                       \
> +               if (pm_debug_messages_on &&                     \
> +                   pm_suspend_target_state != PM_SUSPEND_ON)   \

Instead of this, I would define a function, say
pm_debug_messages_should_print(), that would do the check and I would
use it also in __pm_deferred_pr_dbg().

>                         printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__);  \
>                 else if (pm_dyn_debug_messages_on())            \
>                         pr_debug(fmt, ##__VA_ARGS__);   \
> @@ -589,7 +590,8 @@ static inline int pm_dyn_debug_messages_on(void)
>  /**
>   * pm_pr_dbg - print pm sleep debug messages
>   *
> - * If pm_debug_messages_on is enabled, print message.
> + * If pm_debug_messages_on is enabled and the system is entering/leaving
> + *      suspend, print message.
>   * If pm_debug_messages_on is disabled and CONFIG_DYNAMIC_DEBUG is enabled,
>   *     print message only from instances explicitly enabled on dynamic debug's
>   *     control.
>
> base-commit: 42dfdd08422dec99bfe526072063f65c0b9fb7d2
> --