[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>
diff --git a/drivers/media/platform/s5p-fimc/fimc-capture.c b/drivers/media/platform/s5p-fimc/fimc-capture.c
index aad0850..18a70e4 100644
--- a/drivers/media/platform/s5p-fimc/fimc-capture.c
+++ b/drivers/media/platform/s5p-fimc/fimc-capture.c
@@ -510,8 +510,8 @@
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 @@
}
unlock:
mutex_unlock(&fimc->lock);
+ fimc_md_graph_unlock(fimc);
return ret;
}
@@ -962,6 +963,10 @@
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 @@
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 @@
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 @@
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 @@
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 @@
struct v4l2_format *f)
{
struct fimc_dev *fimc = video_drvdata(file);
+ int ret;
- return fimc_capture_set_format(fimc, f);
+ 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);
+
+ 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 @@
},
};
- return fimc_capture_set_format(fimc, &fmt);
+ return __fimc_capture_set_format(fimc, &fmt);
}
/* fimc->lock must be already initialized */
@@ -1789,6 +1811,12 @@
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)