[PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops

[PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops

[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]
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 ]---

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/of/of_i2c.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
index b2ccdcb..95a24de 100644
--- a/drivers/of/of_i2c.c
+++ b/drivers/of/of_i2c.c
@@ -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);
-- 
1.5.5.4

Re: [PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops

[Anton Vorontsov]

On Fri, Jul 11, 2008 at 09:48:59PM +0400, Anton Vorontsov wrote:
> 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]
[..]
> 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.

> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  drivers/of/of_i2c.c |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
> index b2ccdcb..95a24de 100644
> --- a/drivers/of/of_i2c.c
> +++ b/drivers/of/of_i2c.c
> @@ -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);
> -- 
> 1.5.5.4

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops

[Grant Likely]

On Fri, Jul 11, 2008 at 09:48:59PM +0400, Anton Vorontsov wrote:
> Firstly kernel warns at set_irq_chip, and then dies completely:
> 
> Trying to install chip for IRQ-1
>
> diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
> index b2ccdcb..95a24de 100644
> --- a/drivers/of/of_i2c.c
> +++ b/drivers/of/of_i2c.c
> @@ -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.

Re: [PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops

[Anton Vorontsov]

On Fri, Jul 11, 2008 at 12:23:23PM -0600, Grant Likely wrote:
> On Fri, Jul 11, 2008 at 09:48:59PM +0400, Anton Vorontsov wrote:
> > Firstly kernel warns at set_irq_chip, and then dies completely:
> > 
> > Trying to install chip for IRQ-1
> >
> > diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
> > index b2ccdcb..95a24de 100644
> > --- a/drivers/of/of_i2c.c
> > +++ b/drivers/of/of_i2c.c
> > @@ -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/

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops

[Jochen Friedrich]

Hi Grant,

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

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=8e29da9ee8958cc17e27f4053420f1c982614793

Thanks,
Jochen

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops

[Jochen Friedrich]

Hi Grant,

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

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=8e29da9ee8958cc17e27f4053420f1c982614793

Thanks,
Jochen

Re: [PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops

[Anton Vorontsov]

On Fri, Jul 11, 2008 at 09:01:10PM +0200, Jochen Friedrich wrote:
> Hi Grant,
> 
> > 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.

> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=8e29da9ee8958cc17e27f4053420f1c982614793

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1062 bytes --]

On Fri, Jul 11, 2008 at 11:15:02PM +0400, Anton Vorontsov wrote:

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

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [i2c] [PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops

[Wolfram Sang]

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

On Fri, Jul 11, 2008 at 11:15:02PM +0400, Anton Vorontsov wrote:

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

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [i2c] [PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1147 bytes --]

On Fri, Jul 11, 2008 at 12:23:23PM -0600, Grant Likely wrote:
> On Fri, Jul 11, 2008 at 09:48:59PM +0400, Anton Vorontsov wrote:
> > Firstly kernel warns at set_irq_chip, and then dies completely:
> > 
> > Trying to install chip for IRQ-1
> >
> > diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
> > index b2ccdcb..95a24de 100644
> > --- a/drivers/of/of_i2c.c
> > +++ b/drivers/of/of_i2c.c
> > @@ -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

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 146 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [i2c] [PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops

[Wolfram Sang]

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

On Fri, Jul 11, 2008 at 12:23:23PM -0600, Grant Likely wrote:
> On Fri, Jul 11, 2008 at 09:48:59PM +0400, Anton Vorontsov wrote:
> > Firstly kernel warns at set_irq_chip, and then dies completely:
> > 
> > Trying to install chip for IRQ-1
> >
> > diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
> > index b2ccdcb..95a24de 100644
> > --- a/drivers/of/of_i2c.c
> > +++ b/drivers/of/of_i2c.c
> > @@ -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

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [i2c] [PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops

[Jon Smirl]

On 7/12/08, Wolfram Sang <w.sang@pengutronix.de> wrote:
> On Fri, Jul 11, 2008 at 12:23:23PM -0600, Grant Likely wrote:
>
> > On Fri, Jul 11, 2008 at 09:48:59PM +0400, Anton Vorontsov wrote:
>  > > Firstly kernel warns at set_irq_chip, and then dies completely:
>  > >
>  > > Trying to install chip for IRQ-1
>  > >
>  > > diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
>  > > index b2ccdcb..95a24de 100644
>  > > --- a/drivers/of/of_i2c.c
>  > > +++ b/drivers/of/of_i2c.c
>  > > @@ -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.

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.


>
>  (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
>  Linuxppc-dev@ozlabs.org
>  https://ozlabs.org/mailman/listinfo/linuxppc-dev
>
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops

[Grant Likely]

On Sat, Jul 12, 2008 at 2:00 AM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> On Fri, Jul 11, 2008 at 12:23:23PM -0600, Grant Likely wrote:
>> On Fri, Jul 11, 2008 at 09:48:59PM +0400, Anton Vorontsov wrote:
>> > Firstly kernel warns at set_irq_chip, and then dies completely:
>> >
>> > Trying to install chip for IRQ-1
>> >
>> > diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
>> > index b2ccdcb..95a24de 100644
>> > --- a/drivers/of/of_i2c.c
>> > +++ b/drivers/of/of_i2c.c
>> > @@ -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.

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.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [i2c] [PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops

[Grant Likely]

On Sat, Jul 12, 2008 at 2:00 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> On Fri, Jul 11, 2008 at 12:23:23PM -0600, Grant Likely wrote:
>> On Fri, Jul 11, 2008 at 09:48:59PM +0400, Anton Vorontsov wrote:
>> > Firstly kernel warns at set_irq_chip, and then dies completely:
>> >
>> > Trying to install chip for IRQ-1
>> >
>> > diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
>> > index b2ccdcb..95a24de 100644
>> > --- a/drivers/of/of_i2c.c
>> > +++ b/drivers/of/of_i2c.c
>> > @@ -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.

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.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops

[Jochen Friedrich]

Hi Anton,

> diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
> index b2ccdcb..95a24de 100644
> --- a/drivers/of/of_i2c.c
> +++ b/drivers/of/of_i2c.c
> @@ -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);

Shouldn't this be:
		if (info.irq != NO_IRQ)

>  	}
>  }
>  EXPORT_SYMBOL(of_register_i2c_devices);

Thanks,
Jochen

Re: [PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops

[Anton Vorontsov]

On Fri, Jul 11, 2008 at 09:11:02PM +0200, Jochen Friedrich wrote:
> Hi Anton,
> 
> > diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
> > index b2ccdcb..95a24de 100644
> > --- a/drivers/of/of_i2c.c
> > +++ b/drivers/of/of_i2c.c
> > @@ -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);
> 
> 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.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops

[Grant Likely]

On Fri, Jul 11, 2008 at 11:19:30PM +0400, Anton Vorontsov wrote:
> On Fri, Jul 11, 2008 at 09:11:02PM +0200, Jochen Friedrich wrote:
> > 
> > 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.

Re: [PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops

[Anton Vorontsov]

On Fri, Jul 11, 2008 at 03:18:13PM -0600, Grant Likely wrote:
> On Fri, Jul 11, 2008 at 11:19:30PM +0400, Anton Vorontsov wrote:
> > On Fri, Jul 11, 2008 at 09:11:02PM +0200, Jochen Friedrich wrote:
> > > 
> > > 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.

In rc9? ;-)

Something tells me that even for 2.6.27 such change would be somewhat
late...

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel will oops

[Grant Likely]

On Sat, Jul 12, 2008 at 01:46:14AM +0400, Anton Vorontsov wrote:
> On Fri, Jul 11, 2008 at 03:18:13PM -0600, Grant Likely wrote:
> > On Fri, Jul 11, 2008 at 11:19:30PM +0400, Anton Vorontsov wrote:
> > > On Fri, Jul 11, 2008 at 09:11:02PM +0200, Jochen Friedrich wrote:
> > > > 
> > > > 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.
> 
> 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.