2023-01-13 19:47:03

by Jim Cromie

Subject: [PATCH v2 00/20] DRM_USE_DYNAMIC_DEBUG regression

Hi everyone,

DRM_USE_DYNAMIC_DEBUG=y has a regression enabling drm.debug in drivers

It is due to a chicken-egg problem loading modules; on `modprobe
i915`, drm is loaded 1st, and drm/parameters/debug is set. When
drm_debug_enabled() tested it at runtime, things just worked.

But with DRM_USE_DYNAMIC_DEBUG=y, the runtime test is replaced with a
post-insmod enablement of drm_dbg/dyndbg callsites, via dyndbg's
callback on __drm_debug. So with drm.ko loaded before the dependent
modules, their debug callsites arent present to be enabled.

OVERVIEW

As Jani Nikula noted rather more gently, DECLARE_DYNDBG_CLASSMAP is
error-prone enough to call broken: it relied upon identical classmap
definitions in all modules using DRM_UT_*. IOW, it muddled the K&R
distinction between a (single) definition, and multiple references.

So 4 patches here split 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.

DYNDBG_CLASSMAP_DEFINE initializes the classmap, stores it into the
(existing) __dyndbg_classes section, and exports the struct var
(unlike DECLARE_DYNDBG_CLASSMAP).

DYNDBG_CLASSMAP_USE initializes a class-ref struct, containing the
user-module-name, and a ref to the exported classmap var.

The distinction allows separate treatment of classmaps and
classmap-refs, the latter getting additional behavior to propagate
class'd prdbg settings to modules being loaded.

Consider i915.ko, a DYNDBG_CLASSMAP_USEr: due to dependencies,
`modprobe drm debug=$val` is done 1st. For DRM_USE_DYNAMIC_DEBUG=y,
drm.debug=$val invokes dyndbg's kparam callback, which applies "class
DRM_UT_*" >controls as given by the bits. But i915.ko isn't modprobed
yet, so misses those >controls. These must be 're-delivered' when
i915 is modprobed.

Recapitulating ddebug_attach_module_classes(1) for (existing) classes;
ddebug_attach_client_module_classes(2) does this for (new) class-refs,
as they are found when modprobing drm-drivers.

2 calls ddebug_apply_parents_params(3) on each referred classmap defn.

3 scans kernel-params owned by the module DEFINEing the classmap,
either builtin or loadable, calls ddebug_match_apply_kparam(4) on each.

4 finds those kparams wired to dyndbg's param-ops, which are therefore
castable to struct ddebug_class_param, and have a ref to the classmap
that they "control". So 4 finds classmap definitions whose owner
matches the user-module being loaded, and applies the kparam's state
(__drm_debug in this case) by calling ddebug_apply_class_bitmap().

test_dynamic_debug is extended to recreate DRM's multi-module
regression, it builds both test_dynamic_debug.ko and _submod.ko, with
an ifdef to _DEFINE in the main module, and _USE in the submod. This
gives both modules identical set of prdbgs, which is helpful for
humans comparing results.

STATUS

here it is, "working":

doing class DRM_UT_CORE -p
[ 9904.961750] dyndbg: read 21 bytes from userspace
[ 9904.962286] dyndbg: query 0: "class DRM_UT_CORE -p" mod:*
[ 9904.962848] dyndbg: split into words: "class" "DRM_UT_CORE" "-p"
[ 9904.963444] dyndbg: op='-' flags=0x0 maskp=0xfffffffe
[ 9904.963945] dyndbg: parsed: func="" file="" module="" format="" lineno=0-0 class=DRM_UT_CORE
[ 9904.964781] dyndbg: good-class: drm.DRM_UT_CORE module:drm nd:302 nc:1 nu:0
[ 9904.966411] dyndbg: class-ref: drm_kms_helper.DRM_UT_CORE module:drm_kms_helper nd:95 nc:0 nu:1
[ 9904.967265] dyndbg: class-ref: drm_display_helper.DRM_UT_CORE module:drm_display_helper nd:150 nc:0 nu:1
[ 9904.968349] dyndbg: class-ref: i915.DRM_UT_CORE module:i915 nd:1659 nc:0 nu:1
[ 9904.969801] dyndbg: class-ref: amdgpu.DRM_UT_CORE module:amdgpu nd:4425 nc:0 nu:1
[ 9904.977079] dyndbg: class-ref: nouveau.DRM_UT_CORE module:nouveau nd:103 nc:0 nu:1
[ 9904.977830] dyndbg: processed 1 queries, with 507 matches, 0 errs
doing class DRM_UT_DRIVER +p
[ 9906.151761] dyndbg: read 23 bytes from userspace
[ 9906.152241] dyndbg: query 0: "class DRM_UT_DRIVER +p" mod:*
[ 9906.152793] dyndbg: split into words: "class" "DRM_UT_DRIVER" "+p"
[ 9906.153388] dyndbg: op='+' flags=0x1 maskp=0xffffffff
[ 9906.153896] dyndbg: parsed: func="" file="" module="" format="" lineno=0-0 class=DRM_UT_DRIVER
[ 9906.154746] dyndbg: good-class: drm.DRM_UT_DRIVER module:drm nd:302 nc:1 nu:0
[ 9906.155433] dyndbg: class-ref: drm_kms_helper.DRM_UT_DRIVER module:drm_kms_helper nd:95 nc:0 nu:1
[ 9906.156267] dyndbg: class-ref: drm_display_helper.DRM_UT_DRIVER module:drm_display_helper nd:150 nc:0 nu:1
[ 9906.157365] dyndbg: class-ref: i915.DRM_UT_DRIVER module:i915 nd:1659 nc:0 nu:1
[ 9906.163848] dyndbg: class-ref: amdgpu.DRM_UT_DRIVER module:amdgpu nd:4425 nc:0 nu:1
[ 9906.178963] dyndbg: class-ref: nouveau.DRM_UT_DRIVER module:nouveau nd:103 nc:0 nu:1
[ 9906.179934] dyndbg: processed 1 queries, with 1286 matches, 0 errs

It is still WIP, but Daniel Vetter inquired, so I figured Id send what I got.

The series ends in an RFC patch; __jump_label_patch can "crash the
box" when the jump-entry is in the wrong state. The code makes no
distinction between a well-formed "toggled" state and "insane". This
patch gets me past some mis-initialization problem Im still probing.

Things seem to be mostly working.

Heres the script that Ive been testing with:
(see submod_test at bottom)

#!/bin/bash

# shell funcs to test module load behavior of dynamic-debug, using
# test_dynamic_debug module. Meant to be useful at shell command
# line, after sourcing this.

PS1=":#> " # obvious clue we're in the "environment"

# grease for use/play
alias ddcmd='echo $* > /proc/dynamic_debug/control'

# support modprobe foo dyndbg=$VAR testing
export THML="class HI +p;class MID +p;class LOW +p;"
export TLVLS="class L1 +p;class L2 +p;class L3 +p;class L4 -p;"

alias lmt='modprobe test_dynamic_debug dyndbg=+pmf'
alias rmt='rmmod test_dynamic_debug'
alias look='grep test_dynamic_debug /proc/dynamic_debug/control'

alias mpd='modprobe $1 dyndbg="$2"'
alias mpt='modprobe test_dynamic_debug dyndbg="$*"'

alias skt='cd /sys/kernel/tracing'
alias smtddp='cd /sys/module/test_dynamic_debug/parameters'

alias cget='grep $1 /proc/dynamic_debug/control'
lookfor () {
grep $1 /proc/dynamic_debug/control
}

vx() {
echo $* > /sys/module/dynamic_debug/parameters/verbose
}

# amdgpu has ~2200 pr-debugs (before drm-debug-on-dyndbg),
# use them to prove kernel has both DYNAMIC_DEBUG and a DRM module
# and modprobe <mod> dyndbg=+p works

load_drm_mod_test() {
local mod=${1:-amdgpu}
rmmod $mod 2>/dev/null
modprobe $mod dyndbg=+pfm
let count=$(grep =pmf /proc/dynamic_debug/control | grep $mod | wc -l)
rmmod $mod

if [ $count -gt 100 ] ; then
echo $mod has $count pr-dbgs
return 0
else
echo $mod: too few sites: $count, suspect bad config
return -1
fi
}


SMTDDP=/sys/module/test_dynamic_debug/parameters

function doit() {
cat $SMTDDP/do_prints
}
qry_cmd() { # like ddcmd, with feedback
echo "QCMD: $* to-control" >&2
echo $* > /proc/dynamic_debug/control
}

# exercise class FOO behavior of test_dynamic_debug module
ttest_module__() {
flg=$1
dmesg -C
modprobe test_dynamic_debug dyndbg=+pfm
doit

for cls in L1 L2 L3; do
qry_cmd module test_dynamic_debug class $cls $flg
doit
done
doit

for cls in LOW MID HI; do
qry_cmd module test_dynamic_debug class $cls $flg
doit
done
doit

for cls in D2_CORE D2_KMS D2_DRIVER; do
qry_cmd module test_dynamic_debug class $cls $flg
doit
done
doit

dmesg | grep class
}

ttest_module() {

ttest_module__ +p
ttest_module__ -p

#ttest_module__ +T
#ttest_module__ -T
}


# use/test bitmaps
# knob is a sysfs-node

tddm_is_knob_() {
knob=$1;
if [ -f $SMTDDP/$knob ]; then
return 0
else
echo missing sysfs node: $knob, skipping tests
return 1
fi
}

tddm_set_knob_() {
knob=$1;
val=$2;
expect=$3;
[[ -z $knob || -z $val ]] && echo "expecting 2+args: knob val [result]" && return

echo "TDDM: $val >$knob" >&2 && sleep 0.1
echo $val > $SMTDDP/$knob
echo "? $?"

val=`cat $SMTDDP/$knob`
[[ -z $expect ]] && expect=$val

if [[ $val == $expect ]]; then
echo pass-ish $val
else
echo fail $val != $expect
fi
}

tddm_knob_clname_() {
targ=$1
cls=$2
res=$3
tddm_is_knob_ $targ || continue;
tddm_set_knob_ $targ $cls $res
cat $SMTDDP/do_prints
}

tddm_knob_classes_() {
targ=$1
shift
cls=$*

tddm_is_knob_ $targ || return;
for bitsym in $cls;
do
tddm_knob_clname_ $targ $bitsym
cat $SMTDDP/do_prints
done
}

# round-trip-test - args: node val want explanation
rtt_nvwx() {
node=$1 # to this
val=$2 # write this
want=$3 # expect this on readback
[ -z $3 ] && echo "want args: node input result" && return

shift 3;
echo "RTT: $val -> $node : $want :: DOING: $*" >&2 && sleep 0.1

echo $val > $SMTDDP/$node
rc=$(cat $SMTDDP/$node)
# echo rc $rc
# pretty literal test
[[ $rc != $want ]] && echo crap [[ $rc != $want ]] && return 1
}

# test-round-trip, using above.

# test disjoint - disjoint interface
trx_disjoint_names() {
nm=$1
rtt_nvwx $nm -HI,-MID,-LOW 0x0 all-off
rtt_nvwx $nm HI 0x4 hi
rtt_nvwx $nm MID 0x6 mid,hi
rtt_nvwx $nm LOW 0x7 low,mid,hi
rtt_nvwx $nm -HI 0x3 -hi,low,mid
rtt_nvwx $nm -MID 0x1 -mid,low
rtt_nvwx $nm -LOW 0x0 -low

# multi-bit update
rtt_nvwx $nm +LOW,MID,HI 0x7 3-on

# same-bit, multi-toggle (for exersize)
rtt_nvwx $nm -LOW,+LOW,-LOW,LOW,-LOW,LOW 0x7 on-off-repeat
}
trt_disjoint_names() {
trx_disjoint_names T_disjoint_names
}
trp_disjoint_names() {
trx_disjoint_names p_disjoint_names
}

# traditional bitmap
trx_disjoint_bits() {
nm=$1
echo 0 > $SMTDDP/$nm
rtt_nvwx $nm 0x0 0x0 no change
rtt_nvwx $nm 0x1 0x1 disjoint_CORE
rtt_nvwx $nm 0x2 0x2 disjoint_DRIVER,
rtt_nvwx $nm 0x4 0x4 disjoint_KMS,
rtt_nvwx $nm 0x10 0x10 disjoint_PRIME,
rtt_nvwx $nm 0x20 0x20 disjoint_ATOMIC,
rtt_nvwx $nm 0x40 0x40 disjoint_VBL,
rtt_nvwx $nm 0x80 0x80 disjoint_STATE,
rtt_nvwx $nm 0x100 0x100 disjoint_LEASE,

rtt_nvwx $nm 0x4f 0x4f disjoint_VBL and below
}
trt_disjoint_bits() {
trx_disjoint_bits T_disjoint_bits
}
trp_disjoint_bits() {
trx_disjoint_bits p_disjoint_bits
}

# traditional numeric verbose
trx_level_num() {
nm=$1
echo 0 > $SMTDDP/$nm
rtt_nvwx $nm 0 0 clear all
rtt_nvwx $nm 1 1 level 1
rtt_nvwx $nm 2 2 levels 2,1
rtt_nvwx $nm 3 3 levels 3,2,1
rtt_nvwx $nm 4 4 levels 4-1
rtt_nvwx $nm 5 5 levels 5-1
rtt_nvwx $nm 6 6 levels 6-1
rtt_nvwx $nm 7 7 levels 7-1
rtt_nvwx $nm 2 2 levels 2,1
}
trt_level_num() {
trx_level_num T_level_num
}
trp_level_num() {
trx_level_num p_level_num
}

# levels
trx_level_names() {
nm=$1
rtt_nvwx $nm -L0 0 clear all
rtt_nvwx $nm L0 0 clear all
rtt_nvwx $nm L1 1 level 1
rtt_nvwx $nm L2 2 levels 2,1
rtt_nvwx $nm L3 3 levels 3-1
rtt_nvwx $nm L4 4 levels 4-1
rtt_nvwx $nm L5 5 levels 5-1
rtt_nvwx $nm L7 7 levels 7-1
}
trt_level_names() {
trx_level_names T_level_names
}
trp_level_names() {
trx_level_names p_level_names
}

# these are more like exersize
#tddm_syslog_classes_1
#tddm_syslog_classes_2
#tddm_syslog_prio
#tddm_syslog_verbose


trt_all() {
# 1st, cuz its got base = 0, simplest
trt_disjoint_bits
trt_disjoint_names
trt_level_names
trt_level_num
}
trp_all() {
# 1st, cuz its got base = 0, simplest
trp_disjoint_bits
trp_disjoint_names
trp_level_names
trp_level_num
}

init () {
modprobe test_dynamic_debug
smtddp
echo 0 > p_disjoint_names
echo 0 > p_level_names
echo 0 > p_level_num
echo 0 > p_disjoint_bits
}
init_ () {
echo 0 > T_disjoint_names
echo 0 > T_level_names
echo 0 > T_level_num
echo 0 > T_disjoint_bits
}

alias refresh="cd -; . test-funcs.rc; cd -"


# need tests for
# p_disjoint_bits
# p_disjoint_names
# p_level_num

submod () {
# set dyndbg-params at load
echo mp test_dynamic_debug p_disjoint_bits=${1:-0} p_level_num=${2:-0}
modprobe test_dynamic_debug p_disjoint_bits=${1:-0} p_level_num=${2:-0} dyndbg=+p
modprobe test_dynamic_debug_submod dyndbg=+p
}
unmod () {
rmmod test_dynamic_debug_submod
rmmod test_dynamic_debug
}

ddgrep () {
grep $1 /proc/dynamic_debug/control
}


# these dont use bitmap interface:
# so no levels behavior is available here


VX="V0 V1 V2 V3 V4 V5 V6"

for_X () {
flag=$1
classes=$2
for C in $classes; do
ddc_grep class:$C
qry_cmd class $C $flag
ddc_grep class:$C
done
}
for_Vn () {
flag=$1
for C in V0 V1 V2 V3 V4 V5 V6; do
ddgrep class:$C
qry_cmd class $C $flag
ddgrep class:$C
done
}

for_D2 () {
flag=$1
for C in D2_CORE D2_DRIVER D2_KMS D2_PRIME D2_ATOMIC D2_VBL D2_STATE D2_LEASE D2_DP D2_DRMRES; do
ddgrep class:$C
qry_cmd class $C $flag
ddgrep class:$C
done
}

toggle_D2 () {
for_D2 -p
for_D2 +p
}

toggle_Vn () {
for_Vn +p
for_Vn -p
}


submod_test () {
vx 4
unmod
submod 0x3ff 3

ddgrep _submod
echo 1 > /sys/module/test_dynamic_debug/parameters/do_prints
echo 1 > /sys/module/test_dynamic_debug_submod/parameters/do_prints

echo 0 > /sys/module/test_dynamic_debug/parameters/p_disjoint_bits
echo 0x2ff > /sys/module/test_dynamic_debug/parameters/p_disjoint_bits

echo 1 > /sys/module/test_dynamic_debug/parameters/do_prints
echo 1 > /sys/module/test_dynamic_debug_submod/parameters/do_prints
}


Jim Cromie (20):
test-dyndbg: fixup CLASSMAP usage error
test-dyndbg: show that DEBUG enables prdbgs at compiletime
dyndbg: replace classmap list with a vector
dyndbg: make ddebug_apply_class_bitmap more selective
dyndbg: split param_set_dyndbg_classes to inner/outer
dyndbg: drop NUM_TYPE_ARRAY
dyndbg: reduce verbose/debug clutter
dyndbg: tighten ddebug_class_name() 1st arg
dyndbg: constify ddebug_apply_class_bitmap args
dyndbg-API: split DECLARE_(DYNDBG_CLASSMAP) to $1(_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
test-dyndbg: build test_dynamic_debug_submod
test-dyndbg: rename DD_SYS_WRAP to DYNDBG_CLASSMAP_PARAM
test-dyndbg: disable WIP dyndbg-trace params
test-dyndbg: tune sub-module behavior
dyndbg: unwrap __ddebug_add_module inner function NOTYET
jump_label: RFC - tolerate toggled state

arch/x86/kernel/jump_label.c | 26 ++-
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/Makefile | 3 +-
lib/dynamic_debug.c | 268 +++++++++++++++++-------
lib/test_dynamic_debug.c | 116 ++++++----
lib/test_dynamic_debug_submod.c | 10 +
16 files changed, 437 insertions(+), 200 deletions(-)
create mode 100644 include/linux/map.h
create mode 100644 lib/test_dynamic_debug_submod.c

--
2.39.0

2023-01-13 19:47:12

by Jim Cromie

Subject: [PATCH v2 07/20] dyndbg: reduce verbose/debug clutter

currently, for verbose=3, this is logged:

dyndbg: query 0: "class DRM_UT_CORE +p" mod:*
dyndbg: split into words: "class" "DRM_UT_CORE" "+p"

dyndbg: op='+'
dyndbg: flags=0x1
dyndbg: *flagsp=0x1 *maskp=0xffffffff

dyndbg: parsed: func="" file="" module="" format="" lineno=0-0 class=DRM_UT_CORE
dyndbg: no matches for query
dyndbg: no-match: func="" file="" module="" format="" lineno=0-0 class=DRM_UT_CORE
dyndbg: processed 1 queries, with 0 matches, 0 errs

This patch:

shrinks 3 lines of 2nd stanza to single line

drops 2 middle lines of 3rd stanza
3 differs from 1 only by status
2 is just status, retold in 4, with more info.

Signed-off-by: Jim Cromie <[email protected]>
---
lib/dynamic_debug.c | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 0a5efc735b36..2d4640479e5b 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -265,9 +265,6 @@ static int ddebug_change(const struct ddebug_query *query,
}
mutex_unlock(&ddebug_lock);

- if (!nfound && verbose)
- pr_info("no matches for query\n");
-
return nfound;
}

@@ -496,7 +493,6 @@ static int ddebug_parse_flags(const char *str, struct flag_settings *modifiers)
pr_err("bad flag-op %c, at start of %s\n", *str, str);
return -EINVAL;
}
- v3pr_info("op='%c'\n", op);

for (; *str ; ++str) {
for (i = ARRAY_SIZE(opt_array) - 1; i >= 0; i--) {
@@ -510,7 +506,6 @@ static int ddebug_parse_flags(const char *str, struct flag_settings *modifiers)
return -EINVAL;
}
}
- v3pr_info("flags=0x%x\n", modifiers->flags);

/* calculate final flags, mask based upon op */
switch (op) {
@@ -526,7 +521,7 @@ static int ddebug_parse_flags(const char *str, struct flag_settings *modifiers)
modifiers->flags = 0;
break;
}
- v3pr_info("*flagsp=0x%x *maskp=0x%x\n", modifiers->flags, modifiers->mask);
+ v3pr_info("op='%c' flags=0x%x maskp=0x%x\n", op, modifiers->flags, modifiers->mask);

return 0;
}
@@ -536,7 +531,7 @@ static int ddebug_exec_query(char *query_string, const char *modname)
struct flag_settings modifiers = {};
struct ddebug_query query = {};
#define MAXWORDS 9
- int nwords, nfound;
+ int nwords;
char *words[MAXWORDS];

nwords = ddebug_tokenize(query_string, words, MAXWORDS);
@@ -554,10 +549,7 @@ static int ddebug_exec_query(char *query_string, const char *modname)
return -EINVAL;
}
/* actually go and implement the change */
- nfound = ddebug_change(&query, &modifiers);
- vpr_info_dq(&query, nfound ? "applied" : "no-match");
-
- return nfound;
+ return ddebug_change(&query, &modifiers);
}

/* handle multiple queries in query string, continue on error, return
--
2.39.0

2023-01-13 20:15:28

by Jim Cromie

Subject: [PATCH v2 15/20] test-dyndbg: build test_dynamic_debug_submod

CONFIG_DRM_USE_DYNAMIC_DEBUG=y has a regression; drm subsystem
modules, which depend upon drm.ko and use the drm.debug API, do not
get enabled when __drm_debug is set by `modprobe drm debug=0x1f`.

With =N, __drm_debug is checked before logging the msg, so the
end-of-modprobe debug=$init affected all later checks. But with =y,
each run-time check is replaced by a static-key that is set at
end-of-modprobe.

This creates a chicken-egg dependency; i915 must be modprobed before
its drm.debugs are enabled, but drm.ko (and __drm_debug=$init) must be
done before modprobe i915, so its callsites arent there yet to be
enabled.

The WIP-fix is to split DECLARE_DYNDBG_CLASSMAP to:

DYNDBG_CLASSMAP_DEFINE - invoked in 'parent'
DYNDBG_CLASSMAP_USE - invoked in dependent, to USE the exported definition

To prove the fix w/o involving DRM, we need 2 modules, one dependent
on the other. Add test_dynamic_debug_submod.ko, which _USEs the
classmaps _exported by test_dynamic_debug.ko

To keep code to a minimum, test_dynamic_debug.c ifdefs on
TEST_DYNAMIC_DEBUG_SUBMOD to build either parent or dependent, with
either DYNDBG_CLASSMAP_DEFINE or DYNDBG_CLASSMAP_USE blocks.

So test_dynamic_debug_submod.c is just 2 lines: include parent after
defining SUBMOD. This also gives the 2 modules identical logging
behavior as a baseline.

Signed-off-by: Jim Cromie <[email protected]>
---
lib/Makefile | 3 +-
lib/test_dynamic_debug.c | 50 ++++++++++++++++++++++++++++-----
lib/test_dynamic_debug_submod.c | 10 +++++++
3 files changed, 55 insertions(+), 8 deletions(-)
create mode 100644 lib/test_dynamic_debug_submod.c

diff --git a/lib/Makefile b/lib/Makefile
index 59bd7c2f793a..0f04c59bdc8e 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -78,7 +78,7 @@ obj-$(CONFIG_TEST_SORT) += test_sort.o
obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
-obj-$(CONFIG_TEST_DYNAMIC_DEBUG) += test_dynamic_debug.o
+obj-$(CONFIG_TEST_DYNAMIC_DEBUG) += test_dynamic_debug.o test_dynamic_debug_submod.o
obj-$(CONFIG_TEST_PRINTF) += test_printf.o
obj-$(CONFIG_TEST_SCANF) += test_scanf.o
obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o
@@ -100,6 +100,7 @@ obj-$(CONFIG_KPROBES_SANITY_TEST) += test_kprobes.o
obj-$(CONFIG_TEST_REF_TRACKER) += test_ref_tracker.o
CFLAGS_test_fprobe.o += $(CC_FLAGS_FTRACE)
obj-$(CONFIG_FPROBE_SANITY_TEST) += test_fprobe.o
+
#
# CFLAGS for compiling floating point code inside the kernel. x86/Makefile turns
# off the generation of FPU/SSE* instructions for kernel proper but FPU_FLAGS
diff --git a/lib/test_dynamic_debug.c b/lib/test_dynamic_debug.c
index e678884066bf..39d4f63cade1 100644
--- a/lib/test_dynamic_debug.c
+++ b/lib/test_dynamic_debug.c
@@ -6,7 +6,11 @@
* Jim Cromie <[email protected]>
*/

-#define pr_fmt(fmt) "test_dd: " fmt
+#if defined(TEST_DYNAMIC_DEBUG_SUBMOD)
+ #define pr_fmt(fmt) "test_dd_submod: " fmt
+#else
+ #define pr_fmt(fmt) "test_dd: " fmt
+#endif

#define DEBUG /* enable all prdbgs (plain & class'd) at compiletime */

@@ -49,6 +53,13 @@ module_param_cb(do_prints, &param_ops_do_prints, NULL, 0600);
}; \
module_param_cb(_flags##_##_model, &param_ops_dyndbg_classes, &_flags##_model, 0600)

+/*
+ * dynamic-debug imitates drm.debug model's use of enums (DRM_UT_CORE
+ * etc) to define it's classes/categories. dyndbg allows class-id's
+ * 0..62, reserving 63 for plain old (non-class'd) prdbgs. A module
+ * can define multiple classmaps, as long as they share the range.
+ */
+
/* numeric input, independent bits */
enum cat_disjoint_bits {
D2_CORE = 0,
@@ -61,7 +72,36 @@ enum cat_disjoint_bits {
D2_LEASE,
D2_DP,
D2_DRMRES };
+
+/* symbolic input, independent bits */
+enum cat_disjoint_names { LOW = 10, MID, HI };
+
+/* numeric verbosity, V2 > V1 related */
+enum cat_level_num { V0 = 14, V1, V2, V3, V4, V5, V6, V7 };
+
+/* symbolic verbosity */
+enum cat_level_names { L0 = 22, L1, L2, L3, L4, L5, L6, L7 };
+
+#if defined(TEST_DYNAMIC_DEBUG_SUBMOD)
+
+/* use the classmaps defined in 'parent' module below */
+DYNDBG_CLASSMAP_USE(map_disjoint_bits);
+DYNDBG_CLASSMAP_USE(map_disjoint_names);
+DYNDBG_CLASSMAP_USE(map_level_num);
+DYNDBG_CLASSMAP_USE(map_level_names);
+
+#else
+
+/*
+ * parent module, define a classmap of each of 4 types.
+ * enum values are class-ids
+ * enum symbols are stringified, used as classnames
+ * param bits are mapped in order: 0..N
+ * (a straight, obvious, linear map is encouraged)
+ */
+
DYNDBG_CLASSMAP_DEFINE(map_disjoint_bits, DD_CLASS_TYPE_DISJOINT_BITS,
+ /* bits 0..N of param are mapped to these class-ids */
D2_CORE,
D2_DRIVER,
D2_KMS,
@@ -75,27 +115,23 @@ DYNDBG_CLASSMAP_DEFINE(map_disjoint_bits, DD_CLASS_TYPE_DISJOINT_BITS,
DD_SYS_WRAP(disjoint_bits, p);
DD_SYS_WRAP(disjoint_bits, T);

-/* symbolic input, independent bits */
-enum cat_disjoint_names { LOW = 10, MID, HI };
DYNDBG_CLASSMAP_DEFINE(map_disjoint_names, DD_CLASS_TYPE_DISJOINT_NAMES,
LOW, MID, HI);
DD_SYS_WRAP(disjoint_names, p);
DD_SYS_WRAP(disjoint_names, T);

-/* numeric verbosity, V2 > V1 related */
-enum cat_level_num { V0 = 14, V1, V2, V3, V4, V5, V6, V7 };
DYNDBG_CLASSMAP_DEFINE(map_level_num, DD_CLASS_TYPE_LEVEL_NUM,
V0, V1, V2, V3, V4, V5, V6, V7);
DD_SYS_WRAP(level_num, p);
DD_SYS_WRAP(level_num, T);

-/* symbolic verbosity */
-enum cat_level_names { L0 = 22, L1, L2, L3, L4, L5, L6, L7 };
DYNDBG_CLASSMAP_DEFINE(map_level_names, DD_CLASS_TYPE_LEVEL_NAMES,
L0, L1, L2, L3, L4, L5, L6, L7);
DD_SYS_WRAP(level_names, p);
DD_SYS_WRAP(level_names, T);

+#endif /* TEST_DYNAMIC_DEBUG_SUBMOD */
+
/* stand-in for all pr_debug etc */
#define prdbg(SYM) __pr_debug_cls(SYM, #SYM " msg\n")

diff --git a/lib/test_dynamic_debug_submod.c b/lib/test_dynamic_debug_submod.c
new file mode 100644
index 000000000000..f4ee7a6d6989
--- /dev/null
+++ b/lib/test_dynamic_debug_submod.c
@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Kernel module for testing dynamic_debug
+ *
+ * Authors:
+ * Jim Cromie <[email protected]>
+ */
+
+#define TEST_DYNAMIC_DEBUG_SUBMOD
+#include "test_dynamic_debug.c"
--
2.39.0

2023-01-13 20:19:16

by Jim Cromie

Subject: [PATCH v2 13/20] dyndbg-API: DYNDBG_CLASSMAP_DEFINE() improvements

patch 1 in this series fixed a CLASSMAP usage error, this improves the
api so that misuse is less likely.

changes here:

0- Add William Swanson's public domain map macro:
https://github.com/swansontec/map-macro/blob/master/map.h
this makes 1 possible.

1- classnames were formerly specified as strings: "DRM_UT_CORE"
now they are the actual enum const symbols: DRM_UT_CORE
direct use of symbols is tighter, more comprehensible by tools, grep

2- drop _base arg.
_base was the value of the 1st classname
that is now available due to 1, no need to require it 2x

So take _base out of the API/kdoc. Note that the macro impl keeps the
_base arg so that it can be used to set classmap.base, but reuses it
in the MAP-stringify _base, __VA_ARGS__ expression.

Also cleanup the API usage comment in test_dynamic_debug.c, and since
comments in test-code might not be noticed, restate that here.

Using the CLASSMAP api:

- class-specifications are enum consts/symbols,
like DRM_UT_CORE, DRM_UT_KMS, etc.
their values define bits in the sysfs-node (like drm.debug)

- they are stringified and accepted at >control
echo class DRM_UT_CORE +p >control

- multiple class-maps must share the per-module: 0-62 class_id space
(by setting initial enum values to non-overlapping subranges)

todo: fixup the 'i' prefix, a quick/dirty avoidance of MAP.

NOTE: test_dynamic_debug.c also has this helper macro to wire a
classmap to a drm.debug style parameter; its easier to just use it as
a model/template as needed, rather than try to make it general enough
to be an official API helper.

define DD_SYS_WRAP(_model, _flags) \
static unsigned long bits_##_model; \
static struct ddebug_class_param _flags##_model = { \
.bits = &bits_##_model, \
.flags = #_flags, \
.map = &map_##_model, \
}; \
module_param_cb(_flags##_##_model, &param_ops_dyndbg_classes, &_flags##_model, 0600)

Signed-off-by: Jim Cromie <[email protected]>
---
drivers/gpu/drm/drm_print.c | 22 +++++++-------
include/drm/drm_print.h | 1 +
include/linux/dynamic_debug.h | 17 ++++++-----
include/linux/map.h | 54 +++++++++++++++++++++++++++++++++++
lib/test_dynamic_debug.c | 43 ++++++++++++++--------------
5 files changed, 95 insertions(+), 42 deletions(-)
create mode 100644 include/linux/map.h

diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index 4b697e18238d..07c25241e8cc 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -56,17 +56,17 @@ MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug cat
module_param_named(debug, __drm_debug, ulong, 0600);
#else
/* classnames must match vals of enum drm_debug_category */
-DYNDBG_CLASSMAP_DEFINE(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
- "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");
+DYNDBG_CLASSMAP_DEFINE(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS,
+ 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);

static struct ddebug_class_param drm_debug_bitmap = {
.bits = &__drm_debug,
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index a44fb7ef257f..6a27e8f26770 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -333,6 +333,7 @@ static inline bool drm_debug_enabled_raw(enum drm_debug_category category)
})

#if defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
+//extern struct ddebug_class_map drm_debug_classes[];
/*
* the drm.debug API uses dyndbg, so each drm_*dbg macro/callsite gets
* a descriptor, and only enabled callsites are reachable. They use
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 91015d1a04e0..7cdfc4b533ae 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -7,6 +7,7 @@
#endif

#include <linux/build_bug.h>
+#include <linux/map.h>

/*
* An instance of this structure is created in a special
@@ -90,18 +91,16 @@ struct ddebug_class_map {
};

/**
- * DYNDBG_CLASSMAP_DEFINE - define debug-classes used by a module.
- * @_var: name of the classmap, exported for other modules coordinated use.
- * @_type: enum class_map_type, chooses bits/verbose, numeric/symbolic
- * @_base: offset of 1st class-name. splits .class_id space
- * @classes: enum-map - symbol names are "classnames", vals are .class_ids
+ * DYNDBG_CLASSMAP_DEFINE - define the debug classes used in this module.
+ * This tells dyndbg what debug classes it should control for the client.
*
- * @classes vals are _ddebug.class_ids used in the module, the symbol
- * names are stringified; they authorize "class FOO" to >control.
- * Connection to a kernel-param is done separately.
+ * @_var: struct ddebug_class_map, as passed to module_param_cb
+ * @_type: enum ddebug_class_map_type, chooses bits/verbose, numeric/symbolic
+ * @classes: enum class values used in module, such as: DRM_UT_*
*/
#define DYNDBG_CLASSMAP_DEFINE(_var, _maptype, _base, ...) \
- const char *_var##_classnames[] = { __VA_ARGS__ }; \
+ const char *_var##_classnames[] = { \
+ iMAP_LIST(__stringify, _base, __VA_ARGS__) }; \
struct ddebug_class_map __aligned(8) __used \
__section("__dyndbg_classes") _var = { \
.mod = THIS_MODULE, \
diff --git a/include/linux/map.h b/include/linux/map.h
new file mode 100644
index 000000000000..4348313a596f
--- /dev/null
+++ b/include/linux/map.h
@@ -0,0 +1,54 @@
+/*
+ * Created by William Swanson in 2012.
+ *
+ * I, William Swanson, dedicate this work to the public domain.
+ * I waive all rights to the work worldwide under copyright law,
+ * including all related and neighboring rights,
+ * to the extent allowed by law.
+ *
+ * You can copy, modify, distribute and perform the work,
+ * even for commercial purposes, all without asking permission.
+ */
+
+#ifndef MAP_H_INCLUDED
+#define MAP_H_INCLUDED
+
+#define iEVAL0(...) __VA_ARGS__
+#define iEVAL1(...) iEVAL0(iEVAL0(iEVAL0(__VA_ARGS__)))
+#define iEVAL2(...) iEVAL1(iEVAL1(iEVAL1(__VA_ARGS__)))
+#define iEVAL3(...) iEVAL2(iEVAL2(iEVAL2(__VA_ARGS__)))
+#define iEVAL4(...) iEVAL3(iEVAL3(iEVAL3(__VA_ARGS__)))
+#define iEVAL(...) iEVAL4(iEVAL4(iEVAL4(__VA_ARGS__)))
+
+#define iMAP_END(...)
+#define iMAP_OUT
+#define iMAP_COMMA ,
+
+#define iMAP_GET_END2() 0, iMAP_END
+#define iMAP_GET_END1(...) iMAP_GET_END2
+#define iMAP_GET_END(...) iMAP_GET_END1
+#define iMAP_NEXT0(test, next, ...) next iMAP_OUT
+#define iMAP_NEXT1(test, next) iMAP_NEXT0(test, next, 0)
+#define iMAP_NEXT(test, next) iMAP_NEXT1(iMAP_GET_END test, next)
+
+#define iMAP0(f, x, peek, ...) f(x) iMAP_NEXT(peek, iMAP1)(f, peek, __VA_ARGS__)
+#define iMAP1(f, x, peek, ...) f(x) iMAP_NEXT(peek, iMAP0)(f, peek, __VA_ARGS__)
+
+#define iMAP_LIST_NEXT1(test, next) iMAP_NEXT0(test, iMAP_COMMA next, 0)
+#define iMAP_LIST_NEXT(test, next) iMAP_LIST_NEXT1(iMAP_GET_END test, next)
+
+#define iMAP_LIST0(f, x, peek, ...) f(x) iMAP_LIST_NEXT(peek, iMAP_LIST1)(f, peek, __VA_ARGS__)
+#define iMAP_LIST1(f, x, peek, ...) f(x) iMAP_LIST_NEXT(peek, iMAP_LIST0)(f, peek, __VA_ARGS__)
+
+/**
+ * Applies the function macro `f` to each of the remaining parameters.
+ */
+#define iMAP(f, ...) iEVAL(iMAP1(f, __VA_ARGS__, ()()(), ()()(), ()()(), 0))
+
+/**
+ * Applies the function macro `f` to each of the remaining parameters and
+ * inserts commas between the results.
+ */
+#define iMAP_LIST(f, ...) iEVAL(iMAP_LIST1(f, __VA_ARGS__, ()()(), ()()(), ()()(), 0))
+
+#endif
diff --git a/lib/test_dynamic_debug.c b/lib/test_dynamic_debug.c
index 8d384b979e74..e678884066bf 100644
--- a/lib/test_dynamic_debug.c
+++ b/lib/test_dynamic_debug.c
@@ -33,11 +33,10 @@ module_param_cb(do_prints, &param_ops_do_prints, NULL, 0600);

/*
* Using the CLASSMAP api:
- * - classmaps must have corresponding enum
- * - enum symbols must match/correlate with class-name strings in the map.
- * - base must equal enum's 1st value
- * - multiple maps must set their base to share the 0-30 class_id space !!
- * (build-bug-on tips welcome)
+ * - class-names are enum consts/symbols, like DRM_UT_CORE, DRM_UT_KMS, etc
+ * - those names are accepted at >control interface
+ * - multiple class-maps must share the per-module: 0-62 class_id space
+ * (by setting initial enum values to non-overlapping subranges)
* Additionally, here:
* - tie together sysname, mapname, bitsname, flagsname
*/
@@ -62,38 +61,38 @@ enum cat_disjoint_bits {
D2_LEASE,
D2_DP,
D2_DRMRES };
-DYNDBG_CLASSMAP_DEFINE(map_disjoint_bits, DD_CLASS_TYPE_DISJOINT_BITS, 0,
- "D2_CORE",
- "D2_DRIVER",
- "D2_KMS",
- "D2_PRIME",
- "D2_ATOMIC",
- "D2_VBL",
- "D2_STATE",
- "D2_LEASE",
- "D2_DP",
- "D2_DRMRES");
+DYNDBG_CLASSMAP_DEFINE(map_disjoint_bits, DD_CLASS_TYPE_DISJOINT_BITS,
+ D2_CORE,
+ D2_DRIVER,
+ D2_KMS,
+ D2_PRIME,
+ D2_ATOMIC,
+ D2_VBL,
+ D2_STATE,
+ D2_LEASE,
+ D2_DP,
+ D2_DRMRES);
DD_SYS_WRAP(disjoint_bits, p);
DD_SYS_WRAP(disjoint_bits, T);

/* symbolic input, independent bits */
enum cat_disjoint_names { LOW = 10, MID, HI };
-DYNDBG_CLASSMAP_DEFINE(map_disjoint_names, DD_CLASS_TYPE_DISJOINT_NAMES, 10,
- "LOW", "MID", "HI");
+DYNDBG_CLASSMAP_DEFINE(map_disjoint_names, DD_CLASS_TYPE_DISJOINT_NAMES,
+ LOW, MID, HI);
DD_SYS_WRAP(disjoint_names, p);
DD_SYS_WRAP(disjoint_names, T);

/* numeric verbosity, V2 > V1 related */
enum cat_level_num { V0 = 14, V1, V2, V3, V4, V5, V6, V7 };
-DYNDBG_CLASSMAP_DEFINE(map_level_num, DD_CLASS_TYPE_LEVEL_NUM, 14,
- "V0", "V1", "V2", "V3", "V4", "V5", "V6", "V7");
+DYNDBG_CLASSMAP_DEFINE(map_level_num, DD_CLASS_TYPE_LEVEL_NUM,
+ V0, V1, V2, V3, V4, V5, V6, V7);
DD_SYS_WRAP(level_num, p);
DD_SYS_WRAP(level_num, T);

/* symbolic verbosity */
enum cat_level_names { L0 = 22, L1, L2, L3, L4, L5, L6, L7 };
-DYNDBG_CLASSMAP_DEFINE(map_level_names, DD_CLASS_TYPE_LEVEL_NAMES, 22,
- "L0", "L1", "L2", "L3", "L4", "L5", "L6", "L7");
+DYNDBG_CLASSMAP_DEFINE(map_level_names, DD_CLASS_TYPE_LEVEL_NAMES,
+ L0, L1, L2, L3, L4, L5, L6, L7);
DD_SYS_WRAP(level_names, p);
DD_SYS_WRAP(level_names, T);

--
2.39.0

2023-01-13 20:19:22

by Jim Cromie

Subject: [PATCH v2 20/20] jump_label: RFC - tolerate toggled state

__jump_label_patch currently will "crash the box" if it finds a
jump_entry not as expected. ISTM this overly harsh; it doesn't
distinguish between "alternate/opposite" state, and truly
"insane/corrupted".

The "opposite" (but well-formed) state is a milder mis-initialization
problem, and some less severe mitigation seems practical. ATM this
just warns about it; a range/enum of outcomes: warn, crash, silence,
ok, fixup-continue, etc, are possible on a case-by-case basis.

Ive managed to create this mis-initialization condition with
test_dynamic_debug.ko & _submod.ko. These replicate DRM's regression
on DRM_USE_DYNAMIC_DEBUG=y; drm.debug callsites in drivers/helpers
(dependent modules) are not enabled along with those in drm.ko itself.

Heres that "opposite" state, occurring:

:#> echo 1 > /sys/module/test_dynamic_debug/parameters/p_disjoint_bits
[ 235.258452] dyndbg: bits:0x1 > *.p_disjoint_bits
[ 235.258908] dyndbg: apply bitmap: 0x1 to: 0x4f for '*'
[ 235.259351] dyndbg: query 0: "class D2_DRIVER -p" mod:*
[ 235.259799] dyndbg: split into words: "class" "D2_DRIVER" "-p"
[ 235.260290] dyndbg: op='-' flags=0x0 maskp=0xfffffffe
[ 235.260720] dyndbg: parsed: func="" file="" module="" format="" lineno=0-0 class=D2_DRIVER
[ 235.261418] dyndbg: good-class: test_dynamic_debug.D2_DRIVER module: test_dynamic_debug nd: 32 nc: 4 nu: 0
[ 235.262276] dyndbg: class-ref: test_dynamic_debug_submod.D2_DRIVER module: test_dynamic_debug_submod nd: 32 nc: 0 nu: 4
[ 235.263178] jump_label: found alternate op at do_cats+0x16/0x180 [test_dynamic_debug_submod] [00000000ba944cc2] (0f 1f 44 00 00 != e9 e1 00 00 00)) size:5 type:0
[ 235.264431] dyndbg: processed 1 queries, with 2 matches, 0 errs
[ 235.264951] dyndbg: bit_1: 2 matches on class: D2_DRIVER -> 0x1
[ 235.265444] dyndbg: query 0: "class D2_KMS -p" mod:*
[ 235.265869] dyndbg: split into words: "class" "D2_KMS" "-p"
[ 235.266337] dyndbg: op='-' flags=0x0 maskp=0xfffffffe
[ 235.266767] dyndbg: parsed: func="" file="" module="" format="" lineno=0-0 class=D2_KMS
[ 235.267432] dyndbg: good-class: test_dynamic_debug.D2_KMS module: test_dynamic_debug nd: 32 nc: 4 nu: 0
[ 235.268264] dyndbg: class-ref: test_dynamic_debug_submod.D2_KMS module: test_dynamic_debug_submod nd: 32 nc: 0 nu: 4
[ 235.269142] jump_label: found alternate op at do_cats+0x1b/0x180 [test_dynamic_debug_submod] [00000000e8d6c160] (0f 1f 44 00 00 != e9 c4 00 00 00)) size:5 type:0
[ 235.270384] dyndbg: processed 1 queries, with 2 matches, 0 errs

RFC:

Ive hit this case a few times, but havent been able to isolate the
when and why.

warn-only is something of a punt, and I'm still left with remaining
bugs which are likely related; I'm able to toggle the p-flag on
callsites in the submod, but their enablement still doesn't yield
logging activity.

CC: Jason Baron <[email protected]>
CC: Peter Zijlstra <[email protected]>
Signed-off-by: Jim Cromie <[email protected]>
---

heres the Fatal kernel bug: (its on a -dirty kernel)

[ 2831.639643] dyndbg: good-class: test_dynamic_debug.D2_CORE module: test_dynamic_debug nd: 32 nc: 4 nu: 0
[ 2831.641316] dyndbg: class-ref: test_dynamic_debug_submod.D2_CORE module: test_dynamic_debug_submod nd: 32 nc: 0 nu: 4
[ 2831.642048] jump_label: Fatal kernel bug, unexpected op at do_cats+0x11/0x180 [test_dynamic_debug_submod] [00000000fa724232] (0f 1f 44 00 00 != e9 fe 00 00 00)) size:5 type:0
[ 2831.643077] ------------[ cut here ]------------
[ 2831.643537] kernel BUG at arch/x86/kernel/jump_label.c:73!
[ 2831.643902] invalid opcode: 0000 [#1] PREEMPT SMP KASAN
[ 2831.644248] CPU: 1 PID: 425 Comm: bash Tainted: G E 6.1.0-tf2-00021-gbe37efe8728f-dirty #696
[ 2831.644862] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.1-2.fc37 04/01/2014
[ 2831.645431] RIP: 0010:__jump_label_patch.cold+0x24/0x26
[ 2831.645773] Code: 83 e9 ef 00 ac fe 48 c7 c3 20 4c 5c 85 41 56 45 89 f9 49 89 d8 4c 89 e9 4c 89 ea 4c 89 ee 48 c7 c7 00 70 c5 82 e8 d7 1a 01 00 <0f> 0b 41 54 45 31 e4 55 48 89 fd 53 bb 00 00 00 80 48 81 c3 00 20
[ 2831.646896] RSP: 0018:ffffc9000101f738 EFLAGS: 00010282
[ 2831.647242] RAX: 00000000000000a2 RBX: ffffffff855c4c20 RCX: 0000000000000000
[ 2831.647702] RDX: 00000000000000a2 RSI: ffffffff81294fc4 RDI: fffff52000203ede
[ 2831.648155] RBP: ffffc9000101f778 R08: 0000000000000003 R09: ffffc9000101f4ff
[ 2831.648626] R10: fffff52000203e9f R11: 78656e75202c756a R12: ffffffff855c4c20
[ 2831.649096] R13: ffffffffc0250011 R14: 0000000000000000 R15: 0000000000000005
[ 2831.649498] FS: 00007fe4014aa740(0000) GS:ffff88805ae00000(0000) knlGS:0000000000000000
[ 2831.649894] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 2831.650175] CR2: 00007f9c326e6c08 CR3: 000000000ff59000 CR4: 0000000000750ee0
[ 2831.650520] PKRU: 55555554
[ 2831.650667] Call Trace:
[ 2831.650793] <TASK>
[ 2831.650903] ? do_cats+0x11/0x180 [test_dynamic_debug_submod]
[ 2831.651195] arch_jump_label_transform_queue+0x43/0xa0
[ 2831.651450] __jump_label_update+0x9b/0x1b0
[ 2831.651670] static_key_disable_cpuslocked+0x9f/0xd0
[ 2831.651917] static_key_disable+0x16/0x20
[ 2831.652117] ddebug_change+0x45d/0x5c0
---
arch/x86/kernel/jump_label.c | 26 +++++++++++++++++---------
1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c
index f5b8ef02d172..9175f7e8c267 100644
--- a/arch/x86/kernel/jump_label.c
+++ b/arch/x86/kernel/jump_label.c
@@ -35,7 +35,7 @@ struct jump_label_patch {
static struct jump_label_patch
__jump_label_patch(struct jump_entry *entry, enum jump_label_type type)
{
- const void *expect, *code, *nop;
+ const void *expect, *code, *nop, *alt;
const void *addr, *dest;
int size;

@@ -57,20 +57,28 @@ __jump_label_patch(struct jump_entry *entry, enum jump_label_type type)
default: BUG();
}

- if (type == JUMP_LABEL_JMP)
+ if (type == JUMP_LABEL_JMP) {
expect = nop;
- else
+ alt = code;
+ } else {
expect = code;
-
+ alt = nop;
+ }
if (memcmp(addr, expect, size)) {
/*
- * The location is not an op that we were expecting.
- * Something went wrong. Crash the box, as something could be
- * corrupting the kernel.
+ * The location is not the op that we were expecting.
+ * If its the alternate/toggled op, then warn, otherwise
+ * something went more wrong. Crash the box, as
+ * something could be corrupting the kernel.
*/
- pr_crit("jump_label: Fatal kernel bug, unexpected op at %pS [%p] (%5ph != %5ph)) size:%d type:%d\n",
+ if (memcmp(addr, alt, size)) {
+ pr_crit("jump_label: Fatal kernel bug, unexpected op at %pS [%p] (%5ph != %5ph)) size:%d type:%d\n",
+ addr, addr, addr, expect, size, type);
+ BUG();
+ } else {
+ pr_warn("jump_label: found alternate op at %pS [%p] (%5ph != %5ph)) size:%d type:%d\n",
addr, addr, addr, expect, size, type);
- BUG();
+ }
}

if (type == JUMP_LABEL_NOP)
--
2.39.0

2023-01-13 20:22:15

by Jim Cromie

Subject: [PATCH v2 03/20] dyndbg: replace classmap list with a vector

Classmaps are stored/linked in a section/array, but are each added to
the module's ddebug_table.maps list-head.

This is unnecessary; even when ddebug_attach_classmap() is handling
the builtin section (with classmaps for multiple builtin modules), its
contents are ordered, so a module's possibly multiple classmaps will
be consecutive in the section, and could be treated as a vector/block,
since both start-addy and subrange length are in the ddebug_info arg.

So this changes:

struct ddebug_class_map drops list-head link.

struct ddebug_table drops the list-head maps, and gets: classes &
num_classes for the start-addy and num_classes, placed to improve
struct packing.

The loading: in ddebug_attach_module_classes(), replace the
for-the-modname list-add loop, with a forloop that finds the module's
subrange (start,length) of matching classmaps within the possibly
builtin classmaps vector, and saves those to the ddebug_table.

The reading/using: change list-foreach loops in ddebug_class_name() &
ddebug_find_valid_class() to walk the array from start to length.

Also:
Move #define __outvar up, above an added use in a fn-prototype.
Simplify ddebug_attach_module_classes args, ref has both addy,len.

This isn't technically a bugfix, but IMO simplifies later fixes for
the chicken-egg post-init enablement regression.

Signed-off-by: Jim Cromie <[email protected]>
---
include/linux/dynamic_debug.h | 1 -
lib/dynamic_debug.c | 61 ++++++++++++++++++-----------------
2 files changed, 32 insertions(+), 30 deletions(-)

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 41682278d2e8..bf47bcfad8e6 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -81,7 +81,6 @@ enum class_map_type {
};

struct ddebug_class_map {
- struct list_head link;
struct module *mod;
const char *mod_name; /* needed for builtins */
const char **class_names;
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 009f2ead09c1..823190094350 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -45,10 +45,11 @@ extern struct ddebug_class_map __start___dyndbg_classes[];
extern struct ddebug_class_map __stop___dyndbg_classes[];

struct ddebug_table {
- struct list_head link, maps;
+ struct list_head link;
const char *mod_name;
- unsigned int num_ddebugs;
struct _ddebug *ddebugs;
+ struct ddebug_class_map *classes;
+ unsigned int num_ddebugs, num_classes;
};

struct ddebug_query {
@@ -146,13 +147,15 @@ static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
query->first_lineno, query->last_lineno, query->class_string);
}

+#define __outvar /* filled by callee */
static struct ddebug_class_map *ddebug_find_valid_class(struct ddebug_table const *dt,
- const char *class_string, int *class_id)
+ const char *class_string,
+ __outvar int *class_id)
{
struct ddebug_class_map *map;
- int idx;
+ int i, idx;

- list_for_each_entry(map, &dt->maps, link) {
+ for (map = dt->classes, i = 0; i < dt->num_classes; i++, map++) {
idx = match_string(map->class_names, map->length, class_string);
if (idx >= 0) {
*class_id = idx + map->base;
@@ -163,7 +166,6 @@ static struct ddebug_class_map *ddebug_find_valid_class(struct ddebug_table cons
return NULL;
}

-#define __outvar /* filled by callee */
/*
* Search the tables for _ddebug's which match the given `query' and
* apply the `flags' and `mask' to them. Returns number of matching
@@ -1107,9 +1109,10 @@ static void *ddebug_proc_next(struct seq_file *m, void *p, loff_t *pos)

static const char *ddebug_class_name(struct ddebug_iter *iter, struct _ddebug *dp)
{
- struct ddebug_class_map *map;
+ struct ddebug_class_map *map = iter->table->classes;
+ int i, nc = iter->table->num_classes;

- list_for_each_entry(map, &iter->table->maps, link)
+ for (i = 0; i < nc; i++, map++)
if (class_in_range(dp->class_id, map))
return map->class_names[dp->class_id - map->base];

@@ -1193,30 +1196,31 @@ static const struct proc_ops proc_fops = {
.proc_write = ddebug_proc_write
};

-static void ddebug_attach_module_classes(struct ddebug_table *dt,
- struct ddebug_class_map *classes,
- int num_classes)
+static void ddebug_attach_module_classes(struct ddebug_table *dt, struct _ddebug_info *di)
{
struct ddebug_class_map *cm;
- int i, j, ct = 0;
+ int i, nc = 0;

- for (cm = classes, i = 0; i < num_classes; i++, cm++) {
+ /*
+ * Find this module's classmaps in a subrange/wholerange of
+ * the builtin/modular classmap vector/section. Save the start
+ * and length of the subrange at its edges.
+ */
+ for (cm = di->classes, i = 0; i < di->num_classes; i++, cm++) {

if (!strcmp(cm->mod_name, dt->mod_name)) {
-
- v2pr_info("class[%d]: module:%s base:%d len:%d ty:%d\n", i,
- cm->mod_name, cm->base, cm->length, cm->map_type);
-
- for (j = 0; j < cm->length; j++)
- v3pr_info(" %d: %d %s\n", j + cm->base, j,
- cm->class_names[j]);
-
- list_add(&cm->link, &dt->maps);
- ct++;
+ if (!nc) {
+ v2pr_info("start subrange, class[%d]: module:%s base:%d len:%d ty:%d\n",
+ i, cm->mod_name, cm->base, cm->length, cm->map_type);
+ dt->classes = cm;
+ }
+ nc++;
}
}
- if (ct)
- vpr_info("module:%s attached %d classes\n", dt->mod_name, ct);
+ if (nc) {
+ dt->num_classes = nc;
+ vpr_info("module:%s attached %d classes\n", dt->mod_name, nc);
+ }
}

/*
@@ -1250,10 +1254,9 @@ static int __ddebug_add_module(struct _ddebug_info *di, unsigned int base,
dt->num_ddebugs = di->num_descs;

INIT_LIST_HEAD(&dt->link);
- INIT_LIST_HEAD(&dt->maps);

if (di->classes && di->num_classes)
- ddebug_attach_module_classes(dt, di->classes, di->num_classes);
+ ddebug_attach_module_classes(dt, di);

mutex_lock(&ddebug_lock);
list_add_tail(&dt->link, &ddebug_tables);
@@ -1342,8 +1345,8 @@ static void ddebug_remove_all_tables(void)
mutex_lock(&ddebug_lock);
while (!list_empty(&ddebug_tables)) {
struct ddebug_table *dt = list_entry(ddebug_tables.next,
- struct ddebug_table,
- link);
+ struct ddebug_table,
+ link);
ddebug_table_free(dt);
}
mutex_unlock(&ddebug_lock);
--
2.39.0

2023-01-13 20:22:33

by Jim Cromie

Subject: [PATCH v2 06/20] dyndbg: drop NUM_TYPE_ARRAY

ARRAY_SIZE works here, since array decl is complete.

Signed-off-by: Jim Cromie <[email protected]>
---
include/linux/dynamic_debug.h | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index bf47bcfad8e6..81b643ab7f6e 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -104,11 +104,9 @@ struct ddebug_class_map {
.mod_name = KBUILD_MODNAME, \
.base = _base, \
.map_type = _maptype, \
- .length = NUM_TYPE_ARGS(char*, __VA_ARGS__), \
+ .length = ARRAY_SIZE(_var##_classnames), \
.class_names = _var##_classnames, \
}
-#define NUM_TYPE_ARGS(eltype, ...) \
- (sizeof((eltype[]){__VA_ARGS__}) / sizeof(eltype))

/* encapsulate linker provided built-in (or module) dyndbg data */
struct _ddebug_info {
--
2.39.0

2023-01-13 20:23:35

by Jim Cromie

Subject: [PATCH v2 14/20] drm_print: fix stale macro-name in comment

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.39.0

2023-01-13 20:25:12

by Jim Cromie

Subject: [PATCH v2 05/20] dyndbg: split param_set_dyndbg_classes to inner/outer

Split param_set_dyndbg_classes() to interface-preserving wrapper &
inner function with an additional param: mod_name, which is passed
into ddebug_apply_class_bitmap() to allow adjusting a single module's
prdbgs. Wrapper passes NULL, preserving current behavior for now.

no functional change.

Signed-off-by: Jim Cromie <[email protected]>
---
lib/dynamic_debug.c | 36 +++++++++++++++++++++---------------
1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 943e0597ecd4..0a5efc735b36 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -707,18 +707,9 @@ static int param_set_dyndbg_classnames(const char *instr, const struct kernel_pa
return 0;
}

-/**
- * param_set_dyndbg_classes - class FOO >control
- * @instr: string echo>d to sysfs, input depends on map_type
- * @kp: kp->arg has state: bits/lvl, map, map_type
- *
- * Enable/disable prdbgs by their class, as given in the arguments to
- * DECLARE_DYNDBG_CLASSMAP. For LEVEL map-types, enforce relative
- * levels by bitpos.
- *
- * Returns: 0 or <0 if error.
- */
-int param_set_dyndbg_classes(const char *instr, const struct kernel_param *kp)
+static int param_set_dyndbg_module_classes(const char *instr,
+ const struct kernel_param *kp,
+ const char *modnm)
{
const struct ddebug_class_param *dcp = kp->arg;
const struct ddebug_class_map *map = dcp->map;
@@ -755,8 +746,8 @@ int param_set_dyndbg_classes(const char *instr, const struct kernel_param *kp)
KP_NAME(kp), inrep, CLASSMAP_BITMASK(map->length));
inrep &= CLASSMAP_BITMASK(map->length);
}
- v2pr_info("bits:%lx > %s\n", inrep, KP_NAME(kp));
- totct += ddebug_apply_class_bitmap(dcp, &inrep, dcp->bits, NULL);
+ v2pr_info("bits:0x%lx > %s.%s\n", inrep, modnm ?: "*", KP_NAME(kp));
+ totct += ddebug_apply_class_bitmap(dcp, &inrep, dcp->bits, modnm);
*dcp->bits = inrep;
break;
case DD_CLASS_TYPE_LEVEL_NUM:
@@ -769,7 +760,7 @@ int param_set_dyndbg_classes(const char *instr, const struct kernel_param *kp)
old_bits = CLASSMAP_BITMASK(*dcp->lvl);
new_bits = CLASSMAP_BITMASK(inrep);
v2pr_info("lvl:%ld bits:0x%lx > %s\n", inrep, new_bits, KP_NAME(kp));
- totct += ddebug_apply_class_bitmap(dcp, &new_bits, &old_bits, NULL);
+ totct += ddebug_apply_class_bitmap(dcp, &new_bits, &old_bits, modnm);
*dcp->lvl = inrep;
break;
default:
@@ -778,6 +769,21 @@ int param_set_dyndbg_classes(const char *instr, const struct kernel_param *kp)
vpr_info("%s: total matches: %d\n", KP_NAME(kp), totct);
return 0;
}
+/**
+ * param_set_dyndbg_classes - class FOO >control
+ * @instr: string echo>d to sysfs, input depends on map_type
+ * @kp: kp->arg has state: bits/lvl, map, map_type
+ *
+ * Enable/disable prdbgs by their class, as given in the arguments to
+ * DECLARE_DYNDBG_CLASSMAP. For LEVEL map-types, enforce relative
+ * levels by bitpos.
+ *
+ * Returns: 0 or <0 if error.
+ */
+int param_set_dyndbg_classes(const char *instr, const struct kernel_param *kp)
+{
+ return param_set_dyndbg_module_classes(instr, kp, NULL);
+}
EXPORT_SYMBOL(param_set_dyndbg_classes);

/**
--
2.39.0

2023-01-13 20:25:39

by Jim Cromie

Subject: [PATCH v2 01/20] test-dyndbg: fixup CLASSMAP usage error

more careful reading of test output reveals:

lib/test_dynamic_debug.c:103 [test_dynamic_debug]do_cats =pmf "doing categories\n"
lib/test_dynamic_debug.c:105 [test_dynamic_debug]do_cats =p "LOW msg\n" class:MID
lib/test_dynamic_debug.c:106 [test_dynamic_debug]do_cats =p "MID msg\n" class:HI
lib/test_dynamic_debug.c:107 [test_dynamic_debug]do_cats =_ "HI msg\n" class unknown, _id:13

That last line is wrong, the HI class is declared.

But the enum's 1st val (explicitly initialized) was wrong; it must be
_base, not _base+1 (a DECLARE_DYNDBG_CLASSMAP param). So the last
enumeration exceeded the range of mapped class-id's, which triggered
the "class unknown" report. Basically, I coded in an error, and
forgot to verify it and remove it.

RFC:

This patch fixes a bad usage of DECLARE_DYNDBG_CLASSMAP([1]), showing that
it is too error-prone. As noted in test-dynamic-debug.c comments:

* Using the CLASSMAP api:
* - classmaps must have corresponding enum
* - enum symbols must match/correlate with class-name strings in the map.
* - base must equal enum's 1st value
* - multiple maps must set their base to share the 0-62 class_id space !!
* (build-bug-on tips welcome)

Those shortcomings could largely be fixed with a __stringify_list
(which doesn't exist) used in DEFINE_DYNAMIC_DEBUG_CLASSMAP(), on
__VA_ARGS__ a 2nd time. Then, DRM would pass DRM_UT_* ; all the
categories, in order, and not their stringifications, which created
all the usage complications above.

[1] name changed later to DYNDBG_CLASSMAP_DEFINE

Signed-off-by: Jim Cromie <[email protected]>
---
lib/test_dynamic_debug.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/test_dynamic_debug.c b/lib/test_dynamic_debug.c
index 8dd250ad022b..a01f0193a419 100644
--- a/lib/test_dynamic_debug.c
+++ b/lib/test_dynamic_debug.c
@@ -75,7 +75,7 @@ DD_SYS_WRAP(disjoint_bits, p);
DD_SYS_WRAP(disjoint_bits, T);

/* symbolic input, independent bits */
-enum cat_disjoint_names { LOW = 11, MID, HI };
+enum cat_disjoint_names { LOW = 10, MID, HI };
DECLARE_DYNDBG_CLASSMAP(map_disjoint_names, DD_CLASS_TYPE_DISJOINT_NAMES, 10,
"LOW", "MID", "HI");
DD_SYS_WRAP(disjoint_names, p);
--
2.39.0

2023-01-13 20:29:13

by Jim Cromie

Subject: [PATCH v2 18/20] test-dyndbg: tune sub-module behavior

lib/test_dynamic_debug.c is used to build 2 modules:
test_dynamic_debug.ko and test_dynamic_debug_submod.ko

Define DEBUG only in the main module, not in the submod. Its purpose
is to insure that prdbgs are enabled by default, so that a modprobe
without params actually logs something, showing that compile-time
enablement works. This doesn't need to be repeated in the submodule.

Rather, the submodule's purpose is to prove that classmaps defined and
exported from a parent module are propagated to submodules, setting
their class'd debugs accordingly.

Signed-off-by: Jim Cromie <[email protected]>
---
lib/test_dynamic_debug.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/test_dynamic_debug.c b/lib/test_dynamic_debug.c
index 9e66c5a7e138..94fe3b3438d0 100644
--- a/lib/test_dynamic_debug.c
+++ b/lib/test_dynamic_debug.c
@@ -6,13 +6,13 @@
* Jim Cromie <[email protected]>
*/

-#if defined(TEST_DYNAMIC_DEBUG_SUBMOD)
- #define pr_fmt(fmt) "test_dd_submod: " fmt
-#else
+#if !defined(TEST_DYNAMIC_DEBUG_SUBMOD)
#define pr_fmt(fmt) "test_dd: " fmt
+ #define DEBUG /* enable all prdbgs (plain & class'd) at compiletime */
+#else
+ #define pr_fmt(fmt) "test_dd_submod: " fmt
#endif

-#define DEBUG /* enable all prdbgs (plain & class'd) at compiletime */

#include <linux/module.h>

--
2.39.0

2023-01-13 20:30:35

by Jim Cromie

Subject: [PATCH v2 11/20] dyndbg-API: specialize DYNDBG_CLASSMAP_(DEFINE|USE)

Now that the DECLARE_DYNDBG_CLASSMAP macro has been split into
DYNDBG_CLASSMAP_DEFINE and DYNDBG_CLASSMAP_USE, lets differentiate
them according to their separate jobs.

Dyndbg's existing __dyndbg_classes[] section does:

. catalogs the classmaps defined by the module (or builtin modules)
. authorizes dyndbg to >control those class'd prdbgs for the module.

This patch adds __dyndbg_class_refs[] section:

. catalogs references/uses of the above classmap definitions.
. authorizes dyndbg to >control those class'd prdbgs in ref'g module.
. maps the client module to classmap definitions
drm-drivers and helpers are clients.
this allows dyndbg to apply drm.debug to the client module, when added.

The distinction of the 2 roles yields 2 gains:

It follows the define-once-declare-elsewhere pattern that K&R gave us,
dumping the weird coordinated-changes-by-identical-classmaps API.

It should help solve the chicken-egg problem that DRM_USE_DYNAMIC_DEBUG
has; the _USEr module must propagate the drm.debug setting once the
using module has loaded.

The new DYNDBG_CLASSMAP_* macros add records to the sections:

DYNDBG_CLASSMAP_DEFINE:
invoked just once per sub-system.
for drm, its drm_print, where drm.debug is exposed.
defines the classmap, names "DRM_UT_*", maps to class_id's
authorizes dyndbg to exert >control
populates __dyndbg_classes[] "section", __used.
exports the classmap.

DYNDBG_CLASSMAP_USE:
invoked by modules using classmaps defined & exported elsewhere
populates __dyndbg_class_refs[] "section", __used.
maps client-module name to the extern'd classmap.
has client-name, so dyndbg can recognize loading client modules.

also:

struct ddebug_info gets 2 new fields to encapsulate the new section:
class_refs, num_class_refs.
set by dynamic_debug_init() for builtins.
or by kernel/module/main:load_info() for loadable modules.

. struct ddebug_class_user
contains: user-module-name, ref to classmap-defn
dyndbg finds drm-driver's use of a classmap, gets/applies its settings

. vmlinux.lds.h additions: linker symbols, KEEP for new section

dynamic_debug.c: 2 sets of changes;
those "under" ddebug_add_module(), immediately below
those "under" ddebug_change(), further below

ddebug_attach_module_classes():
as before:
called from ddebug_add_module
finds classmaps whose .mod_name matches module being added.
attaches them to the module's ddebug_table.

ddebug_attach_client_module_classes():
new:
called from ddebug_add_module, after list-add to ddebug-tables.
like above, but works class-refs, not classes.
foreach __dyndbg_class_refs: ddebug_param_load_dependent_class(classmap*)

s/ddebug_find_kparams/ddebug_apply_parents_params/.

ddebug_find_kparam(classmap*):

scans module's/builtin kernel-params, calls ddebug_match_attach_kparam
for each to find the params/sysfs-nodes using a classmap.

ddebug_match_apply_kparam():

1st, it tests the kernel-param.ops is dyndbg's; this guarantees that
the attached arg is a struct ddebug_class_param, which has a ref to
the param's state.

Then compare the modname being loaded to the classmap.mod_name; if it
matches, save the debug_class_param into the classmap.dc_parm. This
lets users of the classmap (in particular a client) access the state.

ddebug_param_load_dependent_class():

Called from ddebug_attach_client_module_classes() on each class_refs[]
entry. Each user refs a classmap which has already been seen by
ddebug_find_kparam(), and whose .dc_parm has the state.

So this fn just copies the state to a local var, then calls
ddebug_apply_class_bitmap() to apply it to the dependent module.
ddebug_apply_class_bitmap() tests for bit changes before sending
messages without making changes.

ddebug_find_valid_class():

This helps ddebug_change(), doing the search over classmaps, looking
for the class given to >control. So now it searches over
__dyndbg_class_refs[] after __dyndbg_classes[].

Thats the theory anyway. things are still broken (differently) for
both builtins and loadables. For loadables, the >control is applied,
but doesnt alter any callsites. For builtins, things break earlier.

[*] consider swapping struct ddebug_class_param's dc_parm for kparam.
This gives access to kparam->name, helpful for current debugging.
Attaching that kp via macro would simplify attach-*module-classes too.

Signed-off-by: Jim Cromie <[email protected]>

Signed-off-by: Jim Cromie <[email protected]>

dyndbg: rework ddebug_(|client_)_module_attach_classes

move ddebug_attach_module_classes() up.

name-refinements++:

s/ddebug_param_load_dependent_class/ddebug_apply_parents_params/

Signed-off-by: Jim Cromie <[email protected]>
---
include/asm-generic/vmlinux.lds.h | 3 +
include/linux/dynamic_debug.h | 39 ++++---
kernel/module/main.c | 2 +
lib/dynamic_debug.c | 166 ++++++++++++++++++++++++++----
4 files changed, 176 insertions(+), 34 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 3dc5824141cd..7100701fb68c 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -363,6 +363,9 @@
__start___dyndbg_classes = .; \
KEEP(*(__dyndbg_classes)) \
__stop___dyndbg_classes = .; \
+ __start___dyndbg_class_refs = .; \
+ KEEP(*(__dyndbg_class_refs)) \
+ __stop___dyndbg_class_refs = .; \
__start___dyndbg = .; \
KEEP(*(__dyndbg)) \
__stop___dyndbg = .; \
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 1cdfd62fd2e4..397ac8294230 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -81,8 +81,8 @@ enum ddebug_class_map_type {
};

struct ddebug_class_map {
- struct module *mod;
- const char *mod_name; /* needed for builtins */
+ const struct module *mod; /* NULL for builtins */
+ const char *mod_name;
const char **class_names;
const int length;
const int base; /* index of 1st .class_id, allows split/shared space */
@@ -99,8 +99,8 @@ struct ddebug_class_map {
* @classes: class-names used to control class'd prdbgs
*/
#define DYNDBG_CLASSMAP_DEFINE(_var, _maptype, _base, ...) \
- static const char *_var##_classnames[] = { __VA_ARGS__ }; \
- static struct ddebug_class_map __aligned(8) __used \
+ const char *_var##_classnames[] = { __VA_ARGS__ }; \
+ struct ddebug_class_map __aligned(8) __used \
__section("__dyndbg_classes") _var = { \
.mod = THIS_MODULE, \
.mod_name = KBUILD_MODNAME, \
@@ -108,24 +108,37 @@ struct ddebug_class_map {
.map_type = _maptype, \
.length = ARRAY_SIZE(_var##_classnames), \
.class_names = _var##_classnames, \
- }
+ }; \
+ EXPORT_SYMBOL(_var)

-/*
- * refer to the classmap instantiated once, by the macro above. This
- * distinguishes the multiple users of drm.debug from the single
- * definition, allowing them to specialize. ATM its a pass-thru, but
- * it should help regularize the admittedly wierd sharing by identical
- * definitions.
+struct ddebug_class_user {
+ char *user_mod_name;
+ struct ddebug_class_map *map;
+};
+/**
+ * DYNDBG_CLASSMAP_USE - Use a classmap DEFINEd in another module.
+ * This lets dyndbg initialize the dependent module's prdbgs from the
+ * other module's controlling sysfs node.
*/
-#define DYNDBG_CLASSMAP_USE(_var, _maptype, _base, ...) \
- DYNDBG_CLASSMAP_DEFINE(_var, _maptype, _base, __VA_ARGS__)
+#define DYNDBG_CLASSMAP_USE(_var, ...) \
+ DYNDBG_CLASSMAP_USE_(_var, __UNIQUE_ID(ddebug_class_user), \
+ __VA_ARGS__)
+#define DYNDBG_CLASSMAP_USE_(_var, _uname, ...) \
+ extern struct ddebug_class_map _var; \
+ static struct ddebug_class_user __used \
+ __section("__dyndbg_class_refs") _uname = { \
+ .user_mod_name = KBUILD_MODNAME, \
+ .map = &_var, \
+ }

/* encapsulate linker provided built-in (or module) dyndbg data */
struct _ddebug_info {
struct _ddebug *descs;
struct ddebug_class_map *classes;
+ struct ddebug_class_user *class_refs;
unsigned int num_descs;
unsigned int num_classes;
+ unsigned int num_class_refs;
};

struct ddebug_class_param {
diff --git a/kernel/module/main.c b/kernel/module/main.c
index d02d39c7174e..ee4f85a3b8f0 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2111,6 +2111,8 @@ static int find_module_sections(struct module *mod, struct load_info *info)
sizeof(*info->dyndbg.descs), &info->dyndbg.num_descs);
info->dyndbg.classes = section_objs(info, "__dyndbg_classes",
sizeof(*info->dyndbg.classes), &info->dyndbg.num_classes);
+ info->dyndbg.class_refs = section_objs(info, "__dyndbg_class_refs",
+ sizeof(*info->dyndbg.class_refs), &info->dyndbg.num_class_refs);

return 0;
}
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index b51f4bde6198..19bf66229d45 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -43,13 +43,16 @@ extern struct _ddebug __start___dyndbg[];
extern struct _ddebug __stop___dyndbg[];
extern struct ddebug_class_map __start___dyndbg_classes[];
extern struct ddebug_class_map __stop___dyndbg_classes[];
+extern struct ddebug_class_user __start___dyndbg_class_refs[];
+extern struct ddebug_class_user __stop___dyndbg_class_refs[];

struct ddebug_table {
struct list_head link;
const char *mod_name;
struct _ddebug *ddebugs;
struct ddebug_class_map *classes;
- unsigned int num_ddebugs, num_classes;
+ struct ddebug_class_user *class_refs;
+ unsigned int num_ddebugs, num_classes, num_class_refs;
};

struct ddebug_query {
@@ -147,21 +150,36 @@ static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
query->first_lineno, query->last_lineno, query->class_string);
}

+#define vpr_dt_info(dt, _msg, ...) \
+ v2pr_info(_msg " module:%s nd:%d nc:%d nu:%d\n", ##__VA_ARGS__, \
+ dt->mod_name, dt->num_ddebugs, dt->num_classes, dt->num_class_refs);
+
#define __outvar /* filled by callee */
static struct ddebug_class_map *ddebug_find_valid_class(struct ddebug_table const *dt,
const char *class_string,
__outvar int *class_id)
{
struct ddebug_class_map *map;
+ struct ddebug_class_user *cli;
int i, idx;

- for (map = dt->classes, i = 0; i < dt->num_classes; i++, map++) {
+ for (i = 0, map = dt->classes; i < dt->num_classes; i++, map++) {
idx = match_string(map->class_names, map->length, class_string);
if (idx >= 0) {
*class_id = idx + map->base;
+ vpr_dt_info(dt, "good-class: %s.%s ", map->mod_name, class_string);
return map;
}
}
+ for (i = 0, cli = dt->class_refs; i < dt->num_class_refs; i++, cli++) {
+ idx = match_string(cli->map->class_names, cli->map->length, class_string);
+ if (idx >= 0) {
+ *class_id = idx + cli->map->base;
+ vpr_dt_info(dt, "class-ref: %s.%s ",
+ cli->user_mod_name, class_string);
+ return cli->map;
+ }
+ }
*class_id = -ENOENT;
return NULL;
}
@@ -590,9 +608,10 @@ static int ddebug_exec_queries(char *query, const char *modname)
return nfound;
}

-/* apply a new bitmap to the sys-knob's current bit-state */
+/* apply a new class-param setting */
static int ddebug_apply_class_bitmap(const struct ddebug_class_param *dcp,
- const unsigned long *new_bits, const unsigned long *old_bits,
+ const unsigned long *new_bits,
+ const unsigned long *old_bits,
const char *query_modname)
{
#define QUERY_SIZE 128
@@ -601,8 +620,9 @@ static int ddebug_apply_class_bitmap(const struct ddebug_class_param *dcp,
int matches = 0;
int bi, ct;

- v2pr_info("apply bitmap: 0x%lx to: 0x%lx for %s\n", *new_bits, *old_bits,
- query_modname ?: "");
+ if (*new_bits != *old_bits)
+ v2pr_info("apply bitmap: 0x%lx to: 0x%lx for %s\n", *new_bits,
+ *old_bits, query_modname ?: "'*'");

for (bi = 0; bi < map->length; bi++) {
if (test_bit(bi, new_bits) == test_bit(bi, old_bits))
@@ -617,8 +637,9 @@ 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 for %s\n", *new_bits, *old_bits,
- query_modname ?: "");
+ if (*new_bits != *old_bits)
+ v2pr_info("applied bitmap: 0x%lx to: 0x%lx for %s\n", *new_bits,
+ *old_bits, query_modname ?: "'*'");

return matches;
}
@@ -720,7 +741,10 @@ static int param_set_dyndbg_module_classes(const char *instr,
/* numeric input, accept and fall-thru */
rc = kstrtoul(instr, 0, &inrep);
if (rc) {
- pr_err("expecting numeric input: %s > %s\n", instr, KP_NAME(kp));
+ char *nl = strchr(instr, '\n');
+ if (nl)
+ *nl = '\0';
+ pr_err("expecting numeric input, not: %s > %s\n", instr, KP_NAME(kp));
return -EINVAL;
}
break;
@@ -1113,12 +1137,17 @@ static void *ddebug_proc_next(struct seq_file *m, void *p, loff_t *pos)
static const char *ddebug_class_name(struct ddebug_table *dt, struct _ddebug *dp)
{
struct ddebug_class_map *map = dt->classes;
+ struct ddebug_class_user *cli = dt->class_refs;
int i;

for (i = 0; i < dt->num_classes; i++, map++)
if (class_in_range(dp->class_id, map))
return map->class_names[dp->class_id - map->base];

+ for (i = 0; i < dt->num_class_refs; i++, cli++)
+ if (class_in_range(dp->class_id, cli->map))
+ return cli->map->class_names[dp->class_id - cli->map->base];
+
return NULL;
}

@@ -1199,31 +1228,120 @@ static const struct proc_ops proc_fops = {
.proc_write = ddebug_proc_write
};

+
+/*
+ * Find this module's classmaps in a sub/whole-range of the builtin/
+ * modular classmap vector/section. Save the start and length of the
+ * subrange at its edges.
+ */
static void ddebug_attach_module_classes(struct ddebug_table *dt, struct _ddebug_info *di)
{
struct ddebug_class_map *cm;
int i, nc = 0;

- /*
- * Find this module's classmaps in a subrange/wholerange of
- * the builtin/modular classmap vector/section. Save the start
- * and length of the subrange at its edges.
- */
- for (cm = di->classes, i = 0; i < di->num_classes; i++, cm++) {
+ for (i = 0, cm = di->classes; i < di->num_classes; i++, cm++) {

if (!strcmp(cm->mod_name, dt->mod_name)) {
if (!nc) {
- v2pr_info("start subrange, class[%d]: module:%s base:%d len:%d ty:%d\n",
- i, cm->mod_name, cm->base, cm->length, cm->map_type);
dt->classes = cm;
+ v2pr_info("classes[0..]: module:%s base:%d len:%d ty:%d\n",
+ cm->mod_name, cm->base, cm->length, cm->map_type);
}
nc++;
}
}
- if (nc) {
- dt->num_classes = nc;
+ dt->num_classes = nc;
+ if (nc)
vpr_info("module:%s attached %d classes\n", dt->mod_name, nc);
+}
+
+#define vpr_cm_info(cm, _msg, ...) \
+ v2pr_info(_msg "module:%s base:%d len:%d type:%d\n", ##__VA_ARGS__, \
+ cm->mod_name, cm->base, cm->length, cm->map_type)
+
+static void ddebug_match_apply_kparam(const struct kernel_param *kp,
+ const struct ddebug_class_map *cm,
+ const char *cli_name)
+{
+ struct ddebug_class_param *dcp;
+ unsigned long new_bits, old_bits = 0;
+ int totct = 0;
+
+ if (kp->ops != &param_ops_dyndbg_classes)
+ return;
+
+ dcp = (struct ddebug_class_param *)kp->arg;
+
+ if (cm == dcp->map) {
+ v2pr_info("found kp:%s =0x%lx", kp->name, *dcp->bits);
+ vpr_cm_info(cm, "mapped to:");
+ /*
+ * using apply-bitmap is too low.
+ * param_set_dyndbg_classes uses map_type to sort
+ * levels-to-bits.
+ * param_set_dyndbg_classes is too high, it takes
+ * string inputs.
+ * de-union-ing levels/bits might solve it (partly),
+ * at least simplifying the translation, and possibly
+ * the apply-bitmap fn/iface
+ */
+ new_bits = *dcp->bits;
+ totct += ddebug_apply_class_bitmap(dcp, &new_bits, &old_bits, cli_name);
+ }
+}
+
+static void ddebug_apply_parents_params(const struct ddebug_class_map *cm,
+ const char *user_modname)
+{
+ const struct kernel_param *kp;
+#if IS_ENABLED(CONFIG_MODULES)
+ int i;
+
+ if (cm->mod) {
+ vpr_cm_info(cm, "loaded class:");
+ for (i = 0, kp = cm->mod->kp; i < cm->mod->num_kp; i++, kp++)
+ ddebug_match_apply_kparam(kp, cm, user_modname);
+ }
+#endif
+ if (!cm->mod) {
+ vpr_cm_info(cm, "builtin class:");
+ for (kp = __start___param; kp < __stop___param; kp++)
+ ddebug_match_apply_kparam(kp, cm, user_modname);
+ }
+}
+
+/*
+ * propagates class-params thru their classmaps to class-users. this
+ * means a query against the dt/module, which means it must be on the
+ * list to be seen by ddebug_change.
+ */
+static void ddebug_attach_client_module_classes(struct ddebug_table *dt, const struct _ddebug_info *di)
+{
+ struct ddebug_class_user *cli;
+ const struct ddebug_class_map *cm;
+ int i, nc = 0;
+
+ for (i = 0, cli = di->class_refs; i < di->num_class_refs; i++, cli++) {
+
+ BUG_ON(!cli || !cli->map || !cli->user_mod_name);
+
+ if (!strcmp(cli->user_mod_name, dt->mod_name)) {
+
+ v2pr_info("class_ref[%d] %s -> %s\n", i,
+ cli->user_mod_name, cli->map->mod_name);
+
+ if (!nc++)
+ dt->class_refs = cli;
+ }
+ }
+ dt->num_class_refs = nc;
+
+ for (i = 0, cli = dt->class_refs; i < dt->num_class_refs; i++, cli++) {
+ cm = cli->map;
+ ddebug_apply_parents_params(cm, cli->user_mod_name);
}
+
+ vpr_dt_info(dt, "attach-client-module: ");
}

/*
@@ -1235,7 +1353,8 @@ static int __ddebug_add_module(struct _ddebug_info *di, unsigned int base,
{
struct ddebug_table *dt;

- v3pr_info("add-module: %s.%d sites\n", modname, di->num_descs);
+ v3pr_info("add-module: %s %d sites %d.%d\n", modname, di->num_descs,
+ di->num_classes, di->num_class_refs);
if (!di->num_descs) {
v3pr_info(" skip %s\n", modname);
return 0;
@@ -1258,13 +1377,16 @@ static int __ddebug_add_module(struct _ddebug_info *di, unsigned int base,

INIT_LIST_HEAD(&dt->link);

- if (di->classes && di->num_classes)
+ if (di->num_classes)
ddebug_attach_module_classes(dt, di);

mutex_lock(&ddebug_lock);
list_add_tail(&dt->link, &ddebug_tables);
mutex_unlock(&ddebug_lock);

+ if (di->num_class_refs)
+ ddebug_attach_client_module_classes(dt, di);
+
vpr_info("%3u debug prints in module %s\n", di->num_descs, modname);
return 0;
}
@@ -1390,8 +1512,10 @@ static int __init dynamic_debug_init(void)
struct _ddebug_info di = {
.descs = __start___dyndbg,
.classes = __start___dyndbg_classes,
+ .class_refs = __start___dyndbg_class_refs,
.num_descs = __stop___dyndbg - __start___dyndbg,
.num_classes = __stop___dyndbg_classes - __start___dyndbg_classes,
+ .num_class_refs = __stop___dyndbg_class_refs - __start___dyndbg_class_refs,
};

if (&__start___dyndbg == &__stop___dyndbg) {
--
2.39.0

2023-01-13 20:52:22

by Jim Cromie

Subject: [PATCH v2 09/20] dyndbg: constify ddebug_apply_class_bitmap args

ddebug_apply_class_bitmap() does not alter its 2 bitmap args, make
this guarantee in the interface.

NOTE: the bitmap is also available in the dcp arg, but the 2 vars
serve a 2nd purpose; the CLASS_TYPE callers use them to translate
levels into their underlying disjoint representation.

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 10c29bc19901..b51f4bde6198 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -592,7 +592,7 @@ static int ddebug_exec_queries(char *query, const char *modname)

/* apply a new bitmap to the sys-knob's current bit-state */
static int ddebug_apply_class_bitmap(const struct ddebug_class_param *dcp,
- unsigned long *new_bits, unsigned long *old_bits,
+ const unsigned long *new_bits, const unsigned long *old_bits,
const char *query_modname)
{
#define QUERY_SIZE 128
--
2.39.0

2023-01-13 20:53:04

by Jim Cromie

Subject: [PATCH v2 10/20] dyndbg-API: split DECLARE_(DYNDBG_CLASSMAP) to $1(_DEFINE|_USE)

DECLARE_DYNDBG_CLASSMAP's job was to allow modules to declare the debug
classes/categories they want dyndbg to >control on their behalf. Its
args give the class-names, their mapping to class_ids, and the sysfs
interface style (usually a class-bitmap). Modules wanting a drm.debug
style knob need to create the kparam, and call module_param_cb() to
wire the sysfs node to the classmap. DRM does this is in drm_print.c

In DRM, multiple modules declare identical DRM_UT_* classmaps, so that
the class'd prdbgs are modified across those modules in a coordinated
way across the subsystem, by either explicit class DRM_UT_* queries to
>control, or by writes to /sys/module/drm/parameters/debug (drm.debug)

This coordination-by-identical-declarations is weird, so this patch
splits the macro into _DEFINE and _USE flavors. This distinction
follows the definition vs declaration that K&R gave us, improving the
api; _DEFINE is used once to specify the classmap, and multiple users
_USE the single definition explicitly.

Currently the latter just reuses the former, and still needs all the
same args, but that can be tuned later; the _DEFINE can initialize an
(extern/global) struct classmap, and _USE can, well use/reference
that struct.

Also wrap DYNDBG_CLASSMAP_USEs with ifdef DRM_USE_DYNAMIC_DEBUG to
balance with the one around drm_print's use of DYNDBG_CLASSMAP_DEFINE.

Signed-off-by: Jim Cromie <[email protected]>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 4 +++-
drivers/gpu/drm/display/drm_dp_helper.c | 4 +++-
drivers/gpu/drm/drm_crtc_helper.c | 4 +++-
drivers/gpu/drm/drm_print.c | 2 +-
drivers/gpu/drm/i915/i915_params.c | 4 +++-
drivers/gpu/drm/nouveau/nouveau_drm.c | 4 +++-
include/linux/dynamic_debug.h | 20 ++++++++++++----
lib/test_dynamic_debug.c | 32 ++++++++++++-------------
8 files changed, 48 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index bf2d50c8c92a..0075184b5d93 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -188,7 +188,8 @@ int amdgpu_vcnfw_log;

static void amdgpu_drv_delayed_reset_work_handler(struct work_struct *work);

-DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
+#if defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
+DYNDBG_CLASSMAP_USE(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
"DRM_UT_CORE",
"DRM_UT_DRIVER",
"DRM_UT_KMS",
@@ -199,6 +200,7 @@ DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
"DRM_UT_LEASE",
"DRM_UT_DP",
"DRM_UT_DRMRES");
+#endif

struct amdgpu_mgpu_info mgpu_info = {
.mutex = __MUTEX_INITIALIZER(mgpu_info.mutex),
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
index 16565a0a5da6..8fa7a88299e7 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -41,7 +41,8 @@

#include "drm_dp_helper_internal.h"

-DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
+#if defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
+DYNDBG_CLASSMAP_USE(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
"DRM_UT_CORE",
"DRM_UT_DRIVER",
"DRM_UT_KMS",
@@ -52,6 +53,7 @@ DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
"DRM_UT_LEASE",
"DRM_UT_DP",
"DRM_UT_DRMRES");
+#endif

struct dp_aux_backlight {
struct backlight_device *base;
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 7d86020b5244..2f747c9c8f60 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -51,7 +51,8 @@

#include "drm_crtc_helper_internal.h"

-DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
+#if defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
+DYNDBG_CLASSMAP_USE(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
"DRM_UT_CORE",
"DRM_UT_DRIVER",
"DRM_UT_KMS",
@@ -62,6 +63,7 @@ DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
"DRM_UT_LEASE",
"DRM_UT_DP",
"DRM_UT_DRMRES");
+#endif

/**
* DOC: overview
diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index 5b93c11895bb..4b697e18238d 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -56,7 +56,7 @@ MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug cat
module_param_named(debug, __drm_debug, ulong, 0600);
#else
/* classnames must match vals of enum drm_debug_category */
-DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
+DYNDBG_CLASSMAP_DEFINE(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
"DRM_UT_CORE",
"DRM_UT_DRIVER",
"DRM_UT_KMS",
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index d1e4d528cb17..b5b2542ae364 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -29,7 +29,8 @@
#include "i915_params.h"
#include "i915_drv.h"

-DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
+#if defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
+DYNDBG_CLASSMAP_USE(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
"DRM_UT_CORE",
"DRM_UT_DRIVER",
"DRM_UT_KMS",
@@ -40,6 +41,7 @@ DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
"DRM_UT_LEASE",
"DRM_UT_DP",
"DRM_UT_DRMRES");
+#endif

#define i915_param_named(name, T, perm, desc) \
module_param_named(name, i915_modparams.name, T, perm); \
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index fd99ec0f4257..2963cf5b0807 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -71,7 +71,8 @@
#include "nouveau_svm.h"
#include "nouveau_dmem.h"

-DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
+#if defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
+DYNDBG_CLASSMAP_USE(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
"DRM_UT_CORE",
"DRM_UT_DRIVER",
"DRM_UT_KMS",
@@ -82,6 +83,7 @@ DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
"DRM_UT_LEASE",
"DRM_UT_DP",
"DRM_UT_DRMRES");
+#endif

MODULE_PARM_DESC(config, "option string to pass to driver core");
static char *nouveau_config;
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 81b643ab7f6e..1cdfd62fd2e4 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -56,7 +56,7 @@ struct _ddebug {
#endif
} __attribute__((aligned(8)));

-enum class_map_type {
+enum ddebug_class_map_type {
DD_CLASS_TYPE_DISJOINT_BITS,
/**
* DD_CLASS_TYPE_DISJOINT_BITS: classes are independent, one per bit.
@@ -86,17 +86,19 @@ struct ddebug_class_map {
const char **class_names;
const int length;
const int base; /* index of 1st .class_id, allows split/shared space */
- enum class_map_type map_type;
+ enum ddebug_class_map_type map_type;
};

/**
- * DECLARE_DYNDBG_CLASSMAP - declare classnames known by a module
+ * DYNDBG_CLASSMAP_DEFINE - define the class_map that names the
+ * debug classes used in this module. This tells dyndbg the authorized
+ * classnames it should manipulate.
* @_var: a struct ddebug_class_map, passed to module_param_cb
* @_type: enum class_map_type, chooses bits/verbose, numeric/symbolic
* @_base: offset of 1st class-name. splits .class_id space
* @classes: class-names used to control class'd prdbgs
*/
-#define DECLARE_DYNDBG_CLASSMAP(_var, _maptype, _base, ...) \
+#define DYNDBG_CLASSMAP_DEFINE(_var, _maptype, _base, ...) \
static const char *_var##_classnames[] = { __VA_ARGS__ }; \
static struct ddebug_class_map __aligned(8) __used \
__section("__dyndbg_classes") _var = { \
@@ -108,6 +110,16 @@ struct ddebug_class_map {
.class_names = _var##_classnames, \
}

+/*
+ * refer to the classmap instantiated once, by the macro above. This
+ * distinguishes the multiple users of drm.debug from the single
+ * definition, allowing them to specialize. ATM its a pass-thru, but
+ * it should help regularize the admittedly wierd sharing by identical
+ * definitions.
+ */
+#define DYNDBG_CLASSMAP_USE(_var, _maptype, _base, ...) \
+ DYNDBG_CLASSMAP_DEFINE(_var, _maptype, _base, __VA_ARGS__)
+
/* encapsulate linker provided built-in (or module) dyndbg data */
struct _ddebug_info {
struct _ddebug *descs;
diff --git a/lib/test_dynamic_debug.c b/lib/test_dynamic_debug.c
index 89dd7f285e31..8d384b979e74 100644
--- a/lib/test_dynamic_debug.c
+++ b/lib/test_dynamic_debug.c
@@ -62,38 +62,38 @@ enum cat_disjoint_bits {
D2_LEASE,
D2_DP,
D2_DRMRES };
-DECLARE_DYNDBG_CLASSMAP(map_disjoint_bits, DD_CLASS_TYPE_DISJOINT_BITS, 0,
- "D2_CORE",
- "D2_DRIVER",
- "D2_KMS",
- "D2_PRIME",
- "D2_ATOMIC",
- "D2_VBL",
- "D2_STATE",
- "D2_LEASE",
- "D2_DP",
- "D2_DRMRES");
+DYNDBG_CLASSMAP_DEFINE(map_disjoint_bits, DD_CLASS_TYPE_DISJOINT_BITS, 0,
+ "D2_CORE",
+ "D2_DRIVER",
+ "D2_KMS",
+ "D2_PRIME",
+ "D2_ATOMIC",
+ "D2_VBL",
+ "D2_STATE",
+ "D2_LEASE",
+ "D2_DP",
+ "D2_DRMRES");
DD_SYS_WRAP(disjoint_bits, p);
DD_SYS_WRAP(disjoint_bits, T);

/* symbolic input, independent bits */
enum cat_disjoint_names { LOW = 10, MID, HI };
-DECLARE_DYNDBG_CLASSMAP(map_disjoint_names, DD_CLASS_TYPE_DISJOINT_NAMES, 10,
- "LOW", "MID", "HI");
+DYNDBG_CLASSMAP_DEFINE(map_disjoint_names, DD_CLASS_TYPE_DISJOINT_NAMES, 10,
+ "LOW", "MID", "HI");
DD_SYS_WRAP(disjoint_names, p);
DD_SYS_WRAP(disjoint_names, T);

/* numeric verbosity, V2 > V1 related */
enum cat_level_num { V0 = 14, V1, V2, V3, V4, V5, V6, V7 };
-DECLARE_DYNDBG_CLASSMAP(map_level_num, DD_CLASS_TYPE_LEVEL_NUM, 14,
+DYNDBG_CLASSMAP_DEFINE(map_level_num, DD_CLASS_TYPE_LEVEL_NUM, 14,
"V0", "V1", "V2", "V3", "V4", "V5", "V6", "V7");
DD_SYS_WRAP(level_num, p);
DD_SYS_WRAP(level_num, T);

/* symbolic verbosity */
enum cat_level_names { L0 = 22, L1, L2, L3, L4, L5, L6, L7 };
-DECLARE_DYNDBG_CLASSMAP(map_level_names, DD_CLASS_TYPE_LEVEL_NAMES, 22,
- "L0", "L1", "L2", "L3", "L4", "L5", "L6", "L7");
+DYNDBG_CLASSMAP_DEFINE(map_level_names, DD_CLASS_TYPE_LEVEL_NAMES, 22,
+ "L0", "L1", "L2", "L3", "L4", "L5", "L6", "L7");
DD_SYS_WRAP(level_names, p);
DD_SYS_WRAP(level_names, T);

--
2.39.0

2023-01-17 12:23:46

by Peter Zijlstra

Subject: Re: [PATCH v2 20/20] jump_label: RFC - tolerate toggled state

On Fri, Jan 13, 2023 at 12:30:16PM -0700, Jim Cromie wrote:
> __jump_label_patch currently will "crash the box" if it finds a
> jump_entry not as expected. ISTM this overly harsh; it doesn't
> distinguish between "alternate/opposite" state, and truly
> "insane/corrupted".
>
> The "opposite" (but well-formed) state is a milder mis-initialization
> problem, and some less severe mitigation seems practical. ATM this
> just warns about it; a range/enum of outcomes: warn, crash, silence,
> ok, fixup-continue, etc, are possible on a case-by-case basis.
>
> Ive managed to create this mis-initialization condition with
> test_dynamic_debug.ko & _submod.ko. These replicate DRM's regression
> on DRM_USE_DYNAMIC_DEBUG=y; drm.debug callsites in drivers/helpers
> (dependent modules) are not enabled along with those in drm.ko itself.
>

> Ive hit this case a few times, but havent been able to isolate the
> when and why.
>
> warn-only is something of a punt, and I'm still left with remaining
> bugs which are likely related; I'm able to toggle the p-flag on
> callsites in the submod, but their enablement still doesn't yield
> logging activity.

Right; having been in this is state is bad since it will generate
inconsistent code-flow. Full on panic *might* not be warranted (as it
does for corrupted text) but it is still a fairly bad situation -- so
I'm not convinced we want to warn and carry on.

It would be really good to figure out why the site was skipped over and
got out of skew.

Given it's all module stuff, the 'obvious' case would be something like
a race between adding the new sites and flipping it, but I'm not seeing
how -- things are rather crudely serialized by jump_label_mutex.

The only other option I can come up with is that somehow the update
condition in jump_label_add_module() is somehow wrong.

2023-02-16 17:21:35

by Jason Baron

Subject: Re: [PATCH v2 20/20] jump_label: RFC - tolerate toggled state

On 1/17/23 6:57 AM, Peter Zijlstra wrote:
> On Fri, Jan 13, 2023 at 12:30:16PM -0700, Jim Cromie wrote:
>> __jump_label_patch currently will "crash the box" if it finds a
>> jump_entry not as expected. ISTM this overly harsh; it doesn't
>> distinguish between "alternate/opposite" state, and truly
>> "insane/corrupted".
>>
>> The "opposite" (but well-formed) state is a milder mis-initialization
>> problem, and some less severe mitigation seems practical. ATM this
>> just warns about it; a range/enum of outcomes: warn, crash, silence,
>> ok, fixup-continue, etc, are possible on a case-by-case basis.
>>
>> Ive managed to create this mis-initialization condition with
>> test_dynamic_debug.ko & _submod.ko. These replicate DRM's regression
>> on DRM_USE_DYNAMIC_DEBUG=y; drm.debug callsites in drivers/helpers
>> (dependent modules) are not enabled along with those in drm.ko itself.
>>
>
>> Ive hit this case a few times, but havent been able to isolate the
>> when and why.
>>
>> warn-only is something of a punt, and I'm still left with remaining
>> bugs which are likely related; I'm able to toggle the p-flag on
>> callsites in the submod, but their enablement still doesn't yield
>> logging activity.
>
> Right; having been in this is state is bad since it will generate
> inconsistent code-flow. Full on panic *might* not be warranted (as it
> does for corrupted text) but it is still a fairly bad situation -- so
> I'm not convinced we want to warn and carry on.
>
> It would be really good to figure out why the site was skipped over and
> got out of skew.
>
> Given it's all module stuff, the 'obvious' case would be something like
> a race between adding the new sites and flipping it, but I'm not seeing
> how -- things are rather crudely serialized by jump_label_mutex.

Indeed, looks like dynamic debug introduces a new path in this series to
that tries to toggle the jump label sites before they have been
initialized, which ends up with the jump label key enabled but only some
of the sites in the correct state. Then when the key is subsequently
disabled it finds some in the wrong state. I just posted a patch for
dynamic debug to use the module callback notifiers so it's ordered
properly against jump label.

Note this isn't an issue in the current tree b/c there is no path to
toggle the sites currently before they are initialized, but Jim's series
here adds such a path.

Thanks,

-Jason


>
> The only other option I can come up with is that somehow the update
> condition in jump_label_add_module() is somehow wrong.
>