Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1485153 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/4] gpio: fix an incorrect lockdep warning Bartosz Golaszewski <bgolaszewski@baylibre.com> - 2016-09-16 18:10 +0200
    [PATCH v2 3/4] i2c: add a warning to i2c_adapter_depth() Bartosz Golaszewski <bgolaszewski@baylibre.com> - 2016-09-16 18:10 +0200
    Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning Wolfram Sang <wsa@the-dreams.de> - 2016-09-16 19:30 +0200
      Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning Wolfram Sang <wsa@the-dreams.de> - 2016-09-16 20:00 +0200
        Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning Bartosz Golaszewski <bgolaszewski@baylibre.com> - 2016-09-18 21:50 +0200
          Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning Peter Zijlstra <peterz@infradead.org> - 2016-09-19 10:20 +0200
            Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning Peter Zijlstra <peterz@infradead.org> - 2016-09-19 11:10 +0200
              Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning Bartosz Golaszewski <bgolaszewski@baylibre.com> - 2016-09-20 12:10 +0200
                Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning Peter Zijlstra <peterz@infradead.org> - 2016-09-20 12:30 +0200
                Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning Geert Uytterhoeven <geert@linux-m68k.org> - 2016-09-20 13:40 +0200
                  Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning Bartosz Golaszewski <bgolaszewski@baylibre.com> - 2016-09-20 14:40 +0200
              Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning Thomas Gleixner <tglx@linutronix.de> - 2016-09-20 17:40 +0200
        Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning Bartosz Golaszewski <bgolaszewski@baylibre.com> - 2016-09-18 21:50 +0200
    Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning Peter Zijlstra <peterz@infradead.org> - 2016-09-17 03:30 +0200
      Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning Wolfram Sang <wsa@the-dreams.de> - 2016-09-17 12:20 +0200
        Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning Bartosz Golaszewski <bgolaszewski@baylibre.com> - 2016-09-17 21:10 +0200

#1485153 — [PATCH v2 0/4] gpio: 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.

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1485197

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1485220

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1485932

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

[toc] | [prev] | [next] | [standalone]

#1486236

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.

[toc] | [prev] | [next] | [standalone]

#1486275

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.

[toc] | [prev] | [next] | [standalone]

#1487201

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

[toc] | [prev] | [next] | [standalone]

#1487215

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.

[toc] | [prev] | [next] | [standalone]

#1487258

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

[toc] | [prev] | [next] | [standalone]

#1487290

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

[toc] | [prev] | [next] | [standalone]

#1487445

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

[toc] | [prev] | [next] | [standalone]

#1485933

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

[toc] | [prev] | [next] | [standalone]

#1485454

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

[toc] | [prev] | [next] | [standalone]

#1485515

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1485591

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web