On Mon, Mar 14, 2022 at 3:30 PM Jason Baron <jbaron@xxxxxxxxxx> wrote: > > > > On 3/11/22 20:06, jim.cromie@xxxxxxxxx wrote: > > On Fri, Mar 11, 2022 at 12:06 PM Jason Baron <jbaron@xxxxxxxxxx> wrote: > >> > >> > >> > >> On 3/10/22 23:47, Jim Cromie wrote: > >>> > >>> 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: hi Jason, Im now in basically full agreement with you 1. .class_id is a "global" space, in that all callsites have one. 2. 0-15 is an exceedingly small range for a global space Fix that by 3. make it private (by removing "class N" parsing) now nobody can do echo module * class N +p >control 4. add private/per-module "string" -> class_id map each module will have to declare the class-strings they use/accept opt-in - want coordinated / shared names for DRM_UT_KMS etc. 5. validate input using the known class_string -> class_id then, this is knowably right or wrong, depending on DRM_FOO: echo module drm class DRM_FOO +p > control it also means that echo class DRM_UT_KMS +p >control is both wellformed and minimal; any module that has DRM_UT_KMS defined will know which class_id *they* have mapped it to. theres no global "DRM_UT_KMS" to be abused. So Ive been working towards that.. Heres my current biggest roadblock DEFINE_DYNAMIC_DEBUG_CLASSBITS creates the class-strings[] array declaratively, at compile-time. This array is attached to the kernel-param.args, so it can be used by the callbacks (to construct the command) But its not obviously available from outside the sysfs knob that its attached to, as is needed to apply command >control generally. If I can attach the class-strings[] to struct ddebug_table, then ddebug_change() can use it to validate >control input. So, how to attach ? its got to work for both builtin & loadable modules. (which precludes something in struct module ?) I looked for a kernel_param_register callback, came up empty. Trying to add it feels invasive / imposing. > > > > 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. > > So Ive been tinkering hard on this macro, since its pretty central to the interface defn. heres some choices: this is what Ive been working towards. using enum symbols directly like this associates them by grep, in contrast, class-strings are mealy-mouthed, milquetoast. DEFINE_DYNAMIC_DEBUG_CLASSBITS(debug, __drm_debug, "p", "enable drm.debug categories - 1 bit per category", DRM_UT_CORE, DRM_UT_DRIVER, DRM_UT_KMS, DRM_UT_PRIME, DRM_UT_ATOMIC, DRM_UT_VBL, DRM_UT_STATE, DRM_UT_LEASE, DRM_UT_DP, DRM_UT_DRMRES); I found a slick MAP ( ) macro to do this: #define DEFINE_DYNAMIC_DEBUG_CLASSBITS(fsname, _var, _flgs, desc, ...) \ MODULE_PARM_DESC(fsname, desc); \ static struct dyndbg_classbits_param ddcats_##_var = { \ .bits = &(_var), \ .flags = _flgs, \ .classes = { __VA_ARGS__, _DPRINTK_CLASS_DFLT }, \ .class_names = { mgMAP(__stringify, sCOMMA, \ __VA_ARGS__, _DPRINTK_CLASS_DFLT) } \ }; \ module_param_cb(fsname, ¶m_ops_dyndbg_classbits, \ &ddcats_##_var, 0644) __VA_ARGS__ is used 2x .class_names is available for validating command >control As much as I like the above, the MAP macro has a longer, more risky path to the kernel so a more modest alternative: module user defines class-strings in interface, but they *must* align manually with the enum values they correspond to; the order determines the bit-pos in the sysfs node, since the interface no longer provides the enum values themselves. DEFINE_DYNAMIC_DEBUG_CLASS_STRINGS(debug, __drm_debug, "p", "enable drm.debug categories - 1 bit per category", "DRM_UT_CORE", "DRM_UT_DRIVER", "DRM_UT_KMS", different name allows CLASSBITs or similar in future, if MAP works out. class-strings are completely defined by users, drm can drop UT naming TLDR: FWIW iSTM that the same macro will support the coordinated use of class-strings across multiple modules. drm_print.c - natural home for exposed sysfs node amdgpu/, drm_helper/ i915/ nouveau/ will all need a DEFINE_DD added, so that ddebug_change() can allow those .class_ids to be controlled. sysfs perm inits can disable their nodes, since theyre coordinated. > > > > Ok, yeah so I guess I'm thinking about the 'class' more as global namespace, > so that for example, it could span modules, or be specific to certain modules. > I'm also thinking of it as a 'string' which is maybe hierarchical, so that it's > clear what sub-system, or sub-sub-system it belongs to. So for DRM for example, > it could be a string like "DRM:CORE". The index num I think is still helpful for > implementation so we don't have to store a pointer size, but I don't think it's > really exposed (except perhaps in module params b/c drm is doing that already?). > So what Ive got here is as described above, I just need a few bright ideas, then I can bring it together. got a spare tuit? Jim