* [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; 5+ 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] 5+ 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-09-10 23:27 ` Steven Rostedt 2018-07-20 8:39 ` [PATCH v4 2/2] i2c: mux: annotate the nested rt_mutex usage Peter Rosin 1 sibling, 1 reply; 5+ 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] 5+ 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-09-10 23:27 ` Steven Rostedt 2018-09-11 0:58 ` Peter Rosin 0 siblings, 1 reply; 5+ 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] 5+ messages in thread
* Re: [PATCH v4 1/2] rtmutex: allow specifying a subclass for nested locking 2018-09-10 23:27 ` Steven Rostedt @ 2018-09-11 0:58 ` Peter Rosin 0 siblings, 0 replies; 5+ 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] 5+ 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 1 sibling, 0 replies; 5+ 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] 5+ messages in thread
end of thread, other threads:[~2018-09-11 0:58 UTC | newest] Thread overview: 5+ 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-09-10 23:27 ` 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
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).