V4L-DVB drivers and BKL

V4L-DVB drivers and BKL

[Hans Verkuil]

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.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: V4L-DVB drivers and BKL

[Laurent Pinchart]

Hi Hans,

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
> 
> 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 ?

diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 7090699..14e1b1c 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -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.

-- 
Regards,

Laurent Pinchart

Re: V4L-DVB drivers and BKL

[Hans Verkuil]

On Thursday 01 April 2010 11:23:30 Laurent Pinchart wrote:
> Hi Hans,
> 
> 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
> > 
> > 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 ?

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

Regards,

	Hans

> 
> diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
> index 7090699..14e1b1c 100644
> --- a/drivers/media/video/v4l2-dev.c
> +++ b/drivers/media/video/v4l2-dev.c
> @@ -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.
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: V4L-DVB drivers and BKL

[Stefan Richter]

Hans Verkuil wrote:
> 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.

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?

> 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.

As a dvb/firewire/firedtv user, I started to mess around with dvbdev and
firedtv:  https://patchwork.kernel.org/patch/88778/
-- 
Stefan Richter
-=====-==-=- -=-- ----=
http://arcgraph.de/sr/

Re: V4L-DVB drivers and BKL

[Stefan Richter]

Hans Verkuil wrote:
> 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();

This is from an incomplete conversion from .ioctl to .unlocked_ioctl (no
conversion really, only a BKL push-down).

> 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.)

Apart from those two BKL uses in media/dvb/, there are also
  - remaining .ioctl that need to be checked for possible concurrency
    issues, then converted to .unlocked_ioctl,
  - remaining .llseek uses (all implicit) which need to be checked
    whether they should be no_llseek() (accompanied by nonseekable_open)
    or generic_file_llseek() or default_llseek().
-- 
Stefan Richter
-=====-==-=- -=-- ----=
http://arcgraph.de/sr/

Re: V4L-DVB drivers and BKL

[Laurent Pinchart]

Hi Hans,

On Thursday 01 April 2010 13:11:51 Hans Verkuil wrote:
> On Thursday 01 April 2010 11:23:30 Laurent Pinchart wrote:
> > 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.
> > > 
> > > 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.

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. 

> > diff --git a/drivers/media/video/v4l2-dev.c
> > b/drivers/media/video/v4l2-dev.c index 7090699..14e1b1c 100644
> > --- a/drivers/media/video/v4l2-dev.c
> > +++ b/drivers/media/video/v4l2-dev.c
> > @@ -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.

-- 
Regards,

Laurent Pinchart

Re: V4L-DVB drivers and BKL

[Hans Verkuil]

On Thursday 01 April 2010 13:57:13 Stefan Richter wrote:
> Hans Verkuil wrote:
> > 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.
> 
> 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?

They are. The file op .llseek is always set to no_llseek for v4l. DVB seems
to leave it at NULL but I don't believe it is ever implemented.

> > 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.
> 
> As a dvb/firewire/firedtv user, I started to mess around with dvbdev and
> firedtv:  https://patchwork.kernel.org/patch/88778/
> 

Great!

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: V4L-DVB drivers and BKL

[Stefan Richter]

Stefan Richter wrote:
>> 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.)

Last sentence was supposed to mean:  Before the char device is being
registered, a driver's module_init has to have set up everything that's
going to be used by openers of the file.

(Traditionally, the BKL serialized open() with module initialization,
which was not obvious to driver writers because it happened deep in the
core kernel.)
-- 
Stefan Richter
-=====-==-=- -=-- ----=
http://arcgraph.de/sr/

Re: V4L-DVB drivers and BKL

[Mauro Carvalho Chehab]

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
> 
> 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.

The removal of BKL is generally as simple as review the locks inside the driver,
being sure that an ioctl won't interfere on another ioctl, or on open/close ops.

> 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.

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. It will probably come a time where the drivers that still
uses BKL will stop working, as they will remove BKL. I remember that, during KS/2009, 
it was proposed by someone to just mark all drivers that use BKL as BROKEN. This
didn't happen (yet), but I don't doubt it will happen on the next few kernel versions.

-- 

Cheers,
Mauro

Re: V4L-DVB drivers and BKL

[Mauro Carvalho Chehab]

Laurent Pinchart wrote:
> Hi Hans,
> 
> On Thursday 01 April 2010 13:11:51 Hans Verkuil wrote:
>> On Thursday 01 April 2010 11:23:30 Laurent Pinchart wrote:
>>> 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.
>>>>
>>>> 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.
> 
> 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.

-- 

Cheers,
Mauro

Re: V4L-DVB drivers and BKL

[Laurent Pinchart]

Hi Mauro,

On Thursday 01 April 2010 16:12:50 Mauro Carvalho Chehab wrote:
> Laurent Pinchart wrote:
> > On Thursday 01 April 2010 13:11:51 Hans Verkuil wrote:
> >> On Thursday 01 April 2010 11:23:30 Laurent Pinchart wrote:
> >>> 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.
> >>>> 
> >>>> 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.
> > 
> > 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.

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.

> 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.

-- 
Regards,

Laurent Pinchart

Re: V4L-DVB drivers and BKL

[Hans Verkuil]

On Thursday 01 April 2010 16:12:50 Mauro Carvalho Chehab wrote:
> Laurent Pinchart wrote:
> > Hi Hans,
> > 
> > On Thursday 01 April 2010 13:11:51 Hans Verkuil wrote:
> >> On Thursday 01 April 2010 11:23:30 Laurent Pinchart wrote:
> >>> 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.
> >>>>
> >>>> 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.
> > 
> > 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.

Correct.

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?

> 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.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: V4L-DVB drivers and BKL

[Mauro Carvalho Chehab]

Laurent Pinchart wrote:

>> 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.

Before the BKL changes, open were allowed only after the full module loading.

>> 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.

You may have, as one file handler may be doing an ioctl, while another application
opens or closes another file handler. Depending on what the driver have on the open
handler, it might interfere on the ioctl.

> 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.

Yes, that's my point. It is not hard to write such patch, moving from BKL to an
ioctl/open/close mutex, and it should be safe, provided that it doesn't introduce
any dead lock with some existing mutexes.

-- 

Cheers,
Mauro

Re: V4L-DVB drivers and BKL

[Mauro Carvalho Chehab]

Hans Verkuil wrote:

> 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.


> 
>> 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.

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.

> 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.

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.

-- 

Cheers,
Mauro

Re: V4L-DVB drivers and BKL

[Hans Verkuil]

On Thursday 01 April 2010 17:02:01 Mauro Carvalho Chehab wrote:
> Hans Verkuil wrote:
> 
> > 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,

Yes, that is very annoying when you also have to do firmware uploads. The ivtv
driver does that on the first open, but that doesn't help anymore when hal opens
it automatically.

> 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.
> 
> 
> > 
> >> 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.
> 
> 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.

What should be the correct sequence in an application when the device it is
capturing from is suddenly unplugged?

I guess it is a STREAMOFF followed by a REQBUFS with a count of 0. At least
according to the spec (videobuf doesn't accept a count of 0 at the moment).

So those two ioctls would need to be allowed through.

> 
> > 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.
> 
> 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.

Why not? Seriously, most drivers only need a simple serialization flag.
Adding this feature in the core by just setting a V4L2_FL_SERIALIZE flag
is easy to do and it is very simple to implement and review. Given the fact
that a lot of drivers (esp. older ones) have very poor locking schemes I am
very much in favor of having basic serialization support in the v4l core.

I'll see if I can make a patch for this to help this discussion.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: V4L-DVB drivers and BKL

[Devin Heitmueller]

On Thu, Apr 1, 2010 at 11:02 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> 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);
>

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

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: V4L-DVB drivers and BKL

[Mauro Carvalho Chehab]

Devin Heitmueller wrote:
> On Thu, Apr 1, 2010 at 11:02 AM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> 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);
>>
> 
> 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).

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. 


> 
> Devin
> 


-- 

Cheers,
Mauro

Re: V4L-DVB drivers and BKL

[Devin Heitmueller]

On Thu, Apr 1, 2010 at 1:36 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> 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.

> 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

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: V4L-DVB drivers and BKL

[Mauro Carvalho Chehab]

Devin Heitmueller wrote:
> On Thu, Apr 1, 2010 at 1:36 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> 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.

In the case of the initialization, the lock is needed, even if we fing a
way to load the module synchronously.

> 
>> 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. 

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.

> 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.
 
> 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
> 


-- 

Cheers,
Mauro

Re: V4L-DVB drivers and BKL

[Devin Heitmueller]

On Thu, Apr 1, 2010 at 2:42 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> 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.

>> 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

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: V4L-DVB drivers and BKL

[Hans Verkuil]

On Thursday 01 April 2010 20:29:52 Devin Heitmueller wrote:
> On Thu, Apr 1, 2010 at 1:36 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
> > 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.
> 
> > 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.

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

Regards,

	Hans

> 
> Devin
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: V4L-DVB drivers and BKL

[Mauro Carvalho Chehab]

Devin Heitmueller wrote:
> On Thu, Apr 1, 2010 at 2:42 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> 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.

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).

>>> 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.

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.

-- 

Cheers,
Mauro

Re: V4L-DVB drivers and BKL

[Hans Verkuil]

On Thursday 01 April 2010 20:56:19 Devin Heitmueller wrote:
> On Thu, Apr 1, 2010 at 2:42 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
> > 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.
> 
> >> 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.

I agree. The biggest problem with v4l-dvb devices is driver complexity. It
has never been performance. Reducing that complexity by moving some of that
into the core is a good thing in my view.

> 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.

Right.

	Hans

> 
> Devin
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: V4L-DVB drivers and BKL

[Mauro Carvalho Chehab]

Hans Verkuil wrote:
> On Thursday 01 April 2010 20:29:52 Devin Heitmueller wrote:
>> On Thu, Apr 1, 2010 at 1:36 PM, Mauro Carvalho Chehab
>> <mchehab@redhat.com> wrote:
>>> 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.
>>
>>> 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.
> 
> 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.

-- 

Cheers,
Mauro

Re: V4L-DVB drivers and BKL

[Devin Heitmueller]

On Thu, Apr 1, 2010 at 5:16 PM, Mauro Carvalho Chehab <mchehab@redhat.com>
>> 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).

I believe these problems can be avoided with a common entry point for
initializing the DVB submodule (where the loading of the subordinate
module sets a pointer to a function for the main module to call).  In
fact, doesn't em28xx do that today with it's "init" function for its
submodules?

> 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.

I think though that we need to favor stability/reliability over
performance.  In this case, I have seen a number of bridges where
having a "-dvb.ko" exposes this race, and I would much rather have it
take another 200ms to load the driver than continue to deal with
intermittent problems with hardware being in an unknown state.  Don't
quote me on this number, but on em28xx I run into problems about 20%
of the time where the dvb device fails to get created successfully
because of this race (forcing me to reboot the system).

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: V4L-DVB drivers and BKL

[Devin Heitmueller]

On Thu, Apr 1, 2010 at 5:07 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
>> 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.

>> 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.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: V4L-DVB drivers and BKL

[Mauro Carvalho Chehab]

Devin Heitmueller wrote:
> On Thu, Apr 1, 2010 at 5:07 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>>> 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.

I agree with a caller logic.

Yet, even moving to the caller, an i2c lock is still needed. For example, i2c IR
events are polling at interrupt time, so, they can happen anytime, including
in the middle of one i2c transaction (by transaction, I mean gate control+i2c
read/write ops go get/set a single register).

>>> 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.

Ok, but alsa driver may also try to access things like i2c. For example,
msp34xx audio controls are reached via I2C.

> 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.

I agree on having some sort of resource locking in core, but this type
of lock between radio/audio/analog/analog mpeg-encoded/digital/vbi/...
is different than a mere mutex-type of locking from what we've discussed
so far. It has nothing to do with BKL.

-- 

Cheers,
Mauro

Re: V4L-DVB drivers and BKL

[Andy Walls]

On Thu, 2010-04-01 at 17:29 -0400, Devin Heitmueller wrote:
> On Thu, Apr 1, 2010 at 5:16 PM, Mauro Carvalho Chehab <mchehab@redhat.com>

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

You mean doing the wrong thing, as fast as you can, isn't performing? ;)

I usually consider performance with two broad criteria

	- requirements correctly implemented (can be a weighted sum)
	- efficient use of limited resources (usually time or bandwidth)

Regards,
Andy

Re: V4L-DVB drivers and BKL

[Stefan Richter]

Mauro Carvalho Chehab wrote:
> Hans Verkuil wrote:
>> 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.
> 
> The main issue is at dvbdev, since it is used by all devices. We need to get rid
> of it.

Get rid of its lock_kernel in open and its locked ioctl, or of the
dvbdev 'library' itself?

> 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.

I will attempt to continue with this (convert more drivers if not all,
in a properly organized patch series for review), though it won't happen
overnight as I'm chronically short of spare time.  I.e. if others step
in, even better.
-- 
Stefan Richter
-=====-==-=- -=-- ---==
http://arcgraph.de/sr/

[PATCH] em28xx: fix locks during dvb init sequence - was: Re: V4L-DVB drivers and BKL

[Mauro Carvalho Chehab]

Devin Heitmueller wrote:
> On Thu, Apr 1, 2010 at 11:02 AM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> 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);
>>
> 
> 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,

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 <mchehab@redhat.com>

diff --git a/drivers/media/video/em28xx/em28xx-core.c b/drivers/media/video/em28xx/em28xx-core.c
index d4f4525..c8c4e8f 100644
--- a/drivers/media/video/em28xx/em28xx-core.c
+++ b/drivers/media/video/em28xx/em28xx-core.c
@@ -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);
 }
diff --git a/drivers/media/video/em28xx/em28xx-dvb.c b/drivers/media/video/em28xx/em28xx-dvb.c
index 8f23aa1..f0de731 100644
--- a/drivers/media/video/em28xx/em28xx-dvb.c
+++ b/drivers/media/video/em28xx/em28xx-dvb.c
@@ -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)

Re: [PATCH] em28xx: fix locks during dvb init sequence - was: Re: V4L-DVB drivers and BKL

[Devin Heitmueller]

On Wed, Apr 7, 2010 at 4:07 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Devin Heitmueller wrote:
>> On Thu, Apr 1, 2010 at 11:02 AM, Mauro Carvalho Chehab
>> <mchehab@redhat.com> wrote:
>>> 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);
>>>
>>
>> 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,
>
> 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 <mchehab@redhat.com>
>
> diff --git a/drivers/media/video/em28xx/em28xx-core.c b/drivers/media/video/em28xx/em28xx-core.c
> index d4f4525..c8c4e8f 100644
> --- a/drivers/media/video/em28xx/em28xx-core.c
> +++ b/drivers/media/video/em28xx/em28xx-core.c
> @@ -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);
>  }
> diff --git a/drivers/media/video/em28xx/em28xx-dvb.c b/drivers/media/video/em28xx/em28xx-dvb.c
> index 8f23aa1..f0de731 100644
> --- a/drivers/media/video/em28xx/em28xx-dvb.c
> +++ b/drivers/media/video/em28xx/em28xx-dvb.c
> @@ -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)
>

Hello Mauro,

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).

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH] em28xx: fix locks during dvb init sequence - was: Re: V4L-DVB drivers and BKL

[Mauro Carvalho Chehab]

Devin Heitmueller wrote:
> 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).

Ok. Please test it when you have some time, for me to apply it upstream. 
On my tests, it worked like a charm, and the Kernel circular lock detector 
also didn't complain (it is always a good idea to have it turn on when 
touching on locks).

-- 

Cheers,
Mauro