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.hirschf...@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
