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@xxxxxxxxxxxx>
> ---
> 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