diff mbox series

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

Message ID 20230522200033.2605-1-mario.limonciello@amd.com
State New
Headers show
Series [v2,1/4] include/linux/suspend.h: Only show pm_pr_dbg messages at suspend/resume | expand

Commit Message

Mario Limonciello May 22, 2023, 8 p.m. UTC 
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(-)


base-commit: 42dfdd08422dec99bfe526072063f65c0b9fb7d2

Comments

Hans de Goede May 23, 2023, 11:07 a.m. UTC | #1 
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
Mario Limonciello May 23, 2023, 4:21 p.m. UTC | #2 
[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.
Andy Shevchenko May 23, 2023, 4:49 p.m. UTC | #3 
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.
Andy Shevchenko May 23, 2023, 4:56 p.m. UTC | #4 
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.
diff mbox series

Patch

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.