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

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

[Shane]

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

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

[-- Attachment #2: videobuf_core_export_sym.diff --]
[-- Type: text/plain, Size: 497 bytes --]

diff --git a/drivers/media/video/videobuf-core.c b/drivers/media/video/videobuf-core.c
index de2f56b..44fa76b 100644
--- a/drivers/media/video/videobuf-core.c
+++ b/drivers/media/video/videobuf-core.c
@@ -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);

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

[Adrian Bunk]

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


On Wed, Dec 12, 2007 at 12:20:31AM -0500, Shane wrote:
> 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

> diff --git a/drivers/media/video/videobuf-core.c b/drivers/media/video/videobuf-core.c
> index de2f56b..44fa76b 100644
> --- a/drivers/media/video/videobuf-core.c
> +++ b/drivers/media/video/videobuf-core.c
> @@ -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);

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

[Mauro Carvalho Chehab]

> 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.

> 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

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

[Adrian Bunk]

On Wed, Dec 12, 2007 at 09:03:14AM -0200, Mauro Carvalho Chehab wrote:
>...
> 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.

> 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.

> 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.

> Cheers,
> Mauro

cu
Adrian

-- 

       "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

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

[Mauro Carvalho Chehab]

Em Qua, 2007-12-12 às 13:03 +0100, Adrian Bunk escreveu:
> On Wed, Dec 12, 2007 at 09:03:14AM -0200, Mauro Carvalho Chehab wrote:
> >...
> > 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.

-- 
Cheers,
Mauro

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

[Shane]

On Dec 12, 2007 9:21 AM, Mauro Carvalho Chehab <mchehab@infradead.org> wrote:
...
> 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.

Mauro and Adrian,

Thanks for your prompt attention to this and for promptly pushing the
fix to Linus.

Regards,

Shane

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

[Shane]

On Dec 12, 2007 11:37 AM, Shane <gnome42@gmail.com> wrote:
> On Dec 12, 2007 9:21 AM, Mauro Carvalho Chehab <mchehab@infradead.org> wrote:
> ...
> > 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

[2.6 patch] videobuf-core.c locking fixes

[Adrian Bunk]

On Wed, Dec 12, 2007 at 01:57:27PM -0500, Shane wrote:
> On Dec 12, 2007 11:37 AM, Shane <gnome42@gmail.com> wrote:
> > On Dec 12, 2007 9:21 AM, Mauro Carvalho Chehab <mchehab@infradead.org> wrote:
> > ...
> > > 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.

Thanks for testing, does the patch below fix it?

> 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 <bunk@kernel.org>

---

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

e1f8b4a49d86746f699919531c17fd154787e308 
diff --git a/drivers/media/video/videobuf-core.c b/drivers/media/video/videobuf-core.c
index 81f77d2..c8a5cb5 100644
--- a/drivers/media/video/videobuf-core.c
+++ b/drivers/media/video/videobuf-core.c
@@ -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) {

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

[Shane]

On Dec 12, 2007 2:44 PM, Adrian Bunk <bunk@stusta.de> wrote:
> On Wed, Dec 12, 2007 at 01:57:27PM -0500, Shane wrote:
> > On Dec 12, 2007 11:37 AM, Shane <gnome42@gmail.com> wrote:
> > > On Dec 12, 2007 9:21 AM, Mauro Carvalho Chehab <mchehab@infradead.org> wrote:
> > > ...
> > > > 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.
>
> Thanks for testing, does the patch below fix it?
>
> > 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 <bunk@kernel.org>
>
> ---
>
>  drivers/media/video/videobuf-core.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> e1f8b4a49d86746f699919531c17fd154787e308
> diff --git a/drivers/media/video/videobuf-core.c b/drivers/media/video/videobuf-core.c
> index 81f77d2..c8a5cb5 100644
> --- a/drivers/media/video/videobuf-core.c
> +++ b/drivers/media/video/videobuf-core.c
> @@ -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) {
>

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

Shane

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

[Shane]

> 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

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

[Mauro Carvalho Chehab]

Em Qua, 2007-12-12 às 16:22 -0500, Shane escreveu:
> > 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

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

-- 
Cheers,
Mauro

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

[Mauro Carvalho Chehab]

> > e1f8b4a49d86746f699919531c17fd154787e308
> > diff --git a/drivers/media/video/videobuf-core.c b/drivers/media/video/videobuf-core.c
> > index 81f77d2..c8a5cb5 100644
> > --- a/drivers/media/video/videobuf-core.c
> > +++ b/drivers/media/video/videobuf-core.c
> > @@ -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) {
> >
> 
> 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

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

[Jean Delvare]

Hi Mauro,

On Wed, 12 Dec 2007 12:21:56 -0200, Mauro Carvalho Chehab wrote:
> 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

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

-- 
Jean Delvare

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

[Mauro Carvalho Chehab]

Em Qua, 2007-12-12 às 23:19 +0100, Jean Delvare escreveu:
> Hi Mauro,
> 
> On Wed, 12 Dec 2007 12:21:56 -0200, Mauro Carvalho Chehab wrote:
> > 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
> 
> 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.

-- 
Cheers,
Mauro

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

[Soeren Sonnenburg]

On Wed, 2007-12-12 at 00:20 -0500, Shane wrote:
> 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

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

Soeren

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

[Mauro Carvalho Chehab]

Em Qua, 2007-12-12 às 00:20 -0500, Shane escreveu:
> 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
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