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? > >> + /* Set the AMPDU density. */ > >> + if (sdata->u.mgd.ht_capa_mask.ampdu_params_info& > >> + IEEE80211_HT_AMPDU_PARM_DENSITY) > >> + ht_cap->ampdu_density = > >> + (sdata->u.mgd.ht_capa.ampdu_params_info& > >> + IEEE80211_HT_AMPDU_PARM_DENSITY) > >> + >> IEEE80211_HT_AMPDU_PARM_DENSITY_SHIFT; > >> +} > > > > 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. > > I think there's a lot of data in the ht_cap struct that you don't use, > > is that right? If so you should reject it being configured. I'm also not > > quite sure why you support both disable-HT40, and then this setting here > > that has SUP_WIDTH_20_40 too. > > If I add rejection like this, it will make writing backwards compat user > space very difficult. It would be similar to rejecting unknown netlink > attributes, for instance. Good point. > 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? > > 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. johannes -- 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
