Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] The `+c` flag unintentionally triggers heavy logging and side-effects, and fails to increment the call counter for open-coded `DYNAMIC_DEBUG_BRANCH` callers. - [Medium] The `reset_stats` command parsing in `ddebug_proc_write` silently drops batched queries, ignores leading whitespace, and falsely matches partial commands. --
commit 02710e8ae9ac8d54e910e1db01d0bb84fea61ee7 Author: Jim Cromie <[email protected]> dyndbg: add +c flag to count advantage of classmaps for DRM This commit introduces a +c flag to dynamic debug, allowing users to increment a per-cpu counter when a flagged pr_debug() is called. It also adds a reset_stats command to /proc/dynamic_debug/control. > diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h > index 39de59c8bac2c..064df695b3a4c 100644 > --- a/include/linux/dynamic_debug.h > +++ b/include/linux/dynamic_debug.h [ ... ] > @@ -403,14 +407,20 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor, > > #ifdef DEBUG > #define DYNAMIC_DEBUG_BRANCH(descriptor) \ > - likely(descriptor.flags & _DPRINTK_FLAGS_PRINT) > + likely(descriptor.flags & _DPRINTK_FLAGS_ENABLED) > #else > #define DYNAMIC_DEBUG_BRANCH(descriptor) \ > - unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT) > + unlikely(descriptor.flags & _DPRINTK_FLAGS_ENABLED) > #endif > > #endif /* CONFIG_JUMP_LABEL */ [Severity: High] Does changing this macro to check for _DPRINTK_FLAGS_ENABLED (which includes the new +c count flag) break open-coded callers of DYNAMIC_DEBUG_BRANCH? For instance, in include/linux/printk.h, pr_debug_ratelimited() uses this: if (DYNAMIC_DEBUG_BRANCH(descriptor) && __ratelimit(&_rs)) __dynamic_pr_debug(&descriptor, pr_fmt(fmt), ##__VA_ARGS__); If a user enables only the +c flag, DYNAMIC_DEBUG_BRANCH evaluates to true, but because it bypasses the DYNAMIC_DEBUG_COUNT() macro, it fails to increment the counter. Furthermore, it falls through to __dynamic_pr_debug() causing it to print anyway. Other callers like alloc_contig_dump_pages() in mm/page_alloc.c or sdhci_dbg_dumpregs() in drivers/mmc/host/sdhci.h will similarly execute full stack and register dumps unintentionally if the +c flag is set, without counting the calls. [ ... ] > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c > index 77976e7b264ca..9fa1a6e3afe6c 100644 > --- a/lib/dynamic_debug.c > +++ b/lib/dynamic_debug.c [ ... ] > @@ -1104,6 +1122,11 @@ static ssize_t ddebug_proc_write(struct file *file, > const char __user *ubuf, > return PTR_ERR(tmpbuf); > v2pr_info("read %zu bytes from userspace\n", len); > > + if (len >= 11 && !strncmp(tmpbuf, "reset_stats", 11)) { > + reset_ddebug_call_count(); > + kfree(tmpbuf); > + return len; > + } > ret = ddebug_exec_queries(tmpbuf, NULL); > kfree(tmpbuf); > if (ret < 0) [Severity: Medium] Could this early return logic silently drop batched queries? Since ddebug_exec_queries() handles parsing multiple commands separated by newlines or semicolons, returning early here means a batched input like "reset_stats\nfile foo.c +c" would reset the stats but completely ignore the subsequent commands without returning an error. Additionally, does this strncmp() check falsely match partial commands like "reset_stats_and_more", while also failing to handle leading whitespace? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=38
