V4L-DVB drivers and BKL

Hi all,

I just read on LWN that the core kernel guys are putting more effort into removing the BKL. We are still using it in our own drivers, mostly V4L.

I added a BKL column to my driver list:

http://www.linuxtv.org/wiki/index.php/V4L_framework_progress#Bridge_Drivers

If you 'own' one of these drivers that still use BKL, then it would be nice if you can try and remove the use of the BKL from those drivers.

The other part that needs to be done is to move from using the .ioctl file op to using .unlocked_ioctl. Very few drivers do that, but I suspect almost no driver actually needs to use .ioctl.

Hi Hans,

Quoted 9 lines · On Thursday 01 April 2010 10:01:10 Hans Verkuil wrote:

Hi all,

I just read on LWN that the core kernel guys are putting more effort into removing the BKL. We are still using it in our own drivers, mostly V4L.

I added a BKL column to my driver list:

http://www.linuxtv.org/wiki/index.php/V4L_framework_progress#Bridge_Drivers

Quoted 6 lines

If you 'own' one of these drivers that still use BKL, then it would be nice if you can try and remove the use of the BKL from those drivers.

The other part that needs to be done is to move from using the .ioctl file op to using .unlocked_ioctl. Very few drivers do that, but I suspect almost no driver actually needs to use .ioctl.

What about something like this patch as a first step ?

drivers/media/video/v4l2-dev.c +11 -35

index 7090699..14e1b1c 100644
@@ -25,6 +25,7 @@
 #include <linux/init.h>
 #include <linux/kmod.h>
 #include <linux/slab.h>
+#include <linux/smp_lock.h>
 #include <asm/uaccess.h>
 #include <asm/system.h>
 

@@ -215,28 +216,22 @@ static unsigned int v4l2_poll(struct file *filp, struct poll_table_struct *poll)
 	return vdev->fops->poll(filp, poll);
 }
 
-static int v4l2_ioctl(struct inode *inode, struct file *filp,
-		unsigned int cmd, unsigned long arg)
-{
-	struct video_device *vdev = video_devdata(filp);
-
-	if (!vdev->fops->ioctl)
-		return -ENOTTY;
-	/* Allow ioctl to continue even if the device was unregistered.
-	   Things like dequeueing buffers might still be useful. */
-	return vdev->fops->ioctl(filp, cmd, arg);
-}
-
 static long v4l2_unlocked_ioctl(struct file *filp,
 		unsigned int cmd, unsigned long arg)
 {
 	struct video_device *vdev = video_devdata(filp);
+	int ret = -ENOTTY;
 
-	if (!vdev->fops->unlocked_ioctl)
-		return -ENOTTY;
 	/* Allow ioctl to continue even if the device was unregistered.
 	   Things like dequeueing buffers might still be useful. */
-	return vdev->fops->unlocked_ioctl(filp, cmd, arg);
+	if (vdev->fops->ioctl) {
+		lock_kernel();
+		ret = vdev->fops->ioctl(filp, cmd, arg);
+		unlock_kernel();
+	} else if (vdev->fops->unlocked_ioctl)
+		ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
+
+	return ret;
 }
 
 #ifdef CONFIG_MMU

@@ -323,22 +318,6 @@ static const struct file_operations v4l2_unlocked_fops = {
 	.llseek = no_llseek,
 };
 
-static const struct file_operations v4l2_fops = {
-	.owner = THIS_MODULE,
-	.read = v4l2_read,
-	.write = v4l2_write,
-	.open = v4l2_open,
-	.get_unmapped_area = v4l2_get_unmapped_area,
-	.mmap = v4l2_mmap,
-	.ioctl = v4l2_ioctl,
-#ifdef CONFIG_COMPAT
-	.compat_ioctl = v4l2_compat_ioctl32,
-#endif
-	.release = v4l2_release,
-	.poll = v4l2_poll,
-	.llseek = no_llseek,
-};
-
 /**
  * get_index - assign stream index number based on parent device
  * @vdev: video_device to assign index number to, vdev->parent should be assigned

@@ -517,10 +496,7 @@ static int __video_register_device(struct video_device *vdev, int type, int nr,
 		ret = -ENOMEM;
 		goto cleanup;
 	}
-	if (vdev->fops->unlocked_ioctl)
-		vdev->cdev->ops = &v4l2_unlocked_fops;
-	else
-		vdev->cdev->ops = &v4l2_fops;
+	vdev->cdev->ops = &v4l2_unlocked_fops;
 	vdev->cdev->owner = vdev->fops->owner;
 	ret = cdev_add(vdev->cdev, MKDEV(VIDEO_MAJOR, vdev->minor), 1);
 	if (ret < 0) {

A second step would be to replace lock_kernel/unlock_kernel with a V4L-specific lock, and the third step to push the lock into drivers.

Quoted 15 lines · On Thursday 01 April 2010 11:23:30 Laurent Pinchart wrote:

Quoted 14 lines · On Thursday 01 April 2010 10:01:10 Hans Verkuil wrote:

Hi all,

I just read on LWN that the core kernel guys are putting more effort into removing the BKL. We are still using it in our own drivers, mostly V4L.

I added a BKL column to my driver list:

http://www.linuxtv.org/wiki/index.php/V4L_framework_progress#Bridge_Dri
vers

If you 'own' one of these drivers that still use BKL, then it would be nice if you can try and remove the use of the BKL from those drivers.

Quoted 5 lines

Quoted 3 lines

The other part that needs to be done is to move from using the .ioctl file op to using .unlocked_ioctl. Very few drivers do that, but I suspect almost no driver actually needs to use .ioctl.

What about something like this patch as a first step ?

That doesn't fix anything. You just move the BKL from one place to another. I don't see any benefit from that.

Quoted 116 lines

(quoted diff) +0 -0

b/drivers/media/video/v4l2-dev.c index 7090699..14e1b1c 100644

drivers/media/video/v4l2-dev.c +1 -0

@@ -25,6 +25,7 @@
 
 #include <linux/init.h>
 #include <linux/kmod.h>
 #include <linux/slab.h>
 
+#include <linux/smp_lock.h>
 

#include <asm/uaccess.h>
#include <asm/system.h>

drivers/media/video/v4l2-dev.c +9 -31

@@ -215,28 +216,22 @@ static unsigned int v4l2_poll(struct file *filp,
 struct poll_table_struct *poll)
 
 	return vdev->fops->poll(filp, poll);
 
 }
 
-static int v4l2_ioctl(struct inode *inode, struct file *filp,
-		unsigned int cmd, unsigned long arg)
-{
-	struct video_device *vdev = video_devdata(filp);
-
-	if (!vdev->fops->ioctl)
-		return -ENOTTY;
-	/* Allow ioctl to continue even if the device was unregistered.
-	   Things like dequeueing buffers might still be useful. */
-	return vdev->fops->ioctl(filp, cmd, arg);
-}
-
 
 static long v4l2_unlocked_ioctl(struct file *filp,
 
 		unsigned int cmd, unsigned long arg)
 
 {
 
 	struct video_device *vdev = video_devdata(filp);
 
+	int ret = -ENOTTY;
 
-	if (!vdev->fops->unlocked_ioctl)
-		return -ENOTTY;
 
 	/* Allow ioctl to continue even if the device was unregistered.
 
 	   Things like dequeueing buffers might still be useful. */
 
-	return vdev->fops->unlocked_ioctl(filp, cmd, arg);
+	if (vdev->fops->ioctl) {
+		lock_kernel();
+		ret = vdev->fops->ioctl(filp, cmd, arg);
+		unlock_kernel();
+	} else if (vdev->fops->unlocked_ioctl)
+		ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
+
+	return ret;
 
 }
 
 #ifdef CONFIG_MMU

@@ -323,22 +318,6 @@ static const struct file_operations
 v4l2_unlocked_fops = {
 
 	.llseek = no_llseek,
 
 };
 
-static const struct file_operations v4l2_fops = {
-	.owner = THIS_MODULE,
-	.read = v4l2_read,
-	.write = v4l2_write,
-	.open = v4l2_open,
-	.get_unmapped_area = v4l2_get_unmapped_area,
-	.mmap = v4l2_mmap,
-	.ioctl = v4l2_ioctl,
-#ifdef CONFIG_COMPAT
-	.compat_ioctl = v4l2_compat_ioctl32,
-#endif
-	.release = v4l2_release,
-	.poll = v4l2_poll,
-	.llseek = no_llseek,
-};
-

/**

* get_index - assign stream index number based on parent device
* @vdev: video_device to assign index number to, vdev->parent should be assigned

drivers/media/video/v4l2-dev.c +1 -4

@@ -517,10 +496,7 @@ static int __video_register_device(struct
 video_device *vdev, int type, int nr,
 
 		ret = -ENOMEM;
 		goto cleanup;
 
 	}
 
-	if (vdev->fops->unlocked_ioctl)
-		vdev->cdev->ops = &v4l2_unlocked_fops;
-	else
-		vdev->cdev->ops = &v4l2_fops;
+	vdev->cdev->ops = &v4l2_unlocked_fops;
 
 	vdev->cdev->owner = vdev->fops->owner;
 	ret = cdev_add(vdev->cdev, MKDEV(VIDEO_MAJOR, vdev->minor), 1);
 	if (ret < 0) {

A second step would be to replace lock_kernel/unlock_kernel with a V4L-specific lock, and the third step to push the lock into drivers.

Hi Hans,

Quoted 20 lines · On Thursday 01 April 2010 13:11:51 Hans Verkuil wrote:

Quoted 19 lines · On Thursday 01 April 2010 11:23:30 Laurent Pinchart wrote:

Quoted 11 lines · On Thursday 01 April 2010 10:01:10 Hans Verkuil wrote:

Hi all,

I just read on LWN that the core kernel guys are putting more effort into removing the BKL. We are still using it in our own drivers, mostly V4L.

I added a BKL column to my driver list:

http://www.linuxtv.org/wiki/index.php/V4L_framework_progress#Bridge_Dri
vers

Quoted 6 lines

If you 'own' one of these drivers that still use BKL, then it would be nice if you can try and remove the use of the BKL from those drivers.

The other part that needs to be done is to move from using the .ioctl file op to using .unlocked_ioctl. Very few drivers do that, but I suspect almost no driver actually needs to use .ioctl.

What about something like this patch as a first step ?

Quoted 2 lines

That doesn't fix anything. You just move the BKL from one place to another. I don't see any benefit from that.

Removing the BKL is a long-term project that basically pushes the BKL from core code to subsystems and drivers, and then replace it on a case by case basis. This patch (along with a replacement of lock_kernel/unlock_kernel by a V4L-specific lock) goes into that direction and removes the BKL usage from V4L ioctls. The V4L lock would then need to be pushed into individual drivers.

Quoted 26 lines · Laurent Pinchart

Quoted 16 lines · On Thursday 01 April 2010 13:11:51 Hans Verkuil wrote:

Quoted 15 lines · On Thursday 01 April 2010 11:23:30 Laurent Pinchart wrote:

Quoted 14 lines · On Thursday 01 April 2010 10:01:10 Hans Verkuil wrote:

Hi all,

I just read on LWN that the core kernel guys are putting more effort into removing the BKL. We are still using it in our own drivers, mostly V4L.

I added a BKL column to my driver list:

http://www.linuxtv.org/wiki/index.php/V4L_framework_progress#Bridge_Dr
i vers

If you 'own' one of these drivers that still use BKL, then it would be nice if you can try and remove the use of the BKL from those drivers.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

The other part that needs to be done is to move from using the .ioctl file op to using .unlocked_ioctl. Very few drivers do that, but I suspect almost no driver actually needs to use .ioctl.

What about something like this patch as a first step ?

That doesn't fix anything. You just move the BKL from one place to another. I don't see any benefit from that.

Quoted 6 lines

Removing the BKL is a long-term project that basically pushes the BKL from core code to subsystems and drivers, and then replace it on a case by case basis. This patch (along with a replacement of lock_kernel/unlock_kernel by a V4L-specific lock) goes into that direction and removes the BKL usage from V4L ioctls. The V4L lock would then need to be pushed into individual drivers.

True, but, as almost all V4L drivers share a "common ancestor", fixing the problems for one will also fix for the others.

One typical problem that I see is that some drivers register too soon: they first register, then initialize some things. I've seen (and fixed) some race conditions due to that. Just moving the register to the end solves this issue.

One (far from perfect) solution, would be to add a mutex protecting the entire ioctl loop inside the drivers, and the open/close methods. This can later be optimized by a mutex that will just protect the operations that can actually cause problems if happening in parallel.

Quoted 4 lines

One typical problem that I see is that some drivers register too soon: they first register, then initialize some things. I've seen (and fixed) some race conditions due to that. Just moving the register to the end solves this issue.

That's right, devices should not be registered until they are ready to be opened by userspace. However, I don't see how that's related to the BKL.

Quoted 4 lines

One (far from perfect) solution, would be to add a mutex protecting the entire ioctl loop inside the drivers, and the open/close methods. This can later be optimized by a mutex that will just protect the operations that can actually cause problems if happening in parallel.

The BKL protects both open() and ioctl(), but the ioctl operation can't be called before open succeeds anyway, so I'm not sure we have a problem there.

The real problem is that most drivers rely on ioctls being serialized by the BKL. The drivers need to be fixed on a case by case basis, but we could already drop the BKL there by using a V4L-specific lock to serialize ioctl calls.

Quoted 23 lines · Laurent Pinchart

Hi Hans,

Quoted 20 lines · On Thursday 01 April 2010 13:11:51 Hans Verkuil wrote:

Quoted 19 lines · On Thursday 01 April 2010 11:23:30 Laurent Pinchart wrote:

Quoted 11 lines · On Thursday 01 April 2010 10:01:10 Hans Verkuil wrote:

Hi all,

I just read on LWN that the core kernel guys are putting more effort into removing the BKL. We are still using it in our own drivers, mostly V4L.

I added a BKL column to my driver list:

http://www.linuxtv.org/wiki/index.php/V4L_framework_progress#Bridge_Dri
vers

Quoted 6 lines

If you 'own' one of these drivers that still use BKL, then it would be nice if you can try and remove the use of the BKL from those drivers.

The other part that needs to be done is to move from using the .ioctl file op to using .unlocked_ioctl. Very few drivers do that, but I suspect almost no driver actually needs to use .ioctl.

What about something like this patch as a first step ?

Quoted 8 lines

Quoted 2 lines

That doesn't fix anything. You just move the BKL from one place to another. I don't see any benefit from that.

Removing the BKL is a long-term project that basically pushes the BKL from core code to subsystems and drivers, and then replace it on a case by case basis. This patch (along with a replacement of lock_kernel/unlock_kernel by a V4L-specific lock) goes into that direction and removes the BKL usage from V4L ioctls. The V4L lock would then need to be pushed into individual drivers.

True, but, as almost all V4L drivers share a "common ancestor", fixing the problems for one will also fix for the others.

One typical problem that I see is that some drivers register too soon: they first register, then initialize some things. I've seen (and fixed) some race conditions due to that. Just moving the register to the end solves this issue.

One (far from perfect) solution, would be to add a mutex protecting the entire ioctl loop inside the drivers, and the open/close methods. This can later be optimized by a mutex that will just protect the operations that can actually cause problems if happening in parallel.

What to do if we have multiple device nodes? E.g. video0 and vbi0? Should we allow access to video0 when vbi0 is not yet registered? Or should we block access until all video nodes are registered?

Quoted 5 lines

One (far from perfect) solution, would be to add a mutex protecting the entire ioctl loop inside the drivers, and the open/close methods. This can later be optimized by a mutex that will just protect the operations that can actually cause problems if happening in parallel.

I have thought about this in the past.

What I think would be needed to make locking much more reliable is the following:

1) Currently when a device is unregistered all read()s, write()s, poll()s, etc. are blocked. Except for ioctl().

The comment in v4l2-dev.c says this:

/* Allow ioctl to continue even if the device was unregistered.
Things like dequeueing buffers might still be useful. */

I disagree with this. Once the device is gone (USB disconnect and similar hotplug scenarios), then the only thing an application can do is to close.

Allowing ioctl to still work makes it hard for drivers since every ioctl op that might do something with the device has to call video_is_registered() to check whether the device is still alive.

I know, this is not directly related to the BKL, but it is an additional complication.

2) Add a new video_device flag that turns on serialization. Basically all calls are serialized with a mutex in v4l2_device. To handle blocking calls like read() or VIDIOC_DQBUF we can either not take the serialization mutex in the core, or instead the driver needs to unlock the mutex before it waits for an event and lock it afterwards.

In the first case the core has to know all the exceptions.

Perhaps we should just add a second flag: whether the core should do full serialization (and the driver will have to unlock/lock around blocking waits) or smart serialization where know blocking operations are allowed unserialized.

I think it is fairly simple to add this serialization mechanism. And for many drivers this will actually be more than enough.

Quoted 3 lines · Hans Verkuil

What to do if we have multiple device nodes? E.g. video0 and vbi0? Should we allow access to video0 when vbi0 is not yet registered? Or should we block access until all video nodes are registered?

It will depend on the driver implementation, but, as new udev implementations try to open v4l devices asap,

the better is to lock the register operation
to avoid an open while not finished.

I remember I had to do it on em28xx:

This is the init code for it:

	...
        mutex_init(&dev->lock);
        mutex_lock(&dev->lock);
        em28xx_init_dev(&dev, udev, interface, nr);
	...
        request_modules(dev);

        /* Should be the last thing to do, to avoid newer udev's to
           open the device before fully initializing it
         */
        mutex_unlock(&dev->lock);
	...

And this is the open code:

static int em28xx_v4l2_open(struct file *filp)

{
	...
        mutex_lock(&dev->lock);
	...
	mutex_unlock(&dev->lock);

The same lock is also used at the ioctl handlers that need to be protected, like:

static int radio_g_tuner(struct file *file, void *priv,
struct v4l2_tuner *t)

{
	...
        mutex_lock(&dev->lock);
        v4l2_device_call_all(&dev->v4l2_dev, 0, tuner, g_tuner, t);
        mutex_unlock(&dev->lock);
	...
}

There are some obvious cases where no lock is needed, like for example vidioc_querycap.

Quoted 25 lines

Quoted 5 lines

One (far from perfect) solution, would be to add a mutex protecting the entire ioctl loop inside the drivers, and the open/close methods. This can later be optimized by a mutex that will just protect the operations that can actually cause problems if happening in parallel.

I have thought about this in the past.

What I think would be needed to make locking much more reliable is the following:

1) Currently when a device is unregistered all read()s, write()s, poll()s, etc. are blocked. Except for ioctl().

The comment in v4l2-dev.c says this:

/* Allow ioctl to continue even if the device was unregistered.
Things like dequeueing buffers might still be useful. */

I disagree with this. Once the device is gone (USB disconnect and similar hotplug scenarios), then the only thing an application can do is to close.

Allowing ioctl to still work makes it hard for drivers since every ioctl op that might do something with the device has to call video_is_registered() to check whether the device is still alive.

Quoted 2 lines

I know, this is not directly related to the BKL, but it is an additional complication.

Depending on how the video buffers are implemented, you may need to run dequeue, in order to allow freeing the mmaped memories. That's said, maybe we could use a kref implementation for those kind or resources.

Quoted 7 lines

2) Add a new video_device flag that turns on serialization. Basically all calls are serialized with a mutex in v4l2_device. To handle blocking calls like read() or VIDIOC_DQBUF we can either not take the serialization mutex in the core, or instead the driver needs to unlock the mutex before it waits for an event and lock it afterwards.

Quoted 8 lines

In the first case the core has to know all the exceptions.

Perhaps we should just add a second flag: whether the core should do full serialization (and the driver will have to unlock/lock around blocking waits) or smart serialization where know blocking operations are allowed unserialized.

I think it is fairly simple to add this serialization mechanism. And for many drivers this will actually be more than enough.

I remember I proposed a solution to implement the mutex at V4L core level, when we had this discussion with Alan Cox BKL patches.

The conclusion I had from the discussion is that, while this is a simple way, it may end that a poorly implemented lock would stay there forever.

Also, core has no way to foresee what the driver is doing on their side, and may miss some cases where the lock needs to be used.

I don't think that adding flags would help to improve it.

I remember I had to do it on em28xx:

This is the init code for it:
       ...
       mutex_init(&dev->lock); mutex_lock(&dev->lock); em28xx_init_dev(&dev, udev, interface, nr);
       ...
       request_modules(dev);

       /* Should be the last thing to do, to avoid newer udev's to open the device before fully initializing it */
       mutex_unlock(&dev->lock);
       ...

And this is the open code:

static int em28xx_v4l2_open(struct file *filp)
{
       ...
       mutex_lock(&dev->lock);
       ...
       mutex_unlock(&dev->lock);

Quoted 16 lines · Mauro Carvalho Chehab

I remember I had to do it on em28xx:

This is the init code for it:

       ...
       mutex_init(&dev->lock);
       mutex_lock(&dev->lock);
       em28xx_init_dev(&dev, udev, interface, nr);
       ...
       request_modules(dev);

       /* Should be the last thing to do, to avoid newer udev's to
          open the device before fully initializing it
        */
       mutex_unlock(&dev->lock);
       ...

Quoted 9 lines

And this is the open code:

static int em28xx_v4l2_open(struct file *filp)

{
       ...
       mutex_lock(&dev->lock);
       ...
       mutex_unlock(&dev->lock);

It's probably worth noting that this change is actually pretty badly broken. Because the modules are loading asynchronously, there is a high probability that the em28xx-dvb driver will still be loading when hald connects in to the v4l device. That's the big reason people often see things like tvp5150 i2c errors when the driver is first loaded up.

It's a good idea in theory, but pretty fatally flawed due to the async loading (as to make it work properly you would have to do something like locking the mutex in em28xx and clearing it in em28xx-dvb at the end of its initialization).

Devin

If you take a look at em28xx-dvb, it is not lock-protected. If the bug is due to the async load, we'll need to add the same locking at *alsa and *dvb parts of em28xx.

Yet, in this specific case, as the errors are due to the reception of wrong data from tvp5150, maybe the problem is due to the lack of a proper lock at the i2c access.

Quoted 3 lines · Mauro Carvalho Chehab

If you take a look at em28xx-dvb, it is not lock-protected. If the bug is due to the async load, we'll need to add the same locking at *alsa and *dvb parts of em28xx.

Yes, that is correct. The problem effects both dvb and alsa, although empirically it is more visible with the dvb case.

Quoted 4 lines

Yet, in this specific case, as the errors are due to the reception of wrong data from tvp5150, maybe the problem is due to the lack of a proper lock at the i2c access.

The problem is because hald sees the new device and is making v4l2 calls against the tvp5150 even though the gpio has been toggled over to digital mode. Hence an i2c lock won't help.

We would need to implement proper locking of analog versus digital mode, which unfortunately would either result in hald getting back -EBUSY on open of the V4L device or the DVB module loading being deferred while the v4l side of the board is in use (neither of which is a very good solution).

This is what got me thinking a few weeks ago that perhaps the submodules should not be loaded asynchronously. In that case, at least the main em28xx module could continue to hold the lock while the submodules are still being loaded.

Devin

If the i2c lock was toggled to digital mode, then it means that the i2c code is being in use simultaneously by analog and digital mode. It also means that an i2c IR device, or alsa will have troubles also. So, we really need one i2c lock that will protect the access to the I2C bus as a hole, including the i2c gate.

Quoted 5 lines

We would need to implement proper locking of analog versus digital mode, which unfortunately would either result in hald getting back -EBUSY on open of the V4L device or the DVB module loading being deferred while the v4l side of the board is in use (neither of which is a very good solution).

Yes, this is also needed: we shouldn't let simultaneous stream access to the analog and digital mode at the same time, but I don't see, by itself, any problem on having both analog and digital nodes opened at the same time. So, the solution seems to lock some resources between digital/analog access.

Quoted 5 lines · Mauro Carvalho Chehab

If the i2c lock was toggled to digital mode, then it means that the i2c code is being in use simultaneously by analog and digital mode. It also means that an i2c IR device, or alsa will have troubles also. So, we really need one i2c lock that will protect the access to the I2C bus as a hole, including the i2c gate.

Most i2c locks typically are only held for the duration of a single i2c transaction. What you are proposing would likely result in just about every function having to explicitly lock/unlock, which just seems bound to be error prone.

Quoted 9 lines

Quoted 5 lines

We would need to implement proper locking of analog versus digital mode, which unfortunately would either result in hald getting back -EBUSY on open of the V4L device or the DVB module loading being deferred while the v4l side of the board is in use (neither of which is a very good solution).

Yes, this is also needed: we shouldn't let simultaneous stream access to the analog and digital mode at the same time, but I don't see, by itself, any problem on having both analog and digital nodes opened at the same time. So, the solution seems to lock some resources between digital/analog access.

I think this is probably a bad idea. The additional granularity provides you little benefit, and you could easily end up in situations where power is being continuously being toggled on the decoder and demodulator as ioctls come in from both analog and digital. The solution would probably not be too bad if you're only talking about boards where everything is powered up all the time (like most of the PCI boards), but this approach would be horrific for any board where power management were a concern (e.g. USB devices).

A fairly simple locking scheme at open() would prevent essentially all of race conditions, the change would only be in one or two places per bridge (as opposed to littering the code with locking logic), and the only constraint is that applications would have to be diligent in closing the device when its not in use.

We've got enough power management problems as it is without adding lots additional complexity with little benefit and only increasing the likelihood of buggy code.

Quoted 4 lines

Most i2c locks typically are only held for the duration of a single i2c transaction.  What you are proposing would likely result in just about every function having to explicitly lock/unlock, which just seems bound to be error prone.

The i2c open/close should be part of the transaction. Of course, there's no need to send a command to open an already opened gate (yet, from some sniff dumps, it seems that some drivers for other OS's do it for every single i2c access).

Quoted 3 lines

We've got enough power management problems as it is without adding lots additional complexity with little benefit and only increasing the likelihood of buggy code.

For sure a lock at the open() is simple, but I suspect that this may cause some troubles with applications that may just open everything on startup (even letting the device unused). Just as one example of such apps, kmix, pulseaudio and other alsa mixers love to keep the mixer node opened, even if nobody is using it.

Quoted 8 lines · Mauro Carvalho Chehab

Quoted 4 lines

Most i2c locks typically are only held for the duration of a single i2c transaction. What you are proposing would likely result in just about every function having to explicitly lock/unlock, which just seems bound to be error prone.

The i2c open/close should be part of the transaction. Of course, there's no need to send a command to open an already opened gate (yet, from some sniff dumps, it seems that some drivers for other OS's do it for every single i2c access).

I'm not even talking about i2c gate control, which is a whole separate case where it is applied inconsistently across drivers. Even under Linux, we have lots of cases where there are double opens and double closes of the i2c gate, depending on whether the developer is controlling the gate from the tuner driver or the demodulator.

What I'm getting at though is that the lock granularity today is typically at the i2c transaction level, so something like an demod driver attempting to set two disparate registers is likely to be two i2c transactions. Without moving the locking into the caller, the other half of the driver can take control between those two transactions. And moving the logic into the caller means we will have to litter the code all over the place with lock/unlock calls.

Quoted 8 lines

Quoted 3 lines

We've got enough power management problems as it is without adding lots additional complexity with little benefit and only increasing the likelihood of buggy code.

For sure a lock at the open() is simple, but I suspect that this may cause some troubles with applications that may just open everything on startup (even letting the device unused). Just as one example of such apps, kmix, pulseaudio and other alsa mixers love to keep the mixer node opened, even if nobody is using it.

I'm frankly far less worried about the ALSA devices than I am about DVB versus V4L Vdeo/VBI, based on all the feedback I see from real users.

The cases where we are getting continuously burned are MythTV users who don't have their "input groups" properly defined and as a result MythTV attempts to use both digital and analog at the same time (input groups themselves are really a hack to deal with the fact that the Linux kernel doesn't have any way to inform userland of the relationships).

And the more I think about it, we can probably even implement the locking itself in the V4L and DVB core (further reducing the risk of some bridge maintainer screwing it up). All the bridge driver would have to do is declare the relationship between the DVB and V4L devices (both video and vbi), and the enforcement of the locking could be abstracted out.

Quoted 5 lines · Mauro Carvalho Chehab

If the i2c lock was toggled to digital mode, then it means that the i2c code is being in use simultaneously by analog and digital mode. It also means that an i2c IR device, or alsa will have troubles also. So, we really need one i2c lock that will protect the access to the I2C bus as a hole, including the i2c gate.

Most i2c locks typically are only held for the duration of a single i2c transaction. What you are proposing would likely result in just about every function having to explicitly lock/unlock, which just seems bound to be error prone.

Quoted 10 lines

Quoted 5 lines

We would need to implement proper locking of analog versus digital mode, which unfortunately would either result in hald getting back -EBUSY on open of the V4L device or the DVB module loading being deferred while the v4l side of the board is in use (neither of which is a very good solution).

Yes, this is also needed: we shouldn't let simultaneous stream access to the analog and digital mode at the same time, but I don't see, by itself, any problem on having both analog and digital nodes opened at the same time. So, the solution seems to lock some resources between digital/analog access.

I think this is probably a bad idea. The additional granularity provides you little benefit, and you could easily end up in situations where power is being continuously being toggled on the decoder and demodulator as ioctls come in from both analog and digital. The solution would probably not be too bad if you're only talking about boards where everything is powered up all the time (like most of the PCI boards), but this approach would be horrific for any board where power management were a concern (e.g. USB devices).

A fairly simple locking scheme at open() would prevent essentially all of race conditions, the change would only be in one or two places per bridge (as opposed to littering the code with locking logic), and the only constraint is that applications would have to be diligent in closing the device when its not in use.

We've got enough power management problems as it is without adding lots additional complexity with little benefit and only increasing the likelihood of buggy code.

Devin

Quoted 3 lines · Mauro Carvalho Chehab

If you take a look at em28xx-dvb, it is not lock-protected. If the bug is due to the async load, we'll need to add the same locking at *alsa and *dvb parts of em28xx.

Yes, that is correct. The problem effects both dvb and alsa, although empirically it is more visible with the dvb case.

Quoted 3 lines

Yet, in this specific case, as the errors are due to the reception of wrong data from tvp5150, maybe the problem is due to the lack of a proper lock at the i2c access.

The problem is because hald sees the new device and is making v4l2 calls against the tvp5150 even though the gpio has been toggled over to digital mode. Hence an i2c lock won't help. We would need to implement proper locking of analog versus digital mode, which unfortunately would either result in hald getting back -EBUSY on open of the V4L device or the DVB module loading being deferred while the v4l side of the board is in use (neither of which is a very good solution).

This is what got me thinking a few weeks ago that perhaps the submodules should not be loaded asynchronously. In that case, at least the main em28xx module could continue to hold the lock while the submodules are still being loaded.

Devin

Quoted 20 lines · On Thursday 01 April 2010 20:29:52 Devin Heitmueller wrote:

Quoted 3 lines · Mauro Carvalho Chehab

If you take a look at em28xx-dvb, it is not lock-protected. If the bug is due to the async load, we'll need to add the same locking at *alsa and *dvb parts of em28xx.

Yes, that is correct. The problem effects both dvb and alsa, although empirically it is more visible with the dvb case.

Quoted 3 lines

Yet, in this specific case, as the errors are due to the reception of wrong data from tvp5150, maybe the problem is due to the lack of a proper lock at the i2c access.

The problem is because hald sees the new device and is making v4l2 calls against the tvp5150 even though the gpio has been toggled over to digital mode. Hence an i2c lock won't help. We would need to implement proper locking of analog versus digital mode, which unfortunately would either result in hald getting back -EBUSY on open of the V4L device or the DVB module loading being deferred while the v4l side of the board is in use (neither of which is a very good solution).

Quoted 4 lines

This is what got me thinking a few weeks ago that perhaps the submodules should not be loaded asynchronously. In that case, at least the main em28xx module could continue to hold the lock while the submodules are still being loaded.

What was the reason behind the asynchronous loading? In general it simplifies things a lot if you load modules up front.

Quoted 2 lines

What was the reason behind the asynchronous loading? In general it simplifies things a lot if you load modules up front.

The reason is to avoid a dead lock: driver A depends on symbols on B (the other driver init code) that depends on symbols at A (core stuff, locks, etc).

There are other approaches to avoid this trouble, like the attach method used by the DVB modules, but an asynchronous (and parallel) load offers another
advantage: it speeds up boot time, as other processors can take care of the load of the additonal modules.

On Thu, Apr 1, 2010 at 5:16 PM, Mauro Carvalho Chehab <[email protected]>

I think though that we need to favor stability/reliability over performance.

Quoted 16 lines · Mauro Carvalho Chehab

I remember I had to do it on em28xx:

This is the init code for it:

       ...
       mutex_init(&dev->lock);
       mutex_lock(&dev->lock);
       em28xx_init_dev(&dev, udev, interface, nr);
       ...
       request_modules(dev);

       /* Should be the last thing to do, to avoid newer udev's to
          open the device before fully initializing it
        */
       mutex_unlock(&dev->lock);
       ...

Quoted 9 lines

And this is the open code:

static int em28xx_v4l2_open(struct file *filp)

{
       ...
       mutex_lock(&dev->lock);
       ...
       mutex_unlock(&dev->lock);

It's probably worth noting that this change is actually pretty badly broken. Because the modules are loading asynchronously, there is a high probability that the em28xx-dvb driver will still be loading when hald connects in to the v4l device. That's the big reason people often see things like tvp5150 i2c errors when the driver is first loaded up.

It's a good idea in theory, but pretty fatally flawed due to the async loading (as to make it work properly you would have to do something like locking the mutex in em28xx and clearing it in em28xx-dvb at the end of its initialization).

Quoted 35 lines · Devin Heitmueller

Quoted 16 lines · Mauro Carvalho Chehab

I remember I had to do it on em28xx:

This is the init code for it:
       ...
       mutex_init(&dev->lock); mutex_lock(&dev->lock); em28xx_init_dev(&dev, udev, interface, nr);
       ...
       request_modules(dev);

       /* Should be the last thing to do, to avoid newer udev's to open the device before fully initializing it */
       mutex_unlock(&dev->lock);
       ...

Quoted 9 lines

And this is the open code:

static int em28xx_v4l2_open(struct file *filp)
{
       ...
       mutex_lock(&dev->lock);
       ...
       mutex_unlock(&dev->lock);

It's probably worth noting that this change is actually pretty badly broken.  Because the modules are loading asynchronously, there is a high probability that the em28xx-dvb driver will still be loading when hald connects in to the v4l device.  That's the big reason people often see things like tvp5150 i2c errors when the driver is first loaded up.

Quoted 4 lines

It's a good idea in theory, but pretty fatally flawed due to the async loading (as to make it work properly you would have to do something like locking the mutex in em28xx and clearing it in em28xx-dvb at the end of its initialization).

Devin,

I found some time to fix the above reported issue. Patch follows.

---

V4L/DVB: em28xx: fix locks during dvb init sequence

During em28xx init, em28xx-dvb needs to change to digital mode, in order to properly initialize. However, as soon as em28xx-video registers /dev/video0, udev will try to run v4l_id program, to retrieve some information that it is needed by udev device creation.

So, while v4l_id is opening the /dev/video? device and setting the device in analog mode, the em28xx-dvb is putting the same device on digital mode, and trying to initialize the DVB demod.

On devices that have a I2C bridge, this results on one of the devices to not being accessed, either resulting on I2C error or on wrong readings at devices like tvp5150.

As the analog operations are protected by dev->lock, the fix is as simple as locking it also during em28xx-dvb initialization.

While here, also simplifies the locking schema for the extension register/unregister functions.

Tested on WinTV HVR-950 (2040:6513), doing several sequences of unload/reload. On all cases, the proper init happened:

[ 1075.497596] tvp5150 2-005c: tvp5150am1 detected.
[ 1075.647916] xc2028 2-0061: attaching existing instance
[ 1075.653106] xc2028 2-0061: type set to XCeive xc2028/xc3028 tuner
[ 1075.659254] em28xx #0: em28xx #0/2: xc3028 attached

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/video/em28xx/em28xx-core.c +4 -9

index d4f4525..c8c4e8f 100644
@@ -1177,21 +1177,18 @@ void em28xx_add_into_devlist(struct em28xx *dev)
  */
 
  static LIST_HEAD(em28xx_extension_devlist);
-static DEFINE_MUTEX(em28xx_extension_devlist_lock);
 
  int em28xx_register_extension(struct em28xx_ops *ops)
  {
        struct em28xx *dev = NULL;
 
        mutex_lock(&em28xx_devlist_mutex);
-       mutex_lock(&em28xx_extension_devlist_lock);
        list_add_tail(&ops->next, &em28xx_extension_devlist);
        list_for_each_entry(dev, &em28xx_devlist, devlist) {
                if (dev)
                        ops->init(dev);
        }
        printk(KERN_INFO "Em28xx: Initialized (%s) extension\n", ops->name);
-       mutex_unlock(&em28xx_extension_devlist_lock);
        mutex_unlock(&em28xx_devlist_mutex);
        return 0;
  }

@@ -1207,10 +1204,8 @@ void em28xx_unregister_extension(struct em28xx_ops *ops)
                        ops->fini(dev);
        }
 
-       mutex_lock(&em28xx_extension_devlist_lock);
        printk(KERN_INFO "Em28xx: Removed (%s) extension\n", ops->name);
        list_del(&ops->next);
-       mutex_unlock(&em28xx_extension_devlist_lock);
        mutex_unlock(&em28xx_devlist_mutex);
  }
  EXPORT_SYMBOL(em28xx_unregister_extension);

@@ -1219,26 +1214,26 @@ void em28xx_init_extension(struct em28xx *dev)
  {
        struct em28xx_ops *ops = NULL;
 
-       mutex_lock(&em28xx_extension_devlist_lock);
+       mutex_lock(&em28xx_devlist_mutex);
        if (!list_empty(&em28xx_extension_devlist)) {
                list_for_each_entry(ops, &em28xx_extension_devlist, next) {
                        if (ops->init)
                                ops->init(dev);
                }
        }
-       mutex_unlock(&em28xx_extension_devlist_lock);
+       mutex_unlock(&em28xx_devlist_mutex);
  }
 
  void em28xx_close_extension(struct em28xx *dev)
  {
        struct em28xx_ops *ops = NULL;
 
-       mutex_lock(&em28xx_extension_devlist_lock);
+       mutex_lock(&em28xx_devlist_mutex);
        if (!list_empty(&em28xx_extension_devlist)) {
                list_for_each_entry(ops, &em28xx_extension_devlist, next) {
                        if (ops->fini)
                                ops->fini(dev);
                }
        }
-       mutex_unlock(&em28xx_extension_devlist_lock);
+       mutex_unlock(&em28xx_devlist_mutex);
  }

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

index 8f23aa1..f0de731 100644
@@ -466,6 +466,7 @@ static int dvb_init(struct em28xx *dev)
        }
        dev->dvb = dvb;
 
+       mutex_lock(&dev->lock);
        em28xx_set_mode(dev, EM28XX_DIGITAL_MODE);
        /* init frontend */
        switch (dev->model) {

@@ -589,15 +590,16 @@ static int dvb_init(struct em28xx *dev)
        if (result < 0)
                goto out_free;
 
-       em28xx_set_mode(dev, EM28XX_SUSPEND);
        em28xx_info("Successfully loaded em28xx-dvb\n");
-       return 0;
+ret:
+       em28xx_set_mode(dev, EM28XX_SUSPEND);
+       mutex_unlock(&dev->lock);
+       return result;
 

 out_free:
-       em28xx_set_mode(dev, EM28XX_SUSPEND);
       kfree(dvb);
       dev->dvb = NULL;
-       return result;
+       goto ret;
 }

 static int dvb_fini(struct em28xx *dev)

On Wed, Apr 7, 2010 at 4:07 PM, Mauro Carvalho Chehab

At first glance, this looks really promising. I will have to look at this in more detail when I have access to the source code (I'm at the office right now).

On the DVB side there seem to be only two sources that use the BKL:

linux/drivers/media/dvb/bt8xx/dst_ca.c: lock_kernel(); linux/drivers/media/dvb/bt8xx/dst_ca.c: unlock_kernel();

linux/drivers/media/dvb/dvb-core/dvbdev.c: lock_kernel();
linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel();
linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel();

Quoted 3 lines

linux/drivers/media/dvb/dvb-core/dvbdev.c: lock_kernel();
linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel();
linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel();

This is from when the BKL was pushed down into drivers' open() methods. To remove BKL from open(), check for possible races with module insertion. (A driver's module_init has to have set up everything that's going to be used by open() before the char device is being registered.)

I just read on LWN that the core kernel guys are putting more effort into removing the BKL. We are still using it in our own drivers, mostly V4L.

I added a BKL column to my driver list:

http://www.linuxtv.org/wiki/index.php/V4L_framework_progress#Bridge_Drivers

If you 'own' one of these drivers that still use BKL, then it would be nice if you can try and remove the use of the BKL from those drivers.

The other part that needs to be done is to move from using the .ioctl file op to using .unlocked_ioctl. Very few drivers do that, but I suspect almost no driver actually needs to use .ioctl.

On the DVB side there seem to be only two sources that use the BKL:

linux/drivers/media/dvb/bt8xx/dst_ca.c: lock_kernel(); linux/drivers/media/dvb/bt8xx/dst_ca.c: unlock_kernel();
linux/drivers/media/dvb/dvb-core/dvbdev.c: lock_kernel();
linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel();
linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel();

At first glance it doesn't seem too difficult to remove them, but I leave that to the DVB experts.

Quoted 5 lines · Hans Verkuil

I just read on LWN that the core kernel guys are putting more effort into removing the BKL. We are still using it in our own drivers, mostly V4L.

I added a BKL column to my driver list:

Quoted 8 lines

http://www.linuxtv.org/wiki/index.php/V4L_framework_progress#Bridge_Drivers

If you 'own' one of these drivers that still use BKL, then it would be nice if you can try and remove the use of the BKL from those drivers.

The other part that needs to be done is to move from using the .ioctl file op to using .unlocked_ioctl. Very few drivers do that, but I suspect almost no driver actually needs to use .ioctl.

Also note that struct file_operations.llseek() grabs the BKL if .llseek = default_llseek, or if .llseek == NULL && (struct file.f_mode & FMODE_LSEEK) != 0.

I guess V4L/DVB character device file ABIs do not involve lseek() and friends, do they? If so, are the files flagged as non-seekable?

Quoted 8 lines

On the DVB side there seem to be only two sources that use the BKL:

linux/drivers/media/dvb/bt8xx/dst_ca.c: lock_kernel(); linux/drivers/media/dvb/bt8xx/dst_ca.c: unlock_kernel();
linux/drivers/media/dvb/dvb-core/dvbdev.c: lock_kernel();
linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel();
linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel();

Quoted 2 lines

At first glance it doesn't seem too difficult to remove them, but I leave that to the DVB experts.

As a dvb/firewire/firedtv user, I started to mess around with dvbdev and
firedtv: https://patchwork.kernel.org/patch/88778/

Hi all,

I just read on LWN that the core kernel guys are putting more effort into removing the BKL. We are still using it in our own drivers, mostly V4L.

I added a BKL column to my driver list:

http://www.linuxtv.org/wiki/index.php/V4L_framework_progress#Bridge_Drivers

If you 'own' one of these drivers that still use BKL, then it would be nice if you can try and remove the use of the BKL from those drivers.

The other part that needs to be done is to move from using the .ioctl file op to using .unlocked_ioctl. Very few drivers do that, but I suspect almost no driver actually needs to use .ioctl.

On the DVB side there seem to be only two sources that use the BKL:

linux/drivers/media/dvb/bt8xx/dst_ca.c: lock_kernel(); linux/drivers/media/dvb/bt8xx/dst_ca.c: unlock_kernel();
linux/drivers/media/dvb/dvb-core/dvbdev.c: lock_kernel();
linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel();
linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel();

At first glance it doesn't seem too difficult to remove them, but I leave that to the DVB experts.

Quoted 8 lines · Hans Verkuil

On the DVB side there seem to be only two sources that use the BKL:

linux/drivers/media/dvb/bt8xx/dst_ca.c: lock_kernel(); linux/drivers/media/dvb/bt8xx/dst_ca.c: unlock_kernel();
linux/drivers/media/dvb/dvb-core/dvbdev.c: lock_kernel();
linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel();
linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel();

Quoted 2 lines

At first glance it doesn't seem too difficult to remove them, but I leave that to the DVB experts.

The main issue is at dvbdev, since it is used by all devices. We need to get rid of it.

That's said, Stefan Richter sent a patch meant to reduce the issues with DVB. Unfortunately, I haven't seen any comments on it. It would be really important to test his approach.