Skip to contentSkip to conversation

2.6.24-rc5 "videobuf_read_start" [drivers/media/video/videobuf-dvb.ko] undefined!

PATCH In review Shane 6 participants 16 messages Dec 12, 2007 – Dec 14, 2007 Media (V4L/DVB)

lore
Share & tools
lore

Shane

author
#0 lore raw

In 2.6.24-rc5+, I hit this problem with videobuf_read_start not being exported. Patch attached, only compile tested.

  CHK     include/linux/version.h
  CHK     include/linux/utsrelease.h
  CALL    scripts/checksyscalls.sh
  CHK     include/linux/compile.h
  CC [M]  drivers/media/video/videobuf-core.o

Building modules, stage 2.
Kernel: arch/x86/boot/bzImage is ready (#1)
MODPOST 202 modules
ERROR: "videobuf_read_start" [drivers/media/video/videobuf-dvb.ko] undefined! make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

Shane

drivers/media/video/videobuf-core.c +1 -0
index de2f56b..44fa76b 100644
@@ -1058,6 +1058,7 @@ EXPORT_SYMBOL_GPL(videobuf_dqbuf);
 EXPORT_SYMBOL_GPL(videobuf_streamon);
 EXPORT_SYMBOL_GPL(videobuf_streamoff);
 
+EXPORT_SYMBOL_GPL(videobuf_read_start);
 EXPORT_SYMBOL_GPL(videobuf_read_stop);
 EXPORT_SYMBOL_GPL(videobuf_stop);
 EXPORT_SYMBOL_GPL(videobuf_read_stream);
#1 lore raw

Patch looks good, it seems a merge conflict of
commit 19bc5133dae9562e8824ef101464061f9854c1d8
was resolved the wrong way.

@Mauro:
Any objections against a later path that changes the exports to the general "immediately after the function" convention?
It would have avoided at least two such bugs in this file alone since 2.6.23...

cu
Adrian

Quoted 29 lines · Shane

In 2.6.24-rc5+, I hit this problem with videobuf_read_start not being exported. Patch attached, only compile tested.

  CHK     include/linux/version.h
  CHK     include/linux/utsrelease.h
  CALL    scripts/checksyscalls.sh
  CHK     include/linux/compile.h
  CC [M]  drivers/media/video/videobuf-core.o

Building modules, stage 2.
Kernel: arch/x86/boot/bzImage is ready (#1)
MODPOST 202 modules
ERROR: "videobuf_read_start" [drivers/media/video/videobuf-dvb.ko] undefined! make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

Shane

drivers/media/video/videobuf-core.c +1 -0
index de2f56b..44fa76b 100644
@@ -1058,6 +1058,7 @@ EXPORT_SYMBOL_GPL(videobuf_dqbuf);
 EXPORT_SYMBOL_GPL(videobuf_streamon);
 EXPORT_SYMBOL_GPL(videobuf_streamoff);
 
+EXPORT_SYMBOL_GPL(videobuf_read_start);
 EXPORT_SYMBOL_GPL(videobuf_read_stop);
 EXPORT_SYMBOL_GPL(videobuf_stop);
 EXPORT_SYMBOL_GPL(videobuf_read_stream);
#2 lore raw
Quoted 2 lines

Any objections against a later path that changes the exports to the general "immediately after the function" convention?

No objections. Please generate against "devel" branch on my -git, since I did a patch fixing most CodingStyle issues reported by checkpatch.pl.

Several files under /media still uses the old convention of having such things at the end of the file.

Quoted 2 lines

It would have avoided at least two such bugs in this file alone since 2.6.23...

I'm afraid that this wouldn't avoid this bug, however.

The removal of the EXPORT_SYMBOL_GPL seemed to be the intention of Brandon, since he renamed the function, removing the locks. I think he didn't noticed that videobuf_dvb were using videobuf_read_start. The patch I've just send fixes it properly.

Btw, Shane patch reveals a small trouble with EXPORT_SYMBOL_GPL: adding the tag for a non-existing function didn't rise any error. IMO, it should generate some compilation error, if you try to export a symbol that doesn't exist at the file that is being compiled.

Cheers,
Mauro

#3 lore raw
Quoted 7 lines · Mauro Carvalho Chehab

...
I'm afraid that this wouldn't avoid this bug, however.

The removal of the EXPORT_SYMBOL_GPL seemed to be the intention of Brandon, since he renamed the function, removing the locks. I think he didn't noticed that videobuf_dvb were using videobuf_read_start. The patch I've just send fixes it properly.

At least in the commit in Linus' tree, videobuf_read_start() stayed nearly unchanged.

Quoted 2 lines

Btw, Shane patch reveals a small trouble with EXPORT_SYMBOL_GPL: adding the tag for a non-existing function didn't rise any error.

See above.

Quoted 3 lines

IMO, it
should generate some compilation error, if you try to export a symbol that doesn't exist at the file that is being compiled.

It does give you a compile error when there's not at least a prototype for the stuff to be exported, and a link error if there's no variable/function.

Quoted 2 lines

Cheers,
Mauro

cu
Adrian

Signature · 4 lines
       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed
#4 lore raw

Em Qua, 2007-12-12 às 13:03 +0100, Adrian Bunk escreveu:

Quoted 11 lines · Mauro Carvalho Chehab
Quoted 7 lines

...
I'm afraid that this wouldn't avoid this bug, however.

The removal of the EXPORT_SYMBOL_GPL seemed to be the intention of Brandon, since he renamed the function, removing the locks. I think he didn't noticed that videobuf_dvb were using videobuf_read_start. The patch I've just send fixes it properly.

At least in the commit in Linus' tree, videobuf_read_start() stayed nearly unchanged.

Hmm... I should have reviewed the patch instead of trusting on my memory. Yes, you're right. Brandon didn't rename this function.

What happened is that changeset 19bc5133dae9562e8824ef101464061f9854c1d8 fixed some bad locks.

After this changeset, videobuf_read_stream() holds q->lock and calls videobuf_read_start(). To avoid waiting forever for the lock to be released, he removed the mutex from videobuf_read_start with this line [1]:

- err = videobuf_mmap_setup(q, count, size, V4L2_MEMORY_USERPTR);
+ err = __videobuf_mmap_setup(q, count, size, V4L2_MEMORY_USERPTR);

So, after the patch, videobuf_read_start() can't be safely called. So, just adding EXPORT_SYMBOL_GPL() breaks videobuf-dvb at runtime.

The proper solution is provided by this changeset:
http://git.kernel.org/?p=linux/kernel/git/mchehab/v4l-dvb.git;a=commitdiff;h=19fb1457990b6b7e15586ec7331541a184233acc

This renames the old videobuf_read_start() to __videobuf_read_start() and adds a newer one to be called externally, that holds the lock, before calling __videobuf_mmap_setup().

[1] After his changeset, all functions with __foo() don't touch on q->lock.

Signature · 2 lines
Cheers,
Mauro
author
#6 lore raw
Quoted 6 lines · Shane

On Dec 12, 2007 9:21 AM, Mauro Carvalho Chehab <[email protected]> wrote:
...

Quoted 2 lines

The proper solution is provided by this changeset:
http://git.kernel.org/?p=linux/kernel/git/mchehab/v4l-dvb.git;a=commitdiff;h=19fb1457990b6b7e15586ec7331541a184233acc

I applied this and it seems fine with a bttv card.

Ugh, after further testing with a bttv card it seems this is not fine.

vbi doesn't work anymore and my application gets stuck in a Zombie, unkillable, have to reboot state :(

mythtv 3683 1 -3 2.4 0.0 0 0 ? Z<l 13:40:35
00:00:06 [mythbackend] <defunct>

Reverting Mauro's patch above does fix the problem.

Shane

#7 lore raw

PATCH 1/1 videobuf-core.c locking fixes

Quoted 10 lines · Shane
Quoted 6 lines · Shane

On Dec 12, 2007 9:21 AM, Mauro Carvalho Chehab <[email protected]> wrote:
...

Quoted 2 lines

The proper solution is provided by this changeset:
http://git.kernel.org/?p=linux/kernel/git/mchehab/v4l-dvb.git;a=commitdiff;h=19fb1457990b6b7e15586ec7331541a184233acc

I applied this and it seems fine with a bttv card.

Ugh, after further testing with a bttv card it seems this is not fine.

Quoted 7 lines

vbi doesn't work anymore and my application gets stuck in a Zombie, unkillable, have to reboot state :(

mythtv 3683 1 -3 2.4 0.0 0 0 ? Z<l 13:40:35
00:00:06 [mythbackend] <defunct>

Reverting Mauro's patch above does fix the problem.

Thanks for testing, does the patch below fix it?

Quoted 1 line

Shane

cu
Adrian

<-- snip -->

After commit 19fb1457990b6b7e15586ec7331541a184233acc the callers in videobuf-core.c that already hold the lock must call __videobuf_read_start() instead of videobuf_read_start().

Signed-off-by Adrian Bunk <[email protected]>

---

 drivers/media/video/videobuf-core.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

e1f8b4a49d86746f699919531c17fd154787e308

drivers/media/video/videobuf-core.c +2 -2
index 81f77d2..c8a5cb5 100644
@@ -909,7 +909,7 @@ ssize_t videobuf_read_stream(struct videobuf_queue *q,
 	if (q->streaming)
 		goto done;
 	if (!q->reading) {
-		retval = videobuf_read_start(q);
+		retval = __videobuf_read_start(q);
 		if (retval < 0)
 			goto done;
 	}

@@ -982,7 +982,7 @@ unsigned int videobuf_poll_stream(struct file *file,
 					 struct videobuf_buffer, stream);
 	} else {
 		if (!q->reading)
-			videobuf_read_start(q);
+			__videobuf_read_start(q);
 		if (!q->reading) {
 			rc = POLLERR;
 		} else if (NULL == q->read_buf) {
author
#8 lore raw

Re: [2.6 patch] videobuf-core.c locking fixes

Quoted 57 lines · Adrian Bunk
Quoted 10 lines · Shane
Quoted 6 lines · Shane

On Dec 12, 2007 9:21 AM, Mauro Carvalho Chehab <[email protected]> wrote:
...

Quoted 2 lines

The proper solution is provided by this changeset:
http://git.kernel.org/?p=linux/kernel/git/mchehab/v4l-dvb.git;a=commitdiff;h=19fb1457990b6b7e15586ec7331541a184233acc

I applied this and it seems fine with a bttv card.

Ugh, after further testing with a bttv card it seems this is not fine.

Quoted 7 lines

vbi doesn't work anymore and my application gets stuck in a Zombie, unkillable, have to reboot state :(

mythtv 3683 1 -3 2.4 0.0 0 0 ? Z<l 13:40:35
00:00:06 [mythbackend] <defunct>

Reverting Mauro's patch above does fix the problem.

Thanks for testing, does the patch below fix it?

Quoted 1 line

Shane

cu
Adrian

<-- snip -->

After commit 19fb1457990b6b7e15586ec7331541a184233acc the callers in videobuf-core.c that already hold the lock must call __videobuf_read_start() instead of videobuf_read_start().

Signed-off-by Adrian Bunk <[email protected]>

---

 drivers/media/video/videobuf-core.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

e1f8b4a49d86746f699919531c17fd154787e308

drivers/media/video/videobuf-core.c +1 -1
index 81f77d2..c8a5cb5 100644
@@ -909,7 +909,7 @@ ssize_t videobuf_read_stream(struct videobuf_queue *q,
        if (q->streaming)
                goto done;
        if (!q->reading) {
-               retval = videobuf_read_start(q);
+               retval = __videobuf_read_start(q);
                if (retval < 0)
                        goto done;
        }

drivers/media/video/videobuf-core.c unsigned int videobuf_poll_stream(struct file *file, · line 988

Quoted 8 lines
drivers/media/video/videobuf-core.c +1 -1
@@ -982,7 +982,7 @@ unsigned int videobuf_poll_stream(struct file *file,
                                         struct videobuf_buffer, stream);
        } else {
                if (!q->reading)
-                       videobuf_read_start(q);
+                       __videobuf_read_start(q);
                if (!q->reading) {
                        rc = POLLERR;
                } else if (NULL == q->read_buf) {

Yes it does! I was just going to send the same patch myself :)

Shane

author
#9 lore raw

Re: [2.6 patch] videobuf-core.c locking fixes

Quoted 1 line

Yes it does! I was just going to send the same patch myself :)

But, I am now seeing some errors that weren't there in 2.6.23

kernel: bttv0: SCERR @ 1fa0401c,bits: HSYNC OFLOW SCERR* last message repeated 15 times
kernel: bttv0: timeout: drop=16 irq=105615/105615, risc=1fa0401c,
bits: HSYNC OFLOW
kernel: bttv0: reset, reinitialize
kernel: bttv0: PLL can sleep, using XTAL (28636363).

kernel: bttv0: SCERR @ 1fa0401c,bits: HSYNC OFLOW SCERR* last message repeated 15 times
kernel: bttv0: timeout: drop=16 irq=106741/106741, risc=1fa0401c,
bits: HSYNC OFLOW
kernel: bttv0: reset, reinitialize
kernel: bttv0: PLL can sleep, using XTAL (28636363).

These happen occasionally and it causes an EIO DQBUF error and the application has to re queue the buffers but it recovers OK. Not sure if it causes some sort of internal kernel corruption that will only be noticed later possibly?

I am using 15 userptr buffers so whatever is happening may be happening once per buffer sometimes. dunno

Shane

#10 lore raw

Re: [2.6 patch] videobuf-core.c locking fixes

Em Qua, 2007-12-12 às 16:22 -0500, Shane escreveu:

Quoted 18 lines
Quoted 1 line

Yes it does! I was just going to send the same patch myself :)

But, I am now seeing some errors that weren't there in 2.6.23

kernel: bttv0: SCERR @ 1fa0401c,bits: HSYNC OFLOW SCERR* last message repeated 15 times
kernel: bttv0: timeout: drop=16 irq=105615/105615, risc=1fa0401c,
bits: HSYNC OFLOW
kernel: bttv0: reset, reinitialize
kernel: bttv0: PLL can sleep, using XTAL (28636363).

kernel: bttv0: SCERR @ 1fa0401c,bits: HSYNC OFLOW SCERR* last message repeated 15 times
kernel: bttv0: timeout: drop=16 irq=106741/106741, risc=1fa0401c,
bits: HSYNC OFLOW
kernel: bttv0: reset, reinitialize
kernel: bttv0: PLL can sleep, using XTAL (28636363).

Quoted 7 lines

These happen occasionally and it causes an EIO DQBUF error and the application has to re queue the buffers but it recovers OK. Not sure if it causes some sort of internal kernel corruption that will only be noticed later possibly?

I am using 15 userptr buffers so whatever is happening may be happening once per buffer sometimes. dunno

You may see such troubles with weak signals, where bttv is not capable of getting the proper sync.

Signature · 2 lines
Cheers,
Mauro
#11 lore raw

Re: [2.6 patch] videobuf-core.c locking fixes

In reply to Shane
Quoted 18 lines
Quoted 18 lines

e1f8b4a49d86746f699919531c17fd154787e308

drivers/media/video/videobuf-core.c +1 -1
index 81f77d2..c8a5cb5 100644
@@ -909,7 +909,7 @@ ssize_t videobuf_read_stream(struct videobuf_queue *q,
        if (q->streaming)
                goto done;
        if (!q->reading) {
-               retval = videobuf_read_start(q);
+               retval = __videobuf_read_start(q);
                if (retval < 0)
                        goto done;
        }

@@ -982,7 +982,7 @@ unsigned int videobuf_poll_stream(struct file *file,
                                         struct videobuf_buffer, stream);
        } else {
                if (!q->reading)
Quoted 8 lines

drivers/media/video/videobuf-core.c unsigned int videobuf_poll_stream(struct file *file, · line 988

Quoted 6 lines
drivers/media/video/videobuf-core.c +1 -1
-                       videobuf_read_start(q);
+                       __videobuf_read_start(q);
                if (!q->reading) {
                        rc = POLLERR;
                } else if (NULL == q->read_buf) {

Yes it does! I was just going to send the same patch myself :)

The patch seems ok to my eyes. I dunno why I forgot to replace those two occurrences on my patch :(

Cheers,
Mauro

#12 lore raw
In reply to Mauro Carvalho Chehab

Hi Mauro,

Quoted 11 lines · Mauro Carvalho Chehab

What happened is that changeset 19bc5133dae9562e8824ef101464061f9854c1d8 fixed some bad locks.

After this changeset, videobuf_read_stream() holds q->lock and calls videobuf_read_start(). To avoid waiting forever for the lock to be released, he removed the mutex from videobuf_read_start with this line [1]:

(quoted diff) +1 -1
-       err = videobuf_mmap_setup(q, count, size, V4L2_MEMORY_USERPTR);
+       err = __videobuf_mmap_setup(q, count, size, V4L2_MEMORY_USERPTR);
Quoted 5 lines

So, after the patch, videobuf_read_start() can't be safely called. So, just adding EXPORT_SYMBOL_GPL() breaks videobuf-dvb at runtime.

The proper solution is provided by this changeset:
http://git.kernel.org/?p=linux/kernel/git/mchehab/v4l-dvb.git;a=commitdiff;h=19fb1457990b6b7e15586ec7331541a184233acc

There's a "static" missing in that patch: __videobuf_read_start is only used internally.

Signature · 1 line
Jean Delvare
#13 lore raw

Em Qua, 2007-12-12 às 23:19 +0100, Jean Delvare escreveu:

Quoted 14 lines

Hi Mauro,

Quoted 11 lines · Mauro Carvalho Chehab

What happened is that changeset 19bc5133dae9562e8824ef101464061f9854c1d8 fixed some bad locks.

After this changeset, videobuf_read_stream() holds q->lock and calls videobuf_read_start(). To avoid waiting forever for the lock to be released, he removed the mutex from videobuf_read_start with this line [1]:

(quoted diff) +1 -1
-       err = videobuf_mmap_setup(q, count, size, V4L2_MEMORY_USERPTR);
+       err = __videobuf_mmap_setup(q, count, size, V4L2_MEMORY_USERPTR);
Quoted 8 lines
Quoted 5 lines

So, after the patch, videobuf_read_start() can't be safely called. So, just adding EXPORT_SYMBOL_GPL() breaks videobuf-dvb at runtime.

The proper solution is provided by this changeset:
http://git.kernel.org/?p=linux/kernel/git/mchehab/v4l-dvb.git;a=commitdiff;h=19fb1457990b6b7e15586ec7331541a184233acc

There's a "static" missing in that patch: __videobuf_read_start is only used internally.

Thanks. It seems that I were not on my best day when I wrote this patch....

Fixed.

Signature · 2 lines
Cheers,
Mauro
#14 lore raw
In reply to Shane
Quoted 8 lines · Shane

In 2.6.24-rc5+, I hit this problem with videobuf_read_start not being exported. Patch attached, only compile tested.

  CHK     include/linux/version.h
  CHK     include/linux/utsrelease.h
  CALL    scripts/checksyscalls.sh
  CHK     include/linux/compile.h
  CC [M]  drivers/media/video/videobuf-core.o
Quoted 6 lines

Building modules, stage 2.
Kernel: arch/x86/boot/bzImage is ready (#1)
MODPOST 202 modules
ERROR: "videobuf_read_start" [drivers/media/video/videobuf-dvb.ko] undefined! make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

FWIW, I've seen the same thing and Shane's patch fixes things for me.

Soeren

#15 lore raw
In reply to Shane

Em Qua, 2007-12-12 às 00:20 -0500, Shane escreveu:

Quoted 8 lines

In 2.6.24-rc5+, I hit this problem with videobuf_read_start not being exported. Patch attached, only compile tested.

  CHK     include/linux/version.h
  CHK     include/linux/utsrelease.h
  CALL    scripts/checksyscalls.sh
  CHK     include/linux/compile.h
  CC [M]  drivers/media/video/videobuf-core.o
Quoted 6 lines

Building modules, stage 2.
Kernel: arch/x86/boot/bzImage is ready (#1)
MODPOST 202 modules
ERROR: "videobuf_read_start" [drivers/media/video/videobuf-dvb.ko] undefined! make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

Thanks for reporting.

The patch is incomplete. I've just sent Linus the complete fix for this.

The issue here is that videobuf_read_start were replaced by __videobuf_read_start, but mutex is missing at the last.

The patch I've just sent adds a mutex-safe version of videobuf_read_start, adding the proper EXPORT_SYMBOL_GPL().

Cheers,
Mauro