Message ID | 20200409082822.27314-1-madhuparnabhowmik10@gmail.com |
---|---|
State | Awaiting Upstream |
Delegated to: | David Miller |
Headers | show |
Series | [1/4] net: mac80211: util.c: Fix RCU list usage warnings | expand |
Hi, > This patch fixes the following warning (CONIG_PROVE_RCU_LIST) > in ieee80211_check_combinations(). Thanks, and sorry for the delay. > +++ b/net/mac80211/util.c > @@ -254,7 +254,7 @@ static void __ieee80211_wake_txqs(struct ieee80211_sub_if_data *sdata, int ac) > > sdata->vif.txqs_stopped[ac] = false; > > - list_for_each_entry_rcu(sta, &local->sta_list, list) { > + list_for_each_entry(sta, &local->sta_list, list) { > if (sdata != sta->sdata) > continue; In this case, for example, I don't even understand why the warning would happen, because certainly the only caller of this (_ieee80211_wake_txqs) does rcu_read_lock()? I'm also not convinced that the necessary lock is actually held here, this comes from a tasklet that doesn't hold any locks? I'd appreciate if you could add comments/explain why you think the changes were right, or ideally even add "lockdep_assert_held()" annotations. That would make it much easier to check this patch. > @@ -3931,7 +3932,7 @@ int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata, > params.num_different_channels++; > } > > - list_for_each_entry_rcu(sdata_iter, &local->interfaces, list) { > + list_for_each_entry(sdata_iter, &local->interfaces, list) { > struct wireless_dev *wdev_iter; > > wdev_iter = &sdata_iter->wdev; > @@ -3982,7 +3983,7 @@ int ieee80211_max_num_channels(struct ieee80211_local *local) > ieee80211_chanctx_radar_detect(local, ctx); > } > > - list_for_each_entry_rcu(sdata, &local->interfaces, list) > + list_for_each_entry(sdata, &local->interfaces, list) > params.iftype_num[sdata->wdev.iftype]++; These changes correct, as far as I can tell, in that they rely on the RTNL now - but can you perhaps document that as well? There doesn't seem to be any multi-lock version of lockdep_assert_held() or is there? That'd be _really_ useful here, because I want to get rid of some RTNL reliance in the longer term, and having annotation here saying "either RTNL or iflist_mtx is fine" would be good. johannes
diff --git a/net/mac80211/util.c b/net/mac80211/util.c index 20436c86b9bf..f4b0434024c0 100644 --- a/net/mac80211/util.c +++ b/net/mac80211/util.c @@ -254,7 +254,7 @@ static void __ieee80211_wake_txqs(struct ieee80211_sub_if_data *sdata, int ac) sdata->vif.txqs_stopped[ac] = false; - list_for_each_entry_rcu(sta, &local->sta_list, list) { + list_for_each_entry(sta, &local->sta_list, list) { if (sdata != sta->sdata) continue; @@ -719,7 +719,8 @@ static void __iterate_interfaces(struct ieee80211_local *local, struct ieee80211_sub_if_data *sdata; bool active_only = iter_flags & IEEE80211_IFACE_ITER_ACTIVE; - list_for_each_entry_rcu(sdata, &local->interfaces, list) { + list_for_each_entry_rcu(sdata, &local->interfaces, list, + (lockdep_is_held(&local->iflist_mtx)|| lockdep_rtnl_is_held())) { switch (sdata->vif.type) { case NL80211_IFTYPE_MONITOR: if (!(sdata->u.mntr.flags & MONITOR_FLAG_ACTIVE)) @@ -3931,7 +3932,7 @@ int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata, params.num_different_channels++; } - list_for_each_entry_rcu(sdata_iter, &local->interfaces, list) { + list_for_each_entry(sdata_iter, &local->interfaces, list) { struct wireless_dev *wdev_iter; wdev_iter = &sdata_iter->wdev; @@ -3982,7 +3983,7 @@ int ieee80211_max_num_channels(struct ieee80211_local *local) ieee80211_chanctx_radar_detect(local, ctx); } - list_for_each_entry_rcu(sdata, &local->interfaces, list) + list_for_each_entry(sdata, &local->interfaces, list) params.iftype_num[sdata->wdev.iftype]++; err = cfg80211_iter_combinations(local->hw.wiphy, ¶ms,