[media] s5p-fimc: Prevent AB-BA deadlock during links reconfiguration

This patch patch eliminates potential AB-BA deadlock when one process calls
open(), or VIDIOC_S/TRY_FMT ioctl  on the FIMC capture video node, while
other thread is reconfiguring media links via media device node:
/dev/video? open()		  /dev/media? MEDIA_IOC_SETUP_LINK ioctl
mutex_lock(video_lock)            mutex_lock(graph_lock)
    fimc_pipeline_open()               fimc_md_link_notify()
        mutex_lock(graph_lock)	          mutex_lock(video_lock)
          ...                               ...
The deadlock is avoided by always taking the graph mutex first in video
node open() or an ioctl, before the video lock is acquired. Reversed
order seems impossible, since media device driver's link_notify callback
is called with media graph mutex already held.
To ensure proper locking order VIDIOC_S_FMT and VIDIOC_TRY_FMT ioctls are
not serialized in the v4l2-core and the driver takes care of it itself.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

1 files changed, 41 insertions, 13 deletions

diff --git a/drivers/media/platform/s5p-fimc/fimc-capture.c b/drivers/media/platform/s5p-fimc/fimc-capture.c
index aad0850d0c01..18a70e4a0b0f 100644
--- a/drivers/media/platform/s5p-fimc/fimc-capture.c
+++ b/drivers/media/platform/s5p-fimc/fimc-capture.c
@@ -510,8 +510,8 @@ static int fimc_capture_open(struct file *file)
 
 	dbg("pid: %d, state: 0x%lx", task_pid_nr(current), fimc->state);
 
-	if (mutex_lock_interruptible(&fimc->lock))
-		return -ERESTARTSYS;
+	fimc_md_graph_lock(fimc);
+	mutex_lock(&fimc->lock);
 
 	if (fimc_m2m_active(fimc))
 		goto unlock;
@@ -546,6 +546,7 @@ static int fimc_capture_open(struct file *file)
 	}
 unlock:
 	mutex_unlock(&fimc->lock);
+	fimc_md_graph_unlock(fimc);
 	return ret;
 }
 
@@ -962,6 +963,10 @@ static int fimc_cap_try_fmt_mplane(struct file *file, void *fh,
 	struct fimc_ctx *ctx = fimc->vid_cap.ctx;
 	struct v4l2_mbus_framefmt mf;
 	struct fimc_fmt *ffmt = NULL;
+	int ret = 0;
+
+	fimc_md_graph_lock(fimc);
+	mutex_lock(&fimc->lock);
 
 	if (fimc_jpeg_fourcc(pix->pixelformat)) {
 		fimc_capture_try_format(ctx, &pix->width, &pix->height,
@@ -973,16 +978,16 @@ static int fimc_cap_try_fmt_mplane(struct file *file, void *fh,
 	ffmt = fimc_capture_try_format(ctx, &pix->width, &pix->height,
 				       NULL, &pix->pixelformat,
 				       FIMC_SD_PAD_SOURCE);
-	if (!ffmt)
-		return -EINVAL;
+	if (!ffmt) {
+		ret = -EINVAL;
+		goto unlock;
+	}
 
 	if (!fimc->vid_cap.user_subdev_api) {
 		mf.width = pix->width;
 		mf.height = pix->height;
 		mf.code = ffmt->mbus_code;
-		fimc_md_graph_lock(fimc);
 		fimc_pipeline_try_format(ctx, &mf, &ffmt, false);
-		fimc_md_graph_unlock(fimc);
 		pix->width = mf.width;
 		pix->height = mf.height;
 		if (ffmt)
@@ -994,8 +999,11 @@ static int fimc_cap_try_fmt_mplane(struct file *file, void *fh,
 	if (ffmt->flags & FMT_FLAGS_COMPRESSED)
 		fimc_get_sensor_frame_desc(fimc->pipeline.subdevs[IDX_SENSOR],
 					pix->plane_fmt, ffmt->memplanes, true);
+unlock:
+	mutex_unlock(&fimc->lock);
+	fimc_md_graph_unlock(fimc);
 
-	return 0;
+	return ret;
 }
 
 static void fimc_capture_mark_jpeg_xfer(struct fimc_ctx *ctx,
@@ -1012,7 +1020,8 @@ static void fimc_capture_mark_jpeg_xfer(struct fimc_ctx *ctx,
 		clear_bit(ST_CAPT_JPEG, &ctx->fimc_dev->state);
 }
 
-static int fimc_capture_set_format(struct fimc_dev *fimc, struct v4l2_format *f)
+static int __fimc_capture_set_format(struct fimc_dev *fimc,
+				     struct v4l2_format *f)
 {
 	struct fimc_ctx *ctx = fimc->vid_cap.ctx;
 	struct v4l2_pix_format_mplane *pix = &f->fmt.pix_mp;
@@ -1047,12 +1056,10 @@ static int fimc_capture_set_format(struct fimc_dev *fimc, struct v4l2_format *f)
 		mf->code   = ff->fmt->mbus_code;
 		mf->width  = pix->width;
 		mf->height = pix->height;
-
-		fimc_md_graph_lock(fimc);
 		ret = fimc_pipeline_try_format(ctx, mf, &s_fmt, true);
-		fimc_md_graph_unlock(fimc);
 		if (ret)
 			return ret;
+
 		pix->width  = mf->width;
 		pix->height = mf->height;
 	}
@@ -1091,8 +1098,23 @@ static int fimc_cap_s_fmt_mplane(struct file *file, void *priv,
 				 struct v4l2_format *f)
 {
 	struct fimc_dev *fimc = video_drvdata(file);
+	int ret;
+
+	fimc_md_graph_lock(fimc);
+	mutex_lock(&fimc->lock);
+	/*
+	 * The graph is walked within __fimc_capture_set_format() to set
+	 * the format at subdevs thus the graph mutex needs to be held at
+	 * this point and acquired before the video mutex, to avoid  AB-BA
+	 * deadlock when fimc_md_link_notify() is called by other thread.
+	 * Ideally the graph walking and setting format at the whole pipeline
+	 * should be removed from this driver and handled in userspace only.
+	 */
+	ret = __fimc_capture_set_format(fimc, f);
 
-	return fimc_capture_set_format(fimc, f);
+	mutex_unlock(&fimc->lock);
+	fimc_md_graph_unlock(fimc);
+	return ret;
 }
 
 static int fimc_cap_enum_input(struct file *file, void *priv,
@@ -1727,7 +1749,7 @@ static int fimc_capture_set_default_format(struct fimc_dev *fimc)
 		},
 	};
 
-	return fimc_capture_set_format(fimc, &fmt);
+	return __fimc_capture_set_format(fimc, &fmt);
 }
 
 /* fimc->lock must be already initialized */
@@ -1789,6 +1811,12 @@ static int fimc_register_capture_device(struct fimc_dev *fimc,
 	ret = media_entity_init(&vfd->entity, 1, &vid_cap->vd_pad, 0);
 	if (ret)
 		goto err_ent;
+	/*
+	 * For proper order of acquiring/releasing the video
+	 * and the graph mutex.
+	 */
+	v4l2_disable_ioctl_locking(vfd, VIDIOC_TRY_FMT);
+	v4l2_disable_ioctl_locking(vfd, VIDIOC_S_FMT);
 
 	ret = video_register_device(vfd, VFL_TYPE_GRABBER, -1);
 	if (ret)