V4L/DVB: v4l2: add core serialization lock

Drivers can optionally set a pointer to a mutex in struct video_device. The core will use that to lock before calling open, read, write, unlocked_ioctl, poll, mmap or release. Updated the documentation as well and ensure that v4l2-event knows about the lock: it will unlock it before doing a blocking wait on an event and relock it afterwards. Ensure that the 'video_is_registered' check is done when the lock is held: a typical disconnect will take the lock as well before unregistering the device nodes, so to prevent race conditions the video_is_registered check should also be done with the lock held. Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

V4L/DVB: v4l2: add core serialization lock
Drivers can optionally set a pointer to a mutex in struct video_device. The core will use that to lock before calling open, read, write, unlocked_ioctl, poll, mmap or release. Updated the documentation as well and ensure that v4l2-event knows about the lock: it will unlock it before doing a blocking wait on an event and relock it afterwards. Ensure that the 'video_is_registered' check is done when the lock is held: a typical disconnect will take the lock as well before unregistering the device nodes, so to prevent race conditions the video_is_registered check should also be done with the lock held. Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
Hans Verkuil · Mauro Carvalho Chehab
1 parent c29fcff3da
Showing 4 changed files with 89 additions and 19 deletions Side-by-side Diff
Documentation/video4linux/v4l2-framework.txt
drivers/media/video/v4l2-dev.c
drivers/media/video/v4l2-event.c
include/media/v4l2-dev.h
@@ -453,6 +453,10 @@
 - ioctl_ops: if you use the v4l2_ioctl_ops to simplify ioctl maintenance
   (highly recommended to use this and it might become compulsory in the
   future!), then set this to your v4l2_ioctl_ops struct.
+- lock: leave to NULL if you want to do all the locking in the driver.
+  Otherwise you give it a pointer to a struct mutex_lock and before any
+  of the v4l2_file_operations is called this lock will be taken by the
+  core and released afterwards.
 - parent: you only set this if v4l2_device was registered with NULL as
   the parent device struct. This only happens in cases where one hardware
   device has multiple PCI devices that all share the same v4l2_device core.
@@ -469,6 +473,22 @@
 The v4l2_file_operations struct is a subset of file_operations. The main
 difference is that the inode argument is omitted since it is never used.
  
+v4l2_file_operations and locking
+--------------------------------
+
+You can set a pointer to a mutex_lock in struct video_device. Usually this
+will be either a top-level mutex or a mutex per device node. If you want
+finer-grained locking then you have to set it to NULL and do you own locking.
+
+If a lock is specified then all file operations will be serialized on that
+lock. If you use videobuf then you must pass the same lock to the videobuf
+queue initialize function: if videobuf has to wait for a frame to arrive, then
+it will temporarily unlock the lock and relock it afterwards. If your driver
+also waits in the code, then you should do the same to allow other processes
+to access the device node while the first process is waiting for something.
+
+The implementation of a hotplug disconnect should also take the lock before
+calling v4l2_device_disconnect and video_unregister_device.
  
 video_device registration
 -------------------------
@@ -187,33 +187,50 @@
 		size_t sz, loff_t *off)
 {
 	struct video_device *vdev = video_devdata(filp);
+	int ret = -EIO;
  
 	if (!vdev->fops->read)
 		return -EINVAL;
-	if (!video_is_registered(vdev))
-		return -EIO;
-	return vdev->fops->read(filp, buf, sz, off);
+	if (vdev->lock)
+		mutex_lock(vdev->lock);
+	if (video_is_registered(vdev))
+		ret = vdev->fops->read(filp, buf, sz, off);
+	if (vdev->lock)
+		mutex_unlock(vdev->lock);
+	return ret;
 }
  
 static ssize_t v4l2_write(struct file *filp, const char __user *buf,
 		size_t sz, loff_t *off)
 {
 	struct video_device *vdev = video_devdata(filp);
+	int ret = -EIO;
  
 	if (!vdev->fops->write)
 		return -EINVAL;
-	if (!video_is_registered(vdev))
-		return -EIO;
-	return vdev->fops->write(filp, buf, sz, off);
+	if (vdev->lock)
+		mutex_lock(vdev->lock);
+	if (video_is_registered(vdev))
+		ret = vdev->fops->write(filp, buf, sz, off);
+	if (vdev->lock)
+		mutex_unlock(vdev->lock);
+	return ret;
 }
  
 static unsigned int v4l2_poll(struct file *filp, struct poll_table_struct *poll)
 {
 	struct video_device *vdev = video_devdata(filp);
+	int ret = DEFAULT_POLLMASK;
  
-	if (!vdev->fops->poll || !video_is_registered(vdev))
-		return DEFAULT_POLLMASK;
-	return vdev->fops->poll(filp, poll);
+	if (!vdev->fops->poll)
+		return ret;
+	if (vdev->lock)
+		mutex_lock(vdev->lock);
+	if (video_is_registered(vdev))
+		ret = vdev->fops->poll(filp, poll);
+	if (vdev->lock)
+		mutex_unlock(vdev->lock);
+	return ret;
 }
  
 static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
  
@@ -224,7 +241,11 @@
 	if (!vdev->fops->ioctl)
 		return -ENOTTY;
 	if (vdev->fops->unlocked_ioctl) {
+		if (vdev->lock)
+			mutex_lock(vdev->lock);
 		ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
+		if (vdev->lock)
+			mutex_unlock(vdev->lock);
 	} else if (vdev->fops->ioctl) {
 		/* TODO: convert all drivers to unlocked_ioctl */
 		lock_kernel();
  
@@ -239,10 +260,17 @@
 static int v4l2_mmap(struct file *filp, struct vm_area_struct *vm)
 {
 	struct video_device *vdev = video_devdata(filp);
+	int ret = -ENODEV;
  
-	if (!vdev->fops->mmap || !video_is_registered(vdev))
-		return -ENODEV;
-	return vdev->fops->mmap(filp, vm);
+	if (!vdev->fops->mmap)
+		return ret;
+	if (vdev->lock)
+		mutex_lock(vdev->lock);
+	if (video_is_registered(vdev))
+		ret = vdev->fops->mmap(filp, vm);
+	if (vdev->lock)
+		mutex_unlock(vdev->lock);
+	return ret;
 }
  
 /* Override for the open function */
  
@@ -254,17 +282,24 @@
 	/* Check if the video device is available */
 	mutex_lock(&videodev_lock);
 	vdev = video_devdata(filp);
-	/* return ENODEV if the video device has been removed
-	   already or if it is not registered anymore. */
-	if (vdev == NULL || !video_is_registered(vdev)) {
+	/* return ENODEV if the video device has already been removed. */
+	if (vdev == NULL) {
 		mutex_unlock(&videodev_lock);
 		return -ENODEV;
 	}
 	/* and increase the device refcount */
 	video_get(vdev);
 	mutex_unlock(&videodev_lock);
-	if (vdev->fops->open)
-		ret = vdev->fops->open(filp);
+	if (vdev->fops->open) {
+		if (vdev->lock)
+			mutex_lock(vdev->lock);
+		if (video_is_registered(vdev))
+			ret = vdev->fops->open(filp);
+		else
+			ret = -ENODEV;
+		if (vdev->lock)
+			mutex_unlock(vdev->lock);
+	}
  
 	/* decrease the refcount in case of an error */
 	if (ret)
  
@@ -278,8 +313,13 @@
 	struct video_device *vdev = video_devdata(filp);
 	int ret = 0;
  
-	if (vdev->fops->release)
+	if (vdev->fops->release) {
+		if (vdev->lock)
+			mutex_lock(vdev->lock);
 		vdev->fops->release(filp);
+		if (vdev->lock)
+			mutex_unlock(vdev->lock);
+	}
  
 	/* decrease the refcount unconditionally since the release()
 	   return value is ignored. */
@@ -134,14 +134,21 @@
 	if (nonblocking)
 		return __v4l2_event_dequeue(fh, event);
  
+	/* Release the vdev lock while waiting */
+	if (fh->vdev->lock)
+		mutex_unlock(fh->vdev->lock);
+
 	do {
 		ret = wait_event_interruptible(events->wait,
 					       events->navailable != 0);
 		if (ret < 0)
-			return ret;
+			break;
  
 		ret = __v4l2_event_dequeue(fh, event);
 	} while (ret == -ENOENT);
+
+	if (fh->vdev->lock)
+		mutex_lock(fh->vdev->lock);
  
 	return ret;
 }
@@ -94,6 +94,9 @@
  
 	/* ioctl callbacks */
 	const struct v4l2_ioctl_ops *ioctl_ops;
+
+	/* serialization lock */
+	struct mutex *lock;
 };
  
 /* dev to video-device */
...	...	@@ -453,6 +453,10 @@
453	453	- ioctl_ops: if you use the v4l2_ioctl_ops to simplify ioctl maintenance
454	454	(highly recommended to use this and it might become compulsory in the
455	455	future!), then set this to your v4l2_ioctl_ops struct.
	456	+- lock: leave to NULL if you want to do all the locking in the driver.
	457	+ Otherwise you give it a pointer to a struct mutex_lock and before any
	458	+ of the v4l2_file_operations is called this lock will be taken by the
	459	+ core and released afterwards.
456	460	- parent: you only set this if v4l2_device was registered with NULL as
457	461	the parent device struct. This only happens in cases where one hardware
458	462	device has multiple PCI devices that all share the same v4l2_device core.
...	...	@@ -469,6 +473,22 @@
469	473	The v4l2_file_operations struct is a subset of file_operations. The main
470	474	difference is that the inode argument is omitted since it is never used.
471	475
	476	+v4l2_file_operations and locking
	477	+--------------------------------
	478	+
	479	+You can set a pointer to a mutex_lock in struct video_device. Usually this
	480	+will be either a top-level mutex or a mutex per device node. If you want
	481	+finer-grained locking then you have to set it to NULL and do you own locking.
	482	+
	483	+If a lock is specified then all file operations will be serialized on that
	484	+lock. If you use videobuf then you must pass the same lock to the videobuf
	485	+queue initialize function: if videobuf has to wait for a frame to arrive, then
	486	+it will temporarily unlock the lock and relock it afterwards. If your driver
	487	+also waits in the code, then you should do the same to allow other processes
	488	+to access the device node while the first process is waiting for something.
	489	+
	490	+The implementation of a hotplug disconnect should also take the lock before
	491	+calling v4l2_device_disconnect and video_unregister_device.
472	492
473	493	video_device registration
474	494	-------------------------
...	...	@@ -187,33 +187,50 @@
187	187	size_t sz, loff_t *off)
188	188	{
189	189	struct video_device *vdev = video_devdata(filp);
	190	+ int ret = -EIO;
190	191
191	192	if (!vdev->fops->read)
192	193	return -EINVAL;
193		- if (!video_is_registered(vdev))
194		- return -EIO;
195		- return vdev->fops->read(filp, buf, sz, off);
	194	+ if (vdev->lock)
	195	+ mutex_lock(vdev->lock);
	196	+ if (video_is_registered(vdev))
	197	+ ret = vdev->fops->read(filp, buf, sz, off);
	198	+ if (vdev->lock)
	199	+ mutex_unlock(vdev->lock);
	200	+ return ret;
196	201	}
197	202
198	203	static ssize_t v4l2_write(struct file filp, const char __user buf,
199	204	size_t sz, loff_t *off)
200	205	{
201	206	struct video_device *vdev = video_devdata(filp);
	207	+ int ret = -EIO;
202	208
203	209	if (!vdev->fops->write)
204	210	return -EINVAL;
205		- if (!video_is_registered(vdev))
206		- return -EIO;
207		- return vdev->fops->write(filp, buf, sz, off);
	211	+ if (vdev->lock)
	212	+ mutex_lock(vdev->lock);
	213	+ if (video_is_registered(vdev))
	214	+ ret = vdev->fops->write(filp, buf, sz, off);
	215	+ if (vdev->lock)
	216	+ mutex_unlock(vdev->lock);
	217	+ return ret;
208	218	}
209	219
210	220	static unsigned int v4l2_poll(struct file filp, struct poll_table_struct poll)
211	221	{
212	222	struct video_device *vdev = video_devdata(filp);
	223	+ int ret = DEFAULT_POLLMASK;
213	224
214		- if (!vdev->fops->poll \|\| !video_is_registered(vdev))
215		- return DEFAULT_POLLMASK;
216		- return vdev->fops->poll(filp, poll);
	225	+ if (!vdev->fops->poll)
	226	+ return ret;
	227	+ if (vdev->lock)
	228	+ mutex_lock(vdev->lock);
	229	+ if (video_is_registered(vdev))
	230	+ ret = vdev->fops->poll(filp, poll);
	231	+ if (vdev->lock)
	232	+ mutex_unlock(vdev->lock);
	233	+ return ret;
217	234	}
218	235
219	236	static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
220	237
...	...	@@ -224,7 +241,11 @@
224	241	if (!vdev->fops->ioctl)
225	242	return -ENOTTY;
226	243	if (vdev->fops->unlocked_ioctl) {
	244	+ if (vdev->lock)
	245	+ mutex_lock(vdev->lock);
227	246	ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
	247	+ if (vdev->lock)
	248	+ mutex_unlock(vdev->lock);
228	249	} else if (vdev->fops->ioctl) {
229	250	/* TODO: convert all drivers to unlocked_ioctl */
230	251	lock_kernel();
231	252
...	...	@@ -239,10 +260,17 @@
239	260	static int v4l2_mmap(struct file filp, struct vm_area_struct vm)
240	261	{
241	262	struct video_device *vdev = video_devdata(filp);
	263	+ int ret = -ENODEV;
242	264
243		- if (!vdev->fops->mmap \|\| !video_is_registered(vdev))
244		- return -ENODEV;
245		- return vdev->fops->mmap(filp, vm);
	265	+ if (!vdev->fops->mmap)
	266	+ return ret;
	267	+ if (vdev->lock)
	268	+ mutex_lock(vdev->lock);
	269	+ if (video_is_registered(vdev))
	270	+ ret = vdev->fops->mmap(filp, vm);
	271	+ if (vdev->lock)
	272	+ mutex_unlock(vdev->lock);
	273	+ return ret;
246	274	}
247	275
248	276	/* Override for the open function */
249	277
...	...	@@ -254,17 +282,24 @@
254	282	/* Check if the video device is available */
255	283	mutex_lock(&videodev_lock);
256	284	vdev = video_devdata(filp);
257		- /* return ENODEV if the video device has been removed
258		- already or if it is not registered anymore. */
259		- if (vdev == NULL \|\| !video_is_registered(vdev)) {
	285	+ /* return ENODEV if the video device has already been removed. */
	286	+ if (vdev == NULL) {
260	287	mutex_unlock(&videodev_lock);
261	288	return -ENODEV;
262	289	}
263	290	/* and increase the device refcount */
264	291	video_get(vdev);
265	292	mutex_unlock(&videodev_lock);
266		- if (vdev->fops->open)
267		- ret = vdev->fops->open(filp);
	293	+ if (vdev->fops->open) {
	294	+ if (vdev->lock)
	295	+ mutex_lock(vdev->lock);
	296	+ if (video_is_registered(vdev))
	297	+ ret = vdev->fops->open(filp);
	298	+ else
	299	+ ret = -ENODEV;
	300	+ if (vdev->lock)
	301	+ mutex_unlock(vdev->lock);
	302	+ }
268	303
269	304	/* decrease the refcount in case of an error */
270	305	if (ret)
271	306
...	...	@@ -278,8 +313,13 @@
278	313	struct video_device *vdev = video_devdata(filp);
279	314	int ret = 0;
280	315
281		- if (vdev->fops->release)
	316	+ if (vdev->fops->release) {
	317	+ if (vdev->lock)
	318	+ mutex_lock(vdev->lock);
282	319	vdev->fops->release(filp);
	320	+ if (vdev->lock)
	321	+ mutex_unlock(vdev->lock);
	322	+ }
283	323
284	324	/* decrease the refcount unconditionally since the release()
285	325	return value is ignored. */
...	...	@@ -134,14 +134,21 @@
134	134	if (nonblocking)
135	135	return __v4l2_event_dequeue(fh, event);
136	136
	137	+ /* Release the vdev lock while waiting */
	138	+ if (fh->vdev->lock)
	139	+ mutex_unlock(fh->vdev->lock);
	140	+
137	141	do {
138	142	ret = wait_event_interruptible(events->wait,
139	143	events->navailable != 0);
140	144	if (ret < 0)
141		- return ret;
	145	+ break;
142	146
143	147	ret = __v4l2_event_dequeue(fh, event);
144	148	} while (ret == -ENOENT);
	149	+
	150	+ if (fh->vdev->lock)
	151	+ mutex_lock(fh->vdev->lock);
145	152
146	153	return ret;
147	154	}
...	...	@@ -94,6 +94,9 @@
94	94
95	95	/* ioctl callbacks */
96	96	const struct v4l2_ioctl_ops *ioctl_ops;
	97	+
	98	+ /* serialization lock */
	99	+ struct mutex *lock;
97	100	};
98	101
99	102	/* dev to video-device */