* [PATCH v2 1/3] lockdep: add lockdep_assert_not_held()
2021-02-26 17:52 [PATCH v2 0/3] Add lockdep_assert_not_held() Shuah Khan
@ 2021-02-26 17:52 ` Shuah Khan
2021-02-26 21:07 ` Peter Zijlstra
2021-02-26 17:52 ` [PATCH v2 2/3] lockdep: add lockdep lock state defines Shuah Khan
2021-02-26 17:52 ` [PATCH v2 3/3] ath10k: detect conf_mutex held ath10k_drain_tx() calls Shuah Khan
2 siblings, 1 reply; 9+ messages in thread
From: Shuah Khan @ 2021-02-26 17:52 UTC (permalink / raw)
To: peterz, mingo, will, kvalo, davem, kuba
Cc: netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan
Some 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().
Link: https://lore.kernel.org/lkml/37a29c383bff2fb1605241ee6c7c9be3784fb3c6.1613171185.git.skhan@linuxfoundation.org/
Link: https://lore.kernel.org/linux-wireless/871rdmu9z9.fsf@codeaurora.org/
Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
include/linux/lockdep.h | 11 ++++++++---
kernel/locking/lockdep.c | 5 ++++-
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index b9e9adec73e8..93eb5f797fc1 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -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)
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index bdaf4829098c..d3299fd85c4a 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -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);
--
2.27.0
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] lockdep: add lockdep_assert_not_held()
2021-02-26 17:52 ` [PATCH v2 1/3] lockdep: add lockdep_assert_not_held() Shuah Khan
@ 2021-02-26 21:07 ` Peter Zijlstra
2021-02-26 21:16 ` Shuah Khan
0 siblings, 1 reply; 9+ messages in thread
From: Peter Zijlstra @ 2021-02-26 21:07 UTC (permalink / raw)
To: Shuah Khan
Cc: netdev, linux-wireless, linux-kernel, ath10k, mingo, kuba, will,
davem, kvalo
On Fri, Feb 26, 2021 at 10:52:13AM -0700, Shuah Khan wrote:
> + /* avoid false negative lockdep_assert_not_held()
> + * and lockdep_assert_held()
> + */
That's a coding style fail.
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] lockdep: add lockdep_assert_not_held()
2021-02-26 21:07 ` Peter Zijlstra
@ 2021-02-26 21:16 ` Shuah Khan
2021-03-01 8:28 ` Peter Zijlstra
0 siblings, 1 reply; 9+ messages in thread
From: Shuah Khan @ 2021-02-26 21:16 UTC (permalink / raw)
To: Peter Zijlstra
Cc: netdev, linux-wireless, linux-kernel, ath10k, mingo, Shuah Khan,
kuba, will, davem, kvalo
On 2/26/21 2:07 PM, Peter Zijlstra wrote:
> On Fri, Feb 26, 2021 at 10:52:13AM -0700, Shuah Khan wrote:
>> + /* 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
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] lockdep: add lockdep_assert_not_held()
2021-02-26 21:16 ` Shuah Khan
@ 2021-03-01 8:28 ` Peter Zijlstra
0 siblings, 0 replies; 9+ messages in thread
From: Peter Zijlstra @ 2021-03-01 8:28 UTC (permalink / raw)
To: Shuah Khan
Cc: netdev, linux-wireless, linux-kernel, ath10k, mingo, kuba, will,
davem, kvalo
On Fri, Feb 26, 2021 at 02:16:12PM -0700, Shuah Khan wrote:
> On 2/26/21 2:07 PM, Peter Zijlstra wrote:
> > On Fri, Feb 26, 2021 at 10:52:13AM -0700, Shuah Khan wrote:
> > > + /* 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/
> What's your preference? Does the following work for you?
>
> /*
> * avoid false negative lockdep_assert_not_held()
> * and lockdep_assert_held()
> */
Yep (ideally even with a Capital and full stop).
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/3] lockdep: add lockdep lock state defines
2021-02-26 17:52 [PATCH v2 0/3] Add lockdep_assert_not_held() Shuah Khan
2021-02-26 17:52 ` [PATCH v2 1/3] lockdep: add lockdep_assert_not_held() Shuah Khan
@ 2021-02-26 17:52 ` Shuah Khan
2021-02-26 18:03 ` Johannes Berg
2021-02-26 17:52 ` [PATCH v2 3/3] ath10k: detect conf_mutex held ath10k_drain_tx() calls Shuah Khan
2 siblings, 1 reply; 9+ messages in thread
From: Shuah Khan @ 2021-02-26 17:52 UTC (permalink / raw)
To: peterz, mingo, will, kvalo, davem, kuba
Cc: netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan
Adds 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.
Link: https://lore.kernel.org/lkml/37a29c383bff2fb1605241ee6c7c9be3784fb3c6.1613171185.git.skhan@linuxfoundation.org/
Link: https://lore.kernel.org/linux-wireless/871rdmu9z9.fsf@codeaurora.org/
Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
include/linux/lockdep.h | 11 +++++++++--
kernel/locking/lockdep.c | 3 ++-
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 93eb5f797fc1..7c2622854152 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -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 { \
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index d3299fd85c4a..e3898c888ad2 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -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);
--
2.27.0
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] lockdep: add lockdep lock state defines
2021-02-26 17:52 ` [PATCH v2 2/3] lockdep: add lockdep lock state defines Shuah Khan
@ 2021-02-26 18:03 ` Johannes Berg
2021-02-26 18:16 ` Shuah Khan
0 siblings, 1 reply; 9+ messages in thread
From: Johannes Berg @ 2021-02-26 18:03 UTC (permalink / raw)
To: Shuah Khan, peterz, mingo, will, kvalo, davem, kuba
Cc: netdev, linux-wireless, linux-kernel, ath10k
> @@ -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
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] lockdep: add lockdep lock state defines
2021-02-26 18:03 ` Johannes Berg
@ 2021-02-26 18:16 ` Shuah Khan
0 siblings, 0 replies; 9+ messages in thread
From: Shuah Khan @ 2021-02-26 18:16 UTC (permalink / raw)
To: Johannes Berg, peterz, mingo, will, kvalo, davem, kuba
Cc: netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan
On 2/26/21 11:03 AM, Johannes Berg wrote:
>
>> @@ -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
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 3/3] ath10k: detect conf_mutex held ath10k_drain_tx() calls
2021-02-26 17:52 [PATCH v2 0/3] Add lockdep_assert_not_held() Shuah Khan
2021-02-26 17:52 ` [PATCH v2 1/3] lockdep: add lockdep_assert_not_held() Shuah Khan
2021-02-26 17:52 ` [PATCH v2 2/3] lockdep: add lockdep lock state defines Shuah Khan
@ 2021-02-26 17:52 ` Shuah Khan
2 siblings, 0 replies; 9+ messages in thread
From: Shuah Khan @ 2021-02-26 17:52 UTC (permalink / raw)
To: peterz, mingo, will, kvalo, davem, kuba
Cc: netdev, linux-wireless, linux-kernel, ath10k, Shuah Khan
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.
Link: https://lore.kernel.org/lkml/37a29c383bff2fb1605241ee6c7c9be3784fb3c6.1613171185.git.skhan@linuxfoundation.org/
Link: https://lore.kernel.org/linux-wireless/871rdmu9z9.fsf@codeaurora.org/
Acked-by: Kalle Valo <kvalo@codeaurora.org>
Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
drivers/net/wireless/ath/ath10k/mac.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 7d98250380ec..a595ff4f0843 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -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();
--
2.27.0
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply related [flat|nested] 9+ messages in thread