From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: media: meson: vdec: Fix concurrent STREAMON / STREAMOFF race conditions Date: Thu, 04 Jun 2026 15:48:46 +1000 Message-ID: In-Reply-To: <20260530094326.11892-3-linux.amoon@gmail.com> References: <20260530094326.11892-1-linux.amoon@gmail.com> <20260530094326.11892-3-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 **Good idea:** Adding mutex protection around `core->cur_sess` and `sess->s= tatus` transitions. **Issue =E2=80=94 `STATUS_INIT` set too early:** The patch sets `sess->stat= us =3D STATUS_INIT` and `core->cur_sess =3D sess` inside the mutex *before*= any hardware initialization (vififo allocation, poweron, codec start, kthr= ead). If any of these fail, the error unwind correctly resets the state, bu= t there's a window where `sess->status =3D=3D STATUS_INIT` while the hardwa= re isn't actually initialized. Another call to `vdec_stop_streaming()` duri= ng this window could try to stop a partially-initialized session, calling `= kthread_stop()` on an uninitialized `recycle_thread`, `vdec_poweroff()` on = hardware that was never powered on, etc. The original code set `STATUS_INIT= ` and `cur_sess` at the end, which was better in that regard (though also r= acy). **Issue =E2=80=94 `STATUS_INIT` early return bypasses mutex unlock properly= :** ```c if (sess->status =3D=3D STATUS_INIT) { mutex_unlock(&core->lock); return 0; } ``` This is fine structurally but means a second STREAMON on a session that's a= lready STATUS_INIT but not yet STATUS_RUNNING returns success without compl= eting initialization. This might already be the intent of the original code= though. **Issue =E2=80=94 stop_streaming sets STATUS_STOPPED before hardware teardo= wn:** ```c mutex_lock(&core->lock); old_status =3D sess->status; if (core->cur_sess =3D=3D sess) core->cur_sess =3D NULL; sess->status =3D STATUS_STOPPED; mutex_unlock(&core->lock); ``` Setting `STATUS_STOPPED` and clearing `cur_sess` before actually stopping t= he hardware means that during the hardware teardown (kthread_stop, vdec_pow= eroff, etc.), a concurrent `vdec_start_streaming()` could see `STATUS_STOPP= ED` / `cur_sess =3D=3D NULL` and attempt to start a new session while the o= ld one is still being torn down. The mutex only protects the state variable= s, not the actual hardware operations that follow. --- Generated by Claude Code Patch Reviewer