[PATCH 1/1] v4l: subdev: Serialise open and release internal ops

[Sakari Ailus <sakari.ailus@linux.intel.com>]

By default, serialise open and release internal ops using a mutex.

The underlying problem is that a large proportion of the drivers do use
v4l2_fh_is_singular() in their open() handlers (struct
v4l2_subdev_internal_ops). v4l2_subdev_open(), .open file operation handler
of the V4L2 sub-device framework, calls v4l2_fh_add() which adds the file
handle to the list of open file handles of the device. Later on in the
open() handler of the sub-device driver uses v4l2_fh_is_singular() to
determine whether it was the file handle which was first opened. The check
may go wrong if the device was opened by multiple process closely enough in
time.

Why this is especially important is because many drivers perform power
management and other hardware initialisation based on open file handles.

Example one: Two processes open the device, but both end up determining
neither was the first file handle opened on the device.

	process A: v4l2_fh_add()
	process B: v4l2_fh_add()

	...

	process A: v4l2_fh_is_singular() # false
	process B: v4l2_fh_is_singular() # false

Example two:

	process A: v4l2_fh_add()

	...

	process A: v4l2_fh_is_singular() # true
					 # at this point the driver does
					 # time-consuming hardware
					 # initialisation in the context of
					 # process A

	process B: v4l2_fh_add()
	process B: v4l2_fh_is_singular() # false
					 # open system call finishes

	...

	process B proceeds to access the sub-device

	...

	process A finishes hardware initialisation

If the sub-device's open() handler in struct v4l2_subdev_internal_ops is not
defined, then the information on whether the file handle was the first to be
opened is not needed to complete the open system call on the device, and
serialisation of the open and release system calls thus is not needed, with
the possible exception for the driver's own reasons, but that is entirely
the job of the driver.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-device.c |  4 ++++
 drivers/media/v4l2-core/v4l2-subdev.c | 41 +++++++++++++++++++++++++++++------
 include/media/v4l2-subdev.h           |  4 ++++
 3 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index 5b0a30b..a13e0be 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -191,6 +191,8 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
 	}
 #endif
 
+	mutex_init(&sd->open_lock);
+
 	spin_lock(&v4l2_dev->lock);
 	list_add_tail(&sd->list, &v4l2_dev->subdevs);
 	spin_unlock(&v4l2_dev->lock);
@@ -292,5 +294,7 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
 	video_unregister_device(sd->devnode);
 	if (!sd->owner_v4l2_dev)
 		module_put(sd->owner);
+
+	mutex_destroy(&sd->open_lock);
 }
 EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev);
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index b3f7da9..d8a98c4 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -50,6 +50,18 @@ static void subdev_fh_free(struct v4l2_subdev_fh *fh)
 #endif
 }
 
+static void subdev_open_lock(struct v4l2_subdev *sd)
+{
+	if (sd->flags & V4L2_SUBDEV_FL_SERIALISE_OPEN)
+		mutex_lock(&sd->open_lock);
+}
+
+static void subdev_open_unlock(struct v4l2_subdev *sd)
+{
+	if (sd->flags & V4L2_SUBDEV_FL_SERIALISE_OPEN)
+		mutex_unlock(&sd->open_lock);
+}
+
 static int subdev_open(struct file *file)
 {
 	struct video_device *vdev = video_devdata(file);
@@ -64,11 +76,11 @@ static int subdev_open(struct file *file)
 	if (subdev_fh == NULL)
 		return -ENOMEM;
 
+	subdev_open_lock(sd);
+
 	ret = subdev_fh_init(subdev_fh, sd);
-	if (ret) {
-		kfree(subdev_fh);
-		return ret;
-	}
+	if (ret)
+		goto err_free_subdev_fh;
 
 	v4l2_fh_init(&subdev_fh->vfh, vdev);
 	v4l2_fh_add(&subdev_fh->vfh);
@@ -78,7 +90,7 @@ static int subdev_open(struct file *file)
 		entity = media_entity_get(&sd->entity);
 		if (!entity) {
 			ret = -EBUSY;
-			goto err;
+			goto err_media_entity_put;
 		}
 	}
 #endif
@@ -86,20 +98,26 @@ static int subdev_open(struct file *file)
 	if (sd->internal_ops && sd->internal_ops->open) {
 		ret = sd->internal_ops->open(sd, subdev_fh);
 		if (ret < 0)
-			goto err;
+			goto err_media_entity_put;
 	}
 
+	subdev_open_unlock(sd);
+
 	return 0;
 
-err:
+err_media_entity_put:
 #if defined(CONFIG_MEDIA_CONTROLLER)
 	media_entity_put(entity);
 #endif
 	v4l2_fh_del(&subdev_fh->vfh);
 	v4l2_fh_exit(&subdev_fh->vfh);
 	subdev_fh_free(subdev_fh);
+
+err_free_subdev_fh:
 	kfree(subdev_fh);
 
+	subdev_open_unlock(sd);
+
 	return ret;
 }
 
@@ -110,6 +128,8 @@ static int subdev_close(struct file *file)
 	struct v4l2_fh *vfh = file->private_data;
 	struct v4l2_subdev_fh *subdev_fh = to_v4l2_subdev_fh(vfh);
 
+	subdev_open_lock(sd);
+
 	if (sd->internal_ops && sd->internal_ops->close)
 		sd->internal_ops->close(sd, subdev_fh);
 #if defined(CONFIG_MEDIA_CONTROLLER)
@@ -117,7 +137,11 @@ static int subdev_close(struct file *file)
 		media_entity_put(&sd->entity);
 #endif
 	v4l2_fh_del(vfh);
+
+	subdev_open_unlock(sd);
+
 	v4l2_fh_exit(vfh);
+
 	subdev_fh_free(subdev_fh);
 	kfree(subdev_fh);
 	file->private_data = NULL;
@@ -586,6 +610,9 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
 	sd->ops = ops;
 	sd->v4l2_dev = NULL;
 	sd->flags = 0;
+	/* Default to serialised open and release. */
+	if (sd->internal_ops && sd->internal_ops->open)
+		sd->flags |= V4L2_SUBDEV_FL_SERIALISE_OPEN;
 	sd->name[0] = '\0';
 	sd->grp_id = 0;
 	sd->dev_priv = NULL;
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 5622699..11ffd50 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -624,6 +624,8 @@ struct v4l2_subdev_internal_ops {
 #define V4L2_SUBDEV_FL_HAS_DEVNODE		(1U << 2)
 /* Set this flag if this subdev generates events. */
 #define V4L2_SUBDEV_FL_HAS_EVENTS		(1U << 3)
+/* Set this flag if the sub-device open/close do NOT need to be serialised. */
+#define V4L2_SUBDEV_FL_SERIALISE_OPEN		(1U << 4)
 
 struct regulator_bulk_data;
 
@@ -672,6 +674,8 @@ struct v4l2_subdev {
 	struct v4l2_async_notifier *notifier;
 	/* common part of subdevice platform data */
 	struct v4l2_subdev_platform_data *pdata;
+	/* serialise open and release based on the flags field */
+	struct mutex open_lock;
 };
 
 #define media_entity_to_v4l2_subdev(ent) \
-- 
2.1.0.231.g7484e3b