Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] Compilation failure for !CONFIG_DYNAMIC_DEBUG builds due to incorrect macro argument count in DYNAMIC_DEBUG_CLASSMAP_PARAM_REF stub. - [Low] Incorrect format specifier %d used for u32 in ddebug_class_param_clamp_input. - [Low] Duplicate log statement in param_set_dyndbg_module_classes. - [Low] Unused CLASSMAP_BITMASK macro introduced in lib/test_dynamic_debug.c. - [Low] The commit message claims to modify `ddebug_sync_classbits()`, but the patch does not modify it. - [Low] The API documentation for `DYNAMIC_DEBUG_CLASSMAP_PARAM_REF` incorrectly claims `@_bits` expects an `unsigned long`. --
commit fa1c83c075dcd96521ed0eca436783391777f24c Author: Jim Cromie <[email protected]> dyndbg-API: promote DYNAMIC_DEBUG_CLASSMAP_PARAM to API This commit promotes the DYNAMIC_DEBUG_CLASSMAP_PARAM macro into the dynamic debug header for broader use (e.g., DRM). It splits the macro into a standard version and a _REF version, modifies argument handling, and updates the selftest modules. > Modify ddebug_sync_classbits() argtype deref inside the fn, to give > access to all kp members. [Severity: Low] Is this part of the commit message accurate? The commit message mentions modifying ddebug_sync_classbits(), but looking at the patch, its body remains unchanged. > diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h > index 471b9891bd83b..a740b3fabc09e 100644 > --- a/include/linux/dynamic_debug.h > +++ b/include/linux/dynamic_debug.h [ ... ] > @@ -291,6 +291,44 @@ struct ddebug_class_param { [ ... ] > +/** > + * DYNAMIC_DEBUG_CLASSMAP_PARAM_REF - wrap a classmap with a controlling > sys-param > + * @_name: sysfs node name > + * @_bits: name of the module's unsigned long bit-vector, ex: __drm_debug [Severity: Low] Does this comment correctly describe the expected type? The documentation mentions @_bits expects an unsigned long, but the underlying struct ddebug_class_param uses a u32 *. Could passing an unsigned long variable here cause endianness issues on 64-bit big-endian systems? > + * @_var: name of the (exported) classmap var defining the classes/bits > + * @_flags: flags to be toggled, typically just 'p' [ ... ] > @@ -473,6 +511,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_PARAM(_name, _var, _flags) > +#define DYNAMIC_DEBUG_CLASSMAP_PARAM_REF(_name, _var, _flags) [Severity: High] Should DYNAMIC_DEBUG_CLASSMAP_PARAM_REF accept four arguments here? The stub macro for the !CONFIG_DYNAMIC_DEBUG configuration only takes three arguments (_name, _var, _flags), but callers of the active macro pass four. This will cause compilation to fail for any modules using it when CONFIG_DYNAMIC_DEBUG is disabled. > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c > index 0f89b6784ab26..3f9821f747d80 100644 > --- a/lib/dynamic_debug.c > +++ b/lib/dynamic_debug.c > @@ -697,9 +697,42 @@ static int ddebug_apply_class_bitmap(const struct > ddebug_class_param *dcp, [ ... ] > +static void __maybe_unused ddebug_class_param_clamp_input(u32 *inrep, const > struct kernel_param *kp) > +{ > + const struct ddebug_class_param *dcp = kp->arg; > + const struct ddebug_class_map *map = dcp->map; > + > + switch (map->map_type) { [ ... ] > + case DD_CLASS_TYPE_LEVEL_NUM: > + /* input is bitpos, of highest verbosity to be enabled */ > + if (*inrep > map->length) { > + pr_warn("%s: level:%d exceeds max:%d, clamping\n", > + KP_NAME(kp), *inrep, map->length); [Severity: Low] Does this format string match the type of *inrep? Since inrep is a u32 *, using %d might result in misleading warning logs where large unsigned values are printed as negative integers. > + *inrep = map->length; > + } > + break; > + } > +} [ ... ] > @@ -718,31 +751,21 @@ static int param_set_dyndbg_module_classes(const char > *instr, [ ... ] > case DD_CLASS_TYPE_LEVEL_NUM: > old_val = READ_ONCE(*dcp->lvl); > old_bits = CLASSMAP_BITMASK(old_val); > new_bits = CLASSMAP_BITMASK(inrep); > v2pr_info("lvl:%u bits:0x%x > %s\n", inrep, new_bits, > KP_NAME(kp)); > + v2pr_info("lvl:%u bits:0x%x > %s\n", inrep, new_bits, > KP_NAME(kp)); [Severity: Low] It looks like this v2pr_info() statement was accidentally duplicated. > totct += ddebug_apply_class_bitmap(dcp, &new_bits, old_bits, > mod_name); > WRITE_ONCE(*dcp->lvl, inrep); > break; > diff --git a/lib/test_dynamic_debug.c b/lib/test_dynamic_debug.c > index 39499e52d7c04..def44524b762d 100644 > --- a/lib/test_dynamic_debug.c > +++ b/lib/test_dynamic_debug.c > @@ -67,32 +68,7 @@ static const struct kernel_param_ops param_ops_do_repeats > = { [ ... ] > -#endif > +#define CLASSMAP_BITMASK(width, base) (((1ULL << (width)) - 1) << (base)) [Severity: Low] Is this CLASSMAP_BITMASK macro used anywhere in the test module? It seems to be defined but left unused, which might cause confusion since it shares a name with a macro in lib/dynamic_debug.c but takes different arguments. > /* > * Demonstrate/test DISJOINT & LEVEL typed classmaps with a sys-param. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=32
