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

[Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>]

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