V4L/DVB (7565): em28xx: fix buffer underrun handling

This patch fixes three related issues and a fourth trivial one:

- Use buffers even if no-one's currently waiting for them (fixes
  underrun issues);

- Don't return incomplete/mangled frames at the start of streaming and
  in the case of buffer underruns;

- Fix an issue which could cause the driver to write to a buffer that's
  been freed after videobuf_queue_cancel is called (exposed by the
  previous two fixes - for some reason, ignoring buffers that weren't
  being waited on worked around the issue);

- Fix a bug which could cause only one field to be filled in the first
  buffer (or first few buffers) after streaming is started.

Signed-off-by: Aidan Thornton <makosoft@googlemail.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@infradead.org>

diff --git a/drivers/media/video/em28xx/em28xx-video.c b/drivers/media/video/em28xx/em28xx-video.c
index d5728c2..f8bbf39 100644 (file)
--- a/drivers/media/video/em28xx/em28xx-video.c
+++ b/drivers/media/video/em28xx/em28xx-video.c
@@ -266,7 +266,7 @@ static inline void print_err_status(struct em28xx *dev,
 /*
  * video-buf generic routine to get the next available buffer
  */
-static inline int get_next_buf(struct em28xx_dmaqueue *dma_q,
+static inline void get_next_buf(struct em28xx_dmaqueue *dma_q,
                                          struct em28xx_buffer **buf)
 {
        struct em28xx *dev = container_of(dma_q, struct em28xx, vidq);
@@ -274,39 +274,21 @@ static inline int get_next_buf(struct em28xx_dmaqueue *dma_q,
 
        if (list_empty(&dma_q->active)) {
                em28xx_isocdbg("No active queue to serve\n");
-               dev->isoc_ctl.buf = NULL;
-               return 0;
-       }
-
-       /* Check if the last buffer were fully filled */
-       *buf = dev->isoc_ctl.buf;
-
-       /* Nobody is waiting on this buffer - discards */
-       if (*buf && !waitqueue_active(&(*buf)->vb.done)) {
                dev->isoc_ctl.buf = NULL;
                *buf = NULL;
+               return;
        }
 
-       /* Returns the last buffer, to be filled with remaining data */
-       if (*buf)
-               return 1;
-
        /* Get the next buffer */
        *buf = list_entry(dma_q->active.next, struct em28xx_buffer, vb.queue);
 
-       /* Nobody is waiting on the next buffer. returns */
-       if (!*buf || !waitqueue_active(&(*buf)->vb.done)) {
-               em28xx_isocdbg("No active queue to serve\n");
-               return 0;
-       }
-
        /* Cleans up buffer - Usefull for testing for frame/URB loss */
        outp = videobuf_to_vmalloc(&(*buf)->vb);
        memset(outp, 0, (*buf)->vb.size);
 
        dev->isoc_ctl.buf = *buf;
 
-       return 1;
+       return;
 }
 
 /*
@@ -317,7 +299,7 @@ static inline int em28xx_isoc_copy(struct urb *urb)
        struct em28xx_buffer    *buf;
        struct em28xx_dmaqueue  *dma_q = urb->context;
        struct em28xx *dev = container_of(dma_q, struct em28xx, vidq);
-       unsigned char *outp;
+       unsigned char *outp = NULL;
        int i, len = 0, rc = 1;
        unsigned char *p;
 
@@ -333,11 +315,9 @@ static inline int em28xx_isoc_copy(struct urb *urb)
                        return 0;
        }
 
-       rc = get_next_buf(dma_q, &buf);
-       if (rc <= 0)
-               return rc;
-
-       outp = videobuf_to_vmalloc(&buf->vb);
+       buf = dev->isoc_ctl.buf;
+       if (buf != NULL)
+               outp = videobuf_to_vmalloc(&buf->vb);
 
        for (i = 0; i < urb->number_of_packets; i++) {
                int status = urb->iso_frame_desc[i].status;
@@ -374,24 +354,27 @@ static inline int em28xx_isoc_copy(struct urb *urb)
                        em28xx_isocdbg("Video frame %d, length=%i, %s\n", p[2],
                                       len, (p[2] & 1)? "odd" : "even");
 
-                       if (p[2] & 1)
-                               buf->top_field = 0;
-                       else
-                               buf->top_field = 1;
-
-//                     if (dev->isoc_ctl.last_field && !buf->top_field) {
-                       if (dev->isoc_ctl.last_field != buf->top_field) {
-                               buffer_filled(dev, dma_q, buf);
-                               rc = get_next_buf(dma_q, &buf);
-                               if (rc <= 0)
-                                       return rc;
-                               outp = videobuf_to_vmalloc(&buf->vb);
+                       if (!(p[2] & 1)) {
+                               if (buf != NULL)
+                                       buffer_filled(dev, dma_q, buf);
+                               get_next_buf(dma_q, &buf);
+                               if (buf == NULL)
+                                       outp = NULL;
+                               else
+                                       outp = videobuf_to_vmalloc(&buf->vb);
+                       }
+
+                       if (buf != NULL) {
+                               if (p[2] & 1)
+                                       buf->top_field = 0;
+                               else
+                                       buf->top_field = 1;
                        }
-                       dev->isoc_ctl.last_field =  buf->top_field;
 
                        dma_q->pos = 0;
                }
-               em28xx_copy_video(dev, dma_q, buf, p, outp, len);
+               if (buf != NULL)
+                       em28xx_copy_video(dev, dma_q, buf, p, outp, len);
        }
        return rc;
 }
@@ -597,12 +580,29 @@ buffer_setup(struct videobuf_queue *vq, unsigned int *count, unsigned int *size)
        return 0;
 }
 
+/* This is called *without* dev->slock held; please keep it that way */
 static void free_buffer(struct videobuf_queue *vq, struct em28xx_buffer *buf)
 {
+       struct em28xx_fh     *fh  = vq->priv_data;
+       struct em28xx        *dev = fh->dev;
+       unsigned long flags = 0;
        if (in_interrupt())
                BUG();
 
-       videobuf_waiton(&buf->vb, 0, 0);
+       /* We used to wait for the buffer to finish here, but this didn't work
+          because, as we were keeping the state as VIDEOBUF_QUEUED,
+          videobuf_queue_cancel marked it as finished for us.
+          (Also, it could wedge forever if the hardware was misconfigured.)
+
+          This should be safe; by the time we get here, the buffer isn't
+          queued anymore. If we ever start marking the buffers as
+          VIDEOBUF_ACTIVE, it won't be, though.
+       */
+       spin_lock_irqsave(&dev->slock, flags);
+       if (dev->isoc_ctl.buf == buf)
+               dev->isoc_ctl.buf = NULL;
+       spin_unlock_irqrestore(&dev->slock, flags);
+
        videobuf_vmalloc_free(&buf->vb);
        buf->vb.state = VIDEOBUF_NEEDS_INIT;
 }

diff --git a/drivers/media/video/em28xx/em28xx.h b/drivers/media/video/em28xx/em28xx.h
index 993c1ed..6d62357 100644 (file)
--- a/drivers/media/video/em28xx/em28xx.h
+++ b/drivers/media/video/em28xx/em28xx.h
@@ -114,9 +114,6 @@ struct em28xx_usb_isoc_ctl {
                /* Stores already requested buffers */
        struct em28xx_buffer            *buf;
 
-               /* Store last filled frame */
-       int                             last_field;
-
                /* Stores the number of received fields */
        int                             nfields;
 };