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 15:20:24 +1000
Message-ID: <review-patch10-20260511130017.96867-11-robin.clark@oss.qualcomm.com>
In-Reply-To: <20260511130017.96867-11-robin.clark@oss.qualcomm.com>
References: <20260511130017.96867-1-robin.clark@oss.qualcomm.com> <20260511130017.96867-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

Several issues:

**(1) NULL pointer dereference in init error path.** `msm_perfcntr_init()` calls `msm_perfcntr_cleanup(gpu)` on inner allocation failure, but `gpu->perfcntrs` hasn't been set yet at that point (the caller assigns the return value). `msm_perfcntr_cleanup()` dereferences `gpu->perfcntrs`:

```c
struct msm_perfcntr_state *perfcntrs = gpu->perfcntrs; // NULL!
gpu->perfcntrs = NULL;
for (unsigned i = 0; i < gpu->num_perfcntr_groups; i++)
    devm_kfree(dev, perfcntrs->groups[i]);  // CRASH
```

Fix: either pass the `perfcntrs` pointer to cleanup, or just remove the `msm_perfcntr_cleanup()` call from init since `devm_kzalloc` will free everything on device release.

**(2) Off-by-one in `msm_perfcntr_group_idx()`:**

```c
WARN_ON_ONCE(n > stream->nr_groups);
```

Should be `n >= stream->nr_groups` since valid indices are `0..nr_groups-1`.

**(3) Inconsistent null check in suspend vs resume:**

```c
// resume checks:
if (!gpu->perfcntrs)
    return 0;

// suspend checks:
if (!gpu->perfcntr_groups)
    return;
```

If `perfcntr_groups` is set but `perfcntrs` is NULL (shouldn't happen, but defensively), suspend would dereference NULL. Should use the same guard: `if (!gpu->perfcntrs)`.

**(4) Typo:** "correspnding" in the `countables` doc comment.

---
Generated by Claude Code Patch Reviewer