Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1485148 > unrolled thread
| Started by | Bartosz Golaszewski <bgolaszewski@baylibre.com> |
|---|---|
| First post | 2016-09-16 18:10 +0200 |
| Last post | 2016-09-24 16:30 +0200 |
| Articles | 6 — 4 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH v2 4/4] gpio: pca953x: fix an incorrect lockdep warning Bartosz Golaszewski <bgolaszewski@baylibre.com> - 2016-09-16 18:10 +0200
Re: [PATCH v2 4/4] gpio: pca953x: fix an incorrect lockdep warning Wolfram Sang <wsa@the-dreams.de> - 2016-09-21 07:50 +0200
Re: [PATCH v2 4/4] gpio: pca953x: fix an incorrect lockdep warning Linus Walleij <linus.walleij@linaro.org> - 2016-09-23 10:20 +0200
Re: [PATCH v2 4/4] gpio: pca953x: fix an incorrect lockdep warning Wolfram Sang <wsa@the-dreams.de> - 2016-09-24 11:00 +0200
Re: [PATCH v2 4/4] gpio: pca953x: fix an incorrect lockdep warning Wolfram Sang <wsa@the-dreams.de> - 2016-09-24 11:20 +0200
Re: [PATCH v2 4/4] gpio: pca953x: fix an incorrect lockdep warning Geert Uytterhoeven <geert@linux-m68k.org> - 2016-09-24 16:30 +0200
| From | Bartosz Golaszewski <bgolaszewski@baylibre.com> |
|---|---|
| Date | 2016-09-16 18:10 +0200 |
| Subject | [PATCH v2 4/4] gpio: pca953x: fix an incorrect lockdep warning |
| Message-ID | <si3S9-6Ya-9@gated-at.bofh.it> |
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, retrieve the adapter nesting depth
and use it as lockdep subclass for chip->i2c_lock.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/gpio/gpio-pca953x.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 02f2a56..892dc04 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -786,6 +786,8 @@ static int pca953x_probe(struct i2c_client *client,
chip->chip_type = PCA_CHIP_TYPE(chip->driver_data);
mutex_init(&chip->i2c_lock);
+ lockdep_set_subclass(&chip->i2c_lock,
+ i2c_adapter_depth(client->adapter));
/* initialize cached registers from their original values.
* we can't share this chip with another i2c master.
--
2.7.4
[toc] | [next] | [standalone]
| From | Wolfram Sang <wsa@the-dreams.de> |
|---|---|
| Date | 2016-09-21 07:50 +0200 |
| Message-ID | <sjIzT-4Vo-9@gated-at.bofh.it> |
| In reply to | #1485148 |
[Multipart message — attachments visible in raw view] — view raw
On Fri, Sep 16, 2016 at 06:02:45PM +0200, 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, retrieve the adapter nesting depth > and use it as lockdep subclass for chip->i2c_lock. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> Linus, we'd like that in 4.9. Can I get your ack for the gpio part?
[toc] | [prev] | [next] | [standalone]
| From | Linus Walleij <linus.walleij@linaro.org> |
|---|---|
| Date | 2016-09-23 10:20 +0200 |
| Message-ID | <sktSa-1fi-23@gated-at.bofh.it> |
| In reply to | #1487817 |
On Wed, Sep 21, 2016 at 7:45 AM, Wolfram Sang <wsa@the-dreams.de> wrote: >> In order to get rid of the warning, retrieve the adapter nesting depth >> and use it as lockdep subclass for chip->i2c_lock. >> >> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Linus, we'd like that in 4.9. Can I get your ack for the gpio part? Acked-by: Linus Walleij <linus.walleij@linaro.org> Sorry for not attending quick enough, I was down in the block layer. Yours, Linus Walleij
[toc] | [prev] | [next] | [standalone]
| From | Wolfram Sang <wsa@the-dreams.de> |
|---|---|
| Date | 2016-09-24 11:00 +0200 |
| Message-ID | <skQYp-7h6-7@gated-at.bofh.it> |
| In reply to | #1489834 |
[Multipart message — attachments visible in raw view] — view raw
On Fri, Sep 23, 2016 at 10:10:57AM +0200, Linus Walleij wrote: > On Wed, Sep 21, 2016 at 7:45 AM, Wolfram Sang <wsa@the-dreams.de> wrote: > > >> In order to get rid of the warning, retrieve the adapter nesting depth > >> and use it as lockdep subclass for chip->i2c_lock. > >> > >> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Linus, we'd like that in 4.9. Can I get your ack for the gpio part? > > Acked-by: Linus Walleij <linus.walleij@linaro.org> > > Sorry for not attending quick enough, I was down in the > block layer. Well, it was close but still quick enough. Glad to see that one can come back from the depths of the block layer ;)
[toc] | [prev] | [next] | [standalone]
| From | Wolfram Sang <wsa@the-dreams.de> |
|---|---|
| Date | 2016-09-24 11:20 +0200 |
| Message-ID | <skRhL-7Cx-5@gated-at.bofh.it> |
| In reply to | #1485148 |
[Multipart message — attachments visible in raw view] — view raw
> drivers/gpio/gpio-pca953x.c | 2 ++
> 1 file changed, 2 insertions(+)
FYI, my code checkers found this in this driver:
SMATCH
drivers/gpio/gpio-pca953x.c:562 pca953x_irq_pending() error: buffer overflow 'cur_stat' 5 <= 8191
drivers/gpio/gpio-pca953x.c:573 pca953x_irq_pending() warn: buffer overflow 'old_stat' 5 <= 8191
Didn't check further. I fixed a sparse warning, though.
[toc] | [prev] | [next] | [standalone]
| From | Geert Uytterhoeven <geert@linux-m68k.org> |
|---|---|
| Date | 2016-09-24 16:30 +0200 |
| Message-ID | <skW7L-26Y-1@gated-at.bofh.it> |
| In reply to | #1490604 |
On Sat, Sep 24, 2016 at 11:15 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> drivers/gpio/gpio-pca953x.c | 2 ++
>> 1 file changed, 2 insertions(+)
>
> FYI, my code checkers found this in this driver:
>
> SMATCH
> drivers/gpio/gpio-pca953x.c:562 pca953x_irq_pending() error: buffer overflow 'cur_stat' 5 <= 8191
> drivers/gpio/gpio-pca953x.c:573 pca953x_irq_pending() warn: buffer overflow 'old_stat' 5 <= 8191
>
> Didn't check further. I fixed a sparse warning, though.
I guess those lines are
memcpy(old_stat, chip->irq_stat, NBANK(chip));
and
memcpy(chip->irq_stat, cur_stat, NBANK(chip));
?
#define NBANK(chip) DIV_ROUND_UP(chip->gpio_chip.ngpio, BANK_SZ)
ngpio is u16, BANK_SZ is 8, so smatch assumes someone may set
ngpio to 65535. Which someone could do through driver_data.
But none of the predefined entries in pca953x_dt_ids[] does.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web