[PATCH] v4l: Deadlock in videobuf-core for DQBUF waiting on QBUF

[PATCH] v4l: Deadlock in videobuf-core for DQBUF waiting on QBUF

[Brandon Philips]

# HG changeset patch
# User Brandon Philips <brandon@ifup.org>
# Date 1204098775 28800
# Node ID 54fa1a0d9c5bcdfcb2ba70be2fc68d51a1ab7b9c
# Parent  9756fb75295872570d1fa49b4bcf5d0967ecedaa
v4l: Deadlock in videobuf-core for DQBUF waiting on QBUF

Avoid a deadlock where DQBUF is holding the vb_lock while waiting on a QBUF
which also needs the vb_lock.  Reported by Hans Verkuil <hverkuil@xs4all.nl>.

Signed-off-by: Brandon Philips <bphilips@suse.de>

---
Pull from: http://ifup.org/hg/v4l-dvb

Should be merged for 2.6.25

diff --git a/linux/drivers/media/video/videobuf-core.c b/linux/drivers/media/video/videobuf-core.c
--- a/linux/drivers/media/video/videobuf-core.c
+++ b/linux/drivers/media/video/videobuf-core.c
@@ -606,7 +606,9 @@ int videobuf_dqbuf(struct videobuf_queue
 		goto done;
 	}
 	buf = list_entry(q->stream.next, struct videobuf_buffer, stream);
+	mutex_unlock(&q->vb_lock);
 	retval = videobuf_waiton(buf, nonblocking, 1);
+	mutex_lock(&q->vb_lock);
 	if (retval < 0) {
 		dprintk(1, "dqbuf: waiton returned %d\n", retval);
 		goto done;


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [v4l-dvb-maintainer] [PATCH] v4l: Deadlock in videobuf-core for DQBUF waiting on QBUF

[Trent Piepho]

On Tue, 26 Feb 2008, Brandon Philips wrote:
> v4l: Deadlock in videobuf-core for DQBUF waiting on QBUF
>
> Avoid a deadlock where DQBUF is holding the vb_lock while waiting on a QBUF
> which also needs the vb_lock.  Reported by Hans Verkuil <hverkuil@xs4all.nl>.
>
>  	buf = list_entry(q->stream.next, struct videobuf_buffer, stream);
> +	mutex_unlock(&q->vb_lock);
>  	retval = videobuf_waiton(buf, nonblocking, 1);
> +	mutex_lock(&q->vb_lock);

Does this create a race condition in videobuf_waiton?  It seems like the
stuff done there with buf would have races with any other thread trying to
access buf at the same time.

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] v4l: Deadlock in videobuf-core for DQBUF waiting on QBUF

[Brandon Philips]

On 09:08 Wed 27 Feb 2008, Jonathan Corbet wrote:
> Brandon Philips <brandon@ifup.org> wrote:
> 
> >  	buf = list_entry(q->stream.next, struct videobuf_buffer, stream);
> > +	mutex_unlock(&q->vb_lock);
> >  	retval = videobuf_waiton(buf, nonblocking, 1);
> > +	mutex_lock(&q->vb_lock);
> 
> Are you sure that this doesn't create a race where two threads could end
> up waiting on the same buf?  

You are right... I thought I had thought through this but a race can be
created with two threads doing DQBUF.

> Actually, almost anything could happen to buf by the time you've
> gotten the mutex back - it might not even exist anymore - but there
> are no checks for that.  It seems like a better fix might be to set
> nonblocking unconditionally to 1 for the videobuf_waiton() call, then
> start over from the beginning on a -EAGAIN return (if the caller has
> not requested nonblocking behavior).

Hrm, that is one solution.  I will think about it for a bit and submit a
new patch.

Thanks for catching this, I was being stupid.

Mauro: Please don't push this patch out.  Thanks.

Cheers,

	Brandon

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] v4l: Deadlock in videobuf-core for DQBUF waiting on QBUF

[Mauro Carvalho Chehab]

On Wed, 27 Feb 2008 18:56:51 -0800
Brandon Philips <brandon@ifup.org> wrote:

> On 09:08 Wed 27 Feb 2008, Jonathan Corbet wrote:
> > Brandon Philips <brandon@ifup.org> wrote:
> > 
> > >  	buf = list_entry(q->stream.next, struct videobuf_buffer, stream);
> > > +	mutex_unlock(&q->vb_lock);
> > >  	retval = videobuf_waiton(buf, nonblocking, 1);
> > > +	mutex_lock(&q->vb_lock);
> > 
> > Are you sure that this doesn't create a race where two threads could end
> > up waiting on the same buf?  
> 
> You are right... I thought I had thought through this but a race can be
> created with two threads doing DQBUF.
> 
> > Actually, almost anything could happen to buf by the time you've
> > gotten the mutex back - it might not even exist anymore - but there
> > are no checks for that.  It seems like a better fix might be to set
> > nonblocking unconditionally to 1 for the videobuf_waiton() call, then
> > start over from the beginning on a -EAGAIN return (if the caller has
> > not requested nonblocking behavior).
> 
> Hrm, that is one solution.  I will think about it for a bit and submit a
> new patch.
> 
> Thanks for catching this, I was being stupid.
> 
> Mauro: Please don't push this patch out.  Thanks.

The patch were already applied at the staging tree. I'll keep it there until we
have a definitive solution. After that, the better would be to fold the both
patches and send to 2.6.25-rc.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] v4l: Deadlock in videobuf-core for DQBUF waiting on QBUF

[Brandon Philips]

[-- Attachment #1: Type: text/plain, Size: 3213 bytes --]

On 06:34 Fri 29 Feb 2008, Mauro Carvalho Chehab wrote:
> On Wed, 27 Feb 2008 18:56:51 -0800
> Brandon Philips <brandon@ifup.org> wrote:
> 
> > On 09:08 Wed 27 Feb 2008, Jonathan Corbet wrote:
> > > Brandon Philips <brandon@ifup.org> wrote:
> > > 
> > > >  	buf = list_entry(q->stream.next, struct videobuf_buffer, stream);
> > > > +	mutex_unlock(&q->vb_lock);
> > > >  	retval = videobuf_waiton(buf, nonblocking, 1);
> > > > +	mutex_lock(&q->vb_lock);
> > > 
> > > Are you sure that this doesn't create a race where two threads could end
> > > up waiting on the same buf?  
> > 
> > You are right... I thought I had thought through this but a race can be
> > created with two threads doing DQBUF.
> > 
> > > Actually, almost anything could happen to buf by the time you've
> > > gotten the mutex back - it might not even exist anymore - but there
> > > are no checks for that.  It seems like a better fix might be to set
> > > nonblocking unconditionally to 1 for the videobuf_waiton() call, then
> > > start over from the beginning on a -EAGAIN return (if the caller has
> > > not requested nonblocking behavior).
> > 
> > Hrm, that is one solution.  I will think about it for a bit and submit a
> > new patch.
> > 
> > Thanks for catching this, I was being stupid.
> > 
> > Mauro: Please don't push this patch out.  Thanks.
> 
> The patch were already applied at the staging tree. I'll keep it there until we
> have a definitive solution. After that, the better would be to fold the both
> patches and send to 2.6.25-rc.

Quick update:

Sorry for the delay I was mentoring my robotics team since Thursday.

I wrote a test program (attached, super hacky) and I have found that the
current vivi can easily hang a kernel on a spinlock even before my
patch...

[  501.674961] vivi: 51946: Underrun, losed 1 frames. Current frame is
2. Will sleep for 5 jiffies
[  542.170534] BUG: spinlock lockup on CPU#0, vivi/17280, cc7ce448
[  542.173473] Pid: 17280, comm: vivi Tainted: P
2.6.25-rc2-default #25
[  542.181500]  [<c02251e1>] _raw_spin_lock+0xa6/0xbf
[  542.189520]  [<cee1ee73>] vivi_vid_timeout+0x18/0x285 [vivi]
[  542.197487]  [<c0332c4f>] _spin_lock_irq+0x21/0x2c
[  542.205477]  [<cee1ee5b>] vivi_vid_timeout+0x0/0x285 [vivi]
[  542.213483]  [<c0121b6e>] run_timer_softirq+0x128/0x180
[  542.221471]  [<cee1ee5b>] vivi_vid_timeout+0x0/0x285 [vivi]
[  542.229539]  [<c011ed81>] __do_softirq+0x3e/0x87
[  542.237489]  [<c011edff>] do_softirq+0x35/0x43
[  542.245479]  [<c011f0b5>] irq_exit+0x25/0x53
[  542.253473]  [<c0110486>] smp_apic_timer_interrupt+0x59/0x61
[  542.261480]  [<c0106918>] apic_timer_interrupt+0x28/0x30
[  542.269535]  [<c02251aa>] _raw_spin_lock+0x6f/0xbf
[  542.277508]  [<cee1e5ff>] vivi_thread+0x1c7/0x712 [vivi]
[  542.285630]  [<c011778f>] default_wake_function+0x0/0x5
[  542.293504]  [<cee1e438>] vivi_thread+0x0/0x712 [vivi]
[  542.301488]  [<c012a175>] kthread+0x36/0x5d
[  542.309476]  [<c012a13f>] kthread+0x0/0x5d
[  542.317483]  [<c0106ac7>] kernel_thread_helper+0x7/0x10
[  542.325511]  =======================

I will work on fixing this tomorrow before fixing the videobuf issue.  I
would like to be able to test my fix before submitting it this time.

Cheers,

	Brandon

[-- Attachment #2: v4l-thread.c --]
[-- Type: text/plain, Size: 14770 bytes --]

/*
 * V4L2 video capture example with pthread qbuf
 * Based on example from v4l2 spec
 *
 * brandon@ifup.org
 *
 * Compile: gcc v4l-thread.c -lpthread -o v4l-thread
 *
 */

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <assert.h>

#include <getopt.h>             /* getopt_long() */

#include <fcntl.h>              /* low-level i/o */
#include <unistd.h>
#include <errno.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <sys/time.h>
#include <sys/mman.h>
#include <sys/ioctl.h>

#include <asm/types.h>          /* for videodev2.h */

#include <linux/videodev2.h>

#define CLEAR(x) memset (&(x), 0, sizeof (x))

typedef enum {
        IO_METHOD_READ,
        IO_METHOD_MMAP,
} io_method;

struct buffer {
        void *                  start;
        size_t                  length;
};

static char *           dev_name        = NULL;
static io_method        io              = IO_METHOD_MMAP;
static int              fd              = -1;
struct buffer *         buffers         = NULL;
static unsigned int     n_buffers       = 0;


static void errno_exit(const char *s)
{
        fprintf (stderr, "%s error %d, %s\n",
                 s, errno, strerror (errno));

        exit (EXIT_FAILURE);
}

static int xioctl (int fd, int request, void *arg)
{
        int r;

        do r = ioctl (fd, request, arg);
        while (-1 == r && EINTR == errno);

        return r;
}

static void process_image  (const void *p)
{
        fputc ('.', stdout);
        fflush (stdout);
}

void *qbuf_thread(void *arg) {
	int retval;
        struct v4l2_buffer buf;
	int i;
	buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
	buf.memory = V4L2_MEMORY_MMAP;

        while (1) {
		sleep(5);
		for (i = 0; i < n_buffers; i++) {
			buf.index = i;
			retval = xioctl(fd, VIDIOC_QBUF, &buf);
			if (retval < 0)
				printf("QBUF %i %i %s\n", i, errno, strerror (errno));
			else
				printf("QBUF %i\n", i);
		}
        }
}


static int read_frame (void)
{
        struct v4l2_buffer buf;
        unsigned int i;

        switch (io) {
        case IO_METHOD_READ:
                if (-1 == read (fd, buffers[0].start, buffers[0].length)) {
                        switch (errno) {
                        case EAGAIN:
                                return 0;

                        case EIO:
				return 0;

                        default:
                                errno_exit ("read");
                        }
                }

                process_image (buffers[0].start);

                break;

        case IO_METHOD_MMAP:
                CLEAR (buf);

                buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
                buf.memory = V4L2_MEMORY_MMAP;

                if (-1 == xioctl (fd, VIDIOC_DQBUF, &buf)) {
                        switch (errno) {
			case 16:
                        case EAGAIN:
                        case EINVAL:
                                return 0;

                        case EIO:
                                /* Could ignore EIO, see spec. */

                                /* fall through */

                        default:
                                errno_exit ("VIDIOC_DQBUF");
                        }
                }

                assert (buf.index < n_buffers);
		printf("DQBUF %i\n", buf.index);
                process_image (buffers[buf.index].start);

                break;
        }

        return 1;
}

static void mainloop (void)
{
        unsigned int count;

        count = 100;

        while (count-- > 0) {
                for (;;) {
                        fd_set fds;
                        struct timeval tv;
                        int r;

                        FD_ZERO (&fds);
                        FD_SET (fd, &fds);

                        /* Timeout. */
                        tv.tv_sec = 2;
                        tv.tv_usec = 0;

                        r = select (fd + 1, &fds, NULL, NULL, &tv);

                        if (-1 == r) {
                                if (EINTR == errno)
                                        continue;

                                errno_exit ("select");
                        }

                        if (0 == r) {
                                fprintf (stderr, "select timeout\n");
                                exit (EXIT_FAILURE);
                        }

                        if (read_frame ())
                                break;

                        /* EAGAIN - continue select loop. */
                }
        }
}

static void stop_capturing (void)
{
        enum v4l2_buf_type type;

        switch (io) {
        case IO_METHOD_READ:
                /* Nothing to do. */
                break;

        case IO_METHOD_MMAP:
                type = V4L2_BUF_TYPE_VIDEO_CAPTURE;

                if (-1 == xioctl (fd, VIDIOC_STREAMOFF, &type))
                        errno_exit ("VIDIOC_STREAMOFF");

                break;
        }
}

static void start_capturing (void)
{
        unsigned int i;
        enum v4l2_buf_type type;
	int retval;

        switch (io) {
        case IO_METHOD_READ:
                /* Nothing to do. */
                break;

        case IO_METHOD_MMAP:
                for (i = 0; i < n_buffers; ++i) {
                        struct v4l2_buffer buf;

                        CLEAR (buf);

                        buf.type        = V4L2_BUF_TYPE_VIDEO_CAPTURE;
                        buf.memory      = V4L2_MEMORY_MMAP;
                        buf.index       = i;

                        if (-1 == xioctl (fd, VIDIOC_QBUF, &buf))
                                errno_exit ("VIDIOC_QBUF");
                }

                type = V4L2_BUF_TYPE_VIDEO_CAPTURE;

                if (-1 == xioctl (fd, VIDIOC_STREAMON, &type))
                        errno_exit ("VIDIOC_STREAMON");

                break;
        }

	pthread_t q_thread;
	retval = pthread_create (
		&q_thread, NULL, qbuf_thread, NULL);
	if (retval != 0)
		errno_exit("Create interrupt thread");

}

static void uninit_device(void)
{
        unsigned int i;

        switch (io) {
        case IO_METHOD_READ:
                free (buffers[0].start);
                break;

        case IO_METHOD_MMAP:
                for (i = 0; i < n_buffers; ++i)
                        if (-1 == munmap (buffers[i].start, buffers[i].length))
                                errno_exit ("munmap");
                break;

        }

        free (buffers);
}

static void init_read(unsigned int buffer_size)
{
        buffers = calloc (1, sizeof (*buffers));

        if (!buffers) {
                fprintf (stderr, "Out of memory\n");
                exit (EXIT_FAILURE);
        }

        buffers[0].length = buffer_size;
        buffers[0].start = malloc (buffer_size);

        if (!buffers[0].start) {
                fprintf (stderr, "Out of memory\n");
                exit (EXIT_FAILURE);
        }
}

static void init_mmap(void)
{
        struct v4l2_requestbuffers req;

        CLEAR (req);

        req.count               = 4;
        req.type                = V4L2_BUF_TYPE_VIDEO_CAPTURE;
        req.memory              = V4L2_MEMORY_MMAP;

        if (-1 == xioctl (fd, VIDIOC_REQBUFS, &req)) {
                if (EINVAL == errno) {
                        fprintf (stderr, "%s does not support "
                                 "memory mapping\n", dev_name);
                        exit (EXIT_FAILURE);
                } else {
                        errno_exit ("VIDIOC_REQBUFS");
                }
        }

        if (req.count < 2) {
                fprintf (stderr, "Insufficient buffer memory on %s\n",
                         dev_name);
                exit (EXIT_FAILURE);
        }

        buffers = calloc (req.count, sizeof (*buffers));

        if (!buffers) {
                fprintf (stderr, "Out of memory\n");
                exit (EXIT_FAILURE);
        }

        for (n_buffers = 0; n_buffers < req.count; ++n_buffers) {
                struct v4l2_buffer buf;

                CLEAR (buf);

                buf.type        = V4L2_BUF_TYPE_VIDEO_CAPTURE;
                buf.memory      = V4L2_MEMORY_MMAP;
                buf.index       = n_buffers;

                if (-1 == xioctl (fd, VIDIOC_QUERYBUF, &buf))
                        errno_exit ("VIDIOC_QUERYBUF");

                buffers[n_buffers].length = buf.length;
                buffers[n_buffers].start =
                        mmap (NULL /* start anywhere */,
                              buf.length,
                              PROT_READ | PROT_WRITE /* required */,
                              MAP_SHARED /* recommended */,
                              fd, buf.m.offset);

                if (MAP_FAILED == buffers[n_buffers].start)
                        errno_exit ("mmap");
        }
}

static void init_device(void)
{
        struct v4l2_capability cap;
        struct v4l2_cropcap cropcap;
        struct v4l2_crop crop;
        struct v4l2_format fmt;
        unsigned int min;

        if (-1 == xioctl (fd, VIDIOC_QUERYCAP, &cap)) {
                if (EINVAL == errno) {
                        fprintf (stderr, "%s is no V4L2 device\n",
                                 dev_name);
                        exit (EXIT_FAILURE);
                } else {
                        errno_exit ("VIDIOC_QUERYCAP");
                }
        }

        if (!(cap.capabilities & V4L2_CAP_VIDEO_CAPTURE)) {
                fprintf (stderr, "%s is no video capture device\n",
                         dev_name);
                exit (EXIT_FAILURE);
        }

        switch (io) {
        case IO_METHOD_READ:
                if (!(cap.capabilities & V4L2_CAP_READWRITE)) {
                        fprintf (stderr, "%s does not support read i/o\n",
                                 dev_name);
                        exit (EXIT_FAILURE);
                }

                break;

        case IO_METHOD_MMAP:
                if (!(cap.capabilities & V4L2_CAP_STREAMING)) {
                        fprintf (stderr, "%s does not support streaming i/o\n",
                                 dev_name);
                        exit (EXIT_FAILURE);
                }

                break;
        }

        /* Select video input, video standard and tune here. */

        cropcap.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;

        if (-1 == xioctl (fd, VIDIOC_CROPCAP, &cropcap)) {
                /* Errors ignored. */
        }

        crop.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
        crop.c = cropcap.defrect; /* reset to default */

        if (-1 == xioctl (fd, VIDIOC_S_CROP, &crop)) {
                switch (errno) {
                case EINVAL:
                        /* Cropping not supported. */
                        break;
                default:
                        /* Errors ignored. */
                        break;
                }
        }

        CLEAR (fmt);

        fmt.type                = V4L2_BUF_TYPE_VIDEO_CAPTURE;
        fmt.fmt.pix.width       = 640;
        fmt.fmt.pix.height      = 480;
        fmt.fmt.pix.pixelformat = V4L2_PIX_FMT_YUYV;
        fmt.fmt.pix.field       = V4L2_FIELD_INTERLACED;

        if (-1 == xioctl (fd, VIDIOC_S_FMT, &fmt))
                errno_exit ("VIDIOC_S_FMT");

        /* Note VIDIOC_S_FMT may change width and height. */

        /* Buggy driver paranoia. */
        min = fmt.fmt.pix.width * 2;
        if (fmt.fmt.pix.bytesperline < min)
                fmt.fmt.pix.bytesperline = min;
        min = fmt.fmt.pix.bytesperline * fmt.fmt.pix.height;
        if (fmt.fmt.pix.sizeimage < min)
                fmt.fmt.pix.sizeimage = min;

        switch (io) {
        case IO_METHOD_READ:
                init_read (fmt.fmt.pix.sizeimage);
                break;

        case IO_METHOD_MMAP:
                init_mmap ();
                break;

        }
}

static void close_device(void)
{
        if (-1 == close (fd))
                errno_exit ("close");

        fd = -1;
}

static void open_device(void)
{
        struct stat st;

        if (-1 == stat (dev_name, &st)) {
                fprintf (stderr, "Cannot identify '%s': %d, %s\n",
                         dev_name, errno, strerror (errno));
                exit (EXIT_FAILURE);
        }

        if (!S_ISCHR (st.st_mode)) {
                fprintf (stderr, "%s is no device\n", dev_name);
                exit (EXIT_FAILURE);
        }

        fd = open (dev_name, O_RDWR /* required */ | O_NONBLOCK, 0);

        if (-1 == fd) {
                fprintf (stderr, "Cannot open '%s': %d, %s\n",
                         dev_name, errno, strerror (errno));
                exit (EXIT_FAILURE);
        }
}

static void usage(FILE *fp, int argc, char **argv)
{
        fprintf (fp,
                 "Usage: %s [options]\n\n"
                 "Options:\n"
                 "-d | --device name   Video device name [/dev/video]\n"
                 "-h | --help          Print this message\n"
                 "-m | --mmap          Use memory mapped buffers\n"
                 "-r | --read          Use read() calls\n"
                 "-u | --userp         Use application allocated buffers\n"
                 "",
                 argv[0]);
}

static const char short_options [] = "d:hmru";

static const struct option
long_options [] = {
	{ "device",	required_argument,	NULL,	'd' },
	{ "help",	no_argument,		NULL,	'h' },
	{ "mmap",	no_argument,		NULL,	'm' }, 
	{ "read",	no_argument,		NULL,	'r' }, 
	{ "userp",	no_argument,		NULL,	'u' },
	{ 0, 0, 0, 0 }
};

int main (int argc, char **argv)
{
        dev_name = "/dev/video";

        for (;;) {
                int index;
                int c;

                c = getopt_long (argc, argv,
                                 short_options, long_options,
                                 &index);

                if (-1 == c)
                        break;

                switch (c) {
                case 0: /* getopt_long() flag */
                        break;

                case 'd':
                        dev_name = optarg;
                        break;

                case 'h':
                        usage (stdout, argc, argv);
                        exit (EXIT_SUCCESS);

                case 'm':
                        io = IO_METHOD_MMAP;
                        break;

                case 'r':
                        io = IO_METHOD_READ;
                        break;

                default:
                        usage (stderr, argc, argv);
                        exit (EXIT_FAILURE);
                }
        }

        open_device ();
        init_device ();
        start_capturing ();
        mainloop ();
        stop_capturing ();
        uninit_device ();
        close_device ();
        exit (EXIT_SUCCESS);

        return 0;
}


[-- Attachment #3: Type: text/plain, Size: 164 bytes --]

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] v4l: Deadlock in videobuf-core for DQBUF waiting on QBUF

[Mauro Carvalho Chehab]

On Mon, 3 Mar 2008 00:13:05 -0800
Brandon Philips <brandon@ifup.org> wrote:

> On 06:34 Fri 29 Feb 2008, Mauro Carvalho Chehab wrote:
> > On Wed, 27 Feb 2008 18:56:51 -0800
> > Brandon Philips <brandon@ifup.org> wrote:
> > 
> > > On 09:08 Wed 27 Feb 2008, Jonathan Corbet wrote:
> > > > Brandon Philips <brandon@ifup.org> wrote:
> > > > 
> > > > >  	buf = list_entry(q->stream.next, struct videobuf_buffer, stream);
> > > > > +	mutex_unlock(&q->vb_lock);
> > > > >  	retval = videobuf_waiton(buf, nonblocking, 1);
> > > > > +	mutex_lock(&q->vb_lock);
> > > > 
> > > > Are you sure that this doesn't create a race where two threads could end
> > > > up waiting on the same buf?  
> > > 
> > > You are right... I thought I had thought through this but a race can be
> > > created with two threads doing DQBUF.
> > > 
> > > > Actually, almost anything could happen to buf by the time you've
> > > > gotten the mutex back - it might not even exist anymore - but there
> > > > are no checks for that.  It seems like a better fix might be to set
> > > > nonblocking unconditionally to 1 for the videobuf_waiton() call, then
> > > > start over from the beginning on a -EAGAIN return (if the caller has
> > > > not requested nonblocking behavior).
> > > 
> > > Hrm, that is one solution.  I will think about it for a bit and submit a
> > > new patch.
> > > 
> > > Thanks for catching this, I was being stupid.
> > > 
> > > Mauro: Please don't push this patch out.  Thanks.
> > 
> > The patch were already applied at the staging tree. I'll keep it there until we
> > have a definitive solution. After that, the better would be to fold the both
> > patches and send to 2.6.25-rc.
> 
> Quick update:
> 
> Sorry for the delay I was mentoring my robotics team since Thursday.
> 
> I wrote a test program (attached, super hacky) and I have found that the
> current vivi can easily hang a kernel on a spinlock even before my
> patch...

Argh!
> 
> I will work on fixing this tomorrow before fixing the videobuf issue.  I
> would like to be able to test my fix before submitting it this time.
This would be nice.

About your testing program, it would be a good idea to add it at v4l2-apps/test
dir. The original non-threaded code from V4L2 specs is also there.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] v4l: Deadlock in videobuf-core for DQBUF waiting on QBUF

[Brandon Philips]

On 10:45 Tue 04 Mar 2008, Mauro Carvalho Chehab wrote:
> On Mon, 3 Mar 2008 00:13:05 -0800 Brandon Philips <brandon@ifup.org>
> wrote:
>
> About your testing program, it would be a good idea to add it at
> v4l2-apps/test dir. The original non-threaded code from V4L2 specs is
> also there.

I will add some polish and just add in the pthread stuff to the original
program.  It is super hacky right now.

Cheers,

	Brandon

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list