From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/msm/a6xx: Allow IFPC with perfcntr stream Date: Thu, 07 May 2026 13:19:07 +1000 Message-ID: In-Reply-To: <20260506171127.133572-17-robin.clark@oss.qualcomm.com> References: <20260506171127.133572-1-robin.clark@oss.qualcomm.com> <20260506171127.133572-17-robin.clark@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review **Status: Looks good with observation** Changes `sysprof_setup()` signature to take a `force_on` bool, then uses it in `sample_worker()` to briefly inhibit IFPC only while reading counters. This is a nice optimization: IFPC can stay active between samples, with SEL regs restored by the GMU via the pwrup reglist. The approach of calling `sysprof_setup(gpu, true)` before reading and `sysprof_setup(gpu, false)` after is clean. The `msm_gpu_sysprof_no_ifpc()` is now decoupled from the perfcntr stream check, which is the correct semantic separation. **Observation:** `sample_worker()` runs as kthread_work and calls `sysprof_setup()` which touches GMU registers. This should be safe since the GPU is powered (the timer/work are cancelled in suspend), but the function doesn't hold `gpu->lock`. If `sysprof_setup()` races with other GMU state changes, that could be problematic. In practice, the GMU IFPC disable/enable should be idempotent enough that this is okay, but worth verifying. --- **Summary of key items for the author:** 1. **bufsz_shift validation** (patch 13) -- shift before bounds check is UB for large values 2. **group_stride overflow** (patch 13) -- `copy_from_user` with `args->group_stride > sizeof(g)` overflows stack 3. **msm_perfcntr_cleanup crash** (patch 10) -- called before `gpu->perfcntrs` is assigned in init error path 4. **No reglist bounds checking** (patch 15) -- pwrup reglist writes aren't bounds-checked 5. **Missing O_CLOEXEC** (patch 13) -- fd leak risk across exec --- Generated by Claude Code Patch Reviewer