Re: [PATCH 1/5] media: vimc: upon streaming, check that the pipeline starts with a source entity

[Dafna Hirschfeld <dafna.hirschfeld@collabora.com>]

Hi,
Thanks for the review,

On Thu, 2019-09-26 at 08:32 -0600, Shuah Khan wrote:
> On 9/19/19 2:32 PM, Dafna Hirschfeld wrote:
> > Userspace can disable links and create pipelines that
> > do not start with a source entity. Trying to stream
> > from such a pipeline should fail with -EPIPE
> > currently this is not handled and cause kernel crash.
> > 
> 
> Minor: Can you make these 75 long. Makes it easier to read.
> 
> > Reproducing the crash:
> > media-ctl -d0 -l "5:1->21:0[0]" -v
> > v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=1920,height=1440
> > v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video2
> 
> I really would like to see the panic message so it can checked during
> testing.
> 
> If you are fixing a panic, please include the panic info. in the future.
> 
> > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> > ---
> >   drivers/media/platform/vimc/vimc-streamer.c | 39 +++++++++++++++------
> >   1 file changed, 28 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/media/platform/vimc/vimc-streamer.c b/drivers/media/platform/vimc/vimc-streamer.c
> > index faa2879c25df..d0a9f8a0f26a 100644
> > --- a/drivers/media/platform/vimc/vimc-streamer.c
> > +++ b/drivers/media/platform/vimc/vimc-streamer.c
> > @@ -12,6 +12,19 @@
> >   
> >   #include "vimc-streamer.h"
> >   
> > +/**
> > + * Check if the entity has only source pads
> > + */
> > +static bool vimc_is_source(struct media_entity *ent)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < ent->num_pads; i++)
> > +		if (ent->pads[i].flags & MEDIA_PAD_FL_SINK)
> > +			return false;
> > +	return true;
> > +}
> > +
> 
> Why not make this a common routine and add it to vimc-common.c?
> 
> >   /**
> >    * vimc_get_source_entity - get the entity connected with the first sink pad
> >    *
> > @@ -82,14 +95,12 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> >   	struct media_entity *entity;
> >   	struct video_device *vdev;
> >   	struct v4l2_subdev *sd;
> > -	int ret = 0;
> > +	int ret = -EINVAL;
> >   
> >   	stream->pipe_size = 0;
> >   	while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
> > -		if (!ved) {
> > -			vimc_streamer_pipeline_terminate(stream);
> > -			return -EINVAL;
> > -		}
> > +		if (!ved)
> > +			break;
> >   		stream->ved_pipeline[stream->pipe_size++] = ved;
> >   
> >   		if (is_media_entity_v4l2_subdev(ved->ent)) {
> > @@ -98,15 +109,22 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> >   			if (ret && ret != -ENOIOCTLCMD) {
> >   				pr_err("subdev_call error %s\n",
> >   				       ved->ent->name);
> 
> While you are at it, can you make this a dev_err() instead? I think we
> have access to dev here.

Actually, there is no access to dev here. It can be sent to the function as
an argument from the vimc-capture.c code, but maybe a better solution is to move the dev
pointer of every vimc entity to the common `vimc_ent_dev ved` field, since all entities have a pointer to it.

Thanks,
Dafna
> 
> > -				vimc_streamer_pipeline_terminate(stream);
> > -				return ret;
> > +				break;
> >   			}
> >   		}
> >   
> >   		entity = vimc_get_source_entity(ved->ent);
> > -		/* Check if the end of the pipeline was reached*/
> > -		if (!entity)
> > +		/* Check if the end of the pipeline was reached */
> > +		if (!entity) {
> > +			/* the first entity of the pipe should be source only */
> > +			if (!vimc_is_source(ved->ent)) {
> > +				pr_err("first entity in the pipe '%s' is not a source\n",
> > +				       ved->ent->name);
> 
> Same commnet about dev_err() here.
> 
> > +				ret = -EPIPE;
> > +				break;
> > +			}
> >   			return 0;
> > +		}
> >   
> >   		/* Get the next device in the pipeline */
> >   		if (is_media_entity_v4l2_subdev(entity)) {
> > @@ -119,9 +137,8 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
> >   			ved = video_get_drvdata(vdev);
> >   		}
> >   	}
> > -
> >   	vimc_streamer_pipeline_terminate(stream);
> > -	return -EINVAL;
> > +	return ret;
> >   }
> >   
> >   /**
> > 
> 
> thanks,
> -- Shuah