| Message ID | 200908061709.41211.laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Mauro Carvalho Chehab |
| Headers | show |
Hi Laurent, > Hi everybody, > > this patch moves the BKL one level down by removing the non-unlocked ioctl > in > v4l2-dev.c and calling lock_kernel/unlock_kernel in the unlocked_ioctl > handler > if the driver only supports locked ioctl. > > Opinions/comments/applause/kicks ? I've been thinking about this as well, and my idea was to properly implement this by letting the v4l core serialize ioctls if the driver doesn't do its own serialization (either through mutexes or lock_kernel). The driver can just set a flag in video_device if it wants to do serialization manually, otherwise the core will serialize using a mutex and we should be able to completely remove the BKL from all v4l drivers. I was actually planning an RFC for this myself, but you've beaten me to it :-) Regards, Hans
Hi Hans, On Thursday 06 August 2009 17:16:08 Hans Verkuil wrote: > Hi Laurent, > > > Hi everybody, > > > > this patch moves the BKL one level down by removing the non-unlocked > > ioctl in v4l2-dev.c and calling lock_kernel/unlock_kernel in the > > unlocked_ioctl handler if the driver only supports locked ioctl. > > > > Opinions/comments/applause/kicks ? > > I've been thinking about this as well, and my idea was to properly > implement this by letting the v4l core serialize ioctls if the driver > doesn't do its own serialization (either through mutexes or lock_kernel). A v4l-specific (or even device-specific) mutex would of course be better than the BKL. Are there file operations other than ioctl that are protected by the BKL ? Blindly replacing the BKL by a mutex on ioctl would then introduce race conditions. > The driver can just set a flag in video_device if it wants to do > serialization manually, otherwise the core will serialize using a mutex > and we should be able to completely remove the BKL from all v4l drivers. Whether the driver fills v4l2_operations::ioctl or v4l2_operations::unlocked_ioctl can be considered as such a flag :-) Many drivers are currently using the BKL in an unlocked_ioctl handler. I'm not sure it would be a good idea to move the BKL back to the v4l2 core, as the long term goal is to remove it completely and use fine-grain driver-level locking. Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 07 August 2009 13:03:22 Laurent Pinchart wrote: > Hi Hans, > > On Thursday 06 August 2009 17:16:08 Hans Verkuil wrote: > > Hi Laurent, > > > > > Hi everybody, > > > > > > this patch moves the BKL one level down by removing the non-unlocked > > > ioctl in v4l2-dev.c and calling lock_kernel/unlock_kernel in the > > > unlocked_ioctl handler if the driver only supports locked ioctl. > > > > > > Opinions/comments/applause/kicks ? > > > > I've been thinking about this as well, and my idea was to properly > > implement this by letting the v4l core serialize ioctls if the driver > > doesn't do its own serialization (either through mutexes or lock_kernel). > > A v4l-specific (or even device-specific) mutex would of course be better than > the BKL. > > Are there file operations other than ioctl that are protected by the BKL ? > Blindly replacing the BKL by a mutex on ioctl would then introduce race > conditions. I'd say that drivers that add sysfs or proc entries might be a problem, but there are few drivers that do that and I suspect that quite a few have their own locking. Allowing the mutex to be accessed from elsewhere would be a simple solution to this problem as all the sysfs/proc accesses can just attempt to get the lock first, thus doing proper serializing. > > The driver can just set a flag in video_device if it wants to do > > serialization manually, otherwise the core will serialize using a mutex > > and we should be able to completely remove the BKL from all v4l drivers. > > Whether the driver fills v4l2_operations::ioctl or > v4l2_operations::unlocked_ioctl can be considered as such a flag :-) Basically there are three situations: 1) unlocked_ioctl is set: in that case the driver takes care of all the locking. 2) ioctl is set: in that case we keep the old BKL behavior. 3) unlocked_ioctl is set together with a serialize flag in struct v4l2_device: in that case the v4l2 core will take care of most of the serialization for the driver. > Many drivers are currently using the BKL in an unlocked_ioctl handler. I'm not > sure it would be a good idea to move the BKL back to the v4l2 core, as the > long term goal is to remove it completely and use fine-grain driver-level > locking. I agree with that. But I think the best approach to be able to remove the BKLs is to provide a good alternative. Doing proper locking in a driver is quite hard. Doing it in the v4l2 core is a lot easier to do right. If the relevant file_operations are properly serialized, then that will simplify drivers enormously. And it is also fairly easy to optimize certain ioctls: e.g. the VIDIOC_QUERYCAP ioctl does not normally need a lock since it does not access anything that needs to be protected. Intelligence like that can be enabled on demand by setting a flag as well. Basically I have no trust in device driver writers to do locking right, and yes, that includes myself. It's hard to be sure that you covered all your bases, especially for complicated drivers like most v4l drivers are. Letting the core do most of the heavy lifting, even if you get somewhat sub-optimal locking, is more than good enough in almost all cases. And I expect that it is definitely more than good enough for drivers that use the BKL. Regards, Hans
diff -r 4533a406fddb linux/drivers/media/video/v4l2-dev.c --- a/linux/drivers/media/video/v4l2-dev.c Thu Aug 06 16:41:17 2009 +0200 +++ b/linux/drivers/media/video/v4l2-dev.c Thu Aug 06 17:04:37 2009 +0200 @@ -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> @@ -211,28 +212,22 @@ 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->unlocked_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 @@ -320,22 +315,6 @@ .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 number based on parent device * @vdev: video_device to assign index number to, vdev->parent should be assigned @@ -534,15 +513,9 @@ goto cleanup; } #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 17) - if (vdev->fops->unlocked_ioctl) - vdev->cdev->ops = &v4l2_unlocked_fops; - else - vdev->cdev->ops = &v4l2_fops; + vdev->cdev->ops = &v4l2_unlocked_fops; #else - if (vdev->fops->unlocked_ioctl) - vdev->cdev->ops = (struct file_operations *)&v4l2_unlocked_fops; - else - vdev->cdev->ops = (struct file_operations *)&v4l2_fops; + vdev->cdev->ops = (struct file_operations *)&v4l2_unlocked_fops; #endif vdev->cdev->owner = vdev->fops->owner; ret = cdev_add(vdev->cdev, MKDEV(VIDEO_MAJOR, vdev->minor), 1);