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: <1585156347-8365-1-git-send-email-orson.unisoc@gmail.com>
 <CA+H2tpE5Fe5zJKEUM7H9xT10+ZGKVmZ2Aa5D7j_eq7dvZyX5wA@mail.gmail.com> <b8119cba-e660-d831-1f09-f99641e3d938@akamai.com>
In-Reply-To: <b8119cba-e660-d831-1f09-f99641e3d938@akamai.com>
From:   Orson Zhai <orsonzhai@gmail.com>
Date:   Tue, 31 Mar 2020 07:44:55 +0800
Message-ID: <CA+H2tpEdtLBMVif3hk5YOAd6oQLT2pjY3882p2S8PAGzu6Vv4A@mail.gmail.com>
Subject: Re: [RFC PATCH V2] dynamic_debug: Add config option of DYNAMIC_DEBUG_CORE
To:     Jason Baron <jbaron@akamai.com>
Cc:     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>, joe@perches.com,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Android Kernel Team <kernel-team@android.com>,
        Orson Zhai <orson.unisoc@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Jason,

On Mon, Mar 30, 2020 at 11:19 PM Jason Baron <jbaron@akamai.com> wrote:
>
>
>
> On 3/28/20 2:17 AM, Orson Zhai wrote:
> > Hi Jason,
> >
> > On Thu, Mar 26, 2020 at 1:13 AM Orson Zhai <orson.unisoc@gmail.com> wrote:
> >>
> >> Instead of enabling whole kernel with CONFIG_DYNAMIC_DEBUG, CONFIG_
> >> DYNAMIC_DEBUG_CORE will only make the basic function definition and
> >> exported symbols to be built without replacing pr_debug or dev_dbg
> >> to dynamic version unless DEBUG is defined for any desired modules
> >
> > How do you think about this idea?
> > Optionally enable dynamic debug of modules by the DEBUG macro.
> >
>
> Hi Orson,
>
> So I like the idea of being able to use pr_debug() as a way for modules to
> tie into dynamic_debug() without having to enable everything via
> CONFIG_DYNAMIC_DEBUG. However the 'DEBUG' already has a specific meaning
> which is to enable the printing or enable by default (can be turned off
> at run-time if CONFIG_DYNAMIC_DEBUG is set).

You are right. We'd better not to break user's experience in their
mind by new added
feature.

>
> So I think generally modules want the printing off by default. So maybe
> what we want is instead of:
>
> >> +#if defined(CONFIG_DYNAMIC_DEBUG) || \
> >> +       (defined(CONFIG_DYNAMIC_CORE) && defined(DEBUG))
>
> is:
>
> >> +#if defined(CONFIG_DYNAMIC_DEBUG) || \
> >> +       (defined(CONFIG_DYNAMIC_CORE) && defined(DEBUG_MODULE))
>
>
> That is introduce a new per-module definition I've called it
> 'DEBUG_MODULE' (perhaps we can name it better), that enables

How about use DYNDBG for this?
It will be consistent with dyndbg in boot command line parameters.

Best,
-Orson

> dynamic debugging without having to turn it on globally. Then,
> modules can in addition enabled 'DEBUG' if they want all the printing
> on by default.
>
> Thanks,
>
> -Jason
>
>
> > Best Regards,
> > -Orson
> >
> >> together by users.
> >>
> >> This is useful for people who only want to enable dynamic debug for some
> >> specific kernel modules without worrying about whole kernel image size will
> >> be significantly increased and more memory consumption caused by CONFIG_
> >> DYNAMIC_DEBUG.
> >>
> >> Signed-off-by: Orson Zhai <orson.unisoc@gmail.com>
> >> ---
> >> Changes from V1:
> >> - Rewrite commit message for more generic usage.
> >> - Add combination use of CONFIG_DYNAMIC_DEBUG_CORE and DEBUG to enable
> >>   dynamic debug seperately.
> >> - Ignore empty _ddtable and return success when only the core is enabled.
> >> - Fix all typoes.
> >>
> >>  include/linux/dev_printk.h    |  6 ++++--
> >>  include/linux/dynamic_debug.h |  2 +-
> >>  include/linux/printk.h        | 12 ++++++++----
> >>  lib/Kconfig.debug             | 18 ++++++++++++++++++
> >>  lib/Makefile                  |  2 +-
> >>  lib/dynamic_debug.c           |  9 +++++++--
> >>  6 files changed, 39 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/include/linux/dev_printk.h b/include/linux/dev_printk.h
> >> index 5aad06b..ed40030 100644
> >> --- a/include/linux/dev_printk.h
> >> +++ b/include/linux/dev_printk.h
> >> @@ -109,7 +109,8 @@ void _dev_info(const struct device *dev, const char *fmt, ...)
> >>  #define dev_info(dev, fmt, ...)                                                \
> >>         _dev_info(dev, dev_fmt(fmt), ##__VA_ARGS__)
> >>
> >> -#if defined(CONFIG_DYNAMIC_DEBUG)
> >> +#if defined(CONFIG_DYNAMIC_DEBUG) || \
> >> +       (defined(CONFIG_DYNAMIC_CORE) && defined(DEBUG))
> >>  #define dev_dbg(dev, fmt, ...)                                         \
> >>         dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
> >>  #elif defined(DEBUG)
> >> @@ -181,7 +182,8 @@ do {                                                                        \
> >>         dev_level_ratelimited(dev_notice, dev, fmt, ##__VA_ARGS__)
> >>  #define dev_info_ratelimited(dev, fmt, ...)                            \
> >>         dev_level_ratelimited(dev_info, dev, fmt, ##__VA_ARGS__)
> >> -#if defined(CONFIG_DYNAMIC_DEBUG)
> >> +#if defined(CONFIG_DYNAMIC_DEBUG) || \
> >> +       (defined(CONFIG_DYNAMIC_CORE) && defined(DEBUG))
> >>  /* descriptor check is first to prevent flooding with "callbacks suppressed" */
> >>  #define dev_dbg_ratelimited(dev, fmt, ...)                             \
> >>  do {                                                                   \
> >> 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/include/linux/printk.h b/include/linux/printk.h
> >> index 1e6108b..44d5378 100644
> >> --- a/include/linux/printk.h
> >> +++ b/include/linux/printk.h
> >> @@ -292,7 +292,8 @@ extern int kptr_restrict;
> >>   * These can be used to print at the various log levels.
> >>   * All of these will print unconditionally, although note that pr_debug()
> >>   * and other debug macros are compiled out unless either DEBUG is defined
> >> - * or CONFIG_DYNAMIC_DEBUG is set.
> >> + * or CONFIG_DYNAMIC_DEBUG is set, or both CONFIG_DYNAMIC_DEBUG_CORE and
> >> + * DEBUG is defined.
> >>   */
> >>  #define pr_emerg(fmt, ...) \
> >>         printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
> >> @@ -327,7 +328,8 @@ extern int kptr_restrict;
> >>
> >>
> >>  /* If you are writing a driver, please use dev_dbg instead */
> >> -#if defined(CONFIG_DYNAMIC_DEBUG)
> >> +#if defined(CONFIG_DYNAMIC_DEBUG) || \
> >> +       (defined(CONFIG_DYNAMIC_CORE) && defined(DEBUG))
> >>  #include <linux/dynamic_debug.h>
> >>
> >>  /* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */
> >> @@ -453,7 +455,8 @@ extern int kptr_restrict;
> >>  #endif
> >>
> >>  /* If you are writing a driver, please use dev_dbg instead */
> >> -#if defined(CONFIG_DYNAMIC_DEBUG)
> >> +#if defined(CONFIG_DYNAMIC_DEBUG) || \
> >> +       (defined(CONFIG_DYNAMIC_CORE) && defined(DEBUG))
> >>  /* descriptor check is first to prevent flooding with "callbacks suppressed" */
> >>  #define pr_debug_ratelimited(fmt, ...)                                 \
> >>  do {                                                                   \
> >> @@ -500,7 +503,8 @@ static inline void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
> >>
> >>  #endif
> >>
> >> -#if defined(CONFIG_DYNAMIC_DEBUG)
> >> +#if defined(CONFIG_DYNAMIC_DEBUG) || \
> >> +       (defined(CONFIG_DYNAMIC_CORE) && defined(DEBUG))
> >>  #define print_hex_dump_debug(prefix_str, prefix_type, rowsize, \
> >>                              groupsize, buf, len, ascii)        \
> >>         dynamic_hex_dump(prefix_str, prefix_type, rowsize,      \
> >> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> >> index 69def4a..8381b19 100644
> >> --- a/lib/Kconfig.debug
> >> +++ b/lib/Kconfig.debug
> >> @@ -99,6 +99,7 @@ config DYNAMIC_DEBUG
> >>         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 +165,23 @@ 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 as well.
> >> +
> >> +         This option can be selected with DEBUG together which could be
> >> +         defined for desired kernel modules to enable dynamic debug
> >> +         features instead for whole kernel. Especially being used in
> >> +         the case that kernel modules are built out of kernel tree to
> >> +         have dynamic debug capabilities without affecting the kernel
> >> +         base.
> >> +
> >>  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
> >> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> >> index c604091..34f303a 100644
> >> --- a/lib/dynamic_debug.c
> >> +++ b/lib/dynamic_debug.c
> >> @@ -1014,8 +1014,13 @@ static int __init dynamic_debug_init(void)
> >>         int verbose_bytes = 0;
> >>
> >>         if (__start___verbose == __stop___verbose) {
> >> -               pr_warn("_ddebug table is empty in a CONFIG_DYNAMIC_DEBUG build\n");
> >> -               return 1;
> >> +               if (IS_ENABLED(CONFIG_DYNAMIC_DEBUG)) {
> >> +                       pr_warn("_ddebug table is empty in a CONFIG_DYNAMIC_DEBUG build\n");
> >> +                       return 1;
> >> +               }
> >> +               pr_info("Ignore empty _ddebug table in a core only build\n");
> >> +               ddebug_init_success = 1;
> >> +               return 0;
> >>         }
> >>         iter = __start___verbose;
> >>         modname = iter->modname;
> >> --
> >> 2.7.4
> >>