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

Karthikeyan Periyasamy
2024-04-01T09:56:57Z

On 3/28/2024 6:52 PM, Johannes Berg wrote:
> 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.

Sure, will split into the cfg/mac patches.

>
> 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)

sure, will move it local to that block

> bm?

bitmap used for hw_id sanity checks.

>
>> + 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 ...

sure.

>> +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.

radar_detect_widths itself not sufficient for the radar specific sanity
check in the multiple hardware iface combination approach. So include
this helper for the radar sanity check.

This helper used to check the given channel list present in the 5GHz
band or not.

>
>> + if (WARN_ON(hl->max_interfaces < 2 &&
>> + (!comb->radar_detect_widths ||
>> + !(cfg80211_hw_chans_includes_dfs(chans)))))
> No need for extra parentheses.

sure

>> @@ -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.

sure, will address all the comments in the next version of the patch

--
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி