On Fri, Mar 11, 2022 at 12:06 PM Jason Baron <jbaron@xxxxxxxxxx> wrote:
>
>
>
> On 3/10/22 23:47, Jim Cromie wrote:
> > DRM defines/uses 10 enum drm_debug_category's to create exclusive
> > classes of debug messages. To support this directly in dynamic-debug,
> > add the following:
> >
> > - struct _ddebug.class_id:4 - 4 bits is enough
> > - define _DPRINTK_SITE_UNCLASSED 15 - see below
> >
> > and the query support:
> > - struct _ddebug_query.class_id
> > - ddebug_parse_query - looks for "class" keyword, then calls..
> > - parse_class - accepts uint: 0-15, sets query.class_id and marker
> > - vpr_info_dq - displays new field
> > - ddebug_proc_show - append column with "cls:%d" if !UNCLASSED
> >
> > With the patch, one can enable current/unclassed callsites by:
> >
> > #> echo drm class 15 +p > /proc/dynamic_debug/control
> >
>
> To me, this is hard to read, what the heck is '15'? I have to go look it
> up in the control file and it's not descriptive. I think that using
> classes/categories makes sense but I'm wondering if it can be a bit more
> user friendly? Perhaps, we can pass an array of strings that is indexed
> by the class id to each pr_debug() site that wants to use class. So
> something like:
>
Im not at all averse to nice names, but as something added on.
1st, the interface to make friendlier is DRM's
echo 0x04 > /sys/module/drm/parameters/debug # which category ?
parm: debug:Enable debug output, where each bit enables a
debug category.
Bit 0 (0x01) will enable CORE messages (drm core code)
Bit 1 (0x02) will enable DRIVER messages (drm controller code)
Bit 2 (0x04) will enable KMS messages (modesetting code)
echo DRM_UT_DRIVER,DRM_UT_KMS > /sys/module/drm/parameters/debug #
now its pretty clear
If that works, its cuz DEFINE_DYNAMIC_DEBUG_CLASSBITS()
plucks class symbols out of its __VA_ARGS__, and #stringifes them.
So that macro could then build the 1-per-module static constant string array
and (only) the callbacks would be able to use it.
from there, it shouldnt be hard to ref that from the module's ddebug_table,
so parse_query could validate class args against the module given (or
fail if none given)
Speaking strictly, theres a gap:
echo module * class DRM_UT_KMS +p > control
is nonsense for * other than drm + drivers,
but its fair/safe to just disallow wildcards on modname for this purpose.
it does however imply that module foo must exist for FOO_CAT_1 to be usable.
thats not currently the case:
bash-5.1# echo module foo +p > /proc/dynamic_debug/control
[ 15.403749] dyndbg: read 14 bytes from userspace
[ 15.405413] dyndbg: query 0: "module foo +p" mod:*
[ 15.406486] dyndbg: split into words: "module" "foo" "+p"
[ 15.407070] dyndbg: op='+'
[ 15.407388] dyndbg: flags=0x1
[ 15.407809] dyndbg: *flagsp=0x1 *maskp=0xffffffff
[ 15.408300] dyndbg: parsed: func="" file="" module="foo" format=""
lineno=0-0 class=15
[ 15.409151] dyndbg: no matches for query
[ 15.409591] dyndbg: no-match: func="" file="" module="foo"
format="" lineno=0-0 class=15
[ 15.410524] dyndbg: processed 1 queries, with 0 matches, 0 errs
bash-5.1#
ISTM we can keep that "0 errs" response for that case, but still reject this:
echo module foo class FOO_NOT_HERE +p > /proc/dynamic_debug/control
> enum levels {
> LOW,
> MEDIUM,
> HIGH
> };
I want to steer clear of "level" anything,
since 2>1 implies non independence of the categories
>
> static const char * const level_to_strings[] = {
> [LOW] = "low",
> [MEDIUM] = "medium",
> [HIGH] = "high",
> };
>
> And then you'd have a wrapper macros in your driver:
>
> #define module_foo_pr_debug_class(level, fmt, args...)
> pr_debug_class(level, level_to_strings, fmt, args);
>
> Such that call sites look like:
>
> module_foo_pr_debug_class(LOW, fmt, args...);
>
That macro, minus the "module_foo_" prefix,
could go into dynamic_debug.h
I didnt do that, for 2 reasons:
DRM didnt need it - it had an enum var,
and a set of macros to encapsulate the categories.
- the "prototype" looks like this might be ok:
define LOW "low"
pr_debug_class(LOW, "mumble about something %p %p\n", foo, bar)
ok thats a stretch, but...
Basically, I didnt want to deal with creating a new interface. KIS
> Such that you're not always passing the strings array around. Now, this
> does mean another pointer for struct _ddebug and most wouldn't have it.
> Maybe we could just add another linker section for these so as to save
> space.
