Re: [PATCH v9 38/39] dyndbg: add +c flag to count advantage of classmaps for DRM
flat view
From: sashiko-bot@kernel.org
Date: 2026-09-09 01:11:30
Also in:
dri-devel
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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. [ ... ]
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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/20260908-dd-cmap-part2-clean-v9-0-3a3f621f4a10@gmail.com?part=38