Le 20/03/2025 à 19:52, Jim Cromie a écrit :
Current classmap code protects class'd pr_debugs from unintended
changes by "legacy" unclassed queries:
# this doesn't disable all of DRM_UT_* categories
echo "-p" > /proc/dynamic_debug/control
# name the class to change it - protective but tedious
echo "class DRM_UT_CORE +p" > /proc/dynamic_debug/control
# or do it the subsystem way
echo 1 > /sys/module/drm/parameters/debug
This "name the class to change it" behavior gave a modicum of
protection to classmap users (ie DRM) so their debug settings aren't
trivially and unintentionally altered underneath them.
But this made the class keyword special in some sense; the other
keywords skip only on explicit mismatch, otherwize the code falls thru
s/otherwize/otherwise/
to adjust the pr-debug site.
So Jason Baron didn't like this special case when I 1st proposed it;
I argued 2 points:
- "protection gives stable-debug, improving utility"
- __drm_debug is authoritative w/o dyndbg under it.
I thought I'd convinced him back then, (and the patchset got merged),
but he noted it again when he reviewed this series. So this commit
names the "special case": ddebug_client_module_protects_classes(), and
reverts it to Jason's preference.
>
If a class mismatch is seen, code distinguishes whether the class was
explicitly given (and always skips/continue), or the DFLT was assumed
because no class was given. Here we test
ddebug_client_module_protects_classes(), skip if so.
Later, if any user/module wants to protect its classes, we could add a
flag to ddebug_table, a means to set it from CLASSMAP_DEFINE, and
check it when applying a classless query/cmd.
I don't really understand the goal of the protection, do you have the
discussion between you and Jason so I can have some context and some
answer to my questions?
With the example you gave above, I think this could lead to a very odd
behavior: if I enable dyndbg, I expect any pr_dbg to be managed by
dyndbg settings.
If a user writes stuff on dyndbg control, he clearly knows what he is
doing, and he wants to control what logs he wants.
And if you allow multiple "protected" users, the normal way to disable
all dyndbg logs will be:
ddcmd -p
ddcmd class DRM_UT_CORE -p
ddcmd class DRM_... -p # all drm classes
ddcmd class SPI_... -p # all spi classes
ddcmd class WHATEVER_... -p # all other subsystem
# And only now you can enable only what you want
ddcmd module my_mod +p
This is clearly annoying to write.
If DRM (or whatever subsystem) wants to add a debug parameter and use it
to control their logs without being impacted by dyndbg, I believe it
should not use dyndbg classes to do it.
CC: jba...@akamai.com
Signed-off-by: Jim Cromie <jim.cro...@gmail.com>
---
lib/dynamic_debug.c | 34 +++++++++++++++++++++++++---------
1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c44502787c2b..13de0dd3a4ad 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -193,6 +193,17 @@ static int ddebug_find_valid_class(struct ddebug_table
const *dt, const char *cl
return -ENOENT;
}
+/*
+ * classmaps-v1 protected classes from changes by legacy commands
+ * (those selecting _DPRINTK_CLASS_DFLT by omission), v2 undoes that
+ * special treatment. State so explicitly. Later we could give
+ * modules the choice to protect their classes or to keep v2 behavior.
+ */
+static inline bool ddebug_client_module_protects_classes(const struct
ddebug_table *dt)
+{
+ return false;
+}
+
/*
* Search the tables for _ddebug's which match the given `query' and
* apply the `flags' and `mask' to them. Returns number of matching
@@ -206,7 +217,7 @@ static int ddebug_change(const struct ddebug_query *query,
struct flag_settings
unsigned int newflags;
unsigned int nfound = 0;
struct flagsbuf fbuf, nbuf;
- int valid_class;
+ int slctd_class;
Nitpick: can you use full words? slctd is difficult to read.
/* search for matching ddebugs */
mutex_lock(&ddebug_lock);
@@ -218,21 +229,26 @@ static int ddebug_change(const struct ddebug_query
*query, struct flag_settings
continue;
if (query->class_string) {
- valid_class = ddebug_find_valid_class(dt,
query->class_string);
- if (valid_class < 0)
+ slctd_class = ddebug_find_valid_class(dt,
query->class_string);
+ if (slctd_class < 0)
+ /* skip/reject classes unknown by module */
continue;
} else {
- /* constrain query, do not touch class'd callsites */
- valid_class = _DPRINTK_CLASS_DFLT;
+ slctd_class = _DPRINTK_CLASS_DFLT;
}
for (i = 0; i < dt->info.descs.len; i++) {
struct _ddebug *dp = &dt->info.descs.start[i];
- /* match site against query-class */
- if (dp->class_id != valid_class)
- continue;
-
+ if (dp->class_id != slctd_class) {
+ if (query->class_string)
+ /* site.class != given class */
+ continue;
+ /* legacy query, class'd site */
+ else if
(ddebug_client_module_protects_classes(dt))
+ continue;
+ /* allow change on class'd pr_debug */
+ }
/* match against the source filename */
if (query->filename &&
!match_wildcard(query->filename, dp->filename) &&
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
