[i2c] [PATCH] i2c: Clean up core locking

[Jean Delvare]

With the redundant adapter and driver lists gone, there's room for
locking cleanups in i2c-core. We used to have a single lock
(core_lists), it still protects i2c_adapter_idr and
i2c_adapter_class.devices.

i2c_adapter_class has its own semaphore protecting (amongst others)
its device list, so we can use it instead of our own lock.

This only leaves i2c_adapter_idr to protect, so rename the mutex to
idr_lock to make it more obvious, and limit the code sections where
we hold that mutex to the minimum.

While the change was originally meant for increased code readability
and not for performance, it also happens to bring appreciable speedups
at driver loading time.

Signed-off-by: Jean Delvare <khali at linux-fr.org>
Cc: David Brownell <david-b at pacbell.net>
---
On my test system, "modprobe eeprom" is down from 5.2 s to 1.4 s after
applying this patch. That's probably an extreme case though (many i2c
adapters and one of them is broken).

 drivers/i2c/i2c-core.c |   42 +++++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

--- linux-2.6.24-rc4.orig/drivers/i2c/i2c-core.c	2007-12-06 09:09:06.000000000 +0100
+++ linux-2.6.24-rc4/drivers/i2c/i2c-core.c	2007-12-06 09:09:47.000000000 +0100
@@ -34,12 +34,13 @@
 #include <linux/mutex.h>
 #include <linux/completion.h>
 #include <asm/uaccess.h>
+#include <asm/semaphore.h>
 
 #include "i2c-core.h"
 
 
-static DEFINE_MUTEX(core_lists);
 static DEFINE_IDR(i2c_adapter_idr);
+static DEFINE_MUTEX(idr_lock);
 
 #define is_newstyle_driver(d) ((d)->probe || (d)->remove)
 
@@ -332,14 +333,12 @@ static int i2c_do_add_adapter(struct dev
 
 static int i2c_register_adapter(struct i2c_adapter *adap)
 {
-	int res = 0, dummy;
+	int res, dummy;
 
 	mutex_init(&adap->bus_lock);
 	mutex_init(&adap->clist_lock);
 	INIT_LIST_HEAD(&adap->clients);
 
-	mutex_lock(&core_lists);
-
 	/* Add the adapter to the driver core.
 	 * If the parent pointer is not set up,
 	 * we add this adapter to the host bus.
@@ -366,13 +365,13 @@ static int i2c_register_adapter(struct i
 	dummy = bus_for_each_drv(&i2c_bus_type, NULL, adap,
 				 i2c_do_add_adapter);
 
-out_unlock:
-	mutex_unlock(&core_lists);
-	return res;
+	return 0;
 
 out_list:
+	mutex_lock(&idr_lock);
 	idr_remove(&i2c_adapter_idr, adap->nr);
-	goto out_unlock;
+	mutex_unlock(&idr_lock);
+	return res;
 }
 
 /**
@@ -396,11 +395,11 @@ retry:
 	if (idr_pre_get(&i2c_adapter_idr, GFP_KERNEL) == 0)
 		return -ENOMEM;
 
-	mutex_lock(&core_lists);
+	mutex_lock(&idr_lock);
 	/* "above" here means "above or equal to", sigh */
 	res = idr_get_new_above(&i2c_adapter_idr, adapter,
 				__i2c_first_dynamic_bus_num, &id);
-	mutex_unlock(&core_lists);
+	mutex_unlock(&idr_lock);
 
 	if (res < 0) {
 		if (res == -EAGAIN)
@@ -445,7 +444,7 @@ retry:
 	if (idr_pre_get(&i2c_adapter_idr, GFP_KERNEL) == 0)
 		return -ENOMEM;
 
-	mutex_lock(&core_lists);
+	mutex_lock(&idr_lock);
 	/* "above" here means "above or equal to", sigh;
 	 * we need the "equal to" result to force the result
 	 */
@@ -454,7 +453,7 @@ retry:
 		status = -EBUSY;
 		idr_remove(&i2c_adapter_idr, id);
 	}
-	mutex_unlock(&core_lists);
+	mutex_unlock(&idr_lock);
 	if (status == -EAGAIN)
 		goto retry;
 
@@ -493,7 +492,7 @@ int i2c_del_adapter(struct i2c_adapter *
 	struct i2c_client *client;
 	int res = 0;
 
-	mutex_lock(&core_lists);
+	mutex_lock(&idr_lock);
 
 	/* First make sure that this adapter was ever added */
 	if (idr_find(&i2c_adapter_idr, adap->nr) != adap) {
@@ -545,7 +544,7 @@ int i2c_del_adapter(struct i2c_adapter *
 	dev_dbg(&adap->dev, "adapter [%s] unregistered\n", adap->name);
 
  out_unlock:
-	mutex_unlock(&core_lists);
+	mutex_unlock(&idr_lock);
 	return res;
 }
 EXPORT_SYMBOL(i2c_del_adapter);
@@ -588,21 +587,20 @@ int i2c_register_driver(struct module *o
 	if (res)
 		return res;
 
-	mutex_lock(&core_lists);
-
 	pr_debug("i2c-core: driver [%s] registered\n", driver->driver.name);
 
 	/* legacy drivers scan i2c busses directly */
 	if (driver->attach_adapter) {
 		struct i2c_adapter *adapter;
 
+		down(&i2c_adapter_class.sem);
 		list_for_each_entry(adapter, &i2c_adapter_class.devices,
 				    dev.node) {
 			driver->attach_adapter(adapter);
 		}
+		up(&i2c_adapter_class.sem);
 	}
 
-	mutex_unlock(&core_lists);
 	return 0;
 }
 EXPORT_SYMBOL(i2c_register_driver);
@@ -618,8 +616,6 @@ void i2c_del_driver(struct i2c_driver *d
 	struct i2c_client  *client;
 	struct i2c_adapter *adap;
 
-	mutex_lock(&core_lists);
-
 	/* new-style driver? */
 	if (is_newstyle_driver(driver))
 		goto unregister;
@@ -628,6 +624,7 @@ void i2c_del_driver(struct i2c_driver *d
 	 * attached. If so, detach them to be able to kill the driver
 	 * afterwards.
 	 */
+	down(&i2c_adapter_class.sem);
 	list_for_each_entry(adap, &i2c_adapter_class.devices, dev.node) {
 		if (driver->detach_adapter) {
 			if (driver->detach_adapter(adap)) {
@@ -652,12 +649,11 @@ void i2c_del_driver(struct i2c_driver *d
 			}
 		}
 	}
+	up(&i2c_adapter_class.sem);
 
  unregister:
 	driver_unregister(&driver->driver);
 	pr_debug("i2c-core: driver [%s] unregistered\n", driver->driver.name);
-
-	mutex_unlock(&core_lists);
 }
 EXPORT_SYMBOL(i2c_del_driver);
 
@@ -1116,12 +1112,12 @@ struct i2c_adapter* i2c_get_adapter(int 
 {
 	struct i2c_adapter *adapter;
 
-	mutex_lock(&core_lists);
+	mutex_lock(&idr_lock);
 	adapter = (struct i2c_adapter *)idr_find(&i2c_adapter_idr, id);
 	if (adapter && !try_module_get(adapter->owner))
 		adapter = NULL;
 
-	mutex_unlock(&core_lists);
+	mutex_unlock(&idr_lock);
 	return adapter;
 }
 EXPORT_SYMBOL(i2c_get_adapter);


-- 
Jean Delvare