| Message ID | 20220920100518.19705-2-quic_vthiagar@quicinc.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: cfg80211/mac80211: capability advertisement infra for multi-hw abstraction under one wiphy | expand |
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. > + * 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? > + 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 :) 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
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; + struct ieee80211_channel chans[]; +}; + /** * struct wiphy - wireless hardware description * @mtx: mutex for the data (structures) of this device @@ -5297,6 +5309,15 @@ struct wiphy_iftype_akm_suites { * NL80211_MAX_NR_AKM_SUITES in order to avoid compatibility issues with * legacy userspace and maximum allowed value is * CFG80211_MAX_NUM_AKM_SUITES. + * + * @hw_chans: list of the channels supported by every constituent underlying + * 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; + char priv[] __aligned(NETDEV_ALIGN); }; diff --git a/net/wireless/core.c b/net/wireless/core.c index 5b0c4d5b80cf..4163602a1b9a 100644 --- a/net/wireless/core.c +++ b/net/wireless/core.c @@ -653,6 +653,110 @@ static int wiphy_verify_combinations(struct wiphy *wiphy) return 0; } +static int cfg80211_check_hw_chans(const struct ieee80211_chans_per_hw *chans1, + const struct ieee80211_chans_per_hw *chans2) +{ + int i, j; + + if (!chans1 || !chans2) + return -EINVAL; + + if (!chans1->n_chans || !chans2->n_chans) + return -EINVAL; + + /* + * for now same channel is not allowed in more than one + * physical hardware. + */ + for (i = 0; i < chans1->n_chans; i++) + for (j = 0; j < chans2->n_chans; j++) + if (chans1->chans[i].center_freq == + chans2->chans[i].center_freq) + return -EINVAL; + return 0; +} + +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; + for (band = 0; band < NUM_NL80211_BANDS; band++) { + sband = wiphy->bands[band]; + if (!sband) + continue; + for (j = 0; j < sband->n_channels; j++) { + if (chans->chans[i].center_freq == + sband->channels[j].center_freq) { + found = true; + break; + } + } + + if (found) + break; + } + + if (!found) + return false; + } + + return true; +} + +static int cfg80211_validate_per_hw_chans(struct wiphy *wiphy) +{ + int i, j; + int ret; + + if (!wiphy->num_hw) + return 0; + + if (!wiphy->hw_chans) + return -EINVAL; + + /* + * advertisement of supported channels in wiphy->bands should be + * sufficient when physical hardware is one. + */ + if (wiphy->num_hw < 2) + return -EINVAL; + + 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; + + hw_chans1 = wiphy->hw_chans[i]; + hw_chans2 = wiphy->hw_chans[j]; + ret = cfg80211_check_hw_chans(hw_chans1, hw_chans2); + if (ret) + return ret; + } + } + + for (i = 0; i < wiphy->num_hw; i++) { + const struct ieee80211_chans_per_hw *hw_chans; + + hw_chans = wiphy->hw_chans[i]; + if (!cfg80211_hw_chans_in_supported_list(wiphy, hw_chans)) { + WARN_ON(1); + return -EINVAL; + } + } + + return 0; +} + int wiphy_register(struct wiphy *wiphy) { struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy); @@ -908,6 +1012,11 @@ int wiphy_register(struct wiphy *wiphy) return -EINVAL; } + if (cfg80211_validate_per_hw_chans(&rdev->wiphy)) { + WARN_ON(1); + return -EINVAL; + } + for (i = 0; i < rdev->wiphy.n_vendor_commands; i++) { /* * Validate we have a policy (can be explicitly set to
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(+)