linux-kernel - Re: [PATCH v2] PM: fix dynamic debug within pm_pr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YjigXzUovGDhx06p@jiban>
Date:   Mon, 21 Mar 2022 15:57:27 +0000
From:   David Cohen <dacohen@...me>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     Pavel Machek <pavel@....cz>, Len Brown <len.brown@...el.com>,
        Linux PM <linux-pm@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] PM: fix dynamic debug within pm_pr_debug()

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