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;
Date:   Thu, 19 Mar 2020 23:28:21 +0800
From:   Orson Zhai <orson.unisoc@gmail.com>
To:     Jason Baron <jbaron@akamai.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Changbin Du <changbin.du@intel.com>,
        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>, orsonzhai@gmail.com,
        linux-kernel@vger.kernel.org, kernel-team@android.com
Subject: Re: [RFC PATCH] dynamic_debug: Add config option of
 DYNAMIC_DEBUG_CORE
Message-ID: <20200319152820.GA2793@lenovo>
Mail-Followup-To: Jason Baron <jbaron@akamai.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Changbin Du <changbin.du@intel.com>,
        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>, orsonzhai@gmail.com,
        linux-kernel@vger.kernel.org, kernel-team@android.com
References: <1584558186-23373-1-git-send-email-orson.unisoc@gmail.com>
 <51568376-da8b-3265-ddb3-6ddba74207dc@akamai.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <51568376-da8b-3265-ddb3-6ddba74207dc@akamai.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Jason,

On Wed, Mar 18, 2020 at 05:18:43PM -0400, Jason Baron 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
> 
> And then the Kconfig:
> 
> config DYNAMIC_DRIVER_FOO_DEBUG
> 	bool "Enable dynamic driver foo printk() support"
> 	select DYNAMIC_DEBUG_CORE
> 
I highly appreciate you for giving this good example to us.
To be honest I did not really think of this kind of usage. :)
But it makes much sense. I think dynamic debug might be a little
bit high for requirement of memory. Every line of pr_debug will be
added with a static data structure and malloc with an item in link table.
It might be sensitive especially in embeded system.
So this example shows how to avoid to turn on dynamci debug for whole
system but part of it when being needed.

> 
> Or did you have something else in mind? Do you have an example
> code for the drivers that you mention?

My motivation comes from new Andorid GKI release flow. Android kernel team will
be in charge of GKI release. And SoC vendors will build their device driver as
kernel modules which are diffrent from each vendor. End-users will get their phones
installed with GKI plus some modules all together.

So at Google side, they can only set DYNAMIC_DEBUG_CORE in their defconfig to build
out GKI without worrying about the kernel image size increased too much. Actually
GKI is relatively stable as a common binary and there is no strong reason to do 
dynamic debugging to it.

And at vendor side, they will use a local defconfig which is same with Google one but add 
CONFIG_DYNAMIC_DEBUG to build their kenrel modules. As DYNAMIC_DEBUG enables only a
set of macro expansion, so it has no impact to kernel ABI or the modversion.
All modules will be compatible with GKI and with dynamic debug enabled.

Then the result will be that Google has his clean GKI and vendors have their dynamic-debug-powered modules.

Best Regards,
-Orson

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