Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] dyndbg: unused ddebug_class_param_to_bits() and unclamped init in ddebug_sync_classbits() --
commit 061cb31380a7641ad87f8c44222d55e83ea7da16 Author: Jim Cromie <[email protected]> dyndbg-API: replace DECLARE_DYNDBG_CLASSMAP This commit replaces DECLARE_DYNDBG_CLASSMAP with DYNAMIC_DEBUG_CLASSMAP_DEFINE and DYNAMIC_DEBUG_CLASSMAP_USE to support exporting a classmap from a core module and referencing it from user modules. At module load time, dynamic debug scans both classmap sections and syncs the initial class bitmap from the controlling kernel parameter. > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c > index 18289ca7170b7..ad86365f2474f 100644 > --- a/lib/dynamic_debug.c > +++ b/lib/dynamic_debug.c [ ... ] > @@ -1174,6 +1213,163 @@ static const struct proc_ops proc_fops = { > .proc_write = ddebug_proc_write > }; > > +#define vpr_cm_info(cm_p, msg_fmt, ...) ({ \ > + struct ddebug_class_map const *_cm = cm_p; \ > + v2pr_info(msg_fmt "%s [%d..%d] %s..%s\n", ##__VA_ARGS__, \ > + _cm->mod_name, _cm->base, _cm->base + _cm->length, \ > + _cm->class_names[0], _cm->class_names[_cm->length - 1]); \ > + }) > + > +/* > + * Modules which define classmaps get them initialized by > + * param-callback via module.c:parse_one. Modules which use other's > + * classmaps must be initialized explicitly. > + */ > +static inline u32 ddebug_class_param_to_bits(const struct ddebug_class_param > *dcp) > +{ > + const struct ddebug_class_map *map = dcp->map; > + > + switch (map->map_type) { > + case DD_CLASS_TYPE_DISJOINT_BITS: > + return *dcp->bits & CLASSMAP_BITMASK(map->length); > + case DD_CLASS_TYPE_LEVEL_NUM: > + return CLASSMAP_BITMASK(min_t(u32, *dcp->lvl, map->length)); > + default: > + return 0; > + } > +} > + > +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_DISJOINT_BITS: > + /* expect bits. mask and warn if too many */ > + if (*inrep & ~CLASSMAP_BITMASK(map->length)) { > + pr_warn("%s: input: 0x%x exceeds mask: 0x%x, masking\n", > + KP_NAME(kp), *inrep, > CLASSMAP_BITMASK(map->length)); > + *inrep &= CLASSMAP_BITMASK(map->length); > + } > + break; > + 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); > + *inrep = map->length; > + } > + break; > + } > +} > + > +/* > + * Called for class-users and native classmaps during module load. > + * > + * Initial sync assumes 0 as old_bits, so it only asserts (+p) classes > + * explicitly set by the parameter. -DDEBUGs invalidate this assumption > + * for some pr_debugs; a user can override the developer/builder choice > + * by toggling the param at runtime. > + * > + * Parameters are sampled via READ_ONCE without ddebug_lock; > + * holding ddebug_lock here would deadlock against ddebug_change(). > + */ > +static void ddebug_sync_classbits(const struct kernel_param *kp, const char > *modname) > +{ > + const struct ddebug_class_param *dcp = kp->arg; > + u32 val, new_bits; > + > + if (!dcp || !dcp->map) > + return; > + > + switch (dcp->map->map_type) { > + case DD_CLASS_TYPE_DISJOINT_BITS: > + val = READ_ONCE(*dcp->bits); > + new_bits = val; > + v2pr_info(" %s: classbits: 0x%x\n", KP_NAME(kp), new_bits); > + ddebug_apply_class_bitmap(dcp, &new_bits, 0, modname); > + break; > + case DD_CLASS_TYPE_LEVEL_NUM: > + val = READ_ONCE(*dcp->lvl); > + new_bits = CLASSMAP_BITMASK(val); > + v2pr_info(" %s: lvl:%d bits:0x%x\n", KP_NAME(kp), val, > new_bits); > + ddebug_apply_class_bitmap(dcp, &new_bits, 0, modname); > + break; [Severity: Low] Is ddebug_class_param_to_bits() intended to be used here, or should it be dropped? It is introduced in this patch, but never called here or in the rest of the series (while ddebug_class_param_clamp_input() is only wired into param_set_dyndbg_module_classes() in the later commit "dyndbg-API: promote DYNAMIC_DEBUG_CLASSMAP_PARAM to API"). Also, in lib/test_dynamic_debug.c (and later in include/linux/dynamic_debug.h when promoted in "dyndbg-API: promote DYNAMIC_DEBUG_CLASSMAP_PARAM to API"), DYNAMIC_DEBUG_CLASSMAP_PARAM initializes the backing u32 to ~0 when DEBUG is defined: #ifdef DEBUG #define DYNAMIC_DEBUG_CLASSMAP_PARAM(_model, _flags) \ DYNAMIC_DEBUG_CLASSMAP_PARAM_(_model, _flags, ~0) #else #define DYNAMIC_DEBUG_CLASSMAP_PARAM(_model, _flags) \ DYNAMIC_DEBUG_CLASSMAP_PARAM_(_model, _flags, 0) #endif Because ddebug_sync_classbits() reads *dcp->bits and *dcp->lvl directly without masking or clamping them to dcp->map->length, won't param_get_dyndbg_classes() report 0xffffffff for DD_CLASS_TYPE_DISJOINT_BITS (instead of CLASSMAP_BITMASK(map->length)) and 4294967295 for DD_CLASS_TYPE_LEVEL_NUM (instead of map->length) when built with -DDEBUG? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=22
