[PATCH] i2c-core: fix for dep-lock validator

[PATCH] i2c-core: fix for dep-lock validator

[Michael Lawnick]

If kernel is compiled with CONFIG_PROVE_LOCKING the
validator raises an error when a multiplexer is removed
via sysfs and sub-clients are connected to it. This is a
false positive.
Documentation/lockdep-design.txt recommends to handle this
via calls to mutex_lock_nested()

Signed-off-by: Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org>
Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org
---
Documentation originally recommends to use an enum.
This is not applicable for a tree with unlimited depth.
This is why I use the adapter id which is expected
to be unique and monotonic increasing with the depth of
the tree.

--- linux/drivers/i2c/i2c-core.c.dist	2012-09-05 09:46:50.000000000 +0200
+++ linux/drivers/i2c/i2c-core.c	2012-09-05 09:56:58.000000000 +0200
@@ -628,7 +628,7 @@ i2c_sysfs_delete_device(struct device *d

 	/* Make sure the device was added through sysfs */
 	res = -ENOENT;
-	mutex_lock(&adap->userspace_clients_lock);
+	mutex_lock_nested(&adap->userspace_clients_lock, i2c_adapter_id(adap));
 	list_for_each_entry_safe(client, next, &adap->userspace_clients,
 				 detected) {
 		if (client->addr == addr) {
@@ -936,7 +936,7 @@ int i2c_del_adapter(struct i2c_adapter *
 		return res;

 	/* Remove devices instantiated from sysfs */
-	mutex_lock(&adap->userspace_clients_lock);
+	mutex_lock_nested(&adap->userspace_clients_lock, i2c_adapter_id(adap));
 	list_for_each_entry_safe(client, next, &adap->userspace_clients,
 				 detected) {
 		dev_dbg(&adap->dev, "Removing %s at 0x%x\n", client->name,

Re: [PATCH] i2c-core: fix for dep-lock validator

[Michael Lawnick]

Am 05.09.2012 14:04, schrieb Michael Lawnick:
> If kernel is compiled with CONFIG_PROVE_LOCKING the
> validator raises an error when a multiplexer is removed
> via sysfs and sub-clients are connected to it. This is a
> false positive.
> Documentation/lockdep-design.txt recommends to handle this
> via calls to mutex_lock_nested()
> 
> Signed-off-by: Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org>
> Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org
> ---
> Documentation originally recommends to use an enum.
> This is not applicable for a tree with unlimited depth.
> This is why I use the adapter id which is expected
> to be unique and monotonic increasing with the depth of
> the tree.
> 
> --- linux/drivers/i2c/i2c-core.c.dist	2012-09-05 09:46:50.000000000 +0200
> +++ linux/drivers/i2c/i2c-core.c	2012-09-05 09:56:58.000000000 +0200
> @@ -628,7 +628,7 @@ i2c_sysfs_delete_device(struct device *d
> 
>  	/* Make sure the device was added through sysfs */
>  	res = -ENOENT;
> -	mutex_lock(&adap->userspace_clients_lock);
> +	mutex_lock_nested(&adap->userspace_clients_lock, i2c_adapter_id(adap));
>  	list_for_each_entry_safe(client, next, &adap->userspace_clients,
>  				 detected) {
>  		if (client->addr == addr) {
> @@ -936,7 +936,7 @@ int i2c_del_adapter(struct i2c_adapter *
>  		return res;
> 
>  	/* Remove devices instantiated from sysfs */
> -	mutex_lock(&adap->userspace_clients_lock);
> +	mutex_lock_nested(&adap->userspace_clients_lock, i2c_adapter_id(adap));
>  	list_for_each_entry_safe(client, next, &adap->userspace_clients,
>  				 detected) {
>  		dev_dbg(&adap->dev, "Removing %s at 0x%x\n", client->name,

This will get problems with maximum log level :-(
Please ignore, other solution is under construction.

-- 
KR
Michael

Re: [PATCH] i2c-core: fix for dep-lock validator

[Jean Delvare]

Hi Michael,

On Fri, 07 Sep 2012 15:00:53 +0200, Michael Lawnick wrote:
> Am 05.09.2012 14:04, schrieb Michael Lawnick:
> > If kernel is compiled with CONFIG_PROVE_LOCKING the
> > validator raises an error when a multiplexer is removed
> > via sysfs and sub-clients are connected to it. This is a
> > false positive.
> > Documentation/lockdep-design.txt recommends to handle this
> > via calls to mutex_lock_nested()
> > 
> > Signed-off-by: Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org>
> > Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org
> > ---
> > Documentation originally recommends to use an enum.
> > This is not applicable for a tree with unlimited depth.
> > This is why I use the adapter id which is expected
> > to be unique and monotonic increasing with the depth of
> > the tree.
> > 
> > --- linux/drivers/i2c/i2c-core.c.dist	2012-09-05 09:46:50.000000000 +0200
> > +++ linux/drivers/i2c/i2c-core.c	2012-09-05 09:56:58.000000000 +0200
> > @@ -628,7 +628,7 @@ i2c_sysfs_delete_device(struct device *d
> > 
> >  	/* Make sure the device was added through sysfs */
> >  	res = -ENOENT;
> > -	mutex_lock(&adap->userspace_clients_lock);
> > +	mutex_lock_nested(&adap->userspace_clients_lock, i2c_adapter_id(adap));
> >  	list_for_each_entry_safe(client, next, &adap->userspace_clients,
> >  				 detected) {
> >  		if (client->addr == addr) {
> > @@ -936,7 +936,7 @@ int i2c_del_adapter(struct i2c_adapter *
> >  		return res;
> > 
> >  	/* Remove devices instantiated from sysfs */
> > -	mutex_lock(&adap->userspace_clients_lock);
> > +	mutex_lock_nested(&adap->userspace_clients_lock, i2c_adapter_id(adap));
> >  	list_for_each_entry_safe(client, next, &adap->userspace_clients,
> >  				 detected) {
> >  		dev_dbg(&adap->dev, "Removing %s at 0x%x\n", client->name,
> 
> This will get problems with maximum log level :-(

Yes, I noticed the problem too. Actually I have a reply for you in my
draft folder, saying exactly that.

> Please ignore, other solution is under construction.

I do have one too, based on the depth level of each adapter:

--- linux-3.6-rc4.orig/drivers/i2c/i2c-core.c	2012-09-06 17:24:17.449441802 +0200
+++ linux-3.6-rc4/drivers/i2c/i2c-core.c	2012-09-07 10:44:11.339368610 +0200
@@ -636,6 +636,16 @@ static void i2c_adapter_dev_release(stru
 	complete(&adap->dev_released);
 }
 
+static unsigned int i2c_adapter_depth(struct i2c_adapter *adapter)
+{
+	unsigned int depth = 0;
+
+	while ((adapter = i2c_parent_is_i2c_adapter(adapter)))
+		depth++;
+
+	return depth;
+}
+
 /*
  * Let users instantiate I2C devices through sysfs. This can be used when
  * platform initialization code doesn't contain the proper data for
@@ -726,7 +736,8 @@ i2c_sysfs_delete_device(struct device *d
 
 	/* Make sure the device was added through sysfs */
 	res = -ENOENT;
-	mutex_lock(&adap->userspace_clients_lock);
+	mutex_lock_nested(&adap->userspace_clients_lock,
+			  i2c_adapter_depth(adap));
 	list_for_each_entry_safe(client, next, &adap->userspace_clients,
 				 detected) {
 		if (client->addr == addr) {
@@ -869,7 +880,8 @@ static int i2c_register_adapter(struct i
 	if (res)
 		goto out_list;
 
-	dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name);
+	dev_dbg(&adap->dev, "adapter [%s] registered, depth %u\n", adap->name,
+		i2c_adapter_depth(adap));
 
 #ifdef CONFIG_I2C_COMPAT
 	res = class_compat_create_link(i2c_adapter_compat_class, &adap->dev,
@@ -1073,7 +1085,8 @@ int i2c_del_adapter(struct i2c_adapter *
 		return res;
 
 	/* Remove devices instantiated from sysfs */
-	mutex_lock(&adap->userspace_clients_lock);
+	mutex_lock_nested(&adap->userspace_clients_lock,
+			  i2c_adapter_depth(adap));
 	list_for_each_entry_safe(client, next, &adap->userspace_clients,
 				 detected) {
 		dev_dbg(&adap->dev, "Removing %s at 0x%x\n", client->name,

I was about to test it but was side-tracked by a different issue.

-- 
Jean Delvare

Re: [PATCH] i2c-core: fix for dep-lock validator

[Michael Lawnick]

Hi Jean,

Am 07.09.2012 15:28, schrieb Jean Delvare:
> I do have one too, based on the depth level of each adapter:
> 
> --- linux-3.6-rc4.orig/drivers/i2c/i2c-core.c	2012-09-06 17:24:17.449441802 +0200
> +++ linux-3.6-rc4/drivers/i2c/i2c-core.c	2012-09-07 10:44:11.339368610 +0200
> @@ -636,6 +636,16 @@ static void i2c_adapter_dev_release(stru
>  	complete(&adap->dev_released);
>  }
>  
> +static unsigned int i2c_adapter_depth(struct i2c_adapter *adapter)
> +{
> +	unsigned int depth = 0;
> +
> +	while ((adapter = i2c_parent_is_i2c_adapter(adapter)))
> +		depth++;
> +
> +	return depth;
> +}
> +
>  /*
>   * Let users instantiate I2C devices through sysfs. This can be used when
>   * platform initialization code doesn't contain the proper data for
> @@ -726,7 +736,8 @@ i2c_sysfs_delete_device(struct device *d
>  
>  	/* Make sure the device was added through sysfs */
>  	res = -ENOENT;
> -	mutex_lock(&adap->userspace_clients_lock);
> +	mutex_lock_nested(&adap->userspace_clients_lock,
> +			  i2c_adapter_depth(adap));
>  	list_for_each_entry_safe(client, next, &adap->userspace_clients,
>  				 detected) {
>  		if (client->addr == addr) {
> @@ -869,7 +880,8 @@ static int i2c_register_adapter(struct i
>  	if (res)
>  		goto out_list;
>  
> -	dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name);
> +	dev_dbg(&adap->dev, "adapter [%s] registered, depth %u\n", adap->name,
> +		i2c_adapter_depth(adap));
>  
>  #ifdef CONFIG_I2C_COMPAT
>  	res = class_compat_create_link(i2c_adapter_compat_class, &adap->dev,
> @@ -1073,7 +1085,8 @@ int i2c_del_adapter(struct i2c_adapter *
>  		return res;
>  
>  	/* Remove devices instantiated from sysfs */
> -	mutex_lock(&adap->userspace_clients_lock);
> +	mutex_lock_nested(&adap->userspace_clients_lock,
> +			  i2c_adapter_depth(adap));
>  	list_for_each_entry_safe(client, next, &adap->userspace_clients,
>  				 detected) {
>  		dev_dbg(&adap->dev, "Removing %s at 0x%x\n", client->name,
> 
> I was about to test it but was side-tracked by a different issue.
> 

I did not test yet, but thought already in same direction. The draw back
of this is that there is still maximum tree depth, but we could say 8
should be enough and the performance issue. May be introducing the depth
into struct i2c_adapter and setting it to parent->depth + 1?

-- 
KR
Michael

Re: [PATCH] i2c-core: fix for dep-lock validator

[Jean Delvare]

On Fri, 07 Sep 2012 15:58:22 +0200, Michael Lawnick wrote:
> I did not test yet, but thought already in same direction. The draw back
> of this is that there is still maximum tree depth, but we could say 8
> should be enough and the performance issue. May be introducing the depth
> into struct i2c_adapter and setting it to parent->depth + 1?

I think we are safe with max depth 8 (actually 7.) I still have to see
a setup with depth > 1, and I seriously don't expect ever seeing one
with depth > 3.

As far as performance is concerned, I thought about storing the depth
value in struct i2c_adapter, but in fact I suspect the call to
i2c_adapter_depth(adap) will be dropped at compilation time if
CONFIG_PROVE_LOCKING isn't set, so the overhead should be zero. While a
small overhead will be present if we store the value (the compiler
won't be able to optimize that away.) I couldn't find the time to check
this, though, which is why my original reply is still in my draft
folder.

-- 
Jean Delvare

Re: [PATCH] i2c-core: fix for dep-lock validator

[Jean Delvare]

On Fri, 7 Sep 2012 16:20:32 +0200, Jean Delvare wrote:
> As far as performance is concerned, I thought about storing the depth
> value in struct i2c_adapter, but in fact I suspect the call to
> i2c_adapter_depth(adap) will be dropped at compilation time if
> CONFIG_PROVE_LOCKING isn't set, so the overhead should be zero.

I confirm the nonexistent overhead, except that the relevant
configuration option isn't CONFIG_PROVE_LOCKING but
CONFIG_DEBUG_LOCK_ALLOC. I think this is acceptable, that's not an
option distribution kernels would enable.

-- 
Jean Delvare

Re: [PATCH] i2c-core: fix for dep-lock validator

[Jean Delvare]

On Wed, 05 Sep 2012 14:04:25 +0200, Michael Lawnick wrote:
> Documentation originally recommends to use an enum.
> This is not applicable for a tree with unlimited depth.
> This is why I use the adapter id which is expected
> to be unique and monotonic increasing with the depth of
> the tree.

For the records, "monotonic increasing with the depth of the tree"
isn't guaranteed. It is generally true, but with i2c adapter removal
and hot-plugging, it can happen that an adapter has a lower id than its
parent. For example:

* Register adapters 0, 1 and 2.
* Remove adapter 1.
* Instantiate a multiplexer on bus 2. The first child adapter will get
  number 1, and 1 < 2.

That being said, after looking at the code, I don't think lockdep cares
about locks with lower class being taken first. OTOH it cares about
locking order being always the same, and with i2c adapters coming and
going, this would have been impossible to guarantee with your first
implementation (although I very much doubt this would have been a
problem in practice.)

Of course these points are all moot anyway as we have a better
implementation which (hopefully) doesn't suffer from any of the
problems yours had.

-- 
Jean Delvare