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
