Commit 75910053 authored by Mike Isely's avatar Mike Isely Committed by Mauro Carvalho Chehab

V4L/DVB (4663): Pvrusb2: Get rid of private global context array brain damage

A previous attempt to deal with the upcoming loss of
video_set_drvdata() and video_get_drvdata() resulted in logic which
causes a circular locking dependency - also known as a deadlock.  This
changeset attacks the problem in a different manner, using a technique
that no longer requires the problematic mutex (or that private global
array either).
Signed-off-by: default avatarMike Isely <isely@pobox.com>
Signed-off-by: default avatarMauro Carvalho Chehab <mchehab@infradead.org>
parent 32ffa9ae
...@@ -28,6 +28,7 @@ ...@@ -28,6 +28,7 @@
#include "pvrusb2-debug.h" #include "pvrusb2-debug.h"
#include "pvrusb2-v4l2.h" #include "pvrusb2-v4l2.h"
#include "pvrusb2-ioread.h" #include "pvrusb2-ioread.h"
#include <linux/videodev.h>
#include <linux/videodev2.h> #include <linux/videodev2.h>
#include <media/v4l2-common.h> #include <media/v4l2-common.h>
...@@ -35,21 +36,10 @@ struct pvr2_v4l2_dev; ...@@ -35,21 +36,10 @@ struct pvr2_v4l2_dev;
struct pvr2_v4l2_fh; struct pvr2_v4l2_fh;
struct pvr2_v4l2; struct pvr2_v4l2;
/* V4L no longer provide the ability to set / get a private context pointer
(i.e. video_get_drvdata / video_set_drvdata), which means we have to
concoct our own context locating mechanism. Supposedly this is intended
to simplify driver implementation. It's not clear to me how that can
possibly be true. Our solution here is to maintain a lookup table of
our context instances, indexed by the minor device number of the V4L
device. See pvr2_v4l2_open() for some implications of this approach. */
static struct pvr2_v4l2_dev *devices[256];
static DEFINE_MUTEX(device_lock);
struct pvr2_v4l2_dev { struct pvr2_v4l2_dev {
struct video_device devbase; /* MUST be first! */
struct pvr2_v4l2 *v4lp; struct pvr2_v4l2 *v4lp;
struct video_device *vdev;
struct pvr2_context_stream *stream; struct pvr2_context_stream *stream;
int ctxt_idx;
enum pvr2_config config; enum pvr2_config config;
}; };
...@@ -74,7 +64,7 @@ struct pvr2_v4l2 { ...@@ -74,7 +64,7 @@ struct pvr2_v4l2 {
struct v4l2_prio_state prio; struct v4l2_prio_state prio;
/* streams */ /* streams */
struct pvr2_v4l2_dev video_dev; struct pvr2_v4l2_dev *vdev;
}; };
static int video_nr[PVR_NUM] = {[0 ... PVR_NUM-1] = -1}; static int video_nr[PVR_NUM] = {[0 ... PVR_NUM-1] = -1};
...@@ -718,21 +708,22 @@ static int pvr2_v4l2_do_ioctl(struct inode *inode, struct file *file, ...@@ -718,21 +708,22 @@ static int pvr2_v4l2_do_ioctl(struct inode *inode, struct file *file,
static void pvr2_v4l2_dev_destroy(struct pvr2_v4l2_dev *dip) static void pvr2_v4l2_dev_destroy(struct pvr2_v4l2_dev *dip)
{ {
printk(KERN_INFO "pvrusb2: unregistering device video%d [%s]\n", printk(KERN_INFO "pvrusb2: unregistering device video%d [%s]\n",
dip->vdev->minor,pvr2_config_get_name(dip->config)); dip->devbase.minor,pvr2_config_get_name(dip->config));
if (dip->ctxt_idx >= 0) {
mutex_lock(&device_lock); /* Paranoia */
devices[dip->ctxt_idx] = NULL; dip->v4lp = 0;
dip->ctxt_idx = -1; dip->stream = 0;
mutex_unlock(&device_lock);
} /* Actual deallocation happens later when all internal references
video_unregister_device(dip->vdev); are gone. */
video_unregister_device(&dip->devbase);
} }
static void pvr2_v4l2_destroy_no_lock(struct pvr2_v4l2 *vp) static void pvr2_v4l2_destroy_no_lock(struct pvr2_v4l2 *vp)
{ {
pvr2_hdw_v4l_store_minor_number(vp->channel.mc_head->hdw,-1); pvr2_hdw_v4l_store_minor_number(vp->channel.mc_head->hdw,-1);
pvr2_v4l2_dev_destroy(&vp->video_dev); pvr2_v4l2_dev_destroy(vp->vdev);
pvr2_trace(PVR2_TRACE_STRUCT,"Destroying pvr2_v4l2 id=%p",vp); pvr2_trace(PVR2_TRACE_STRUCT,"Destroying pvr2_v4l2 id=%p",vp);
pvr2_channel_done(&vp->channel); pvr2_channel_done(&vp->channel);
...@@ -740,6 +731,14 @@ static void pvr2_v4l2_destroy_no_lock(struct pvr2_v4l2 *vp) ...@@ -740,6 +731,14 @@ static void pvr2_v4l2_destroy_no_lock(struct pvr2_v4l2 *vp)
} }
static void pvr2_video_device_release(struct video_device *vdev)
{
struct pvr2_v4l2_dev *dev;
dev = container_of(vdev,struct pvr2_v4l2_dev,devbase);
kfree(dev);
}
static void pvr2_v4l2_internal_check(struct pvr2_channel *chp) static void pvr2_v4l2_internal_check(struct pvr2_channel *chp)
{ {
struct pvr2_v4l2 *vp; struct pvr2_v4l2 *vp;
...@@ -811,40 +810,12 @@ static int pvr2_v4l2_release(struct inode *inode, struct file *file) ...@@ -811,40 +810,12 @@ static int pvr2_v4l2_release(struct inode *inode, struct file *file)
static int pvr2_v4l2_open(struct inode *inode, struct file *file) static int pvr2_v4l2_open(struct inode *inode, struct file *file)
{ {
struct pvr2_v4l2_dev *dip = NULL; /* Our own context pointer */ struct pvr2_v4l2_dev *dip; /* Our own context pointer */
struct pvr2_v4l2_fh *fhp; struct pvr2_v4l2_fh *fhp;
struct pvr2_v4l2 *vp; struct pvr2_v4l2 *vp;
struct pvr2_hdw *hdw; struct pvr2_hdw *hdw;
mutex_lock(&device_lock); dip = container_of(video_devdata(file),struct pvr2_v4l2_dev,devbase);
/* MCI 7-Jun-2006 Even though we're just doing what amounts to an
atomic read of the device mapping array here, we still need the
mutex. The problem is that there is a tiny race possible when
we register the device. We can't update the device mapping
array until after the device has been registered, owing to the
fact that we can't know the minor device number until after the
registration succeeds. And if another thread tries to open the
device in the window of time after registration but before the
map is updated, then it will get back an erroneous null pointer
and the open will result in a spurious failure. The only way to
prevent that is to (a) be inside the mutex here before we access
the array, and (b) cover the entire registration process later
on with this same mutex. Thus if we get inside the mutex here,
then we can be assured that the registration process actually
completed correctly. This is an unhappy complication from the
use of global data in a driver that lives in a preemptible
environment. It sure would be nice if the video device itself
had a means for storing and retrieving a local context pointer.
Oh wait. It did. But now it's gone. Silly me. */
{
unsigned int midx = iminor(file->f_dentry->d_inode);
if (midx < sizeof(devices)/sizeof(devices[0])) {
dip = devices[midx];
}
}
mutex_unlock(&device_lock);
if (!dip) return -ENODEV; /* Should be impossible but I'm paranoid */
vp = dip->v4lp; vp = dip->v4lp;
hdw = vp->channel.hdw; hdw = vp->channel.hdw;
...@@ -1074,38 +1045,24 @@ static void pvr2_v4l2_dev_init(struct pvr2_v4l2_dev *dip, ...@@ -1074,38 +1045,24 @@ static void pvr2_v4l2_dev_init(struct pvr2_v4l2_dev *dip,
return; return;
} }
dip->vdev = video_device_alloc(); memcpy(&dip->devbase,&vdev_template,sizeof(vdev_template));
if (!dip->vdev) { dip->devbase.release = pvr2_video_device_release;
err("Alloc of pvrusb2 v4l video device failed");
return;
}
memcpy(dip->vdev,&vdev_template,sizeof(vdev_template));
dip->vdev->release = video_device_release;
mutex_lock(&device_lock);
mindevnum = -1; mindevnum = -1;
unit_number = pvr2_hdw_get_unit_number(vp->channel.mc_head->hdw); unit_number = pvr2_hdw_get_unit_number(vp->channel.mc_head->hdw);
if ((unit_number >= 0) && (unit_number < PVR_NUM)) { if ((unit_number >= 0) && (unit_number < PVR_NUM)) {
mindevnum = video_nr[unit_number]; mindevnum = video_nr[unit_number];
} }
if ((video_register_device(dip->vdev, v4l_type, mindevnum) < 0) && if ((video_register_device(&dip->devbase, v4l_type, mindevnum) < 0) &&
(video_register_device(dip->vdev, v4l_type, -1) < 0)) { (video_register_device(&dip->devbase, v4l_type, -1) < 0)) {
err("Failed to register pvrusb2 v4l video device"); err("Failed to register pvrusb2 v4l video device");
} else { } else {
printk(KERN_INFO "pvrusb2: registered device video%d [%s]\n", printk(KERN_INFO "pvrusb2: registered device video%d [%s]\n",
dip->vdev->minor,pvr2_config_get_name(dip->config)); dip->devbase.minor,pvr2_config_get_name(dip->config));
}
if ((dip->vdev->minor < sizeof(devices)/sizeof(devices[0])) &&
(devices[dip->vdev->minor] == NULL)) {
dip->ctxt_idx = dip->vdev->minor;
devices[dip->ctxt_idx] = dip;
} }
mutex_unlock(&device_lock);
pvr2_hdw_v4l_store_minor_number(vp->channel.mc_head->hdw, pvr2_hdw_v4l_store_minor_number(vp->channel.mc_head->hdw,
dip->vdev->minor); dip->devbase.minor);
} }
...@@ -1116,15 +1073,19 @@ struct pvr2_v4l2 *pvr2_v4l2_create(struct pvr2_context *mnp) ...@@ -1116,15 +1073,19 @@ struct pvr2_v4l2 *pvr2_v4l2_create(struct pvr2_context *mnp)
vp = kmalloc(sizeof(*vp),GFP_KERNEL); vp = kmalloc(sizeof(*vp),GFP_KERNEL);
if (!vp) return vp; if (!vp) return vp;
memset(vp,0,sizeof(*vp)); memset(vp,0,sizeof(*vp));
vp->video_dev.ctxt_idx = -1; vp->vdev = kmalloc(sizeof(*vp->vdev),GFP_KERNEL);
if (!vp->vdev) {
kfree(vp);
return 0;
}
memset(vp->vdev,0,sizeof(*vp->vdev));
pvr2_channel_init(&vp->channel,mnp); pvr2_channel_init(&vp->channel,mnp);
pvr2_trace(PVR2_TRACE_STRUCT,"Creating pvr2_v4l2 id=%p",vp); pvr2_trace(PVR2_TRACE_STRUCT,"Creating pvr2_v4l2 id=%p",vp);
vp->channel.check_func = pvr2_v4l2_internal_check; vp->channel.check_func = pvr2_v4l2_internal_check;
/* register streams */ /* register streams */
pvr2_v4l2_dev_init(&vp->video_dev,vp,pvr2_config_mpeg); pvr2_v4l2_dev_init(vp->vdev,vp,pvr2_config_mpeg);
return vp; return vp;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment