On 2020-12-09 23:00, Ben Greear wrote:
On 12/9/20 1:24 AM, Kalle Valo wrote:
Wen Gong <wgong@xxxxxxxxxxxxxx> writes:
On 2020-09-08 00:22, Kalle Valo wrote:
Just like with the recent firmware restart patch, isn't
ar->napi_enabled
racy? Wouldn't test_and_set_bit() and test_and_clear_bit() be safer?
Or are we holding a lock? But then that should be documented with
lockdep_assert_held().
yes, ath10k_hif_start is only called from ath10k_core_start, it has
"lockdep_assert_held(&ar->conf_mutex)", and ath10k_hif_stop is only
called from ath10k_core_stop, it also has
"lockdep_assert_held(&ar->conf_mutex)". then it will not 2 thread
both
enter ath10k_hif_start/ath10k_hif_stop meanwhile.
Ok, but every function depending on a lock being held should still
call
lockdep_assert_held(), that way we can catch the bug if locking
changes
later. So it's not enough that ath10k_core_stop() has
lockdep_assert_held(), also these napi functions should have it.
I actually decided to switch using ATH10K_FLAG_NAPI_ENABLED with
set_bit() & co, simpler locking that way and no lockdep_assert_held()
needed anymore. Please check my changes in the pending branch, I have
only compile tested them:
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=e0a466d296bd862080f7796b41349f9f586272c9
Why do you not need locking? You can't just check a bit is set and
then do work and set
it later without locking, two concurrent CPU threads can pass the
first check and both get into
the logic below it?
maybe because which I said before:
ath10k_hif_start is only called from ath10k_core_start, it has
"lockdep_assert_held(&ar->conf_mutex)", and ath10k_hif_stop is only
called from ath10k_core_stop, it also has
"lockdep_assert_held(&ar->conf_mutex)". then it will not 2 thread both
enter ath10k_hif_start/ath10k_hif_stop meanwhile.
Thanks,
Ben
