Re: [PATCH v2 1/2] band: add band_estimate_he_rx_rate

[Denis Kenzior <denkenz@gmail.com>]

Hi James,

On 7/19/22 18:39, James Prestwood wrote:
> Similar to the HT/VHT APIs, this estimates the data rate based on the
> HE Capabilities element, in addition to our own capabilities. The
> logic is much the same as HT/VHT. The major difference being that HE
> uses several MCS tables depending on the channel width. Each width
> MCS set is checked (if supported) and the highest estimated rate out
> of all the MCS sets is used.
> ---
>   src/band.c | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>   src/band.h |   3 +-
>   2 files changed, 238 insertions(+), 9 deletions(-)
> 
> v2:
>   * Removed confusing loop to iterate widths. Replaced with 3 if blocks
>     that calculate the rate based on the MCS set
>   * Use OFDM enum instead of band_chandef_width. This did require a special
>     case for 80+80, but really simplifies everything else.
> 

<snip>

> +
> +/*
> + * Finds the best width supported according to capabilities. The 802.11ax spec
> + * has a separate MCS set for 80+80, but in the end 160 and 80+80 are treated
> + * the same in terms of the rate tables. This MCS set still needs to be checked
> + * though which is why the mcs80p80 flag is set in this case.
> + */
> +static enum ofdm_channel_width find_he_width_offset(enum band_freq freq,
> +				const struct band_he_capabilities *he_cap,
> +				const uint8_t *he_phy, bool *mcs80p80)
> +{
> +
> +	uint8_t own_width_set = bit_field(he_cap->he_phy_capa[0], 1, 7);
> +	uint8_t peer_width_set = bit_field(he_phy[0], 1, 7);

Why not just & these together and then test just the result?

> +	enum ofdm_channel_width max_width = OFDM_CHANNEL_WIDTH_20MHZ;
> +
> +	/*
> +	 * 802.11ax Table 9-322b
> +	 */
> +	switch (freq) {
> +	case BAND_FREQ_2_4_GHZ:
> +		/* B0 indicates support for 40MHz */
> +		if (test_bit(&peer_width_set, 0) && test_bit(&own_width_set, 0))

These test_bit lines end up over 80 chars...

> +			max_width = OFDM_CHANNEL_WIDTH_40MHZ;
> +
> +		break;
> +	case BAND_FREQ_5_GHZ:
> +	case BAND_FREQ_6_GHZ:
> +		/* B1 indicates support for 40MHz and 80MHz, choose 80 */
> +		if (test_bit(&peer_width_set, 1) && test_bit(&own_width_set, 1))
> +			max_width = OFDM_CHANNEL_WIDTH_80MHZ;
> +
> +		/* B2 indicates support for 160MHz */
> +		if (test_bit(&peer_width_set, 2) && test_bit(&own_width_set, 2))
> +			max_width = OFDM_CHANNEL_WIDTH_160MHZ;
> +
> +		/* B3 indicates support for 80+80MHz */
> +		if (test_bit(&peer_width_set, 3) &&
> +						test_bit(&own_width_set, 3)) {
> +			max_width = OFDM_CHANNEL_WIDTH_160MHZ;
> +			*mcs80p80 = true;

If we wanted to be extra paranoid, I guess we should also check that bit 3 is 
set IFF bit 2 is set and IFF bit 1 is set.  Maybe validating this and the 
presence of RX/TX MCS Maps for 160/80+80 fields should be done at entry to 
band_estimate_he_rx_rate();

> +		}
> +
> +		break;
> +	}
> +
> +	return max_width;
> +}
> +
> +/*
> + * HE data rate is calculated based on 802.11ax - Section 27.5
> + */
> +int band_estimate_he_rx_rate(const struct band *band, const uint8_t *hec,
> +				int32_t rssi, uint64_t *out_data_rate)
> +{
> +	enum ofdm_channel_width width;
> +	const struct band_he_capabilities *he_cap = NULL;
> +	const struct l_queue_entry *entry;
> +	const uint8_t *rx_map;
> +	const uint8_t *tx_map;
> +	bool mcs80p80 = false;
> +	uint64_t rate = 0;
> +	uint64_t new_rate = 0;
> +
> +	if (!hec || !band->he_capabilities)
> +		return -EBADMSG;
> +
> +	for (entry = l_queue_get_entries(band->he_capabilities);
> +						entry; entry = entry->next)
> +	{

That's not our style.  Also, maybe we should introduce QUEUE_FOREACH macro or 
something?  We can do this later I guess.

> +		const struct band_he_capabilities *cap = entry->data;
> +
> +		/*
> +		 * TODO: Station type is assumed here since it is the only
> +		 *       consumer of these data rate estimation APIs. If this
> +		 *       changes the iftype would need to be passed in.
> +		 */
> +		if (he_cap->iftypes & (1 << NETDEV_IFTYPE_STATION)) {
> +			he_cap = cap;
> +			break;
> +		}
> +	}
> +
> +	if (!he_cap)
> +		return -ENOTSUP;
> +
> +	/* The maximum width sets where to stop checking MCS sets */
> +	width = find_he_width_offset(band->freq, he_cap, hec + 8, &mcs80p80);
> +

What about just checking the capability bits in-line to this function? Something 
like:

if (test_bit(intersected_caps, 3)) {
	find_rate_he(..., OFDM_CHANNEL_WIDTH_160MHZ);
}

if (test_bit(intersected_caps, 2)) {
	find_rate_he(..., OFDM_CHANNEL_WIDTH_160MHZ);
}

if (test_bit(intersected_caps, 1))
	max_width = OFDM_CHANNEL_WIDTH_80;

if (test_bit(intersected_caps, 0))
	max_width = OFDM_CHANNEL_WIDTH_40;

> +	/*
> +	 * The HE-MCS maps are 19 bytes into the HE Capabilities IE, and
> +	 * alternate RX/TX every 2 bytes. Start the TX map 19 + 2 bytes
> +	 * into the MCS set. For each MCS set find the best data rate.
> +	 *
> +	 * Note: Since 'hec' is an extended IE index 0 is actually the length
> +	 *       since wiphy offsets data - 2.
> +	 */
> +	rx_map = he_cap->he_mcs_set;
> +	tx_map = hec + 21;
> +
> +	/* 80+80MHz MCS set */
> +	if (mcs80p80) {
> +		/* 21 + 8 bytes into the IE, plus 2 for the MCS set itself */
> +		if (hec[0] < 31)
> +			return -EBADMSG;
> +
> +		if (find_rate_he(rx_map + 8, tx_map + 8,
> +				OFDM_CHANNEL_WIDTH_160MHZ, rssi, &new_rate))
> +			rate = new_rate;
> +	}
> +
> +	/* 160MHz MCS set */
> +	if (width == OFDM_CHANNEL_WIDTH_160MHZ) {
> +		/* 21 + 4 bytes into the IE, plus 2 for the MCS set itself */
> +		if (hec[0] < 27)
> +			return -EBADMSG;
> +
> +		if (find_rate_he(rx_map + 4, tx_map + 4,
> +				OFDM_CHANNEL_WIDTH_160MHZ, rssi, &new_rate) &&
> +				new_rate > rate)
> +			rate = new_rate;
> +	}
> +
> +	/* <= 80MHz MCS set, length checked as this field always exists */
> +	if (find_rate_he(rx_map, tx_map, width, rssi, &new_rate) &&
> +					new_rate > rate)
> +		rate = new_rate;

And looks like this last part should be done in a loop for 80,40,20 (max_width) 
in the case of 5G/6G and 40/20 in the case of 2.4G?  Similar to how VHT is done?

I think in theory you could have a RSSI where the best rate would be at a lower 
channel width...?

Some unit tests would also be nice to verify this.

> +
> +	if (!rate)
> +		return -EBADMSG;
> +
> +	*out_data_rate = rate;
> +
> +	return 0;
> +}
> +

Regards,
-Denis