[PATCH v2 0/3] Trivial dynamic debug fixups

[PATCH v2 0/3] Trivial dynamic debug fixups

[Andrew Halaney]

Here is round 2 of some trivial dynamic debug fixups. Hopefully I'm
sending the series correctly, first time trying that out.

v1 -> v2:
 * Use different example when showing misleading error message
 * Justify dynamic debug scanning the whole command line
 * Add patch removing ddebug_query
 * Add patch improving cli param examples for dyndbg/$module.dyndbg

v1: https://lore.kernel.org/all/20210909161755.61743-1-ahalaney@redhat.com/

Thanks!

Andrew Halaney (3):
  dyndbg: make dyndbg a known cli param
  dyndbg: Remove support for ddebug_query param
  Documentation: dyndbg: Improve cli param examples

 .../admin-guide/dynamic-debug-howto.rst       | 13 ++++-----
 .../admin-guide/kernel-parameters.txt         |  5 ----
 lib/dynamic_debug.c                           | 27 +++++--------------
 3 files changed, 14 insertions(+), 31 deletions(-)

-- 
2.31.1

[PATCH v2 1/3] dyndbg: make dyndbg a known cli param

[Andrew Halaney]

Right now dyndbg shows up as an unknown parameter if used on boot:

    Unknown command line parameters: dyndbg=+p

That's because it is unknown, it doesn't sit in the __param
section, so the processing done to warn users supplying an unknown
parameter doesn't think it is legitimate.

Install a dummy handler to register it. dynamic debug needs to search
the whole command line for modules listed that are currently builtin,
so there's no real work to be done in this callback.

Fixes: 86d1919a4fb0 ("init: print out unknown kernel parameters")
Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
 lib/dynamic_debug.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index cb5abb42c16a..84c16309cc63 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -761,6 +761,18 @@ static __init int ddebug_setup_query(char *str)
 
 __setup("ddebug_query=", ddebug_setup_query);
 
+/*
+ * Install a noop handler to make dyndbg look like a normal kernel cli param.
+ * This avoids warnings about dyndbg being an unknown cli param when supplied
+ * by a user.
+ */
+static __init int dyndbg_setup(char *str)
+{
+	return 1;
+}
+
+__setup("dyndbg=", dyndbg_setup);
+
 /*
  * File_ops->write method for <debugfs>/dynamic_debug/control.  Gathers the
  * command text from userspace, parses and executes it.
-- 
2.31.1

[PATCH v2 2/3] dyndbg: Remove support for ddebug_query param

[Andrew Halaney]

This param has been deprecated for a very long time now, let's rip it
out.

Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
 .../admin-guide/dynamic-debug-howto.rst       |  6 ++---
 .../admin-guide/kernel-parameters.txt         |  5 ----
 lib/dynamic_debug.c                           | 25 -------------------
 3 files changed, 2 insertions(+), 34 deletions(-)

diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index b119b8277b3e..d0911e7cc271 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -249,8 +249,7 @@ Debug messages during Boot Process
 
 To activate debug messages for core code and built-in modules during
 the boot process, even before userspace and debugfs exists, use
-``dyndbg="QUERY"``, ``module.dyndbg="QUERY"``, or ``ddebug_query="QUERY"``
-(``ddebug_query`` is obsoleted by ``dyndbg``, and deprecated).  QUERY follows
+``dyndbg="QUERY"`` or ``module.dyndbg="QUERY"``.  QUERY follows
 the syntax described above, but must not exceed 1023 characters.  Your
 bootloader may impose lower limits.
 
@@ -270,8 +269,7 @@ this boot parameter for debugging purposes.
 
 If ``foo`` module is not built-in, ``foo.dyndbg`` will still be processed at
 boot time, without effect, but will be reprocessed when module is
-loaded later. ``ddebug_query=`` and bare ``dyndbg=`` are only processed at
-boot.
+loaded later. Bare ``dyndbg=`` is only processed at boot.
 
 
 Debug Messages at Module Initialization Time
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 91ba391f9b32..9c0c57d1ca78 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -841,11 +841,6 @@
 			Format: <port#>,<type>
 			See also Documentation/input/devices/joystick-parport.rst
 
-	ddebug_query=	[KNL,DYNAMIC_DEBUG] Enable debug messages at early boot
-			time. See
-			Documentation/admin-guide/dynamic-debug-howto.rst for
-			details.  Deprecated, see dyndbg.
-
 	debug		[KNL] Enable kernel debugging (events log level).
 
 	debug_boot_weak_hash
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 84c16309cc63..130ce0eb550d 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -746,21 +746,6 @@ EXPORT_SYMBOL(__dynamic_ibdev_dbg);
 
 #endif
 
-#define DDEBUG_STRING_SIZE 1024
-static __initdata char ddebug_setup_string[DDEBUG_STRING_SIZE];
-
-static __init int ddebug_setup_query(char *str)
-{
-	if (strlen(str) >= DDEBUG_STRING_SIZE) {
-		pr_warn("ddebug boot param string too large\n");
-		return 0;
-	}
-	strlcpy(ddebug_setup_string, str, DDEBUG_STRING_SIZE);
-	return 1;
-}
-
-__setup("ddebug_query=", ddebug_setup_query);
-
 /*
  * Install a noop handler to make dyndbg look like a normal kernel cli param.
  * This avoids warnings about dyndbg being an unknown cli param when supplied
@@ -1133,16 +1118,6 @@ static int __init dynamic_debug_init(void)
 		 entries, modct, (int)((modct * sizeof(struct ddebug_table)) >> 10),
 		 (int)((entries * sizeof(struct _ddebug)) >> 10));
 
-	/* apply ddebug_query boot param, dont unload tables on err */
-	if (ddebug_setup_string[0] != '\0') {
-		pr_warn("ddebug_query param name is deprecated, change it to dyndbg\n");
-		ret = ddebug_exec_queries(ddebug_setup_string, NULL);
-		if (ret < 0)
-			pr_warn("Invalid ddebug boot param %s\n",
-				ddebug_setup_string);
-		else
-			pr_info("%d changes by ddebug_query\n", ret);
-	}
 	/* now that ddebug tables are loaded, process all boot args
 	 * again to find and activate queries given in dyndbg params.
 	 * While this has already been done for known boot params, it
-- 
2.31.1

[PATCH v2 3/3] Documentation: dyndbg: Improve cli param examples

[Andrew Halaney]

Jim pointed out that using $module.dyndbg= is always a more flexible
choice for using dynamic debug on the command line. The $module.dyndbg
style is checked at boot and handles if $module is a builtin. If it is
actually a loadable module, it is handled again later when the module is
loaded.

If you just use dyndbg="module $module +p" dynamic debug is only enabled
when $module is a builtin.

It was recommended to illustrate wildcard usage as well.

Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
Suggested-by: Jim Cromie <jim.cromie@gmail.com>
---
 Documentation/admin-guide/dynamic-debug-howto.rst | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index d0911e7cc271..4bfb23ed64ec 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -357,7 +357,10 @@ Examples
   Kernel command line: ...
     // see whats going on in dyndbg=value processing
     dynamic_debug.verbose=1
-    // enable pr_debugs in 2 builtins, #cmt is stripped
-    dyndbg="module params +p #cmt ; module sys +p"
+    // Enable pr_debugs in the params builtin
+    params.dyndbg="+p"
+    // enable pr_debugs in all files under init/
+    // and the function pc87360_init_device, #cmt is stripped
+    dyndbg="file init/* +p #cmt ; func pc87360_init_device +p"
     // enable pr_debugs in 2 functions in a module loaded later
     pc87360.dyndbg="func pc87360_init_device +p; func pc87360_find +p"
-- 
2.31.1

Re: [PATCH v2 3/3] Documentation: dyndbg: Improve cli param examples

[Jason Baron]

On 9/13/21 6:24 PM, Andrew Halaney wrote:
> Jim pointed out that using $module.dyndbg= is always a more flexible
> choice for using dynamic debug on the command line. The $module.dyndbg
> style is checked at boot and handles if $module is a builtin. If it is
> actually a loadable module, it is handled again later when the module is
> loaded.
>
> If you just use dyndbg="module $module +p" dynamic debug is only enabled
> when $module is a builtin.
>
> It was recommended to illustrate wildcard usage as well.
>
> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> Suggested-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>   Documentation/admin-guide/dynamic-debug-howto.rst | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
> index d0911e7cc271..4bfb23ed64ec 100644
> --- a/Documentation/admin-guide/dynamic-debug-howto.rst
> +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
> @@ -357,7 +357,10 @@ Examples
>     Kernel command line: ...
>       // see whats going on in dyndbg=value processing
>       dynamic_debug.verbose=1
> -    // enable pr_debugs in 2 builtins, #cmt is stripped
> -    dyndbg="module params +p #cmt ; module sys +p"
> +    // Enable pr_debugs in the params builtin
> +    params.dyndbg="+p"

If we are going out of our way to change this to indicate that it works 
for builtin and modules, it seems like the comment above should reflect 
that? IE, something like this?

'// Enable pr_debugs in the params module or if params is builtin.

The first two patches look fine to me, so if you agree maybe just 
re-spin this one?

Thanks,

-Jason

> +    // enable pr_debugs in all files under init/
> +    // and the function pc87360_init_device, #cmt is stripped
> +    dyndbg="file init/* +p #cmt ; func pc87360_init_device +p"
>       // enable pr_debugs in 2 functions in a module loaded later
>       pc87360.dyndbg="func pc87360_init_device +p; func pc87360_find +p"

Re: [PATCH v2 3/3] Documentation: dyndbg: Improve cli param examples

[jim.cromie]

On Fri, Sep 17, 2021 at 1:50 PM Jason Baron <jbaron@akamai.com> wrote:
>
>
>
> On 9/13/21 6:24 PM, Andrew Halaney wrote:
> > Jim pointed out that using $module.dyndbg= is always a more flexible
> > choice for using dynamic debug on the command line. The $module.dyndbg
> > style is checked at boot and handles if $module is a builtin. If it is
> > actually a loadable module, it is handled again later when the module is
> > loaded.
> >
> > If you just use dyndbg="module $module +p" dynamic debug is only enabled
> > when $module is a builtin.
> >
> > It was recommended to illustrate wildcard usage as well.
> >
> > Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> > Suggested-by: Jim Cromie <jim.cromie@gmail.com>
> > ---
> >   Documentation/admin-guide/dynamic-debug-howto.rst | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
> > index d0911e7cc271..4bfb23ed64ec 100644
> > --- a/Documentation/admin-guide/dynamic-debug-howto.rst
> > +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
> > @@ -357,7 +357,10 @@ Examples
> >     Kernel command line: ...
> >       // see whats going on in dyndbg=value processing
> >       dynamic_debug.verbose=1
> > -    // enable pr_debugs in 2 builtins, #cmt is stripped
> > -    dyndbg="module params +p #cmt ; module sys +p"
> > +    // Enable pr_debugs in the params builtin
> > +    params.dyndbg="+p"
>
> If we are going out of our way to change this to indicate that it works
> for builtin and modules, it seems like the comment above should reflect
> that? IE, something like this?
>
> '// Enable pr_debugs in the params module or if params is builtin.
>

I dont think params can be a loadable module, so its not a great
example of this.
it should be one that "everyone" knows is usually loaded.

conversely, bare dyndbg example should have only builtin modules,
then the contrast between 2 forms is most evident.


> The first two patches look fine to me, so if you agree maybe just
> re-spin this one?
>
> Thanks,
>
> -Jason
>
> > +    // enable pr_debugs in all files under init/
> > +    // and the function pc87360_init_device, #cmt is stripped
> > +    dyndbg="file init/* +p #cmt ; func pc87360_init_device +p"
> >       // enable pr_debugs in 2 functions in a module loaded later
> >       pc87360.dyndbg="func pc87360_init_device +p; func pc87360_find +p"
>

Re: [PATCH v2 3/3] Documentation: dyndbg: Improve cli param examples

[Andrew Halaney]

On Fri, Sep 17, 2021 at 02:30:09PM -0600, jim.cromie@gmail.com wrote:
> On Fri, Sep 17, 2021 at 1:50 PM Jason Baron <jbaron@akamai.com> wrote:
> >
> >
> >
> > On 9/13/21 6:24 PM, Andrew Halaney wrote:
> > > Jim pointed out that using $module.dyndbg= is always a more flexible
> > > choice for using dynamic debug on the command line. The $module.dyndbg
> > > style is checked at boot and handles if $module is a builtin. If it is
> > > actually a loadable module, it is handled again later when the module is
> > > loaded.
> > >
> > > If you just use dyndbg="module $module +p" dynamic debug is only enabled
> > > when $module is a builtin.
> > >
> > > It was recommended to illustrate wildcard usage as well.
> > >
> > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> > > Suggested-by: Jim Cromie <jim.cromie@gmail.com>
> > > ---
> > >   Documentation/admin-guide/dynamic-debug-howto.rst | 7 +++++--
> > >   1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
> > > index d0911e7cc271..4bfb23ed64ec 100644
> > > --- a/Documentation/admin-guide/dynamic-debug-howto.rst
> > > +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
> > > @@ -357,7 +357,10 @@ Examples
> > >     Kernel command line: ...
> > >       // see whats going on in dyndbg=value processing
> > >       dynamic_debug.verbose=1
> > > -    // enable pr_debugs in 2 builtins, #cmt is stripped
> > > -    dyndbg="module params +p #cmt ; module sys +p"
> > > +    // Enable pr_debugs in the params builtin
> > > +    params.dyndbg="+p"
> >
> > If we are going out of our way to change this to indicate that it works
> > for builtin and modules, it seems like the comment above should reflect
> > that? IE, something like this?
> >
> > '// Enable pr_debugs in the params module or if params is builtin.
> >
> 
> I dont think params can be a loadable module, so its not a great
> example of this.
> it should be one that "everyone" knows is usually loaded.
> 
> conversely, bare dyndbg example should have only builtin modules,
> then the contrast between 2 forms is most evident.
> 

Thank you both for the feedback, good points.

Does something like:

    // Enable pr_debugs in the btrfs module (can be builtin or loadable)
    btrfs.dyndbg="+p"
    // enable pr_debugs in all files under init/
    // and the function parse_one, #cmt is stripped
    dyndbg="file init/* +p #cmt ; func parse_one +p"

Work for you both? I think that makes the advantages of $module.dyndbg=
more clear and makes the usage of dyndbg= stick to strictly builtins.
If so I'll respin this patch in v3 of the series.

Thanks,
Andrew

> 
> > The first two patches look fine to me, so if you agree maybe just
> > re-spin this one?
> >
> > Thanks,
> >
> > -Jason
> >
> > > +    // enable pr_debugs in all files under init/
> > > +    // and the function pc87360_init_device, #cmt is stripped
> > > +    dyndbg="file init/* +p #cmt ; func pc87360_init_device +p"
> > >       // enable pr_debugs in 2 functions in a module loaded later
> > >       pc87360.dyndbg="func pc87360_init_device +p; func pc87360_find +p"
> >
> 

Re: [PATCH v2 3/3] Documentation: dyndbg: Improve cli param examples

[Jason Baron]

On 9/17/21 4:53 PM, Andrew Halaney wrote:
> On Fri, Sep 17, 2021 at 02:30:09PM -0600, jim.cromie@gmail.com wrote:
>> On Fri, Sep 17, 2021 at 1:50 PM Jason Baron <jbaron@akamai.com> wrote:
>>>
>>>
>>> On 9/13/21 6:24 PM, Andrew Halaney wrote:
>>>> Jim pointed out that using $module.dyndbg= is always a more flexible
>>>> choice for using dynamic debug on the command line. The $module.dyndbg
>>>> style is checked at boot and handles if $module is a builtin. If it is
>>>> actually a loadable module, it is handled again later when the module is
>>>> loaded.
>>>>
>>>> If you just use dyndbg="module $module +p" dynamic debug is only enabled
>>>> when $module is a builtin.
>>>>
>>>> It was recommended to illustrate wildcard usage as well.
>>>>
>>>> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
>>>> Suggested-by: Jim Cromie <jim.cromie@gmail.com>
>>>> ---
>>>>    Documentation/admin-guide/dynamic-debug-howto.rst | 7 +++++--
>>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
>>>> index d0911e7cc271..4bfb23ed64ec 100644
>>>> --- a/Documentation/admin-guide/dynamic-debug-howto.rst
>>>> +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
>>>> @@ -357,7 +357,10 @@ Examples
>>>>      Kernel command line: ...
>>>>        // see whats going on in dyndbg=value processing
>>>>        dynamic_debug.verbose=1
>>>> -    // enable pr_debugs in 2 builtins, #cmt is stripped
>>>> -    dyndbg="module params +p #cmt ; module sys +p"
>>>> +    // Enable pr_debugs in the params builtin
>>>> +    params.dyndbg="+p"
>>> If we are going out of our way to change this to indicate that it works
>>> for builtin and modules, it seems like the comment above should reflect
>>> that? IE, something like this?
>>>
>>> '// Enable pr_debugs in the params module or if params is builtin.
>>>
>> I dont think params can be a loadable module, so its not a great
>> example of this.
>> it should be one that "everyone" knows is usually loaded.
>>
>> conversely, bare dyndbg example should have only builtin modules,
>> then the contrast between 2 forms is most evident.
>>
> Thank you both for the feedback, good points.
>
> Does something like:
>
>      // Enable pr_debugs in the btrfs module (can be builtin or loadable)
>      btrfs.dyndbg="+p"
>      // enable pr_debugs in all files under init/
>      // and the function parse_one, #cmt is stripped
>      dyndbg="file init/* +p #cmt ; func parse_one +p"
>
> Work for you both? I think that makes the advantages of $module.dyndbg=
> more clear and makes the usage of dyndbg= stick to strictly builtins.
> If so I'll respin this patch in v3 of the series.

Fine with me.

Thanks,

-Jason

>
> Thanks,
> Andrew
>
>>> The first two patches look fine to me, so if you agree maybe just
>>> re-spin this one?
>>>
>>> Thanks,
>>>
>>> -Jason
>>>
>>>> +    // enable pr_debugs in all files under init/
>>>> +    // and the function pc87360_init_device, #cmt is stripped
>>>> +    dyndbg="file init/* +p #cmt ; func pc87360_init_device +p"
>>>>        // enable pr_debugs in 2 functions in a module loaded later
>>>>        pc87360.dyndbg="func pc87360_init_device +p; func pc87360_find +p"