public inbox for linux-i2c@vger.kernel.org
 help / color / mirror / Atom feed
* [patch/rft 2.6.26-rc1] i2c-core: stop using i2c_adapter.clients
@ 2008-05-05  4:24 David Brownell
       [not found] ` <200805042124.45551.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
  0 siblings, 1 reply; 4+ messages in thread
From: David Brownell @ 2008-05-05  4:24 UTC (permalink / raw)
  To: i2c-GZX6beZjE8VD60Wz+7aTrA

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 <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
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(-)

--- g26.orig/drivers/i2c/i2c-core.c	2008-05-04 20:51:58.000000000 -0700
+++ g26/drivers/i2c/i2c-core.c	2008-05-04 20:55:46.000000000 -0700
@@ -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
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [patch/rft 2.6.26-rc1] i2c-core: stop using i2c_adapter.clients
       [not found] ` <200805042124.45551.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
@ 2008-05-20 13:52   ` Jean Delvare
       [not found]     ` <20080520155232.4937c52a-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
  0 siblings, 1 reply; 4+ messages in thread
From: Jean Delvare @ 2008-05-20 13:52 UTC (permalink / raw)
  To: David Brownell; +Cc: i2c-GZX6beZjE8VD60Wz+7aTrA

Hi David,

On Sun, 4 May 2008 21:24:45 -0700, David Brownell wrote:
> 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 <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> ---
> 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,
-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [patch/rft 2.6.26-rc1] i2c-core: stop using i2c_adapter.clients
       [not found]     ` <20080520155232.4937c52a-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
@ 2008-05-20 16:22       ` David Brownell
       [not found]         ` <200805200922.19990.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
  0 siblings, 1 reply; 4+ messages in thread
From: David Brownell @ 2008-05-20 16:22 UTC (permalink / raw)
  To: Jean Delvare; +Cc: i2c-GZX6beZjE8VD60Wz+7aTrA

On Tuesday 20 May 2008, Jean Delvare wrote:
> 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?


> 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...

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


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



_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [patch/rft 2.6.26-rc1] i2c-core: stop using i2c_adapter.clients
       [not found]         ` <200805200922.19990.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
@ 2008-05-20 18:15           ` Jean Delvare
  0 siblings, 0 replies; 4+ messages in thread
From: Jean Delvare @ 2008-05-20 18:15 UTC (permalink / raw)
  To: David Brownell; +Cc: i2c-GZX6beZjE8VD60Wz+7aTrA

Hi David,

On Tue, 20 May 2008 09:22:19 -0700, David Brownell wrote:
> On Tuesday 20 May 2008, Jean Delvare wrote:
> > 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.

> 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.

> > 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...
> 
> Yeah, that's why I have various i2c lock removal patches
> floating around.

Thanks,
-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2008-05-20 18:15 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-05-05  4:24 [patch/rft 2.6.26-rc1] i2c-core: stop using i2c_adapter.clients David Brownell
     [not found] ` <200805042124.45551.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-05-20 13:52   ` Jean Delvare
     [not found]     ` <20080520155232.4937c52a-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2008-05-20 16:22       ` David Brownell
     [not found]         ` <200805200922.19990.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
2008-05-20 18:15           ` Jean Delvare

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox