| Message ID | 20190919203208.12515-2-dafna.hirschfeld@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: vimc: use configfs in order to configure devices topologies | expand |
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. > - 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
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
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; +} + /** * 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); - 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); + 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; } /**
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. 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 Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> --- drivers/media/platform/vimc/vimc-streamer.c | 39 +++++++++++++++------ 1 file changed, 28 insertions(+), 11 deletions(-)