On 11/04/2011 07:41 AM, Johannes Berg wrote:
On Thu, 2011-11-03 at 10:13 -0700, Ben Greear wrote:
Here's another argument for splitting the patches differently -- this
ought to be part of the disable HT40 patch.
One thing I could do is move patch 3 to be the first patch. That gives this method
reason to exist, but I can leave out the disable-HT40 parts and (re)add that in
the disable-ht40 patch.
I think the whole thing could just be one cfg80211 and one mac80211
patch, wouldn't that make it simpler?
Ok, I will do that.
The AMPDU density should only allow increasing.
I think a lot of this validation should live in cfg80211 so if another
driver wants to implement it, this kind of thing is already covered.
The ath9k driver supports 0, and then every value that corresponds to 1us or higher.
If you set it to 1/2us, for instance, it just quietly rounds up to 1us. It defaults
to 8, so it appears valid to decrease or increase this value.
What quietly rounds up?
If it defaults to 8 then I'm sure there's a reason for it, such as the
crypto engine not being fast enough and needing 8us buffer between
frames. As such, I really don't think decreasing it is valid.
See this code in ath9k, top of main.c. It appears to support more
than the default of 8. I tested it out, and it appears to work
when set to lower values. I am disabling hw-crypt since I need
multiple VIFS, but not sure that matters or not.
static u8 parse_mpdudensity(u8 mpdudensity)
{
/*
* 802.11n D2.0 defined values for "Minimum MPDU Start Spacing":
* 0 for no restriction
* 1 for 1/4 us
* 2 for 1/2 us
* 3 for 1 us
* 4 for 2 us
* 5 for 4 us
* 6 for 8 us
* 7 for 16 us
*/
switch (mpdudensity) {
case 0:
return 0;
case 1:
case 2:
case 3:
/* Our lower layer calculations limit our precision to
1 microsecond */
return 1;
case 4:
return 2;
case 5:
return 4;
case 6:
return 8;
case 7:
return 16;
default:
return 0;
}
}
I was thinking that if ht-40 is disabled, then I should clear both the
IEEE80211_HT_CAP_SUP_WIDTH_20_40 and the IEEE80211_HT_CAP_SGI_40 from
the capabilities. Perhaps there are other flags I should clear as well?
I don't know?
Well, it can always be changed later if I missed something. This code
should have no affect unless the users specifically enable the feature
anyway...and we'll be doing lots of testing on our systems at various
settings...
I'm more and more coming to the conclusion that it would be clearer to
make separate configuration items for the various things. Most
capabilities you could only disable (greenfield, ...) except for maybe
40mhz-intol, so maybe that would be easier as a separate u16 attribute
"disable these HT capabilities"?
It seemed like more work for not much gain to me, but I don't mind splitting
it out into separate netlink configurables if you want.
It just seems to me it would clarify the semantics. Not really sure I
care all that much though.
If it's OK with you, I'll skip this for now. If anyone ever cares,
it would be easy enough to add.
Thanks,
Ben
--
Ben Greear <greearb@xxxxxxxxxxxxxxx>
Candela Technologies Inc http://www.candelatech.com
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
