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/adreno: fix userspace-triggered crash on a2xx-a4xx
Date: Sun, 12 Apr 2026 14:17:19 +1000
Message-ID: <review-patch1-20260407-adreno-fix-ubwc-v1-1-bb2b09450b87@oss.qualcomm.com>
In-Reply-To: <20260407-adreno-fix-ubwc-v1-1-bb2b09450b87@oss.qualcomm.com>
References: <20260407-adreno-fix-ubwc-v1-1-bb2b09450b87@oss.qualcomm.com> <20260407-adreno-fix-ubwc-v1-1-bb2b09450b87@oss.qualcomm.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**The bug is real and the fix prevents the crash.** The NULL dereference is=
 userspace-triggerable via the `MSM_GET_PARAM` ioctl, making this a local D=
oS / kernel oops from unprivileged userspace. The `Fixes:` tag is appropria=
te.

**Design concern: returning fabricated defaults vs returning -EINVAL.**

The patch returns hardcoded values (`13` for `HIGHEST_BANK_BIT`, `0` for th=
e others) when `ubwc_config` is NULL:

```c
	case MSM_PARAM_HIGHEST_BANK_BIT:
		if (!adreno_gpu->ubwc_config)
			*value =3D 13;
		else
			*value =3D adreno_gpu->ubwc_config->highest_bank_bit;
```

While `13` is a reasonable default for `highest_bank_bit` (it matches the v=
alue used in `a5xx_gpu.c:838` where `BUG_ON(adreno_gpu->ubwc_config->highes=
t_bank_bit < 13)` shows 13 is the baseline), returning fabricated UBWC para=
meters for hardware that **doesn't support UBWC at all** is semantically qu=
estionable. These params are meaningless on a2xx-a4xx.

An alternative would be to return `-EINVAL` for these three params when `ub=
wc_config` is NULL, similar to how `MSM_PARAM_TIMESTAMP` returns `-EINVAL` =
when the GPU doesn't support timestamps. This would clearly communicate to =
userspace "this GPU does not have UBWC" rather than silently returning plau=
sible-looking but meaningless values. If existing userspace (e.g., freedren=
o in Mesa) depends on getting a success return from these params without ch=
ecking the GPU generation first, that would justify the current approach =
=E2=80=94 but a comment explaining why defaults were chosen over `-EINVAL` =
would be helpful.

**Minor style observation: repetitive NULL checks.**

The three cases repeat the same `if (!adreno_gpu->ubwc_config)` pattern. A =
small refactor could consolidate:

```c
	case MSM_PARAM_HIGHEST_BANK_BIT:
	case MSM_PARAM_UBWC_SWIZZLE:
	case MSM_PARAM_MACROTILE_MODE:
		if (!adreno_gpu->ubwc_config) {
			*value =3D (param =3D=3D MSM_PARAM_HIGHEST_BANK_BIT) ? 13 : 0;
			return 0;
		}
		/* fall through to individual handling */
```

But this is a matter of taste for a simple bugfix, and the current form is =
perfectly readable.

**Missing Cc: stable.** Given this is a userspace-triggerable NULL pointer =
dereference, this should probably carry `Cc: stable@vger.kernel.org` to get=
 backported to affected stable kernels (those carrying `a452510aad53`).

**Overall verdict:** The patch fixes a real crash. The only substantive que=
stion is whether returning `-EINVAL` would be more appropriate than returni=
ng dummy values for hardware that has no UBWC support. If the maintainers a=
re happy with the default-value approach (and Mesa handles it correctly), t=
he patch is ready to merge with the possible addition of a `Cc: stable` tag.

---
Generated by Claude Code Patch Reviewer