Re: [PATCH v9 29/39] dyndbg-API: promote DYNAMIC_DEBUG_CLASSMAP_PARAM to API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] When CONFIG_DYNAMIC_DEBUG is disabled, DYNAMIC_DEBUG_CLASSMAP_PARAM_REF evaluates to an empty macro, which will silently drop the module parameter for consumers like DRM.
- [Medium] The commit message claims a modification to `ddebug_sync_classbits()` that does not exist in the patch.
- [Medium] DYNAMIC_DEBUG_CLASSMAP_PARAM zeroes the bitvector regardless of the `DEBUG` macro, causing state desynchronization and an inability to disable debug classes when compiled with DEBUG.
--

commit a5fb641199d77a5e65f10185304cd8aae102e99f
Author: Jim Cromie <jim.cromie@gmail.com>

dyndbg-API: promote DYNAMIC_DEBUG_CLASSMAP_PARAM to API

move the DYNAMIC_DEBUG_CLASSMAP_PARAM macro from test-dynamic-debug.c into
the header, and refine it, by distinguishing the 2 use cases.

> Modify ddebug_sync_classbits() argtype deref inside the fn, to give
> access to all kp members.

[Severity: Medium]
The commit message claims a modification to ddebug_sync_classbits() that does
not exist in the patch. Is this an oversight or an artifact from an older
revision?

> diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> index b53e7488b7f7d..cc203b6c1b565 100644
> --- a/include/linux/dynamic_debug.h
> +++ b/include/linux/dynamic_debug.h
> @@ -293,6 +293,44 @@ struct ddebug_class_param {
>  		.offset = _offset					\
>  	}
>  
> +/**
> + * DYNAMIC_DEBUG_CLASSMAP_PARAM - control a ddebug-classmap from a sys-param
> + * @_name:  sysfs node name
> + * @_var:   name of the classmap var defining the controlled classes/bits
> + * @_flags: flags to be toggled, typically just 'p'
> + *
> + * Creates a sysfs-param to control the classes defined by the
> + * exported classmap, with bits 0..N-1 mapped to the classes named.
> + * This version keeps class-state in a private long int.
> + */
> +#define DYNAMIC_DEBUG_CLASSMAP_PARAM(_name, _var, _flags)		\
> +	static u32 _name##_bvec;					\
> +	__DYNAMIC_DEBUG_CLASSMAP_PARAM(_name, _name##_bvec, _var, _flags)

[Severity: Medium]
Does this macro unconditionally zero-initialize the state bitvector, breaking
synchronization with the actual enabled state of call sites if the module is
compiled with DEBUG?

If a module using this macro is compiled with -DDEBUG, its dynamic debug call
sites are enabled by default. However, the static state vector _name##_bvec
is forcibly zero-initialized.

If a user writes 0 to the sysfs parameter to disable the logs,
param_set_dyndbg_module_classes() compares the new 0 against the old 0 state
and skips sending the -p queries, leaving the logs permanently enabled.

[ ... ]

> @@ -476,6 +514,8 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
>  #define DYNAMIC_DEBUG_CLASSMAP_DEFINE(_var, _mapty, _base, ...)
>  #define DYNAMIC_DEBUG_CLASSMAP_USE(_var)
>  #define DYNAMIC_DEBUG_CLASSMAP_USE_(_var, _offset)
> +#define DYNAMIC_DEBUG_CLASSMAP_PARAM(_name, _var, _flags)
> +#define DYNAMIC_DEBUG_CLASSMAP_PARAM_REF(_name, _bits, _var, _flags)
>  #define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt)

[Severity: High]
When CONFIG_DYNAMIC_DEBUG is disabled, DYNAMIC_DEBUG_CLASSMAP_PARAM_REF
evaluates to an empty macro. Will this silently drop the module parameter
for consumers like DRM?

Consumers of this macro will completely lose their sysfs module parameters
(e.g., /sys/module/drm/parameters/debug) on kernels built without
CONFIG_DYNAMIC_DEBUG.

Does the macro need to provide a fallback like module_param_named() to avoid
breaking kernel ABI expectations for these debugging parameters?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-dd-cmap-part2-clean-v9-0-3a3f621f4a10@gmail.com?part=29