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: Thu, 07 May 2026 13:19:05 +1000
Message-ID: <review-patch10-20260506171127.133572-11-robin.clark@oss.qualcomm.com>
In-Reply-To: <20260506171127.133572-11-robin.clark@oss.qualcomm.com>
References: <20260506171127.133572-1-robin.clark@oss.qualcomm.com> <20260506171127.133572-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

**Status: Minor issues**

This introduces `msm_perfcntr_state`, `msm_perfcntr_group_state`, and the init/cleanup/resume/suspend functions.

**Issue 1: Inconsistent null-check in suspend vs resume**

In `msm_perfcntr_resume()`, the guard checks `gpu->perfcntrs`:
```c
if (!gpu->perfcntrs)
    return 0;
```
But `msm_perfcntr_suspend()` checks `gpu->perfcntr_groups`:
```c
if (!gpu->perfcntr_groups)
    return;
```
These should be consistent. Since `perfcntrs` is only allocated when `num_perfcntr_groups > 0`, both work in practice, but it's confusing. Prefer checking the same field in both.

**Issue 2: Potential null deref in msm_perfcntr_resume()**

`msm_perfcntr_resume_locked()` is called with `gpu->perfcntrs->stream`, but this could be NULL. In the base version of this patch (before patch 13 adds the null check), `msm_perfcntr_resume_locked()` is a no-op stub, so it's fine. But this is fragile across patches -- worth noting that patch 13 does add the null check inside `msm_perfcntr_resume_locked()`.

**Issue 3: devm_kfree in cleanup for error path**

```c
void msm_perfcntr_cleanup(struct msm_gpu *gpu)
{
    struct msm_perfcntr_state *perfcntrs = gpu->perfcntrs;
    ...
    for (unsigned i = 0; i < gpu->num_perfcntr_groups; i++)
        devm_kfree(dev, perfcntrs->groups[i]);
```
If `msm_perfcntr_init()` fails mid-way through allocating group states, `msm_perfcntr_cleanup()` is called but `gpu->perfcntrs` has not been set yet -- it's only set by the caller *after* `msm_perfcntr_init()` returns. So `gpu->perfcntrs` is NULL, and this will crash. The init function calls `msm_perfcntr_cleanup(gpu)` on failure but `gpu->perfcntrs` hasn't been assigned yet at that point.

---
Generated by Claude Code Patch Reviewer