Re: [PATCH v4] cfg80211: Add support to configure a beacon data rate

[Johannes Berg <johannes@sipsolutions.net>]

Hi,

Thanks for the new version. I was going to apply it but while changing
something small - see below - found what I think is another issue?

> +static int validate_beacon_tx_rate(struct cfg80211_ap_settings
> *params)
> +{
> +	u32 rate, count_ht, count_vht, i;
> +	enum nl80211_band band;
> +
> +	band = params->chandef.chan->band;
> +	rate = params->beacon_rate.control[band].legacy;
> +
> +	/* Allow only one rate */
> +	if (rate && (rate & (rate - 1)))
> +		return -EINVAL;

I was going to change that to just
	if (hweight32(rate) > 1)
		return -EINVAL;

I realize that your code is equivalent, but I doubt that we need to be
really efficient here, and IMHO hweight32() is easier to understand.

> +	count_ht = 0;
> +	for (i = 0; i < IEEE80211_HT_MCS_MASK_LEN; i++) {
> +		if (params->beacon_rate.control[band].ht_mcs[i]) {
> +			count_ht++;
> +			if (count_ht > 1)
> +				return -EINVAL;
> +		}
> +	}

But this is where I got confused - this seems wrong, you should be
checking the hweight8() of each of the ht_mcs[i] values?

Similarly, the VHT one, but with hweight16() of course, no?

I was going to move a "if (rate) return -EINVAL;" check into this and
the VHT loop, so that we only need the count_ht && count_vht and the
"all empty" portion of this:

> +	if ((rate && count_ht) ||
> +	    (rate && count_vht) ||
> +	    (count_ht && count_vht) ||
> +	    (!rate && !count_ht && !count_vht))
> +		return -EINVAL;
> +
> +	return 0;
> +}


johannes