-stable review patch. If anyone has any objections, please let us know. ------------------ From: Mike Isely There is a mutex ordering problem between the pvrusb2 driver and the v4l core. Two different pathways take mutexes in opposing orders and this (under rare circumstances) can cause a deadlock. The two mutexes in question are videodev_lock in the v4l core and device_lock inside the pvrusb2 driver. The device_lock instance in the driver protects a private global array of context pointers which had been implemented in advance of v4l core changes which eliminate the video_set_drvdata() and video_get_drvdata() functions. This patch restores the use of video_get_drvdata() and video_set_drvdata(), eliminating the need for the array and the mutex. (This is actually a patch to restore the previous implementation.) We can do this for 2.6.18 since those functions are in fact still present. A better (and larger) solution will be done for later kernels. Signed-off-by: Mike Isely Signed-off-by: Michael Krufky Signed-off-by: Greg Kroah-Hartman --- a/drivers/media/video/pvrusb2/pvrusb2-v4l2.c +++ b/drivers/media/video/pvrusb2/pvrusb2-v4l2.c @@ -22,6 +22,7 @@ #include #include +#include #include "pvrusb2-context.h" #include "pvrusb2-hdw.h" #include "pvrusb2.h" @@ -35,21 +36,11 @@ struct pvr2_v4l2_dev; struct pvr2_v4l2_fh; 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 *v4lp; struct video_device *vdev; struct pvr2_context_stream *stream; - int ctxt_idx; enum pvr2_config config; }; @@ -703,12 +694,6 @@ static void pvr2_v4l2_dev_destroy(struct { printk(KERN_INFO "pvrusb2: unregistering device video%d [%s]\n", dip->vdev->minor,pvr2_config_get_name(dip->config)); - if (dip->ctxt_idx >= 0) { - mutex_lock(&device_lock); - devices[dip->ctxt_idx] = NULL; - dip->ctxt_idx = -1; - mutex_unlock(&device_lock); - } video_unregister_device(dip->vdev); } @@ -800,33 +785,10 @@ static int pvr2_v4l2_open(struct inode * struct pvr2_v4l2 *vp; struct pvr2_hdw *hdw; - mutex_lock(&device_lock); - /* 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]; - } + struct video_device *vdev = video_devdata(file); + dip = (struct pvr2_v4l2_dev *)video_get_drvdata(vdev); } - mutex_unlock(&device_lock); if (!dip) return -ENODEV; /* Should be impossible but I'm paranoid */ @@ -1066,7 +1028,7 @@ static void pvr2_v4l2_dev_init(struct pv memcpy(dip->vdev,&vdev_template,sizeof(vdev_template)); dip->vdev->release = video_device_release; - mutex_lock(&device_lock); + video_set_drvdata(dip->vdev,dip); mindevnum = -1; unit_number = pvr2_hdw_get_unit_number(vp->channel.mc_head->hdw); @@ -1081,12 +1043,6 @@ static void pvr2_v4l2_dev_init(struct pv dip->vdev->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, dip->vdev->minor); @@ -1100,7 +1056,6 @@ struct pvr2_v4l2 *pvr2_v4l2_create(struc vp = kmalloc(sizeof(*vp),GFP_KERNEL); if (!vp) return vp; memset(vp,0,sizeof(*vp)); - vp->video_dev.ctxt_idx = -1; pvr2_channel_init(&vp->channel,mnp); pvr2_trace(PVR2_TRACE_STRUCT,"Creating pvr2_v4l2 id=%p",vp); _______________________________________________ stable mailing list stable@linux.kernel.org http://linux.kernel.org/mailman/listinfo/stable -- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/