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

[Johannes Berg <johannes@sipsolutions.net>]

[-- Attachment #1: Type: text/plain, Size: 4239 bytes --]

On Wed, 2008-11-26 at 10:38 +0200, Jouni Malinen wrote:
> On Tue, Nov 25, 2008 at 09:05:33PM +0200, Jouni Malinen wrote:
> > This patch adds new NL80211_CMD_SET_WIPHY attributes
> > NL80211_ATTR_WIPHY_FREQ and NL80211_ATTR_WIPHY_SEC_CHAN_OFFSET to allow
> > userspace to set the operating channel (e.g., hostapd for AP mode).
> 
> This version addresses the feedback I received by adding regulatory
> enforcement in net/wireless/nl80211.c and mechanism for indicating
> non-HT vs. HT20. In addition, the ht_enable/sec_chan_offset values are
> now handled similarly to local->oper_channel to avoid changing
> parameters for scan.

Thanks. A few other things that I just thought of:

> @@ -26,8 +26,9 @@
>   * @NL80211_CMD_GET_WIPHY: request information about a wiphy or dump request
>   *	to get a list of all present wiphys.
>   * @NL80211_CMD_SET_WIPHY: set wiphy parameters, needs %NL80211_ATTR_WIPHY or
> - *	%NL80211_ATTR_IFINDEX; can be used to set %NL80211_ATTR_WIPHY_NAME
> - *	and/or %NL80211_ATTR_WIPHY_TXQ_PARAMS.
> + *	%NL80211_ATTR_IFINDEX; can be used to set %NL80211_ATTR_WIPHY_NAME,
> + *	%NL80211_ATTR_WIPHY_TXQ_PARAMS, %NL80211_ATTR_WIPHY_FREQ, and/or
> + *	%NL80211_ATTR_WIPHY_SEC_CHAN_OFFSET.
>   * @NL80211_CMD_NEW_WIPHY: Newly created wiphy, response to get request
>   *	or rename notification. Has attributes %NL80211_ATTR_WIPHY and
>   *	%NL80211_ATTR_WIPHY_NAME.
> @@ -180,6 +181,12 @@
>   *	/sys/class/ieee80211/<phyname>/index
>   * @NL80211_ATTR_WIPHY_NAME: wiphy name (used for renaming)
>   * @NL80211_ATTR_WIPHY_TXQ_PARAMS: a nested array of TX queue parameters
> + * @NL80211_ATTR_WIPHY_FREQ: frequency of the selected channel in MHz
> + * @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
> + *	NL80211_SEC_CHAN_BELOW = secondary channel is below the primary channel
> + *	NL80211_SEC_CHAN_ABOVE = secondary channel is above the primary channel


> + *
> + * @set_channel: Set channel (freq = frequency in MHz, sec_chan_offset:
> + *	0 = HT40 disabled; 1 = HT40 enabled, secondary channel above primary;
> + *	-1 = HT40 enabled, secondary channel below primary)
>   */
>  struct cfg80211_ops {
>  	int	(*add_virtual_intf)(struct wiphy *wiphy, char *name,
> @@ -513,6 +517,9 @@
>  
>  	int	(*set_txq_params)(struct wiphy *wiphy,
>  				  struct ieee80211_txq_params *params);
> +
> +	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.
 
> +static int ieee80211_set_channel(struct wiphy *wiphy, int freq,
> +				 bool ht_enabled, int sec_chan_offset)
> +{
> +	struct ieee80211_local *local = wiphy_priv(wiphy);
> +	struct ieee80211_channel *chan;
> +
> +	chan = ieee80211_get_channel(local->hw.wiphy, freq);
> +	if (!chan)
> +		return -EINVAL;
> +
> +	local->oper_channel = chan;
> +	local->oper_ht_enabled = ht_enabled;
> +	local->oper_sec_chan_offset = sec_chan_offset;

And you could even store the enum here, and translate it only in
_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.

> +		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.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]