[RFC PATCH 0/8] V4L BKL removal: first round

[RFC PATCH 0/8] V4L BKL removal: first round

[Hans Verkuil]

This patch series converts 24 v4l drivers to unlocked_ioctl. These are low
hanging fruit but you have to start somewhere :-)

The first patch replaces mutex_lock in the V4L2 core by mutex_lock_interruptible
for most fops.

Hans Verkuil (8):
  v4l2-dev: use mutex_lock_interruptible instead of plain mutex_lock
  BKL: trivial BKL removal from V4L2 radio drivers
  cadet: use unlocked_ioctl
  tea5764: convert to unlocked_ioctl
  si4713: convert to unlocked_ioctl
  typhoon: convert to unlocked_ioctl.
  dsbr100: convert to unlocked_ioctl.
  BKL: trivial ioctl -> unlocked_ioctl video driver conversions

 drivers/media/radio/dsbr100.c          |    2 +-
 drivers/media/radio/radio-aimslab.c    |   16 +++++-----
 drivers/media/radio/radio-aztech.c     |    6 ++--
 drivers/media/radio/radio-cadet.c      |   12 ++++++--
 drivers/media/radio/radio-gemtek-pci.c |    6 ++--
 drivers/media/radio/radio-gemtek.c     |   14 ++++----
 drivers/media/radio/radio-maestro.c    |   14 ++++-----
 drivers/media/radio/radio-maxiradio.c  |    2 +-
 drivers/media/radio/radio-miropcm20.c  |    6 ++-
 drivers/media/radio/radio-rtrack2.c    |   10 +++---
 drivers/media/radio/radio-sf16fmi.c    |    7 ++--
 drivers/media/radio/radio-sf16fmr2.c   |   11 +++----
 drivers/media/radio/radio-si4713.c     |    3 +-
 drivers/media/radio/radio-tea5764.c    |   49 ++++++--------------------------
 drivers/media/radio/radio-terratec.c   |    8 ++--
 drivers/media/radio/radio-trust.c      |   18 ++++++------
 drivers/media/radio/radio-typhoon.c    |   16 +++++-----
 drivers/media/radio/radio-zoltrix.c    |   30 ++++++++++----------
 drivers/media/video/arv.c              |    2 +-
 drivers/media/video/bw-qcam.c          |    2 +-
 drivers/media/video/c-qcam.c           |    2 +-
 drivers/media/video/meye.c             |   14 ++++----
 drivers/media/video/pms.c              |    2 +-
 drivers/media/video/v4l2-dev.c         |   44 +++++++++++++++++++++--------
 drivers/media/video/w9966.c            |    2 +-
 25 files changed, 147 insertions(+), 151 deletions(-)

[RFC PATCH 1/8] v4l2-dev: use mutex_lock_interruptible instead of plain mutex_lock

[Hans Verkuil]

Where reasonable use mutex_lock_interruptible instead of mutex_lock.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/video/v4l2-dev.c |   44 +++++++++++++++++++++++++++++----------
 1 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 0ca7978..8eb0756 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -191,8 +191,12 @@ static ssize_t v4l2_read(struct file *filp, char __user *buf,
 
 	if (!vdev->fops->read)
 		return -EINVAL;
-	if (vdev->lock)
-		mutex_lock(vdev->lock);
+	if (vdev->lock) {
+		int res = mutex_lock_interruptible(vdev->lock);
+
+		if (res)
+			return res;
+	}
 	if (video_is_registered(vdev))
 		ret = vdev->fops->read(filp, buf, sz, off);
 	if (vdev->lock)
@@ -208,8 +212,12 @@ static ssize_t v4l2_write(struct file *filp, const char __user *buf,
 
 	if (!vdev->fops->write)
 		return -EINVAL;
-	if (vdev->lock)
-		mutex_lock(vdev->lock);
+	if (vdev->lock) {
+		int res = mutex_lock_interruptible(vdev->lock);
+
+		if (res)
+			return res;
+	}
 	if (video_is_registered(vdev))
 		ret = vdev->fops->write(filp, buf, sz, off);
 	if (vdev->lock)
@@ -224,8 +232,8 @@ static unsigned int v4l2_poll(struct file *filp, struct poll_table_struct *poll)
 
 	if (!vdev->fops->poll)
 		return ret;
-	if (vdev->lock)
-		mutex_lock(vdev->lock);
+	if (vdev->lock && mutex_lock_interruptible(vdev->lock))
+		return ret;
 	if (video_is_registered(vdev))
 		ret = vdev->fops->poll(filp, poll);
 	if (vdev->lock)
@@ -239,8 +247,12 @@ static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	int ret = -ENODEV;
 
 	if (vdev->fops->unlocked_ioctl) {
-		if (vdev->lock)
-			mutex_lock(vdev->lock);
+		if (vdev->lock) {
+			int res = mutex_lock_interruptible(vdev->lock);
+
+			if (res)
+				return res;
+		}
 		if (video_is_registered(vdev))
 			ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
 		if (vdev->lock)
@@ -264,8 +276,12 @@ static int v4l2_mmap(struct file *filp, struct vm_area_struct *vm)
 
 	if (!vdev->fops->mmap)
 		return ret;
-	if (vdev->lock)
-		mutex_lock(vdev->lock);
+	if (vdev->lock) {
+		int res = mutex_lock_interruptible(vdev->lock);
+
+		if (res)
+			return res;
+	}
 	if (video_is_registered(vdev))
 		ret = vdev->fops->mmap(filp, vm);
 	if (vdev->lock)
@@ -291,8 +307,11 @@ static int v4l2_open(struct inode *inode, struct file *filp)
 	video_get(vdev);
 	mutex_unlock(&videodev_lock);
 	if (vdev->fops->open) {
-		if (vdev->lock)
-			mutex_lock(vdev->lock);
+		if (vdev->lock) {
+			ret = mutex_lock_interruptible(vdev->lock);
+			if (ret)
+				goto err;
+		}
 		if (video_is_registered(vdev))
 			ret = vdev->fops->open(filp);
 		else
@@ -301,6 +320,7 @@ static int v4l2_open(struct inode *inode, struct file *filp)
 			mutex_unlock(vdev->lock);
 	}
 
+err:
 	/* decrease the refcount in case of an error */
 	if (ret)
 		video_put(vdev);
-- 
1.7.0.4

[RFC PATCH 2/8] BKL: trivial BKL removal from V4L2 radio drivers

[Hans Verkuil]

The patch converts a bunch of V4L2 radio drivers to unlocked_ioctl.

These are all simple conversions: most already had a lock and so the ioctl
fop could simply be replaced by unlocked_ioctl.

radio-miropcm20.c was converted to use the new V4L2 core lock.

While doing this work I noticed that many of these drivers initialized
some more fields or muted audio or something like that *after* creating
the device node. This should be done before the device node is created
to prevent problems. Especially hal tends to grab a device node as soon
as it is created.

In one or two cases the mutex_init was even done after the device creation!

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/radio/radio-aimslab.c    |   16 ++++++++--------
 drivers/media/radio/radio-aztech.c     |    6 +++---
 drivers/media/radio/radio-gemtek-pci.c |    6 +++---
 drivers/media/radio/radio-gemtek.c     |   14 +++++++-------
 drivers/media/radio/radio-maestro.c    |   14 ++++++--------
 drivers/media/radio/radio-maxiradio.c  |    2 +-
 drivers/media/radio/radio-miropcm20.c  |    6 ++++--
 drivers/media/radio/radio-rtrack2.c    |   10 +++++-----
 drivers/media/radio/radio-sf16fmi.c    |    7 ++++---
 drivers/media/radio/radio-sf16fmr2.c   |   11 +++++------
 drivers/media/radio/radio-terratec.c   |    8 ++++----
 drivers/media/radio/radio-trust.c      |   18 +++++++++---------
 drivers/media/radio/radio-zoltrix.c    |   30 +++++++++++++++---------------
 13 files changed, 74 insertions(+), 74 deletions(-)

diff --git a/drivers/media/radio/radio-aimslab.c b/drivers/media/radio/radio-aimslab.c
index 5bf4985..05e832f 100644
--- a/drivers/media/radio/radio-aimslab.c
+++ b/drivers/media/radio/radio-aimslab.c
@@ -361,7 +361,7 @@ static int vidioc_s_audio(struct file *file, void *priv,
 
 static const struct v4l2_file_operations rtrack_fops = {
 	.owner		= THIS_MODULE,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 };
 
 static const struct v4l2_ioctl_ops rtrack_ioctl_ops = {
@@ -412,13 +412,6 @@ static int __init rtrack_init(void)
 	rt->vdev.release = video_device_release_empty;
 	video_set_drvdata(&rt->vdev, rt);
 
-	if (video_register_device(&rt->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
-		v4l2_device_unregister(&rt->v4l2_dev);
-		release_region(rt->io, 2);
-		return -EINVAL;
-	}
-	v4l2_info(v4l2_dev, "AIMSlab RadioTrack/RadioReveal card driver.\n");
-
 	/* Set up the I/O locking */
 
 	mutex_init(&rt->lock);
@@ -430,6 +423,13 @@ static int __init rtrack_init(void)
 	sleep_delay(2000000);	/* make sure it's totally down	*/
 	outb(0xc0, rt->io);		/* steady volume, mute card	*/
 
+	if (video_register_device(&rt->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
+		v4l2_device_unregister(&rt->v4l2_dev);
+		release_region(rt->io, 2);
+		return -EINVAL;
+	}
+	v4l2_info(v4l2_dev, "AIMSlab RadioTrack/RadioReveal card driver.\n");
+
 	return 0;
 }
 
diff --git a/drivers/media/radio/radio-aztech.c b/drivers/media/radio/radio-aztech.c
index c223113..dd8a6ab 100644
--- a/drivers/media/radio/radio-aztech.c
+++ b/drivers/media/radio/radio-aztech.c
@@ -324,7 +324,7 @@ static int vidioc_s_ctrl(struct file *file, void *priv,
 
 static const struct v4l2_file_operations aztech_fops = {
 	.owner		= THIS_MODULE,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 };
 
 static const struct v4l2_ioctl_ops aztech_ioctl_ops = {
@@ -375,6 +375,8 @@ static int __init aztech_init(void)
 	az->vdev.ioctl_ops = &aztech_ioctl_ops;
 	az->vdev.release = video_device_release_empty;
 	video_set_drvdata(&az->vdev, az);
+	/* mute card - prevents noisy bootups */
+	outb(0, az->io);
 
 	if (video_register_device(&az->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
 		v4l2_device_unregister(v4l2_dev);
@@ -383,8 +385,6 @@ static int __init aztech_init(void)
 	}
 
 	v4l2_info(v4l2_dev, "Aztech radio card driver v1.00/19990224 rkroll@exploits.org\n");
-	/* mute card - prevents noisy bootups */
-	outb(0, az->io);
 	return 0;
 }
 
diff --git a/drivers/media/radio/radio-gemtek-pci.c b/drivers/media/radio/radio-gemtek-pci.c
index 7903967..28fa85b 100644
--- a/drivers/media/radio/radio-gemtek-pci.c
+++ b/drivers/media/radio/radio-gemtek-pci.c
@@ -361,7 +361,7 @@ MODULE_DEVICE_TABLE(pci, gemtek_pci_id);
 
 static const struct v4l2_file_operations gemtek_pci_fops = {
 	.owner		= THIS_MODULE,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 };
 
 static const struct v4l2_ioctl_ops gemtek_pci_ioctl_ops = {
@@ -422,11 +422,11 @@ static int __devinit gemtek_pci_probe(struct pci_dev *pdev, const struct pci_dev
 	card->vdev.release = video_device_release_empty;
 	video_set_drvdata(&card->vdev, card);
 
+	gemtek_pci_mute(card);
+
 	if (video_register_device(&card->vdev, VFL_TYPE_RADIO, nr_radio) < 0)
 		goto err_video;
 
-	gemtek_pci_mute(card);
-
 	v4l2_info(v4l2_dev, "Gemtek PCI Radio (rev. %d) found at 0x%04x-0x%04x.\n",
 		pdev->revision, card->iobase, card->iobase + card->length - 1);
 
diff --git a/drivers/media/radio/radio-gemtek.c b/drivers/media/radio/radio-gemtek.c
index 73985f6..2599364 100644
--- a/drivers/media/radio/radio-gemtek.c
+++ b/drivers/media/radio/radio-gemtek.c
@@ -378,7 +378,7 @@ static int gemtek_probe(struct gemtek *gt)
 
 static const struct v4l2_file_operations gemtek_fops = {
 	.owner		= THIS_MODULE,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 };
 
 static int vidioc_querycap(struct file *file, void *priv,
@@ -577,12 +577,6 @@ static int __init gemtek_init(void)
 	gt->vdev.release = video_device_release_empty;
 	video_set_drvdata(&gt->vdev, gt);
 
-	if (video_register_device(&gt->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
-		v4l2_device_unregister(v4l2_dev);
-		release_region(gt->io, 1);
-		return -EBUSY;
-	}
-
 	/* Set defaults */
 	gt->lastfreq = GEMTEK_LOWFREQ;
 	gt->bu2614data = 0;
@@ -590,6 +584,12 @@ static int __init gemtek_init(void)
 	if (initmute)
 		gemtek_mute(gt);
 
+	if (video_register_device(&gt->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
+		v4l2_device_unregister(v4l2_dev);
+		release_region(gt->io, 1);
+		return -EBUSY;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/media/radio/radio-maestro.c b/drivers/media/radio/radio-maestro.c
index 08f1051..6af61bf 100644
--- a/drivers/media/radio/radio-maestro.c
+++ b/drivers/media/radio/radio-maestro.c
@@ -299,7 +299,7 @@ static int vidioc_s_audio(struct file *file, void *priv,
 
 static const struct v4l2_file_operations maestro_fops = {
 	.owner		= THIS_MODULE,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 };
 
 static const struct v4l2_ioctl_ops maestro_ioctl_ops = {
@@ -383,22 +383,20 @@ static int __devinit maestro_probe(struct pci_dev *pdev,
 	dev->vdev.release = video_device_release_empty;
 	video_set_drvdata(&dev->vdev, dev);
 
+	if (!radio_power_on(dev)) {
+		retval = -EIO;
+		goto errfr1;
+	}
+
 	retval = video_register_device(&dev->vdev, VFL_TYPE_RADIO, radio_nr);
 	if (retval) {
 		v4l2_err(v4l2_dev, "can't register video device!\n");
 		goto errfr1;
 	}
 
-	if (!radio_power_on(dev)) {
-		retval = -EIO;
-		goto errunr;
-	}
-
 	v4l2_info(v4l2_dev, "version " DRIVER_VERSION "\n");
 
 	return 0;
-errunr:
-	video_unregister_device(&dev->vdev);
 errfr1:
 	v4l2_device_unregister(v4l2_dev);
 errfr:
diff --git a/drivers/media/radio/radio-maxiradio.c b/drivers/media/radio/radio-maxiradio.c
index 255d40d..6459a22 100644
--- a/drivers/media/radio/radio-maxiradio.c
+++ b/drivers/media/radio/radio-maxiradio.c
@@ -346,7 +346,7 @@ static int vidioc_s_ctrl(struct file *file, void *priv,
 
 static const struct v4l2_file_operations maxiradio_fops = {
 	.owner		= THIS_MODULE,
-	.ioctl          = video_ioctl2,
+	.unlocked_ioctl = video_ioctl2,
 };
 
 static const struct v4l2_ioctl_ops maxiradio_ioctl_ops = {
diff --git a/drivers/media/radio/radio-miropcm20.c b/drivers/media/radio/radio-miropcm20.c
index 4ff8854..3fb76e3 100644
--- a/drivers/media/radio/radio-miropcm20.c
+++ b/drivers/media/radio/radio-miropcm20.c
@@ -33,6 +33,7 @@ struct pcm20 {
 	unsigned long freq;
 	int muted;
 	struct snd_miro_aci *aci;
+	struct mutex lock;
 };
 
 static struct pcm20 pcm20_card = {
@@ -72,7 +73,7 @@ static int pcm20_setfreq(struct pcm20 *dev, unsigned long freq)
 
 static const struct v4l2_file_operations pcm20_fops = {
 	.owner		= THIS_MODULE,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 };
 
 static int vidioc_querycap(struct file *file, void *priv,
@@ -229,7 +230,7 @@ static int __init pcm20_init(void)
 		return -ENODEV;
 	}
 	strlcpy(v4l2_dev->name, "miropcm20", sizeof(v4l2_dev->name));
-
+	mutex_init(&dev->lock);
 
 	res = v4l2_device_register(NULL, v4l2_dev);
 	if (res < 0) {
@@ -242,6 +243,7 @@ static int __init pcm20_init(void)
 	dev->vdev.fops = &pcm20_fops;
 	dev->vdev.ioctl_ops = &pcm20_ioctl_ops;
 	dev->vdev.release = video_device_release_empty;
+	dev->vdev.lock = &dev->lock;
 	video_set_drvdata(&dev->vdev, dev);
 
 	if (video_register_device(&dev->vdev, VFL_TYPE_RADIO, radio_nr) < 0)
diff --git a/drivers/media/radio/radio-rtrack2.c b/drivers/media/radio/radio-rtrack2.c
index a79296a..8d6ea59 100644
--- a/drivers/media/radio/radio-rtrack2.c
+++ b/drivers/media/radio/radio-rtrack2.c
@@ -266,7 +266,7 @@ static int vidioc_s_audio(struct file *file, void *priv,
 
 static const struct v4l2_file_operations rtrack2_fops = {
 	.owner		= THIS_MODULE,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 };
 
 static const struct v4l2_ioctl_ops rtrack2_ioctl_ops = {
@@ -315,6 +315,10 @@ static int __init rtrack2_init(void)
 	dev->vdev.release = video_device_release_empty;
 	video_set_drvdata(&dev->vdev, dev);
 
+	/* mute card - prevents noisy bootups */
+	outb(1, dev->io);
+	dev->muted = 1;
+
 	mutex_init(&dev->lock);
 	if (video_register_device(&dev->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
 		v4l2_device_unregister(v4l2_dev);
@@ -324,10 +328,6 @@ static int __init rtrack2_init(void)
 
 	v4l2_info(v4l2_dev, "AIMSlab Radiotrack II card driver.\n");
 
-	/* mute card - prevents noisy bootups */
-	outb(1, dev->io);
-	dev->muted = 1;
-
 	return 0;
 }
 
diff --git a/drivers/media/radio/radio-sf16fmi.c b/drivers/media/radio/radio-sf16fmi.c
index 985359d..b5a5f89 100644
--- a/drivers/media/radio/radio-sf16fmi.c
+++ b/drivers/media/radio/radio-sf16fmi.c
@@ -260,7 +260,7 @@ static int vidioc_s_audio(struct file *file, void *priv,
 
 static const struct v4l2_file_operations fmi_fops = {
 	.owner		= THIS_MODULE,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 };
 
 static const struct v4l2_ioctl_ops fmi_ioctl_ops = {
@@ -382,6 +382,9 @@ static int __init fmi_init(void)
 
 	mutex_init(&fmi->lock);
 
+	/* mute card - prevents noisy bootups */
+	fmi_mute(fmi);
+
 	if (video_register_device(&fmi->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
 		v4l2_device_unregister(v4l2_dev);
 		release_region(fmi->io, 2);
@@ -391,8 +394,6 @@ static int __init fmi_init(void)
 	}
 
 	v4l2_info(v4l2_dev, "card driver at 0x%x\n", fmi->io);
-	/* mute card - prevents noisy bootups */
-	fmi_mute(fmi);
 	return 0;
 }
 
diff --git a/drivers/media/radio/radio-sf16fmr2.c b/drivers/media/radio/radio-sf16fmr2.c
index 52c7bbb..dc3f04c 100644
--- a/drivers/media/radio/radio-sf16fmr2.c
+++ b/drivers/media/radio/radio-sf16fmr2.c
@@ -376,7 +376,7 @@ static int vidioc_s_audio(struct file *file, void *priv,
 
 static const struct v4l2_file_operations fmr2_fops = {
 	.owner          = THIS_MODULE,
-	.ioctl          = video_ioctl2,
+	.unlocked_ioctl = video_ioctl2,
 };
 
 static const struct v4l2_ioctl_ops fmr2_ioctl_ops = {
@@ -424,6 +424,10 @@ static int __init fmr2_init(void)
 	fmr2->vdev.release = video_device_release_empty;
 	video_set_drvdata(&fmr2->vdev, fmr2);
 
+	/* mute card - prevents noisy bootups */
+	fmr2_mute(fmr2->io);
+	fmr2_product_info(fmr2);
+
 	if (video_register_device(&fmr2->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
 		v4l2_device_unregister(v4l2_dev);
 		release_region(fmr2->io, 2);
@@ -431,11 +435,6 @@ static int __init fmr2_init(void)
 	}
 
 	v4l2_info(v4l2_dev, "SF16FMR2 radio card driver at 0x%x.\n", fmr2->io);
-	/* mute card - prevents noisy bootups */
-	mutex_lock(&fmr2->lock);
-	fmr2_mute(fmr2->io);
-	fmr2_product_info(fmr2);
-	mutex_unlock(&fmr2->lock);
 	debug_print((KERN_DEBUG "card_type %d\n", fmr2->card_type));
 	return 0;
 }
diff --git a/drivers/media/radio/radio-terratec.c b/drivers/media/radio/radio-terratec.c
index fc1c860..a326639 100644
--- a/drivers/media/radio/radio-terratec.c
+++ b/drivers/media/radio/radio-terratec.c
@@ -338,7 +338,7 @@ static int vidioc_s_audio(struct file *file, void *priv,
 
 static const struct v4l2_file_operations terratec_fops = {
 	.owner		= THIS_MODULE,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 };
 
 static const struct v4l2_ioctl_ops terratec_ioctl_ops = {
@@ -389,6 +389,9 @@ static int __init terratec_init(void)
 
 	mutex_init(&tt->lock);
 
+	/* mute card - prevents noisy bootups */
+	tt_write_vol(tt, 0);
+
 	if (video_register_device(&tt->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
 		v4l2_device_unregister(&tt->v4l2_dev);
 		release_region(tt->io, 2);
@@ -396,9 +399,6 @@ static int __init terratec_init(void)
 	}
 
 	v4l2_info(v4l2_dev, "TERRATEC ActivRadio Standalone card driver.\n");
-
-	/* mute card - prevents noisy bootups */
-	tt_write_vol(tt, 0);
 	return 0;
 }
 
diff --git a/drivers/media/radio/radio-trust.c b/drivers/media/radio/radio-trust.c
index 9d6dcf8..22fa9cc 100644
--- a/drivers/media/radio/radio-trust.c
+++ b/drivers/media/radio/radio-trust.c
@@ -344,7 +344,7 @@ static int vidioc_s_audio(struct file *file, void *priv,
 
 static const struct v4l2_file_operations trust_fops = {
 	.owner		= THIS_MODULE,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 };
 
 static const struct v4l2_ioctl_ops trust_ioctl_ops = {
@@ -396,14 +396,6 @@ static int __init trust_init(void)
 	tr->vdev.release = video_device_release_empty;
 	video_set_drvdata(&tr->vdev, tr);
 
-	if (video_register_device(&tr->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
-		v4l2_device_unregister(v4l2_dev);
-		release_region(tr->io, 2);
-		return -EINVAL;
-	}
-
-	v4l2_info(v4l2_dev, "Trust FM Radio card driver v1.0.\n");
-
 	write_i2c(tr, 2, TDA7318_ADDR, 0x80);	/* speaker att. LF = 0 dB */
 	write_i2c(tr, 2, TDA7318_ADDR, 0xa0);	/* speaker att. RF = 0 dB */
 	write_i2c(tr, 2, TDA7318_ADDR, 0xc0);	/* speaker att. LR = 0 dB */
@@ -418,6 +410,14 @@ static int __init trust_init(void)
 	/* mute card - prevents noisy bootups */
 	tr_setmute(tr, 1);
 
+	if (video_register_device(&tr->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
+		v4l2_device_unregister(v4l2_dev);
+		release_region(tr->io, 2);
+		return -EINVAL;
+	}
+
+	v4l2_info(v4l2_dev, "Trust FM Radio card driver v1.0.\n");
+
 	return 0;
 }
 
diff --git a/drivers/media/radio/radio-zoltrix.c b/drivers/media/radio/radio-zoltrix.c
index f31eab9..af99c5b 100644
--- a/drivers/media/radio/radio-zoltrix.c
+++ b/drivers/media/radio/radio-zoltrix.c
@@ -377,7 +377,7 @@ static int vidioc_s_audio(struct file *file, void *priv,
 static const struct v4l2_file_operations zoltrix_fops =
 {
 	.owner		= THIS_MODULE,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 };
 
 static const struct v4l2_ioctl_ops zoltrix_ioctl_ops = {
@@ -424,20 +424,6 @@ static int __init zoltrix_init(void)
 		return res;
 	}
 
-	strlcpy(zol->vdev.name, v4l2_dev->name, sizeof(zol->vdev.name));
-	zol->vdev.v4l2_dev = v4l2_dev;
-	zol->vdev.fops = &zoltrix_fops;
-	zol->vdev.ioctl_ops = &zoltrix_ioctl_ops;
-	zol->vdev.release = video_device_release_empty;
-	video_set_drvdata(&zol->vdev, zol);
-
-	if (video_register_device(&zol->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
-		v4l2_device_unregister(v4l2_dev);
-		release_region(zol->io, 2);
-		return -EINVAL;
-	}
-	v4l2_info(v4l2_dev, "Zoltrix Radio Plus card driver.\n");
-
 	mutex_init(&zol->lock);
 
 	/* mute card - prevents noisy bootups */
@@ -452,6 +438,20 @@ static int __init zoltrix_init(void)
 	zol->curvol = 0;
 	zol->stereo = 1;
 
+	strlcpy(zol->vdev.name, v4l2_dev->name, sizeof(zol->vdev.name));
+	zol->vdev.v4l2_dev = v4l2_dev;
+	zol->vdev.fops = &zoltrix_fops;
+	zol->vdev.ioctl_ops = &zoltrix_ioctl_ops;
+	zol->vdev.release = video_device_release_empty;
+	video_set_drvdata(&zol->vdev, zol);
+
+	if (video_register_device(&zol->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
+		v4l2_device_unregister(v4l2_dev);
+		release_region(zol->io, 2);
+		return -EINVAL;
+	}
+	v4l2_info(v4l2_dev, "Zoltrix Radio Plus card driver.\n");
+
 	return 0;
 }
 
-- 
1.7.0.4

[RFC PATCH 3/8] cadet: use unlocked_ioctl

[Hans Verkuil]

Converted from ioctl to unlocked_ioctl.

This driver already used an internal lock, but it was missing in cadet_open and
cadet_release and it was not used correctly in cadet_read.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/radio/radio-cadet.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/media/radio/radio-cadet.c b/drivers/media/radio/radio-cadet.c
index b701ea6..bc9ad08 100644
--- a/drivers/media/radio/radio-cadet.c
+++ b/drivers/media/radio/radio-cadet.c
@@ -328,11 +328,10 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo
 	unsigned char readbuf[RDS_BUFFER];
 	int i = 0;
 
+	mutex_lock(&dev->lock);
 	if (dev->rdsstat == 0) {
-		mutex_lock(&dev->lock);
 		dev->rdsstat = 1;
 		outb(0x80, dev->io);        /* Select RDS fifo */
-		mutex_unlock(&dev->lock);
 		init_timer(&dev->readtimer);
 		dev->readtimer.function = cadet_handler;
 		dev->readtimer.data = (unsigned long)dev;
@@ -340,12 +339,15 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo
 		add_timer(&dev->readtimer);
 	}
 	if (dev->rdsin == dev->rdsout) {
+		mutex_unlock(&dev->lock);
 		if (file->f_flags & O_NONBLOCK)
 			return -EWOULDBLOCK;
 		interruptible_sleep_on(&dev->read_queue);
+		mutex_lock(&dev->lock);
 	}
 	while (i < count && dev->rdsin != dev->rdsout)
 		readbuf[i++] = dev->rdsbuf[dev->rdsout++];
+	mutex_unlock(&dev->lock);
 
 	if (copy_to_user(data, readbuf, i))
 		return -EFAULT;
@@ -525,9 +527,11 @@ static int cadet_open(struct file *file)
 {
 	struct cadet *dev = video_drvdata(file);
 
+	mutex_lock(&dev->lock);
 	dev->users++;
 	if (1 == dev->users)
 		init_waitqueue_head(&dev->read_queue);
+	mutex_unlock(&dev->lock);
 	return 0;
 }
 
@@ -535,11 +539,13 @@ static int cadet_release(struct file *file)
 {
 	struct cadet *dev = video_drvdata(file);
 
+	mutex_lock(&dev->lock);
 	dev->users--;
 	if (0 == dev->users) {
 		del_timer_sync(&dev->readtimer);
 		dev->rdsstat = 0;
 	}
+	mutex_unlock(&dev->lock);
 	return 0;
 }
 
@@ -559,7 +565,7 @@ static const struct v4l2_file_operations cadet_fops = {
 	.open		= cadet_open,
 	.release       	= cadet_release,
 	.read		= cadet_read,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 	.poll		= cadet_poll,
 };
 
-- 
1.7.0.4

[RFC PATCH 4/8] tea5764: convert to unlocked_ioctl

[Hans Verkuil]

Convert from ioctl to unlocked_ioctl using the v4l2 core lock.

Also removed the 'exclusive access' limitation. There was no need for it
and it violates the v4l2 spec as well.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/radio/radio-tea5764.c |   49 ++++++----------------------------
 1 files changed, 9 insertions(+), 40 deletions(-)

diff --git a/drivers/media/radio/radio-tea5764.c b/drivers/media/radio/radio-tea5764.c
index 789d2ec..0e71d81 100644
--- a/drivers/media/radio/radio-tea5764.c
+++ b/drivers/media/radio/radio-tea5764.c
@@ -142,7 +142,6 @@ struct tea5764_device {
 	struct video_device		*videodev;
 	struct tea5764_regs		regs;
 	struct mutex			mutex;
-	int				users;
 };
 
 /* I2C code related */
@@ -458,41 +457,10 @@ static int vidioc_s_audio(struct file *file, void *priv,
 	return 0;
 }
 
-static int tea5764_open(struct file *file)
-{
-	/* Currently we support only one device */
-	struct tea5764_device *radio = video_drvdata(file);
-
-	mutex_lock(&radio->mutex);
-	/* Only exclusive access */
-	if (radio->users) {
-		mutex_unlock(&radio->mutex);
-		return -EBUSY;
-	}
-	radio->users++;
-	mutex_unlock(&radio->mutex);
-	file->private_data = radio;
-	return 0;
-}
-
-static int tea5764_close(struct file *file)
-{
-	struct tea5764_device *radio = video_drvdata(file);
-
-	if (!radio)
-		return -ENODEV;
-	mutex_lock(&radio->mutex);
-	radio->users--;
-	mutex_unlock(&radio->mutex);
-	return 0;
-}
-
 /* File system interface */
 static const struct v4l2_file_operations tea5764_fops = {
 	.owner		= THIS_MODULE,
-	.open           = tea5764_open,
-	.release        = tea5764_close,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 };
 
 static const struct v4l2_ioctl_ops tea5764_ioctl_ops = {
@@ -527,7 +495,7 @@ static int __devinit tea5764_i2c_probe(struct i2c_client *client,
 	int ret;
 
 	PDEBUG("probe");
-	radio = kmalloc(sizeof(struct tea5764_device), GFP_KERNEL);
+	radio = kzalloc(sizeof(struct tea5764_device), GFP_KERNEL);
 	if (!radio)
 		return -ENOMEM;
 
@@ -555,12 +523,7 @@ static int __devinit tea5764_i2c_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, radio);
 	video_set_drvdata(radio->videodev, radio);
-
-	ret = video_register_device(radio->videodev, VFL_TYPE_RADIO, radio_nr);
-	if (ret < 0) {
-		PWARN("Could not register video device!");
-		goto errrel;
-	}
+	radio->videodev->lock = &radio->mutex;
 
 	/* initialize and power off the chip */
 	tea5764_i2c_read(radio);
@@ -568,6 +531,12 @@ static int __devinit tea5764_i2c_probe(struct i2c_client *client,
 	tea5764_mute(radio, 1);
 	tea5764_power_down(radio);
 
+	ret = video_register_device(radio->videodev, VFL_TYPE_RADIO, radio_nr);
+	if (ret < 0) {
+		PWARN("Could not register video device!");
+		goto errrel;
+	}
+
 	PINFO("registered.");
 	return 0;
 errrel:
-- 
1.7.0.4

[RFC PATCH 5/8] si4713: convert to unlocked_ioctl

[Hans Verkuil]

Convert ioctl to unlocked_ioctl. Note that for this driver the locking
is done inside the sub-device.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/radio/radio-si4713.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/media/radio/radio-si4713.c b/drivers/media/radio/radio-si4713.c
index 6a43578..3a84c3d 100644
--- a/drivers/media/radio/radio-si4713.c
+++ b/drivers/media/radio/radio-si4713.c
@@ -53,7 +53,8 @@ struct radio_si4713_device {
 /* radio_si4713_fops - file operations interface */
 static const struct v4l2_file_operations radio_si4713_fops = {
 	.owner		= THIS_MODULE,
-	.ioctl		= video_ioctl2,
+	/* Note: locking is done at the subdev level in the i2c driver. */
+	.unlocked_ioctl	= video_ioctl2,
 };
 
 /* Video4Linux Interface */
-- 
1.7.0.4

[RFC PATCH 6/8] typhoon: convert to unlocked_ioctl.

[Hans Verkuil]

Convert the typhoon driver from ioctl to unlocked_ioctl.

When doing this I noticed a bug where curfreq was not initialized correctly
to mutefreq (it wasn't multiplied by 16).

The initialization is now also done before the device node is created.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/radio/radio-typhoon.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/media/radio/radio-typhoon.c b/drivers/media/radio/radio-typhoon.c
index b1f6305..8dbbf08 100644
--- a/drivers/media/radio/radio-typhoon.c
+++ b/drivers/media/radio/radio-typhoon.c
@@ -317,7 +317,7 @@ static int vidioc_log_status(struct file *file, void *priv)
 
 static const struct v4l2_file_operations typhoon_fops = {
 	.owner		= THIS_MODULE,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 };
 
 static const struct v4l2_ioctl_ops typhoon_ioctl_ops = {
@@ -344,18 +344,18 @@ static int __init typhoon_init(void)
 
 	strlcpy(v4l2_dev->name, "typhoon", sizeof(v4l2_dev->name));
 	dev->io = io;
-	dev->curfreq = dev->mutefreq = mutefreq;
 
 	if (dev->io == -1) {
 		v4l2_err(v4l2_dev, "You must set an I/O address with io=0x316 or io=0x336\n");
 		return -EINVAL;
 	}
 
-	if (dev->mutefreq < 87000 || dev->mutefreq > 108500) {
+	if (mutefreq < 87000 || mutefreq > 108500) {
 		v4l2_err(v4l2_dev, "You must set a frequency (in kHz) used when muting the card,\n");
 		v4l2_err(v4l2_dev, "e.g. with \"mutefreq=87500\" (87000 <= mutefreq <= 108500)\n");
 		return -EINVAL;
 	}
+	dev->curfreq = dev->mutefreq = mutefreq << 4;
 
 	mutex_init(&dev->lock);
 	if (!request_region(dev->io, 8, "typhoon")) {
@@ -378,17 +378,17 @@ static int __init typhoon_init(void)
 	dev->vdev.ioctl_ops = &typhoon_ioctl_ops;
 	dev->vdev.release = video_device_release_empty;
 	video_set_drvdata(&dev->vdev, dev);
+
+	/* mute card - prevents noisy bootups */
+	typhoon_mute(dev);
+
 	if (video_register_device(&dev->vdev, VFL_TYPE_RADIO, radio_nr) < 0) {
 		v4l2_device_unregister(&dev->v4l2_dev);
 		release_region(dev->io, 8);
 		return -EINVAL;
 	}
 	v4l2_info(v4l2_dev, "port 0x%x.\n", dev->io);
-	v4l2_info(v4l2_dev, "mute frequency is %lu kHz.\n", dev->mutefreq);
-	dev->mutefreq <<= 4;
-
-	/* mute card - prevents noisy bootups */
-	typhoon_mute(dev);
+	v4l2_info(v4l2_dev, "mute frequency is %lu kHz.\n", mutefreq);
 
 	return 0;
 }
-- 
1.7.0.4

[RFC PATCH 7/8] dsbr100: convert to unlocked_ioctl.

[Hans Verkuil]

Trivial conversion to unlocked_ioctl.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/radio/dsbr100.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index ed9cd7a..c3e952f 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -605,7 +605,7 @@ static void usb_dsbr100_video_device_release(struct video_device *videodev)
 /* File system interface */
 static const struct v4l2_file_operations usb_dsbr100_fops = {
 	.owner		= THIS_MODULE,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 };
 
 static const struct v4l2_ioctl_ops usb_dsbr100_ioctl_ops = {
-- 
1.7.0.4

[RFC PATCH 8/8] BKL: trivial ioctl -> unlocked_ioctl video driver conversions

[Hans Verkuil]

These drivers could be trivially converted to unlocked_ioctl since they
already did locking.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/video/arv.c     |    2 +-
 drivers/media/video/bw-qcam.c |    2 +-
 drivers/media/video/c-qcam.c  |    2 +-
 drivers/media/video/meye.c    |   14 +++++++-------
 drivers/media/video/pms.c     |    2 +-
 drivers/media/video/w9966.c   |    2 +-
 6 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/media/video/arv.c b/drivers/media/video/arv.c
index 31e7a12..f989f28 100644
--- a/drivers/media/video/arv.c
+++ b/drivers/media/video/arv.c
@@ -712,7 +712,7 @@ static int ar_initialize(struct ar *ar)
 static const struct v4l2_file_operations ar_fops = {
 	.owner		= THIS_MODULE,
 	.read		= ar_read,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 };
 
 static const struct v4l2_ioctl_ops ar_ioctl_ops = {
diff --git a/drivers/media/video/bw-qcam.c b/drivers/media/video/bw-qcam.c
index 935e0c9..c119350 100644
--- a/drivers/media/video/bw-qcam.c
+++ b/drivers/media/video/bw-qcam.c
@@ -860,7 +860,7 @@ static ssize_t qcam_read(struct file *file, char __user *buf,
 
 static const struct v4l2_file_operations qcam_fops = {
 	.owner		= THIS_MODULE,
-	.ioctl          = video_ioctl2,
+	.unlocked_ioctl = video_ioctl2,
 	.read		= qcam_read,
 };
 
diff --git a/drivers/media/video/c-qcam.c b/drivers/media/video/c-qcam.c
index 6e4b196..24fc009 100644
--- a/drivers/media/video/c-qcam.c
+++ b/drivers/media/video/c-qcam.c
@@ -718,7 +718,7 @@ static ssize_t qcam_read(struct file *file, char __user *buf,
 
 static const struct v4l2_file_operations qcam_fops = {
 	.owner		= THIS_MODULE,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 	.read		= qcam_read,
 };
 
diff --git a/drivers/media/video/meye.c b/drivers/media/video/meye.c
index 2be23bc..48d2c24 100644
--- a/drivers/media/video/meye.c
+++ b/drivers/media/video/meye.c
@@ -1659,7 +1659,7 @@ static const struct v4l2_file_operations meye_fops = {
 	.open		= meye_open,
 	.release	= meye_release,
 	.mmap		= meye_mmap,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 	.poll		= meye_poll,
 };
 
@@ -1831,12 +1831,6 @@ static int __devinit meye_probe(struct pci_dev *pcidev,
 	msleep(1);
 	mchip_set(MCHIP_MM_INTA, MCHIP_MM_INTA_HIC_1_MASK);
 
-	if (video_register_device(meye.vdev, VFL_TYPE_GRABBER,
-				  video_nr) < 0) {
-		v4l2_err(v4l2_dev, "video_register_device failed\n");
-		goto outvideoreg;
-	}
-
 	mutex_init(&meye.lock);
 	init_waitqueue_head(&meye.proc_list);
 	meye.brightness = 32 << 10;
@@ -1858,6 +1852,12 @@ static int __devinit meye_probe(struct pci_dev *pcidev,
 	sony_pic_camera_command(SONY_PIC_COMMAND_SETCAMERAPICTURE, 0);
 	sony_pic_camera_command(SONY_PIC_COMMAND_SETCAMERAAGC, 48);
 
+	if (video_register_device(meye.vdev, VFL_TYPE_GRABBER,
+				  video_nr) < 0) {
+		v4l2_err(v4l2_dev, "video_register_device failed\n");
+		goto outvideoreg;
+	}
+
 	v4l2_info(v4l2_dev, "Motion Eye Camera Driver v%s.\n",
 	       MEYE_DRIVER_VERSION);
 	v4l2_info(v4l2_dev, "mchip KL5A72002 rev. %d, base %lx, irq %d\n",
diff --git a/drivers/media/video/pms.c b/drivers/media/video/pms.c
index 7129b50..7551907 100644
--- a/drivers/media/video/pms.c
+++ b/drivers/media/video/pms.c
@@ -932,7 +932,7 @@ static ssize_t pms_read(struct file *file, char __user *buf,
 
 static const struct v4l2_file_operations pms_fops = {
 	.owner		= THIS_MODULE,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= video_ioctl2,
 	.read           = pms_read,
 };
 
diff --git a/drivers/media/video/w9966.c b/drivers/media/video/w9966.c
index 635420d..019ee20 100644
--- a/drivers/media/video/w9966.c
+++ b/drivers/media/video/w9966.c
@@ -815,7 +815,7 @@ out:
 
 static const struct v4l2_file_operations w9966_fops = {
 	.owner		= THIS_MODULE,
-	.ioctl          = video_ioctl2,
+	.unlocked_ioctl = video_ioctl2,
 	.read           = w9966_v4l_read,
 };
 
-- 
1.7.0.4

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Arnd Bergmann]

On Sunday 14 November 2010, Hans Verkuil wrote:
> This patch series converts 24 v4l drivers to unlocked_ioctl. These are low
> hanging fruit but you have to start somewhere :-)
> 
> The first patch replaces mutex_lock in the V4L2 core by mutex_lock_interruptible
> for most fops.

The patches all look good as far as I can tell, but I suppose the title is
obsolete now that the BKL has been replaced with a v4l-wide mutex, which
is what you are removing in the series.

	Arnd

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Hans Verkuil]

On Sunday, November 14, 2010 22:53:29 Arnd Bergmann wrote:
> On Sunday 14 November 2010, Hans Verkuil wrote:
> > This patch series converts 24 v4l drivers to unlocked_ioctl. These are low
> > hanging fruit but you have to start somewhere :-)
> > 
> > The first patch replaces mutex_lock in the V4L2 core by mutex_lock_interruptible
> > for most fops.
> 
> The patches all look good as far as I can tell, but I suppose the title is
> obsolete now that the BKL has been replaced with a v4l-wide mutex, which
> is what you are removing in the series.

I guess I have to rename it, even though strictly speaking the branch I'm
working in doesn't have your patch merged yet.

BTW, replacing the BKL with a static mutex is rather scary: the BKL gives up
the lock whenever you sleep, the mutex doesn't. Since sleeping is very common
in V4L (calling VIDIOC_DQBUF will typically sleep while waiting for a new frame
to arrive), this will make it impossible for another process to access *any*
v4l2 device node while the ioctl is sleeping.

I am not sure whether that is what you intended. Or am I missing something?

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Arnd Bergmann]

On Sunday 14 November 2010 23:48:51 Hans Verkuil wrote:
> On Sunday, November 14, 2010 22:53:29 Arnd Bergmann wrote:
> > On Sunday 14 November 2010, Hans Verkuil wrote:
> > > This patch series converts 24 v4l drivers to unlocked_ioctl. These are low
> > > hanging fruit but you have to start somewhere :-)
> > > 
> > > The first patch replaces mutex_lock in the V4L2 core by mutex_lock_interruptible
> > > for most fops.
> > 
> > The patches all look good as far as I can tell, but I suppose the title is
> > obsolete now that the BKL has been replaced with a v4l-wide mutex, which
> > is what you are removing in the series.
> 
> I guess I have to rename it, even though strictly speaking the branch I'm
> working in doesn't have your patch merged yet.
> 
> BTW, replacing the BKL with a static mutex is rather scary: the BKL gives up
> the lock whenever you sleep, the mutex doesn't. Since sleeping is very common
> in V4L (calling VIDIOC_DQBUF will typically sleep while waiting for a new frame
> to arrive), this will make it impossible for another process to access any
> v4l2 device node while the ioctl is sleeping.
> 
> I am not sure whether that is what you intended. Or am I missing something?

I was aware that something like this could happen, but I apparently
misjudged how big the impact is. The general pattern for ioctls is that
those that get called frequently do not sleep, so it can almost always be
called with a mutex held.

	Arnd

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Hans Verkuil]

> On Sunday 14 November 2010 23:48:51 Hans Verkuil wrote:
>> On Sunday, November 14, 2010 22:53:29 Arnd Bergmann wrote:
>> > On Sunday 14 November 2010, Hans Verkuil wrote:
>> > > This patch series converts 24 v4l drivers to unlocked_ioctl. These
>> are low
>> > > hanging fruit but you have to start somewhere :-)
>> > >
>> > > The first patch replaces mutex_lock in the V4L2 core by
>> mutex_lock_interruptible
>> > > for most fops.
>> >
>> > The patches all look good as far as I can tell, but I suppose the
>> title is
>> > obsolete now that the BKL has been replaced with a v4l-wide mutex,
>> which
>> > is what you are removing in the series.
>>
>> I guess I have to rename it, even though strictly speaking the branch
>> I'm
>> working in doesn't have your patch merged yet.
>>
>> BTW, replacing the BKL with a static mutex is rather scary: the BKL
>> gives up
>> the lock whenever you sleep, the mutex doesn't. Since sleeping is very
>> common
>> in V4L (calling VIDIOC_DQBUF will typically sleep while waiting for a
>> new frame
>> to arrive), this will make it impossible for another process to access
>> any
>> v4l2 device node while the ioctl is sleeping.
>>
>> I am not sure whether that is what you intended. Or am I missing
>> something?
>
> I was aware that something like this could happen, but I apparently
> misjudged how big the impact is. The general pattern for ioctls is that
> those that get called frequently do not sleep, so it can almost always be
> called with a mutex held.

True in general, but most definitely not true for V4L. The all important
VIDIOC_DQBUF ioctl will almost always sleep.

Mauro, I think this patch will have to be reverted and we just have to do
the hard work ourselves.

Regards,

       Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Mauro Carvalho Chehab]

Em 15-11-2010 07:49, Hans Verkuil escreveu:
> 
>> On Sunday 14 November 2010 23:48:51 Hans Verkuil wrote:
>>> On Sunday, November 14, 2010 22:53:29 Arnd Bergmann wrote:
>>>> On Sunday 14 November 2010, Hans Verkuil wrote:
>>>>> This patch series converts 24 v4l drivers to unlocked_ioctl. These
>>> are low
>>>>> hanging fruit but you have to start somewhere :-)
>>>>>
>>>>> The first patch replaces mutex_lock in the V4L2 core by
>>> mutex_lock_interruptible
>>>>> for most fops.
>>>>
>>>> The patches all look good as far as I can tell, but I suppose the
>>> title is
>>>> obsolete now that the BKL has been replaced with a v4l-wide mutex,
>>> which
>>>> is what you are removing in the series.
>>>
>>> I guess I have to rename it, even though strictly speaking the branch
>>> I'm
>>> working in doesn't have your patch merged yet.
>>>
>>> BTW, replacing the BKL with a static mutex is rather scary: the BKL
>>> gives up
>>> the lock whenever you sleep, the mutex doesn't. Since sleeping is very
>>> common
>>> in V4L (calling VIDIOC_DQBUF will typically sleep while waiting for a
>>> new frame
>>> to arrive), this will make it impossible for another process to access
>>> any
>>> v4l2 device node while the ioctl is sleeping.
>>>
>>> I am not sure whether that is what you intended. Or am I missing
>>> something?
>>
>> I was aware that something like this could happen, but I apparently
>> misjudged how big the impact is. The general pattern for ioctls is that
>> those that get called frequently do not sleep, so it can almost always be
>> called with a mutex held.
> 
> True in general, but most definitely not true for V4L. The all important
> VIDIOC_DQBUF ioctl will almost always sleep.
> 
> Mauro, I think this patch will have to be reverted and we just have to do
> the hard work ourselves.

The VIDIOC_QBUF/VIDIOC_DQBUF ioctls are called after having the V4L device ready
for stream. During the qbuf/dqbuf loop, the only other ioctls that may appear are
the control change ioctl's, to adjust things like bright. I doubt that his will
cause a really serious trouble.

On the other hand, currently, if BKL is disabled, the entire V4L subsystem is
disabled.

So, IMO, the impact of having Arnd's patch applied is less than just having
almost all drivers disabled if BKL is not compiled. So, I prefer to apply 
his patch and then fix it, driver by driver, than to disable the entire
subsystem on .37.

Cheers,
Mauro

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Hans Verkuil]

> Em 15-11-2010 07:49, Hans Verkuil escreveu:
>>
>>> On Sunday 14 November 2010 23:48:51 Hans Verkuil wrote:
>>>> On Sunday, November 14, 2010 22:53:29 Arnd Bergmann wrote:
>>>>> On Sunday 14 November 2010, Hans Verkuil wrote:
>>>>>> This patch series converts 24 v4l drivers to unlocked_ioctl. These
>>>> are low
>>>>>> hanging fruit but you have to start somewhere :-)
>>>>>>
>>>>>> The first patch replaces mutex_lock in the V4L2 core by
>>>> mutex_lock_interruptible
>>>>>> for most fops.
>>>>>
>>>>> The patches all look good as far as I can tell, but I suppose the
>>>> title is
>>>>> obsolete now that the BKL has been replaced with a v4l-wide mutex,
>>>> which
>>>>> is what you are removing in the series.
>>>>
>>>> I guess I have to rename it, even though strictly speaking the branch
>>>> I'm
>>>> working in doesn't have your patch merged yet.
>>>>
>>>> BTW, replacing the BKL with a static mutex is rather scary: the BKL
>>>> gives up
>>>> the lock whenever you sleep, the mutex doesn't. Since sleeping is very
>>>> common
>>>> in V4L (calling VIDIOC_DQBUF will typically sleep while waiting for a
>>>> new frame
>>>> to arrive), this will make it impossible for another process to access
>>>> any
>>>> v4l2 device node while the ioctl is sleeping.
>>>>
>>>> I am not sure whether that is what you intended. Or am I missing
>>>> something?
>>>
>>> I was aware that something like this could happen, but I apparently
>>> misjudged how big the impact is. The general pattern for ioctls is that
>>> those that get called frequently do not sleep, so it can almost always
>>> be
>>> called with a mutex held.
>>
>> True in general, but most definitely not true for V4L. The all important
>> VIDIOC_DQBUF ioctl will almost always sleep.
>>
>> Mauro, I think this patch will have to be reverted and we just have to
>> do
>> the hard work ourselves.
>
> The VIDIOC_QBUF/VIDIOC_DQBUF ioctls are called after having the V4L device
> ready
> for stream. During the qbuf/dqbuf loop, the only other ioctls that may
> appear are
> the control change ioctl's, to adjust things like bright. I doubt that his
> will
> cause a really serious trouble.

Yes, it does. Anyone who is using multiple capture/output devices at the
same time will be affected. For example, anyone who uses the davinci
dm6467 driver for both input and output. And yes, that's what we use at
work. And ship to thousands of customers. Or think about surveillance
applications where you are capturing from many streams simultaneously.

This change *does* cause serious trouble.

>
> On the other hand, currently, if BKL is disabled, the entire V4L subsystem
> is
> disabled.
>
> So, IMO, the impact of having Arnd's patch applied is less than just
> having
> almost all drivers disabled if BKL is not compiled. So, I prefer to apply
> his patch and then fix it, driver by driver, than to disable the entire
> subsystem on .37.

Can't we test for CONFIG_LOCK_KERNEL and switch to lock_kernel if set? For
now it is just a kernel config option.

That seems much more reasonable.

Regards,

          Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Mauro Carvalho Chehab]

Em 16-11-2010 10:35, Hans Verkuil escreveu:
> 
>> Em 15-11-2010 07:49, Hans Verkuil escreveu:
>>>
>>>> On Sunday 14 November 2010 23:48:51 Hans Verkuil wrote:
>>>>> On Sunday, November 14, 2010 22:53:29 Arnd Bergmann wrote:
>>>>>> On Sunday 14 November 2010, Hans Verkuil wrote:
>>>>>>> This patch series converts 24 v4l drivers to unlocked_ioctl. These
>>>>> are low
>>>>>>> hanging fruit but you have to start somewhere :-)
>>>>>>>
>>>>>>> The first patch replaces mutex_lock in the V4L2 core by
>>>>> mutex_lock_interruptible
>>>>>>> for most fops.
>>>>>>
>>>>>> The patches all look good as far as I can tell, but I suppose the
>>>>> title is
>>>>>> obsolete now that the BKL has been replaced with a v4l-wide mutex,
>>>>> which
>>>>>> is what you are removing in the series.
>>>>>
>>>>> I guess I have to rename it, even though strictly speaking the branch
>>>>> I'm
>>>>> working in doesn't have your patch merged yet.
>>>>>
>>>>> BTW, replacing the BKL with a static mutex is rather scary: the BKL
>>>>> gives up
>>>>> the lock whenever you sleep, the mutex doesn't. Since sleeping is very
>>>>> common
>>>>> in V4L (calling VIDIOC_DQBUF will typically sleep while waiting for a
>>>>> new frame
>>>>> to arrive), this will make it impossible for another process to access
>>>>> any
>>>>> v4l2 device node while the ioctl is sleeping.
>>>>>
>>>>> I am not sure whether that is what you intended. Or am I missing
>>>>> something?
>>>>
>>>> I was aware that something like this could happen, but I apparently
>>>> misjudged how big the impact is. The general pattern for ioctls is that
>>>> those that get called frequently do not sleep, so it can almost always
>>>> be
>>>> called with a mutex held.
>>>
>>> True in general, but most definitely not true for V4L. The all important
>>> VIDIOC_DQBUF ioctl will almost always sleep.
>>>
>>> Mauro, I think this patch will have to be reverted and we just have to
>>> do
>>> the hard work ourselves.
>>
>> The VIDIOC_QBUF/VIDIOC_DQBUF ioctls are called after having the V4L device
>> ready
>> for stream. During the qbuf/dqbuf loop, the only other ioctls that may
>> appear are
>> the control change ioctl's, to adjust things like bright. I doubt that his
>> will
>> cause a really serious trouble.
> 
> Yes, it does. Anyone who is using multiple capture/output devices at the
> same time will be affected.

One correction to your comment:
	"Anyone that uses multiple capture/output devices that were not converted to unlocked ioctl will be affected."
This means that devices with multiple entries need to be fixed first.

> For example, anyone who uses the davinci
> dm6467 driver for both input and output. And yes, that's what we use at
> work. And ship to thousands of customers. Or think about surveillance
> applications where you are capturing from many streams simultaneously.

Cheers,
Mauro

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Hans Verkuil]

> Em 16-11-2010 10:35, Hans Verkuil escreveu:
>>
>>> Em 15-11-2010 07:49, Hans Verkuil escreveu:
>>>>
>>>>> On Sunday 14 November 2010 23:48:51 Hans Verkuil wrote:
>>>>>> On Sunday, November 14, 2010 22:53:29 Arnd Bergmann wrote:
>>>>>>> On Sunday 14 November 2010, Hans Verkuil wrote:
>>>>>>>> This patch series converts 24 v4l drivers to unlocked_ioctl. These
>>>>>> are low
>>>>>>>> hanging fruit but you have to start somewhere :-)
>>>>>>>>
>>>>>>>> The first patch replaces mutex_lock in the V4L2 core by
>>>>>> mutex_lock_interruptible
>>>>>>>> for most fops.
>>>>>>>
>>>>>>> The patches all look good as far as I can tell, but I suppose the
>>>>>> title is
>>>>>>> obsolete now that the BKL has been replaced with a v4l-wide mutex,
>>>>>> which
>>>>>>> is what you are removing in the series.
>>>>>>
>>>>>> I guess I have to rename it, even though strictly speaking the
>>>>>> branch
>>>>>> I'm
>>>>>> working in doesn't have your patch merged yet.
>>>>>>
>>>>>> BTW, replacing the BKL with a static mutex is rather scary: the BKL
>>>>>> gives up
>>>>>> the lock whenever you sleep, the mutex doesn't. Since sleeping is
>>>>>> very
>>>>>> common
>>>>>> in V4L (calling VIDIOC_DQBUF will typically sleep while waiting for
>>>>>> a
>>>>>> new frame
>>>>>> to arrive), this will make it impossible for another process to
>>>>>> access
>>>>>> any
>>>>>> v4l2 device node while the ioctl is sleeping.
>>>>>>
>>>>>> I am not sure whether that is what you intended. Or am I missing
>>>>>> something?
>>>>>
>>>>> I was aware that something like this could happen, but I apparently
>>>>> misjudged how big the impact is. The general pattern for ioctls is
>>>>> that
>>>>> those that get called frequently do not sleep, so it can almost
>>>>> always
>>>>> be
>>>>> called with a mutex held.
>>>>
>>>> True in general, but most definitely not true for V4L. The all
>>>> important
>>>> VIDIOC_DQBUF ioctl will almost always sleep.
>>>>
>>>> Mauro, I think this patch will have to be reverted and we just have to
>>>> do
>>>> the hard work ourselves.
>>>
>>> The VIDIOC_QBUF/VIDIOC_DQBUF ioctls are called after having the V4L
>>> device
>>> ready
>>> for stream. During the qbuf/dqbuf loop, the only other ioctls that may
>>> appear are
>>> the control change ioctl's, to adjust things like bright. I doubt that
>>> his
>>> will
>>> cause a really serious trouble.
>>
>> Yes, it does. Anyone who is using multiple capture/output devices at the
>> same time will be affected.
>
> One correction to your comment:
> 	"Anyone that uses multiple capture/output devices that were not converted
> to unlocked ioctl will be affected."
> This means that devices with multiple entries need to be fixed first.

No, it will also affect e.g. two bttv cards that you capture from in
parallel. Or two webcams, or...

We can't just ditch the BKL yet for 2.6.37 IMHO. Perhaps for 2.6.38 if we
all work really hard to convert everything.

I would prefer to have those drivers that depend on the BKL to have a
dependency on CONFIG_LOCK_KERNEL. Drivers should either work or not be
available at all, rather than working poorly (if at all).

Regards,

          Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Arnd Bergmann]

On Tuesday 16 November 2010, Hans Verkuil wrote:
> No, it will also affect e.g. two bttv cards that you capture from in
> parallel. Or two webcams, or...

Would it be safe to turn the global mutex into a per-driver or per-device
mutex? That would largely mitigate the impact as far as I can tell.

> We can't just ditch the BKL yet for 2.6.37 IMHO. Perhaps for 2.6.38 if we
> all work really hard to convert everything.

Linus was pretty clear in that he wanted to make the default for the BKL
disabled for 2.6.37. That may of course change if there are significant
problems with this, but as long as there is an easier way, we could do
that instead.

I have not tested the patch below, but maybe that would solve the
immediate problem without reverting v4l2-dev back to use the BKL.

It would not work if we have drivers that need to serialize access
to multiple v4l2 devices in their ioctl functions because they access
global data, which is unlikely but possible.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 03f7f46..5873d12 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -246,12 +246,11 @@ static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			mutex_unlock(vdev->lock);
 	} else if (vdev->fops->ioctl) {
 		/* TODO: convert all drivers to unlocked_ioctl */
-		static DEFINE_MUTEX(v4l2_ioctl_mutex);
-
-		mutex_lock(&v4l2_ioctl_mutex);
-		if (video_is_registered(vdev))
+		if (video_is_registered(vdev)) {
+			mutex_lock(&vdev->ioctl_lock);
 			ret = vdev->fops->ioctl(filp, cmd, arg);
-		mutex_unlock(&v4l2_ioctl_mutex);
+			mutex_unlock(&vdev->ioctl_lock);
+		}
 	} else
 		ret = -ENOTTY;
 
@@ -507,6 +506,7 @@ static int __video_register_device(struct video_device *vdev, int type, int nr,
 #endif
 	vdev->minor = i + minor_offset;
 	vdev->num = nr;
+	mutex_init(&vdev->ioctl_lock);
 	devnode_set(vdev);
 
 	/* Should not happen since we thought this minor was free */
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 15802a0..e8a8485 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -97,6 +97,9 @@ struct video_device
 
 	/* serialization lock */
 	struct mutex *lock;
+
+	/* used for the legacy locked ioctl */
+	struct mutex ioctl_lock;
 };
 
 /* dev to video-device */

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Hans Verkuil]

> On Tuesday 16 November 2010, Hans Verkuil wrote:
>> No, it will also affect e.g. two bttv cards that you capture from in
>> parallel. Or two webcams, or...
>
> Would it be safe to turn the global mutex into a per-driver or per-device
> mutex? That would largely mitigate the impact as far as I can tell.

What would work better is to add a mutex to struct v4l2_device (which is
the top-level struct for v4l devices) and have the core lock that.

A pointer to this struct is available in vdev->v4l2_dev. However, not all
drivers implement struct v4l2_device. But on the other hand, most relevant
drivers do. So as a fallback we would still need a static mutex.

What would be best is to add an #ifdef CONFIG_LOCK_KERNEL and use the BKL
there. In the #else we can use a v4l2_device mutex, or (if not set) a
static mutex.

Hardly elegant, but it'll have to do.

>> We can't just ditch the BKL yet for 2.6.37 IMHO. Perhaps for 2.6.38 if
>> we
>> all work really hard to convert everything.
>
> Linus was pretty clear in that he wanted to make the default for the BKL
> disabled for 2.6.37. That may of course change if there are significant
> problems with this, but as long as there is an easier way, we could do
> that instead.
>
> I have not tested the patch below, but maybe that would solve the
> immediate problem without reverting v4l2-dev back to use the BKL.
>
> It would not work if we have drivers that need to serialize access
> to multiple v4l2 devices in their ioctl functions because they access
> global data, which is unlikely but possible.

It's very likely, I'm afraid :-(

Regards,

       Hans

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> diff --git a/drivers/media/video/v4l2-dev.c
> b/drivers/media/video/v4l2-dev.c
> index 03f7f46..5873d12 100644
> --- a/drivers/media/video/v4l2-dev.c
> +++ b/drivers/media/video/v4l2-dev.c
> @@ -246,12 +246,11 @@ static long v4l2_ioctl(struct file *filp, unsigned
> int cmd, unsigned long arg)
>  			mutex_unlock(vdev->lock);
>  	} else if (vdev->fops->ioctl) {
>  		/* TODO: convert all drivers to unlocked_ioctl */
> -		static DEFINE_MUTEX(v4l2_ioctl_mutex);
> -
> -		mutex_lock(&v4l2_ioctl_mutex);
> -		if (video_is_registered(vdev))
> +		if (video_is_registered(vdev)) {
> +			mutex_lock(&vdev->ioctl_lock);
>  			ret = vdev->fops->ioctl(filp, cmd, arg);
> -		mutex_unlock(&v4l2_ioctl_mutex);
> +			mutex_unlock(&vdev->ioctl_lock);
> +		}
>  	} else
>  		ret = -ENOTTY;
>
> @@ -507,6 +506,7 @@ static int __video_register_device(struct video_device
> *vdev, int type, int nr,
>  #endif
>  	vdev->minor = i + minor_offset;
>  	vdev->num = nr;
> +	mutex_init(&vdev->ioctl_lock);
>  	devnode_set(vdev);
>
>  	/* Should not happen since we thought this minor was free */
> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> index 15802a0..e8a8485 100644
> --- a/include/media/v4l2-dev.h
> +++ b/include/media/v4l2-dev.h
> @@ -97,6 +97,9 @@ struct video_device
>
>  	/* serialization lock */
>  	struct mutex *lock;
> +
> +	/* used for the legacy locked ioctl */
> +	struct mutex ioctl_lock;
>  };
>
>  /* dev to video-device */
>


-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Arnd Bergmann]

On Tuesday 16 November 2010, Hans Verkuil wrote:
> A pointer to this struct is available in vdev->v4l2_dev. However, not all
> drivers implement struct v4l2_device. But on the other hand, most relevant
> drivers do. So as a fallback we would still need a static mutex.

Wouldn't that suffer the same problem as putting the mutex into videodev
as I suggested? You said that there are probably drivers that need to
serialize between multiple devices, so if we have a mutex per v4l2_device,
you can still get races between multiple ioctl calls accessing the same
per-driver data. To solve this, we'd have to put the lock into a per-driver
structure like v4l2_file_operations or v4l2_ioctl_ops, which would add
to the ugliness.

	Arnd

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Hans Verkuil]

> On Tuesday 16 November 2010, Hans Verkuil wrote:
>> A pointer to this struct is available in vdev->v4l2_dev. However, not
>> all
>> drivers implement struct v4l2_device. But on the other hand, most
>> relevant
>> drivers do. So as a fallback we would still need a static mutex.
>
> Wouldn't that suffer the same problem as putting the mutex into videodev
> as I suggested? You said that there are probably drivers that need to
> serialize between multiple devices, so if we have a mutex per v4l2_device,
> you can still get races between multiple ioctl calls accessing the same
> per-driver data. To solve this, we'd have to put the lock into a
> per-driver
> structure like v4l2_file_operations or v4l2_ioctl_ops, which would add
> to the ugliness.

I think there is a misunderstanding. One V4L device (e.g. a TV capture
card, a webcam, etc.) has one v4l2_device struct. But it can have multiple
V4L device nodes (/dev/video0, /dev/radio0, etc.), each represented by a
struct video_device (and I really hope I can rename that to v4l2_devnode
soon since that's a very confusing name).

You typically need to serialize between all the device nodes belonging to
the same video hardware. A mutex in struct video_device doesn't do that,
that just serializes access to that single device node. But a mutex in
v4l2_device is at the right level.

      Hans

>
> 	Arnd
>


-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Hans Verkuil]

>
>> On Tuesday 16 November 2010, Hans Verkuil wrote:
>>> A pointer to this struct is available in vdev->v4l2_dev. However, not
>>> all
>>> drivers implement struct v4l2_device. But on the other hand, most
>>> relevant
>>> drivers do. So as a fallback we would still need a static mutex.
>>
>> Wouldn't that suffer the same problem as putting the mutex into videodev
>> as I suggested? You said that there are probably drivers that need to
>> serialize between multiple devices, so if we have a mutex per
>> v4l2_device,
>> you can still get races between multiple ioctl calls accessing the same
>> per-driver data. To solve this, we'd have to put the lock into a
>> per-driver
>> structure like v4l2_file_operations or v4l2_ioctl_ops, which would add
>> to the ugliness.
>
> I think there is a misunderstanding. One V4L device (e.g. a TV capture
> card, a webcam, etc.) has one v4l2_device struct. But it can have multiple
> V4L device nodes (/dev/video0, /dev/radio0, etc.), each represented by a
> struct video_device (and I really hope I can rename that to v4l2_devnode
> soon since that's a very confusing name).
>
> You typically need to serialize between all the device nodes belonging to
> the same video hardware. A mutex in struct video_device doesn't do that,
> that just serializes access to that single device node. But a mutex in
> v4l2_device is at the right level.

A quick follow-up as I saw I didn't fully answer your question: to my
knowledge there are no per-driver data structures that need a BKL for
protection. It's definitely not something I am worried about.

Regards,

         Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Arnd Bergmann]

On Tuesday 16 November 2010, Hans Verkuil wrote:
> > I think there is a misunderstanding. One V4L device (e.g. a TV capture
> > card, a webcam, etc.) has one v4l2_device struct. But it can have multiple
> > V4L device nodes (/dev/video0, /dev/radio0, etc.), each represented by a
> > struct video_device (and I really hope I can rename that to v4l2_devnode
> > soon since that's a very confusing name).
> >
> > You typically need to serialize between all the device nodes belonging to
> > the same video hardware. A mutex in struct video_device doesn't do that,
> > that just serializes access to that single device node. But a mutex in
> > v4l2_device is at the right level.

Ok, got it now.

> A quick follow-up as I saw I didn't fully answer your question: to my
> knowledge there are no per-driver data structures that need a BKL for
> protection. It's definitely not something I am worried about.

Good. Are you preparing a patch for a per-v4l2_device then? This sounds
like the right place with your explanation. I would not put in the
CONFIG_BKL switch, because I tried that for two other subsystems and got
called back, but I'm not going to stop you.

As for the fallback to a global mutex, I guess you can set the
videodev->lock pointer and use unlocked_ioctl for those drivers
that do not use a v4l2_device yet, if there are only a handful of them.

	Arnd

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Mauro Carvalho Chehab]

Em 16-11-2010 14:01, Arnd Bergmann escreveu:
> On Tuesday 16 November 2010, Hans Verkuil wrote:
>>> I think there is a misunderstanding. One V4L device (e.g. a TV capture
>>> card, a webcam, etc.) has one v4l2_device struct. But it can have multiple
>>> V4L device nodes (/dev/video0, /dev/radio0, etc.), each represented by a
>>> struct video_device (and I really hope I can rename that to v4l2_devnode
>>> soon since that's a very confusing name).
>>>
>>> You typically need to serialize between all the device nodes belonging to
>>> the same video hardware. A mutex in struct video_device doesn't do that,
>>> that just serializes access to that single device node. But a mutex in
>>> v4l2_device is at the right level.
> 
> Ok, got it now.
> 
>> A quick follow-up as I saw I didn't fully answer your question: to my
>> knowledge there are no per-driver data structures that need a BKL for
>> protection. It's definitely not something I am worried about.
> 
> Good. Are you preparing a patch for a per-v4l2_device then? This sounds
> like the right place with your explanation. I would not put in the
> CONFIG_BKL switch, because I tried that for two other subsystems and got
> called back, but I'm not going to stop you.
> 
> As for the fallback to a global mutex, I guess you can set the
> videodev->lock pointer and use unlocked_ioctl for those drivers
> that do not use a v4l2_device yet, if there are only a handful of them.

Hans,

FYI, Linus already pull Arnd patch that removed BKL on -rc2 (commit 
0edf2e5e2bd0ae7689ce8a57ae3c87cc1f0c6548). I've pulled back from -rc2 into
linux-media.git tree. So, the patch is now visible on our main tree.

Cheers,
Mauro

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Hans Verkuil]

On Tuesday, November 16, 2010 17:01:36 Arnd Bergmann wrote:
> On Tuesday 16 November 2010, Hans Verkuil wrote:
> > > I think there is a misunderstanding. One V4L device (e.g. a TV capture
> > > card, a webcam, etc.) has one v4l2_device struct. But it can have multiple
> > > V4L device nodes (/dev/video0, /dev/radio0, etc.), each represented by a
> > > struct video_device (and I really hope I can rename that to v4l2_devnode
> > > soon since that's a very confusing name).
> > >
> > > You typically need to serialize between all the device nodes belonging to
> > > the same video hardware. A mutex in struct video_device doesn't do that,
> > > that just serializes access to that single device node. But a mutex in
> > > v4l2_device is at the right level.
> 
> Ok, got it now.
> 
> > A quick follow-up as I saw I didn't fully answer your question: to my
> > knowledge there are no per-driver data structures that need a BKL for
> > protection. It's definitely not something I am worried about.
> 
> Good. Are you preparing a patch for a per-v4l2_device then? This sounds
> like the right place with your explanation. I would not put in the
> CONFIG_BKL switch, because I tried that for two other subsystems and got
> called back, but I'm not going to stop you.
> 
> As for the fallback to a global mutex, I guess you can set the
> videodev->lock pointer and use unlocked_ioctl for those drivers
> that do not use a v4l2_device yet, if there are only a handful of them.
> 
> 	Arnd
> 

I will look into it. I'll try to have something today or tomorrow.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Hans Verkuil]

On Tuesday, November 16, 2010 17:49:05 Hans Verkuil wrote:
> On Tuesday, November 16, 2010 17:01:36 Arnd Bergmann wrote:
> > On Tuesday 16 November 2010, Hans Verkuil wrote:
> > > > I think there is a misunderstanding. One V4L device (e.g. a TV capture
> > > > card, a webcam, etc.) has one v4l2_device struct. But it can have multiple
> > > > V4L device nodes (/dev/video0, /dev/radio0, etc.), each represented by a
> > > > struct video_device (and I really hope I can rename that to v4l2_devnode
> > > > soon since that's a very confusing name).
> > > >
> > > > You typically need to serialize between all the device nodes belonging to
> > > > the same video hardware. A mutex in struct video_device doesn't do that,
> > > > that just serializes access to that single device node. But a mutex in
> > > > v4l2_device is at the right level.
> > 
> > Ok, got it now.
> > 
> > > A quick follow-up as I saw I didn't fully answer your question: to my
> > > knowledge there are no per-driver data structures that need a BKL for
> > > protection. It's definitely not something I am worried about.
> > 
> > Good. Are you preparing a patch for a per-v4l2_device then? This sounds
> > like the right place with your explanation. I would not put in the
> > CONFIG_BKL switch, because I tried that for two other subsystems and got
> > called back, but I'm not going to stop you.
> > 
> > As for the fallback to a global mutex, I guess you can set the
> > videodev->lock pointer and use unlocked_ioctl for those drivers
> > that do not use a v4l2_device yet, if there are only a handful of them.
> > 
> > 	Arnd
> > 
> 
> I will look into it. I'll try to have something today or tomorrow.

OK, here is my patch adding a mutex to v4l2_device.

I did some tests if we merge this patch then there are three classes of
drivers:

1) Those implementing unlocked_ioctl: these work like a charm.
2) Those implementing v4l2_device: capturing works fine, but calling ioctls
at the same time from another process or thread is *exceedingly* slow. But at
least there is no interference from other drivers.
3) Those not implementing v4l2_device: using a core lock makes it simply
impossible to capture from e.g. two devices at the same time. I tried with two
uvc webcams: the capture rate is simply horrible.

Note that this is tested in blocking mode. These problems do not appear if you
capture in non-blocking mode.

I consider class 3 unacceptable for commonly seen devices. I did a quick scan
of the v4l drivers and the only common driver that falls in that class is uvc.

There is one other option, although it is very dirty: don't take the lock if
the ioctl command is VIDIOC_DQBUF. It works and reliably as well for uvc and
videobuf (I did a quick code analysis). But I don't know if it works everywhere.

I would like to get the opinion of others before I implement such a check. But
frankly, I think this may be our best bet.

So the patch below would look like this if I add the check:

-               mutex_lock(&v4l2_ioctl_mutex);
+               if (cmd != VIDIOC_DQBUF)
+                       mutex_lock(m);
                if (video_is_registered(vdev))
                        ret = vdev->fops->ioctl(filp, cmd, arg);
-               mutex_unlock(&v4l2_ioctl_mutex);
+               if (cmd != VIDIOC_DQBUF)
+                       mutex_unlock(m);

Comments?

Regards,

	Hans

diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 03f7f46..026bf38 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -247,11 +247,13 @@ static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	} else if (vdev->fops->ioctl) {
 		/* TODO: convert all drivers to unlocked_ioctl */
 		static DEFINE_MUTEX(v4l2_ioctl_mutex);
+		struct mutex *m = vdev->v4l2_dev ?
+			&vdev->v4l2_dev->ioctl_lock : &v4l2_ioctl_mutex;
 
-		mutex_lock(&v4l2_ioctl_mutex);
+		mutex_lock(m);
 		if (video_is_registered(vdev))
 			ret = vdev->fops->ioctl(filp, cmd, arg);
-		mutex_unlock(&v4l2_ioctl_mutex);
+		mutex_unlock(m);
 	} else
 		ret = -ENOTTY;
 
diff --git a/drivers/media/video/v4l2-device.c b/drivers/media/video/v4l2-device.c
index 0b08f96..7fe6f92 100644
--- a/drivers/media/video/v4l2-device.c
+++ b/drivers/media/video/v4l2-device.c
@@ -35,6 +35,7 @@ int v4l2_device_register(struct device *dev, struct v4l2_device *v4l2_dev)
 
 	INIT_LIST_HEAD(&v4l2_dev->subdevs);
 	spin_lock_init(&v4l2_dev->lock);
+	mutex_init(&v4l2_dev->ioctl_lock);
 	v4l2_dev->dev = dev;
 	if (dev == NULL) {
 		/* If dev == NULL, then name must be filled in by the caller */
diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h
index 6648036..b16f307 100644
--- a/include/media/v4l2-device.h
+++ b/include/media/v4l2-device.h
@@ -51,6 +51,8 @@ struct v4l2_device {
 			unsigned int notification, void *arg);
 	/* The control handler. May be NULL. */
 	struct v4l2_ctrl_handler *ctrl_handler;
+	/* BKL replacement mutex. Temporary solution only. */
+	struct mutex ioctl_lock;
 };
 
 /* Initialize v4l2_dev and make dev->driver_data point to v4l2_dev.

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Arnd Bergmann]

On Tuesday 16 November 2010, Hans Verkuil wrote:
> I consider class 3 unacceptable for commonly seen devices. I did a quick scan
> of the v4l drivers and the only common driver that falls in that class is uvc.

If uvc is the only important one, that should be easy enough to fix by adding
a per-device mutex around uvc_v4l2_do_ioctl() or uvc_v4l2_ioctl().

> There is one other option, although it is very dirty: don't take the lock if
> the ioctl command is VIDIOC_DQBUF. It works and reliably as well for uvc and
> videobuf (I did a quick code analysis). But I don't know if it works everywhere.
> 
> I would like to get the opinion of others before I implement such a check. But
> frankly, I think this may be our best bet.
> 
> So the patch below would look like this if I add the check:
> 
> -               mutex_lock(&v4l2_ioctl_mutex);
> +               if (cmd != VIDIOC_DQBUF)
> +                       mutex_lock(m);
>                 if (video_is_registered(vdev))
>                         ret = vdev->fops->ioctl(filp, cmd, arg);
> -               mutex_unlock(&v4l2_ioctl_mutex);
> +               if (cmd != VIDIOC_DQBUF)
> +                       mutex_unlock(m);
> 

I was thinking of this as well, but didn't bring it up because I considered
it too hacky.

The patch you posted looks good, thanks for bringing up the problem with
my patch and the solution!

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Andy Walls]

On Tue, 2010-11-16 at 19:38 +0100, Hans Verkuil wrote:
> On Tuesday, November 16, 2010 17:49:05 Hans Verkuil wrote:
> > On Tuesday, November 16, 2010 17:01:36 Arnd Bergmann wrote:
> > > On Tuesday 16 November 2010, Hans Verkuil wrote:
> > > > > I think there is a misunderstanding. One V4L device (e.g. a TV capture
> > > > > card, a webcam, etc.) has one v4l2_device struct. But it can have multiple
> > > > > V4L device nodes (/dev/video0, /dev/radio0, etc.), each represented by a
> > > > > struct video_device (and I really hope I can rename that to v4l2_devnode
> > > > > soon since that's a very confusing name).
> > > > >
> > > > > You typically need to serialize between all the device nodes belonging to
> > > > > the same video hardware. A mutex in struct video_device doesn't do that,
> > > > > that just serializes access to that single device node. But a mutex in
> > > > > v4l2_device is at the right level.
> > > 
> > > Ok, got it now.
> > > 
> > > > A quick follow-up as I saw I didn't fully answer your question: to my
> > > > knowledge there are no per-driver data structures that need a BKL for
> > > > protection. It's definitely not something I am worried about.
> > > 
> > > Good. Are you preparing a patch for a per-v4l2_device then? This sounds
> > > like the right place with your explanation. I would not put in the
> > > CONFIG_BKL switch, because I tried that for two other subsystems and got
> > > called back, but I'm not going to stop you.
> > > 
> > > As for the fallback to a global mutex, I guess you can set the
> > > videodev->lock pointer and use unlocked_ioctl for those drivers
> > > that do not use a v4l2_device yet, if there are only a handful of them.
> > > 
> > > 	Arnd
> > > 
> > 
> > I will look into it. I'll try to have something today or tomorrow.
> 
> OK, here is my patch adding a mutex to v4l2_device.
> 
> I did some tests if we merge this patch then there are three classes of
> drivers:
> 
> 1) Those implementing unlocked_ioctl: these work like a charm.
> 2) Those implementing v4l2_device: capturing works fine, but calling ioctls
> at the same time from another process or thread is *exceedingly* slow. But at
> least there is no interference from other drivers.
> 3) Those not implementing v4l2_device: using a core lock makes it simply
> impossible to capture from e.g. two devices at the same time. I tried with two
> uvc webcams: the capture rate is simply horrible.
> 
> Note that this is tested in blocking mode. These problems do not appear if you
> capture in non-blocking mode.
> 
> I consider class 3 unacceptable for commonly seen devices. I did a quick scan
> of the v4l drivers and the only common driver that falls in that class is uvc.
> 
> There is one other option, although it is very dirty: don't take the lock if
> the ioctl command is VIDIOC_DQBUF.

Is this "in addition to" or "instead of" the mutex lock at
v4l2_device ? 

>  It works and reliably as well for uvc and
> videobuf (I did a quick code analysis). But I don't know if it works everywhere.
> 
> I would like to get the opinion of others before I implement such a check. But
> frankly, I think this may be our best bet.

Opinions? No problem! ;)

<opinions>

I think it is probably bad.


> So the patch below would look like this if I add the check:
> 
> -               mutex_lock(&v4l2_ioctl_mutex);
> +               if (cmd != VIDIOC_DQBUF)
> +                       mutex_lock(m);
>                 if (video_is_registered(vdev))
>                         ret = vdev->fops->ioctl(filp, cmd, arg);
> -               mutex_unlock(&v4l2_ioctl_mutex);
> +               if (cmd != VIDIOC_DQBUF)
> +                       mutex_unlock(m);

What happens to driver state when VIDIOC_STREAMOFF has the lock held and
VIDIOC_DQBUF comes through?  I think it is legitimate design for an
application to have a playback control thread separate from a thread
that reads in the capture data.

If this quirk of "infrastructure locking" is going in, might I suggest
that you please document in code comments:

a. The scope of what infrastructure lock is intended to protect.  That
is obvious right now, but may not be in the future.
 
b. Why there is an exception to taking the infrastructure lock or what
conditions necessitate having the lock ignored/dropped.

c. What code maintenance must be done to remove the exception to taking
the lock.  A specific bullet-list of problem drivers might be nice.

We won't do future maintainers any favors by letting the operation,
intended behavior, intended scope, and rationale for this odd locking
semantic be lost to history.  We just introduce a BKL with smaller
scope.



> Comments?
> 
> Regards,
> 
> 	Hans
> 
> diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
> index 03f7f46..026bf38 100644
> --- a/drivers/media/video/v4l2-dev.c
> +++ b/drivers/media/video/v4l2-dev.c
> @@ -247,11 +247,13 @@ static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	} else if (vdev->fops->ioctl) {
>  		/* TODO: convert all drivers to unlocked_ioctl */
>  		static DEFINE_MUTEX(v4l2_ioctl_mutex);
> +		struct mutex *m = vdev->v4l2_dev ?
> +			&vdev->v4l2_dev->ioctl_lock : &v4l2_ioctl_mutex;
>  
> -		mutex_lock(&v4l2_ioctl_mutex);
> +		mutex_lock(m);
>  		if (video_is_registered(vdev))
>  			ret = vdev->fops->ioctl(filp, cmd, arg);
> -		mutex_unlock(&v4l2_ioctl_mutex);
> +		mutex_unlock(m);
>  	} else
>  		ret = -ENOTTY;
>  
> diff --git a/drivers/media/video/v4l2-device.c b/drivers/media/video/v4l2-device.c
> index 0b08f96..7fe6f92 100644
> --- a/drivers/media/video/v4l2-device.c
> +++ b/drivers/media/video/v4l2-device.c
> @@ -35,6 +35,7 @@ int v4l2_device_register(struct device *dev, struct v4l2_device *v4l2_dev)
>  
>  	INIT_LIST_HEAD(&v4l2_dev->subdevs);
>  	spin_lock_init(&v4l2_dev->lock);
> +	mutex_init(&v4l2_dev->ioctl_lock);
>  	v4l2_dev->dev = dev;
>  	if (dev == NULL) {
>  		/* If dev == NULL, then name must be filled in by the caller */
> diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h
> index 6648036..b16f307 100644
> --- a/include/media/v4l2-device.h
> +++ b/include/media/v4l2-device.h
> @@ -51,6 +51,8 @@ struct v4l2_device {
>  			unsigned int notification, void *arg);
>  	/* The control handler. May be NULL. */
>  	struct v4l2_ctrl_handler *ctrl_handler;
> +	/* BKL replacement mutex. Temporary solution only. */
> +	struct mutex ioctl_lock;

Perhaps please add a comment on the specific software maintenance tasks
that are required to remove this temporary solution in the future.
Knowing is half the battle for future maintainers.


I know an SCM change log comments can capture the rationale, etc., but
relying on change logs doesn't work when the SCM tool changes. (e.g. the
transition to git)

</opinions>

:)

Regards,
Andy

>  };
>  
>  /* Initialize v4l2_dev and make dev->driver_data point to v4l2_dev.
> 

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Hans Verkuil]

On Tuesday, November 16, 2010 20:59:41 Andy Walls wrote:
> On Tue, 2010-11-16 at 19:38 +0100, Hans Verkuil wrote:
> > On Tuesday, November 16, 2010 17:49:05 Hans Verkuil wrote:
> > > On Tuesday, November 16, 2010 17:01:36 Arnd Bergmann wrote:
> > > > On Tuesday 16 November 2010, Hans Verkuil wrote:
> > > > > > I think there is a misunderstanding. One V4L device (e.g. a TV capture
> > > > > > card, a webcam, etc.) has one v4l2_device struct. But it can have multiple
> > > > > > V4L device nodes (/dev/video0, /dev/radio0, etc.), each represented by a
> > > > > > struct video_device (and I really hope I can rename that to v4l2_devnode
> > > > > > soon since that's a very confusing name).
> > > > > >
> > > > > > You typically need to serialize between all the device nodes belonging to
> > > > > > the same video hardware. A mutex in struct video_device doesn't do that,
> > > > > > that just serializes access to that single device node. But a mutex in
> > > > > > v4l2_device is at the right level.
> > > > 
> > > > Ok, got it now.
> > > > 
> > > > > A quick follow-up as I saw I didn't fully answer your question: to my
> > > > > knowledge there are no per-driver data structures that need a BKL for
> > > > > protection. It's definitely not something I am worried about.
> > > > 
> > > > Good. Are you preparing a patch for a per-v4l2_device then? This sounds
> > > > like the right place with your explanation. I would not put in the
> > > > CONFIG_BKL switch, because I tried that for two other subsystems and got
> > > > called back, but I'm not going to stop you.
> > > > 
> > > > As for the fallback to a global mutex, I guess you can set the
> > > > videodev->lock pointer and use unlocked_ioctl for those drivers
> > > > that do not use a v4l2_device yet, if there are only a handful of them.
> > > > 
> > > > 	Arnd
> > > > 
> > > 
> > > I will look into it. I'll try to have something today or tomorrow.
> > 
> > OK, here is my patch adding a mutex to v4l2_device.
> > 
> > I did some tests if we merge this patch then there are three classes of
> > drivers:
> > 
> > 1) Those implementing unlocked_ioctl: these work like a charm.
> > 2) Those implementing v4l2_device: capturing works fine, but calling ioctls
> > at the same time from another process or thread is *exceedingly* slow. But at
> > least there is no interference from other drivers.

BTW, with 'exceedingly slow' I mean that e.g. a v4l2-ctl --list-ctrls takes 10-20
seconds if the device node is streaming at the same time. Something that normally
takes less than 0.01s.

> > 3) Those not implementing v4l2_device: using a core lock makes it simply
> > impossible to capture from e.g. two devices at the same time. I tried with two
> > uvc webcams: the capture rate is simply horrible.
> > 
> > Note that this is tested in blocking mode. These problems do not appear if you
> > capture in non-blocking mode.
> > 
> > I consider class 3 unacceptable for commonly seen devices. I did a quick scan
> > of the v4l drivers and the only common driver that falls in that class is uvc.
> > 
> > There is one other option, although it is very dirty: don't take the lock if
> > the ioctl command is VIDIOC_DQBUF.
> 
> Is this "in addition to" or "instead of" the mutex lock at
> v4l2_device ? 

In addition to.

> >  It works and reliably as well for uvc and
> > videobuf (I did a quick code analysis). But I don't know if it works everywhere.
> > 
> > I would like to get the opinion of others before I implement such a check. But
> > frankly, I think this may be our best bet.
> 
> Opinions? No problem! ;)
> 
> <opinions>
> 
> I think it is probably bad.
> 
> 
> > So the patch below would look like this if I add the check:
> > 
> > -               mutex_lock(&v4l2_ioctl_mutex);
> > +               if (cmd != VIDIOC_DQBUF)
> > +                       mutex_lock(m);
> >                 if (video_is_registered(vdev))
> >                         ret = vdev->fops->ioctl(filp, cmd, arg);
> > -               mutex_unlock(&v4l2_ioctl_mutex);
> > +               if (cmd != VIDIOC_DQBUF)
> > +                       mutex_unlock(m);
> 
> What happens to driver state when VIDIOC_STREAMOFF has the lock held and
> VIDIOC_DQBUF comes through?  I think it is legitimate design for an
> application to have a playback control thread separate from a thread
> that reads in the capture data.

All I can say is that it will work OK for drivers using videobuf since
videobuf will take its own lock.

UVC? I'm not sure. But I think that it is probably best to fix uvc for
2.6.37 regardless given the importance of this driver.

It's dirty, but I actually think that it will work fine.

> If this quirk of "infrastructure locking" is going in, might I suggest
> that you please document in code comments:
> 
> a. The scope of what infrastructure lock is intended to protect.  That
> is obvious right now, but may not be in the future.
>  
> b. Why there is an exception to taking the infrastructure lock or what
> conditions necessitate having the lock ignored/dropped.
> 
> c. What code maintenance must be done to remove the exception to taking
> the lock.  A specific bullet-list of problem drivers might be nice.

Obviously. The right solution is simply that all drivers should move to
unlocked_ioctl. Preferably for 2.6.38. I don't like this hack at all, but
circumstances basically force this on us. Not doing this hack leads to
unacceptable side-effects.

We can also fast-track for 2.6.37 the set of trivial unlocked_ioctl
conversions I did this week. That will reduce the impact as well.

> We won't do future maintainers any favors by letting the operation,
> intended behavior, intended scope, and rationale for this odd locking
> semantic be lost to history.  We just introduce a BKL with smaller
> scope.

We have to aggressively track this and convert all drivers to unlocked_ioctl
as soon as possible. There is no other option.

Regards,

	Hans

> > 
> > diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
> > index 03f7f46..026bf38 100644
> > --- a/drivers/media/video/v4l2-dev.c
> > +++ b/drivers/media/video/v4l2-dev.c
> > @@ -247,11 +247,13 @@ static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> >  	} else if (vdev->fops->ioctl) {
> >  		/* TODO: convert all drivers to unlocked_ioctl */
> >  		static DEFINE_MUTEX(v4l2_ioctl_mutex);
> > +		struct mutex *m = vdev->v4l2_dev ?
> > +			&vdev->v4l2_dev->ioctl_lock : &v4l2_ioctl_mutex;
> >  
> > -		mutex_lock(&v4l2_ioctl_mutex);
> > +		mutex_lock(m);
> >  		if (video_is_registered(vdev))
> >  			ret = vdev->fops->ioctl(filp, cmd, arg);
> > -		mutex_unlock(&v4l2_ioctl_mutex);
> > +		mutex_unlock(m);
> >  	} else
> >  		ret = -ENOTTY;
> >  
> > diff --git a/drivers/media/video/v4l2-device.c b/drivers/media/video/v4l2-device.c
> > index 0b08f96..7fe6f92 100644
> > --- a/drivers/media/video/v4l2-device.c
> > +++ b/drivers/media/video/v4l2-device.c
> > @@ -35,6 +35,7 @@ int v4l2_device_register(struct device *dev, struct v4l2_device *v4l2_dev)
> >  
> >  	INIT_LIST_HEAD(&v4l2_dev->subdevs);
> >  	spin_lock_init(&v4l2_dev->lock);
> > +	mutex_init(&v4l2_dev->ioctl_lock);
> >  	v4l2_dev->dev = dev;
> >  	if (dev == NULL) {
> >  		/* If dev == NULL, then name must be filled in by the caller */
> > diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h
> > index 6648036..b16f307 100644
> > --- a/include/media/v4l2-device.h
> > +++ b/include/media/v4l2-device.h
> > @@ -51,6 +51,8 @@ struct v4l2_device {
> >  			unsigned int notification, void *arg);
> >  	/* The control handler. May be NULL. */
> >  	struct v4l2_ctrl_handler *ctrl_handler;
> > +	/* BKL replacement mutex. Temporary solution only. */
> > +	struct mutex ioctl_lock;
> 
> Perhaps please add a comment on the specific software maintenance tasks
> that are required to remove this temporary solution in the future.
> Knowing is half the battle for future maintainers.
> 
> 
> I know an SCM change log comments can capture the rationale, etc., but
> relying on change logs doesn't work when the SCM tool changes. (e.g. the
> transition to git)
> 
> </opinions>
> 
> :)
> 
> Regards,
> Andy
> 
> >  };
> >  
> >  /* Initialize v4l2_dev and make dev->driver_data point to v4l2_dev.
> > 
> 
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Hans Verkuil]

On Tuesday, November 16, 2010 21:29:11 Hans Verkuil wrote:
> On Tuesday, November 16, 2010 20:59:41 Andy Walls wrote:
> > On Tue, 2010-11-16 at 19:38 +0100, Hans Verkuil wrote:
> > > On Tuesday, November 16, 2010 17:49:05 Hans Verkuil wrote:
> > > > On Tuesday, November 16, 2010 17:01:36 Arnd Bergmann wrote:
> > > > > On Tuesday 16 November 2010, Hans Verkuil wrote:
> > > > > > > I think there is a misunderstanding. One V4L device (e.g. a TV capture
> > > > > > > card, a webcam, etc.) has one v4l2_device struct. But it can have multiple
> > > > > > > V4L device nodes (/dev/video0, /dev/radio0, etc.), each represented by a
> > > > > > > struct video_device (and I really hope I can rename that to v4l2_devnode
> > > > > > > soon since that's a very confusing name).
> > > > > > >
> > > > > > > You typically need to serialize between all the device nodes belonging to
> > > > > > > the same video hardware. A mutex in struct video_device doesn't do that,
> > > > > > > that just serializes access to that single device node. But a mutex in
> > > > > > > v4l2_device is at the right level.
> > > > > 
> > > > > Ok, got it now.
> > > > > 
> > > > > > A quick follow-up as I saw I didn't fully answer your question: to my
> > > > > > knowledge there are no per-driver data structures that need a BKL for
> > > > > > protection. It's definitely not something I am worried about.
> > > > > 
> > > > > Good. Are you preparing a patch for a per-v4l2_device then? This sounds
> > > > > like the right place with your explanation. I would not put in the
> > > > > CONFIG_BKL switch, because I tried that for two other subsystems and got
> > > > > called back, but I'm not going to stop you.
> > > > > 
> > > > > As for the fallback to a global mutex, I guess you can set the
> > > > > videodev->lock pointer and use unlocked_ioctl for those drivers
> > > > > that do not use a v4l2_device yet, if there are only a handful of them.
> > > > > 
> > > > > 	Arnd
> > > > > 
> > > > 
> > > > I will look into it. I'll try to have something today or tomorrow.
> > > 
> > > OK, here is my patch adding a mutex to v4l2_device.
> > > 
> > > I did some tests if we merge this patch then there are three classes of
> > > drivers:
> > > 
> > > 1) Those implementing unlocked_ioctl: these work like a charm.
> > > 2) Those implementing v4l2_device: capturing works fine, but calling ioctls
> > > at the same time from another process or thread is *exceedingly* slow. But at
> > > least there is no interference from other drivers.
> 
> BTW, with 'exceedingly slow' I mean that e.g. a v4l2-ctl --list-ctrls takes 10-20
> seconds if the device node is streaming at the same time. Something that normally
> takes less than 0.01s.
> 
> > > 3) Those not implementing v4l2_device: using a core lock makes it simply
> > > impossible to capture from e.g. two devices at the same time. I tried with two
> > > uvc webcams: the capture rate is simply horrible.
> > > 
> > > Note that this is tested in blocking mode. These problems do not appear if you
> > > capture in non-blocking mode.
> > > 
> > > I consider class 3 unacceptable for commonly seen devices. I did a quick scan
> > > of the v4l drivers and the only common driver that falls in that class is uvc.
> > > 
> > > There is one other option, although it is very dirty: don't take the lock if
> > > the ioctl command is VIDIOC_DQBUF.
> > 
> > Is this "in addition to" or "instead of" the mutex lock at
> > v4l2_device ? 
> 
> In addition to.
> 
> > >  It works and reliably as well for uvc and
> > > videobuf (I did a quick code analysis). But I don't know if it works everywhere.
> > > 
> > > I would like to get the opinion of others before I implement such a check. But
> > > frankly, I think this may be our best bet.
> > 
> > Opinions? No problem! ;)
> > 
> > <opinions>
> > 
> > I think it is probably bad.
> > 
> > 
> > > So the patch below would look like this if I add the check:
> > > 
> > > -               mutex_lock(&v4l2_ioctl_mutex);
> > > +               if (cmd != VIDIOC_DQBUF)
> > > +                       mutex_lock(m);
> > >                 if (video_is_registered(vdev))
> > >                         ret = vdev->fops->ioctl(filp, cmd, arg);
> > > -               mutex_unlock(&v4l2_ioctl_mutex);
> > > +               if (cmd != VIDIOC_DQBUF)
> > > +                       mutex_unlock(m);
> > 
> > What happens to driver state when VIDIOC_STREAMOFF has the lock held and
> > VIDIOC_DQBUF comes through?  I think it is legitimate design for an
> > application to have a playback control thread separate from a thread
> > that reads in the capture data.
> 
> All I can say is that it will work OK for drivers using videobuf since
> videobuf will take its own lock.
> 
> UVC? I'm not sure. But I think that it is probably best to fix uvc for
> 2.6.37 regardless given the importance of this driver.
> 
> It's dirty, but I actually think that it will work fine.
> 
> > If this quirk of "infrastructure locking" is going in, might I suggest
> > that you please document in code comments:
> > 
> > a. The scope of what infrastructure lock is intended to protect.  That
> > is obvious right now, but may not be in the future.
> >  
> > b. Why there is an exception to taking the infrastructure lock or what
> > conditions necessitate having the lock ignored/dropped.
> > 
> > c. What code maintenance must be done to remove the exception to taking
> > the lock.  A specific bullet-list of problem drivers might be nice.
> 
> Obviously. The right solution is simply that all drivers should move to
> unlocked_ioctl. Preferably for 2.6.38. I don't like this hack at all, but
> circumstances basically force this on us. Not doing this hack leads to
> unacceptable side-effects.
> 
> We can also fast-track for 2.6.37 the set of trivial unlocked_ioctl
> conversions I did this week. That will reduce the impact as well.
> 
> > We won't do future maintainers any favors by letting the operation,
> > intended behavior, intended scope, and rationale for this odd locking
> > semantic be lost to history.  We just introduce a BKL with smaller
> > scope.
> 
> We have to aggressively track this and convert all drivers to unlocked_ioctl
> as soon as possible. There is no other option.

I did some more analysis: 

Not taking the lock for VIDIOC_DQBUF is fine for videobuf-based drivers. So
this is a good temporary fix for those. So we are left with drivers that:

1) do not use unlocked_ioctl
2) do not use videobuf
3) no trivial patch converting them to unlocked_ioctl exists (i.e., the patches
I posted for review this week).

The set of those is very small:

stk-webcam, usbvision and uvc.

Of these three uvc is the only important one for 2.6.37. The others we can tackle
for 2.6.38.

So I propose to put in this hack (with lots of comments), add the 'trivial BKL
removal' patch series, and send the whole lot to 2.6.37. Next to that uvc needs
to be converted to unlocked_ioctl for 2.6.37 as well.

Does anyone have a better idea?

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[David Ellingsworth]

Hans,

I've had some patches pending for a while now that affect the dsbr100
driver. The patches can be seen here:
http://desource.dyndns.org/~atog/gitweb/?p=linux-media.git in the
dsbr100 branch. The first patch in the series fixes locking issues
throughout the driver and converts it to use the unlocked ioctl. The
series is a bit old, so it doesn't make use of the v4l2 core assisted
locking; but that is trivial to implement after this patch.

Regards,

David Ellingsworth

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[Hans Verkuil]

On Tuesday, November 16, 2010 22:32:57 David Ellingsworth wrote:
> Hans,
> 
> I've had some patches pending for a while now that affect the dsbr100
> driver. The patches can be seen here:
> http://desource.dyndns.org/~atog/gitweb/?p=linux-media.git in the
> dsbr100 branch. The first patch in the series fixes locking issues
> throughout the driver and converts it to use the unlocked ioctl. The
> series is a bit old, so it doesn't make use of the v4l2 core assisted
> locking; but that is trivial to implement after this patch.

Would it be a problem for you if for 2.6.37 I just replace .ioctl by
.unlocked_ioctl? And do the full conversion for 2.6.38? That way the
2.6.37 patches remain small.

Regards.

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [RFC PATCH 0/8] V4L BKL removal: first round

[David Ellingsworth]

On Tue, Nov 16, 2010 at 4:42 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Tuesday, November 16, 2010 22:32:57 David Ellingsworth wrote:
>> Hans,
>>
>> I've had some patches pending for a while now that affect the dsbr100
>> driver. The patches can be seen here:
>> http://desource.dyndns.org/~atog/gitweb/?p=linux-media.git in the
>> dsbr100 branch. The first patch in the series fixes locking issues
>> throughout the driver and converts it to use the unlocked ioctl. The
>> series is a bit old, so it doesn't make use of the v4l2 core assisted
>> locking; but that is trivial to implement after this patch.
>
> Would it be a problem for you if for 2.6.37 I just replace .ioctl by
> .unlocked_ioctl? And do the full conversion for 2.6.38? That way the
> 2.6.37 patches remain small.
>

If you look at the first patch in that series, you'll see that the
conversion isn't that simple. There are a lot of places in that driver
that should have been protected by a lock that weren't. At the moment,
I think the BKL protects these areas from racing so just replacing the
.ioctl with the .unlocked_ioctl here isn't a good solution for this
driver. Applying the patch I've provided will however remove the BKL
and the resolve all the locking issues as well.

Regards,

David Ellingsworth