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

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

[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.

This series exports an already existing function from i2c-core as
public API and reuses it in pca953x to pass a correct lock subclass
to lockdep.

Note: if this series gets merged, I'll prepare follow-up patches for
other expanders for which a similar problem could potentially occur.

Tested with the following setup:

 -------             ---------  Bus segment 1 |         |
|       |           |         |---------------  Devices
|       | SCL/SDA   |         |               |         |
| Linux |-----------| I2C MUX |                - - - - -
|       |    |      |         | Bus segment 2
|       |    |      |         |-------------------
 -------     |       ---------                    |
             |           |                    - - - - -
        ------------     | MUX GPIO          |         |
       |            |    |                     Devices
       |    GPIO    |    |                   |         |
       | Expander 1 |----                     - - - - -
       |            |                             |
        ------------                              | SCL/SDA
                                                  |
                                             ------------
                                            |            |
                                            |    GPIO    |
                                            | Expander 2 |
                                            |            |
                                             ------------

where expander 1 is a pca9534 and expander 2 is a pca9535.

v1 -> v2:
- added patches 1/4, 2/4 & 3/4
- used i2c_adapter_depth() in patch 4/4 in order to detect multiple
  adapter nesting

Bartosz Golaszewski (4):
  i2c: export i2c_adapter_depth()
  lockdep: make MAX_LOCKDEP_SUBCLASSES unconditionally visible
  i2c: add a warning to i2c_adapter_depth()
  gpio: pca953x: fix an incorrect lockdep warning

 drivers/gpio/gpio-pca953x.c |  2 ++
 drivers/i2c/i2c-core.c      | 12 +++++-------
 include/linux/i2c.h         |  1 +
 include/linux/lockdep.h     |  4 ++--
 4 files changed, 10 insertions(+), 9 deletions(-)

-- 
2.7.4

[PATCH v2 1/4] i2c: export i2c_adapter_depth()

[Bartosz Golaszewski]

For crazy setups in which an i2c gpio expander is behind an i2c gpio
multiplexer controlled by a gpio provided a second expander using the
same device driver we need to explicitly tell lockdep how to handle
nested locking.

Export i2c_adapter_depth() as public API to be reused outside of i2c
core code.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/i2c/i2c-core.c | 9 ++-------
 include/linux/i2c.h    | 1 +
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index da3a02e..c9b8df8 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1335,13 +1335,7 @@ static void i2c_adapter_dev_release(struct device *dev)
 	complete(&adap->dev_released);
 }
 
-/*
- * This function is only needed for mutex_lock_nested, so it is never
- * called unless locking correctness checking is enabled. Thus we
- * make it inline to avoid a compiler warning. That's what gcc ends up
- * doing anyway.
- */
-static inline unsigned int i2c_adapter_depth(struct i2c_adapter *adapter)
+unsigned int i2c_adapter_depth(struct i2c_adapter *adapter)
 {
 	unsigned int depth = 0;
 
@@ -1350,6 +1344,7 @@ static inline unsigned int i2c_adapter_depth(struct i2c_adapter *adapter)
 
 	return depth;
 }
+EXPORT_SYMBOL_GPL(i2c_adapter_depth);
 
 /*
  * Let users instantiate I2C devices through sysfs. This can be used when
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index fffdc27..cfacc03 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -673,6 +673,7 @@ extern void i2c_clients_command(struct i2c_adapter *adap,
 
 extern struct i2c_adapter *i2c_get_adapter(int nr);
 extern void i2c_put_adapter(struct i2c_adapter *adap);
+extern unsigned int i2c_adapter_depth(struct i2c_adapter *adapter);
 
 void i2c_parse_fw_timings(struct device *dev, struct i2c_timings *t, bool use_defaults);
 
-- 
2.7.4

[PATCH v2 2/4] lockdep: make MAX_LOCKDEP_SUBCLASSES unconditionally visible

[Bartosz Golaszewski]

This define is needed by i2c_adapter_depth() to detect if we don't
exceed the maximum number of lock subclasses. Make it visible even
if lockdep is disabled.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 include/linux/lockdep.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index eabe013..c1458fe 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -16,6 +16,8 @@ struct lockdep_map;
 extern int prove_locking;
 extern int lock_stat;
 
+#define MAX_LOCKDEP_SUBCLASSES		8UL
+
 #ifdef CONFIG_LOCKDEP
 
 #include <linux/linkage.h>
@@ -29,8 +31,6 @@ extern int lock_stat;
  */
 #define XXX_LOCK_USAGE_STATES		(1+3*4)
 
-#define MAX_LOCKDEP_SUBCLASSES		8UL
-
 /*
  * NR_LOCKDEP_CACHING_CLASSES ... Number of classes
  * cached in the instance of lockdep_map
-- 
2.7.4

[PATCH v2 3/4] i2c: add a warning to i2c_adapter_depth()

[Bartosz Golaszewski]

This routine is only used together with lockdep for nested locking.
The number of lock subclasses is limited to 8 as defined in lockdep.h

Emit a warning if the adapter depth exceeds the maximum number of
lockdep subclasses.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/i2c/i2c-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index c9b8df8..75cefa8 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1342,6 +1342,9 @@ unsigned int i2c_adapter_depth(struct i2c_adapter *adapter)
 	while ((adapter = i2c_parent_is_i2c_adapter(adapter)))
 		depth++;
 
+	WARN_ONCE(depth >= MAX_LOCKDEP_SUBCLASSES,
+		  "adapter depth exceeds lockdep subclass limit\n");
+
 	return depth;
 }
 EXPORT_SYMBOL_GPL(i2c_adapter_depth);
-- 
2.7.4

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

[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.

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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 747 bytes --]

On Fri, Sep 16, 2016 at 06:02:41PM +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.
> 
> This series exports an already existing function from i2c-core as
> public API and reuses it in pca953x to pass a correct lock subclass
> to lockdep.

Looks good from my POV, but will wait for Peter to comment.

If accepted, I'd think this should go via my I2C tree and I would like
to ask Linus to ack patch 4. D'accord, everyone?

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Peter Rosin]

On 2016-09-16 19:26, Wolfram Sang wrote:
> On Fri, Sep 16, 2016 at 06:02:41PM +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.
>>
>> This series exports an already existing function from i2c-core as
>> public API and reuses it in pca953x to pass a correct lock subclass
>> to lockdep.
> 
> Looks good from my POV, but will wait for Peter to comment.
> 
> If accepted, I'd think this should go via my I2C tree and I would like
> to ask Linus to ack patch 4. D'accord, everyone?

Since it is not clear if "Peter" is me or PeterZ (I suspect PeterZ...),
I'm just adding that it all looks fine by me as well, just to prevent
this from being held up by a misunderstanding.

It does unconditionally add a new function to i2c-core that is only
ever used if lockdep is enabled, but it is tiny and I'm not bothered
by that memory waste.

Cheers,
Peter

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 880 bytes --]


> > Looks good from my POV, but will wait for Peter to comment.
> > 
> > If accepted, I'd think this should go via my I2C tree and I would like
> > to ask Linus to ack patch 4. D'accord, everyone?
> 
> Since it is not clear if "Peter" is me or PeterZ (I suspect PeterZ...),

Nope, I meant you :) I really value your input, it especially helps me
on topics like locking, nesting, muxing... etc. Much appreciated, thanks
a lot for doing that!

> I'm just adding that it all looks fine by me as well, just to prevent
> this from being held up by a misunderstanding.

OK. I read this as Acked-by.

> It does unconditionally add a new function to i2c-core that is only
> ever used if lockdep is enabled, but it is tiny and I'm not bothered
> by that memory waste.

Same here. And if it prevents us from false positive lockdep reports, I
am all for fixing it.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Peter Zijlstra]

On Fri, Sep 16, 2016 at 06:02:41PM +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.
> 
> This series exports an already existing function from i2c-core as
> public API and reuses it in pca953x to pass a correct lock subclass
> to lockdep.

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> Note: if this series gets merged, I'll prepare follow-up patches for
> other expanders for which a similar problem could potentially occur.

We can't push this annotation into the i2c core, can we? Since the mutex
is in driver specific code, not more generic...

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]

On Sat, Sep 17, 2016 at 03:19:52AM +0200, Peter Zijlstra wrote:
> On Fri, Sep 16, 2016 at 06:02:41PM +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.
> > 
> > This series exports an already existing function from i2c-core as
> > public API and reuses it in pca953x to pass a correct lock subclass
> > to lockdep.
> 
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Oops, I overlooked that I need your ack as well :) Thanks!

> > Note: if this series gets merged, I'll prepare follow-up patches for
> > other expanders for which a similar problem could potentially occur.
> 
> We can't push this annotation into the i2c core, can we? Since the mutex
> is in driver specific code, not more generic...

Afraid so.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Bartosz Golaszewski]

2016-09-17 12:18 GMT+02:00 Wolfram Sang <wsa@the-dreams.de>:
> On Sat, Sep 17, 2016 at 03:19:52AM +0200, Peter Zijlstra wrote:
>> On Fri, Sep 16, 2016 at 06:02:41PM +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.
>> >
>> > This series exports an already existing function from i2c-core as
>> > public API and reuses it in pca953x to pass a correct lock subclass
>> > to lockdep.
>>
>> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>
> Oops, I overlooked that I need your ack as well :) Thanks!
>
>> > Note: if this series gets merged, I'll prepare follow-up patches for
>> > other expanders for which a similar problem could potentially occur.
>>
>> We can't push this annotation into the i2c core, can we? Since the mutex
>> is in driver specific code, not more generic...
>
> Afraid so.
>

Most i2c controlled GPIO expanders could potentially be converted to
using the regmap framework which unfortunately suffers from the same
locking issue in its current form.

I'm already working on adding adapter depth dependent subclasses for
i2c regmaps - maybe I'll be able to submit it for 4.9 next week.

Using regmap would allow us to drop the separate mutexes in each gpio
driver and instead use the regmap-internal locking.

Best regards,
Bartosz Golaszewski

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

[Peter Rosin]

On 2016-09-16 19:58, Wolfram Sang wrote:
> 
>>> Looks good from my POV, but will wait for Peter to comment.
>>>
>>> If accepted, I'd think this should go via my I2C tree and I would like
>>> to ask Linus to ack patch 4. D'accord, everyone?
>>
>> Since it is not clear if "Peter" is me or PeterZ (I suspect PeterZ...),
> 
> Nope, I meant you :) I really value your input, it especially helps me
> on topics like locking, nesting, muxing... etc. Much appreciated, thanks
> a lot for doing that!
> 
>> I'm just adding that it all looks fine by me as well, just to prevent
>> this from being held up by a misunderstanding.
> 
> OK. I read this as Acked-by.
> 
>> It does unconditionally add a new function to i2c-core that is only
>> ever used if lockdep is enabled, but it is tiny and I'm not bothered
>> by that memory waste.
> 
> Same here. And if it prevents us from false positive lockdep reports, I
> am all for fixing it.

Except it doesn't, when I think some more about it...

If you have two gpio-expanders on the same depth but on different i2c
branches you still end up with a splat if one is used to control a mux
to reach the other.

The only way to solve it for good, that I see, is to have every instance
of the gpio-expander mutex in its own class. That might lead to many
lockdep classes but then again, how many gpio expanders could there be
in a system? A dozen or two seems extreme, so maybe that is the correct
approach anyway?

Cheers,
Peter

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

[Bartosz Golaszewski]

2016-09-18 10:52 GMT+02:00 Peter Rosin <peda@axentia.se>:
> On 2016-09-16 19:58, Wolfram Sang wrote:
>>
>> Same here. And if it prevents us from false positive lockdep reports, I
>> am all for fixing it.
>
> Except it doesn't, when I think some more about it...
>
> If you have two gpio-expanders on the same depth but on different i2c
> branches you still end up with a splat if one is used to control a mux
> to reach the other.
>
> The only way to solve it for good, that I see, is to have every instance
> of the gpio-expander mutex in its own class. That might lead to many
> lockdep classes but then again, how many gpio expanders could there be
> in a system? A dozen or two seems extreme, so maybe that is the correct
> approach anyway?

Wouldn't it be enough to have a separate class for every base (as in:
not having any parent adapters) i2c adapter?

Best regards,
Bartosz Golaszewski

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

[Bartosz Golaszewski]

2016-09-18 21:43 GMT+02:00 Bartosz Golaszewski <bgolaszewski@baylibre.com>:
> 2016-09-18 10:52 GMT+02:00 Peter Rosin <peda@axentia.se>:
>> On 2016-09-16 19:58, Wolfram Sang wrote:
>>>
>>> Same here. And if it prevents us from false positive lockdep reports, I
>>> am all for fixing it.
>>
>> Except it doesn't, when I think some more about it...
>>
>> If you have two gpio-expanders on the same depth but on different i2c
>> branches you still end up with a splat if one is used to control a mux
>> to reach the other.
>>
>> The only way to solve it for good, that I see, is to have every instance
>> of the gpio-expander mutex in its own class. That might lead to many
>> lockdep classes but then again, how many gpio expanders could there be
>> in a system? A dozen or two seems extreme, so maybe that is the correct
>> approach anyway?
>
> Wouldn't it be enough to have a separate class for every base (as in:
> not having any parent adapters) i2c adapter?
>

Eeek -ESENTTOOEARLY

Of course not - since we could have two branches deeper on the tree
with the same problem.

Nevermind my last e-mail.

Best regards,
Bartosz Golaszewski

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

[Peter Rosin]

On 2016-09-18 21:45, Bartosz Golaszewski wrote:
> 2016-09-18 21:43 GMT+02:00 Bartosz Golaszewski <bgolaszewski@baylibre.com>:
>> 2016-09-18 10:52 GMT+02:00 Peter Rosin <peda@axentia.se>:
>>> On 2016-09-16 19:58, Wolfram Sang wrote:
>>>>
>>>> Same here. And if it prevents us from false positive lockdep reports, I
>>>> am all for fixing it.
>>>
>>> Except it doesn't, when I think some more about it...
>>>
>>> If you have two gpio-expanders on the same depth but on different i2c
>>> branches you still end up with a splat if one is used to control a mux
>>> to reach the other.
>>>
>>> The only way to solve it for good, that I see, is to have every instance
>>> of the gpio-expander mutex in its own class. That might lead to many
>>> lockdep classes but then again, how many gpio expanders could there be
>>> in a system? A dozen or two seems extreme, so maybe that is the correct
>>> approach anyway?
>>
>> Wouldn't it be enough to have a separate class for every base (as in:
>> not having any parent adapters) i2c adapter?
>>
> 
> Eeek -ESENTTOOEARLY
> 
> Of course not - since we could have two branches deeper on the tree
> with the same problem.
> 
> Nevermind my last e-mail.

Right, but you have a point, we can avoid some lockdep classes if
we work at it.

We could recognize that no gpio-expander can be involved with
muxing for a gpio-expander sitting on a root i2c adapter. That
much is trivially obvious.

That means that we could treat all gpio-expanders sitting
directly on a root i2c-adapter as we always have, just like
v1 (and v2, implicitly) do, but instead of putting all other
gpio-exanders in one lockdep class (as in v1) or in one lockdep
class per depth (as in v2), we put all other gpio-mutexes in
a lockdep class of their own.

Then we do not get any extra lockdep classes unless there are
i2c muxes, which is better.

But only i2c-mux-gpio can cause the recursion.

I think the theoretical minimal number of lockdep classes is to
create a new lockdep class for every i2c-mux-gpio instance.
gpio-expanders needing a mutex could walk up the adapter tree
looking for a i2c-mux-gpio and getting the lockdep class from
there. If the root i2c adapter is reached, use some default
lockdep class.

That would consolidate muxes from different gpio-expander
drivers into the one lockdep class. I don't know if that is
a big no-no? I guess that can be handled too with more code,
but it is starting to get messy...

Or, do what the i2c-mux code is doing and use an rt_mutex instead
of an ordinary mutex. That way you are very sure to not get any
lockdep splat ... at all. Ok, sorry, that was not a serious
suggestion, but it would be a tad bit simpler to implement...

Cheers,
Peter

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

[Peter Zijlstra]

On Mon, Sep 19, 2016 at 10:01:49AM +0200, Peter Rosin wrote:
> Or, do what the i2c-mux code is doing and use an rt_mutex instead
> of an ordinary mutex. That way you are very sure to not get any
> lockdep splat ... at all. Ok, sorry, that was not a serious
> suggestion, but it would be a tad bit simpler to implement...

So I find it weird that people use rt_mutex as a locking primitive,
since its only that one lock that then does PI and all the other locks
that are related still create inversions.

In any case, since people have started doing this, adding lockdep
support for rt_mutex is on the todo _somewhere_, so don't expect that to
avoid splats forever.

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

[Peter Rosin]

On 2016-09-19 10:14, Peter Zijlstra wrote:
> On Mon, Sep 19, 2016 at 10:01:49AM +0200, Peter Rosin wrote:
>> Or, do what the i2c-mux code is doing and use an rt_mutex instead
>> of an ordinary mutex. That way you are very sure to not get any
>> lockdep splat ... at all. Ok, sorry, that was not a serious
>> suggestion, but it would be a tad bit simpler to implement...
> 
> So I find it weird that people use rt_mutex as a locking primitive,
> since its only that one lock that then does PI and all the other locks
> that are related still create inversions.

So, someone took the bait :-)

Yes, I too find it weird, and would like to get rid of it. It's just
odd. It's been some years since the start though, waaay before me
entering kernel space.

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=194684e596af4b


But it's hard to argue with the numbers given in the discussion:

http://linux-i2c.vger.kernel.narkive.com/nokldJcc/patch-1-1-i2c-prevent-priority-inversion-on-top-of-bus-lock

Has anything happened to the regular mutex implementation that might
have changed the picture? *crosses fingers*


> In any case, since people have started doing this, adding lockdep
> support for rt_mutex is on the todo _somewhere_, so don't expect that to
> avoid splats forever.

I was actually looking quite hard to find out how I should declare the
lockdep class for the rt_mutex in order to prevent future splats, before
I realized that it wasn't even possible...

Cheers,
Peter

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

[Peter Zijlstra]

On Mon, Sep 19, 2016 at 10:48:44AM +0200, Peter Rosin wrote:
> On 2016-09-19 10:14, Peter Zijlstra wrote:
> > On Mon, Sep 19, 2016 at 10:01:49AM +0200, Peter Rosin wrote:
> >> Or, do what the i2c-mux code is doing and use an rt_mutex instead
> >> of an ordinary mutex. That way you are very sure to not get any
> >> lockdep splat ... at all. Ok, sorry, that was not a serious
> >> suggestion, but it would be a tad bit simpler to implement...
> > 
> > So I find it weird that people use rt_mutex as a locking primitive,
> > since its only that one lock that then does PI and all the other locks
> > that are related still create inversions.
> 
> So, someone took the bait :-)
> 
> Yes, I too find it weird, and would like to get rid of it. It's just
> odd. It's been some years since the start though, waaay before me
> entering kernel space.
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=194684e596af4b
> 
> 
> But it's hard to argue with the numbers given in the discussion:
> 
> http://linux-i2c.vger.kernel.narkive.com/nokldJcc/patch-1-1-i2c-prevent-priority-inversion-on-top-of-bus-lock
> 
> Has anything happened to the regular mutex implementation that might
> have changed the picture? *crosses fingers*

Use the -RT kernel and all locks will end up as rt_mutex. Avoiding
inversion on one specific lock, while there are then a gazillion other
than can equally create inversion doesn't make sense to me.

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

[Peter Rosin]

On 2016-09-19 11:03, Peter Zijlstra wrote:
> On Mon, Sep 19, 2016 at 10:48:44AM +0200, Peter Rosin wrote:
>> On 2016-09-19 10:14, Peter Zijlstra wrote:
>>> On Mon, Sep 19, 2016 at 10:01:49AM +0200, Peter Rosin wrote:
>>>> Or, do what the i2c-mux code is doing and use an rt_mutex instead
>>>> of an ordinary mutex. That way you are very sure to not get any
>>>> lockdep splat ... at all. Ok, sorry, that was not a serious
>>>> suggestion, but it would be a tad bit simpler to implement...
>>>
>>> So I find it weird that people use rt_mutex as a locking primitive,
>>> since its only that one lock that then does PI and all the other locks
>>> that are related still create inversions.
>>
>> So, someone took the bait :-)
>>
>> Yes, I too find it weird, and would like to get rid of it. It's just
>> odd. It's been some years since the start though, waaay before me
>> entering kernel space.
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=194684e596af4b
>>
>>
>> But it's hard to argue with the numbers given in the discussion:
>>
>> http://linux-i2c.vger.kernel.narkive.com/nokldJcc/patch-1-1-i2c-prevent-priority-inversion-on-top-of-bus-lock
>>
>> Has anything happened to the regular mutex implementation that might
>> have changed the picture? *crosses fingers*
> 
> Use the -RT kernel and all locks will end up as rt_mutex. Avoiding
> inversion on one specific lock, while there are then a gazillion other
> than can equally create inversion doesn't make sense to me.

So, are we willing to risk a regression and ask any victims of that
fallout to switch to the -RT kernel? -RT is still not completely
mainline, right? Is the part with the mutex->rt_mutex conversion
in mainline? In other words, is it just a config option that we
can ask victims to use, or will they have to defer to patches?

I guess the relatively freshly introduced rt_mutex "mux_lock" in
include/linux/i2c.h can be safely converted to an ordinary mutex since
the only reason I went with rt_mutex was that "bus_lock" was an
rt_mutex and they are grabbed in similar circumstances. I can certainly
send a patch. However, the i2c muxing suffers from the issue we were
originally discussing, potentially causing false positive lockdep
reports when you build complex hierarchies with additional
dependencies between branches coming from client devices in the
hierarchy (such as i2c-muxes controlled by i2c-gpio-expanders).

One pretty simple problematic case is:

  .---.          .----.
  |   |          |    |-- i2c2
  |   |-- i2c0 --|mux0|          .----.
  | l |          |    |-- i2c3 --|gpio|
  | i |          '----'          '----'
  | n |             .--------------'
  | u |          .----.          .----.
  | x |          |    |-- i2c4 --|dev0|
  |   |-- i2c1 --|mux1|          '----'
  |   |          |    |-- i2c5
  '---'          '----'

Accesses to dev0 will:

1. lock i2c1:mux_lock (depth 0)
2. switch mux1 to i2c4 using gpio
 a lock i2c0:mux_lock (depth 0)
 b switch mux0 to i2c3 using whatever
 c access gpio
 d unlock i2c0:mux_lock 
3. access dev0
4. unlock i2c1:mux_lock

2a will cause a lockdep splat if i2c0:mux_lock is in the same
lockdep class & subclass as i2c1:mux_lock. So, lockdep needs
separate lockdep classes depending on the i2c root adapter
(subclasses are needed to handle deeper trees, so they are off
limits). Great fun. How do I go about creating a new lockdep
class for every i2c root adapter instance?

Cheers,
Peter

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

[Bartosz Golaszewski]

2016-09-20 10:48 GMT+02:00 Peter Rosin <peda@axentia.se>:
>
> One pretty simple problematic case is:
>
>   .---.          .----.
>   |   |          |    |-- i2c2
>   |   |-- i2c0 --|mux0|          .----.
>   | l |          |    |-- i2c3 --|gpio|
>   | i |          '----'          '----'
>   | n |             .--------------'
>   | u |          .----.          .----.
>   | x |          |    |-- i2c4 --|dev0|
>   |   |-- i2c1 --|mux1|          '----'
>   |   |          |    |-- i2c5
>   '---'          '----'
>
> Accesses to dev0 will:
>
> 1. lock i2c1:mux_lock (depth 0)
> 2. switch mux1 to i2c4 using gpio
>  a lock i2c0:mux_lock (depth 0)
>  b switch mux0 to i2c3 using whatever
>  c access gpio
>  d unlock i2c0:mux_lock
> 3. access dev0
> 4. unlock i2c1:mux_lock
>
> 2a will cause a lockdep splat if i2c0:mux_lock is in the same
> lockdep class & subclass as i2c1:mux_lock. So, lockdep needs
> separate lockdep classes depending on the i2c root adapter
> (subclasses are needed to handle deeper trees, so they are off
> limits). Great fun. How do I go about creating a new lockdep
> class for every i2c root adapter instance?
>

I feel like it's just wrong to set an arbitrary limit on the number of
i2c branches - and this is what the result of this approach would be.

One solution that comes to mind is to have a separate, global set of
lock classes solely for gpio expanders. I think you mentioned earlier
that it's the only thing that can cause this kind of lockdep false
positives. We could potentially have a limited set of lock classes and
every expander that would need one would request it using some kind of
API ensuring that every instance gets a separate class. But this
sounds like a big hack too I'm afraid... And regmap would need to be
aware of that as well.

Anyways, we're past rc7 already and 4.9 will be the next LTS kernel.
We have real hardware here that runs on mainline linux and is
suffering from this issue. Are there any objections against merging
this series now and continuing the work on improving the solution for
4.10?

Best regards,
Bartosz Golaszewski

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

[Peter Zijlstra]

On Tue, Sep 20, 2016 at 12:07:39PM +0200, Bartosz Golaszewski wrote:
> 2016-09-20 10:48 GMT+02:00 Peter Rosin <peda@axentia.se>:
> >
> > One pretty simple problematic case is:
> >
> >   .---.          .----.
> >   |   |          |    |-- i2c2
> >   |   |-- i2c0 --|mux0|          .----.
> >   | l |          |    |-- i2c3 --|gpio|
> >   | i |          '----'          '----'
> >   | n |             .--------------'
> >   | u |          .----.          .----.
> >   | x |          |    |-- i2c4 --|dev0|
> >   |   |-- i2c1 --|mux1|          '----'
> >   |   |          |    |-- i2c5
> >   '---'          '----'

Shees, and I suppose this is all external to SoC stuff, so people can
stick on whatever they pretty well please. I mean, its an i2c bus, just
order parts from ebay and stick on a board.

> > Accesses to dev0 will:
> >
> > 1. lock i2c1:mux_lock (depth 0)
> > 2. switch mux1 to i2c4 using gpio
> >  a lock i2c0:mux_lock (depth 0)
> >  b switch mux0 to i2c3 using whatever
> >  c access gpio
> >  d unlock i2c0:mux_lock
> > 3. access dev0
> > 4. unlock i2c1:mux_lock
> >
> > 2a will cause a lockdep splat if i2c0:mux_lock is in the same
> > lockdep class & subclass as i2c1:mux_lock. So, lockdep needs
> > separate lockdep classes depending on the i2c root adapter
> > (subclasses are needed to handle deeper trees, so they are off
> > limits). Great fun. How do I go about creating a new lockdep
> > class for every i2c root adapter instance?
> >
> 
> I feel like it's just wrong to set an arbitrary limit on the number of
> i2c branches - and this is what the result of this approach would be.
> 
> One solution that comes to mind is to have a separate, global set of
> lock classes solely for gpio expanders. I think you mentioned earlier
> that it's the only thing that can cause this kind of lockdep false
> positives. We could potentially have a limited set of lock classes and
> every expander that would need one would request it using some kind of
> API ensuring that every instance gets a separate class. But this
> sounds like a big hack too I'm afraid... And regmap would need to be
> aware of that as well.
> 
> Anyways, we're past rc7 already and 4.9 will be the next LTS kernel.
> We have real hardware here that runs on mainline linux and is
> suffering from this issue. Are there any objections against merging
> this series now and continuing the work on improving the solution for
> 4.10?

Seems sensible, I'll also see if I can come up with a better annotation
that can help here.

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

[Peter Rosin]

On 2016-09-20 12:07, Bartosz Golaszewski wrote:
> 2016-09-20 10:48 GMT+02:00 Peter Rosin <peda@axentia.se>:
>>
>> One pretty simple problematic case is:
>>
>>   .---.          .----.
>>   |   |          |    |-- i2c2
>>   |   |-- i2c0 --|mux0|          .----.
>>   | l |          |    |-- i2c3 --|gpio|
>>   | i |          '----'          '----'
>>   | n |             .--------------'
>>   | u |          .----.          .----.
>>   | x |          |    |-- i2c4 --|dev0|
>>   |   |-- i2c1 --|mux1|          '----'
>>   |   |          |    |-- i2c5
>>   '---'          '----'
>>
>> Accesses to dev0 will:
>>
>> 1. lock i2c1:mux_lock (depth 0)
>> 2. switch mux1 to i2c4 using gpio
>>  a lock i2c0:mux_lock (depth 0)
>>  b switch mux0 to i2c3 using whatever
>>  c access gpio
>>  d unlock i2c0:mux_lock
>> 3. access dev0
>> 4. unlock i2c1:mux_lock
>>
>> 2a will cause a lockdep splat if i2c0:mux_lock is in the same
>> lockdep class & subclass as i2c1:mux_lock. So, lockdep needs
>> separate lockdep classes depending on the i2c root adapter
>> (subclasses are needed to handle deeper trees, so they are off
>> limits). Great fun. How do I go about creating a new lockdep
>> class for every i2c root adapter instance?
>>
> 
> I feel like it's just wrong to set an arbitrary limit on the number of
> i2c branches - and this is what the result of this approach would be.

What arbitrary limit would that be? The number of lockdep classes
can't be *that* limited? Or?

I mean one lockdep class per root adapter and one subclass within that
class per mux level doesn't sound too bad. How many root adapters do
we need to design for?

> One solution that comes to mind is to have a separate, global set of
> lock classes solely for gpio expanders. I think you mentioned earlier
> that it's the only thing that can cause this kind of lockdep false
> positives.

No, that's not true, so if I said that, it was simply wrong. E.g. I
think the new mlxcpld i2c mux can mux another i2c adapter and will
cause the same problem without involving any gpio expander.

[OT: Wolfram, are you still following this? Are you picking up the
 mlxcpld mux driver, or did my ack fly by without you noticing?]

Oh, and the there is a pinctrl-based i2c mux that also suffers as
there are pinctrl chips that are i2c clients.

And even though I don't really know the pinctrl demux, it will
probably also cause interesting nesting if an i2c based pinctrl
is used as demuxer. If that's relevant, probably isn't...

>            We could potentially have a limited set of lock classes and
> every expander that would need one would request it using some kind of
> API ensuring that every instance gets a separate class. But this
> sounds like a big hack too I'm afraid... And regmap would need to be
> aware of that as well.
> 
> Anyways, we're past rc7 already and 4.9 will be the next LTS kernel.
> We have real hardware here that runs on mainline linux and is
> suffering from this issue. Are there any objections against merging
> this series now and continuing the work on improving the solution for
> 4.10?

No, I have no objection. It's not wrong per se, it's just not complete.

Cheers,
Peter

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

[Geert Uytterhoeven]

On Tue, Sep 20, 2016 at 12:48 PM, Peter Rosin <peda@axentia.se> wrote:
> On 2016-09-20 12:07, Bartosz Golaszewski wrote:
>> I feel like it's just wrong to set an arbitrary limit on the number of
>> i2c branches - and this is what the result of this approach would be.
>
> What arbitrary limit would that be? The number of lockdep classes
> can't be *that* limited? Or?
>
> I mean one lockdep class per root adapter and one subclass within that
> class per mux level doesn't sound too bad. How many root adapters do
> we need to design for?

'git grep -c i2c@ -- "*dts*"' told me exynos7 has 12 i2c interfaces.
And as long as I have gpios (pcf8574?), I can add more using i2c-gpio.
Hence the upper limit is infinity.

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

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

[Bartosz Golaszewski]

2016-09-20 13:30 GMT+02:00 Geert Uytterhoeven <geert@linux-m68k.org>:
> On Tue, Sep 20, 2016 at 12:48 PM, Peter Rosin <peda@axentia.se> wrote:
>> On 2016-09-20 12:07, Bartosz Golaszewski wrote:
>>> I feel like it's just wrong to set an arbitrary limit on the number of
>>> i2c branches - and this is what the result of this approach would be.
>>
>> What arbitrary limit would that be? The number of lockdep classes
>> can't be *that* limited? Or?
>>
>> I mean one lockdep class per root adapter and one subclass within that
>> class per mux level doesn't sound too bad. How many root adapters do
>> we need to design for?
>
> 'git grep -c i2c@ -- "*dts*"' told me exynos7 has 12 i2c interfaces.
> And as long as I have gpios (pcf8574?), I can add more using i2c-gpio.
> Hence the upper limit is infinity.
>

Agreed. If you add to that dynamic i2c adapters like hid-cp2112 or
greybus-i2c then it makes infinity plus some more.

Best regards,
Bartosz Golaszewski

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

[Thomas Gleixner]

On Tue, 20 Sep 2016, Peter Rosin wrote:
> On 2016-09-19 11:03, Peter Zijlstra wrote:
> >
> > Use the -RT kernel and all locks will end up as rt_mutex. Avoiding
> > inversion on one specific lock, while there are then a gazillion other
> > than can equally create inversion doesn't make sense to me.

That's true, but the locking of the i2c stuff is pretty much self contained
and the results of using an rtmutex speak for themself.

One of the issues is that i2c needs to use threaded interrupt handlers and
blocking out the handler thread with a preempted user space task is hurting
performance badly.

I don't think that using a rtmutex there is wrong. It cures at least a very
clear priority inversion issue versus the threaded interrupt handler.

Forcing all i2c users off to RT is not really an option. RT has other
drawbacks vs. throughput which you don't want to impose on everything which
happens to use i2c.

> 2a will cause a lockdep splat if i2c0:mux_lock is in the same
> lockdep class & subclass as i2c1:mux_lock. So, lockdep needs
> separate lockdep classes depending on the i2c root adapter
> (subclasses are needed to handle deeper trees, so they are off
> limits). Great fun. How do I go about creating a new lockdep
> class for every i2c root adapter instance?

lockdep_set_class() .....

Thanks,

	tglx

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2545 bytes --]

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?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Peter Rosin]

On 2016-09-20 17:33, Thomas Gleixner wrote:
> On Tue, 20 Sep 2016, Peter Rosin wrote:
>> On 2016-09-19 11:03, Peter Zijlstra wrote:
>>>
>>> Use the -RT kernel and all locks will end up as rt_mutex. Avoiding
>>> inversion on one specific lock, while there are then a gazillion other
>>> than can equally create inversion doesn't make sense to me.
> 
> That's true, but the locking of the i2c stuff is pretty much self contained
> and the results of using an rtmutex speak for themself.
> 
> One of the issues is that i2c needs to use threaded interrupt handlers and
> blocking out the handler thread with a preempted user space task is hurting
> performance badly.
> 
> I don't think that using a rtmutex there is wrong. It cures at least a very
> clear priority inversion issue versus the threaded interrupt handler.
> 
> Forcing all i2c users off to RT is not really an option. RT has other
> drawbacks vs. throughput which you don't want to impose on everything which
> happens to use i2c.

Oh, we're not talking about forcing *all* i2c users off to RT, only
those that suffer from the priority inversion. The original report
leading to the bus_lock changing to a rt_mutex seemed like if
someone had suggested an -RT kernel instead of the patch being
accepted, it would have been the better option. If -RT was a viable
option back then? But ok, that was a long time ago and by now
there's no way to be sure what troubles a change back to an ordinary
mutex is going cause. The problem is of course that there is no way
of knowing what, if anything, will suffer from inversion with
ordinary mutexes instead of rt_mutexes.

And this is also true about changing mux_lock to an ordinary mutex.
There is simply no way of knowing if someone depends on avoiding
inversion in a muxing scenario, that dependency may have developed
at any point in time and is not related to the relatively recent
addition of the mux_lock. So, there is a greater risk for
regressions with turning mux_lock into a mutex than I originally
thought.

>> 2a will cause a lockdep splat if i2c0:mux_lock is in the same
>> lockdep class & subclass as i2c1:mux_lock. So, lockdep needs
>> separate lockdep classes depending on the i2c root adapter
>> (subclasses are needed to handle deeper trees, so they are off
>> limits). Great fun. How do I go about creating a new lockdep
>> class for every i2c root adapter instance?

Bzzzt. It is not enough to have one class per root adapter, just
move everything down one mux level:

  .---.                             .----.
  |   |                             |    |-- i2c3
  |   |                           .-|mux1|          .----.
  | l |          .----.          /  |    |-- i2c4 --|gpio|
  | i |          |    |-- i2c1 -'   '----'          '----'
  | n |-- i2c0 --|mux0|                .--------------'
  | u |          |    |-- i2c2 -.   .----.          .----.
  | x |          '----'          \  |    |-- i2c5 --|dev0|
  |   |                           '-|mux2|          '----'
  |   |                             |    |-- i2c6
  '---'                             '----'

access dev0 on i2c5
1. lock i2c2:mux_lock
2.   switch mux2 to i2c5 using gpio on i2c4
 a     lock i2c1:mux_lock
 b       switch mux1 to i2c4 using whatever
 c       access gpio on i2c1->i2c4
  i        lock i2c0:mux_lock
  ii         switch mux0 to i2c1 using whatever
  iii        access gpio on i2c0->i2c1->i2c4
  iv       unlock i2c0:mux_lock
 d     unlock i2c1:mux_lock
3.   access dev0 on i2c2->i2c5
 a     lock i2c0:mux_lock
 b       switch mux0 to i2c2 using whatever
 c       access dev0 on i2c0->i2c2->i2c5
 d     unlock i2c0:mux_lock
4. unlock i2c2:mux_lock

2a is the problematic step here too. i2c1:mux_lock and i2c2:mux_lock
are on the same depth, so needs fully separate lockdep classes. Which
means that using the adapter depth to set the lockdep subclass
is pointless. And since they are branches on the same root adapter,
using one lockdep class per root adapter also falls apart.

> lockdep_set_class() .....

Right, thanks. But given the above, the only way I can think of to
reduce the number of lockdep classes is to defer creating a lockdep
class for an adapter until a mux is attached as a child adapter.
But having extra code to reduce lockdep classes for mux_lock is
pointless, since first of all given the above risk for regression,
I'm no longer all that happy about the rt_mutex -> mutex conversion
even for mux_lock. Then the only reason to consider lockdep classes
is if someone adds support for lockdep to the rt_mutex, which is
on the todo anyway. By then I bet the lockdep class problem will also
present itself for the bus_lock in some manner with some form of
interdependent devices requiring one lockdep class per i2c adapter
anyway.

So, bottom line is that the best we can probably do is to create
one lockdep class per instantiated adapter (i.e. 7 classes for
the above tree), and use one subclass for bus_lock and one for
mux_lock. And the problem is of course that a lockdep class will
be leaked on each de-registration of an i2c adapter.

i2c-based gpio expanders could piggy-back on that and use the same
lockdep class as the i2c adapter it sits on, but use a special
"gpio" subclass. I don't know if that scales though? I mean, can
two drivers for different i2c gpio expanders share lockdep subclass
in the general case?

We can possibly defer the creation of the lockdep class until
either a client or a mux is attached to the adapter in order to
reduce the lockdep class leakage to actual in-use adapters,
whatever that may be worth...

Cheers,
Peter

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 <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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 690 bytes --]

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 ;)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2441 bytes --]

On Fri, Sep 16, 2016 at 06:02:41PM +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.
> 
> This series exports an already existing function from i2c-core as
> public API and reuses it in pca953x to pass a correct lock subclass
> to lockdep.
> 
> Note: if this series gets merged, I'll prepare follow-up patches for
> other expanders for which a similar problem could potentially occur.
> 
> Tested with the following setup:
> 
>  -------             ---------  Bus segment 1 |         |
> |       |           |         |---------------  Devices
> |       | SCL/SDA   |         |               |         |
> | Linux |-----------| I2C MUX |                - - - - -
> |       |    |      |         | Bus segment 2
> |       |    |      |         |-------------------
>  -------     |       ---------                    |
>              |           |                    - - - - -
>         ------------     | MUX GPIO          |         |
>        |            |    |                     Devices
>        |    GPIO    |    |                   |         |
>        | Expander 1 |----                     - - - - -
>        |            |                             |
>         ------------                              | SCL/SDA
>                                                   |
>                                              ------------
>                                             |            |
>                                             |    GPIO    |
>                                             | Expander 2 |
>                                             |            |
>                                              ------------
> 
> where expander 1 is a pca9534 and expander 2 is a pca9535.
> 
> v1 -> v2:
> - added patches 1/4, 2/4 & 3/4
> - used i2c_adapter_depth() in patch 4/4 in order to detect multiple
>   adapter nesting
> 
> Bartosz Golaszewski (4):
>   i2c: export i2c_adapter_depth()
>   lockdep: make MAX_LOCKDEP_SUBCLASSES unconditionally visible
>   i2c: add a warning to i2c_adapter_depth()
>   gpio: pca953x: fix an incorrect lockdep warning

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 388 bytes --]

>  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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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 <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