Skip to contentSkip to conversation

i2c-core: stop using i2c_adapter.clients

patch/rft 2.6.26-rc1 David Brownell 2 participants 4 messages May 5, 2008 – May 20, 2008 3 messages not in the archive

Subsystems (2)
I2C subsystem host drivers I2C
lore
Share & tools
lore

David Brownell

author
#0 lore raw

This changes how the I2C core iterates devices connected to a given i2c_adapter, making it use driver model facilities instead of the deprecated i2c_adapter.clients list. The two functions affected are i2c_del_adapter() and i2c_del_driver().

This is something of a bugfix: updates to that list are protected by a lock (i2c_adapter.clist_lock) which is completly ignored by those two iteration loops ...

Signed-off-by David Brownell <[email protected]>

---
Builds fine, and I have a hard time seeing this break. But this hasn't really been tested.

 drivers/i2c/i2c-core.c |   91 +++++++++++++++++++++++++------------------------
 1 file changed, 48 insertions(+), 43 deletions(-)
drivers/i2c/i2c-core.c +48 -43
@@ -725,6 +725,44 @@ static int i2c_do_del_adapter(struct dev
 	return res;
 }
 
+static int i2c_do_detach_client(struct device *dev, void *target)
+{
+	struct i2c_client	*client = i2c_verify_client(dev);
+	struct i2c_driver	*driver;
+	int			status;
+
+	/* ignore non-i2c nodes */
+	if (!client)
+		return 0;
+
+	/* we may care only about one specific (legacy) driver */
+	driver = client->driver;
+	if (target && driver != target)
+		return 0;
+
+	/* cleanup new-style nodes */
+	if (!driver || is_newstyle_driver(driver)) {
+		i2c_unregister_device(client);
+		return 0;
+	}
+
+	/* detach legacy drivers */
+	if (driver->detach_client)
+		status = driver->detach_client(client);
+	else
+		status = -EOPNOTSUPP;
+	if (status < 0)
+		dev_err(&client->adapter->dev,
+			"detach_client failed for client "
+			"[%s] at address 0x%02x\n",
+			client->name, client->addr);
+
+	/* backwards compat: ignore failure while removing a single
+	 * legacy driver; maybe we can disconnect it from other devs.
+	 */
+	return target ? 0 : status;
+}
+
 /**
  * i2c_del_adapter - unregister I2C adapter
  * @adap: the adapter being unregistered

@@ -735,8 +773,6 @@ static int i2c_do_del_adapter(struct dev
  */
 int i2c_del_adapter(struct i2c_adapter *adap)
 {
-	struct list_head  *item, *_n;
-	struct i2c_client *client;
 	int res = 0;
 
 	mutex_lock(&core_lock);

@@ -749,34 +785,18 @@ int i2c_del_adapter(struct i2c_adapter *
 		goto out_unlock;
 	}
 
-	/* Tell drivers about this removal */
+	/* Tell legacy drivers about this removal.  They can fail...
+	 * in which case, give up.
+	 */
 	res = bus_for_each_drv(&i2c_bus_type, NULL, adap,
 			       i2c_do_del_adapter);
 	if (res)
 		goto out_unlock;
 
-	/* detach any active clients. This must be done first, because
-	 * it can fail; in which case we give up. */
-	list_for_each_safe(item, _n, &adap->clients) {
-		struct i2c_driver	*driver;
-
-		client = list_entry(item, struct i2c_client, list);
-		driver = client->driver;
-
-		/* new style, follow standard driver model */
-		if (!driver || is_newstyle_driver(driver)) {
-			i2c_unregister_device(client);
-			continue;
-		}
-
-		/* legacy drivers create and remove clients themselves */
-		if ((res = driver->detach_client(client))) {
-			dev_err(&adap->dev, "detach_client failed for client "
-				"[%s] at address 0x%02x\n", client->name,
-				client->addr);
-			goto out_unlock;
-		}
-	}
+	/* Detach any active clients, both legacy and new style. */
+	res = device_for_each_child(&adap->dev, NULL, i2c_do_detach_client);
+	if (res)
+		goto out_unlock;
 
 	if (adap->has_alert_irq) {
 		devm_free_irq(&adap->dev, adap->irq, adap);

@@ -868,8 +888,6 @@ EXPORT_SYMBOL(i2c_register_driver);
  */
 void i2c_del_driver(struct i2c_driver *driver)
 {
-	struct list_head   *item2, *_n;
-	struct i2c_client  *client;
 	struct i2c_adapter *adap;
 
 	mutex_lock(&core_lock);

@@ -890,22 +908,9 @@ void i2c_del_driver(struct i2c_driver *d
 					"for driver [%s]\n",
 					driver->driver.name);
 			}
-		} else {
-			list_for_each_safe(item2, _n, &adap->clients) {
-				client = list_entry(item2, struct i2c_client, list);
-				if (client->driver != driver)
-					continue;
-				dev_dbg(&adap->dev, "detaching client [%s] "
-					"at 0x%02x\n", client->name,
-					client->addr);
-				if (driver->detach_client(client)) {
-					dev_err(&adap->dev, "detach_client "
-						"failed for client [%s] at "
-						"0x%02x\n", client->name,
-						client->addr);
-				}
-			}
-		}
+		} else
+			device_for_each_child(&adap->dev, driver,
+					i2c_do_detach_client);
 	}
 	up(&i2c_adapter_class.sem);

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): 200805042124.45551.david-b-yBeKhBN/[email protected]

#2 lore raw

Hi David,

Quoted 9 lines · David Brownell

This changes how the I2C core iterates devices connected to a given i2c_adapter, making it use driver model facilities instead of the deprecated i2c_adapter.clients list. The two functions affected are i2c_del_adapter() and i2c_del_driver().

This is something of a bugfix: updates to that list are protected by a lock (i2c_adapter.clist_lock) which is completly ignored by those two iteration loops ...

Quoted 7 lines
Signed-off-by David Brownell <[email protected]>

---
Builds fine, and I have a hard time seeing this break. But this hasn't really been tested.

 drivers/i2c/i2c-core.c |   91 +++++++++++++++++++++++++------------------------
 1 file changed, 48 insertions(+), 43 deletions(-)

I remember doing the same change several months ago, but finally dropping the patch because it was deadlocking. Your patch has the exact same problem: rmmod'ing a legacy i2c driver deadlocks.

rmmod D ffffffff80443380 0 4669 4652
ffff81002e177cc8 0000000000000086 ffff81002e1dc190 ffff81003f8a1240 ffff81002e177cc8 ffff81002e177cc8 ffff81002e226b90 7fffffffffffffff 0000000000000002 7fffffffffffffff ffff81003f172648 ffff81002e177d48

Call Trace:
 [<ffffffff8042c175>] schedule_timeout+0x95/0xd0
 [<ffffffff802c3ce0>] ? sysfs_remove_dir+0x60/0x80
 [<ffffffff8042bf31>] wait_for_common+0xd1/0x160
 [<ffffffff80225080>] ? default_wake_function+0x0/0x10
 [<ffffffff8030d4e7>] ? kobject_put+0x27/0x60
 [<ffffffff8042c058>] wait_for_completion+0x18/0x20
 [<ffffffff803b4fe2>] i2c_detach_client+0x82/0xc0
 [<ffffffffa02e4071>] :lm90:lm90_detach_client+0x71/0x90
 [<ffffffff803b4f0b>] i2c_do_detach_client+0x8b/0xe0
 [<ffffffff803b4e80>] ? i2c_do_detach_client+0x0/0xe0
 [<ffffffff8038d473>] device_for_each_child+0x33/0x60
 [<ffffffff803b45e6>] i2c_del_driver+0x106/0x120
 [<ffffffffa02e4f10>] :lm90:sensors_lm90_exit+0x10/0x12
 [<ffffffff8024d41e>] sys_delete_module+0x12e/0x1f0
 [<ffffffff8020b43b>] system_call_after_swapgs+0x7b/0x80

I even seem to remember that you had been able to explain this deadlock back then...

Obviously I can't take your patch until this is solved.

Thanks,

Signature · 6 lines
Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#4 lore raw
Quoted 3 lines · Jean Delvare

I remember doing the same change several months ago, but finally dropping the patch because it was deadlocking. Your patch has the exact same problem: rmmod'ing a legacy i2c driver deadlocks.

I thought that had been resolved as part of another patch resolving a self-deadlock ... I guess not! I'll have to set up a test rig with a legacy driver module, I guess.

I'll have a closer look at this. This wait-for-completion hook is clearly an abuse of the driver model code, but it might be practical to solve it by upping refcounts for legacy code and having a list of pending deletes.

Did lockdep tell you more about locking goofage here, or is this the only symptiom?

Quoted 21 lines

rmmod         D ffffffff80443380     0  4669   4652 ffff81002e177cc8 0000000000000086 ffff81002e1dc190 ffff81003f8a1240 ffff81002e177cc8 ffff81002e177cc8 ffff81002e226b90 7fffffffffffffff 0000000000000002 7fffffffffffffff ffff81003f172648 ffff81002e177d48

Call Trace:
 [<ffffffff8042c175>] schedule_timeout+0x95/0xd0
 [<ffffffff802c3ce0>] ? sysfs_remove_dir+0x60/0x80
 [<ffffffff8042bf31>] wait_for_common+0xd1/0x160
 [<ffffffff80225080>] ? default_wake_function+0x0/0x10
 [<ffffffff8030d4e7>] ? kobject_put+0x27/0x60
 [<ffffffff8042c058>] wait_for_completion+0x18/0x20
 [<ffffffff803b4fe2>] i2c_detach_client+0x82/0xc0
 [<ffffffffa02e4071>] :lm90:lm90_detach_client+0x71/0x90
 [<ffffffff803b4f0b>] i2c_do_detach_client+0x8b/0xe0
 [<ffffffff803b4e80>] ? i2c_do_detach_client+0x0/0xe0
 [<ffffffff8038d473>] device_for_each_child+0x33/0x60
 [<ffffffff803b45e6>] i2c_del_driver+0x106/0x120
 [<ffffffffa02e4f10>] :lm90:sensors_lm90_exit+0x10/0x12
 [<ffffffff8024d41e>] sys_delete_module+0x12e/0x1f0
 [<ffffffff8020b43b>] system_call_after_swapgs+0x7b/0x80
Quoted 2 lines

I even seem to remember that you had been able to explain this deadlock back then...

Yeah, that's why I have various i2c lock removal patches floating around.

Quoted 1 line

Obviously I can't take your patch until this is solved.

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): 200805200922.19990.david-b-yBeKhBN/[email protected]

#6 lore raw

Hi David,

Quoted 8 lines · David Brownell
Quoted 3 lines · Jean Delvare

I remember doing the same change several months ago, but finally dropping the patch because it was deadlocking. Your patch has the exact same problem: rmmod'ing a legacy i2c driver deadlocks.

I thought that had been resolved as part of another patch resolving a self-deadlock ... I guess not! I'll have to set up a test rig with a legacy driver module, I guess.

An easy way to do this, is to build the i2c-stub and eeprom drivers, load i2c-stub with chip_addr=0x50, and then load eeprom. It should attach to the fake chip at 0x50 on the stub bus.

Quoted 7 lines

I'll have a closer look at this. This wait-for-completion hook is clearly an abuse of the driver model code, but it might be practical to solve it by upping refcounts for legacy code and having a list of pending deletes.

Did lockdep tell you more about locking goofage here, or is this the only symptiom?

Unfortunately, lockdep wasn't enabled in my kernel at that time, so I can't tell. I've just enabled it again.

Quoted 21 lines
Quoted 21 lines

rmmod         D ffffffff80443380     0  4669   4652 ffff81002e177cc8 0000000000000086 ffff81002e1dc190 ffff81003f8a1240 ffff81002e177cc8 ffff81002e177cc8 ffff81002e226b90 7fffffffffffffff 0000000000000002 7fffffffffffffff ffff81003f172648 ffff81002e177d48

Call Trace:
 [<ffffffff8042c175>] schedule_timeout+0x95/0xd0
 [<ffffffff802c3ce0>] ? sysfs_remove_dir+0x60/0x80
 [<ffffffff8042bf31>] wait_for_common+0xd1/0x160
 [<ffffffff80225080>] ? default_wake_function+0x0/0x10
 [<ffffffff8030d4e7>] ? kobject_put+0x27/0x60
 [<ffffffff8042c058>] wait_for_completion+0x18/0x20
 [<ffffffff803b4fe2>] i2c_detach_client+0x82/0xc0
 [<ffffffffa02e4071>] :lm90:lm90_detach_client+0x71/0x90
 [<ffffffff803b4f0b>] i2c_do_detach_client+0x8b/0xe0
 [<ffffffff803b4e80>] ? i2c_do_detach_client+0x0/0xe0
 [<ffffffff8038d473>] device_for_each_child+0x33/0x60
 [<ffffffff803b45e6>] i2c_del_driver+0x106/0x120
 [<ffffffffa02e4f10>] :lm90:sensors_lm90_exit+0x10/0x12
 [<ffffffff8024d41e>] sys_delete_module+0x12e/0x1f0
 [<ffffffff8020b43b>] system_call_after_swapgs+0x7b/0x80
Quoted 5 lines
Quoted 2 lines

I even seem to remember that you had been able to explain this deadlock back then...

Yeah, that's why I have various i2c lock removal patches floating around.

Thanks,

Signature · 6 lines
Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c