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: <20241021230519.GB8113@pendragon.ideasonboard.com>
Date: Tue, 22 Oct 2024 02:05:19 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Laurentiu Palcu <laurentiu.palcu@....nxp.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	Stefan Riedmueller <s.riedmueller@...tec.de>,
	Guoniu Zhou <guoniu.zhou@....com>,
	Dong Aisheng <aisheng.dong@....com>,
	Christian Hemp <c.hemp@...tec.de>, linux-media@...r.kernel.org,
	imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] media: nxp: imx8-isi: better handle the m2m
 usage_count

On Fri, Sep 27, 2024 at 11:04:20AM +0300, Laurentiu Palcu wrote:
> Currently, if streamon/streamoff calls are imbalanced we can either end up
> with a negative ISI m2m usage_count (if streamoff() is called more times
> than streamon()) in which case we'll not be able to restart the ISI pipe
> next time, or the usage_count never gets to 0 and the pipe is never
> switched off.
> 
> So, to avoid that, add an 'in_use' flag in the ctx structure that will
> keep track whether the output/capture queues have been started or not,
> and use it to avoid decrementing/incrementing the usage_count
> unnecessarily.
> 
> Fixes: cf21f328fcafac ("media: nxp: Add i.MX8 ISI driver")
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@....nxp.com>
> ---
> v2:
>  * Changed the way 'usage_count' is incremented/decremented by taking
>    into account the context the streamon/streamoff functions are called
>    from;
>  * Changed the commit message and subject to reflect the changes;
> 
>  .../platform/nxp/imx8-isi/imx8-isi-m2m.c      | 24 +++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
> index 9745d6219a166..3f06ae1349e53 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
> @@ -65,6 +65,7 @@ struct mxc_isi_m2m_ctx {
>  	} ctrls;
>  
>  	bool chained;
> +	bool in_use[2];

I think you can store this in mxc_isi_m2m_ctx_queue_data instead as a

	bool streaming;

>  };
>  
>  static inline struct mxc_isi_m2m_buffer *
> @@ -491,6 +492,7 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
>  	const struct mxc_isi_format_info *cap_info = ctx->queues.cap.info;
>  	const struct mxc_isi_format_info *out_info = ctx->queues.out.info;
>  	struct mxc_isi_m2m *m2m = ctx->m2m;
> +	bool already_in_use;
>  	bool bypass;
>  
>  	int ret;
> @@ -502,6 +504,8 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
>  		goto unlock;
>  	}
>  
> +	already_in_use = ctx->in_use[type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE];
> +

As the streamon and streamoff operation are covered by the same queue
lock for all contexts, you can do all this outside of the m2m->lock
sections. I think the following patch (untested) should do and would be
simpler:

diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
index 9745d6219a16..cd6c52e9d158 100644
--- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
+++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
@@ -43,6 +43,7 @@ struct mxc_isi_m2m_ctx_queue_data {
 	struct v4l2_pix_format_mplane format;
 	const struct mxc_isi_format_info *info;
 	u32 sequence;
+	bool streaming;
 };

 struct mxc_isi_m2m_ctx {
@@ -486,15 +487,18 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
 				enum v4l2_buf_type type)
 {
 	struct mxc_isi_m2m_ctx *ctx = to_isi_m2m_ctx(fh);
+	struct mxc_isi_m2m_ctx_queue_data *q = mxc_isi_m2m_ctx_qdata(ctx, type);
 	const struct v4l2_pix_format_mplane *out_pix = &ctx->queues.out.format;
 	const struct v4l2_pix_format_mplane *cap_pix = &ctx->queues.cap.format;
 	const struct mxc_isi_format_info *cap_info = ctx->queues.cap.info;
 	const struct mxc_isi_format_info *out_info = ctx->queues.out.info;
 	struct mxc_isi_m2m *m2m = ctx->m2m;
 	bool bypass;
-
 	int ret;

+	if (q->streaming)
+		return 0;
+
 	mutex_lock(&m2m->lock);

 	if (m2m->usage_count == INT_MAX) {
@@ -547,6 +551,8 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
 		goto unchain;
 	}

+	q->streaming = true;
+
 	return 0;

 unchain:
@@ -569,10 +575,14 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh,
 				 enum v4l2_buf_type type)
 {
 	struct mxc_isi_m2m_ctx *ctx = to_isi_m2m_ctx(fh);
+	struct mxc_isi_m2m_ctx_queue_data *q = mxc_isi_m2m_ctx_qdata(ctx, type);
 	struct mxc_isi_m2m *m2m = ctx->m2m;

 	v4l2_m2m_ioctl_streamoff(file, fh, type);

+	if (!q->streaming)
+		return 0;
+
 	mutex_lock(&m2m->lock);

 	/*
@@ -598,6 +608,8 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh,

 	mutex_unlock(&m2m->lock);

+	q->streaming = false;
+
 	return 0;
 }

If this works for you, could you turn it into a v3 ?

>  	bypass = cap_pix->width == out_pix->width &&
>  		 cap_pix->height == out_pix->height &&
>  		 cap_info->encoding == out_info->encoding;
> @@ -520,7 +524,10 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
>  		mxc_isi_channel_get(m2m->pipe);
>  	}
>  
> -	m2m->usage_count++;
> +	if (!already_in_use) {
> +		m2m->usage_count++;
> +		ctx->in_use[type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE] = true;
> +	}
>  
>  	/*
>  	 * Allocate resources for the channel, counting how many users require
> @@ -555,7 +562,12 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
>  	ctx->chained = false;
>  
>  deinit:
> -	if (--m2m->usage_count == 0) {
> +	if (!already_in_use) {
> +		m2m->usage_count--;
> +		ctx->in_use[type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE] = false;
> +	}
> +
> +	if (m2m->usage_count == 0) {
>  		mxc_isi_channel_put(m2m->pipe);
>  		mxc_isi_channel_release(m2m->pipe);
>  	}
> @@ -575,6 +587,9 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh,
>  
>  	mutex_lock(&m2m->lock);
>  
> +	if (!ctx->in_use[type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE])
> +		goto unlock;
> +
>  	/*
>  	 * If the last context is this one, reset it to make sure the device
>  	 * will be reconfigured when streaming is restarted.
> @@ -587,6 +602,8 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh,
>  		mxc_isi_channel_unchain(m2m->pipe);
>  	ctx->chained = false;
>  
> +	ctx->in_use[type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE] = false;
> +
>  	/* Turn off the light with the last user. */
>  	if (--m2m->usage_count == 0) {
>  		mxc_isi_channel_disable(m2m->pipe);
> @@ -594,8 +611,7 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh,
>  		mxc_isi_channel_release(m2m->pipe);
>  	}
>  
> -	WARN_ON(m2m->usage_count < 0);
> -
> +unlock:
>  	mutex_unlock(&m2m->lock);
>  
>  	return 0;

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ