Re: [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls

[Shuah Khan]

On 2/10/21 1:25 AM, Kalle Valo wrote:
> Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx> writes:
>
> > ath10k_drain_tx() must not be called with conf_mutex held as workers can
> > use that also. Add check to detect conf_mutex held calls.
> >
> > Signed-off-by: Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>
>
> The commit log does not answer to "Why?". How did you find this? What
> actual problem are you trying to solve?

I came across the comment block above the ath10k_drain_tx() as I was
reviewing at conf_mutex holds while I was debugging the conf_mutex
lock assert in ath10k_debug_fw_stats_request().

My reasoning is that having this will help detect incorrect usages
of ath10k_drain_tx() while holding conf_mutex which could lead to
locking problems when async worker routines try to call this routine.

> > ---
> >   drivers/net/wireless/ath/ath10k/mac.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> > index 53f92945006f..3545ce7dce0a 100644
> > --- a/drivers/net/wireless/ath/ath10k/mac.c
> > +++ b/drivers/net/wireless/ath/ath10k/mac.c
> > @@ -4566,6 +4566,7 @@ 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)
> >   {
> > +	WARN_ON(lockdep_is_held(&ar->conf_mutex));
>
> Empty line after WARN_ON().

Will do.

> Shouldn't this check debug_locks similarly lockdep_assert_held() does?
>
> #define lockdep_assert_held(l)	do {				\
> 		WARN_ON(debug_locks && !lockdep_is_held(l));	\
> 	} while (0)
>
> And I suspect you need #ifdef CONFIG_LOCKDEP which should fix the kbuild
> bot error.

Yes.

> But honestly I would prefer to have lockdep_assert_not_held() in
> include/linux/lockdep.h, much cleaner that way. Also
> i915_gem_object_lookup_rcu() could then use the same macro.

Right. This is the right way to go. That was first instinct and
decided to have the discussion evolve in that direction. Now that
it has, I will combine this change with
include/linux/lockdep.h and add lockdep_assert_not_held()

I think we might have other places in the kernel that could use
lockdep_assert_not_held() in addition to i915_gem_object_lookup_rcu()

thanks,
-- Shuah