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 <[email protected]> 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/[email protected]?part=29
