Re: [RFC] nl80211: Add frequency configuration (including HT40)

[Jouni Malinen <j@w1.fi>]

On Wed, Nov 26, 2008 at 12:04:24PM +0100, Johannes Berg wrote:
> On Wed, 2008-11-26 at 10:38 +0200, Jouni Malinen wrote:

> > + * @NL80211_ATTR_WIPHY_SEC_CHAN_OFFSET: included with NL80211_ATTR_WIPHY_FREQ
> > + *	if HT20 or HT40 are allowed (i.e., 802.11n disabled if not included):
> 
> It's also possible to explicitly give the _NO_HT value too.
> 
> > + *	NL80211_SEC_CHAN_DISABLED = HT20 only

Yes, I did not include in documentation, but I can add it here as
another option for not including the attribute.

> > +
> > +	int	(*set_channel)(struct wiphy *wiphy, int freq, bool ht_enabled,
> > +			       int sec_chan_offset);
> 
> I wonder if we shouldn't simply pass in the enum nl80211_sec_chan_offset
> value here, that seems easier to understand when you're looking at the
> whole thing from userspace down to a possible non-mac80211 driver. Not
> that I think that'll ever happen, but still? It might even make sense to
> use that value within mac80211's driver API at some point, instead of
> the enabled / +/-1 thing.

I don't care much either way. nl80211.c uses the +/-1 to get the
secondary channel frequency and the driver API does not use it yet, so
conversion is needed somewhere at this point. I can move it somewhere
else if you want to; just point me to the place where you want it ;-).
cfg.c::ieee80211_set_channel()? main.c::ieee80211_hw_config()?


> > +	[NL80211_ATTR_WIPHY_FREQ] = { .type = NLA_U32 },
> > +	[NL80211_ATTR_WIPHY_SEC_CHAN_OFFSET] = { .type = NLA_U8 },
> 
> I think it's more "normal" to use a u32 for an enum although I don't
> think we'll ever add anything, so I guess it doesn't actually matter.

I'm quite sure we won't need more than eight bits to store the possible
options for this attribute, but I can change this to u32 to be
consistent with existing attributes if you prefer it.

> > +		freq = nla_get_u32(info->attrs[NL80211_ATTR_WIPHY_FREQ]);
> > +		chan = ieee80211_get_channel(&rdev->wiphy, freq);

> > +		result = rdev->ops->set_channel(&rdev->wiphy, freq, ht_enabled,
> > +						sec_chan_offset);
> 
> would it make sense to pass in 'chan'? mac80211 will do the lookup
> again, and if a driver doesn't it can just use chan->center_freq.

Now that chan is here in nl80211.c (was added with regulatory
enforcement), sure, it could be passed to set_channel() to remove some
code.

-- 
Jouni Malinen                                            PGP id EFC895FA