Hi everyone,
DRM_USE_DYNAMIC_DEBUG=y has a regression on rc-*
Regression is due to a chicken-egg problem loading modules; on
`modprobe i915`, drm is loaded 1st, and drm.debug is set. When
drm_debug_enabled() tested __drm_debug at runtime, that just worked.
But with DRM_USE_DYNAMIC_DEBUG=y, the runtime test is replaced with a
post-load enablement of drm_dbg/dyndbg callsites (static-keys), via
dyndbg's callback on __drm_debug. Since all drm-drivers need drm.ko,
it is loaded 1st, then drm.debug=X is applied, then drivers load, but
too late for drm_dbgs to be enabled.
STATUS
For all-loadable drm,i915,amdgpu configs, it almost works, but
propagating drm.debug to dependent modules doesnt actually apply,
though the motions are there. This is not the problem I want to chase
here.
The more basic trouble is:
For builtin drm + helpers, things are broken pretty early; at the
beginning of dynamic_debug_init(). As the ddebug_sanity() commit-msg
describes in some detail, the records added by _USE fail to reference
the struct ddebug_class_map created and exported by _DEFINE, but get
separate addresses to "other" data that segv's when used as the
expected pointer. FWIW, the pointer val starts with "revi".
OVERVIEW
DECLARE_DYNDBG_CLASSMAP is broken: it is one-size-fits-all-poorly.
It muddles the distinction between a (single) definition, and multiple
references. Something exported should suffice.
The core of this patchset splits it into:
DYNDBG_CLASSMAP_DEFINE used once per subsystem to define each classmap
DYNDBG_CLASSMAP_USE declare dependence on a DEFINEd classmap
This makes the weird coordinated-changes-by-identical-classmaps
"feature" unnecessary; the DEFINE can export the var, and USE refers
to the exported var.
So this patchset adds another section: __dyndbg_class_refs.
It is like __dyndbg_classes; it is scanned under ddebug_add_module(),
and attached to each module's ddebug_table. Once attached, it can be
used like classes to validate and apply class FOO >control queries.
It also maps the class user -> definer explicitly, so that when the
module is loaded, the section scan can find the kernel-param that is
wired to dyndbg's kparam-callback, and apply its state-var, forex:
__drm_debug to the just loaded helper/driver module.
Theres plenty to address Im sure.
Jim Cromie (17):
test-dyndbg: fixup CLASSMAP usage error
test-dyndbg: show that DEBUG enables prdbgs at compiletime
dyndbg: fix readback value on LEVEL_NAMES interfaces
dyndbg: replace classmap list with a vector
dyndbg: make ddebug_apply_class_bitmap more selective
dyndbg: dynamic_debug_init - use pointer inequality, not strcmp
dyndbg: drop NUM_TYPE_ARRAY
dyndbg: reduce verbose/debug clutter
dyndbg-API: replace DECLARE_DYNDBG_CLASSMAP with
DYNDBG_CLASSMAP(_DEFINE|_USE)
dyndbg-API: specialize DYNDBG_CLASSMAP_(DEFINE|USE)
dyndbg-API: DYNDBG_CLASSMAP_USE drop extra args
dyndbg-API: DYNDBG_CLASSMAP_DEFINE() improvements
drm_print: fix stale macro-name in comment
dyndbg: unwrap __ddebug_add_module inner function NOTYET
dyndbg: ddebug_sanity()
dyndbg: mess-w-dep-class
dyndbg: miss-on HACK
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 14 +-
drivers/gpu/drm/display/drm_dp_helper.c | 14 +-
drivers/gpu/drm/drm_crtc_helper.c | 14 +-
drivers/gpu/drm/drm_print.c | 22 +--
drivers/gpu/drm/i915/i915_params.c | 14 +-
drivers/gpu/drm/nouveau/nouveau_drm.c | 14 +-
include/asm-generic/vmlinux.lds.h | 3 +
include/drm/drm_print.h | 6 +-
include/linux/dynamic_debug.h | 57 ++++--
include/linux/map.h | 54 ++++++
kernel/module/main.c | 2 +
lib/dynamic_debug.c | 240 +++++++++++++++++++-----
lib/test_dynamic_debug.c | 47 ++---
13 files changed, 344 insertions(+), 157 deletions(-)
create mode 100644 include/linux/map.h
--
2.38.1
dynamic_debug_init() currently uses strcmp to find the module
boundaries in the builtin _ddebug[] table.
The table is filled by the linker; for its content, pointer inequality
works, is faster, and communicates the data properties more tightly.
Signed-off-by: Jim Cromie <[email protected]>
---
lib/dynamic_debug.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 5d609ff0d559..a0dc681cd215 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1410,7 +1410,7 @@ static int __init dynamic_debug_init(void)
for (; iter < __stop___dyndbg; iter++, i++, mod_sites++) {
- if (strcmp(modname, iter->modname)) {
+ if (modname != iter->modname) {
mod_ct++;
di.num_descs = mod_sites;
di.descs = iter_mod_start;
--
2.38.1
Cited commit uses stale macro name, fix this, and explain better.
When DRM_USE_DYNAMIC_DEBUG=y, DYNDBG_CLASSMAP_DEFINE() maps DRM_UT_*
onto BITs in drm.debug. This still uses enum drm_debug_category, but
it is somewhat indirect, with the ordered set of DRM_UT_* enum-vals.
This requires that the macro args: DRM_UT_* list must be kept in sync
and in order.
Fixes: f158936b60a7 ("drm: POC drm on dyndbg - use in core, 2 helpers, 3 drivers.")
Signed-off-by: Jim Cromie <[email protected]>
---
. emphasize ABI non-change despite enum val change - Jani Nikula
. reorder to back of patchset to follow API name changes.
---
include/drm/drm_print.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 6a27e8f26770..7695ba31b3a4 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -276,7 +276,10 @@ static inline struct drm_printer drm_err_printer(const char *prefix)
*
*/
enum drm_debug_category {
- /* These names must match those in DYNAMIC_DEBUG_CLASSBITS */
+ /*
+ * Keep DYNDBG_CLASSMAP_DEFINE args in sync with changes here,
+ * the enum-values define BIT()s in drm.debug, so are ABI.
+ */
/**
* @DRM_UT_CORE: Used in the generic drm code: drm_ioctl.c, drm_mm.c,
* drm_memory.c, ...
--
2.38.1
dont break the loop, to see multiple clients. the 3 client records
are differently wrong.
---
lib/dynamic_debug.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 3ef1c0a1f0cd..a26eaa348731 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -629,6 +629,7 @@ static int ddebug_apply_class_bitmap(const struct ddebug_class_param *dcp,
v2pr_info("bit_%d: %d matches on class: %s -> 0x%lx\n", bi,
ct, map->class_names[bi], *new_bits);
}
+ v2pr_info("applied bitmap: 0x%lx to: 0x%lx\n", *new_bits, *old_bits);
return matches;
}
@@ -1321,8 +1322,8 @@ static void ddebug_attach_client_module_classes(struct ddebug_table *dt, struct
*/
v2pr_info("break on %d/%d\n", i, di->num_class_refs);
dt->num_class_refs = 1;
- break;
- }
+ } else
+ v2pr_info("miss on %d/%d\n", i, di->num_class_refs);
}
}
--
2.38.1
Since sysfs knobs should generally read-back what was just written
(unless theres a good reason to do otherwise), this result (using
test_dynamic_debug.ko) is suboptimal:
echo L3 > p_level_names
cat p_level_names
4
Fix this with a -1 offset in LEVEL_NAMES readback.
NOTE:
Calling this a BUG is debatable, and the above is slightly inaccurate
wrt "read-back"; whats written is a LEVEL_NAME (a string). Whats read
back is an integer, giving the 'edge' of the 'low-pass-filter'
The actual test looks like:
RTT: L4 -> p_level_names : 4 :: DOING: levels 4-1
[ 17.509594] dyndbg: "L4" > p_level_names:0x4
[ 17.510115] dyndbg: apply: 0x1f to: 0xf
[ 17.510506] dyndbg: query 0: "class L4 +p" mod:*
[ 17.510992] dyndbg: split into words: "class" "L4" "+p"
[ 17.511521] dyndbg: op='+'
[ 17.511811] dyndbg: flags=0x1
[ 17.512127] dyndbg: *flagsp=0x1 *maskp=0xffffffff
[ 17.512604] dyndbg: parsed: func="" file="" module="" format="" lineno=0-0 class=L4
[ 17.513414] dyndbg: applied: func="" file="" module="" format="" lineno=0-0 class=L4
[ 17.514204] dyndbg: processed 1 queries, with 1 matches, 0 errs
[ 17.514809] dyndbg: bit_4: 1 matches on class: L4 -> 0x1f
[ 17.515355] dyndbg: p_level_names: changed bit-4: "L4" f->1f
[ 17.515933] dyndbg: total matches: 1
crap [[ 5 != 4 ]]
This -1 adjustment just reports the edge consistently with its
input-mapping.
Fixes: b9400852c080 (dyndbg: add drm.debug style (drm/parameters/debug) bitmap support)
Signed-off-by: Jim Cromie <[email protected]>
---
lib/dynamic_debug.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 009f2ead09c1..48ca1387a409 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -794,6 +794,8 @@ int param_get_dyndbg_classes(char *buffer, const struct kernel_param *kp)
return scnprintf(buffer, PAGE_SIZE, "0x%lx\n", *dcp->bits);
case DD_CLASS_TYPE_LEVEL_NAMES:
+ return scnprintf(buffer, PAGE_SIZE, "%d\n", *dcp->lvl - 1);
+
case DD_CLASS_TYPE_LEVEL_NUM:
return scnprintf(buffer, PAGE_SIZE, "%d\n", *dcp->lvl);
default:
--
2.38.1
It appears that, at least for builtin drm,i915 loadable amdgpu, data
in the class_refs section is not properly linked, this works partway
towards isolating the problem.
The "NO CLI" name test is failing.
This version of the report fails with a non-canonical address:
// v2pr_info("NO CLI name on: %s\n", cli->map->mod_name);
[ 0.109299] dyndbg: add-module: main 6 sites 1.3
[ 0.109595] general protection fault, probably for non-canonical address 0x7265766972640000: 0000
fwiw:
$ perl -e ' $a = pack "H8", "7265766972640000"; print "a:<$a>\n"'
a:<revi>
These records are added to __dyndbg_class_refs section by
DYNDBG_CLASSMAP_USE
This patch adds ddebug_sanity(), and calls it 3 times to characterize
what goes wrong and when. It turns out that its contents are wrong
immediately, in 1st step of dynamic_debug_init().
[ 0.107327] dyndbg: classes
[ 0.107537] dyndbg: class[0]: module:drm base:0 len:10 ty:0 cm:ffffffff834fc4c8
[ 0.107592] dyndbg: class-refs
[ 0.107823] dyndbg: class-ref[0]: cli:ffffffff834fc508 map:ffffffff8293e35e nm:0000000000000000 nm:(null)
[ 0.108554] dyndbg: class-ref[1]: cli:ffffffff834fc518 map:ffffffff8293fe86 nm:ffffffff834fc4c8 nm:
[ 0.108590] dyndbg: class-ref[2]: cli:ffffffff834fc528 map:ffffffff829785aa nm:ffffffff834fc4c8 nm:
Those maps are wrong:
class-refs should all ref the same map, ie class[0]: module:drm.
the nm:s should also show module names of 3 builtin clients of drm.
So things must end poorly.
modprobing the loadable module does better:
bash-5.2# modprobe amdgpu
[ 6645.212706] AMD-Vi: AMD IOMMUv2 functionality not available on this system - This is not a bug.
[ 6645.653124] dyndbg: add-module: amdgpu 4425 sites 0.1
[ 6645.653582] dyndbg: classes
[ 6645.653830] dyndbg: class-refs
[ 6645.654124] dyndbg: class-ref[0]: cli:ffffffffc0a31b90 map:ffffffff834fc4c8 nm:ffffffffc08bc176 nm:amdgpu
[ 6645.654936] dyndbg: classes
[ 6645.655188] dyndbg: class-refs
[ 6645.655450] dyndbg: class-ref[0]: cli:ffffffffc0a31b90 map:ffffffff834fc4c8 nm:ffffffffc08bc176 nm:amdgpu
[ 6645.656246] dyndbg: class_ref[0] amdgpu -> drm
[ 6645.656613] dyndbg: amdgpu needs drm, 0x0
[ 6645.656953] dyndbg: apply bitmap: 0x0 to: 0x0
[ 6645.657322] dyndbg: break on 0/1
[ 6645.657592] dyndbg: 4425 debug prints in module amdgpu
Here, the maps are correct; they ref the class[0] module:drm above.
That said, apply bitmap is wrong.
Signed-off-by: Jim Cromie <[email protected]>
---
[ 1.210094] dyndbg: add-module: drm 302 sites 1.3
[ 1.210496] dyndbg: classes
[ 1.210674] dyndbg: class[0]: module:drm base:0 len:10 ty:0 cm:ffffffff834fc4c8
[ 1.211290] dyndbg: class-refs
[ 1.211548] dyndbg: class-ref[0]: cli:ffffffff834fc508 map:ffffffff8293e376 nm:0000000000000000 nm:(null)
[ 1.211674] dyndbg: class-ref[1]: cli:ffffffff834fc518 map:ffffffff8293fe9e nm:ffffffff834fc4c8 nm:
[ 1.212464] dyndbg: class-ref[2]: cli:ffffffff834fc528 map:ffffffff829785c2 nm:ffffffff834fc4c8 nm:
[ 1.212675] dyndbg: classes start: class[0]: module:drm base:0 len:10 ty:0
[ 1.213257] dyndbg: builtin class: module:drm base:0 len:10 ty:0
[ 1.213675] dyndbg: controlling kp: drm.drm.debug
[ 1.214087] dyndbg: module:drm attached 1 classes
[ 1.214490] dyndbg: classes
[ 1.214674] dyndbg: class[0]: module:drm base:0 len:10 ty:0 cm:ffffffff834fc4c8
[ 1.215291] dyndbg: class-refs
[ 1.215552] dyndbg: class-ref[0]: cli:ffffffff834fc508 map:ffffffff8293e376 nm:0000000000000000 nm:(null)
[ 1.215674] dyndbg: class-ref[1]: cli:ffffffff834fc518 map:ffffffff8293fe9e nm:ffffffff834fc4c8 nm:
[ 1.216430] dyndbg: class-ref[2]: cli:ffffffff834fc528 map:ffffffff829785c2 nm:ffffffff834fc4c8 nm:
[ 1.216674] dyndbg: NO CLI ffffffff8293e376 7265766972640000
[ 1.217149] dyndbg: 302 debug prints in module drm
[ 1.217549] dyndbg: add-module: drm_kms_helper 95 sites 1.3
[ 1.217674] dyndbg: classes
[ 1.217913] dyndbg: class[0]: module:drm base:0 len:10 ty:0 cm:ffffffff834fc4c8
[ 1.218525] dyndbg: class-refs
[ 1.218674] dyndbg: class-ref[0]: cli:ffffffff834fc508 map:ffffffff8293e376 nm:0000000000000000 nm:(null)
[ 1.219486] dyndbg: class-ref[1]: cli:ffffffff834fc518 map:ffffffff8293fe9e nm:ffffffff834fc4c8 nm:
[ 1.219674] dyndbg: class-ref[2]: cli:ffffffff834fc528 map:ffffffff829785c2 nm:ffffffff834fc4c8 nm:
[ 1.220469] dyndbg: classes
[ 1.220674] dyndbg: class[0]: module:drm base:0 len:10 ty:0 cm:ffffffff834fc4c8
[ 1.221324] dyndbg: class-refs
[ 1.221606] dyndbg: class-ref[0]: cli:ffffffff834fc508 map:ffffffff8293e376 nm:0000000000000000 nm:(null)
[ 1.221674] dyndbg: class-ref[1]: cli:ffffffff834fc518 map:ffffffff8293fe9e nm:ffffffff834fc4c8 nm:
[ 1.222472] dyndbg: class-ref[2]: cli:ffffffff834fc528 map:ffffffff829785c2 nm:ffffffff834fc4c8 nm:
[ 1.222674] dyndbg: NO CLI ffffffff8293e376 7265766972640000
[ 1.223173] dyndbg: 95 debug prints in module drm_kms_helper
[ 1.223675] dyndbg: add-module: drm_display_helper 150 sites 1.3
[ 1.224223] dyndbg: classes
[ 1.224484] dyndbg: class[0]: module:drm base:0 len:10 ty:0 cm:ffffffff834fc4c8
[ 1.224676] dyndbg: class-refs
[ 1.224954] dyndbg: class-ref[0]: cli:ffffffff834fc508 map:ffffffff8293e376 nm:0000000000000000 nm:(null)
[ 1.225674] dyndbg: class-ref[1]: cli:ffffffff834fc518 map:ffffffff8293fe9e nm:ffffffff834fc4c8 nm:
[ 1.226498] dyndbg: class-ref[2]: cli:ffffffff834fc528 map:ffffffff829785c2 nm:ffffffff834fc4c8 nm:
[ 1.226674] dyndbg: classes
[ 1.226931] dyndbg: class[0]: module:drm base:0 len:10 ty:0 cm:ffffffff834fc4c8
[ 1.227577] dyndbg: class-refs
[ 1.227674] dyndbg: class-ref[0]: cli:ffffffff834fc508 map:ffffffff8293e376 nm:0000000000000000 nm:(null)
[ 1.228501] dyndbg: class-ref[1]: cli:ffffffff834fc518 map:ffffffff8293fe9e nm:ffffffff834fc4c8 nm:
[ 1.228674] dyndbg: class-ref[2]: cli:ffffffff834fc528 map:ffffffff829785c2 nm:ffffffff834fc4c8 nm:
[ 1.229443] dyndbg: NO CLI ffffffff8293e376 7265766972640000
[ 1.229674] dyndbg: 150 debug prints in module drm_display_helper
[ 1.230195] dyndbg: add-module: ttm 2 sites 1.3
[ 1.230581] dyndbg: classes
[ 1.230674] dyndbg: class[0]: module:drm base:0 len:10 ty:0 cm:ffffffff834fc4c8
[ 1.231291] dyndbg: class-refs
[ 1.231559] dyndbg: class-ref[0]: cli:ffffffff834fc508 map:ffffffff8293e376 nm:0000000000000000 nm:(null)
[ 1.231674] dyndbg: class-ref[1]: cli:ffffffff834fc518 map:ffffffff8293fe9e nm:ffffffff834fc4c8 nm:
[ 1.232439] dyndbg: class-ref[2]: cli:ffffffff834fc528 map:ffffffff829785c2 nm:ffffffff834fc4c8 nm:
[ 1.232674] dyndbg: classes
[ 1.232915] dyndbg: class[0]: module:drm base:0 len:10 ty:0 cm:ffffffff834fc4c8
[ 1.233535] dyndbg: class-refs
[ 1.233674] dyndbg: class-ref[0]: cli:ffffffff834fc508 map:ffffffff8293e376 nm:0000000000000000 nm:(null)
[ 1.234470] dyndbg: class-ref[1]: cli:ffffffff834fc518 map:ffffffff8293fe9e nm:ffffffff834fc4c8 nm:
[ 1.234674] dyndbg: class-ref[2]: cli:ffffffff834fc528 map:ffffffff829785c2 nm:ffffffff834fc4c8 nm:
[ 1.235427] dyndbg: NO CLI ffffffff8293e376 7265766972640000
[ 1.235674] dyndbg: 2 debug prints in module ttm
[ 1.236079] dyndbg: add-module: i915 1657 sites 1.3
[ 1.236490] dyndbg: classes
[ 1.236674] dyndbg: class[0]: module:drm base:0 len:10 ty:0 cm:ffffffff834fc4c8
[ 1.237283] dyndbg: class-refs
[ 1.237545] dyndbg: class-ref[0]: cli:ffffffff834fc508 map:ffffffff8293e376 nm:0000000000000000 nm:(null)
[ 1.237674] dyndbg: class-ref[1]: cli:ffffffff834fc518 map:ffffffff8293fe9e nm:ffffffff834fc4c8 nm:
[ 1.238431] dyndbg: class-ref[2]: cli:ffffffff834fc528 map:ffffffff829785c2 nm:ffffffff834fc4c8 nm:
[ 1.238674] dyndbg: classes
[ 1.238911] dyndbg: class[0]: module:drm base:0 len:10 ty:0 cm:ffffffff834fc4c8
[ 1.239519] dyndbg: class-refs
[ 1.239674] dyndbg: class-ref[0]: cli:ffffffff834fc508 map:ffffffff8293e376 nm:0000000000000000 nm:(null)
[ 1.240467] dyndbg: class-ref[1]: cli:ffffffff834fc518 map:ffffffff8293fe9e nm:ffffffff834fc4c8 nm:
[ 1.240674] dyndbg: class-ref[2]: cli:ffffffff834fc528 map:ffffffff829785c2 nm:ffffffff834fc4c8 nm:
[ 1.241478] dyndbg: NO CLI ffffffff8293e376 7265766972640000
[ 1.241674] dyndbg: 1657 debug prints in module i915
---
include/linux/dynamic_debug.h | 4 ++--
lib/dynamic_debug.c | 32 +++++++++++++++++++++++++++++++-
2 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 6f53a687cb32..2a1199aadab6 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -115,8 +115,8 @@ struct ddebug_class_map {
EXPORT_SYMBOL(_var)
struct ddebug_class_user {
- char *user_mod_name;
- struct ddebug_class_map *map;
+ const char *user_mod_name;
+ const struct ddebug_class_map *map;
};
/**
* DYNDBG_CLASSMAP_USE - refer to a classmap, DEFINEd elsewhere.
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 02f36c553835..46684aa7284d 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1301,7 +1301,8 @@ static void ddebug_attach_client_module_classes(struct ddebug_table *dt, struct
continue;
}
if (!cli->user_mod_name) {
- v2pr_info("NO CLI name\n");
+ v2pr_info("NO CLI name %px %px\n", cli->map, cli->map->mod_name);
+ // v2pr_info("NO CLI name on: %s\n", cli->map->mod_name);
continue;
}
@@ -1325,6 +1326,29 @@ static void ddebug_attach_client_module_classes(struct ddebug_table *dt, struct
}
}
+static void ddebug_sanity(struct _ddebug_info *di)
+{
+ struct ddebug_class_map *cm;
+ struct ddebug_class_user *cli;
+ int i;
+
+ if (di->num_classes)
+ v2pr_info("classes\n");
+
+ for (i = 0, cm = di->classes; i < di->num_classes; i++, cm++) {
+ v2pr_info("class[%d]: module:%s base:%d len:%d ty:%d cm:%px\n",
+ i, cm->mod_name, cm->base, cm->length, cm->map_type, cm);
+ }
+ if (di->num_class_refs)
+ v2pr_info("class-refs\n");
+
+ for (i = 0, cli = di->class_refs; i < di->num_class_refs; i++, cli++) {
+ // cli->map->mod_name will segv
+ v2pr_info("class-ref[%d]: cli:%px map:%px nm:%px nm:%s\n", i, cli,
+ cli->map, cli->user_mod_name, cli->user_mod_name);
+ }
+}
+
/*
* Allocate a new ddebug_table for the given module
* and add it to the global list.
@@ -1357,6 +1381,8 @@ int ddebug_add_module(struct _ddebug_info *di, const char *modname)
INIT_LIST_HEAD(&dt->link);
+ ddebug_sanity(di);
+
if (di->num_classes)
ddebug_attach_module_classes(dt, di);
@@ -1364,6 +1390,8 @@ int ddebug_add_module(struct _ddebug_info *di, const char *modname)
list_add_tail(&dt->link, &ddebug_tables);
mutex_unlock(&ddebug_lock);
+ ddebug_sanity(di);
+
if (di->num_class_refs)
ddebug_attach_client_module_classes(dt, di);
@@ -1493,6 +1521,8 @@ static int __init dynamic_debug_init(void)
.num_class_refs = __stop___dyndbg_class_refs - __start___dyndbg_class_refs,
};
+ ddebug_sanity(&di);
+
if (&__start___dyndbg == &__stop___dyndbg) {
if (IS_ENABLED(CONFIG_DYNAMIC_DEBUG)) {
pr_warn("_ddebug table is empty in a CONFIG_DYNAMIC_DEBUG build\n");
--
2.38.1
On Mon, Dec 05, 2022 at 05:34:20PM -0700, Jim Cromie wrote:
> Cited commit uses stale macro name, fix this, and explain better.
>
> When DRM_USE_DYNAMIC_DEBUG=y, DYNDBG_CLASSMAP_DEFINE() maps DRM_UT_*
> onto BITs in drm.debug. This still uses enum drm_debug_category, but
> it is somewhat indirect, with the ordered set of DRM_UT_* enum-vals.
> This requires that the macro args: DRM_UT_* list must be kept in sync
> and in order.
>
> Fixes: f158936b60a7 ("drm: POC drm on dyndbg - use in core, 2 helpers, 3 drivers.")
> Signed-off-by: Jim Cromie <[email protected]>
Should I land this already?
-Daniel
> ---
> . emphasize ABI non-change despite enum val change - Jani Nikula
> . reorder to back of patchset to follow API name changes.
> ---
> include/drm/drm_print.h | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index 6a27e8f26770..7695ba31b3a4 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -276,7 +276,10 @@ static inline struct drm_printer drm_err_printer(const char *prefix)
> *
> */
> enum drm_debug_category {
> - /* These names must match those in DYNAMIC_DEBUG_CLASSBITS */
> + /*
> + * Keep DYNDBG_CLASSMAP_DEFINE args in sync with changes here,
> + * the enum-values define BIT()s in drm.debug, so are ABI.
> + */
> /**
> * @DRM_UT_CORE: Used in the generic drm code: drm_ioctl.c, drm_mm.c,
> * drm_memory.c, ...
> --
> 2.38.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Mon, Dec 05, 2022 at 05:34:07PM -0700, Jim Cromie wrote:
> Hi everyone,
>
> DRM_USE_DYNAMIC_DEBUG=y has a regression on rc-*
>
> Regression is due to a chicken-egg problem loading modules; on
> `modprobe i915`, drm is loaded 1st, and drm.debug is set. When
> drm_debug_enabled() tested __drm_debug at runtime, that just worked.
>
> But with DRM_USE_DYNAMIC_DEBUG=y, the runtime test is replaced with a
> post-load enablement of drm_dbg/dyndbg callsites (static-keys), via
> dyndbg's callback on __drm_debug. Since all drm-drivers need drm.ko,
> it is loaded 1st, then drm.debug=X is applied, then drivers load, but
> too late for drm_dbgs to be enabled.
>
> STATUS
>
> For all-loadable drm,i915,amdgpu configs, it almost works, but
> propagating drm.debug to dependent modules doesnt actually apply,
> though the motions are there. This is not the problem I want to chase
> here.
>
> The more basic trouble is:
>
> For builtin drm + helpers, things are broken pretty early; at the
> beginning of dynamic_debug_init(). As the ddebug_sanity() commit-msg
> describes in some detail, the records added by _USE fail to reference
> the struct ddebug_class_map created and exported by _DEFINE, but get
> separate addresses to "other" data that segv's when used as the
> expected pointer. FWIW, the pointer val starts with "revi".
So I honestly have no idea here, linker stuff is way beyond where I have
clue. So what's the way forward here?
The DEFINE/USE split does like the right thing to do at least from the
"how it's used in drivers" pov. But if we're just running circles not
quite getting there I dunno :-/
-Daniel
>
> OVERVIEW
>
> DECLARE_DYNDBG_CLASSMAP is broken: it is one-size-fits-all-poorly.
> It muddles the distinction between a (single) definition, and multiple
> references. Something exported should suffice.
>
> The core of this patchset splits it into:
>
> DYNDBG_CLASSMAP_DEFINE used once per subsystem to define each classmap
> DYNDBG_CLASSMAP_USE declare dependence on a DEFINEd classmap
>
> This makes the weird coordinated-changes-by-identical-classmaps
> "feature" unnecessary; the DEFINE can export the var, and USE refers
> to the exported var.
>
> So this patchset adds another section: __dyndbg_class_refs.
>
> It is like __dyndbg_classes; it is scanned under ddebug_add_module(),
> and attached to each module's ddebug_table. Once attached, it can be
> used like classes to validate and apply class FOO >control queries.
>
> It also maps the class user -> definer explicitly, so that when the
> module is loaded, the section scan can find the kernel-param that is
> wired to dyndbg's kparam-callback, and apply its state-var, forex:
> __drm_debug to the just loaded helper/driver module.
>
> Theres plenty to address Im sure.
>
> Jim Cromie (17):
> test-dyndbg: fixup CLASSMAP usage error
> test-dyndbg: show that DEBUG enables prdbgs at compiletime
> dyndbg: fix readback value on LEVEL_NAMES interfaces
> dyndbg: replace classmap list with a vector
> dyndbg: make ddebug_apply_class_bitmap more selective
> dyndbg: dynamic_debug_init - use pointer inequality, not strcmp
> dyndbg: drop NUM_TYPE_ARRAY
> dyndbg: reduce verbose/debug clutter
> dyndbg-API: replace DECLARE_DYNDBG_CLASSMAP with
> DYNDBG_CLASSMAP(_DEFINE|_USE)
> dyndbg-API: specialize DYNDBG_CLASSMAP_(DEFINE|USE)
> dyndbg-API: DYNDBG_CLASSMAP_USE drop extra args
> dyndbg-API: DYNDBG_CLASSMAP_DEFINE() improvements
> drm_print: fix stale macro-name in comment
> dyndbg: unwrap __ddebug_add_module inner function NOTYET
> dyndbg: ddebug_sanity()
> dyndbg: mess-w-dep-class
> dyndbg: miss-on HACK
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 14 +-
> drivers/gpu/drm/display/drm_dp_helper.c | 14 +-
> drivers/gpu/drm/drm_crtc_helper.c | 14 +-
> drivers/gpu/drm/drm_print.c | 22 +--
> drivers/gpu/drm/i915/i915_params.c | 14 +-
> drivers/gpu/drm/nouveau/nouveau_drm.c | 14 +-
> include/asm-generic/vmlinux.lds.h | 3 +
> include/drm/drm_print.h | 6 +-
> include/linux/dynamic_debug.h | 57 ++++--
> include/linux/map.h | 54 ++++++
> kernel/module/main.c | 2 +
> lib/dynamic_debug.c | 240 +++++++++++++++++++-----
> lib/test_dynamic_debug.c | 47 ++---
> 13 files changed, 344 insertions(+), 157 deletions(-)
> create mode 100644 include/linux/map.h
>
> --
> 2.38.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Wed, Jan 11, 2023 at 4:09 PM Daniel Vetter <[email protected]> wrote:
>
> On Mon, Dec 05, 2022 at 05:34:07PM -0700, Jim Cromie wrote:
> > Hi everyone,
> >
> > DRM_USE_DYNAMIC_DEBUG=y has a regression on rc-*
> >
> > Regression is due to a chicken-egg problem loading modules; on
> > `modprobe i915`, drm is loaded 1st, and drm.debug is set. When
> > drm_debug_enabled() tested __drm_debug at runtime, that just worked.
> >
> > But with DRM_USE_DYNAMIC_DEBUG=y, the runtime test is replaced with a
> > post-load enablement of drm_dbg/dyndbg callsites (static-keys), via
> > dyndbg's callback on __drm_debug. Since all drm-drivers need drm.ko,
> > it is loaded 1st, then drm.debug=X is applied, then drivers load, but
> > too late for drm_dbgs to be enabled.
> >
> > STATUS
> >
> > For all-loadable drm,i915,amdgpu configs, it almost works, but
> > propagating drm.debug to dependent modules doesnt actually apply,
> > though the motions are there. This is not the problem I want to chase
> > here.
> >
> > The more basic trouble is:
> >
> > For builtin drm + helpers, things are broken pretty early; at the
> > beginning of dynamic_debug_init(). As the ddebug_sanity() commit-msg
> > describes in some detail, the records added by _USE fail to reference
> > the struct ddebug_class_map created and exported by _DEFINE, but get
> > separate addresses to "other" data that segv's when used as the
> > expected pointer. FWIW, the pointer val starts with "revi".
>
> So I honestly have no idea here, linker stuff is way beyond where I have
> clue. So what's the way forward here?
>
Ive fixed this aspect.
Unsurprisingly, it wasnt the linker :-}
> The DEFINE/USE split does like the right thing to do at least from the
> "how it's used in drivers" pov. But if we're just running circles not
> quite getting there I dunno :-/
> -Daniel
>
Sending new rev next.
I think its getting close.
On Fri, Jan 13, 2023 at 11:29:57AM -0700, [email protected] wrote:
> On Wed, Jan 11, 2023 at 4:09 PM Daniel Vetter <[email protected]> wrote:
> >
> > On Mon, Dec 05, 2022 at 05:34:07PM -0700, Jim Cromie wrote:
> > > Hi everyone,
> > >
> > > DRM_USE_DYNAMIC_DEBUG=y has a regression on rc-*
> > >
> > > Regression is due to a chicken-egg problem loading modules; on
> > > `modprobe i915`, drm is loaded 1st, and drm.debug is set. When
> > > drm_debug_enabled() tested __drm_debug at runtime, that just worked.
> > >
> > > But with DRM_USE_DYNAMIC_DEBUG=y, the runtime test is replaced with a
> > > post-load enablement of drm_dbg/dyndbg callsites (static-keys), via
> > > dyndbg's callback on __drm_debug. Since all drm-drivers need drm.ko,
> > > it is loaded 1st, then drm.debug=X is applied, then drivers load, but
> > > too late for drm_dbgs to be enabled.
> > >
> > > STATUS
> > >
> > > For all-loadable drm,i915,amdgpu configs, it almost works, but
> > > propagating drm.debug to dependent modules doesnt actually apply,
> > > though the motions are there. This is not the problem I want to chase
> > > here.
> > >
> > > The more basic trouble is:
> > >
> > > For builtin drm + helpers, things are broken pretty early; at the
> > > beginning of dynamic_debug_init(). As the ddebug_sanity() commit-msg
> > > describes in some detail, the records added by _USE fail to reference
> > > the struct ddebug_class_map created and exported by _DEFINE, but get
> > > separate addresses to "other" data that segv's when used as the
> > > expected pointer. FWIW, the pointer val starts with "revi".
> >
> > So I honestly have no idea here, linker stuff is way beyond where I have
> > clue. So what's the way forward here?
> >
>
> Ive fixed this aspect.
> Unsurprisingly, it wasnt the linker :-}
Awesome!
> > The DEFINE/USE split does like the right thing to do at least from the
> > "how it's used in drivers" pov. But if we're just running circles not
> > quite getting there I dunno :-/
> > -Daniel
> >
>
> Sending new rev next.
> I think its getting close.
Thanks a lot for keeping on pushing this.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch