i2c-core: fix for dep-lock validator

If kernel is compiled with CONFIG_PROVE_LOCKING the validator raises an error when a multiplexer is removed via sysfs and sub-clients are connected to it. This is a false positive.
Documentation/lockdep-design.txt recommends to handle this via calls to mutex_lock_nested()

Signed-off-by Michael Lawnick <[email protected]>

Cc Jean Delvare <[email protected]

---
Documentation originally recommends to use an enum. This is not applicable for a tree with unlimited depth. This is why I use the adapter id which is expected to be unique and monotonic increasing with the depth of the tree.

drivers/i2c/i2c-core.c renamed from drivers/i2c/i2c-core.c.dist +1 -1

@@ -628,7 +628,7 @@ i2c_sysfs_delete_device(struct device *d
 
 	/* Make sure the device was added through sysfs */
 	res = -ENOENT;
-	mutex_lock(&adap->userspace_clients_lock);
+	mutex_lock_nested(&adap->userspace_clients_lock, i2c_adapter_id(adap));
 	list_for_each_entry_safe(client, next, &adap->userspace_clients,
 				 detected) {
 		if (client->addr == addr) {

Am 05.09.2012 14:04, schrieb Michael Lawnick:

Quoted 30 lines

If kernel is compiled with CONFIG_PROVE_LOCKING the validator raises an error when a multiplexer is removed via sysfs and sub-clients are connected to it. This is a false positive.
Documentation/lockdep-design.txt recommends to handle this via calls to mutex_lock_nested()

Signed-off-by Michael Lawnick <[email protected]>

Cc Jean Delvare <[email protected]

---
Documentation originally recommends to use an enum. This is not applicable for a tree with unlimited depth. This is why I use the adapter id which is expected to be unique and monotonic increasing with the depth of the tree.

drivers/i2c/i2c-core.c renamed from drivers/i2c/i2c-core.c.dist +1 -1

@@ -628,7 +628,7 @@ i2c_sysfs_delete_device(struct device *d
 
 	/* Make sure the device was added through sysfs */
 	res = -ENOENT;
-	mutex_lock(&adap->userspace_clients_lock);
+	mutex_lock_nested(&adap->userspace_clients_lock, i2c_adapter_id(adap));
 	list_for_each_entry_safe(client, next, &adap->userspace_clients,
 				 detected) {
 		if (client->addr == addr) {

@@ -936,7 +936,7 @@ int i2c_del_adapter(struct i2c_adapter *
 		return res;
 

drivers/i2c/i2c-core.c int i2c_del_adapter(struct i2c_adapter * · line 942

Quoted 6 lines

drivers/i2c/i2c-core.c renamed from drivers/i2c/i2c-core.c.dist +1 -1

 	/* Remove devices instantiated from sysfs */
-	mutex_lock(&adap->userspace_clients_lock);
+	mutex_lock_nested(&adap->userspace_clients_lock, i2c_adapter_id(adap));
 	list_for_each_entry_safe(client, next, &adap->userspace_clients,
 				 detected) {
 		dev_dbg(&adap->dev, "Removing %s at 0x%x\n", client->name,

This will get problems with maximum log level :-(

Please ignore, other solution is under construction.

I do have one too, based on the depth level of each adapter:

drivers/i2c/i2c-core.c +16 -3

@@ -636,6 +636,16 @@ static void i2c_adapter_dev_release(stru
 	complete(&adap->dev_released);
 }
 
+static unsigned int i2c_adapter_depth(struct i2c_adapter *adapter)
+{
+	unsigned int depth = 0;
+
+	while ((adapter = i2c_parent_is_i2c_adapter(adapter)))
+		depth++;
+
+	return depth;
+}
+
 /*
  * Let users instantiate I2C devices through sysfs. This can be used when
  * platform initialization code doesn't contain the proper data for

@@ -726,7 +736,8 @@ i2c_sysfs_delete_device(struct device *d
 
 	/* Make sure the device was added through sysfs */
 	res = -ENOENT;
-	mutex_lock(&adap->userspace_clients_lock);
+	mutex_lock_nested(&adap->userspace_clients_lock,
+			  i2c_adapter_depth(adap));
 	list_for_each_entry_safe(client, next, &adap->userspace_clients,
 				 detected) {
 		if (client->addr == addr) {

@@ -869,7 +880,8 @@ static int i2c_register_adapter(struct i
 	if (res)
 		goto out_list;
 
-	dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name);
+	dev_dbg(&adap->dev, "adapter [%s] registered, depth %u\n", adap->name,
+		i2c_adapter_depth(adap));
 
 #ifdef CONFIG_I2C_COMPAT
 	res = class_compat_create_link(i2c_adapter_compat_class, &adap->dev,

@@ -1073,7 +1085,8 @@ int i2c_del_adapter(struct i2c_adapter *
 		return res;
 
 	/* Remove devices instantiated from sysfs */
-	mutex_lock(&adap->userspace_clients_lock);
+	mutex_lock_nested(&adap->userspace_clients_lock,
+			  i2c_adapter_depth(adap));
 	list_for_each_entry_safe(client, next, &adap->userspace_clients,
 				 detected) {
 		dev_dbg(&adap->dev, "Removing %s at 0x%x\n", client->name,

I was about to test it but was side-tracked by a different issue.

I did not test yet, but thought already in same direction. The draw back of this is that there is still maximum tree depth, but we could say 8 should be enough and the performance issue. May be introducing the depth into struct i2c_adapter and setting it to parent->depth + 1?

As far as performance is concerned, I thought about storing the depth value in struct i2c_adapter, but in fact I suspect the call to i2c_adapter_depth(adap) will be dropped at compilation time if CONFIG_PROVE_LOCKING isn't set, so the overhead should be zero.

Documentation originally recommends to use an enum. This is not applicable for a tree with unlimited depth. This is why I use the adapter id which is expected to be unique and monotonic increasing with the depth of the tree.