| Message ID | 1474035484-25134-1-git-send-email-bgolaszewski@baylibre.com |
|---|---|
| State | Superseded |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 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(+)