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

[Geert Uytterhoeven]

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

[Geert Uytterhoeven]

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

[Wolfram Sang]

>  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

[Wolfram Sang]

>  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

[Wolfram Sang]

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

[Wolfram Sang]

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

[Linus Walleij]

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

[Linus Walleij]

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

[Wolfram Sang]

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

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

[Wolfram Sang]

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