linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kalle Valo <kvalo@codeaurora.org>
To: Shuah Khan <skhan@linuxfoundation.org>
Cc: netdev@vger.kernel.org, linux-wireless@vger.kernel.org,
	linux-kernel@vger.kernel.org, ath10k@lists.infradead.org,
	kuba@kernel.org, davem@davemloft.net
Subject: Re: [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls
Date: Thu, 11 Feb 2021 13:20:26 +0200	[thread overview]
Message-ID: <871rdmu9z9.fsf@codeaurora.org> (raw)
In-Reply-To: <d6d8c7b8-f69d-01ef-6d66-8a33ea98920f@linuxfoundation.org> (Shuah Khan's message of "Wed, 10 Feb 2021 08:57:04 -0700")

Shuah Khan <skhan@linuxfoundation.org> writes:

> On 2/10/21 1:25 AM, Kalle Valo wrote:
>> Shuah Khan <skhan@linuxfoundation.org> 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@linuxfoundation.org>
>>
>> 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.

Ok, makes sense. I prefer having this background info in the commit log,
for example "found by code review" or something like that. Or just copy
what you wrote above :)

>>> --- 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()

Great, thank you. The only problem is that lockdep.h changes have to go
via some other tree, I just don't know which :) I think it would be
easiest if also the ath10k patch goes via that other tree, I can ack the
ath10k changes.

Another option is that I'll apply the ath10k patch after the lockdep.h
change has trickled down to my tree, but that usually happens only after
the merge window and means weeks of waiting. Either is fine for me.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

  reply	other threads:[~2021-02-11 11:25 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-10  0:42 [PATCH 0/5] ath10k fixes for warns Shuah Khan
2021-02-10  0:42 ` [PATCH 1/5] ath10k: fix conf_mutex lock assert in ath10k_debug_fw_stats_request() Shuah Khan
2021-02-10  8:09   ` Kalle Valo
     [not found]   ` <20210210080915.8A81AC433C6@smtp.codeaurora.org>
2021-02-10 16:21     ` Shuah Khan
2021-02-10  0:42 ` [PATCH 2/5] ath10k: fix WARNING: suspicious RCU usage Shuah Khan
2021-02-10  8:13   ` Kalle Valo
     [not found]   ` <20210210081320.2FBE5C433CA@smtp.codeaurora.org>
2021-02-10 16:22     ` Shuah Khan
2021-02-10  0:42 ` [PATCH 3/5] ath10k: change ath10k_offchan_tx_work() peer present msg to a warn Shuah Khan
2021-02-11  6:50   ` Kalle Valo
2021-02-10  0:42 ` [PATCH 4/5] ath10k: detect conf_mutex held ath10k_drain_tx() calls Shuah Khan
2021-02-10  8:03   ` kernel test robot
2021-02-10  8:25   ` Kalle Valo
2021-02-10 15:57     ` Shuah Khan
2021-02-11 11:20       ` Kalle Valo [this message]
2021-02-11 20:53         ` Shuah Khan
2021-02-11  0:13   ` kernel test robot
2021-02-10  0:42 ` [PATCH 5/5] ath10k: reduce invalid ht params rate message noise Shuah Khan
2021-02-10  2:36   ` Wen Gong
2021-02-10  8:28     ` Kalle Valo
2021-02-10 16:13       ` Shuah Khan
2021-02-11 11:24         ` Kalle Valo
2021-02-26 18:01           ` Shuah Khan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=871rdmu9z9.fsf@codeaurora.org \
    --to=kvalo@codeaurora.org \
    --cc=ath10k@lists.infradead.org \
    --cc=davem@davemloft.net \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=skhan@linuxfoundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).