The UVC driver processes frames in URBs. A frame usually is divided into multiple URBs. The URB handler parses the header and the metadata, but leaves the expensive memcpy to a workqueue. At any given moment, while streaming we will have: - the current frame with (0-N) async memcpys waiting to happen. The current frame is in queue->irqqueue. It has a refcnt of 1 + N memcpys - Previous frames with (N) async memcpys waiting to happen. They are not in queue->irqqueue. They have a refcnt of N memcpys With the current behaviour, when a URB completes with an error we flush the queue->irqqueue irrespective of whether the buffers have any pending memcpys. There can be situations where the memcpy occurs after we have flushed the frames and they are returned to the user. Instead, we should exploit the refcnt mechanism to return the frames only after the memcpys have been completed. Fixes: 01e90464e42e ("media: uvcvideo: queue: Support asynchronous buffer handling") Cc: stable@vger.kernel.org Signed-off-by: Hongbing Hu Reviewed-by: Ricardo Ribalda --- v4: - Reworded the commit message as suggested during review. - Documented in uvc_queue_buffer_complete() that it may be called with queue->irqlock held when buf->cancelled is true. drivers/media/usb/uvc/uvc_queue.c | 24 ++++++++++++++++++++++-- drivers/media/usb/uvc/uvcvideo.h | 1 + 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c index 3c002c8f44..c491735cfe 100644 --- a/drivers/media/usb/uvc/uvc_queue.c +++ b/drivers/media/usb/uvc/uvc_queue.c @@ -122,6 +122,7 @@ static int uvc_buffer_prepare(struct vb2_buffer *vb) buf->state = UVC_BUF_STATE_QUEUED; buf->error = 0; + buf->cancelled = false; buf->mem = vb2_plane_vaddr(vb, 0); buf->length = vb2_plane_size(vb, 0); if (vb->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) @@ -289,10 +290,17 @@ int uvc_queue_init(struct uvc_streaming *stream, struct uvc_video_queue *queue, */ void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect) { + struct uvc_buffer *buf; unsigned long flags; spin_lock_irqsave(&queue->irqlock, flags); - __uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR); + while (!list_empty(&queue->irqqueue)) { + buf = list_first_entry(&queue->irqqueue, struct uvc_buffer, queue); + list_del(&buf->queue); + buf->error = 1; + buf->cancelled = true; + uvc_queue_buffer_release(buf); + } /* * This must be protected by the irqlock spinlock to avoid race * conditions between uvc_buffer_queue and the disconnection event that @@ -350,13 +358,25 @@ static void uvc_queue_buffer_requeue(struct uvc_video_queue *queue, uvc_buffer_queue(&buf->buf.vb2_buf); } +/* + * kref release callback for uvc_buffer. + * + * If buf->cancelled is true, this function may be called with + * queue->irqlock held, as uvc_queue_cancel() releases the queue's + * reference on cancelled buffers while holding the lock. + */ static void uvc_queue_buffer_complete(struct kref *ref) { struct uvc_buffer *buf = container_of(ref, struct uvc_buffer, ref); struct vb2_buffer *vb = &buf->buf.vb2_buf; struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue); - if (buf->error && !uvc_no_drop_param) { + /* + * Buffers cancelled from uvc_queue_cancel() are forced to complete as + * errors. They must not be requeued by the corrupted-frame policy even + * when buf->error is set. + */ + if (!buf->cancelled && buf->error && !uvc_no_drop_param) { uvc_queue_buffer_requeue(queue, buf); return; } diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index b6bcee4a22..1b611715fa 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -317,6 +317,7 @@ struct uvc_buffer { enum uvc_buffer_state state; unsigned int error; + bool cancelled; void *mem; unsigned int length; -- 2.34.1