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

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

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

[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