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(>i2c_lock);
>lock(>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 
> ---
> 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(>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(>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

[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(>i2c_lock);
>lock(>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 
> ---
> 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(>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(>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:18 GMT+02:00 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.

[snip]

Superseded by v2.

Thanks,
Bartosz

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

[Bartosz Golaszewski]

2016-09-16 16:18 GMT+02:00 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.

[snip]

Superseded by v2.

Thanks,
Bartosz

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

[Bartosz Golaszewski]

2016-09-16 16:33 GMT+02:00 Peter Rosin :
>
> 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:33 GMT+02:00 Peter Rosin :
>
> 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