[git,pull] DVB + ieee1394: firedtv driver

Commit Message

Linus Torvalds Feb. 26, 2009, 4:40 p.m. UTC

On Thu, 26 Feb 2009, Stefan Richter wrote:
> 
> Alas I have no Linux box at hand before Saturday.  I wonder, was
> ieee1394_bus_type not yet registered (i.e. ieee1394's ieee1394_init() not yet
> called) at the moment when fdtv_init is executed?
> 
> Or what else could have been the NULL pointer dereference in driver_find?

Afaik, the only way that NULL pointer dereference can happen is if

	struct kobject *k = kset_find_obj(bus->p->drivers_kset, name);

gets a NULL pointer exception either because of a NULL "bus" pointer as 
the argument, or if "bus->p" is NULL.

And in this case, it's the latter. The Code: decode shows the whole 
function in this case:

  1c:	55                   	push   %rbp
  1d:	89 e5                	mov    %esp,%ebp
  1f:	e8 34 5f c8 ff       	callq  0xffffffffffc85f58
  24:	89 c1                	mov    %eax,%ecx
  26:	8b 42 38             	mov    0x38(%rdx),%eax
  29:	89 ca                	mov    %ecx,%edx
  2b:*	8b 40 54             	mov    0x54(%rax),%eax     <-- trapping instruction
  2e:	e8 6c 48 f9 ff       	callq  0xfffffffffff9489f
  33:	31 d2                	xor    %edx,%edx
  35:	85 c0                	test   %eax,%eax
  37:	74 03                	je     0x3c
  39:	8b 50 6c             	mov    0x6c(%rax),%edx
  3c:	5d                   	pop    %rbp
  3d:	89 d0                	mov    %edx,%eax
  3f:	c3                   	retq   

and that first "callq" seems to be due to Ingo having function tracing on, 
while the second callq would have been that 'kset_find_obj()' thing.

So it does look like "&ieee1394_bus_type" has simply not been initialized 
yet. It looks like ieee1394_init() is called too late.

Two choices that I can see:

 - do the ieee1394_init() as a fs_initcall(), earlier

 - move drivers/ieee1394 linking up to before drivers/media

but I suspect that drivers/media wants to be early, in order to do the 
_media_ layer initialization early.

Does this work?

		Linus

---
 drivers/ieee1394/ieee1394_core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Ingo Molnar Feb. 26, 2009, 5:03 p.m. UTC | #1

* Linus Torvalds <[e-mail]> wrote:

> Two choices that I can see:
> 
>  - do the ieee1394_init() as a fs_initcall(), earlier
> 
>  - move drivers/ieee1394 linking up to before drivers/media
> 
> but I suspect that drivers/media wants to be early, in order to do the 
> _media_ layer initialization early.
> 
> Does this work?

yes, i just tested it and your patch fixes the crash:

 mercury:~> uname -a
 Linux mercury 2.6.29-rc6-tip-02011-gb62a1ed-dirty #250 SMP Thu 
 Feb 26 19:00:54 CET 2009 i686 athlon i386 GNU/Linux
 mercury:~> uptime
  19:02:51 up 0 min,  1 user,  load average: 3.97, 1.10, 0.37

Tested-by: Ingo Molnar <[e-mail]>

Thanks!

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stefan Richter Feb. 27, 2009, 8:20 a.m. UTC | #2

Ingo Molnar wrote:
> * Linus Torvalds <[e-mail]> wrote:
>> Two choices that I can see:
>>
>>  - do the ieee1394_init() as a fs_initcall(), earlier
>>
>>  - move drivers/ieee1394 linking up to before drivers/media
>>
>> but I suspect that drivers/media wants to be early, in order to do the 
>> _media_ layer initialization early.

The former seems the better choice to me.  Changing the linking order 
would just break the next time around.

>> Does this work?
> 
> yes, i just tested it and your patch fixes the crash:
> 
>  mercury:~> uname -a
>  Linux mercury 2.6.29-rc6-tip-02011-gb62a1ed-dirty #250 SMP Thu 
>  Feb 26 19:00:54 CET 2009 i686 athlon i386 GNU/Linux
>  mercury:~> uptime
>   19:02:51 up 0 min,  1 user,  load average: 3.97, 1.10, 0.37
> 
> Tested-by: Ingo Molnar <[e-mail]>

Thanks guys.  I'm very sorry that this basic issue escaped my attention. 
It's the first time that a 1394 driver lives outside 
drivers/ieee1394/Makefile.

(Also shows that even very long exposure in linux-next does not catch 
runtime issues like this one.)

Stefan Richter
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/ieee1394/ieee1394_core.c b/drivers/ieee1394/ieee1394_core.c
index 1028e72..8723380 100644
--- a/drivers/ieee1394/ieee1394_core.c
+++ b/drivers/ieee1394/ieee1394_core.c
@@ -1275,7 +1275,7 @@  static void __exit ieee1394_cleanup(void)
 	unregister_chrdev_region(IEEE1394_CORE_DEV, 256);
 }
 
-module_init(ieee1394_init);
+fs_initcall(ieee1394_init);
 module_exit(ieee1394_cleanup);
 
 /* Exported symbols */