linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] PM: fix dynamic debug within pm_pr_debug()
@ 2022-03-12  4:36 David Cohen
  2022-03-17 13:45 ` Rafael J. Wysocki
  0 siblings, 1 reply; 8+ messages in thread
From: David Cohen @ 2022-03-12  4:36 UTC (permalink / raw)
  To: rafael, pavel, len.brown; +Cc: David Cohen, linux-pm, linux-kernel

Currently, pm_pr_debug() and pm_deferred_pr_debug() use __pm_pr_debug()
to filter pm debug messages based on pm_debug_messages_on flag.
According to __pm_pr_debug() implementation, pm_deferred_pr_debug()
indirectly calls printk_deferred() within __pm_pr_debug() which doesn't
support dynamic debug, but pm_pr_debug() indirectly calls pr_debug()
which does support dynamic debug.

The problem is if we enable/disable dynamic debug inside __pm_pr_debug()
it will affect all pm_pr_debug() calls at once, so we can't individually
control them.

This patch changes __pm_pr_debug() implementation into macros to make
pr_debug() to be directly called by all pm_pr_debug() cases. As a direct
side effect all pm_pr_debug() can be individually controlled by dynamic
debug feature.

Signed-off-by: David Cohen <dacohen@pm.me>
---
 include/linux/suspend.h | 19 +++++++++++++++----
 kernel/power/main.c     | 29 -----------------------------
 2 files changed, 15 insertions(+), 33 deletions(-)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 300273ff40cc..d727d3c867e3 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -542,22 +542,33 @@ static inline void unlock_system_sleep(void) {}
 #ifdef CONFIG_PM_SLEEP_DEBUG
 extern bool pm_print_times_enabled;
 extern bool pm_debug_messages_on;
-extern __printf(2, 3) void __pm_pr_dbg(bool defer, const char *fmt, ...);
+#define __pm_pr_dbg(fmt, ...)					\
+	do {							\
+		if (pm_debug_messages_on)			\
+			pr_debug("PM: " fmt, ##__VA_ARGS__);	\
+	} while(0)
+#define __pm_deferred_pr_dbg(fmt, ...)				\
+	do {							\
+		if (pm_debug_messages_on)			\
+			printk_deferred(KERN_DEBUG "PM: " fmt, ##__VA_ARGS__);	\
+	} while(0)
 #else
 #define pm_print_times_enabled	(false)
 #define pm_debug_messages_on	(false)

 #include <linux/printk.h>

-#define __pm_pr_dbg(defer, fmt, ...) \
+#define __pm_pr_dbg(fmt, ...) \
+	no_printk(KERN_DEBUG fmt, ##__VA_ARGS__)
+#define __pm_deferred_pr_dbg(fmt, ...) \
 	no_printk(KERN_DEBUG fmt, ##__VA_ARGS__)
 #endif

 #define pm_pr_dbg(fmt, ...) \
-	__pm_pr_dbg(false, fmt, ##__VA_ARGS__)
+	__pm_pr_dbg(fmt, ##__VA_ARGS__)

 #define pm_deferred_pr_dbg(fmt, ...) \
-	__pm_pr_dbg(true, fmt, ##__VA_ARGS__)
+	__pm_deferred_pr_dbg(fmt, ##__VA_ARGS__)

 #ifdef CONFIG_PM_AUTOSLEEP

diff --git a/kernel/power/main.c b/kernel/power/main.c
index 7e646079fbeb..5242bf2ee469 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -545,35 +545,6 @@ static int __init pm_debug_messages_setup(char *str)
 }
 __setup("pm_debug_messages", pm_debug_messages_setup);

-/**
- * __pm_pr_dbg - Print a suspend debug message to the kernel log.
- * @defer: Whether or not to use printk_deferred() to print the message.
- * @fmt: Message format.
- *
- * The message will be emitted if enabled through the pm_debug_messages
- * sysfs attribute.
- */
-void __pm_pr_dbg(bool defer, const char *fmt, ...)
-{
-	struct va_format vaf;
-	va_list args;
-
-	if (!pm_debug_messages_on)
-		return;
-
-	va_start(args, fmt);
-
-	vaf.fmt = fmt;
-	vaf.va = &args;
-
-	if (defer)
-		printk_deferred(KERN_DEBUG "PM: %pV", &vaf);
-	else
-		printk(KERN_DEBUG "PM: %pV", &vaf);
-
-	va_end(args);
-}
-
 #else /* !CONFIG_PM_SLEEP_DEBUG */
 static inline void pm_print_times_init(void) {}
 #endif /* CONFIG_PM_SLEEP_DEBUG */
--
2.35.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] PM: fix dynamic debug within pm_pr_debug()
  2022-03-12  4:36 [PATCH v2] PM: fix dynamic debug within pm_pr_debug() David Cohen
@ 2022-03-17 13:45 ` Rafael J. Wysocki
  2022-03-19  1:54   ` David Cohen
  0 siblings, 1 reply; 8+ messages in thread
From: Rafael J. Wysocki @ 2022-03-17 13:45 UTC (permalink / raw)
  To: David Cohen
  Cc: Rafael J. Wysocki, Pavel Machek, Len Brown, Linux PM,
	Linux Kernel Mailing List

On Sat, Mar 12, 2022 at 5:37 AM David Cohen <dacohen@pm.me> wrote:
>
> Currently, pm_pr_debug() and pm_deferred_pr_debug() use __pm_pr_debug()
> to filter pm debug messages based on pm_debug_messages_on flag.
> According to __pm_pr_debug() implementation, pm_deferred_pr_debug()
> indirectly calls printk_deferred() within __pm_pr_debug() which doesn't
> support dynamic debug, but pm_pr_debug() indirectly calls pr_debug()

I'm not sure what you mean by pm_pr_debug().  There's no such thing in
the kernel tree.

Assuming that it means pm_pr_dbg(), it doesn't call pr_debug():

#define pm_pr_dbg(fmt, ...) __pm_pr_dbg(false, fmt, ##__VA_ARGS__)

and

void __pm_pr_dbg(bool defer, const char *fmt, ...)
{
...
        if (defer)
               printk_deferred(KERN_DEBUG "PM: %pV", &vaf);
        else
               printk(KERN_DEBUG "PM: %pV", &vaf);

And as I said printk(KERN_DEBUG ...) is not equivalent to
pr_debug(...), because it is not dynamic printk().

pm_pr_dbg() is not dynamic printk() on purpose, so they both can be
controlled independently.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] PM: fix dynamic debug within pm_pr_debug()
  2022-03-17 13:45 ` Rafael J. Wysocki
@ 2022-03-19  1:54   ` David Cohen
  2022-03-21 14:20     ` Rafael J. Wysocki
  0 siblings, 1 reply; 8+ messages in thread
From: David Cohen @ 2022-03-19  1:54 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Pavel Machek, Len Brown, Linux PM, Linux Kernel Mailing List

On Thu, Mar 17, 2022 at 02:45:11PM +0100, Rafael J. Wysocki wrote:
> On Sat, Mar 12, 2022 at 5:37 AM David Cohen <dacohen@pm.me> wrote:
> >
> > Currently, pm_pr_debug() and pm_deferred_pr_debug() use __pm_pr_debug()
> > to filter pm debug messages based on pm_debug_messages_on flag.
> > According to __pm_pr_debug() implementation, pm_deferred_pr_debug()
> > indirectly calls printk_deferred() within __pm_pr_debug() which doesn't
> > support dynamic debug, but pm_pr_debug() indirectly calls pr_debug()
>
> I'm not sure what you mean by pm_pr_debug().  There's no such thing in
> the kernel tree.
>
> Assuming that it means pm_pr_dbg(), it doesn't call pr_debug():

Yeah, I apologize for the typo. I meant pm_pr_dbg(). I can fix that if
you're ok with the patch as per comments below.

>
> #define pm_pr_dbg(fmt, ...) __pm_pr_dbg(false, fmt, ##__VA_ARGS__)
>
> and
>
> void __pm_pr_dbg(bool defer, const char *fmt, ...)
> {
> ...
>         if (defer)
>                printk_deferred(KERN_DEBUG "PM: %pV", &vaf);
>         else
>                printk(KERN_DEBUG "PM: %pV", &vaf);
>
> And as I said printk(KERN_DEBUG ...) is not equivalent to
> pr_debug(...), because it is not dynamic printk().

The problem is not about __pm_pr_dbg() calling printk(). The problem is
the pm files that used to call pr_debug() were modified to call
pm_pr_dbg() in order to be behing the pm_debug_messages_on flag, as per
this commit:
8d8b2441db96 PM / sleep: Do not print debug messages by default

That's the moment dynamic debug was no longer available for kernel pm files.

>
> pm_pr_dbg() is not dynamic printk() on purpose, so they both can be
> controlled independently.

The current solution is all or nothing (using pm_debug_messages_on). The
patch I'm sending is making dynamic debug available on the kernel pm
files, while still allowing the pm_debug_messages_on flag to work
independently.

Regards, David


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] PM: fix dynamic debug within pm_pr_debug()
  2022-03-19  1:54   ` David Cohen
@ 2022-03-21 14:20     ` Rafael J. Wysocki
  2022-03-21 14:51       ` David Cohen
  0 siblings, 1 reply; 8+ messages in thread
From: Rafael J. Wysocki @ 2022-03-21 14:20 UTC (permalink / raw)
  To: David Cohen
  Cc: Rafael J. Wysocki, Pavel Machek, Len Brown, Linux PM,
	Linux Kernel Mailing List

On Sat, Mar 19, 2022 at 2:54 AM David Cohen <dacohen@pm.me> wrote:
>
> On Thu, Mar 17, 2022 at 02:45:11PM +0100, Rafael J. Wysocki wrote:
> > On Sat, Mar 12, 2022 at 5:37 AM David Cohen <dacohen@pm.me> wrote:
> > >
> > > Currently, pm_pr_debug() and pm_deferred_pr_debug() use __pm_pr_debug()
> > > to filter pm debug messages based on pm_debug_messages_on flag.
> > > According to __pm_pr_debug() implementation, pm_deferred_pr_debug()
> > > indirectly calls printk_deferred() within __pm_pr_debug() which doesn't
> > > support dynamic debug, but pm_pr_debug() indirectly calls pr_debug()
> >
> > I'm not sure what you mean by pm_pr_debug().  There's no such thing in
> > the kernel tree.
> >
> > Assuming that it means pm_pr_dbg(), it doesn't call pr_debug():
>
> Yeah, I apologize for the typo. I meant pm_pr_dbg(). I can fix that if
> you're ok with the patch as per comments below.
>
> >
> > #define pm_pr_dbg(fmt, ...) __pm_pr_dbg(false, fmt, ##__VA_ARGS__)
> >
> > and
> >
> > void __pm_pr_dbg(bool defer, const char *fmt, ...)
> > {
> > ...
> >         if (defer)
> >                printk_deferred(KERN_DEBUG "PM: %pV", &vaf);
> >         else
> >                printk(KERN_DEBUG "PM: %pV", &vaf);
> >
> > And as I said printk(KERN_DEBUG ...) is not equivalent to
> > pr_debug(...), because it is not dynamic printk().
>
> The problem is not about __pm_pr_dbg() calling printk(). The problem is
> the pm files that used to call pr_debug() were modified to call
> pm_pr_dbg() in order to be behing the pm_debug_messages_on flag, as per
> this commit:
> 8d8b2441db96 PM / sleep: Do not print debug messages by default

So what's the problem with setting pm_debug_messages_on in addition to
enabling dynamic debug for a given file?

> That's the moment dynamic debug was no longer available for kernel pm files.
>
> >
> > pm_pr_dbg() is not dynamic printk() on purpose, so they both can be
> > controlled independently.
>
> The current solution is all or nothing (using pm_debug_messages_on). The
> patch I'm sending is making dynamic debug available on the kernel pm
> files, while still allowing the pm_debug_messages_on flag to work
> independently.

If you need a combination of pm_debug_messages_on and the pr_debug()
type of dynamic debug, I would suggest adding a new macro, say
pm_pr_dbg_dyn() or similar, for this purpose and use it as needed
instead of attempting to modify the existing behavior everywhere.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] PM: fix dynamic debug within pm_pr_debug()
  2022-03-21 14:20     ` Rafael J. Wysocki
@ 2022-03-21 14:51       ` David Cohen
  2022-03-21 15:12         ` Rafael J. Wysocki
  0 siblings, 1 reply; 8+ messages in thread
From: David Cohen @ 2022-03-21 14:51 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Pavel Machek, Len Brown, Linux PM, Linux Kernel Mailing List

On Mon, Mar 21, 2022 at 03:20:20PM +0100, Rafael J. Wysocki wrote:
> On Sat, Mar 19, 2022 at 2:54 AM David Cohen <dacohen@pm.me> wrote:
> >
> > On Thu, Mar 17, 2022 at 02:45:11PM +0100, Rafael J. Wysocki wrote:
> > > On Sat, Mar 12, 2022 at 5:37 AM David Cohen <dacohen@pm.me> wrote:
> > > >
> > > > Currently, pm_pr_debug() and pm_deferred_pr_debug() use __pm_pr_debug()
> > > > to filter pm debug messages based on pm_debug_messages_on flag.
> > > > According to __pm_pr_debug() implementation, pm_deferred_pr_debug()
> > > > indirectly calls printk_deferred() within __pm_pr_debug() which doesn't
> > > > support dynamic debug, but pm_pr_debug() indirectly calls pr_debug()
> > >
> > > I'm not sure what you mean by pm_pr_debug().  There's no such thing in
> > > the kernel tree.
> > >
> > > Assuming that it means pm_pr_dbg(), it doesn't call pr_debug():
> >
> > Yeah, I apologize for the typo. I meant pm_pr_dbg(). I can fix that if
> > you're ok with the patch as per comments below.
> >
> > >
> > > #define pm_pr_dbg(fmt, ...) __pm_pr_dbg(false, fmt, ##__VA_ARGS__)
> > >
> > > and
> > >
> > > void __pm_pr_dbg(bool defer, const char *fmt, ...)
> > > {
> > > ...
> > >         if (defer)
> > >                printk_deferred(KERN_DEBUG "PM: %pV", &vaf);
> > >         else
> > >                printk(KERN_DEBUG "PM: %pV", &vaf);
> > >
> > > And as I said printk(KERN_DEBUG ...) is not equivalent to
> > > pr_debug(...), because it is not dynamic printk().
> >
> > The problem is not about __pm_pr_dbg() calling printk(). The problem is
> > the pm files that used to call pr_debug() were modified to call
> > pm_pr_dbg() in order to be behing the pm_debug_messages_on flag, as per
> > this commit:
> > 8d8b2441db96 PM / sleep: Do not print debug messages by default
>
> So what's the problem with setting pm_debug_messages_on in addition to
> enabling dynamic debug for a given file?

Let me be a bit more detailed:

Before "8d8b2441db96 PM / sleep: Do not print debug messages by default":
- pr_debug() was used
- The kernel pm files had dynamic debug support
- All the instances using pr_debug() are visible on
  /sys/kernel/debug/dynamic_debug/control
- pm_debug_messages_on flag was not supported

After "8d8b2441db96 PM / sleep: Do not print debug messages by default":
- pr_debug() was replaced with pm_pr_dbg()
- The kernel pm files where pm_pr_dbg() replaced pr_debug() *lost* dynamic
  debug support and they are no longer visible on
  /sys/kernel/debug/dynamic_debug/control
- pm_debug_messages_on flag was introduced

What my patch is doing:
- Reintroducing dynamic debug support to the same files who lost it
  after the patch mentioned above
- The instances using pr_pm_dbg() (which originally came from pr_debug())
  are reintroduced to /sys/kernel/debug/dynamic_debug/control
- pm_debug_messages_on flag is unaltered

Regards, David


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] PM: fix dynamic debug within pm_pr_debug()
  2022-03-21 14:51       ` David Cohen
@ 2022-03-21 15:12         ` Rafael J. Wysocki
  2022-03-21 15:57           ` David Cohen
  0 siblings, 1 reply; 8+ messages in thread
From: Rafael J. Wysocki @ 2022-03-21 15:12 UTC (permalink / raw)
  To: David Cohen
  Cc: Rafael J. Wysocki, Pavel Machek, Len Brown, Linux PM,
	Linux Kernel Mailing List

On Mon, Mar 21, 2022 at 3:51 PM David Cohen <dacohen@pm.me> wrote:
>
> On Mon, Mar 21, 2022 at 03:20:20PM +0100, Rafael J. Wysocki wrote:
> > On Sat, Mar 19, 2022 at 2:54 AM David Cohen <dacohen@pm.me> wrote:
> > >
> > > On Thu, Mar 17, 2022 at 02:45:11PM +0100, Rafael J. Wysocki wrote:
> > > > On Sat, Mar 12, 2022 at 5:37 AM David Cohen <dacohen@pm.me> wrote:
> > > > >
> > > > > Currently, pm_pr_debug() and pm_deferred_pr_debug() use __pm_pr_debug()
> > > > > to filter pm debug messages based on pm_debug_messages_on flag.
> > > > > According to __pm_pr_debug() implementation, pm_deferred_pr_debug()
> > > > > indirectly calls printk_deferred() within __pm_pr_debug() which doesn't
> > > > > support dynamic debug, but pm_pr_debug() indirectly calls pr_debug()
> > > >
> > > > I'm not sure what you mean by pm_pr_debug().  There's no such thing in
> > > > the kernel tree.
> > > >
> > > > Assuming that it means pm_pr_dbg(), it doesn't call pr_debug():
> > >
> > > Yeah, I apologize for the typo. I meant pm_pr_dbg(). I can fix that if
> > > you're ok with the patch as per comments below.
> > >
> > > >
> > > > #define pm_pr_dbg(fmt, ...) __pm_pr_dbg(false, fmt, ##__VA_ARGS__)
> > > >
> > > > and
> > > >
> > > > void __pm_pr_dbg(bool defer, const char *fmt, ...)
> > > > {
> > > > ...
> > > >         if (defer)
> > > >                printk_deferred(KERN_DEBUG "PM: %pV", &vaf);
> > > >         else
> > > >                printk(KERN_DEBUG "PM: %pV", &vaf);
> > > >
> > > > And as I said printk(KERN_DEBUG ...) is not equivalent to
> > > > pr_debug(...), because it is not dynamic printk().
> > >
> > > The problem is not about __pm_pr_dbg() calling printk(). The problem is
> > > the pm files that used to call pr_debug() were modified to call
> > > pm_pr_dbg() in order to be behing the pm_debug_messages_on flag, as per
> > > this commit:
> > > 8d8b2441db96 PM / sleep: Do not print debug messages by default
> >
> > So what's the problem with setting pm_debug_messages_on in addition to
> > enabling dynamic debug for a given file?
>
> Let me be a bit more detailed:
>
> Before "8d8b2441db96 PM / sleep: Do not print debug messages by default":
> - pr_debug() was used
> - The kernel pm files had dynamic debug support
> - All the instances using pr_debug() are visible on
>   /sys/kernel/debug/dynamic_debug/control
> - pm_debug_messages_on flag was not supported
>
> After "8d8b2441db96 PM / sleep: Do not print debug messages by default":
> - pr_debug() was replaced with pm_pr_dbg()

Just for some PM-related messages, not in general.

> - The kernel pm files where pm_pr_dbg() replaced pr_debug() *lost* dynamic
>   debug support and they are no longer visible on
>   /sys/kernel/debug/dynamic_debug/control

Again, some of them.

> - pm_debug_messages_on flag was introduced

So what exactly is the problem you are trying to address?

> What my patch is doing:
> - Reintroducing dynamic debug support to the same files who lost it
>   after the patch mentioned above

Why is this necessary?

> - The instances using pr_pm_dbg() (which originally came from pr_debug())
>   are reintroduced to /sys/kernel/debug/dynamic_debug/control
> - pm_debug_messages_on flag is unaltered

I think that you really want to turn off some PM messages by default
when pm_debug_messages_on is set.

OTOH I want to be able to see pr_pm_dbg() messages in the log when
pm_debug_messages_on is set without having to turn on dynamic debug
for every file in which pr_pm_dbg() is used.  Your patch breaks this.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] PM: fix dynamic debug within pm_pr_debug()
  2022-03-21 15:12         ` Rafael J. Wysocki
@ 2022-03-21 15:57           ` David Cohen
  2022-03-21 17:09             ` Rafael J. Wysocki
  0 siblings, 1 reply; 8+ messages in thread
From: David Cohen @ 2022-03-21 15:57 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Pavel Machek, Len Brown, Linux PM, Linux Kernel Mailing List

On Mon, Mar 21, 2022 at 04:12:24PM +0100, Rafael J. Wysocki wrote:
> On Mon, Mar 21, 2022 at 3:51 PM David Cohen <dacohen@pm.me> wrote:
> >
> > On Mon, Mar 21, 2022 at 03:20:20PM +0100, Rafael J. Wysocki wrote:
> > > On Sat, Mar 19, 2022 at 2:54 AM David Cohen <dacohen@pm.me> wrote:
> > > >
> > > > On Thu, Mar 17, 2022 at 02:45:11PM +0100, Rafael J. Wysocki wrote:
> > > > > On Sat, Mar 12, 2022 at 5:37 AM David Cohen <dacohen@pm.me> wrote:
> > > > > >
> > > > > > Currently, pm_pr_debug() and pm_deferred_pr_debug() use __pm_pr_debug()
> > > > > > to filter pm debug messages based on pm_debug_messages_on flag.
> > > > > > According to __pm_pr_debug() implementation, pm_deferred_pr_debug()
> > > > > > indirectly calls printk_deferred() within __pm_pr_debug() which doesn't
> > > > > > support dynamic debug, but pm_pr_debug() indirectly calls pr_debug()
> > > > >
> > > > > I'm not sure what you mean by pm_pr_debug().  There's no such thing in
> > > > > the kernel tree.
> > > > >
> > > > > Assuming that it means pm_pr_dbg(), it doesn't call pr_debug():
> > > >
> > > > Yeah, I apologize for the typo. I meant pm_pr_dbg(). I can fix that if
> > > > you're ok with the patch as per comments below.
> > > >
> > > > >
> > > > > #define pm_pr_dbg(fmt, ...) __pm_pr_dbg(false, fmt, ##__VA_ARGS__)
> > > > >
> > > > > and
> > > > >
> > > > > void __pm_pr_dbg(bool defer, const char *fmt, ...)
> > > > > {
> > > > > ...
> > > > >         if (defer)
> > > > >                printk_deferred(KERN_DEBUG "PM: %pV", &vaf);
> > > > >         else
> > > > >                printk(KERN_DEBUG "PM: %pV", &vaf);
> > > > >
> > > > > And as I said printk(KERN_DEBUG ...) is not equivalent to
> > > > > pr_debug(...), because it is not dynamic printk().
> > > >
> > > > The problem is not about __pm_pr_dbg() calling printk(). The problem is
> > > > the pm files that used to call pr_debug() were modified to call
> > > > pm_pr_dbg() in order to be behing the pm_debug_messages_on flag, as per
> > > > this commit:
> > > > 8d8b2441db96 PM / sleep: Do not print debug messages by default
> > >
> > > So what's the problem with setting pm_debug_messages_on in addition to
> > > enabling dynamic debug for a given file?
> >
> > Let me be a bit more detailed:
> >
> > Before "8d8b2441db96 PM / sleep: Do not print debug messages by default":
> > - pr_debug() was used
> > - The kernel pm files had dynamic debug support
> > - All the instances using pr_debug() are visible on
> >   /sys/kernel/debug/dynamic_debug/control
> > - pm_debug_messages_on flag was not supported
> >
> > After "8d8b2441db96 PM / sleep: Do not print debug messages by default":
> > - pr_debug() was replaced with pm_pr_dbg()
>
> Just for some PM-related messages, not in general.

Yes, I'm only addressing the kernel PM cases.

>
> > - The kernel pm files where pm_pr_dbg() replaced pr_debug() *lost* dynamic
> >   debug support and they are no longer visible on
> >   /sys/kernel/debug/dynamic_debug/control
>
> Again, some of them.
>
> > - pm_debug_messages_on flag was introduced
>
> So what exactly is the problem you are trying to address?
>
> > What my patch is doing:
> > - Reintroducing dynamic debug support to the same files who lost it
> >   after the patch mentioned above
>
> Why is this necessary?

I'm trying to have more granularity when enabling the debug messages.
See below for more context.

>
> > - The instances using pr_pm_dbg() (which originally came from pr_debug())
> >   are reintroduced to /sys/kernel/debug/dynamic_debug/control
> > - pm_debug_messages_on flag is unaltered
>
> I think that you really want to turn off some PM messages by default
> when pm_debug_messages_on is set.
>
> OTOH I want to be able to see pr_pm_dbg() messages in the log when
> pm_debug_messages_on is set without having to turn on dynamic debug
> for every file in which pr_pm_dbg() is used.  Your patch breaks this.

When debugging an issue with s2idle on my laptop where it enters an
infinite loop instead of suspending, the excess of PM messages were
changing the behavior and making it more a lot difficult to reproduce
the issue (and a less severe side effect, printing many debug info I
didn't need).

But as you pointed out, my patch is indeed changing the behavior for
when the pm_debug_messages_on is enabled. Would you be open for this
behavior instead:
 - If dynamic debug support is disabled, pm_pr_dbg() is printed if
   pm_debug_messages_on == 1
 - If dynamic debug support is enabled, pm_pr_dbg() is printed if
   pm_debug_messages_on == 1 or if pm_pr_dbg() was explicitly enabled on
   /sys/kernel/debug/dynamic_debug/control

Regards, David


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] PM: fix dynamic debug within pm_pr_debug()
  2022-03-21 15:57           ` David Cohen
@ 2022-03-21 17:09             ` Rafael J. Wysocki
  0 siblings, 0 replies; 8+ messages in thread
From: Rafael J. Wysocki @ 2022-03-21 17:09 UTC (permalink / raw)
  To: David Cohen
  Cc: Rafael J. Wysocki, Pavel Machek, Len Brown, Linux PM,
	Linux Kernel Mailing List

On Mon, Mar 21, 2022 at 4:57 PM David Cohen <dacohen@pm.me> wrote:
>
> On Mon, Mar 21, 2022 at 04:12:24PM +0100, Rafael J. Wysocki wrote:
> > On Mon, Mar 21, 2022 at 3:51 PM David Cohen <dacohen@pm.me> wrote:
> > >
> > > On Mon, Mar 21, 2022 at 03:20:20PM +0100, Rafael J. Wysocki wrote:
> > > > On Sat, Mar 19, 2022 at 2:54 AM David Cohen <dacohen@pm.me> wrote:
> > > > >
> > > > > On Thu, Mar 17, 2022 at 02:45:11PM +0100, Rafael J. Wysocki wrote:
> > > > > > On Sat, Mar 12, 2022 at 5:37 AM David Cohen <dacohen@pm.me> wrote:
> > > > > > >
> > > > > > > Currently, pm_pr_debug() and pm_deferred_pr_debug() use __pm_pr_debug()
> > > > > > > to filter pm debug messages based on pm_debug_messages_on flag.
> > > > > > > According to __pm_pr_debug() implementation, pm_deferred_pr_debug()
> > > > > > > indirectly calls printk_deferred() within __pm_pr_debug() which doesn't
> > > > > > > support dynamic debug, but pm_pr_debug() indirectly calls pr_debug()
> > > > > >
> > > > > > I'm not sure what you mean by pm_pr_debug().  There's no such thing in
> > > > > > the kernel tree.
> > > > > >
> > > > > > Assuming that it means pm_pr_dbg(), it doesn't call pr_debug():
> > > > >
> > > > > Yeah, I apologize for the typo. I meant pm_pr_dbg(). I can fix that if
> > > > > you're ok with the patch as per comments below.
> > > > >
> > > > > >
> > > > > > #define pm_pr_dbg(fmt, ...) __pm_pr_dbg(false, fmt, ##__VA_ARGS__)
> > > > > >
> > > > > > and
> > > > > >
> > > > > > void __pm_pr_dbg(bool defer, const char *fmt, ...)
> > > > > > {
> > > > > > ...
> > > > > >         if (defer)
> > > > > >                printk_deferred(KERN_DEBUG "PM: %pV", &vaf);
> > > > > >         else
> > > > > >                printk(KERN_DEBUG "PM: %pV", &vaf);
> > > > > >
> > > > > > And as I said printk(KERN_DEBUG ...) is not equivalent to
> > > > > > pr_debug(...), because it is not dynamic printk().
> > > > >
> > > > > The problem is not about __pm_pr_dbg() calling printk(). The problem is
> > > > > the pm files that used to call pr_debug() were modified to call
> > > > > pm_pr_dbg() in order to be behing the pm_debug_messages_on flag, as per
> > > > > this commit:
> > > > > 8d8b2441db96 PM / sleep: Do not print debug messages by default
> > > >
> > > > So what's the problem with setting pm_debug_messages_on in addition to
> > > > enabling dynamic debug for a given file?
> > >
> > > Let me be a bit more detailed:
> > >
> > > Before "8d8b2441db96 PM / sleep: Do not print debug messages by default":
> > > - pr_debug() was used
> > > - The kernel pm files had dynamic debug support
> > > - All the instances using pr_debug() are visible on
> > >   /sys/kernel/debug/dynamic_debug/control
> > > - pm_debug_messages_on flag was not supported
> > >
> > > After "8d8b2441db96 PM / sleep: Do not print debug messages by default":
> > > - pr_debug() was replaced with pm_pr_dbg()
> >
> > Just for some PM-related messages, not in general.
>
> Yes, I'm only addressing the kernel PM cases.
>
> >
> > > - The kernel pm files where pm_pr_dbg() replaced pr_debug() *lost* dynamic
> > >   debug support and they are no longer visible on
> > >   /sys/kernel/debug/dynamic_debug/control
> >
> > Again, some of them.
> >
> > > - pm_debug_messages_on flag was introduced
> >
> > So what exactly is the problem you are trying to address?
> >
> > > What my patch is doing:
> > > - Reintroducing dynamic debug support to the same files who lost it
> > >   after the patch mentioned above
> >
> > Why is this necessary?
>
> I'm trying to have more granularity when enabling the debug messages.
> See below for more context.
>
> >
> > > - The instances using pr_pm_dbg() (which originally came from pr_debug())
> > >   are reintroduced to /sys/kernel/debug/dynamic_debug/control
> > > - pm_debug_messages_on flag is unaltered
> >
> > I think that you really want to turn off some PM messages by default
> > when pm_debug_messages_on is set.
> >
> > OTOH I want to be able to see pr_pm_dbg() messages in the log when
> > pm_debug_messages_on is set without having to turn on dynamic debug
> > for every file in which pr_pm_dbg() is used.  Your patch breaks this.
>
> When debugging an issue with s2idle on my laptop where it enters an
> infinite loop instead of suspending, the excess of PM messages were
> changing the behavior and making it more a lot difficult to reproduce
> the issue (and a less severe side effect, printing many debug info I
> didn't need).

I see.

> But as you pointed out, my patch is indeed changing the behavior for
> when the pm_debug_messages_on is enabled. Would you be open for this
> behavior instead:
>  - If dynamic debug support is disabled, pm_pr_dbg() is printed if
>    pm_debug_messages_on == 1
>  - If dynamic debug support is enabled, pm_pr_dbg() is printed if
>    pm_debug_messages_on == 1 or if pm_pr_dbg() was explicitly enabled on
>    /sys/kernel/debug/dynamic_debug/control

Yes, that would work.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-03-21 17:09 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-12  4:36 [PATCH v2] PM: fix dynamic debug within pm_pr_debug() David Cohen
2022-03-17 13:45 ` Rafael J. Wysocki
2022-03-19  1:54   ` David Cohen
2022-03-21 14:20     ` Rafael J. Wysocki
2022-03-21 14:51       ` David Cohen
2022-03-21 15:12         ` Rafael J. Wysocki
2022-03-21 15:57           ` David Cohen
2022-03-21 17:09             ` Rafael J. Wysocki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).