linux-kernel - Re: [PATCH v1 3/3] staging: media: ipu3: Stop streaming in inverse order of starting

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 for Android: free password hash cracker in your pocket

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

Message-ID: <4e01aa78-497e-477e-a5c1-951cfb1df907@chromium.org>
Date: Fri, 5 Jul 2024 10:54:50 +0900
From: Max Staudt <mstaudt@...omium.org>
To: Bingbu Cao <bingbu.cao@...ux.intel.com>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>,
 Bingbu Cao <bingbu.cao@...el.com>, Tianshu Qiu <tian.shu.qiu@...el.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
 Ricardo Ribalda <ribalda@...omium.org>
Subject: Re: [PATCH v1 3/3] staging: media: ipu3: Stop streaming in inverse
 order of starting

Hi Bingbu,

Thanks for your review! Replies inline...


On 7/4/24 4:03 PM, Bingbu Cao wrote:
>> diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
>> index 3ff390b04e1a..e7aee7e3db5b 100644
>> --- a/drivers/staging/media/ipu3/ipu3-v4l2.c
>> +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
>> @@ -535,29 +535,51 @@ static void imgu_vb2_stop_streaming(struct vb2_queue *vq)
>>   		container_of(vq, struct imgu_video_device, vbq);
>>   	int r;
>>   	unsigned int pipe;
>> +	bool stop_streaming = false;
>>   
>> +	/* Verify that the node had been setup with imgu_v4l2_node_setup() */
>>   	WARN_ON(!node->enabled);
>>   
>>   	pipe = node->pipe;
>>   	dev_dbg(dev, "Try to stream off node [%u][%u]", pipe, node->id);
>> -	imgu_pipe = &imgu->imgu_pipe[pipe];
>> -	r = v4l2_subdev_call(&imgu_pipe->imgu_sd.subdev, video, s_stream, 0);
>> -	if (r)
>> -		dev_err(&imgu->pci_dev->dev,
>> -			"failed to stop subdev streaming\n");
> 
> I guess the subdev streams API can help us on this, but current fix
> looks fine for me.

I don't understand what you're referring to, since your comment is in 
relation to a block of existing code that I have merely shuffled around. 
Could you please elaborate?


>>   
>> +	/*
>> +	 * When the first node of a streaming setup is stopped, the entire
>> +	 * pipeline needs to stop before individual nodes are disabled.
>> +	 * Perform the inverse of the initial setup.
>> +	 *
>> +	 * Part 1 - s_stream on the entire pipeline
> 
> stream on or off? it is a little confusing.

I meant that s_stream(off) is called "on the entire pipeline".

Thanks, I'll rephrase this in v2 :)


>> +	 */
>>   	mutex_lock(&imgu->streaming_lock);
>> -	/* Was this the first node with streaming disabled? */
>>   	if (imgu->streaming) {
>>   		/* Yes, really stop streaming now */
>>   		dev_dbg(dev, "IMGU streaming is ready to stop");
>>   		r = imgu_s_stream(imgu, false);
>>   		if (!r)
>>   			imgu->streaming = false;
>> +		stop_streaming = true;
>>   	}
>> -
>>   	mutex_unlock(&imgu->streaming_lock);
>>   
>> +	/* Part 2 - s_stream on subdevs
>> +	 *
>> +	 * If we call s_stream multiple times, Linux v6.7's call_s_stream()
>> +	 * WARNs and aborts. Thus, disable all pipes at once, and only once.
>> +	 */
>> +	if (stop_streaming) {
> 
>> +		for_each_set_bit(pipe, imgu->css.enabled_pipes,
>> +				 IMGU_MAX_PIPE_NUM) {
>> +			imgu_pipe = &imgu->imgu_pipe[pipe];
>> +
>> +			r = v4l2_subdev_call(&imgu_pipe->imgu_sd.subdev,
>> +					     video, s_stream, 0);
>> +			if (r)
>> +				dev_err(&imgu->pci_dev->dev,
>> +					"failed to stop subdev streaming\n");
>> +		}
> 
> Is it possible to move this loop into 'if (imgu->streaming)' above?

The point of separating the loop from 'if (imgu->streaming)', and why 
the stop_streaming variable exists, is to keep the loop out of the 
mutex's hot path. This follows the code in imgu_vb2_start_streaming(), 
as mentioned in the commit message.

Is it safe to do this work with the mutex taken?

Is there any need to do this work with the mutex taken?

Should the streamoff path really be different from the streamon path?

Does this mean that the streamon path should also have this happen with 
the mutex taken?



Thanks,

Max