Re: [RFC 2/3] mac80211: Implement data xmit for 802.11 encap offload

[Thiagarajan, Vasanthakumar]

On Monday 19 December 2016 05:15 PM, Kalle Valo wrote:
> "Thiagarajan, Vasanthakumar"  writes:
>
>> On Thursday 15 December 2016 07:23 PM, Johannes Berg wrote:
>>>
> I agree. Dynamic switch part is buggy, we can start with not
> allowing interfaces resulting in dynamic switch.

 Does this mean that when bringing up multiple interfaces, users would
 need to figure out the 'magic' order that works?
>>>
>>> I think we need to talk about hardware capabilities at this point.
>>
>> QCA988X does not have capability to configure vif specific decap mode. Encap 
>> mode
>> is configurable per packet for all the ath10k based chips so this part 
>> should be
>> fine to support per vif configuration. Newer QCA chips like QCA9984, 
>> QCA4019, QCA9888
>> and QCA99X0 supports decap mode configuration per vif.
>
> When you say "all" are you also taking into account QCA6174 and QCA9377?

Good point. I see some workarounds for QCA6174 to send data frames in ethernet 
mode,
so QCA6174 should be fine in this regard. I assume QCA9377 also works fine with
ethernet encap configuration per htt desc, need to confirm.

Vasanth

Re: [RFC 2/3] mac80211: Implement data xmit for 802.11 encap offload

[Kalle Valo]

"Thiagarajan, Vasanthakumar"  writes:

> On Thursday 15 December 2016 07:23 PM, Johannes Berg wrote:
>>
 I agree. Dynamic switch part is buggy, we can start with not
 allowing interfaces resulting in dynamic switch.
>>>
>>> Does this mean that when bringing up multiple interfaces, users would
>>> need to figure out the 'magic' order that works?
>>
>> I think we need to talk about hardware capabilities at this point.
>
> QCA988X does not have capability to configure vif specific decap mode. Encap 
> mode
> is configurable per packet for all the ath10k based chips so this part should 
> be
> fine to support per vif configuration. Newer QCA chips like QCA9984, QCA4019, 
> QCA9888
> and QCA99X0 supports decap mode configuration per vif.

When you say "all" are you also taking into account QCA6174 and QCA9377?

Just checking...

-- 
Kalle Valo

Re: [RFC 2/3] mac80211: Implement data xmit for 802.11 encap offload

[Johannes Berg]

On Fri, 2016-12-16 at 05:37 +, Thiagarajan, Vasanthakumar wrote:

> QCA988X does not have capability to configure vif specific decap
> mode. Encap mode is configurable per packet for all the ath10k based
> chips so this part should be fine to support per vif configuration. 

Ok, that's good.

> Newer QCA chips like QCA9984, QCA4019, QCA9888 and QCA99X0 supports
> decap mode configuration per vif.

Also good.

> To reduce the complexity, we can probably make per vif encap/decap
> configuration mandatory to enable ethernet frame format, not sure how
> this will work with non-QCA capable hardware.

I don't know either, nobody else has talked about it yet :-)

Anyway, if we (for now) we can assume that TX can be constant 802.3
encap mode once enabled, we don't have to deal with the messy dynamic
switching you had there.

RX can switch more dynamically independent of all the mac80211 code
since it basically just means the driver calls one function or the
other - if everything is offloaded correctly there shouldn't really be
a difference.

Then also things like IEEE80211_CONF_CHANGE_80211_HDR_OFFL etc. can go
away entirely because you don't have to switch anything dynamically at
a global level. That will simplify everything a lot.

johannes

Re: [RFC 2/3] mac80211: Implement data xmit for 802.11 encap offload

[Thiagarajan, Vasanthakumar]

On Thursday 15 December 2016 07:23 PM, Johannes Berg wrote:
>
>>> I agree. Dynamic switch part is buggy, we can start with not
>>> allowing interfaces resulting in dynamic switch.
>>
>> Does this mean that when bringing up multiple interfaces, users would
>> need to figure out the 'magic' order that works?
>
> I think we need to talk about hardware capabilities at this point.

QCA988X does not have capability to configure vif specific decap mode. Encap 
mode
is configurable per packet for all the ath10k based chips so this part should be
fine to support per vif configuration. Newer QCA chips like QCA9984, QCA4019, 
QCA9888
and QCA99X0 supports decap mode configuration per vif. To reduce the complexity,
we can probably make per vif encap/decap configuration mandatory to enable 
ethernet
frame format, not sure how this will work with non-QCA capable hardware.

>
> I was assuming that it would actually be possible to run two interfaces
> with different paths here concurrently - is that not true? If that's
> not true, then we absolutely _need_ dynamic switching, I agree with
> Felix, but then we have a pretty big complication to figure out. But we
> can't let this optimisation affect user experience.

Sure.

Vasanth

Re: [RFC 2/3] mac80211: Implement data xmit for 802.11 encap offload

[Johannes Berg]

> > I agree. Dynamic switch part is buggy, we can start with not
> > allowing interfaces resulting in dynamic switch.
> 
> Does this mean that when bringing up multiple interfaces, users would
> need to figure out the 'magic' order that works?

I think we need to talk about hardware capabilities at this point.

I was assuming that it would actually be possible to run two interfaces
with different paths here concurrently - is that not true? If that's
not true, then we absolutely _need_ dynamic switching, I agree with
Felix, but then we have a pretty big complication to figure out. But we
can't let this optimisation affect user experience.

johannes

Re: [RFC 2/3] mac80211: Implement data xmit for 802.11 encap offload

[Felix Fietkau]

On 2016-12-15 13:01, Thiagarajan, Vasanthakumar wrote:
> On Thursday 15 December 2016 02:59 PM, Johannes Berg wrote:
>>> + * @IEEE80211_CONF_CHANGE_80211_HDR_OFFL: Offload configuration
>>> + * implementing 802.11 encap/decap for data frames changed.
>>>*/
>>>   enum ieee80211_conf_changed {
>>> IEEE80211_CONF_CHANGE_SMPS  = BIT(1),
>>> @@ -1279,6 +1286,7 @@ enum ieee80211_conf_changed {
>>> IEEE80211_CONF_CHANGE_CHANNEL   = BIT(6),
>>> IEEE80211_CONF_CHANGE_RETRY_LIMITS  = BIT(7),
>>> IEEE80211_CONF_CHANGE_IDLE  = BIT(8),
>>> +   IEEE80211_CONF_CHANGE_80211_HDR_OFFL= BIT(9),
>>>   };
>>
>> Given the requirements (PN check, etc.) I'm not sure how this can
>> change dynamically?
> 
> I agree. Dynamic switch part is buggy, we can start with not allowing
> interfaces resulting in dynamic switch.
Does this mean that when bringing up multiple interfaces, users would
need to figure out the 'magic' order that works?

- Felix

Re: [RFC 2/3] mac80211: Implement data xmit for 802.11 encap offload

[Thiagarajan, Vasanthakumar]

On Thursday 15 December 2016 02:59 PM, Johannes Berg wrote:
>
>> There is a field, no_80211_encap, added to ieee80211_tx_info:control
>> to mark if the 802.11 encapsulation is offloaded to driver.
>> There is also a new callback for tx completion status indication
>> to handle data frames using 802.11 encap offload.
>
> I'm not sure I see the need for this? Maybe I'll find out in this patch
> :)
>

I think we may not need this if we make the design in such a way that all
the interfaces will use uniform encap/decap mode for the data packet.

>> +/* XXX: This frame is not encaptulated with
>> 802.11
>> + * header. Should this be added to
>> %IEEE80211_TX_CTRL_*
>> + * flags?.
>> + */
>> +bool no_80211_encap;
>> +/* 3 bytes free */
>>  } control;
>
> probably - just to preserve more space.

Correct.

>
>> + * @IEEE80211_CONF_CHANGE_80211_HDR_OFFL: Offload configuration
>> + *  implementing 802.11 encap/decap for data frames changed.
>>*/
>>   enum ieee80211_conf_changed {
>>  IEEE80211_CONF_CHANGE_SMPS  = BIT(1),
>> @@ -1279,6 +1286,7 @@ enum ieee80211_conf_changed {
>>  IEEE80211_CONF_CHANGE_CHANNEL   = BIT(6),
>>  IEEE80211_CONF_CHANGE_RETRY_LIMITS  = BIT(7),
>>  IEEE80211_CONF_CHANGE_IDLE  = BIT(8),
>> +IEEE80211_CONF_CHANGE_80211_HDR_OFFL= BIT(9),
>>   };
>
> Given the requirements (PN check, etc.) I'm not sure how this can
> change dynamically?

I agree. Dynamic switch part is buggy, we can start with not allowing
interfaces resulting in dynamic switch.

>
>> + * @encap_decap_80211_offloaded: Whether 802.11 header encap/decap
>> offload
>> + *  is enabled
>>*/
>>   struct ieee80211_conf {
>>  u32 flags;
>> @@ -1346,6 +1357,7 @@ struct ieee80211_conf {
>>  struct cfg80211_chan_def chandef;
>>  bool radar_enabled;
>>  enum ieee80211_smps_mode smps_mode;
>> +bool encap_decap_80211_offloaded;
>
> Please don't add anything here that's interface specific.

Ok, this is mainly hw configuration of encap/decap mode, not
vif specific per say. Any pointers where this should belong to?

>
>> --- a/net/mac80211/cfg.c
>> +++ b/net/mac80211/cfg.c
>> @@ -107,6 +107,10 @@ static int ieee80211_change_iface(struct wiphy
>> *wiphy,
>>  }
>>  }
>>
>> +ieee80211_if_check_80211_hdr_offl(sdata,
>> +  params ? params->use_4addr
>> : false,
>> +  true);
>> +
>>  return 0;
>>   }
>
> Wouldn't it be better to simply prohibit changing this while the
> interface is up, and re-init it later when it goes up?

I agree.

>
>> +++ b/net/mac80211/ieee80211_i.h
>> @@ -1373,6 +1373,8 @@ struct ieee80211_local {
>>  /* TDLS channel switch */
>>  struct work_struct tdls_chsw_work;
>>  struct sk_buff_head skb_queue_tdls_chsw;
>> +
>> +bool data_80211_hdr_offloaded;
>
> Again, don't put interface specific things into device structures.

Ok, this is also current hw configuration of encap/decap mode, not vif
specific.

>
>> +int ieee80211_lookup_ra_sta(struct ieee80211_sub_if_data *sdata,
>> +struct sk_buff *skb,
>> +struct sta_info **sta_out);
>
> Return the sta_info pointer, and ERR_PTR() if needed.

Correct, sta_out is checked for ERR_PTR() as well before using it.

>
>> +++ b/net/mac80211/iface.c
>> @@ -698,6 +698,11 @@ int ieee80211_do_open(struct wireless_dev *wdev,
>> bool coming_up)
>>  rcu_assign_pointer(local->p2p_sdata, sdata);
>>  }
>>
>> +if (local->open_count == 0 && local-
>>> data_80211_hdr_offloaded) {
>> +local->hw.conf.encap_decap_80211_offloaded = true;
>> +hw_reconf_flags |=
>> IEEE80211_CONF_CHANGE_80211_HDR_OFFL;
>> +}
>
> I don't see how this helps anything, I think you should remove it.

Yeah, I think this was added for dynamic switch.

>
>> +void ieee80211_if_config_80211_hdr_offl(struct ieee80211_local
>> *local,
>> +bool enable_80211_hdr_offl)
>> +{
>> +struct ieee80211_sub_if_data *sdata;
>> +unsigned long flags;
>> +int n_acs = IEEE80211_NUM_ACS;
>> +int ac;
>> +
>> +ASSERT_RTNL();
>> +
>> +if (!ieee80211_hw_check(>hw,
>> SUPPORTS_80211_ENCAP_DECAP) ||
>> +!(ieee80211_hw_check(>hw, HAS_RATE_CONTROL)))
>> +return;
>> +
>> +if (local->hw.wiphy->frag_threshold != (u32)-1 &&
>> +!local->ops->set_frag_threshold)
>> +return;
>> +
>> +mutex_lock(>iflist_mtx);
>> +
>> +list_for_each_entry(sdata, >interfaces, list) {
>> +if (!sdata->dev)
>> +continue;
>> +
>> +netif_tx_stop_all_queues(sdata->dev);
>> +
>> +if (enable_80211_hdr_offl)
>> +sdata->dev->netdev_ops =
>> _dataif_8023_ops;
>> +else
>> + 

Re: [RFC 2/3] mac80211: Implement data xmit for 802.11 encap offload

[Johannes Berg]

> There is a field, no_80211_encap, added to ieee80211_tx_info:control
> to mark if the 802.11 encapsulation is offloaded to driver.
> There is also a new callback for tx completion status indication
> to handle data frames using 802.11 encap offload.

I'm not sure I see the need for this? Maybe I'll find out in this patch
:)

> + /* XXX: This frame is not encaptulated with
> 802.11
> +  * header. Should this be added to
> %IEEE80211_TX_CTRL_*
> +  * flags?.
> +  */
> + bool no_80211_encap;
> + /* 3 bytes free */
>   } control;

probably - just to preserve more space.

> + * @IEEE80211_CONF_CHANGE_80211_HDR_OFFL: Offload configuration
> + *   implementing 802.11 encap/decap for data frames changed.
>   */
>  enum ieee80211_conf_changed {
>   IEEE80211_CONF_CHANGE_SMPS  = BIT(1),
> @@ -1279,6 +1286,7 @@ enum ieee80211_conf_changed {
>   IEEE80211_CONF_CHANGE_CHANNEL   = BIT(6),
>   IEEE80211_CONF_CHANGE_RETRY_LIMITS  = BIT(7),
>   IEEE80211_CONF_CHANGE_IDLE  = BIT(8),
> + IEEE80211_CONF_CHANGE_80211_HDR_OFFL= BIT(9),
>  };

Given the requirements (PN check, etc.) I'm not sure how this can
change dynamically?

> + * @encap_decap_80211_offloaded: Whether 802.11 header encap/decap
> offload
> + *   is enabled
>   */
>  struct ieee80211_conf {
>   u32 flags;
> @@ -1346,6 +1357,7 @@ struct ieee80211_conf {
>   struct cfg80211_chan_def chandef;
>   bool radar_enabled;
>   enum ieee80211_smps_mode smps_mode;
> + bool encap_decap_80211_offloaded;

Please don't add anything here that's interface specific.

> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -107,6 +107,10 @@ static int ieee80211_change_iface(struct wiphy
> *wiphy,
>   }
>   }
>  
> + ieee80211_if_check_80211_hdr_offl(sdata,
> +   params ? params->use_4addr 
> : false,
> +   true);
> +
>   return 0;
>  }

Wouldn't it be better to simply prohibit changing this while the
interface is up, and re-init it later when it goes up?

> +++ b/net/mac80211/ieee80211_i.h
> @@ -1373,6 +1373,8 @@ struct ieee80211_local {
>   /* TDLS channel switch */
>   struct work_struct tdls_chsw_work;
>   struct sk_buff_head skb_queue_tdls_chsw;
> +
> + bool data_80211_hdr_offloaded;

Again, don't put interface specific things into device structures.

> +int ieee80211_lookup_ra_sta(struct ieee80211_sub_if_data *sdata,
> + struct sk_buff *skb,
> + struct sta_info **sta_out);

Return the sta_info pointer, and ERR_PTR() if needed.

> +++ b/net/mac80211/iface.c
> @@ -698,6 +698,11 @@ int ieee80211_do_open(struct wireless_dev *wdev,
> bool coming_up)
>   rcu_assign_pointer(local->p2p_sdata, sdata);
>   }
>  
> + if (local->open_count == 0 && local-
> >data_80211_hdr_offloaded) {
> + local->hw.conf.encap_decap_80211_offloaded = true;
> + hw_reconf_flags |=
> IEEE80211_CONF_CHANGE_80211_HDR_OFFL;
> + }

I don't see how this helps anything, I think you should remove it.

> +void ieee80211_if_config_80211_hdr_offl(struct ieee80211_local
> *local,
> + bool enable_80211_hdr_offl)
> +{
> + struct ieee80211_sub_if_data *sdata;
> + unsigned long flags;
> + int n_acs = IEEE80211_NUM_ACS;
> + int ac;
> +
> + ASSERT_RTNL();
> +
> + if (!ieee80211_hw_check(>hw,
> SUPPORTS_80211_ENCAP_DECAP) ||
> + !(ieee80211_hw_check(>hw, HAS_RATE_CONTROL)))
> + return;
> +
> + if (local->hw.wiphy->frag_threshold != (u32)-1 &&
> + !local->ops->set_frag_threshold)
> + return;
> +
> + mutex_lock(>iflist_mtx);
> +
> + list_for_each_entry(sdata, >interfaces, list) {
> + if (!sdata->dev)
> + continue;
> +
> + netif_tx_stop_all_queues(sdata->dev);
> +
> + if (enable_80211_hdr_offl)
> + sdata->dev->netdev_ops =
> _dataif_8023_ops;
> + else
> + sdata->dev->netdev_ops =
> _dataif_ops;
> + }
> +
> + mutex_unlock(>iflist_mtx);
> +
> + local->data_80211_hdr_offloaded = enable_80211_hdr_offl;
> +
> + if (local->started) {
> + if (enable_80211_hdr_offl)
> + local->hw.conf.encap_decap_80211_offloaded =
> true;
> + else
> + local->hw.conf.encap_decap_80211_offloaded =
> false;
> + ieee80211_hw_config(local,
> + IEEE80211_CONF_CHANGE_80211_HDR_
> OFFL);
> + }
> +
> + mutex_lock(>iflist_mtx);
> +
> + list_for_each_entry(sdata, >interfaces, list) {
> + if (!sdata->dev)
> + continue;
> +
> + if (local->hw.queues < 

[RFC 2/3] mac80211: Implement data xmit for 802.11 encap offload

[Vasanthakumar Thiagarajan]

Driver (or hw) supporting 802.11 encapsulation offload
for data frames can make use of this new xmit path.

This patch defines new ndo_ops, all these callbacks are same as
ieee80211_dataif_ops other than ndo_start_xmit() which does
minimal processing leaving 802.11 encap related to driver.
This patch makes netdev_ops registration dynamic based on the
interface type, at any time the netdev_ops of netdev will be
assigned to either the ndo_ops defined to do 802.11 encap or
the ones defined for 802.11 encap offload. There is a new hw
config notification, IEEE80211_CONF_CHANGE_80211_HDR_OFFL, to make
the driver aware of any configuration change in 802.11 encap/decap
offload.

There is a field, no_80211_encap, added to ieee80211_tx_info:control
to mark if the 802.11 encapsulation is offloaded to driver.
There is also a new callback for tx completion status indication
to handle data frames using 802.11 encap offload. Currently ath10k
fw is capable of doing 802.11 encapsulation/decapsulationi offload.
With the corresponding driver changes, using 802.11 encap/decap offload
might improve performance.

Signed-off-by: Vasanthakumar Thiagarajan 
---
 include/net/mac80211.h |  33 ++-
 net/mac80211/cfg.c |   8 ++
 net/mac80211/ieee80211_i.h |  12 +++
 net/mac80211/iface.c   | 147 +
 net/mac80211/key.c |  16 +++-
 net/mac80211/main.c|   3 +
 net/mac80211/status.c  |  83 -
 net/mac80211/tx.c  | 225 -
 8 files changed, 519 insertions(+), 8 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 1e3c8b5..225abaa 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -910,7 +910,12 @@ struct ieee80211_tx_info {
};
struct ieee80211_key_conf *hw_key;
u32 flags;
-   /* 4 bytes free */
+   /* XXX: This frame is not encaptulated with 802.11
+* header. Should this be added to %IEEE80211_TX_CTRL_*
+* flags?.
+*/
+   bool no_80211_encap;
+   /* 3 bytes free */
} control;
struct {
u64 cookie;
@@ -1269,6 +1274,8 @@ enum ieee80211_conf_flags {
  * @IEEE80211_CONF_CHANGE_SMPS: Spatial multiplexing powersave mode changed
  * Note that this is only valid if channel contexts are not used,
  * otherwise each channel context has the number of chains listed.
+ * @IEEE80211_CONF_CHANGE_80211_HDR_OFFL: Offload configuration
+ * implementing 802.11 encap/decap for data frames changed.
  */
 enum ieee80211_conf_changed {
IEEE80211_CONF_CHANGE_SMPS  = BIT(1),
@@ -1279,6 +1286,7 @@ enum ieee80211_conf_changed {
IEEE80211_CONF_CHANGE_CHANNEL   = BIT(6),
IEEE80211_CONF_CHANGE_RETRY_LIMITS  = BIT(7),
IEEE80211_CONF_CHANGE_IDLE  = BIT(8),
+   IEEE80211_CONF_CHANGE_80211_HDR_OFFL= BIT(9),
 };
 
 /**
@@ -1333,6 +1341,9 @@ enum ieee80211_smps_mode {
  * configured for an HT channel.
  * Note that this is only valid if channel contexts are not used,
  * otherwise each channel context has the number of chains listed.
+ *
+ * @encap_decap_80211_offloaded: Whether 802.11 header encap/decap offload
+ * is enabled
  */
 struct ieee80211_conf {
u32 flags;
@@ -1346,6 +1357,7 @@ struct ieee80211_conf {
struct cfg80211_chan_def chandef;
bool radar_enabled;
enum ieee80211_smps_mode smps_mode;
+   bool encap_decap_80211_offloaded;
 };
 
 /**
@@ -4178,6 +4190,25 @@ void ieee80211_tx_status_irqsafe(struct ieee80211_hw *hw,
 struct sk_buff *skb);
 
 /**
+ * ieee80211_tx_status_8023 - transmit status callback for 802.3 frame format
+ *
+ * Call this function for all transmitted data frames after their transmit
+ * completion. This callback should only be called for data frames which
+ * are are using driver's (or hardware's) offload capability of encap/decap
+ * 802.11 frames.
+ *
+ * This function may not be called in IRQ context. Calls to this function
+ * for a single hardware must be synchronized against each other.
+ *
+ * @hw: the hardware the frame was transmitted by
+ * @vif: the interface for which the frame was transmitted
+ * @skb: the frame that was transmitted, owned by mac80211 after this call
+ */
+void ieee80211_tx_status_8023(struct ieee80211_hw *hw,
+ struct ieee80211_vif *vif,
+ struct sk_buff *skb);
+
+/**
  * ieee80211_report_low_ack - report non-responding station
  *
  * When operating in AP-mode, call this function to report a non-responding
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 47e99ab8..0e53873 100644
--- a/net/mac80211/cfg.c
+++