| Message ID | 20200609104604.1594-1-stanimir.varbanov@linaro.org |
|---|---|
| Headers | show |
| Series | Venus dynamic debug | expand |
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
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.
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
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
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.
(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/
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
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
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
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
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)
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)
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.
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
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
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 >
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
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