Re: [PATCH 5/9] AP: Validate VHT config for 6 GHz Band Capabilities

On 2020-05-17 06:51, Jouni Malinen wrote:
> On Fri, Apr 24, 2020 at 03:45:38PM -0700, Rajkumar Manoharan wrote:
>> Following VHT Capabilities are mandatory for HE 6 GHz band 
>> capabilities
>> and corresponding configs should be validated for 6 GHz band.
>> 
>> * VHT_CAP_MAX_MPDU_LENGTH_MASK
>> * VHT_CAP_MAX_A_MPDU_LENGTH_EXPONENT_MAX
>> * VHT_CAP_RX_ANTENNA_PATTERN
>> * VHT_CAP_TX_ANTENNA_PATTERN
> 
> This sounds quite strange.. VHT is explicitly not used on the 6 GHz
> band. Why would we check VHT capabilities when operating on the 6 GHz
> band?
> 
Jouni,

Let me clarify. The driver doesn't advertise band specific capabilities 
explicitly.
IIRC spec mentions that the 6 GHz capabilities are referred from 
device's HT, VHT
capabilities as 6 GHz prohibits HT/VHT operations. Hence only mandatory 
HT/VHT
capabilities required to build 6 GHz cap are validated against user 
config.
In future, there will be dual band devices that support both 5 GHz & 6 
GHz. IMHO
it is simpler to check mandatory HT/VHT capability needed for 6 GHz 
against user config,
instead of defining 6 GHz capability check explicitly.

>> diff --git a/src/ap/hw_features.c b/src/ap/hw_features.c
>> @@ -671,8 +671,17 @@ int hostapd_check_ht_capab(struct hostapd_iface 
>> *iface)
> 
> And why would this check be in a function that is checking HT
> capabilities? HT is also explicitly not used on the 6 GHz band and
> checking VHT parameters in a function for checking HT parameters does
> not sound correct.
> 
Leveraging the HT/VHT capability checks for building 6 GHz cap as the 
device won't advertise
band specific HE capabilities. From user perspective, just op_class and 
channel number
are enough to differentiate operating band and rest of config unaltered.

>>  {
>>  	int ret;
>> 
>> -	if (is_6ghz_freq(iface->freq))
>> +	if (is_6ghz_freq(iface->freq)) {
>> +		/*
>> +		 * VHT capabilities needed for HE 6 GHz Band Capabilities
>> +		 * needs to be validated against config.
>> +		 */
>> +#ifdef CONFIG_IEEE80211AC
>> +		if (!ieee80211ac_supported_vht_capab(iface))
>> +			return -1;
>> +#endif
>>  		return 0;
>> +	}
>>  	if (!iface->conf->ieee80211n)
>>  		return 0;
> 
> This cannot really be logically correct and certainly not the correct
> place for doing this check.
> 
Same as above. thoughts?

-Rajkumar