| Message ID | 754815488294f5b7f599b2adba085f1889b53b1a.1717696995.git-series.nbd@nbd.name (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | cfg80211/mac80211: support defining multiple radios per wiphy | expand |
On Thu, 2024-06-06 at 20:07 +0200, Felix Fietkau wrote: > > @@ -4577,6 +4579,7 @@ struct mgmt_frame_regs { > * > * @set_hw_timestamp: Enable/disable HW timestamping of TM/FTM frames. > * @set_ttlm: set the TID to link mapping. > + * @get_radio_mask: get bitmask of radios in use > */ > struct cfg80211_ops { > int (*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow); > @@ -4938,6 +4941,8 @@ struct cfg80211_ops { > struct cfg80211_set_hw_timestamp *hwts); > int (*set_ttlm)(struct wiphy *wiphy, struct net_device *dev, > struct cfg80211_ttlm_params *params); > + int (*get_radio_mask)(struct wiphy *wiphy, struct net_device *dev, > + u32 *mask); not sure I see the point of this being a callback rather than being passed in? (Also, if really needed, do you actually expect a device with 32 radios? if not you can use a return value instead of u32 *mask out pointer :) ) > +DEFINE_EVENT(wiphy_netdev_evt, rdev_get_radio_mask, > + TP_PROTO(struct wiphy *wiphy, struct net_device *netdev), > + TP_ARGS(wiphy, netdev) > +); and if we do need it that really should trace not just the fact that it happened but also the return value and mask > static void cfg80211_calculate_bi_data(struct wiphy *wiphy, u32 new_beacon_int, > u32 *beacon_int_gcd, > - bool *beacon_int_different) > + bool *beacon_int_different, > + const struct wiphy_radio *radio) > { > + struct cfg80211_registered_device *rdev; > struct wireless_dev *wdev; > + int radio_idx = -1; > > *beacon_int_gcd = 0; > *beacon_int_different = false; > + if (radio) > + radio_idx = radio - wiphy->radio; This can go oh so wrong ... and technically even be UB. I'd rather pass the index from the driver, I guess, and validate it against n_radios. > + rdev = wiphy_to_rdev(wiphy); > list_for_each_entry(wdev, &wiphy->wdev_list, list) { > int wdev_bi; > + u32 mask; > > /* this feature isn't supported with MLO */ > if (wdev->valid_links) > continue; Are we expecting this to change? because the premise of this patchset is MLO support, and yet with real MLO we won't get here? Or is that because non-MLO interfaces could be created on this wiphy? > > + if (radio_idx >= 0) { > + if (rdev_get_radio_mask(rdev, wdev->netdev, &mask)) > + continue; here: given that 'radio'/'radio_idx' is passed in, not sure I see why the mask couldn't also be passed in? > + if (!(mask & BIT(radio_idx))) > + continue; that could use a comment > - for (i = 0; i < wiphy->n_iface_combinations; i++) { > - const struct ieee80211_iface_combination *c; > + if (radio) { > + c = radio->iface_combinations; > + n = radio->n_iface_combinations; > + } else { > + c = wiphy->iface_combinations; > + n = wiphy->n_iface_combinations; > + } > + for (i = 0; i < n; i++, c++) { that c++ is a bit too hidden for my taste there, but YMMV and I guess if I wasn't reading the diff it'd be more obvious :) johannes
On 07.06.24 11:32, Johannes Berg wrote: > On Thu, 2024-06-06 at 20:07 +0200, Felix Fietkau wrote: >> >> @@ -4577,6 +4579,7 @@ struct mgmt_frame_regs { >> * >> * @set_hw_timestamp: Enable/disable HW timestamping of TM/FTM frames. >> * @set_ttlm: set the TID to link mapping. >> + * @get_radio_mask: get bitmask of radios in use >> */ >> struct cfg80211_ops { >> int (*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow); >> @@ -4938,6 +4941,8 @@ struct cfg80211_ops { >> struct cfg80211_set_hw_timestamp *hwts); >> int (*set_ttlm)(struct wiphy *wiphy, struct net_device *dev, >> struct cfg80211_ttlm_params *params); >> + int (*get_radio_mask)(struct wiphy *wiphy, struct net_device *dev, >> + u32 *mask); > > > not sure I see the point of this being a callback rather than being > passed in? > > (Also, if really needed, do you actually expect a device with 32 radios? > if not you can use a return value instead of u32 *mask out pointer :) ) I'll update the callback to return u32 >> +DEFINE_EVENT(wiphy_netdev_evt, rdev_get_radio_mask, >> + TP_PROTO(struct wiphy *wiphy, struct net_device *netdev), >> + TP_ARGS(wiphy, netdev) >> +); > > and if we do need it that really should trace not just the fact that it > happened but also the return value and mask > >> static void cfg80211_calculate_bi_data(struct wiphy *wiphy, u32 new_beacon_int, >> u32 *beacon_int_gcd, >> - bool *beacon_int_different) >> + bool *beacon_int_different, >> + const struct wiphy_radio *radio) >> { >> + struct cfg80211_registered_device *rdev; >> struct wireless_dev *wdev; >> + int radio_idx = -1; >> >> *beacon_int_gcd = 0; >> *beacon_int_different = false; >> + if (radio) >> + radio_idx = radio - wiphy->radio; > > This can go oh so wrong ... and technically even be UB. I'd rather pass > the index from the driver, I guess, and validate it against n_radios. Will pass the index in directly. >> + rdev = wiphy_to_rdev(wiphy); >> list_for_each_entry(wdev, &wiphy->wdev_list, list) { >> int wdev_bi; >> + u32 mask; >> >> /* this feature isn't supported with MLO */ >> if (wdev->valid_links) >> continue; > > Are we expecting this to change? because the premise of this patchset is > MLO support, and yet with real MLO we won't get here? > > Or is that because non-MLO interfaces could be created on this wiphy? Not sure about this. I guess we can revisit it later since it's out of scope for this series. >> >> + if (radio_idx >= 0) { >> + if (rdev_get_radio_mask(rdev, wdev->netdev, &mask)) >> + continue; > > > here: given that 'radio'/'radio_idx' is passed in, not sure I see why > the mask couldn't also be passed in? mask is supposed to be per wdev, which is iterated in a loop here. > >> + if (!(mask & BIT(radio_idx))) >> + continue; > > that could use a comment > >> - for (i = 0; i < wiphy->n_iface_combinations; i++) { >> - const struct ieee80211_iface_combination *c; >> + if (radio) { >> + c = radio->iface_combinations; >> + n = radio->n_iface_combinations; >> + } else { >> + c = wiphy->iface_combinations; >> + n = wiphy->n_iface_combinations; >> + } >> + for (i = 0; i < n; i++, c++) { > > that c++ is a bit too hidden for my taste there, but YMMV and I guess if > I wasn't reading the diff it'd be more obvious :) Will move it into the loop body to make it more obvious. Thanks, - Felix
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 27355e08ae52..35d4d18e0500 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -1595,6 +1595,7 @@ struct cfg80211_color_change_settings { * * Used to pass interface combination parameters * + * @radio: when set, check radio specific interface combinations. * @num_different_channels: the number of different channels we want * to use for verification * @radar_detect: a bitmap where each bit corresponds to a channel @@ -1608,6 +1609,7 @@ struct cfg80211_color_change_settings { * the verification */ struct iface_combination_params { + const struct wiphy_radio *radio; int num_different_channels; u8 radar_detect; int iftype_num[NUM_NL80211_IFTYPES]; @@ -4577,6 +4579,7 @@ struct mgmt_frame_regs { * * @set_hw_timestamp: Enable/disable HW timestamping of TM/FTM frames. * @set_ttlm: set the TID to link mapping. + * @get_radio_mask: get bitmask of radios in use */ struct cfg80211_ops { int (*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow); @@ -4938,6 +4941,8 @@ struct cfg80211_ops { struct cfg80211_set_hw_timestamp *hwts); int (*set_ttlm)(struct wiphy *wiphy, struct net_device *dev, struct cfg80211_ttlm_params *params); + int (*get_radio_mask)(struct wiphy *wiphy, struct net_device *dev, + u32 *mask); }; /* diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h index 43897a5269b6..43d72d09ef34 100644 --- a/net/wireless/rdev-ops.h +++ b/net/wireless/rdev-ops.h @@ -1542,4 +1542,21 @@ rdev_set_ttlm(struct cfg80211_registered_device *rdev, return ret; } + +static inline int +rdev_get_radio_mask(struct cfg80211_registered_device *rdev, + struct net_device *dev, u32 *mask) +{ + struct wiphy *wiphy = &rdev->wiphy; + int ret; + + if (!rdev->ops->get_radio_mask) + return -EOPNOTSUPP; + + trace_rdev_get_radio_mask(wiphy, dev); + ret = rdev->ops->get_radio_mask(wiphy, dev, mask); + trace_rdev_return_int(wiphy, ret); + + return ret; +} #endif /* __CFG80211_RDEV_OPS */ diff --git a/net/wireless/trace.h b/net/wireless/trace.h index 14cfa0aba93a..12081e81f9c5 100644 --- a/net/wireless/trace.h +++ b/net/wireless/trace.h @@ -4081,6 +4081,11 @@ TRACE_EVENT(cfg80211_links_removed, __entry->link_mask) ); +DEFINE_EVENT(wiphy_netdev_evt, rdev_get_radio_mask, + TP_PROTO(struct wiphy *wiphy, struct net_device *netdev), + TP_ARGS(wiphy, netdev) +); + #endif /* !__RDEV_OPS_TRACE || TRACE_HEADER_MULTI_READ */ #undef TRACE_INCLUDE_PATH diff --git a/net/wireless/util.c b/net/wireless/util.c index 2bde8a354631..cab40e4c3491 100644 --- a/net/wireless/util.c +++ b/net/wireless/util.c @@ -2305,20 +2305,35 @@ static int cfg80211_wdev_bi(struct wireless_dev *wdev) static void cfg80211_calculate_bi_data(struct wiphy *wiphy, u32 new_beacon_int, u32 *beacon_int_gcd, - bool *beacon_int_different) + bool *beacon_int_different, + const struct wiphy_radio *radio) { + struct cfg80211_registered_device *rdev; struct wireless_dev *wdev; + int radio_idx = -1; *beacon_int_gcd = 0; *beacon_int_different = false; + if (radio) + radio_idx = radio - wiphy->radio; + rdev = wiphy_to_rdev(wiphy); list_for_each_entry(wdev, &wiphy->wdev_list, list) { int wdev_bi; + u32 mask; /* this feature isn't supported with MLO */ if (wdev->valid_links) continue; + if (radio_idx >= 0) { + if (rdev_get_radio_mask(rdev, wdev->netdev, &mask)) + continue; + + if (!(mask & BIT(radio_idx))) + continue; + } + wdev_bi = cfg80211_wdev_bi(wdev); if (!wdev_bi) @@ -2366,9 +2381,11 @@ int cfg80211_iter_combinations(struct wiphy *wiphy, void *data), void *data) { + const struct wiphy_radio *radio = params->radio; + const struct ieee80211_iface_combination *c; const struct ieee80211_regdomain *regdom; enum nl80211_dfs_regions region = 0; - int i, j, iftype; + int i, j, n, iftype; int num_interfaces = 0; u32 used_iftypes = 0; u32 beacon_int_gcd; @@ -2385,7 +2402,8 @@ int cfg80211_iter_combinations(struct wiphy *wiphy, * interfaces (while being brought up) and channel/radar data. */ cfg80211_calculate_bi_data(wiphy, params->new_beacon_int, - &beacon_int_gcd, &beacon_int_different); + &beacon_int_gcd, &beacon_int_different, + radio); if (params->radar_detect) { rcu_read_lock(); @@ -2402,13 +2420,17 @@ int cfg80211_iter_combinations(struct wiphy *wiphy, used_iftypes |= BIT(iftype); } - for (i = 0; i < wiphy->n_iface_combinations; i++) { - const struct ieee80211_iface_combination *c; + if (radio) { + c = radio->iface_combinations; + n = radio->n_iface_combinations; + } else { + c = wiphy->iface_combinations; + n = wiphy->n_iface_combinations; + } + for (i = 0; i < n; i++, c++) { struct ieee80211_iface_limit *limits; u32 all_iftypes = 0; - c = &wiphy->iface_combinations[i]; - if (num_interfaces > c->max_interfaces) continue; if (params->num_different_channels > c->num_different_channels)
Add a field in struct iface_combination_params to check per-radio interface combinations instead of per-wiphy ones. Signed-off-by: Felix Fietkau <nbd@nbd.name> --- include/net/cfg80211.h | 5 +++++ net/wireless/rdev-ops.h | 17 +++++++++++++++++ net/wireless/trace.h | 5 +++++ net/wireless/util.c | 36 +++++++++++++++++++++++++++++------- 4 files changed, 56 insertions(+), 7 deletions(-)