Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1584558186-23373-1-git-send-email-orson.unisoc@gmail.com> <51568376-da8b-3265-ddb3-6ddba74207dc@akamai.com>
In-Reply-To: <51568376-da8b-3265-ddb3-6ddba74207dc@akamai.com>
From:   Orson Zhai <orsonzhai@gmail.com>
Date:   Sat, 21 Mar 2020 12:25:19 +0800
Message-ID: <CA+H2tpHL0NHipiT1cnUbRYFKoxDsbU1BHRGmLjA=diUw-H3Pgw@mail.gmail.com>
Subject: Re: [RFC PATCH] dynamic_debug: Add config option of DYNAMIC_DEBUG_CORE
To:     Jason Baron <jbaron@akamai.com>
Cc:     Orson Zhai <orson.unisoc@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Randy Dunlap <rdunlap@infradead.org>,
        Masahiro Yamada <yamada.masahiro@socionext.com>,
        Shuah Khan <skhan@linuxfoundation.org>,
        Krzysztof Kozlowski <krzk@kernel.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Brendan Higgins <brendanhiggins@google.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        Ard Biesheuvel <ardb@kernel.org>,
        Andy Shevchenko <andy.shevchenko@gmail.com>,
        David Gow <davidgow@google.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Android Kernel Team <kernel-team@android.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Jason,

On Thu, Mar 19, 2020 at 5:18 AM Jason Baron <jbaron@akamai.com> wrote:
>
>
>
> On 3/18/20 3:03 PM, Orson Zhai wrote:
> > There is the requirement from new Android that kernel image (GKI) and
> > kernel modules are supposed to be built at differnet places. Some people
> > want to enable dynamic debug for kernel modules only but not for kernel
> > image itself with the consideration of binary size increased or more
> > memory being used.
> >
> > By this patch, dynamic debug is divided into core part (the defination of
> > functions) and macro replacement part. We can only have the core part to
> > be built-in and do not have to activate the debug output from kenrel image.
> >
> > Signed-off-by: Orson Zhai <orson.unisoc@gmail.com>
>
> Hi Orson,
>
> I think this is a nice feature. Is the idea then that driver can do
> something like:
>
> #if defined(CONFIG_DRIVER_FOO_DEBUG)
> #define driver_foo_debug(fmt, ...) \
>         dynamic_pr_debug(fmt, ##__VA_ARGS__)
> #else
>         no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
> #enif

I am thinking this again.
Why don't we add a local macro ( I mean not like the global macro from
Kconfig ) just like the DEBUG macro?
Say DDEBUG or DYN_DEBUG.

We can add this to printk.h to let module owners decide when & which
module should be dynamic debugged?
#ifdef CONFIG_DYNAMIC_DUBUG || DDEUG
.....

Module owner can add this to his module's Makefile, for example:
....
ccflags-$(CONFIG_UFS_DEBUG)    += -DDDEBUG
....
This will enable macro replacement for even a single c file.

How do you think about it?

Best,
-Orson

>
> And then the Kconfig:
>
> config DYNAMIC_DRIVER_FOO_DEBUG
>         bool "Enable dynamic driver foo printk() support"
>         select DYNAMIC_DEBUG_CORE
>
>
> Or did you have something else in mind? Do you have an example
> code for the drivers that you mention?
>
> Thanks,
>
> -Jason
>
>
> > ---
> >  include/linux/dynamic_debug.h |  2 +-
> >  lib/Kconfig.debug             | 18 ++++++++++++++++--
> >  lib/Makefile                  |  2 +-
> >  3 files changed, 18 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> > index 4cf02ec..abcd5fd 100644
> > --- a/include/linux/dynamic_debug.h
> > +++ b/include/linux/dynamic_debug.h
> > @@ -48,7 +48,7 @@ struct _ddebug {
> >
> >
> >
> > -#if defined(CONFIG_DYNAMIC_DEBUG)
> > +#if defined(CONFIG_DYNAMIC_DEBUG_CORE)
> >  int ddebug_add_module(struct _ddebug *tab, unsigned int n,
> >                               const char *modname);
> >  extern int ddebug_remove_module(const char *mod_name);
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 69def4a..78a7256 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -97,8 +97,7 @@ config BOOT_PRINTK_DELAY
> >  config DYNAMIC_DEBUG
> >       bool "Enable dynamic printk() support"
> >       default n
> > -     depends on PRINTK
> > -     depends on DEBUG_FS
> > +     select DYNAMIC_DEBUG_CORE
> >       help
> >
> >         Compiles debug level messages into the kernel, which would not
> > @@ -164,6 +163,21 @@ config DYNAMIC_DEBUG
> >         See Documentation/admin-guide/dynamic-debug-howto.rst for additional
> >         information.
> >
> > +config DYNAMIC_DEBUG_CORE
> > +     bool "Enable core functions of dynamic debug support"
> > +     depends on PRINTK
> > +     depends on DEBUG_FS
> > +     help
> > +       Enable this option to build ddebug_* and __dynamic_* routines
> > +       into kernel. If you want enable whole dynamic debug features,
> > +       select CONFIG_DYNAMIC_DEBUG directly and this option will be
> > +       automatically selected.
> > +
> > +       This option is selected when you want to enable dynamic debug
> > +       for kernel modules only but not for the kernel base. Especailly
> > +       in the case that kernel modules are built out of the place where
> > +       kernel base is built.
> > +
> >  config SYMBOLIC_ERRNAME
> >       bool "Support symbolic error names in printf"
> >       default y if PRINTK
> > diff --git a/lib/Makefile b/lib/Makefile
> > index 611872c..2096d83 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -183,7 +183,7 @@ lib-$(CONFIG_GENERIC_BUG) += bug.o
> >
> >  obj-$(CONFIG_HAVE_ARCH_TRACEHOOK) += syscall.o
> >
> > -obj-$(CONFIG_DYNAMIC_DEBUG) += dynamic_debug.o
> > +obj-$(CONFIG_DYNAMIC_DEBUG_CORE) += dynamic_debug.o
> >  obj-$(CONFIG_SYMBOLIC_ERRNAME) += errname.o
> >
> >  obj-$(CONFIG_NLATTR) += nlattr.o
> >