From 9a7b2d1f0eb0a6b674726c9a9d77ce83fd0b27fe Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Mon, 6 Jun 2011 14:43:39 -0300 Subject: [PATCH] [media] pwc: better usb disconnect handling 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 Cc: stable@kernel.org Signed-off-by: Mauro Carvalho Chehab --- drivers/media/video/pwc/pwc-ctrl.c | 2 +- drivers/media/video/pwc/pwc-if.c | 152 +++++++++-------------------- drivers/media/video/pwc/pwc.h | 4 +- 3 files changed, 50 insertions(+), 108 deletions(-) diff --git a/drivers/media/video/pwc/pwc-ctrl.c b/drivers/media/video/pwc/pwc-ctrl.c index 1593f8deb810..760b4de13adf 100644 --- a/drivers/media/video/pwc/pwc-ctrl.c +++ b/drivers/media/video/pwc/pwc-ctrl.c @@ -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; diff --git a/drivers/media/video/pwc/pwc-if.c b/drivers/media/video/pwc/pwc-if.c index 356cd42b593b..b0bde5a87c8a 100644 --- a/drivers/media/video/pwc/pwc-if.c +++ b/drivers/media/video/pwc/pwc-if.c @@ -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 } diff --git a/drivers/media/video/pwc/pwc.h b/drivers/media/video/pwc/pwc.h index e947766337d6..083f8b15df73 100644 --- a/drivers/media/video/pwc/pwc.h +++ b/drivers/media/video/pwc/pwc.h @@ -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) */ -- 2.20.1