[PATCH] em28xx: fix locks during dvb init sequence - was: Re: V4L-DVB drivers and BKL
Devin Heitmueller wrote:
On Thu, Apr 1, 2010 at 11:02 AM, Mauro Carvalho Chehab
mche...@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 mche...@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)
Re: [PATCH] em28xx: fix locks during dvb init sequence - was: Re: V4L-DVB drivers and BKL
On Wed, Apr 7, 2010 at 4:07 PM, Mauro Carvalho Chehab
mche...@redhat.com wrote:
Devin Heitmueller wrote:
On Thu, Apr 1, 2010 at 11:02 AM, Mauro Carvalho Chehab
mche...@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 mche...@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
Re: [PATCH] em28xx: fix locks during dvb init sequence - was: Re: V4L-DVB drivers and BKL
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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
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/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
On Thu, 2010-04-01 at 17:29 -0400, Devin Heitmueller wrote: On Thu, Apr 1, 2010 at 5:16 PM, Mauro Carvalho Chehab mche...@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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
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/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
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/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
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/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
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
Re: V4L-DVB drivers and BKL
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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
On Thu, Apr 1, 2010 at 11:02 AM, Mauro Carvalho Chehab
mche...@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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
Devin Heitmueller wrote:
On Thu, Apr 1, 2010 at 11:02 AM, Mauro Carvalho Chehab
mche...@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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
On Thu, Apr 1, 2010 at 1:36 PM, Mauro Carvalho Chehab mche...@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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
Devin Heitmueller wrote: On Thu, Apr 1, 2010 at 1:36 PM, Mauro Carvalho Chehab mche...@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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
On Thu, Apr 1, 2010 at 2:42 PM, Mauro Carvalho Chehab mche...@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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
On Thursday 01 April 2010 20:29:52 Devin Heitmueller wrote: On Thu, Apr 1, 2010 at 1:36 PM, Mauro Carvalho Chehab mche...@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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
Devin Heitmueller wrote: On Thu, Apr 1, 2010 at 2:42 PM, Mauro Carvalho Chehab mche...@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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
On Thursday 01 April 2010 20:56:19 Devin Heitmueller wrote: On Thu, Apr 1, 2010 at 2:42 PM, Mauro Carvalho Chehab mche...@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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
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 mche...@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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
On Thu, Apr 1, 2010 at 5:16 PM, Mauro Carvalho Chehab mche...@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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: V4L-DVB drivers and BKL
On Thu, Apr 1, 2010 at 5:07 PM, Mauro Carvalho Chehab mche...@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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Aw: Re: V4L-DVB drivers and BKL
Hi! - Original Nachricht Von: Mauro Carvalho Chehab mche...@redhat.com An: Devin Heitmueller dheitmuel...@kernellabs.com Datum: 02.04.2010 01:10 Betreff: Re: V4L-DVB drivers and BKL Devin Heitmueller wrote: On Thu, Apr 1, 2010 at 5:07 PM, Mauro Carvalho Chehab mche...@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). We see the same on other OSs as well. In fact, to use digital and analog at once is totally valid. It is much too short to think about a device with a single hybrid tuner, best known meanwhile for causing troubles. The Medion Quad (md8080) on the saa713x with two hybrid tuners, two DVB-S tuners, four digital and two analog demodulators with two dedicated PCI bridges, with its restrictions too, is already ancient stuff. 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. On older dual tuner, triple and quad stuff it needs granularity through each driver down to the physically existing device. No app on any OS seems to have it right. In best case they have some framework around to ask the user about his knowledge for what is a go and what a no go. Newer hardware can really do triple stuff at once for example. Nothing much left to configure wrong from the user and the app. But, to start simple, if a single bridge can pass DVB-T and DVB-S at once is as important to know these days as all details about tuners, demodulators and SECs on a given device. That mixture of old and new will continue over years anyway and to escape with some easy rules doesn't look simple. Some either digital or analog does not exist, except for a single hybrid tuner. Even then, on such a simplest hybrid device, the tuner can be in digital mode and without any problems you can have analog video from external inputs at once. 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
