[patch 2.6.27-rc7] i2c: guard against oopses from bad init sequences

[patch 2.6.27-rc7] i2c: guard against oopses from bad init sequences

[David Brownell]

From: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>

Guard I2C against oopsing because of init sequence problems, by
verifying that i2c_init() has been called before calling any
routines that rely on that initialization.  This specific test
just requires that bus_register(&i2c_bus_type) was called.

Examples of this kind of oopsing come from subystems and drivers
which register I2C drivers in their subsys_initcall code but
which are statically linked before I2C by drivers/Makefile.

Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
Alternatively have postcore_initcall(i2c_init), which may
be better ... the initcall levels are pretty limited, and
in these cases the "subsystem" of interest builds on I2C
and needs to work before device_initcall.  Having I2C use
subsys_initcall kind of forces things into fs_initcall.

I'd encourage the anti-oopsing paranoia in any case, even
if i2c switches to postcore_initcall (or earlier).

 drivers/i2c/i2c-core.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -443,6 +443,12 @@ static int i2c_register_adapter(struct i
 
 	mutex_lock(&core_lock);
 
+	/* can't register until after driver model init */
+	if (WARN_ON(!i2c_bus_type.p)) {
+		res = -ENOENT;
+		goto out_list;
+	}
+
 	/* Add the adapter to the driver core.
 	 * If the parent pointer is not set up,
 	 * we add this adapter to the host bus.
@@ -696,6 +702,10 @@ int i2c_register_driver(struct module *o
 {
 	int res;
 
+	/* can't register until after driver model init */
+	if (WARN_ON(!i2c_bus_type.p))
+		return -ENOENT;
+
 	/* new style driver methods can't mix with legacy ones */
 	if (is_newstyle_driver(driver)) {
 		if (driver->attach_adapter || driver->detach_adapter

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

Re: [patch 2.6.27-rc7] i2c: guard against oopses from bad init sequences

[Jean Delvare]

Hi David,

On Tue, 23 Sep 2008 11:38:19 -0700, David Brownell wrote:
> From: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> 
> Guard I2C against oopsing because of init sequence problems, by
> verifying that i2c_init() has been called before calling any
> routines that rely on that initialization.  This specific test
> just requires that bus_register(&i2c_bus_type) was called.
> 
> Examples of this kind of oopsing come from subystems and drivers
> which register I2C drivers in their subsys_initcall code but
> which are statically linked before I2C by drivers/Makefile.
> 
> Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> ---
> Alternatively have postcore_initcall(i2c_init), which may
> be better ... the initcall levels are pretty limited, and
> in these cases the "subsystem" of interest builds on I2C
> and needs to work before device_initcall.  Having I2C use
> subsys_initcall kind of forces things into fs_initcall.
> 
> I'd encourage the anti-oopsing paranoia in any case, even
> if i2c switches to postcore_initcall (or earlier).

I'm confused. If i2c_register_adapter() is called before i2c_init(),
your patch prevents an oops, but things will still not work, right? So
this doesn't seem to add much value. In any case we need to fix the
init sequence, so let's just do that.

> 
>  drivers/i2c/i2c-core.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -443,6 +443,12 @@ static int i2c_register_adapter(struct i
>  
>  	mutex_lock(&core_lock);
>  
> +	/* can't register until after driver model init */
> +	if (WARN_ON(!i2c_bus_type.p)) {
> +		res = -ENOENT;
> +		goto out_list;
> +	}
> +
>  	/* Add the adapter to the driver core.
>  	 * If the parent pointer is not set up,
>  	 * we add this adapter to the host bus.
> @@ -696,6 +702,10 @@ int i2c_register_driver(struct module *o
>  {
>  	int res;
>  
> +	/* can't register until after driver model init */
> +	if (WARN_ON(!i2c_bus_type.p))
> +		return -ENOENT;
> +
>  	/* new style driver methods can't mix with legacy ones */
>  	if (is_newstyle_driver(driver)) {
>  		if (driver->attach_adapter || driver->detach_adapter


-- 
Jean Delvare

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

Re: [patch 2.6.27-rc7] i2c: guard against oopses from bad init sequences

[David Brownell]

On Wednesday 24 September 2008, Jean Delvare wrote:
> Hi David,
> 
> On Tue, 23 Sep 2008 11:38:19 -0700, David Brownell wrote:
> > From: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> > 
> > Guard I2C against oopsing because of init sequence problems...
> > 
> > Alternatively have postcore_initcall(i2c_init), which may
> > be better ... the initcall levels are pretty limited, and
> > in these cases the "subsystem" of interest builds on I2C
> > and needs to work before device_initcall.  Having I2C use
> > subsys_initcall kind of forces things into fs_initcall.
> > 
> > I'd encourage the anti-oopsing paranoia in any case, even
> > if i2c switches to postcore_initcall (or earlier).
> 
> I'm confused. If i2c_register_adapter() is called before i2c_init(),
> your patch prevents an oops, but things will still not work, right?

And it warns about exactly where the problem came from,
because of the WARN_ON.  That addresses a long-standing
debuggability problem which has afflicted these issues.


> So 
> this doesn't seem to add much value. In any case we need to fix the
> init sequence, so let's just do that.

I'll send in a separate patch switching the driver model init
to a postcore_initcall() -- it's not arch-specific and there
seems no reason to have it earlier.

Semi-related:  that module init/exit code is at a wierd place,
in the middle of that file ... vs the conventional location at
the end.  OK to change that?  In the same patch, or separate?

- Dave


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

Re: [patch 2.6.27-rc7] i2c: guard against oopses from bad init sequences

[Jean Delvare]

Hi David,

On Tue, 23 Sep 2008 11:38:19 -0700, David Brownell wrote:
> From: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> 
> Guard I2C against oopsing because of init sequence problems, by
> verifying that i2c_init() has been called before calling any
> routines that rely on that initialization.  This specific test
> just requires that bus_register(&i2c_bus_type) was called.
> 
> Examples of this kind of oopsing come from subystems and drivers
> which register I2C drivers in their subsys_initcall code but
> which are statically linked before I2C by drivers/Makefile.
> 
> Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> ---
> Alternatively have postcore_initcall(i2c_init), which may
> be better ... the initcall levels are pretty limited, and
> in these cases the "subsystem" of interest builds on I2C
> and needs to work before device_initcall.  Having I2C use
> subsys_initcall kind of forces things into fs_initcall.
> 
> I'd encourage the anti-oopsing paranoia in any case, even
> if i2c switches to postcore_initcall (or earlier).
> 
>  drivers/i2c/i2c-core.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -443,6 +443,12 @@ static int i2c_register_adapter(struct i
>  
>  	mutex_lock(&core_lock);
>  
> +	/* can't register until after driver model init */
> +	if (WARN_ON(!i2c_bus_type.p)) {
> +		res = -ENOENT;
> +		goto out_list;
> +	}
> +

Why don't you test before acquiring core_lock? Or even, before doing
anything else, as you do in i2c_register_driver. That's more consistent
and makes the error path lighter.

>  	/* Add the adapter to the driver core.
>  	 * If the parent pointer is not set up,
>  	 * we add this adapter to the host bus.
> @@ -696,6 +702,10 @@ int i2c_register_driver(struct module *o
>  {
>  	int res;
>  
> +	/* can't register until after driver model init */
> +	if (WARN_ON(!i2c_bus_type.p))
> +		return -ENOENT;
> +
>  	/* new style driver methods can't mix with legacy ones */
>  	if (is_newstyle_driver(driver)) {
>  		if (driver->attach_adapter || driver->detach_adapter

Also, I see that you still have some love for unique error codes even
where they don't match the actual error. There's hardly a file or
directory involved here... I think -EAGAIN would make more sense, as
the i2c bus type will become available at some later point in time.

So, I would apply the following patch if that's OK with you:

 drivers/i2c/i2c-core.c |    8 ++++++++
 1 file changed, 8 insertions(+)

--- linux-2.6.27-rc8.orig/drivers/i2c/i2c-core.c	2008-09-30 10:14:21.000000000 +0200
+++ linux-2.6.27-rc8/drivers/i2c/i2c-core.c	2008-09-30 11:19:30.000000000 +0200
@@ -437,6 +437,10 @@ static int i2c_register_adapter(struct i
 {
 	int res = 0, dummy;
 
+	/* Can't register until after driver model init */
+	if (WARN_ON(!i2c_bus_type.p))
+		return -EAGAIN;
+
 	mutex_init(&adap->bus_lock);
 	mutex_init(&adap->clist_lock);
 	INIT_LIST_HEAD(&adap->clients);
@@ -696,6 +700,10 @@ int i2c_register_driver(struct module *o
 {
 	int res;
 
+	/* Can't register until after driver model init */
+	if (WARN_ON(!i2c_bus_type.p))
+		return -EAGAIN;
+
 	/* new style driver methods can't mix with legacy ones */
 	if (is_newstyle_driver(driver)) {
 		if (driver->attach_adapter || driver->detach_adapter


-- 
Jean Delvare

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

Re: [patch 2.6.27-rc7] i2c: guard against oopses from bad init sequences

[David Brownell]

On Tuesday 30 September 2008, Jean Delvare wrote:

> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -443,6 +443,12 @@ static int i2c_register_adapter(struct i
> >  
> >  	mutex_lock(&core_lock);
> >  
> > +	/* can't register until after driver model init */
> > +	if (WARN_ON(!i2c_bus_type.p)) {
> > +		res = -ENOENT;
> > +		goto out_list;
> > +	}
> > +
> 
> Why don't you test before acquiring core_lock? Or even, before doing
> anything else, as you do in i2c_register_driver. That's more consistent
> and makes the error path lighter.

One or the other was an afterthought.  I don't recall which one.  ;)


> So, I would apply the following patch if that's OK with you:

Sure ... I'll be happy to have such issues be more diagnosable.

- Dave


>  drivers/i2c/i2c-core.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> --- linux-2.6.27-rc8.orig/drivers/i2c/i2c-core.c	2008-09-30 10:14:21.000000000 +0200
> +++ linux-2.6.27-rc8/drivers/i2c/i2c-core.c	2008-09-30 11:19:30.000000000 +0200
> @@ -437,6 +437,10 @@ static int i2c_register_adapter(struct i
>  {
>  	int res = 0, dummy;
>  
> +	/* Can't register until after driver model init */
> +	if (WARN_ON(!i2c_bus_type.p))
> +		return -EAGAIN;
> +
>  	mutex_init(&adap->bus_lock);
>  	mutex_init(&adap->clist_lock);
>  	INIT_LIST_HEAD(&adap->clients);
> @@ -696,6 +700,10 @@ int i2c_register_driver(struct module *o
>  {
>  	int res;
>  
> +	/* Can't register until after driver model init */
> +	if (WARN_ON(!i2c_bus_type.p))
> +		return -EAGAIN;
> +
>  	/* new style driver methods can't mix with legacy ones */
>  	if (is_newstyle_driver(driver)) {
>  		if (driver->attach_adapter || driver->detach_adapter
> 
> 

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