[patch 2.6.28-rc5] i2c: remove i2c_adapter.clist_lock

[patch 2.6.28-rc5] i2c: remove i2c_adapter.clist_lock

[David Brownell]

From: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>

Move list add/remove operations now protected by i2c_adapter.clist_lock
into the driver model probe() and remove() calls, where driver model
locking protects them.  This is a minor code and data shrink.

Note that the adapter "child list" is still protected inconsistently;
this patch just simplifies locking, and makes this list more directly
mirror the one maintained by the driver model.  Removing this list may
need to wait until the legacy I2C driver binding model is removed.

Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
 drivers/i2c/i2c-core.c |   17 ++++-------------
 include/linux/i2c.h    |    1 -
 2 files changed, 4 insertions(+), 14 deletions(-)

--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -116,6 +116,8 @@ static int i2c_device_probe(struct devic
 	status = driver->probe(client, i2c_match_id(driver->id_table, client));
 	if (status)
 		client->driver = NULL;
+	else
+		list_add_tail(&client->list, &client->adapter->clients);
 	return status;
 }
 
@@ -125,6 +127,8 @@ static int i2c_device_remove(struct devi
 	struct i2c_driver	*driver;
 	int			status;
 
+	list_del(&client->list);
+
 	if (!dev->driver)
 		return 0;
 
@@ -315,10 +319,6 @@ void i2c_unregister_device(struct i2c_cl
 		}
 	}
 
-	mutex_lock(&adapter->clist_lock);
-	list_del(&client->list);
-	mutex_unlock(&adapter->clist_lock);
-
 	device_unregister(&client->dev);
 }
 EXPORT_SYMBOL_GPL(i2c_unregister_device);
@@ -445,7 +445,6 @@ static int i2c_register_adapter(struct i
 		return -EAGAIN;
 
 	mutex_init(&adap->bus_lock);
-	mutex_init(&adap->clist_lock);
 	INIT_LIST_HEAD(&adap->clients);
 
 	mutex_lock(&core_lock);
@@ -851,10 +850,6 @@ int i2c_attach_client(struct i2c_client 
 	if (res)
 		goto out_err;
 
-	mutex_lock(&adapter->clist_lock);
-	list_add_tail(&client->list, &adapter->clients);
-	mutex_unlock(&adapter->clist_lock);
-
 	dev_dbg(&adapter->dev, "client [%s] registered with bus id %s\n",
 		client->name, client->dev.bus_id);
 
@@ -890,10 +885,6 @@ int i2c_detach_client(struct i2c_client 
 		}
 	}
 
-	mutex_lock(&adapter->clist_lock);
-	list_del(&client->list);
-	mutex_unlock(&adapter->clist_lock);
-
 	init_completion(&client->released);
 	device_unregister(&client->dev);
 	wait_for_completion(&client->released);
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -359,7 +359,6 @@ struct i2c_adapter {
 	/* data fields that are valid for all devices	*/
 	u8 level; 			/* nesting level for lockdep */
 	struct mutex bus_lock;
-	struct mutex clist_lock;
 
 	int timeout;
 	int retries;

Re: [patch 2.6.28-rc5] i2c: remove i2c_adapter.clist_lock

[Jean Delvare]

Hi David,

On Thu, 20 Nov 2008 14:38:08 -0800, David Brownell wrote:
> From: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> 
> Move list add/remove operations now protected by i2c_adapter.clist_lock
> into the driver model probe() and remove() calls, where driver model
> locking protects them.  This is a minor code and data shrink.
> 
> Note that the adapter "child list" is still protected inconsistently;
> this patch just simplifies locking, and makes this list more directly
> mirror the one maintained by the driver model.  Removing this list may
> need to wait until the legacy I2C driver binding model is removed.
> 
> Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> ---
>  drivers/i2c/i2c-core.c |   17 ++++-------------
>  include/linux/i2c.h    |    1 -
>  2 files changed, 4 insertions(+), 14 deletions(-)

I'm not sure I want to take this change at this point. Call me chicken,
but I seem to recall problems with legacy i2c clients last time we
tried to clean up this area. So I'd rather work on killing the legacy
binding model now, and once this is done, we can look into i2c-core
cleanups again.

The list of i2c drivers that still need to be converted can be found
here:
http://jdelvare.pck.nerim.net/linux/legacy_i2c_driver.list

Drivers that are neither marked "*" (done) nor "-" (deleted) are left
to convert.

Thanks,
-- 
Jean Delvare

Re: [patch 2.6.28-rc5] i2c: remove i2c_adapter.clist_lock

[David Brownell]

On Tuesday 25 November 2008, Jean Delvare wrote:
> I'm not sure I want to take this change at this point. Call me chicken,
> but I seem to recall problems with legacy i2c clients last time we
> tried to clean up this area.

I did test this with the legacy "eeprom" and "i2c-stub" as you had
suggested.  Worked fine ... this is different from previous passes,
as it removes the lock instead of the list, and doesn't attempt to
change how the list is (mis/ab)used.

But if you prefer to wait until the list can go too, OK.

- Dave

Re: [patch 2.6.28-rc5] i2c: remove i2c_adapter.clist_lock

[Jean Delvare]

On Tue, 25 Nov 2008 02:24:01 -0800, David Brownell wrote:
> On Tuesday 25 November 2008, Jean Delvare wrote:
> > I'm not sure I want to take this change at this point. Call me chicken,
> > but I seem to recall problems with legacy i2c clients last time we
> > tried to clean up this area.
> 
> I did test this with the legacy "eeprom" and "i2c-stub" as you had
> suggested.  Worked fine ... this is different from previous passes,
> as it removes the lock instead of the list, and doesn't attempt to
> change how the list is (mis/ab)used.

Which kernel version? Since 2.6.27, the eeprom driver is no longer a
legacy driver. Instead it's a new-style driver with the
optional .detect() callback.

This is why I am reluctant to change this now: there aren't too many
legacy drivers left, so testing their code paths isn't easy, and if
anything isn't correct we might not notice it until it hits the users.

> But if you prefer to wait until the list can go too, OK.

Yes, please.

-- 
Jean Delvare

Re: [patch 2.6.28-rc5] i2c: remove i2c_adapter.clist_lock

[David Brownell]

On Tuesday 25 November 2008, Jean Delvare wrote:
> 
> > I did test this with the legacy "eeprom" and "i2c-stub" as you had
> > suggested.  Worked fine ... this is different from previous passes,
> > as it removes the lock instead of the list, and doesn't attempt to
> > change how the list is (mis/ab)used.
> 
> Which kernel version? Since 2.6.27, the eeprom driver is no longer a
> legacy driver. Instead it's a new-style driver with the
> optional .detect() callback.

2.6.28-rc6 ... hmm, seeing the top of that driver with

  /* Addresses to scan */
  static const unsigned short normal_i2c[] = { 0x50, 0x51, 0x52, 0x53, 0x54,
                                        0x55, 0x56, 0x57, I2C_CLIENT_END };

  /* Insmod parameters */
  I2C_CLIENT_INSMOD_1(eeprom);

strongly implied that it's still a legacy driver.  But
at the bottom, I see otherwise.  I suppose your "how to
test such stuff" message predated 2.6.27 ... :)

Re: [patch 2.6.28-rc5] i2c: remove i2c_adapter.clist_lock

[Jean Delvare]

On Tue, 25 Nov 2008 03:27:08 -0800, David Brownell wrote:
> On Tuesday 25 November 2008, Jean Delvare wrote:
> > 
> > > I did test this with the legacy "eeprom" and "i2c-stub" as you had
> > > suggested.  Worked fine ... this is different from previous passes,
> > > as it removes the lock instead of the list, and doesn't attempt to
> > > change how the list is (mis/ab)used.
> > 
> > Which kernel version? Since 2.6.27, the eeprom driver is no longer a
> > legacy driver. Instead it's a new-style driver with the
> > optional .detect() callback.
> 
> 2.6.28-rc6 ... hmm, seeing the top of that driver with
> 
>   /* Addresses to scan */
>   static const unsigned short normal_i2c[] = { 0x50, 0x51, 0x52, 0x53, 0x54,
>                                         0x55, 0x56, 0x57, I2C_CLIENT_END };
> 
>   /* Insmod parameters */
>   I2C_CLIENT_INSMOD_1(eeprom);
> 
> strongly implied that it's still a legacy driver.  But
> at the bottom, I see otherwise.

I voluntarily made the .detect() callback reuse the same data
structures, to make the transition easier. I still hope to be able to
kill the I2C_CLIENT_INSMOD*() macros someday though.

> (...) I suppose your "how to
> test such stuff" message predated 2.6.27 ... :)

Definitely.

-- 
Jean Delvare