On Sat, 2024-04-27 at 08:45 +0530, Karthikeyan Periyasamy wrote: > Currently, the interface combination input parameter num_different_channels > and iftype_num are directly filled in by the caller under the assumption > that all channels and interfaces belong to a single hardware device. This > assumption is incorrect for multi-device interface combinations because > each device supports a different set of channels and interfaces. As > discussed in [1], need to refactor the input parameters to encode enough > data to handle both single and multiple device interface combinations. > This can be achieved by encoding the frequency and interface type under > the interface entity itself. With this new input parameter structure, the > cfg80211 can classify and construct the device parameters, then verify them > against the device specific interface combinations. ^^ This should probably mention _something_ about links too :) > [1]: https://lore.kernel.org/linux-wireless/ca70eeb3cdee1e8c3caee69db595bc8160eb4115.camel@xxxxxxxxxxxxxxxx/ > > Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1 > > Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@xxxxxxxxxxx> > --- > drivers/net/wireless/ath/wil6210/cfg80211.c | 44 +++++-- > .../broadcom/brcm80211/brcmfmac/cfg80211.c | 60 +++++++-- > .../net/wireless/quantenna/qtnfmac/cfg80211.c | 32 +++-- > include/net/cfg80211.h | 37 +++++- > net/mac80211/util.c | 124 +++++++++++++++--- > net/wireless/util.c | 56 ++++++-- > 6 files changed, 276 insertions(+), 77 deletions(-) > > diff --git a/drivers/net/wireless/ath/wil6210/cfg80211.c b/drivers/net/wireless/ath/wil6210/cfg80211.c > index 8993028709ec..3f9f5f56bd19 100644 > --- a/drivers/net/wireless/ath/wil6210/cfg80211.c > +++ b/drivers/net/wireless/ath/wil6210/cfg80211.c > @@ -625,17 +625,25 @@ static int wil_cfg80211_validate_add_iface(struct wil6210_priv *wil, > { > int i; > struct wireless_dev *wdev; > - struct iface_combination_params params = { > - .num_different_channels = 1, > - }; > + struct iface_combination_params params = { 0 }; nit: just use "= {}". > + ifaces = kcalloc(total_iface, sizeof(*ifaces), GFP_KERNEL); > + if (!ifaces) > + return -ENOMEM; > + > + list_for_each_entry(pos, &cfg->vif_list, list) { > + if (params.num_iface >= total_iface) > + continue; ?? Seems like that should be a WARN_ON or something? > + struct iface_combination_interface *ifaces = NULL; > + u16 total_iface = 0; > + int ret; > > list_for_each_entry(pos, &cfg->vif_list, list) > - params.iftype_num[pos->wdev.iftype]++; > + total_iface++; > > - params.iftype_num[new_type]++; > - return cfg80211_check_combinations(cfg->wiphy, ¶ms); > + ifaces = kcalloc(total_iface, sizeof(*ifaces), GFP_KERNEL); No point in "= NULL" if you overwrite it immediately. > +/** > + * struct iface_combination_iface_link - Interface combination link parameter > + * > + * Used to pass link specific interface combination parameters > + * > + * @freq: center frequency used for verification against the different channels > + */ > +struct iface_combination_iface_link { > + u32 freq; > +}; > + > +/** > + * struct iface_combination_interface - Interface parameter for iface combination > + * > + * Used to pass interface specific parameter for iface combination > + * > + * @iftype: interface type as specified in &enum nl80211_iftype. > + * @links: array with the number of link parameter used for verification > + * @num_link: the length of the @links parameter used in this interface > + */ > +struct iface_combination_interface { > + enum nl80211_iftype iftype; > + struct iface_combination_iface_link links[IEEE80211_MLD_MAX_NUM_LINKS]; > + u8 num_link; Might be simpler (for the producers at least, but not really much more difficult for the consumer) to just remove num_link, use the link ID as the index, and declare freq==0 means unused? > - * @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 > * width where radar detection is needed, as in the definition of > * &struct ieee80211_iface_combination.@radar_detect_widths > - * @iftype_num: array with the number of interfaces of each interface > - * type. The index is the interface type as specified in &enum > - * nl80211_iftype. > * @new_beacon_int: set this to the beacon interval of a new interface > * that's not operating yet, if such is to be checked as part of > * the verification > + * @ifaces: array with the number of interface parameter use for verification > + * @num_iface: the length of the @ifaces interface parameter > */ > struct iface_combination_params { > - int num_different_channels; > u8 radar_detect; > - int iftype_num[NUM_NL80211_IFTYPES]; > u32 new_beacon_int; > + const struct iface_combination_interface *ifaces; > + u16 num_iface; The "new_beacon_int" also needs to be for a specific link, witha a specific freq, so you can check for *that* part of the wiphy? Similarly for radar_detect? > + if (iftype != NL80211_IFTYPE_UNSPECIFIED || chandef) { > + struct iface_combination_interface *iface; > + > + iface = &ifaces[params.num_iface]; > + iface->iftype = iftype; > + > + if (chandef && cfg80211_chandef_valid(chandef)) { > + iface->links[0].freq = chandef->chan->center_freq; > + iface->num_link++; > } Not sure I understand this. > @@ -4009,14 +4029,37 @@ int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata, > wdev_iter->iftype, 0, 1)) > continue; > > - params.iftype_num[wdev_iter->iftype]++; > + iface = &ifaces[params.num_iface]; > + iface->iftype = wdev_iter->iftype; > + > + rcu_read_lock(); > + for_each_vif_active_link(&sdata_iter->vif, link_conf, link_id) { > + struct ieee80211_chanctx_conf *chanctx_conf; > + struct iface_combination_iface_link *link; > + > + chanctx_conf = rcu_dereference(link_conf->chanctx_conf); > + if (chanctx_conf && > + cfg80211_chandef_valid(&chanctx_conf->def)) { Why the valid check, btw? How could that possibly *not* be valid? > + link = &iface->links[iface->num_link]; > + link->freq = chanctx_conf->def.chan->center_freq; > + iface->num_link++; > + } > + } > + rcu_read_unlock(); when you also have this? But maybe separating out actual logic changes in mac80211 to a separate patch would be good. > list_for_each_entry_rcu(sdata, &local->interfaces, list) > - params.iftype_num[sdata->wdev.iftype]++; > + total_iface++; > + > + if (!total_iface) > + goto skip; > + > + ifaces = kcalloc(total_iface, sizeof(*ifaces), GFP_KERNEL); > + if (!ifaces) > + return -ENOMEM; > + > + list_for_each_entry_rcu(sdata, &local->interfaces, list) { > + struct iface_combination_interface *iface; > + > + if (params.num_iface >= total_iface) > + continue; > + > + iface = &ifaces[params.num_iface]; > + iface->iftype = sdata->wdev.iftype; > + > + rcu_read_lock(); > + for_each_vif_active_link(&sdata->vif, link_conf, link_id) { > + struct ieee80211_chanctx_conf *chanctx_conf; > + struct iface_combination_iface_link *link; > + > + chanctx_conf = rcu_dereference(link_conf->chanctx_conf); > + if (chanctx_conf && > + cfg80211_chandef_valid(&chanctx_conf->def)) { > + link = &iface->links[iface->num_link]; > + link->freq = chanctx_conf->def.chan->center_freq; > + iface->num_link++; > + } > + } > + rcu_read_unlock(); > + > + params.num_iface++; > + } Please don't add the same code twice. johannes
