Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [Critical] The patch introduces an unhandled double decrement of `sess->esparser_queued_bufs`, causing the atomic counter to underflow, corrupting stream state evaluation, and deadlocking the driver. - [High] Using `break` inside `scoped_guard` fails to exit the worker loop, resulting in a 100% CPU infinite loop deadlock. - [High] Returning 0 on `!vaddr` in `esparser_pad_start_code()` skips writing the mandatory start code pattern, leading to hardware out-of-bounds reads. - [Medium] The commit message erroneously claims to relocate a decrement from the 'entry point' of the function, presenting a false premise for the change. Pre-existing issues: - [High] Missing decrement of `esparser_queued_bufs` on error paths permanently leaks logical buffer slots. -- --- Patch [14]: [PATCH v7 14/19] media: meson: vdec: Correct atomic counter placement in dst_buf_done --- commit 3bacd92d37472439ff6f7550b68eb503f9d760f0 Author: Anand Moon <[email protected]> media: meson: vdec: Correct atomic counter placement in dst_buf_done This commit fixes an end-of-stream boundary condition by relocating the atomic decrement helper to execute after state evaluation. It aims to ensure accurate stream termination signaling. > Previously, decrementing sess->esparser_queued_bufs at the entry point > of the function corrupted the conditional checks for the final video > frame. [Severity: Medium] Is this description accurate? Looking at the baseline code, dst_buf_done() never contained an atomic decrement at its entry point or anywhere else inside the function. The decrement only existed in the caller functions. > diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c > b/drivers/staging/media/meson/vdec/vdec_helpers.c > index f02c21d5a9c18..303236f0647c0 100644 > --- a/drivers/staging/media/meson/vdec/vdec_helpers.c > +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c > @@ -314,6 +314,9 @@ static void dst_buf_done(struct amvdec_session *sess, > dev_dbg(dev, "Buffer %u done, ts = %llu, flags = %08X\n", > vbuf->vb2_buf.index, timestamp, flags); > vbuf->field = field; > + > + atomic_dec(&sess->esparser_queued_bufs); > + [Severity: Critical] Does this introduce a double decrement? Since the callers like amvdec_dst_buf_done() and amvdec_dst_buf_done_offset() still contain their original atomic_dec() calls, this new decrement causes the counter to underflow. When the counter becomes negative, the EOS condition atomic_read(&sess->esparser_queued_bufs) <= 1 evaluates to true, which can falsely trigger V4L2_EVENT_EOS on the wrong buffer and cause a permanent -EAGAIN deadlock. > v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_DONE); > > /* Buffer done probably means the vififo got freed */ [Severity: High] This is a pre-existing issue, but does the error path in amvdec_dst_buf_done() permanently leak logical buffer slots? drivers/staging/media/meson/vdec/vdec_helpers.c:amvdec_dst_buf_done() { ... if (list_empty(&sess->timestamps)) { v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR); spin_unlock_irqrestore(&sess->ts_spinlock, flags); return; } ... } When list_empty() is true, the function returns early without decrementing sess->esparser_queued_bufs. If this triggers enough times, the counter artificially hits the max limit, preventing new buffers from being queued. [Severity: High] Does the break statement in esparser_queue_all_src() inadvertently cause an infinite loop? drivers/staging/media/meson/vdec/esparser.c:esparser_queue_all_src() { while (1) { scoped_guard(mutex, &sess->lock) { ... if (!vbuf) break; ... } ... } } Because scoped_guard() is implemented using an internal for-loop, calling break here only exits the scoped_guard's internal scope, not the outer while (1) loop. This causes the worker thread to permanently hang in an infinite busy-wait loop. [Severity: High] Can the failure to propagate an error in esparser_pad_start_code() cause hardware out-of-bounds reads? drivers/staging/media/meson/vdec/esparser.c:esparser_pad_start_code() { ... if (!sess || READ_ONCE(sess->should_stop) || !sess->priv || !vaddr) return 0; ... } If a client provides an unmapped buffer, !vaddr is true and this returns 0, skipping the mandatory 512-byte start code padding. Its caller esparser_queue() ignores this and passes the physical address to esparser_write_data(), which unconditionally adds 512 to the payload size. Since the padding wasn't written, the hardware DMA engine reads past the valid buffer boundary. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
