Re: [PATCH] [media] uvcvideo: Retry usb_submit_urb on -EPERM return

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Shawn,

On Friday 03 May 2013 16:00:24 Shawn Nematbakhsh wrote:
> Hi Laurent,
> 
> Thanks for the changes! I agree that your synchronization logic is
> correct. Just two small comments:
> 
> On Mon, Apr 29, 2013 at 1:34 PM, Laurent Pinchart wrote:
> > On Tuesday 23 April 2013 17:42:32 Shawn Nematbakhsh wrote:
> >> While usb_kill_urb is in progress, calls to usb_submit_urb will fail
> >> with -EPERM (documented in Documentation/usb/URB.txt). The UVC driver
> >> does not correctly handle this case -- there is no synchronization
> >> between uvc_v4l2_open / uvc_status_start and uvc_v4l2_release /
> >> uvc_status_stop.
> > 
> > Wouldn't it be better to synchronize status operations in open/release ?
> > Something like the following patch:
> > 
> > From 9285d678ed2f823bb215f6bdec3ca1a9e1cac977 Mon Sep 17 00:00:00 2001
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Date: Fri, 26 Apr 2013 03:28:51 +0200
> > Subject: [PATCH] uvcvideo: Fix open/close race condition
> > 
> > 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@chromium.org>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/media/usb/uvc/uvc_driver.c | 22 ++++++++++++++++------
> >  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, 31 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c
> > b/drivers/media/usb/uvc/uvc_driver.c index e68fa53..b638037 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);
> 
> I think dev->users is uninitialized now? Probably we should initialize
> to 0 here.

The whole dev structure is memset to 0 when allocated a couple of lines above, 
so there's no need to explicitly zero all fields.

> >         atomic_set(&dev->nmappings, 0);
> > +       mutex_init(&dev->lock);
> > 
> >         dev->udev = usb_get_dev(udev);
> >         dev->intf = usb_get_intf(intf);
> > @@ -1953,8 +1953,12 @@ 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);
> 
> To keep the same control flow, should we return here?

Oops, my bad, I'll fix that and resubmit.

> > +       }
> >         list_for_each_entry(stream, &dev->streams, list) {
> >                 if (stream->intf == intf)

-- 
Regards,

Laurent Pinchart