Hi Hans,

The entire series looks great, I do have a few remarks wrt this
patch, which I have fixed in my own tree (new version attached,
note untested sofar).

On 05/06/2012 02:28 PM, Hans Verkuil wrote:
> From: Hans Verkuil<hans.verkuil@cisco.com>
>
> Due to latency concerns the VIDIOC_QBUF, DQBUF and QUERYBUF do not use the
> core lock, instead they rely only on queue_lock.
>
> Signed-off-by: Hans Verkuil<hans.verkuil@cisco.com>
> ---
>   drivers/media/video/gspca/gspca.c |  203 ++++++++-----------------------------
>   1 file changed, 41 insertions(+), 162 deletions(-)
>
> diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
> index f840bed..edca4f3 100644
> --- a/drivers/media/video/gspca/gspca.c
> +++ b/drivers/media/video/gspca/gspca.c
> @@ -850,14 +850,6 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
>   	struct ep_tb_s ep_tb[MAX_ALT];
>   	int n, ret, xfer, alt, alt_idx;
>
> -	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
> -		return -ERESTARTSYS;
> -
> -	if (!gspca_dev->present) {
> -		ret = -ENODEV;
> -		goto unlock;
> -	}
> -
>   	/* reset the streaming variables */
>   	gspca_dev->image = NULL;
>   	gspca_dev->image_len = 0;

You're removing a lot of checks for gspca_dev->present, relying on the
video_is_registered checks in v4l2-dev instead I assume, this is a good
idea, *but* it requires a small hack in disconnect to close a race.

Currently the end of gspca_disconnect looks like this:

         gspca_dev->dev = NULL;
         v4l2_device_disconnect(&gspca_dev->v4l2_dev);
         mutex_unlock(&gspca_dev->usb_lock);

         usb_set_intfdata(intf, NULL);

         /* release the device */
         /* (this will call gspca_release() immediately or on last close) */
         video_unregister_device(&gspca_dev->vdev);
}

Notice that usb_lock is unlocked before video_unregister_device gets called,
which means that any ioctl or other fops waiting for usb_lock can run
before video_unregister_device runs, and thus before they are protected
against being called on an disconnected device by the
video_is_registered checks in v4l2-dev.

Unfortunately simply moving the unlock down won't work, because if there
are no open file handles referencing the device, then the memory
referenced by gspca_dev will be free-ed after the video_unregister_device
call.

So I've changed disconnect to the following in my version, to allow the
present check removal you've did, as I quite like being able to
remove all those present checks :)   :

         /* The USB-interface device is freed at exit of this function */
         gspca_dev->dev = NULL;
         v4l2_device_disconnect(&gspca_dev->v4l2_dev);

         /* Ensure gspca_dev sticks around for the usb_lock unlock! */
         get_device(&gspca_dev->vdev.dev);
         video_unregister_device(&gspca_dev->vdev);
         mutex_unlock(&gspca_dev->usb_lock);
         /* (this will call gspca_release() immediately or on last close) */
         put_device(&gspca_dev->vdev.dev);

         usb_set_intfdata(intf, NULL);
}


<snip chunks on which I've no comments>

> @@ -1736,10 +1658,8 @@ static int vidioc_streamoff(struct file *file, void *priv,
>   	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
>   		return -ERESTARTSYS;
>
> -	if (!gspca_dev->streaming) {
> -		ret = 0;
> -		goto out;
> -	}
> +	if (!gspca_dev->streaming)
> +		return 0;
>

BAD! queue_lock is held here, so you cannot just change this to a return!

>   	/* check the capture file */
>   	if (gspca_dev->capt_file != file) {

<snip chunks on which I've no comments>

> @@ -2009,11 +1883,9 @@ static int vidioc_dqbuf(struct file *file, void *priv,
>   	gspca_dev->fr_o = (i + 1) % GSPCA_MAX_FRAMES;
>
>   	if (gspca_dev->sd_desc->dq_callback) {
> -		mutex_lock(&gspca_dev->usb_lock);
>   		gspca_dev->usb_err = 0;
>   		if (gspca_dev->present)
>   			gspca_dev->sd_desc->dq_callback(gspca_dev);
> -		mutex_unlock(&gspca_dev->usb_lock);
>   	}
>
>   	frame->v4l2_buf.flags&= ~V4L2_BUF_FLAG_DONE;

You cannot remove the locking here, as dq_callback expects to be
called with the usb-lock locked.

Since usb-lock now is the device lock and thus gets locked before
the queue_lock, we cannot simply drop this chunk. Instead I've
moved the dq_callback to the end of vidioc_dqbuf, so after the
stream_lock has been released (there is no reason to have
the stream_lock hold when calling the dq_callback).

The dq_callback is used to do camera control adjustments which
need to be done after every X frames, and which cannot be done
from the isoc frame interrupts since they should not be done under
interrupt. When the drivers using dq_callback are converted to the
control framework, they will likely end up calling v4l2_ctrl_s_ctrl
from the dq_callback.

<snip chunks on which I've no comments>

Regards,

Hans


p.s.

I've yet to take a good look at the driver conversions other then
the zc3xx conversion.