linux-kernel - Re: [PATCH v4 6/7] media: mediatek: vcodec: replace v4l2_m2m_next_src_buf with v4l2_m2m_src_buf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240823152338.n7i7cnvolvke2hqp@basti-XPS-13-9310>
Date: Fri, 23 Aug 2024 17:23:38 +0200
From: Sebastian Fricke <sebastian.fricke@...labora.com>
To: Yunfei Dong <yunfei.dong@...iatek.com>
Cc: Nícolas F . R . A . Prado <nfraprado@...labora.com>,
	Nicolas Dufresne <nicolas.dufresne@...labora.com>,
	Hans Verkuil <hverkuil-cisco@...all.nl>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Benjamin Gaignard <benjamin.gaignard@...labora.com>,
	Nathan Hebert <nhebert@...omium.org>,
	Daniel Almeida <daniel.almeida@...labora.com>,
	Hsin-Yi Wang <hsinyi@...omium.org>,
	Fritz Koenig <frkoenig@...omium.org>,
	Daniel Vetter <daniel@...ll.ch>, Steve Cho <stevecho@...omium.org>,
	linux-media@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org,
	Project_Global_Chrome_Upstream_Group@...iatek.com
Subject: Re: [PATCH v4 6/7] media: mediatek: vcodec: replace
 v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove

Hey Yunfei,

On 07.08.2024 16:24, Yunfei Dong wrote:
>There isn't lock to protect source buffer when get next src buffer,
>if the source buffer is removed for some unknown reason before lat
>work queue execute done, will lead to remove source buffer or buffer
>done error.

This is really hard to understand, can try wording this a bit clearer?
Stuff like: if the source buffer is removed ... will lead to remove
source buffer, just leaves me scratching my head.
And there is a spinlock in the m2m framework in `v4l2_m2m_next_buf` so I
suppose you mean something else when you say that there is no lock to
protect the source buffer?

You might not know all reasons but for this commit description you
should at least know one reason. Please highlight a case how this can
happen, so that you can justify the change.

>
>Signed-off-by: Yunfei Dong <yunfei.dong@...iatek.com>
>---
> .../vcodec/decoder/mtk_vcodec_dec_stateless.c | 30 +++++++++++++------
> 1 file changed, 21 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
>index 8aa379872ddc..3dba3549000a 100644
>--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
>+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
>@@ -321,6 +321,7 @@ static void mtk_vdec_worker(struct work_struct *work)
> 		container_of(work, struct mtk_vcodec_dec_ctx, decode_work);
> 	struct mtk_vcodec_dec_dev *dev = ctx->dev;
> 	struct vb2_v4l2_buffer *vb2_v4l2_src = ctx->last_vb2_v4l2_src;
>+	struct vb2_v4l2_buffer *vb2_v4l2_dst;
> 	struct vb2_buffer *vb2_src;
> 	struct mtk_vcodec_mem *bs_src;
> 	struct mtk_video_dec_buf *dec_buf_src;
>@@ -329,7 +330,7 @@ static void mtk_vdec_worker(struct work_struct *work)
> 	bool res_chg = false;
> 	int ret;
>
>-	vb2_v4l2_src = vb2_v4l2_src ? vb2_v4l2_src : v4l2_m2m_next_src_buf(ctx->m2m_ctx);
>+	vb2_v4l2_src = vb2_v4l2_src ? vb2_v4l2_src : v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
> 	if (!vb2_v4l2_src) {
> 		v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
> 		mtk_v4l2_vdec_dbg(1, ctx, "[%d] no available source buffer", ctx->id);
>@@ -381,17 +382,28 @@ static void mtk_vdec_worker(struct work_struct *work)
> 	    ctx->current_codec == V4L2_PIX_FMT_VP8_FRAME) {
> 		if (src_buf_req)
> 			v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
>-		v4l2_m2m_buf_done_and_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx, state);
>-	} else {
>-		if (ret != -EAGAIN) {
>-			v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
>-			ctx->last_vb2_v4l2_src = NULL;
>-		} else {
>-			ctx->last_vb2_v4l2_src = vb2_v4l2_src;
>-		}
>+		vb2_v4l2_dst = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
>+		v4l2_m2m_buf_done(vb2_v4l2_dst, state);
>+		v4l2_m2m_buf_done(vb2_v4l2_src, state);

This is another case where you just remove again completely what you
have added in the previous patch.

>
> 		v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
>+		return;
> 	}
>+
>+	/* If each codec return -EAGAIN to decode again, need to backup current source
>+	 * buffer, then the driver will get this buffer next time.

I would reword this like:

	/* Store the current source buffer for the next attempt to decode,
    * if this decode returned -EAGAIN */

>+	 *
>+	 * If each codec decode error, must to set buffer done with error status for
>+	 * this buffer have been removed from ready list.
>+	 */
>+	ctx->last_vb2_v4l2_src = (ret != -EAGAIN) ? NULL : vb2_v4l2_src;

Okay and here you add the same thing again as in the previous patch but
differently, this collection of commits feels more and more to me like a
work in progress. Please make sure in the future that each commit does
one job and does it completely.
It is not only confussing but also makes it hard to read the changes as
the bigger picture is missing in these tiny commits.

Please try to combine the patches where possible.

Regards,
Sebastian Fricke

>+	if (ret && ret != -EAGAIN) {
>+		if (src_buf_req)
>+			v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
>+		v4l2_m2m_buf_done(vb2_v4l2_src, state);
>+	}
>+
>+	v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
> }
>
> static void vb2ops_vdec_stateless_buf_queue(struct vb2_buffer *vb)
>-- 
>2.46.0
>
>