Skip to content

Commit

Permalink
[media] v4l: kill the BKL
Browse files Browse the repository at this point in the history
All of the hard problems for BKL removal appear to be solved in the
v4l-dvb/master tree. This removes the BKL from the various open
functions that do not need it, or only use it to protect an
open count.

The zoran driver is nontrivial in this regard, so I introduce
a new mutex that locks both the open/release and the ioctl
functions. Someone with access to the hardware can probably
improve that by using the existing lock in all cases.

Finally, all drivers that still use the locked version of the
ioctl function now get called under a new mutex instead of
the BKL.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
  • Loading branch information
Arnd Bergmann authored and Mauro Carvalho Chehab committed Nov 9, 2010
1 parent 2c2742d commit 0edf2e5
Show file tree
Hide file tree
Showing 12 changed files with 51 additions and 59 deletions.
1 change: 0 additions & 1 deletion drivers/media/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ comment "Multimedia core support"

config VIDEO_DEV
tristate "Video For Linux"
depends on BKL # used in many drivers for ioctl handling, need to kill
---help---
V4L core support for video capture and overlay devices, webcams and
AM/FM radio cards.
Expand Down
6 changes: 2 additions & 4 deletions drivers/media/video/cx231xx/cx231xx-417.c
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/firmware.h>
#include <linux/smp_lock.h>
#include <linux/vmalloc.h>
#include <media/v4l2-common.h>
#include <media/v4l2-ioctl.h>
Expand Down Expand Up @@ -1927,10 +1926,9 @@ static int mpeg_open(struct file *file)
dev = h;
}

if (dev == NULL) {
unlock_kernel();
if (dev == NULL)
return -ENODEV;
}

mutex_lock(&dev->lock);

/* allocate + initialize per filehandle data */
Expand Down
9 changes: 1 addition & 8 deletions drivers/media/video/cx23885/cx23885-417.c
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/firmware.h>
#include <linux/smp_lock.h>
#include <linux/slab.h>
#include <media/v4l2-common.h>
#include <media/v4l2-ioctl.h>
Expand Down Expand Up @@ -1576,12 +1575,8 @@ static int mpeg_open(struct file *file)

/* allocate + initialize per filehandle data */
fh = kzalloc(sizeof(*fh), GFP_KERNEL);
if (NULL == fh) {
unlock_kernel();
if (!fh)
return -ENOMEM;
}

lock_kernel();

file->private_data = fh;
fh->dev = dev;
Expand All @@ -1592,8 +1587,6 @@ static int mpeg_open(struct file *file)
V4L2_FIELD_INTERLACED,
sizeof(struct cx23885_buffer),
fh, NULL);
unlock_kernel();

return 0;
}

Expand Down
5 changes: 0 additions & 5 deletions drivers/media/video/cx23885/cx23885-video.c
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
#include <linux/kmod.h>
#include <linux/kernel.h>
#include <linux/slab.h>
#include <linux/smp_lock.h>
#include <linux/interrupt.h>
#include <linux/delay.h>
#include <linux/kthread.h>
Expand Down Expand Up @@ -743,8 +742,6 @@ static int video_open(struct file *file)
if (NULL == fh)
return -ENOMEM;

lock_kernel();

file->private_data = fh;
fh->dev = dev;
fh->radio = radio;
Expand All @@ -762,8 +759,6 @@ static int video_open(struct file *file)

dprintk(1, "post videobuf_queue_init()\n");

unlock_kernel();

return 0;
}

Expand Down
7 changes: 3 additions & 4 deletions drivers/media/video/se401.c
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ static const char version[] = "0.24";
#include <linux/init.h>
#include <linux/vmalloc.h>
#include <linux/slab.h>
#include <linux/smp_lock.h>
#include <linux/pagemap.h>
#include <linux/usb.h>
#include "se401.h"
Expand Down Expand Up @@ -951,9 +950,9 @@ static int se401_open(struct file *file)
struct usb_se401 *se401 = (struct usb_se401 *)dev;
int err = 0;

lock_kernel();
mutex_lock(&se401->lock);
if (se401->user) {
unlock_kernel();
mutex_unlock(&se401->lock);
return -EBUSY;
}
se401->fbuf = rvmalloc(se401->maxframesize * SE401_NUMFRAMES);
Expand All @@ -962,7 +961,7 @@ static int se401_open(struct file *file)
else
err = -ENOMEM;
se401->user = !err;
unlock_kernel();
mutex_unlock(&se401->lock);

return err;
}
Expand Down
4 changes: 0 additions & 4 deletions drivers/media/video/stk-webcam.c
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
#include <linux/kernel.h>
#include <linux/errno.h>
#include <linux/slab.h>
#include <linux/smp_lock.h>

#include <linux/usb.h>
#include <linux/mm.h>
Expand Down Expand Up @@ -673,14 +672,11 @@ static int v4l_stk_open(struct file *fp)
vdev = video_devdata(fp);
dev = vdev_to_camera(vdev);

lock_kernel();
if (dev == NULL || !is_present(dev)) {
unlock_kernel();
return -ENXIO;
}
fp->private_data = dev;
usb_autopm_get_interface(dev->interface);
unlock_kernel();

return 0;
}
Expand Down
13 changes: 4 additions & 9 deletions drivers/media/video/tlg2300/pd-main.c
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
#include <linux/string.h>
#include <linux/types.h>
#include <linux/firmware.h>
#include <linux/smp_lock.h>

#include "vendorcmds.h"
#include "pd-common.h"
Expand Down Expand Up @@ -485,15 +484,11 @@ static void poseidon_disconnect(struct usb_interface *interface)
/*unregister v4l2 device */
v4l2_device_unregister(&pd->v4l2_dev);

lock_kernel();
{
pd_dvb_usb_device_exit(pd);
poseidon_fm_exit(pd);
pd_dvb_usb_device_exit(pd);
poseidon_fm_exit(pd);

poseidon_audio_free(pd);
pd_video_exit(pd);
}
unlock_kernel();
poseidon_audio_free(pd);
pd_video_exit(pd);

usb_set_intfdata(interface, NULL);
kref_put(&pd->kref, poseidon_delete);
Expand Down
29 changes: 14 additions & 15 deletions drivers/media/video/usbvideo/vicam.c
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@
#include <linux/vmalloc.h>
#include <linux/mm.h>
#include <linux/slab.h>
#include <linux/smp_lock.h>
#include <linux/mutex.h>
#include <linux/firmware.h>
#include <linux/ihex.h>
Expand Down Expand Up @@ -483,40 +482,46 @@ vicam_open(struct file *file)
return -EINVAL;
}

/* the videodev_lock held above us protects us from
* simultaneous opens...for now. we probably shouldn't
* rely on this fact forever.
/* cam_lock/open_count protects us from simultaneous opens
* ... for now. we probably shouldn't rely on this fact forever.
*/

lock_kernel();
mutex_lock(&cam->cam_lock);
if (cam->open_count > 0) {
printk(KERN_INFO
"vicam_open called on already opened camera");
unlock_kernel();
mutex_unlock(&cam->cam_lock);
return -EBUSY;
}

cam->raw_image = kmalloc(VICAM_MAX_READ_SIZE, GFP_KERNEL);
if (!cam->raw_image) {
unlock_kernel();
mutex_unlock(&cam->cam_lock);
return -ENOMEM;
}

cam->framebuf = rvmalloc(VICAM_MAX_FRAME_SIZE * VICAM_FRAMES);
if (!cam->framebuf) {
kfree(cam->raw_image);
unlock_kernel();
mutex_unlock(&cam->cam_lock);
return -ENOMEM;
}

cam->cntrlbuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
if (!cam->cntrlbuf) {
kfree(cam->raw_image);
rvfree(cam->framebuf, VICAM_MAX_FRAME_SIZE * VICAM_FRAMES);
unlock_kernel();
mutex_unlock(&cam->cam_lock);
return -ENOMEM;
}

cam->needsDummyRead = 1;
cam->open_count++;

file->private_data = cam;
mutex_unlock(&cam->cam_lock);


// First upload firmware, then turn the camera on

if (!cam->is_initialized) {
Expand All @@ -527,12 +532,6 @@ vicam_open(struct file *file)

set_camera_power(cam, 1);

cam->needsDummyRead = 1;
cam->open_count++;

file->private_data = cam;
unlock_kernel();

return 0;
}

Expand Down
7 changes: 4 additions & 3 deletions drivers/media/video/v4l2-dev.c
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
#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>

Expand Down Expand Up @@ -247,10 +246,12 @@ static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
mutex_unlock(vdev->lock);
} else if (vdev->fops->ioctl) {
/* TODO: convert all drivers to unlocked_ioctl */
lock_kernel();
static DEFINE_MUTEX(v4l2_ioctl_mutex);

mutex_lock(&v4l2_ioctl_mutex);
if (video_is_registered(vdev))
ret = vdev->fops->ioctl(filp, cmd, arg);
unlock_kernel();
mutex_unlock(&v4l2_ioctl_mutex);
} else
ret = -ENOTTY;

Expand Down
1 change: 1 addition & 0 deletions drivers/media/video/zoran/zoran.h
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,7 @@ struct zoran {
struct videocodec *vfe; /* video front end */

struct mutex resource_lock; /* prevent evil stuff */
struct mutex other_lock; /* please merge with above */

u8 initialized; /* flag if zoran has been correctly initialized */
int user; /* number of current users */
Expand Down
1 change: 1 addition & 0 deletions drivers/media/video/zoran/zoran_card.c
Original file line number Diff line number Diff line change
Expand Up @@ -1227,6 +1227,7 @@ static int __devinit zoran_probe(struct pci_dev *pdev,
snprintf(ZR_DEVNAME(zr), sizeof(ZR_DEVNAME(zr)), "MJPEG[%u]", zr->id);
spin_lock_init(&zr->spinlock);
mutex_init(&zr->resource_lock);
mutex_init(&zr->other_lock);
if (pci_enable_device(pdev))
goto zr_unreg;
pci_read_config_byte(zr->pci_dev, PCI_CLASS_REVISION, &zr->revision);
Expand Down
27 changes: 21 additions & 6 deletions drivers/media/video/zoran/zoran_driver.c
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
#include <linux/module.h>
#include <linux/delay.h>
#include <linux/slab.h>
#include <linux/smp_lock.h>
#include <linux/pci.h>
#include <linux/vmalloc.h>
#include <linux/wait.h>
Expand Down Expand Up @@ -913,7 +912,7 @@ static int zoran_open(struct file *file)
dprintk(2, KERN_INFO "%s: %s(%s, pid=[%d]), users(-)=%d\n",
ZR_DEVNAME(zr), __func__, current->comm, task_pid_nr(current), zr->user + 1);

lock_kernel();
mutex_lock(&zr->other_lock);

if (zr->user >= 2048) {
dprintk(1, KERN_ERR "%s: too many users (%d) on device\n",
Expand Down Expand Up @@ -963,14 +962,14 @@ static int zoran_open(struct file *file)
file->private_data = fh;
fh->zr = zr;
zoran_open_init_session(fh);
unlock_kernel();
mutex_unlock(&zr->other_lock);

return 0;

fail_fh:
kfree(fh);
fail_unlock:
unlock_kernel();
mutex_unlock(&zr->other_lock);

dprintk(2, KERN_INFO "%s: open failed (%d), users(-)=%d\n",
ZR_DEVNAME(zr), res, zr->user);
Expand All @@ -989,7 +988,7 @@ zoran_close(struct file *file)

/* kernel locks (fs/device.c), so don't do that ourselves
* (prevents deadlocks) */
/*mutex_lock(&zr->resource_lock);*/
mutex_lock(&zr->other_lock);

zoran_close_end_session(fh);

Expand Down Expand Up @@ -1023,6 +1022,7 @@ zoran_close(struct file *file)
encoder_call(zr, video, s_routing, 2, 0, 0);
}
}
mutex_unlock(&zr->other_lock);

file->private_data = NULL;
kfree(fh->overlay_mask);
Expand Down Expand Up @@ -3370,11 +3370,26 @@ static const struct v4l2_ioctl_ops zoran_ioctl_ops = {
#endif
};

/* please use zr->resource_lock consistently and kill this wrapper */
static long zoran_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
struct zoran_fh *fh = file->private_data;
struct zoran *zr = fh->zr;
int ret;

mutex_lock(&zr->other_lock);
ret = video_ioctl2(file, cmd, arg);
mutex_unlock(&zr->other_lock);

return ret;
}

static const struct v4l2_file_operations zoran_fops = {
.owner = THIS_MODULE,
.open = zoran_open,
.release = zoran_close,
.ioctl = video_ioctl2,
.unlocked_ioctl = zoran_ioctl,
.read = zoran_read,
.write = zoran_write,
.mmap = zoran_mmap,
Expand Down

0 comments on commit 0edf2e5

Please sign in to comment.