I was looking into this as part of https://bugzilla.kernel.org/show_bug.cgi?id=206971 and had a similar fix in flight. My concern was that queue_owner being used outside of the RCU might be an issue as now you have no guaranty that the eventual use of sta->txq[tid] in iwl_mvm_free_inactive_queue() is going to be valid. The only way to work around this is instead of storing queue_owner, store mvmtxq = iwl_mvm_txq_from_tid(sta, i), then adjust iwl_mvm_free_inactive_queue(), iwl_mvm_disable_txq() and whatnot to take struct iwl_mvm_txq * instead of struct ieee80211_sta *. If you open the bug you will see the latest version of my work as the attached patch. I am not an RCU expert so I am curious to hear your thoughts. On Fri, Apr 3, 2020 at 4:31 AM Luca Coelho <luca@xxxxxxxxx> wrote: > > From: Johannes Berg <johannes.berg@xxxxxxxxx> > > iwl_mvm_free_inactive_queue() will sleep in synchronize_net() under > some circumstances, so don't call it under RCU. There doesn't appear > to be a need for RCU protection around this particular call. > > Cc: stable@xxxxxxxxxxxxxxx # v5.4+ > Signed-off-by: Johannes Berg <johannes.berg@xxxxxxxxx> > Signed-off-by: Luca Coelho <luciano.coelho@xxxxxxxxx> > --- > drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c > index 56ae72debb96..9ca433fdf634 100644 > --- a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c > @@ -1184,17 +1184,15 @@ static int iwl_mvm_inactivity_check(struct iwl_mvm *mvm, u8 alloc_for_sta) > for_each_set_bit(i, &changetid_queues, IWL_MAX_HW_QUEUES) > iwl_mvm_change_queue_tid(mvm, i); > > + rcu_read_unlock(); > + > if (free_queue >= 0 && alloc_for_sta != IWL_MVM_INVALID_STA) { > ret = iwl_mvm_free_inactive_queue(mvm, free_queue, queue_owner, > alloc_for_sta); > - if (ret) { > - rcu_read_unlock(); > + if (ret) > return ret; > - } > } > > - rcu_read_unlock(); > - > return free_queue; > } > > -- > 2.25.1 >
