Date:   Sat, 21 Mar 2020 02:30:31 +0800
From:   Orson Zhai <orsonzhai@...il.com>
To:     Jason Baron <jbaron@...mai.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Changbin Du <changbin.du@...el.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Shuah Khan <skhan@...uxfoundation.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Brendan Higgins <brendanhiggins@...gle.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Ard Biesheuvel <ardb@...nel.org>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        David Gow <davidgow@...gle.com>,
        Mark Rutland <mark.rutland@....com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Android Kernel Team <kernel-team@...roid.com>
Subject: Re: [RFC PATCH] dynamic_debug: Add config option of DYNAMIC_DEBUG_CORE

On Fri, Mar 20, 2020 at 4:19 AM Jason Baron <jbaron@...mai.com> wrote:
>
>
>
> On 3/19/20 11:28 AM, Orson Zhai wrote:
> > 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@...il.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.
> >
>
>
> static int __init dynamic_debug_init(void)
> {
>         struct _ddebug *iter, *iter_start;
>         const char *modname = NULL;
>         char *cmdline;
>         int ret = 0;
>         int n = 0, entries = 0, modct = 0;
>         int verbose_bytes = 0;
>
>         if (__start___verbose == __stop___verbose) {
>                 pr_warn("_ddebug table is empty in a CONFIG_DYNAMIC_DEBUG build\n");
>                 return 1;

Oh, I forgot this.
If return error here, "ddebug_init_success = 1;" will be never
executed and there will be no debugfs
or /proc operation interface for user.

>         }
>
> ...
>
> I wonder if we should just remove it now.

I think we could keep it by adding "... &&
IS_ENABLED(CONFIG_DYNAMIC_DEBUG)" into the condition.
Then do the comparison again to __start_verbose and __stop_verbose.
If no entries we set ddebug_init_success = 1 and return immediately.

I will make patch V2 if you agree with this.

Best,
-Orson

>
> Thanks,
>
> -Jason
>

Powered by blists - more mailing lists