Hi,
the attached patch fixes an SMP race in open and adds interruptible locking
to read and write.
The locking is a new feature, but IMHO very sensible. Allowing only exclusive
open but concurrent reads and writes is not in the same spirit.
In the original code these functions were at least SMP safe, which the patch
restores and it adds some further locking as some drivers are buggy and the
lost latent functionality is very small. (If you absolutly must do so drop
the lock yourself.)
The behaviour of open is _not_ changed. Regardless of whether you want
exclusive open or not, the situation as is is a bug.
Please tell me whether you have fundamental problems with this patch or find
bugs.
Regards
Oliver
--- drivers/media/video/videodev.c.alt Sun Oct 29 20:42:54 2000
+++ drivers/media/video/videodev.c Tue Oct 31 18:18:56 2000
@@ -13,6 +13,8 @@
*
* Fixes: 20000516 Claudio Matsuoka <claudio@xxxxxxxxxxxxx>
* - Added procfs support
+ * 20001031 Oliver Neukum <Oliver.Neukum@xxxxxxxxxxxxxxxx>
+ * - Open SMP safe, locking for read and write
*/
#include <linux/config.h>
@@ -30,16 +32,17 @@
#include <asm/uaccess.h>
#include <asm/system.h>
+#include <asm/bitops.h>
#include <linux/kmod.h>
-#define VIDEO_NUM_DEVICES 256
+#define VIDEO_NUM_DEVICES 256
/*
- * Active devices
+ * Active devices
*/
-
+
static struct video_device *video_device[VIDEO_NUM_DEVICES];
@@ -78,31 +81,37 @@
static struct video_init video_init_list[]={
#ifdef CONFIG_VIDEO_BWQCAM
{"bw-qcam", init_bw_qcams},
-#endif
+#endif
#ifdef CONFIG_VIDEO_CPIA
{"cpia", cpia_init},
-#endif
+#endif
#ifdef CONFIG_VIDEO_PLANB
{"planb", init_planbs},
#endif
#ifdef CONFIG_VIDEO_ZORAN
{"zoran", init_zoran_cards},
-#endif
+#endif
{"end", NULL}
};
/*
* Read will do some smarts later on. Buffer pin etc.
*/
-
+
static ssize_t video_read(struct file *file,
char *buf, size_t count, loff_t *ppos)
{
+ ssize_t retval;
struct video_device *vfl=video_device[MINOR(file->f_dentry->d_inode->i_rdev)];
- if(vfl->read)
- return vfl->read(vfl, buf, count, file->f_flags&O_NONBLOCK);
+ if(vfl->read) {
+ if (down_interruptible(&(vfl->lock)))
+ return -EINTR;
+ retval = vfl->read(vfl, buf, count, file->f_flags&O_NONBLOCK);
+ up(&(vfl->lock));
+ }
else
- return -EINVAL;
+ retval = -EINVAL;
+ return retval;
}
@@ -111,14 +120,20 @@
* for some boards I guess..
*/
-static ssize_t video_write(struct file *file, const char *buf,
+static ssize_t video_write(struct file *file, const char *buf,
size_t count, loff_t *ppos)
{
+ ssize_t retval;
struct video_device *vfl=video_device[MINOR(file->f_dentry->d_inode->i_rdev)];
- if(vfl->write)
- return vfl->write(vfl, buf, count, file->f_flags&O_NONBLOCK);
+ if(vfl->write) {
+ if (down_interruptible(&(vfl->lock)))
+ return -EINTR;
+ retval = vfl->write(vfl, buf, count, file->f_flags&O_NONBLOCK);
+ up(&(vfl->lock));
+ }
else
- return 0;
+ retval = 0;
+ return retval;
}
/*
@@ -145,10 +160,10 @@
unsigned int minor = MINOR(inode->i_rdev);
int err;
struct video_device *vfl;
-
+
if(minor>=VIDEO_NUM_DEVICES)
return -ENODEV;
-
+
vfl=video_device[minor];
if(vfl==NULL) {
char modname[20];
@@ -159,26 +174,26 @@
if (vfl==NULL)
return -ENODEV;
}
- if(vfl->busy)
+ if(test_and_set_bit(0,&(vfl->busy)))
return -EBUSY;
- vfl->busy=1; /* In case vfl->open sleeps */
-
+
if(vfl->open)
{
err=vfl->open(vfl,0); /* Tell the device it is open */
if(err)
{
- vfl->busy=0;
+ clear_bit(0,&(vfl->busy));
return err;
}
}
+ init_MUTEX(&(vfl->lock));
return 0;
}
/*
* Last close of a video for Linux device
*/
-
+
static int video_release(struct inode *inode, struct file *file)
{
struct video_device *vfl;
@@ -186,7 +201,7 @@
vfl=video_device[MINOR(inode->i_rdev)];
if(vfl->close)
vfl->close(vfl);
- vfl->busy=0;
+ clear_bit(0,&(vfl->busy));
unlock_kernel();
return 0;
}
@@ -195,7 +210,7 @@
* Question: Should we be able to capture and then seek around the
* image ?
*/
-
+
static long long video_lseek(struct file * file,
long long offset, int origin)
{
@@ -210,26 +225,22 @@
if(err!=-ENOIOCTLCMD)
return err;
-
- switch(cmd)
- {
- default:
- return -EINVAL;
- }
+ else
+ return -EINVAL;
}
/*
* We need to do MMAP support
*/
-
-
+
+
int video_mmap(struct file *file, struct vm_area_struct *vma)
{
int ret = -EINVAL;
struct video_device *vfl=video_device[MINOR(file->f_dentry->d_inode->i_rdev)];
if(vfl->mmap) {
lock_kernel();
- ret = vfl->mmap(vfl, (char *)vma->vm_start,
+ ret = vfl->mmap(vfl, (char *)vma->vm_start,
(unsigned long)(vma->vm_end-vma->vm_start));
unlock_kernel();
}
@@ -264,7 +275,7 @@
/* Sanity check */
if (tmp == &videodev_proc_list)
goto skip;
-
+
#define PRINT_VID_TYPE(x) do { if (vfd->type & x) \
out += sprintf (out, "%c%s", c, #x); c='|';} while (0)
@@ -393,7 +404,7 @@
* @vfd: video device structure we want to register
* @type: type of device to register
* FIXME: needs a semaphore on 2.3.x
- *
+ *
* The registration code assigns minor numbers based on the type
* requested. -ENFILE is returned in all the device slots for this
* category are full. If not then the minor field is set and the
@@ -409,9 +420,9 @@
*
* %VFL_TYPE_VBI - Vertical blank data (undecoded)
*
- * %VFL_TYPE_RADIO - A radio card
+ * %VFL_TYPE_RADIO - A radio card
*/
-
+
int video_register_device(struct video_device *vfd, int type)
{
int i=0;
@@ -419,7 +430,7 @@
int err;
int end;
char *name_base;
-
+
switch(type)
{
case VFL_TYPE_GRABBER:
@@ -579,3 +590,4 @@
MODULE_AUTHOR("Alan Cox");
MODULE_DESCRIPTION("Device registrar for Video4Linux drivers");
+
--- include/linux/videodev.h.alt Mon Oct 30 07:41:41 2000
+++ include/linux/videodev.h Mon Oct 30 07:41:44 2000
@@ -32,6 +32,7 @@
int busy;
int minor;
devfs_handle_t devfs_handle;
+ struct semaphore lock;
};
extern int videodev_init(void);
