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

[Ben Greear <greearb@candelatech.com>]

On 11/07/2011 01:13 AM, Johannes Berg wrote:
> 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.

It is valid to request that the station use a lesser rate, it just
needs to advertise that it supports the first 8 rates.  So, for the
rate-control logic, we use exactly what is passed in from user-space,
but when generating the HT-caps objects, we set a minimum of 8 MCS
rates.

So, if you want validation, then I'm going to have to add a second
mcs argument and mask to distinguish between what we should advertise
v/s what we should use to over-ride the local rate-control mechanism.

I will do this if you ask, but in my opinion, the current functionality
is good enough.

>> +	/* 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 */
> };

Well, you suggested a pointer in the wiphy struct that was null
for non mac80211 interfaces.  I'm not sure how to distinguish between
mac80211 and other wiphys when reporting the capabilities if I use
this global static.  I also like that the non-static logic lets
us tweak this for individual drivers if that becomes an issue.

>> @@ -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. :-)

Thanks...making progress :)

Ben

>
> johannes


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com