Re: [PATCH 2/3] cfg80211/mac80211: Use more generic bitrate mask for rate control

[Johannes Berg <johannes@sipsolutions.net>]

[-- Attachment #1: Type: text/plain, Size: 2954 bytes --]

On Tue, 2009-12-29 at 12:59 +0200, Jouni Malinen wrote:
> plain text document attachment (mac80211-rc-rate-mask.patch)
> Extend struct cfg80211_bitrate_mask to actually use a bitfield mask
> instead of just a single fixed or maximum rate index. This change
> itself does not modify the behavior (except for debugfs files), but it
> prepares cfg80211 and mac80211 for a new nl80211 command for setting
> which rates can be used in TX rate control.
> 
> Since frames are now going through the rate control algorithm
> unconditionally, the internal IEEE80211_TX_INTFL_RCALGO flag can now
> be removed. The RC implementations can use the rate_idx_mask value top

s/top/to/ :P

> @@ -257,7 +257,8 @@ void rate_control_get_rate(struct ieee80
>  	void *priv_sta = NULL;
>  	struct ieee80211_sta *ista = NULL;
>  	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(txrc->skb);
> -	int i;
> +	int i, j;
> +	u32 mask;
>  
>  	if (sta) {
>  		ista = &sta->sta;
> @@ -270,23 +271,26 @@ void rate_control_get_rate(struct ieee80
>  		info->control.rates[i].count = 1;
>  	}
>  
> -	if (sta && sdata->force_unicast_rateidx > -1) {
> -		info->control.rates[0].idx = sdata->force_unicast_rateidx;
> -	} else {
> -		ref->ops->get_rate(ref->priv, ista, priv_sta, txrc);
> -		info->flags |= IEEE80211_TX_INTFL_RCALGO;
> -	}
> +	ref->ops->get_rate(ref->priv, ista, priv_sta, txrc);
>  
>  	/*
> -	 * try to enforce the maximum rate the user wanted
> +	 * try to enforce the rateidx mask the user wanted
>  	 */
> -	if (sdata->max_ratectrl_rateidx > -1)
> +	mask = sdata->rc_rateidx_mask[info->band];
> +	if (mask != (1 << txrc->sband->n_bitrates) - 1) {
> +		if (sta)
> +			mask &= sta->sta.supp_rates[info->band];
>  		for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) {
>  			if (info->control.rates[i].flags & IEEE80211_TX_RC_MCS)
>  				continue;
> -			info->control.rates[i].idx =
> -				min_t(s8, info->control.rates[i].idx,
> -				      sdata->max_ratectrl_rateidx);
> +			for (j = info->control.rates[i].idx;
> +			     j < txrc->sband->n_bitrates; j++) {
> +				if (mask & (1 << j)) {
> +					info->control.rates[i].idx = j;
> +					break;
> +				}
> +			}

This could benefit from some comments :) I'm having trouble to see that
it's correct, but the min_t probably makes it correct?

> @@ -519,7 +519,11 @@ ieee80211_tx_h_rate_ctrl(struct ieee8021
>  	txrc.bss_conf = &tx->sdata->vif.bss_conf;
>  	txrc.skb = tx->skb;
>  	txrc.reported_rate.idx = -1;
> -	txrc.max_rate_idx = tx->sdata->max_ratectrl_rateidx;
> +	txrc.rate_idx_mask = tx->sdata->rc_rateidx_mask[tx->channel->band];
> +	if (txrc.rate_idx_mask == (1 << sband->n_bitrates) - 1)
> +		txrc.max_rate_idx = -1;
> +	else
> +		txrc.max_rate_idx = fls(txrc.rate_idx_mask) - 1;

I wonder if we should maintain the rate_idx_mask separately, so it's
outside the hotpath, or will we be removing all remaining uses of it
very soon?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]