Hi David,
On Tue, 23 Sep 2008 11:38:19 -0700, David Brownell wrote:
> From: David Brownell <[EMAIL PROTECTED]>
>
> 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 <[EMAIL PROTECTED]>
> ---
> 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
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c
