[media] pwc: Rework locking

While testing gtk-v4l's new ctrl event code, I hit the following deadlock
in the pwc driver:

Thread 1:
-Does a VIDIOC_G_CTRL
-video2_ioctl takes the modlock
-video2_ioctl calls v4l2_g_ctrl
-v4l2_g_ctrl takes the ctrl_handler lock
-v4l2_g_ctrl calls pwc_g_volatile_ctrl
-pwc_g_volatile_ctrl releases the modlock as the usb transfer can take a
 significant amount of time and we don't want to block DQBUF / QBUF too long
Thread 2:
-Does a VIDIOC_FOO_CTRL
-video2_ioctl takes the modlock
-video2_ioctl calls v4l2_foo_ctrl
-v4l2_foo_ctrl blocks while trying to take the ctrl_handler lock
Thread 1:
-Blocks while trying to re-take the modlock, as its caller will eventually
 unlock that

Now we have thread 1 waiting for the modlock while holding the ctrl_handler
lock and thread 2 waiting for the ctrl_handler lock while holding the
modlock -> deadlock.

Conclusion:
1) We cannot unlock modlock from pwc_s_ctrl / pwc_g_volatile_ctrl,
   but this can cause QBUF / DQBUF to block for up to a full second
2) After evaluating various option I came to the conclusion that pwc should
   stop using the v4l2 core locking, and instead do its own locking

Thus this patch stops pwc using the v4l2 core locking, and replaces that with
it doing its own locking where necessary.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/media/video/pwc/pwc-ctrl.c b/drivers/media/video/pwc/pwc-ctrl.c
index def9120..5eddfab 100644 (file)
--- a/drivers/media/video/pwc/pwc-ctrl.c
+++ b/drivers/media/video/pwc/pwc-ctrl.c
@@ -649,10 +649,20 @@ static int pwc_get_leds(struct pwc_device *pdev, int *on_value, int *off_value)
 static int _pwc_mpt_reset(struct pwc_device *pdev, int flags)
 {
        unsigned char buf;
+       int r;
+
+       mutex_lock(&pdev->udevlock);
+       if (!pdev->udev) {
+               r = -ENODEV;
+               goto leave;
+       }
 
        buf = flags & 0x03; // only lower two bits are currently used
-       return send_control_msg(pdev,
+       r = send_control_msg(pdev,
                SET_MPT_CTL, PT_RESET_CONTROL_FORMATTER, &buf, sizeof(buf));
+leave:
+       mutex_unlock(&pdev->udevlock);
+       return r;
 }
 
 int pwc_mpt_reset(struct pwc_device *pdev, int flags)
@@ -669,6 +679,13 @@ int pwc_mpt_reset(struct pwc_device *pdev, int flags)
 static int _pwc_mpt_set_angle(struct pwc_device *pdev, int pan, int tilt)
 {
        unsigned char buf[4];
+       int r;
+
+       mutex_lock(&pdev->udevlock);
+       if (!pdev->udev) {
+               r = -ENODEV;
+               goto leave;
+       }
 
        /* set new relative angle; angles are expressed in degrees * 100,
           but cam as .5 degree resolution, hence divide by 200. Also
@@ -681,8 +698,11 @@ static int _pwc_mpt_set_angle(struct pwc_device *pdev, int pan, int tilt)
        buf[1] = (pan >> 8) & 0xFF;
        buf[2] = tilt & 0xFF;
        buf[3] = (tilt >> 8) & 0xFF;
-       return send_control_msg(pdev,
+       r = send_control_msg(pdev,
                SET_MPT_CTL, PT_RELATIVE_CONTROL_FORMATTER, &buf, sizeof(buf));
+leave:
+       mutex_unlock(&pdev->udevlock);
+       return r;
 }
 
 int pwc_mpt_set_angle(struct pwc_device *pdev, int pan, int tilt)
@@ -718,14 +738,22 @@ static int pwc_mpt_get_status(struct pwc_device *pdev, struct pwc_mpt_status *st
        int ret;
        unsigned char buf[5];
 
+       mutex_lock(&pdev->udevlock);
+       if (!pdev->udev) {
+               ret = -ENODEV;
+               goto leave;
+       }
+
        ret = recv_control_msg(pdev,
                GET_MPT_CTL, PT_STATUS_FORMATTER, &buf, sizeof(buf));
        if (ret < 0)
-               return ret;
+               goto leave;
        status->status = buf[0] & 0x7; // 3 bits are used for reporting
        status->time_pan = (buf[1] << 8) + buf[2];
        status->time_tilt = (buf[3] << 8) + buf[4];
-       return 0;
+leave:
+       mutex_unlock(&pdev->udevlock);
+       return ret;
 }
 
 #ifdef CONFIG_USB_PWC_DEBUG
@@ -794,24 +822,30 @@ long pwc_ioctl(struct pwc_device *pdev, unsigned int cmd, void *arg)
 
        switch(cmd) {
        case VIDIOCPWCRUSER:
-               ret = pwc_button_ctrl(pdev, RESTORE_USER_DEFAULTS_FORMATTER);
+               ret = v4l2_ctrl_s_ctrl(pdev->restore_user, 0);
                break;
 
        case VIDIOCPWCSUSER:
-               ret = pwc_button_ctrl(pdev, SAVE_USER_DEFAULTS_FORMATTER);
+               ret = v4l2_ctrl_s_ctrl(pdev->save_user, 0);
                break;
 
        case VIDIOCPWCFACTORY:
-               ret = pwc_button_ctrl(pdev, RESTORE_FACTORY_DEFAULTS_FORMATTER);
+               ret = v4l2_ctrl_s_ctrl(pdev->restore_factory, 0);
                break;
 
        case VIDIOCPWCSCQUAL:
        {
                ARG_DEF(int, qual)
 
-               if (vb2_is_streaming(&pdev->vb_queue)) {
+               mutex_lock(&pdev->udevlock);
+               if (!pdev->udev) {
+                       ret = -ENODEV;
+                       goto leave;
+               }
+
+               if (pdev->iso_init) {
                        ret = -EBUSY;
-                       break;
+                       goto leave;
                }
 
                ARG_IN(qual)
@@ -819,6 +853,8 @@ long pwc_ioctl(struct pwc_device *pdev, unsigned int cmd, void *arg)
                        ret = -EINVAL;
                else
                        ret = pwc_set_video_mode(pdev, pdev->view.x, pdev->view.y, pdev->vframes, ARGR(qual), pdev->vsnapshot);
+leave:
+               mutex_unlock(&pdev->udevlock);
                break;
        }
 
@@ -939,8 +975,16 @@ long pwc_ioctl(struct pwc_device *pdev, unsigned int cmd, void *arg)
        {
                ARG_DEF(struct pwc_leds, leds)
 
+               mutex_lock(&pdev->udevlock);
+               if (!pdev->udev) {
+                       ret = -ENODEV;
+                       break;
+               }
+
                ARG_IN(leds)
                ret = pwc_set_leds(pdev, ARGR(leds).led_on, ARGR(leds).led_off);
+
+               mutex_unlock(&pdev->udevlock);
                break;
        }
 
@@ -949,8 +993,16 @@ long pwc_ioctl(struct pwc_device *pdev, unsigned int cmd, void *arg)
        {
                ARG_DEF(struct pwc_leds, leds)
 
+               mutex_lock(&pdev->udevlock);
+               if (!pdev->udev) {
+                       ret = -ENODEV;
+                       break;
+               }
+
                ret = pwc_get_leds(pdev, &ARGR(leds).led_on, &ARGR(leds).led_off);
                ARG_OUT(leds)
+
+               mutex_unlock(&pdev->udevlock);
                break;
        }
 

diff --git a/drivers/media/video/pwc/pwc-dec23.c b/drivers/media/video/pwc/pwc-dec23.c
index 06a4e87..740bf6b 100644 (file)
--- a/drivers/media/video/pwc/pwc-dec23.c
+++ b/drivers/media/video/pwc/pwc-dec23.c
@@ -315,6 +315,8 @@ int pwc_dec23_init(struct pwc_device *pwc, int type, unsigned char *cmd)
        }
        pdec = pwc->decompress_data;
 
+       mutex_init(&pdec->lock);
+
        if (DEVICE_USE_CODEC3(type)) {
                flags = cmd[2] & 0x18;
                if (flags == 8)
@@ -858,6 +860,9 @@ void pwc_dec23_decompress(const struct pwc_device *pwc,
                          int flags)
 {
        int bandlines_left, stride, bytes_per_block;
+       struct pwc_dec23_private *pdec = pwc->decompress_data;
+
+       mutex_lock(&pdec->lock);
 
        bandlines_left = pwc->image.y / 4;
        bytes_per_block = pwc->view.x * 4;
@@ -917,4 +922,6 @@ void pwc_dec23_decompress(const struct pwc_device *pwc,
 
                }
        }
+
+       mutex_unlock(&pdec->lock);
 }

diff --git a/drivers/media/video/pwc/pwc-dec23.h b/drivers/media/video/pwc/pwc-dec23.h
index a0ac4f3..9cba74d 100644 (file)
--- a/drivers/media/video/pwc/pwc-dec23.h
+++ b/drivers/media/video/pwc/pwc-dec23.h
@@ -29,6 +29,8 @@
 
 struct pwc_dec23_private
 {
+       struct mutex lock;
+
   unsigned int scalebits;
   unsigned int nbitsmask, nbits; /* Number of bits of a color in the compressed stream */
 

diff --git a/drivers/media/video/pwc/pwc-if.c b/drivers/media/video/pwc/pwc-if.c
index c1806e3..550ad07 100644 (file)
--- a/drivers/media/video/pwc/pwc-if.c
+++ b/drivers/media/video/pwc/pwc-if.c
@@ -515,12 +515,11 @@ static void pwc_isoc_cleanup(struct pwc_device *pdev)
        PWC_DEBUG_OPEN("<< pwc_isoc_cleanup()\n");
 }
 
-/*
- * Release all queued buffers, no need to take queued_bufs_lock, since all
- * iso urbs have been killed when we're called so pwc_isoc_handler won't run.
- */
 static void pwc_cleanup_queued_bufs(struct pwc_device *pdev)
 {
+       unsigned long flags = 0;
+
+       spin_lock_irqsave(&pdev->queued_bufs_lock, flags);
        while (!list_empty(&pdev->queued_bufs)) {
                struct pwc_frame_buf *buf;
 
@@ -529,6 +528,7 @@ static void pwc_cleanup_queued_bufs(struct pwc_device *pdev)
                list_del(&buf->list);
                vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
        }
+       spin_unlock_irqrestore(&pdev->queued_bufs_lock, flags);
 }
 
 /*********
@@ -642,6 +642,22 @@ static const char *pwc_sensor_type_to_string(unsigned int sensor_type)
 /***************************************************************************/
 /* Video4Linux functions */
 
+int pwc_test_n_set_capt_file(struct pwc_device *pdev, struct file *file)
+{
+       int r = 0;
+
+       mutex_lock(&pdev->capt_file_lock);
+       if (pdev->capt_file != NULL &&
+           pdev->capt_file != file) {
+               r = -EBUSY;
+               goto leave;
+       }
+       pdev->capt_file = file;
+leave:
+       mutex_unlock(&pdev->capt_file_lock);
+       return r;
+}
+
 static void pwc_video_release(struct v4l2_device *v)
 {
        struct pwc_device *pdev = container_of(v, struct pwc_device, v4l2_dev);
@@ -684,12 +700,9 @@ static ssize_t pwc_video_read(struct file *file, char __user *buf,
        if (!pdev->udev)
                return -ENODEV;
 
-       if (pdev->capt_file != NULL &&
-           pdev->capt_file != file)
+       if (pwc_test_n_set_capt_file(pdev, file))
                return -EBUSY;
 
-       pdev->capt_file = file;
-
        return vb2_read(&pdev->vb_queue, buf, count, ppos,
                        file->f_flags & O_NONBLOCK);
 }
@@ -785,16 +798,24 @@ static void buffer_queue(struct vb2_buffer *vb)
        unsigned long flags = 0;
 
        spin_lock_irqsave(&pdev->queued_bufs_lock, flags);
-       list_add_tail(&buf->list, &pdev->queued_bufs);
+       /* Check the device has not disconnected between prep and queuing */
+       if (pdev->udev)
+               list_add_tail(&buf->list, &pdev->queued_bufs);
+       else
+               vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
        spin_unlock_irqrestore(&pdev->queued_bufs_lock, flags);
 }
 
 static int start_streaming(struct vb2_queue *vq, unsigned int count)
 {
        struct pwc_device *pdev = vb2_get_drv_priv(vq);
+       int r;
 
-       if (!pdev->udev)
-               return -ENODEV;
+       mutex_lock(&pdev->udevlock);
+       if (!pdev->udev) {
+               r = -ENODEV;
+               goto leave;
+       }
 
        /* Turn on camera and set LEDS on */
        pwc_camera_power(pdev, 1);
@@ -806,35 +827,29 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
        }
        pwc_set_leds(pdev, led_on, led_off);
 
-       return pwc_isoc_init(pdev);
+       r = pwc_isoc_init(pdev);
+leave:
+       mutex_unlock(&pdev->udevlock);
+       return r;
 }
 
 static int stop_streaming(struct vb2_queue *vq)
 {
        struct pwc_device *pdev = vb2_get_drv_priv(vq);
 
+       mutex_lock(&pdev->udevlock);
        if (pdev->udev) {
                pwc_set_leds(pdev, 0, 0);
                pwc_camera_power(pdev, 0);
                pwc_isoc_cleanup(pdev);
        }
+       mutex_unlock(&pdev->udevlock);
+
        pwc_cleanup_queued_bufs(pdev);
 
        return 0;
 }
 
-static void pwc_lock(struct vb2_queue *vq)
-{
-       struct pwc_device *pdev = vb2_get_drv_priv(vq);
-       mutex_lock(&pdev->modlock);
-}
-
-static void pwc_unlock(struct vb2_queue *vq)
-{
-       struct pwc_device *pdev = vb2_get_drv_priv(vq);
-       mutex_unlock(&pdev->modlock);
-}
-
 static struct vb2_ops pwc_vb_queue_ops = {
        .queue_setup            = queue_setup,
        .buf_init               = buffer_init,
@@ -844,8 +859,6 @@ static struct vb2_ops pwc_vb_queue_ops = {
        .buf_queue              = buffer_queue,
        .start_streaming        = start_streaming,
        .stop_streaming         = stop_streaming,
-       .wait_prepare           = pwc_unlock,
-       .wait_finish            = pwc_lock,
 };
 
 /***************************************************************************/
@@ -1137,7 +1150,7 @@ static int usb_pwc_probe(struct usb_interface *intf, const struct usb_device_id
        }
        pwc_construct(pdev); /* set min/max sizes correct */
 
-       mutex_init(&pdev->modlock);
+       mutex_init(&pdev->capt_file_lock);
        mutex_init(&pdev->udevlock);
        spin_lock_init(&pdev->queued_bufs_lock);
        INIT_LIST_HEAD(&pdev->queued_bufs);
@@ -1158,7 +1171,6 @@ static int usb_pwc_probe(struct usb_interface *intf, const struct usb_device_id
 
        /* Init video_device structure */
        memcpy(&pdev->vdev, &pwc_template, sizeof(pwc_template));
-       pdev->vdev.lock = &pdev->modlock;
        strcpy(pdev->vdev.name, name);
        set_bit(V4L2_FL_USE_FH_PRIO, &pdev->vdev.flags);
        video_set_drvdata(&pdev->vdev, pdev);
@@ -1285,16 +1297,13 @@ static void usb_pwc_disconnect(struct usb_interface *intf)
        struct pwc_device *pdev = container_of(v, struct pwc_device, v4l2_dev);
 
        mutex_lock(&pdev->udevlock);
-       mutex_lock(&pdev->modlock);
-
        /* No need to keep the urbs around after disconnection */
        pwc_isoc_cleanup(pdev);
-       pwc_cleanup_queued_bufs(pdev);
        pdev->udev = NULL;
-
-       mutex_unlock(&pdev->modlock);
        mutex_unlock(&pdev->udevlock);
 
+       pwc_cleanup_queued_bufs(pdev);
+
        pwc_remove_sysfs_files(pdev);
        video_unregister_device(&pdev->vdev);
        v4l2_device_unregister(&pdev->v4l2_dev);

diff --git a/drivers/media/video/pwc/pwc-v4l.c b/drivers/media/video/pwc/pwc-v4l.c
index 1303641..97e8d62 100644 (file)
--- a/drivers/media/video/pwc/pwc-v4l.c
+++ b/drivers/media/video/pwc/pwc-v4l.c
@@ -475,17 +475,11 @@ static int pwc_s_fmt_vid_cap(struct file *file, void *fh, struct v4l2_format *f)
        struct pwc_device *pdev = video_drvdata(file);
        int ret, fps, snapshot, compression, pixelformat;
 
-       if (!pdev->udev)
-               return -ENODEV;
-
-       if (pdev->capt_file != NULL &&
-           pdev->capt_file != file)
+       if (pwc_test_n_set_capt_file(pdev, file))
                return -EBUSY;
 
-       pdev->capt_file = file;
-
        ret = pwc_vidioc_try_fmt(pdev, f);
-       if (ret<0)
+       if (ret < 0)
                return ret;
 
        pixelformat = f->fmt.pix.pixelformat;
@@ -505,8 +499,16 @@ static int pwc_s_fmt_vid_cap(struct file *file, void *fh, struct v4l2_format *f)
            pixelformat != V4L2_PIX_FMT_PWC2)
                return -EINVAL;
 
-       if (vb2_is_streaming(&pdev->vb_queue))
-               return -EBUSY;
+       mutex_lock(&pdev->udevlock);
+       if (!pdev->udev) {
+               ret = -ENODEV;
+               goto leave;
+       }
+
+       if (pdev->iso_init) {
+               ret = -EBUSY;
+               goto leave;
+       }
 
        PWC_DEBUG_IOCTL("Trying to set format to: width=%d height=%d fps=%d "
                        "compression=%d snapshot=%d format=%c%c%c%c\n",
@@ -526,15 +528,14 @@ static int pwc_s_fmt_vid_cap(struct file *file, void *fh, struct v4l2_format *f)
 
        PWC_DEBUG_IOCTL("pwc_set_video_mode(), return=%d\n", ret);
 
-       if (ret)
-               return ret;
-
-       pdev->pixfmt = pixelformat;
-
-       pwc_vidioc_fill_fmt(pdev, f);
-
-       return 0;
+       if (ret == 0) {
+               pdev->pixfmt = pixelformat;
+               pwc_vidioc_fill_fmt(pdev, f);
+       }
 
+leave:
+       mutex_unlock(&pdev->udevlock);
+       return ret;
 }
 
 static int pwc_querycap(struct file *file, void *fh, struct v4l2_capability *cap)
@@ -580,18 +581,6 @@ static int pwc_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
                container_of(ctrl->handler, struct pwc_device, ctrl_handler);
        int ret = 0;
 
-       /*
-        * Sometimes it can take quite long for the pwc to complete usb control
-        * transfers, so release the modlock to give streaming by another
-        * process / thread the chance to continue with a dqbuf.
-        */
-       mutex_unlock(&pdev->modlock);
-
-       /*
-        * Take the udev-lock to protect against the disconnect handler
-        * completing and setting dev->udev to NULL underneath us. Other code
-        * does not need to do this since it is protected by the modlock.
-        */
        mutex_lock(&pdev->udevlock);
 
        if (!pdev->udev) {
@@ -664,7 +653,6 @@ static int pwc_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 
 leave:
        mutex_unlock(&pdev->udevlock);
-       mutex_lock(&pdev->modlock);
        return ret;
 }
 
@@ -844,8 +832,6 @@ static int pwc_s_ctrl(struct v4l2_ctrl *ctrl)
                container_of(ctrl->handler, struct pwc_device, ctrl_handler);
        int ret = 0;
 
-       /* See the comments on locking in pwc_g_volatile_ctrl */
-       mutex_unlock(&pdev->modlock);
        mutex_lock(&pdev->udevlock);
 
        if (!pdev->udev) {
@@ -951,7 +937,6 @@ static int pwc_s_ctrl(struct v4l2_ctrl *ctrl)
 
 leave:
        mutex_unlock(&pdev->udevlock);
-       mutex_lock(&pdev->modlock);
        return ret;
 }
 
@@ -981,9 +966,11 @@ static int pwc_g_fmt_vid_cap(struct file *file, void *fh, struct v4l2_format *f)
 {
        struct pwc_device *pdev = video_drvdata(file);
 
+       mutex_lock(&pdev->udevlock); /* To avoid race with s_fmt */
        PWC_DEBUG_IOCTL("ioctl(VIDIOC_G_FMT) return size %dx%d\n",
                        pdev->image.x, pdev->image.y);
        pwc_vidioc_fill_fmt(pdev, f);
+       mutex_unlock(&pdev->udevlock);
        return 0;
 }
 
@@ -999,12 +986,9 @@ static int pwc_reqbufs(struct file *file, void *fh,
 {
        struct pwc_device *pdev = video_drvdata(file);
 
-       if (pdev->capt_file != NULL &&
-           pdev->capt_file != file)
+       if (pwc_test_n_set_capt_file(pdev, file))
                return -EBUSY;
 
-       pdev->capt_file = file;
-
        return vb2_reqbufs(&pdev->vb_queue, rb);
 }
 

diff --git a/drivers/media/video/pwc/pwc.h b/drivers/media/video/pwc/pwc.h
index 04e9524..c2594f4 100644 (file)
--- a/drivers/media/video/pwc/pwc.h
+++ b/drivers/media/video/pwc/pwc.h
@@ -202,11 +202,9 @@ struct pwc_device
 {
        struct video_device vdev;
        struct v4l2_device v4l2_dev;
-       struct mutex modlock;
 
        /* Pointer to our usb_device, may be NULL after unplug */
        struct usb_device *udev;
-       /* Protects the setting of udev to NULL by our disconnect handler */
        struct mutex udevlock;
 
        /* type of cam (645, 646, 675, 680, 690, 720, 730, 740, 750) */
@@ -217,6 +215,7 @@ struct pwc_device
 
        /*** Video data ***/
        struct file *capt_file; /* file doing video capture */
+       struct mutex capt_file_lock;
        int vendpoint;          /* video isoc endpoint */
        int vcinterface;        /* video control interface */
        int valternate;         /* alternate interface needed */
@@ -350,6 +349,8 @@ struct pwc_device
 extern int pwc_trace;
 #endif
 
+int pwc_test_n_set_capt_file(struct pwc_device *pdev, struct file *file);
+
 /** Functions in pwc-misc.c */
 /* sizes in pixels */
 extern const struct pwc_coord pwc_image_sizes[PSZ_MAX];