* [PATCH v4 0/2] Fix lockdep warning with nested instances of i2c-mux
@ 2018-07-20 8:39 Peter Rosin
2018-07-20 8:39 ` [PATCH v4 1/2] rtmutex: allow specifying a subclass for nested locking Peter Rosin
2018-07-20 8:39 ` [PATCH v4 2/2] i2c: mux: annotate the nested rt_mutex usage Peter Rosin
0 siblings, 2 replies; 9+ messages in thread
From: Peter Rosin @ 2018-07-20 8:39 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Rosin, Wolfram Sang, Peter Zijlstra, Ingo Molnar,
Will Deacon, Davidlohr Bueso, Philippe Ombredanne,
Thomas Gleixner, Greg Kroah-Hartman, linux-i2c, Peter Chang,
Deepa Dinamani, John Sperbeck
Hi!
Ping. This is almost a resend of v3, but I added tested-by tags from
John and unwrapped a line in the commit message, so it's a v4.
How is this going to find its way to Linus? I can move this through
the i2c-mux tree, but I obviously need a tag for patch 1/2 for that...
Cheers,
Peter
Changes since v3 https://lkml.org/lkml/2018/5/24/994
- Added Tested-by tags from John.
- Unwrapped commit message for patch 2/2.
Changes since v2 https://lkml.org/lkml/2018/5/24/176
- EXPORT_SYMBOL_GPL(rt_mutex_lock_nested) is more appropriate (the
rt_ prefix was missing).
Changes since v1 https://lkml.org/lkml/2018/5/24/93
- Further compile tests indicated a missing #define for rt_mutex_lock
with lockdep enabled, so that one is added.
- I have verified that I don't get any lockdep splat for a local i2c-mux
setup with these patches applied, and that I do without them.
Cheers,
Peter
Peter Rosin (2):
rtmutex: allow specifying a subclass for nested locking
i2c: mux: annotate the nested rt_mutex usage
drivers/i2c/i2c-core-base.c | 2 +-
drivers/i2c/i2c-mux.c | 4 ++--
include/linux/rtmutex.h | 7 +++++++
kernel/locking/rtmutex.c | 29 +++++++++++++++++++++++++----
4 files changed, 35 insertions(+), 7 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v4 1/2] rtmutex: allow specifying a subclass for nested locking
2018-07-20 8:39 [PATCH v4 0/2] Fix lockdep warning with nested instances of i2c-mux Peter Rosin
@ 2018-07-20 8:39 ` Peter Rosin
2018-07-25 14:18 ` [tip:locking/urgent] locking/rtmutex: Allow " tip-bot for Peter Rosin
2018-09-10 23:27 ` [PATCH v4 1/2] rtmutex: allow " Steven Rostedt
2018-07-20 8:39 ` [PATCH v4 2/2] i2c: mux: annotate the nested rt_mutex usage Peter Rosin
1 sibling, 2 replies; 9+ messages in thread
From: Peter Rosin @ 2018-07-20 8:39 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Rosin, Wolfram Sang, Peter Zijlstra, Ingo Molnar,
Will Deacon, Davidlohr Bueso, Philippe Ombredanne,
Thomas Gleixner, Greg Kroah-Hartman, linux-i2c, Peter Chang,
Deepa Dinamani, John Sperbeck
Needed for annotating rt_mutex locks.
Tested-by: John Sperbeck <jsperbeck@google.com>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
include/linux/rtmutex.h | 7 +++++++
kernel/locking/rtmutex.c | 29 +++++++++++++++++++++++++----
2 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/include/linux/rtmutex.h b/include/linux/rtmutex.h
index 1b92a28dd672..6fd615a0eea9 100644
--- a/include/linux/rtmutex.h
+++ b/include/linux/rtmutex.h
@@ -106,7 +106,14 @@ static inline int rt_mutex_is_locked(struct rt_mutex *lock)
extern void __rt_mutex_init(struct rt_mutex *lock, const char *name, struct lock_class_key *key);
extern void rt_mutex_destroy(struct rt_mutex *lock);
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+extern void rt_mutex_lock_nested(struct rt_mutex *lock, unsigned int subclass);
+#define rt_mutex_lock(lock) rt_mutex_lock_nested(lock, 0)
+#else
extern void rt_mutex_lock(struct rt_mutex *lock);
+#define rt_mutex_lock_nested(lock, subclass) rt_mutex_lock(lock)
+#endif
+
extern int rt_mutex_lock_interruptible(struct rt_mutex *lock);
extern int rt_mutex_timed_lock(struct rt_mutex *lock,
struct hrtimer_sleeper *timeout);
diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 4f014be7a4b8..2823d4163a37 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1465,6 +1465,29 @@ rt_mutex_fastunlock(struct rt_mutex *lock,
rt_mutex_postunlock(&wake_q);
}
+static inline void __rt_mutex_lock(struct rt_mutex *lock, unsigned int subclass)
+{
+ might_sleep();
+
+ mutex_acquire(&lock->dep_map, subclass, 0, _RET_IP_);
+ rt_mutex_fastlock(lock, TASK_UNINTERRUPTIBLE, rt_mutex_slowlock);
+}
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+/**
+ * rt_mutex_lock_nested - lock a rt_mutex
+ *
+ * @lock: the rt_mutex to be locked
+ * @subclass: the lockdep subclass
+ */
+void __sched rt_mutex_lock_nested(struct rt_mutex *lock, unsigned int subclass)
+{
+ __rt_mutex_lock(lock, subclass);
+}
+EXPORT_SYMBOL_GPL(rt_mutex_lock_nested);
+#endif
+
+#ifndef CONFIG_DEBUG_LOCK_ALLOC
/**
* rt_mutex_lock - lock a rt_mutex
*
@@ -1472,12 +1495,10 @@ rt_mutex_fastunlock(struct rt_mutex *lock,
*/
void __sched rt_mutex_lock(struct rt_mutex *lock)
{
- might_sleep();
-
- mutex_acquire(&lock->dep_map, 0, 0, _RET_IP_);
- rt_mutex_fastlock(lock, TASK_UNINTERRUPTIBLE, rt_mutex_slowlock);
+ __rt_mutex_lock(lock, 0);
}
EXPORT_SYMBOL_GPL(rt_mutex_lock);
+#endif
/**
* rt_mutex_lock_interruptible - lock a rt_mutex interruptible
--
2.11.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v4 2/2] i2c: mux: annotate the nested rt_mutex usage
2018-07-20 8:39 [PATCH v4 0/2] Fix lockdep warning with nested instances of i2c-mux Peter Rosin
2018-07-20 8:39 ` [PATCH v4 1/2] rtmutex: allow specifying a subclass for nested locking Peter Rosin
@ 2018-07-20 8:39 ` Peter Rosin
2018-07-25 14:19 ` [tip:locking/urgent] i2c/mux, locking/core: Annotate " tip-bot for Peter Rosin
1 sibling, 1 reply; 9+ messages in thread
From: Peter Rosin @ 2018-07-20 8:39 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Rosin, Wolfram Sang, Peter Zijlstra, Ingo Molnar,
Will Deacon, Davidlohr Bueso, Philippe Ombredanne,
Thomas Gleixner, Greg Kroah-Hartman, linux-i2c, Peter Chang,
Deepa Dinamani, John Sperbeck
If an i2c topology has instances of nested muxes, then a lockdep splat
is produced when when i2c_parent_lock_bus() is called. Here is an
example:
============================================
WARNING: possible recursive locking detected
--------------------------------------------
insmod/68159 is trying to acquire lock:
(i2c_register_adapter#2){+.+.}, at: i2c_parent_lock_bus+0x32/0x50 [i2c_mux]
but task is already holding lock:
(i2c_register_adapter#2){+.+.}, at: i2c_parent_lock_bus+0x32/0x50 [i2c_mux]
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(i2c_register_adapter#2);
lock(i2c_register_adapter#2);
*** DEADLOCK ***
May be due to missing lock nesting notation
1 lock held by insmod/68159:
#0: (i2c_register_adapter#2){+.+.}, at: i2c_parent_lock_bus+0x32/0x50 [i2c_mux]
stack backtrace:
CPU: 13 PID: 68159 Comm: insmod Tainted: G O
Call Trace:
dump_stack+0x67/0x98
__lock_acquire+0x162e/0x1780
lock_acquire+0xba/0x200
rt_mutex_lock+0x44/0x60
i2c_parent_lock_bus+0x32/0x50 [i2c_mux]
i2c_parent_lock_bus+0x3e/0x50 [i2c_mux]
i2c_smbus_xfer+0xf0/0x700
i2c_smbus_read_byte+0x42/0x70
my2c_init+0xa2/0x1000 [my2c]
do_one_initcall+0x51/0x192
do_init_module+0x62/0x216
load_module+0x20f9/0x2b50
SYSC_init_module+0x19a/0x1c0
SyS_init_module+0xe/0x10
do_syscall_64+0x6c/0x1a0
entry_SYSCALL_64_after_hwframe+0x42/0xb7
Reported-by: John Sperbeck <jsperbeck@google.com>
Tested-by: John Sperbeck <jsperbeck@google.com>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
drivers/i2c/i2c-core-base.c | 2 +-
drivers/i2c/i2c-mux.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index f5ec6ec6776f..1157a64c7be3 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -615,7 +615,7 @@ static int i2c_check_addr_busy(struct i2c_adapter *adapter, int addr)
static void i2c_adapter_lock_bus(struct i2c_adapter *adapter,
unsigned int flags)
{
- rt_mutex_lock(&adapter->bus_lock);
+ rt_mutex_lock_nested(&adapter->bus_lock, i2c_adapter_depth(adapter));
}
/**
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 9669ca4937b8..7ba31f6bf148 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -144,7 +144,7 @@ static void i2c_mux_lock_bus(struct i2c_adapter *adapter, unsigned int flags)
struct i2c_mux_priv *priv = adapter->algo_data;
struct i2c_adapter *parent = priv->muxc->parent;
- rt_mutex_lock(&parent->mux_lock);
+ rt_mutex_lock_nested(&parent->mux_lock, i2c_adapter_depth(adapter));
if (!(flags & I2C_LOCK_ROOT_ADAPTER))
return;
i2c_lock_bus(parent, flags);
@@ -181,7 +181,7 @@ static void i2c_parent_lock_bus(struct i2c_adapter *adapter,
struct i2c_mux_priv *priv = adapter->algo_data;
struct i2c_adapter *parent = priv->muxc->parent;
- rt_mutex_lock(&parent->mux_lock);
+ rt_mutex_lock_nested(&parent->mux_lock, i2c_adapter_depth(adapter));
i2c_lock_bus(parent, flags);
}
--
2.11.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [tip:locking/urgent] locking/rtmutex: Allow specifying a subclass for nested locking
2018-07-20 8:39 ` [PATCH v4 1/2] rtmutex: allow specifying a subclass for nested locking Peter Rosin
@ 2018-07-25 14:18 ` tip-bot for Peter Rosin
2018-09-10 23:27 ` [PATCH v4 1/2] rtmutex: allow " Steven Rostedt
1 sibling, 0 replies; 9+ messages in thread
From: tip-bot for Peter Rosin @ 2018-07-25 14:18 UTC (permalink / raw)
To: linux-tip-commits
Cc: tglx, linux-kernel, peda, mingo, gregkh, will.deacon, wsa,
deepadinamani, dave, peterz, torvalds, jsperbeck, hpa, dpf,
pombredanne
Commit-ID: 62cedf3e60af03e47849fe2bd6a03ec179422a8a
Gitweb: https://git.kernel.org/tip/62cedf3e60af03e47849fe2bd6a03ec179422a8a
Author: Peter Rosin <peda@axentia.se>
AuthorDate: Fri, 20 Jul 2018 10:39:13 +0200
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 25 Jul 2018 11:22:19 +0200
locking/rtmutex: Allow specifying a subclass for nested locking
Needed for annotating rt_mutex locks.
Tested-by: John Sperbeck <jsperbeck@google.com>
Signed-off-by: Peter Rosin <peda@axentia.se>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Deepa Dinamani <deepadinamani@google.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Chang <dpf@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Philippe Ombredanne <pombredanne@nexb.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Link: http://lkml.kernel.org/r/20180720083914.1950-2-peda@axentia.se
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
include/linux/rtmutex.h | 7 +++++++
kernel/locking/rtmutex.c | 29 +++++++++++++++++++++++++----
2 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/include/linux/rtmutex.h b/include/linux/rtmutex.h
index 1b92a28dd672..6fd615a0eea9 100644
--- a/include/linux/rtmutex.h
+++ b/include/linux/rtmutex.h
@@ -106,7 +106,14 @@ static inline int rt_mutex_is_locked(struct rt_mutex *lock)
extern void __rt_mutex_init(struct rt_mutex *lock, const char *name, struct lock_class_key *key);
extern void rt_mutex_destroy(struct rt_mutex *lock);
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+extern void rt_mutex_lock_nested(struct rt_mutex *lock, unsigned int subclass);
+#define rt_mutex_lock(lock) rt_mutex_lock_nested(lock, 0)
+#else
extern void rt_mutex_lock(struct rt_mutex *lock);
+#define rt_mutex_lock_nested(lock, subclass) rt_mutex_lock(lock)
+#endif
+
extern int rt_mutex_lock_interruptible(struct rt_mutex *lock);
extern int rt_mutex_timed_lock(struct rt_mutex *lock,
struct hrtimer_sleeper *timeout);
diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 4f014be7a4b8..2823d4163a37 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1465,6 +1465,29 @@ rt_mutex_fastunlock(struct rt_mutex *lock,
rt_mutex_postunlock(&wake_q);
}
+static inline void __rt_mutex_lock(struct rt_mutex *lock, unsigned int subclass)
+{
+ might_sleep();
+
+ mutex_acquire(&lock->dep_map, subclass, 0, _RET_IP_);
+ rt_mutex_fastlock(lock, TASK_UNINTERRUPTIBLE, rt_mutex_slowlock);
+}
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+/**
+ * rt_mutex_lock_nested - lock a rt_mutex
+ *
+ * @lock: the rt_mutex to be locked
+ * @subclass: the lockdep subclass
+ */
+void __sched rt_mutex_lock_nested(struct rt_mutex *lock, unsigned int subclass)
+{
+ __rt_mutex_lock(lock, subclass);
+}
+EXPORT_SYMBOL_GPL(rt_mutex_lock_nested);
+#endif
+
+#ifndef CONFIG_DEBUG_LOCK_ALLOC
/**
* rt_mutex_lock - lock a rt_mutex
*
@@ -1472,12 +1495,10 @@ rt_mutex_fastunlock(struct rt_mutex *lock,
*/
void __sched rt_mutex_lock(struct rt_mutex *lock)
{
- might_sleep();
-
- mutex_acquire(&lock->dep_map, 0, 0, _RET_IP_);
- rt_mutex_fastlock(lock, TASK_UNINTERRUPTIBLE, rt_mutex_slowlock);
+ __rt_mutex_lock(lock, 0);
}
EXPORT_SYMBOL_GPL(rt_mutex_lock);
+#endif
/**
* rt_mutex_lock_interruptible - lock a rt_mutex interruptible
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [tip:locking/urgent] i2c/mux, locking/core: Annotate the nested rt_mutex usage
2018-07-20 8:39 ` [PATCH v4 2/2] i2c: mux: annotate the nested rt_mutex usage Peter Rosin
@ 2018-07-25 14:19 ` tip-bot for Peter Rosin
2018-07-30 16:09 ` Peter Rosin
0 siblings, 1 reply; 9+ messages in thread
From: tip-bot for Peter Rosin @ 2018-07-25 14:19 UTC (permalink / raw)
To: linux-tip-commits
Cc: will.deacon, torvalds, hpa, tglx, jsperbeck, wsa, linux-kernel,
mingo, dave, dpf, pombredanne, peda, peterz, gregkh,
deepadinamani
Commit-ID: 7b94ea50514d1a0dc94f02723b603c27bc0ea597
Gitweb: https://git.kernel.org/tip/7b94ea50514d1a0dc94f02723b603c27bc0ea597
Author: Peter Rosin <peda@axentia.se>
AuthorDate: Fri, 20 Jul 2018 10:39:14 +0200
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 25 Jul 2018 11:22:20 +0200
i2c/mux, locking/core: Annotate the nested rt_mutex usage
If an i2c topology has instances of nested muxes, then a lockdep splat
is produced when when i2c_parent_lock_bus() is called. Here is an
example:
============================================
WARNING: possible recursive locking detected
--------------------------------------------
insmod/68159 is trying to acquire lock:
(i2c_register_adapter#2){+.+.}, at: i2c_parent_lock_bus+0x32/0x50 [i2c_mux]
but task is already holding lock:
(i2c_register_adapter#2){+.+.}, at: i2c_parent_lock_bus+0x32/0x50 [i2c_mux]
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(i2c_register_adapter#2);
lock(i2c_register_adapter#2);
*** DEADLOCK ***
May be due to missing lock nesting notation
1 lock held by insmod/68159:
#0: (i2c_register_adapter#2){+.+.}, at: i2c_parent_lock_bus+0x32/0x50 [i2c_mux]
stack backtrace:
CPU: 13 PID: 68159 Comm: insmod Tainted: G O
Call Trace:
dump_stack+0x67/0x98
__lock_acquire+0x162e/0x1780
lock_acquire+0xba/0x200
rt_mutex_lock+0x44/0x60
i2c_parent_lock_bus+0x32/0x50 [i2c_mux]
i2c_parent_lock_bus+0x3e/0x50 [i2c_mux]
i2c_smbus_xfer+0xf0/0x700
i2c_smbus_read_byte+0x42/0x70
my2c_init+0xa2/0x1000 [my2c]
do_one_initcall+0x51/0x192
do_init_module+0x62/0x216
load_module+0x20f9/0x2b50
SYSC_init_module+0x19a/0x1c0
SyS_init_module+0xe/0x10
do_syscall_64+0x6c/0x1a0
entry_SYSCALL_64_after_hwframe+0x42/0xb7
Reported-by: John Sperbeck <jsperbeck@google.com>
Tested-by: John Sperbeck <jsperbeck@google.com>
Signed-off-by: Peter Rosin <peda@axentia.se>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Deepa Dinamani <deepadinamani@google.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Chang <dpf@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Philippe Ombredanne <pombredanne@nexb.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Link: http://lkml.kernel.org/r/20180720083914.1950-3-peda@axentia.se
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
drivers/i2c/i2c-core-base.c | 2 +-
drivers/i2c/i2c-mux.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 301285c54603..15c95aaa484c 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -624,7 +624,7 @@ static int i2c_check_addr_busy(struct i2c_adapter *adapter, int addr)
static void i2c_adapter_lock_bus(struct i2c_adapter *adapter,
unsigned int flags)
{
- rt_mutex_lock(&adapter->bus_lock);
+ rt_mutex_lock_nested(&adapter->bus_lock, i2c_adapter_depth(adapter));
}
/**
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 300ab4b672e4..29646aa6132e 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -144,7 +144,7 @@ static void i2c_mux_lock_bus(struct i2c_adapter *adapter, unsigned int flags)
struct i2c_mux_priv *priv = adapter->algo_data;
struct i2c_adapter *parent = priv->muxc->parent;
- rt_mutex_lock(&parent->mux_lock);
+ rt_mutex_lock_nested(&parent->mux_lock, i2c_adapter_depth(adapter));
if (!(flags & I2C_LOCK_ROOT_ADAPTER))
return;
i2c_lock_bus(parent, flags);
@@ -181,7 +181,7 @@ static void i2c_parent_lock_bus(struct i2c_adapter *adapter,
struct i2c_mux_priv *priv = adapter->algo_data;
struct i2c_adapter *parent = priv->muxc->parent;
- rt_mutex_lock(&parent->mux_lock);
+ rt_mutex_lock_nested(&parent->mux_lock, i2c_adapter_depth(adapter));
i2c_lock_bus(parent, flags);
}
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [tip:locking/urgent] i2c/mux, locking/core: Annotate the nested rt_mutex usage
2018-07-25 14:19 ` [tip:locking/urgent] i2c/mux, locking/core: Annotate " tip-bot for Peter Rosin
@ 2018-07-30 16:09 ` Peter Rosin
2018-07-30 17:17 ` Ingo Molnar
0 siblings, 1 reply; 9+ messages in thread
From: Peter Rosin @ 2018-07-30 16:09 UTC (permalink / raw)
To: tglx, jsperbeck, linux-kernel, wsa, mingo, dave, dpf,
pombredanne, deepadinamani, peterz, gregkh, will.deacon,
torvalds, hpa, linux-tip-commits
On 2018-07-25 16:19, tip-bot for Peter Rosin wrote:
> Commit-ID: 7b94ea50514d1a0dc94f02723b603c27bc0ea597
> Gitweb: https://git.kernel.org/tip/7b94ea50514d1a0dc94f02723b603c27bc0ea597
> Author: Peter Rosin <peda@axentia.se>
> AuthorDate: Fri, 20 Jul 2018 10:39:14 +0200
> Committer: Ingo Molnar <mingo@kernel.org>
> CommitDate: Wed, 25 Jul 2018 11:22:20 +0200
>
> i2c/mux, locking/core: Annotate the nested rt_mutex usage
Hi!
I'm a bit curious as to why the subject line was changed on this patch?
(it was "[PATCH v4 2/2] i2c: mux: annotate the nested rt_mutex usage")
I thought the subject as I wrote it was just perfect. Was it so bad that it
had to be edited?
The patch has no impact on "locking/core" so where is that coming from?
The "i2c/mux" prefix is not in tune with the tradition of previous commits
to drivers/i2c/i2c-mux.c ...
i2c: mux: improve error message for failed symlink
i2c: mux: only print failure message on error
i2c: mux: provide more info on failure in i2c_mux_add_adapter
i2c: add missing of_node_put in i2c_mux_del_adapters
i2c: move locking operations to their own struct
Merge branch 'i2c-mux-dt-3' of https://github.com/peda-r/i2c-mux into i2c/for-4.9
i2c: mux: add support for 'i2c-mux', 'i2c-arb' and 'i2c-gate' DT subnodes
i2c: add i2c_trylock_bus wrapper, use it
i2c: mux: relax locking of the top i2c adapter during mux-locked muxing
i2c: muxes always lock the parent adapter
i2c: mux: drop old unused i2c-mux api
i2c: mux: add common data for every i2c-mux instance
i2c: i2c-mux: sort includes
i2c: add ACPI support for I2C mux ports
i2c: mux: Use __i2c_transfer() instead of calling parent's master_xfer()
i2c: use parent adapter quirks in mux
i2c: mux: use proper dev when removing "channel-X" symlinks
i2c: mux: create "channel-n" symlinks for child segments in the mux device
i2c: mux: create symlink to actual mux device
...
... and my subject followed the canonical subject format as described in
section 14 of Documentation/process/submitting-patches.rst ...
Subject: [PATCH 001/123] subsystem: summary phrase
... I.e. I don't see why you insist on starting the summary phrase of the
subject with a capitalized "Annotate". As I have understood it, the summary
phrase is not a sentence (or it should end with a period).
Three "bad" decisions on one line is a bit much, I think. I don't care deeply,
though (but apparently it bugs me enough to write this, and the reason for that
is probably that one will get abundant feedback on such syntax during review
that it is a pest to then have all that hard-learned "experience" eradicated
on what seems like a whim). Anyway, as I said, the change sparked my curiosity...
And I want *answers* :-)
Cheers,
Peter
>
> If an i2c topology has instances of nested muxes, then a lockdep splat
> is produced when when i2c_parent_lock_bus() is called. Here is an
> example:
>
> ============================================
> WARNING: possible recursive locking detected
> --------------------------------------------
> insmod/68159 is trying to acquire lock:
> (i2c_register_adapter#2){+.+.}, at: i2c_parent_lock_bus+0x32/0x50 [i2c_mux]
>
> but task is already holding lock:
> (i2c_register_adapter#2){+.+.}, at: i2c_parent_lock_bus+0x32/0x50 [i2c_mux]
>
> other info that might help us debug this:
> Possible unsafe locking scenario:
>
> CPU0
> ----
> lock(i2c_register_adapter#2);
> lock(i2c_register_adapter#2);
>
> *** DEADLOCK ***
>
> May be due to missing lock nesting notation
>
> 1 lock held by insmod/68159:
> #0: (i2c_register_adapter#2){+.+.}, at: i2c_parent_lock_bus+0x32/0x50 [i2c_mux]
>
> stack backtrace:
> CPU: 13 PID: 68159 Comm: insmod Tainted: G O
> Call Trace:
> dump_stack+0x67/0x98
> __lock_acquire+0x162e/0x1780
> lock_acquire+0xba/0x200
> rt_mutex_lock+0x44/0x60
> i2c_parent_lock_bus+0x32/0x50 [i2c_mux]
> i2c_parent_lock_bus+0x3e/0x50 [i2c_mux]
> i2c_smbus_xfer+0xf0/0x700
> i2c_smbus_read_byte+0x42/0x70
> my2c_init+0xa2/0x1000 [my2c]
> do_one_initcall+0x51/0x192
> do_init_module+0x62/0x216
> load_module+0x20f9/0x2b50
> SYSC_init_module+0x19a/0x1c0
> SyS_init_module+0xe/0x10
> do_syscall_64+0x6c/0x1a0
> entry_SYSCALL_64_after_hwframe+0x42/0xb7
>
> Reported-by: John Sperbeck <jsperbeck@google.com>
> Tested-by: John Sperbeck <jsperbeck@google.com>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Deepa Dinamani <deepadinamani@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Peter Chang <dpf@google.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Philippe Ombredanne <pombredanne@nexb.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Link: http://lkml.kernel.org/r/20180720083914.1950-3-peda@axentia.se
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
> drivers/i2c/i2c-core-base.c | 2 +-
> drivers/i2c/i2c-mux.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 301285c54603..15c95aaa484c 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -624,7 +624,7 @@ static int i2c_check_addr_busy(struct i2c_adapter *adapter, int addr)
> static void i2c_adapter_lock_bus(struct i2c_adapter *adapter,
> unsigned int flags)
> {
> - rt_mutex_lock(&adapter->bus_lock);
> + rt_mutex_lock_nested(&adapter->bus_lock, i2c_adapter_depth(adapter));
> }
>
> /**
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index 300ab4b672e4..29646aa6132e 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -144,7 +144,7 @@ static void i2c_mux_lock_bus(struct i2c_adapter *adapter, unsigned int flags)
> struct i2c_mux_priv *priv = adapter->algo_data;
> struct i2c_adapter *parent = priv->muxc->parent;
>
> - rt_mutex_lock(&parent->mux_lock);
> + rt_mutex_lock_nested(&parent->mux_lock, i2c_adapter_depth(adapter));
> if (!(flags & I2C_LOCK_ROOT_ADAPTER))
> return;
> i2c_lock_bus(parent, flags);
> @@ -181,7 +181,7 @@ static void i2c_parent_lock_bus(struct i2c_adapter *adapter,
> struct i2c_mux_priv *priv = adapter->algo_data;
> struct i2c_adapter *parent = priv->muxc->parent;
>
> - rt_mutex_lock(&parent->mux_lock);
> + rt_mutex_lock_nested(&parent->mux_lock, i2c_adapter_depth(adapter));
> i2c_lock_bus(parent, flags);
> }
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [tip:locking/urgent] i2c/mux, locking/core: Annotate the nested rt_mutex usage
2018-07-30 16:09 ` Peter Rosin
@ 2018-07-30 17:17 ` Ingo Molnar
0 siblings, 0 replies; 9+ messages in thread
From: Ingo Molnar @ 2018-07-30 17:17 UTC (permalink / raw)
To: Peter Rosin
Cc: tglx, jsperbeck, linux-kernel, wsa, dave, dpf, pombredanne,
deepadinamani, peterz, gregkh, will.deacon, torvalds, hpa,
linux-tip-commits
* Peter Rosin <peda@axentia.se> wrote:
> On 2018-07-25 16:19, tip-bot for Peter Rosin wrote:
> > Commit-ID: 7b94ea50514d1a0dc94f02723b603c27bc0ea597
> > Gitweb: https://git.kernel.org/tip/7b94ea50514d1a0dc94f02723b603c27bc0ea597
> > Author: Peter Rosin <peda@axentia.se>
> > AuthorDate: Fri, 20 Jul 2018 10:39:14 +0200
> > Committer: Ingo Molnar <mingo@kernel.org>
> > CommitDate: Wed, 25 Jul 2018 11:22:20 +0200
> >
> > i2c/mux, locking/core: Annotate the nested rt_mutex usage
>
> Hi!
>
> I'm a bit curious as to why the subject line was changed on this patch?
>
> (it was "[PATCH v4 2/2] i2c: mux: annotate the nested rt_mutex usage")
>
> I thought the subject as I wrote it was just perfect. Was it so bad that it
> had to be edited?
It wasn't "bad", I improved it to signal that it has a new locking API dependency,
in particular that's it's dependent on this commit:
62cedf3e60af: locking/rtmutex: Allow specifying a subclass for nested locking
This also clarified it why this i2c commit is in locking/urgent.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] rtmutex: allow specifying a subclass for nested locking
2018-07-20 8:39 ` [PATCH v4 1/2] rtmutex: allow specifying a subclass for nested locking Peter Rosin
2018-07-25 14:18 ` [tip:locking/urgent] locking/rtmutex: Allow " tip-bot for Peter Rosin
@ 2018-09-10 23:27 ` Steven Rostedt
2018-09-11 0:58 ` Peter Rosin
1 sibling, 1 reply; 9+ messages in thread
From: Steven Rostedt @ 2018-09-10 23:27 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel, Wolfram Sang, Peter Zijlstra, Ingo Molnar,
Will Deacon, Davidlohr Bueso, Philippe Ombredanne,
Thomas Gleixner, Greg Kroah-Hartman, linux-i2c, Peter Chang,
Deepa Dinamani, John Sperbeck
I just noticed this patch because it conflicted with the PREEMPT_RT v4.14
patch.
On Fri, Jul 20, 2018 at 10:39:13AM +0200, Peter Rosin wrote:
> Needed for annotating rt_mutex locks.
>
> Tested-by: John Sperbeck <jsperbeck@google.com>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
> include/linux/rtmutex.h | 7 +++++++
> kernel/locking/rtmutex.c | 29 +++++++++++++++++++++++++----
> 2 files changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/rtmutex.h b/include/linux/rtmutex.h
> index 1b92a28dd672..6fd615a0eea9 100644
> --- a/include/linux/rtmutex.h
> +++ b/include/linux/rtmutex.h
> @@ -106,7 +106,14 @@ static inline int rt_mutex_is_locked(struct rt_mutex *lock)
> extern void __rt_mutex_init(struct rt_mutex *lock, const char *name, struct lock_class_key *key);
> extern void rt_mutex_destroy(struct rt_mutex *lock);
>
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> +extern void rt_mutex_lock_nested(struct rt_mutex *lock, unsigned int subclass);
> +#define rt_mutex_lock(lock) rt_mutex_lock_nested(lock, 0)
> +#else
> extern void rt_mutex_lock(struct rt_mutex *lock);
> +#define rt_mutex_lock_nested(lock, subclass) rt_mutex_lock(lock)
> +#endif
> +
> extern int rt_mutex_lock_interruptible(struct rt_mutex *lock);
> extern int rt_mutex_timed_lock(struct rt_mutex *lock,
> struct hrtimer_sleeper *timeout);
> diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
> index 4f014be7a4b8..2823d4163a37 100644
> --- a/kernel/locking/rtmutex.c
> +++ b/kernel/locking/rtmutex.c
> @@ -1465,6 +1465,29 @@ rt_mutex_fastunlock(struct rt_mutex *lock,
> rt_mutex_postunlock(&wake_q);
> }
>
> +static inline void __rt_mutex_lock(struct rt_mutex *lock, unsigned int subclass)
> +{
> + might_sleep();
> +
> + mutex_acquire(&lock->dep_map, subclass, 0, _RET_IP_);
> + rt_mutex_fastlock(lock, TASK_UNINTERRUPTIBLE, rt_mutex_slowlock);
> +}
> +
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> +/**
> + * rt_mutex_lock_nested - lock a rt_mutex
> + *
> + * @lock: the rt_mutex to be locked
> + * @subclass: the lockdep subclass
> + */
> +void __sched rt_mutex_lock_nested(struct rt_mutex *lock, unsigned int subclass)
> +{
> + __rt_mutex_lock(lock, subclass);
> +}
> +EXPORT_SYMBOL_GPL(rt_mutex_lock_nested);
> +#endif
> +
> +#ifndef CONFIG_DEBUG_LOCK_ALLOC
Why this:
#ifdef CONFIG_DEBUG_LOCK_ALLOC
[..]
#endif
#ifndef CONFIG_DEBUG_LOCK_ALLOC
[..]
#endif
???
This should use #else.
-- Steve
> /**
> * rt_mutex_lock - lock a rt_mutex
> *
> @@ -1472,12 +1495,10 @@ rt_mutex_fastunlock(struct rt_mutex *lock,
> */
> void __sched rt_mutex_lock(struct rt_mutex *lock)
> {
> - might_sleep();
> -
> - mutex_acquire(&lock->dep_map, 0, 0, _RET_IP_);
> - rt_mutex_fastlock(lock, TASK_UNINTERRUPTIBLE, rt_mutex_slowlock);
> + __rt_mutex_lock(lock, 0);
> }
> EXPORT_SYMBOL_GPL(rt_mutex_lock);
> +#endif
>
> /**
> * rt_mutex_lock_interruptible - lock a rt_mutex interruptible
> --
> 2.11.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] rtmutex: allow specifying a subclass for nested locking
2018-09-10 23:27 ` [PATCH v4 1/2] rtmutex: allow " Steven Rostedt
@ 2018-09-11 0:58 ` Peter Rosin
0 siblings, 0 replies; 9+ messages in thread
From: Peter Rosin @ 2018-09-11 0:58 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Wolfram Sang, Peter Zijlstra, Ingo Molnar,
Will Deacon, Davidlohr Bueso, Philippe Ombredanne,
Thomas Gleixner, Greg Kroah-Hartman, linux-i2c, Peter Chang,
Deepa Dinamani, John Sperbeck
On 2018-09-11 01:27, Steven Rostedt wrote:
>
> Why this:
>
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> [..]
> #endif
>
> #ifndef CONFIG_DEBUG_LOCK_ALLOC
> [..]
> #endif
>
> ???
>
> This should use #else.
Yes. Water under the bridge...
Cheers,
Peter
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-09-11 0:59 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-20 8:39 [PATCH v4 0/2] Fix lockdep warning with nested instances of i2c-mux Peter Rosin
2018-07-20 8:39 ` [PATCH v4 1/2] rtmutex: allow specifying a subclass for nested locking Peter Rosin
2018-07-25 14:18 ` [tip:locking/urgent] locking/rtmutex: Allow " tip-bot for Peter Rosin
2018-09-10 23:27 ` [PATCH v4 1/2] rtmutex: allow " Steven Rostedt
2018-09-11 0:58 ` Peter Rosin
2018-07-20 8:39 ` [PATCH v4 2/2] i2c: mux: annotate the nested rt_mutex usage Peter Rosin
2018-07-25 14:19 ` [tip:locking/urgent] i2c/mux, locking/core: Annotate " tip-bot for Peter Rosin
2018-07-30 16:09 ` Peter Rosin
2018-07-30 17:17 ` Ingo Molnar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).