Re: [PATCH v2 4/4] gpio: pca953x: fix an incorrect lockdep warning
On Sat, Sep 24, 2016 at 11:15 AM, Wolfram Sangwrote: >> 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 -- ge...@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
Re: [PATCH v2 4/4] gpio: pca953x: fix an incorrect lockdep warning
On Sat, Sep 24, 2016 at 11:15 AM, Wolfram Sang 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 -- ge...@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
Re: [PATCH v2 4/4] gpio: pca953x: fix an incorrect lockdep warning
> 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. signature.asc Description: PGP signature
Re: [PATCH v2 4/4] gpio: pca953x: fix an incorrect lockdep warning
> 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. signature.asc Description: PGP signature
Re: [PATCH v2 4/4] gpio: pca953x: fix an incorrect lockdep warning
On Fri, Sep 23, 2016 at 10:10:57AM +0200, Linus Walleij wrote: > On Wed, Sep 21, 2016 at 7:45 AM, Wolfram Sangwrote: > > >> 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 > > > > Linus, we'd like that in 4.9. Can I get your ack for the gpio part? > > Acked-by: Linus Walleij > > 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 ;) signature.asc Description: PGP signature
Re: [PATCH v2 4/4] gpio: pca953x: fix an incorrect lockdep warning
On Fri, Sep 23, 2016 at 10:10:57AM +0200, Linus Walleij wrote: > On Wed, Sep 21, 2016 at 7:45 AM, Wolfram Sang 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 > > > > Linus, we'd like that in 4.9. Can I get your ack for the gpio part? > > Acked-by: Linus Walleij > > 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 ;) signature.asc Description: PGP signature
Re: [PATCH v2 4/4] gpio: pca953x: fix an incorrect lockdep warning
On Wed, Sep 21, 2016 at 7:45 AM, Wolfram Sangwrote: >> 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 > > Linus, we'd like that in 4.9. Can I get your ack for the gpio part? Acked-by: Linus Walleij Sorry for not attending quick enough, I was down in the block layer. Yours, Linus Walleij
Re: [PATCH v2 4/4] gpio: pca953x: fix an incorrect lockdep warning
On Wed, Sep 21, 2016 at 7:45 AM, Wolfram Sang 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 > > Linus, we'd like that in 4.9. Can I get your ack for the gpio part? Acked-by: Linus Walleij Sorry for not attending quick enough, I was down in the block layer. Yours, Linus Walleij
Re: [PATCH v2 4/4] gpio: pca953x: fix an incorrect lockdep warning
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(>i2c_lock); >lock(>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 GolaszewskiLinus, we'd like that in 4.9. Can I get your ack for the gpio part? signature.asc Description: PGP signature
Re: [PATCH v2 4/4] gpio: pca953x: fix an incorrect lockdep warning
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(>i2c_lock); >lock(>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 Linus, we'd like that in 4.9. Can I get your ack for the gpio part? signature.asc Description: PGP signature
[PATCH v2 4/4] gpio: pca953x: fix an incorrect lockdep warning
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, retrieve the adapter nesting depth and use it as lockdep subclass for chip->i2c_lock. Signed-off-by: Bartosz Golaszewski--- 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(>i2c_lock); + lockdep_set_subclass(>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
[PATCH v2 4/4] gpio: pca953x: fix an incorrect lockdep warning
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, retrieve the adapter nesting depth and use it as lockdep subclass for chip->i2c_lock. Signed-off-by: Bartosz Golaszewski --- 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(>i2c_lock); + lockdep_set_subclass(>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