mbox series

[v3,0/7] Venus dynamic debug

Message ID 20200609104604.1594-1-stanimir.varbanov@linaro.org
Headers show
Series Venus dynamic debug | expand

Message

Stanimir Varbanov June 9, 2020, 10:45 a.m. UTC
Hello,

Here is the third version of dynamic debug improvements in Venus
driver.  As has been suggested on previous version by Joe [1] I've
made the relevant changes in dynamic debug core to handle leveling
as more generic way and not open-code/workaround it in the driver.

About changes:
 - added change in the dynamic_debug and in documentation
 - added respective pr_debug_level and dev_dbg_level

regards,
Stan

[1] https://lkml.org/lkml/2020/5/21/668

Stanimir Varbanov (7):
  Documentation: dynamic-debug: Add description of level bitmask
  dynamic_debug: Group debug messages by level bitmask
  dev_printk: Add dev_dbg_level macro over dynamic one
  printk: Add pr_debug_level macro over dynamic one
  venus: Add debugfs interface to set firmware log level
  venus: Make debug infrastructure more flexible
  venus: Add a debugfs file for SSR trigger

 .../admin-guide/dynamic-debug-howto.rst       | 10 +++
 drivers/media/platform/qcom/venus/Makefile    |  2 +-
 drivers/media/platform/qcom/venus/core.c      |  5 ++
 drivers/media/platform/qcom/venus/core.h      |  8 +++
 drivers/media/platform/qcom/venus/dbgfs.c     | 57 +++++++++++++++++
 drivers/media/platform/qcom/venus/dbgfs.h     | 12 ++++
 drivers/media/platform/qcom/venus/helpers.c   |  2 +-
 drivers/media/platform/qcom/venus/hfi_msgs.c  | 30 ++++-----
 drivers/media/platform/qcom/venus/hfi_venus.c | 27 ++++++--
 .../media/platform/qcom/venus/pm_helpers.c    |  3 +-
 drivers/media/platform/qcom/venus/vdec.c      | 63 +++++++++++++++++--
 drivers/media/platform/qcom/venus/venc.c      |  4 ++
 fs/btrfs/ctree.h                              | 12 ++--
 include/linux/acpi.h                          |  3 +-
 include/linux/dev_printk.h                    | 12 +++-
 include/linux/dynamic_debug.h                 | 55 +++++++++++-----
 include/linux/net.h                           |  3 +-
 include/linux/printk.h                        |  9 ++-
 lib/dynamic_debug.c                           | 30 +++++++++
 19 files changed, 289 insertions(+), 58 deletions(-)
 create mode 100644 drivers/media/platform/qcom/venus/dbgfs.c
 create mode 100644 drivers/media/platform/qcom/venus/dbgfs.h

-- 
2.17.1

Comments

Greg KH June 9, 2020, 11:12 a.m. UTC | #1
On Tue, Jun 09, 2020 at 01:46:02PM +0300, Stanimir Varbanov wrote:
> +int venus_dbgfs_init(struct venus_core *core)
> +{
> +	core->root = debugfs_create_dir("venus", NULL);
> +	if (IS_ERR(core->root))
> +		return IS_ERR(core->root);

You really do not care, and obviously did not test this on a system with
CONFIG_DEBUG_FS disabled :)

Just make the call to debugfs, and move on, feed it into other debugfs
calls, all is good.

This function should just return 'void', no need to care about this at
all.

> +	ret = venus_sys_set_debug(hdev, venus_fw_debug);
> +	if (ret)
> +		dev_warn(dev, "setting fw debug msg ON failed (%d)\n", ret);

Why do you care about this "error"?

thanks,

greg k-h
Matthew Wilcox June 9, 2020, 11:13 a.m. UTC | #2
On Tue, Jun 09, 2020 at 01:45:57PM +0300, Stanimir Varbanov wrote:
> Here is the third version of dynamic debug improvements in Venus

> driver.  As has been suggested on previous version by Joe [1] I've

> made the relevant changes in dynamic debug core to handle leveling

> as more generic way and not open-code/workaround it in the driver.

> 

> About changes:

>  - added change in the dynamic_debug and in documentation

>  - added respective pr_debug_level and dev_dbg_level


Honestly, this seems like you want to use tracepoints, not dynamic debug.
Greg KH June 9, 2020, 11:16 a.m. UTC | #3
On Tue, Jun 09, 2020 at 01:45:58PM +0300, Stanimir Varbanov wrote:
> This adds description of the level bitmask feature.

> 

> Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)

> 

> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

> ---

>  Documentation/admin-guide/dynamic-debug-howto.rst | 10 ++++++++++

>  1 file changed, 10 insertions(+)

> 

> diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst

> index 0dc2eb8e44e5..c2b751fc8a17 100644

> --- a/Documentation/admin-guide/dynamic-debug-howto.rst

> +++ b/Documentation/admin-guide/dynamic-debug-howto.rst

> @@ -208,6 +208,12 @@ line

>  	line -1605          // the 1605 lines from line 1 to line 1605

>  	line 1600-          // all lines from line 1600 to the end of the file

>  

> +level

> +    The given level will be a bitmask ANDed with the level of the each ``pr_debug()``

> +    callsite. This will allow to group debug messages and show only those of the

> +    same level.  The -p flag takes precedence over the given level. Note that we can

> +    have up to five groups of debug messages.


As was pointed out, this isn't a "level", it's some arbitrary type of
"grouping".

But step back, why?  What is wrong with the existing control of dynamic
debug messages that you want to add another type of arbitrary grouping
to it?  And who defines that grouping?  Will it be
driver/subsystem/arch/author specific?  Or kernel-wide?

This feels like it could easily get out of hand really quickly.

Why not just use tracepoints if you really want to be fine-grained?

thanks,

greg k-h
Petr Mladek June 9, 2020, 12:27 p.m. UTC | #4
On Tue 2020-06-09 13:45:59, Stanimir Varbanov wrote:
> This will allow dynamic debug users and driver writers to group

> debug messages by level bitmask.  The level bitmask should be a

> hex number.

> 

> Done this functionality by extending dynamic debug metadata with

> new level member and propagate it over all the users.  Also

> introduce new dynamic_pr_debug_level and dynamic_dev_dbg_level

> macros to be used by the drivers.


Could you please provide more details?

What is the use case?
What is the exact meaning of the level value?
How the levels will get defined?

Dynamic debug is used for messages with KERN_DEBUG log level.
Is this another dimension of the message leveling?

Given that the filter is defined by bits, it is rather grouping
by context or so.


> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c

> index 8f199f403ab5..5d28d388f6dd 100644

> --- a/lib/dynamic_debug.c

> +++ b/lib/dynamic_debug.c

> @@ -55,6 +55,7 @@ struct ddebug_query {

>  	const char *function;

>  	const char *format;

>  	unsigned int first_lineno, last_lineno;

> +	unsigned int level;

>  };

>  

>  struct ddebug_iter {

> @@ -187,6 +188,18 @@ static int ddebug_change(const struct ddebug_query *query,

>  

>  			nfound++;

>  

> +#ifdef CONFIG_JUMP_LABEL

> +			if (query->level && query->level & dp->level) {

> +				if (flags & _DPRINTK_FLAGS_PRINT)

> +					static_branch_enable(&dp->key.dd_key_true);

> +				else

> +					static_branch_disable(&dp->key.dd_key_true);

> +			} else if (query->level &&

> +				   flags & _DPRINTK_FLAGS_PRINT) {

> +				static_branch_disable(&dp->key.dd_key_true);

> +				continue;

> +			}

> +#endif


This looks like a hack in the existing code:

  + It is suspicious that "continue" is only in one branch. It means
    that static_branch_enable/disable() might get called 2nd time
    by the code below. Or newflags are not stored when there is a change.

  + It changes the behavior and the below vpr_info("changed ...")
    is not called.

Or do I miss anything?

>			newflags = (dp->flags & mask) | flags;

>  			if (newflags == dp->flags)

>  				continue;


Best Regards,
Petr
Randy Dunlap June 9, 2020, 4:03 p.m. UTC | #5
On 6/9/20 4:13 AM, Matthew Wilcox wrote:
> On Tue, Jun 09, 2020 at 01:45:57PM +0300, Stanimir Varbanov wrote:
>> Here is the third version of dynamic debug improvements in Venus
>> driver.  As has been suggested on previous version by Joe [1] I've
>> made the relevant changes in dynamic debug core to handle leveling
>> as more generic way and not open-code/workaround it in the driver.
>>
>> About changes:
>>  - added change in the dynamic_debug and in documentation
>>  - added respective pr_debug_level and dev_dbg_level
> 
> Honestly, this seems like you want to use tracepoints, not dynamic debug.
> 

Also see this patch series:
https://lore.kernel.org/lkml/20200605162645.289174-1-jim.cromie@gmail.com/
[PATCH 00/16] dynamic_debug: cleanups, 2 features

It adds/expands dynamic debug flags quite a bit.
Joe Perches June 9, 2020, 4:49 p.m. UTC | #6
(adding Jim Cromie and comments)

On Tue, 2020-06-09 at 09:03 -0700, Randy Dunlap wrote:
> On 6/9/20 4:13 AM, Matthew Wilcox wrote:

> > On Tue, Jun 09, 2020 at 01:45:57PM +0300, Stanimir Varbanov wrote:

> > > Here is the third version of dynamic debug improvements in Venus

> > > driver.  As has been suggested on previous version by Joe [1] I've

> > > made the relevant changes in dynamic debug core to handle leveling

> > > as more generic way and not open-code/workaround it in the driver.

> > > 

> > > About changes:

> > >  - added change in the dynamic_debug and in documentation

> > >  - added respective pr_debug_level and dev_dbg_level

> > 

> > Honestly, this seems like you want to use tracepoints, not dynamic debug.


Tracepoints are a bit heavy and do not have any class
or grouping mechanism.

debug_class is likely a better name than debug_level

> Also see this patch series:

> https://lore.kernel.org/lkml/20200605162645.289174-1-jim.cromie@gmail.com/

> [PATCH 00/16] dynamic_debug: cleanups, 2 features

> 

> It adds/expands dynamic debug flags quite a bit.


Yes, and thanks Randy and Jim and Stanimir

I haven't gone through Jim's proposal enough yet.
It's unfortunate these patches series conflict.

And for Jim, a link to Stanimir's patch series:
https://lore.kernel.org/lkml/20200609104604.1594-1-stanimir.varbanov@linaro.org/
Joe Perches June 9, 2020, 4:58 p.m. UTC | #7
On Tue, 2020-06-09 at 13:16 +0200, Greg Kroah-Hartman wrote:
> What is wrong with the existing control of dynamic

> debug messages that you want to add another type of arbitrary grouping

> to it? 


There is no existing grouping mechanism.

Many drivers and some subsystems used an internal one
before dynamic debug.

$ git grep "MODULE_PARM.*\bdebug\b"|wc -l
501

This is an attempt to unify those homebrew mechanisms.

Stanimir attempted to add one for his driver via a
driver specific standardized format substring for level.

> And who defines that grouping?


Individual driver authors

> Will it be driver/subsystem/arch/author specific?  Or kernel-wide?


driver specific

> This feels like it could easily get out of hand really quickly.


Likely not.  A question might be how useful all these
old debugging printks are today and if it's reasonable
to just delete them.

> Why not just use tracepoints if you really want to be fine-grained?


Weight and lack of class/group capability
Edward Cree June 9, 2020, 5:42 p.m. UTC | #8
On 09/06/2020 17:58, Joe Perches wrote:
> On Tue, 2020-06-09 at 13:16 +0200, Greg Kroah-Hartman wrote:

>> What is wrong with the existing control of dynamic

>> debug messages that you want to add another type of arbitrary grouping

>> to it? 

> There is no existing grouping mechanism.

>

> Many drivers and some subsystems used an internal one

> before dynamic debug.

>

> $ git grep "MODULE_PARM.*\bdebug\b"|wc -l

> 501

>

> This is an attempt to unify those homebrew mechanisms.

In network drivers, this is probablyusing the existing groupings
 defined by netif_level() - see NETIF_MSG_DRV and friends.  Note
 that those groups are orthogonal to the level, i.e. they control
 netif_err() etc. as well, not just debug messages.
Certainly in the case of sfc, and I'd imagine for many other net
 drivers too, the 'debug' modparam is setting the default for
 net_dev->msg_enable, which can be changed after probe with
 ethtool.
It doesn't look like the proposed mechanism subsumes that (we have
 rather more than 5 groups, and it's not clear how you'd connect
 it to the existing msg_enable (which uapi must be maintained); if
 you don't have a way to do this, better exclude drivers/net/ from
 your grep|wc because you won't be unifying those - in my tree
 that's 119 hits.

-ed
Joe Perches June 9, 2020, 5:56 p.m. UTC | #9
On Tue, 2020-06-09 at 18:42 +0100, Edward Cree wrote:
> On 09/06/2020 17:58, Joe Perches wrote:
> > On Tue, 2020-06-09 at 13:16 +0200, Greg Kroah-Hartman wrote:
> > > What is wrong with the existing control of dynamic
> > > debug messages that you want to add another type of arbitrary grouping
> > > to it? 
> > There is no existing grouping mechanism.
> > 
> > Many drivers and some subsystems used an internal one
> > before dynamic debug.
> > 
> > $ git grep "MODULE_PARM.*\bdebug\b"|wc -l
> > 501
> > 
> > This is an attempt to unify those homebrew mechanisms.
> In network drivers, this is probablyusing the existing groupings
>  defined by netif_level() - see NETIF_MSG_DRV and friends.  Note
>  that those groups are orthogonal to the level, i.e. they control
>  netif_err() etc. as well, not just debug messages.

These are _not_ netif_<level> control flags.  Some are though.

For instance:

$ git grep "MODULE_PARM.*\bdebug\b" drivers/net | head -10
drivers/net/ethernet/3com/3c509.c:MODULE_PARM_DESC(debug, "debug level (0-6)");
drivers/net/ethernet/3com/3c515.c:MODULE_PARM_DESC(debug, "3c515 debug level (0-6)");
drivers/net/ethernet/3com/3c59x.c:MODULE_PARM_DESC(debug, "3c59x debug level (0-6)");
drivers/net/ethernet/adaptec/starfire.c:MODULE_PARM_DESC(debug, "Debug level (0-6)");
drivers/net/ethernet/allwinner/sun4i-emac.c:MODULE_PARM_DESC(debug, "debug message flags");
drivers/net/ethernet/altera/altera_tse_main.c:MODULE_PARM_DESC(debug, "Message Level (-1: default, 0: no output, 16: all)");
drivers/net/ethernet/amazon/ena/ena_netdev.c:MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
drivers/net/ethernet/amd/atarilance.c:MODULE_PARM_DESC(lance_debug, "atarilance debug level (0-3)");
drivers/net/ethernet/amd/lance.c:MODULE_PARM_DESC(lance_debug, "LANCE/PCnet debug level (0-7)");
drivers/net/ethernet/amd/pcnet32.c:MODULE_PARM_DESC(debug, DRV_NAME " debug level");

These are all level/class output controls.

> Certainly in the case of sfc, and I'd imagine for many other net
>  drivers too, the 'debug' modparam is setting the default for
>  net_dev->msg_enable, which can be changed after probe with
>  ethtool.

True.

> It doesn't look like the proposed mechanism subsumes that (we have
>  rather more than 5 groups, and it's not clear how you'd connect
>  it to the existing msg_enable (which uapi must be maintained); if
>  you don't have a way to do this, better exclude drivers/net/ from
>  your grep|wc because you won't be unifying those - in my tree
>  that's 119 hits.

Likely not.

I agree it'd be useful to attach the modparam control flag
to the dynamic debug use somehow.

cheers, Joe
Edward Cree June 9, 2020, 6:08 p.m. UTC | #10
On 09/06/2020 18:56, Joe Perches wrote:
> These are _not_ netif_<level> control flags. Some are though.

> For instance:

>

> $ git grep "MODULE_PARM.*\bdebug\b" drivers/net | head -10

> [...]

>

> These are all level/class output controls.

TIL, thanks!  I should have looked deeperrather than assuming
 they were all like ours.

Though judging just by that grep output, it also looks like
 quite a lot of those won't fit into 5 groups either, so some
 rethink may still be needed...

-ed
Jim Cromie June 9, 2020, 9:21 p.m. UTC | #11
On Tue, Jun 9, 2020 at 10:49 AM Joe Perches <joe@perches.com> wrote:
>
> (adding Jim Cromie and comments)
>

thanks for bringing me in...


> On Tue, 2020-06-09 at 09:03 -0700, Randy Dunlap wrote:
> > On 6/9/20 4:13 AM, Matthew Wilcox wrote:
> > > On Tue, Jun 09, 2020 at 01:45:57PM +0300, Stanimir Varbanov wrote:
> > > > Here is the third version of dynamic debug improvements in Venus
> > > > driver.  As has been suggested on previous version by Joe [1] I've
> > > > made the relevant changes in dynamic debug core to handle leveling
> > > > as more generic way and not open-code/workaround it in the driver.
> > > >
> > > > About changes:
> > > >  - added change in the dynamic_debug and in documentation
> > > >  - added respective pr_debug_level and dev_dbg_level
> > >
> > > Honestly, this seems like you want to use tracepoints, not dynamic debug.
>
> Tracepoints are a bit heavy and do not have any class
> or grouping mechanism.
>
> debug_class is likely a better name than debug_level
>
> > Also see this patch series:
> > https://lore.kernel.org/lkml/20200605162645.289174-1-jim.cromie@gmail.com/
> > [PATCH 00/16] dynamic_debug: cleanups, 2 features
> >
> > It adds/expands dynamic debug flags quite a bit.
>
> Yes, and thanks Randy and Jim and Stanimir
>
> I haven't gone through Jim's proposal enough yet.
> It's unfortunate these patches series conflict.
>
> And for Jim, a link to Stanimir's patch series:
> https://lore.kernel.org/lkml/20200609104604.1594-1-stanimir.varbanov@linaro.org/
>
>


As Joe noted, there is a lot of ad-hockery to possibly clean up,
but I dont grok how these levels should be distinguished from
KERN_(WARN|INFO|DEBUG) constants.
Those constants are used by coders, partly to convey how bad things are
As a user, Id be reluctant to disable an EMERG callsite.

are you trying to add a User Bit ? or maybe 7-9 of them ?

I have a patchset which adds a 'u' flag, for user.
An earlier version had x,y,z flags for 3 different user purposes.
I simplified, since only 1 was needed to mark up arbitrary sets of callsites.
Another patchset feature lets u select on that flag.

 #> echo u+p > control

Joe suggested class, I certainly find level confusing.

Is what you want user-flags u[1-7], or driver-flags d]1-7]   ?
and let me distinguish,
your flags are set in code, not modifiable by user, only for filtering
on flag/bit state ?
so theyd be different than [pfmltu_] flags, which are user changed.

my patchset also adds filtering on flag-state,
so that "echo u+p > control " could work.

if you had
     echo 'module venus 1+p; 2+p; 9+p' > control
how far would you get ?

if it covers your needs, then we could add
numerical flags (aka U1, U9) can be distinguished from  [pfmltu_PFMLTU]
and excluded from the mod-flags changes

from there, it shouldnt be hard to add some macro help

DECLARE_DYNDBG_FLAG ( 1, 'x' )
DECLARE_DYNDBG_FLAG ( 2, 'y' )
DECLARE_DYNDBG_FLAG ( 3, 'z' )

DECLARE_DYNDBG_FLAG_INFO ( 4, 'q', "unquiet a programmer defined debug
callsite set" )

also, since Im here, and this is pretty much on-topic,
I perused https://lkml.org/lkml/2020/5/21/399

I see 3 things;
- s / dev_dbg / VDBGL /
- add a bunch of VDBGM calls
- sys_get_prop_image_version  signature change.   (this doesnt really
belong here)

ISTM most of the selection of dyndbg callsites in that part of venus
could be selected by format.

   echo "module venus format error +p" > control

if so, refining your messages will define the logical sets for you ?


thanks
JimC (one of them anyway)
Joe Perches June 10, 2020, 1:58 a.m. UTC | #12
On Tue, 2020-06-09 at 15:23 -0700, Joe Perches wrote:
> These are just driver developer mechanisms to enable/disable

> groups of formats via some test for < level or | bitmap


duh: & bitmask

> 	if (is_bitmask)

> 		enable/disable(value|flag)


obviously
		enable/disable(value & flag)
Jim Cromie June 10, 2020, 3:10 a.m. UTC | #13
On Tue, Jun 9, 2020 at 4:23 PM Joe Perches <joe@perches.com> wrote:
>

> On Tue, 2020-06-09 at 15:21 -0600, jim.cromie@gmail.com wrote:

> >

> > As Joe noted, there is a lot of ad-hockery to possibly clean up,

> > but I dont grok how these levels should be distinguished from

> > KERN_(WARN|INFO|DEBUG) constants.

>

> These are not KERN_<LEVEL> at all, all are emitted at KERN_DEBUG


yes indeed.  but they are chosen by programmer, fixed by compiler.  not dynamic.
<pmladek@suse.com> also noted the conceptual adjacency (ambiguity),
and referenced KERN_<lvl>



If we need this extra query-term, lets call it   mbits / mflags /
module_flags / module_bits
it needs to be module specific, so also requiring "module foo" search
term in the query.
( "modflags" is no good, cuz "mod" also means "modified" - just mflags
is better )

Already, we have function, file, module, all of which convey semantic
structure of the code,
and they also match wildcards, so " function foo_*_* " is an effective grouping.
Id think this would cover most cases.

Finally, all "module venus +p " callsites could be explicitly
specified individually in
universe=`grep venus control | wc -l`
lines, likely a small set.
Using the semantic structure exposed by `grep venus control`, it would
likely be far less.
Greg KH June 10, 2020, 6:31 a.m. UTC | #14
On Tue, Jun 09, 2020 at 09:58:07AM -0700, Joe Perches wrote:
> On Tue, 2020-06-09 at 13:16 +0200, Greg Kroah-Hartman wrote:

> > What is wrong with the existing control of dynamic

> > debug messages that you want to add another type of arbitrary grouping

> > to it? 

> 

> There is no existing grouping mechanism.


info/warn/err/dbg is what I am referring to.

> Many drivers and some subsystems used an internal one

> before dynamic debug.

> 

> $ git grep "MODULE_PARM.*\bdebug\b"|wc -l

> 501


Yes, and it's horrid and needs to be cleaned up, not added to.

In the beginning, yes, adding loads of different types of debugging
options to a driver is needed by the author, but by the time it is added
to the kernel, all of that should be able to be removed and only a
single "enable debug" should be all that is needed.

We do not need each individual driver thinking it needs to have some
sort of special classification of each type of debug message.  Just use
the framework that we have, you can enable/disable them on a
line-by-line basis as needed.

> This is an attempt to unify those homebrew mechanisms.


All of those should just be removed.

> Stanimir attempted to add one for his driver via a

> driver specific standardized format substring for level.

> 

> > And who defines that grouping?

> 

> Individual driver authors


That way lies madness, let's try to fix all of that up.

greg k-h
Greg KH June 10, 2020, 7:09 a.m. UTC | #15
On Tue, Jun 09, 2020 at 11:35:31PM -0700, Joe Perches wrote:
> On Wed, 2020-06-10 at 08:31 +0200, Greg Kroah-Hartman wrote:
> > On Tue, Jun 09, 2020 at 09:58:07AM -0700, Joe Perches wrote:
> > > On Tue, 2020-06-09 at 13:16 +0200, Greg Kroah-Hartman wrote:
> > > > What is wrong with the existing control of dynamic
> > > > debug messages that you want to add another type of arbitrary grouping
> > > > to it? 
> > > 
> > > There is no existing grouping mechanism.
> > 
> > info/warn/err/dbg is what I am referring to.
> > 
> > > Many drivers and some subsystems used an internal one
> > > before dynamic debug.
> > > 
> > > $ git grep "MODULE_PARM.*\bdebug\b"|wc -l
> > > 501
> > 
> > Yes, and it's horrid and needs to be cleaned up, not added to.
> 
> Or unified so driver authors have a standardized mechanism
> rather than reinventing or doing things differently.

But each "level" you all come up with will be intrepreted differently
per driver, causing total confusion (like we have today.)  Try to make
it better by just removing that mess.

> > In the beginning, yes, adding loads of different types of debugging
> > options to a driver is needed by the author, but by the time it is added
> > to the kernel, all of that should be able to be removed and only a
> > single "enable debug" should be all that is needed.
> 
> No one does that.

We did that for USB drivers a decade ago, it can be done.

greg k-h
Stanimir Varbanov June 10, 2020, 10:29 a.m. UTC | #16
Hi Greg,

On 6/9/20 2:16 PM, Greg Kroah-Hartman wrote:
> On Tue, Jun 09, 2020 at 01:45:58PM +0300, Stanimir Varbanov wrote:
>> This adds description of the level bitmask feature.
>>
>> Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>  Documentation/admin-guide/dynamic-debug-howto.rst | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
>> index 0dc2eb8e44e5..c2b751fc8a17 100644
>> --- a/Documentation/admin-guide/dynamic-debug-howto.rst
>> +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
>> @@ -208,6 +208,12 @@ line
>>  	line -1605          // the 1605 lines from line 1 to line 1605
>>  	line 1600-          // all lines from line 1600 to the end of the file
>>  
>> +level
>> +    The given level will be a bitmask ANDed with the level of the each ``pr_debug()``
>> +    callsite. This will allow to group debug messages and show only those of the
>> +    same level.  The -p flag takes precedence over the given level. Note that we can
>> +    have up to five groups of debug messages.
> 
> As was pointed out, this isn't a "level", it's some arbitrary type of
> "grouping".

Yes, it is grouping of KERN_DEBUG level messages by importance (my
fault, I put incorrect name).  What is important is driver author
decision.  Usually when the driver is huge and has a lot of debug
messages it is not practical to enable all of them to chasing a
particular bug or issue.  You know that debugging (printk) add delays
which could hide or rise additional issue(s) which would complicate
debug and waste time.

For the Venus driver I have defined three groups of KERN_DEBUG - low,
medium and high (again the driver author(s) will decide what the
importance is depending on his past experience).

There is another point where the debugging is made by person who is not
familiar with the driver code. In that case he/she cannot enable lines
or range of lines because he don't know the details. Here the grouping
by importance could help.

> 
> But step back, why?  What is wrong with the existing control of dynamic
> debug messages that you want to add another type of arbitrary grouping
> to it?  And who defines that grouping?  Will it be
> driver/subsystem/arch/author specific?  Or kernel-wide?
> 
> This feels like it could easily get out of hand really quickly.
> 
> Why not just use tracepoints if you really want to be fine-grained?
> 
> thanks,
> 
> greg k-h
>
Greg KH June 10, 2020, 12:26 p.m. UTC | #17
On Wed, Jun 10, 2020 at 01:29:20PM +0300, Stanimir Varbanov wrote:
> Hi Greg,

> 

> On 6/9/20 2:16 PM, Greg Kroah-Hartman wrote:

> > On Tue, Jun 09, 2020 at 01:45:58PM +0300, Stanimir Varbanov wrote:

> >> This adds description of the level bitmask feature.

> >>

> >> Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)

> >>

> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

> >> ---

> >>  Documentation/admin-guide/dynamic-debug-howto.rst | 10 ++++++++++

> >>  1 file changed, 10 insertions(+)

> >>

> >> diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst

> >> index 0dc2eb8e44e5..c2b751fc8a17 100644

> >> --- a/Documentation/admin-guide/dynamic-debug-howto.rst

> >> +++ b/Documentation/admin-guide/dynamic-debug-howto.rst

> >> @@ -208,6 +208,12 @@ line

> >>  	line -1605          // the 1605 lines from line 1 to line 1605

> >>  	line 1600-          // all lines from line 1600 to the end of the file

> >>  

> >> +level

> >> +    The given level will be a bitmask ANDed with the level of the each ``pr_debug()``

> >> +    callsite. This will allow to group debug messages and show only those of the

> >> +    same level.  The -p flag takes precedence over the given level. Note that we can

> >> +    have up to five groups of debug messages.

> > 

> > As was pointed out, this isn't a "level", it's some arbitrary type of

> > "grouping".

> 

> Yes, it is grouping of KERN_DEBUG level messages by importance (my

> fault, I put incorrect name).  What is important is driver author

> decision.  Usually when the driver is huge and has a lot of debug

> messages it is not practical to enable all of them to chasing a

> particular bug or issue.  You know that debugging (printk) add delays

> which could hide or rise additional issue(s) which would complicate

> debug and waste time.


That is why it is possible to turn on and off debugging messages on a
function/line basis already.  Why not just use that instead?

> For the Venus driver I have defined three groups of KERN_DEBUG - low,

> medium and high (again the driver author(s) will decide what the

> importance is depending on his past experience).

> 

> There is another point where the debugging is made by person who is not

> familiar with the driver code. In that case he/she cannot enable lines

> or range of lines because he don't know the details. Here the grouping

> by importance could help.


And they will really know what "low/medium/high" are?

Anyway, that makes a bit more sense, but the documentation could use a
lot more in order to describe this type of behavior, and what is
expected by both driver authors, and users of the interface.

thanks,

greg k-h
Stanimir Varbanov June 11, 2020, 11:51 a.m. UTC | #18
Hi Greg,

Thanks for the comments!

On 6/9/20 2:12 PM, Greg Kroah-Hartman wrote:
> On Tue, Jun 09, 2020 at 01:46:02PM +0300, Stanimir Varbanov wrote:

>> +int venus_dbgfs_init(struct venus_core *core)

>> +{

>> +	core->root = debugfs_create_dir("venus", NULL);

>> +	if (IS_ERR(core->root))

>> +		return IS_ERR(core->root);

> 

> You really do not care, and obviously did not test this on a system with

> CONFIG_DEBUG_FS disabled :)


Probably not :(

> 

> Just make the call to debugfs, and move on, feed it into other debugfs

> calls, all is good.

> 

> This function should just return 'void', no need to care about this at

> all.

> 

>> +	ret = venus_sys_set_debug(hdev, venus_fw_debug);

>> +	if (ret)

>> +		dev_warn(dev, "setting fw debug msg ON failed (%d)\n", ret);

> 

> Why do you care about this "error"?


I don't care so much, that's why it is dev_warn. But if I enable debug
messages from the firmware and I don't see them this warn will give me
an idea why.


-- 
regards,
Stan