Hi Pekka,
On Thursday 29 May 2008, Laurent Pinchart wrote:
> Hi Pekka,
>
> On Saturday 24 May 2008, Pekka Vuorela wrote:
> > Hello,
> >
> > I debugged a bug[1] I was having with Motion and noticed that the UVC
> > driver doesn't return an error when doing an ioctl() on a disconnected
> > device. Wouldn't ENODEV be a sensible value to return in such
> > situations?
> >
> > Anyway, this small patch adds a check in the beginning of
> > uvc_v4l2_do_ioctl(). With the patch Motion works with me as it seems to
> > work with the pwc driver.
> >
> > 1:
> > http://www.lavrsen.dk/twiki/bin/view/Motion/BugReport2008x05x17x175521
>
> Thanks for debugging the problem. I'm still not sure if all ioctls should
> return -ENODEV when the device gets disconnected. It might make sense to
> allow applications to retrieve the last few captured images for instance.
>
> I'll think about this and I'll work on a patch in the next few days.
I gave this some more thought and concluded that blindly disabling all ioctls
once the device is disconnected is not the best solution. VIDIOC_REQBUFS
should still be callable to free the buffers to let applications terminate
gracefully. Several other ioctls can also be useful (VIDIOC_DQBUF for
instance, to dequeue the remaining buffers).
Beside, there was a race condition in your patch. URBs are cancelled before
the disconnect callback is invoked. As there is no locking there, an
application could possibly dequeue and requeue buffers in that small time
window.
I've attached a patch that should not suffer from he race condition. It makes
VIDIOC_QBUF return -ENODEV when the device is disconnected. VIDIOC_DQBUF
still succeeds if there as long as there are queued buffers.
Could you please test the patch ?
Best regards,
Laurent Pinchart
Index: uvc_queue.c
===================================================================
--- uvc_queue.c (revision 211)
+++ uvc_queue.c (working copy)
@@ -257,10 +257,15 @@
goto done;
}
+ spin_lock_irqsave(&queue->irqlock, flags);
+ if (queue->flags & UVC_QUEUE_DISCONNECTED) {
+ spin_unlock_irqrestore(&queue->irqlock, flags);
+ ret = -ENODEV;
+ goto done;
+ }
buf->state = UVC_BUF_STATE_QUEUED;
buf->buf.bytesused = 0;
list_add_tail(&buf->stream, &queue->mainqueue);
- spin_lock_irqsave(&queue->irqlock, flags);
list_add_tail(&buf->queue, &queue->irqqueue);
spin_unlock_irqrestore(&queue->irqlock, flags);
@@ -394,20 +399,20 @@
mutex_lock(&queue->mutex);
if (enable) {
- if (queue->streaming) {
+ if (uvc_queue_streaming(queue)) {
ret = -EBUSY;
goto done;
}
queue->sequence = 0;
- queue->streaming = 1;
+ queue->flags |= UVC_QUEUE_STREAMING;
} else {
- uvc_queue_cancel(queue);
+ uvc_queue_cancel(queue, 0);
INIT_LIST_HEAD(&queue->mainqueue);
for (i = 0; i < queue->count; ++i)
queue->buffer[i].state = UVC_BUF_STATE_IDLE;
- queue->streaming = 0;
+ queue->flags &= ~UVC_QUEUE_STREAMING;
}
done:
@@ -421,10 +426,13 @@
* Cancelling the queue marks all buffers on the irq queue as erroneous,
* wakes them up and remove them from the queue.
*
+ * If the disconnect parameter is set, further calls to uvc_queue_buffer will
+ * fail with -ENODEV.
+ *
* This function acquires the irq spinlock and can be called from interrupt
* context.
*/
-void uvc_queue_cancel(struct uvc_video_queue *queue)
+void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect)
{
struct uvc_buffer *buf;
unsigned long flags;
@@ -437,6 +445,14 @@
buf->state = UVC_BUF_STATE_ERROR;
wake_up(&buf->wait);
}
+ /* This must be protected by the irqlock spinlock to avoid race
+ * conditions between uvc_queue_buffer and the disconnection event that
+ * could result in an interruptible wait in uvc_dequeue_buffer. Do not
+ * blindly replace this logic by checking for the UVC_DEV_DISCONNECTED
+ * state outside the queue code.
+ */
+ if (disconnect)
+ queue->flags |= UVC_QUEUE_DISCONNECTED;
spin_unlock_irqrestore(&queue->irqlock, flags);
}
@@ -446,7 +462,8 @@
struct uvc_buffer *nextbuf;
unsigned long flags;
- if (queue->drop_incomplete && buf->buf.length != buf->buf.bytesused) {
+ if ((queue->flags & UVC_QUEUE_DROP_INCOMPLETE) &&
+ buf->buf.length != buf->buf.bytesused) {
buf->state = UVC_BUF_STATE_QUEUED;
buf->buf.bytesused = 0;
return buf;
Index: uvc_video.c
===================================================================
--- uvc_video.c (revision 211)
+++ uvc_video.c (working copy)
@@ -536,12 +536,12 @@
"completion handler.\n", urb->status);
case -ENOENT: /* usb_kill_urb() called. */
- if (queue->frozen)
+ if (video->frozen)
return;
case -ECONNRESET: /* usb_unlink_urb() called. */
case -ESHUTDOWN: /* The endpoint is being disabled. */
- uvc_queue_cancel(queue);
+ uvc_queue_cancel(queue, urb->status == -ESHUTDOWN);
return;
}
@@ -788,15 +788,15 @@
* Stop streaming without disabling the video queue.
*
* To let userspace applications resume without trouble, we must not touch the
- * video buffers in any way. We mark the queue as frozen to make sure the URB
+ * video buffers in any way. We mark the device as frozen to make sure the URB
* completion handler won't try to cancel the queue when we kill the URBs.
*/
int uvc_video_suspend(struct uvc_video_device *video)
{
- if (!video->queue.streaming)
+ if (!uvc_queue_streaming(&video->queue))
return 0;
- video->queue.frozen = 1;
+ video->frozen = 1;
uvc_uninit_video(video);
usb_set_interface(video->dev->udev, video->streaming->intfnum, 0);
return 0;
@@ -814,14 +814,14 @@
{
int ret;
- video->queue.frozen = 0;
+ video->frozen = 0;
if ((ret = uvc_set_video_ctrl(video, &video->streaming->ctrl, 0)) < 0) {
uvc_queue_enable(&video->queue, 0);
return ret;
}
- if (!video->queue.streaming)
+ if (!uvc_queue_streaming(&video->queue))
return 0;
if ((ret = uvc_init_video(video)) < 0)
Index: uvc_v4l2.c
===================================================================
--- uvc_v4l2.c (revision 212)
+++ uvc_v4l2.c (working copy)
@@ -246,7 +246,7 @@
if (fmt->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
return -EINVAL;
- if (video->queue.streaming)
+ if (uvc_queue_streaming(&video->queue))
return -EBUSY;
ret = uvc_v4l2_try_format(video, fmt, &probe, &format, &frame);
@@ -298,7 +298,7 @@
if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
return -EINVAL;
- if (video->queue.streaming)
+ if (uvc_queue_streaming(&video->queue))
return -EBUSY;
memcpy(&probe, &video->streaming->ctrl, sizeof probe);
@@ -843,9 +843,9 @@
if (ret < 0)
return ret;
- video->queue.drop_incomplete =
- video->streaming->cur_format->flags &
- UVC_FMT_FLAG_COMPRESSED ? 0 : 1;
+ if (!(video->streaming->cur_format->flags &
+ UVC_FMT_FLAG_COMPRESSED))
+ video->queue.flags |= UVC_QUEUE_DROP_INCOMPLETE;
rb->count = ret;
ret = 0;
Index: uvcvideo.h
===================================================================
--- uvcvideo.h (revision 211)
+++ uvcvideo.h (working copy)
@@ -551,11 +551,13 @@
enum uvc_buffer_state state;
};
+#define UVC_QUEUE_STREAMING (1 << 0)
+#define UVC_QUEUE_DISCONNECTED (1 << 1)
+#define UVC_QUEUE_DROP_INCOMPLETE (1 << 2)
+
struct uvc_video_queue {
void *mem;
- unsigned int streaming : 1,
- frozen : 1,
- drop_incomplete : 1;
+ unsigned int flags;
__u32 sequence;
unsigned int count;
@@ -572,6 +574,7 @@
struct uvc_device *dev;
struct video_device *vdev;
atomic_t active;
+ unsigned int frozen : 1;
struct list_head iterms;
struct uvc_entity *oterm;
@@ -713,11 +716,15 @@
extern int uvc_dequeue_buffer(struct uvc_video_queue *queue,
struct v4l2_buffer *v4l2_buf, int nonblocking);
extern int uvc_queue_enable(struct uvc_video_queue *queue, int enable);
-extern void uvc_queue_cancel(struct uvc_video_queue *queue);
+extern void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect);
extern struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
struct uvc_buffer *buf);
extern unsigned int uvc_queue_poll(struct uvc_video_queue *queue,
struct file *file, poll_table *wait);
+static inline int uvc_queue_streaming(struct uvc_video_queue *queue)
+{
+ return queue->flags & UVC_QUEUE_STREAMING;
+}
/* V4L2 interface */
extern struct file_operations uvc_fops;
_______________________________________________
Linux-uvc-devel mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/linux-uvc-devel
