Re: [PATCH 04/13] wifi: cfg80211/mac80211: extend iface comb advertisement for multi-hardware dev

Johannes Berg
2024-03-28T13:22:50Z

Not sure why this is a combined cfg/mac patch, there doesn't seem to be
all that much need for that? I'd probably rather see this and patch 6
squashed, but cfg/mac kept separate.

On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:

>
> +static int
> +ieee80211_check_per_hw_iface_comb(struct ieee80211_local *local,
> + const struct ieee80211_iface_combination *c)
> +{
> + int hw_idx, lmt_idx;
> + u32 hw_idx_bm = 0;

lmt_idx? (also that's only needed in the loop)
bm?

> + if (!local->hw.wiphy->num_hw)
> + return -EINVAL;
> +
> + if (local->emulate_chanctx)
> + return -EINVAL;
> +
> + for (hw_idx = 0; hw_idx < c->n_hw_list; hw_idx++) {
> + const struct ieee80211_iface_per_hw *hl;

hl?

Could have a bit more evocative names :)

I'd rather see 'i' for some iteration thingie than "lmt_idx" which
means nothing on first reading either, but you start thinking it should
mean something ...

> +static bool
> +cfg80211_hw_chans_includes_dfs(const struct ieee80211_chans_per_hw *chans)
> +{
> + int i;
> +
> + for (i = 0; i < chans->n_chans; i++) {
> + if (chans->chans[i].band == NL80211_BAND_5GHZ &&
> + ((chans->chans[i].center_freq >= 5250 &&
> + chans->chans[i].center_freq <= 5340) ||
> + (chans->chans[i].center_freq >= 5480 &&
> + chans->chans[i].center_freq <= 5720)))

???

That's not how this works upstream.

> + if (WARN_ON(hl->max_interfaces < 2 &&
> + (!comb->radar_detect_widths ||
> + !(cfg80211_hw_chans_includes_dfs(chans)))))

No need for extra parentheses.

> @@ -701,6 +786,13 @@ static int wiphy_verify_combinations(struct wiphy *wiphy)
> /* You can't even choose that many! */
> if (WARN_ON(cnt < c->max_interfaces))
> return -EINVAL;
> +
> + /* Do similar validations on the freq range specific interface
> + * combinations when advertised.
> + */
> + if (WARN_ON(c->n_hw_list &&
> + wiphy_verify_comb_per_hw(wiphy, c)))

Don't need the n_hw_list check here, the function just does nothing if
it's 0 anyway.

johannes