Received-SPF: pass (google.com: domain of linux-wireless+bounces-5710-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <f5bac583-522f-1593-a1a3-b18e0290f40c@quicinc.com>
Date: Mon, 1 Apr 2024 15:26:39 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH 04/13] wifi: cfg80211/mac80211: extend iface comb
 advertisement for multi-hardware dev
Content-Language: en-US
To: Johannes Berg <johannes@sipsolutions.net>, <ath12k@lists.infradead.org>
CC: <linux-wireless@vger.kernel.org>,
        Vasanthakumar Thiagarajan
	<quic_vthiagar@quicinc.com>
References: <20240328072916.1164195-1-quic_periyasa@quicinc.com>
 <20240328072916.1164195-5-quic_periyasa@quicinc.com>
 <8e1d403251a8c8e5c9fe34d9dea51b8ea60b6b6b.camel@sipsolutions.net>
From: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
In-Reply-To: <8e1d403251a8c8e5c9fe34d9dea51b8ea60b6b6b.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit


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
--
கார்த்திகேயன் பெரியசாமி