[RFC 0/2] mac80211: Enable VHT for non chanctxes drivers

[RFC 0/2] mac80211: Enable VHT for non chanctxes drivers

[Karl Beldan]

Hi,

Here are 2 patches so that we can use vht with drivers that don't use 
channel contexes (which include non-hw scan,roc devices drivers).
These require modifications for said drivers, and I only have included 
mods for mac80211_hwsim. 

Johannes, could you please have a look and give me your feedback ? 
 
If this gets approved I will do a pass on every drivers and ask for testing
help (plus I only have an htc and an ar9170).

Thanks,
 
Karl

[RFC 1/2] mac80211: Use a cfg80211_chan_def in ieee80211_hw_conf_chan

[Karl Beldan]

From: Karl Beldan <karl.beldan@rivierawaves.com>

Drivers that don't use chanctxes cannot perform VHT association because
they still use a "backward compatibility" pair of {ieee80211_channel,
nl80211_channel_type} in ieee80211_conf and ieee80211_local.
(FIXME: this only changes mac80211_hwsim for the RFC)

Signed-off-by: Karl Beldan <karl.beldan@rivierawaves.com>
---
 drivers/net/wireless/mac80211_hwsim.c |   45 +++++++++++++++++++---------
 include/net/mac80211.h                |   15 +++++----
 net/mac80211/cfg.c                    |    7 +---
 net/mac80211/chan.c                   |   10 +++---
 net/mac80211/ieee80211_i.h            |    4 +--
 net/mac80211/main.c                   |   52 +++++++++++++++++++++------------
 net/mac80211/mlme.c                   |    6 ++--
 net/mac80211/scan.c                   |    6 ++--
 net/mac80211/trace.h                  |    9 ++---
 net/mac80211/tx.c                     |    4 +-
 net/mac80211/util.c                   |    3 +-
 11 files changed, 92 insertions(+), 69 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 7490c4f..71cb109 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -1056,11 +1056,13 @@ out:
 	return HRTIMER_NORESTART;
 }
 
-static const char *hwsim_chantypes[] = {
-	[NL80211_CHAN_NO_HT] = "noht",
-	[NL80211_CHAN_HT20] = "ht20",
-	[NL80211_CHAN_HT40MINUS] = "ht40-",
-	[NL80211_CHAN_HT40PLUS] = "ht40+",
+static const char *hwsim_chanwidth[] = {
+	[NL80211_CHAN_WIDTH_20_NOHT] = "noht",
+	[NL80211_CHAN_WIDTH_20] = "ht20",
+	[NL80211_CHAN_WIDTH_40] = "ht40",
+	[NL80211_CHAN_WIDTH_80] = "ht80",
+	[NL80211_CHAN_WIDTH_80P80] = "ht80p80",
+	[NL80211_CHAN_WIDTH_160] = "ht160",
 };
 
 static int mac80211_hwsim_config(struct ieee80211_hw *hw, u32 changed)
@@ -1074,18 +1076,31 @@ static int mac80211_hwsim_config(struct ieee80211_hw *hw, u32 changed)
 		[IEEE80211_SMPS_DYNAMIC] = "dynamic",
 	};
 
-	wiphy_debug(hw->wiphy,
-		    "%s (freq=%d/%s idle=%d ps=%d smps=%s)\n",
-		    __func__,
-		    conf->channel ? conf->channel->center_freq : 0,
-		    hwsim_chantypes[conf->channel_type],
-		    !!(conf->flags & IEEE80211_CONF_IDLE),
-		    !!(conf->flags & IEEE80211_CONF_PS),
-		    smps_modes[conf->smps_mode]);
+	if (conf->chandef.chan)
+		wiphy_debug(hw->wiphy,
+			    "%s (freq=%d(%d - %d)/%s idle=%d ps=%d smps=%s)\n",
+			    __func__,
+			    conf->chandef.chan->center_freq,
+			    conf->chandef.width > NL80211_CHAN_WIDTH_20 ?
+			    conf->chandef.center_freq1 : 0,
+			    conf->chandef.width > NL80211_CHAN_WIDTH_40 ?
+			    conf->chandef.center_freq2 : 0,
+			    hwsim_chanwidth[conf->chandef.width],
+			    !!(conf->flags & IEEE80211_CONF_IDLE),
+			    !!(conf->flags & IEEE80211_CONF_PS),
+			    smps_modes[conf->smps_mode]);
+	else
+		wiphy_debug(hw->wiphy,
+			    "%s (freq=0/%s idle=%d ps=%d smps=%s)\n",
+			    __func__,
+			    hwsim_chanwidth[conf->chandef.width],
+			    !!(conf->flags & IEEE80211_CONF_IDLE),
+			    !!(conf->flags & IEEE80211_CONF_PS),
+			    smps_modes[conf->smps_mode]);
 
 	data->idle = !!(conf->flags & IEEE80211_CONF_IDLE);
 
-	data->channel = conf->channel;
+	data->channel = conf->chandef.chan;
 
 	WARN_ON(data->channel && channels > 1);
 
@@ -1271,7 +1286,7 @@ static int mac80211_hwsim_get_survey(
 		return -ENOENT;
 
 	/* Current channel */
-	survey->channel = conf->channel;
+	survey->channel = conf->chandef.chan;
 
 	/*
 	 * Magically conjured noise level --- this is only ok for simulated hardware.
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 8c0ca11..2eb3de9 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1001,8 +1001,7 @@ struct ieee80211_conf {
 
 	u8 long_frame_max_tx_count, short_frame_max_tx_count;
 
-	struct ieee80211_channel *channel;
-	enum nl80211_channel_type channel_type;
+	struct cfg80211_chan_def chandef;
 	bool radar_enabled;
 	enum ieee80211_smps_mode smps_mode;
 };
@@ -4204,31 +4203,33 @@ void ieee80211_rate_control_unregister(struct rate_control_ops *ops);
 static inline bool
 conf_is_ht20(struct ieee80211_conf *conf)
 {
-	return conf->channel_type == NL80211_CHAN_HT20;
+	return conf->chandef.width == NL80211_CHAN_WIDTH_20;
 }
 
 static inline bool
 conf_is_ht40_minus(struct ieee80211_conf *conf)
 {
-	return conf->channel_type == NL80211_CHAN_HT40MINUS;
+	return conf->chandef.width == NL80211_CHAN_WIDTH_40 &&
+	       conf->chandef.center_freq1 < conf->chandef.chan->center_freq;
 }
 
 static inline bool
 conf_is_ht40_plus(struct ieee80211_conf *conf)
 {
-	return conf->channel_type == NL80211_CHAN_HT40PLUS;
+	return conf->chandef.width == NL80211_CHAN_WIDTH_40 &&
+	       conf->chandef.center_freq1 > conf->chandef.chan->center_freq;
 }
 
 static inline bool
 conf_is_ht40(struct ieee80211_conf *conf)
 {
-	return conf_is_ht40_minus(conf) || conf_is_ht40_plus(conf);
+	return conf->chandef.width == NL80211_CHAN_WIDTH_40;
 }
 
 static inline bool
 conf_is_ht(struct ieee80211_conf *conf)
 {
-	return conf->channel_type != NL80211_CHAN_NO_HT;
+	return conf->chandef.width != NL80211_CHAN_WIDTH_20_NOHT;
 }
 
 static inline enum nl80211_iftype
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index e5c1441..d7924b0 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -805,8 +805,7 @@ static int ieee80211_set_monitor_channel(struct wiphy *wiphy,
 					IEEE80211_CHANCTX_EXCLUSIVE);
 		}
 	} else if (local->open_count == local->monitors) {
-		local->_oper_channel = chandef->chan;
-		local->_oper_channel_type = cfg80211_get_chandef_type(chandef);
+		memcpy(&local->oper_chandef, chandef, sizeof(local->oper_chandef));
 		ieee80211_hw_config(local, 0);
 	}
 
@@ -3360,9 +3359,7 @@ static int ieee80211_cfg_get_channel(struct wiphy *wiphy,
 		if (local->use_chanctx)
 			*chandef = local->monitor_chandef;
 		else
-			cfg80211_chandef_create(chandef,
-						local->_oper_channel,
-						local->_oper_channel_type);
+			memcpy(chandef, &local->oper_chandef, sizeof(chandef));
 		ret = 0;
 	}
 	rcu_read_unlock();
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 78c0d90..8b7f47a 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -22,7 +22,9 @@ static void ieee80211_change_chanctx(struct ieee80211_local *local,
 	drv_change_chanctx(local, ctx, IEEE80211_CHANCTX_CHANGE_WIDTH);
 
 	if (!local->use_chanctx) {
-		local->_oper_channel_type = cfg80211_get_chandef_type(chandef);
+		local->oper_chandef.width = chandef->width;
+		local->oper_chandef.center_freq1 = chandef->center_freq1;
+		local->oper_chandef.center_freq2 = chandef->center_freq2;
 		ieee80211_hw_config(local, 0);
 	}
 }
@@ -77,9 +79,7 @@ ieee80211_new_chanctx(struct ieee80211_local *local,
 	ctx->mode = mode;
 
 	if (!local->use_chanctx) {
-		local->_oper_channel_type =
-			cfg80211_get_chandef_type(chandef);
-		local->_oper_channel = chandef->chan;
+		memcpy(&local->oper_chandef, chandef, sizeof(local->oper_chandef));
 		ieee80211_hw_config(local, 0);
 	} else {
 		err = drv_add_chanctx(local, ctx);
@@ -106,7 +106,7 @@ static void ieee80211_free_chanctx(struct ieee80211_local *local,
 	WARN_ON_ONCE(ctx->refcount != 0);
 
 	if (!local->use_chanctx) {
-		local->_oper_channel_type = NL80211_CHAN_NO_HT;
+		local->oper_chandef.width = NL80211_CHAN_NO_HT;
 		ieee80211_hw_config(local, 0);
 	} else {
 		drv_remove_chanctx(local, ctx);
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 95beb18..b72a1d1 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1021,9 +1021,7 @@ struct ieee80211_local {
 	struct delayed_work scan_work;
 	struct ieee80211_sub_if_data __rcu *scan_sdata;
 	struct ieee80211_channel *csa_channel;
-	/* For backward compatibility only -- do not use */
-	struct ieee80211_channel *_oper_channel;
-	enum nl80211_channel_type _oper_channel_type;
+	struct cfg80211_chan_def oper_chandef;
 
 	/* Temporary remain-on-channel for off-channel operations */
 	struct ieee80211_channel *tmp_channel;
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index eee1768..25db628 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -92,47 +92,61 @@ static void ieee80211_reconfig_filter(struct work_struct *work)
 	ieee80211_configure_filter(local);
 }
 
+static int ieee80211_chandef_cmp(const struct cfg80211_chan_def *def1,
+				 const struct cfg80211_chan_def *def2)
+{
+	if (def1->chan != def2->chan || def1->width != def2->width)
+		return 1;
+	if (def1->width < NL80211_CHAN_WIDTH_40)
+		return 0;
+	if (def1->width != NL80211_CHAN_WIDTH_80P80)
+		return def1->center_freq1 != def2->center_freq1 ||
+		       def1->center_freq2 != def2->center_freq2;
+	return def1->center_freq1 != def2->center_freq1;
+}
+
 static u32 ieee80211_hw_conf_chan(struct ieee80211_local *local)
 {
 	struct ieee80211_sub_if_data *sdata;
+	struct cfg80211_chan_def chandef;
 	struct ieee80211_channel *chan;
 	u32 changed = 0;
 	int power;
-	enum nl80211_channel_type channel_type;
 	u32 offchannel_flag;
 
 	offchannel_flag = local->hw.conf.flags & IEEE80211_CONF_OFFCHANNEL;
+
 	if (local->scan_channel) {
-		chan = local->scan_channel;
+		chandef.chan = local->scan_channel;
 		/* If scanning on oper channel, use whatever channel-type
 		 * is currently in use.
 		 */
-		if (chan == local->_oper_channel)
-			channel_type = local->_oper_channel_type;
+		if (chandef.chan == local->oper_chandef.chan)
+			memcpy(&chandef, &local->oper_chandef, sizeof(chandef));
 		else
-			channel_type = NL80211_CHAN_NO_HT;
+			chandef.width = NL80211_CHAN_WIDTH_20_NOHT;
 	} else if (local->tmp_channel) {
-		chan = local->tmp_channel;
-		channel_type = NL80211_CHAN_NO_HT;
+		chandef.chan = local->tmp_channel;
+		chandef.width = NL80211_CHAN_WIDTH_20_NOHT;
 	} else {
-		chan = local->_oper_channel;
-		channel_type = local->_oper_channel_type;
+		memcpy(&chandef, &local->oper_chandef, sizeof(chandef));
 	}
 
-	if (chan != local->_oper_channel ||
-	    channel_type != local->_oper_channel_type)
+	if (ieee80211_chandef_cmp(&chandef, &local->oper_chandef))
 		local->hw.conf.flags |= IEEE80211_CONF_OFFCHANNEL;
 	else
 		local->hw.conf.flags &= ~IEEE80211_CONF_OFFCHANNEL;
 
 	offchannel_flag ^= local->hw.conf.flags & IEEE80211_CONF_OFFCHANNEL;
 
-	if (offchannel_flag || chan != local->hw.conf.channel ||
-	    channel_type != local->hw.conf.channel_type) {
-		local->hw.conf.channel = chan;
-		local->hw.conf.channel_type = channel_type;
+	if (offchannel_flag ||
+	    ieee80211_chandef_cmp(&local->hw.conf.chandef,
+				  &local->oper_chandef)) {
+		memcpy(&local->hw.conf.chandef, &chandef,
+		       sizeof(local->hw.conf.chandef));
 		changed |= IEEE80211_CONF_CHANGE_CHANNEL;
 	}
+	chan = chandef.chan;
 
 	if (!conf_is_ht(&local->hw.conf)) {
 		/*
@@ -738,11 +752,11 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
 		sband = local->hw.wiphy->bands[band];
 		if (!sband)
 			continue;
-		if (!local->use_chanctx && !local->_oper_channel) {
+		if (!local->use_chanctx && !local->oper_chandef.chan) {
 			/* init channel we're on */
-			local->hw.conf.channel =
-			local->_oper_channel = &sband->channels[0];
-			local->hw.conf.channel_type = NL80211_CHAN_NO_HT;
+			local->hw.conf.chandef.chan =
+			local->oper_chandef.chan = &sband->channels[0];
+			local->hw.conf.chandef.width = NL80211_CHAN_NO_HT;
 		}
 		cfg80211_chandef_create(&local->monitor_chandef,
 					&sband->channels[0],
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index fdc06e3..3a98660 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -994,18 +994,18 @@ static void ieee80211_chswitch_work(struct work_struct *work)
 	if (!ifmgd->associated)
 		goto out;
 
-	sdata->local->_oper_channel = sdata->local->csa_channel;
+	sdata->local->oper_chandef.chan = sdata->local->csa_channel;
 	if (!sdata->local->ops->channel_switch) {
 		/* call "hw_config" only if doing sw channel switch */
 		ieee80211_hw_config(sdata->local,
 			IEEE80211_CONF_CHANGE_CHANNEL);
 	} else {
 		/* update the device channel directly */
-		sdata->local->hw.conf.channel = sdata->local->_oper_channel;
+		sdata->local->hw.conf.chandef.chan = sdata->local->oper_chandef.chan;
 	}
 
 	/* XXX: shouldn't really modify cfg80211-owned data! */
-	ifmgd->associated->channel = sdata->local->_oper_channel;
+	ifmgd->associated->channel = sdata->local->oper_chandef.chan;
 
 	/* XXX: wait for a beacon first? */
 	ieee80211_wake_queues_by_reason(&sdata->local->hw,
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 5dc17c6..1b1959e 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -384,7 +384,7 @@ static void ieee80211_scan_state_send_probe(struct ieee80211_local *local,
 {
 	int i;
 	struct ieee80211_sub_if_data *sdata;
-	enum ieee80211_band band = local->hw.conf.channel->band;
+	enum ieee80211_band band = local->hw.conf.chandef.chan->band;
 	u32 tx_flags;
 
 	tx_flags = IEEE80211_TX_INTFL_OFFCHAN_TX_OK;
@@ -401,7 +401,7 @@ static void ieee80211_scan_state_send_probe(struct ieee80211_local *local,
 			local->scan_req->ssids[i].ssid_len,
 			local->scan_req->ie, local->scan_req->ie_len,
 			local->scan_req->rates[band], false,
-			tx_flags, local->hw.conf.channel, true);
+			tx_flags, local->hw.conf.chandef.chan, true);
 
 	/*
 	 * After sending probe requests, wait for probe responses
@@ -467,7 +467,7 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
 	if (local->ops->hw_scan) {
 		__set_bit(SCAN_HW_SCANNING, &local->scanning);
 	} else if ((req->n_channels == 1) &&
-		   (req->channels[0] == local->_oper_channel)) {
+		   (req->channels[0] == local->oper_chandef.chan)) {
 		/*
 		 * If we are scanning only on the operating channel
 		 * then we do not need to stop normal activities
diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
index e7db2b8..e623daa 100644
--- a/net/mac80211/trace.h
+++ b/net/mac80211/trace.h
@@ -269,7 +269,6 @@ DEFINE_EVENT(local_sdata_addr_evt, drv_remove_interface,
 		 struct ieee80211_sub_if_data *sdata),
 	TP_ARGS(local, sdata)
 );
-
 TRACE_EVENT(drv_config,
 	TP_PROTO(struct ieee80211_local *local,
 		 u32 changed),
@@ -287,7 +286,7 @@ TRACE_EVENT(drv_config,
 		__field(u8, long_frame_max_tx_count)
 		__field(u8, short_frame_max_tx_count)
 		__field(int, center_freq)
-		__field(int, channel_type)
+		__field(int, channel_width)
 		__field(int, smps)
 	),
 
@@ -303,9 +302,9 @@ TRACE_EVENT(drv_config,
 			local->hw.conf.long_frame_max_tx_count;
 		__entry->short_frame_max_tx_count =
 			local->hw.conf.short_frame_max_tx_count;
-		__entry->center_freq = local->hw.conf.channel ?
-					local->hw.conf.channel->center_freq : 0;
-		__entry->channel_type = local->hw.conf.channel_type;
+		__entry->center_freq = local->hw.conf.chandef.chan ?
+				       local->hw.conf.chandef.chan->center_freq : 0;
+		__entry->channel_width = local->hw.conf.chandef.width;
 		__entry->smps = local->hw.conf.smps_mode;
 	),
 
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 3fcdf21..efa0aef 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1708,7 +1708,7 @@ netdev_tx_t ieee80211_monitor_start_xmit(struct sk_buff *skb,
 	if (chanctx_conf)
 		chan = chanctx_conf->def.chan;
 	else if (!local->use_chanctx)
-		chan = local->_oper_channel;
+		chan = local->oper_chandef.chan;
 	else
 		goto fail_rcu;
 
@@ -1842,7 +1842,7 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
 		 * This is the exception! WDS style interfaces are prohibited
 		 * when channel contexts are in used so this must be valid
 		 */
-		band = local->hw.conf.channel->band;
+		band = local->hw.conf.chandef.chan->band;
 		break;
 #ifdef CONFIG_MAC80211_MESH
 	case NL80211_IFTYPE_MESH_POINT:
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index b7a856e..53248b5 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -2137,8 +2137,7 @@ void ieee80211_dfs_radar_detected_work(struct work_struct *work)
 		/* currently not handled */
 		WARN_ON(1);
 	else {
-		cfg80211_chandef_create(&chandef, local->hw.conf.channel,
-					local->hw.conf.channel_type);
+		memcpy(&chandef, &local->hw.conf.chandef, sizeof(chandef));
 		cfg80211_radar_event(local->hw.wiphy, &chandef, GFP_KERNEL);
 	}
 }
-- 
1.7.9.dirty

[RFC 2/2] mac80211: Let drivers not supporting channel contexts use VHT

[Karl Beldan]

From: Karl Beldan <karl.beldan@rivierawaves.com>

It is possible since the global hw config and local switched to
cfg80211_chan_def.

Signed-off-by: Karl Beldan <karl.beldan@rivierawaves.com>
---
 net/mac80211/main.c |   14 +-------------
 1 files changed, 1 insertions(+), 13 deletions(-)

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 25db628..276c0ef 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -843,22 +843,10 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
 	if (supp_ht)
 		local->scan_ies_len += 2 + sizeof(struct ieee80211_ht_cap);
 
-	if (supp_vht) {
+	if (supp_vht)
 		local->scan_ies_len +=
 			2 + sizeof(struct ieee80211_vht_cap);
 
-		/*
-		 * (for now at least), drivers wanting to use VHT must
-		 * support channel contexts, as they contain all the
-		 * necessary VHT information and the global hw config
-		 * doesn't (yet)
-		 */
-		if (WARN_ON(!local->use_chanctx)) {
-			result = -EINVAL;
-			goto fail_wiphy_register;
-		}
-	}
-
 	if (!local->ops->hw_scan) {
 		/* For hw_scan, driver needs to set these up. */
 		local->hw.wiphy->max_scan_ssids = 4;
-- 
1.7.9.dirty

Re: [RFC 1/2] mac80211: Use a cfg80211_chan_def in ieee80211_hw_conf_chan

[Karl Beldan]

On Sun, Mar 17, 2013 at 09:30:33PM +0100, Karl Beldan wrote:
> From: Karl Beldan <karl.beldan@rivierawaves.com>
> 
> Drivers that don't use chanctxes cannot perform VHT association because
> they still use a "backward compatibility" pair of {ieee80211_channel,
> nl80211_channel_type} in ieee80211_conf and ieee80211_local.
> (FIXME: this only changes mac80211_hwsim for the RFC)
> 
I forgot the monitor part (I thought I would not miss a part from what
I've been using at work, sorry), anyways I'll wait for your feedback
before adjusting that.

 
Karl

Re: [RFC 1/2] mac80211: Use a cfg80211_chan_def in ieee80211_hw_conf_chan

[Johannes Berg]

On Sun, 2013-03-17 at 21:30 +0100, Karl Beldan wrote:

> +	if (conf->chandef.chan)

> +	else
> +		wiphy_debug(hw->wiphy,
> +			    "%s (freq=0/%s idle=%d ps=%d smps=%s)\n",

I don't think the else should be allowed to happen.

> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -805,8 +805,7 @@ static int ieee80211_set_monitor_channel(struct wiphy *wiphy,
>  					IEEE80211_CHANCTX_EXCLUSIVE);
>  		}
>  	} else if (local->open_count == local->monitors) {
> -		local->_oper_channel = chandef->chan;
> -		local->_oper_channel_type = cfg80211_get_chandef_type(chandef);
> +		memcpy(&local->oper_chandef, chandef, sizeof(local->oper_chandef));

Somehow I'd prefer an assignment:

local->oper_chandef = *chandef;

I know it'll just be a memcpy() again, but ... reads nicer, imho.

> +			memcpy(chandef, &local->oper_chandef, sizeof(chandef));

ditto

> @@ -22,7 +22,9 @@ static void ieee80211_change_chanctx(struct ieee80211_local *local,
>  	drv_change_chanctx(local, ctx, IEEE80211_CHANCTX_CHANGE_WIDTH);
>  
>  	if (!local->use_chanctx) {
> -		local->_oper_channel_type = cfg80211_get_chandef_type(chandef);
> +		local->oper_chandef.width = chandef->width;
> +		local->oper_chandef.center_freq1 = chandef->center_freq1;
> +		local->oper_chandef.center_freq2 = chandef->center_freq2;

Why not assign all here as well? Is the channel pointer itself special?

> -	/* For backward compatibility only -- do not use */
> -	struct ieee80211_channel *_oper_channel;
> -	enum nl80211_channel_type _oper_channel_type;
> +	struct cfg80211_chan_def oper_chandef;

You shouldn't remove the comment, and I think you should also keep the
leading underscore -- this is still for backward compatibility with
non-chanctx drivers only, and mac80211 internally must use chanctxes all
the way except in SW scan and SW roc code.
 
> +static int ieee80211_chandef_cmp(const struct cfg80211_chan_def *def1,
> +				 const struct cfg80211_chan_def *def2)

indentation

However this already exists in cfg80211.h --
cfg80211_chandef_identical().

>  static u32 ieee80211_hw_conf_chan(struct ieee80211_local *local)
>  {
>  	struct ieee80211_sub_if_data *sdata;
> +	struct cfg80211_chan_def chandef;

better initialize this: struct ... chandef = {};

> +	if (offchannel_flag ||
> +	    ieee80211_chandef_cmp(&local->hw.conf.chandef,
> +				  &local->oper_chandef)) {

indentation

> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -994,18 +994,18 @@ static void ieee80211_chswitch_work(struct work_struct *work)
>  	if (!ifmgd->associated)
>  		goto out;
>  
> -	sdata->local->_oper_channel = sdata->local->csa_channel;
> +	sdata->local->oper_chandef.chan = sdata->local->csa_channel;
>  	if (!sdata->local->ops->channel_switch) {
>  		/* call "hw_config" only if doing sw channel switch */
>  		ieee80211_hw_config(sdata->local,
>  			IEEE80211_CONF_CHANGE_CHANNEL);
>  	} else {
>  		/* update the device channel directly */
> -		sdata->local->hw.conf.channel = sdata->local->_oper_channel;
> +		sdata->local->hw.conf.chandef.chan = sdata->local->oper_chandef.chan;
>  	}

All of this is now even more obviously wrong. If it's an HT40 channel,
it previously would try to switch to the same HT40 channel, but now it'd
result in an invalid chandef, the center_freq1 would now be wrong.

> --- a/net/mac80211/trace.h
> +++ b/net/mac80211/trace.h
> @@ -269,7 +269,6 @@ DEFINE_EVENT(local_sdata_addr_evt, drv_remove_interface,
>  		 struct ieee80211_sub_if_data *sdata),
>  	TP_ARGS(local, sdata)
>  );
> -
>  TRACE_EVENT(drv_config,

please don't remove that line :)

> @@ -287,7 +286,7 @@ TRACE_EVENT(drv_config,
>  		__field(u8, long_frame_max_tx_count)
>  		__field(u8, short_frame_max_tx_count)
>  		__field(int, center_freq)
> -		__field(int, channel_type)
> +		__field(int, channel_width)

I think you should add center_freq1/2 ... there are also macros for
chandef tracing, so you could use those.

johannes

Re: [RFC 1/2] mac80211: Use a cfg80211_chan_def in ieee80211_hw_conf_chan

[Karl Beldan]

On Mon, Mar 18, 2013 at 08:25:10PM +0100, Johannes Berg wrote:
> On Sun, 2013-03-17 at 21:30 +0100, Karl Beldan wrote:
> 
> > +	if (conf->chandef.chan)
> 
> > +	else
> > +		wiphy_debug(hw->wiphy,
> > +			    "%s (freq=0/%s idle=%d ps=%d smps=%s)\n",
> 
> I don't think the else should be allowed to happen.
> 
> > --- a/net/mac80211/cfg.c
> > +++ b/net/mac80211/cfg.c
> > @@ -805,8 +805,7 @@ static int ieee80211_set_monitor_channel(struct wiphy *wiphy,
> >  					IEEE80211_CHANCTX_EXCLUSIVE);
> >  		}
> >  	} else if (local->open_count == local->monitors) {
> > -		local->_oper_channel = chandef->chan;
> > -		local->_oper_channel_type = cfg80211_get_chandef_type(chandef);
> > +		memcpy(&local->oper_chandef, chandef, sizeof(local->oper_chandef));
> 
> Somehow I'd prefer an assignment:
> 
> local->oper_chandef = *chandef;
> 
> I know it'll just be a memcpy() again, but ... reads nicer, imho.
> 
> > +			memcpy(chandef, &local->oper_chandef, sizeof(chandef));
> 
> ditto
> 
> > @@ -22,7 +22,9 @@ static void ieee80211_change_chanctx(struct ieee80211_local *local,
> >  	drv_change_chanctx(local, ctx, IEEE80211_CHANCTX_CHANGE_WIDTH);
> >  
> >  	if (!local->use_chanctx) {
> > -		local->_oper_channel_type = cfg80211_get_chandef_type(chandef);
> > +		local->oper_chandef.width = chandef->width;
> > +		local->oper_chandef.center_freq1 = chandef->center_freq1;
> > +		local->oper_chandef.center_freq2 = chandef->center_freq2;
> 
> Why not assign all here as well? Is the channel pointer itself special?
> 
> > -	/* For backward compatibility only -- do not use */
> > -	struct ieee80211_channel *_oper_channel;
> > -	enum nl80211_channel_type _oper_channel_type;
> > +	struct cfg80211_chan_def oper_chandef;
> 
> You shouldn't remove the comment, and I think you should also keep the
> leading underscore -- this is still for backward compatibility with
> non-chanctx drivers only, and mac80211 internally must use chanctxes all
> the way except in SW scan and SW roc code.
>  
> > +static int ieee80211_chandef_cmp(const struct cfg80211_chan_def *def1,
> > +				 const struct cfg80211_chan_def *def2)
> 
> indentation
> 
> However this already exists in cfg80211.h --
> cfg80211_chandef_identical().
> 
> >  static u32 ieee80211_hw_conf_chan(struct ieee80211_local *local)
> >  {
> >  	struct ieee80211_sub_if_data *sdata;
> > +	struct cfg80211_chan_def chandef;
> 
> better initialize this: struct ... chandef = {};
> 
> > +	if (offchannel_flag ||
> > +	    ieee80211_chandef_cmp(&local->hw.conf.chandef,
> > +				  &local->oper_chandef)) {
> 
> indentation
> 
> > --- a/net/mac80211/mlme.c
> > +++ b/net/mac80211/mlme.c
> > @@ -994,18 +994,18 @@ static void ieee80211_chswitch_work(struct work_struct *work)
> >  	if (!ifmgd->associated)
> >  		goto out;
> >  
> > -	sdata->local->_oper_channel = sdata->local->csa_channel;
> > +	sdata->local->oper_chandef.chan = sdata->local->csa_channel;
> >  	if (!sdata->local->ops->channel_switch) {
> >  		/* call "hw_config" only if doing sw channel switch */
> >  		ieee80211_hw_config(sdata->local,
> >  			IEEE80211_CONF_CHANGE_CHANNEL);
> >  	} else {
> >  		/* update the device channel directly */
> > -		sdata->local->hw.conf.channel = sdata->local->_oper_channel;
> > +		sdata->local->hw.conf.chandef.chan = sdata->local->oper_chandef.chan;
> >  	}
> 
> All of this is now even more obviously wrong. If it's an HT40 channel,
> it previously would try to switch to the same HT40 channel, but now it'd
> result in an invalid chandef, the center_freq1 would now be wrong.
> 
> > --- a/net/mac80211/trace.h
> > +++ b/net/mac80211/trace.h
> > @@ -269,7 +269,6 @@ DEFINE_EVENT(local_sdata_addr_evt, drv_remove_interface,
> >  		 struct ieee80211_sub_if_data *sdata),
> >  	TP_ARGS(local, sdata)
> >  );
> > -
> >  TRACE_EVENT(drv_config,
> 
> please don't remove that line :)
> 
> > @@ -287,7 +286,7 @@ TRACE_EVENT(drv_config,
> >  		__field(u8, long_frame_max_tx_count)
> >  		__field(u8, short_frame_max_tx_count)
> >  		__field(int, center_freq)
> > -		__field(int, channel_type)
> > +		__field(int, channel_width)
> 
> I think you should add center_freq1/2 ... there are also macros for
> chandef tracing, so you could use those.
> 

Thank you for the review Johannes, tomorrow I will match it against what
I had at the office and will send an updated version tomorrow.
 
Karl

Re: [RFC 1/2] mac80211: Use a cfg80211_chan_def in ieee80211_hw_conf_chan

[Karl Beldan]

On Mon, Mar 18, 2013 at 08:25:10PM +0100, Johannes Berg wrote:
> On Sun, 2013-03-17 at 21:30 +0100, Karl Beldan wrote:
> 
> > +	if (conf->chandef.chan)
> 
> > +	else
> > +		wiphy_debug(hw->wiphy,
> > +			    "%s (freq=0/%s idle=%d ps=%d smps=%s)\n",
> 
> I don't think the else should be allowed to happen.
> 

Although I saw the hw config got an initial channel right from the start
in ieee80211_register_hw, the original checks in the code confused since
it tested for it in the tracing of drv_config.
I'll get rid of it (in trace.h too).

> > --- a/net/mac80211/cfg.c
> > +++ b/net/mac80211/cfg.c
> > @@ -805,8 +805,7 @@ static int ieee80211_set_monitor_channel(struct wiphy *wiphy,
> >  					IEEE80211_CHANCTX_EXCLUSIVE);
> >  		}
> >  	} else if (local->open_count == local->monitors) {
> > -		local->_oper_channel = chandef->chan;
> > -		local->_oper_channel_type = cfg80211_get_chandef_type(chandef);
> > +		memcpy(&local->oper_chandef, chandef, sizeof(local->oper_chandef));
> 
> Somehow I'd prefer an assignment:
> 
> local->oper_chandef = *chandef;
> 
> I know it'll just be a memcpy() again, but ... reads nicer, imho.
> 
> > +			memcpy(chandef, &local->oper_chandef, sizeof(chandef));
> 
> ditto
> 
> > @@ -22,7 +22,9 @@ static void ieee80211_change_chanctx(struct ieee80211_local *local,
> >  	drv_change_chanctx(local, ctx, IEEE80211_CHANCTX_CHANGE_WIDTH);
> >  
> >  	if (!local->use_chanctx) {
> > -		local->_oper_channel_type = cfg80211_get_chandef_type(chandef);
> > +		local->oper_chandef.width = chandef->width;
> > +		local->oper_chandef.center_freq1 = chandef->center_freq1;
> > +		local->oper_chandef.center_freq2 = chandef->center_freq2;
> 
> Why not assign all here as well? Is the channel pointer itself special?
> 
I thought it would be more explicit to highlight we are keeping the same
channel, but you are right, better assign the whole thing.

> > -	/* For backward compatibility only -- do not use */
> > -	struct ieee80211_channel *_oper_channel;
> > -	enum nl80211_channel_type _oper_channel_type;
> > +	struct cfg80211_chan_def oper_chandef;
> 
> You shouldn't remove the comment, and I think you should also keep the
> leading underscore -- this is still for backward compatibility with
> non-chanctx drivers only, and mac80211 internally must use chanctxes all
> the way except in SW scan and SW roc code.
>  
Yes.

> > +static int ieee80211_chandef_cmp(const struct cfg80211_chan_def *def1,
> > +				 const struct cfg80211_chan_def *def2)
> 
> indentation
> 
This one is good, it's just the side effect of the addition of the
leading '+'.

> However this already exists in cfg80211.h --
> cfg80211_chandef_identical().
> 
> >  static u32 ieee80211_hw_conf_chan(struct ieee80211_local *local)
> >  {
> >  	struct ieee80211_sub_if_data *sdata;
> > +	struct cfg80211_chan_def chandef;
> 

> better initialize this: struct ... chandef = {};
> 
> > +	if (offchannel_flag ||
> > +	    ieee80211_chandef_cmp(&local->hw.conf.chandef,
> > +				  &local->oper_chandef)) {
> 
> indentation
> 
Yes ieee80211_chandef_cmp fits on the 1st line.

> > --- a/net/mac80211/mlme.c
> > +++ b/net/mac80211/mlme.c
> > @@ -994,18 +994,18 @@ static void ieee80211_chswitch_work(struct work_struct *work)
> >  	if (!ifmgd->associated)
> >  		goto out;
> >  
> > -	sdata->local->_oper_channel = sdata->local->csa_channel;
> > +	sdata->local->oper_chandef.chan = sdata->local->csa_channel;
> >  	if (!sdata->local->ops->channel_switch) {
> >  		/* call "hw_config" only if doing sw channel switch */
> >  		ieee80211_hw_config(sdata->local,
> >  			IEEE80211_CONF_CHANGE_CHANNEL);
> >  	} else {
> >  		/* update the device channel directly */
> > -		sdata->local->hw.conf.channel = sdata->local->_oper_channel;
> > +		sdata->local->hw.conf.chandef.chan = sdata->local->oper_chandef.chan;
> >  	}
> 
> All of this is now even more obviously wrong. If it's an HT40 channel,
> it previously would try to switch to the same HT40 channel, but now it'd
> result in an invalid chandef, the center_freq1 would now be wrong.
> 
Yes, thanks ! I was careless here.
We should replace the csa_channel with a csa_chandef and get the freqs
with the vif processed in ieee80211_sta_process_chanswitch, or is it
utterly wrong ?

> > --- a/net/mac80211/trace.h
> > +++ b/net/mac80211/trace.h
> > @@ -269,7 +269,6 @@ DEFINE_EVENT(local_sdata_addr_evt, drv_remove_interface,
> >  		 struct ieee80211_sub_if_data *sdata),
> >  	TP_ARGS(local, sdata)
> >  );
> > -
> >  TRACE_EVENT(drv_config,
> 
> please don't remove that line :)
> 
> > @@ -287,7 +286,7 @@ TRACE_EVENT(drv_config,
> >  		__field(u8, long_frame_max_tx_count)
> >  		__field(u8, short_frame_max_tx_count)
> >  		__field(int, center_freq)
> > -		__field(int, channel_type)
> > +		__field(int, channel_width)
> 
> I think you should add center_freq1/2 ... there are also macros for
> chandef tracing, so you could use those.
> 
Yep, will do.
I hadn't spotted the CHANDEF_*, but even then I would have remained
confused by the "local->hw.conf.channel ?" tests (as in mac80211_hwsim),
- I will drop these "channel == NULL" tests.

Annnd I think I'll make another round right now.

 
Karl

Re: [RFC 1/2] mac80211: Use a cfg80211_chan_def in ieee80211_hw_conf_chan

[Johannes Berg]

On Mon, 2013-03-18 at 22:14 +0100, Karl Beldan wrote:
> On Mon, Mar 18, 2013 at 08:25:10PM +0100, Johannes Berg wrote:
> > On Sun, 2013-03-17 at 21:30 +0100, Karl Beldan wrote:
> > 
> > > +	if (conf->chandef.chan)
> > 
> > > +	else
> > > +		wiphy_debug(hw->wiphy,
> > > +			    "%s (freq=0/%s idle=%d ps=%d smps=%s)\n",
> > 
> > I don't think the else should be allowed to happen.
> > 
> 
> Although I saw the hw config got an initial channel right from the start
> in ieee80211_register_hw, the original checks in the code confused since
> it tested for it in the tracing of drv_config.
> I'll get rid of it (in trace.h too).

It has to check in tracing because tracing can be invoked when the
driver uses chanctx, in which case it will be NULL. And indeed, that's
also the reason for hwsim -- it can operate in both chanctx and
non-chanctx modes. Sorry about the confusion!

> > > +static int ieee80211_chandef_cmp(const struct cfg80211_chan_def *def1,
> > > +				 const struct cfg80211_chan_def *def2)
> > 
> > indentation
> > 
> This one is good, it's just the side effect of the addition of the
> leading '+'.

Ahrg, yeah, I can't trust my email client any more. It used to get this
right :-(

> > >  		/* update the device channel directly */
> > > -		sdata->local->hw.conf.channel = sdata->local->_oper_channel;
> > > +		sdata->local->hw.conf.chandef.chan = sdata->local->oper_chandef.chan;
> > >  	}
> > 
> > All of this is now even more obviously wrong. If it's an HT40 channel,
> > it previously would try to switch to the same HT40 channel, but now it'd
> > result in an invalid chandef, the center_freq1 would now be wrong.
> > 
> Yes, thanks ! I was careless here.
> We should replace the csa_channel with a csa_chandef and get the freqs
> with the vif processed in ieee80211_sta_process_chanswitch, or is it
> utterly wrong ?

It's all kinda bad. We really have to implement extended CSA that tells
us what channel width to switch to, I have some patches but it's tricky
and I can't really test it :-(

johannes

Re: [RFC 1/2] mac80211: Use a cfg80211_chan_def in ieee80211_hw_conf_chan

[Karl Beldan]

On Mon, Mar 18, 2013 at 08:25:10PM +0100, Johannes Berg wrote:
> On Sun, 2013-03-17 at 21:30 +0100, Karl Beldan wrote:
> 
> > +	if (conf->chandef.chan)
> 
> > +	else
> > +		wiphy_debug(hw->wiphy,
> > +			    "%s (freq=0/%s idle=%d ps=%d smps=%s)\n",
> 
> I don't think the else should be allowed to happen.
> 
> > --- a/net/mac80211/cfg.c
> > +++ b/net/mac80211/cfg.c
> > @@ -805,8 +805,7 @@ static int ieee80211_set_monitor_channel(struct wiphy *wiphy,
> >  					IEEE80211_CHANCTX_EXCLUSIVE);
> >  		}
> >  	} else if (local->open_count == local->monitors) {
> > -		local->_oper_channel = chandef->chan;
> > -		local->_oper_channel_type = cfg80211_get_chandef_type(chandef);
> > +		memcpy(&local->oper_chandef, chandef, sizeof(local->oper_chandef));
> 
> Somehow I'd prefer an assignment:
> 
> local->oper_chandef = *chandef;
> 
> I know it'll just be a memcpy() again, but ... reads nicer, imho.
> 
> > +			memcpy(chandef, &local->oper_chandef, sizeof(chandef));
> 
> ditto
> 
> > @@ -22,7 +22,9 @@ static void ieee80211_change_chanctx(struct ieee80211_local *local,
> >  	drv_change_chanctx(local, ctx, IEEE80211_CHANCTX_CHANGE_WIDTH);
> >  
> >  	if (!local->use_chanctx) {
> > -		local->_oper_channel_type = cfg80211_get_chandef_type(chandef);
> > +		local->oper_chandef.width = chandef->width;
> > +		local->oper_chandef.center_freq1 = chandef->center_freq1;
> > +		local->oper_chandef.center_freq2 = chandef->center_freq2;
> 
> Why not assign all here as well? Is the channel pointer itself special?
> 
> > -	/* For backward compatibility only -- do not use */
> > -	struct ieee80211_channel *_oper_channel;
> > -	enum nl80211_channel_type _oper_channel_type;
> > +	struct cfg80211_chan_def oper_chandef;
> 
> You shouldn't remove the comment, and I think you should also keep the
> leading underscore -- this is still for backward compatibility with
> non-chanctx drivers only, and mac80211 internally must use chanctxes all
> the way except in SW scan and SW roc code.
>  
> > +static int ieee80211_chandef_cmp(const struct cfg80211_chan_def *def1,
> > +				 const struct cfg80211_chan_def *def2)
> 
> indentation
> 
> However this already exists in cfg80211.h --
> cfg80211_chandef_identical().
> 
cfg80211_chandef_identical() will compare the whole chandef whatever the
channel width.
 
Karl

Re: [RFC 1/2] mac80211: Use a cfg80211_chan_def in ieee80211_hw_conf_chan

[Karl Beldan]

My own input to highlight the changes in V2.

On Sun, Mar 17, 2013 at 09:30:33PM +0100, Karl Beldan wrote:
> From: Karl Beldan <karl.beldan@rivierawaves.com>
> 
> Drivers that don't use chanctxes cannot perform VHT association because
> they still use a "backward compatibility" pair of {ieee80211_channel,
> nl80211_channel_type} in ieee80211_conf and ieee80211_local.
> (FIXME: this only changes mac80211_hwsim for the RFC)
> 
> Signed-off-by: Karl Beldan <karl.beldan@rivierawaves.com>
> ---
>  drivers/net/wireless/mac80211_hwsim.c |   45 +++++++++++++++++++---------
>  include/net/mac80211.h                |   15 +++++----
>  net/mac80211/cfg.c                    |    7 +---
>  net/mac80211/chan.c                   |   10 +++---
>  net/mac80211/ieee80211_i.h            |    4 +--
>  net/mac80211/main.c                   |   52 +++++++++++++++++++++------------
>  net/mac80211/mlme.c                   |    6 ++--
>  net/mac80211/scan.c                   |    6 ++--
>  net/mac80211/trace.h                  |    9 ++---
>  net/mac80211/tx.c                     |    4 +-
>  net/mac80211/util.c                   |    3 +-
>  11 files changed, 92 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
> index 7490c4f..71cb109 100644
> --- a/drivers/net/wireless/mac80211_hwsim.c
> +++ b/drivers/net/wireless/mac80211_hwsim.c
> @@ -1056,11 +1056,13 @@ out:
>  	return HRTIMER_NORESTART;
>  }
>  
> -static const char *hwsim_chantypes[] = {
> -	[NL80211_CHAN_NO_HT] = "noht",
> -	[NL80211_CHAN_HT20] = "ht20",
> -	[NL80211_CHAN_HT40MINUS] = "ht40-",
> -	[NL80211_CHAN_HT40PLUS] = "ht40+",
> +static const char *hwsim_chanwidth[] = {
> +	[NL80211_CHAN_WIDTH_20_NOHT] = "noht",
> +	[NL80211_CHAN_WIDTH_20] = "ht20",
> +	[NL80211_CHAN_WIDTH_40] = "ht40",
> +	[NL80211_CHAN_WIDTH_80] = "ht80",
> +	[NL80211_CHAN_WIDTH_80P80] = "ht80p80",
> +	[NL80211_CHAN_WIDTH_160] = "ht160",
>  };
>  
>  static int mac80211_hwsim_config(struct ieee80211_hw *hw, u32 changed)
> @@ -1074,18 +1076,31 @@ static int mac80211_hwsim_config(struct ieee80211_hw *hw, u32 changed)
>  		[IEEE80211_SMPS_DYNAMIC] = "dynamic",
>  	};
>  
> -	wiphy_debug(hw->wiphy,
> -		    "%s (freq=%d/%s idle=%d ps=%d smps=%s)\n",
> -		    __func__,
> -		    conf->channel ? conf->channel->center_freq : 0,
> -		    hwsim_chantypes[conf->channel_type],
> -		    !!(conf->flags & IEEE80211_CONF_IDLE),
> -		    !!(conf->flags & IEEE80211_CONF_PS),
> -		    smps_modes[conf->smps_mode]);
> +	if (conf->chandef.chan)
> +		wiphy_debug(hw->wiphy,
> +			    "%s (freq=%d(%d - %d)/%s idle=%d ps=%d smps=%s)\n",
> +			    __func__,
> +			    conf->chandef.chan->center_freq,
> +			    conf->chandef.width > NL80211_CHAN_WIDTH_20 ?
> +			    conf->chandef.center_freq1 : 0,
> +			    conf->chandef.width > NL80211_CHAN_WIDTH_40 ?
> +			    conf->chandef.center_freq2 : 0,
> +			    hwsim_chanwidth[conf->chandef.width],
> +			    !!(conf->flags & IEEE80211_CONF_IDLE),
> +			    !!(conf->flags & IEEE80211_CONF_PS),
> +			    smps_modes[conf->smps_mode]);
> +	else
> +		wiphy_debug(hw->wiphy,
> +			    "%s (freq=0/%s idle=%d ps=%d smps=%s)\n",
> +			    __func__,
> +			    hwsim_chanwidth[conf->chandef.width],
No chan, so I'll drop the channel width.

> +			    !!(conf->flags & IEEE80211_CONF_IDLE),
> +			    !!(conf->flags & IEEE80211_CONF_PS),
> +			    smps_modes[conf->smps_mode]);
>  
>  	data->idle = !!(conf->flags & IEEE80211_CONF_IDLE);
>  
> -	data->channel = conf->channel;
> +	data->channel = conf->chandef.chan;
>  
>  	WARN_ON(data->channel && channels > 1);
>  
> @@ -1271,7 +1286,7 @@ static int mac80211_hwsim_get_survey(
>  		return -ENOENT;
>  
>  	/* Current channel */
> -	survey->channel = conf->channel;
> +	survey->channel = conf->chandef.chan;
>  
>  	/*
>  	 * Magically conjured noise level --- this is only ok for simulated hardware.
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 8c0ca11..2eb3de9 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -1001,8 +1001,7 @@ struct ieee80211_conf {
>  
>  	u8 long_frame_max_tx_count, short_frame_max_tx_count;
>  
> -	struct ieee80211_channel *channel;
> -	enum nl80211_channel_type channel_type;
> +	struct cfg80211_chan_def chandef;
>  	bool radar_enabled;
>  	enum ieee80211_smps_mode smps_mode;
>  };
> @@ -4204,31 +4203,33 @@ void ieee80211_rate_control_unregister(struct rate_control_ops *ops);
>  static inline bool
>  conf_is_ht20(struct ieee80211_conf *conf)
>  {
> -	return conf->channel_type == NL80211_CHAN_HT20;
> +	return conf->chandef.width == NL80211_CHAN_WIDTH_20;
>  }
>  
>  static inline bool
>  conf_is_ht40_minus(struct ieee80211_conf *conf)
>  {
> -	return conf->channel_type == NL80211_CHAN_HT40MINUS;
> +	return conf->chandef.width == NL80211_CHAN_WIDTH_40 &&
> +	       conf->chandef.center_freq1 < conf->chandef.chan->center_freq;
>  }
>  
>  static inline bool
>  conf_is_ht40_plus(struct ieee80211_conf *conf)
>  {
> -	return conf->channel_type == NL80211_CHAN_HT40PLUS;
> +	return conf->chandef.width == NL80211_CHAN_WIDTH_40 &&
> +	       conf->chandef.center_freq1 > conf->chandef.chan->center_freq;
>  }
>  
>  static inline bool
>  conf_is_ht40(struct ieee80211_conf *conf)
>  {
> -	return conf_is_ht40_minus(conf) || conf_is_ht40_plus(conf);
> +	return conf->chandef.width == NL80211_CHAN_WIDTH_40;
>  }
>  
>  static inline bool
>  conf_is_ht(struct ieee80211_conf *conf)
>  {
> -	return conf->channel_type != NL80211_CHAN_NO_HT;
> +	return conf->chandef.width != NL80211_CHAN_WIDTH_20_NOHT;
>  }
>  
>  static inline enum nl80211_iftype
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index e5c1441..d7924b0 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -805,8 +805,7 @@ static int ieee80211_set_monitor_channel(struct wiphy *wiphy,
>  					IEEE80211_CHANCTX_EXCLUSIVE);
>  		}
>  	} else if (local->open_count == local->monitors) {
> -		local->_oper_channel = chandef->chan;
> -		local->_oper_channel_type = cfg80211_get_chandef_type(chandef);
> +		memcpy(&local->oper_chandef, chandef, sizeof(local->oper_chandef));
>  		ieee80211_hw_config(local, 0);
>  	}
>  
> @@ -3360,9 +3359,7 @@ static int ieee80211_cfg_get_channel(struct wiphy *wiphy,
>  		if (local->use_chanctx)
>  			*chandef = local->monitor_chandef;
>  		else
> -			cfg80211_chandef_create(chandef,
> -						local->_oper_channel,
> -						local->_oper_channel_type);
> +			memcpy(chandef, &local->oper_chandef, sizeof(chandef));
>  		ret = 0;
>  	}
>  	rcu_read_unlock();
> diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
> index 78c0d90..8b7f47a 100644
> --- a/net/mac80211/chan.c
> +++ b/net/mac80211/chan.c
> @@ -22,7 +22,9 @@ static void ieee80211_change_chanctx(struct ieee80211_local *local,
>  	drv_change_chanctx(local, ctx, IEEE80211_CHANCTX_CHANGE_WIDTH);
>  
>  	if (!local->use_chanctx) {
> -		local->_oper_channel_type = cfg80211_get_chandef_type(chandef);
> +		local->oper_chandef.width = chandef->width;
> +		local->oper_chandef.center_freq1 = chandef->center_freq1;
> +		local->oper_chandef.center_freq2 = chandef->center_freq2;
>  		ieee80211_hw_config(local, 0);
>  	}
>  }
> @@ -77,9 +79,7 @@ ieee80211_new_chanctx(struct ieee80211_local *local,
>  	ctx->mode = mode;
>  
>  	if (!local->use_chanctx) {
> -		local->_oper_channel_type =
> -			cfg80211_get_chandef_type(chandef);
> -		local->_oper_channel = chandef->chan;
> +		memcpy(&local->oper_chandef, chandef, sizeof(local->oper_chandef));
>  		ieee80211_hw_config(local, 0);
>  	} else {
>  		err = drv_add_chanctx(local, ctx);
> @@ -106,7 +106,7 @@ static void ieee80211_free_chanctx(struct ieee80211_local *local,
>  	WARN_ON_ONCE(ctx->refcount != 0);
>  
>  	if (!local->use_chanctx) {
> -		local->_oper_channel_type = NL80211_CHAN_NO_HT;
> +		local->oper_chandef.width = NL80211_CHAN_NO_HT;
>  		ieee80211_hw_config(local, 0);
>  	} else {
>  		drv_remove_chanctx(local, ctx);
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index 95beb18..b72a1d1 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -1021,9 +1021,7 @@ struct ieee80211_local {
>  	struct delayed_work scan_work;
>  	struct ieee80211_sub_if_data __rcu *scan_sdata;
>  	struct ieee80211_channel *csa_channel;
> -	/* For backward compatibility only -- do not use */
> -	struct ieee80211_channel *_oper_channel;
> -	enum nl80211_channel_type _oper_channel_type;
> +	struct cfg80211_chan_def oper_chandef;
>  
>  	/* Temporary remain-on-channel for off-channel operations */
>  	struct ieee80211_channel *tmp_channel;
> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> index eee1768..25db628 100644
> --- a/net/mac80211/main.c
> +++ b/net/mac80211/main.c
> @@ -92,47 +92,61 @@ static void ieee80211_reconfig_filter(struct work_struct *work)
>  	ieee80211_configure_filter(local);
>  }
>  
> +static int ieee80211_chandef_cmp(const struct cfg80211_chan_def *def1,
> +				 const struct cfg80211_chan_def *def2)
> +{
> +	if (def1->chan != def2->chan || def1->width != def2->width)
> +		return 1;
> +	if (def1->width < NL80211_CHAN_WIDTH_40)
> +		return 0;
> +	if (def1->width != NL80211_CHAN_WIDTH_80P80)
> +		return def1->center_freq1 != def2->center_freq1 ||
> +		       def1->center_freq2 != def2->center_freq2;
> +	return def1->center_freq1 != def2->center_freq1;
> +}
> +
>  static u32 ieee80211_hw_conf_chan(struct ieee80211_local *local)
>  {
>  	struct ieee80211_sub_if_data *sdata;
> +	struct cfg80211_chan_def chandef;
>  	struct ieee80211_channel *chan;
>  	u32 changed = 0;
>  	int power;
> -	enum nl80211_channel_type channel_type;
>  	u32 offchannel_flag;
>  
>  	offchannel_flag = local->hw.conf.flags & IEEE80211_CONF_OFFCHANNEL;
> +
>  	if (local->scan_channel) {
> -		chan = local->scan_channel;
> +		chandef.chan = local->scan_channel;
>  		/* If scanning on oper channel, use whatever channel-type
>  		 * is currently in use.
>  		 */
> -		if (chan == local->_oper_channel)
> -			channel_type = local->_oper_channel_type;
> +		if (chandef.chan == local->oper_chandef.chan)
> +			memcpy(&chandef, &local->oper_chandef, sizeof(chandef));
>  		else
> -			channel_type = NL80211_CHAN_NO_HT;
> +			chandef.width = NL80211_CHAN_WIDTH_20_NOHT;
>  	} else if (local->tmp_channel) {
> -		chan = local->tmp_channel;
> -		channel_type = NL80211_CHAN_NO_HT;
> +		chandef.chan = local->tmp_channel;
> +		chandef.width = NL80211_CHAN_WIDTH_20_NOHT;
>  	} else {
> -		chan = local->_oper_channel;
> -		channel_type = local->_oper_channel_type;
> +		memcpy(&chandef, &local->oper_chandef, sizeof(chandef));
>  	}
>  
> -	if (chan != local->_oper_channel ||
> -	    channel_type != local->_oper_channel_type)
> +	if (ieee80211_chandef_cmp(&chandef, &local->oper_chandef))
>  		local->hw.conf.flags |= IEEE80211_CONF_OFFCHANNEL;
>  	else
>  		local->hw.conf.flags &= ~IEEE80211_CONF_OFFCHANNEL;
>  
>  	offchannel_flag ^= local->hw.conf.flags & IEEE80211_CONF_OFFCHANNEL;
>  
> -	if (offchannel_flag || chan != local->hw.conf.channel ||
> -	    channel_type != local->hw.conf.channel_type) {
> -		local->hw.conf.channel = chan;
> -		local->hw.conf.channel_type = channel_type;
> +	if (offchannel_flag ||
> +	    ieee80211_chandef_cmp(&local->hw.conf.chandef,
> +				  &local->oper_chandef)) {
> +		memcpy(&local->hw.conf.chandef, &chandef,
> +		       sizeof(local->hw.conf.chandef));
>  		changed |= IEEE80211_CONF_CHANGE_CHANNEL;
>  	}
> +	chan = chandef.chan;
>  
drop chan, it is only used once afterwards for txpower

>  	if (!conf_is_ht(&local->hw.conf)) {
>  		/*
> @@ -738,11 +752,11 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
>  		sband = local->hw.wiphy->bands[band];
>  		if (!sband)
>  			continue;
> -		if (!local->use_chanctx && !local->_oper_channel) {
> +		if (!local->use_chanctx && !local->oper_chandef.chan) {
>  			/* init channel we're on */
> -			local->hw.conf.channel =
> -			local->_oper_channel = &sband->channels[0];
> -			local->hw.conf.channel_type = NL80211_CHAN_NO_HT;
> +			local->hw.conf.chandef.chan =
> +			local->oper_chandef.chan = &sband->channels[0];
> +			local->hw.conf.chandef.width = NL80211_CHAN_NO_HT;
s/NL80211_CHAN_NO_HT/NL80211_CHAN_WIDTH_20_NOHT

>  		}
>  		cfg80211_chandef_create(&local->monitor_chandef,
>  					&sband->channels[0],
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index fdc06e3..3a98660 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -994,18 +994,18 @@ static void ieee80211_chswitch_work(struct work_struct *work)
>  	if (!ifmgd->associated)
>  		goto out;
>  
> -	sdata->local->_oper_channel = sdata->local->csa_channel;
> +	sdata->local->oper_chandef.chan = sdata->local->csa_channel;
Here I'll adjust oper_chandef.center_freq{1,2} with 
csa_channel->center_freq - _oper_chandef.chan->center_freq

>  	if (!sdata->local->ops->channel_switch) {
>  		/* call "hw_config" only if doing sw channel switch */
>  		ieee80211_hw_config(sdata->local,
>  			IEEE80211_CONF_CHANGE_CHANNEL);
>  	} else {
>  		/* update the device channel directly */
> -		sdata->local->hw.conf.channel = sdata->local->_oper_channel;
> +		sdata->local->hw.conf.chandef.chan = sdata->local->oper_chandef.chan;
And here I'll assign the chandef instead of the chan
 
Karl

Re: [RFC 1/2] mac80211: Use a cfg80211_chan_def in ieee80211_hw_conf_chan

[Karl Beldan]

On Mon, Mar 18, 2013 at 08:25:10PM +0100, Johannes Berg wrote:
> On Sun, 2013-03-17 at 21:30 +0100, Karl Beldan wrote:
> 
> > +	if (conf->chandef.chan)
> 
> > +	else
> > +		wiphy_debug(hw->wiphy,
> > +			    "%s (freq=0/%s idle=%d ps=%d smps=%s)\n",
> 
> I don't think the else should be allowed to happen.
> 
> > --- a/net/mac80211/cfg.c
> > +++ b/net/mac80211/cfg.c
> > @@ -805,8 +805,7 @@ static int ieee80211_set_monitor_channel(struct wiphy *wiphy,
> >  					IEEE80211_CHANCTX_EXCLUSIVE);
> >  		}
> >  	} else if (local->open_count == local->monitors) {
> > -		local->_oper_channel = chandef->chan;
> > -		local->_oper_channel_type = cfg80211_get_chandef_type(chandef);
> > +		memcpy(&local->oper_chandef, chandef, sizeof(local->oper_chandef));
> 
> Somehow I'd prefer an assignment:
> 
> local->oper_chandef = *chandef;
> 
> I know it'll just be a memcpy() again, but ... reads nicer, imho.
> 
> > +			memcpy(chandef, &local->oper_chandef, sizeof(chandef));
> 
> ditto
> 
> > @@ -22,7 +22,9 @@ static void ieee80211_change_chanctx(struct ieee80211_local *local,
> >  	drv_change_chanctx(local, ctx, IEEE80211_CHANCTX_CHANGE_WIDTH);
> >  
> >  	if (!local->use_chanctx) {
> > -		local->_oper_channel_type = cfg80211_get_chandef_type(chandef);
> > +		local->oper_chandef.width = chandef->width;
> > +		local->oper_chandef.center_freq1 = chandef->center_freq1;
> > +		local->oper_chandef.center_freq2 = chandef->center_freq2;
> 
> Why not assign all here as well? Is the channel pointer itself special?
> 
> > -	/* For backward compatibility only -- do not use */
> > -	struct ieee80211_channel *_oper_channel;
> > -	enum nl80211_channel_type _oper_channel_type;
> > +	struct cfg80211_chan_def oper_chandef;
> 
> You shouldn't remove the comment, and I think you should also keep the
> leading underscore -- this is still for backward compatibility with
> non-chanctx drivers only, and mac80211 internally must use chanctxes all
> the way except in SW scan and SW roc code.
>  
> > +static int ieee80211_chandef_cmp(const struct cfg80211_chan_def *def1,
> > +				 const struct cfg80211_chan_def *def2)
> 
> indentation
> 
> However this already exists in cfg80211.h --
> cfg80211_chandef_identical().
> 
> >  static u32 ieee80211_hw_conf_chan(struct ieee80211_local *local)
> >  {
> >  	struct ieee80211_sub_if_data *sdata;
> > +	struct cfg80211_chan_def chandef;
> 
> better initialize this: struct ... chandef = {};
> 
Ok, even though I have done it in the v2 I think I get why now (after
the comment of Mahesh on center_freq2 != 0).
And the reason I'd see is that you take care of zeroing center_freq{1,2}
when they are not used (i.e wrt to the width), and the code behaves
accordingly ?
Then, indeed I could obviously drop cfg80211_chandef_identical for
cfg80211_chandef_identical.
 
Karl

Re: [RFC 1/2] mac80211: Use a cfg80211_chan_def in ieee80211_hw_conf_chan

[Johannes Berg]

On Mon, 2013-03-18 at 22:38 +0100, Karl Beldan wrote:

> > However this already exists in cfg80211.h --
> > cfg80211_chandef_identical().
> > 
> cfg80211_chandef_identical() will compare the whole chandef whatever the
> channel width.

As it should, a chandef where the unused fields are random isn't even
valid and will cause warnings left and right. That's a deliberate design
choice to simplify usage.

johannes

Re: [RFC 1/2] mac80211: Use a cfg80211_chan_def in ieee80211_hw_conf_chan

[Karl Beldan]

On Tue, Mar 19, 2013 at 07:24:51PM +0100, Johannes Berg wrote:
> On Mon, 2013-03-18 at 22:38 +0100, Karl Beldan wrote:
> 
> > > However this already exists in cfg80211.h --
> > > cfg80211_chandef_identical().
> > > 
> > cfg80211_chandef_identical() will compare the whole chandef whatever the
> > channel width.
> 
> As it should, a chandef where the unused fields are random isn't even
> valid and will cause warnings left and right. That's a deliberate design
> choice to simplify usage.
> 
Yes, that's what I figured _afterwards_ (c.f todays emails) ;)
 
Karl