video4linux: Push down the BKL

PATCH In review Alan Cox 9 participants 44 messages May 22, 2008 – Jun 4, 2008

Subsystems (3)

Network drivers Media (V4L/DVB) TUN/TAP driver

lore

Share & tools

lore

Alan Cox

author

2008-05-22 21:49 UTC #0 lore raw

PATCH 1/1 video4linux: Push down the BKL

For most drivers the generic ioctl handler does the work and we update it and it becomes the unlocked_ioctl method. Older drivers use the usercopy method so we make it do the work. Finally there are a few special cases.

Signed-off-by Alan Cox <[email protected]>

drivers/media/video/bt8xx/bttv-driver.c +2 -2

index 2ca3e9c..964fc31 100644
@@ -3349,7 +3349,7 @@ static const struct file_operations bttv_fops =
 	.owner	  = THIS_MODULE,
 	.open	  = bttv_open,
 	.release  = bttv_release,
-	.ioctl	  = video_ioctl2,
+	.unlocked_ioctl	  = video_ioctl2,
 	.compat_ioctl	= v4l_compat_ioctl32,
 	.llseek	  = no_llseek,
 	.read	  = bttv_read,

@@ -3630,7 +3630,7 @@ static const struct file_operations radio_fops =
 	.read     = radio_read,
 	.release  = radio_release,
 	.compat_ioctl	= v4l_compat_ioctl32,
-	.ioctl	  = video_ioctl2,
+	.unlocked_ioctl	  = video_ioctl2,
 	.llseek	  = no_llseek,
 	.poll     = radio_poll,
 };

drivers/media/video/bw-qcam.c +3 -4

index b364ada..e953bc9 100644
@@ -863,10 +863,9 @@ static int qcam_do_ioctl(struct inode *inode, struct file *file,
 	return 0;
 }
 
-static int qcam_ioctl(struct inode *inode, struct file *file,
-		     unsigned int cmd, unsigned long arg)
+static long qcam_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, qcam_do_ioctl);
+	return video_usercopy(file, cmd, arg, qcam_do_ioctl);
 }
 
 static ssize_t qcam_read(struct file *file, char __user *buf,

@@ -897,7 +896,7 @@ static const struct file_operations qcam_fops = {
 	.owner		= THIS_MODULE,
 	.open           = video_exclusive_open,
 	.release        = video_exclusive_release,
-	.ioctl          = qcam_ioctl,
+	.unlocked_ioctl = qcam_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= v4l_compat_ioctl32,
 #endif

drivers/media/video/c-qcam.c +4 -5

index fe1e67b..7541e2c 100644
@@ -499,7 +499,7 @@ static long qc_capture(struct qcam_device *q, char __user *buf, unsigned long le
  */
 
 static int qcam_do_ioctl(struct inode *inode, struct file *file,
-			 unsigned int cmd, void *arg)
+						unsigned int cmd, void *arg)
 {
 	struct video_device *dev = video_devdata(file);
 	struct qcam_device *qcam=(struct qcam_device *)dev;

@@ -664,10 +664,9 @@ static int qcam_do_ioctl(struct inode *inode, struct file *file,
 	return 0;
 }
 
-static int qcam_ioctl(struct inode *inode, struct file *file,
-		     unsigned int cmd, unsigned long arg)
+static long qcam_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, qcam_do_ioctl);
+	return video_usercopy(file, cmd, arg, qcam_do_ioctl);
 }
 
 static ssize_t qcam_read(struct file *file, char __user *buf,

@@ -691,7 +690,7 @@ static const struct file_operations qcam_fops = {
 	.owner		= THIS_MODULE,
 	.open           = video_exclusive_open,
 	.release        = video_exclusive_release,
-	.ioctl          = qcam_ioctl,
+	.unlocked_ioctl = qcam_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= v4l_compat_ioctl32,
 #endif

drivers/media/video/cafe_ccic.c +1 -1

index 5195b1f..8b697e3 100644
@@ -1764,7 +1764,7 @@ static const struct file_operations cafe_v4l_fops = {
 	.read = cafe_v4l_read,
 	.poll = cafe_v4l_poll,
 	.mmap = cafe_v4l_mmap,
-	.ioctl = video_ioctl2,
+	.unlocked_ioctl = video_ioctl2,
 	.llseek = no_llseek,
 };

drivers/media/video/cpia.c +3 -4

index 2a81376..93766a1 100644
@@ -3725,10 +3725,9 @@ static int cpia_do_ioctl(struct inode *inode, struct file *file,
 	return retval;
 }
 
-static int cpia_ioctl(struct inode *inode, struct file *file,
-		     unsigned int cmd, unsigned long arg)
+static long cpia_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, cpia_do_ioctl);
+	return video_usercopy(file, cmd, arg, cpia_do_ioctl);
 }

@@ -3791,7 +3790,7 @@ static const struct file_operations cpia_fops = {
 	.release       	= cpia_close,
 	.read		= cpia_read,
 	.mmap		= cpia_mmap,
-	.ioctl          = cpia_ioctl,
+	.unlocked_ioctl = cpia_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= v4l_compat_ioctl32,
 #endif

drivers/media/video/cpia2/cpia2_v4l.c +4 -4

index 7ce2789..bf551f8 100644
@@ -1847,10 +1847,10 @@ static int cpia2_do_ioctl(struct inode *inode, struct file *file,
 	return retval;
 }
 
-static int cpia2_ioctl(struct inode *inode, struct file *file,
-		       unsigned int ioctl_nr, unsigned long iarg)
+static long cpia2_ioctl(struct file *file, unsigned int ioctl_nr,
+							unsigned long iarg)
 {
-	return video_usercopy(inode, file, ioctl_nr, iarg, cpia2_do_ioctl);
+	return video_usercopy(file, ioctl_nr, iarg, cpia2_do_ioctl);
 }
 
 /******************************************************************************

@@ -1925,7 +1925,7 @@ static const struct file_operations fops_template = {
 	.release	= cpia2_close,
 	.read		= cpia2_v4l_read,
 	.poll		= cpia2_v4l_poll,
-	.ioctl		= cpia2_ioctl,
+	.unlocked_ioctl	= cpia2_ioctl,
 	.llseek		= no_llseek,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= v4l_compat_ioctl32,

drivers/media/video/cx18/cx18-ioctl.c +4 -3

index dbdcb86..faf3a31 100644
@@ -837,15 +837,16 @@ static int cx18_v4l2_do_ioctl(struct inode *inode, struct file *filp,
 	return 0;
 }
 
-int cx18_v4l2_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
-		    unsigned long arg)
+long cx18_v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	struct cx18_open_id *id = (struct cx18_open_id *)filp->private_data;
 	struct cx18 *cx = id->cx;
 	int res;
 
+	lock_kernel();
 	mutex_lock(&cx->serialize_lock);
-	res = video_usercopy(inode, filp, cmd, arg, cx18_v4l2_do_ioctl);
+	res = video_usercopy(filp, cmd, arg, cx18_v4l2_do_ioctl);
 	mutex_unlock(&cx->serialize_lock);
+	unlock_kernel();
 	return res;
 }

drivers/media/video/cx18/cx18-ioctl.h +2 -3

index 9f4c7eb..32bede3 100644
@@ -1,4 +1,4 @@
-/*
+ /*
  *  cx18 ioctl system call
  *
  *  Derived from ivtv-ioctl.h

@@ -24,7 +24,6 @@
 u16 cx18_service2vbi(int type);
 void cx18_expand_service_set(struct v4l2_sliced_vbi_format *fmt, int is_pal);
 u16 cx18_get_service_set(struct v4l2_sliced_vbi_format *fmt);
-int cx18_v4l2_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
-		    unsigned long arg);
+long cx18_v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
 int cx18_v4l2_ioctls(struct cx18 *cx, struct file *filp, unsigned cmd,
 		     void *arg);

drivers/media/video/cx18/cx18-streams.c +1 -1

index afb141b..7348b82 100644
@@ -39,7 +39,7 @@ static struct file_operations cx18_v4l2_enc_fops = {
       .owner = THIS_MODULE,
       .read = cx18_v4l2_read,
       .open = cx18_v4l2_open,
-      .ioctl = cx18_v4l2_ioctl,
+      .unlocked_ioctl = cx18_v4l2_ioctl,
       .release = cx18_v4l2_close,
       .poll = cx18_v4l2_enc_poll,
 };

drivers/media/video/cx23885/cx23885-417.c +3 -4

index acdd3b6..8530c60 100644
@@ -1542,10 +1542,9 @@ static int mpeg_do_ioctl(struct inode *inode, struct file *file,
 	return 0;
 }
 
-static int mpeg_ioctl(struct inode *inode, struct file *file,
-			unsigned int cmd, unsigned long arg)
+static int mpeg_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, mpeg_do_ioctl);
+	return video_usercopy(file, cmd, arg, mpeg_do_ioctl);
 }
 
 static int mpeg_open(struct inode *inode, struct file *file)

@@ -1670,7 +1669,7 @@ static struct file_operations mpeg_fops = {
 	.read	       = mpeg_read,
 	.poll          = mpeg_poll,
 	.mmap	       = mpeg_mmap,
-	.ioctl	       = mpeg_ioctl,
+	.unlocked_ioctl= mpeg_ioctl,
 	.llseek        = no_llseek,
 };

drivers/media/video/cx23885/cx23885-video.c +2 -2

index 8465221..c4059fa 100644
@@ -1428,7 +1428,7 @@ static const struct file_operations video_fops = {
 	.read	       = video_read,
 	.poll          = video_poll,
 	.mmap	       = video_mmap,
-	.ioctl	       = video_ioctl2,
+	.unlocked_ioctl	       = video_ioctl2,
 	.compat_ioctl  = v4l_compat_ioctl32,
 	.llseek        = no_llseek,
 };

@@ -1479,7 +1479,7 @@ static const struct file_operations radio_fops = {
 	.owner         = THIS_MODULE,
 	.open          = video_open,
 	.release       = video_release,
-	.ioctl         = video_ioctl2,
+	.unlocked_ioctl         = video_ioctl2,
 	.compat_ioctl  = v4l_compat_ioctl32,
 	.llseek        = no_llseek,
 };

drivers/media/video/cx88/cx88-blackbird.c +1 -1

index 6c0c94c..9b5f761 100644
@@ -1169,7 +1169,7 @@ static const struct file_operations mpeg_fops =
 	.read	       = mpeg_read,
 	.poll          = mpeg_poll,
 	.mmap	       = mpeg_mmap,
-	.ioctl	       = video_ioctl2,
+	.unlocked_ioctl= video_ioctl2,
 	.llseek        = no_llseek,
 };

drivers/media/video/cx88/cx88-video.c +2 -2

index eea23f9..ee1d29b 100644
@@ -1677,7 +1677,7 @@ static const struct file_operations video_fops =
 	.read	       = video_read,
 	.poll          = video_poll,
 	.mmap	       = video_mmap,
-	.ioctl	       = video_ioctl2,
+	.unlocked_ioctl = video_ioctl2,
 	.compat_ioctl  = v4l_compat_ioctl32,
 	.llseek        = no_llseek,
 };

@@ -1730,7 +1730,7 @@ static const struct file_operations radio_fops =
 	.owner         = THIS_MODULE,
 	.open          = video_open,
 	.release       = video_release,
-	.ioctl         = video_ioctl2,
+	.unlocked_ioctl = video_ioctl2,
 	.compat_ioctl  = v4l_compat_ioctl32,
 	.llseek        = no_llseek,
 };

drivers/media/video/dabusb.c +3 -2

index 8d1f8ee..ae26ade 100644
@@ -636,7 +636,8 @@ static int dabusb_release (struct inode *inode, struct file *file)
 	return 0;
 }
 
-static int dabusb_ioctl (struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg)
+static long dabusb_ioctl(struct file *file, unsigned int cmd,
+							unsigned long arg)
 {
 	pdabusb_t s = (pdabusb_t) file->private_data;
 	pbulk_transfer_t pbulk;

@@ -700,7 +701,7 @@ static const struct file_operations dabusb_fops =
 	.owner =	THIS_MODULE,
 	.llseek =	no_llseek,
 	.read =		dabusb_read,
-	.ioctl =	dabusb_ioctl,
+	.unlocked_ioctl = dabusb_ioctl,
 	.open =		dabusb_open,
 	.release =	dabusb_release,
 };

drivers/media/video/em28xx/em28xx-video.c +2 -2

index 8996175..f1e08b2 100644
@@ -1756,7 +1756,7 @@ static const struct file_operations em28xx_v4l_fops = {
 	.read          = em28xx_v4l2_read,
 	.poll          = em28xx_v4l2_poll,
 	.mmap          = em28xx_v4l2_mmap,
-	.ioctl	       = video_ioctl2,
+	.unlocked_ioctl= video_ioctl2,
 	.llseek        = no_llseek,
 	.compat_ioctl  = v4l_compat_ioctl32,
 };

@@ -1765,7 +1765,7 @@ static const struct file_operations radio_fops = {
 	.owner         = THIS_MODULE,
 	.open          = em28xx_v4l2_open,
 	.release       = em28xx_v4l2_close,
-	.ioctl	       = video_ioctl2,
+	.unlocked_ioctl= video_ioctl2,
 	.compat_ioctl  = v4l_compat_ioctl32,
 	.llseek        = no_llseek,
 };

drivers/media/video/et61x251/et61x251_core.c +8 -6

index 5e749c5..9f13cca 100644
@@ -2391,8 +2391,8 @@ et61x251_vidioc_s_parm(struct et61x251_device* cam, void __user * arg)
 }
 
 
-static int et61x251_ioctl_v4l2(struct inode* inode, struct file* filp,
-			       unsigned int cmd, void __user * arg)
+static int et61x251_ioctl_v4l2(struct file* filp, unsigned int cmd,
+							void __user * arg)
 {
 	struct et61x251_device* cam = video_get_drvdata(video_devdata(filp));

@@ -2486,8 +2486,8 @@ static int et61x251_ioctl_v4l2(struct inode* inode, struct file* filp,
 }
 
 
-static int et61x251_ioctl(struct inode* inode, struct file* filp,
-			 unsigned int cmd, unsigned long arg)
+static long et61x251_ioctl(struct file* filp, unsigned int cmd,
+							unsigned long arg)
 {
 	struct et61x251_device* cam = video_get_drvdata(video_devdata(filp));
 	int err = 0;

@@ -2510,7 +2510,9 @@ static int et61x251_ioctl(struct inode* inode, struct file* filp,
 
 	V4LDBG(3, "et61x251", cmd);
 
-	err = et61x251_ioctl_v4l2(inode, filp, cmd, (void __user *)arg);
+	lock_kernel();
+	err = et61x251_ioctl_v4l2(filp, cmd, (void __user *)arg);
+	unlock_kernel();
 
 	mutex_unlock(&cam->fileop_mutex);

@@ -2522,7 +2524,7 @@ static const struct file_operations et61x251_fops = {
 	.owner = THIS_MODULE,
 	.open =    et61x251_open,
 	.release = et61x251_release,
-	.ioctl =   et61x251_ioctl,
+	.unlocked_ioctl =   et61x251_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl = v4l_compat_ioctl32,
 #endif

drivers/media/video/ivtv/ivtv-ioctl.c +6 -5

index d508b5d..a481b2d 100644
@@ -1726,7 +1726,7 @@ static int ivtv_v4l2_do_ioctl(struct inode *inode, struct file *filp,
 	return 0;
 }
 
-static int ivtv_serialized_ioctl(struct ivtv *itv, struct inode *inode, struct file *filp,
+static int ivtv_serialized_ioctl(struct ivtv *itv, struct file *filp,
 		unsigned int cmd, unsigned long arg)
 {
 	/* Filter dvb ioctls that cannot be handled by video_usercopy */

@@ -1761,18 +1761,19 @@ static int ivtv_serialized_ioctl(struct ivtv *itv, struct inode *inode, struct f
 	default:
 		break;
 	}
-	return video_usercopy(inode, filp, cmd, arg, ivtv_v4l2_do_ioctl);
+	return video_usercopy(filp, cmd, arg, ivtv_v4l2_do_ioctl);
 }
 
-int ivtv_v4l2_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
-		    unsigned long arg)
+long ivtv_v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	struct ivtv_open_id *id = (struct ivtv_open_id *)filp->private_data;
 	struct ivtv *itv = id->itv;
 	int res;
 
+	lock_kernel();
 	mutex_lock(&itv->serialize_lock);
-	res = ivtv_serialized_ioctl(itv, inode, filp, cmd, arg);
+	res = ivtv_serialized_ioctl(itv, filp, cmd, arg);
 	mutex_unlock(&itv->serialize_lock);
+	unlock_kernel();
 	return res;
 }

drivers/media/video/ivtv/ivtv-ioctl.h +1 -2

index a03351b..6708ea0 100644
@@ -24,8 +24,7 @@
 u16 service2vbi(int type);
 void expand_service_set(struct v4l2_sliced_vbi_format *fmt, int is_pal);
 u16 get_service_set(struct v4l2_sliced_vbi_format *fmt);
-int ivtv_v4l2_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
-		    unsigned long arg);
+long ivtv_v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
 int ivtv_v4l2_ioctls(struct ivtv *itv, struct file *filp, unsigned int cmd, void *arg);
 void ivtv_set_osd_alpha(struct ivtv *itv);
 int ivtv_set_speed(struct ivtv *itv, int speed);

drivers/media/video/ivtv/ivtv-streams.c +2 -2

index 4ab8d36..3131f68 100644
@@ -48,7 +48,7 @@ static const struct file_operations ivtv_v4l2_enc_fops = {
       .read = ivtv_v4l2_read,
       .write = ivtv_v4l2_write,
       .open = ivtv_v4l2_open,
-      .ioctl = ivtv_v4l2_ioctl,
+      .unlocked_ioctl = ivtv_v4l2_ioctl,
       .release = ivtv_v4l2_close,
       .poll = ivtv_v4l2_enc_poll,
 };

@@ -58,7 +58,7 @@ static const struct file_operations ivtv_v4l2_dec_fops = {
       .read = ivtv_v4l2_read,
       .write = ivtv_v4l2_write,
       .open = ivtv_v4l2_open,
-      .ioctl = ivtv_v4l2_ioctl,
+      .unlocked_ioctl = ivtv_v4l2_ioctl,
       .release = ivtv_v4l2_close,
       .poll = ivtv_v4l2_dec_poll,
 };

drivers/media/video/meye.c +1 -1

index e7ccbc8..a92c0ba 100644
@@ -1687,7 +1687,7 @@ static const struct file_operations meye_fops = {
 	.open		= meye_open,
 	.release	= meye_release,
 	.mmap		= meye_mmap,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= v4l_compat_ioctl32,
 #endif

drivers/media/video/ov511.c +11 ~~-12~~

index eafb0c7..3e3f70b 100644
@@ -4440,9 +4440,8 @@ redo:
 	return 0;
 }
 
-static int
-ov51x_v4l1_ioctl(struct inode *inode, struct file *file,
-		 unsigned int cmd, unsigned long arg)
+static long
+ov51x_v4l1_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct video_device *vdev = file->private_data;
 	struct usb_ov511 *ov = video_get_drvdata(vdev);

@@ -4451,7 +4450,7 @@ ov51x_v4l1_ioctl(struct inode *inode, struct file *file,
 	if (mutex_lock_interruptible(&ov->lock))
 		return -EINTR;
 
-	rc = video_usercopy(inode, file, cmd, arg, ov51x_v4l1_ioctl_internal);
+	rc = video_usercopy(file, cmd, arg, ov51x_v4l1_ioctl_internal);
 
 	mutex_unlock(&ov->lock);
 	return rc;

@@ -4653,16 +4652,16 @@ ov51x_v4l1_mmap(struct file *file, struct vm_area_struct *vma)
 }
 
 static const struct file_operations ov511_fops = {
-	.owner =	THIS_MODULE,
-	.open =		ov51x_v4l1_open,
-	.release =	ov51x_v4l1_close,
-	.read =		ov51x_v4l1_read,
-	.mmap =		ov51x_v4l1_mmap,
-	.ioctl =	ov51x_v4l1_ioctl,
+	.owner =		THIS_MODULE,
+	.open =			ov51x_v4l1_open,
+	.release =		ov51x_v4l1_close,
+	.read =			ov51x_v4l1_read,
+	.mmap =			ov51x_v4l1_mmap,
+	.unlocked_ioctl =	ov51x_v4l1_ioctl,
 #ifdef CONFIG_COMPAT
-	.compat_ioctl = v4l_compat_ioctl32,
+	.compat_ioctl = 	v4l_compat_ioctl32,
 #endif
-	.llseek =	no_llseek,
+	.llseek =		no_llseek,
 };
 
 static struct video_device vdev_template = {

drivers/media/video/pms.c +3 -4

index 51b1461..4cfe7aa 100644
@@ -861,10 +861,9 @@ static int pms_do_ioctl(struct inode *inode, struct file *file,
 	return 0;
 }
 
-static int pms_ioctl(struct inode *inode, struct file *file,
-		     unsigned int cmd, unsigned long arg)
+static long pms_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, pms_do_ioctl);
+	return video_usercopy(file, cmd, arg, pms_do_ioctl);
 }
 
 static ssize_t pms_read(struct file *file, char __user *buf,

@@ -884,7 +883,7 @@ static const struct file_operations pms_fops = {
 	.owner		= THIS_MODULE,
 	.open           = video_exclusive_open,
 	.release        = video_exclusive_release,
-	.ioctl          = pms_ioctl,
+	.unlocked_ioctl = pms_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= v4l_compat_ioctl32,
 #endif

drivers/media/video/pvrusb2/pvrusb2-v4l2.c +3 -3

index e9b5d4e..5deb006 100644
@@ -862,7 +862,7 @@ static void pvr2_v4l2_internal_check(struct pvr2_channel *chp)
 }
 
 
-static int pvr2_v4l2_ioctl(struct inode *inode, struct file *file,
+static long pvr2_v4l2_ioctl(struct file *file,
 			   unsigned int cmd, unsigned long arg)
 {

@@ -870,7 +870,7 @@ static int pvr2_v4l2_ioctl(struct inode *inode, struct file *file,
 #define IVTV_IOC_G_CODEC        0xFFEE7703
 #define IVTV_IOC_S_CODEC        0xFFEE7704
 	if (cmd == IVTV_IOC_G_CODEC || cmd == IVTV_IOC_S_CODEC) return 0;
-	return video_usercopy(inode, file, cmd, arg, pvr2_v4l2_do_ioctl);
+	return video_usercopy(file, cmd, arg, pvr2_v4l2_do_ioctl);
 }

@@ -1154,7 +1154,7 @@ static const struct file_operations vdev_fops = {
 	.open       = pvr2_v4l2_open,
 	.release    = pvr2_v4l2_release,
 	.read       = pvr2_v4l2_read,
-	.ioctl      = pvr2_v4l2_ioctl,
+	.unlocked_ioctl = pvr2_v4l2_ioctl,
 	.llseek     = no_llseek,
 	.poll       = pvr2_v4l2_poll,
 };

drivers/media/video/pwc/pwc-if.c +6 -6

index 423fa7c..d907966 100644
@@ -147,8 +147,8 @@ static int pwc_video_close(struct inode *inode, struct file *file);
 static ssize_t pwc_video_read(struct file *file, char __user *buf,
 			  size_t count, loff_t *ppos);
 static unsigned int pwc_video_poll(struct file *file, poll_table *wait);
-static int  pwc_video_ioctl(struct inode *inode, struct file *file,
-			    unsigned int ioctlnr, unsigned long arg);
+static long pwc_video_ioctl(struct file *file, unsigned int ioctlnr,
+							unsigned long arg);
 static int  pwc_video_mmap(struct file *file, struct vm_area_struct *vma);
 
 static const struct file_operations pwc_fops = {

@@ -158,7 +158,7 @@ static const struct file_operations pwc_fops = {
 	.read =		pwc_video_read,
 	.poll =		pwc_video_poll,
 	.mmap =		pwc_video_mmap,
-	.ioctl =        pwc_video_ioctl,
+	.unlocked_ioctl = pwc_video_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl = v4l_compat_ioctl32,
 #endif

@@ -1400,8 +1400,8 @@ static unsigned int pwc_video_poll(struct file *file, poll_table *wait)
 	return 0;
 }
 
-static int pwc_video_ioctl(struct inode *inode, struct file *file,
-			   unsigned int cmd, unsigned long arg)
+static long pwc_video_ioctl(struct file *file, unsigned int cmd,
+						unsigned long arg)
 {
 	struct video_device *vdev = file->private_data;
 	struct pwc_device *pdev;

@@ -1413,7 +1413,7 @@ static int pwc_video_ioctl(struct inode *inode, struct file *file,
 
 	mutex_lock(&pdev->modlock);
 	if (!pdev->unplugged)
-		r = video_usercopy(inode, file, cmd, arg, pwc_video_do_ioctl);
+		r = video_usercopy(file, cmd, arg, pwc_video_do_ioctl);
 	mutex_unlock(&pdev->modlock);
 out:
 	return r;

drivers/media/video/saa5246a.c +4 -4

index 996b494..9691ac1 100644
@@ -715,8 +715,8 @@ static inline unsigned int vtx_fix_command(unsigned int cmd)
 /*
  *	Handle the locking
  */
-static int saa5246a_ioctl(struct inode *inode, struct file *file,
-			 unsigned int cmd, unsigned long arg)
+static int saa5246a_ioctl(struct file *file, unsigned int cmd,
+							unsigned long arg)
 {
 	struct video_device *vd = video_devdata(file);
 	struct saa5246a_device *t = vd->priv;

@@ -724,7 +724,7 @@ static int saa5246a_ioctl(struct inode *inode, struct file *file,
 
 	cmd = vtx_fix_command(cmd);
 	mutex_lock(&t->lock);
-	err = video_usercopy(inode, file, cmd, arg, do_saa5246a_ioctl);
+	err = video_usercopy(file, cmd, arg, do_saa5246a_ioctl);
 	mutex_unlock(&t->lock);
 	return err;
 }

@@ -822,7 +822,7 @@ static const struct file_operations saa_fops = {
 	.owner	 = THIS_MODULE,
 	.open	 = saa5246a_open,
 	.release = saa5246a_release,
-	.ioctl	 = saa5246a_ioctl,
+	.unlocked_ioctl = saa5246a_ioctl,
 	.llseek	 = no_llseek,
 };

drivers/media/video/saa5249.c +4 -4

index ec8c65d..159f7f0 100644
@@ -611,8 +611,8 @@ static inline unsigned int vtx_fix_command(unsigned int cmd)
  *	Handle the locking
  */
 
-static int saa5249_ioctl(struct inode *inode, struct file *file,
-			 unsigned int cmd, unsigned long arg)
+static long saa5249_ioctl(struct file *file, unsigned int cmd,
+						unsigned long arg)
 {
 	struct video_device *vd = video_devdata(file);
 	struct saa5249_device *t=vd->priv;

@@ -620,7 +620,7 @@ static int saa5249_ioctl(struct inode *inode, struct file *file,
 
 	cmd = vtx_fix_command(cmd);
 	mutex_lock(&t->lock);
-	err = video_usercopy(inode,file,cmd,arg,do_saa5249_ioctl);
+	err = video_usercopy(file,cmd,arg,do_saa5249_ioctl);
 	mutex_unlock(&t->lock);
 	return err;
 }

@@ -700,7 +700,7 @@ static const struct file_operations saa_fops = {
 	.owner		= THIS_MODULE,
 	.open		= saa5249_open,
 	.release       	= saa5249_release,
-	.ioctl          = saa5249_ioctl,
+	.unlocked_ioctl = saa5249_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= v4l_compat_ioctl32,
 #endif

drivers/media/video/saa7134/saa7134-empress.c +1 -1

index 1314522..a768f70 100644
@@ -339,7 +339,7 @@ static const struct file_operations ts_fops =
 	.read	  = ts_read,
 	.poll	  = ts_poll,
 	.mmap	  = ts_mmap,
-	.ioctl	  = video_ioctl2,
+	.unlocked_ioctl	  = video_ioctl2,
 	.llseek   = no_llseek,
 };

drivers/media/video/saa7134/saa7134-video.c +2 -2

index 48e1a01..abdd5c3 100644
@@ -2322,7 +2322,7 @@ static const struct file_operations video_fops =
 	.read	  = video_read,
 	.poll     = video_poll,
 	.mmap	  = video_mmap,
-	.ioctl	  = video_ioctl2,
+	.unlocked_ioctl = video_ioctl2,
 	.compat_ioctl	= v4l_compat_ioctl32,
 	.llseek   = no_llseek,
 };

@@ -2332,7 +2332,7 @@ static const struct file_operations radio_fops =
 	.owner	  = THIS_MODULE,
 	.open	  = video_open,
 	.release  = video_release,
-	.ioctl	  = video_ioctl2,
+	.unlocked_ioctl = video_ioctl2,
 	.compat_ioctl	= v4l_compat_ioctl32,
 	.llseek   = no_llseek,
 };

drivers/media/video/se401.c +4 -4

index 1cd6293..f02fdc0 100644
@@ -1133,10 +1133,10 @@ static int se401_do_ioctl(struct inode *inode, struct file *file,
 	return 0;
 }
 
-static int se401_ioctl(struct inode *inode, struct file *file,
-		       unsigned int cmd, unsigned long arg)
+static long se401_ioctl(struct file *file, unsigned int cmd,
+						unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, se401_do_ioctl);
+	return video_usercopy(file, cmd, arg, se401_do_ioctl);
 }
 
 static ssize_t se401_read(struct file *file, char __user *buf,

@@ -1223,7 +1223,7 @@ static const struct file_operations se401_fops = {
 	.release =      se401_close,
 	.read =         se401_read,
 	.mmap =         se401_mmap,
-	.ioctl =        se401_ioctl,
+	.unlocked_ioctl = se401_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl = v4l_compat_ioctl32,
 #endif

drivers/media/video/sn9c102/sn9c102_core.c +8 -7

index 5748b1e..fa6eaef 100644
@@ -3082,8 +3082,8 @@ sn9c102_vidioc_s_audio(struct sn9c102_device* cam, void __user * arg)
 }
 
 
-static int sn9c102_ioctl_v4l2(struct inode* inode, struct file* filp,
-			      unsigned int cmd, void __user * arg)
+static long sn9c102_ioctl_v4l2(struct file* filp, unsigned int cmd,
+							void __user * arg)
 {
 	struct sn9c102_device* cam = video_get_drvdata(video_devdata(filp));

@@ -3186,8 +3186,8 @@ static int sn9c102_ioctl_v4l2(struct inode* inode, struct file* filp,
 }
 
 
-static int sn9c102_ioctl(struct inode* inode, struct file* filp,
-			 unsigned int cmd, unsigned long arg)
+static long sn9c102_ioctl(struct file* filp, unsigned int cmd,
+							unsigned long arg)
 {
 	struct sn9c102_device* cam = video_get_drvdata(video_devdata(filp));
 	int err = 0;

@@ -3210,8 +3210,9 @@ static int sn9c102_ioctl(struct inode* inode, struct file* filp,
 
 	V4LDBG(3, "sn9c102", cmd);
 
-	err = sn9c102_ioctl_v4l2(inode, filp, cmd, (void __user *)arg);
-
+	lock_kernel();
+	err = sn9c102_ioctl_v4l2(filp, cmd, (void __user *)arg);
+	unlock_kernel();
 	mutex_unlock(&cam->fileop_mutex);
 
 	return err;

@@ -3223,7 +3224,7 @@ static const struct file_operations sn9c102_fops = {
 	.owner = THIS_MODULE,
 	.open = sn9c102_open,
 	.release = sn9c102_release,
-	.ioctl = sn9c102_ioctl,
+	.unlocked_ioctl = sn9c102_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl = v4l_compat_ioctl32,
 #endif

drivers/media/video/soc_camera.c +1 -1

index a1b9244..2f6ede2 100644
@@ -334,7 +334,7 @@ static struct file_operations soc_camera_fops = {
 	.owner		= THIS_MODULE,
 	.open		= soc_camera_open,
 	.release	= soc_camera_close,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 	.read		= soc_camera_read,
 	.mmap		= soc_camera_mmap,
 	.poll		= soc_camera_poll,

drivers/media/video/stk-webcam.c +1 -1

index 9276ed9..bd32c58 100644
@@ -1313,7 +1313,7 @@ static struct file_operations v4l_stk_fops = {
 	.read = v4l_stk_read,
 	.poll = v4l_stk_poll,
 	.mmap = v4l_stk_mmap,
-	.ioctl = video_ioctl2,
+	.unlocked_ioctl = video_ioctl2,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl = v4l_compat_ioctl32,
 #endif

drivers/media/video/stradis.c +81 ~~-13~~

343 lines not on this page · raw message

drivers/media/video/stv680.c +4 -4

index d7f130b..2d38d76 100644
@@ -1295,10 +1295,10 @@ static int stv680_do_ioctl (struct inode *inode, struct file *file,
 	return 0;
 }
 
-static int stv680_ioctl(struct inode *inode, struct file *file,
-			unsigned int cmd, unsigned long arg)
+static long stv680_ioctl(struct file *file, unsigned int cmd,
+							unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, stv680_do_ioctl);
+	return video_usercopy(file, cmd, arg, stv680_do_ioctl);
 }
 
 static int stv680_mmap (struct file *file, struct vm_area_struct *vma)

@@ -1393,7 +1393,7 @@ static const struct file_operations stv680_fops = {
 	.release =     	stv_close,
 	.read =		stv680_read,
 	.mmap =		stv680_mmap,
-	.ioctl =        stv680_ioctl,
+	.unlocked_ioctl =        stv680_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl = v4l_compat_ioctl32,
 #endif

drivers/media/video/usbvideo/usbvideo.c +6 -6

index 4128ee2..b48a8fb 100644
@@ -41,8 +41,8 @@ module_param(video_nr, int, 0);
 static void usbvideo_Disconnect(struct usb_interface *intf);
 static void usbvideo_CameraRelease(struct uvd *uvd);
 
-static int usbvideo_v4l_ioctl(struct inode *inode, struct file *file,
-			      unsigned int cmd, unsigned long arg);
+static long usbvideo_v4l_ioctl(struct file *file, unsigned int cmd,
+							unsigned long arg);
 static int usbvideo_v4l_mmap(struct file *file, struct vm_area_struct *vma);
 static int usbvideo_v4l_open(struct inode *inode, struct file *file);
 static ssize_t usbvideo_v4l_read(struct file *file, char __user *buf,

@@ -945,7 +945,7 @@ static const struct file_operations usbvideo_fops = {
 	.release =usbvideo_v4l_close,
 	.read =   usbvideo_v4l_read,
 	.mmap =   usbvideo_v4l_mmap,
-	.ioctl =  usbvideo_v4l_ioctl,
+	.unlocked_ioctl =  usbvideo_v4l_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl = v4l_compat_ioctl32,
 #endif

@@ -1477,10 +1477,10 @@ static int usbvideo_v4l_do_ioctl(struct inode *inode, struct file *file,
 	return 0;
 }
 
-static int usbvideo_v4l_ioctl(struct inode *inode, struct file *file,
-		       unsigned int cmd, unsigned long arg)
+static long usbvideo_v4l_ioctl(struct file *file, unsigned int cmd,
+							unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, usbvideo_v4l_do_ioctl);
+	return video_usercopy(file, cmd, arg, usbvideo_v4l_do_ioctl);
 }
 
 /*

drivers/media/video/usbvideo/vicam.c +9 -7

index 17f542d..e7cd160 100644
@@ -504,8 +504,8 @@ set_camera_power(struct vicam_camera *cam, int state)
 	return 0;
 }
 
-static int
-vicam_ioctl(struct inode *inode, struct file *file, unsigned int ioctlnr, unsigned long arg)
+static long
+vicam_ioctl(struct file *file, unsigned int ioctlnr, unsigned long arg)
 {
 	void __user *user_arg = (void __user *)arg;
 	struct vicam_camera *cam = file->private_data;

@@ -600,10 +600,11 @@ vicam_ioctl(struct inode *inode, struct file *file, unsigned int ioctlnr, unsign
 				break;
 			}
 
+			lock_kernel();
 			DBG("VIDIOCSPICT depth = %d, pal = %d\n", vp.depth,
 			    vp.palette);
-
 			cam->gain = vp.brightness >> 8;
+			unlock_kernel();
 
 			if (vp.depth != 24
 			    || vp.palette != VIDEO_PALETTE_RGB24)

@@ -648,7 +649,7 @@ vicam_ioctl(struct inode *inode, struct file *file, unsigned int ioctlnr, unsign
 			DBG("VIDIOCSWIN %d x %d\n", vw.width, vw.height);
 
 			if ( vw.width != 320 || vw.height != 240 )
-				retval = -EFAULT;
+				retval = -EINVAL;
 
 			break;
 		}

@@ -706,11 +707,12 @@ vicam_ioctl(struct inode *inode, struct file *file, unsigned int ioctlnr, unsign
 				break;
 			}
 			DBG("VIDIOCSYNC: %d\n", frame);
-
+			lock_kernel();
 			read_frame(cam, frame);
 			vicam_decode_color(cam->raw_image,
 					   cam->framebuf +
 					   frame * VICAM_MAX_FRAME_SIZE );
+			unlock_kernel();
 
 			break;
 		}

@@ -737,7 +739,7 @@ vicam_ioctl(struct inode *inode, struct file *file, unsigned int ioctlnr, unsign
 		retval = -EINVAL;
 		break;
 	default:
-		retval = -ENOIOCTLCMD;
+		retval = -ENOTTY;
 		break;
 	}

@@ -1059,7 +1061,7 @@ static const struct file_operations vicam_fops = {
 	.release	= vicam_close,
 	.read		= vicam_read,
 	.mmap		= vicam_mmap,
-	.ioctl		= vicam_ioctl,
+	.unlocked_ioctl	= vicam_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= v4l_compat_ioctl32,
 #endif

drivers/media/video/usbvision/usbvision-video.c +6 -6

index d97261a..3a7453f 100644
@@ -1376,10 +1376,10 @@ static int usbvision_do_vbi_ioctl(struct inode *inode, struct file *file,
 	return -ENOIOCTLCMD;
 }
 
-static int usbvision_vbi_ioctl(struct inode *inode, struct file *file,
-		       unsigned int cmd, unsigned long arg)
+static long usbvision_vbi_ioctl(struct file *file, unsigned int cmd,
+							unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, usbvision_do_vbi_ioctl);
+	return video_usercopy(file, cmd, arg, usbvision_do_vbi_ioctl);
 }

@@ -1394,7 +1394,7 @@ static const struct file_operations usbvision_fops = {
 	.release	= usbvision_v4l2_close,
 	.read		= usbvision_v4l2_read,
 	.mmap		= usbvision_v4l2_mmap,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 	.llseek		= no_llseek,
 /* 	.poll          = video_poll, */
 	.compat_ioctl  = v4l_compat_ioctl32,

@@ -1447,7 +1447,7 @@ static const struct file_operations usbvision_radio_fops = {
 	.owner             = THIS_MODULE,
 	.open		= usbvision_radio_open,
 	.release	= usbvision_radio_close,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 	.llseek		= no_llseek,
 	.compat_ioctl  = v4l_compat_ioctl32,
 };

@@ -1483,7 +1483,7 @@ static const struct file_operations usbvision_vbi_fops = {
 	.owner             = THIS_MODULE,
 	.open		= usbvision_vbi_open,
 	.release	= usbvision_vbi_close,
-	.ioctl		= usbvision_vbi_ioctl,
+	.unlocked_ioctl	= usbvision_vbi_ioctl,
 	.llseek		= no_llseek,
 	.compat_ioctl  = v4l_compat_ioctl32,
 };

drivers/media/video/videodev.c +12 -7

index 31e8af0..520aafc 100644
@@ -36,7 +36,8 @@
 #include <linux/init.h>
 #include <linux/kmod.h>
 #include <linux/slab.h>
-#include <asm/uaccess.h>
+#include <linux/smp_lock.h>
+#include <linux/uaccess.h>
 #include <asm/system.h>
 
 #define __OLD_VIDIOC_ /* To allow fixing old calls*/

@@ -556,8 +557,7 @@ video_fix_command(unsigned int cmd)
 /*
  * Obsolete usercopy function - Should be removed soon
  */
-int
-video_usercopy(struct inode *inode, struct file *file,
+long video_usercopy(struct file *file,
 	       unsigned int cmd, unsigned long arg,
 	       int (*func)(struct inode *inode, struct file *file,
 			   unsigned int cmd, void *arg))

@@ -565,10 +565,11 @@ video_usercopy(struct inode *inode, struct file *file,
 	char	sbuf[128];
 	void    *mbuf = NULL;
 	void	*parg = NULL;
-	int	err  = -EINVAL;
+	long	err  = -EINVAL;
 	int     is_ext_ctrl;
 	size_t  ctrls_size = 0;
 	void __user *user_ptr = NULL;
+	struct inode *inode = file->f_path.dentry->d_inode;
 
 #ifdef __OLD_VIDIOC_
 	cmd = video_fix_command(cmd);

@@ -621,8 +622,10 @@ video_usercopy(struct inode *inode, struct file *file,
 		}
 	}
 
-	/* call driver */
+	/* call driver old style method */
+	lock_kernel();
 	err = func(inode, file, cmd, parg);
+	unlock_kernel();
 	if (err == -ENOIOCTLCMD)
 		err = -EINVAL;
 	if (is_ext_ctrl) {

@@ -1881,8 +1884,7 @@ static int __video_do_ioctl(struct inode *inode, struct file *file,
 	return ret;
 }
 
-int video_ioctl2 (struct inode *inode, struct file *file,
-	       unsigned int cmd, unsigned long arg)
+long video_ioctl2 (struct file *file, unsigned int cmd, unsigned long arg)
 {
 	char	sbuf[128];
 	void    *mbuf = NULL;

@@ -1891,6 +1893,7 @@ int video_ioctl2 (struct inode *inode, struct file *file,
 	int     is_ext_ctrl;
 	size_t  ctrls_size = 0;
 	void __user *user_ptr = NULL;
+	struct inode *inode = file->f_path.dentry->d_inode;
 
 #ifdef __OLD_VIDIOC_
 	cmd = video_fix_command(cmd);

@@ -1945,7 +1948,9 @@ int video_ioctl2 (struct inode *inode, struct file *file,
 	}
 
 	/* Handles IOCTL */
+	lock_kernel();
 	err = __video_do_ioctl(inode, file, cmd, parg);
+	unlock_kernel();
 	if (err == -ENOIOCTLCMD)
 		err = -EINVAL;
 	if (is_ext_ctrl) {

drivers/media/video/vivi.c +1 -1

index 845be18..1a7cd3a 100644
@@ -1051,7 +1051,7 @@ static const struct file_operations vivi_fops = {
 	.release        = vivi_close,
 	.read           = vivi_read,
 	.poll		= vivi_poll,
-	.ioctl          = video_ioctl2, /* V4L2 ioctl handler */
+	.unlocked_ioctl = video_ioctl2, /* V4L2 ioctl handler */
 	.compat_ioctl   = v4l_compat_ioctl32,
 	.mmap           = vivi_mmap,
 	.llseek         = no_llseek,

drivers/media/video/w9966.c +6 -6

index 33f7026..7ea1b64 100644
@@ -178,8 +178,8 @@ static int w9966_i2c_wbyte(struct w9966_dev* cam, int data);
 static int w9966_i2c_rbyte(struct w9966_dev* cam);
 #endif
 
-static int w9966_v4l_ioctl(struct inode *inode, struct file *file,
-			   unsigned int cmd, unsigned long arg);
+static long w9966_v4l_ioctl(struct file *file, unsigned int cmd,
+						unsigned long arg);
 static ssize_t w9966_v4l_read(struct file *file, char __user *buf,
 			      size_t count, loff_t *ppos);

@@ -187,7 +187,7 @@ static const struct file_operations w9966_fops = {
 	.owner		= THIS_MODULE,
 	.open           = video_exclusive_open,
 	.release        = video_exclusive_release,
-	.ioctl          = w9966_v4l_ioctl,
+	.unlocked_ioctl = w9966_v4l_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= v4l_compat_ioctl32,
 #endif

@@ -863,10 +863,10 @@ static int w9966_v4l_do_ioctl(struct inode *inode, struct file *file,
 	return 0;
 }
 
-static int w9966_v4l_ioctl(struct inode *inode, struct file *file,
-			   unsigned int cmd, unsigned long arg)
+static long w9966_v4l_ioctl(struct file *file, unsigned int cmd,
+							unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, w9966_v4l_do_ioctl);
+	return video_usercopy(file, cmd, arg, w9966_v4l_do_ioctl);
 }
 
 // Capture data

drivers/media/video/w9968cf.c +10 ~~-12~~

index 8405224..1a711a3 100644
@@ -402,10 +402,9 @@ static const struct file_operations w9968cf_fops;
 static int w9968cf_open(struct inode*, struct file*);
 static int w9968cf_release(struct inode*, struct file*);
 static int w9968cf_mmap(struct file*, struct vm_area_struct*);
-static int w9968cf_ioctl(struct inode*, struct file*, unsigned, unsigned long);
+static long w9968cf_ioctl(struct file*, unsigned, unsigned long);
 static ssize_t w9968cf_read(struct file*, char __user *, size_t, loff_t*);
-static int w9968cf_v4l_ioctl(struct inode*, struct file*, unsigned int,
-			     void __user *);
+static long w9968cf_v4l_ioctl(struct file*, unsigned int, void __user *);
 
 /* USB-specific */
 static int w9968cf_start_transfer(struct w9968cf_device*);

@@ -2884,9 +2883,8 @@ static int w9968cf_mmap(struct file* filp, struct vm_area_struct *vma)
 }
 
 
-static int
-w9968cf_ioctl(struct inode* inode, struct file* filp,
-	      unsigned int cmd, unsigned long arg)
+static long w9968cf_ioctl(struct file * filp, unsigned int cmd,
+						unsigned long arg)
 {
 	struct w9968cf_device* cam;
 	int err;

@@ -2907,16 +2905,16 @@ w9968cf_ioctl(struct inode* inode, struct file* filp,
 		mutex_unlock(&cam->fileop_mutex);
 		return -EIO;
 	}
-
-	err = w9968cf_v4l_ioctl(inode, filp, cmd, (void __user *)arg);
-
+	lock_kernel();
+	err = w9968cf_v4l_ioctl(filp, cmd, (void __user *)arg);
+	unlock_kernel();
 	mutex_unlock(&cam->fileop_mutex);
 	return err;
 }
 
 
-static int w9968cf_v4l_ioctl(struct inode* inode, struct file* filp,
-			     unsigned int cmd, void __user * arg)
+static long  w9968cf_v4l_ioctl(struct file* filp, unsigned int cmd,
+							void __user * arg)
 {
 	struct w9968cf_device* cam;
 	const char* v4l1_ioctls[] = {

@@ -3460,7 +3458,7 @@ static const struct file_operations w9968cf_fops = {
 	.open =    w9968cf_open,
 	.release = w9968cf_release,
 	.read =    w9968cf_read,
-	.ioctl =   w9968cf_ioctl,
+	.unlocked_ioctl =   w9968cf_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl = v4l_compat_ioctl32,
 #endif

drivers/media/video/zc0301/zc0301_core.c +8 -6

index 363dd2b..759db81 100644
@@ -1793,8 +1793,8 @@ zc0301_vidioc_s_parm(struct zc0301_device* cam, void __user * arg)
 }
 
 
-static int zc0301_ioctl_v4l2(struct inode* inode, struct file* filp,
-			     unsigned int cmd, void __user * arg)
+static long zc0301_ioctl_v4l2(struct file* filp, unsigned int cmd,
+							void __user * arg)
 {
 	struct zc0301_device* cam = video_get_drvdata(video_devdata(filp));

@@ -1888,8 +1888,8 @@ static int zc0301_ioctl_v4l2(struct inode* inode, struct file* filp,
 }
 
 
-static int zc0301_ioctl(struct inode* inode, struct file* filp,
-			unsigned int cmd, unsigned long arg)
+static long zc0301_ioctl(struct file* filp, unsigned int cmd,
+						unsigned long arg)
 {
 	struct zc0301_device* cam = video_get_drvdata(video_devdata(filp));
 	int err = 0;

@@ -1912,7 +1912,9 @@ static int zc0301_ioctl(struct inode* inode, struct file* filp,
 
 	V4LDBG(3, "zc0301", cmd);
 
-	err = zc0301_ioctl_v4l2(inode, filp, cmd, (void __user *)arg);
+	lock_kernel();
+	err = zc0301_ioctl_v4l2(filp, cmd, (void __user *)arg);
+	unlock_kernel();
 
 	mutex_unlock(&cam->fileop_mutex);

@@ -1924,7 +1926,7 @@ static const struct file_operations zc0301_fops = {
 	.owner =   THIS_MODULE,
 	.open =    zc0301_open,
 	.release = zc0301_release,
-	.ioctl =   zc0301_ioctl,
+	.unlocked_ioctl = zc0301_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl = v4l_compat_ioctl32,
 #endif

drivers/media/video/zoran_driver.c +4 -5

index 0134bec..28fe8e3 100644
@@ -4198,13 +4198,12 @@ zoran_do_ioctl (struct inode *inode,
 }
 
 
-static int
-zoran_ioctl (struct inode *inode,
-	     struct file  *file,
+static long
+zoran_ioctl (struct file  *file,
 	     unsigned int  cmd,
 	     unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, zoran_do_ioctl);
+	return video_usercopy(file, cmd, arg, zoran_do_ioctl);
 }
 
 static unsigned int

@@ -4631,7 +4630,7 @@ static const struct file_operations zoran_fops = {
 	.owner = THIS_MODULE,
 	.open = zoran_open,
 	.release = zoran_close,
-	.ioctl = zoran_ioctl,
+	.unlocked_ioctl = zoran_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= v4l_compat_ioctl32,
 #endif

drivers/media/video/zr364xx.c +1 -1

index a0e49dc..15780fa 100644
@@ -757,7 +757,7 @@ static const struct file_operations zr364xx_fops = {
 	.release = zr364xx_release,
 	.read = zr364xx_read,
 	.mmap = zr364xx_mmap,
-	.ioctl = video_ioctl2,
+	.unlocked_ioctl = video_ioctl2,
 	.llseek = no_llseek,
 };

drivers/net/tun.c +22 -6

index 0ce07a3..0a6e4f0 100644
@@ -596,8 +596,8 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 	return err;
 }
 
-static int tun_chr_ioctl(struct inode *inode, struct file *file,
-			 unsigned int cmd, unsigned long arg)
+static long tun_chr_ioctl(struct file *file, unsigned int cmd,
+							unsigned long arg)
 {
 	struct tun_struct *tun = file->private_data;
 	void __user* argp = (void __user*)arg;

@@ -633,36 +633,42 @@ static int tun_chr_ioctl(struct inode *inode, struct file *file,
 	switch (cmd) {
 	case TUNSETNOCSUM:
 		/* Disable/Enable checksum */
+		lock_kernel();
 		if (arg)
 			tun->flags |= TUN_NOCHECKSUM;
 		else
 			tun->flags &= ~TUN_NOCHECKSUM;
-
+		unlock_kernel();
 		DBG(KERN_INFO "%s: checksum %s\n",
 		    tun->dev->name, arg ? "disabled" : "enabled");
 		break;
 
 	case TUNSETPERSIST:
 		/* Disable/Enable persist mode */
+		lock_kernel();
 		if (arg)
 			tun->flags |= TUN_PERSIST;
 		else
 			tun->flags &= ~TUN_PERSIST;
-
+		unlock_kernel();
 		DBG(KERN_INFO "%s: persist %s\n",
 		    tun->dev->name, arg ? "enabled" : "disabled");
 		break;
 
 	case TUNSETOWNER:
 		/* Set owner of the device */
+		lock_kernel();
 		tun->owner = (uid_t) arg;
+		unlock_kernel();
 
 		DBG(KERN_INFO "%s: owner set to %d\n", tun->dev->name, tun->owner);
 		break;
 
 	case TUNSETGROUP:
 		/* Set group of the device */
+		lock_kernel();
 		tun->group= (gid_t) arg;
+		unlock_kernel();
 
 		DBG(KERN_INFO "%s: group set to %d\n", tun->dev->name, tun->group);
 		break;

@@ -688,12 +694,16 @@ static int tun_chr_ioctl(struct inode *inode, struct file *file,
 
 #ifdef TUN_DEBUG
 	case TUNSETDEBUG:
+		lock_kernel();
 		tun->debug = arg;
+		unlock_kernel();
 		break;
 #endif
 
 	case SIOCGIFFLAGS:
+		lock_kernel();
 		ifr.ifr_flags = tun->if_flags;
+		unlock_kernel();
 		if (copy_to_user( argp, &ifr, sizeof ifr))
 			return -EFAULT;
 		return 0;

@@ -701,15 +711,19 @@ static int tun_chr_ioctl(struct inode *inode, struct file *file,
 	case SIOCSIFFLAGS:
 		/** Set the character device's interface flags. Currently only
 		 * IFF_PROMISC and IFF_ALLMULTI are used. */
+		lock_kernel();
 		tun->if_flags = ifr.ifr_flags;
 		DBG(KERN_INFO "%s: interface flags 0x%lx\n",
 				tun->dev->name, tun->if_flags);
+		unlock_kernel();
 		return 0;
 
 	case SIOCGIFHWADDR:
 		/* Note: the actual net device's address may be different */
+		lock_kernel();
 		memcpy(ifr.ifr_hwaddr.sa_data, tun->dev_addr,
 				min(sizeof ifr.ifr_hwaddr.sa_data, sizeof tun->dev_addr));
+		unlock_kernel();
 		if (copy_to_user( argp, &ifr, sizeof ifr))
 			return -EFAULT;
 		return 0;

@@ -719,6 +733,7 @@ static int tun_chr_ioctl(struct inode *inode, struct file *file,
 		/* try to set the actual net device's hw address */
 		int ret;
 
+		lock_kernel();
 		rtnl_lock();
 		ret = dev_set_mac_address(tun->dev, &ifr.ifr_hwaddr);
 		rtnl_unlock();

@@ -734,6 +749,7 @@ static int tun_chr_ioctl(struct inode *inode, struct file *file,
 					tun->dev_addr[0], tun->dev_addr[1], tun->dev_addr[2],
 					tun->dev_addr[3], tun->dev_addr[4], tun->dev_addr[5]);
 		}
+		unlock_kernel();
 
 		return  ret;
 	}

@@ -765,7 +781,7 @@ static int tun_chr_ioctl(struct inode *inode, struct file *file,
 		return 0;
 
 	default:
-		return -EINVAL;
+		return -ENOTTY;
 	};
 
 	return 0;

@@ -841,7 +857,7 @@ static const struct file_operations tun_fops = {
 	.write = do_sync_write,
 	.aio_write = tun_chr_aio_write,
 	.poll	= tun_chr_poll,
-	.ioctl	= tun_chr_ioctl,
+	.unlocked_ioctl	= tun_chr_ioctl,
 	.open	= tun_chr_open,
 	.release = tun_chr_close,
 	.fasync = tun_chr_fasync

include/media/v4l2-dev.h +4 -4

index a807d2f..a08773b 100644
@@ -344,8 +344,8 @@ void *priv;
 /* Version 2 functions */
 extern int video_register_device(struct video_device *vfd, int type, int nr);
 void video_unregister_device(struct video_device *);
-extern int video_ioctl2(struct inode *inode, struct file *file,
-			  unsigned int cmd, unsigned long arg);
+extern long video_ioctl2(struct file *file, unsigned int cmd,
+							unsigned long arg);
 
 /* helper functions to alloc / release struct video_device, the
    later can be used for video_device->release() */

@@ -353,8 +353,8 @@ struct video_device *video_device_alloc(void);
 void video_device_release(struct video_device *vfd);
 
 /* Include support for obsoleted stuff */
-extern int video_usercopy(struct inode *inode, struct file *file,
-			  unsigned int cmd, unsigned long arg,
+extern long video_usercopy(struct file *file, unsigned int cmd,
+			  unsigned long arg,
 			  int (*func)(struct inode *inode, struct file *file,
 				      unsigned int cmd, void *arg));

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Andy Walls

2008-05-23 02:09 UTC #1 lore raw

Quoted 5 lines · Alan Cox

For most drivers the generic ioctl handler does the work and we update it and it becomes the unlocked_ioctl method. Older drivers use the usercopy method so we make it do the work. Finally there are a few special cases.
Signed-off-by Alan Cox <[email protected]>

I'd like to start planning out the changes to eliminate the BKL from cx18.

Could someone give me a brief education as to what elements of cx18/ivtv_v4l2_do_ioctl() would be forcing the use of the BKL for these
drivers' ioctls? I'm assuming it's not the
mutex_un/lock(&....->serialize_lock) and that the answer's not in the diff.

Thanks.
Andy

Quoted 122 lines

drivers/media/video/cx18/cx18-ioctl.c +4 -3

index dbdcb86..faf3a31 100644
@@ -837,15 +837,16 @@ static int cx18_v4l2_do_ioctl(struct inode *inode, struct file *filp,
 	return 0;
 }
 
-int cx18_v4l2_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
-		    unsigned long arg)
+long cx18_v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	struct cx18_open_id *id = (struct cx18_open_id *)filp->private_data;
 	struct cx18 *cx = id->cx;
 	int res;
 
+	lock_kernel();
 	mutex_lock(&cx->serialize_lock);
-	res = video_usercopy(inode, filp, cmd, arg, cx18_v4l2_do_ioctl);
+	res = video_usercopy(filp, cmd, arg, cx18_v4l2_do_ioctl);
 	mutex_unlock(&cx->serialize_lock);
+	unlock_kernel();
 	return res;
 }

drivers/media/video/cx18/cx18-ioctl.h +2 -3

index 9f4c7eb..32bede3 100644
@@ -1,4 +1,4 @@
-/*
+ /*
  *  cx18 ioctl system call
  *
  *  Derived from ivtv-ioctl.h

@@ -24,7 +24,6 @@
 u16 cx18_service2vbi(int type);
 void cx18_expand_service_set(struct v4l2_sliced_vbi_format *fmt, int is_pal);
 u16 cx18_get_service_set(struct v4l2_sliced_vbi_format *fmt);
-int cx18_v4l2_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
-		    unsigned long arg);
+long cx18_v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
 int cx18_v4l2_ioctls(struct cx18 *cx, struct file *filp, unsigned cmd,
 		     void *arg);

drivers/media/video/cx18/cx18-streams.c +1 -1

index afb141b..7348b82 100644
@@ -39,7 +39,7 @@ static struct file_operations cx18_v4l2_enc_fops = {
       .owner = THIS_MODULE,
       .read = cx18_v4l2_read,
       .open = cx18_v4l2_open,
-      .ioctl = cx18_v4l2_ioctl,
+      .unlocked_ioctl = cx18_v4l2_ioctl,
       .release = cx18_v4l2_close,
       .poll = cx18_v4l2_enc_poll,
 };

drivers/media/video/ivtv/ivtv-ioctl.c +6 -5

index d508b5d..a481b2d 100644
@@ -1726,7 +1726,7 @@ static int ivtv_v4l2_do_ioctl(struct inode *inode, struct file *filp,
 	return 0;
 }
 
-static int ivtv_serialized_ioctl(struct ivtv *itv, struct inode *inode, struct file *filp,
+static int ivtv_serialized_ioctl(struct ivtv *itv, struct file *filp,
 		unsigned int cmd, unsigned long arg)
 {
 	/* Filter dvb ioctls that cannot be handled by video_usercopy */

@@ -1761,18 +1761,19 @@ static int ivtv_serialized_ioctl(struct ivtv *itv, struct inode *inode, struct f
 	default:
 		break;
 	}
-	return video_usercopy(inode, filp, cmd, arg, ivtv_v4l2_do_ioctl);
+	return video_usercopy(filp, cmd, arg, ivtv_v4l2_do_ioctl);
 }
 
-int ivtv_v4l2_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
-		    unsigned long arg)
+long ivtv_v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	struct ivtv_open_id *id = (struct ivtv_open_id *)filp->private_data;
 	struct ivtv *itv = id->itv;
 	int res;
 
+	lock_kernel();
 	mutex_lock(&itv->serialize_lock);
-	res = ivtv_serialized_ioctl(itv, inode, filp, cmd, arg);
+	res = ivtv_serialized_ioctl(itv, filp, cmd, arg);
 	mutex_unlock(&itv->serialize_lock);
+	unlock_kernel();
 	return res;
 }

drivers/media/video/ivtv/ivtv-ioctl.h +1 -2

index a03351b..6708ea0 100644
@@ -24,8 +24,7 @@
 u16 service2vbi(int type);
 void expand_service_set(struct v4l2_sliced_vbi_format *fmt, int is_pal);
 u16 get_service_set(struct v4l2_sliced_vbi_format *fmt);
-int ivtv_v4l2_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
-		    unsigned long arg);
+long ivtv_v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
 int ivtv_v4l2_ioctls(struct ivtv *itv, struct file *filp, unsigned int cmd, void *arg);
 void ivtv_set_osd_alpha(struct ivtv *itv);
 int ivtv_set_speed(struct ivtv *itv, int speed);

drivers/media/video/ivtv/ivtv-streams.c +1 -1

index 4ab8d36..3131f68 100644
@@ -48,7 +48,7 @@ static const struct file_operations ivtv_v4l2_enc_fops = {
       .read = ivtv_v4l2_read,
       .write = ivtv_v4l2_write,
       .open = ivtv_v4l2_open,
-      .ioctl = ivtv_v4l2_ioctl,
+      .unlocked_ioctl = ivtv_v4l2_ioctl,
       .release = ivtv_v4l2_close,
       .poll = ivtv_v4l2_enc_poll,
 };

drivers/media/video/ivtv/ivtv-streams.c static const struct file_operations ivtv_v4l2_dec_fops = { · line 64

Quoted 8 lines

drivers/media/video/ivtv/ivtv-streams.c +1 -1

@@ -58,7 +58,7 @@ static const struct file_operations ivtv_v4l2_dec_fops = {
       .read = ivtv_v4l2_read,
       .write = ivtv_v4l2_write,
       .open = ivtv_v4l2_open,
-      .ioctl = ivtv_v4l2_ioctl,
+      .unlocked_ioctl = ivtv_v4l2_ioctl,
       .release = ivtv_v4l2_close,
       .poll = ivtv_v4l2_dec_poll,
 };

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Hans Verkuil

2008-05-23 06:16 UTC #2 lore raw

Quoted 8 lines · On Friday 23 May 2008 04:08:04 Andy Walls wrote:

Quoted 6 lines · Alan Cox
For most drivers the generic ioctl handler does the work and we update it and it becomes the unlocked_ioctl method. Older drivers use the usercopy method so we make it do the work. Finally there are a few special cases.
Signed-off-by Alan Cox <[email protected]>

Quoted 8 lines

I'd like to start planning out the changes to eliminate the BKL from cx18.
Could someone give me a brief education as to what elements of cx18/ivtv_v4l2_do_ioctl() would be forcing the use of the BKL for
these drivers' ioctls? I'm assuming it's not the
mutex_un/lock(&....->serialize_lock) and that the answer's not in the diff.

To the best of my knowledge there is no need for a BKL in ivtv or cx18. It was just laziness on my part that I hadn't switched to unlocked_ioctl yet. If you know of a reason why it should be kept for now, then I'd like to know, otherwise the BKL can be removed altogether for ivtv/cx18. I suspect you just pushed the lock down into the driver and in that case you can just remove it for ivtv/cx18.

Regards,

Hans

Quoted 139 lines

Thanks.
Andy
Quoted 135 lines
(quoted diff) +0 -0
b/drivers/media/video/cx18/cx18-ioctl.c index dbdcb86..faf3a31 100644
drivers/media/video/cx18/cx18-ioctl.c +4 -2
@@ -837,15 +837,16 @@ static int cx18_v4l2_do_ioctl(struct inode
 *inode, struct file *filp, return 0;
 }
 
-int cx18_v4l2_ioctl(struct inode *inode, struct file *filp,
 unsigned int cmd, -		    unsigned long arg)
+long cx18_v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned
 long arg) {
 	struct cx18_open_id *id = (struct cx18_open_id
 *)filp->private_data; struct cx18 *cx = id->cx;
 	int res;
 
+	lock_kernel();
 	mutex_lock(&cx->serialize_lock);
-	res = video_usercopy(inode, filp, cmd, arg, cx18_v4l2_do_ioctl);
+	res = video_usercopy(filp, cmd, arg, cx18_v4l2_do_ioctl);
 	mutex_unlock(&cx->serialize_lock);
+	unlock_kernel();
 	return res;
 }
(quoted diff) +0 -0
b/drivers/media/video/cx18/cx18-ioctl.h index 9f4c7eb..32bede3 100644
drivers/media/video/cx18/cx18-ioctl.h +1 -1
@@ -1,4 +1,4 @@
-/*
+ /*
  *  cx18 ioctl system call
  *
  *  Derived from ivtv-ioctl.h
@@ -24,7 +24,6 @@
 u16 cx18_service2vbi(int type);
 void cx18_expand_service_set(struct v4l2_sliced_vbi_format *fmt,
int is_pal); u16 cx18_get_service_set(struct v4l2_sliced_vbi_format *fmt); -int cx18_v4l2_ioctl(struct inode *inode, struct file *filp,
unsigned int cmd, - unsigned long arg);
+long cx18_v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg); int cx18_v4l2_ioctls(struct cx18 *cx, struct file *filp, unsigned cmd, void *arg);
(quoted diff) +0 -0
b/drivers/media/video/cx18/cx18-streams.c index afb141b..7348b82 100644
drivers/media/video/cx18/cx18-streams.c +1 -1
@@ -39,7 +39,7 @@ static struct file_operations cx18_v4l2_enc_fops
 = { .owner = THIS_MODULE,
       .read = cx18_v4l2_read,
       .open = cx18_v4l2_open,
-      .ioctl = cx18_v4l2_ioctl,
+      .unlocked_ioctl = cx18_v4l2_ioctl,
       .release = cx18_v4l2_close,
       .poll = cx18_v4l2_enc_poll,
 };
(quoted diff) +0 -0
b/drivers/media/video/ivtv/ivtv-ioctl.c index d508b5d..a481b2d 100644
drivers/media/video/ivtv/ivtv-ioctl.c +0 -1
@@ -1726,7 +1726,7 @@ static int ivtv_v4l2_do_ioctl(struct inode
 *inode, struct file *filp, return 0;
 }
 
-static int ivtv_serialized_ioctl(struct ivtv *itv, struct inode
 *inode, struct file *filp, +static int ivtv_serialized_ioctl(struct
 ivtv *itv, struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	/* Filter dvb ioctls that cannot be handled by video_usercopy */
@@ -1761,18 +1761,19 @@ static int ivtv_serialized_ioctl(struct
ivtv *itv, struct inode *inode, struct f default:
drivers/media/video/ivtv/ivtv-ioctl.c +1 -2
 		break;
 	}
-	return video_usercopy(inode, filp, cmd, arg, ivtv_v4l2_do_ioctl);
+	return video_usercopy(filp, cmd, arg, ivtv_v4l2_do_ioctl);
 }
 
-int ivtv_v4l2_ioctl(struct inode *inode, struct file *filp,
unsigned int cmd, - unsigned long arg)
+long ivtv_v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) {
struct ivtv_open_id *id = (struct ivtv_open_id
*)filp->private_data; struct ivtv *itv = id->itv;
drivers/media/video/ivtv/ivtv-ioctl.c +3 -1
 	int res;
 
+	lock_kernel();
 	mutex_lock(&itv->serialize_lock);
-	res = ivtv_serialized_ioctl(itv, inode, filp, cmd, arg);
+	res = ivtv_serialized_ioctl(itv, filp, cmd, arg);
 	mutex_unlock(&itv->serialize_lock);
+	unlock_kernel();
 	return res;
 }
(quoted diff) +0 -0
b/drivers/media/video/ivtv/ivtv-ioctl.h index a03351b..6708ea0 100644
drivers/media/video/ivtv/ivtv-ioctl.h +1 -1
@@ -24,8 +24,7 @@
 u16 service2vbi(int type);
 void expand_service_set(struct v4l2_sliced_vbi_format *fmt, int
 is_pal); u16 get_service_set(struct v4l2_sliced_vbi_format *fmt);
-int ivtv_v4l2_ioctl(struct inode *inode, struct file *filp,
 unsigned int cmd, -		    unsigned long arg);
+long ivtv_v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned
long arg); int ivtv_v4l2_ioctls(struct ivtv *itv, struct file *filp, unsigned int cmd, void *arg); void ivtv_set_osd_alpha(struct ivtv *itv);
int ivtv_set_speed(struct ivtv *itv, int speed);
(quoted diff) +0 -0
b/drivers/media/video/ivtv/ivtv-streams.c index 4ab8d36..3131f68 100644
drivers/media/video/ivtv/ivtv-streams.c +2 -2
@@ -48,7 +48,7 @@ static const struct file_operations
 ivtv_v4l2_enc_fops = { .read = ivtv_v4l2_read,
       .write = ivtv_v4l2_write,
       .open = ivtv_v4l2_open,
-      .ioctl = ivtv_v4l2_ioctl,
+      .unlocked_ioctl = ivtv_v4l2_ioctl,
       .release = ivtv_v4l2_close,
       .poll = ivtv_v4l2_enc_poll,
 };
@@ -58,7 +58,7 @@ static const struct file_operations
 ivtv_v4l2_dec_fops = { .read = ivtv_v4l2_read,
       .write = ivtv_v4l2_write,
       .open = ivtv_v4l2_open,
-      .ioctl = ivtv_v4l2_ioctl,
+      .unlocked_ioctl = ivtv_v4l2_ioctl,
       .release = ivtv_v4l2_close,

Quoted 8 lines

drivers/media/video/ivtv/ivtv-streams.c static const struct file_operations ivtv_v4l2_dec_fops = { · line 64
Quoted 2 lines
drivers/media/video/ivtv/ivtv-streams.c +0 -0
       .poll = ivtv_v4l2_dec_poll,
 };
--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Hans Verkuil

2008-05-23 06:29 UTC #3 lore raw

Quoted 18 lines · On Friday 23 May 2008 08:16:05 Hans Verkuil wrote:

Quoted 8 lines · On Friday 23 May 2008 04:08:04 Andy Walls wrote:
Quoted 6 lines · Alan Cox
For most drivers the generic ioctl handler does the work and we update it and it becomes the unlocked_ioctl method. Older drivers use the usercopy method so we make it do the work. Finally there are a few special cases.
Signed-off-by Alan Cox <[email protected]>
Quoted 8 lines
I'd like to start planning out the changes to eliminate the BKL from cx18.
Could someone give me a brief education as to what elements of cx18/ivtv_v4l2_do_ioctl() would be forcing the use of the BKL for
these drivers' ioctls? I'm assuming it's not the
mutex_un/lock(&....->serialize_lock) and that the answer's not in the diff.

Quoted 7 lines

To the best of my knowledge there is no need for a BKL in ivtv or cx18. It was just laziness on my part that I hadn't switched to unlocked_ioctl yet. If you know of a reason why it should be kept for now, then I'd like to know, otherwise the BKL can be removed altogether for ivtv/cx18. I suspect you just pushed the lock down into the driver and in that case you can just remove it for ivtv/cx18.

Hmm, of course you just pushed down the lock, the subject said so. Sorry, it's early morning here and I'm apparently not yet fully awake :-)

Regards,

Hans

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Mauro Carvalho Chehab

2008-05-26 16:39 UTC #4 lore raw

In reply to Hans Verkuil

Quoted 18 lines · On Fri, 23 May 2008 08:16:05 +0200 Hans Verkuil <[email protected]> wrote:

Quoted 8 lines · On Friday 23 May 2008 04:08:04 Andy Walls wrote:
Quoted 6 lines · Alan Cox
For most drivers the generic ioctl handler does the work and we update it and it becomes the unlocked_ioctl method. Older drivers use the usercopy method so we make it do the work. Finally there are a few special cases.
Signed-off-by Alan Cox <[email protected]>
Quoted 8 lines
I'd like to start planning out the changes to eliminate the BKL from cx18.
Could someone give me a brief education as to what elements of cx18/ivtv_v4l2_do_ioctl() would be forcing the use of the BKL for
these drivers' ioctls? I'm assuming it's not the
mutex_un/lock(&....->serialize_lock) and that the answer's not in the diff.

Quoted 3 lines

To the best of my knowledge there is no need for a BKL in ivtv or cx18. It was just laziness on my part that I hadn't switched to unlocked_ioctl yet.

I suspect that this is true with almost all drivers. Yet, we may need to enable BKL or add other locks on a few places. For example, the old tuner i2c probing method relies on a global and unique static var to identify radio and tv tuners. I can foresee a race condition there, if you have two boards probing tuners at the same time.

Of course, we need to do several tests, since probably there are some issues that we cannot foresee.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Alan Cox

2008-05-23 09:09 UTC #5 lore raw

In reply to Andy Walls

Quoted 5 lines · Andy Walls

Could someone give me a brief education as to what elements of cx18/ivtv_v4l2_do_ioctl() would be forcing the use of the BKL for these
drivers' ioctls? I'm assuming it's not the
mutex_un/lock(&....->serialize_lock) and that the answer's not in the diff.

As it stood previous for historical reasons the kernel called the driver ioctl method already holding the big kernel lock. That lock effectively serialized a lot of ioctl processing and also serializes against module loading and registration/open for the most part. If all the resources you are working on within the ioctl handler are driver owned as is likely with a video capture driver, and you have sufficient locking of your own you can drop the lock.

video_usercopy currently also uses the BKL so you might want to copy a version to video_usercopy_unlocked() without that.

A small number of other things still depend on the BKL but probably don't affect a capture driver: Setting file flags and using the deprecated sleep_on and interruptible_sleep_on. Other things like memory allocators wait_event, wake_up and the like all do their own locking internally and that is a model we want to end up at - whether everything has "proper" locking.

You may also find a driver depends on other things (eg video on i2c and you can't yet prove the i2c (or don't know) is locked entirely privately in which cases I've ended up leaving the drivers I worked on with

	lock_kernel();
	ret = subsys_some_foo(bar);
	unlock_kernel();

which makes it clear what the remaining problem points are so they can be revisited.

For a lot of ioctl conversions the big issue is "two ioctls at once in parallel" which can happen even on a single opener device. The other tends be stuff like "ioctl versus unplug" although for PCI and many other subsystems the resources are ref-counted so a pci_dev won't vanish on you without warning.

Alan

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Mauro Carvalho Chehab

2008-05-26 16:34 UTC #6 lore raw

Quoted 15 lines · On Fri, 23 May 2008 05:09:19 -0400 Alan Cox <[email protected]> wrote:

Quoted 5 lines · Andy Walls
Could someone give me a brief education as to what elements of cx18/ivtv_v4l2_do_ioctl() would be forcing the use of the BKL for these
drivers' ioctls? I'm assuming it's not the
mutex_un/lock(&....->serialize_lock) and that the answer's not in the diff.
As it stood previous for historical reasons the kernel called the driver ioctl method already holding the big kernel lock. That lock effectively serialized a lot of ioctl processing and also serializes against module loading and registration/open for the most part. If all the resources you are working on within the ioctl handler are driver owned as is likely with a video capture driver, and you have sufficient locking of your own you can drop the lock.

Quoted 2 lines

video_usercopy currently also uses the BKL so you might want to copy a version to video_usercopy_unlocked() without that.

In the specific case of ivtv and cx18, I think that the better would be to convert it first to video_ioctl2. Then, remove the BKL, with a video_ioctl2_unlocked version.

Douglas already did an experimental patch converting ivtv to video_ioctl2 and sent to Hans. It needs testing, since he doesn't have any ivtv board. It should be trivial to port this to cx18, since both drivers have similar structures.

Douglas,

Could you send this patch to the ML for people to review and for Andy to port it to cx18?

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Hans Verkuil

2008-05-26 16:46 UTC #7 lore raw

Quoted 30 lines · On Monday 26 May 2008 18:34:57 Mauro Carvalho Chehab wrote:

On Fri, 23 May 2008 05:09:19 -0400
Quoted 15 lines · Alan Cox
Quoted 5 lines · Andy Walls
Could someone give me a brief education as to what elements of cx18/ivtv_v4l2_do_ioctl() would be forcing the use of the BKL for
these drivers' ioctls? I'm assuming it's not the
mutex_un/lock(&....->serialize_lock) and that the answer's not in the diff.
As it stood previous for historical reasons the kernel called the driver ioctl method already holding the big kernel lock. That lock effectively serialized a lot of ioctl processing and also serializes against module loading and registration/open for the most part. If all the resources you are working on within the ioctl handler are driver owned as is likely with a video capture driver, and you have sufficient locking of your own you can drop the lock.
Quoted 2 lines
video_usercopy currently also uses the BKL so you might want to copy a version to video_usercopy_unlocked() without that.
In the specific case of ivtv and cx18, I think that the better would be to convert it first to video_ioctl2. Then, remove the BKL, with a video_ioctl2_unlocked version.
Douglas already did an experimental patch converting ivtv to video_ioctl2 and sent to Hans. It needs testing, since he doesn't have any ivtv board. It should be trivial to port this to cx18, since both drivers have similar structures.

Quoted 4 lines

Douglas,
Could you send this patch to the ML for people to review and for Andy to port it to cx18?

Unless there is an objection, I would prefer to take Douglas' patch and merge it into the v4l-dvb ivtv driver myself. There were several things in the patch I didn't like, but I need to 'work' with it a bit to see how/if it can be done better.

I can work on it tonight and tomorrow. Hopefully it is finished by then. I can move the BKL down at the same time for ivtv. It is unlikely that I will have time to do cx18 as well as I'm abroad from Wednesday until Monday, but I think Andy can do that easily based on the ivtv changes.

Regards,

Hans

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Mauro Carvalho Chehab

2008-05-26 21:14 UTC #8 lore raw

Quoted 8 lines

Quoted 3 lines
Could you send this patch to the ML for people to review and for Andy to port it to cx18?
Unless there is an objection, I would prefer to take Douglas' patch and merge it into the v4l-dvb ivtv driver myself. There were several things in the patch I didn't like, but I need to 'work' with it a bit to see how/if it can be done better.

I didn't analyze his patch in detail. However, it is a starting point for you to work with. I'm sure you can improve it if needed.

Quoted 4 lines

I can work on it tonight and tomorrow. Hopefully it is finished by then. I can move the BKL down at the same time for ivtv. It is unlikely that I will have time to do cx18 as well as I'm abroad from Wednesday until Monday, but I think Andy can do that easily based on the ivtv changes.

This strategy seems OK to me.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Andy Walls

2008-06-01 02:40 UTC #9 lore raw

PATCH 1/1 cx18: convert driver to video_ioctl2() (Re: [PATCH] video4linux: Push down the BKL)

In reply to Hans Verkuil

Quoted 21 lines · Hans Verkuil

Quoted 9 lines · On Monday 26 May 2008 18:34:57 Mauro Carvalho Chehab wrote:
In the specific case of ivtv and cx18, I think that the better would be to convert it first to video_ioctl2. Then, remove the BKL, with a video_ioctl2_unlocked version.
Douglas already did an experimental patch converting ivtv to video_ioctl2 and sent to Hans. It needs testing, since he doesn't have any ivtv board. It should be trivial to port this to cx18, since both drivers have similar structures.
Quoted 4 lines
Douglas,
Could you send this patch to the ML for people to review and for Andy to port it to cx18?
Unless there is an objection, I would prefer to take Douglas' patch and merge it into the v4l-dvb ivtv driver myself. There were several things in the patch I didn't like, but I need to 'work' with it a bit to see how/if it can be done better.

Quoted 4 lines

I can work on it tonight and tomorrow. Hopefully it is finished by then. I can move the BKL down at the same time for ivtv. It is unlikely that I will have time to do cx18 as well as I'm abroad from Wednesday until Monday, but I think Andy can do that easily based on the ivtv changes.

I have attached a patch, made against Hans' v4l-dvb-ioctl2 repository, to convert the cx18 driver to use video_ioctl2(). In the process I pushed down the priority checks and the debug messages into the individual functions. I did not remove the serialization lock as I have not had the time to assess if that would be safe. I "#if 0"'ed out some sliced VBI code that was being skipped in the original code.

Comments are welcome.

Many thanks to Hans, for without his changeset to ivtv for reference, this would have taken me much, much longer.

Short term initial tests of this patch with MythTV, v4l-ctl, and v4l-dbg indicate things are OK. I did notice once strange artifact with MythTV. When switching from one analog channel to another, for about a second, I see the two television video fields non-interlaced, stacked one atop of the other in the frame. Weird, but not a big deal.

Regards,
Andy

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Hans Verkuil

2008-06-01 10:16 UTC #10 lore raw

Re: [PATCH] cx18: convert driver to video_ioctl2() (Re: [PATCH] video4linux: Push down the BKL)

Quoted 41 lines · Andy Walls

Quoted 20 lines · Hans Verkuil
Quoted 9 lines · On Monday 26 May 2008 18:34:57 Mauro Carvalho Chehab wrote:
In the specific case of ivtv and cx18, I think that the better would be to convert it first to video_ioctl2. Then, remove the BKL, with a video_ioctl2_unlocked version.
Douglas already did an experimental patch converting ivtv to video_ioctl2 and sent to Hans. It needs testing, since he doesn't have any ivtv board. It should be trivial to port this to cx18, since both drivers have similar structures.
Quoted 4 lines
Douglas,
Could you send this patch to the ML for people to review and for Andy to port it to cx18?
Unless there is an objection, I would prefer to take Douglas' patch and merge it into the v4l-dvb ivtv driver myself. There were several things in the patch I didn't like, but I need to 'work' with it a bit to see how/if it can be done better.
Quoted 5 lines
I can work on it tonight and tomorrow. Hopefully it is finished by then. I can move the BKL down at the same time for ivtv. It is unlikely that I will have time to do cx18 as well as I'm abroad from Wednesday until Monday, but I think Andy can do that easily based on the ivtv changes.
I have attached a patch, made against Hans' v4l-dvb-ioctl2 repository, to convert the cx18 driver to use video_ioctl2(). In the process I pushed down the priority checks and the debug messages into the individual functions. I did not remove the serialization lock as I have not had the time to assess if that would be safe. I "#if 0"'ed out some sliced VBI code that was being skipped in the original code.
Comments are welcome.
Many thanks to Hans, for without his changeset to ivtv for reference, this would have taken me much, much longer.

Quoted 6 lines

Short term initial tests of this patch with MythTV, v4l-ctl, and v4l-dbg indicate things are OK. I did notice once strange artifact with MythTV. When switching from one analog channel to another, for about a second, I see the two television video fields non-interlaced, stacked one atop of the other in the frame. Weird, but not a big deal.

Thanks Andy!

I'll take a closer look on Tuesday or Wednesday, but I noticed one
thing: you set unused callbacks to NULL in cx18_set_funcs(), however these can just be removed as they are NULL by default.

Regards,

Hans

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Andy Walls

2008-06-01 19:06 UTC #11 lore raw

Re: [PATCH] cx18: convert driver to video_ioctl2() (Re: [PATCH] video4linux: Push down the BKL)

Quoted 5 lines · Hans Verkuil

Thanks Andy!
I'll take a closer look on Tuesday or Wednesday, but I noticed one
thing: you set unused callbacks to NULL in cx18_set_funcs(), however these can just be removed as they are NULL by default.

Yeah, they can go. I left them in as an aid for double checking that I didn't forget any callbacks that needed to be implemented.

Regards,
Andy

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Mauro Carvalho Chehab

2008-06-03 21:21 UTC #12 lore raw

Re: [PATCH] cx18: convert driver to video_ioctl2() (Re: [PATCH] video4linux: Push down the BKL)

Quoted 11 lines · On Sun, 01 Jun 2008 15:01:59 -0400 Andy Walls <[email protected]> wrote:

Quoted 5 lines · Hans Verkuil
Thanks Andy!
I'll take a closer look on Tuesday or Wednesday, but I noticed one
thing: you set unused callbacks to NULL in cx18_set_funcs(), however these can just be removed as they are NULL by default.
Yeah, they can go. I left them in as an aid for double checking that I didn't forget any callbacks that needed to be implemented.

Please don't do that. All static vars that have a value 0 or NULL shouldn't be initialized, since this will eat some space inside the module.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Alan Cox

2008-06-03 22:08 UTC #13 lore raw

Re: [ivtv-devel] [PATCH] cx18: convert driver to video_ioctl2() (Re: [PATCH] video4linux: Push down the BKL)

Quoted 5 lines · Mauro Carvalho Chehab

Quoted 2 lines
Yeah, they can go. I left them in as an aid for double checking that I didn't forget any callbacks that needed to be implemented.
Please don't do that. All static vars that have a value 0 or NULL shouldn't be initialized, since this will eat some space inside the module.

The compiler is smarter than that. Besides which 4 bytes will make no difference whether it is data or bss given 4K disk block sizes 8)

It is coding style not to do it but it isn't a bad idea to leave them in if they make something explicitly clear IMHO.

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Andy Walls

2008-06-04 00:44 UTC #14 lore raw

Re: [ivtv-devel] [PATCH] cx18: convert driver to video_ioctl2() (Re: [PATCH] video4linux: Push down the BKL)

Quoted 6 lines · Alan Cox

Quoted 5 lines · Mauro Carvalho Chehab
Quoted 2 lines
Yeah, they can go. I left them in as an aid for double checking that I didn't forget any callbacks that needed to be implemented.
Please don't do that. All static vars that have a value 0 or NULL shouldn't be initialized, since this will eat some space inside the module.

Mauro,

OK.

Alan,

Quoted 2 lines

The compiler is smarter than that. Besides which 4 bytes will make no difference whether it is data or bss given 4K disk block sizes 8)

Objdump reveals, unfortunately the compiler isn't that smart. It does explicitly create code to store the NULL pointers.

For the record, every pointer store looks something like this (before relocations) on 64-bit:

1f9: 48 c7 87 c0 04 00 00 movq $0x0,0x4c0(%rdi)
200: 00 00 00 00

11 bytes * 19 NULL pointer stores = 209 bytes + wasted CPU cycles.

Quite honestly, I would have thought the compiler smart enough to do something like

	xor %rax, %rax
	mov %rax, 0x123(%rdi)

as most of the NULL pointer stores happened in groups of two or three. But apparently, it wasn't.

-Andy

Quoted 2 lines

It is coding style not to do it but it isn't a bad idea to leave them in if they make something explicitly clear IMHO.

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Alan Cox

2008-06-04 10:02 UTC #15 lore raw

Re: [ivtv-devel] [PATCH] cx18: convert driver to video_ioctl2() (Re: [PATCH] video4linux: Push down the BKL)

Quoted 7 lines · Andy Walls

For the record, every pointer store looks something like this (before relocations) on 64-bit:
   1f9:       48 c7 87 c0 04 00 00    movq   $0x0,0x4c0(%rdi)
   200:       00 00 00 00 
11 bytes * 19 NULL pointer stores = 209 bytes + wasted CPU cycles.

Static objects don't get stored at init.. they are either in the data segment (older compilers) or BSS (smarter ones)

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Jonathan Corbet

Acked-by on v1 1/1

2008-05-23 13:57 UTC #16 lore raw

In reply to Alan Cox

drivers/media/video/cafe_ccic.c

Quoted 4 lines

drivers/media/video/cafe_ccic.c +1 -1
[...]
-	.ioctl = video_ioctl2,
+	.unlocked_ioctl = video_ioctl2,

Weird, that was always intended to be that way, I'm not sure what happened. In any case, consider this part to be

Acked-by Jonathan Corbet <[email protected]>

However, in this part:

drivers/media/video/videodev.c int video_ioctl2 (struct inode *inode, struct file *file, · line 1953

Quoted 6 lines

drivers/media/video/videodev.c +2 -0

[...]
 
+	lock_kernel();
 	err = __video_do_ioctl(inode, file, cmd, parg);
+	unlock_kernel();

This forces the BKL on all V4L2 drivers whether or not they need it. Given that this code can, in fact, be a path where latency matters (consider VIDIOC_DQBUF), I'm not sure that's the best way to do it.

On the other hand, the next level of BKL pushdown would be painful as all hell, given the massive number of callbacks in the V4L2 API. So I'm thinking it might be justified to create a video_ioctl2_locked() for V4L2 drivers which are not yet known to be safe in the absence of the BKL. The amount of extra code would be quite small, and it would let safe drivers operate BKL-free.

jon

Alan Cox

author

2008-05-23 15:55 UTC #17 lore raw

Quoted 6 lines

On the other hand, the next level of BKL pushdown would be painful as all hell, given the massive number of callbacks in the V4L2 API. So I'm thinking it might be justified to create a video_ioctl2_locked() for V4L2 drivers which are not yet known to be safe in the absence of the BKL. The amount of extra code would be quite small, and it would let safe drivers operate BKL-free.

The problem is that currently they are almost all unsafe - I did a quick survey as part of the changes. Pushing it down to the video2_ioctl is a starting point, but the v4l layer is going to need a lot of love and its own gradual migration.

Right now we've gone from BKL buried in fs to BKL at top of v4l layer which is indeed only a starting point. I'd assumed the same as you are think a new video_ioctl2 and switching drivers one by one (I was assuming adding video2_ioctl_unlocked())

Alan

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Jonathan Corbet

2008-05-23 16:09 UTC #18 lore raw

Quoted 4 lines · Alan Cox

The problem is that currently they are almost all unsafe - I did a quick survey as part of the changes. Pushing it down to the video2_ioctl is a starting point, but the v4l layer is going to need a lot of love and its own gradual migration.

Agreed.

Quoted 4 lines

Right now we've gone from BKL buried in fs to BKL at top of v4l layer which is indeed only a starting point. I'd assumed the same as you are think a new video_ioctl2 and switching drivers one by one (I was assuming adding video2_ioctl_unlocked())

It sound like we're thinking the same way, yes. Except I'd pick the long name for the one we want to get rid of :)

BTW, drop me a pointer if you'd like this series pulled into the ever-growing bkl-removal tree.

jon

Alan Cox

author

2008-05-23 19:11 UTC #19 lore raw

Quoted 2 lines

BTW, drop me a pointer if you'd like this series pulled into the ever-growing bkl-removal tree.

Its a single big patch right now and I need to split it up further and tidy it, plus see what the maintainers take and think. Once I've done that I'd be in favour of actually patching ->ioctl out of the bkl-removal tree and letting people fix the other architectures by neccessity ;)

Alan

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Hans Verkuil

Acked-by on v1 1/1

2008-05-23 19:08 UTC #20 lore raw

In reply to Alan Cox

Quoted 6 lines · On Thursday 22 May 2008 23:37:00 Alan Cox wrote:

For most drivers the generic ioctl handler does the work and we update it and it becomes the unlocked_ioctl method. Older drivers use the usercopy method so we make it do the work. Finally there are a few special cases.
Signed-off-by Alan Cox <[email protected]>

Here is my Ack for the ivtv/cx18 parts of this patch:

Acked-by Hans Verkuil <[email protected]>

Regards,

Hans

Mike Isely

Acked-by on v1 1/1

2008-05-25 23:46 UTC #21 lore raw

In reply to Alan Cox

Quoted 5 lines · Alan Cox

For most drivers the generic ioctl handler does the work and we update it and it becomes the unlocked_ioctl method. Older drivers use the usercopy method so we make it do the work. Finally there are a few special cases.
Signed-off-by Alan Cox <[email protected]>

The pvrusb2 driver already should not require the BKL. Though since this driver (like every V4L driver) is using video_usercopy() then it's obviously going along for the ride here with respect to the overall issue here in pushing the lock down into video_usercopy(). In the mean time, for the pvrusb2 portion of this patch...

Acked-by Mike Isely <[email protected]>

-Mike

Signature · 3 lines

Mike Isely
isely @ pobox (dot) com
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

Mauro Carvalho Chehab

2008-05-26 17:00 UTC #22 lore raw

In reply to Alan Cox

Quoted 3 lines · On Thu, 22 May 2008 22:37:00 +0100 Alan Cox <[email protected]> wrote:

For most drivers the generic ioctl handler does the work and we update it and it becomes the unlocked_ioctl method. Older drivers use the usercopy method so we make it do the work. Finally there are a few special cases.

I liked the patch also.

Still, it didn't apply cleanly on my -git tree, probably due to some fixes at cx18 and ivtv (see the attached log). Also, IMO, it would be better if you split drivers/net/tun.c into a different changeset.

I think you should add a video_ioctl2_unlocked() function, without BKL (and keeping BKL explicit locks at video_ioctl2). This would help on migrating the drivers to the unlocked version, since I suspect that most drivers already have enough locks for the removal of BKL.

I would just implement video_ioctl2 as:

video_ioctl2_locked (...)

{
	lock_kernel();
	video_ioctl2_unlocked();
	unlock_kernel();
}

The next step can be to add the obvious locks inside video_ioctl2_unlocked(). Like, for example, locking the VIDIOC_S calls, if someone is calling the corresponding VIDIOC_G or VIDIOC_TRY ones.

Cheers,
Mauro.

---

patching file drivers/media/video/bt8xx/bttv-driver.c
patching file drivers/media/video/bw-qcam.c
patching file drivers/media/video/c-qcam.c
patching file drivers/media/video/cafe_ccic.c
patching file drivers/media/video/cpia.c
patching file drivers/media/video/cpia2/cpia2_v4l.c
patching file drivers/media/video/cx18/cx18-ioctl.c
patching file drivers/media/video/cx18/cx18-ioctl.h
patching file drivers/media/video/cx18/cx18-streams.c
Hunk #1 FAILED at 39.
1 out of 1 hunk FAILED -- saving rejects to file drivers/media/video/cx18/cx18-streams.c.rej patching file drivers/media/video/cx23885/cx23885-417.c
patching file drivers/media/video/cx23885/cx23885-video.c
patching file drivers/media/video/cx88/cx88-blackbird.c
patching file drivers/media/video/cx88/cx88-video.c
patching file drivers/media/video/dabusb.c
patching file drivers/media/video/em28xx/em28xx-video.c
patching file drivers/media/video/et61x251/et61x251_core.c
patching file drivers/media/video/ivtv/ivtv-ioctl.c
patching file drivers/media/video/ivtv/ivtv-ioctl.h
Hunk #1 FAILED at 24.
1 out of 1 hunk FAILED -- saving rejects to file drivers/media/video/ivtv/ivtv-ioctl.h.rej patching file drivers/media/video/ivtv/ivtv-streams.c
Hunk #1 FAILED at 48.
Hunk #2 FAILED at 58.
2 out of 2 hunks FAILED -- saving rejects to file drivers/media/video/ivtv/ivtv-streams.c.rej patching file drivers/media/video/meye.c
patching file drivers/media/video/ov511.c
patching file drivers/media/video/pms.c
patching file drivers/media/video/pvrusb2/pvrusb2-v4l2.c
Hunk #1 succeeded at 861 (offset -1 lines).
Hunk #2 succeeded at 869 (offset -1 lines).
Hunk #3 succeeded at 1153 (offset -1 lines).
patching file drivers/media/video/pwc/pwc-if.c
patching file drivers/media/video/saa5246a.c
patching file drivers/media/video/saa5249.c
patching file drivers/media/video/saa7134/saa7134-empress.c
patching file drivers/media/video/saa7134/saa7134-video.c
patching file drivers/media/video/se401.c
patching file drivers/media/video/sn9c102/sn9c102_core.c
patching file drivers/media/video/soc_camera.c
patching file drivers/media/video/stk-webcam.c
Hunk #1 succeeded at 1320 (offset 7 lines).
patching file drivers/media/video/stradis.c
patching file drivers/media/video/stv680.c
patching file drivers/media/video/usbvideo/usbvideo.c
patching file drivers/media/video/usbvideo/vicam.c
patching file drivers/media/video/usbvision/usbvision-video.c
patching file drivers/media/video/videodev.c
patching file drivers/media/video/vivi.c
patching file drivers/media/video/w9966.c
patching file drivers/media/video/w9968cf.c
patching file drivers/media/video/zc0301/zc0301_core.c
patching file drivers/media/video/zoran_driver.c
patching file drivers/media/video/zr364xx.c
patching file drivers/net/tun.c
patching file include/media/v4l2-dev.h
Hunk #1 succeeded at 343 (offset -1 lines).
Hunk #2 succeeded at 352 (offset -1 lines).

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Alan Cox

2008-05-26 20:23 UTC #23 lore raw

Quoted 2 lines · Mauro Carvalho Chehab

cx18 and ivtv (see the attached log). Also, IMO, it would be better if you split drivers/net/tun.c into a different changeset.

That was most definitely not intended to be in the video patches 8)

Quoted 3 lines

The next step can be to add the obvious locks inside video_ioctl2_unlocked(). Like, for example, locking the VIDIOC_S calls, if someone is calling the corresponding VIDIOC_G or VIDIOC_TRY ones.

Concentrate on the dats structures not the code - its one of those oft quoted and very true rules - "lock data not code"

I'll tidy these up later in the week as I get time and merge them against a current linux-next tree in bits with the rework done.

Alan

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Mauro Carvalho Chehab

2008-05-26 21:10 UTC #24 lore raw

Quoted 6 lines · On Mon, 26 May 2008 16:23:17 -0400 Alan Cox <[email protected]> wrote:

The next step can be to add the obvious locks inside video_ioctl2_unlocked(). Like, for
Quoted 2 lines
example, locking the VIDIOC_S calls, if someone is calling the corresponding VIDIOC_G or VIDIOC_TRY ones.
Concentrate on the dats structures not the code - its one of those oft quoted and very true rules - "lock data not code"

True. I'm thinking on locking the data that is being GET/SET by VIDIOC_foo ioctls.

I can see a few strategies for removing KBL.

The hardest and optimal scenario is to look inside all drivers, fix the locks (and pray for a further patch to not break them). I'm afraid that this is a long term strategy.

There are also other strategies that will also produce non-optimal results, but may avoid the usage of BKL.

For example, a very simple scenario would simply replace BKL by one mutex. This way, just one ioctl could be handled at the same time. This is something dumb, but will free the machine to do other tasks while an ioctl is being executed. I bet this would work with all (or almost all) drivers. I don't see any big drawback of this scenario, from the userspace POV.

Smarter scenarios can be designed if you use different mutexes for different groups of data, at video_ioctl2 level.

Of course, at video_ioctl2 level, you don't see the real data. However, you'll see data blocks. For example, if you're calling a VIDIOC_S_CROP, you're certainly changing whatever data structures you have that changes crop window. A concurrent call to VIDIOC_G_CROP needs to be blocked, otherwise the get will return a data that is being changing. IMO, such trivial dependencies can be safely handled at vidioc_ioctl2.

We may try to group VIDIOC functions into some mutexes to avoid bad usages. For example, one mutex for ioctl's that touches on video buffers. This will lock on calls for iocls like VIDIOC_DBUF, VIDIOC_REQBUF, etc.

We take some care with ioctls that would affect all groups of data, like VIDIOC_S_STD. This ioctl will likely change several data structures that affects other get operations, like changing resolution. It may also need to stop and/or return -EBUSY, if video buffers are mmapped.

Such scenarios wouldn't avoid the need of implementing a few locks at the drivers, but will make driver's live simpler.

Quoted 2 lines

I'll tidy these up later in the week as I get time and merge them against a current linux-next tree in bits with the rework done.

Seems fine to me ;)

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Alan Cox

2008-05-26 22:01 UTC #25 lore raw

Quoted 3 lines · Mauro Carvalho Chehab

The hardest and optimal scenario is to look inside all drivers, fix the locks (and pray for a further patch to not break them). I'm afraid that this is a long term strategy.

Ultimately that is where you end up.

Quoted 2 lines

For example, a very simple scenario would simply replace BKL by one mutex. This way, just one ioctl could be handled at the same time. This is something

video2_ioctl_serialized() ?

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Mauro Carvalho Chehab

2008-05-27 13:10 UTC #26 lore raw

Quoted 6 lines · On Mon, 26 May 2008 18:01:54 -0400 Alan Cox <[email protected]> wrote:

Quoted 3 lines · Mauro Carvalho Chehab
The hardest and optimal scenario is to look inside all drivers, fix the locks (and pray for a further patch to not break them). I'm afraid that this is a long term strategy.
Ultimately that is where you end up.

Agreed.

Quoted 4 lines

Quoted 2 lines
For example, a very simple scenario would simply replace BKL by one mutex. This way, just one ioctl could be handled at the same time. This is something
video2_ioctl_serialized() ?

Hmm... it maybe an interesting interim solution to create such function, and moving the drivers to it.

What if we create 3 functions:

video_ioctl2_bkl()
video_ioctl2_serialized()
video_ioctl2_unlocked()

The first patch will point .ioctl_unlock to video_ioctl2_bkl.

A next step would be to move the drivers to use the serialized one. I suspect that this will work properly on all devices that are using video_ioctl2, if the videobuf locks are now 100% ok. So, it is just a matter of doing some stress tests. We may start with vivi, since we have a complete domain on what this driver is doing (e.g. no hardware surprises).

After having all those drivers using the _serialized() one, we can remove the bkl.

Then, we can focus on properly fixing the locks inside the drivers, and moving one by one to video_ioctl2_unlocked.

IMO, we need to create a multi-thread stress userspace tool for checking the locks at the ioctls. There are a few testing utils at mercurial tree, under v4l2-apps/test. This can be a starting point for this tool. Also, Brandon improved one of those tools to work with multithread.

What do you think?

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Jonathan Corbet

2008-05-27 15:41 UTC #27 lore raw

Quoted 9 lines · On Tue, 27 May 2008 10:10:39 -0300 Mauro Carvalho Chehab…

What if we create 3 functions:
video_ioctl2_bkl()
video_ioctl2_serialized()
video_ioctl2_unlocked()
The first patch will point .ioctl_unlock to video_ioctl2_bkl.
A next step would be to move the drivers to use the serialized one.

So we're replacing the big kernel lock with the big v4l2 lock. That might help the situation, but you'd need to be sure to serialize against other calls (open(), for example) which are also currently done under the BKL.

Quoted 5 lines

IMO, we need to create a multi-thread stress userspace tool for checking the locks at the ioctls. There are a few testing utils at mercurial tree, under v4l2-apps/test. This can be a starting point for this tool. Also, Brandon improved one of those tools to work with multithread.

I don't think that stress tools are the way to eliminate the BKL. You'll never find all the problems that way. There's really no way to avoid the task of actually *looking* at each driver and ensuring that it has its act together with regard to locking.

jon

Mauro Carvalho Chehab

2008-05-27 16:31 UTC #28 lore raw

Quoted 6 lines · On Tue, 27 May 2008 09:41:44 -0600 Jonathan Corbet <[email protected]> wrote:

Quoted 1 line
A next step would be to move the drivers to use the serialized one.
So we're replacing the big kernel lock with the big v4l2 lock. That might help the situation, but you'd need to be sure to serialize against other calls (open(), for example) which are also currently done under the BKL.

True, but on a quick analysis, I suspect that this is already somewhat broken on some drivers.

Since the other methods don't explicitly call BKL (and, AFAIK, kernel open handler don't call it neither), if a program 1 is opening a device and initializing some data, and a program 2 starts doing ioctl, interrupting program 1 execution in the middle of a data initialization procedure, you may have a race condition, since some devices initialize some device global data during open [1].

[1] For example: cx88 and saa7134 will change some data structures if you open a device via /dev/radio or via /dev/video. So, if program 1 opens as radio and program 2 opens as video, you may have a race condition. A way to fix this is to initialize such structs only by the first program that is opening the device, and serialize a concurrent open.

Quoted 10 lines

Quoted 5 lines
IMO, we need to create a multi-thread stress userspace tool for checking the locks at the ioctls. There are a few testing utils at mercurial tree, under v4l2-apps/test. This can be a starting point for this tool. Also, Brandon improved one of those tools to work with multithread.
I don't think that stress tools are the way to eliminate the BKL. You'll never find all the problems that way. There's really no way to avoid the task of actually *looking* at each driver and ensuring that it has its act together with regard to locking.

True. Yet, just looking at the code may not be enough, since people make mistakes. The recent changes at videobuf lock showed this. The lock fix patches caused several new locking issues.

It is safer to have a tool to test and stress the driver before going to production.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Jonathan Corbet

2008-05-27 16:38 UTC #29 lore raw

Quoted 6 lines · On Tue, 27 May 2008 13:31:00 -0300 Mauro Carvalho Chehab…

Since the other methods don't explicitly call BKL (and, AFAIK, kernel open handler don't call it neither), if a program 1 is opening a device and initializing some data, and a program 2 starts doing ioctl, interrupting program 1 execution in the middle of a data initialization procedure, you may have a race condition, since some devices initialize some device global data during open [1].

In fact, 2.6.26 and prior kernels *do* acquire the BKL on open (for char devices) - that's the behavior that the bkl-removal tree is there to do away with. So, for example, I've pushed that acquisition down into video_open() instead.

So, for now, open() is serialized against ioctl() in video drivers. As soon as you take the BKL out of ioctl(), though, that won't happen, unless the mutex you use is also acquired in the open path.

jon

Mauro Carvalho Chehab

2008-05-27 18:59 UTC #30 lore raw

Quoted 10 lines · On Tue, 27 May 2008 10:37:55 -0600 Jonathan Corbet <[email protected]> wrote:

Quoted 6 lines · On Tue, 27 May 2008 13:31:00 -0300 Mauro Carvalho Chehab…
Since the other methods don't explicitly call BKL (and, AFAIK, kernel open handler don't call it neither), if a program 1 is opening a device and initializing some data, and a program 2 starts doing ioctl, interrupting program 1 execution in the middle of a data initialization procedure, you may have a race condition, since some devices initialize some device global data during open [1].

Quoted 8 lines

In fact, 2.6.26 and prior kernels *do* acquire the BKL on open (for char devices) - that's the behavior that the bkl-removal tree is there to do away with. So, for example, I've pushed that acquisition down into video_open() instead.
So, for now, open() is serialized against ioctl() in video drivers. As soon as you take the BKL out of ioctl(), though, that won't happen, unless the mutex you use is also acquired in the open path.

Ok.

A few drivers seem to be almost read to work without BKL.

For example, em28xx has already a lock at the operations that change values at "dev" struct, including open() method. However, since the lock is not called at get operations, it needs to be fixed. I would also change it from mutex to a read/write semaphore, since two (or more) get operations can safely happen in parallel.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Devin Heitmueller

2008-05-27 19:26 UTC #31 lore raw

Hello Mauro,

Quoted 5 lines · Mauro Carvalho Chehab

For example, em28xx has already a lock at the operations that change values at "dev" struct, including open() method. However, since the lock is not called at get operations, it needs to be fixed. I would also change it from mutex to a read/write semaphore, since two (or more) get operations can safely happen in parallel.

Please bear in mind that we have not worked out the locking semantics for hybrid tuner devices, and it's entirely possible that the get() routines will need to switch the tuner mode, which would eliminate any benefits of converting to a read/write semaphore.

I'm not sure yet exactly how that's going to work, but it's something that might prompt you to defer converting it from a mutex until we have that worked out.

Signature · 3 lines

Devin J. Heitmueller
http://www.devinheitmueller.com
AIM: devinheitmueller

Mauro Carvalho Chehab

2008-05-27 21:00 UTC #32 lore raw

Hi Devin,

Quoted 10 lines · On Tue, 27 May 2008 15:26:27 -0400 "Devin Heitmueller"…

Hello Mauro,
Quoted 5 lines · Mauro Carvalho Chehab
For example, em28xx has already a lock at the operations that change values at "dev" struct, including open() method. However, since the lock is not called at get operations, it needs to be fixed. I would also change it from mutex to a read/write semaphore, since two (or more) get operations can safely happen in parallel.

Quoted 4 lines

Please bear in mind that we have not worked out the locking semantics for hybrid tuner devices, and it's entirely possible that the get() routines will need to switch the tuner mode, which would eliminate any benefits of converting to a read/write semaphore.

Arjan pointed some good reasons about why we shouldn't use r/w semaphores. So, it seems better to keep using mutexes.

Quoted 3 lines

I'm not sure yet exactly how that's going to work, but it's something that might prompt you to defer converting it from a mutex until we have that worked out.

The hybrid device mode lock is somewhat complex. The simplest solution would be to block an open() call, if the device is already used by a different mode.

This will minimize things like firmware reload, on xc3028 devices, where you have different firmwares for analog and digital modes.

Also, some USB devices (like HVR-900/HVR-950) switches off analog audio and tv demod chips when in digital mode (the reverse is also true - e.g. digital demod is switched off at analog mode).

So, if you are in digital mode, on HVR-900, and changes to analog, you'll need to re-initialize tvp5150/msp3400. The current code for those devices handles this at open(). If we let this to happen later, we'll need to re-send the video and audio parameters to all I2C connected devices, when switching mode.

On the other hand, some userspace apps, like mythtv, opens both analog and digital API's. I'm not sure if it does this at the same time, but, if so, a lock at open() will cause a regression there (someone told me that this is the case - I didn't test it here yet).

One possible solution of providing a proper code to change mode, and not blocking open() would be to write something like this:

static int check_and_switch_mode(void *priv, int digital)

{
	struct dev_foo *dev = priv;

	mutex_lock(dev->lock);

	if (digital)
		return change_to_digital(dev);
	else
		return change_to_analog(dev);

	mutex_unlock(dev->lock);
}

Since this should be called for every valid V4L2 and DVB ioctl, the better place for it would be to add this as a new function callback, at video_ioctl2. Something like [1]:

linux/drivers/media/video/videodev.c +4 -0

@@ -821,6 +821,10 @@
 		v4l_print_ioctl(vfd->name, cmd);
 		printk("\n");
 	}
+
+	if (_IOC_TYPE(cmd)=='v') || _IOC_TYPE(cmd)=='V') &&
+		vfd->vidioc_switch_mode)
+		ret=vfd->vidioc_switch_mode(fh, 0);
 
 #ifdef CONFIG_VIDEO_V4L1_COMPAT
 	/***********************************************************

And something like this, at dvb core [2]:

linux/drivers/media/dvb/dvb-core/dvb_frontend.c +3 -0

diff -r b94d587ee596 linux/drivers/media/dvb/dvb-core/dvb_frontend.c
@@ -784,6 +784,9 @@
 	     cmd == FE_DISEQC_RECV_SLAVE_REPLY))
 		return -EPERM;
 
+	if (fe->ops.switch_mode)
+		err = fe->ops.switch_mode(fe, 1);
+
 	if (down_interruptible (&fepriv->sem))
 		return -ERESTARTSYS;

[1] The code can be more conservative, changing mode only if S_STD or a video stream ioctl is called.

[2] We need to think more about the proper places for the DVB changing mode. I suspect that we'll need to add the mode change callback there and/or at other different places.

PS.: I suspect that the real code will be much more complex than the above skeletons.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Devin Heitmueller

2008-05-27 21:22 UTC #33 lore raw

Hello Mauro,

Quoted 80 lines · Mauro Carvalho Chehab

Quoted 3 lines
I'm not sure yet exactly how that's going to work, but it's something that might prompt you to defer converting it from a mutex until we have that worked out.
The hybrid device mode lock is somewhat complex. The simplest solution would be to block an open() call, if the device is already used by a different mode.
This will minimize things like firmware reload, on xc3028 devices, where you have different firmwares for analog and digital modes.
Also, some USB devices (like HVR-900/HVR-950) switches off analog audio and tv demod chips when in digital mode (the reverse is also true - e.g. digital demod is switched off at analog mode).
So, if you are in digital mode, on HVR-900, and changes to analog, you'll need to re-initialize tvp5150/msp3400. The current code for those devices handles this at open(). If we let this to happen later, we'll need to re-send the video and audio parameters to all I2C connected devices, when switching mode.
On the other hand, some userspace apps, like mythtv, opens both analog and digital API's. I'm not sure if it does this at the same time, but, if so, a lock at open() will cause a regression there (someone told me that this is the case - I didn't test it here yet).
One possible solution of providing a proper code to change mode, and not blocking open() would be to write something like this:
static int check_and_switch_mode(void *priv, int digital)
{
       struct dev_foo *dev = priv;

       mutex_lock(dev->lock);

       if (digital)
               return change_to_digital(dev);
       else
               return change_to_analog(dev);

       mutex_unlock(dev->lock);
}
Since this should be called for every valid V4L2 and DVB ioctl, the better place for it would be to add this as a new function callback, at video_ioctl2. Something like [1]:
linux/drivers/media/video/videodev.c Tue May 27 17:34:04 2008 -0300 renamed from linux/drivers/media/video/videodev.c Tue May 27 16:02:56 2008 -0300 +4 -0
@@ -821,6 +821,10 @@
               v4l_print_ioctl(vfd->name, cmd);
               printk("\n");
       }
+
+       if (_IOC_TYPE(cmd)=='v') || _IOC_TYPE(cmd)=='V') &&
+               vfd->vidioc_switch_mode)
+               ret=vfd->vidioc_switch_mode(fh, 0);
 
 #ifdef CONFIG_VIDEO_V4L1_COMPAT
       /***********************************************************
And something like this, at dvb core [2]:
diff -r b94d587ee596 linux/drivers/media/dvb/dvb-core/dvb_frontend.c
linux/drivers/media/dvb/dvb-core/dvb_frontend.c Tue May 27 17:35:57 2008 -0300 renamed from linux/drivers/media/dvb/dvb-core/dvb_frontend.c Tue May 27 16:02:56 2008 -0300 +3 -0
@@ -784,6 +784,9 @@
            cmd == FE_DISEQC_RECV_SLAVE_REPLY))
               return -EPERM;
 
+       if (fe->ops.switch_mode)
+               err = fe->ops.switch_mode(fe, 1);
+
       if (down_interruptible (&fepriv->sem))
               return -ERESTARTSYS;
 
[1] The code can be more conservative, changing mode only if S_STD or a video stream ioctl is called.

Quoted 8 lines

[2] We need to think more about the proper places for the DVB changing mode. I suspect that we'll need to add the mode change callback there and/or at other different places.
PS.: I suspect that the real code will be much more complex than the above skeletons.
Cheers,
Mauro

These are all good points (and I had come to some of the same conclusions). I was just trying to say that the fact that we haven't worked through these details is a good reason to not attempt to optimize the locking routines in the em28xx driver at this point. But it seems like Arjan's comments make that a moot argument anyway.

Thanks,

Signature · 3 lines

Devin J. Heitmueller
http://www.devinheitmueller.com
AIM: devinheitmueller

Andy Walls

2008-05-27 23:48 UTC #34 lore raw

In reply to Mauro Carvalho Chehab

Quoted 43 lines · Mauro Carvalho Chehab

Hi Devin,
Quoted 10 lines · On Tue, 27 May 2008 15:26:27 -0400 "Devin Heitmueller"…
Hello Mauro,
Quoted 5 lines · Mauro Carvalho Chehab
For example, em28xx has already a lock at the operations that change values at "dev" struct, including open() method. However, since the lock is not called at get operations, it needs to be fixed. I would also change it from mutex to a read/write semaphore, since two (or more) get operations can safely happen in parallel.
Quoted 4 lines
Please bear in mind that we have not worked out the locking semantics for hybrid tuner devices, and it's entirely possible that the get() routines will need to switch the tuner mode, which would eliminate any benefits of converting to a read/write semaphore.
Arjan pointed some good reasons about why we shouldn't use r/w semaphores. So, it seems better to keep using mutexes.
Quoted 3 lines
I'm not sure yet exactly how that's going to work, but it's something that might prompt you to defer converting it from a mutex until we have that worked out.
The hybrid device mode lock is somewhat complex. The simplest solution would be to block an open() call, if the device is already used by a different mode.
This will minimize things like firmware reload, on xc3028 devices, where you have different firmwares for analog and digital modes.
Also, some USB devices (like HVR-900/HVR-950) switches off analog audio and tv demod chips when in digital mode (the reverse is also true - e.g. digital demod is switched off at analog mode).
So, if you are in digital mode, on HVR-900, and changes to analog, you'll need to re-initialize tvp5150/msp3400. The current code for those devices handles this at open(). If we let this to happen later, we'll need to re-send the video and audio parameters to all I2C connected devices, when switching mode.

Quoted 4 lines

On the other hand, some userspace apps, like mythtv, opens both analog and digital API's. I'm not sure if it does this at the same time, but, if so, a lock at open() will cause a regression there (someone told me that this is the case - I didn't test it here yet).

MythTV's mythbackend can open both sides of the card at the same time and the cx18 driver supports it. On my HVR-1600, MythTV may have the digital side of the card open pulling EPG data off of the ATSC broadcasts, when I open up the MythTV frontend and start watching live TV on the analog side of the card. MythTV also supports Picture-in-Picture using both the analog and digital parts of the HVR-1600.

Quoted 61 lines

One possible solution of providing a proper code to change mode, and not blocking open() would be to write something like this:
static int check_and_switch_mode(void *priv, int digital)
{
	struct dev_foo *dev = priv;

	mutex_lock(dev->lock);

	if (digital)
		return change_to_digital(dev);
	else
		return change_to_analog(dev);

	mutex_unlock(dev->lock);
}
Since this should be called for every valid V4L2 and DVB ioctl, the better place for it would be to add this as a new function callback, at video_ioctl2. Something like [1]:
linux/drivers/media/video/videodev.c +4 -0
@@ -821,6 +821,10 @@
 		v4l_print_ioctl(vfd->name, cmd);
 		printk("\n");
 	}
+
+	if (_IOC_TYPE(cmd)=='v') || _IOC_TYPE(cmd)=='V') &&
+		vfd->vidioc_switch_mode)
+		ret=vfd->vidioc_switch_mode(fh, 0);
 
 #ifdef CONFIG_VIDEO_V4L1_COMPAT
 	/***********************************************************
And something like this, at dvb core [2]:
diff -r b94d587ee596 linux/drivers/media/dvb/dvb-core/dvb_frontend.c
linux/drivers/media/dvb/dvb-core/dvb_frontend.c +3 -0
@@ -784,6 +784,9 @@
 	     cmd == FE_DISEQC_RECV_SLAVE_REPLY))
 		return -EPERM;
 
+	if (fe->ops.switch_mode)
+		err = fe->ops.switch_mode(fe, 1);
+
 	if (down_interruptible (&fepriv->sem))
 		return -ERESTARTSYS;
 
[1] The code can be more conservative, changing mode only if S_STD or a video stream ioctl is called.
[2] We need to think more about the proper places for the DVB changing mode. I suspect that we'll need to add the mode change callback there and/or at other different places.
PS.: I suspect that the real code will be much more complex than the above skeletons.

Quoted 8 lines

Cheers,
Mauro
--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Devin Heitmueller

2008-05-28 00:46 UTC #35 lore raw

Hello Andy,

Quoted 7 lines · Andy Walls

MythTV's mythbackend can open both sides of the card at the same time and the cx18 driver supports it. On my HVR-1600, MythTV may have the digital side of the card open pulling EPG data off of the ATSC broadcasts, when I open up the MythTV frontend and start watching live TV on the analog side of the card. MythTV also supports Picture-in-Picture using both the analog and digital parts of the HVR-1600.

In this case, what you see as a 'feature' in MythTV is actually a problem in our case. While the HVR-1600 can support this scenario, the HVR-950 can only use one or the other (the em28xx chip uses GPIOs to enable the demodulator and presumably you should never have both demodulators enabled at the same time). Because of this we need a lock. If MythTV only opened one device or the other at a time, we could put the lock on the open() call, but since MythTV opens both simultaneously even though it may only be using one, we would need a much more granular lock.

Certainly I'm not blaming MythTV for this behavior, but it will make the locking much more complicated in some hybrid devices.

Signature · 3 lines

Devin J. Heitmueller
http://www.devinheitmueller.com
AIM: devinheitmueller

Andy Walls

2008-05-28 02:37 UTC #36 lore raw

Quoted 11 lines · Devin Heitmueller

Hello Andy,
Quoted 7 lines · Andy Walls
MythTV's mythbackend can open both sides of the card at the same time and the cx18 driver supports it. On my HVR-1600, MythTV may have the digital side of the card open pulling EPG data off of the ATSC broadcasts, when I open up the MythTV frontend and start watching live TV on the analog side of the card. MythTV also supports Picture-in-Picture using both the analog and digital parts of the HVR-1600.

Quoted 9 lines

In this case, what you see as a 'feature' in MythTV is actually a problem in our case. While the HVR-1600 can support this scenario, the HVR-950 can only use one or the other (the em28xx chip uses GPIOs to enable the demodulator and presumably you should never have both demodulators enabled at the same time). Because of this we need a lock. If MythTV only opened one device or the other at a time, we could put the lock on the open() call, but since MythTV opens both simultaneously even though it may only be using one, we would need a much more granular lock.

I don't think a lock would be good for MythTV or any other app that open()s multiple nodes at once. How can an app know that it's dead-locking or barring itself via the kernel driver?

Maybe return an EBUSY or E-something else for these cases when Myth tries to open() the second device node, when there's an underlying factor that requires things to be mutually exclusive. Allowing things like read() to allow hardware mode switching between analog and digital seems like it could result in really weird behaviors at the application.

I'll cite a precedent:
ivtv returns EBUSY on open() when there's a conflict with it's various analog devices nodes that depend on the same underlying hardware: MPG, YUV, FM Radio, etc.

I note the man page for open() doesn't list EBUSY as a valid errno. However, the V4L2 API Spec does list EBUSY as a valid errno for V4L2 open().

Quoted 2 lines

Certainly I'm not blaming MythTV for this behavior, but it will make the locking much more complicated in some hybrid devices.

I like to blame MythTV for a lot of things. ;)

But in this case I can't. The driver probably shouldn't hold a lock and suspend an open() indefinitely (IMO). It should say the device is BUSY as that is the truth: an underlying hardware device or resource is busy.

Regards,
Andy

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Devin Heitmueller

2008-05-28 02:47 UTC #37 lore raw

Quoted 5 lines

Maybe return an EBUSY or E-something else for these cases when Myth tries to open() the second device node, when there's an underlying factor that requires things to be mutually exclusive. Allowing things like read() to allow hardware mode switching between analog and digital seems like it could result in really weird behaviors at the application.

Pardon me for not being clear. I wasn't suggesting having a mutex for on the open call itself. The mutex we have in the em28xx driver is only in place when we are *switching* between modes. The thinking was to have open return EBUSY if the device is already in use in the other mode, but we weren't sure if that would cause problems with MythTV (since the open call would fail) If MythTV can gracefully handle that scenario, then that would be the ideal solution from a driver perspective.

Quoted 3 lines

But in this case I can't. The driver probably shouldn't hold a lock and suspend an open() indefinitely (IMO). It should say the device is BUSY as that is the truth: an underlying hardware device or resource is busy.

Yeah, this was just me not being clear.

Signature · 3 lines

Devin J. Heitmueller
http://www.devinheitmueller.com
AIM: devinheitmueller

Andy Walls

2008-05-28 23:30 UTC #38 lore raw

Quoted 8 lines · Devin Heitmueller

Quoted 5 lines
Maybe return an EBUSY or E-something else for these cases when Myth tries to open() the second device node, when there's an underlying factor that requires things to be mutually exclusive. Allowing things like read() to allow hardware mode switching between analog and digital seems like it could result in really weird behaviors at the application.
Pardon me for not being clear. I wasn't suggesting having a mutex for on the open call itself.

Devin,

My apologies, I didn't quite understand what you meant.

Quoted 2 lines

The mutex we have in the em28xx driver is
only in place when we are *switching* between modes.

I took a look at the em28xx driver, and I see what you mean.

Quoted 6 lines

The thinking was
to have open return EBUSY if the device is already in use in the other mode, but we weren't sure if that would cause problems with MythTV (since the open call would fail) If MythTV can gracefully handle that scenario, then that would be the ideal solution from a driver perspective.

MythTV's backend can handle it, but the user experience with the frontend is "suboptimal".

What follows probably belongs on a MythTV list really, but it may help you make design and specification decisions.

In the case where the cx18 driver normally allows the two sides of the card to be treated independently, we had a bug actually enforcing mutual exclusion upon an ioctl(), if the DVB side of the card was opened before the analog side. Here's what showed up in the mythbackend log (noting that MythTV was told the analog side of the cx18 supported HVR-1600 was an ivtv supported PVR-xx0 card) :

2008-05-12 14:21:16.358 TVRec(5): Changing from None to WatchingLiveTV
2008-05-12 14:21:16.361 TVRec(5): HW Tuner: 5->5
2008-05-12 14:21:17.586

Not ivtv driver??

2008-05-12 14:21:17.592 AutoExpire: CalcParams(): Max required Free Space: 31.0 GB w/freq: 15 min
2008-05-12 14:21:17.600 MPEGRec(/dev/video1) Error: Error setting format
                        eno: Device or resource busy (16)
2008-05-12 14:21:36.656 AutoExpire: CalcParams(): Max required Free Space: 31.0 GB w/freq: 15 min
2008-05-12 14:22:04.112 TVRec(5): Changing from WatchingLiveTV to None

MythTV saw the EBUSY and then proceeded to hang around for about 30 seconds before giving up. The user experience is a fairly non-responsive black screen on the MythTV frontend with no error indications, returning to the main menu when the 30 second timeout finally occurs.

So even if MythTV isn't informed about mutually exclusive conditions, it will try to operate and then give up. But MythTV won't crash or cease to try and run scheduled events.

I have noticed that MythTV never holds the V4L2 device nodes open, unless it's actively trying to capture something.

I have also noticed that MythTV will hold the DVB frontend0 node open for quite a long while when nothing else is going on. I suspect this is due to:

a. me configuring MythTV to use the DVB side of the card to pull EPG data from the broadcasts even when no MythTV frontend nor scheduled capture is running, and

b. no lock or EPG data is available from some channel

So MythTV is robust enough to handle EBUSY returns, but could use some improvement on the MythTV frontend application. Also, MythTV could stand to have some settings for the user to indicate mutually exclusive actions due to underlying hardware.

I think a bad thing for a driver to do is to not return an error code, and let MythTV go into it's event loop, giving the user a "black screen forever" and a log file full of select() timeout messages because no data is available for read(). (We had a cx18 driver bug that gave us those symptoms too!)

I hope that helps.

-Andy

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Alan Cox

2008-05-28 08:34 UTC #39 lore raw

In reply to Andy Walls

Quoted 3 lines · Andy Walls

I note the man page for open() doesn't list EBUSY as a valid errno. However, the V4L2 API Spec does list EBUSY as a valid errno for V4L2 open().

The posix spec doesn't limit the errors returnable this way - it says if the error is one of the ones listed it must return the error code stated so EBUSY is just fine.

Quoted 2 lines

But in this case I can't. The driver probably shouldn't hold a lock and suspend an open() indefinitely (IMO). It should say the device is BUSY

Agreed - especially if it is opened with O_NDELAY

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Hans Verkuil

2008-05-28 06:13 UTC #40 lore raw

In reply to Devin Heitmueller

Quoted 12 lines · On Wednesday 28 May 2008 02:46:22 Devin Heitmueller wrote:

Hello Andy,
Quoted 8 lines · Andy Walls
MythTV's mythbackend can open both sides of the card at the same time and the cx18 driver supports it. On my HVR-1600, MythTV may have the digital side of the card open pulling EPG data off of the ATSC broadcasts, when I open up the MythTV frontend and start watching live TV on the analog side of the card. MythTV also supports
Picture-in-Picture using both the analog and digital parts of the HVR-1600.

Quoted 2 lines

In this case, what you see as a 'feature' in MythTV is actually a problem in our case. While the HVR-1600 can support this scenario,

Actually, it depends on how the card was defined. There are cards that only utilize the analog part of the cx23418, cards that can do both digital and analog at the same time and cards that can do either digital or analog. The cx18 driver supports the first two cases but not yet the last.

Regards,

Hans

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Arjan van de Ven

2008-05-27 19:51 UTC #41 lore raw

In reply to Mauro Carvalho Chehab

Quoted 26 lines · On Tue, 27 May 2008 15:59:42 -0300 Mauro Carvalho Chehab…

Quoted 16 lines · On Tue, 27 May 2008 10:37:55 -0600 Jonathan Corbet <[email protected]> wrote:
Quoted 7 lines · On Tue, 27 May 2008 13:31:00 -0300 Mauro Carvalho Chehab…
Since the other methods don't explicitly call BKL (and, AFAIK, kernel open handler don't call it neither), if a program 1 is opening a device and initializing some data, and a program 2 starts doing ioctl, interrupting program 1 execution in the middle of a data initialization procedure, you may have a race condition, since some devices initialize some device global data during open [1].
In fact, 2.6.26 and prior kernels *do* acquire the BKL on open (for char devices) - that's the behavior that the bkl-removal tree is there to do away with. So, for example, I've pushed that acquisition down into video_open() instead.
Quoted 4 lines
So, for now, open() is serialized against ioctl() in video drivers. As soon as you take the BKL out of ioctl(), though, that won't happen, unless the mutex you use is also acquired in the open path.
Ok.

Quoted 8 lines

A few drivers seem to be almost read to work without BKL.
For example, em28xx has already a lock at the operations that change values at "dev" struct, including open() method. However, since the lock is not called at get operations, it needs to be fixed. I would also change it from mutex to a read/write semaphore, since two (or more) get operations can safely happen in parallel.

please don't use rw/sems just because there MIGHT be parallel. THey're more expensive than mutexes by quite a bit and you get a lot less checking from lockdep. They make sense for very specific, very read biased, contended cases. But please don't use them "just because"...

Mauro Carvalho Chehab

2008-05-27 20:24 UTC #42 lore raw

Quoted 37 lines · On Tue, 27 May 2008 12:50:41 -0700 Arjan van de Ven <[email protected]> wrote:

Quoted 26 lines · On Tue, 27 May 2008 15:59:42 -0300 Mauro Carvalho Chehab…
Quoted 16 lines · On Tue, 27 May 2008 10:37:55 -0600 Jonathan Corbet <[email protected]> wrote:
Quoted 7 lines · On Tue, 27 May 2008 13:31:00 -0300 Mauro Carvalho Chehab…
Since the other methods don't explicitly call BKL (and, AFAIK, kernel open handler don't call it neither), if a program 1 is opening a device and initializing some data, and a program 2 starts doing ioctl, interrupting program 1 execution in the middle of a data initialization procedure, you may have a race condition, since some devices initialize some device global data during open [1].
In fact, 2.6.26 and prior kernels *do* acquire the BKL on open (for char devices) - that's the behavior that the bkl-removal tree is there to do away with. So, for example, I've pushed that acquisition down into video_open() instead.
Quoted 4 lines
So, for now, open() is serialized against ioctl() in video drivers. As soon as you take the BKL out of ioctl(), though, that won't happen, unless the mutex you use is also acquired in the open path.
Ok.
Quoted 8 lines
A few drivers seem to be almost read to work without BKL.
For example, em28xx has already a lock at the operations that change values at "dev" struct, including open() method. However, since the lock is not called at get operations, it needs to be fixed. I would also change it from mutex to a read/write semaphore, since two (or more) get operations can safely happen in parallel.

Quoted 8 lines

\
please don't use rw/sems just because there MIGHT be parallel. THey're more expensive than mutexes by quite a bit and you get a lot less checking from lockdep. They make sense for very specific, very read biased, contended cases. But please don't use them "just because"...

Good point. The nature of get operations on V4L are not worthy enough to justify the loss of lockdep checking.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Alan Cox

2008-05-27 18:14 UTC #43 lore raw

In reply to Mauro Carvalho Chehab

Quoted 2 lines · Mauro Carvalho Chehab

It is safer to have a tool to test and stress the driver before going to production.

Stress testing is rarely useful here - things like ioctl races you find by thinking evil thoughts. Stress tests that fail obviously help find some stuff but its error cases and hotplug corner cases that are usually the most foul

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list