[PATCH v2 1/2] mac80211: add offload channel switch support

[PATCH v2 1/2] mac80211: add offload channel switch support

[wey-yi.w.guy]

From: Wey-Yi Guy <wey-yi.w.guy@intel.com>

Adding support to offload the channel switch operation to driver if
driver can support the function.

The original approach, mac80211 utilize the mac_config callback function
and set the CHANNEL_CHANGE flag which is difficult to separate the true
channel switch request from all the other channel changes condition;
with this offload approach, driver has more control on how to handle the
channel switch request from AP, also can provide more accurate timing
calculation

The drivers like to support the channel switch offloading function will have
to provide the mactime information (at least for mgmt and action frames) 

Signed-off-by: Wey-Yi Guy <wey-yi.w.guy@intel.com>
---
v2: use "mactime" instead of timestamp in the frame body, the driver has to provide the mactime if need to support the channel switch function
---
 include/net/mac80211.h      |   32 ++++++++++++++++++++++++++++
 net/mac80211/driver-ops.h   |   11 +++++++++
 net/mac80211/driver-trace.h |   49 +++++++++++++++++++++++++++++++++++++++++++
 net/mac80211/ieee80211_i.h  |    3 +-
 net/mac80211/mlme.c         |   48 ++++++++++++++++++++++++++++++++++++++---
 5 files changed, 138 insertions(+), 5 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 2879c8e..dc5ae23 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -708,6 +708,26 @@ struct ieee80211_conf {
 };
 
 /**
+ * struct ieee80211_channel_switch - holds the channel switch data
+ *
+ * The information provided in this structure is required for channel switch
+ * operation.
+ *
+ * @timestamp: value in microseconds of the 64-bit Time Synchronization
+ *	Function (TSF) timer when the channel switch ie received.
+ * @block_tx: set to true if no more future frames transmission within the BSS
+ *	until the scheduled channel switch
+ * @channel: pointer to ieee80211_channel contain then new channel information
+ * @count: specified the number of TBTT's until the channel switch event.
+ */
+struct ieee80211_channel_switch {
+	u64 timestamp;
+	bool block_tx;
+	struct ieee80211_channel *channel;
+	u8 count;
+};
+
+/**
  * struct ieee80211_vif - per-interface data
  *
  * Data in this structure is continually present for driver
@@ -1694,6 +1714,8 @@ struct ieee80211_ops {
 	int (*testmode_cmd)(struct ieee80211_hw *hw, void *data, int len);
 #endif
 	void (*flush)(struct ieee80211_hw *hw, bool drop);
+	void (*channel_switch)(struct ieee80211_hw *hw,
+			       struct ieee80211_channel_switch *ch_switch);
 };
 
 /**
@@ -2444,6 +2466,16 @@ void ieee80211_cqm_rssi_notify(struct ieee80211_vif *vif,
 			       enum nl80211_cqm_rssi_threshold_event rssi_event,
 			       gfp_t gfp);
 
+/**
+ * ieee80211_chswitch_done - Complete channel switch process
+ * @vif: &struct ieee80211_vif pointer from the add_interface callback.
+ * @is_seccess: make the channel switch successful or not
+ *
+ * Complete the channel switch post-process: set the new operational channel
+ * and wake up the suspended queues.
+ */
+void ieee80211_chswitch_done(struct ieee80211_vif *vif, bool is_success);
+
 /* Rate control API */
 
 /**
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 997008e..5662bb5 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -373,4 +373,15 @@ static inline void drv_flush(struct ieee80211_local *local, bool drop)
 	if (local->ops->flush)
 		local->ops->flush(&local->hw, drop);
 }
+
+static inline void drv_channel_switch(struct ieee80211_local *local,
+				     struct ieee80211_channel_switch *ch_switch)
+{
+	might_sleep();
+
+	local->ops->channel_switch(&local->hw, ch_switch);
+
+	trace_drv_channel_switch(local, ch_switch);
+}
+
 #endif /* __MAC80211_DRIVER_OPS */
diff --git a/net/mac80211/driver-trace.h b/net/mac80211/driver-trace.h
index ce734b5..cd1ae9f 100644
--- a/net/mac80211/driver-trace.h
+++ b/net/mac80211/driver-trace.h
@@ -774,6 +774,34 @@ TRACE_EVENT(drv_flush,
 	)
 );
 
+TRACE_EVENT(drv_channel_switch,
+	TP_PROTO(struct ieee80211_local *local,
+		 struct ieee80211_channel_switch *ch_switch),
+
+	TP_ARGS(local, ch_switch),
+
+	TP_STRUCT__entry(
+		LOCAL_ENTRY
+		__field(u64, timestamp)
+		__field(bool, block_tx)
+		__field(u16, freq)
+		__field(u8, count)
+	),
+
+	TP_fast_assign(
+		LOCAL_ASSIGN;
+		__entry->timestamp = ch_switch->timestamp;
+		__entry->block_tx = ch_switch->block_tx;
+		__entry->freq = ch_switch->channel->center_freq;
+		__entry->count = ch_switch->count;
+	),
+
+	TP_printk(
+		LOCAL_PR_FMT " new freq:%u count:%d",
+		LOCAL_PR_ARG, __entry->freq, __entry->count
+	)
+);
+
 /*
  * Tracing for API calls that drivers call.
  */
@@ -992,6 +1020,27 @@ TRACE_EVENT(api_sta_block_awake,
 	)
 );
 
+TRACE_EVENT(api_chswitch_done,
+	TP_PROTO(struct ieee80211_sub_if_data *sdata, bool is_success),
+
+	TP_ARGS(sdata, is_success),
+
+	TP_STRUCT__entry(
+		VIF_ENTRY
+		__field(bool, is_success)
+	),
+
+	TP_fast_assign(
+		VIF_ASSIGN;
+		__entry->is_success = is_success;
+	),
+
+	TP_printk(
+		VIF_PR_FMT " chswitch:%d",
+		VIF_PR_ARG, __entry->is_success
+	)
+);
+
 /*
  * Tracing for internal functions
  * (which may also be called in response to driver calls)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 4e73660..be9dda9 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -999,7 +999,8 @@ int ieee80211_max_network_latency(struct notifier_block *nb,
 				  unsigned long data, void *dummy);
 void ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 				      struct ieee80211_channel_sw_ie *sw_elem,
-				      struct ieee80211_bss *bss);
+				      struct ieee80211_bss *bss,
+				      u64 timestamp);
 void ieee80211_sta_quiesce(struct ieee80211_sub_if_data *sdata);
 void ieee80211_sta_restart(struct ieee80211_sub_if_data *sdata);
 
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 358226f..19b53a8 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -340,7 +340,11 @@ static void ieee80211_chswitch_work(struct work_struct *work)
 		goto out;
 
 	sdata->local->oper_channel = sdata->local->csa_channel;
-	ieee80211_hw_config(sdata->local, IEEE80211_CONF_CHANGE_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);
+	}
 
 	/* XXX: shouldn't really modify cfg80211-owned data! */
 	ifmgd->associated->channel = sdata->local->oper_channel;
@@ -352,6 +356,22 @@ static void ieee80211_chswitch_work(struct work_struct *work)
 	mutex_unlock(&ifmgd->mtx);
 }
 
+void ieee80211_chswitch_done(struct ieee80211_vif *vif, bool is_success)
+{
+	struct ieee80211_sub_if_data *sdata;
+	struct ieee80211_if_managed *ifmgd;
+
+	sdata = vif_to_sdata(vif);
+	ifmgd = &sdata->u.mgd;
+
+	trace_api_chswitch_done(sdata, is_success);
+	if (!is_success)
+		sdata->local->csa_channel = sdata->local->oper_channel;
+
+	ieee80211_queue_work(&sdata->local->hw, &ifmgd->chswitch_work);
+}
+EXPORT_SYMBOL(ieee80211_chswitch_done);
+
 static void ieee80211_chswitch_timer(unsigned long data)
 {
 	struct ieee80211_sub_if_data *sdata =
@@ -368,7 +388,8 @@ static void ieee80211_chswitch_timer(unsigned long data)
 
 void ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 				      struct ieee80211_channel_sw_ie *sw_elem,
-				      struct ieee80211_bss *bss)
+				      struct ieee80211_bss *bss,
+				      u64 timestamp)
 {
 	struct cfg80211_bss *cbss =
 		container_of((void *)bss, struct cfg80211_bss, priv);
@@ -396,6 +417,23 @@ void ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 
 	sdata->local->csa_channel = new_ch;
 
+	if (sdata->local->ops->channel_switch) {
+		/* use driver's channel switch callback */
+		struct ieee80211_channel_switch ch_switch;
+		memset(&ch_switch, 0, sizeof(ch_switch));
+		ch_switch.timestamp = timestamp;
+		if (sw_elem->mode) {
+			ch_switch.block_tx = true;
+			ieee80211_stop_queues_by_reason(&sdata->local->hw,
+					IEEE80211_QUEUE_STOP_REASON_CSA);
+		}
+		ch_switch.channel = new_ch;
+		ch_switch.count = sw_elem->count;
+		ifmgd->flags |= IEEE80211_STA_CSA_RECEIVED;
+		drv_channel_switch(sdata->local, &ch_switch);
+		return;
+	}
+	/* mac80211 handle channel switch */
 	if (sw_elem->count <= 1) {
 		ieee80211_queue_work(&sdata->local->hw, &ifmgd->chswitch_work);
 	} else {
@@ -1315,7 +1353,8 @@ static void ieee80211_rx_bss_info(struct ieee80211_sub_if_data *sdata,
 							ETH_ALEN) == 0)) {
 		struct ieee80211_channel_sw_ie *sw_elem =
 			(struct ieee80211_channel_sw_ie *)elems->ch_switch_elem;
-		ieee80211_sta_process_chanswitch(sdata, sw_elem, bss);
+		ieee80211_sta_process_chanswitch(sdata, sw_elem,
+						 bss, rx_status->mactime);
 	}
 }
 
@@ -1647,7 +1686,8 @@ static void ieee80211_sta_rx_queued_mgmt(struct ieee80211_sub_if_data *sdata,
 
 			ieee80211_sta_process_chanswitch(sdata,
 					&mgmt->u.action.u.chan_switch.sw_elem,
-					(void *)ifmgd->associated->priv);
+					(void *)ifmgd->associated->priv,
+					rx_status->mactime);
 			break;
 		}
 		mutex_unlock(&ifmgd->mtx);
-- 
1.5.6.3

Re: [PATCH v2 1/2] mac80211: add offload channel switch support

[Luis R. Rodriguez]

On Thu, May 6, 2010 at 8:25 AM,  <wey-yi.w.guy@intel.com> wrote:
> From: Wey-Yi Guy <wey-yi.w.guy@intel.com>
>
> Adding support to offload the channel switch operation to driver if
> driver can support the function.

Can you please reword this a little, maybe, "This adds support for a
driver specific channel switch callback to be used by drivers which
might require a finer grain control of the operation, typically if you
have a respective firmware API call".

> The original approach, mac80211 utilize the mac_config callback function

Do you mean the mac80211 config() callback?

> and set the CHANNEL_CHANGE flag which is difficult to separate the true
> channel switch request from all the other channel changes condition;

Good point, but why is this needed? More below.

> with this offload approach, driver has more control on how to handle the
> channel switch request from AP, also can provide more accurate timing
> calculation

Is the current timing insufficient, and if so can you provide more
details. If the real reason for this callback is not timing
considerations is the real reason a firmware API thing? If so it
doesn't hurt to just say that to avoid confusing developer in deciding
which approach to take.

> The drivers like to support the channel switch offloading function

Maybe: "The drivers that require a dedicated channel switch callback"...

> will have
> to provide the mactime information (at least for mgmt and action frames)

Might be good to specify why, or at least in the documentation code below.

> Signed-off-by: Wey-Yi Guy <wey-yi.w.guy@intel.com>
> ---
> v2: use "mactime" instead of timestamp in the frame body, the driver has to provide the mactime if need to support the channel switch function
> ---
>  include/net/mac80211.h      |   32 ++++++++++++++++++++++++++++
>  net/mac80211/driver-ops.h   |   11 +++++++++
>  net/mac80211/driver-trace.h |   49 +++++++++++++++++++++++++++++++++++++++++++
>  net/mac80211/ieee80211_i.h  |    3 +-
>  net/mac80211/mlme.c         |   48 ++++++++++++++++++++++++++++++++++++++---
>  5 files changed, 138 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 2879c8e..dc5ae23 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -708,6 +708,26 @@ struct ieee80211_conf {
>  };
>
>  /**
> + * struct ieee80211_channel_switch - holds the channel switch data
> + *
> + * The information provided in this structure is required for channel switch
> + * operation.
> + *
> + * @timestamp: value in microseconds of the 64-bit Time Synchronization
> + *     Function (TSF) timer when the channel switch ie received.

Might be good to indicate this is derived from the rx status mactime
and the beacon interval from the BSS.

> + * @block_tx: set to true if no more future frames transmission within the BSS
> + *     until the scheduled channel switch
> + * @channel: pointer to ieee80211_channel contain then new channel information
> + * @count: specified the number of TBTT's until the channel switch event.
> + */
> +struct ieee80211_channel_switch {
> +       u64 timestamp;
> +       bool block_tx;
> +       struct ieee80211_channel *channel;
> +       u8 count;
> +};
> +
> +/**
>  * struct ieee80211_vif - per-interface data
>  *
>  * Data in this structure is continually present for driver
> @@ -1694,6 +1714,8 @@ struct ieee80211_ops {
>        int (*testmode_cmd)(struct ieee80211_hw *hw, void *data, int len);
>  #endif
>        void (*flush)(struct ieee80211_hw *hw, bool drop);
> +       void (*channel_switch)(struct ieee80211_hw *hw,
> +                              struct ieee80211_channel_switch *ch_switch);
>  };
>
>  /**
> @@ -2444,6 +2466,16 @@ void ieee80211_cqm_rssi_notify(struct ieee80211_vif *vif,
>                               enum nl80211_cqm_rssi_threshold_event rssi_event,
>                               gfp_t gfp);
>
> +/**
> + * ieee80211_chswitch_done - Complete channel switch process
> + * @vif: &struct ieee80211_vif pointer from the add_interface callback.
> + * @is_seccess: make the channel switch successful or not

Typo, is_seccess, not success. Also, I don't get what this is for, can
you elaborate?

> + *
> + * Complete the channel switch post-process: set the new operational channel
> + * and wake up the suspended queues.

I don't get it.. who calls this and why, under what conditions?

If drivers have to call this once they are done with the channel
switch operation callback then you might want to specify that and/or
check for the existance of the op upon it being called and warn if it
is not.

> + */
> +void ieee80211_chswitch_done(struct ieee80211_vif *vif, bool is_success);
> +
>  /* Rate control API */
>
>  /**
> diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
> index 997008e..5662bb5 100644
> --- a/net/mac80211/driver-ops.h
> +++ b/net/mac80211/driver-ops.h
> @@ -373,4 +373,15 @@ static inline void drv_flush(struct ieee80211_local *local, bool drop)
>        if (local->ops->flush)
>                local->ops->flush(&local->hw, drop);
>  }
> +
> +static inline void drv_channel_switch(struct ieee80211_local *local,
> +                                    struct ieee80211_channel_switch *ch_switch)
> +{
> +       might_sleep();
> +
> +       local->ops->channel_switch(&local->hw, ch_switch);
> +
> +       trace_drv_channel_switch(local, ch_switch);
> +}
> +
>  #endif /* __MAC80211_DRIVER_OPS */
> diff --git a/net/mac80211/driver-trace.h b/net/mac80211/driver-trace.h
> index ce734b5..cd1ae9f 100644
> --- a/net/mac80211/driver-trace.h
> +++ b/net/mac80211/driver-trace.h
> @@ -774,6 +774,34 @@ TRACE_EVENT(drv_flush,
>        )
>  );
>
> +TRACE_EVENT(drv_channel_switch,
> +       TP_PROTO(struct ieee80211_local *local,
> +                struct ieee80211_channel_switch *ch_switch),
> +
> +       TP_ARGS(local, ch_switch),
> +
> +       TP_STRUCT__entry(
> +               LOCAL_ENTRY
> +               __field(u64, timestamp)
> +               __field(bool, block_tx)
> +               __field(u16, freq)
> +               __field(u8, count)
> +       ),
> +
> +       TP_fast_assign(
> +               LOCAL_ASSIGN;
> +               __entry->timestamp = ch_switch->timestamp;
> +               __entry->block_tx = ch_switch->block_tx;
> +               __entry->freq = ch_switch->channel->center_freq;
> +               __entry->count = ch_switch->count;
> +       ),
> +
> +       TP_printk(
> +               LOCAL_PR_FMT " new freq:%u count:%d",
> +               LOCAL_PR_ARG, __entry->freq, __entry->count
> +       )
> +);
> +
>  /*
>  * Tracing for API calls that drivers call.
>  */
> @@ -992,6 +1020,27 @@ TRACE_EVENT(api_sta_block_awake,
>        )
>  );
>
> +TRACE_EVENT(api_chswitch_done,
> +       TP_PROTO(struct ieee80211_sub_if_data *sdata, bool is_success),
> +
> +       TP_ARGS(sdata, is_success),
> +
> +       TP_STRUCT__entry(
> +               VIF_ENTRY
> +               __field(bool, is_success)
> +       ),
> +
> +       TP_fast_assign(
> +               VIF_ASSIGN;
> +               __entry->is_success = is_success;
> +       ),
> +
> +       TP_printk(
> +               VIF_PR_FMT " chswitch:%d",
> +               VIF_PR_ARG, __entry->is_success
> +       )
> +);
> +
>  /*
>  * Tracing for internal functions
>  * (which may also be called in response to driver calls)
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index 4e73660..be9dda9 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -999,7 +999,8 @@ int ieee80211_max_network_latency(struct notifier_block *nb,
>                                  unsigned long data, void *dummy);
>  void ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
>                                      struct ieee80211_channel_sw_ie *sw_elem,
> -                                     struct ieee80211_bss *bss);
> +                                     struct ieee80211_bss *bss,
> +                                     u64 timestamp);
>  void ieee80211_sta_quiesce(struct ieee80211_sub_if_data *sdata);
>  void ieee80211_sta_restart(struct ieee80211_sub_if_data *sdata);
>
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index 358226f..19b53a8 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -340,7 +340,11 @@ static void ieee80211_chswitch_work(struct work_struct *work)
>                goto out;
>
>        sdata->local->oper_channel = sdata->local->csa_channel;

If the new driver callback for channel switch failed this operation
channel is still being set to the csa_channel. This *only* works
because on the channels witch done routine you write below you
overwrite the csa_channel variable to the existing operation channel.
This seem rather hackish... if the channel switch callback failed why
would we call the work to do the channel switch?

> -       ieee80211_hw_config(sdata->local, IEEE80211_CONF_CHANGE_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);
> +       }

Notice how the existing implementation also addresses quiescing in the
timer, how will you address this with this new driver API?

>        /* XXX: shouldn't really modify cfg80211-owned data! */
>        ifmgd->associated->channel = sdata->local->oper_channel;
> @@ -352,6 +356,22 @@ static void ieee80211_chswitch_work(struct work_struct *work)
>        mutex_unlock(&ifmgd->mtx);
>  }
>
> +void ieee80211_chswitch_done(struct ieee80211_vif *vif, bool is_success)
> +{
> +       struct ieee80211_sub_if_data *sdata;
> +       struct ieee80211_if_managed *ifmgd;
> +
> +       sdata = vif_to_sdata(vif);
> +       ifmgd = &sdata->u.mgd;
> +
> +       trace_api_chswitch_done(sdata, is_success);
> +       if (!is_success)
> +               sdata->local->csa_channel = sdata->local->oper_channel;

If this routine is to be called by drivers once they complete the
channel switch operation why do we continue by queuing the channel
switch work below?

> +
> +       ieee80211_queue_work(&sdata->local->hw, &ifmgd->chswitch_work);
> +}
> +EXPORT_SYMBOL(ieee80211_chswitch_done);
> +
>  static void ieee80211_chswitch_timer(unsigned long data)
>  {
>        struct ieee80211_sub_if_data *sdata =
> @@ -368,7 +388,8 @@ static void ieee80211_chswitch_timer(unsigned long data)
>
>  void ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
>                                      struct ieee80211_channel_sw_ie *sw_elem,
> -                                     struct ieee80211_bss *bss)
> +                                     struct ieee80211_bss *bss,
> +                                     u64 timestamp)
>  {
>        struct cfg80211_bss *cbss =
>                container_of((void *)bss, struct cfg80211_bss, priv);
> @@ -396,6 +417,23 @@ void ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
>
>        sdata->local->csa_channel = new_ch;
>
> +       if (sdata->local->ops->channel_switch) {
> +               /* use driver's channel switch callback */
> +               struct ieee80211_channel_switch ch_switch;
> +               memset(&ch_switch, 0, sizeof(ch_switch));
> +               ch_switch.timestamp = timestamp;

If timestamp is 0 (when the driver does not set it) then this is all
busted and I can see a few bug reports coming out due to it. These bug
reports could be avoided if you check for the timestamp to be
reasonable here and fail otherwise, in fact if the we know the channel
switch operation is busted we may just want to disassociate given the
DFS considerations are sensitive. What do you think?

> +               if (sw_elem->mode) {
> +                       ch_switch.block_tx = true;
> +                       ieee80211_stop_queues_by_reason(&sdata->local->hw,
> +                                       IEEE80211_QUEUE_STOP_REASON_CSA);
> +               }
> +               ch_switch.channel = new_ch;
> +               ch_switch.count = sw_elem->count;
> +               ifmgd->flags |= IEEE80211_STA_CSA_RECEIVED;
> +               drv_channel_switch(sdata->local, &ch_switch);
> +               return;
> +       }
> +       /* mac80211 handle channel switch */
>        if (sw_elem->count <= 1) {
>                ieee80211_queue_work(&sdata->local->hw, &ifmgd->chswitch_work);
>        } else {
> @@ -1315,7 +1353,8 @@ static void ieee80211_rx_bss_info(struct ieee80211_sub_if_data *sdata,
>                                                        ETH_ALEN) == 0)) {
>                struct ieee80211_channel_sw_ie *sw_elem =
>                        (struct ieee80211_channel_sw_ie *)elems->ch_switch_elem;
> -               ieee80211_sta_process_chanswitch(sdata, sw_elem, bss);
> +               ieee80211_sta_process_chanswitch(sdata, sw_elem,
> +                                                bss, rx_status->mactime);
>        }
>  }
>
> @@ -1647,7 +1686,8 @@ static void ieee80211_sta_rx_queued_mgmt(struct ieee80211_sub_if_data *sdata,
>
>                        ieee80211_sta_process_chanswitch(sdata,
>                                        &mgmt->u.action.u.chan_switch.sw_elem,
> -                                       (void *)ifmgd->associated->priv);
> +                                       (void *)ifmgd->associated->priv,
> +                                       rx_status->mactime);
>                        break;
>                }
>                mutex_unlock(&ifmgd->mtx);

I'd appreciate more feedback on the why this is being done. Its not
clear to me how we are limited by the current implementation. If you
have a firmware API need that is different and I think that should be
made clear, but if you do need it we need to ensure both methods are
properly documented and their different use cases outlined. I also
think the way these method are split are rather hacky right now.

  Luis

Re: [PATCH v2 1/2] mac80211: add offload channel switch support

[Johannes Berg]

I think I'll adopt this patchset.

On Thu, 2010-05-06 at 14:58 -0700, Luis R. Rodriguez wrote:
> On Thu, May 6, 2010 at 8:25 AM,  <wey-yi.w.guy@intel.com> wrote:
> > From: Wey-Yi Guy <wey-yi.w.guy@intel.com>
> >
> > Adding support to offload the channel switch operation to driver if
> > driver can support the function.
> 
> Can you please reword this a little, maybe, "This adds support for a
> driver specific channel switch callback to be used by drivers which
> might require a finer grain control of the operation, typically if you
> have a respective firmware API call".

Easy enough.

> > The original approach, mac80211 utilize the mac_config callback function
> 
> Do you mean the mac80211 config() callback?

Yeah, mac_config (or rather iwl_mac_config) is the iwlwifi callback
function's name.

> > and set the CHANNEL_CHANGE flag which is difficult to separate the true
> > channel switch request from all the other channel changes condition;
> 
> Good point, but why is this needed? More below.

Mostly because we get much better timing with firmware assist.

> > with this offload approach, driver has more control on how to handle the
> > channel switch request from AP, also can provide more accurate timing
> > calculation
> 
> Is the current timing insufficient, and if so can you provide more
> details. If the real reason for this callback is not timing
> considerations is the real reason a firmware API thing? If so it
> doesn't hurt to just say that to avoid confusing developer in deciding
> which approach to take.

The current mac80211 approach is flawed, for various reasons which I
won't get into here, most of which we can fix. However, due to
regulatory concerns our firmware also wants to have more control, like
checking that the AP is beaconing again after a channel switch before
letting us transmit frames. Timing is obviously also a consideration,
since the firmware can re-enable transmission quickly after the channel
switch, regardless of the delay in processing the frame or the timer on
the host.

> > The drivers like to support the channel switch offloading function
> 
> Maybe: "The drivers that require a dedicated channel switch callback"...
> 
> > will have
> > to provide the mactime information (at least for mgmt and action frames)
> 
> Might be good to specify why, or at least in the documentation code below.

Actually, it's up to them. But if you implement the callback, you'll
want to know precisely when to expect the channel switch, so you'll want
to know when the frame that contained the CSA was received, which you
have to provide to mac80211 in the "mactime" rx status field.

> > + * @timestamp: value in microseconds of the 64-bit Time Synchronization
> > + *     Function (TSF) timer when the channel switch ie received.
> 
> Might be good to indicate this is derived from the rx status mactime
> and the beacon interval from the BSS.

It's not derived from the beacon interval, it's just the plain mactime
passed through. As such, where else would it come from? But yeah I guess
it could say that.

> > +/**
> > + * ieee80211_chswitch_done - Complete channel switch process
> > + * @vif: &struct ieee80211_vif pointer from the add_interface callback.
> > + * @is_seccess: make the channel switch successful or not
> 
> Typo, is_seccess, not success. Also, I don't get what this is for, can
> you elaborate?

Channel switching could fail, for instance if the AP doesn't show up on
the new channel. We don't have a way to handle that yet in mac80211, but
why not let it know.

> > + *
> > + * Complete the channel switch post-process: set the new operational channel
> > + * and wake up the suspended queues.
> 
> I don't get it.. who calls this and why, under what conditions?
> 
> If drivers have to call this once they are done with the channel
> switch operation callback then you might want to specify that and/or
> check for the existance of the op upon it being called and warn if it
> is not.

Can't check that since it's also called by mac80211 itself for software
implemented channel switch.

> > --- a/net/mac80211/mlme.c
> > +++ b/net/mac80211/mlme.c
> > @@ -340,7 +340,11 @@ static void ieee80211_chswitch_work(struct work_struct *work)
> >                goto out;
> >
> >        sdata->local->oper_channel = sdata->local->csa_channel;
> 
> If the new driver callback for channel switch failed this operation
> channel is still being set to the csa_channel. This *only* works
> because on the channels witch done routine you write below you
> overwrite the csa_channel variable to the existing operation channel.
> This seem rather hackish... if the channel switch callback failed why
> would we call the work to do the channel switch?

Not the callback failed .. the switch itself failed. Ap not showing up,
or advertising the wrong channel could be reasons.

> > -       ieee80211_hw_config(sdata->local, IEEE80211_CONF_CHANGE_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);
> > +       }
> 
> Notice how the existing implementation also addresses quiescing in the
> timer, how will you address this with this new driver API?

The timer is not started when the callback exists. It can still be
deleted even if it was never started, right? What's the question?

> > +void ieee80211_chswitch_done(struct ieee80211_vif *vif, bool is_success)
> > +{
> > +       struct ieee80211_sub_if_data *sdata;
> > +       struct ieee80211_if_managed *ifmgd;
> > +
> > +       sdata = vif_to_sdata(vif);
> > +       ifmgd = &sdata->u.mgd;
> > +
> > +       trace_api_chswitch_done(sdata, is_success);
> > +       if (!is_success)
> > +               sdata->local->csa_channel = sdata->local->oper_channel;
> 
> If this routine is to be called by drivers once they complete the
> channel switch operation why do we continue by queuing the channel
> switch work below?
> 
> > +
> > +       ieee80211_queue_work(&sdata->local->hw, &ifmgd->chswitch_work);
> > +}
> > +EXPORT_SYMBOL(ieee80211_chswitch_done);


because that will complete the channel switch. I think you're getting
confused by the name, chswitch_work should be called chswitch_done_work
but blame that on Sujith :P

> > +       if (sdata->local->ops->channel_switch) {
> > +               /* use driver's channel switch callback */
> > +               struct ieee80211_channel_switch ch_switch;
> > +               memset(&ch_switch, 0, sizeof(ch_switch));
> > +               ch_switch.timestamp = timestamp;
> 
> If timestamp is 0 (when the driver does not set it) then this is all
> busted and I can see a few bug reports coming out due to it. These bug
> reports could be avoided if you check for the timestamp to be
> reasonable here and fail otherwise, in fact if the we know the channel
> switch operation is busted we may just want to disassociate given the
> DFS considerations are sensitive. What do you think?

I don't follow? The mactime only gets back to the driver, so if the
driver didn't report it properly it will get bogus values, mac80211
won't ever care. As such, in the unlikely event that the driver doesn't
need it, it wouldn't have to provide it, so any such checking doesn't
seem proper?

> I'd appreciate more feedback on the why this is being done. Its not
> clear to me how we are limited by the current implementation. 

Ok like I said -- timing is a big thing. Regulatory enforcement in our
firmware is another.

> If you
> have a firmware API need that is different and I think that should be
> made clear, but if you do need it we need to ensure both methods are
> properly documented and their different use cases outlined. I also
> think the way these method are split are rather hacky right now.

I'm not sure how you could split it better?

I'll fix up some docs and stuff and respin.

johannes

[PATCH v3 1/2] mac80211: add offload channel switch support

[Johannes Berg]

This adds support for offloading the channel switch
operation to devices that support such, typically
by having specific firmware API for it. The reasons
for this could be that the firmware provides better
timing or that regulatory enforcement done by the
device requires special handling of CSAs.

In order to allow drivers to specify the timing to
the device, the new channel_switch callback will
pass through the received frame's mactime, where
available.

Signed-off-by: Wey-Yi Guy <wey-yi.w.guy@intel.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/mac80211.h      |   39 ++++++++++++++++++++++++++++++
 net/mac80211/driver-ops.h   |   11 ++++++++
 net/mac80211/driver-trace.h |   49 +++++++++++++++++++++++++++++++++++++
 net/mac80211/ieee80211_i.h  |    3 +-
 net/mac80211/mlme.c         |   56 +++++++++++++++++++++++++++++++++++++++---
 5 files changed, 153 insertions(+), 5 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 9448a5b..389e86a 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -712,6 +712,28 @@ struct ieee80211_conf {
 };
 
 /**
+ * struct ieee80211_channel_switch - holds the channel switch data
+ *
+ * The information provided in this structure is required for channel switch
+ * operation.
+ *
+ * @timestamp: value in microseconds of the 64-bit Time Synchronization
+ *	Function (TSF) timer when the frame containing the channel switch
+ *	announcement was received. This is simply the rx.mactime parameter
+ *	the driver passed into mac80211.
+ * @block_tx: Indicates whether transmission must be blocked before the
+ *	scheduled channel switch, as indicated by the AP.
+ * @channel: the new channel to switch to
+ * @count: the number of TBTT's until the channel switch event
+ */
+struct ieee80211_channel_switch {
+	u64 timestamp;
+	bool block_tx;
+	struct ieee80211_channel *channel;
+	u8 count;
+};
+
+/**
  * struct ieee80211_vif - per-interface data
  *
  * Data in this structure is continually present for driver
@@ -1631,6 +1653,11 @@ enum ieee80211_ampdu_mlme_action {
  * @flush: Flush all pending frames from the hardware queue, making sure
  *	that the hardware queues are empty. If the parameter @drop is set
  *	to %true, pending frames may be dropped. The callback can sleep.
+ *
+ * @channel_switch: Drivers that need (or want) to offload the channel
+ *	switch operation for CSAs received from the AP may implement this
+ *	callback. They must then call ieee80211_chswitch_done() to indicate
+ *	completion of the channel switch.
  */
 struct ieee80211_ops {
 	int (*tx)(struct ieee80211_hw *hw, struct sk_buff *skb);
@@ -1694,6 +1721,8 @@ struct ieee80211_ops {
 	int (*testmode_cmd)(struct ieee80211_hw *hw, void *data, int len);
 #endif
 	void (*flush)(struct ieee80211_hw *hw, bool drop);
+	void (*channel_switch)(struct ieee80211_hw *hw,
+			       struct ieee80211_channel_switch *ch_switch);
 };
 
 /**
@@ -2444,6 +2473,16 @@ void ieee80211_cqm_rssi_notify(struct ieee80211_vif *vif,
 			       enum nl80211_cqm_rssi_threshold_event rssi_event,
 			       gfp_t gfp);
 
+/**
+ * ieee80211_chswitch_done - Complete channel switch process
+ * @vif: &struct ieee80211_vif pointer from the add_interface callback.
+ * @success: make the channel switch successful or not
+ *
+ * Complete the channel switch post-process: set the new operational channel
+ * and wake up the suspended queues.
+ */
+void ieee80211_chswitch_done(struct ieee80211_vif *vif, bool success);
+
 /* Rate control API */
 
 /**
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 997008e..5662bb5 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -373,4 +373,15 @@ static inline void drv_flush(struct ieee80211_local *local, bool drop)
 	if (local->ops->flush)
 		local->ops->flush(&local->hw, drop);
 }
+
+static inline void drv_channel_switch(struct ieee80211_local *local,
+				     struct ieee80211_channel_switch *ch_switch)
+{
+	might_sleep();
+
+	local->ops->channel_switch(&local->hw, ch_switch);
+
+	trace_drv_channel_switch(local, ch_switch);
+}
+
 #endif /* __MAC80211_DRIVER_OPS */
diff --git a/net/mac80211/driver-trace.h b/net/mac80211/driver-trace.h
index ce734b5..6a9b234 100644
--- a/net/mac80211/driver-trace.h
+++ b/net/mac80211/driver-trace.h
@@ -774,6 +774,34 @@ TRACE_EVENT(drv_flush,
 	)
 );
 
+TRACE_EVENT(drv_channel_switch,
+	TP_PROTO(struct ieee80211_local *local,
+		 struct ieee80211_channel_switch *ch_switch),
+
+	TP_ARGS(local, ch_switch),
+
+	TP_STRUCT__entry(
+		LOCAL_ENTRY
+		__field(u64, timestamp)
+		__field(bool, block_tx)
+		__field(u16, freq)
+		__field(u8, count)
+	),
+
+	TP_fast_assign(
+		LOCAL_ASSIGN;
+		__entry->timestamp = ch_switch->timestamp;
+		__entry->block_tx = ch_switch->block_tx;
+		__entry->freq = ch_switch->channel->center_freq;
+		__entry->count = ch_switch->count;
+	),
+
+	TP_printk(
+		LOCAL_PR_FMT " new freq:%u count:%d",
+		LOCAL_PR_ARG, __entry->freq, __entry->count
+	)
+);
+
 /*
  * Tracing for API calls that drivers call.
  */
@@ -992,6 +1020,27 @@ TRACE_EVENT(api_sta_block_awake,
 	)
 );
 
+TRACE_EVENT(api_chswitch_done,
+	TP_PROTO(struct ieee80211_sub_if_data *sdata, bool success),
+
+	TP_ARGS(sdata, success),
+
+	TP_STRUCT__entry(
+		VIF_ENTRY
+		__field(bool, success)
+	),
+
+	TP_fast_assign(
+		VIF_ASSIGN;
+		__entry->success = success;
+	),
+
+	TP_printk(
+		VIF_PR_FMT " success=%d",
+		VIF_PR_ARG, __entry->success
+	)
+);
+
 /*
  * Tracing for internal functions
  * (which may also be called in response to driver calls)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 69e7f41..1c8e247 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -999,7 +999,8 @@ int ieee80211_max_network_latency(struct notifier_block *nb,
 				  unsigned long data, void *dummy);
 void ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 				      struct ieee80211_channel_sw_ie *sw_elem,
-				      struct ieee80211_bss *bss);
+				      struct ieee80211_bss *bss,
+				      u64 timestamp);
 void ieee80211_sta_quiesce(struct ieee80211_sub_if_data *sdata);
 void ieee80211_sta_restart(struct ieee80211_sub_if_data *sdata);
 
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 3093e46..b2f7f9f 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -342,7 +342,11 @@ static void ieee80211_chswitch_work(struct work_struct *work)
 		goto out;
 
 	sdata->local->oper_channel = sdata->local->csa_channel;
-	ieee80211_hw_config(sdata->local, IEEE80211_CONF_CHANGE_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);
+	}
 
 	/* XXX: shouldn't really modify cfg80211-owned data! */
 	ifmgd->associated->channel = sdata->local->oper_channel;
@@ -354,6 +358,29 @@ static void ieee80211_chswitch_work(struct work_struct *work)
 	mutex_unlock(&ifmgd->mtx);
 }
 
+void ieee80211_chswitch_done(struct ieee80211_vif *vif, bool success)
+{
+	struct ieee80211_sub_if_data *sdata;
+	struct ieee80211_if_managed *ifmgd;
+
+	sdata = vif_to_sdata(vif);
+	ifmgd = &sdata->u.mgd;
+
+	trace_api_chswitch_done(sdata, success);
+	if (!success) {
+		/*
+		 * If the channel switch was not successful, stay
+		 * around on the old channel. We currently lack
+		 * good handling of this situation, possibly we
+		 * should just drop the association.
+		 */
+		sdata->local->csa_channel = sdata->local->oper_channel;
+	}
+
+	ieee80211_queue_work(&sdata->local->hw, &ifmgd->chswitch_work);
+}
+EXPORT_SYMBOL(ieee80211_chswitch_done);
+
 static void ieee80211_chswitch_timer(unsigned long data)
 {
 	struct ieee80211_sub_if_data *sdata =
@@ -370,7 +397,8 @@ static void ieee80211_chswitch_timer(unsigned long data)
 
 void ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 				      struct ieee80211_channel_sw_ie *sw_elem,
-				      struct ieee80211_bss *bss)
+				      struct ieee80211_bss *bss,
+				      u64 timestamp)
 {
 	struct cfg80211_bss *cbss =
 		container_of((void *)bss, struct cfg80211_bss, priv);
@@ -398,6 +426,24 @@ void ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 
 	sdata->local->csa_channel = new_ch;
 
+	if (sdata->local->ops->channel_switch) {
+		/* use driver's channel switch callback */
+		struct ieee80211_channel_switch ch_switch;
+		memset(&ch_switch, 0, sizeof(ch_switch));
+		ch_switch.timestamp = timestamp;
+		if (sw_elem->mode) {
+			ch_switch.block_tx = true;
+			ieee80211_stop_queues_by_reason(&sdata->local->hw,
+					IEEE80211_QUEUE_STOP_REASON_CSA);
+		}
+		ch_switch.channel = new_ch;
+		ch_switch.count = sw_elem->count;
+		ifmgd->flags |= IEEE80211_STA_CSA_RECEIVED;
+		drv_channel_switch(sdata->local, &ch_switch);
+		return;
+	}
+
+	/* channel switch handled in software */
 	if (sw_elem->count <= 1) {
 		ieee80211_queue_work(&sdata->local->hw, &ifmgd->chswitch_work);
 	} else {
@@ -1317,7 +1363,8 @@ static void ieee80211_rx_bss_info(struct ieee80211_sub_if_data *sdata,
 							ETH_ALEN) == 0)) {
 		struct ieee80211_channel_sw_ie *sw_elem =
 			(struct ieee80211_channel_sw_ie *)elems->ch_switch_elem;
-		ieee80211_sta_process_chanswitch(sdata, sw_elem, bss);
+		ieee80211_sta_process_chanswitch(sdata, sw_elem,
+						 bss, rx_status->mactime);
 	}
 }
 
@@ -1649,7 +1696,8 @@ static void ieee80211_sta_rx_queued_mgmt(struct ieee80211_sub_if_data *sdata,
 
 			ieee80211_sta_process_chanswitch(sdata,
 					&mgmt->u.action.u.chan_switch.sw_elem,
-					(void *)ifmgd->associated->priv);
+					(void *)ifmgd->associated->priv,
+					rx_status->mactime);
 			break;
 		}
 		mutex_unlock(&ifmgd->mtx);
-- 
1.7.0.5

Re: [PATCH v2 1/2] mac80211: add offload channel switch support

[Luis R. Rodriguez]

On Tue, May 11, 2010 at 7:00 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2010-05-06 at 14:58 -0700, Luis R. Rodriguez wrote:
>> On Thu, May 6, 2010 at 8:25 AM,  <wey-yi.w.guy@intel.com> wrote:
>> > From: Wey-Yi Guy <wey-yi.w.guy@intel.com>

>> > with this offload approach, driver has more control on how to handle the
>> > channel switch request from AP, also can provide more accurate timing
>> > calculation
>>
>> Is the current timing insufficient, and if so can you provide more
>> details. If the real reason for this callback is not timing
>> considerations is the real reason a firmware API thing? If so it
>> doesn't hurt to just say that to avoid confusing developer in deciding
>> which approach to take.
>
> The current mac80211 approach is flawed, for various reasons which I
> won't get into here, most of which we can fix. However, due to
> regulatory concerns our firmware also wants to have more control, like
> checking that the AP is beaconing again after a channel switch before
> letting us transmit frames.

OK this makes perfect sense if the channel you are switching to is
also a DFS channel, but it sounds like something we can also implement
on mac80211, unless of course firmware can make that guarantee for us
quicker. This is the sort of explanation that I think might be very
useful to the driver developer when choosing which mac80211 CSA
operation to implement -- either the standard mac80211 implementation
or a driver specific one.

I take it this tries to resolve some sort of race condition where the
AP decided to switch to another DFS channel, told the STAs, went to
the other channel, and as it does the swtich gets radar signals and
needs to quite down.

> Timing is obviously also a consideration,
> since the firmware can re-enable transmission quickly after the channel
> switch, regardless of the delay in processing the frame or the timer on
> the host.

Reason for me asking for details about this was because I was
wondering whether the reasons for you guys implementing a separate CSA
callback could be addressed by adjusting the internal existing
mac80211 CSA implementation further. Timing constraints to re-enable
TX seem to be the biggest concern here since checking whether or not
the AP is beaconing *can* certainly be done on mac80211. Is there a
measurable TX drop due to the extra latency introduced by using a host
based implementation?

If a driver developer reads the documentation it would be nice for
them to easily get enough information to decide which approach to take
and to do that the more details we can provide the better. If we
haven't tested a mac80211 enhanced approach why not try that first?

>> > The drivers like to support the channel switch offloading function
>>
>> Maybe: "The drivers that require a dedicated channel switch callback"...
>>
>> > will have
>> > to provide the mactime information (at least for mgmt and action frames)
>>
>> Might be good to specify why, or at least in the documentation code below.
>
> Actually, it's up to them. But if you implement the callback, you'll
> want to know precisely when to expect the channel switch

Right, that was my point, it was not clear that this was the reason
for having it so it might help the developer if the documentation
stated that.

> so you'll want
> to know when the frame that contained the CSA was received, which you
> have to provide to mac80211 in the "mactime" rx status field.

Right, thanks for the details, I was just hoping we could clarify that
to the driver developers a little more on the patch.

>> > +/**
>> > + * ieee80211_chswitch_done - Complete channel switch process
>> > + * @vif: &struct ieee80211_vif pointer from the add_interface callback.
>> > + * @is_seccess: make the channel switch successful or not
>>
>> Typo, is_seccess, not success. Also, I don't get what this is for, can
>> you elaborate?
>
> Channel switching could fail, for instance if the AP doesn't show up on
> the new channel. We don't have a way to handle that yet in mac80211, but
> why not let it know.

Oh definitely I agree, I was just hoping this can be explained ont he
kdoc above, it was not clear from reading the code.

>> I'd appreciate more feedback on the why this is being done. Its not
>> clear to me how we are limited by the current implementation.
>
> Ok like I said -- timing is a big thing. Regulatory enforcement in our
> firmware is another.

Regulatory enforcement is already handled by the mac80211 CSA, the
check for beaconing on the channel we move to *can* be done by
mac80211 as well, so that would only leave timing constraints.

  Luis

Re: [PATCH v2 1/2] mac80211: add offload channel switch support

[Guy, Wey-Yi]

Hi Luis,

On Tue, 2010-05-11 at 13:55 -0700, Luis R. Rodriguez wrote:
> On Tue, May 11, 2010 at 7:00 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Thu, 2010-05-06 at 14:58 -0700, Luis R. Rodriguez wrote:
> >> On Thu, May 6, 2010 at 8:25 AM,  <wey-yi.w.guy@intel.com> wrote:
> >> > From: Wey-Yi Guy <wey-yi.w.guy@intel.com>
> 
> >> > with this offload approach, driver has more control on how to handle the
> >> > channel switch request from AP, also can provide more accurate timing
> >> > calculation
> >>
> >> Is the current timing insufficient, and if so can you provide more
> >> details. If the real reason for this callback is not timing
> >> considerations is the real reason a firmware API thing? If so it
> >> doesn't hurt to just say that to avoid confusing developer in deciding
> >> which approach to take.
> >
> > The current mac80211 approach is flawed, for various reasons which I
> > won't get into here, most of which we can fix. However, due to
> > regulatory concerns our firmware also wants to have more control, like
> > checking that the AP is beaconing again after a channel switch before
> > letting us transmit frames.
> 
> OK this makes perfect sense if the channel you are switching to is
> also a DFS channel, but it sounds like something we can also implement
> on mac80211, unless of course firmware can make that guarantee for us
> quicker. This is the sort of explanation that I think might be very
> useful to the driver developer when choosing which mac80211 CSA
> operation to implement -- either the standard mac80211 implementation
> or a driver specific one.
> 
> I take it this tries to resolve some sort of race condition where the
> AP decided to switch to another DFS channel, told the STAs, went to
> the other channel, and as it does the swtich gets radar signals and
> needs to quite down.
> 
Just like Johannes mention, the current mac80211 approach is flawed and
it is possible to cause multiple problems(at least in iwlwifi driver),
this patch provide the option for driver to offload and take control the
operation if the device can handle the "channel switch" operation. BUt
it is still driver developer's option to choice which approach to use.

> > Timing is obviously also a consideration,
> > since the firmware can re-enable transmission quickly after the channel
> > switch, regardless of the delay in processing the frame or the timer on
> > the host.
> 
> Reason for me asking for details about this was because I was
> wondering whether the reasons for you guys implementing a separate CSA
> callback could be addressed by adjusting the internal existing
> mac80211 CSA implementation further. Timing constraints to re-enable
> TX seem to be the biggest concern here since checking whether or not
> the AP is beaconing *can* certainly be done on mac80211. Is there a
> measurable TX drop due to the extra latency introduced by using a host
> based implementation?
> 
We do not have any measurement being done at this time, Timing is one of
the reason to implement this patch, but it is not the main reason. 
On the other hand, if firmware can handle the timing, for sure it can be
less latency, so why not give the option for driver/firmware to handle
it?

> If a driver developer reads the documentation it would be nice for
> them to easily get enough information to decide which approach to take
> and to do that the more details we can provide the better. If we
> haven't tested a mac80211 enhanced approach why not try that first?
> 
Yes, I can provide better document, from my point of view, both options
are valid option; it is really driver developer's choice which one to
choice based on the firmware capabilities

> >> > The drivers like to support the channel switch offloading function
> >>
> >> Maybe: "The drivers that require a dedicated channel switch callback"...
> >>
> >> > will have
> >> > to provide the mactime information (at least for mgmt and action frames)
> >>
> >> Might be good to specify why, or at least in the documentation code below.
> >
> > Actually, it's up to them. But if you implement the callback, you'll
> > want to know precisely when to expect the channel switch
> 
> Right, that was my point, it was not clear that this was the reason
> for having it so it might help the developer if the documentation
> stated that.
> 
Sure

> > so you'll want
> > to know when the frame that contained the CSA was received, which you
> > have to provide to mac80211 in the "mactime" rx status field.
> 
> Right, thanks for the details, I was just hoping we could clarify that
> to the driver developers a little more on the patch.
> 
> >> > +/**
> >> > + * ieee80211_chswitch_done - Complete channel switch process
> >> > + * @vif: &struct ieee80211_vif pointer from the add_interface callback.
> >> > + * @is_seccess: make the channel switch successful or not
> >>
> >> Typo, is_seccess, not success. Also, I don't get what this is for, can
> >> you elaborate?
> >
> > Channel switching could fail, for instance if the AP doesn't show up on
> > the new channel. We don't have a way to handle that yet in mac80211, but
> > why not let it know.
> 
> Oh definitely I agree, I was just hoping this can be explained ont he
> kdoc above, it was not clear from reading the code.
> 
Agree, more detail document is better

> >> I'd appreciate more feedback on the why this is being done. Its not
> >> clear to me how we are limited by the current implementation.
> >
> > Ok like I said -- timing is a big thing. Regulatory enforcement in our
> > firmware is another.
> 
> Regulatory enforcement is already handled by the mac80211 CSA, the
> check for beaconing on the channel we move to *can* be done by
> mac80211 as well, so that would only leave timing constraints.
> 
 
The current mac80211 implement is not clean enough, 
it mix "channel switch" with all the other possibilities to change channels; 
it might cause protential confusion and issues (at least in iwlwifi driver); 
having separated callback is much cleaner approach. 

Thanks
Wey