* [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