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: media: meson: vdec: Handle kthread failure and free codec state
Date: Thu, 04 Jun 2026 15:48:46 +1000
Message-ID: <review-patch3-20260530094326.11892-4-linux.amoon@gmail.com>
In-Reply-To: <20260530094326.11892-4-linux.amoon@gmail.com>
References: <20260530094326.11892-1-linux.amoon@gmail.com> <20260530094326.11892-4-linux.amoon@gmail.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

**Bug =E2=80=94 double free of `sess->priv`:**
```c
err_cleanup:
	vdec_free_canvas(sess);
	vdec_poweroff(sess);
	if (codec_ops && codec_ops->stop && sess->priv) {
		codec_ops->stop(sess);
		kfree(sess->priv);
		sess->priv =3D NULL;
	}
```

`vdec_poweroff()` calls `vdec_ops->stop(sess)` (i.e., `vdec_1_stop` or `vde=
c_hevc_stop`). Looking at the existing code in `vdec_1.c:160-162`:
```c
if (sess->priv)
    codec_ops->stop(sess);
```
So `vdec_poweroff()` =E2=86=92 `vdec_ops->stop()` already calls `codec_ops-=
>stop()`. Then the error path calls `codec_ops->stop(sess)` again. The code=
c stop functions (e.g., `codec_h264_stop`, `codec_vp9_stop`) free internal =
DMA allocations tracked in `sess->priv` =E2=80=94 calling it twice leads to=
 double-free of those DMA buffers. After that, the explicit `kfree(sess->pr=
iv)` is yet another issue: the codec's own stop function does NOT free `ses=
s->priv` itself (the normal teardown path in `vdec_stop_streaming` does `kf=
ree(sess->priv)`), but this second `codec_ops->stop()` will operate on alre=
ady-freed internal state.

**The fix should be:** remove the `codec_ops->stop()` call from `err_cleanu=
p` since `vdec_poweroff()` already handles it, and just do:
```c
err_cleanup:
	vdec_poweroff(sess);
	vdec_free_canvas(sess);
	kfree(sess->priv);
	sess->priv =3D NULL;
```

**Also note:** `vdec_free_canvas()` is called before `vdec_poweroff()`, but=
 in `vdec_stop_streaming()` the order is `vdec_poweroff()` then `vdec_free_=
canvas()`. The ordering should be consistent =E2=80=94 power off first, the=
n free resources.

**Good:** Adding `IS_ERR()` check for `kthread_run()` is a genuine fix.

**Good:** The `sess->vififo_vaddr =3D NULL` guard and the `vififo_vaddr` ch=
eck in `vdec_stop_streaming` are defensive but reasonable.

---
Generated by Claude Code Patch Reviewer