Hi Shawn,
Could you please confirm whether this patch fixes the issue you've reported ?
On Tuesday 07 May 2013 13:19:37 Laurent Pinchart wrote:
> Maintaining the users count using an atomic variable makes sure that
> access to the counter won't be racy, but doesn't serialize access to the
> operations protected by the counter. This creates a race condition that
> could result in the status URB being submitted multiple times.
>
> Use a mutex to protect the users count and serialize access to the
> status start and stop operations.
>
> Reported-by: Shawn Nematbakhsh <shawnn@xxxxxxxxxxxx>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/usb/uvc/uvc_driver.c | 23 +++++++++++++++++------
> drivers/media/usb/uvc/uvc_status.c | 21 ++-------------------
> drivers/media/usb/uvc/uvc_v4l2.c | 14 ++++++++++----
> drivers/media/usb/uvc/uvcvideo.h | 7 +++----
> 4 files changed, 32 insertions(+), 33 deletions(-)
>
> Changes since v1:
>
> - Add a missing return back in the uvc_suspend() function
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index e68fa53..d704be3 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1836,8 +1836,8 @@ static int uvc_probe(struct usb_interface *intf,
> INIT_LIST_HEAD(&dev->chains);
> INIT_LIST_HEAD(&dev->streams);
> atomic_set(&dev->nstreams, 0);
> - atomic_set(&dev->users, 0);
> atomic_set(&dev->nmappings, 0);
> + mutex_init(&dev->lock);
>
> dev->udev = usb_get_dev(udev);
> dev->intf = usb_get_intf(intf);
> @@ -1953,8 +1953,13 @@ static int uvc_suspend(struct usb_interface *intf,
> pm_message_t message)
>
> /* Controls are cached on the fly so they don't need to be saved. */
> if (intf->cur_altsetting->desc.bInterfaceSubClass ==
> - UVC_SC_VIDEOCONTROL)
> - return uvc_status_suspend(dev);
> + UVC_SC_VIDEOCONTROL) {
> + mutex_lock(&dev->lock);
> + if (dev->users)
> + uvc_status_stop(dev);
> + mutex_unlock(&dev->lock);
> + return 0;
> + }
>
> list_for_each_entry(stream, &dev->streams, list) {
> if (stream->intf == intf)
> @@ -1976,14 +1981,20 @@ static int __uvc_resume(struct usb_interface *intf,
> int reset)
>
> if (intf->cur_altsetting->desc.bInterfaceSubClass ==
> UVC_SC_VIDEOCONTROL) {
> - if (reset) {
> - int ret = uvc_ctrl_resume_device(dev);
> + int ret = 0;
>
> + if (reset) {
> + ret = uvc_ctrl_resume_device(dev);
> if (ret < 0)
> return ret;
> }
>
> - return uvc_status_resume(dev);
> + mutex_lock(&dev->lock);
> + if (dev->users)
> + ret = uvc_status_start(dev, GFP_NOIO);
> + mutex_unlock(&dev->lock);
> +
> + return ret;
> }
>
> list_for_each_entry(stream, &dev->streams, list) {
> diff --git a/drivers/media/usb/uvc/uvc_status.c
> b/drivers/media/usb/uvc/uvc_status.c index b749277..f552ab9 100644
> --- a/drivers/media/usb/uvc/uvc_status.c
> +++ b/drivers/media/usb/uvc/uvc_status.c
> @@ -206,32 +206,15 @@ void uvc_status_cleanup(struct uvc_device *dev)
> uvc_input_cleanup(dev);
> }
>
> -int uvc_status_start(struct uvc_device *dev)
> +int uvc_status_start(struct uvc_device *dev, gfp_t flags)
> {
> if (dev->int_urb == NULL)
> return 0;
>
> - return usb_submit_urb(dev->int_urb, GFP_KERNEL);
> + return usb_submit_urb(dev->int_urb, flags);
> }
>
> void uvc_status_stop(struct uvc_device *dev)
> {
> usb_kill_urb(dev->int_urb);
> }
> -
> -int uvc_status_suspend(struct uvc_device *dev)
> -{
> - if (atomic_read(&dev->users))
> - usb_kill_urb(dev->int_urb);
> -
> - return 0;
> -}
> -
> -int uvc_status_resume(struct uvc_device *dev)
> -{
> - if (dev->int_urb == NULL || atomic_read(&dev->users) == 0)
> - return 0;
> -
> - return usb_submit_urb(dev->int_urb, GFP_NOIO);
> -}
> -
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> b/drivers/media/usb/uvc/uvc_v4l2.c index b2dc326..3afff92 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -498,16 +498,20 @@ static int uvc_v4l2_open(struct file *file)
> return -ENOMEM;
> }
>
> - if (atomic_inc_return(&stream->dev->users) == 1) {
> - ret = uvc_status_start(stream->dev);
> + mutex_lock(&stream->dev->lock);
> + if (stream->dev->users == 0) {
> + ret = uvc_status_start(stream->dev, GFP_KERNEL);
> if (ret < 0) {
> - atomic_dec(&stream->dev->users);
> + mutex_unlock(&stream->dev->lock);
> usb_autopm_put_interface(stream->dev->intf);
> kfree(handle);
> return ret;
> }
> }
>
> + stream->dev->users++;
> + mutex_unlock(&stream->dev->lock);
> +
> v4l2_fh_init(&handle->vfh, stream->vdev);
> v4l2_fh_add(&handle->vfh);
> handle->chain = stream->chain;
> @@ -538,8 +542,10 @@ static int uvc_v4l2_release(struct file *file)
> kfree(handle);
> file->private_data = NULL;
>
> - if (atomic_dec_return(&stream->dev->users) == 0)
> + mutex_lock(&stream->dev->lock);
> + if (--stream->dev->users == 0)
> uvc_status_stop(stream->dev);
> + mutex_unlock(&stream->dev->lock);
>
> usb_autopm_put_interface(stream->dev->intf);
> return 0;
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index 9cd584a..eb90a92 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -515,7 +515,8 @@ struct uvc_device {
> char name[32];
>
> enum uvc_device_state state;
> - atomic_t users;
> + struct mutex lock; /* Protects users */
> + unsigned int users;
> atomic_t nmappings;
>
> /* Video control interface */
> @@ -661,10 +662,8 @@ void uvc_video_clock_update(struct uvc_streaming
> *stream, /* Status */
> extern int uvc_status_init(struct uvc_device *dev);
> extern void uvc_status_cleanup(struct uvc_device *dev);
> -extern int uvc_status_start(struct uvc_device *dev);
> +extern int uvc_status_start(struct uvc_device *dev, gfp_t flags);
> extern void uvc_status_stop(struct uvc_device *dev);
> -extern int uvc_status_suspend(struct uvc_device *dev);
> -extern int uvc_status_resume(struct uvc_device *dev);
>
> /* Controls */
> extern const struct v4l2_subscribed_event_ops uvc_ctrl_sub_ev_ops;
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
