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: <62890f7a-8ce9-47af-be36-e7384d2a99fd@ideasonboard.com>
Date: Fri, 26 Sep 2025 14:22:10 +0300
From: Tomi Valkeinen <tomi.valkeinen+renesas@...asonboard.com>
To: Laurent Pinchart <laurent.pinchart@...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,

On 02/06/2025 12:43, Laurent Pinchart wrote:
> 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.

At this point in the series risp_enable_streams() is called just once,
from a single VIN. That is, assuming only a single stream is supported.
In the cover letter I mention that there seems to be some kind of
attempts for multistreaming in upstream, which breaks during this
series. My understanding from Niklas was that the custom multistreaming
doesn't work or at least can be dropped. Niklas, correct me if I'm wrong
(and if I am wrong, someone else needs to take care of the custom
multistreaming =).

Later in the series risp_enable_streams will be called multiple times.
Each VIN video node will call it once when enabling.

 Tomi

> 
> 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);
>>  
>