From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/msm: Add basic perfcntr infrastructure
Date: Mon, 25 May 2026 18:21:40 +1000
Message-ID: <review-patch10-20260522173349.55491-11-robin.clark@oss.qualcomm.com>
In-Reply-To: <20260522173349.55491-11-robin.clark@oss.qualcomm.com>
References: <20260522173349.55491-1-robin.clark@oss.qualcomm.com> <20260522173349.55491-11-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

The `msm_perfcntr_init` error path properly cleans up partially allocated groups:
```c
if (!perfcntrs->groups[i]) {
    __msm_perfcntr_cleanup(gpu, perfcntrs);
    return ERR_PTR(-ENOMEM);
}
```

`__msm_perfcntr_cleanup` calls `devm_kfree` on each group, which is safe for NULL pointers (devm_kfree on NULL is a no-op).

The use of `devm_kzalloc`/`devm_kfree` for perfcntr state that has explicit cleanup is unusual -- typically devm is for fire-and-forget allocations. It works but `kzalloc`/`kfree` would be more conventional here. Not a bug though -- the devm cleanup would serve as a safety net if `msm_perfcntr_cleanup` were missed.

In `msm_gpu_sysprof_no_perfcntr_zap`:
```c
return (refcount_read(&gpu->sysprof_active) > 1) ||
    (gpu->perfcntrs && READ_ONCE(gpu->perfcntrs->stream));
```

The `READ_ONCE` is appropriate since `stream` can be updated from other threads.

---
Generated by Claude Code Patch Reviewer