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/a6xx: Limit GXPD votes to recovery in A8x
Date: Sun, 12 Apr 2026 13:46:56 +1000
Message-ID: <review-patch6-20260407-gfx-clk-fixes-v1-6-4bb5583a5054@oss.qualcomm.com>
In-Reply-To: <20260407-gfx-clk-fixes-v1-6-4bb5583a5054@oss.qualcomm.com>
References: <20260407-gfx-clk-fixes-v1-0-4bb5583a5054@oss.qualcomm.com> <20260407-gfx-clk-fixes-v1-6-4bb5583a5054@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 is the most complex patch. The new helper functions cleanly separate the A6xx/A7xx and A8xx behaviors.

**Issue: Unchecked `pm_runtime_get_sync()` in `a6xx_gmu_gxpd_put()`.**

```c
	if (adreno_gpu->funcs->gx_is_on(adreno_gpu)) {
		pm_runtime_get_sync(gmu->gxpd);  // return value unchecked
		dev_pm_genpd_synced_poweroff(gmu->gxpd);
		pm_runtime_put_sync(gmu->gxpd);
	}
```

`pm_runtime_get_sync()` can fail (return negative). If it fails, the subsequent `synced_poweroff` + `put_sync` operate on a device with an incorrect refcount, potentially leaving GX in a bad state. At minimum, check the return value and log a warning, or propagate the error.

**Design observation:** The `a6xx_gmu_gxpd_put()` function returns `int` but the callers at the `a6xx_gmu_stop()` and `rpm_put:` error path in `a6xx_gmu_resume()` do not check the return value. For A6xx/A7xx, it returns the value from `pm_runtime_put_sync()`, which should be checked. For A8xx, it always returns 0 even if the internal `pm_runtime_get_sync()` failed. Consider making this void if return values aren't going to be checked, or actually check them at the call sites.

**Existing error path change:** In the `rpm_put:` label of `a6xx_gmu_resume()`:
```c
-	pm_runtime_put(gmu->gxpd);
+	a6xx_gmu_gxpd_put(gmu);
```

The old code called `pm_runtime_put()` (async) on `gmu->gxpd` directly, without NULL checking (the NULL check was only on the get side). If `gmu->gxpd` is NULL/ERR, calling `pm_runtime_put()` on it would crash. The new `a6xx_gmu_gxpd_put()` adds the `IS_ERR_OR_NULL` check, which is actually an improvement for the non-A8xx case too.

**Blank line removed between function definitions:**
```c
+}
+
 
 int a6xx_gmu_resume(struct a6xx_gpu *a6xx_gpu)
```

The diff shows the extra blank line between `gdsc_gx_do_nothing_enable` and `a6xx_gmu_resume` being consumed, which is fine -- one blank line between functions is correct style.

---
Generated by Claude Code Patch Reviewer