Re: [PATCH v4 1/4] v4l: vsp1: Prevent multiple streamon race commencing pipeline early

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Kieran,

Thank you for the patch.

On Friday 06 Jan 2017 12:15:28 Kieran Bingham wrote:
> With multiple inputs through the BRU it is feasible for the streams to
> race each other at stream-on.
> 
> Multiple VIDIOC_STREAMON calls racing each other could have process
> N-1 skipping over the pipeline setup section and then start the pipeline
> early, if videobuf2 has already enqueued buffers to the driver for
> process N but not called the .start_streaming() operation yet
> 
> In the case of the video pipelines, this
> can present two serious issues.
> 
>  1) A null-dereference if the pipe->dl is committed at the same time as
>     the vsp1_video_setup_pipeline() is processing
> 
>  2) A hardware hang, where a display list is committed without having
>     called vsp1_video_setup_pipeline() first
> 
> Repair this issue, by ensuring that only the stream which configures the
> pipeline is able to start it.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> 
> ---
> 
> v4:
>  - Revert and rework back to v1 implementation style
>  - Provide detailed comments on the race
> 
> v3:
>  - Move 'flag reset' to be inside the vsp1_reset_wpf() function call
>  - Tidy up the wpf->pipe reference for the configured flag
> 
> To test this race, I have used the vsp-unit-test-0007.sh from Laurent's
> VSP-Tests [0] in iteration. Without this patch, failures can be seen be
> seen anywhere up to the 150 iterations mark.
> 
> With this patch in place, tests have successfully iterated over 1500
> loops.
> 
> The function affected by this change appears to have been around since
> v4.6-rc2-105-g351bbf99f245 and thus could be included in stable trees
> from that point forward. The issue may have been prevalent before that
> but the solution would need reworking for earlier version.
> 
> [0] http://git.ideasonboard.com/renesas/vsp-tests.git
> ---
>  drivers/media/platform/vsp1/vsp1_video.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> b/drivers/media/platform/vsp1/vsp1_video.c index e6592b576ca3..f7dc249eb398
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -797,6 +797,7 @@ static int vsp1_video_start_streaming(struct vb2_queue
> *vq, unsigned int count) {
>  	struct vsp1_video *video = vb2_get_drv_priv(vq);
>  	struct vsp1_pipeline *pipe = video->rwpf->pipe;
> +	bool start_pipeline = false;
>  	unsigned long flags;
>  	int ret;
> 
> @@ -807,11 +808,23 @@ static int vsp1_video_start_streaming(struct vb2_queue
> *vq, unsigned int count) mutex_unlock(&pipe->lock);
>  			return ret;
>  		}
> +
> +		start_pipeline = true;
>  	}
> 
>  	pipe->stream_count++;
>  	mutex_unlock(&pipe->lock);
> 
> +	/*
> +	 * vsp1_pipeline_ready() is not sufficient to establish that all 
streams
> +	 * are prepared and the pipeline is configured, as multiple streams
> +	 * can race through streamon with buffers already queued; Therefore we
> +	 * don't even attempt to start the pipeline until the last stream has
> +	 * called through here.
> +	 */
> +	if (!start_pipeline)
> +		return 0;
> +
>  	spin_lock_irqsave(&pipe->irqlock, flags);
>  	if (vsp1_pipeline_ready(pipe))
>  		vsp1_video_pipeline_run(pipe);

-- 
Regards,

Laurent Pinchart