[PATCH] gpio: pca953x: fix an incorrect lockdep warning

[PATCH] gpio: pca953x: fix an incorrect lockdep warning

[Bartosz Golaszewski]

If an I2C GPIO multiplexer is driven by a GPIO provided by an expander
when there's a second expander using the same device driver on one of
the I2C bus segments, lockdep prints a deadlock warning when trying to
set the direction or the value of the GPIOs provided by the second
expander.

The below diagram presents the setup:

                                               - - - - -
 -------             ---------  Bus segment 1 |         |
|       |           |         |---------------  Devices
|       | SCL/SDA   |         |               |         |
| Linux |-----------| I2C MUX |                - - - - -
|       |    |      |         | Bus segment 2
|       |    |      |         |-------------------
 -------     |       ---------                    |
             |           |                    - - - - -
        ------------     | MUX GPIO          |         |
       |            |    |                     Devices
       |    GPIO    |    |                   |         |
       | Expander 1 |----                     - - - - -
       |            |                             |
        ------------                              | SCL/SDA
                                                  |
                                             ------------
                                            |            |
                                            |    GPIO    |
                                            | Expander 2 |
                                            |            |
                                             ------------

The reason for lockdep warning is that we take the chip->i2c_lock in
pca953x_gpio_set_value() or pca953x_gpio_direction_output() and then
come right back to pca953x_gpio_set_value() when the GPIO mux kicks
in. The locks actually protect different expanders, but for lockdep
both are of the same class, so it says:

  Possible unsafe locking scenario:

        CPU0
        ----
   lock(&chip->i2c_lock);
   lock(&chip->i2c_lock);

  *** DEADLOCK ***

  May be due to missing lock nesting notation

In order to get rid of the warning, check if the i2c adapter of the
expander is multiplexed (by checking if it has a parent adapter) and,
if so, set a different lock subclass for chip->i2c_lock.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
Note: a similar issue would occur with other gpio expanders under
similar circumstances. If this patch get's merged, I'll prepare
a common solution for all gpio drivers which use an internal i2c lock.

 drivers/gpio/gpio-pca953x.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 02f2a56..2d49b25 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -787,6 +787,18 @@ static int pca953x_probe(struct i2c_client *client,
 
 	mutex_init(&chip->i2c_lock);
 
+	/*
+	 * If the i2c adapter we're connected to is multiplexed (which is
+	 * indicated by it having a parent adapter) we need to use a
+	 * different lock subclass. It's caused by the fact that in a rare
+	 * case of using an i2c-gpio multiplexer controlled by a gpio
+	 * provided by an expander using the same driver, lockdep would
+	 * incorrectly detect a deadlock, since we'd take a second lock
+	 * of the same class without releasing the first one.
+	 */
+	if (i2c_parent_is_i2c_adapter(client->adapter))
+		lockdep_set_subclass(&chip->i2c_lock, SINGLE_DEPTH_NESTING);
+
 	/* initialize cached registers from their original values.
 	 * we can't share this chip with another i2c master.
 	 */
-- 
2.7.4

Re: [PATCH] gpio: pca953x: fix an incorrect lockdep warning

[Peter Rosin]

On 2016-09-16 16:18, Bartosz Golaszewski wrote:
> If an I2C GPIO multiplexer is driven by a GPIO provided by an expander
> when there's a second expander using the same device driver on one of
> the I2C bus segments, lockdep prints a deadlock warning when trying to
> set the direction or the value of the GPIOs provided by the second
> expander.
> 
> The below diagram presents the setup:
> 
>                                                - - - - -
>  -------             ---------  Bus segment 1 |         |
> |       |           |         |---------------  Devices
> |       | SCL/SDA   |         |               |         |
> | Linux |-----------| I2C MUX |                - - - - -
> |       |    |      |         | Bus segment 2
> |       |    |      |         |-------------------
>  -------     |       ---------                    |
>              |           |                    - - - - -
>         ------------     | MUX GPIO          |         |
>        |            |    |                     Devices
>        |    GPIO    |    |                   |         |
>        | Expander 1 |----                     - - - - -
>        |            |                             |
>         ------------                              | SCL/SDA
>                                                   |
>                                              ------------
>                                             |            |
>                                             |    GPIO    |
>                                             | Expander 2 |
>                                             |            |
>                                              ------------
> 
> The reason for lockdep warning is that we take the chip->i2c_lock in
> pca953x_gpio_set_value() or pca953x_gpio_direction_output() and then
> come right back to pca953x_gpio_set_value() when the GPIO mux kicks
> in. The locks actually protect different expanders, but for lockdep
> both are of the same class, so it says:
> 
>   Possible unsafe locking scenario:
> 
>         CPU0
>         ----
>    lock(&chip->i2c_lock);
>    lock(&chip->i2c_lock);
> 
>   *** DEADLOCK ***
> 
>   May be due to missing lock nesting notation
> 
> In order to get rid of the warning, check if the i2c adapter of the
> expander is multiplexed (by checking if it has a parent adapter) and,
> if so, set a different lock subclass for chip->i2c_lock.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> Note: a similar issue would occur with other gpio expanders under
> similar circumstances. If this patch get's merged, I'll prepare
> a common solution for all gpio drivers which use an internal i2c lock.
> 
>  drivers/gpio/gpio-pca953x.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 02f2a56..2d49b25 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -787,6 +787,18 @@ static int pca953x_probe(struct i2c_client *client,
>  
>  	mutex_init(&chip->i2c_lock);
>  
> +	/*
> +	 * If the i2c adapter we're connected to is multiplexed (which is
> +	 * indicated by it having a parent adapter) we need to use a
> +	 * different lock subclass. It's caused by the fact that in a rare
> +	 * case of using an i2c-gpio multiplexer controlled by a gpio
> +	 * provided by an expander using the same driver, lockdep would
> +	 * incorrectly detect a deadlock, since we'd take a second lock
> +	 * of the same class without releasing the first one.
> +	 */
> +	if (i2c_parent_is_i2c_adapter(client->adapter))
> +		lockdep_set_subclass(&chip->i2c_lock, SINGLE_DEPTH_NESTING);
> +
>  	/* initialize cached registers from their original values.
>  	 * we can't share this chip with another i2c master.
>  	 */
> 

If this is to be fixed this even for crazy setups where the pattern is
repeated for more levels, you can look into drivers/i2c/i2c-core.c
i2c_adapter_depth() and how it's used (i.e. for this exact purpose).
Maybe it's time to export that function?

Cheers,
Peter

Re: [PATCH] gpio: pca953x: fix an incorrect lockdep warning

[Bartosz Golaszewski]

2016-09-16 16:33 GMT+02:00 Peter Rosin <peda@axentia.se>:
>
> If this is to be fixed this even for crazy setups where the pattern is
> repeated for more levels, you can look into drivers/i2c/i2c-core.c
> i2c_adapter_depth() and how it's used (i.e. for this exact purpose).
> Maybe it's time to export that function?
>

Hi Peter,

thanks for the heads up. I was not aware of this function. Lockdep
only allows us to specify up to 8 subclasses, but I can't possibly
imagine a setup where more would be needed. I'll submit a series
exporting this function and using it in pca953x.

Best regards,
Bartosz Golaszewski

Re: [PATCH] gpio: pca953x: fix an incorrect lockdep warning

[Bartosz Golaszewski]

2016-09-16 16:18 GMT+02:00 Bartosz Golaszewski <bgolaszewski@baylibre.com>:
> If an I2C GPIO multiplexer is driven by a GPIO provided by an expander
> when there's a second expander using the same device driver on one of
> the I2C bus segments, lockdep prints a deadlock warning when trying to
> set the direction or the value of the GPIOs provided by the second
> expander.

[snip]

Superseded by v2.

Thanks,
Bartosz