Re: [PATCH v3 1/2] nl80211: Add support for EDMG channels

[Alexei Lazar <ailizaro@codeaurora.org>]

On 2019-06-28 16:21, Johannes Berg wrote:
> On Tue, 2019-06-25 at 13:29 +0300, Alexei Avshalom Lazar wrote:

Hi Johannes,

Again, Thank you for the review, comments inline.

Thanks,
Alex.

>> 
>>  /**
>> + * struct ieee80211_sta_edmg_cap - EDMG capabilities
>> + *
>> + * This structure describes most essential parameters needed
>> + * to describe 802.11ay EDMG capabilities
>> + *
>> + * @channels: bitmap that indicates the 2.16 GHz channel(s)
>> + *	that are allowed to be used for transmissions.
>> + *	Bit 0 indicates channel 1, bit 1 indicates channel 2, etc.
>> + *	Set to 0 indicate EDMG not supported.
>> + * @bw_config: Channel BW Configuration subfield encodes
>> + *	the allowed channel bandwidth configurations
>> + */
>> +struct ieee80211_sta_edmg_cap {
>> +	u8 channels;
>> +	u8 bw_config;
>> +};
> 
> [...]
> 
>> + * @edmg: define the EDMG channels configuration.
>> + *	If edmg is set, chan will define the primary channel and all other
>> + *	parameters are ignored.
>> 
>>  struct cfg80211_chan_def {
> 
> Thinking out loud, maybe this should say "If edmg is requested (i.e. 
> the
> .channels member is non-zero) [...]" or so?

Done.

> 
>> @@ -1192,6 +1218,7 @@ enum rate_info_bw {
>>   * @he_dcm: HE DCM value
>>   * @he_ru_alloc: HE RU allocation (from &enum nl80211_he_ru_alloc,
>>   *	only valid if bw is %RATE_INFO_BW_HE_RU)
>> + * @n_bonded_ch: In case of EDMG the number of bonded channels (1-4)
> 
> So, just for the stupid me because I didn't really read the spec.
> 
> You have N channels (can only be 8 since you use a u8, looking at the
> code further it looks like there are only 7) and edmg_cap::channels
> indicates which are supported/requested, and then edmg_cap::bw_config
> indicates how the channels are used?
> 
> I'm not sure I understand this part, because if you, say, request
> channels 1 and 2 (channels=0x3) then what configurations could be
> possible below that?
> 
> Oh, something about the primary channel maybe?
> 
> 
> I guess I would've expected something like a list of bitmaps that the
> device supports, and then at runtime you select only one bitmap.
> 
> If I have channels 1 and 2 enabled, how do the configurations differ?
> 
> Clearly they don't differ enough to make capturing them in the rate
> worthwhile, here n_bonded_ch would presumably only be 2, and that's
> enough to tell the rate. What then does the configuration mean?

Channels is a bitmap of 2.16GHz (normal) channels 1..6
bw_config defines the allowed combinations (bonding) of these "normal"
channels.
Let's say driver reports support for channels number 1,2,3 
(channels=0x7).
Example #1: driver reports bw_config=5
bw_config=5 allows combinations of 1 or 2 channels.
It means driver can operate in one of these combinations:
Channel 1 only
Channel 2 only
Channel 3 only
Channels 1+2 (cb2)
Channels 2+3 (cb2)

Example #2: driver reports bw_config=10
bw_config=10 allows combinations of 1,2 or 3 channels.
It means driver can operate in one of these combinations:
Channel 1 only
Channel 2 only
Channel 3 only
Channels 1+2 (cb2)
Channels 2+3 (cb2)
Channels 1+3 (cb2) - note 1 & 3 are non-contiguous channels, This 
combination
is not allow in bw_config=5
Channels 1+2+3 (cb3)

The primary channel BTW must be one of the operational channels so if
driver choose channels 1+3 then primary channel cannot be 2.

> 
> 
> It seems to me that you should, however, expose n_bonded_ch to
> userspace? We do expose all the details about the bitrate normally, see
> nl80211_put_sta_rate(). We do calculate the bitrate to expose that too,
> but do expose all the details too.
> 
> Hmm. Looking at that, it looks like DMG doesn't actually do that. So
> perhaps not, though it seems to me that it ought to be possible?

We will look into that and address in separate patch.

> 
>> @@ -2436,6 +2467,7 @@ struct cfg80211_connect_params {
>>  	const u8 *fils_erp_rrk;
>>  	size_t fils_erp_rrk_len;
>>  	bool want_1x;
>> +	struct ieee80211_sta_edmg_cap edmg;
> 
> Maybe we really should rename this struct type? It's not a "capability"
> here.

Done.

> 
>> +static bool cfg80211_edmg_chandef_valid(const struct 
>> cfg80211_chan_def *chandef)
>> +{
>> +	int max_continuous = 0;
>> +	int num_of_enabled = 0;
>> +	int contiguous = 0;
> 
> max_continuous vs. contiguous is even more confusing now :-)
> 
> 
>> +		max_continuous = max(contiguous, max_continuous);
> 
> See? :)

Done.

> 
>> +	/* basic verification of edmg configuration according to
>> +	 * IEEE802.11 section 9.4.2.251
> 
> All the IEEE 802.11 references (more than just this one) ... please
> qualify them with a version. I'm thinking these are not in -2016, so
> probably 802.11ay (perhaps even draft?)

Done.

> 
>> +	 */
>> +	/* check bw_config against contiguous edmg channels */
>> +	switch (chandef->edmg.bw_config) {
>> +	case 4:
>> +	case 8:
>> +	case 12:
>> +		if (max_continuous < 1)
>> +			return false;
>> +		break;
> 
> I guess I really should try to find a copy of the appropriate spec ...
> 
> johannes

-- 
Alexei Lazar
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum a
Linux Foundation Collaborative Project