On Fri, Dec 12, 2025 at 10:26 AM Jason Baron <[email protected]> wrote: > > > > On 11/18/25 3:18 PM, Jim Cromie wrote: > > !-------------------------------------------------------------------| > > This Message Is From an External Sender > > This message came from outside your organization. > > |-------------------------------------------------------------------! > > > > classmap-v1 code protected class'd pr_debugs from unintended > > changes by unclassed/_DFLT queries: > > > > # - to declutter examples: > > alias ddcmd='echo $* > /proc/dynamic_debug/control' > > > > # IOW, this should NOT alter drm.debug settings > > ddcmd -p > > > > # Instead, you must name the class to change it. > > # Protective but tedious > > ddcmd class DRM_UT_CORE +p > > > > # Or do it the (old school) subsystem way > > # This is ABI !! > > echo 1 > /sys/module/drm/parameters/debug > > > > Since the debug sysfs-node is ABI, if dyndbg is going to implement it, > > it must also honor its settings; it must at least protect against > > accidental changes to its classes from legacy queries. > > > > The protection allows all previously conceived queries to work the way > > they always have; ie select the same set of pr_debugs, despite the > > inclusion of whole new classes of pr_debugs. > > > > But that choice has 2 downsides: > > > > 1. "name the class to change it" makes a tedious long-winded > > interface, needing many commands to set DRM_UT_* one at a time. > > > > 2. It makes the class keyword special in some sense; the other > > keywords skip only on query mismatch, otherwise the code falls thru to > > adjust the pr-debug site. > > > > Jason Baron didn't like v1 on point 2. > > Louis Chauvet didn't like recent rev on point 1 tedium. > > > > But that said: /sys/ is ABI, so this must be reliable: > > > > #> echo 0x1f > /sys/module/drm/parameters/debug > > > > It 'just works' without dyndbg underneath; we must deliver that same > > stability. Convenience is secondary. > > > > The new resolution: > > > > If ABI is the blocking issue, then no ABI means no blocking issue. > > IOW, if the classmap has no presence under /sys/*, ie no PARAM, there > > is no ABI to guard, and no reason to enforce a tedious interface. > > > > In the future, if DRM wants to alter this protection, that is > > practical, but I think default-on is the correct mode. > > > > So atm classes without a PARAM are unprotected at >control, allowing > > admins their shortcuts. I think this could satisfy all viewpoints. > > > > That said, theres also a possibility of wildcard classes: > > > > #> ddcmd class '*' +p > > > > Currently, the query-class is exact-matched against each module's > > classmaps.names. This gives precise behavior, a good basis. > > > > But class wildcards are possible, they just did'nt appear useful for > > DRM, whose classmap names are a flat DRM_UT_* namespace. > > > > IOW, theres no useful selectivity there: > > > > #> ddcmd class "DRM_*" +p # these enable every DRM_* class > > #> ddcmd class "DRM_UT_*" +p > > > > #> ddcmd class "DRM_UT_V*" +p # finally select just 1: DRM_UT_VBL > > #> ddcmd class "DRM_UT_D*" +p # but this gets 3 > > > > #> ddcmd class "D*V*" +p # here be dragons > > > > But there is debatable utility in the feature. > > > > #> ddcmd class __DEFAULT__ -p # what about this ? > > #> ddcmd -p # thats what this does. > > automatically > > > > Anyway, this patch does: > > > > 1. adds link field from _ddebug_class_map to the .controlling_param > > > > 2. sets it in ddebug_match_apply_kparam(), during modprobe/init, > > when options like drm.debug=VAL are handled. > > > > 3. ddebug_class_has_param() now checks .controlling_param > > > > 4. ddebug_class_wants_protection() macro renames 3. > > this frames it as a separable policy decision > > > > 5. ddebug_match_desc() gets the most attention: > > > > a. move classmap consideration to the bottom > > this insures all other constraints act 1st. > > allows simpler 'final' decisions. > > > > b. split class choices cleanly on query: > > class FOO vs none, and class'd vs _DPRINTK_CLASS_DFLT site. > > > > c. calls 4 when applying a class-less query to a class'd pr_debug > > here we need a new fn to find the classmap with this .class_id > > > > d. calls new ddebug_find_classmap_by_class_id(). > > when class-less query looks at a class'd pr_debug. > > finds classmap, which can then decide, currently by PARAM existence. > > > > NOTES: > > > > protection is only against class-less queries, explicit "class FOO" > > adjustments are allowed (that is the mechanism). > > > > The drm.debug sysfs-node heavily under-specifies the class'd pr_debugs > > it controls; none of the +mfls prefixing flags have any effect, and > > each callsite remains individually controllable. drm.debug just > > toggles the +p flag for all the modules' class'd pr_debugs. > > > > Signed-off-by: Jim Cromie <[email protected]> > > --- > > history > > -v0 - original, before classmaps: no special case keywords > > -v1 - "class DEFAULT" is assumed if not mentioned. > > this protects classes from class-less queries > > > > -v2.pre-this-patch - protection macro'd to false > > -v2.with-this-patch - sysfs knob decides > > -v2.speculative - module decides wrt classmap protection > > seems unneeded now, TBD > > > > v3 - new patch > > v4 > > - drop fn-scope map var, with 2 local vars, renamed to purpose > > - fix for NULL ptr case. > > - Add loop-var to reduce many "&dt->info." exprs to "di->" > > - add 1-liner postcondition comments > > > > fixus > > --- > > include/linux/dynamic_debug.h | 14 ++-- > > lib/dynamic_debug.c | 127 +++++++++++++++++++++++++++------- > > 2 files changed, 110 insertions(+), 31 deletions(-) > > > > diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h > > index b1d11d946780..b22da40e2583 100644 > > --- a/include/linux/dynamic_debug.h > > +++ b/include/linux/dynamic_debug.h > > @@ -75,6 +75,7 @@ enum ddebug_class_map_type { > > * map @class_names 0..N to consecutive constants starting at @base. > > */ > > struct _ddebug_class_map { > > + struct _ddebug_class_param *controlling_param; > > const struct module *mod; /* NULL for builtins */ > > const char *mod_name; > > const char **class_names; > > @@ -259,7 +260,12 @@ struct _ddebug_class_param { > > * > > * Creates a sysfs-param to control the classes defined by the > > * exported classmap, with bits 0..N-1 mapped to the classes named. > > - * This version keeps class-state in a private long int. > > + * > > + * Since sysfs-params are ABI, this also protects the classmap'd > > + * pr_debugs from un-class'd `echo -p > /proc/dynamic_debug/control` > > + * changes. > > + * > > + * This keeps class-state in a private long int. > > */ > > #define DYNAMIC_DEBUG_CLASSMAP_PARAM(_name, _var, _flags) \ > > static unsigned long _name##_bvec; \ > > @@ -272,10 +278,8 @@ struct _ddebug_class_param { > > * @_var: name of the (exported) classmap var defining the classes/bits > > * @_flags: flags to be toggled, typically just 'p' > > * > > - * Creates a sysfs-param to control the classes defined by the > > - * exported clasmap, with bits 0..N-1 mapped to the classes named. > > - * This version keeps class-state in user @_bits. This lets drm check > > - * __drm_debug elsewhere too. > > + * Like DYNAMIC_DEBUG_CLASSMAP_PARAM, but maintains param-state in > > + * extern @_bits. This lets DRM check __drm_debug elsewhere too. > > */ > > #define DYNAMIC_DEBUG_CLASSMAP_PARAM_REF(_name, _bits, _var, _flags) > > \ > > __DYNAMIC_DEBUG_CLASSMAP_PARAM(_name, _bits, _var, _flags) > > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c > > index 636a6b5741f7..1082e0273f0e 100644 > > --- a/lib/dynamic_debug.c > > +++ b/lib/dynamic_debug.c > > @@ -206,6 +206,50 @@ ddebug_find_valid_class(struct _ddebug_info const *di, > > const char *query_class, > > return NULL; > > } > > > > +static bool ddebug_class_in_range(const int class_id, const struct > > _ddebug_class_map *map) > > +{ > > + return (class_id >= map->base && > > + class_id < map->base + map->length); > > +} > > + > > +static struct _ddebug_class_map * > > +ddebug_find_map_by_class_id(struct _ddebug_info *di, int class_id) > > +{ > > + struct _ddebug_class_map *map; > > + struct _ddebug_class_user *cli; > > + int i; > > + > > + for_subvec(i, map, di, maps) > > + if (ddebug_class_in_range(class_id, map)) > > + return map; > > + > > + for_subvec(i, cli, di, users) > > + if (ddebug_class_in_range(class_id, cli->map)) > > + return cli->map; > > + > > + return NULL; > > +} > > + > > +/* > > + * classmaps-V1 protected classes from changes by legacy commands > > + * (those selecting _DPRINTK_CLASS_DFLT by omission). This had the > > + * downside that saying "class FOO" for every change can get tedious. > > + * > > + * V2 is smarter, it protects class-maps if the defining module also > > + * calls DYNAMIC_DEBUG_CLASSMAP_PARAM to create a sysfs parameter. > > + * Since the author wants the knob, we should assume they intend to > > + * use it (in preference to "class FOO +p" >control), and want to > > + * trust its settings. This gives protection when its useful, and not > > + * when its just tedious. > > + */ > > +static inline bool ddebug_class_has_param(const struct _ddebug_class_map > > *map) > > +{ > > + return !!(map->controlling_param); > > +} > > + > > +/* re-framed as a policy choice */ > > +#define ddebug_class_wants_protection(map) (ddebug_class_has_param(map)) > > + > > /* > > * Search the tables for _ddebug's which match the given `query' and > > * apply the `flags' and `mask' to them. Returns number of matching > > @@ -214,11 +258,10 @@ ddebug_find_valid_class(struct _ddebug_info const > > *di, const char *query_class, > > */ > > static bool ddebug_match_desc(const struct ddebug_query *query, > > struct _ddebug *dp, > > - int valid_class) > > + struct _ddebug_info *di, > > + int selected_class) > > { > > - /* match site against query-class */ > > - if (dp->class_id != valid_class) > > - return false; > > + struct _ddebug_class_map *site_map; > > > > /* match against the source filename */ > > if (query->filename && > > @@ -255,7 +298,28 @@ static bool ddebug_match_desc(const struct > > ddebug_query *query, > > dp->lineno > query->last_lineno) > > return false; > > > > - return true; > > + /* > > + * above are all satisfied, so we can make final decisions: > > + * 1- class FOO or implied class __DEFAULT__ > > + * 2- site.is_classed or not > > + */ > > + if (query->class_string) { > > + /* class FOO given, exact match required */ > > + return (dp->class_id == selected_class); > > + } > > + /* query class __DEFAULT__ by omission. */ > > + if (dp->class_id == _DPRINTK_CLASS_DFLT) { > > + /* un-classed site */ > > + return true; > > + } > > + /* site is class'd */ > > + site_map = ddebug_find_map_by_class_id(di, dp->class_id); > > + if (!site_map) { > > + /* _UNKNOWN_ class_id. XXX: Allow changes here ? */ > > + return false; > > + } > > Do we want a WARN_ON_ONCE() here? I think this is the case where we have > class_id for the call site but it's not default, so shouldn't it always > have a map or be a user of the map? >
Yes, I think so. I will add it. > Thanks, > > -Jason >
