Re: [patch 2.6.24-rc5-git] add i2c_new_dummy() utility

Re: [patch 2.6.24-rc5-git] add i2c_new_dummy() utility

[Jean Delvare]

Hi David,

Sorry for the non-threaded reply, I do not have access to the original
post at the moment.

David Brownell wrote:
> This adds a i2c_new_dummy() primitive to help work with devices
> that consume multiple addresses, which include many I2C eeproms
> and at least one RTC.
> 
> Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> ---
> [ Apologies for the duplicate message some of you may have seen. ]
> 
> Tested with a 24c00 EEPROM ... that's the 16-byte one, which
> ignores a0/a1/a2 and thus consumes 8 addresses.
> 
>  drivers/i2c/i2c-core.c |   60 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/i2c.h    |    6 ++++
>  2 files changed, 65 insertions(+), 1 deletion(-)
> 
> --- a/drivers/i2c/i2c-core.c	2007-12-15 20:57:54.000000000 -0800
> +++ b/drivers/i2c/i2c-core.c	2007-12-15 21:01:22.000000000 -0800
> @@ -276,6 +276,51 @@ void i2c_unregister_device(struct i2c_cl
>  EXPORT_SYMBOL_GPL(i2c_unregister_device);
>  
>  
> +static int dummy_nop(struct i2c_client *client)
> +{
> +	return 0;
> +}
> +
> +static struct i2c_driver dummy_driver = {
> +	.driver.name	= "dummy",
> +	.probe		= dummy_nop,
> +	.remove		= dummy_nop,
> +};
> +
> +
> +/**
> + * i2c_new_dummy - return a new i2c device bound to a dummy driver
> + * @adapter: the adapter managing the device
> + * @address: seven bit address to be used
> + * @type: optional label used for i2c_client.name

Do you have a use case in mind for this feature?

> + * Context: can sleep
> + *
> + * This returns an I2C client bound to the "dummy" driver, intended for use
> + * with devices that consume multiple addresses.  Examples of such chips
> + * include various EEPROMS (like 24c04 and 24c08 models).
> + *
> + * These dummy devices have two main uses.  First, most I2C and SMBus calls
> + * except i2c_transfer() need a client handle; the dummy will be that handle.
> + * And second, this prevents the specified address from being bound to a
> + * different driver.
> + *
> + * This returns the new i2c client, which may be saved for later use with
> + * i2c_unregister_device(); or NULL to indicate an error.

I'd say it _should_ be saved. Calling i2c_unregister_device() is pretty
much mandatory, unless the driver can't be built as a module.

> + */
> +struct i2c_client *
> +i2c_new_dummy(struct i2c_adapter *adapter, unsigned address, const char *type)

The rest of i2c-core uses an unsigned short for addresses, maybe do the
same here for consistency?

> +{
> +	struct i2c_board_info info = {
> +		.driver_name	= "dummy",
> +		.addr		= address,
> +	};
> +
> +	if (type)
> +		strncpy(info.type, type, sizeof info.type);

Please use strlcpy() instead, strncpy() is error-prone.

> +	return i2c_new_device(adapter, &info);
> +}
> +EXPORT_SYMBOL_GPL(i2c_new_dummy);
> +
>  /* ------------------------------------------------------------------------- */
>  
>  /* I2C bus adapters -- one roots each I2C or SMBUS segment */
> @@ -848,11 +893,24 @@ static int __init i2c_init(void)
>  	retval = bus_register(&i2c_bus_type);
>  	if (retval)
>  		return retval;
> -	return class_register(&i2c_adapter_class);
> +	retval = class_register(&i2c_adapter_class);
> +	if (retval)
> +		goto bus_err;
> +	retval = i2c_add_driver(&dummy_driver);
> +	if (retval)
> +		goto class_err;
> +	return 0;
> +
> +class_err:
> +	class_unregister(&i2c_adapter_class);
> +bus_err:
> +	bus_unregister(&i2c_bus_type);
> +	return retval;
>  }

Thanks for fixing a broken error path...

>  
>  static void __exit i2c_exit(void)
>  {
> +	i2c_del_driver(&dummy_driver);
>  	class_unregister(&i2c_adapter_class);
>  	bus_unregister(&i2c_bus_type);
>  }
> --- a/include/linux/i2c.h	2007-12-15 20:57:54.000000000 -0800
> +++ b/include/linux/i2c.h	2007-12-15 21:01:22.000000000 -0800
> @@ -261,6 +261,12 @@ i2c_new_probed_device(struct i2c_adapter
>  		      struct i2c_board_info *info,
>  		      unsigned short const *addr_list);
>  
> +/* For devices that use several addresses, use i2c_new_dummy() to make
> + * client handles for the exta addresses.

Typo: extra.

> + */
> +extern struct i2c_client *
> +i2c_new_dummy(struct i2c_adapter *adap, unsigned address, const char *type);
> +
>  extern void i2c_unregister_device(struct i2c_client *);
>  
>  /* Mainboard arch_initcall() code should register all its I2C devices.
> 

Other than that the patch looks just fine. Please respin it and I'll
apply it.

Thanks,
--
Jean Delvare

Re: [patch 2.6.24-rc5-git] add i2c_new_dummy() utility

[David Brownell]

On Saturday 22 December 2007, Jean Delvare wrote:

> > +/**
> > + * i2c_new_dummy - return a new i2c device bound to a dummy driver
> > + * @adapter: the adapter managing the device
> > + * @address: seven bit address to be used
> > + * @type: optional label used for i2c_client.name
> 
> Do you have a use case in mind for this feature?

You mean the "type" field?  In the at24.c driver I posted, it's
the same name used with the "real" i2c_client node.  That helps
make sense of just what the "dummy" is for; sysfs can show it.
Being tied to a "dummy" driver is not very explanatory.


> > + * This returns the new i2c client, which may be saved for later use with
> > + * i2c_unregister_device(); or NULL to indicate an error.
> 
> I'd say it _should_ be saved. Calling i2c_unregister_device() is pretty
> much mandatory, unless the driver can't be built as a module.

OK by me, "which should be saved".


> > + */
> > +struct i2c_client *
> > +i2c_new_dummy(struct i2c_adapter *adapter, unsigned address, const char *type)
> 
> The rest of i2c-core uses an unsigned short for addresses, maybe do the
> same here for consistency?

It's promoted by C!  But no problem.  I'll make it u16 to match
the SMBus calls and struct i2c_msg ... and be more concise, still
fitting on one line.


> > +{
> > +	struct i2c_board_info info = {
> > +		.driver_name	= "dummy",
> > +		.addr		= address,
> > +	};
> > +
> > +	if (type)
> > +		strncpy(info.type, type, sizeof info.type);
> 
> Please use strlcpy() instead, strncpy() is error-prone.

OK.


> > @@ -848,11 +893,24 @@ static int __init i2c_init(void)
> >  	retval = bus_register(&i2c_bus_type);
> >  	if (retval)
> >  		return retval;
> > -	return class_register(&i2c_adapter_class);
> > +	retval = class_register(&i2c_adapter_class);
> > +	if (retval)
> > +		goto bus_err;
> > +	retval = i2c_add_driver(&dummy_driver);
> > +	if (retval)
> > +		goto class_err;
> > +	return 0;
> > +
> > +class_err:
> > +	class_unregister(&i2c_adapter_class);
> > +bus_err:
> > +	bus_unregister(&i2c_bus_type);
> > +	return retval;
> >  }
> 
> Thanks for fixing a broken error path...

I could hardly avoid it!  :)


> > --- a/include/linux/i2c.h	2007-12-15 20:57:54.000000000 -0800
> > +++ b/include/linux/i2c.h	2007-12-15 21:01:22.000000000 -0800
> > @@ -261,6 +261,12 @@ i2c_new_probed_device(struct i2c_adapter
> >  		      struct i2c_board_info *info,
> >  		      unsigned short const *addr_list);
> >  
> > +/* For devices that use several addresses, use i2c_new_dummy() to make
> > + * client handles for the exta addresses.
> 
> Typo: extra.

Glitchey keyboard.  :(


> > + */
> > +extern struct i2c_client *
> > +i2c_new_dummy(struct i2c_adapter *adap, unsigned address, const char *type);
> > +
> >  extern void i2c_unregister_device(struct i2c_client *);
> >  
> >  /* Mainboard arch_initcall() code should register all its I2C devices.
> > 
> 
> Other than that the patch looks just fine. Please respin it and I'll
> apply it.

Thanks.  Update to follow.

- Dave

Re: [patch 2.6.24-rc5-git] add i2c_new_dummy() utility

[David Brownell]

This adds a i2c_new_dummy() primitive to help work with devices
that consume multiple addresses, which include many I2C eeproms
and at least one RTC.

Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
Updated to match Jean's feedback.  Tested with 24c00 EEPROM,
which takes eight addresses (but stores only 16 bytes total).

 drivers/i2c/i2c-core.c |   60 ++++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/i2c.h    |    6 ++++
 2 files changed, 65 insertions(+), 1 deletion(-)

--- a/drivers/i2c/i2c-core.c	2007-12-22 13:54:04.000000000 -0800
+++ b/drivers/i2c/i2c-core.c	2007-12-22 13:57:15.000000000 -0800
@@ -276,6 +276,51 @@ void i2c_unregister_device(struct i2c_cl
 EXPORT_SYMBOL_GPL(i2c_unregister_device);
 
 
+static int dummy_nop(struct i2c_client *client)
+{
+	return 0;
+}
+
+static struct i2c_driver dummy_driver = {
+	.driver.name	= "dummy",
+	.probe		= dummy_nop,
+	.remove		= dummy_nop,
+};
+
+
+/**
+ * i2c_new_dummy - return a new i2c device bound to a dummy driver
+ * @adapter: the adapter managing the device
+ * @address: seven bit address to be used
+ * @type: optional label used for i2c_client.name
+ * Context: can sleep
+ *
+ * This returns an I2C client bound to the "dummy" driver, intended for use
+ * with devices that consume multiple addresses.  Examples of such chips
+ * include various EEPROMS (like 24c04 and 24c08 models).
+ *
+ * These dummy devices have two main uses.  First, most I2C and SMBus calls
+ * except i2c_transfer() need a client handle; the dummy will be that handle.
+ * And second, this prevents the specified address from being bound to a
+ * different driver.
+ *
+ * This returns the new i2c client, which should be saved for later use with
+ * i2c_unregister_device(); or NULL to indicate an error.
+ */
+struct i2c_client *
+i2c_new_dummy(struct i2c_adapter *adapter, u16 address, const char *type)
+{
+	struct i2c_board_info info = {
+		.driver_name	= "dummy",
+		.addr		= address,
+	};
+
+	if (type)
+		strlcpy(info.type, type, sizeof info.type);
+	return i2c_new_device(adapter, &info);
+}
+EXPORT_SYMBOL_GPL(i2c_new_dummy);
+
 /* ------------------------------------------------------------------------- */
 
 /* I2C bus adapters -- one roots each I2C or SMBUS segment */
@@ -848,11 +893,24 @@ static int __init i2c_init(void)
 	retval = bus_register(&i2c_bus_type);
 	if (retval)
 		return retval;
-	return class_register(&i2c_adapter_class);
+	retval = class_register(&i2c_adapter_class);
+	if (retval)
+		goto bus_err;
+	retval = i2c_add_driver(&dummy_driver);
+	if (retval)
+		goto class_err;
+	return 0;
+
+class_err:
+	class_unregister(&i2c_adapter_class);
+bus_err:
+	bus_unregister(&i2c_bus_type);
+	return retval;
 }
 
 static void __exit i2c_exit(void)
 {
+	i2c_del_driver(&dummy_driver);
 	class_unregister(&i2c_adapter_class);
 	bus_unregister(&i2c_bus_type);
 }
--- a/include/linux/i2c.h	2007-12-22 13:54:04.000000000 -0800
+++ b/include/linux/i2c.h	2007-12-22 13:57:15.000000000 -0800
@@ -261,6 +261,12 @@ i2c_new_probed_device(struct i2c_adapter
 		      struct i2c_board_info *info,
 		      unsigned short const *addr_list);
 
+/* For devices that use several addresses, use i2c_new_dummy() to make
+ * client handles for the extra addresses.
+ */
+extern struct i2c_client *
+i2c_new_dummy(struct i2c_adapter *adap, u16 address, const char *type);
+
 extern void i2c_unregister_device(struct i2c_client *);
 
 /* Mainboard arch_initcall() code should register all its I2C devices.

Re: [patch 2.6.24-rc5-git] add i2c_new_dummy() utility

[Jean Delvare]

HI David,

Le 23/12/2007, David Brownell a écrit:
>This adds a i2c_new_dummy() primitive to help work with devices
>that consume multiple addresses, which include many I2C eeproms
>and at least one RTC.
>
>Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
>---
>Updated to match Jean's feedback.  Tested with 24c00 EEPROM,
>which takes eight addresses (but stores only 16 bytes total).

Applied, thanks.

--
Jean Delvare

Re: [patch 2.6.24-rc5-git] add i2c_new_dummy() utility

[Byron Bradley]

On Dec 16, 2007 5:23 AM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> This adds a i2c_new_dummy() primitive to help work with devices
> that consume multiple addresses, which include many I2C eeproms
> and at least one RTC.
>
> Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>

For the S35390A RTC driver I called i2c_new_dummy() in the probe
function in a similar style to the at24 eeprom driver. This failed
because the probe function is called inside an i2c_attach_device()
which has already locked &adap->clist_lock. When you call the
i2c_new_dummy() function it will itself call i2c_attach_device() but
it will never be able to acquire &adap->clist_lock.

Below is the lock detection and backtrace.

=============================================
[ INFO: possible recursive locking detected ]
2.6.24-rc5-g7fffe9cc-dirty #38
---------------------------------------------
swapper/1 is trying to acquire lock:
 (&adap->clist_lock){--..}, at: [<c02b1ba4>] i2c_attach_client+0x28/0x1e8

but task is already holding lock:
 (&adap->clist_lock){--..}, at: [<c02b1ba4>] i2c_attach_client+0x28/0x1e8

other info that might help us debug this:
3 locks held by swapper/1:
 #0:  (core_lists){--..}, at: [<c02b2294>] i2c_register_adapter+0x50/0x26c
 #1:  (__i2c_board_lock){--..}, at: [<c02b23c4>]
i2c_register_adapter+0x180/0x26c
 #2:  (&adap->clist_lock){--..}, at: [<c02b1ba4>] i2c_attach_client+0x28/0x1e8

stack backtrace:
[<c002df24>] (dump_stack+0x0/0x14) from [<c0067330>]
(__lock_acquire+0x92c/0x10cc)
[<c0066a04>] (__lock_acquire+0x0/0x10cc) from [<c0067b38>]
(lock_acquire+0x68/0x80)
[<c0067ad0>] (lock_acquire+0x0/0x80) from [<c035b880>]
(mutex_lock_nested+0x9c/0x2cc) r7:c7c16000 r6:c7c18000 r5:60000013
r4:c7e0e4e8
[<c035b7e4>] (mutex_lock_nested+0x0/0x2cc) from [<c02b1ba4>]
(i2c_attach_client+0x28/0x1e8)
[<c02b1b7c>] (i2c_attach_client+0x0/0x1e8) from [<c02b2204>]
(i2c_new_device+0xa8/0xe8)
[<c02b215c>] (i2c_new_device+0x0/0xe8) from [<c02b26cc>]
(i2c_new_dummy+0xb0/0xdc) r6:c7e0e488 r5:c036f848 r4:00000000
[<c02b261c>] (i2c_new_dummy+0x0/0xdc) from [<c02b0558>]
(s35390a_probe+0x9c/0x330) r6:c7e0e800 r5:c7cbab00 r4:00000001
[<c02b04bc>] (s35390a_probe+0x0/0x330) from [<c02b0b08>]
(i2c_device_probe+0x54/0x5c) r7:c0232b64 r6:c044721c r5:c7e0e828
r4:c7e0e800
[<c02b0ab4>] (i2c_device_probe+0x0/0x5c) from [<c0232a50>]
(driver_probe_device+0xac/0x1c0) r6:c0447248 r5:c7c19d0c r4:c7e0e828
[<c02329a4>] (driver_probe_device+0x0/0x1c0) from [<c0232b74>]
(__device_attach+0x10/0x14) r8:c7e0e628 r7:c0232b64 r6:c7e0e828
r5:c7c19d0c r4:00000000
[<c0232b64>] (__device_attach+0x0/0x14) from [<c0231b94>]
(bus_for_each_drv+0x5c/0x88)
[<c0231b38>] (bus_for_each_drv+0x0/0x88) from [<c0232c48>]
(device_attach+0xa4/0xb0) r7:c7e0e530 r6:00000000 r5:c7e0e93c
r4:c7e0e828
[<c0232ba4>] (device_attach+0x0/0xb0) from [<c0231af8>]
(bus_attach_device+0x48/0x88) r5:c7e0e828 r4:c04473b0
[<c0231ab0>] (bus_attach_device+0x0/0x88) from [<c0230710>]
(device_add+0x470/0x5e8) r5:c7e0e828 r4:c7e0e828
[<c02302a0>] (device_add+0x0/0x5e8) from [<c02308a8>]
(device_register+0x20/0x24)
[<c0230888>] (device_register+0x0/0x24) from [<c02b1c6c>]
(i2c_attach_client+0xf0/0x1e8) r4:c7e0e920
[<c02b1b7c>] (i2c_attach_client+0x0/0x1e8) from [<c02b2204>]
(i2c_new_device+0xa8/0xe8)
[<c02b215c>] (i2c_new_device+0x0/0xe8) from [<c02b240c>]
(i2c_register_adapter+0x1c8/0x26c) r6:c044733c r5:c7e0e488 r4:c7c0a240
[<c02b2244>] (i2c_register_adapter+0x0/0x26c) from [<c02b256c>]
(i2c_add_numbered_adapter+0xbc/0x) r8:c042df28 r7:c042df34 r6:00000020
r5:c7e0e488 r4:00000000
[<c02b24b0>] (i2c_add_numbered_adapter+0x0/0xcc) from [<c02b467c>]
(mv64xxx_i2c_probe+0x174/0x228) r5:c042dc28 r4:c7e0e400
[<c02b4508>] (mv64xxx_i2c_probe+0x0/0x228) from [<c02344dc>]
(platform_drv_probe+0x20/0x24)
[<c02344bc>] (platform_drv_probe+0x0/0x24) from [<c0232a50>]
(driver_probe_device+0xac/0x1c0)
[<c02329a4>] (driver_probe_device+0x0/0x1c0) from [<c0232d58>]
(__driver_attach+0x104/0x10c) r8:c04421a8 r7:c0232c54 r6:c04478f4
r5:c042dc30 r4:c042dd44
[<c0232c54>] (__driver_attach+0x0/0x10c) from [<c0231d00>]
(bus_for_each_dev+0x54/0x80) r6:c04478f4 r5:c7c19ef4 r4:00000000
[<c0231cac>] (bus_for_each_dev+0x0/0x80) from [<c02328b8>]
(driver_attach+0x20/0x28) r7:c0023eb4 r6:00000000 r5:c04478f4
r4:c04478fc
[<c0232898>] (driver_attach+0x0/0x28) from [<c0232104>]
(bus_add_driver+0x84/0x1e0)
[<c0232080>] (bus_add_driver+0x0/0x1e0) from [<c0232fac>]
(driver_register+0x54/0x90) r8:c7c18000 r7:c0023eb4 r6:00000000
r5:00000000 r4:c04478f4
[<c0232f58>] (driver_register+0x0/0x90) from [<c0234768>]
(platform_driver_register+0x6c/0x88) r4:00000000
[<c02346fc>] (platform_driver_register+0x0/0x88) from [<c001e454>]
(mv64xxx_i2c_init+0x14/0x1c)
[<c001e440>] (mv64xxx_i2c_init+0x0/0x1c) from [<c0008958>]
(kernel_init+0x98/0x2ac)
[<c00088c0>] (kernel_init+0x0/0x2ac) from [<c00459b8>] (do_exit+0x0/0x810)

-- 
Byron Bradley

Re: [patch 2.6.24-rc5-git] add i2c_new_dummy() utility

[David Brownell]

On Thursday 27 December 2007, Byron Bradley wrote:
> On Dec 16, 2007 5:23 AM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> > This adds a i2c_new_dummy() primitive to help work with devices
> > that consume multiple addresses, which include many I2C eeproms
> > and at least one RTC.
> >
> > Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> 
> For the S35390A RTC driver I called i2c_new_dummy() in the probe
> function in a similar style to the at24 eeprom driver. This failed
> because the probe function is called inside an i2c_attach_device()
> which has already locked &adap->clist_lock. When you call the
> i2c_new_dummy() function it will itself call i2c_attach_device() but
> it will never be able to acquire &adap->clist_lock.

Right ... make the s35390a driver be a new-style driver.

Or, Jean has patches in his queue [1] to remove other lists
from i2c_core which duplicate driver model core behavior.
I think i2c_adapter.children and i2c_adapter.clist_lock have
not yet been removed, even though they're redundant given the
driver model's device.klist_children records.

So if you're ambitious and don't want to just make that RTC
driver be a new-style driver, you could just get rid of that
functionality duplication.  (Me, I'd take the easier route and
just make it be a new-style driver!)

- Dave

[1] http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/

Re: [patch 2.6.24-rc5-git] add i2c_new_dummy() utility

[Byron Bradley]

On Dec 27, 2007 9:53 PM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> On Thursday 27 December 2007, Byron Bradley wrote:
> > On Dec 16, 2007 5:23 AM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> > > This adds a i2c_new_dummy() primitive to help work with devices
> > > that consume multiple addresses, which include many I2C eeproms
> > > and at least one RTC.
> > >
> > > Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> >
> > For the S35390A RTC driver I called i2c_new_dummy() in the probe
> > function in a similar style to the at24 eeprom driver. This failed
> > because the probe function is called inside an i2c_attach_device()
> > which has already locked &adap->clist_lock. When you call the
> > i2c_new_dummy() function it will itself call i2c_attach_device() but
> > it will never be able to acquire &adap->clist_lock.
>
> Right ... make the s35390a driver be a new-style driver.
>
> Or, Jean has patches in his queue [1] to remove other lists
> from i2c_core which duplicate driver model core behavior.
> I think i2c_adapter.children and i2c_adapter.clist_lock have
> not yet been removed, even though they're redundant given the
> driver model's device.klist_children records.
>
> So if you're ambitious and don't want to just make that RTC
> driver be a new-style driver, you could just get rid of that
> functionality duplication.  (Me, I'd take the easier route and
> just make it be a new-style driver!)

I believe this is already a new-style driver. It uses the RTC
framework, the module init calls i2c_add_driver() and the struct
i2c_driver provides a .probe and a .remove function. Is there
something else that I'm missing?

Thanks,

-- 
Byron Bradley

Re: [patch 2.6.24-rc5-git] add i2c_new_dummy() utility

[David Brownell]

On Thursday 27 December 2007, Byron Bradley wrote:
> On Dec 27, 2007 9:53 PM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> > On Thursday 27 December 2007, Byron Bradley wrote:
> > > On Dec 16, 2007 5:23 AM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> > > > This adds a i2c_new_dummy() primitive to help work with devices
> > > > that consume multiple addresses, which include many I2C eeproms
> > > > and at least one RTC.
> > > >
> > > > Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> > >
> > > For the S35390A RTC driver I called i2c_new_dummy() in the probe
> > > function in a similar style to the at24 eeprom driver. This failed
> > > because the probe function is called inside an i2c_attach_device()

You mean i2c_attach_client() ...

> > > which has already locked &adap->clist_lock. When you call the
> > > i2c_new_dummy() function it will itself call i2c_attach_device() but
> > > it will never be able to acquire &adap->clist_lock.
> >
> > Right ... make the s35390a driver be a new-style driver.
> >
> > ...
> >
> > So if you're ambitious and don't want to just make that RTC
> > driver be a new-style driver, you could just get rid of that
> > functionality duplication.  (Me, I'd take the easier route and
> > just make it be a new-style driver!)
> 
> I believe this is already a new-style driver. It uses the RTC
> framework, the module init calls i2c_add_driver() and the struct
> i2c_driver provides a .probe and a .remove function.

Indeed; sorry, I skimmed over that stacktrace too quickly.


> Is there something else that I'm missing?

I'm not sure yet.  I certainly tested that updated at24 driver
with lockdep active, and it didn't report such a warning.  It's
possible lockdep on that platform isn't fully functional yet;
I'll see if that problem triggers on a different arch (after
some intentional misconfiguration to call this utility).

One thing that looks odd in your stack trace is that it shows
i2c_attach_client() calling mutex_lock_nested(), which is most
certainly not what it does in rc6-git-current ... in fact, a
nested call is made (rather curiously) only in i2c_transfer().

Do you have other I2C patches that may be causing problems?

- Dave

Re: [patch 2.6.24-rc5-git] add i2c_new_dummy() utility

[Byron Bradley]

On Dec 27, 2007 11:06 PM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> On Thursday 27 December 2007, Byron Bradley wrote:
> > On Dec 27, 2007 9:53 PM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> > > On Thursday 27 December 2007, Byron Bradley wrote:
> > > > On Dec 16, 2007 5:23 AM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> > > > > This adds a i2c_new_dummy() primitive to help work with devices
> > > > > that consume multiple addresses, which include many I2C eeproms
> > > > > and at least one RTC.
> > > > >
> > > > > Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> > > >
> > > > For the S35390A RTC driver I called i2c_new_dummy() in the probe
> > > > function in a similar style to the at24 eeprom driver. This failed
> > > > because the probe function is called inside an i2c_attach_device()
>
> You mean i2c_attach_client() ...

Yes sorry, not sure where I got device from.

> > > > which has already locked &adap->clist_lock. When you call the
> > > > i2c_new_dummy() function it will itself call i2c_attach_device() but
> > > > it will never be able to acquire &adap->clist_lock.
> > >
> > > Right ... make the s35390a driver be a new-style driver.
> > >
> > > ...
> > >
> > > So if you're ambitious and don't want to just make that RTC
> > > driver be a new-style driver, you could just get rid of that
> > > functionality duplication.  (Me, I'd take the easier route and
> > > just make it be a new-style driver!)
> >
> > I believe this is already a new-style driver. It uses the RTC
> > framework, the module init calls i2c_add_driver() and the struct
> > i2c_driver provides a .probe and a .remove function.
>
> Indeed; sorry, I skimmed over that stacktrace too quickly.
>
>
> > Is there something else that I'm missing?
>
> I'm not sure yet.  I certainly tested that updated at24 driver
> with lockdep active, and it didn't report such a warning.  It's
> possible lockdep on that platform isn't fully functional yet;
> I'll see if that problem triggers on a different arch (after
> some intentional misconfiguration to call this utility).
>
> One thing that looks odd in your stack trace is that it shows
> i2c_attach_client() calling mutex_lock_nested(), which is most
> certainly not what it does in rc6-git-current ... in fact, a
> nested call is made (rather curiously) only in i2c_transfer().

I'm using the Orion git repo
(http://git.kernel.org/?p=linux/kernel/git/nico/orion.git;a=summary)
which is being rebased but is currently on 2.6.24-rc5. I will try the
latest 2.6-git with the Orion patches and see if that fixes anything.

> Do you have other I2C patches that may be causing problems?

No other I2C patches.

Cheers,

-- 
Byron Bradley

Re: [patch 2.6.24-rc5-git] add i2c_new_dummy() utility

[David Brownell]

On Thursday 27 December 2007, David Brownell wrote:
> > Is there something else that I'm missing?
> 
> I'm not sure yet.  I certainly tested that updated at24 driver
> with lockdep active, and it didn't report such a warning.  It's
> possible lockdep on that platform isn't fully functional yet;
> I'll see if that problem triggers on a different arch (after
> some intentional misconfiguration to call this utility).

Just tried it on ARM, and it worked just fine.  Lockdep reported
no warnings, and that hacked 24c512 EEPROM claims three extra
addresses using i2c_new_dummy().

- Dave


> One thing that looks odd in your stack trace is that it shows
> i2c_attach_client() calling mutex_lock_nested(), which is most
> certainly not what it does in rc6-git-current ... in fact, a
> nested call is made (rather curiously) only in i2c_transfer().
> 
> Do you have other I2C patches that may be causing problems?

Re: [patch 2.6.24-rc5-git] add i2c_new_dummy() utility

[David Brownell]

> > OK, but why don't I get the warnings you get?
> >
> > That's not just on ARM, note.
> >
> >
> > Making i2c_attach_client() not use the lock that way should be
> > easy enough, I'll have a look at that, but I'm concerned about
> > the seeming lockdep misbehavior here... as in, if it gets this
> > wrong then how else is it broken and what other bugs has it
> > been hiding?

I still don't know why you get lockdep warnings and I don't ...
but here's a patch that should help, by removing that lock!  :)

- Dave


================
Remove further duplication between i2c core and driver model:  the
per-adapter list of clients (adapter->clients,  client->list) and
its lock (adapter->clist_lock) duplicate adapter->dev.children.
 
LIGHTLY TESTED ... goes on top of two patches from Jean's I2C queue,

	i2c-remove-redundant-i2c_adapter-list.patch
	i2c-remove-redundant-i2c_driver-list.patch

Continuing in that naming scheme, this might be called

	i2c-remove-redundant-i2c_client-list.patch

---
 drivers/i2c/i2c-core.c |  189 ++++++++++++++++++++++---------------------------
 drivers/i2c/i2c-dev.c  |   33 ++++----
 include/linux/i2c.h    |    4 -
 3 files changed, 102 insertions(+), 124 deletions(-)

--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -256,7 +256,6 @@ EXPORT_SYMBOL_GPL(i2c_new_device);
  */
 void i2c_unregister_device(struct i2c_client *client)
 {
-	struct i2c_adapter	*adapter = client->adapter;
 	struct i2c_driver	*driver = client->driver;
 
 	if (driver && !is_newstyle_driver(driver)) {
@@ -265,11 +264,6 @@ void i2c_unregister_device(struct i2c_cl
 		WARN_ON(1);
 		return;
 	}
-
-	mutex_lock(&adapter->clist_lock);
-	list_del(&client->list);
-	mutex_unlock(&adapter->clist_lock);
-
 	device_unregister(&client->dev);
 }
 EXPORT_SYMBOL_GPL(i2c_unregister_device);
@@ -381,8 +375,6 @@ static int i2c_register_adapter(struct i
 	int res = 0, dummy;
 
 	mutex_init(&adap->bus_lock);
-	mutex_init(&adap->clist_lock);
-	INIT_LIST_HEAD(&adap->clients);
 
 	mutex_lock(&core_lists);
 
@@ -525,6 +517,38 @@ static int i2c_do_del_adapter(struct dev
 	return res;
 }
 
+static struct i2c_client *verify_client(struct device *dev)
+{
+	if (dev->bus != &i2c_bus_type)
+		return NULL;
+	return to_i2c_client(dev);
+}
+
+static int detach_all_clients(struct device *dev, void *x)
+{
+	struct i2c_client	*client = verify_client(dev);
+	struct i2c_driver	*driver;
+	int			res;
+
+	if (!client)
+		return 0;
+
+	driver = client->driver;
+
+	/* new style, follow standard driver model */
+	if (!driver || is_newstyle_driver(driver)) {
+		i2c_unregister_device(client);
+		return 0;
+	}
+
+	/* legacy drivers create and remove clients themselves */
+	if ((res = driver->detach_client(client)))
+		dev_err(dev, "detach_client [%s] failed at address 0x%02x\n",
+			client->name, client->addr);
+
+	return res;
+}
+
 /**
  * i2c_del_adapter - unregister I2C adapter
  * @adap: the adapter being unregistered
@@ -535,8 +559,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_lists);
@@ -557,26 +579,9 @@ int i2c_del_adapter(struct i2c_adapter *
 
 	/* 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;
-		}
-	}
+	res = device_for_each_child(&adap->dev, NULL, detach_all_clients);
+	if (res)
+		goto out_unlock;
 
 	/* clean up the sysfs representation */
 	init_completion(&adap->dev_released);
@@ -655,6 +660,20 @@ int i2c_register_driver(struct module *o
 }
 EXPORT_SYMBOL(i2c_register_driver);
 
+static int detach_legacy_clients(struct device *dev, void *driver)
+{
+	struct i2c_client	*client = verify_client(dev);
+
+	if (client && client->driver == driver) {
+		dev_dbg(dev, "detaching client [%s] at 0x%02x\n",
+				client->name, client->addr);
+		if (client->driver->detach_client(client))
+			dev_err(dev, "failed detach_client [%s] at 0x%02x\n",
+				client->name, client->addr);
+	}
+	return 0;
+}
+
 /**
  * i2c_del_driver - unregister I2C driver
  * @driver: the driver being unregistered
@@ -662,8 +681,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_lists);
@@ -684,22 +701,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,
+					detach_legacy_clients);
 	}
 	up(&i2c_adapter_class.sem);
 
@@ -713,28 +717,19 @@ EXPORT_SYMBOL(i2c_del_driver);
 
 /* ------------------------------------------------------------------------- */
 
-static int __i2c_check_addr(struct i2c_adapter *adapter, unsigned int addr)
+static int i2c_checkaddr(struct device *dev, void *addrp)
 {
-	struct list_head   *item;
-	struct i2c_client  *client;
+	struct i2c_client	*client = verify_client(dev);
+	int			addr = *(int *)addrp;
 
-	list_for_each(item,&adapter->clients) {
-		client = list_entry(item, struct i2c_client, list);
-		if (client->addr == addr)
-			return -EBUSY;
-	}
+	if (client && client->addr == addr)
+		return -EBUSY;
 	return 0;
 }
 
 static int i2c_check_addr(struct i2c_adapter *adapter, int addr)
 {
-	int rval;
-
-	mutex_lock(&adapter->clist_lock);
-	rval = __i2c_check_addr(adapter, addr);
-	mutex_unlock(&adapter->clist_lock);
-
-	return rval;
+	return device_for_each_child(&adapter->dev, &addr, i2c_checkaddr);
 }
 
 int i2c_attach_client(struct i2c_client *client)
@@ -742,13 +737,6 @@ int i2c_attach_client(struct i2c_client 
 	struct i2c_adapter *adapter = client->adapter;
 	int res = 0;
 
-	mutex_lock(&adapter->clist_lock);
-	if (__i2c_check_addr(client->adapter, client->addr)) {
-		res = -EBUSY;
-		goto out_unlock;
-	}
-	list_add_tail(&client->list,&adapter->clients);
-
 	client->usage_count = 0;
 
 	client->dev.parent = &client->adapter->dev;
@@ -765,12 +753,16 @@ int i2c_attach_client(struct i2c_client 
 
 	snprintf(&client->dev.bus_id[0], sizeof(client->dev.bus_id),
 		"%d-%04x", i2c_adapter_id(adapter), client->addr);
+	res = device_register(&client->dev);
+	if (res) {
+		dev_err(&adapter->dev,
+			"Failed to register i2c client %s at 0x%02x (%d)\n",
+			client->name, client->addr, res);
+		return res;
+	}
+
 	dev_dbg(&adapter->dev, "client [%s] registered with bus id %s\n",
 		client->name, client->dev.bus_id);
-	res = device_register(&client->dev);
-	if (res)
-		goto out_list;
-	mutex_unlock(&adapter->clist_lock);
 
 	if (adapter->client_register)  {
 		if (adapter->client_register(client)) {
@@ -781,14 +773,6 @@ int i2c_attach_client(struct i2c_client 
 	}
 
 	return 0;
-
-out_list:
-	list_del(&client->list);
-	dev_err(&adapter->dev, "Failed to attach i2c client %s at 0x%02x "
-		"(%d)\n", client->name, client->addr, res);
-out_unlock:
-	mutex_unlock(&adapter->clist_lock);
-	return res;
 }
 EXPORT_SYMBOL(i2c_attach_client);
 
@@ -813,11 +797,8 @@ int i2c_detach_client(struct i2c_client 
 		}
 	}
 
-	mutex_lock(&adapter->clist_lock);
-	list_del(&client->list);
 	init_completion(&client->released);
 	device_unregister(&client->dev);
-	mutex_unlock(&adapter->clist_lock);
 	wait_for_completion(&client->released);
 
  out:
@@ -873,24 +854,28 @@ int i2c_release_client(struct i2c_client
 }
 EXPORT_SYMBOL(i2c_release_client);
 
+struct i2c_cmd_arg {
+	unsigned	cmd;
+	void		*arg;
+};
+
+static int i2c_cmd(struct device *dev, void *_arg)
+{
+	struct i2c_client	*client = verify_client(dev);
+	struct i2c_cmd_arg	*arg = _arg;
+
+	if (client)
+		client->driver->command(client, arg->cmd, arg->arg);
+	return 0;
+}
+
 void i2c_clients_command(struct i2c_adapter *adap, unsigned int cmd, void *arg)
 {
-	struct list_head  *item;
-	struct i2c_client *client;
+	struct i2c_cmd_arg	cmd_arg;
 
-	mutex_lock(&adap->clist_lock);
-	list_for_each(item,&adap->clients) {
-		client = list_entry(item, struct i2c_client, list);
-		if (!try_module_get(client->driver->driver.owner))
-			continue;
-		if (NULL != client->driver->command) {
-			mutex_unlock(&adap->clist_lock);
-			client->driver->command(client,cmd,arg);
-			mutex_lock(&adap->clist_lock);
-		}
-		module_put(client->driver->driver.owner);
-       }
-       mutex_unlock(&adap->clist_lock);
+	cmd_arg.cmd = cmd;
+	cmd_arg.arg = arg;
+	device_for_each_child(&adap->dev, &cmd_arg, i2c_cmd);
 }
 EXPORT_SYMBOL(i2c_clients_command);
 
@@ -1151,7 +1136,6 @@ i2c_new_probed_device(struct i2c_adapter
 		return NULL;
 	}
 
-	mutex_lock(&adap->clist_lock);
 	for (i = 0; addr_list[i] != I2C_CLIENT_END; i++) {
 		/* Check address validity */
 		if (addr_list[i] < 0x03 || addr_list[i] > 0x77) {
@@ -1161,7 +1145,7 @@ i2c_new_probed_device(struct i2c_adapter
 		}
 
 		/* Check address availability */
-		if (__i2c_check_addr(adap, addr_list[i])) {
+		if (i2c_check_addr(adap, addr_list[i])) {
 			dev_dbg(&adap->dev, "Address 0x%02x already in "
 				"use, not probing\n", addr_list[i]);
 			continue;
@@ -1189,7 +1173,6 @@ i2c_new_probed_device(struct i2c_adapter
 				break;
 		}
 	}
-	mutex_unlock(&adap->clist_lock);
 
 	if (addr_list[i] == I2C_CLIENT_END) {
 		dev_dbg(&adap->dev, "Probing failed, no device found\n");
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -182,27 +182,26 @@ static ssize_t i2cdev_write (struct file
 	return ret;
 }
 
-/* This address checking function differs from the one in i2c-core
-   in that it considers an address with a registered device, but no
-   bounded driver, as NOT busy. */
-static int i2cdev_check_addr(struct i2c_adapter *adapter, unsigned int addr)
+static int i2cdev_check(struct device *dev, void *addrp)
 {
-	struct list_head *item;
 	struct i2c_client *client;
-	int res = 0;
 
-	mutex_lock(&adapter->clist_lock);
-	list_for_each(item, &adapter->clients) {
-		client = list_entry(item, struct i2c_client, list);
-		if (client->addr == addr) {
-			if (client->driver)
-				res = -EBUSY;
-			break;
-		}
-	}
-	mutex_unlock(&adapter->clist_lock);
+	if (!dev->bus || strcmp("i2c", dev->bus->name) != 0)
+		return 0;
 
-	return res;
+	client = to_i2c_client(dev);
+	if (client->addr != *(unsigned int *)addrp)
+		return 0;
+
+	return dev->driver ? -EBUSY : 0;
+}
+
+/* This address checking function differs from the one in i2c-core
+   in that it considers an address with a registered device, but no
+   driver to it, as NOT busy. */
+static int i2cdev_check_addr(struct i2c_adapter *adapter, unsigned int addr)
+{
+	return device_for_each_child(&adapter->dev, &addr, i2cdev_check);
 }
 
 static int i2cdev_ioctl(struct inode *inode, struct file *file,
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -159,7 +159,6 @@ struct i2c_driver {
  * @irq: indicates the IRQ generated by this device (if any)
  * @driver_name: Identifies new-style driver used with this device; also
  *	used as the module name for hotplug/coldplug modprobe support.
- * @list: list of active/busy clients
  * @released: used to synchronize client releases & detaches and references
  *
  * An i2c_client identifies a single device (i.e. chip) connected to an
@@ -179,7 +178,6 @@ struct i2c_client {
 	struct device dev;		/* the device structure		*/
 	int irq;			/* irq issued by device (or -1) */
 	char driver_name[KOBJ_NAME_LEN];
-	struct list_head list;
 	struct completion released;
 };
 #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
@@ -317,14 +315,12 @@ 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;
 	struct device dev;		/* the adapter device */
 
 	int nr;
-	struct list_head clients;
 	char name[48];
 	struct completion dev_released;
 };

Re: [patch 2.6.24-rc5-git] add i2c_new_dummy() utility

[Jean Delvare]

Hi David,

On Sat, 29 Dec 2007 19:05:14 -0800, David Brownell wrote:
> Remove further duplication between i2c core and driver model:  the
> per-adapter list of clients (adapter->clients,  client->list) and
> its lock (adapter->clist_lock) duplicate adapter->dev.children.
>  
> LIGHTLY TESTED ... goes on top of two patches from Jean's I2C queue,
> 
> 	i2c-remove-redundant-i2c_adapter-list.patch
> 	i2c-remove-redundant-i2c_driver-list.patch
> 
> Continuing in that naming scheme, this might be called
> 
> 	i2c-remove-redundant-i2c_client-list.patch
> 
> ---
>  drivers/i2c/i2c-core.c |  189 ++++++++++++++++++++++---------------------------
>  drivers/i2c/i2c-dev.c  |   33 ++++----
>  include/linux/i2c.h    |    4 -
>  3 files changed, 102 insertions(+), 124 deletions(-)

This breaks drivers/media/video/mxb.c, drivers/media/video/dpc7146.c
and drivers/media/video/tvmixer.c. These 3 drivers walk the clients
list that your patch wants to remove. I guess that we need to replace
their code by something equivalent that would walk the driver model's
children list instead of i2c-core's internal one?

-- 
Jean Delvare

Re: [patch 2.6.24-rc5-git] add i2c_new_dummy() utility

[David Brownell]

On Friday 04 January 2008, Jean Delvare wrote:
> Hi David,
> 
> On Sat, 29 Dec 2007 19:05:14 -0800, David Brownell wrote:
> > Remove further duplication between i2c core and driver model:  the
> > per-adapter list of clients (adapter->clients,  client->list) and
> > its lock (adapter->clist_lock) duplicate adapter->dev.children.
> >  
> > LIGHTLY TESTED ... goes on top of two patches from Jean's I2C queue,
> > 
> > 	i2c-remove-redundant-i2c_adapter-list.patch
> > 	i2c-remove-redundant-i2c_driver-list.patch
> > 
> > Continuing in that naming scheme, this might be called
> > 
> > 	i2c-remove-redundant-i2c_client-list.patch
> > 
> > ---
> >  drivers/i2c/i2c-core.c |  189 ++++++++++++++++++++++---------------------------
> >  drivers/i2c/i2c-dev.c  |   33 ++++----
> >  include/linux/i2c.h    |    4 -
> >  3 files changed, 102 insertions(+), 124 deletions(-)
> 
> This breaks drivers/media/video/mxb.c, drivers/media/video/dpc7146.c
> and drivers/media/video/tvmixer.c. These 3 drivers walk the clients
> list that your patch wants to remove.

Whoops.  Good thing I warned y'all that it was only lightly tested!
Presumably this was the result of a "build every line of I2C code"
test.

Those all seem to do the same thing:  list_for_each_entry() scan to
find specific devices connected to their private adapter.


> I guess that we need to replace 
> their code by something equivalent that would walk the driver model's
> children list instead of i2c-core's internal one?

Like the device_for_each_child() scanning in the i2-core parts of
that patch ... yes, that's probably the least invasive change for
the moment.  Eventually I'd like to see the relevant code using
new-style driver binding, but that seems a bit much to change at
the moment.

Changing the users of i2c_adapter.clients could be safely done
before removing that list (and its lock, ignored by most of
those drivers if not all of them).  Which should simplify testing
such changes by a bit.

- Dave

Re: [patch 2.6.24-rc5-git] add i2c_new_dummy() utility

[Jean Delvare]

Hi David,

On Fri, 4 Jan 2008 15:48:54 -0800, David Brownell wrote:
> On Friday 04 January 2008, Jean Delvare wrote:
> > This breaks drivers/media/video/mxb.c, drivers/media/video/dpc7146.c
> > and drivers/media/video/tvmixer.c. These 3 drivers walk the clients
> > list that your patch wants to remove.
> 
> Whoops.  Good thing I warned y'all that it was only lightly tested!
> Presumably this was the result of a "build every line of I2C code"
> test.

This is my default build mode ;) I'm doing changes to i2c all the time
so I want to know quickly if a given change breaks some driver.

> Those all seem to do the same thing:  list_for_each_entry() scan to
> find specific devices connected to their private adapter.

True.

> > I guess that we need to replace 
> > their code by something equivalent that would walk the driver model's
> > children list instead of i2c-core's internal one?
> 
> Like the device_for_each_child() scanning in the i2-core parts of
> that patch ... yes, that's probably the least invasive change for
> the moment.  Eventually I'd like to see the relevant code using
> new-style driver binding, but that seems a bit much to change at
> the moment.

That was pretty much my conclusion as well.

> Changing the users of i2c_adapter.clients could be safely done
> before removing that list (and its lock, ignored by most of
> those drivers if not all of them).  Which should simplify testing
> such changes by a bit.

Agreed, I'll write such patches later today and will send them to the
v4l list for review and testing.

-- 
Jean Delvare

Re: i2c-remove-redundant-i2c_client-list.patch

[Jean Delvare]

Hi David,

On Sat, 29 Dec 2007 19:05:14 -0800, David Brownell wrote:
> Remove further duplication between i2c core and driver model:  the
> per-adapter list of clients (adapter->clients,  client->list) and
> its lock (adapter->clist_lock) duplicate adapter->dev.children.
>  
> LIGHTLY TESTED ... goes on top of two patches from Jean's I2C queue,
> 
> 	i2c-remove-redundant-i2c_adapter-list.patch
> 	i2c-remove-redundant-i2c_driver-list.patch
> 
> Continuing in that naming scheme, this might be called
> 
> 	i2c-remove-redundant-i2c_client-list.patch
> 

Review:

> ---
>  drivers/i2c/i2c-core.c |  189 ++++++++++++++++++++++---------------------------
>  drivers/i2c/i2c-dev.c  |   33 ++++----
>  include/linux/i2c.h    |    4 -
>  3 files changed, 102 insertions(+), 124 deletions(-)
> 
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -256,7 +256,6 @@ EXPORT_SYMBOL_GPL(i2c_new_device);
>   */
>  void i2c_unregister_device(struct i2c_client *client)
>  {
> -	struct i2c_adapter	*adapter = client->adapter;
>  	struct i2c_driver	*driver = client->driver;
>  
>  	if (driver && !is_newstyle_driver(driver)) {
> @@ -265,11 +264,6 @@ void i2c_unregister_device(struct i2c_cl
>  		WARN_ON(1);
>  		return;
>  	}
> -
> -	mutex_lock(&adapter->clist_lock);
> -	list_del(&client->list);
> -	mutex_unlock(&adapter->clist_lock);
> -
>  	device_unregister(&client->dev);
>  }
>  EXPORT_SYMBOL_GPL(i2c_unregister_device);
> @@ -381,8 +375,6 @@ static int i2c_register_adapter(struct i
>  	int res = 0, dummy;
>  
>  	mutex_init(&adap->bus_lock);
> -	mutex_init(&adap->clist_lock);
> -	INIT_LIST_HEAD(&adap->clients);
>  
>  	mutex_lock(&core_lists);
>  
> @@ -525,6 +517,38 @@ static int i2c_do_del_adapter(struct dev
>  	return res;
>  }
>  
> +static struct i2c_client *verify_client(struct device *dev)
> +{
> +	if (dev->bus != &i2c_bus_type)

Is this paranoia, or can this test really succeed? I thought that all
children of an i2c adapter node would always be i2c clients.

> +		return NULL;
> +	return to_i2c_client(dev);
> +}
> +
> +static int detach_all_clients(struct device *dev, void *x)
> +{
> +	struct i2c_client	*client = verify_client(dev);
> +	struct i2c_driver	*driver;
> +	int			res;
> +
> +	if (!client)
> +		return 0;
> +
> +	driver = client->driver;
> +
> +	/* new style, follow standard driver model */
> +	if (!driver || is_newstyle_driver(driver)) {
> +		i2c_unregister_device(client);
> +		return 0;
> +	}
> +
> +	/* legacy drivers create and remove clients themselves */
> +	if ((res = driver->detach_client(client)))
> +		dev_err(dev, "detach_client [%s] failed at address 0x%02x\n",
> +			client->name, client->addr);
> +
> +	return res;
> +}
> +
>  /**
>   * i2c_del_adapter - unregister I2C adapter
>   * @adap: the adapter being unregistered
> @@ -535,8 +559,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_lists);
> @@ -557,26 +579,9 @@ int i2c_del_adapter(struct i2c_adapter *
>  
>  	/* 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;
> -		}
> -	}
> +	res = device_for_each_child(&adap->dev, NULL, detach_all_clients);
> +	if (res)
> +		goto out_unlock;
>  
>  	/* clean up the sysfs representation */
>  	init_completion(&adap->dev_released);
> @@ -655,6 +660,20 @@ int i2c_register_driver(struct module *o
>  }
>  EXPORT_SYMBOL(i2c_register_driver);
>  
> +static int detach_legacy_clients(struct device *dev, void *driver)
> +{
> +	struct i2c_client	*client = verify_client(dev);
> +
> +	if (client && client->driver == driver) {
> +		dev_dbg(dev, "detaching client [%s] at 0x%02x\n",
> +				client->name, client->addr);
> +		if (client->driver->detach_client(client))
> +			dev_err(dev, "failed detach_client [%s] at 0x%02x\n",
> +				client->name, client->addr);
> +	}
> +	return 0;
> +}
> +
>  /**
>   * i2c_del_driver - unregister I2C driver
>   * @driver: the driver being unregistered
> @@ -662,8 +681,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_lists);
> @@ -684,22 +701,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,
> +					detach_legacy_clients);
>  	}
>  	up(&i2c_adapter_class.sem);
>  
> @@ -713,28 +717,19 @@ EXPORT_SYMBOL(i2c_del_driver);
>  
>  /* ------------------------------------------------------------------------- */
>  
> -static int __i2c_check_addr(struct i2c_adapter *adapter, unsigned int addr)
> +static int i2c_checkaddr(struct device *dev, void *addrp)

I don't much like this name, why don't you keep __i2c_check_addr?

>  {
> -	struct list_head   *item;
> -	struct i2c_client  *client;
> +	struct i2c_client	*client = verify_client(dev);
> +	int			addr = *(int *)addrp;
>  
> -	list_for_each(item,&adapter->clients) {
> -		client = list_entry(item, struct i2c_client, list);
> -		if (client->addr == addr)
> -			return -EBUSY;
> -	}
> +	if (client && client->addr == addr)
> +		return -EBUSY;
>  	return 0;
>  }
>  
>  static int i2c_check_addr(struct i2c_adapter *adapter, int addr)
>  {
> -	int rval;
> -
> -	mutex_lock(&adapter->clist_lock);
> -	rval = __i2c_check_addr(adapter, addr);
> -	mutex_unlock(&adapter->clist_lock);
> -
> -	return rval;
> +	return device_for_each_child(&adapter->dev, &addr, i2c_checkaddr);
>  }
>  
>  int i2c_attach_client(struct i2c_client *client)
> @@ -742,13 +737,6 @@ int i2c_attach_client(struct i2c_client 
>  	struct i2c_adapter *adapter = client->adapter;
>  	int res = 0;
>  
> -	mutex_lock(&adapter->clist_lock);
> -	if (__i2c_check_addr(client->adapter, client->addr)) {
> -		res = -EBUSY;
> -		goto out_unlock;
> -	}
> -	list_add_tail(&client->list,&adapter->clients);
> -
>  	client->usage_count = 0;
>  
>  	client->dev.parent = &client->adapter->dev;
> @@ -765,12 +753,16 @@ int i2c_attach_client(struct i2c_client 
>  
>  	snprintf(&client->dev.bus_id[0], sizeof(client->dev.bus_id),
>  		"%d-%04x", i2c_adapter_id(adapter), client->addr);
> +	res = device_register(&client->dev);
> +	if (res) {
> +		dev_err(&adapter->dev,
> +			"Failed to register i2c client %s at 0x%02x (%d)\n",
> +			client->name, client->addr, res);
> +		return res;
> +	}
> +
>  	dev_dbg(&adapter->dev, "client [%s] registered with bus id %s\n",
>  		client->name, client->dev.bus_id);
> -	res = device_register(&client->dev);
> -	if (res)
> -		goto out_list;
> -	mutex_unlock(&adapter->clist_lock);
>  
>  	if (adapter->client_register)  {
>  		if (adapter->client_register(client)) {
> @@ -781,14 +773,6 @@ int i2c_attach_client(struct i2c_client 
>  	}
>  
>  	return 0;
> -
> -out_list:
> -	list_del(&client->list);
> -	dev_err(&adapter->dev, "Failed to attach i2c client %s at 0x%02x "
> -		"(%d)\n", client->name, client->addr, res);
> -out_unlock:
> -	mutex_unlock(&adapter->clist_lock);
> -	return res;
>  }
>  EXPORT_SYMBOL(i2c_attach_client);
>  
> @@ -813,11 +797,8 @@ int i2c_detach_client(struct i2c_client 
>  		}
>  	}
>  
> -	mutex_lock(&adapter->clist_lock);
> -	list_del(&client->list);
>  	init_completion(&client->released);
>  	device_unregister(&client->dev);
> -	mutex_unlock(&adapter->clist_lock);
>  	wait_for_completion(&client->released);
>  
>   out:
> @@ -873,24 +854,28 @@ int i2c_release_client(struct i2c_client
>  }
>  EXPORT_SYMBOL(i2c_release_client);
>  
> +struct i2c_cmd_arg {
> +	unsigned	cmd;
> +	void		*arg;
> +};
> +
> +static int i2c_cmd(struct device *dev, void *_arg)
> +{
> +	struct i2c_client	*client = verify_client(dev);
> +	struct i2c_cmd_arg	*arg = _arg;
> +
> +	if (client)
> +		client->driver->command(client, arg->cmd, arg->arg);

client->driver->command may or may not be defined. The original code
was checking for this, and so should yours.

> +	return 0;
> +}
> +
>  void i2c_clients_command(struct i2c_adapter *adap, unsigned int cmd, void *arg)
>  {
> -	struct list_head  *item;
> -	struct i2c_client *client;
> +	struct i2c_cmd_arg	cmd_arg;
>  
> -	mutex_lock(&adap->clist_lock);
> -	list_for_each(item,&adap->clients) {
> -		client = list_entry(item, struct i2c_client, list);
> -		if (!try_module_get(client->driver->driver.owner))
> -			continue;
> -		if (NULL != client->driver->command) {
> -			mutex_unlock(&adap->clist_lock);
> -			client->driver->command(client,cmd,arg);
> -			mutex_lock(&adap->clist_lock);
> -		}
> -		module_put(client->driver->driver.owner);
> -       }
> -       mutex_unlock(&adap->clist_lock);
> +	cmd_arg.cmd = cmd;
> +	cmd_arg.arg = arg;
> +	device_for_each_child(&adap->dev, &cmd_arg, i2c_cmd);
>  }
>  EXPORT_SYMBOL(i2c_clients_command);
>  
> @@ -1151,7 +1136,6 @@ i2c_new_probed_device(struct i2c_adapter
>  		return NULL;
>  	}
>  
> -	mutex_lock(&adap->clist_lock);
>  	for (i = 0; addr_list[i] != I2C_CLIENT_END; i++) {
>  		/* Check address validity */
>  		if (addr_list[i] < 0x03 || addr_list[i] > 0x77) {
> @@ -1161,7 +1145,7 @@ i2c_new_probed_device(struct i2c_adapter
>  		}
>  
>  		/* Check address availability */
> -		if (__i2c_check_addr(adap, addr_list[i])) {
> +		if (i2c_check_addr(adap, addr_list[i])) {
>  			dev_dbg(&adap->dev, "Address 0x%02x already in "
>  				"use, not probing\n", addr_list[i]);
>  			continue;
> @@ -1189,7 +1173,6 @@ i2c_new_probed_device(struct i2c_adapter
>  				break;
>  		}
>  	}
> -	mutex_unlock(&adap->clist_lock);
>  
>  	if (addr_list[i] == I2C_CLIENT_END) {
>  		dev_dbg(&adap->dev, "Probing failed, no device found\n");
> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c
> @@ -182,27 +182,26 @@ static ssize_t i2cdev_write (struct file
>  	return ret;
>  }
>  
> -/* This address checking function differs from the one in i2c-core
> -   in that it considers an address with a registered device, but no
> -   bounded driver, as NOT busy. */
> -static int i2cdev_check_addr(struct i2c_adapter *adapter, unsigned int addr)
> +static int i2cdev_check(struct device *dev, void *addrp)
>  {
> -	struct list_head *item;
>  	struct i2c_client *client;
> -	int res = 0;
>  
> -	mutex_lock(&adapter->clist_lock);
> -	list_for_each(item, &adapter->clients) {
> -		client = list_entry(item, struct i2c_client, list);
> -		if (client->addr == addr) {
> -			if (client->driver)
> -				res = -EBUSY;
> -			break;
> -		}
> -	}
> -	mutex_unlock(&adapter->clist_lock);
> +	if (!dev->bus || strcmp("i2c", dev->bus->name) != 0)

Please just export i2c_bus_type is you need it, or even verify_client
(then renamed to i2c_verify_client)? If the bus type check is really
needed then we'll need it for the 3 v4l drivers I mentioned earlier
anyway.

Alternatively we could write and export an i2c_for_each_client()
function doing all the required checks so that drivers don't have to
care. What do you think?

> +		return 0;
>  
> -	return res;
> +	client = to_i2c_client(dev);
> +	if (client->addr != *(unsigned int *)addrp)
> +		return 0;
> +
> +	return dev->driver ? -EBUSY : 0;
> +}
> +
> +/* This address checking function differs from the one in i2c-core
> +   in that it considers an address with a registered device, but no
> +   driver to it, as NOT busy. */
> +static int i2cdev_check_addr(struct i2c_adapter *adapter, unsigned int addr)
> +{
> +	return device_for_each_child(&adapter->dev, &addr, i2cdev_check);
>  }
>  
>  static int i2cdev_ioctl(struct inode *inode, struct file *file,
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -159,7 +159,6 @@ struct i2c_driver {
>   * @irq: indicates the IRQ generated by this device (if any)
>   * @driver_name: Identifies new-style driver used with this device; also
>   *	used as the module name for hotplug/coldplug modprobe support.
> - * @list: list of active/busy clients
>   * @released: used to synchronize client releases & detaches and references
>   *
>   * An i2c_client identifies a single device (i.e. chip) connected to an
> @@ -179,7 +178,6 @@ struct i2c_client {
>  	struct device dev;		/* the device structure		*/
>  	int irq;			/* irq issued by device (or -1) */
>  	char driver_name[KOBJ_NAME_LEN];
> -	struct list_head list;
>  	struct completion released;
>  };
>  #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
> @@ -317,14 +315,12 @@ 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;
>  	struct device dev;		/* the adapter device */
>  
>  	int nr;
> -	struct list_head clients;
>  	char name[48];
>  	struct completion dev_released;
>  };

The rest looks OK... nice cleanup, thanks for doing this.

-- 
Jean Delvare

Re: i2c-remove-redundant-i2c_client-list.patch

[David Brownell]

On Sunday 06 January 2008, Jean Delvare wrote:
> > +static struct i2c_client *verify_client(struct device *dev)
> > +{
> > +	if (dev->bus != &i2c_bus_type)
> 
> Is this paranoia, or can this test really succeed? I thought that all
> children of an i2c adapter node would always be i2c clients.

Let's call it well-founded paranoia.  I know that when the SPI
code got a "remove class_device" patch, I had to fix an oops
caused by an unexpected child ... and at the top of my current
GIT snapshot is 911833440b498e3e4fe2f12c1ae2bd44400c7004 which
fixed similar oopsing in SCSI.



> > @@ -713,28 +717,19 @@ EXPORT_SYMBOL(i2c_del_driver);
> >  
> >  /* ------------------------------------------------------------------------- */
> >  
> > -static int __i2c_check_addr(struct i2c_adapter *adapter, unsigned int addr)
> > +static int i2c_checkaddr(struct device *dev, void *addrp)
> 
> I don't much like this name, why don't you keep __i2c_check_addr?

Changing it is no problem.  I guess the name bothered me too,
my working copy uses "i2c_czechaddr", would you believe.  ;)


> >  
> > +struct i2c_cmd_arg {
> > +	unsigned	cmd;
> > +	void		*arg;
> > +};
> > +
> > +static int i2c_cmd(struct device *dev, void *_arg)
> > +{
> > +	struct i2c_client	*client = verify_client(dev);
> > +	struct i2c_cmd_arg	*arg = _arg;
> > +
> > +	if (client)
> > +		client->driver->command(client, arg->cmd, arg->arg);
> 
> client->driver->command may or may not be defined. The original code
> was checking for this, and so should yours.

OK.


> > --- a/drivers/i2c/i2c-dev.c
> > +++ b/drivers/i2c/i2c-dev.c
> > 		...
> > -	}
> > -	mutex_unlock(&adapter->clist_lock);
> > +	if (!dev->bus || strcmp("i2c", dev->bus->name) != 0)
> 
> Please just export i2c_bus_type is you need it, or even verify_client
> (then renamed to i2c_verify_client)? If the bus type check is really
> needed then we'll need it for the 3 v4l drivers I mentioned earlier
> anyway.

Good point.  I didn't like this part much, and that function can
have other uses.  I updated kobj_to_i2c_client() to use it too.


> Alternatively we could write and export an i2c_for_each_client()
> function doing all the required checks so that drivers don't have to
> care. What do you think?

Less good.  We can't know in advance which things they care about.

 
> >			...
> >  
> 
> The rest looks OK... nice cleanup, thanks for doing this.

It seemed appropriate.  :)

Though I was a bit worried it might have bugs in it, since all I had
time to do was code it, and sanity check it with a couple variants of
one new-style config.  (I no longer have those monster "build all I2C
drivers" configs around.)

- Dave

Re: i2c-remove-redundant-i2c_client-list.patch

[David Brownell]

Remove further duplication between i2c core and driver model:  the
per-adapter list of clients (adapter->clients,  client->list) and
its lock (adapter->clist_lock) duplicate adapter->dev.children.

Add and use a new i2c_verify_client() routine to help code which
traverses the driver model tree.
 
Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
LIGHTLY TESTED ... goes on top of two patches from Jean's I2C queue,

	i2c-remove-redundant-i2c_adapter-list.patch
	i2c-remove-redundant-i2c_driver-list.patch

Updated per Jean's comments.  (Note that the i2c_verify_client
bit might usefully be split out into a separate patch, to simplify
converting the V4L code that uses this i2c client list.)

 drivers/i2c/i2c-core.c |  202 ++++++++++++++++++++++++-------------------------
 drivers/i2c/i2c-dev.c  |   29 ++-----
 include/linux/i2c.h    |    9 --
 3 files changed, 114 insertions(+), 126 deletions(-)

--- at91.orig/drivers/i2c/i2c-core.c	2008-01-06 10:47:45.000000000 -0800
+++ at91/drivers/i2c/i2c-core.c	2008-01-06 11:08:00.000000000 -0800
@@ -197,6 +197,25 @@ static struct bus_type i2c_bus_type = {
 	.resume		= i2c_device_resume,
 };
 
+
+/**
+ * i2c_verify_client - return parameter as i2c_client, or NULL
+ * @dev: device, probably from some driver model iterator
+ *
+ * When traversing the driver model tree, perhaps using driver model
+ * iterators like @device_for_each_child(), you can't assume very much
+ * about the nodes you find.  Use this function to avoid oopses caused
+ * by wrongly treating some non-I2C device as an i2c_client.
+ */
+struct i2c_client *i2c_verify_client(struct device *dev)
+{
+	return (dev->bus == &i2c_bus_type)
+			? to_i2c_client(dev)
+			: NULL;
+}
+EXPORT_SYMBOL(i2c_verify_client);
+
+
 /**
  * i2c_new_device - instantiate an i2c device for use with a new style driver
  * @adap: the adapter managing the device
@@ -256,7 +275,6 @@ EXPORT_SYMBOL_GPL(i2c_new_device);
  */
 void i2c_unregister_device(struct i2c_client *client)
 {
-	struct i2c_adapter	*adapter = client->adapter;
 	struct i2c_driver	*driver = client->driver;
 
 	if (driver && !is_newstyle_driver(driver)) {
@@ -265,11 +283,6 @@ void i2c_unregister_device(struct i2c_cl
 		WARN_ON(1);
 		return;
 	}
-
-	mutex_lock(&adapter->clist_lock);
-	list_del(&client->list);
-	mutex_unlock(&adapter->clist_lock);
-
 	device_unregister(&client->dev);
 }
 EXPORT_SYMBOL_GPL(i2c_unregister_device);
@@ -381,8 +394,6 @@ static int i2c_register_adapter(struct i
 	int res = 0, dummy;
 
 	mutex_init(&adap->bus_lock);
-	mutex_init(&adap->clist_lock);
-	INIT_LIST_HEAD(&adap->clients);
 
 	mutex_lock(&core_lists);
 
@@ -525,6 +536,32 @@ static int i2c_do_del_adapter(struct dev
 	return res;
 }
 
+
+static int detach_all_clients(struct device *dev, void *x)
+{
+	struct i2c_client	*client = i2c_verify_client(dev);
+	struct i2c_driver	*driver;
+	int			res;
+
+	if (!client)
+		return 0;
+
+	driver = client->driver;
+
+	/* new style, follow standard driver model */
+	if (!driver || is_newstyle_driver(driver)) {
+		i2c_unregister_device(client);
+		return 0;
+	}
+
+	/* legacy drivers create and remove clients themselves */
+	if ((res = driver->detach_client(client)))
+		dev_err(dev, "detach_client [%s] failed at address 0x%02x\n",
+			client->name, client->addr);
+
+	return res;
+}
+
 /**
  * i2c_del_adapter - unregister I2C adapter
  * @adap: the adapter being unregistered
@@ -535,8 +572,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_lists);
@@ -557,26 +592,9 @@ int i2c_del_adapter(struct i2c_adapter *
 
 	/* 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;
-		}
-	}
+	res = device_for_each_child(&adap->dev, NULL, detach_all_clients);
+	if (res)
+		goto out_unlock;
 
 	/* clean up the sysfs representation */
 	init_completion(&adap->dev_released);
@@ -655,6 +673,20 @@ int i2c_register_driver(struct module *o
 }
 EXPORT_SYMBOL(i2c_register_driver);
 
+static int detach_legacy_clients(struct device *dev, void *driver)
+{
+	struct i2c_client	*client = i2c_verify_client(dev);
+
+	if (client && client->driver == driver) {
+		dev_dbg(dev, "detaching client [%s] at 0x%02x\n",
+				client->name, client->addr);
+		if (client->driver->detach_client(client))
+			dev_err(dev, "failed detach_client [%s] at 0x%02x\n",
+				client->name, client->addr);
+	}
+	return 0;
+}
+
 /**
  * i2c_del_driver - unregister I2C driver
  * @driver: the driver being unregistered
@@ -662,8 +694,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_lists);
@@ -684,22 +714,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,
+					detach_legacy_clients);
 	}
 	up(&i2c_adapter_class.sem);
 
@@ -713,28 +730,19 @@ EXPORT_SYMBOL(i2c_del_driver);
 
 /* ------------------------------------------------------------------------- */
 
-static int __i2c_check_addr(struct i2c_adapter *adapter, unsigned int addr)
+static int __i2c_check_addr(struct device *dev, void *addrp)
 {
-	struct list_head   *item;
-	struct i2c_client  *client;
+	struct i2c_client	*client = i2c_verify_client(dev);
+	int			addr = *(int *)addrp;
 
-	list_for_each(item,&adapter->clients) {
-		client = list_entry(item, struct i2c_client, list);
-		if (client->addr == addr)
-			return -EBUSY;
-	}
+	if (client && client->addr == addr)
+		return -EBUSY;
 	return 0;
 }
 
 static int i2c_check_addr(struct i2c_adapter *adapter, int addr)
 {
-	int rval;
-
-	mutex_lock(&adapter->clist_lock);
-	rval = __i2c_check_addr(adapter, addr);
-	mutex_unlock(&adapter->clist_lock);
-
-	return rval;
+	return device_for_each_child(&adapter->dev, &addr, __i2c_check_addr);
 }
 
 int i2c_attach_client(struct i2c_client *client)
@@ -742,13 +750,6 @@ int i2c_attach_client(struct i2c_client 
 	struct i2c_adapter *adapter = client->adapter;
 	int res = 0;
 
-	mutex_lock(&adapter->clist_lock);
-	if (__i2c_check_addr(client->adapter, client->addr)) {
-		res = -EBUSY;
-		goto out_unlock;
-	}
-	list_add_tail(&client->list,&adapter->clients);
-
 	client->usage_count = 0;
 
 	client->dev.parent = &client->adapter->dev;
@@ -765,12 +766,16 @@ int i2c_attach_client(struct i2c_client 
 
 	snprintf(&client->dev.bus_id[0], sizeof(client->dev.bus_id),
 		"%d-%04x", i2c_adapter_id(adapter), client->addr);
+	res = device_register(&client->dev);
+	if (res) {
+		dev_err(&adapter->dev,
+			"Failed to register i2c client %s at 0x%02x (%d)\n",
+			client->name, client->addr, res);
+		return res;
+	}
+
 	dev_dbg(&adapter->dev, "client [%s] registered with bus id %s\n",
 		client->name, client->dev.bus_id);
-	res = device_register(&client->dev);
-	if (res)
-		goto out_list;
-	mutex_unlock(&adapter->clist_lock);
 
 	if (adapter->client_register)  {
 		if (adapter->client_register(client)) {
@@ -781,14 +786,6 @@ int i2c_attach_client(struct i2c_client 
 	}
 
 	return 0;
-
-out_list:
-	list_del(&client->list);
-	dev_err(&adapter->dev, "Failed to attach i2c client %s at 0x%02x "
-		"(%d)\n", client->name, client->addr, res);
-out_unlock:
-	mutex_unlock(&adapter->clist_lock);
-	return res;
 }
 EXPORT_SYMBOL(i2c_attach_client);
 
@@ -813,11 +810,8 @@ int i2c_detach_client(struct i2c_client 
 		}
 	}
 
-	mutex_lock(&adapter->clist_lock);
-	list_del(&client->list);
 	init_completion(&client->released);
 	device_unregister(&client->dev);
-	mutex_unlock(&adapter->clist_lock);
 	wait_for_completion(&client->released);
 
  out:
@@ -873,24 +867,28 @@ int i2c_release_client(struct i2c_client
 }
 EXPORT_SYMBOL(i2c_release_client);
 
+struct i2c_cmd_arg {
+	unsigned	cmd;
+	void		*arg;
+};
+
+static int i2c_cmd(struct device *dev, void *_arg)
+{
+	struct i2c_client	*client = i2c_verify_client(dev);
+	struct i2c_cmd_arg	*arg = _arg;
+
+	if (client && client->driver->command)
+		client->driver->command(client, arg->cmd, arg->arg);
+	return 0;
+}
+
 void i2c_clients_command(struct i2c_adapter *adap, unsigned int cmd, void *arg)
 {
-	struct list_head  *item;
-	struct i2c_client *client;
+	struct i2c_cmd_arg	cmd_arg;
 
-	mutex_lock(&adap->clist_lock);
-	list_for_each(item,&adap->clients) {
-		client = list_entry(item, struct i2c_client, list);
-		if (!try_module_get(client->driver->driver.owner))
-			continue;
-		if (NULL != client->driver->command) {
-			mutex_unlock(&adap->clist_lock);
-			client->driver->command(client,cmd,arg);
-			mutex_lock(&adap->clist_lock);
-		}
-		module_put(client->driver->driver.owner);
-       }
-       mutex_unlock(&adap->clist_lock);
+	cmd_arg.cmd = cmd;
+	cmd_arg.arg = arg;
+	device_for_each_child(&adap->dev, &cmd_arg, i2c_cmd);
 }
 EXPORT_SYMBOL(i2c_clients_command);
 
@@ -1151,7 +1149,6 @@ i2c_new_probed_device(struct i2c_adapter
 		return NULL;
 	}
 
-	mutex_lock(&adap->clist_lock);
 	for (i = 0; addr_list[i] != I2C_CLIENT_END; i++) {
 		/* Check address validity */
 		if (addr_list[i] < 0x03 || addr_list[i] > 0x77) {
@@ -1161,7 +1158,7 @@ i2c_new_probed_device(struct i2c_adapter
 		}
 
 		/* Check address availability */
-		if (__i2c_check_addr(adap, addr_list[i])) {
+		if (i2c_check_addr(adap, addr_list[i])) {
 			dev_dbg(&adap->dev, "Address 0x%02x already in "
 				"use, not probing\n", addr_list[i]);
 			continue;
@@ -1189,7 +1186,6 @@ i2c_new_probed_device(struct i2c_adapter
 				break;
 		}
 	}
-	mutex_unlock(&adap->clist_lock);
 
 	if (addr_list[i] == I2C_CLIENT_END) {
 		dev_dbg(&adap->dev, "Probing failed, no device found\n");
--- at91.orig/drivers/i2c/i2c-dev.c	2008-01-05 16:17:06.000000000 -0800
+++ at91/drivers/i2c/i2c-dev.c	2008-01-06 11:22:30.000000000 -0800
@@ -182,27 +182,22 @@ static ssize_t i2cdev_write (struct file
 	return ret;
 }
 
+static int i2cdev_check(struct device *dev, void *addrp)
+{
+	struct i2c_client *client = i2c_verify_client(dev);
+
+	if (!client || client->addr != *(unsigned int *)addrp)
+		return 0;
+
+	return dev->driver ? -EBUSY : 0;
+}
+
 /* This address checking function differs from the one in i2c-core
    in that it considers an address with a registered device, but no
-   bounded driver, as NOT busy. */
+   driver to it, as NOT busy. */
 static int i2cdev_check_addr(struct i2c_adapter *adapter, unsigned int addr)
 {
-	struct list_head *item;
-	struct i2c_client *client;
-	int res = 0;
-
-	mutex_lock(&adapter->clist_lock);
-	list_for_each(item, &adapter->clients) {
-		client = list_entry(item, struct i2c_client, list);
-		if (client->addr == addr) {
-			if (client->driver)
-				res = -EBUSY;
-			break;
-		}
-	}
-	mutex_unlock(&adapter->clist_lock);
-
-	return res;
+	return device_for_each_child(&adapter->dev, &addr, i2cdev_check);
 }
 
 static int i2cdev_ioctl(struct inode *inode, struct file *file,
--- at91.orig/include/linux/i2c.h	2008-01-06 10:47:45.000000000 -0800
+++ at91/include/linux/i2c.h	2008-01-06 11:10:38.000000000 -0800
@@ -159,7 +159,6 @@ struct i2c_driver {
  * @irq: indicates the IRQ generated by this device (if any)
  * @driver_name: Identifies new-style driver used with this device; also
  *	used as the module name for hotplug/coldplug modprobe support.
- * @list: list of active/busy clients
  * @released: used to synchronize client releases & detaches and references
  *
  * An i2c_client identifies a single device (i.e. chip) connected to an
@@ -179,15 +178,15 @@ struct i2c_client {
 	struct device dev;		/* the device structure		*/
 	int irq;			/* irq issued by device (or -1) */
 	char driver_name[KOBJ_NAME_LEN];
-	struct list_head list;
 	struct completion released;
 };
 #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
 
+extern struct i2c_client *i2c_verify_client(struct device *dev);
+
 static inline struct i2c_client *kobj_to_i2c_client(struct kobject *kobj)
 {
-	struct device * const dev = container_of(kobj, struct device, kobj);
-	return to_i2c_client(dev);
+	return i2c_verify_client(container_of(kobj, struct device, kobj));
 }
 
 static inline void *i2c_get_clientdata (struct i2c_client *dev)
@@ -317,14 +316,12 @@ 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;
 	struct device dev;		/* the adapter device */
 
 	int nr;
-	struct list_head clients;
 	char name[48];
 	struct completion dev_released;
 };

Re: i2c-remove-redundant-i2c_client-list.patch

[Jean Delvare]

Hi David,

On Sun, 6 Jan 2008 11:30:31 -0800, David Brownell wrote:
> On Sunday 06 January 2008, Jean Delvare wrote:
> > > +static struct i2c_client *verify_client(struct device *dev)
> > > +{
> > > +	if (dev->bus != &i2c_bus_type)
> > 
> > Is this paranoia, or can this test really succeed? I thought that all
> > children of an i2c adapter node would always be i2c clients.
> 
> Let's call it well-founded paranoia.  I know that when the SPI
> code got a "remove class_device" patch, I had to fix an oops
> caused by an unexpected child ... and at the top of my current
> GIT snapshot is 911833440b498e3e4fe2f12c1ae2bd44400c7004 which
> fixed similar oopsing in SCSI.

OK, fine with me.

> > > --- a/drivers/i2c/i2c-dev.c
> > > +++ b/drivers/i2c/i2c-dev.c
> > > 		...
> > > -	}
> > > -	mutex_unlock(&adapter->clist_lock);
> > > +	if (!dev->bus || strcmp("i2c", dev->bus->name) != 0)
> > 
> > Please just export i2c_bus_type is you need it, or even verify_client
> > (then renamed to i2c_verify_client)? If the bus type check is really
> > needed then we'll need it for the 3 v4l drivers I mentioned earlier
> > anyway.
> 
> Good point.  I didn't like this part much, and that function can
> have other uses.  I updated kobj_to_i2c_client() to use it too.

I wouldn't change kobj_to_i2c_client(). Drivers using it already know
that their kobj is an i2c_client, so there's no need to check for that,
and the use cases I've seen are in runtime paths that should be fast, I
don't want to slow them down without a good reason.

> > Alternatively we could write and export an i2c_for_each_client()
> > function doing all the required checks so that drivers don't have to
> > care. What do you think?
> 
> Less good.  We can't know in advance which things they care about.

device_for_each_child() doesn't know either, and lets the caller pass
an arbitrary pointer as a parameter to the callbask function. My idea
was to do something similar, so i2c_for_each_client() would essentially
be a wrapper for device_for_each_child().

Here's what I have:

---
 drivers/i2c/i2c-core.c        |   28 ++++++++++++++++++++++++++++
 drivers/i2c/i2c-dev.c         |   10 ++++------
 drivers/media/video/dpc7146.c |    8 ++------
 drivers/media/video/mxb.c     |    8 ++------
 drivers/media/video/tvmixer.c |    9 +++------
 include/linux/i2c.h           |    3 +++
 6 files changed, 42 insertions(+), 24 deletions(-)

--- linux-2.6.24-rc7.orig/drivers/i2c/i2c-core.c	2008-01-08 14:19:39.000000000 +0100
+++ linux-2.6.24-rc7/drivers/i2c/i2c-core.c	2008-01-08 15:09:38.000000000 +0100
@@ -215,6 +215,34 @@ struct i2c_client *i2c_verify_client(str
 }
 EXPORT_SYMBOL(i2c_verify_client);
 
+struct i2c_for_each_client_data {
+	void *data;
+	int (*fn)(struct i2c_client *, void *);
+};
+
+static int __i2c_for_each_client(struct device *dev, void *d)
+{
+	struct i2c_for_each_client_data *_data = d;
+	struct i2c_client *client = i2c_verify_client(dev);
+
+	if (!client)
+		return 0;
+
+	return _data->fn(client, _data->data);
+}
+
+int i2c_for_each_client(struct i2c_adapter *adapter, void *data,
+			int (*fn)(struct i2c_client *, void *))
+{
+	struct i2c_for_each_client_data _data;
+
+	_data.data = data;
+	_data.fn = fn;
+
+	return device_for_each_child(&adapter->dev, &_data,
+				     __i2c_for_each_client);
+}
+EXPORT_SYMBOL(i2c_for_each_client);
 
 /**
  * i2c_new_device - instantiate an i2c device for use with a new style driver
--- linux-2.6.24-rc7.orig/include/linux/i2c.h	2008-01-08 14:19:15.000000000 +0100
+++ linux-2.6.24-rc7/include/linux/i2c.h	2008-01-08 15:09:34.000000000 +0100
@@ -181,6 +181,9 @@ struct i2c_client {
 
 extern struct i2c_client *i2c_verify_client(struct device *dev);
 
+extern int i2c_for_each_client(struct i2c_adapter *adapter, void *data,
+			       int (*fn)(struct i2c_client *, void *));
+
 static inline struct i2c_client *kobj_to_i2c_client(struct kobject *kobj)
 {
 	return i2c_verify_client(container_of(kobj, struct device, kobj));
--- linux-2.6.24-rc7.orig/drivers/media/video/dpc7146.c	2008-01-08 14:37:41.000000000 +0100
+++ linux-2.6.24-rc7/drivers/media/video/dpc7146.c	2008-01-08 14:59:28.000000000 +0100
@@ -87,13 +87,9 @@ struct dpc
 	int cur_input;	/* current input */
 };
 
-static int dpc_check_clients(struct device *dev, void *data)
+static int dpc_check_clients(struct i2c_client *client, void *data)
 {
 	struct dpc* dpc = data;
-	struct i2c_client *client = i2c_verify_client(dev);
-
-	if( !client )
-		return 0;
 
 	if( I2C_SAA7111A == client->addr )
 		dpc->saa7111a = client;
@@ -128,7 +124,7 @@ static int dpc_probe(struct saa7146_dev*
 	}
 
 	/* loop through all i2c-devices on the bus and look who is there */
-	device_for_each_child(&dpc->i2c_adapter.dev, dpc, dpc_check_clients);
+	i2c_for_each_client(&dpc->i2c_adapter, dpc, dpc_check_clients);
 
 	/* check if all devices are present */
 	if( 0 == dpc->saa7111a ) {
--- linux-2.6.24-rc7.orig/drivers/media/video/mxb.c	2008-01-08 14:37:33.000000000 +0100
+++ linux-2.6.24-rc7/drivers/media/video/mxb.c	2008-01-08 15:00:12.000000000 +0100
@@ -149,13 +149,9 @@ struct mxb
 
 static struct saa7146_extension extension;
 
-static int mxb_check_clients(struct device *dev, void *data)
+static int mxb_check_clients(struct i2c_client *client, void *data)
 {
 	struct mxb* mxb = data;
-	struct i2c_client *client = i2c_verify_client(dev);
-
-	if( !client )
-		return 0;
 
 	if( I2C_ADDR_TEA6420_1 == client->addr )
 		mxb->tea6420_1 = client;
@@ -218,7 +214,7 @@ static int mxb_probe(struct saa7146_dev*
 	}
 
 	/* loop through all i2c-devices on the bus and look who is there */
-	device_for_each_child(&mxb->i2c_adapter.dev, mxb, mxb_check_clients);
+	i2c_for_each_client(&mxb->i2c_adapter, mxb, mxb_check_clients);
 
 	/* check if all devices are present */
 	if(    0 == mxb->tea6420_1	|| 0 == mxb->tea6420_2	|| 0 == mxb->tea6415c
--- linux-2.6.24-rc7.orig/drivers/media/video/tvmixer.c	2008-01-08 14:34:30.000000000 +0100
+++ linux-2.6.24-rc7/drivers/media/video/tvmixer.c	2008-01-08 15:01:27.000000000 +0100
@@ -235,18 +235,15 @@ static const struct file_operations tvmi
 
 /* ----------------------------------------------------------------------- */
 
-static int __tvmixer_adapters(struct device *dev, void *data)
+static int __tvmixer_adapters(struct i2c_client *client, void *data)
 {
-	struct i2c_client *client = i2c_verify_client(dev);
-
-	if (client)
-		tvmixer_clients(client);
+	tvmixer_clients(client);
 	return 0;
 }
 
 static int tvmixer_adapters(struct i2c_adapter *adap)
 {
-	device_for_each_child(&adap->dev, NULL, __tvmixer_adapters);
+	i2c_for_each_client(adap, NULL, __tvmixer_adapters);
 	return 0;
 }
 
--- linux-2.6.24-rc7.orig/drivers/i2c/i2c-dev.c	2008-01-08 14:19:15.000000000 +0100
+++ linux-2.6.24-rc7/drivers/i2c/i2c-dev.c	2008-01-08 15:05:25.000000000 +0100
@@ -182,14 +182,12 @@ static ssize_t i2cdev_write (struct file
 	return ret;
 }
 
-static int i2cdev_check(struct device *dev, void *addrp)
+static int i2cdev_check(struct i2c_client *client, void *addrp)
 {
-	struct i2c_client *client = i2c_verify_client(dev);
-
-	if (!client || client->addr != *(unsigned int *)addrp)
+	if (client->addr != *(unsigned int *)addrp)
 		return 0;
 
-	return dev->driver ? -EBUSY : 0;
+	return client->dev.driver ? -EBUSY : 0;
 }
 
 /* This address checking function differs from the one in i2c-core
@@ -197,7 +195,7 @@ static int i2cdev_check(struct device *d
    driver to it, as NOT busy. */
 static int i2cdev_check_addr(struct i2c_adapter *adapter, unsigned int addr)
 {
-	return device_for_each_child(&adapter->dev, &addr, i2cdev_check);
+	return i2c_for_each_client(adapter, &addr, i2cdev_check);
 }
 
 static int i2cdev_ioctl(struct inode *inode, struct file *file,

Admittedly it will slow down things a bit as each iteration of the loop
will have one additional level of indirection. This makes the calling
code somewhat simpler though. Whether it is worth the additional
runtime cost, I just don't know. What do you think?

> Though I was a bit worried it might have bugs in it, since all I had
> time to do was code it, and sanity check it with a couple variants of
> one new-style config.  (I no longer have those monster "build all I2C
> drivers" configs around.)

I'll give it good testing, don't worry too much about that.

-- 
Jean Delvare

Re: i2c-remove-redundant-i2c_client-list.patch

[Jean Delvare]

On Sun, 6 Jan 2008 11:43:33 -0800, David Brownell wrote:
> Remove further duplication between i2c core and driver model:  the
> per-adapter list of clients (adapter->clients,  client->list) and
> its lock (adapter->clist_lock) duplicate adapter->dev.children.
> 
> Add and use a new i2c_verify_client() routine to help code which
> traverses the driver model tree.
>  
> Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> ---
> LIGHTLY TESTED ... goes on top of two patches from Jean's I2C queue,
> 
> 	i2c-remove-redundant-i2c_adapter-list.patch
> 	i2c-remove-redundant-i2c_driver-list.patch
> 
> Updated per Jean's comments.  (Note that the i2c_verify_client
> bit might usefully be split out into a separate patch, to simplify
> converting the V4L code that uses this i2c client list.)

Agreed, I'll split things up as needed so that we have a clean
incremental patch set.

> (...)
>  static inline struct i2c_client *kobj_to_i2c_client(struct kobject *kobj)
>  {
> -	struct device * const dev = container_of(kobj, struct device, kobj);
> -	return to_i2c_client(dev);
> +	return i2c_verify_client(container_of(kobj, struct device, kobj));
>  }

Unless you have a reason to think that this is really needed, I think
I'll back this change out as explained in my previous post. All the
rest looks just fine to me, thanks!

-- 
Jean Delvare

Re: i2c-remove-redundant-i2c_client-list.patch

[Jean Delvare]

Hi David,

On Tue, 8 Jan 2008 15:18:17 +0100, Jean Delvare wrote:
> On Sun, 6 Jan 2008 11:30:31 -0800, David Brownell wrote:
> > Though I was a bit worried it might have bugs in it, since all I had
> > time to do was code it, and sanity check it with a couple variants of
> > one new-style config.  (I no longer have those monster "build all I2C
> > drivers" configs around.)
> 
> I'll give it good testing, don't worry too much about that.

Hmm, I get a lockup when removing any legacy chip driver or any bus
driver with legacy clients attached. It seems to remove the 1st client
(no longer visible in sysfs) but never goes on with the second one.

Call Trace:
  [<ffffffff802bdbd1>] sysfs_addrm_finish+0x191/0x1e0
  [<ffffffff8041bca5>] schedule_timeout+0x95/0xd0
  [<ffffffff802bdc50>] remove_dir+0x30/0x40
  [<ffffffff802bdcb6>] sysfs_remove_dir+0x56/0x70
  [<ffffffff8041bab8>] wait_for_common+0xc8/0x130
  [<ffffffff80227e40>] default_wake_function+0x0/0x10
  [<ffffffff80386e55>] device_del+0x1b5/0x290
  [<ffffffff803ae4ce>] i2c_detach_client+0x4e/0x90
  [<ffffffff882f0071>] :lm90:lm90_detach_client+0x71/0xa0
  [<ffffffff803ae8ff>] detach_all_clients+0x6f/0xb0
  [<ffffffff803ae890>] detach_all_clients+0x0/0xb0
  [<ffffffff8038674d>] device_for_each_child+0x2d/0x60
  [<ffffffff803af315>] i2c_del_adapter+0xa5/0x140
  [<ffffffff8800b285>] :i2c_parport:i2c_parport_detach+0x55/0x110
  [<ffffffff88000796>] :parport:parport_unregister_driver+0x46/0x80
  [<ffffffff8024f001>] sys_delete_module+0x141/0x1e0
  [<ffffffff8020b92e>] system_call+0x7e/0x83

Are you certain that it is safe to remove a device's child while
walking the list of children? device_for_each_child() relies on a
klist_iter, and the documentation of klist_next mentions increasing the
reference count of the "active" list item. I would guess that you can't
remove a list item while its reference count is not zero, and that
would explain the lockup. The original code used list_for_each_safe(),
which explicitly allows the removal of the items while walking the list.

OTOH the header comment of lib/klist.c says:

 *	The entire point is to provide an interface for iterating over a list
 *	that is safe and allows for modification of the list during the
 *	iteration (e.g. insertion and removal), including modification of the
 *	current node on the list.

So it is supposed to work somehow...

-- 
Jean Delvare

Re: i2c-remove-redundant-i2c_client-list.patch

[David Brownell]

On Tuesday 08 January 2008, Jean Delvare wrote:
> > Good point.  I didn't like this part much, and that function can
> > have other uses.  I updated kobj_to_i2c_client() to use it too.
>
> I wouldn't change kobj_to_i2c_client(). Drivers using it already know
> that their kobj is an i2c_client, so there's no need to check for that,
> and the use cases I've seen are in runtime paths that should be fast, I
> don't want to slow them down without a good reason.

Hmm, Mr. Grep says all the users of that are in drivers/i2c/chips ...
basically mapping an eeprom binary attribute's kobj to the i2c_client
of the base device.  I have to say that not only would I not call
those "should be fast" paths (how often does anyone read eeproms??),
but I also have no problem using

        client = to_i2c_client(container_of(kobj, struct device, kobj));

instead of a fancy kobj_to_i2c_client() ... in fact, I would never
have thought to look for any kobj_to_*() utilities.  Although Mr. Grep
tellse me there are a few others floating around; back it out if you
feel strongly about it.


> -       return device_for_each_child(&adapter->dev, &addr, i2cdev_check);
> +       return i2c_for_each_client(adapter, &addr, i2cdev_check);
>
>		...
> 
> Admittedly it will slow down things a bit as each iteration of the loop
> will have one additional level of indirection. This makes the calling
> code somewhat simpler though. Whether it is worth the additional
> runtime cost, I just don't know. What do you think?

My bias in such cases is towards interfaces that force less policy.
So in this case that would be to expose i2_verify_client() instead
of an i2c-specific iterator.  The fact that it's got lower callstack
overhead is a bonus.  :)

Re: i2c-remove-redundant-i2c_client-list.patch

[David Brownell]

On Tuesday 08 January 2008, Jean Delvare wrote:
> Hi David,
> 
> On Tue, 8 Jan 2008 15:18:17 +0100, Jean Delvare wrote:
> > On Sun, 6 Jan 2008 11:30:31 -0800, David Brownell wrote:
> > > Though I was a bit worried it might have bugs in it, since all I had
> > > time to do was code it, and sanity check it with a couple variants of
> > > one new-style config.  (I no longer have those monster "build all I2C
> > > drivers" configs around.)
> > 
> > I'll give it good testing, don't worry too much about that.
> 
> Hmm, I get a lockup when removing any legacy chip driver or any bus
> driver with legacy clients attached. 

Lockup?  More details ...


> It seems to remove the 1st client 
> (no longer visible in sysfs) but never goes on with the second one.
> 
> Call Trace:
>   [<ffffffff802bdbd1>] sysfs_addrm_finish+0x191/0x1e0

What's it doing at that line in the call?

>   [<ffffffff8041bca5>] schedule_timeout+0x95/0xd0

This looks like stack garbage...

>   [<ffffffff802bdc50>] remove_dir+0x30/0x40
>   [<ffffffff802bdcb6>] sysfs_remove_dir+0x56/0x70
>   [<ffffffff8041bab8>] wait_for_common+0xc8/0x130
>   [<ffffffff80227e40>] default_wake_function+0x0/0x10
>   [<ffffffff80386e55>] device_del+0x1b5/0x290
>   [<ffffffff803ae4ce>] i2c_detach_client+0x4e/0x90
>   [<ffffffff882f0071>] :lm90:lm90_detach_client+0x71/0xa0
>   [<ffffffff803ae8ff>] detach_all_clients+0x6f/0xb0
>   [<ffffffff803ae890>] detach_all_clients+0x0/0xb0
>   [<ffffffff8038674d>] device_for_each_child+0x2d/0x60
>   [<ffffffff803af315>] i2c_del_adapter+0xa5/0x140
>   [<ffffffff8800b285>] :i2c_parport:i2c_parport_detach+0x55/0x110
>   [<ffffffff88000796>] :parport:parport_unregister_driver+0x46/0x80
>   [<ffffffff8024f001>] sys_delete_module+0x141/0x1e0
>   [<ffffffff8020b92e>] system_call+0x7e/0x83
> 
> Are you certain that it is safe to remove a device's child while
> walking the list of children?

As you point out below, it's supposed to be safe ...


> device_for_each_child() relies on a 
> klist_iter, and the documentation of klist_next mentions increasing the
> reference count of the "active" list item. I would guess that you can't
> remove a list item while its reference count is not zero, and that
> would explain the lockup. The original code used list_for_each_safe(),
> which explicitly allows the removal of the items while walking the list.
> 
> OTOH the header comment of lib/klist.c says:
> 
>  *	The entire point is to provide an interface for iterating over a list
>  *	that is safe and allows for modification of the list during the
>  *	iteration (e.g. insertion and removal), including modification of the
>  *	current node on the list.
> 
> So it is supposed to work somehow...

Yeah, but then again ... the i2c stack does that oddball stuff with
complete(&client->released) for legacy drivers, too.  That's always
been contrary to what the driver model expects, and I never quite
bothered to sort out the issues.  Maybe this is one of them (sigh).

- Dave

Re: i2c-remove-redundant-i2c_client-list.patch

[Jean Delvare]

On Tue, 8 Jan 2008 10:52:31 -0800, David Brownell wrote:
> On Tuesday 08 January 2008, Jean Delvare wrote:
> > > Good point.  I didn't like this part much, and that function can
> > > have other uses.  I updated kobj_to_i2c_client() to use it too.
> >
> > I wouldn't change kobj_to_i2c_client(). Drivers using it already know
> > that their kobj is an i2c_client, so there's no need to check for that,
> > and the use cases I've seen are in runtime paths that should be fast, I
> > don't want to slow them down without a good reason.
> 
> Hmm, Mr. Grep says all the users of that are in drivers/i2c/chips ...
> basically mapping an eeprom binary attribute's kobj to the i2c_client
> of the base device.  I have to say that not only would I not call
> those "should be fast" paths (how often does anyone read eeproms??),

Sorry, I expressed myself incorrectly, I didn't mean to intend that
these were hot paths, just that this was run-time code and not
initialization code, which means that the speed matters a little more.
But the key argument here is that the current code is not broken so I
couldn't see the rationale for touching it.

> but I also have no problem using
> 
>         client = to_i2c_client(container_of(kobj, struct device, kobj));
> 
> instead of a fancy kobj_to_i2c_client() ... in fact, I would never
> have thought to look for any kobj_to_*() utilities.  Although Mr. Grep
> tellse me there are a few others floating around; back it out if you
> feel strongly about it.

I seem to remember that kobj_to_i2c_client() was originally declared
inside one of these drivers by the driver author and I suggested to
move it to i2c.h.

> > -       return device_for_each_child(&adapter->dev, &addr, i2cdev_check);
> > +       return i2c_for_each_client(adapter, &addr, i2cdev_check);
> >
> >		...
> > 
> > Admittedly it will slow down things a bit as each iteration of the loop
> > will have one additional level of indirection. This makes the calling
> > code somewhat simpler though. Whether it is worth the additional
> > runtime cost, I just don't know. What do you think?
> 
> My bias in such cases is towards interfaces that force less policy.
> So in this case that would be to expose i2_verify_client() instead
> of an i2c-specific iterator.  The fact that it's got lower callstack
> overhead is a bonus.  :)

OK, fine with me, I'll forget about it then.

-- 
Jean Delvare

Re: i2c-remove-redundant-i2c_client-list.patch

[Jean Delvare]

On Tue, 8 Jan 2008 11:12:46 -0800, David Brownell wrote:
> On Tuesday 08 January 2008, Jean Delvare wrote:
> > Hi David,
> > 
> > On Tue, 8 Jan 2008 15:18:17 +0100, Jean Delvare wrote:
> > > On Sun, 6 Jan 2008 11:30:31 -0800, David Brownell wrote:
> > > > Though I was a bit worried it might have bugs in it, since all I had
> > > > time to do was code it, and sanity check it with a couple variants of
> > > > one new-style config.  (I no longer have those monster "build all I2C
> > > > drivers" configs around.)
> > > 
> > > I'll give it good testing, don't worry too much about that.
> > 
> > Hmm, I get a lockup when removing any legacy chip driver or any bus
> > driver with legacy clients attached. 
> 
> Lockup?  More details ...

There's not much to say. "rmmod lm90" (or "rmmod i2c-parport") never
returns, that's about it.

> > It seems to remove the 1st client 
> > (no longer visible in sysfs) but never goes on with the second one.
> > 
> > Call Trace:
> >   [<ffffffff802bdbd1>] sysfs_addrm_finish+0x191/0x1e0
> 
> What's it doing at that line in the call?

(From another run, thus the slightly different address:)

(gdb) list *(sysfs_addrm_finish+0x191)
0xffffffff802bdc01 is in sysfs_addrm_finish (fs/sysfs/sysfs.h:138).
133     }
134
135     static inline void sysfs_put(struct sysfs_dirent *sd)
136     {
137             if (sd && atomic_dec_and_test(&sd->s_count))
138                     release_sysfs_dirent(sd);
139     }
140
141     /*
142      * inode.c
(gdb)

If you need more info, feel free to ask.

> >   [<ffffffff8041bca5>] schedule_timeout+0x95/0xd0
> 
> This looks like stack garbage...

What makes you think so? A second stack trace shows it as well.

> 
> >   [<ffffffff802bdc50>] remove_dir+0x30/0x40
> >   [<ffffffff802bdcb6>] sysfs_remove_dir+0x56/0x70
> >   [<ffffffff8041bab8>] wait_for_common+0xc8/0x130
> >   [<ffffffff80227e40>] default_wake_function+0x0/0x10
> >   [<ffffffff80386e55>] device_del+0x1b5/0x290
> >   [<ffffffff803ae4ce>] i2c_detach_client+0x4e/0x90
> >   [<ffffffff882f0071>] :lm90:lm90_detach_client+0x71/0xa0
> >   [<ffffffff803ae8ff>] detach_all_clients+0x6f/0xb0
> >   [<ffffffff803ae890>] detach_all_clients+0x0/0xb0
> >   [<ffffffff8038674d>] device_for_each_child+0x2d/0x60
> >   [<ffffffff803af315>] i2c_del_adapter+0xa5/0x140
> >   [<ffffffff8800b285>] :i2c_parport:i2c_parport_detach+0x55/0x110
> >   [<ffffffff88000796>] :parport:parport_unregister_driver+0x46/0x80
> >   [<ffffffff8024f001>] sys_delete_module+0x141/0x1e0
> >   [<ffffffff8020b92e>] system_call+0x7e/0x83
> > 
> > Are you certain that it is safe to remove a device's child while
> > walking the list of children?
> 
> As you point out below, it's supposed to be safe ...
> 
> 
> > device_for_each_child() relies on a 
> > klist_iter, and the documentation of klist_next mentions increasing the
> > reference count of the "active" list item. I would guess that you can't
> > remove a list item while its reference count is not zero, and that
> > would explain the lockup. The original code used list_for_each_safe(),
> > which explicitly allows the removal of the items while walking the list.
> > 
> > OTOH the header comment of lib/klist.c says:
> > 
> >  *	The entire point is to provide an interface for iterating over a list
> >  *	that is safe and allows for modification of the list during the
> >  *	iteration (e.g. insertion and removal), including modification of the
> >  *	current node on the list.
> > 
> > So it is supposed to work somehow...
> 
> Yeah, but then again ... the i2c stack does that oddball stuff with
> complete(&client->released) for legacy drivers, too.  That's always
> been contrary to what the driver model expects, and I never quite
> bothered to sort out the issues.  Maybe this is one of them (sigh).

I see. I doubt I'll be of much help, as I could never figure out what
this completion stuff was for in the first place.

-- 
Jean Delvare

Re: i2c-remove-redundant-i2c_client-list.patch

[David Brownell]

On Tuesday 08 January 2008, Jean Delvare wrote:
> On Tue, 8 Jan 2008 11:12:46 -0800, David Brownell wrote:
> > On Tuesday 08 January 2008, Jean Delvare wrote:
> > > On Tue, 8 Jan 2008 15:18:17 +0100, Jean Delvare wrote:
> > > > On Sun, 6 Jan 2008 11:30:31 -0800, David Brownell wrote:
> > > > > Though I was a bit worried it might have bugs in it, since all I had
> > > > > time to do was code it, and sanity check it with a couple variants of
> > > > > one new-style config.  (I no longer have those monster "build all I2C
> > > > > drivers" configs around.)
> > > > 
> > > > I'll give it good testing, don't worry too much about that.
> > > 
> > > Hmm, I get a lockup when removing any legacy chip driver or any bus
> > > driver with legacy clients attached. 
> > 
> > Lockup?  More details ...
> 
> There's not much to say. "rmmod lm90" (or "rmmod i2c-parport") never
> returns, that's about it.

The rest of the system behaves, or not?  A "lockup" to me implies
something so catastrophic that the hardware watchdog will soon fire
and the system will reboot.

I'm guessing that's not the case here.  Stil not-good, but not as
much of a catastrophe.


> > > It seems to remove the 1st client 
> > > (no longer visible in sysfs) but never goes on with the second one.
> > > 
> > > Call Trace:
> > >   [<ffffffff802bdbd1>] sysfs_addrm_finish+0x191/0x1e0
> > 
> > What's it doing at that line in the call?
> 
> (From another run, thus the slightly different address:)
> 
> (gdb) list *(sysfs_addrm_finish+0x191)
> 0xffffffff802bdc01 is in sysfs_addrm_finish (fs/sysfs/sysfs.h:138).
> 133     }
> 134
> 135     static inline void sysfs_put(struct sysfs_dirent *sd)
> 136     {
> 137             if (sd && atomic_dec_and_test(&sd->s_count))
> 138                     release_sysfs_dirent(sd);

This isn't wholly meaningful to me.  If release_sysfs_dirent() is
wedging, then why does the stack show it's sysfs_addrm_finish()?
I guess the right question is what's going on where the PC points.  :)


> 139     }
> 140
> 141     /*
> 142      * inode.c
> (gdb)
> 
> If you need more info, feel free to ask.
> 
> > >   [<ffffffff8041bca5>] schedule_timeout+0x95/0xd0
> > 
> > This looks like stack garbage...
> 
> What makes you think so? A second stack trace shows it as well.

Because schedule_timeout() doesn't call into sysfs.  :)

Some platforms have compiler options to make stack tracing be
more sensible -- e.g. to always force frame pointers to be used,
so that stackdump utilities don't ever need to guess.

 
> > > 			device_for_each_child() relies on a 
> > > klist_iter, and the documentation of klist_next mentions increasing the
> > > reference count of the "active" list item. I would guess that you can't
> > > remove a list item while its reference count is not zero, and that
> > > would explain the lockup. The original code used list_for_each_safe(),
> > > which explicitly allows the removal of the items while walking the list.
> > > 
> > > OTOH the header comment of lib/klist.c says:
> > > 
> > >  *  The entire point is to provide an interface for iterating over a list
> > >  *  that is safe and allows for modification of the list during the
> > >  *  iteration (e.g. insertion and removal), including modification of the
> > >  *  current node on the list.
> > >
> > > So it is supposed to work somehow...
> > 
> > Yeah, but then again ... the i2c stack does that oddball stuff with
> > complete(&client->released) for legacy drivers, too.  That's always
> > been contrary to what the driver model expects, and I never quite
> > bothered to sort out the issues.  Maybe this is one of them (sigh).
> 
> I see. I doubt I'll be of much help, as I could never figure out what
> this completion stuff was for in the first place.

In which case I'm guessing it's stuff Greg did way back in the day.
I cc'd him in hope that he can find time to comment on that ...

My understanding is that it was written early in sysfs conversions
to help ensure sysfs wasn't keeping an open handle on the device.
However, that early scheme proved to be quite error prone and now
things are set up differently.  There was a particularly annoying
class of problem with module removal.  Devices allocated by a driver
needed to be freed by that driver, which implies not unloading any
driver's module until device nodes it had allocated were freed.

The simple way around that restriction involves never having modules
allocate such devices ... clearly not easy for legacy I2C drivers.
Another way around it was to synchronize driver removal with the
cleanup of those devices.  And that's what the current I2C stack is
(still) doing for legacy drivers, with that completion logic.

- Dave

p.s. Greg, for context:  there are two patches in Jean's I2C queue
     to remove some redundant I2C lists, in favor of using versions
     maintained by i2c-core.  $SUBJECT relates to a patch removing
     a third redundancy:  i2c_adapter.clients and i2c_client.list,
     plus (the original problem) the lock for that list.

Re: i2c-remove-redundant-i2c_client-list.patch

[Greg KH]

On Tue, Jan 08, 2008 at 01:44:45PM -0800, David Brownell wrote:
> On Tuesday 08 January 2008, Jean Delvare wrote:
> > > > 			device_for_each_child() relies on a 
> > > > klist_iter, and the documentation of klist_next mentions increasing the
> > > > reference count of the "active" list item. I would guess that you can't
> > > > remove a list item while its reference count is not zero, and that
> > > > would explain the lockup. The original code used list_for_each_safe(),
> > > > which explicitly allows the removal of the items while walking the list.
> > > > 
> > > > OTOH the header comment of lib/klist.c says:
> > > > 
> > > > ?*??The entire point is to provide an interface for iterating over a list
> > > > ?*??that is safe and allows for modification of the list during the
> > > > ?*??iteration (e.g. insertion and removal), including modification of the
> > > > ?*??current node on the list.
> > > >
> > > > So it is supposed to work somehow...
> > > 
> > > Yeah, but then again ... the i2c stack does that oddball stuff with
> > > complete(&client->released) for legacy drivers, too.  That's always
> > > been contrary to what the driver model expects, and I never quite
> > > bothered to sort out the issues.  Maybe this is one of them (sigh).
> > 
> > I see. I doubt I'll be of much help, as I could never figure out what
> > this completion stuff was for in the first place.
> 
> In which case I'm guessing it's stuff Greg did way back in the day.
> I cc'd him in hope that he can find time to comment on that ...

Ick, sorry about that mess :(

> My understanding is that it was written early in sysfs conversions
> to help ensure sysfs wasn't keeping an open handle on the device.
> However, that early scheme proved to be quite error prone and now
> things are set up differently.  There was a particularly annoying
> class of problem with module removal.  Devices allocated by a driver
> needed to be freed by that driver, which implies not unloading any
> driver's module until device nodes it had allocated were freed.

Yes, that should not be used anymore, and I thought we had removed it
for some reason.  I guess I was just wishing it were so...

> The simple way around that restriction involves never having modules
> allocate such devices ... clearly not easy for legacy I2C drivers.
> Another way around it was to synchronize driver removal with the
> cleanup of those devices.  And that's what the current I2C stack is
> (still) doing for legacy drivers, with that completion logic.
> 
> - Dave
> 
> p.s. Greg, for context:  there are two patches in Jean's I2C queue
>      to remove some redundant I2C lists, in favor of using versions
>      maintained by i2c-core.  $SUBJECT relates to a patch removing
>      a third redundancy:  i2c_adapter.clients and i2c_client.list,
>      plus (the original problem) the lock for that list.

That sounds like a good idea, if the above race doesn't happen :)

Anything I can do to help out?

thanks,

greg k-h

Re: i2c-remove-redundant-i2c_client-list.patch

[David Brownell]

On Tuesday 08 January 2008, Greg KH wrote:
> > > > > So it is supposed to work somehow...
> > > > 
> > > > Yeah, but then again ... the i2c stack does that oddball stuff with
> > > > complete(&client->released) for legacy drivers, too.

Also the i2c_adapter devices, which might have a similar issue lurking...


> > > > 		That's always 
> > > > been contrary to what the driver model expects, and I never quite
> > > > bothered to sort out the issues.  Maybe this is one of them (sigh).
> > > 
> > > I see. I doubt I'll be of much help, as I could never figure out what
> > > this completion stuff was for in the first place.
> > 
> > In which case I'm guessing it's stuff Greg did way back in the day.
> > I cc'd him in hope that he can find time to comment on that ...
> 
> Ick, sorry about that mess :(

I was *hoping* for more than just sympathy!  ;)


> > My understanding is that it was written early in sysfs conversions
> > to help ensure sysfs wasn't keeping an open handle on the device.
> > However, that early scheme proved to be quite error prone and now
> > things are set up differently.  There was a particularly annoying
> > class of problem with module removal.  Devices allocated by a driver
> > needed to be freed by that driver, which implies not unloading any
> > driver's module until device nodes it had allocated were freed.
> 
> Yes, that should not be used anymore, and I thought we had removed it
> for some reason.  I guess I was just wishing it were so...

Do you remember any particular details about how it all got removed?
Like in particular:
 
> > The simple way around that restriction involves never having modules
> > allocate such devices ... clearly not easy for legacy I2C drivers.

Was there a better solution than that near-term unattractive one?
It involves changing lots of legacy I2C code, making them provide
proper remove methods and thus changing memory management policy ...
Error prone, needs to be done slowly.


> > Another way around it was to synchronize driver removal with the
> > cleanup of those devices.  And that's what the current I2C stack is
> > (still) doing for legacy drivers, with that completion logic.
> > 
> > - Dave
> > 
> > p.s. Greg, for context:  there are two patches in Jean's I2C queue
> >      to remove some redundant I2C lists, in favor of using versions
> >      maintained by i2c-core.  $SUBJECT relates to a patch removing
> >      a third redundancy:  i2c_adapter.clients and i2c_client.list,
> >      plus (the original problem) the lock for that list.
> 
> That sounds like a good idea, if the above race doesn't happen :)
> 
> Anything I can do to help out?

Verify that could cause the hang Jean observed ... you're more on top
of this driver model stuff than most folk.  :)

I'll forward you my copies of the three patches, so you can apply them
and see the basic results of the cleanups.  Then have a look and see if
you agree with me about that one solution being "near-term unattractive",
or can otherwise suggest a clean solution.

- Dave

Re: i2c-remove-redundant-i2c_client-list.patch

[Jean Delvare]

Hi David,

On Tue, 8 Jan 2008 13:44:45 -0800, David Brownell wrote:
> On Tuesday 08 January 2008, Jean Delvare wrote:
> > On Tue, 8 Jan 2008 11:12:46 -0800, David Brownell wrote:
> > > On Tuesday 08 January 2008, Jean Delvare wrote:
> > > > Hmm, I get a lockup when removing any legacy chip driver or any bus
> > > > driver with legacy clients attached. 
> > > 
> > > Lockup?  More details ...
> > 
> > There's not much to say. "rmmod lm90" (or "rmmod i2c-parport") never
> > returns, that's about it.
> 
> The rest of the system behaves, or not?  A "lockup" to me implies
> something so catastrophic that the hardware watchdog will soon fire
> and the system will reboot.
> 
> I'm guessing that's not the case here.  Stil not-good, but not as
> much of a catastrophe.

Sorry for being vague. Yes, the rest of the system behaves, only the
"rmmod" process is stuck.

> This isn't wholly meaningful to me.  If release_sysfs_dirent() is
> wedging, then why does the stack show it's sysfs_addrm_finish()?
> I guess the right question is what's going on where the PC points.  :)

How do I know? I'm using SysRq+t to get the stack trace, it doesn't
seem to give me any extra information.

> > > >   [<ffffffff8041bca5>] schedule_timeout+0x95/0xd0
> > > 
> > > This looks like stack garbage...
> > 
> > What makes you think so? A second stack trace shows it as well.
> 
> Because schedule_timeout() doesn't call into sysfs.  :)

True, but OTOH there are so many ways callback can be stored for later
use that I am no longer surprised when I see apparently unrelated
functions call each other ;)

> Some platforms have compiler options to make stack tracing be
> more sensible -- e.g. to always force frame pointers to be used,
> so that stackdump utilities don't ever need to guess.

I rebuilt my kernel with CONFIG_STACKTRACE=y and
CONFIG_FRAME_POINTER=y, hopefully that's what you were thinking about.
Here's what I get with these options:

rmmod         D 0000000000000002     0  4221   4204
 ffff81002345bcd8 0000000000000046 ffff8100234ab080 ffff81003f848040
 ffff81002345bd08 ffffffff80252327 0000000200000001 7fffffffffffffff
 7fffffffffffffff ffff81002d1e6b60 ffff81002d1e6b58 0000000000000002
Call Trace:
 [<ffffffff80252327>] __lock_acquire+0x967/0x1080
 [<ffffffff80443215>] schedule_timeout+0x95/0xd0
 [<ffffffff8044545b>] _spin_unlock_irq+0x2b/0x40
 [<ffffffff80251485>] trace_hardirqs_on+0xd5/0x170
 [<ffffffff8044545b>] _spin_unlock_irq+0x2b/0x40
 [<ffffffff8044301f>] wait_for_common+0xdf/0x150
 [<ffffffff802292a0>] default_wake_function+0x0/0x10
 [<ffffffff804430f8>] wait_for_completion+0x18/0x20
 [<ffffffff803ce894>] i2c_detach_client+0x54/0x90
 [<ffffffff88309071>] :lm90:lm90_detach_client+0x71/0x90
 [<ffffffff803cd85c>] detach_legacy_clients+0x8c/0xc0
 [<ffffffff803cd7d0>] detach_legacy_clients+0x0/0xc0
 [<ffffffff803a4f13>] device_for_each_child+0x33/0x60
 [<ffffffff803cebe6>] i2c_del_driver+0x126/0x140
 [<ffffffff88309f10>] :lm90:sensors_lm90_exit+0x10/0x12
 [<ffffffff8025a51c>] sys_delete_module+0x13c/0x1e0
 [<ffffffff80251485>] trace_hardirqs_on+0xd5/0x170
 [<ffffffff80444ada>] trace_hardirqs_on_thunk+0x35/0x3a
 [<ffffffff8020ba3e>] system_call+0x7e/0x83

Does it make more sense? The odd thing is:

(gdb) list *(__lock_acquire+0x967)
0xffffffff80252327 is in __lock_acquire (kernel/lockdep.c:2353).
2348     * We maintain the dependency maps and validate the locking attempt:
2349     */
2350    static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
2351                              int trylock, int read, int check, int hardirqs_off,
2352                              unsigned long ip)
2353    {
2354            struct task_struct *curr = current;
2355            struct lock_class *class = NULL;
2356            struct held_lock *hlock;
2357            unsigned int depth, id;
(gdb)

Not sure how it can block on an opening curly brace ;) Maybe I should
disable CONFIG_LOCKDEP for now.

-- 
Jean Delvare

Re: i2c-remove-redundant-i2c_client-list.patch

[Jean Delvare]

Quoting myself:
> Admittedly it will slow down things a bit as each iteration of the loop
> will have one additional level of indirection. This makes the calling
> code somewhat simpler though. Whether it is worth the additional
> runtime cost, I just don't know. What do you think?

For the records, what I wrote above is not entirely true. By folding
i2c_verify_client() into __i2c_for_each_client(), it is possible to get
the exact same runtime cost (in terms of function call count) as we
have without my proposed change. So performance is not a reason for not
doing it.

-- 
Jean Delvare

Re: i2c-remove-redundant-i2c_client-list.patch

[Jean Delvare]

On Wed, 9 Jan 2008 14:29:06 +0100, Jean Delvare wrote:
> I rebuilt my kernel with CONFIG_STACKTRACE=y and
> CONFIG_FRAME_POINTER=y, hopefully that's what you were thinking about.
> Here's what I get with these options:
> 
> rmmod         D 0000000000000002     0  4221   4204
>  ffff81002345bcd8 0000000000000046 ffff8100234ab080 ffff81003f848040
>  ffff81002345bd08 ffffffff80252327 0000000200000001 7fffffffffffffff
>  7fffffffffffffff ffff81002d1e6b60 ffff81002d1e6b58 0000000000000002
> Call Trace:
>  [<ffffffff80252327>] __lock_acquire+0x967/0x1080
>  [<ffffffff80443215>] schedule_timeout+0x95/0xd0
>  [<ffffffff8044545b>] _spin_unlock_irq+0x2b/0x40
>  [<ffffffff80251485>] trace_hardirqs_on+0xd5/0x170
>  [<ffffffff8044545b>] _spin_unlock_irq+0x2b/0x40
>  [<ffffffff8044301f>] wait_for_common+0xdf/0x150
>  [<ffffffff802292a0>] default_wake_function+0x0/0x10
>  [<ffffffff804430f8>] wait_for_completion+0x18/0x20
>  [<ffffffff803ce894>] i2c_detach_client+0x54/0x90
>  [<ffffffff88309071>] :lm90:lm90_detach_client+0x71/0x90
>  [<ffffffff803cd85c>] detach_legacy_clients+0x8c/0xc0
>  [<ffffffff803cd7d0>] detach_legacy_clients+0x0/0xc0
>  [<ffffffff803a4f13>] device_for_each_child+0x33/0x60
>  [<ffffffff803cebe6>] i2c_del_driver+0x126/0x140
>  [<ffffffff88309f10>] :lm90:sensors_lm90_exit+0x10/0x12
>  [<ffffffff8025a51c>] sys_delete_module+0x13c/0x1e0
>  [<ffffffff80251485>] trace_hardirqs_on+0xd5/0x170
>  [<ffffffff80444ada>] trace_hardirqs_on_thunk+0x35/0x3a
>  [<ffffffff8020ba3e>] system_call+0x7e/0x83
> 
> Does it make more sense? The odd thing is:
> 
> (gdb) list *(__lock_acquire+0x967)
> 0xffffffff80252327 is in __lock_acquire (kernel/lockdep.c:2353).
> 2348     * We maintain the dependency maps and validate the locking attempt:
> 2349     */
> 2350    static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
> 2351                              int trylock, int read, int check, int hardirqs_off,
> 2352                              unsigned long ip)
> 2353    {
> 2354            struct task_struct *curr = current;
> 2355            struct lock_class *class = NULL;
> 2356            struct held_lock *hlock;
> 2357            unsigned int depth, id;
> (gdb)
> 
> Not sure how it can block on an opening curly brace ;) Maybe I should
> disable CONFIG_LOCKDEP for now.

Here's what I get with CONFIG_LOCKDEP=n:

rmmod         D ffffffff804365c0     0  4198   4181
 ffff81002db39cd8 0000000000000082 ffff81002d907100 ffff81003f83d7e0
 ffff81003c5d0370 ffff81002db39d08 ffff81002d8408f8 7fffffffffffffff
 7fffffffffffffff ffff81002d840a38 0000000000000002 ffff81002db39d58
Call Trace:
 [<ffffffff80423c15>] schedule_timeout+0x95/0xd0
 [<ffffffff802bfa30>] sysfs_remove_dir+0x60/0x80
 [<ffffffff80308029>] kobject_put+0x19/0x20
 [<ffffffff80423a28>] wait_for_common+0xc8/0x130
 [<ffffffff80228330>] default_wake_function+0x0/0x10
 [<ffffffff80423af8>] wait_for_completion+0x18/0x20
 [<ffffffff803b4fd4>] i2c_detach_client+0x54/0x90
 [<ffffffff882f4071>] :lm90:lm90_detach_client+0x71/0x90
 [<ffffffff803b3fbc>] detach_legacy_clients+0x8c/0xc0
 [<ffffffff803b3f30>] detach_legacy_clients+0x0/0xc0
 [<ffffffff8038cbb3>] device_for_each_child+0x33/0x60
 [<ffffffff803b531c>] i2c_del_driver+0x11c/0x140
 [<ffffffff882f4f00>] :lm90:sensors_lm90_exit+0x10/0x12
 [<ffffffff802501d2>] sys_delete_module+0x132/0x1d0
 [<ffffffff8020b99e>] system_call+0x7e/0x83

(gdb) list *(schedule_timeout+0x95)
0xffffffff80423c15 is in schedule_timeout (kernel/timer.c:1059).
1054                     * in the caller. Nothing more. We could take
1055                     * MAX_SCHEDULE_TIMEOUT from one of the negative value
1056                     * but I' d like to return a valid offset (>=0) to allow
1057                     * the caller to do everything it want with the retval.
1058                     */
1059                    schedule();
1060                    goto out;
1061            default:
1062                    /*
1063                     * Another bit of PARANOID. Note that the retval will be
(gdb)

Hope it helps,
-- 
Jean Delvare

Re: i2c-remove-redundant-i2c_client-list.patch

[David Brownell]

On Wednesday 09 January 2008, Jean Delvare wrote:
> Here's what I get with CONFIG_LOCKDEP=n:

OK, that's more like I'd expect.  Essentially a self-deadlock:

 - increment usecount on the device

 - call i2c_detach_client(), which waits till usecount falls to zero
   and then frees the device (because this is a legacy driver)

 - expect to *then* be able to decrement the usecount, and continue

Yuck.


> rmmod         D ffffffff804365c0     0  4198   4181
>  ffff81002db39cd8 0000000000000082 ffff81002d907100 ffff81003f83d7e0
>  ffff81003c5d0370 ffff81002db39d08 ffff81002d8408f8 7fffffffffffffff
>  7fffffffffffffff ffff81002d840a38 0000000000000002 ffff81002db39d58
> Call Trace:
>  [<ffffffff80423c15>] schedule_timeout+0x95/0xd0

... the schedule() call, which will be left when someone issues
the completion being waited for ... 

>  [<ffffffff802bfa30>] sysfs_remove_dir+0x60/0x80
>  [<ffffffff80308029>] kobject_put+0x19/0x20
>  [<ffffffff80423a28>] wait_for_common+0xc8/0x130
>  [<ffffffff80228330>] default_wake_function+0x0/0x10
>  [<ffffffff80423af8>] wait_for_completion+0x18/0x20

... but that completion can never arrive ...

>  [<ffffffff803b4fd4>] i2c_detach_client+0x54/0x90
>  [<ffffffff882f4071>] :lm90:lm90_detach_client+0x71/0x90
>  [<ffffffff803b3fbc>] detach_legacy_clients+0x8c/0xc0
>  [<ffffffff803b3f30>] detach_legacy_clients+0x0/0xc0
>  [<ffffffff8038cbb3>] device_for_each_child+0x33/0x60

... since this iteration holds the refount it won't yet drop ...

>  [<ffffffff803b531c>] i2c_del_driver+0x11c/0x140
>  [<ffffffff882f4f00>] :lm90:sensors_lm90_exit+0x10/0x12
>  [<ffffffff802501d2>] sys_delete_module+0x132/0x1d0
>  [<ffffffff8020b99e>] system_call+0x7e/0x83

Right now I'm thinking that we'll need a multi-phase approach:

(a) Start phasing out users of i2c_client.list and its lock, ASAP;
    merging those DVB driver patches, and some i2c-core changes.

(b) But don't remove that list from the deletion path until ...

(c) ... We have a solution that removes that wait_for_completion()
    and its infrastructure.  (Note the similar i2c_adapter logic
    too, sigh.)

(d) Meanwhile, come up with a different solution to the deadlock
    observed with i2c_adapter.clist ... which for some unknown
    reason has *NOT* shown up for me with lockdep.

Of course, if (c) happens soon soon, this problem simplifies.  And
maybe someone will come up with a non-invasive solution to that
problem ... but if nobody does so before, say, Monday, I'm thinking
that (d) becomes a priority.

- Dave

Re: i2c-remove-redundant-i2c_client-list.patch

[Jean Delvare]

On Wed, 9 Jan 2008 13:21:28 -0800, David Brownell wrote:
> Right now I'm thinking that we'll need a multi-phase approach:
> 
> (a) Start phasing out users of i2c_client.list and its lock, ASAP;
>     merging those DVB driver patches, and some i2c-core changes.

Fine with me. Can you please send the i2c-core patch? It would include
i2c_verify_client(), then I can deal with the V4L drivers patch and
these 2 patches can be pushed to -mm quickly.

> (b) But don't remove that list from the deletion path until ...
> 
> (c) ... We have a solution that removes that wait_for_completion()
>     and its infrastructure.  (Note the similar i2c_adapter logic
>     too, sigh.)
> 
> (d) Meanwhile, come up with a different solution to the deadlock
>     observed with i2c_adapter.clist ... which for some unknown
>     reason has *NOT* shown up for me with lockdep.
> 
> Of course, if (c) happens soon soon, this problem simplifies.  And
> maybe someone will come up with a non-invasive solution to that
> problem ... but if nobody does so before, say, Monday, I'm thinking
> that (d) becomes a priority.

I won't be able to help with this as I don't understand it fully,
sorry. But I can do any amount of testing if someone sends patches.

-- 
Jean Delvare

Re: i2c-remove-redundant-i2c_client-list.patch

[David Brownell]

On Thursday 10 January 2008, Jean Delvare wrote:
> On Wed, 9 Jan 2008 13:21:28 -0800, David Brownell wrote:
> > Right now I'm thinking that we'll need a multi-phase approach:
> > 
> > (a) Start phasing out users of i2c_client.list and its lock, ASAP;
> >     merging those DVB driver patches, and some i2c-core changes.
> 
> Fine with me. Can you please send the i2c-core patch? It would include
> i2c_verify_client(), then I can deal with the V4L drivers patch and
> these 2 patches can be pushed to -mm quickly.

OK, here's the trimmed-down patch that leaves the deletion
paths untouched until we have a better solution.  I've only
build-tested it, but it's basically $SUBJECT without those
troublesome paths.

- Dave

=======	CUT HERE
The i2c_adapter.clients list of i2c_client nodes duplicates driver
model state.  This patch starts removing that list, letting us remove
most existing users of those i2c-core lists.

 * The core I2C code now iterates over the driver model's list instead
   of the i2c-internal one in some places where it's safe:
      - Passing a command/ioctl to each client, a mechanims
        used almost exclusively by DVB adapters;
      - Device address checking, in both i2c-core and i2c-dev.

 * Provide i2c_verify_client() to use with driver model iterators.

 * Flag the relevant i2c_adapter and i2c_client fields as deprecated,
   to help prevent new users from appearing.

For the moment the list needs to stick around, since some issues show
up when deleting devices created by legacy I2C drivers.  (They don't
follow standard driver model rules.  Removing those devices can cause
self-deadlocks.)

Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
 drivers/i2c/i2c-core.c |   78 ++++++++++++++++++++++++++++---------------------
 drivers/i2c/i2c-dev.c  |   29 +++++++-----------
 include/linux/i2c.h    |    8 +++--
 3 files changed, 63 insertions(+), 52 deletions(-)

--- at91.orig/drivers/i2c/i2c-core.c	2008-01-14 13:56:28.000000000 -0800
+++ at91/drivers/i2c/i2c-core.c	2008-01-14 14:01:50.000000000 -0800
@@ -197,6 +197,25 @@ static struct bus_type i2c_bus_type = {
 	.resume		= i2c_device_resume,
 };
 
+
+/**
+ * i2c_verify_client - return parameter as i2c_client, or NULL
+ * @dev: device, probably from some driver model iterator
+ *
+ * When traversing the driver model tree, perhaps using driver model
+ * iterators like @device_for_each_child(), you can't assume very much
+ * about the nodes you find.  Use this function to avoid oopses caused
+ * by wrongly treating some non-I2C device as an i2c_client.
+ */
+struct i2c_client *i2c_verify_client(struct device *dev)
+{
+	return (dev->bus == &i2c_bus_type)
+			? to_i2c_client(dev)
+			: NULL;
+}
+EXPORT_SYMBOL(i2c_verify_client);
+
+
 /**
  * i2c_new_device - instantiate an i2c device for use with a new style driver
  * @adap: the adapter managing the device
@@ -713,28 +732,19 @@ EXPORT_SYMBOL(i2c_del_driver);
 
 /* ------------------------------------------------------------------------- */
 
-static int __i2c_check_addr(struct i2c_adapter *adapter, unsigned int addr)
+static int __i2c_check_addr(struct device *dev, void *addrp)
 {
-	struct list_head   *item;
-	struct i2c_client  *client;
+	struct i2c_client	*client = i2c_verify_client(dev);
+	int			addr = *(int *)addrp;
 
-	list_for_each(item,&adapter->clients) {
-		client = list_entry(item, struct i2c_client, list);
-		if (client->addr == addr)
-			return -EBUSY;
-	}
+	if (client && client->addr == addr)
+		return -EBUSY;
 	return 0;
 }
 
 static int i2c_check_addr(struct i2c_adapter *adapter, int addr)
 {
-	int rval;
-
-	mutex_lock(&adapter->clist_lock);
-	rval = __i2c_check_addr(adapter, addr);
-	mutex_unlock(&adapter->clist_lock);
-
-	return rval;
+	return device_for_each_child(&adapter->dev, &addr, __i2c_check_addr);
 }
 
 int i2c_attach_client(struct i2c_client *client)
@@ -743,7 +753,7 @@ int i2c_attach_client(struct i2c_client 
 	int res = 0;
 
 	mutex_lock(&adapter->clist_lock);
-	if (__i2c_check_addr(client->adapter, client->addr)) {
+	if (i2c_check_addr(client->adapter, client->addr)) {
 		res = -EBUSY;
 		goto out_unlock;
 	}
@@ -873,24 +883,28 @@ int i2c_release_client(struct i2c_client
 }
 EXPORT_SYMBOL(i2c_release_client);
 
+struct i2c_cmd_arg {
+	unsigned	cmd;
+	void		*arg;
+};
+
+static int i2c_cmd(struct device *dev, void *_arg)
+{
+	struct i2c_client	*client = i2c_verify_client(dev);
+	struct i2c_cmd_arg	*arg = _arg;
+
+	if (client && client->driver && client->driver->command)
+		client->driver->command(client, arg->cmd, arg->arg);
+	return 0;
+}
+
 void i2c_clients_command(struct i2c_adapter *adap, unsigned int cmd, void *arg)
 {
-	struct list_head  *item;
-	struct i2c_client *client;
+	struct i2c_cmd_arg	cmd_arg;
 
-	mutex_lock(&adap->clist_lock);
-	list_for_each(item,&adap->clients) {
-		client = list_entry(item, struct i2c_client, list);
-		if (!try_module_get(client->driver->driver.owner))
-			continue;
-		if (NULL != client->driver->command) {
-			mutex_unlock(&adap->clist_lock);
-			client->driver->command(client,cmd,arg);
-			mutex_lock(&adap->clist_lock);
-		}
-		module_put(client->driver->driver.owner);
-       }
-       mutex_unlock(&adap->clist_lock);
+	cmd_arg.cmd = cmd;
+	cmd_arg.arg = arg;
+	device_for_each_child(&adap->dev, &cmd_arg, i2c_cmd);
 }
 EXPORT_SYMBOL(i2c_clients_command);
 
@@ -1161,7 +1175,7 @@ i2c_new_probed_device(struct i2c_adapter
 		}
 
 		/* Check address availability */
-		if (__i2c_check_addr(adap, addr_list[i])) {
+		if (i2c_check_addr(adap, addr_list[i])) {
 			dev_dbg(&adap->dev, "Address 0x%02x already in "
 				"use, not probing\n", addr_list[i]);
 			continue;
--- at91.orig/drivers/i2c/i2c-dev.c	2008-01-14 13:54:05.000000000 -0800
+++ at91/drivers/i2c/i2c-dev.c	2008-01-14 14:03:43.000000000 -0800
@@ -182,27 +182,22 @@ static ssize_t i2cdev_write (struct file
 	return ret;
 }
 
+static int i2cdev_check(struct device *dev, void *addrp)
+{
+	struct i2c_client *client = i2c_verify_client(dev);
+
+	if (!client || client->addr != *(unsigned int *)addrp)
+		return 0;
+
+	return dev->driver ? -EBUSY : 0;
+}
+
 /* This address checking function differs from the one in i2c-core
    in that it considers an address with a registered device, but no
-   bounded driver, as NOT busy. */
+   driver bound to it, as NOT busy. */
 static int i2cdev_check_addr(struct i2c_adapter *adapter, unsigned int addr)
 {
-	struct list_head *item;
-	struct i2c_client *client;
-	int res = 0;
-
-	mutex_lock(&adapter->clist_lock);
-	list_for_each(item, &adapter->clients) {
-		client = list_entry(item, struct i2c_client, list);
-		if (client->addr == addr) {
-			if (client->driver)
-				res = -EBUSY;
-			break;
-		}
-	}
-	mutex_unlock(&adapter->clist_lock);
-
-	return res;
+	return device_for_each_child(&adapter->dev, &addr, i2cdev_check);
 }
 
 static int i2cdev_ioctl(struct inode *inode, struct file *file,
--- at91.orig/include/linux/i2c.h	2008-01-14 13:56:28.000000000 -0800
+++ at91/include/linux/i2c.h	2008-01-14 13:56:29.000000000 -0800
@@ -159,7 +159,7 @@ struct i2c_driver {
  * @irq: indicates the IRQ generated by this device (if any)
  * @driver_name: Identifies new-style driver used with this device; also
  *	used as the module name for hotplug/coldplug modprobe support.
- * @list: list of active/busy clients
+ * @list: list of active/busy clients (DEPRECATED)
  * @released: used to synchronize client releases & detaches and references
  *
  * An i2c_client identifies a single device (i.e. chip) connected to an
@@ -179,11 +179,13 @@ struct i2c_client {
 	struct device dev;		/* the device structure		*/
 	int irq;			/* irq issued by device (or -1) */
 	char driver_name[KOBJ_NAME_LEN];
-	struct list_head list;
+	struct list_head list;		/* DEPRECATED */
 	struct completion released;
 };
 #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
 
+extern struct i2c_client *i2c_verify_client(struct device *dev);
+
 static inline struct i2c_client *kobj_to_i2c_client(struct kobject *kobj)
 {
 	struct device * const dev = container_of(kobj, struct device, kobj);
@@ -324,7 +326,7 @@ struct i2c_adapter {
 	struct device dev;		/* the adapter device */
 
 	int nr;
-	struct list_head clients;
+	struct list_head clients;	/* DEPRECATED */
 	char name[48];
 	struct completion dev_released;
 };

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

Re: i2c-remove-redundant-i2c_client-list.patch

[David Brownell]

On Thursday 10 January 2008, Jean Delvare wrote:
> > (b) But don't remove that list from the deletion path until ...
> > 
> > (c) ... We have a solution that removes that wait_for_completion()
> >     and its infrastructure.  (Note the similar i2c_adapter logic
> >     too, sigh.)
> > 
> > (d) Meanwhile, come up with a different solution to the deadlock
> >     observed with i2c_adapter.clist ... which for some unknown
> >     reason has *NOT* shown up for me with lockdep.
> > 
> > Of course, if (c) happens soon soon, this problem simplifies.  And
> > maybe someone will come up with a non-invasive solution to that
> > problem ... but if nobody does so before, say, Monday, I'm thinking
> > that (d) becomes a priority.

Oh, and for the record ... here's the part of $SUBJECT patch that
does (b) without (c).  No (d) yet, sigh.  I suspect a few of these
cleanups could become mergable with some work, but certainly not
all of them.  Goes on top of what I just posted.

- Dave

=============	CUT
Finish removing i2c_adapter.clients and its support.

NOTE:  this has self-deadlock issues with legacy drivers.  The issue
seems to be an obsolete idiom whereby code deleting devices (in i2c-core
both i2c_client and i2c_adapter nodes have this problem) waits for a
completion event as a kind of synchronization.  That's problematic since
the driver model turns that kind of synchronization into a self-deadlock.

---
 drivers/i2c/i2c-core.c |  126 +++++++++++++++++++++----------------------------
 include/linux/i2c.h    |    4 -
 2 files changed, 54 insertions(+), 76 deletions(-)

--- at91.orig/drivers/i2c/i2c-core.c	2008-01-14 14:32:00.000000000 -0800
+++ at91/drivers/i2c/i2c-core.c	2008-01-14 14:32:19.000000000 -0800
@@ -275,7 +275,6 @@ EXPORT_SYMBOL_GPL(i2c_new_device);
  */
 void i2c_unregister_device(struct i2c_client *client)
 {
-	struct i2c_adapter	*adapter = client->adapter;
 	struct i2c_driver	*driver = client->driver;
 
 	if (driver && !is_newstyle_driver(driver)) {
@@ -284,11 +283,6 @@ void i2c_unregister_device(struct i2c_cl
 		WARN_ON(1);
 		return;
 	}
-
-	mutex_lock(&adapter->clist_lock);
-	list_del(&client->list);
-	mutex_unlock(&adapter->clist_lock);
-
 	device_unregister(&client->dev);
 }
 EXPORT_SYMBOL_GPL(i2c_unregister_device);
@@ -400,8 +394,6 @@ static int i2c_register_adapter(struct i
 	int res = 0, dummy;
 
 	mutex_init(&adap->bus_lock);
-	mutex_init(&adap->clist_lock);
-	INIT_LIST_HEAD(&adap->clients);
 
 	mutex_lock(&core_lists);
 
@@ -544,6 +536,32 @@ static int i2c_do_del_adapter(struct dev
 	return res;
 }
 
+
+static int detach_all_clients(struct device *dev, void *x)
+{
+	struct i2c_client	*client = i2c_verify_client(dev);
+	struct i2c_driver	*driver;
+	int			res;
+
+	if (!client)
+		return 0;
+
+	driver = client->driver;
+
+	/* new style, follow standard driver model */
+	if (!driver || is_newstyle_driver(driver)) {
+		i2c_unregister_device(client);
+		return 0;
+	}
+
+	/* legacy drivers create and remove clients themselves */
+	if ((res = driver->detach_client(client)))
+		dev_err(dev, "detach_client [%s] failed at address 0x%02x\n",
+			client->name, client->addr);
+
+	return res;
+}
+
 /**
  * i2c_del_adapter - unregister I2C adapter
  * @adap: the adapter being unregistered
@@ -554,8 +572,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_lists);
@@ -576,26 +592,9 @@ int i2c_del_adapter(struct i2c_adapter *
 
 	/* 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;
-		}
-	}
+	res = device_for_each_child(&adap->dev, NULL, detach_all_clients);
+	if (res)
+		goto out_unlock;
 
 	/* clean up the sysfs representation */
 	init_completion(&adap->dev_released);
@@ -674,6 +673,20 @@ int i2c_register_driver(struct module *o
 }
 EXPORT_SYMBOL(i2c_register_driver);
 
+static int detach_legacy_clients(struct device *dev, void *driver)
+{
+	struct i2c_client	*client = i2c_verify_client(dev);
+
+	if (client && client->driver == driver) {
+		dev_dbg(dev, "detaching client [%s] at 0x%02x\n",
+				client->name, client->addr);
+		if (client->driver->detach_client(client))
+			dev_err(dev, "failed detach_client [%s] at 0x%02x\n",
+				client->name, client->addr);
+	}
+	return 0;
+}
+
 /**
  * i2c_del_driver - unregister I2C driver
  * @driver: the driver being unregistered
@@ -681,8 +694,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_lists);
@@ -703,22 +714,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,
+					detach_legacy_clients);
 	}
 	up(&i2c_adapter_class.sem);
 
@@ -752,13 +750,6 @@ int i2c_attach_client(struct i2c_client 
 	struct i2c_adapter *adapter = client->adapter;
 	int res = 0;
 
-	mutex_lock(&adapter->clist_lock);
-	if (i2c_check_addr(client->adapter, client->addr)) {
-		res = -EBUSY;
-		goto out_unlock;
-	}
-	list_add_tail(&client->list,&adapter->clients);
-
 	client->usage_count = 0;
 
 	client->dev.parent = &client->adapter->dev;
@@ -775,12 +766,16 @@ int i2c_attach_client(struct i2c_client 
 
 	snprintf(&client->dev.bus_id[0], sizeof(client->dev.bus_id),
 		"%d-%04x", i2c_adapter_id(adapter), client->addr);
+	res = device_register(&client->dev);
+	if (res) {
+		dev_err(&adapter->dev,
+			"Failed to register i2c client %s at 0x%02x (%d)\n",
+			client->name, client->addr, res);
+		return res;
+	}
+
 	dev_dbg(&adapter->dev, "client [%s] registered with bus id %s\n",
 		client->name, client->dev.bus_id);
-	res = device_register(&client->dev);
-	if (res)
-		goto out_list;
-	mutex_unlock(&adapter->clist_lock);
 
 	if (adapter->client_register)  {
 		if (adapter->client_register(client)) {
@@ -791,14 +786,6 @@ int i2c_attach_client(struct i2c_client 
 	}
 
 	return 0;
-
-out_list:
-	list_del(&client->list);
-	dev_err(&adapter->dev, "Failed to attach i2c client %s at 0x%02x "
-		"(%d)\n", client->name, client->addr, res);
-out_unlock:
-	mutex_unlock(&adapter->clist_lock);
-	return res;
 }
 EXPORT_SYMBOL(i2c_attach_client);
 
@@ -823,11 +810,8 @@ int i2c_detach_client(struct i2c_client 
 		}
 	}
 
-	mutex_lock(&adapter->clist_lock);
-	list_del(&client->list);
 	init_completion(&client->released);
 	device_unregister(&client->dev);
-	mutex_unlock(&adapter->clist_lock);
 	wait_for_completion(&client->released);
 
  out:
@@ -1165,7 +1149,6 @@ i2c_new_probed_device(struct i2c_adapter
 		return NULL;
 	}
 
-	mutex_lock(&adap->clist_lock);
 	for (i = 0; addr_list[i] != I2C_CLIENT_END; i++) {
 		/* Check address validity */
 		if (addr_list[i] < 0x03 || addr_list[i] > 0x77) {
@@ -1203,7 +1186,6 @@ i2c_new_probed_device(struct i2c_adapter
 				break;
 		}
 	}
-	mutex_unlock(&adap->clist_lock);
 
 	if (addr_list[i] == I2C_CLIENT_END) {
 		dev_dbg(&adap->dev, "Probing failed, no device found\n");
--- at91.orig/include/linux/i2c.h	2008-01-14 14:32:00.000000000 -0800
+++ at91/include/linux/i2c.h	2008-01-14 14:32:19.000000000 -0800
@@ -159,7 +159,6 @@ struct i2c_driver {
  * @irq: indicates the IRQ generated by this device (if any)
  * @driver_name: Identifies new-style driver used with this device; also
  *	used as the module name for hotplug/coldplug modprobe support.
- * @list: list of active/busy clients (DEPRECATED)
  * @released: used to synchronize client releases & detaches and references
  *
  * An i2c_client identifies a single device (i.e. chip) connected to an
@@ -179,7 +178,6 @@ struct i2c_client {
 	struct device dev;		/* the device structure		*/
 	int irq;			/* irq issued by device (or -1) */
 	char driver_name[KOBJ_NAME_LEN];
-	struct list_head list;		/* DEPRECATED */
 	struct completion released;
 };
 #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
@@ -319,14 +317,12 @@ 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;
 	struct device dev;		/* the adapter device */
 
 	int nr;
-	struct list_head clients;	/* DEPRECATED */
 	char name[48];
 	struct completion dev_released;
 };


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

Re: i2c-remove-redundant-i2c_client-list.patch

[David Brownell]

On Wednesday 09 January 2008, David Brownell wrote:
> (a) Start phasing out users of i2c_client.list and its lock, ASAP;
>     merging those DVB driver patches, and some i2c-core changes.

Posted:  http://marc.info/?l=i2c&m=120034972711126&w=2

> (d) Meanwhile, come up with a different solution to the deadlock
>     observed with i2c_adapter.clist ... which for some unknown
>     reason has *NOT* shown up for me with lockdep.

This should suffice.  Could be merged with the patch above,
or even made to not depend on it.

And maybe the list_add should be moved after device_register()
so the locking is only needed on that one path.  That'd be a
net code shrink and simplification...

- Dave

====== CUT HERE
This goes on top of the patch removing most i2c_adapter.clients usage,
updating i2c_attach_client:

 - Don't call device_register() while holding clist_lock.  This
   removes a self-deadlock when on the i2c_driver.probe() path,
   for drivers that need to attach new devices (e.g. dummies).

 - Remove a redundant address check.  The driver model core does
   this as a consequence of guaranteeing unique names.

 - Move the "device registered" diagnostic so that it never lies;
   previously, on error paths it would falsely report success.

---
 drivers/i2c/i2c-core.c |   19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

--- at91.orig/drivers/i2c/i2c-core.c	2008-01-17 10:07:38.000000000 -0800
+++ at91/drivers/i2c/i2c-core.c	2008-01-17 10:11:04.000000000 -0800
@@ -752,13 +752,6 @@ int i2c_attach_client(struct i2c_client 
 	struct i2c_adapter *adapter = client->adapter;
 	int res = 0;
 
-	mutex_lock(&adapter->clist_lock);
-	if (i2c_check_addr(client->adapter, client->addr)) {
-		res = -EBUSY;
-		goto out_unlock;
-	}
-	list_add_tail(&client->list,&adapter->clients);
-
 	client->usage_count = 0;
 
 	client->dev.parent = &client->adapter->dev;
@@ -773,14 +766,18 @@ int i2c_attach_client(struct i2c_client 
 	} else
 		client->dev.release = i2c_client_dev_release;
 
+	mutex_lock(&adapter->clist_lock);
+	list_add_tail(&client->list,&adapter->clients);
+	mutex_unlock(&adapter->clist_lock);
+
 	snprintf(&client->dev.bus_id[0], sizeof(client->dev.bus_id),
 		"%d-%04x", i2c_adapter_id(adapter), client->addr);
-	dev_dbg(&adapter->dev, "client [%s] registered with bus id %s\n",
-		client->name, client->dev.bus_id);
 	res = device_register(&client->dev);
 	if (res)
 		goto out_list;
-	mutex_unlock(&adapter->clist_lock);
+
+	dev_dbg(&adapter->dev, "client [%s] registered with bus id %s\n",
+		client->name, client->dev.bus_id);
 
 	if (adapter->client_register)  {
 		if (adapter->client_register(client)) {
@@ -793,7 +790,9 @@ int i2c_attach_client(struct i2c_client 
 	return 0;
 
 out_list:
+	mutex_lock(&adapter->clist_lock);
 	list_del(&client->list);
+	mutex_unlock(&adapter->clist_lock);
 	dev_err(&adapter->dev, "Failed to attach i2c client %s at 0x%02x "
 		"(%d)\n", client->name, client->addr, res);
 out_unlock:



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

Re: i2c-remove-redundant-i2c_client-list.patch

[David Brownell]

On Thursday 17 January 2008, David Brownell wrote:
> > (d) Meanwhile, come up with a different solution to the deadlock
> >     observed with i2c_adapter.clist ... which for some unknown
> >     reason has *NOT* shown up for me with lockdep.
> 
> This should suffice.  Could be merged with the patch above,
> or even made to not depend on it.
> 
> And maybe the list_add should be moved after device_register()
> so the locking is only needed on that one path.  That'd be a
> net code shrink and simplification...

Grr.  Disregard that previous patch.  This version fixes bugs
in that one, and includes those cleanups too.  To make it not
depend on the previous patch, make it "__i2c_check_addr".

========= SNIP!
This goes on top of the patch removing most i2c_adapter.clients usage,
updating i2c_attach_client:

 - Don't call device_register() while holding clist_lock.  This
   removes a self-deadlock when on the i2c_driver.probe() path,
   for drivers that need to attach new devices (e.g. dummies).

 - Remove a redundant address check.  The driver model core does
   this as a consequence of guaranteeing unique names.

 - Move the "device registered" diagnostic so that it never lies;
   previously, on error paths it would falsely report success.

---
 drivers/i2c/i2c-core.c |   22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

--- at91.orig/drivers/i2c/i2c-core.c	2008-01-17 10:07:38.000000000 -0800
+++ at91/drivers/i2c/i2c-core.c	2008-01-17 11:53:01.000000000 -0800
@@ -752,13 +752,6 @@ int i2c_attach_client(struct i2c_client 
 	struct i2c_adapter *adapter = client->adapter;
 	int res = 0;
 
-	mutex_lock(&adapter->clist_lock);
-	if (i2c_check_addr(client->adapter, client->addr)) {
-		res = -EBUSY;
-		goto out_unlock;
-	}
-	list_add_tail(&client->list,&adapter->clients);
-
 	client->usage_count = 0;
 
 	client->dev.parent = &client->adapter->dev;
@@ -775,13 +768,17 @@ int i2c_attach_client(struct i2c_client 
 
 	snprintf(&client->dev.bus_id[0], sizeof(client->dev.bus_id),
 		"%d-%04x", i2c_adapter_id(adapter), client->addr);
-	dev_dbg(&adapter->dev, "client [%s] registered with bus id %s\n",
-		client->name, client->dev.bus_id);
 	res = device_register(&client->dev);
 	if (res)
-		goto out_list;
+		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);
+
 	if (adapter->client_register)  {
 		if (adapter->client_register(client)) {
 			dev_dbg(&adapter->dev, "client_register "
@@ -792,12 +789,9 @@ int i2c_attach_client(struct i2c_client 
 
 	return 0;
 
-out_list:
-	list_del(&client->list);
+out_err:
 	dev_err(&adapter->dev, "Failed to attach i2c client %s at 0x%02x "
 		"(%d)\n", client->name, client->addr, res);
-out_unlock:
-	mutex_unlock(&adapter->clist_lock);
 	return res;
 }
 EXPORT_SYMBOL(i2c_attach_client);


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

Re: i2c-remove-redundant-i2c_client-list.patch

[Jean Delvare]

Hi David,

On Mon, 14 Jan 2008 14:20:48 -0800, David Brownell wrote:
> OK, here's the trimmed-down patch that leaves the deletion
> paths untouched until we have a better solution.  I've only
> build-tested it, but it's basically $SUBJECT without those
> troublesome paths.
> 
> - Dave
> 
> =======	CUT HERE
> The i2c_adapter.clients list of i2c_client nodes duplicates driver
> model state.  This patch starts removing that list, letting us remove
> most existing users of those i2c-core lists.
> 
>  * The core I2C code now iterates over the driver model's list instead
>    of the i2c-internal one in some places where it's safe:
>       - Passing a command/ioctl to each client, a mechanims
>         used almost exclusively by DVB adapters;
>       - Device address checking, in both i2c-core and i2c-dev.
> 
>  * Provide i2c_verify_client() to use with driver model iterators.
> 
>  * Flag the relevant i2c_adapter and i2c_client fields as deprecated,
>    to help prevent new users from appearing.
> 
> For the moment the list needs to stick around, since some issues show
> up when deleting devices created by legacy I2C drivers.  (They don't
> follow standard driver model rules.  Removing those devices can cause
> self-deadlocks.)
> 
> Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> ---
>  drivers/i2c/i2c-core.c |   78 ++++++++++++++++++++++++++++---------------------
>  drivers/i2c/i2c-dev.c  |   29 +++++++-----------
>  include/linux/i2c.h    |    8 +++--
>  3 files changed, 63 insertions(+), 52 deletions(-)

Looks good to me, I've applied that. I was about to suggest possible
improvements with regards to locking scope but I see that you have
already submitted an incremental patch so I'll review it first.

I've given some testing to this patch and I didn't see any problem.

Thanks,
-- 
Jean Delvare

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

Re: [patch 2.6.24-rc5-git] add i2c_new_dummy() utility

[David Brownell]

On Saturday 29 December 2007, David Brownell wrote:
> I still don't know why you get lockdep warnings and I don't ...

Theory:  my test systems were turning lockdep off because of
a lockdep (usage) deficiency that triggers while booting:

	enable_irq_wake(GPIO_irq)
		holds irq_desc[GPIO_irq].lock
		and calls
	enable_irq_wake(parent_of(GPIO_irq))
		which grabs irq_desc[parent_of(GPIO_irq))]

Lockdep falsely detects that as a potential recursion, and
then turns off all further lockdep calls.

I think a genirq patch to make set_irq_chained_handler()
put the parent into a different locking class may help
resolve that issue.

- Dave


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

Re: i2c-remove-redundant-i2c_client-list.patch

[Jean Delvare]

Hi David,

On Thu, 17 Jan 2008 11:59:00 -0800, David Brownell wrote:
> This goes on top of the patch removing most i2c_adapter.clients usage,
> updating i2c_attach_client:
> 
>  - Don't call device_register() while holding clist_lock.  This
>    removes a self-deadlock when on the i2c_driver.probe() path,
>    for drivers that need to attach new devices (e.g. dummies).
> 
>  - Remove a redundant address check.  The driver model core does
>    this as a consequence of guaranteeing unique names.
> 
>  - Move the "device registered" diagnostic so that it never lies;
>    previously, on error paths it would falsely report success.
> 
> ---
>  drivers/i2c/i2c-core.c |   22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
> 
> --- at91.orig/drivers/i2c/i2c-core.c	2008-01-17 10:07:38.000000000 -0800
> +++ at91/drivers/i2c/i2c-core.c	2008-01-17 11:53:01.000000000 -0800
> @@ -752,13 +752,6 @@ int i2c_attach_client(struct i2c_client 
>  	struct i2c_adapter *adapter = client->adapter;
>  	int res = 0;
>  
> -	mutex_lock(&adapter->clist_lock);
> -	if (i2c_check_addr(client->adapter, client->addr)) {
> -		res = -EBUSY;
> -		goto out_unlock;
> -	}
> -	list_add_tail(&client->list,&adapter->clients);
> -
>  	client->usage_count = 0;
>  
>  	client->dev.parent = &client->adapter->dev;
> @@ -775,13 +768,17 @@ int i2c_attach_client(struct i2c_client 
>  
>  	snprintf(&client->dev.bus_id[0], sizeof(client->dev.bus_id),
>  		"%d-%04x", i2c_adapter_id(adapter), client->addr);
> -	dev_dbg(&adapter->dev, "client [%s] registered with bus id %s\n",
> -		client->name, client->dev.bus_id);
>  	res = device_register(&client->dev);
>  	if (res)
> -		goto out_list;
> +		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);
> +
>  	if (adapter->client_register)  {
>  		if (adapter->client_register(client)) {
>  			dev_dbg(&adapter->dev, "client_register "
> @@ -792,12 +789,9 @@ int i2c_attach_client(struct i2c_client 
>  
>  	return 0;
>  
> -out_list:
> -	list_del(&client->list);
> +out_err:
>  	dev_err(&adapter->dev, "Failed to attach i2c client %s at 0x%02x "
>  		"(%d)\n", client->name, client->addr, res);
> -out_unlock:
> -	mutex_unlock(&adapter->clist_lock);
>  	return res;
>  }
>  EXPORT_SYMBOL(i2c_attach_client);

This looks OK to me, I'll add this to my i2c tree. All it is really
missing is your Signed-off-by line.

This gave me the idea of a similar cleanup in i2c_detach_client: I fail
to see why we hold the clist lock while unregistering the device. What
do you think of the following cleanup?

* * * * *

We only need to hold adapter->clist_lock when we touch the client list.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>---
 drivers/i2c/i2c-core.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- linux-2.6.24-rc8.orig/drivers/i2c/i2c-core.c	2008-01-18 09:56:07.000000000 +0100
+++ linux-2.6.24-rc8/drivers/i2c/i2c-core.c	2008-01-18 10:17:01.000000000 +0100
@@ -768,9 +768,10 @@ 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);
-	mutex_unlock(&adapter->clist_lock);
 	wait_for_completion(&client->released);
 
  out:


-- 
Jean Delvare

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

Re: i2c-remove-redundant-i2c_client-list.patch

[Jean Delvare]

Hi David,

On Mon, 14 Jan 2008 14:20:48 -0800, David Brownell wrote:
> OK, here's the trimmed-down patch that leaves the deletion
> paths untouched until we have a better solution.  I've only
> build-tested it, but it's basically $SUBJECT without those
> troublesome paths.
> 
> - Dave
> 
> =======	CUT HERE
> The i2c_adapter.clients list of i2c_client nodes duplicates driver
> model state.  This patch starts removing that list, letting us remove
> most existing users of those i2c-core lists.
> 
>  * The core I2C code now iterates over the driver model's list instead
>    of the i2c-internal one in some places where it's safe:
>       - Passing a command/ioctl to each client, a mechanims
>         used almost exclusively by DVB adapters;
>       - Device address checking, in both i2c-core and i2c-dev.
> 
>  * Provide i2c_verify_client() to use with driver model iterators.
> 
>  * Flag the relevant i2c_adapter and i2c_client fields as deprecated,
>    to help prevent new users from appearing.
> 
> For the moment the list needs to stick around, since some issues show
> up when deleting devices created by legacy I2C drivers.  (They don't
> follow standard driver model rules.  Removing those devices can cause
> self-deadlocks.)
> 
> Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> ---
>  drivers/i2c/i2c-core.c |   78 ++++++++++++++++++++++++++++---------------------
>  drivers/i2c/i2c-dev.c  |   29 +++++++-----------
>  include/linux/i2c.h    |    8 +++--
>  3 files changed, 63 insertions(+), 52 deletions(-)
> 
> --- at91.orig/drivers/i2c/i2c-core.c	2008-01-14 13:56:28.000000000 -0800
> +++ at91/drivers/i2c/i2c-core.c	2008-01-14 14:01:50.000000000 -0800
> (...)
> @@ -713,28 +732,19 @@ EXPORT_SYMBOL(i2c_del_driver);
>  
>  /* ------------------------------------------------------------------------- */
>  
> -static int __i2c_check_addr(struct i2c_adapter *adapter, unsigned int addr)
> +static int __i2c_check_addr(struct device *dev, void *addrp)
>  {
> -	struct list_head   *item;
> -	struct i2c_client  *client;
> +	struct i2c_client	*client = i2c_verify_client(dev);
> +	int			addr = *(int *)addrp;
>  
> -	list_for_each(item,&adapter->clients) {
> -		client = list_entry(item, struct i2c_client, list);
> -		if (client->addr == addr)
> -			return -EBUSY;
> -	}
> +	if (client && client->addr == addr)
> +		return -EBUSY;
>  	return 0;
>  }
>  
>  static int i2c_check_addr(struct i2c_adapter *adapter, int addr)
>  {
> -	int rval;
> -
> -	mutex_lock(&adapter->clist_lock);
> -	rval = __i2c_check_addr(adapter, addr);
> -	mutex_unlock(&adapter->clist_lock);
> -
> -	return rval;
> +	return device_for_each_child(&adapter->dev, &addr, __i2c_check_addr);
>  }
>  
>  int i2c_attach_client(struct i2c_client *client)

With this patch applied, any reason why i2c_new_probed_device() still
acquires adapter->clist_lock? My understanding is that it was there
because i2c_check_addr() was originally walking the internal client
list, but as this is no longer the case, the locking is no longer
needed, is it?

Thanks,
-- 
Jean Delvare

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

Re: i2c-remove-redundant-i2c_client-list.patch

[David Brownell]

On Friday 18 January 2008, Jean Delvare wrote:
> Hi David,
> 
> On Thu, 17 Jan 2008 11:59:00 -0800, David Brownell wrote:
> > This goes on top of the patch removing most i2c_adapter.clients usage,
> > updating i2c_attach_client:
> > 
> > ...
> 
> This looks OK to me, I'll add this to my i2c tree. All it is really
> missing is your Signed-off-by line.

Feel free to cut'n'paste that one from another file.  :)


> This gave me the idea of a similar cleanup in i2c_detach_client: I fail
> to see why we hold the clist lock while unregistering the device. What
> do you think of the following cleanup?

Looks OK.  Similarly, i2c_new_probed_device() could
use a driver model iterator instead of clist_lock.

Something else to think about:  clist_lock isn't used
when iterating over i2c_adapter.clients.  Maybe it should
just be removed, and replaced *everywhere* by core_lists...

- Dave


> * * * * *
> 
> We only need to hold adapter->clist_lock when we touch the client list.
> 
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>---
>  drivers/i2c/i2c-core.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> --- linux-2.6.24-rc8.orig/drivers/i2c/i2c-core.c	2008-01-18 09:56:07.000000000 +0100
> +++ linux-2.6.24-rc8/drivers/i2c/i2c-core.c	2008-01-18 10:17:01.000000000 +0100
> @@ -768,9 +768,10 @@ 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);
> -	mutex_unlock(&adapter->clist_lock);
>  	wait_for_completion(&client->released);
>  
>   out:
> 
> 
> -- 
> Jean Delvare
> 



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

Re: i2c-remove-redundant-i2c_client-list.patch

[David Brownell]

On Friday 18 January 2008, Jean Delvare wrote:
> >  static int i2c_check_addr(struct i2c_adapter *adapter, int addr)
> >  {
> > -     int rval;
> > -
> > -     mutex_lock(&adapter->clist_lock);
> > -     rval = __i2c_check_addr(adapter, addr);
> > -     mutex_unlock(&adapter->clist_lock);
> > -
> > -     return rval;
> > +     return device_for_each_child(&adapter->dev, &addr, __i2c_check_addr);
> >  }
> >  
> >  int i2c_attach_client(struct i2c_client *client)
> 
> With this patch applied, any reason why i2c_new_probed_device() still
> acquires adapter->clist_lock? My understanding is that it was there
> because i2c_check_addr() was originally walking the internal client
> list, but as this is no longer the case, the locking is no longer
> needed, is it?

Doesn't look needed now, no.  It previously called __i2c_check_addr(),
which needed that lock; could have used the non-underscore version
all along.

- Dave


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

Re: i2c-remove-redundant-i2c_client-list.patch

[Jean Delvare]

Hi David,

On Fri, 18 Jan 2008 02:16:22 -0800, David Brownell wrote:
> On Friday 18 January 2008, Jean Delvare wrote:
> > This gave me the idea of a similar cleanup in i2c_detach_client: I fail
> > to see why we hold the clist lock while unregistering the device. What
> > do you think of the following cleanup?
> 
> Looks OK.  Similarly, i2c_new_probed_device() could
> use a driver model iterator instead of clist_lock.

I'm not sure I follow you here. i2c_new_probed_device() doesn't really
walk the list of clients, all it does is calling i2c_check_addr(), as
was discussed elsewhere in this thread. This left apart, I don't see
what kind of "driver model iterator" you would want to use in this
function.

> Something else to think about:  clist_lock isn't used
> when iterating over i2c_adapter.clients.  Maybe it should
> just be removed, and replaced *everywhere* by core_lists...

I don't really want to do that. The redundant client list will be
dropped at some point in time, and it's easier to validate such changes
with dedicated locks. Having a single lock for many things makes it
quite hard to figure out what was being protected after the fact.

-- 
Jean Delvare

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