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: Sat, 16 May 2026 10:50:10 +1000
Message-ID: <review-patch10-20260514134052.361771-11-robin.clark@oss.qualcomm.com>
In-Reply-To: <20260514134052.361771-11-robin.clark@oss.qualcomm.com>
References: <20260514134052.361771-1-robin.clark@oss.qualcomm.com> <20260514134052.361771-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

This sets up the core `msm_perfcntr_state` and `msm_perfcntr_stream` structures, and the init/cleanup/resume/suspend framework.

**Issue: `devm_kzalloc` + manual `devm_kfree` in cleanup.** The `msm_perfcntr_init()` uses `devm_kzalloc()` but `msm_perfcntr_cleanup()` uses `devm_kfree()`. This works but is unusual and somewhat fragile - if cleanup is ever called late (or not called), devm would double-free. Consider using plain `kzalloc`/`kfree` since the lifetime is manually managed anyway.

```c
void msm_perfcntr_cleanup(struct msm_gpu *gpu)
{
    ...
    for (unsigned i = 0; i < gpu->num_perfcntr_groups; i++)
        devm_kfree(dev, perfcntrs->groups[i]);
    devm_kfree(dev, perfcntrs);
}
```

**Issue: `msm_perfcntr_suspend()` null check inconsistency.** `msm_perfcntr_suspend()` checks `gpu->perfcntr_groups` while `msm_perfcntr_resume()` checks `gpu->perfcntrs`. These should be consistent. If a GPU has no perfcntr support, both will be NULL/0, so it works in practice, but the asymmetry is confusing.

**Typo:** In `msm_perfcntr_group_state`, comment says `correspnding` - should be `corresponding`.

---
Generated by Claude Code Patch Reviewer