Re: [PATCH v4 2/2] mac80211: Support ht-cap over-rides.

[Johannes Berg <johannes@sipsolutions.net>]

On Fri, 2011-11-04 at 13:10 -0700, greearb@candelatech.com wrote:

> +/*
> + * Stations supporting 802.11n are required to support
> + * at least the first 8 MCS rates.  See section 7.3.2.56.4
> + * and 20.1.1 of the 802.11n spec.
> + */
> +#define IEEE80211_HT_MCS_REQ_RATES_STA		8

I'd prefer if this was a validation on the input from userspace directly
in cfg80211, that way other drivers that want to implement this don't
have to bother.

That probably goes well with the validation of the supported mask that I
asked for.

> +	/* mac80211 allows over-riding some of the ht-capabilities */
> +	local->hw.wiphy->ht_capa_mod_mask =
> +		kzalloc(sizeof(*local->hw.wiphy->ht_capa_mod_mask),
> +			GFP_KERNEL);
> +	if (local->hw.wiphy->ht_capa_mod_mask) {
> +		struct ieee80211_ht_cap *cm = local->hw.wiphy->ht_capa_mod_mask;
> +		u8 *r = (u8 *)(&cm->mcs.rx_mask);
> +		memset(r, 0xff, IEEE80211_HT_MCS_MASK_LEN);
> +		cm->cap_info |= IEEE80211_HT_CAP_MAX_AMSDU;
> +		cm->ampdu_params_info |= IEEE80211_HT_AMPDU_PARM_FACTOR;
> +		cm->ampdu_params_info |= IEEE80211_HT_AMPDU_PARM_DENSITY;
> +	}

Why is this not just a static const that you fill manually? There's
nothing that's not constant here. So e.g.

static const struct ieee80211_ht_cap mac80211_ht_capa_mod_mask = {
	.ampdu_params_info = IEEE80211_HT_AMPDU_PARM_FACTOR |
			     IEEE80211_HT_AMPDU_PARM_DENSITY,
	.mcs = {
		.rx_mask = { 0xff, 0xff, 0xff, 0xff, 0xff,
			     0xff, 0xff, 0xff, 0xff, 0xff, },
	},
	/* etc */
};

> @@ -988,6 +1001,8 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
>   fail_wiphy_register:
>  	if (local->wiphy_ciphers_allocated)
>  		kfree(local->hw.wiphy->cipher_suites);
> +	kfree(local->hw.wiphy->ht_capa_mod_mask);
> +	local->hw.wiphy->ht_capa_mod_mask = NULL;

No need for this then obviously.


Looks pretty good overall. This is _much_ more readable than the
previous series. :-)

johannes