Right now dyndbg shows up as an unknown parameter if used on boot:
Unknown command line parameters: dyndbg=module params +p ; module sys +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. This was chosen instead of the
approach the (deprecated) ddebug_query param takes, which is to
have a real handler that copies the string taking up a KiB memory.
Fixes: 86d1919a4fb0 ("init: print out unknown kernel parameters")
Signed-off-by: Andrew Halaney <[email protected]>
---
This is the last valid param I know of that was getting flagged on boot
if used correctly. Please let me know if the alternative approach of
actually copying the string is preferred and I'll spin that up instead.
Sort of an aside, but what's the policy for removing deprecated cli
params? ddebug_query has been deprecated for a very long time now, but I
am not sure if removing params like that is considered almost as bad as
breaking userspace considering some systems might update their kernels
but not the bootloader supplying the param.
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
On 9/9/21 12:17 PM, Andrew Halaney wrote:
> Right now dyndbg shows up as an unknown parameter if used on boot:
>
> Unknown command line parameters: dyndbg=module params +p ; module sys +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. This was chosen instead of the
> approach the (deprecated) ddebug_query param takes, which is to
> have a real handler that copies the string taking up a KiB memory.
>
> Fixes: 86d1919a4fb0 ("init: print out unknown kernel parameters")
> Signed-off-by: Andrew Halaney <[email protected]>
> ---
>
> This is the last valid param I know of that was getting flagged on boot
> if used correctly. Please let me know if the alternative approach of
> actually copying the string is preferred and I'll spin that up instead.
>
Hi Andrew,
So I think you are referring to the string copying that ddebug_query= does.
I don't think that works for 'dyndbg=' b/c its actually looking through
the entire command line for stuff like <module_name>.dyndbg="blah".
So I think what you prposed below makes sense, we could perhaps add a note
as to why it's a noop. As I mentioned it needs to look through the entire
string.
> Sort of an aside, but what's the policy for removing deprecated cli
> params? ddebug_query has been deprecated for a very long time now, but I
> am not sure if removing params like that is considered almost as bad as
> breaking userspace considering some systems might update their kernels
> but not the bootloader supplying the param.
I think it's probably ok to remove at this point, especially now that
we are going to flag it as unknown, right? So I feel like that change
can logically go with this series if you want as a separate patch.
Thanks,
-Jason
>
> 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.
>
On Thu, Sep 09, 2021 at 05:34:51PM -0400, Jason Baron wrote:
>
>
> On 9/9/21 12:17 PM, Andrew Halaney wrote:
> > Right now dyndbg shows up as an unknown parameter if used on boot:
> >
> > Unknown command line parameters: dyndbg=module params +p ; module sys +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. This was chosen instead of the
> > approach the (deprecated) ddebug_query param takes, which is to
> > have a real handler that copies the string taking up a KiB memory.
> >
> > Fixes: 86d1919a4fb0 ("init: print out unknown kernel parameters")
> > Signed-off-by: Andrew Halaney <[email protected]>
> > ---
> >
> > This is the last valid param I know of that was getting flagged on boot
> > if used correctly. Please let me know if the alternative approach of
> > actually copying the string is preferred and I'll spin that up instead.
> >
>
> Hi Andrew,
>
> So I think you are referring to the string copying that ddebug_query= does.
> I don't think that works for 'dyndbg=' b/c its actually looking through
> the entire command line for stuff like <module_name>.dyndbg="blah".
>
> So I think what you prposed below makes sense, we could perhaps add a note
> as to why it's a noop. As I mentioned it needs to look through the entire
> string.
>
I think that the ddebug_query= style works fine for dyndbg=, and that
things like <module_name>.dyndbg= go through a different path to
actually have the work done.
When dyndbg= is processed through dynamic_debug_init() all of the
<module_name>.dyndbg= style queries don't do anything ultimately.
When the module is actually loaded unknown_module_param_cb() checks for
the module's dyndbg query and calls ddebug_dyndbg_module_param_cb()
directly. It is at this point that I believe the query is really
applied.
For example, this is what I see in dmesg:
[ 0.045553] Kernel command line: BOOT_IMAGE=(hd0,msdos1)/vmlinuz-5.14.0+ root=UUID=9a0e5b84-1190-465f-a587-f2f2a00a8e91 ro rhgb quiet "dyndbg=module params +p ; module sys +p" dynamic_debug.verbose=2 kvm.dyndbg=+pflmt
# Initial dynamic_debug processing..
[ 0.114308] dyndbg: 6 debug prints in module main
[ 0.114308] dyndbg: 1 debug prints in module initramfs
<snip (no kvm debug prints... since it is a module and not loaded)>
# Processing normal dyndbg= for builtins, param has matches but sys
# does not
[ 0.114308] dyndbg: dyndbg="module params +p ; module sys +p"
[ 0.114308] dyndbg: query 0: "module params +p "
<snip>
[ 0.114308] dyndbg: applied: func="" file="" module="params" format="" lineno=0-0
[ 0.114308] dyndbg: query 1: "module sys +p"
[ 0.114308] dyndbg: no-match: func="" file="" module="sys" format="" lineno=0-0
[ 0.114308] dyndbg: processed 2 queries, with 4 matches, 0 errs
# Trying to process kvm.dyndbg=, but no effect as module isn't loaded
[ 0.114308] doing dyndbg params: kvm.dyndbg='+pflmt'
[ 0.114308] dyndbg: kvm.dyndbg="+pflmt"
<snip>
[ 0.114308] dyndbg: processed 1 queries, with 0 matches, 0 errs
# kvm module is loaded, kernel/module.c calls ddebug_dyndbg_module_param_cb()
# this time it does something
[ 3.651764] doing kvm, parsing ARGS: 'dyndbg=+pflmt '
[ 3.651767] doing kvm: dyndbg='+pflmt'
[ 3.651768] dyndbg: module: kvm dyndbg="+pflmt"
[ 3.651769] dyndbg: query 0: "+pflmt"
<snip>
[ 3.652414] dyndbg: applied: func="" file="" module="kvm" format="" lineno=0-0
[ 3.652416] dyndbg: processed 1 queries, with 10 matches, 0 errs
With that being said, I think ddebug_query= style of copying the
dyndbg="string" is all that is really needed for processing builtins,
and the module loading method that's already in place shouldn't be
affected. Does that change you opinion of the approach (or am I totally
misunderstanding)? Processing the whole cli seems unnecessary, but I'm
also a (unjustified?) stickler for adding additional memory usage in the
patch.
>
> > Sort of an aside, but what's the policy for removing deprecated cli
> > params? ddebug_query has been deprecated for a very long time now, but I
> > am not sure if removing params like that is considered almost as bad as
> > breaking userspace considering some systems might update their kernels
> > but not the bootloader supplying the param.
>
> I think it's probably ok to remove at this point, especially now that
> we are going to flag it as unknown, right? So I feel like that change
> can logically go with this series if you want as a separate patch.
ddebug_query actually _is_ known (it registers with __setup() so there's
no warning like the one I listed in the commit description (although the
deprecated message in lib/dynamic_debug.c is present if you actually use
it). So I sort of feel like the removal would be separate to this
patchset. Let me know when your thoughts when you answer the prior
question and I'll react with a v2 accordingly!
>
> Thanks,
>
> -Jason
>
>
> >
> > 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.
> >
>
On Fri, Sep 10, 2021 at 11:14:45AM -0600, [email protected] wrote:
> On Thu, Sep 9, 2021 at 3:34 PM Jason Baron <[email protected]> wrote:
>
> >
> >
> > On 9/9/21 12:17 PM, Andrew Halaney wrote:
> > > Right now dyndbg shows up as an unknown parameter if used on boot:
> > >
> > > Unknown command line parameters: dyndbg=module params +p ; module
> > sys +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.
> > >
>
>
> your usage is incorrect for what youre trying to do in that example
> what you need is:
>
> params.dyndbg=+p sys.dyndbg=+p
>
> dyndbg is properly unknown as a kernel param, it isnt one.
> ( it was called a "fake" module param when added.)
> $module.dyndbg is good, since its after the $module. (and the dot)
> it also then inherits the "scan bootargs for relevant ones on module load"
> behavior
>
>
That example is (slightly altered) from
Documentation/admin-guide/dynamic-debug-howto.rst,
I can change the example used to be a little less confusing (using the
module keyword is confusing, I could use something like
func or file instead of what the docs use as an example).
Is that what you're after, a better example usage of dyndbg= being
whined about in dmesg for the commit message, or am I misunderstanding?
I agree that dyndbg right now (both dyndbg= and $module.dyndbg=) are
"fake" params, but I'd like to remove that message for dyndbg= as it is
misleading. The code for module loading luckily already handles it all
properly and doesn't warn on proper usage:
static int unknown_module_param_cb(char *param, char *val, const char *modname,
void *arg)
{
struct module *mod = arg;
int ret;
if (strcmp(param, "async_probe") == 0) {
mod->async_probe_requested = true;
return 0;
}
/* Check for magic 'dyndbg' arg */
ret = ddebug_dyndbg_module_param_cb(param, val, modname);
if (ret != 0)
pr_warn("%s: unknown parameter '%s' ignored\n", modname, param);
return 0;
}
>
>
>
> >
> > > Install a dummy handler to register it. This was chosen instead of the
> > > approach the (deprecated) ddebug_query param takes, which is to
> > > have a real handler that copies the string taking up a KiB memory.
> > >
> > > Fixes: 86d1919a4fb0 ("init: print out unknown kernel parameters")
> > > Signed-off-by: Andrew Halaney <[email protected]>
> > > ---
> > >
> > > This is the last valid param I know of that was getting flagged on boot
> > > if used correctly. Please let me know if the alternative approach of
> > > actually copying the string is preferred and I'll spin that up instead.
> > >
> >
> > Hi Andrew,
> >
> > So I think you are referring to the string copying that ddebug_query= does.
> > I don't think that works for 'dyndbg=' b/c its actually looking through
> > the entire command line for stuff like <module_name>.dyndbg="blah".
> >
> > So I think what you prposed below makes sense, we could perhaps add a note
> > as to why it's a noop. As I mentioned it needs to look through the entire
> > string.
> >
> >
> > > Sort of an aside, but what's the policy for removing deprecated cli
> > > params? ddebug_query has been deprecated for a very long time now, but I
> > > am not sure if removing params like that is considered almost as bad as
> > > breaking userspace considering some systems might update their kernels
> > > but not the bootloader supplying the param.
> >
> > I think it's probably ok to remove at this point, especially now that
> > we are going to flag it as unknown, right? So I feel like that change
> > can logically go with this series if you want as a separate patch.
> >
> > Thanks,
> >
> > -Jason
> >
> >
> > >
> > > 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.
> > >
> >
On Fri, Sep 10, 2021 at 12:24 PM Andrew Halaney <[email protected]> wrote:
>
> On Fri, Sep 10, 2021 at 11:14:45AM -0600, [email protected] wrote:
> > On Thu, Sep 9, 2021 at 3:34 PM Jason Baron <[email protected]> wrote:
> >
> > >
> > >
> > > On 9/9/21 12:17 PM, Andrew Halaney wrote:
> > > > Right now dyndbg shows up as an unknown parameter if used on boot:
> > > >
> > > > Unknown command line parameters: dyndbg=module params +p ; module
> > > sys +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.
> > > >
> >
> >
> > your usage is incorrect for what youre trying to do in that example
> > what you need is:
> >
> > params.dyndbg=+p sys.dyndbg=+p
> >
> > dyndbg is properly unknown as a kernel param, it isnt one.
> > ( it was called a "fake" module param when added.)
> > $module.dyndbg is good, since its after the $module. (and the dot)
> > it also then inherits the "scan bootargs for relevant ones on module load"
> > behavior
> >
> >
>
> That example is (slightly altered) from
> Documentation/admin-guide/dynamic-debug-howto.rst,
oh dear, I see the lines youre referring to.
I am surprised.
fyi, those lines are:
// enable pr_debugs in 2 builtins, #cmt is stripped
dyndbg="module params +p #cmt ; module sys +p"
is your patchset removing those lines ?
if so, ack that part.
> I can change the example used to be a little less confusing (using the
> module keyword is confusing, I could use something like
> func or file instead of what the docs use as an example).
yes please, I saw bad usage, thought faulty premise.
> Is that what you're after, a better example usage of dyndbg= being
> whined about in dmesg for the commit message, or am I misunderstanding?
I guess Im inured to it. Heres my regular version with a similar addition.
Unknown command line parameters:
virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0
virtme_initmount0=/root virtme_initmount1=/root/sbin
virtme_stty_con=rows 27 cols 109 iutf8
virtme_chdir=home/jimc/projects/lx dyndbg=+p
most of them do something, just not for the kernel.
I dont think this is worth explicitly silencing.
rather rip out the misleading doc.
> I agree that dyndbg right now (both dyndbg= and $module.dyndbg=) are
> "fake" params, but I'd like to remove that message for dyndbg= as it is
> misleading. The code for module loading luckily already handles it all
> properly and doesn't warn on proper usage:
>
> static int unknown_module_param_cb(char *param, char *val, const char *modname,
> void *arg)
> {
> struct module *mod = arg;
> int ret;
>
> if (strcmp(param, "async_probe") == 0) {
> mod->async_probe_requested = true;
> return 0;
> }
>
> /* Check for magic 'dyndbg' arg */
> ret = ddebug_dyndbg_module_param_cb(param, val, modname);
> if (ret != 0)
> pr_warn("%s: unknown parameter '%s' ignored\n", modname, param);
> return 0;
> }
>
>
> >
> >
> >
> > >
> > > > Install a dummy handler to register it. This was chosen instead of the
> > > > approach the (deprecated) ddebug_query param takes, which is to
> > > > have a real handler that copies the string taking up a KiB memory.
> > > >
> > > > Fixes: 86d1919a4fb0 ("init: print out unknown kernel parameters")
> > > > Signed-off-by: Andrew Halaney <[email protected]>
> > > > ---
> > > >
> > > > This is the last valid param I know of that was getting flagged on boot
> > > > if used correctly. Please let me know if the alternative approach of
> > > > actually copying the string is preferred and I'll spin that up instead.
> > > >
> > >
> > > Hi Andrew,
> > >
> > > So I think you are referring to the string copying that ddebug_query= does.
> > > I don't think that works for 'dyndbg=' b/c its actually looking through
> > > the entire command line for stuff like <module_name>.dyndbg="blah".
> > >
> > > So I think what you prposed below makes sense, we could perhaps add a note
> > > as to why it's a noop. As I mentioned it needs to look through the entire
> > > string.
> > >
> > >
> > > > Sort of an aside, but what's the policy for removing deprecated cli
> > > > params? ddebug_query has been deprecated for a very long time now, but I
> > > > am not sure if removing params like that is considered almost as bad as
> > > > breaking userspace considering some systems might update their kernels
> > > > but not the bootloader supplying the param.
> > >
> > > I think it's probably ok to remove at this point, especially now that
> > > we are going to flag it as unknown, right? So I feel like that change
> > > can logically go with this series if you want as a separate patch.
> > >
> > > Thanks,
> > >
> > > -Jason
> > >
> > >
> > > >
> > > > 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.
> > > >
> > >
>
On Fri, Sep 10, 2021 at 01:31:22PM -0600, [email protected] wrote:
> On Fri, Sep 10, 2021 at 12:24 PM Andrew Halaney <[email protected]> wrote:
> >
> > On Fri, Sep 10, 2021 at 11:14:45AM -0600, [email protected] wrote:
> > > On Thu, Sep 9, 2021 at 3:34 PM Jason Baron <[email protected]> wrote:
> > >
> > > >
> > > >
> > > > On 9/9/21 12:17 PM, Andrew Halaney wrote:
> > > > > Right now dyndbg shows up as an unknown parameter if used on boot:
> > > > >
> > > > > Unknown command line parameters: dyndbg=module params +p ; module
> > > > sys +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.
> > > > >
> > >
> > >
> > > your usage is incorrect for what youre trying to do in that example
> > > what you need is:
> > >
> > > params.dyndbg=+p sys.dyndbg=+p
> > >
> > > dyndbg is properly unknown as a kernel param, it isnt one.
> > > ( it was called a "fake" module param when added.)
> > > $module.dyndbg is good, since its after the $module. (and the dot)
> > > it also then inherits the "scan bootargs for relevant ones on module load"
> > > behavior
> > >
> > >
> >
> > That example is (slightly altered) from
> > Documentation/admin-guide/dynamic-debug-howto.rst,
>
> oh dear, I see the lines youre referring to.
> I am surprised.
> fyi, those lines are:
> // enable pr_debugs in 2 builtins, #cmt is stripped
> dyndbg="module params +p #cmt ; module sys +p"
>
> is your patchset removing those lines ?
> if so, ack that part.
>
> > I can change the example used to be a little less confusing (using the
> > module keyword is confusing, I could use something like
> > func or file instead of what the docs use as an example).
>
> yes please, I saw bad usage, thought faulty premise.
>
> > Is that what you're after, a better example usage of dyndbg= being
> > whined about in dmesg for the commit message, or am I misunderstanding?
>
> I guess Im inured to it. Heres my regular version with a similar addition.
>
> Unknown command line parameters:
> virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0
> virtme_initmount0=/root virtme_initmount1=/root/sbin
> virtme_stty_con=rows 27 cols 109 iutf8
> virtme_chdir=home/jimc/projects/lx dyndbg=+p
>
> most of them do something, just not for the kernel.
>
> I dont think this is worth explicitly silencing.
> rather rip out the misleading doc.
>
Ohhhhh, ok now I think I'm following what you and Jason are saying.
dyndbg= parameter does need to process the whole cli, for cases like
when $module.dyndbg= is supplied but $module is a builtin. And you are
saying that this syntax (although it works)
'dyndbg="module params +p #cmt ; module sys +p"' is not the intended
usage.
So converting dyndbg= to act like ddebug_query= won't work because
$module.dyndbg="+p" should work if $module is builtin or a module
(settles my open discussion with Jason).
I'm going to hold my ground and try and silence the warning, because I
think the kernel parameters interface is well defined enough (kernel
params go before the -- i.e. "these are kernel params -- these are for init"
With that in mind I'll make a v2 series out of this doing:
1. Clean up the doc to show intended usage on cli, something like
(params.dyndbg="+p" sys.dyndbg="+p" dyndbg="file init/main.c +p #cmt ; func pc87360_init_device +p")
2. Remove ddebug_query per Jason's approval
3. Silence the param with this patch here, with a commit message
updated explaining why dyndbg= needs to be able to process the whole
cli, per Jason
Please lemme know if there are any strong objections in the meantime and
thanks for the feedback!
>
> > I agree that dyndbg right now (both dyndbg= and $module.dyndbg=) are
> > "fake" params, but I'd like to remove that message for dyndbg= as it is
> > misleading. The code for module loading luckily already handles it all
> > properly and doesn't warn on proper usage:
> >
> > static int unknown_module_param_cb(char *param, char *val, const char *modname,
> > void *arg)
> > {
> > struct module *mod = arg;
> > int ret;
> >
> > if (strcmp(param, "async_probe") == 0) {
> > mod->async_probe_requested = true;
> > return 0;
> > }
> >
> > /* Check for magic 'dyndbg' arg */
> > ret = ddebug_dyndbg_module_param_cb(param, val, modname);
> > if (ret != 0)
> > pr_warn("%s: unknown parameter '%s' ignored\n", modname, param);
> > return 0;
> > }
> >
> >
> > >
> > >
> > >
> > > >
> > > > > Install a dummy handler to register it. This was chosen instead of the
> > > > > approach the (deprecated) ddebug_query param takes, which is to
> > > > > have a real handler that copies the string taking up a KiB memory.
> > > > >
> > > > > Fixes: 86d1919a4fb0 ("init: print out unknown kernel parameters")
> > > > > Signed-off-by: Andrew Halaney <[email protected]>
> > > > > ---
> > > > >
> > > > > This is the last valid param I know of that was getting flagged on boot
> > > > > if used correctly. Please let me know if the alternative approach of
> > > > > actually copying the string is preferred and I'll spin that up instead.
> > > > >
> > > >
> > > > Hi Andrew,
> > > >
> > > > So I think you are referring to the string copying that ddebug_query= does.
> > > > I don't think that works for 'dyndbg=' b/c its actually looking through
> > > > the entire command line for stuff like <module_name>.dyndbg="blah".
> > > >
> > > > So I think what you prposed below makes sense, we could perhaps add a note
> > > > as to why it's a noop. As I mentioned it needs to look through the entire
> > > > string.
> > > >
> > > >
> > > > > Sort of an aside, but what's the policy for removing deprecated cli
> > > > > params? ddebug_query has been deprecated for a very long time now, but I
> > > > > am not sure if removing params like that is considered almost as bad as
> > > > > breaking userspace considering some systems might update their kernels
> > > > > but not the bootloader supplying the param.
> > > >
> > > > I think it's probably ok to remove at this point, especially now that
> > > > we are going to flag it as unknown, right? So I feel like that change
> > > > can logically go with this series if you want as a separate patch.
> > > >
> > > > Thanks,
> > > >
> > > > -Jason
> > > >
> > > >
> > > > >
> > > > > 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.
> > > > >
> > > >
> >
>
On Fri, Sep 10, 2021 at 03:16:35PM -0500, Andrew Halaney wrote:
> On Fri, Sep 10, 2021 at 01:31:22PM -0600, [email protected] wrote:
> > On Fri, Sep 10, 2021 at 12:24 PM Andrew Halaney <[email protected]> wrote:
> > >
> > > On Fri, Sep 10, 2021 at 11:14:45AM -0600, [email protected] wrote:
> > > > On Thu, Sep 9, 2021 at 3:34 PM Jason Baron <[email protected]> wrote:
> > > >
> > > > >
> > > > >
> > > > > On 9/9/21 12:17 PM, Andrew Halaney wrote:
> > > > > > Right now dyndbg shows up as an unknown parameter if used on boot:
> > > > > >
> > > > > > Unknown command line parameters: dyndbg=module params +p ; module
> > > > > sys +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.
> > > > > >
> > > >
> > > >
> > > > your usage is incorrect for what youre trying to do in that example
> > > > what you need is:
> > > >
> > > > params.dyndbg=+p sys.dyndbg=+p
> > > >
> > > > dyndbg is properly unknown as a kernel param, it isnt one.
> > > > ( it was called a "fake" module param when added.)
> > > > $module.dyndbg is good, since its after the $module. (and the dot)
> > > > it also then inherits the "scan bootargs for relevant ones on module load"
> > > > behavior
> > > >
> > > >
> > >
> > > That example is (slightly altered) from
> > > Documentation/admin-guide/dynamic-debug-howto.rst,
> >
> > oh dear, I see the lines youre referring to.
> > I am surprised.
> > fyi, those lines are:
> > // enable pr_debugs in 2 builtins, #cmt is stripped
> > dyndbg="module params +p #cmt ; module sys +p"
> >
> > is your patchset removing those lines ?
> > if so, ack that part.
> >
> > > I can change the example used to be a little less confusing (using the
> > > module keyword is confusing, I could use something like
> > > func or file instead of what the docs use as an example).
> >
> > yes please, I saw bad usage, thought faulty premise.
> >
> > > Is that what you're after, a better example usage of dyndbg= being
> > > whined about in dmesg for the commit message, or am I misunderstanding?
> >
> > I guess Im inured to it. Heres my regular version with a similar addition.
> >
> > Unknown command line parameters:
> > virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0
> > virtme_initmount0=/root virtme_initmount1=/root/sbin
> > virtme_stty_con=rows 27 cols 109 iutf8
> > virtme_chdir=home/jimc/projects/lx dyndbg=+p
> >
> > most of them do something, just not for the kernel.
> >
> > I dont think this is worth explicitly silencing.
> > rather rip out the misleading doc.
> >
>
> Ohhhhh, ok now I think I'm following what you and Jason are saying.
>
> dyndbg= parameter does need to process the whole cli, for cases like
> when $module.dyndbg= is supplied but $module is a builtin. And you are
> saying that this syntax (although it works)
> 'dyndbg="module params +p #cmt ; module sys +p"' is not the intended
> usage.
>
> So converting dyndbg= to act like ddebug_query= won't work because
> $module.dyndbg="+p" should work if $module is builtin or a module
> (settles my open discussion with Jason).
Egh, sorry for double reply here but I think I got too excited with that
realization. $module.dyndbg="+p" won't work if $module is a builtin
unless dyndbg= is also on the command line independently. So I'm kind of
thinking that the current documentation showing
'dyndbg="module params +p #cmt ; module sys +p"' is correct for
params/sys as long as they're builtins.
So the whole [1] and [3] in the list below don't quite make sense to me
and the questions I had to Jason are still open.... dang.
>
> I'm going to hold my ground and try and silence the warning, because I
> think the kernel parameters interface is well defined enough (kernel
> params go before the -- i.e. "these are kernel params -- these are for init"
>
> With that in mind I'll make a v2 series out of this doing:
> 1. Clean up the doc to show intended usage on cli, something like
> (params.dyndbg="+p" sys.dyndbg="+p" dyndbg="file init/main.c +p #cmt ; func pc87360_init_device +p")
> 2. Remove ddebug_query per Jason's approval
> 3. Silence the param with this patch here, with a commit message
> updated explaining why dyndbg= needs to be able to process the whole
> cli, per Jason
>
> Please lemme know if there are any strong objections in the meantime and
> thanks for the feedback!
>
> >
> > > I agree that dyndbg right now (both dyndbg= and $module.dyndbg=) are
> > > "fake" params, but I'd like to remove that message for dyndbg= as it is
> > > misleading. The code for module loading luckily already handles it all
> > > properly and doesn't warn on proper usage:
> > >
> > > static int unknown_module_param_cb(char *param, char *val, const char *modname,
> > > void *arg)
> > > {
> > > struct module *mod = arg;
> > > int ret;
> > >
> > > if (strcmp(param, "async_probe") == 0) {
> > > mod->async_probe_requested = true;
> > > return 0;
> > > }
> > >
> > > /* Check for magic 'dyndbg' arg */
> > > ret = ddebug_dyndbg_module_param_cb(param, val, modname);
> > > if (ret != 0)
> > > pr_warn("%s: unknown parameter '%s' ignored\n", modname, param);
> > > return 0;
> > > }
> > >
> > >
> > > >
> > > >
> > > >
> > > > >
> > > > > > Install a dummy handler to register it. This was chosen instead of the
> > > > > > approach the (deprecated) ddebug_query param takes, which is to
> > > > > > have a real handler that copies the string taking up a KiB memory.
> > > > > >
> > > > > > Fixes: 86d1919a4fb0 ("init: print out unknown kernel parameters")
> > > > > > Signed-off-by: Andrew Halaney <[email protected]>
> > > > > > ---
> > > > > >
> > > > > > This is the last valid param I know of that was getting flagged on boot
> > > > > > if used correctly. Please let me know if the alternative approach of
> > > > > > actually copying the string is preferred and I'll spin that up instead.
> > > > > >
> > > > >
> > > > > Hi Andrew,
> > > > >
> > > > > So I think you are referring to the string copying that ddebug_query= does.
> > > > > I don't think that works for 'dyndbg=' b/c its actually looking through
> > > > > the entire command line for stuff like <module_name>.dyndbg="blah".
> > > > >
> > > > > So I think what you prposed below makes sense, we could perhaps add a note
> > > > > as to why it's a noop. As I mentioned it needs to look through the entire
> > > > > string.
> > > > >
> > > > >
> > > > > > Sort of an aside, but what's the policy for removing deprecated cli
> > > > > > params? ddebug_query has been deprecated for a very long time now, but I
> > > > > > am not sure if removing params like that is considered almost as bad as
> > > > > > breaking userspace considering some systems might update their kernels
> > > > > > but not the bootloader supplying the param.
> > > > >
> > > > > I think it's probably ok to remove at this point, especially now that
> > > > > we are going to flag it as unknown, right? So I feel like that change
> > > > > can logically go with this series if you want as a separate patch.
> > > > >
> > > > > Thanks,
> > > > >
> > > > > -Jason
> > > > >
> > > > >
> > > > > >
> > > > > > 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.
> > > > > >
> > > > >
> > >
> >
On Fri, Sep 10, 2021 at 2:16 PM Andrew Halaney <[email protected]> wrote:
>
> On Fri, Sep 10, 2021 at 01:31:22PM -0600, [email protected] wrote:
> > On Fri, Sep 10, 2021 at 12:24 PM Andrew Halaney <[email protected]> wrote:
> > >
> > > On Fri, Sep 10, 2021 at 11:14:45AM -0600, [email protected] wrote:
> > > > On Thu, Sep 9, 2021 at 3:34 PM Jason Baron <[email protected]> wrote:
> > > >
> > > > >
> > > > >
> > > > > On 9/9/21 12:17 PM, Andrew Halaney wrote:
> > > > > > Right now dyndbg shows up as an unknown parameter if used on boot:
> > > > > >
> > > > > > Unknown command line parameters: dyndbg=module params +p ; module
> > > > > sys +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.
> > > > > >
> > > >
> > > >
> > > > your usage is incorrect for what youre trying to do in that example
> > > > what you need is:
> > > >
> > > > params.dyndbg=+p sys.dyndbg=+p
> > > >
> > > > dyndbg is properly unknown as a kernel param, it isnt one.
> > > > ( it was called a "fake" module param when added.)
> > > > $module.dyndbg is good, since its after the $module. (and the dot)
> > > > it also then inherits the "scan bootargs for relevant ones on module load"
> > > > behavior
> > > >
> > > >
> > >
> > > That example is (slightly altered) from
> > > Documentation/admin-guide/dynamic-debug-howto.rst,
> >
> > oh dear, I see the lines youre referring to.
> > I am surprised.
> > fyi, those lines are:
> > // enable pr_debugs in 2 builtins, #cmt is stripped
> > dyndbg="module params +p #cmt ; module sys +p"
> >
> > is your patchset removing those lines ?
> > if so, ack that part.
> >
> > > I can change the example used to be a little less confusing (using the
> > > module keyword is confusing, I could use something like
> > > func or file instead of what the docs use as an example).
> >
> > yes please, I saw bad usage, thought faulty premise.
> >
> > > Is that what you're after, a better example usage of dyndbg= being
> > > whined about in dmesg for the commit message, or am I misunderstanding?
> >
> > I guess Im inured to it. Heres my regular version with a similar addition.
> >
> > Unknown command line parameters:
> > virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0
> > virtme_initmount0=/root virtme_initmount1=/root/sbin
> > virtme_stty_con=rows 27 cols 109 iutf8
> > virtme_chdir=home/jimc/projects/lx dyndbg=+p
> >
> > most of them do something, just not for the kernel.
> >
> > I dont think this is worth explicitly silencing.
> > rather rip out the misleading doc.
> >
>
> Ohhhhh, ok now I think I'm following what you and Jason are saying.
>
> dyndbg= parameter does need to process the whole cli, for cases like
> when $module.dyndbg= is supplied but $module is a builtin. And you are
> saying that this syntax (although it works)
> 'dyndbg="module params +p #cmt ; module sys +p"' is not the intended
> usage.
>
> So converting dyndbg= to act like ddebug_query= won't work because
> $module.dyndbg="+p" should work if $module is builtin or a module
> (settles my open discussion with Jason).
yes, $mod.dyndbg=+p works now whether $mod is builtin or loadable.
that must stick.
if bare dyndbg="str" works like an alias for ddebug_query=
(its amazing what one forgets)
then I agree the warning is misleading.
>
> So converting dyndbg= to act like ddebug_query= won't work because
> $module.dyndbg="+p" should work if $module is builtin or a module
> (settles my open discussion with Jason).
>
they can both work, because the dot.
\w+.\w+ is separate, which is why it works for loadable modules.
and Ive just re-verified that these both ( bare/global and after $mod. ) work
dyndbg=+pmfl
turns on all builtins
[ 128.341650] 8250:serial8250_interrupt:140: serial8250_interrupt(4): end
[ 128.344996] 8250:serial8250_interrupt:140: serial8250_interrupt(4): end
^C
bash-5.1# echo -p > /proc/dynamic_debug/control
dyndbg: read 3 bytes from userspace <
-p
>
dyndbg: query 0: <-p> mod:<*>
dyndbg: split into words: <-p>
dyndbg: op=<->
dyndbg: flags=0x1
dyndbg: *flagsp=0x0 *maskp=0xfffffffe
dyndbg: parsed: func=<> file=<> module=<> format=<> lineno=0-0
dyndbg: applied: func=<> file=<> module=<> format=<> lineno=0-0
dyndbg: processed 1 queries, with 3055 matches, 0 errs
bash-5.1# cat /proc/cmdline
virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0
virtme_initmount0=/root virtme_initmount1=/root/sbin
earlyprintk=serial,ttyS0,115200 console=ttyS0 psmouse.proto=exps
"virtme_stty_con=rows 27 cols 109 iutf8" TERM=xterm-256color
virtme_chdir=home/jimc/projects/lx rootfstype=9p
rootflags=version=9p2000.L,trans=virtio,access=any,msize=200M
raid=noautodetect ro nokaslr dynamic_debug.verbose=3
module.dyndbg=+pmf dyndbg=+pmfl init=/bin/sh -- -c "mount -t tmpfs run
/run;mkdir -p /run/virtme/guesttools;/bin/mount -n -t 9p -o
ro,version=9p2000.L,trans=virtio,access=any,msize=104857600
virtme.guesttools /run/virtme/guesttools;exec
/run/virtme/guesttools/virtme-init"
bash-5.1# ^C
bash-5.1# modprobe i915
dyndbg: 384 debug prints in module drm
dyndbg: 211 debug prints in module drm_kms_helper
dyndbg: 2 debug prints in module ttm
dyndbg: 8 debug prints in module video
dyndbg: 1821 debug prints in module i915
**********************************************************
** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **
** **
** trace_printk() being used. Allocating extra memory. **
** **
** This means that this is a DEBUG kernel and it is **
** unsafe for production use. **
** **
** If you see this message and you are not debugging **
** the kernel, report this immediately to your vendor! **
** **
** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **
**********************************************************
bash-5.1# wc /proc/dynamic_debug/control
5482 47545 613066 /proc/dynamic_debug/control
bash-5.1# grep =p /proc/dynamic_debug/control | wc
3 21 244
bash-5.1# QEMU: Terminated
bash-5.1# wc /proc/dynamic_debug/control
3056 24089 307832 /proc/dynamic_debug/control
bash-5.1# grep =pfml /proc/dynamic_debug/control | wc
0 0 0
bash-5.1# cat /proc/cmdline
virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0
virtme_initmount0=/root virtme_initmount1=/root/sbin
earlyprintk=serial,ttyS0,115200 console=ttyS0 psmouse.proto=exps
"virtme_stty_con=rows 27 cols 109 iutf8" TERM=xterm-256color
virtme_chdir=home/jimc/projects/lx rootfstype=9p
rootflags=version=9p2000.L,trans=virtio,access=any,msize=200M
raid=noautodetect ro nokaslr dynamic_debug.verbose=3
module.dyndbg=+pmf *.dyndbg=+pmfl init=/bin/sh -- -c "mount -t tmpfs
run /run;mkdir -p /run/virtme/guesttools;/bin/mount -n -t 9p -o
ro,version=9p2000.L,trans=virtio,access=any,msize=104857600
virtme.guesttools /run/virtme/guesttools;exec
/run/virtme/guesttools/virtme-init"
bash-5.1# grep =p /proc/dynamic_debug/control | wc
3039 23944 305800
bash-5.1# grep =pmfl /proc/dynamic_debug/control |wc
3039 23944 305800
bash-5.1# echo -p > /proc/dynamic_debug/control
dyndbg: read 3 bytes from userspace <
-p
>
dyndbg: query 0: <-p> mod:<*>
dyndbg: split into words: <-p>
dyndbg: op=<->
dyndbg: flags=0x1
dyndbg: *flagsp=0x0 *maskp=0xfffffffe
dyndbg: parsed: func=<> file=<> module=<> format=<> lineno=0-0
dyndbg: applied: func=<> file=<> module=<> format=<> lineno=0-0
dyndbg: processed 1 queries, with 3055 matches, 0 errs
bash-5.1# grep =pmfl /proc/dynamic_debug/control |wc
0 0 0
> I'm going to hold my ground and try and silence the warning, because I
> think the kernel parameters interface is well defined enough (kernel
> params go before the -- i.e. "these are kernel params -- these are for init"
>
Policy zone: DMA32
Kernel command line:
virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0
virtme_initmount0=/root virtme_initmount1=/root/sbin
earlyprintk=serial,ttyS0,115200 console=ttyS0 psmouse.proto=exps
"virtme_stty_con=rows 27 cols 109 iutf8" TERM=xterm-256color
virtme_chdir=home/jimc/projects/lx rootfstype=9p
rootflags=version=9p2000.L,trans=virtio,access=any,msize=200M
raid=noautodetect ro nokaslr dynamic_debug.verbose=3
module.dyndbg=+pmf init=/bin/sh -- -c "mount -t tmpfs run /run;mkdir
-p /run/virtme/guesttools;/bin/mount -n -t 9p -o
ro,version=9p2000.L,trans=virtio,access=any,msize=104857600
virtme.guesttools /run/virtme/guesttools;exec
/run/virtme/guesttools/virtme-init"
Unknown command line parameters:
virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0
virtme_initmount0=/root virtme_initmount1=/root/sbin
virtme_stty_con=rows 27 cols 109 iutf8
virtme_chdir=home/jimc/projects/lx
Dentry cache hash table entries: 262144 (order: 9, 2097152 bytes, linear)
in the above, I see unknown complaints about items after the --
But I agree the warning is misleading.
> With that in mind I'll make a v2 series out of this doing:
> 1. Clean up the doc to show intended usage on cli, something like
> (params.dyndbg="+p" sys.dyndbg="+p" dyndbg="file init/main.c +p #cmt ; func pc87360_init_device +p")
how about 1 builtin, 1 loadable, and use a wildcard somewhere,
to hint that theres more than 2 ways to make a "group"
> 2. Remove ddebug_query per Jason's approval
sorry for not keeping up - is the problem the static storage ?
is there a way to fix that for all setup= stuff ?
if you could make ddebug_query into an alias for dyndbg it might fix
the narrow case.
> 3. Silence the param with this patch here, with a commit message
> updated explaining why dyndbg= needs to be able to process the whole
> cli, per Jason
>
> Please lemme know if there are any strong objections in the meantime and
> thanks for the feedback!
>
> >
> > > I agree that dyndbg right now (both dyndbg= and $module.dyndbg=) are
> > > "fake" params, but I'd like to remove that message for dyndbg= as it is
> > > misleading. The code for module loading luckily already handles it all
> > > properly and doesn't warn on proper usage:
s/luckily/serendipitously/ - the way fake just worked was an aha for me.
just dont kill the fake-ness while quieting the warning
thanks
JIm
On Fri, Sep 10, 2021 at 09:04:20PM -0600, [email protected] wrote:
> >
> > So converting dyndbg= to act like ddebug_query= won't work because
> > $module.dyndbg="+p" should work if $module is builtin or a module
> > (settles my open discussion with Jason).
> >
>
> they can both work, because the dot.
>
> \w+.\w+ is separate, which is why it works for loadable modules.
>
> and Ive just re-verified that these both ( bare/global and after $mod. ) work
>
> dyndbg=+pmfl
> turns on all builtins
Yeah, you are totally right, my head clearly was not working well Friday
afternoon. Although I think converting dyndbg= to behave more like
ddebug_query wrt copying the string is pointless, since you've
shown quite well here that we need to process the whole command line
when we start up dynamic debug to catch any $module.dyndbg= usage that is
actually builtin currently. So you've settled that for me, I think the
noop approach here is best, but will update commit message to reflect
reality.
>
> [ 128.341650] 8250:serial8250_interrupt:140: serial8250_interrupt(4): end
> [ 128.344996] 8250:serial8250_interrupt:140: serial8250_interrupt(4): end
> ^C
> bash-5.1# echo -p > /proc/dynamic_debug/control
> dyndbg: read 3 bytes from userspace <
> -p
> >
> dyndbg: query 0: <-p> mod:<*>
> dyndbg: split into words: <-p>
> dyndbg: op=<->
> dyndbg: flags=0x1
> dyndbg: *flagsp=0x0 *maskp=0xfffffffe
> dyndbg: parsed: func=<> file=<> module=<> format=<> lineno=0-0
> dyndbg: applied: func=<> file=<> module=<> format=<> lineno=0-0
> dyndbg: processed 1 queries, with 3055 matches, 0 errs
> bash-5.1# cat /proc/cmdline
> virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0
> virtme_initmount0=/root virtme_initmount1=/root/sbin
> earlyprintk=serial,ttyS0,115200 console=ttyS0 psmouse.proto=exps
> "virtme_stty_con=rows 27 cols 109 iutf8" TERM=xterm-256color
> virtme_chdir=home/jimc/projects/lx rootfstype=9p
> rootflags=version=9p2000.L,trans=virtio,access=any,msize=200M
> raid=noautodetect ro nokaslr dynamic_debug.verbose=3
> module.dyndbg=+pmf dyndbg=+pmfl init=/bin/sh -- -c "mount -t tmpfs run
> /run;mkdir -p /run/virtme/guesttools;/bin/mount -n -t 9p -o
> ro,version=9p2000.L,trans=virtio,access=any,msize=104857600
> virtme.guesttools /run/virtme/guesttools;exec
> /run/virtme/guesttools/virtme-init"
> bash-5.1# ^C
> bash-5.1# modprobe i915
> dyndbg: 384 debug prints in module drm
> dyndbg: 211 debug prints in module drm_kms_helper
> dyndbg: 2 debug prints in module ttm
> dyndbg: 8 debug prints in module video
> dyndbg: 1821 debug prints in module i915
>
> **********************************************************
> ** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **
> ** **
> ** trace_printk() being used. Allocating extra memory. **
> ** **
> ** This means that this is a DEBUG kernel and it is **
> ** unsafe for production use. **
> ** **
> ** If you see this message and you are not debugging **
> ** the kernel, report this immediately to your vendor! **
> ** **
> ** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **
> **********************************************************
> bash-5.1# wc /proc/dynamic_debug/control
> 5482 47545 613066 /proc/dynamic_debug/control
> bash-5.1# grep =p /proc/dynamic_debug/control | wc
> 3 21 244
> bash-5.1# QEMU: Terminated
>
>
>
> bash-5.1# wc /proc/dynamic_debug/control
> 3056 24089 307832 /proc/dynamic_debug/control
> bash-5.1# grep =pfml /proc/dynamic_debug/control | wc
> 0 0 0
> bash-5.1# cat /proc/cmdline
> virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0
> virtme_initmount0=/root virtme_initmount1=/root/sbin
> earlyprintk=serial,ttyS0,115200 console=ttyS0 psmouse.proto=exps
> "virtme_stty_con=rows 27 cols 109 iutf8" TERM=xterm-256color
> virtme_chdir=home/jimc/projects/lx rootfstype=9p
> rootflags=version=9p2000.L,trans=virtio,access=any,msize=200M
> raid=noautodetect ro nokaslr dynamic_debug.verbose=3
> module.dyndbg=+pmf *.dyndbg=+pmfl init=/bin/sh -- -c "mount -t tmpfs
> run /run;mkdir -p /run/virtme/guesttools;/bin/mount -n -t 9p -o
> ro,version=9p2000.L,trans=virtio,access=any,msize=104857600
> virtme.guesttools /run/virtme/guesttools;exec
> /run/virtme/guesttools/virtme-init"
> bash-5.1# grep =p /proc/dynamic_debug/control | wc
> 3039 23944 305800
> bash-5.1# grep =pmfl /proc/dynamic_debug/control |wc
> 3039 23944 305800
> bash-5.1# echo -p > /proc/dynamic_debug/control
> dyndbg: read 3 bytes from userspace <
> -p
> >
> dyndbg: query 0: <-p> mod:<*>
> dyndbg: split into words: <-p>
> dyndbg: op=<->
> dyndbg: flags=0x1
> dyndbg: *flagsp=0x0 *maskp=0xfffffffe
> dyndbg: parsed: func=<> file=<> module=<> format=<> lineno=0-0
> dyndbg: applied: func=<> file=<> module=<> format=<> lineno=0-0
> dyndbg: processed 1 queries, with 3055 matches, 0 errs
> bash-5.1# grep =pmfl /proc/dynamic_debug/control |wc
> 0 0 0
>
>
>
> > I'm going to hold my ground and try and silence the warning, because I
> > think the kernel parameters interface is well defined enough (kernel
> > params go before the -- i.e. "these are kernel params -- these are for init"
> >
>
>
> Policy zone: DMA32
> Kernel command line:
> virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0
> virtme_initmount0=/root virtme_initmount1=/root/sbin
> earlyprintk=serial,ttyS0,115200 console=ttyS0 psmouse.proto=exps
> "virtme_stty_con=rows 27 cols 109 iutf8" TERM=xterm-256color
> virtme_chdir=home/jimc/projects/lx rootfstype=9p
> rootflags=version=9p2000.L,trans=virtio,access=any,msize=200M
> raid=noautodetect ro nokaslr dynamic_debug.verbose=3
> module.dyndbg=+pmf init=/bin/sh -- -c "mount -t tmpfs run /run;mkdir
> -p /run/virtme/guesttools;/bin/mount -n -t 9p -o
> ro,version=9p2000.L,trans=virtio,access=any,msize=104857600
> virtme.guesttools /run/virtme/guesttools;exec
> /run/virtme/guesttools/virtme-init"
> Unknown command line parameters:
> virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0
> virtme_initmount0=/root virtme_initmount1=/root/sbin
> virtme_stty_con=rows 27 cols 109 iutf8
> virtme_chdir=home/jimc/projects/lx
> Dentry cache hash table entries: 262144 (order: 9, 2097152 bytes, linear)
>
>
> in the above, I see unknown complaints about items after the --
I'm the doofus who added the warning message output, so I definitely
don't want to ignore you claiming there are complaints after the --
because there shouldn't be... but I've re-read that block 3 times now and
don't see anything after the -- that's in the unknown list. Can you
please highlight it for me? I'd like to figure out why it is showing up
if that's happening.
>
> But I agree the warning is misleading.
>
>
>
>
> > With that in mind I'll make a v2 series out of this doing:
> > 1. Clean up the doc to show intended usage on cli, something like
> > (params.dyndbg="+p" sys.dyndbg="+p" dyndbg="file init/main.c +p #cmt ; func pc87360_init_device +p")
>
> how about 1 builtin, 1 loadable, and use a wildcard somewhere,
> to hint that theres more than 2 ways to make a "group"
>
Sure, I'll illustrate using:
builtin (with explicit comment about how this works for when params
module is builtin or loadable): params.dyndbg="+p"
module: kvm.dyndbg="+p"
wildcard/dyndbg: dyndbg="file init/* +p #cmt ; func pc87360_init_device +p"
Please let me know of any objections (or wait till v2) on that list..
not sure I hit the nail on the head so throwing that up for "pre-review"
I suppose.
> > 2. Remove ddebug_query per Jason's approval
>
> sorry for not keeping up - is the problem the static storage ?
> is there a way to fix that for all setup= stuff ?
> if you could make ddebug_query into an alias for dyndbg it might fix
> the narrow case.
>
No real problem, I just looked at this code for the first time to tackle
the dyndbg= warning and looked into ddebug_query a bit. After looking I
realized it has been deprecated for quite a while and thought it would
be good to remove it if there's no objections just to clean up house.
It does eat some static storage for the string. I'm not sure if there's
a great way to solve such a problem systematically, but yes ddebug_query
could be made into an alias for dyndbg and function fine still for this
case. If there's some objection to removing it entirely I'll do that just
to save a little more space.
>
> > 3. Silence the param with this patch here, with a commit message
> > updated explaining why dyndbg= needs to be able to process the whole
> > cli, per Jason
> >
> > Please lemme know if there are any strong objections in the meantime and
> > thanks for the feedback!
> >
> > >
> > > > I agree that dyndbg right now (both dyndbg= and $module.dyndbg=) are
> > > > "fake" params, but I'd like to remove that message for dyndbg= as it is
> > > > misleading. The code for module loading luckily already handles it all
> > > > properly and doesn't warn on proper usage:
>
> s/luckily/serendipitously/ - the way fake just worked was an aha for me.
>
> just dont kill the fake-ness while quieting the warning
>
> thanks
> JIm
>