Re: [PATCH v8 32/43] dyndbg-API: promote DYNAMIC_DEBUG_CLASSMAP_PARAM to API

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 [off-list ref]

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.

[ ... ]

[ ... ]

+/**
+ * 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'

[ ... ]

[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.

[ ... ]

+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;
+	}
+}

[ ... ]

[ ... ]

 	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.

[ ... ]

-#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/20260905-dd-cmap-part2-clean-v8-0-a4cc0674f6fd@gmail.com?part=32