Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] Clearing the active session pointer while the hardware is still running causes unacknowledged interrupts and a CPU-locking interrupt storm. - [High] Premature clearing of the active session pointer in job_abort bypasses hardware teardown and resource cleanup, causing severe memory and resource leaks. - [High] AB-BA lock inversion deadlock between VIDIOC_STREAMOFF and the esparser_queue_work worker thread. - [High] Releasing session ownership without stopping the hardware allows concurrent DMA overwrites and hardware state corruption. - [High] Infinite loop inside esparser_queue_all_src() causing 100% CPU lockup when the queue is empty. --
--- Patch [13]: [PATCH v7 13/19] media: meson: vdec: Fix race conditions in job abort sequence --- commit bc32f101c548ab648eb44ae46f9cc2ea72d4c339 Author: Anand Moon <[email protected]> media: meson: vdec: Fix race conditions in job abort sequence Implement proper cleanup inside vdec_m2m_job_abort to safely stop the hardware job sequence during a streaming abort or teardown. > diff --git a/drivers/staging/media/meson/vdec/vdec.c > b/drivers/staging/media/meson/vdec/vdec.c > index b3e1d99e8889f..ac86a9c4febff 100644 > --- a/drivers/staging/media/meson/vdec/vdec.c > +++ b/drivers/staging/media/meson/vdec/vdec.c > @@ -167,6 +167,17 @@ static void vdec_m2m_device_run(void *priv) > static void vdec_m2m_job_abort(void *priv) > { > struct amvdec_session *sess = priv; > + struct amvdec_core *core = sess->core; > + > + WRITE_ONCE(sess->should_stop, 1); > + > + cancel_work_sync(&sess->esparser_queue_work); [Severity: High] Does this introduce an AB-BA deadlock with VIDIOC_STREAMOFF? The V4L2 core acquires the queue lock (sess->lock) during VIDIOC_STREAMOFF and calls vdec_m2m_job_abort(), which blocks here indefinitely waiting for the worker. Meanwhile, the worker thread unconditionally attempts to acquire sess->lock in esparser_queue_all_src(): drivers/staging/media/meson/vdec/esparser.c:esparser_queue_all_src() { ... scoped_guard(mutex, &sess->lock) { ... } > + > + mutex_lock(&core->lock); > + if (core->cur_sess == sess) > + /* Safely clear hardware ownership since we were confirmed as > the owner */ > + smp_store_release(&core->cur_sess, NULL); [Severity: High] Does clearing cur_sess here prematurely bypass hardware teardown? During a normal VIDIOC_STREAMOFF, vdec_m2m_job_abort() is called before the stop_streaming hook. If cur_sess is cleared here, the later teardown in vdec_stop_streaming() will skip powering off the hardware and freeing the DMA buffer (vififo_size) because core->cur_sess == sess evaluates to false: drivers/staging/media/meson/vdec/vdec.c:vdec_stop_streaming() { ... if (core->cur_sess == sess) { vdec_poweroff(sess); vdec_free_canvas(sess); ... } [Severity: Critical] Can this cause an interrupt storm and CPU lockup? Since the physical hardware isn't stopped, it may complete its active task and assert a level-triggered interrupt. The ISR checks cur_sess, reads NULL, and immediately returns IRQ_HANDLED without acknowledging or clearing the hardware register: drivers/staging/media/meson/vdec/vdec.c:vdec_isr() { ... if (!sess) return IRQ_HANDLED; ... } Will this cause the unacknowledged interrupt to immediately fire again, locking up the CPU? [Severity: High] Could this allow concurrent DMA overwrites and hardware state corruption? By clearing core->cur_sess here, the V4L2 framework considers the hardware idle and can schedule a new session. Since the hardware is actually still running, a newly scheduled session might blindly overwrite the active hardware's registers in esparser_queue(), corrupting the ongoing DMA operations: drivers/staging/media/meson/vdec/esparser.c:esparser_queue() { ... amvdec_write_parser(core, PARSER_VIDEO_START_PTR, payload_paddr); ... } > + mutex_unlock(&core->lock); > > v4l2_m2m_job_finish(sess->core->m2m_dev, sess->m2m_ctx); > } [Severity: High] Does the esparser_queue_all_src() worker have an infinite loop? When looking into the interaction with esparser_queue_work, the outer loop in esparser_queue_all_src() appears to never terminate when the queue is empty: drivers/staging/media/meson/vdec/esparser.c:esparser_queue_all_src() { while (1) { struct vb2_v4l2_buffer *vbuf = NULL; bool processed_frame = false; ... scoped_guard(mutex, &sess->lock) { ... vbuf = v4l2_m2m_next_src_buf(sess->m2m_ctx); if (!vbuf) break; ... } ... } } Since scoped_guard is implemented as a hidden for-loop, the break statement only exits the scoped_guard's internal loop, not the outer while(1) loop. Because processed_frame remains false, the code calls cond_resched() and spins indefinitely. Could this cause a 100% CPU lockup? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=13
