On Sun, 2024-06-30 at 09:50 +0200, Felix Fietkau wrote:
>
> +++ b/net/mac80211/ieee80211_i.h
> @@ -2043,6 +2043,7 @@ static inline bool ieee80211_sdata_running(struct ieee80211_sub_if_data *sdata)
> {
> return test_bit(SDATA_STATE_RUNNING, &sdata->state);
> }
> +u32 ieee80211_get_radio_mask(struct wiphy *wiphy, struct net_device *dev);
nit: I feel like maybe there's a better place, and even if not there
should be a blank line? :)
>
> /* link handling */
> void ieee80211_link_setup(struct ieee80211_link_data *link);
> @@ -2640,8 +2641,8 @@ void ieee80211_recalc_dtim(struct ieee80211_local *local,
> int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
> const struct cfg80211_chan_def *chandef,
> enum ieee80211_chanctx_mode chanmode,
> - u8 radar_detect);
> -int ieee80211_max_num_channels(struct ieee80211_local *local);
> + u8 radar_detect, int radio_idx);
> +int ieee80211_max_num_channels(struct ieee80211_local *local, int radio_idx);
> void ieee80211_recalc_chanctx_chantype(struct ieee80211_local *local,
> struct ieee80211_chanctx *ctx);
but maybe just put it next to ieee80211_max_num_channels()?
> +static u32
> +__ieee80211_get_radio_mask(struct ieee80211_sub_if_data *sdata)
> +{
> + struct ieee80211_bss_conf *link_conf;
> + struct ieee80211_chanctx_conf *conf;
> + unsigned int link_id;
> + u32 mask = 0;
> +
> + for_each_vif_active_link(&sdata->vif, link_conf, link_id) {
> + conf = rcu_dereference(link_conf->chanctx_conf);
> + if (!conf || conf->radio_idx < 0)
> + continue;
> +
> + mask |= BIT(conf->radio_idx);
> + }
> +
> + return mask;
> +}
> +
> +u32 ieee80211_get_radio_mask(struct wiphy *wiphy, struct net_device *dev)
> +{
> + struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> +
> + return __ieee80211_get_radio_mask(sdata);
Here's the context stuff I was talking about - you're using
rcu_dereference() when it almost seems it should be sdata_dereference()
(or so, it's all equivalent now with wiphy dereference)?
I'm not sure how this even gets to be in an RCU critical section, e.g.
when called via
> +static bool
> +ieee80211_sdata_uses_radio(struct ieee80211_sub_if_data *sdata, int radio_idx)
> +{
> + if (radio_idx < 0)
> + return true;
> +
> + return __ieee80211_get_radio_mask(sdata) & BIT(radio_idx);
> +}
...
> +static void
> +ieee80211_fill_ifcomb_params(struct ieee80211_local *local,
> + struct iface_combination_params *params,
> + const struct cfg80211_chan_def *chandef,
> + struct ieee80211_sub_if_data *sdata)
> +{
...
> + list_for_each_entry_rcu(sdata_iter, &local->interfaces, list) {
> + struct wireless_dev *wdev_iter;
> +
> + wdev_iter = &sdata_iter->wdev;
> +
> + if (sdata_iter == sdata ||
> + !ieee80211_sdata_running(sdata_iter) ||
> + cfg80211_iftype_allowed(local->hw.wiphy,
> + wdev_iter->iftype, 0, 1))
> + continue;
> +
> + if (!ieee80211_sdata_uses_radio(sdata_iter, params->radio_idx))
> + continue;
here?
Also the list_for_each_entry_rcu() seems weird (although I see that we
even have pre-existing code like that now), because e.g.
ieee80211_open()
-> ieee80211_check_concurrent_iface()
-> ieee80211_check_combinations()
is called outside RCU critical section? There's an existing
list_for_each_entry_rcu() inside of it which seems like it should be a
problem, so maybe I'm missing something?
... investigates a bit ...
yeah I can't get even CONFIG_PROVE_RCU_LIST to spit out warnings, but
checking manually, we (generally) aren't in RCU critical section here,
at least not unconditionally.
johannes
