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: Add basic perfcntr infrastructure Date: Mon, 25 May 2026 21:26:29 +1000 Message-ID: In-Reply-To: <20260520162454.18391-11-robin.clark@oss.qualcomm.com> References: <20260520162454.18391-1-robin.clark@oss.qualcomm.com> <20260520162454.18391-11-robin.clark@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review The core infrastructure patch. A few observations: - `msm_perfcntr_init()` uses `devm_kzalloc()` for both the top-level state = and the per-group state. But `msm_perfcntr_cleanup()` explicitly calls `dev= m_kfree()` on each. This is fine but slightly unusual =E2=80=94 the `devm_`= allocations would be freed automatically on device teardown. The explicit = cleanup is presumably for the error path in `msm_gpu_init()`. - In `msm_perfcntr_group_idx()`: ```c WARN_ON_ONCE(n > stream->nr_groups); ``` This should be `>=3D` since valid indices are `0..nr_groups-1`. Accessing= `stream->group_idx[n]` when `n =3D=3D nr_groups` is already out of bounds. - `msm_perfcntr_suspend()` checks `gpu->perfcntr_groups` while `msm_perfcnt= r_resume()` checks `gpu->perfcntrs`. Slightly inconsistent =E2=80=94 both s= hould check the same thing. The intent is to avoid dereferencing `gpu->perf= cntrs` when null, and `perfcntr_groups` being set implies the GPU generatio= n supports it, but `perfcntrs` could be NULL if init failed. Using `gpu->pe= rfcntrs` consistently would be clearer. --- Generated by Claude Code Patch Reviewer