of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops
Subsystems (2)
PATCH 1/1 of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops
Firstly kernel warns at set_irq_chip, and then dies completely:
Trying to install chip for IRQ-1
------------[ cut here ]------------ Badness at c00463b0 [verbose debug info unavailable] NIP: c00463b0 LR: c00463b0 CTR: c0158e80 REGS: cf821cd0 TRAP: 0700 Not tainted (2.6.26-rc8-next-20080704-20216-g7bcfaff-dirty) MSR: 00029032 <EE,ME,IR,DR> CR: 24044042 XER: 20000000
TASK = cf81f8a0[1] 'swapper' THREAD: cf820000
GPR00: c00463b0 cf821d80 cf81f8a0 00000024 000015cb ffffffff c015c1fc 00004000
GPR08: c035e960 c0350000 c035e4b8 000015cb 84044042 00000000 0fffd000 00000000
GPR16: 0fff3028 0fff6cf0 00000000 0fff8390 0ff494a0 00000004 00000000 c02ef520
GPR24: cf934210 cf821dfc c02ef4f8 c035ac08 00000000 ffffffff ffffffff 00000000 NIP [c00463b0] set_irq_chip+0x90/0x9c
LR [c00463b0] set_irq_chip+0x90/0x9c
Call Trace: [cf821d80] [c00463b0] set_irq_chip+0x90/0x9c (unreliable) [cf821da0] [c004641c] set_irq_chip_and_handler+0x1c/0x40 [cf821dc0] [c0005a64] irq_dispose_mapping+0x70/0x13c [cf821df0] [c01ebf10] of_register_i2c_devices+0x124/0x504
[cf821e50] [c0276f78] 0xc0276f78
[cf821e70] [c01eba20] of_platform_device_probe+0x5c/0x84 [cf821e90] [c0160724] really_probe+0x78/0x1a8
[cf821eb0] [c016095c] __driver_attach+0x84/0x88
[cf821ed0] [c015fc14] bus_for_each_dev+0x5c/0x98
[cf821f00] [c016056c] driver_attach+0x24/0x34
[cf821f10] [c0160220] bus_add_driver+0xb4/0x1cc
[cf821f30] [c0160c4c] driver_register+0x5c/0x114
[cf821f40] [c01eb8e8] of_register_driver+0x54/0x70
[cf821f50] [c032a2e0] fsl_i2c_init+0x24/0x54
[cf821f60] [c0310244] do_one_initcall+0x34/0x198
[cf821fd0] [c03103e0] do_initcalls+0x38/0x58
[cf821fe0] [c0310464] kernel_init+0x30/0x90
[cf821ff0] [c000f944] kernel_thread+0x44/0x60
Instruction dump:
80010024 bba10014 38210020 7c0803a6 4e800020 3d20c034 3be977b8 4bffffac 3c60c02d 7fc4f378 386329e0 4bfd6465 <0fe00000> 3860ffea 4bffffc8 9421ffe0 Trying to install type control for IRQ-1
Unable to handle kernel paging request for data at address 0x3863ce7c Faulting instruction address: 0xc0005a68 Oops: Kernel access of bad area, sig: 11 [#1] MPC837x RDB Modules linked in: NIP: c0005a68 LR: c0005a64 CTR: c0158e80 REGS: cf821d10 TRAP: 0300 Not tainted (2.6.26-rc8-next-20080704-20216-g7bcfaff-dirty) MSR: 00009032 <EE,ME,IR,DR> CR: 24044042 XER: 20000000 DAR: 3863ce7c, DSISR: 20000000
TASK = cf81f8a0[1] 'swapper' THREAD: cf820000
GPR00: c0005a64 cf821dc0 cf81f8a0 0000002c 00001d4f ffffffff c015c1fc 00004000
GPR08: c035e960 3863ce74 c035e4b8 00001d4f 24044042 00000000 0fffd000 00000000
GPR16: 0fff3028 0fff6cf0 00000000 0fff8390 0ff494a0 00000004 00000000 c02ef520
GPR24: cf934210 cf821dfc c02ef4f8 c035ac08 cf821df8 fffffff8 ffffffff c000d9f0 NIP [c0005a68] irq_dispose_mapping+0x74/0x13c
LR [c0005a64] irq_dispose_mapping+0x70/0x13c
Call Trace: [cf821dc0] [c0005a64] irq_dispose_mapping+0x70/0x13c (unreliable) [cf821df0] [c01ebf10] of_register_i2c_devices+0x124/0x504
[cf821e50] [c0276f78] 0xc0276f78
[cf821e70] [c01eba20] of_platform_device_probe+0x5c/0x84 [cf821e90] [c0160724] really_probe+0x78/0x1a8
[cf821eb0] [c016095c] __driver_attach+0x84/0x88
[cf821ed0] [c015fc14] bus_for_each_dev+0x5c/0x98
[cf821f00] [c016056c] driver_attach+0x24/0x34
[cf821f10] [c0160220] bus_add_driver+0xb4/0x1cc
[cf821f30] [c0160c4c] driver_register+0x5c/0x114
[cf821f40] [c01eb8e8] of_register_driver+0x54/0x70
[cf821f50] [c032a2e0] fsl_i2c_init+0x24/0x54
[cf821f60] [c0310244] do_one_initcall+0x34/0x198
[cf821fd0] [c03103e0] do_initcalls+0x38/0x58
[cf821fe0] [c0310464] kernel_init+0x30/0x90
[cf821ff0] [c000f944] kernel_thread+0x44/0x60
Instruction dump:5400d97e 0f000000 2f9f0000 41beffc8 801f0008 2f800000 41beffbc 7fc3f378 38800000 38a00000 480409a1 813f0018 <80090008> 2f800000 419e0014 7fe3fb78
---[ end trace 1db4087de5c456f2 ]---
--- drivers/of/of_i2c.c | 13 +++++++------ 1 files changed, 7 insertions(+), 6 deletions(-)
drivers/of/of_i2c.c +7 -6
@@ -93,10 +93,8 @@ void of_register_i2c_devices(struct i2c_adapter *adap,if (info.irq ==NO_IRQ ) info.irq =-1 ; -if (of_find_i2c_driver (node, &info) <0 ) { -irq_dispose_mapping (info.irq); -continue ; - } +if (of_find_i2c_driver (node, &info) <0 ) + goto err; info.addr = *addr;
@@ -107,9 +105,12 @@ void of_register_i2c_devices(struct i2c_adapter *adap,printk (KERN_ERR "of-i2c: Failed to load driver for %s\n" , info.type); -irq_dispose_mapping (info.irq); -continue ; + goto err; } +continue ; +err: +if (info.irq >0 ) +irq_dispose_mapping (info.irq); } }EXPORT_SYMBOL (of_register_i2c_devices);
Signature · 1 line
1.5.5.4
Quoted 5 lines · Anton Vorontsov
Firstly kernel warns at
set_irq_chip, and then dies completely:Trying to install chip for IRQ-1
------------[ cut here ]------------ Badness at c00463b0 [verbose debug info unavailable]
[..]
Quoted 1 line
Unable to handle kernel paging request for data at address 0x3863ce7c
Btw, I'm seeing this on linux-next. Just looked into the current Linus' tree, and it seems that the bug is there too. So, this is probably should go for 2.6.26.
Quoted 39 lines
Signed-off-by Anton Vorontsov <[email protected]>--- drivers/of/of_i2c.c | 13 +++++++------ 1 files changed, 7 insertions(+), 6 deletions(-)drivers/of/of_i2c.c +7
-6@@ -93,10 +93,8 @@ void of_register_i2c_devices(struct i2c_adapter *adap,if (info.irq ==NO_IRQ ) info.irq =-1 ; -if (of_find_i2c_driver (node, &info) <0 ) { -irq_dispose_mapping (info.irq); -continue ; - } +if (of_find_i2c_driver (node, &info) <0 ) + goto err; info.addr = *addr;@@ -107,9 +105,12 @@ void of_register_i2c_devices(struct i2c_adapter *adap,printk (KERN_ERR "of-i2c: Failed to load driver for %s\n" , info.type); -irq_dispose_mapping (info.irq); -continue ; + goto err; } +continue ; +err: +if (info.irq >0 ) +irq_dispose_mapping (info.irq); } }EXPORT_SYMBOL (of_register_i2c_devices);Signature · 1 line
1.5.5.4
Signature · 3 lines
Anton Vorontsov email: [email protected] irc://irc.freenode.net/bd2
drivers/of/of_i2c.c void of_register_i2c_devices(struct i2c_adapter *adap, · line 94
Quoted 11 lines · Anton Vorontsov
Firstly kernel warns at
set_irq_chip, and then dies completely:Trying to install chip for IRQ-1
drivers/of/of_i2c.c +0
-0@@ -93,10 +93,8 @@ void of_register_i2c_devices(struct i2c_adapter *adap,if (info.irq ==NO_IRQ ) info.irq =-1 ;
What is the reason that info.irq is set to -1 in the first place? This looks like another bug to me. Does something in the i2c layer depend on the -1 value?
g.
Quoted 5 lines · Grant Likely
Quoted 4 lines · Anton Vorontsov
Firstly kernel warns at
set_irq_chip, and then dies completely:Trying to install chip for IRQ-1
Quoted 9 lines
drivers/of/of_i2c.c void of_register_i2c_devices(struct i2c_adapter *adap, · line 94
Quoted 7 lines
drivers/of/of_i2c.c +0
-0@@ -93,10 +93,8 @@ void of_register_i2c_devices(struct i2c_adapter *adap,if (info.irq ==NO_IRQ ) info.irq =-1 ;What is the reason that info.irq is set to -1 in the first place?
I believe this is because of this checking mess:
grep irq -r drivers/rtc/ drivers/i2c/chips/ | grep if grep "define NO_IRQ" -r include/
Signature · 3 lines
Anton Vorontsov email: [email protected] irc://irc.freenode.net/bd2
A message that is not in the archive (sent off-list or not yet received): 20080711182323.GB15321-s3s/[email protected]
Hi Grant,
Quoted 3 lines
What is the reason that info.irq is set to -1 in the first place? This looks like another bug to me. Does something in the i2c layer depend on the -1 value?
Nope, it was a bug in the i2c documentation fixed recently:
Thanks,
Jochen
_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c
Quoted 7 lines · Jochen Friedrich
Hi Grant,
Quoted 3 lines
What is the reason that info.irq is set to -1 in the first place? This looks like another bug to me. Does something in the i2c layer depend on the -1 value?
Nope, it was a bug in the i2c documentation fixed recently:
Nope? I'm looking into i2c-core.c:
.. i2c_new_device(...)
{
client->irq = info->irq;Core will blindly pass irq, so clients should ensure that irq contains correct value. And as far as there is no common scheme of checking that "there is no irq specified", the most safe option is -1.
Quoted 1 line
Signature · 3 lines
Anton Vorontsov email: [email protected] irc://irc.freenode.net/bd2
A message that is not in the archive (sent off-list or not yet received): 20080711191502.GA21847-PHTr8nzUCjejyJ0x5qLZdcN33GVbZNy3@public.gmane.org
Quoted 11 lines · Anton Vorontsov
Quoted 1 line
Nope, it was a bug in the i2c documentation fixed recently:
Nope? I'm looking into
i2c-core.c:..
i2c_new_device(...){ client->irq = info->irq;Core will blindly pass irq, so clients should ensure that irq contains correct value. And as far as there is no common scheme of checking that "there is no irq specified", the most safe option is -1.
I wonder if -1 is really the safest; even kernel functions related to irqs are not consistent if "irq" is int or unsigned int. So, -1 could cause subtle signedness defects.
The whole "no irq" mess really needs to be cleared generally. It just disturbed me that i2c_core was imposing -1, whilst some other subsystem may have chosen 0. IMHO, subsystems like i2c should pass irqs transparently. This is why I submitted the patch for i2c documentation.
All the best,
Wolfram
Signature · 2 lines
Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de Pengutronix - Linux Solutions for Science and Industry
_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c
Quoted 5 lines · Grant Likely
Quoted 4 lines · Anton Vorontsov
Firstly kernel warns at
set_irq_chip, and then dies completely:Trying to install chip for IRQ-1
Quoted 12 lines
drivers/of/of_i2c.c void of_register_i2c_devices(struct i2c_adapter *adap, · line 94
Quoted 7 lines
drivers/of/of_i2c.c +0
-0@@ -93,10 +93,8 @@ void of_register_i2c_devices(struct i2c_adapter *adap,if (info.irq ==NO_IRQ ) info.irq =-1 ;What is the reason that info.irq is set to -1 in the first place? This looks like another bug to me. Does something in the i2c layer depend on the -1 value?
You already acked a fix to this :) I wasn't sure if my patch would make it on its own, as Jon Smirl was also working on fixes to of_i2c.c and he seemed to pick up this issue, too.
(Original patch is here:
http://ozlabs.org/pipermail/linuxppc-dev/2008-June/058801.html
)
All the best,
Wolfram
Signature · 2 lines
Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de Pengutronix - Linux Solutions for Science and Industry
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Quoted 21 lines · Wolfram Sang
Quoted 5 lines · Grant Likely
Quoted 4 lines · Anton Vorontsov
Firstly kernel warns at
set_irq_chip, and then dies completely:Trying to install chip for IRQ-1
Quoted 12 lines
drivers/of/of_i2c.c void of_register_i2c_devices(struct i2c_adapter *adap, · line 94
Quoted 7 lines
drivers/of/of_i2c.c +0
-0@@ -93,10 +93,8 @@ void of_register_i2c_devices(struct i2c_adapter *adap,if (info.irq ==NO_IRQ ) info.irq =-1 ;What is the reason that info.irq is set to -1 in the first place? This looks like another bug to me. Does something in the i2c layer depend on the -1 value?
Quoted 3 lines
You already acked a fix to this :) I wasn't sure if my patch would
make it on its own, as Jon Smirl was also working on fixes toof_i2c.cand he seemed to pick up this issue, too.
I did another patch for the mpc-i2c driver changing all of the comparisons to NO_IRQ. My understanding of this is the ppc has NO_IRQ set to -1, and powerpc has NO_IRQ = 0. So to make all of this work right you have to use the NO_IRQ symbol and you can't check against 0 or -1 directly. I also believe work is underway to get all platforms to NO_IRQ = 0 but I don't know if it is completed yet.
Quoted 28 lines
(Original patch is here:
http://ozlabs.org/pipermail/linuxppc-dev/2008-June/058801.html
)All the best,
Wolfram
--
Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
Pengutronix - Linux Solutions for Science and Industry-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)iD8DBQFIeGSED27XaX1/VRsRAr+EAJ948UwobnY7WSSR4i/ywjof1+8dJACfWzPN bhW6NXgBCnwqITIC5rSXeAI=
=W3sj
-----END PGP SIGNATURE-----_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Signature · 2 lines
Jon Smirl [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 19 lines · Wolfram Sang
Quoted 5 lines · Grant Likely
Quoted 4 lines · Anton Vorontsov
Firstly kernel warns at
set_irq_chip, and then dies completely:Trying to install chip for IRQ-1
Quoted 12 lines
drivers/of/of_i2c.c void of_register_i2c_devices(struct i2c_adapter *adap, · line 94
Quoted 7 lines
drivers/of/of_i2c.c +0
-0@@ -93,10 +93,8 @@ void of_register_i2c_devices(struct i2c_adapter *adap,if (info.irq ==NO_IRQ ) info.irq =-1 ;What is the reason that info.irq is set to -1 in the first place? This looks like another bug to me. Does something in the i2c layer depend on the -1 value?
Quoted 3 lines
You already acked a fix to this :) I wasn't sure if my patch would make it on its own, as Jon Smirl was also working on fixes to
of_i2c.cand he seemed to pick up this issue, too.
Ugh, I have trouble remembering what I've done from one day to the next. :-/
After looking at it again, I'm inclined to prefer the NO_IRQ fix. There is a smaller user base of of_i2c and so it will have a much smaller impact if it breaks stuff. It also has the added advantage of encouraging people to actually fix the drivers that assume -1.
Cheers,
g.
Signature · 7 lines
Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
Hi Anton,
Quoted 19 lines
drivers/of/of_i2c.c +2
-4@@ -93,10 +93,8 @@ void of_register_i2c_devices(struct i2c_adapter *adap,if (info.irq ==NO_IRQ ) info.irq =-1 ; -if (of_find_i2c_driver (node, &info) <0 ) { -irq_dispose_mapping (info.irq); -continue ; - } +if (of_find_i2c_driver (node, &info) <0 ) + goto err; info.addr = *addr;@@ -107,9 +105,12 @@ void of_register_i2c_devices(struct i2c_adapter *adap, printk(KERN_ERR
drivers/of/of_i2c.c void of_register_i2c_devices(struct i2c_adapter *adap, · line 113
Quoted 10 lines
drivers/of/of_i2c.c +5
-2"of-i2c: Failed to load driver for %s\n" , info.type); -irq_dispose_mapping (info.irq); -continue ; + goto err; } +continue ; +err: +if (info.irq >0 ) +irq_dispose_mapping (info.irq);
Shouldn't this be:
if (info.irq != NO_IRQ)
drivers/of/of_i2c.c void of_register_i2c_devices(struct i2c_adapter *adap, · line 116
Quoted 3 lines
drivers/of/of_i2c.c +0
-0} }EXPORT_SYMBOL (of_register_i2c_devices);
Thanks,
Jochen
Quoted 33 lines · Jochen Friedrich
Hi Anton,
Quoted 19 lines
drivers/of/of_i2c.c +2
-4@@ -93,10 +93,8 @@ void of_register_i2c_devices(struct i2c_adapter *adap,if (info.irq ==NO_IRQ ) info.irq =-1 ; -if (of_find_i2c_driver (node, &info) <0 ) { -irq_dispose_mapping (info.irq); -continue ; - } +if (of_find_i2c_driver (node, &info) <0 ) + goto err; info.addr = *addr;@@ -107,9 +105,12 @@ void of_register_i2c_devices(struct i2c_adapter *adap, printk(KERN_ERRdrivers/of/of_i2c.c void of_register_i2c_devices(struct i2c_adapter *adap, · line 113
Quoted 10 lines
drivers/of/of_i2c.c +5
-2"of-i2c: Failed to load driver for %s\n" , info.type); -irq_dispose_mapping (info.irq); -continue ; + goto err; } +continue ; +err: +if (info.irq >0 ) +irq_dispose_mapping (info.irq);Shouldn't this be:
Quoted 1 line
if (info.irq != NO_IRQ)
Maybe, maybe not. See
http://lkml.org/lkml/2005/11/21/211
I.e. maybe we should use "if (irq)". Or if we should use NO_IRQ, then we need to fixup all the users of .irq.
Signature · 3 lines
Anton Vorontsov email: [email protected] irc://irc.freenode.net/bd2
Quoted 11 lines · Anton Vorontsov
Quoted 3 lines · Jochen Friedrich
Shouldn't this be:
if (info.irq != NO_IRQ)Maybe, maybe not. See
http://lkml.org/lkml/2005/11/21/211
I.e. maybe we should use "if (irq)". Or if we should use
NO_IRQ, then we need to fixup all the users of .irq.
Yes, these are definitely wrong and need to be fixed.
g.
Quoted 6 lines · Grant Likely
Quoted 5 lines · Anton Vorontsov
Quoted 3 lines · Jochen Friedrich
Shouldn't this be:
if (info.irq != NO_IRQ)
Quoted 8 lines
Quoted 6 lines
Maybe, maybe not. See
http://lkml.org/lkml/2005/11/21/211
I.e. maybe we should use "if (irq)". Or if we should use
NO_IRQ, then we need to fixup all the users of .irq.Yes, these are definitely wrong and need to be fixed.
In rc9? ;-)
Something tells me that even for 2.6.27 such change would be somewhat late...
Signature · 3 lines
Anton Vorontsov email: [email protected] irc://irc.freenode.net/bd2
Quoted 9 lines · Anton Vorontsov
Quoted 8 lines · Grant Likely
Quoted 7 lines · Anton Vorontsov
Quoted 3 lines · Jochen Friedrich
Shouldn't this be:
if (info.irq != NO_IRQ)Maybe, maybe not. See
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
http://lkml.org/lkml/2005/11/21/211
I.e. maybe we should use "if (irq)". Or if we should use
NO_IRQ, then we need to fixup all the users of .irq.Yes, these are definitely wrong and need to be fixed.
In rc9? ;-)
I said no such thing. :-) It agree that it is too late for .26, but I think is a reasonable goal to get fixed for 2.6.27.
g.