linux-kernel - Re: [PATCH v3 03/15] media: rcar-isp: Move {enable|disable}

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: <20250602055742.GC11750@pendragon.ideasonboard.com>
Date: Mon, 2 Jun 2025 12:43:33 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Tomi Valkeinen <tomi.valkeinen+renesas@...asonboard.com>
Cc: Niklas Söderlund <niklas.soderlund@...natech.se>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	linux-media@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Niklas Söderlund <niklas.soderlund+renesas@...natech.se>,
	Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
	Jacopo Mondi <jacopo.mondi@...asonboard.com>
Subject: Re: [PATCH v3 03/15] media: rcar-isp: Move
 {enable|disable}_streams() calls

Hi Tomi,

Thank you for the patch.

On Fri, May 30, 2025 at 04:50:32PM +0300, Tomi Valkeinen wrote:
> With multiple streams the operation to enable the ISP hardware and to
> call {enable|disable}_streams() on upstream subdev will need to be
> handled separately.
> 
> Prepare for that by moving {enable|disable}_streams() calls out from
> risp_start() and risp_stop().
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@...asonboard.com>
> ---
>  drivers/media/platform/renesas/rcar-isp/csisp.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rcar-isp/csisp.c b/drivers/media/platform/renesas/rcar-isp/csisp.c
> index 8fb2cc3b5650..2337c5d44c40 100644
> --- a/drivers/media/platform/renesas/rcar-isp/csisp.c
> +++ b/drivers/media/platform/renesas/rcar-isp/csisp.c
> @@ -268,18 +268,11 @@ static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
>  	/* Start ISP. */
>  	risp_write_cs(isp, ISPSTART_REG, ISPSTART_START);
>  
> -	ret = v4l2_subdev_enable_streams(isp->remote, isp->remote_pad,
> -					 BIT_ULL(0));
> -	if (ret)
> -		risp_power_off(isp);
> -
> -	return ret;
> +	return 0;
>  }
>  
>  static void risp_stop(struct rcar_isp *isp)
>  {
> -	v4l2_subdev_disable_streams(isp->remote, isp->remote_pad, BIT_ULL(0));
> -
>  	/* Stop ISP. */
>  	risp_write_cs(isp, ISPSTART_REG, ISPSTART_STOP);
>  
> @@ -305,6 +298,13 @@ static int risp_enable_streams(struct v4l2_subdev *sd,
>  			return ret;
>  	}
>  
> +	ret = v4l2_subdev_enable_streams(isp->remote, isp->remote_pad,
> +					 BIT_ULL(0));

You're now potentially calling v4l2_subdev_disable_streams() multiple
times on the same pad and stream, as this call isn't covered by the
stream_count check anymore. Is that correct ? Maybe because
risp_enable_streams() is guaranteed to never be called multiple times,
with stream_count never becoming larger than 1 ? If so that should be
explained in the commit message, and stream_count should probably be
dropped.

Same when stopping.

> +	if (ret) {
> +		risp_stop(isp);

This is also not covered by the stream_count, while risp_start() is.

> +		return ret;
> +	}
> +
>  	isp->stream_count += 1;
>  
>  	return ret;
> @@ -322,6 +322,8 @@ static int risp_disable_streams(struct v4l2_subdev *sd,
>  	if (!isp->remote)
>  		return -ENODEV;
>  
> +	v4l2_subdev_disable_streams(isp->remote, isp->remote_pad, BIT_ULL(0));
> +
>  	if (isp->stream_count == 1)
>  		risp_stop(isp);
>  

-- 
Regards,

Laurent Pinchart