| Message ID | 20220920100518.19705-1-quic_vthiagar@quicinc.com |
|---|---|
| Headers | show |
| Series | wifi: cfg80211/mac80211: capability advertisement infra for multi-hw abstraction under one wiphy | expand |
Hi, Sorry for the delay! > - Should we make the capability advertisement changes to mac80211_hwsim? I don't think we can, unless we teach hwsim and even mac80211 about this? IMHO doesn't make much sense. > - Should we enable some of concurrent operations like allow scan on each physical > hardware concurrently? Isn't that up to the driver? If userspace requests scanning all bands in a single scan request you can parallelize it using multiple HW resources? johannes
On 10/21/2022 5:27 PM, Johannes Berg wrote: > Hi, > > Sorry for the delay! no worries. > >> - Should we make the capability advertisement changes to mac80211_hwsim? > > I don't think we can, unless we teach hwsim and even mac80211 about > this? IMHO doesn't make much sense. > Sure. >> - Should we enable some of concurrent operations like allow scan on each physical >> hardware concurrently? > > Isn't that up to the driver? If userspace requests scanning all bands in > a single scan request you can parallelize it using multiple HW > resources? Correct, driver should be able to handle the scanning on multiple HW in parallel. We saw resource busy error code when tried to bring up multiple AP interfaces on different band at the same time due to an existing scan request still in progress. I agree, it makes sense to give a single scan request with all the bands and let the ACS in user space share the results for AP bring up on any band. Vasanth
On Tue, 2022-09-20 at 15:35 +0530, Vasanthakumar Thiagarajan wrote: > > +struct ieee80211_iface_per_hw { > + u8 hw_chans_idx; > + const struct ieee80211_iface_limit *limits; > + u32 num_different_channels; > + u16 max_interfaces; > + u8 n_limits; > +}; nit: moving hw_chans_idx last would get rid of all the padding :) > + * Drivers advertising per-hardware interface combination should also > + * advertise a sub-set of capabilities using existing interface mainly for > + * maintaining compatibility with the user space which is not aware of the > + * new per-hardware advertisement. > + * > + * Sub-set interface combination advertised in the existing infrastructure: > + * Allow #STA <= 1, #AP <= 1, channels = 1, total 2 > + * > + * .. code-block:: c > + * > + * struct ieee80211_iface_limit limits4[] = { > + * { .max = 1, .types = BIT(NL80211_IFTYPE_STATION), }, > + * { .max = 1, .types = BIT(NL80211_IFTYPE_AP), }, > + * }; > + * struct ieee80211_iface_limit limits5_2ghz[] = { > + * { .max = 1, .types = BIT(NL80211_IFTYPE_STATION), }, > + * { .max = 1, .types = BIT(NL80211_IFTYPE_AP), }, > + * }; > + * struct ieee80211_iface_limit limits5_5ghz[] = { > + * { .max = 1, .types = BIT(NL80211_IFTYPE_STATION), }, > + * { .max = 2, .types = BIT(NL80211_IFTYPE_AP), }, > + * }; Where does the limits4/limits5 naming come from? The number of interfaces I guess? To me that wasn't so clear, maybe it makes more sense to name them limits_overall, limits_2ghz, and limits_5ghz respectively? (yeah, obviously I know this is just an example) > +/** > + * cfg80211_hw_chans_includes_dfs - check if per-hardware channel includes DFS > + * @chans: hardware channel list prefer space instead of tab I think? > + * Please note the channel is checked against the entire range of DFS > + * freq in 5 GHz irrespective of regulatory configurations. Not sure what you mean by this? Is that different somehow from what we did before? > +++ b/net/mac80211/main.c > @@ -933,6 +933,45 @@ static int ieee80211_init_cipher_suites(struct ieee80211_local *local) > return 0; > } > > +static int > +ieee80211_check_per_hw_iface_comb(struct ieee80211_local *local, > + const struct ieee80211_iface_combination *c) > +{ Why is this in mac80211? Wouldn't such a check apply equally to any non- mac80211 driver? > + int h, l; > + u32 hw_idx_bm = 0; > + > + if (!local->use_chanctx) > + return -EINVAL; Maybe mac80211 has this extra check, and can keep it, but > + > + for (h = 0; h < c->n_hw_list; h++) { > + const struct ieee80211_iface_per_hw *hl; > + const struct ieee80211_chans_per_hw *chans; > + > + hl = &c->iface_hw_list[h]; > + > + if (hl->hw_chans_idx >= local->hw.wiphy->num_hw) > + return -EINVAL; > + > + chans = local->hw.wiphy->hw_chans[hl->hw_chans_idx]; > + if (c->radar_detect_widths && > + cfg80211_hw_chans_includes_dfs(chans) && > + hl->num_different_channels > 1) > + return -EINVAL; > + > + for (l = 0; l < hl->n_limits; l++) > + if ((hl->limits[l].types & BIT(NL80211_IFTYPE_ADHOC)) && > + hl->limits[l].max > 1) > + return -EINVAL; > + > + if (hw_idx_bm & BIT(h)) > + return -EINVAL; > + > + hw_idx_bm |= BIT(h); this pretty much seems applicable to do in cfg80211? > @@ -1035,6 +1074,21 @@ int ieee80211_register_hw(struct ieee80211_hw *hw) > } > } > > + for (i = 0; i < local->hw.wiphy->n_iface_combinations; i++) { > + const struct ieee80211_iface_combination *comb; > + > + comb = &local->hw.wiphy->iface_combinations[i]; > + > + if (comb->n_hw_list && !local->hw.wiphy->num_hw) > + return -EINVAL; > + > + if (!comb->n_hw_list) > + continue; > + > + if (ieee80211_check_per_hw_iface_comb(local, comb)) > + return -EINVAL; > + } and this then, of course. > +++ b/net/wireless/core.c > @@ -563,10 +563,126 @@ struct wiphy *wiphy_new_nm(const struct cfg80211_ops *ops, int sizeof_priv, > } > EXPORT_SYMBOL(wiphy_new_nm); > > +static int > +wiphy_verify_comb_limit(struct wiphy *wiphy, > + const struct ieee80211_iface_limit *limits, > + u8 n_limits, u32 bcn_int_min_gcd, u32 *iface_cnt, > + u16 *all_iftypes) oh wait, you did it twice? is there anything that mac80211 adds extra? > static int wiphy_verify_combinations(struct wiphy *wiphy) > { > const struct ieee80211_iface_combination *c; > - int i, j; > + int i; > + int ret; > > for (i = 0; i < wiphy->n_iface_combinations; i++) { > u32 cnt = 0; > @@ -593,54 +709,11 @@ static int wiphy_verify_combinations(struct wiphy *wiphy) > if (WARN_ON(!c->n_limits)) > return -EINVAL; > > - for (j = 0; j < c->n_limits; j++) { > - u16 types = c->limits[j].types; > [...] > + ret = wiphy_verify_comb_limit(wiphy, c->limits, c->n_limits, > + c->beacon_int_min_gcd, > + &cnt, &all_iftypes); Might be nice to break out this refactoring to a separate patch (and feel free to send it right away as PATCH, it's kind of worthwhile anyway), I think? Unless I missed something that changed here, but then it'd be even more worthwhile so I see it ;-) > +bool > +cfg80211_hw_chans_includes_dfs(const struct ieee80211_chans_per_hw *chans) > +{ [...] > +EXPORT_SYMBOL(cfg80211_hw_chans_includes_dfs); Since it's exported - who would use it and for what? johannes
On 10/21/2022 5:34 PM, Johannes Berg wrote: > On Tue, 2022-09-20 at 15:35 +0530, Vasanthakumar Thiagarajan wrote: >> The prerequisite for MLO support in cfg80211/mac80211 is that all >> the links participating in MLO must be from the same wiphy/ieee80211_hw. >> To meet this expectation, some drivers may need to group multiple discrete >> hardware each acting as a link in MLO under one wiphy. Though most of the >> hardware abstractions must be handled within the driver itself, it may be >> required to have some sort of mapping while describing interface >> combination capabilities for each of the underlying hardware. This commit >> adds an advertisement provision for drivers supporting such MLO mode of >> operation. >> >> Capability advertisement such as the number of supported channels for each >> physical hardware has been identified to support some of the common use >> cases. >> >> Signed-off-by: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com> >> --- >> include/net/cfg80211.h | 24 +++++++++ >> net/wireless/core.c | 109 +++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 133 insertions(+) >> >> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h >> index e09ff87146c1..4662231ad068 100644 >> --- a/include/net/cfg80211.h >> +++ b/include/net/cfg80211.h >> @@ -5088,6 +5088,18 @@ struct wiphy_iftype_akm_suites { >> int n_akm_suites; >> }; >> >> +/** >> + * struct ieee80211_supported_chans_per_hw - supported channels as per the >> + * underlying physical hardware configuration >> + * >> + * @n_chans: number of channels in @chans >> + * @chans: list of channels supported by the constituent hardware >> + */ >> +struct ieee80211_chans_per_hw { >> + int n_chans; > > nit: unsigned? > >> + * @hw_chans: list of the channels supported by every constituent underlying > > "every" here refers to the fact that you have all the channels, right? I > mean, hw_chans is per hardware, basically. Correct, it refers all the channels supported per hardware registered something like below hw_chans[0] = { // 2 GHz radio <num_2ghz_chans>, { <ieee80211_channel_2ghz_1>, <ieee80211_channel_2ghz_2>, ... } } hw_chans[1] = { { // 5 GHz radio <num_5ghz_chans>, { <ieee80211_channel_5ghz_1>, <ieee80211_channel_5ghz_2>, ... } } ... > >> + * hardware. Drivers abstracting multiple discrete hardware (radio) under >> + * one wiphy can advertise the list of channels supported by each physical >> + * hardware in this list. Underlying hardware specific channel list can be >> + * used while describing interface combination for each of them. >> + * @num_hw: number of underlying hardware for which the channels list are >> + * advertised in @hw_chans. >> + * >> */ >> struct wiphy { >> struct mutex mtx; >> @@ -5445,6 +5466,9 @@ struct wiphy { >> u8 ema_max_profile_periodicity; >> u16 max_num_akm_suites; >> >> + struct ieee80211_chans_per_hw **hw_chans; >> + int num_hw; > > also here, maybe unsigned. > >> +static bool >> +cfg80211_hw_chans_in_supported_list(struct wiphy *wiphy, >> + const struct ieee80211_chans_per_hw *chans) >> +{ >> + enum nl80211_band band; >> + struct ieee80211_supported_band *sband; >> + bool found; >> + int i, j; >> + >> + for (i = 0; i < chans->n_chans; i++) { >> + found = false; > > nit: you can move the variable declaration here > > bool found = false; > \n > > since you don't need it outside the loop. > >> + for (i = 0; i < wiphy->num_hw; i++) { >> + for (j = 0; j < wiphy->num_hw; j++) { >> + const struct ieee80211_chans_per_hw *hw_chans1; >> + const struct ieee80211_chans_per_hw *hw_chans2; >> + >> + if (i == j) >> + continue; > > It's symmetric, if I read the code above right, so you can do > > for (j = i; j < ...; j++) > > in the second loop and avoid this? Sure > > >> + hw_chans = wiphy->hw_chans[i]; >> + if (!cfg80211_hw_chans_in_supported_list(wiphy, hw_chans)) { >> + WARN_ON(1); > > I can kind of see the point in not using WARN_ON(!cfg80211_hw_chans...) > since it gets really long, but maybe just remove the warning? > >> + if (cfg80211_validate_per_hw_chans(&rdev->wiphy)) { >> + WARN_ON(1); >> + return -EINVAL; >> > > Anyway you'll have one here, and it's not directly visible which > condition failed anyway. > > And you could use WARN_ON(validate(...)) here :) Sure, thanks! Vasanth
On 10/21/2022 5:43 PM, Johannes Berg wrote: > On Tue, 2022-09-20 at 15:35 +0530, Vasanthakumar Thiagarajan wrote: >> >> +++ b/include/uapi/linux/nl80211.h >> @@ -2749,6 +2749,12 @@ enum nl80211_commands { >> * When used with %NL80211_CMD_FRAME_TX_STATUS, indicates the ack RX >> * timestamp. When used with %NL80211_CMD_FRAME RX notification, indicates >> * the incoming frame RX timestamp. >> + * >> + * @NL80211_ATTR_MULTI_HW_MACS: nested attribute to send the hardware mac > > Not sure I'd call this multiple MACs? It's multiple devices in some > sense, but from a spec POV at least, I'd think our NIC also has multiple > MACs when it doesn't use this infrastructure. Might get a bit confusing? > > Maybe just stick to "multi_hw" or so? Yeah, I was not very comfortable calling it multiple MACs either. Sure, let me just stick to multi_hw. > >> +/** >> + * nl80211_multi_hw_mac_attrs - multi-hw mac attributes >> + * >> + * @NL80211_MULTI_HW_MAC_ATTR_INVALID: invalid >> + * @NL80211_MULTI_HW_MAC_ATTR_IDX: (u8) array index in wiphy @hw_chans to refer an >> + * underlying hw mac for which the supported channel list is advertised. > > I'd prefer this to be primarily written from a userspace POV, so the > whole "@hw_chans" etc isn't really right. Maybe say something like > > "(u8) multi-HW index used to refer to an underlying HW ...; internally > the index of the wiphy's @hw_chans array." > > or so? Sure, thanks. > >> + * @NL80211_MULTI_HW_MAC_ATTR_FREQS: array of supported center frequencies > > FWIW, Jakub has started advertising for using the same attribute > multiple times to have arrays, so you'd just have > > {NL80211_MULTI_HW_ATTR_FREQ: 2412}, > {NL80211_MULTI_HW_ATTR_FREQ: 2417}, > {NL80211_MULTI_HW_ATTR_FREQ: 2422}, > > etc. in the message. Not sure we want to try that here, but it'd also > simplify splitting messages for dumps. > we have to do that for every channel? let me check. > >> +static int nl80211_put_multi_hw_support(struct wiphy *wiphy, >> + struct sk_buff *msg) >> +{ >> + struct nlattr *hw_macs, *hw_mac; >> + struct nlattr *freqs; >> + int i, c; >> + >> + if (!wiphy->num_hw) >> + return 0; >> + >> + hw_macs = nla_nest_start(msg, NL80211_ATTR_MULTI_HW_MACS); >> + if (!hw_macs) >> + return -ENOBUFS; >> + >> + for (i = 0; i < wiphy->num_hw; i++) { >> + hw_mac = nla_nest_start(msg, i + 1); >> + if (!hw_mac) >> + return -ENOBUFS; >> + >> + if (nla_put_u8(msg, NL80211_MULTI_HW_MAC_ATTR_IDX, i)) >> + return -ENOBUFS; >> + >> + freqs = nla_nest_start(msg, >> + NL80211_MULTI_HW_MAC_ATTR_FREQS); >> + if (!freqs) >> + return -ENOBUFS; >> + >> + for (c = 0; c < wiphy->hw_chans[i]->n_chans; c++) >> + if (nla_put_u32(msg, c + 1, >> + wiphy->hw_chans[i]->chans[c].center_freq)) >> + return -ENOBUFS; > > Ah you used a nested array even. > > So the argument for using a real array would've been that it's smaller, > but I guess with nested that argument goes way. > > Would you mind trying Jakub's preferred approach here and see how that > works out? > > For the generator basically you'd just have > > hw_mac = nla_nest_start(); > nla_put_u8(IDX, i) > for (c = 0; c < ...; c++) > nla_put_u32(MULTI_HW_ATTR_FREQ, ...->chans[c].center_freq); > I'll try this, thanks! Vasanth
There may be hardware design (ath12k as of now) supporting MLO across multiple discrete hardware each acting as a link in the MLO operation. Since the prerequisite for MLO support in cfg80211/mac80211 is that all the links participating in MLO must be from the same wiphy/ieee80211_hw, driver needs to handle the discreate hardware abstraction under single wiphy. Though most of the hw specific abstractions can be handled with in the driver, there are some capabilities like interface combination which can be specific to each constituent physical hardware. This patch set tries to add an infrastructure to advertise underlying hw specific capabilities like channel and interface combinations. Some of the todos - Make runtime iface combination validation logic be aware of this extension - More than one concurrent monitor mode support each operating on different channels under one ieee80211_hw - Mechanism for each underlying radio specific configurations like txpower, channel, etc. - Should we make the capability advertisement changes to mac80211_hwsim? - Should we enable some of concurrent operations like allow scan on each physical hardware concurrently? Vasanthakumar Thiagarajan (4): wifi: cfg80211: Add provision to advertise multiple radio in one wiphy wifi: nl80211: send underlying multi-hardware channel capabilities to user space wifi: cfg80211/mac80211: extend iface comb advertisement for multi-hardware dev wifi: nl80211: send iface combination to user space in multi-hardware wiphy include/net/cfg80211.h | 130 +++++++++++++++++ include/uapi/linux/nl80211.h | 78 +++++++++- net/mac80211/main.c | 54 +++++++ net/wireless/core.c | 275 +++++++++++++++++++++++++++++------ net/wireless/nl80211.c | 118 +++++++++++++++ net/wireless/util.c | 18 +++ 6 files changed, 629 insertions(+), 44 deletions(-)