Add lockdep_assert_not_held()
PATCH v2 2/3 lockdep: add lockdep lock state defines
In reply to Shuah KhanAdds defines for lock state returns from lock_is_held_type() based on Johannes Berg's suggestions as it make it easier to read and maintain the lock states. These are defines and a enum to avoid changes to lock_is_held_type() and lockdep_is_held() return types.
--- include/linux/lockdep.h | 11 +++++++++-- kernel/locking/lockdep.c | 3 ++- 2 files changed, 11 insertions(+), 3 deletions(-)
include/linux/lockdep.h +9 -2
@@ -261,6 +261,11 @@ extern void lock_acquire(struct lockdep_map *lock, unsigned int subclass,extern void lock_release (struct lockdep_map *lock,unsigned long ip); +/* lock_is_held_type() returns */ +#define LOCK_STATE_UNKNOWN -1 +#define LOCK_STATE_NOT_HELD 0 +#define LOCK_STATE_HELD 1 +/* * Same "read" as for lock_acquire(), except -1 means any. */
@@ -295,11 +300,13 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);#define lockdep_depth (tsk) (debug_locks ? (tsk)->lockdep_depth : 0)#define lockdep_assert_held (l) do { \ - WARN_ON(debug_locks && lockdep_is_held(l) == 0); \ + WARN_ON(debug_locks && \ + lockdep_is_held(l) == LOCK_STATE_NOT_HELD); \ } while (0)#define lockdep_assert_not_held (l) do { \ - WARN_ON(debug_locks && lockdep_is_held(l) == 1); \ + WARN_ON(debug_locks && \ + lockdep_is_held(l) == LOCK_STATE_HELD); \ } while (0)#define lockdep_assert_held_write (l) do { \
kernel/locking/lockdep.c +2 -1
@@ -54,6 +54,7 @@#include <linux/nmi.h> #include <linux/rcupdate.h> #include <linux/kprobes.h> +#include <linux/lockdep.h> #include <asm/sections.h>
@@ -5475,7 +5476,7 @@ noinstr int lock_is_held_type(const struct lockdep_map *lock, int read)/* avoid false negative lockdep_assert_not_held() * and lockdep_assert_held() */ -return -1 ; +return LOCK_STATE_UNKNOWN ;raw_local_irq_save (flags);check_flags (flags);
Signature · 7 lines
2.27.0 _______________________________________________ ath10k mailing list [email protected] http://lists.infradead.org/mailman/listinfo/ath10k
v2 cover Add lockdep_assert_not_held()
Some kernel functions must not be called holding a specific lock. Doing so could lead to locking problems. Currently these routines call lock_is_held() to check for lock hold followed by WARN_ON.
Adding a common lockdep interface will help reduce the duplication of this logic in the rest of the kernel.
Add lockdep_assert_not_held() to be used in these functions to detect incorrect calls while holding a lock.
lockdep_assert_not_held() provides the opposite functionality of lockdep_assert_held() which is used to assert calls that require holding a specific lock.
The need for lockdep_assert_not_held() came up in a discussion on ath10k patch. ath10k_drain_tx() and i915_vma_pin_ww() are examples of functions that can use lockdep_assert_not_held().
This patch series adds lockdep_assert_not_held() and uses it in the second patch in ath10k_drain_tx() function.
Patch 1 incorporates suggestions from Peter Zijlstra on v1 series to avoid misfires when lockdep_off() is employed.
Patch 2 Johannes Berg's suggestions as it make it easier to read and maintain the lock states. These are defines and a enum to avoid changes to lock_is_held_type() and lockdep_is_held() return types.
Patch 2 is a separate patch because it adds defines to lockdep.h and kernel/locking/lockdep.c now includes lockdep.h - decided make this a separate patch just in case issues related to header dependencies pop up. I can combine Patches 1&2 if that is preferred.
Patch 3 uses the new interface in ath10k_drain_tx() function. Added Kalle Valo's Ack from v1 for this change.
Tested on the mainline from yesterday.
Shuah Khan (3):
lockdep: add lockdep_assert_not_held()
lockdep: add lockdep lock state defines
ath10k: detect conf_mutex held ath10k_drain_tx() calls
drivers/net/wireless/ath/ath10k/mac.c | 2 ++ include/linux/lockdep.h | 18 +++++++++++++++--- kernel/locking/lockdep.c | 6 +++++- 3 files changed, 22 insertions(+), 4 deletions(-)
Signature · 1 line
2.27.0
PATCH v2 3/3 ath10k: detect conf_mutex held ath10k_drain_tx() calls
ath10k_drain_tx() must not be called with conf_mutex held as workers can use that also. Add call to lockdep_assert_not_held() on conf_mutex to detect if conf_mutex is held by the caller.
The idea for this patch stemmed from coming across the comment block above the ath10k_drain_tx() while reviewing the conf_mutex holds during to debug the conf_mutex lock assert in ath10k_debug_fw_stats_request().
Adding detection to assert on conf_mutex hold will help detect incorrect usages that could lead to locking problems when async worker routines try to call this routine.
--- drivers/net/wireless/ath/ath10k/mac.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/net/wireless/ath/ath10k/mac.c +2 -0
@@ -4567,6 +4567,8 @@ static void ath10k_mac_op_wake_tx_queue(struct ieee80211_hw *hw,/* Must not be called with conf_mutex held as workers can use that also. */ void ath10k_drain_tx (struct ath10k *ar) { +lockdep_assert_not_held (&ar->conf_mutex); +/* make sure rcu-protected mac80211 tx path itself is drained */ synchronize_net ();
Signature · 7 lines
2.27.0 _______________________________________________ ath10k mailing list [email protected] http://lists.infradead.org/mailman/listinfo/ath10k
PATCH v2 1/3 lockdep: add lockdep_assert_not_held()
In reply to Shuah KhanSome kernel functions must be called without holding a specific lock. Add lockdep_assert_not_held() to be used in these functions to detect incorrect calls while holding a lock.
lockdep_assert_not_held() provides the opposite functionality of lockdep_assert_held() which is used to assert calls that require holding a specific lock.
Incorporates suggestions from Peter Zijlstra to avoid misfires when lockdep_off() is employed.
The need for lockdep_assert_not_held() came up in a discussion on ath10k patch. ath10k_drain_tx() and i915_vma_pin_ww() are examples of functions that can use lockdep_assert_not_held().
--- include/linux/lockdep.h | 11 ++++++++--- kernel/locking/lockdep.c | 5 ++++- 2 files changed, 12 insertions(+), 4 deletions(-)
include/linux/lockdep.h +8 -3
@@ -294,8 +294,12 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);#define lockdep_depth (tsk) (debug_locks ? (tsk)->lockdep_depth : 0) -#define lockdep_assert_held (l) do { \ - WARN_ON(debug_locks && !lockdep_is_held(l)); \ +#define lockdep_assert_held (l) do { \ + WARN_ON(debug_locks && lockdep_is_held(l) == 0); \ + } while (0) + +#define lockdep_assert_not_held (l) do { \ + WARN_ON(debug_locks && lockdep_is_held(l) == 1); \ } while (0)#define lockdep_assert_held_write (l) do { \
@@ -384,7 +388,8 @@ extern int lockdep_is_held(const void *);#define lockdep_is_held_type (l, r) (1)#define lockdep_assert_held (l) do { (void)(l); } while (0) -#define lockdep_assert_held_write (l) do { (void)(l); } while (0) +#define lockdep_assert_not_held (l) do { (void)(l); } while (0) +#define lockdep_assert_held_write (l) do { (void)(l); } while (0)#define lockdep_assert_held_read (l) do { (void)(l); } while (0)#define lockdep_assert_held_once (l) do { (void)(l); } while (0)
kernel/locking/lockdep.c +4 -1
@@ -5472,7 +5472,10 @@ noinstr int lock_is_held_type(const struct lockdep_map *lock, int read)int ret =0 ;if (unlikely (!lockdep_enabled ())) -return 1 ;/* avoid false negative lockdep_assert_held() */ +/* avoid false negative lockdep_assert_not_held() +* and lockdep_assert_held() +*/ +return -1 ;raw_local_irq_save (flags);check_flags (flags);
Signature · 7 lines
2.27.0 _______________________________________________ ath10k mailing list [email protected] http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v2 2/3] lockdep: add lockdep lock state defines
In reply to Shuah Khankernel/locking/lockdep.c noinstr int lock_is_held_type(const struct lockdep_map *lock, int read) · line 5479
Quoted 7 lines
kernel/locking/lockdep.c +1
-1@@ -5475,7 +5476,7 @@ noinstr int lock_is_held_type(const struct lockdep_map *lock, int read)/* avoid false negative lockdep_assert_not_held() * and lockdep_assert_held() */ -return -1 ; +return LOCK_STATE_UNKNOWN ;
I'd argue that then the other two return places here should also be changed.
johannes
_______________________________________________
ath10k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v2 2/3] lockdep: add lockdep lock state defines
Quoted 11 lines · Johannes Berg
kernel/locking/lockdep.c noinstr int lock_is_held_type(const struct lockdep_map *lock, int read) · line 5479
Quoted 7 lines
kernel/locking/lockdep.c +1
-1@@ -5475,7 +5476,7 @@ noinstr int lock_is_held_type(const struct lockdep_map *lock, int read)/* avoid false negative lockdep_assert_not_held() * and lockdep_assert_held() */ -return -1 ; +return LOCK_STATE_UNKNOWN ;I'd argue that then the other two return places here should also be changed.
Makes sense.
Since lock_is_held_type() simply returns what __lock_is_held() for the other cases, __lock_is_held() is the one that needs changes to use these defines.
thanks,
-- Shuah
_______________________________________________
ath10k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath10k
Peter Zijlstra
maintainerRe: [PATCH v2 1/3] lockdep: add lockdep_assert_not_held()
In reply to Shuah Khankernel/locking/lockdep.c noinstr int lock_is_held_type(const struct lockdep_map *lock, int read) · line 5477
Quoted 3 lines · Shuah Khan
kernel/locking/lockdep.c +3
-0+/* avoid false negative lockdep_assert_not_held() +* and lockdep_assert_held() +*/
That's a coding style fail.
_______________________________________________
ath10k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v2 1/3] lockdep: add lockdep_assert_not_held()
Quoted 7 lines · Peter Zijlstra
kernel/locking/lockdep.c noinstr int lock_is_held_type(const struct lockdep_map *lock, int read) · line 5477
Quoted 3 lines · Shuah Khan
kernel/locking/lockdep.c +3
-0+/* avoid false negative lockdep_assert_not_held() +* and lockdep_assert_held() +*/ That's a coding style fail.
Checkpatch didn't complain. What's your preference? Does the following work for you?
/*
* avoid false negative lockdep_assert_not_held()
* and lockdep_assert_held()
*/
thanks,
-- Shuah
_______________________________________________
ath10k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath10k
Peter Zijlstra
maintainerRe: [PATCH v2 1/3] lockdep: add lockdep_assert_not_held()
Quoted 10 lines · Shuah Khan
Quoted 7 lines · Peter Zijlstra
kernel/locking/lockdep.c noinstr int lock_is_held_type(const struct lockdep_map *lock, int read) · line 5477
Quoted 3 lines · Shuah Khan
kernel/locking/lockdep.c +3
-0+/* avoid false negative lockdep_assert_not_held() +* and lockdep_assert_held() +*/ That's a coding style fail.
Checkpatch didn't complain.
Documentation/CodingStyle
(or whatever unreadable rst crap it is today :-( )
and:
https://lkml.kernel.org/lkml/CA+55aFyQYJerovMsSoSKS7PessZBr4vNp-3QUUwhqk4A4_jcbg@mail.gmail.com/
Quoted 6 lines
What's your preference? Does the following work for you?
/*
* avoid false negativelockdep_assert_not_held()
* andlockdep_assert_held()
*/
Yep (ideally even with a Capital and full stop).
_______________________________________________
ath10k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath10k