Re: [PATCH v13 36/39] dyndbg: add DEBUG-biased fallback stubs for _dynamic_func_call_cls

[sashiko-bot@kernel.org]

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 <jim.cromie@gmail.com>

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/20261027-dd-cmap-part2-clean-v13-0-9267923be612@gmail.com?part=36