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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
