[media] pwc: better usb disconnect handling
authorHans de Goede <hdegoede@redhat.com>
Mon, 6 Jun 2011 17:43:39 +0000 (14:43 -0300)
committerMauro Carvalho Chehab <mchehab@redhat.com>
Tue, 21 Jun 2011 16:31:12 +0000 (13:31 -0300)
Unplugging a pwc cam while an app has the /dev/video# node open leads
to an oops in pwc_video_close when the app closes the node, because
the disconnect handler has free-ed the pdev struct pwc_video_close
tries to use. Instead of adding some sort of bandaid for this.
fix it properly using the v4l2 core's new(ish) behavior of keeping the
v4l2_dev structure around until both unregister has been called, and
all file handles referring  to it have been closed:

Embed the v4l2_dev structure in the pdev structure and define a v4l2 dev
release callback releasing the pdev structure (and thus also the embedded
v4l2 dev structure.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Cc: stable@kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
drivers/media/video/pwc/pwc-ctrl.c
drivers/media/video/pwc/pwc-if.c
drivers/media/video/pwc/pwc.h

index 1593f8deb8105e18bc9fd1d5b09e5c8b8e9f68ea..760b4de13adf6fa852b358ae88a9751f7cd1b597 100644 (file)
@@ -1414,7 +1414,7 @@ long pwc_ioctl(struct pwc_device *pdev, unsigned int cmd, void *arg)
        {
                ARG_DEF(struct pwc_probe, probe)
 
-               strcpy(ARGR(probe).name, pdev->vdev->name);
+               strcpy(ARGR(probe).name, pdev->vdev.name);
                ARGR(probe).type = pdev->type;
                ARG_OUT(probe)
                break;
index 356cd42b593b3f36b72569d24217768c220e6c70..b0bde5a87c8a8b08575973f3158dde40ed546cc2 100644 (file)
@@ -40,7 +40,7 @@
    Oh yes, convention: to disctinguish between all the various pointers to
    device-structures, I use these names for the pointer variables:
    udev: struct usb_device *
-   vdev: struct video_device *
+   vdev: struct video_device (member of pwc_dev)
    pdev: struct pwc_devive *
 */
 
@@ -152,6 +152,7 @@ static ssize_t pwc_video_read(struct file *file, char __user *buf,
                          size_t count, loff_t *ppos);
 static unsigned int pwc_video_poll(struct file *file, poll_table *wait);
 static int  pwc_video_mmap(struct file *file, struct vm_area_struct *vma);
+static void pwc_video_release(struct video_device *vfd);
 
 static const struct v4l2_file_operations pwc_fops = {
        .owner =        THIS_MODULE,
@@ -164,41 +165,11 @@ static const struct v4l2_file_operations pwc_fops = {
 };
 static struct video_device pwc_template = {
        .name =         "Philips Webcam",       /* Filled in later */
-       .release =      video_device_release,
+       .release =      pwc_video_release,
        .fops =         &pwc_fops,
+       .ioctl_ops =    &pwc_ioctl_ops,
 };
 
-/***************************************************************************/
-
-/* Okay, this is some magic that I worked out and the reasoning behind it...
-
-   The biggest problem with any USB device is of course: "what to do
-   when the user unplugs the device while it is in use by an application?"
-   We have several options:
-   1) Curse them with the 7 plagues when they do (requires divine intervention)
-   2) Tell them not to (won't work: they'll do it anyway)
-   3) Oops the kernel (this will have a negative effect on a user's uptime)
-   4) Do something sensible.
-
-   Of course, we go for option 4.
-
-   It happens that this device will be linked to two times, once from
-   usb_device and once from the video_device in their respective 'private'
-   pointers. This is done when the device is probed() and all initialization
-   succeeded. The pwc_device struct links back to both structures.
-
-   When a device is unplugged while in use it will be removed from the
-   list of known USB devices; I also de-register it as a V4L device, but
-   unfortunately I can't free the memory since the struct is still in use
-   by the file descriptor. This free-ing is then deferend until the first
-   opportunity. Crude, but it works.
-
-   A small 'advantage' is that if a user unplugs the cam and plugs it back
-   in, it should get assigned the same video device minor, but unfortunately
-   it's non-trivial to re-link the cam back to the video device... (that
-   would surely be magic! :))
-*/
-
 /***************************************************************************/
 /* Private functions */
 
@@ -1016,16 +987,15 @@ static ssize_t show_snapshot_button_status(struct device *class_dev,
 static DEVICE_ATTR(button, S_IRUGO | S_IWUSR, show_snapshot_button_status,
                   NULL);
 
-static int pwc_create_sysfs_files(struct video_device *vdev)
+static int pwc_create_sysfs_files(struct pwc_device *pdev)
 {
-       struct pwc_device *pdev = video_get_drvdata(vdev);
        int rc;
 
-       rc = device_create_file(&vdev->dev, &dev_attr_button);
+       rc = device_create_file(&pdev->vdev.dev, &dev_attr_button);
        if (rc)
                goto err;
        if (pdev->features & FEATURE_MOTOR_PANTILT) {
-               rc = device_create_file(&vdev->dev, &dev_attr_pan_tilt);
+               rc = device_create_file(&pdev->vdev.dev, &dev_attr_pan_tilt);
                if (rc)
                        goto err_button;
        }
@@ -1033,19 +1003,17 @@ static int pwc_create_sysfs_files(struct video_device *vdev)
        return 0;
 
 err_button:
-       device_remove_file(&vdev->dev, &dev_attr_button);
+       device_remove_file(&pdev->vdev.dev, &dev_attr_button);
 err:
        PWC_ERROR("Could not create sysfs files.\n");
        return rc;
 }
 
-static void pwc_remove_sysfs_files(struct video_device *vdev)
+static void pwc_remove_sysfs_files(struct pwc_device *pdev)
 {
-       struct pwc_device *pdev = video_get_drvdata(vdev);
-
        if (pdev->features & FEATURE_MOTOR_PANTILT)
-               device_remove_file(&vdev->dev, &dev_attr_pan_tilt);
-       device_remove_file(&vdev->dev, &dev_attr_button);
+               device_remove_file(&pdev->vdev.dev, &dev_attr_pan_tilt);
+       device_remove_file(&pdev->vdev.dev, &dev_attr_button);
 }
 
 #ifdef CONFIG_USB_PWC_DEBUG
@@ -1106,7 +1074,7 @@ static int pwc_video_open(struct file *file)
                if (ret >= 0)
                {
                        PWC_DEBUG_OPEN("This %s camera is equipped with a %s (%d).\n",
-                                       pdev->vdev->name,
+                                       pdev->vdev.name,
                                        pwc_sensor_type_to_string(i), i);
                }
        }
@@ -1180,16 +1148,15 @@ static int pwc_video_open(struct file *file)
        return 0;
 }
 
-
-static void pwc_cleanup(struct pwc_device *pdev)
+static void pwc_video_release(struct video_device *vfd)
 {
-       pwc_remove_sysfs_files(pdev->vdev);
-       video_unregister_device(pdev->vdev);
+       struct pwc_device *pdev = container_of(vfd, struct pwc_device, vdev);
+       int hint;
 
-#ifdef CONFIG_USB_PWC_INPUT_EVDEV
-       if (pdev->button_dev)
-               input_unregister_device(pdev->button_dev);
-#endif
+       /* search device_hint[] table if we occupy a slot, by any chance */
+       for (hint = 0; hint < MAX_DEV_HINTS; hint++)
+               if (device_hint[hint].pdev == pdev)
+                       device_hint[hint].pdev = NULL;
 
        kfree(pdev);
 }
@@ -1199,7 +1166,7 @@ static int pwc_video_close(struct file *file)
 {
        struct video_device *vdev = file->private_data;
        struct pwc_device *pdev;
-       int i, hint;
+       int i;
 
        PWC_DEBUG_OPEN(">> video_close called(vdev = 0x%p).\n", vdev);
 
@@ -1234,12 +1201,6 @@ static int pwc_video_close(struct file *file)
                }
                pdev->vopen--;
                PWC_DEBUG_OPEN("<< video_close() vopen=%d\n", pdev->vopen);
-       } else {
-               pwc_cleanup(pdev);
-               /* search device_hint[] table if we occupy a slot, by any chance */
-               for (hint = 0; hint < MAX_DEV_HINTS; hint++)
-                       if (device_hint[hint].pdev == pdev)
-                               device_hint[hint].pdev = NULL;
        }
 
        return 0;
@@ -1715,19 +1676,12 @@ static int usb_pwc_probe(struct usb_interface *intf, const struct usb_device_id
        init_waitqueue_head(&pdev->frameq);
        pdev->vcompression = pwc_preferred_compression;
 
-       /* Allocate video_device structure */
-       pdev->vdev = video_device_alloc();
-       if (!pdev->vdev) {
-               PWC_ERROR("Err, cannot allocate video_device struture. Failing probe.");
-               rc = -ENOMEM;
-               goto err_free_mem;
-       }
-       memcpy(pdev->vdev, &pwc_template, sizeof(pwc_template));
-       pdev->vdev->parent = &intf->dev;
-       pdev->vdev->lock = &pdev->modlock;
-       pdev->vdev->ioctl_ops = &pwc_ioctl_ops;
-       strcpy(pdev->vdev->name, name);
-       video_set_drvdata(pdev->vdev, pdev);
+       /* Init video_device structure */
+       memcpy(&pdev->vdev, &pwc_template, sizeof(pwc_template));
+       pdev->vdev.parent = &intf->dev;
+       pdev->vdev.lock = &pdev->modlock;
+       strcpy(pdev->vdev.name, name);
+       video_set_drvdata(&pdev->vdev, pdev);
 
        pdev->release = le16_to_cpu(udev->descriptor.bcdDevice);
        PWC_DEBUG_PROBE("Release: %04x\n", pdev->release);
@@ -1746,8 +1700,6 @@ static int usb_pwc_probe(struct usb_interface *intf, const struct usb_device_id
                }
        }
 
-       pdev->vdev->release = video_device_release;
-
        /* occupy slot */
        if (hint < MAX_DEV_HINTS)
                device_hint[hint].pdev = pdev;
@@ -1759,16 +1711,16 @@ static int usb_pwc_probe(struct usb_interface *intf, const struct usb_device_id
        pwc_set_leds(pdev, 0, 0);
        pwc_camera_power(pdev, 0);
 
-       rc = video_register_device(pdev->vdev, VFL_TYPE_GRABBER, video_nr);
+       rc = video_register_device(&pdev->vdev, VFL_TYPE_GRABBER, video_nr);
        if (rc < 0) {
                PWC_ERROR("Failed to register as video device (%d).\n", rc);
-               goto err_video_release;
+               goto err_free_mem;
        }
-       rc = pwc_create_sysfs_files(pdev->vdev);
+       rc = pwc_create_sysfs_files(pdev);
        if (rc)
                goto err_video_unreg;
 
-       PWC_INFO("Registered as %s.\n", video_device_node_name(pdev->vdev));
+       PWC_INFO("Registered as %s.\n", video_device_node_name(&pdev->vdev));
 
 #ifdef CONFIG_USB_PWC_INPUT_EVDEV
        /* register webcam snapshot button input device */
@@ -1776,7 +1728,7 @@ static int usb_pwc_probe(struct usb_interface *intf, const struct usb_device_id
        if (!pdev->button_dev) {
                PWC_ERROR("Err, insufficient memory for webcam snapshot button device.");
                rc = -ENOMEM;
-               pwc_remove_sysfs_files(pdev->vdev);
+               pwc_remove_sysfs_files(pdev);
                goto err_video_unreg;
        }
 
@@ -1794,7 +1746,7 @@ static int usb_pwc_probe(struct usb_interface *intf, const struct usb_device_id
        if (rc) {
                input_free_device(pdev->button_dev);
                pdev->button_dev = NULL;
-               pwc_remove_sysfs_files(pdev->vdev);
+               pwc_remove_sysfs_files(pdev);
                goto err_video_unreg;
        }
 #endif
@@ -1804,10 +1756,7 @@ static int usb_pwc_probe(struct usb_interface *intf, const struct usb_device_id
 err_video_unreg:
        if (hint < MAX_DEV_HINTS)
                device_hint[hint].pdev = NULL;
-       video_unregister_device(pdev->vdev);
-       pdev->vdev = NULL;      /* So we don't try to release it below */
-err_video_release:
-       video_device_release(pdev->vdev);
+       video_unregister_device(&pdev->vdev);
 err_free_mem:
        kfree(pdev);
        return rc;
@@ -1816,10 +1765,8 @@ err_free_mem:
 /* The user yanked out the cable... */
 static void usb_pwc_disconnect(struct usb_interface *intf)
 {
-       struct pwc_device *pdev;
-       int hint;
+       struct pwc_device *pdev  = usb_get_intfdata(intf);
 
-       pdev = usb_get_intfdata (intf);
        mutex_lock(&pdev->modlock);
        usb_set_intfdata (intf, NULL);
        if (pdev == NULL) {
@@ -1836,30 +1783,25 @@ static void usb_pwc_disconnect(struct usb_interface *intf)
        }
 
        /* We got unplugged; this is signalled by an EPIPE error code */
-       if (pdev->vopen) {
-               PWC_INFO("Disconnected while webcam is in use!\n");
-               pdev->error_status = EPIPE;
-       }
+       pdev->error_status = EPIPE;
+       pdev->unplugged = 1;
 
        /* Alert waiting processes */
        wake_up_interruptible(&pdev->frameq);
-       /* Wait until device is closed */
-       if (pdev->vopen) {
-               pdev->unplugged = 1;
-               pwc_iso_stop(pdev);
-       } else {
-               /* Device is closed, so we can safely unregister it */
-               PWC_DEBUG_PROBE("Unregistering video device in disconnect().\n");
 
-disconnect_out:
-               /* search device_hint[] table if we occupy a slot, by any chance */
-               for (hint = 0; hint < MAX_DEV_HINTS; hint++)
-                       if (device_hint[hint].pdev == pdev)
-                               device_hint[hint].pdev = NULL;
-       }
+       /* No need to keep the urbs around after disconnection */
+       pwc_isoc_cleanup(pdev);
 
+disconnect_out:
        mutex_unlock(&pdev->modlock);
-       pwc_cleanup(pdev);
+
+       pwc_remove_sysfs_files(pdev);
+       video_unregister_device(&pdev->vdev);
+
+#ifdef CONFIG_USB_PWC_INPUT_EVDEV
+       if (pdev->button_dev)
+               input_unregister_device(pdev->button_dev);
+#endif
 }
 
 
index e947766337d63d9233ed333539705e677f81a742..083f8b15df7337323666f01af18dc9ab5aa183bc 100644 (file)
@@ -162,9 +162,9 @@ struct pwc_imgbuf
 
 struct pwc_device
 {
-   struct video_device *vdev;
+       struct video_device vdev;
 
-   /* Pointer to our usb_device */
+   /* Pointer to our usb_device, may be NULL after unplug */
    struct usb_device *udev;
 
    int type;                    /* type of cam (645, 646, 675, 680, 690, 720, 730, 740, 750) */