Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] dyndbg: unrelated runtime classmap validation squashed into header stub patch - [Medium] dyndbg: empty DECLARE_DYNDBG_CLASSMAP() stub breaks drm_print.c build --
commit 919e16985df0bc0e411e71184e2e97b087e1097c Author: Jim Cromie <[email protected]> dyndbg: add DEBUG-biased fallback stubs for _dynamic_func_call_cls This commit adds DEBUG-biased fallback stub definitions for the _dynamic_func_call_cls macro family when dynamic debug is disabled for a translation unit. It also modifies runtime classmap validation, descriptor checks, and warning output in lib/dynamic_debug.c. > diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h > index fb182c9030889..6a6cd45e5571c 100644 > --- a/include/linux/dynamic_debug.h > +++ b/include/linux/dynamic_debug.h > @@ -536,6 +536,23 @@ void ddebug_increment_call_count(void); > #define DYNAMIC_DEBUG_BRANCH(descriptor) false > #define DECLARE_DYNDBG_CLASSMAP(...) > > +#if defined(DEBUG) > +#define _dynamic_func_call_cls(cls, fmt, func, ...) \ > + do { func(NULL, ##__VA_ARGS__); } while (0) > +#define _dynamic_func_call_cls_no_desc(cls, fmt, func, ...) \ > + do { func(__VA_ARGS__); } while (0) > +#else > +#define _dynamic_func_call_cls(cls, fmt, func, ...) \ > + do { if (0) func(NULL, ##__VA_ARGS__); } while (0) > +#define _dynamic_func_call_cls_no_desc(cls, fmt, func, ...) \ > + do { if (0) func(__VA_ARGS__); } while (0) > +#endif [Severity: Medium] Does the DRM core build still fail under the configuration described in the commit message (CONFIG_DRM_USE_DYNAMIC_DEBUG=y, CONFIG_DYNAMIC_DEBUG_CORE=y, and CONFIG_DYNAMIC_DEBUG=n)? Because drivers/gpu/drm/Makefile uses: CFLAGS-$(CONFIG_DRM_USE_DYNAMIC_DEBUG) += -DDYNAMIC_DEBUG_MODULE and Kbuild ignores CFLAGS-y in favor of ccflags-y, drivers/gpu/drm/drm_print.c is compiled without DYNAMIC_DEBUG_MODULE defined and falls into this #else block in include/linux/dynamic_debug.h. In this block, DECLARE_DYNDBG_CLASSMAP(...) expands to nothing, while drivers/gpu/drm/drm_print.c references the declared symbol under CONFIG_DRM_USE_DYNAMIC_DEBUG: DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0, "DRM_UT_CORE", ...); static struct ddebug_class_param drm_debug_bitmap = { .bits = &__drm_debug, .flags = "p", .map = &drm_debug_classes, }; With DECLARE_DYNDBG_CLASSMAP expanding to empty, won't compiling drivers/gpu/drm/drm_print.c fail with an undeclared identifier error for drm_debug_classes? > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c > index 027f7c31fe226..8c0a67ad9c922 100644 > --- a/lib/dynamic_debug.c > +++ b/lib/dynamic_debug.c > @@ -329,7 +329,8 @@ static bool ddebug_match_desc(const struct ddebug_query > *query, > /* site is class'd */ > site_map = ddebug_find_map_by_class_id(di, dp->class_id); > if (!site_map) { > - WARN_ONCE(1, "unknown class_id %d, check %s's CLASSMAP > definitions", dp->class_id, di->mod_name); > + pr_warn_ratelimited("unknown class_id %d, check %s's CLASSMAP > definitions\n", > + dp->class_id, di->mod_name); > return false; > } > /* module(-param) decides protection */ > @@ -1583,18 +1584,80 @@ static void ddebug_apply_class_users(const struct > _ddebug_info *di) > __di->_vec.start = __start; \ > }) > > -static int ddebug_class_range_overlap(struct ddebug_class_map *cm, u64 > *reserved_ids) > +static bool ddebug_validate_class_range(const char *modname, > + const char *name, > + int base, int length, > + u64 *reserved_ids) > { > - u64 range = (((1ULL << cm->length) - 1) << cm->base); > + u64 range; > > + if (base < 0 || length <= 0 || > + base + length > _DPRINTK_CLASS_DFLT) { > + pr_err("module %s: base:%d len:%d (from %s) exceeds class_id > limit %d\n", > + modname, base, length, name, _DPRINTK_CLASS_DFLT); > + return false; > + } > + range = (((1ULL << length) - 1) << base); > if (range & *reserved_ids) { > - pr_err("[%d..%d] on %s conflicts with %llx\n", cm->base, > - cm->base + cm->length - 1, cm->class_names[0], > - *reserved_ids); > - return -EINVAL; > + pr_err("module %s: [%d..%d] (from %s) conflicts with 0x%llx\n", > + modname, base, base + length - 1, name, *reserved_ids); > + return false; > } > *reserved_ids |= range; > - return 0; > + return true; > +} > + > +static bool ddebug_validate_classmaps(struct _ddebug_info *di) > +{ > + struct ddebug_class_map *cm; > + struct ddebug_class_user *cli; > + u64 reserved_ids = 0; > + bool valid = true; > + int i; > + > + for_subvec(i, cm, di, maps) > + if (!ddebug_validate_class_range(di->mod_name, > + cm->class_names[0], > + cm->base, cm->length, > + &reserved_ids)) > + valid = false; > + > + for_subvec(i, cli, di, users) { > + if (!cli->map) { > + pr_err("module %s: classmap not found for user\n", > + di->mod_name); > + valid = false; > + continue; > + } > + if (!ddebug_validate_class_range(di->mod_name, > + cli->map->class_names[0], > + cli->map->base + cli->offset, > + cli->map->length, > + &reserved_ids)) > + valid = false; > + } > + return valid; > +} > + > +static void ddebug_validate_descs(struct _ddebug_info *di) > +{ > + struct _ddebug *dp; > + u64 bad_ids = 0; > + int i; > + > + for (i = 0; i < di->descs.len; i++) { > + dp = &di->descs.start[i]; > + > + if (dp->class_id == _DPRINTK_CLASS_DFLT) > + continue; > + if (bad_ids & (1ULL << dp->class_id)) > + continue; > + if (!ddebug_find_map_by_class_id(di, dp->class_id)) { > + pr_warn("module %s uses unknown class_id %d\n", > + di->mod_name, dp->class_id); > + bad_ids |= (1ULL << dp->class_id); > + } > + } > } [ ... ] > @@ -1638,10 +1700,10 @@ static int ddebug_add_module(struct _ddebug_info *di) > dd_set_module_subrange(i, cm, &dt->info, maps); > dd_set_module_subrange(i, cli, &dt->info, users); > > - /* insure 2+ classmaps share the per-module 0..62 class_id space */ > - for_subvec(i, cm, &dt->info, maps) > - if (ddebug_class_range_overlap(cm, &reserved_ids)) > - goto cleanup; > + if (!ddebug_validate_classmaps(&dt->info)) > + goto cleanup; > + > + ddebug_validate_descs(&dt->info); [Severity: Medium] Were the changes in lib/dynamic_debug.c, lib/test_dynamic_debug.c, and tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh accidentally squashed into this patch? The commit description only discusses adding DEBUG-biased fallback stubs for _dynamic_func_call_cls in include/linux/dynamic_debug.h (and notes that the !DEBUG case uses an if (0) statement-expression returning 0, whereas the macro uses do { if (0) ... } while (0)). However, the patch also includes runtime changes in lib/dynamic_debug.c: - Replacing ddebug_class_range_overlap() with ddebug_validate_class_range() and ddebug_validate_classmaps() to check class_id bounds and validate di->users during ddebug_add_module(). - Adding ddebug_validate_descs() to warn on unknown descriptor class_id values in ddebug_add_module(). - Replacing WARN_ONCE() and WARN() with pr_warn_ratelimited() and pr_err() in ddebug_match_desc(), ddebug_add_module(), and ddebug_module_notify(). - Adding the DD_RUNTIME_CLASS_CHECK test block in lib/test_dynamic_debug.c and updating FT_test_classes.3 in dyndbg_selftest.sh. Should these runtime validation and selftest changes be folded into the earlier classmap validation patches in the series or split into a separate commit? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=36
