Re: [RFC 1/3] mac80211: Add provision for 802.11 encap/decap offload

[Johannes Berg]

On Thu, 2016-12-15 at 10:43 +, Thiagarajan, Vasanthakumar wrote:
> On Thursday 15 December 2016 02:46 PM, Johannes Berg wrote:
> > 
> > > Drivers advertising this capability should also implement other
> > > functionalities which deal with 802.11 frame format like below
> > >   - ADDBA/DELBA offload
> > 
> > This shouldn't be necessary.
> 
> Ok. Since driver/hw needs to implement Tx/Rx aggregation related
> functionalities I thought ADDBA/DELBA processing will be little
> important to mac80211. May be I'm missing something.

It needs to do the aggregation of the data flows, but I don't think the
control flows will need to be offloaded entirely? We'd need to have
some feedback mechanism for the SN used etc., so that might be up to
the driver to implement and might not be easy.

Anyway, I'm only suggesting to drop this from the documentation since
it doesn't seem strictly necessary, assuming the driver can report the
correct SSN in the callback all the A-MPDU setup should work more or
less as-is (except I think mac80211 won't buffer frames while doing it,
so also that would have to be done by the driver).

> > >   - Powersave offload
> > 
> > That's ambiguous - you do need to handle sleeping stations (and PS-
> > Poll/U-APSD) in AP mode in the device with this,
> 
> I did not dig deep into PS requirement with ethernet frame format
> because frame control is not available to mac80211, so I thought to
> mention PS offload is a requirement. May be there is already an
> infrastructure in mac80211 to learn PS state of client which was
> notified in the current data frame (other than 802.11 frame control).
> 
>   but I don't see a deep
> > technical reason to require it for client mode. OTOH, client mode
> > is
> > almost always offloaded anyway.
> 
> Ok.

Actually, come to think of it, the whole client-side powersave stuff is
so broken that I can't recommend using it - perhaps just clarify this
and say:

 * AP: offloaded support for powersaving clients
 * non-AP: offloaded support for powersave (if desired)

> > I think you may have forgotten one important item,
> > 
> > - control port handling
> 
> Hmmm, I'm getting WPA-PSK working with this. May be there are other
> control port handling which will not work with ethernet frame format?

I think I later saw control port handling in your 802.3 RX code - my
review at this point was tainted by some thoughts I had how I thought
it should work, but it's different :)

> > > + int (*get_vif_80211_hdr_offload)(struct ieee80211_hw
> > > *hw,
> > > +  struct ieee80211_vif
> > > *vif,
> > > +  bool is_4addr, bool
> > > *supported);
> > 
> > Why are you not simply returning "supported"?
> > 
> > I don't like passing the vif pointer here. At this point, the vif
> > pointer isn't known to the driver yet (through drv_add_interface)
> > so it's a dead pointer as far as the sequencing is concerned.
> > 
> > Is there a possibility that drivers need to switch off ethernet
> > format handling entirely when an incompatible interface is added?
> > For example, if you add a mesh interface, is there a chance that
> > the AP interface might no longer be able to handle this?
> 
>  >
>  > I'd hope this doesn't happen because I think that would 
> be extremely
>  > complicated to handle safely.
> 
> Unfortunately "[RFC 2/3] mac80211: Implement data xmit for 802.11
> encap offload" tries to implement this but more likely buggy :(. You
> are right, it is very complex to get that right. May be we should not
> allow interface needing dynamic switch?

We discussed this over in the other part of the thread :)

johannes

Re: [RFC 1/3] mac80211: Add provision for 802.11 encap/decap offload

[Thiagarajan, Vasanthakumar]

On Thursday 15 December 2016 02:46 PM, Johannes Berg wrote:
>
>> Drivers advertising this capability should also implement other
>> functionalities which deal with 802.11 frame format like below
>
>>  - ADDBA/DELBA offload
>
> This shouldn't be necessary.

Ok. Since driver/hw needs to implement Tx/Rx aggregation related functionalities
I thought ADDBA/DELBA processing will be little important to mac80211. May be 
I'm
missing something.

>
>>  - Hardware rate control
>
> Neither is this, if we find some API to do sampling. The existing rate
> table API already allows setting the rates out of band, so the only
> thing that you'd have to support out of band is sampling.

Ok.

>
>>  - Powersave offload
>
> That's ambiguous - you do need to handle sleeping stations (and PS-
> Poll/U-APSD) in AP mode in the device with this,

I did not dig deep into PS requirement with ethernet frame format because
frame control is not available to mac80211, so I thought to mention PS offload
is a requirement. May be there is already an infrastructure in mac80211 to
learn PS state of client which was notified in the current data frame (other
than 802.11 frame control).

  but I don't see a deep
> technical reason to require it for client mode. OTOH, client mode is
> almost always offloaded anyway.

Ok.

>
> I think you may have forgotten one important item,
>
>   - control port handling

Hmmm, I'm getting WPA-PSK working with this. May be there are other
control port handling which will not work with ethernet frame format?

>
> ?
>
>> + * @IEEE80211_HW_SUPPORTS_80211_ENCAP_DECAP: Hardware/driver
>> supports 802.11
>> + *  encap/decap for data frames. Supporting driver have to
>> implement
>> + *  get_vif_80211_encap_decap_offload() to pass if 802.11
>> encap/decap
>> + *  offload is supported for the vif.
>
> I don't see why you need this, when you have the method - you can just
> assume that the method returns false when it's not implemented.

Ok, I was trying define an interface for driver to return vif type based
encap/decap capability so that in 4-addr and Mesh type 802.11 frame format
is used.

>
>>   struct ieee80211_ops {
>>  void (*tx)(struct ieee80211_hw *hw,
>> @@ -3639,6 +3651,10 @@ struct ieee80211_ops {
>>  void (*wake_tx_queue)(struct ieee80211_hw *hw,
>>struct ieee80211_txq *txq);
>>  void (*sync_rx_queues)(struct ieee80211_hw *hw);
>> +
>> +int (*get_vif_80211_hdr_offload)(struct ieee80211_hw *hw,
>> + struct ieee80211_vif *vif,
>> + bool is_4addr, bool
>> *supported);
>
> Why are you not simply returning "supported"?
>
> I don't like passing the vif pointer here. At this point, the vif
> pointer isn't known to the driver yet (through drv_add_interface) so
> it's a dead pointer as far as the sequencing is concerned.
>
> Is there a possibility that drivers need to switch off ethernet format
> handling entirely when an incompatible interface is added? For example,
> if you add a mesh interface, is there a chance that the AP interface
> might no longer be able to handle this?
 >
 > I'd hope this doesn't happen because I think that would be extremely
 > complicated to handle safely.

Unfortunately "[RFC 2/3] mac80211: Implement data xmit for 802.11 encap offload"
tries to implement this but more likely buggy :(. You are right, it is very
complex to get that right. May be we should not allow interface needing
dynamic switch?

Vasanth

Re: [RFC 1/3] mac80211: Add provision for 802.11 encap/decap offload

[Johannes Berg]

> Drivers advertising this capability should also implement other
> functionalities which deal with 802.11 frame format like below

>   - ADDBA/DELBA offload

This shouldn't be necessary.

>   - Hardware rate control

Neither is this, if we find some API to do sampling. The existing rate
table API already allows setting the rates out of band, so the only
thing that you'd have to support out of band is sampling.

>   - Powersave offload

That's ambiguous - you do need to handle sleeping stations (and PS-
Poll/U-APSD) in AP mode in the device with this, but I don't see a deep
technical reason to require it for client mode. OTOH, client mode is
almost always offloaded anyway.

I think you may have forgotten one important item,

- control port handling

?

> + * @IEEE80211_HW_SUPPORTS_80211_ENCAP_DECAP: Hardware/driver
> supports 802.11
> + *   encap/decap for data frames. Supporting driver have to
> implement
> + *   get_vif_80211_encap_decap_offload() to pass if 802.11
> encap/decap
> + *   offload is supported for the vif.

I don't see why you need this, when you have the method - you can just
assume that the method returns false when it's not implemented.

>  struct ieee80211_ops {
>   void (*tx)(struct ieee80211_hw *hw,
> @@ -3639,6 +3651,10 @@ struct ieee80211_ops {
>   void (*wake_tx_queue)(struct ieee80211_hw *hw,
>     struct ieee80211_txq *txq);
>   void (*sync_rx_queues)(struct ieee80211_hw *hw);
> +
> + int (*get_vif_80211_hdr_offload)(struct ieee80211_hw *hw,
> +  struct ieee80211_vif *vif,
> +  bool is_4addr, bool
> *supported);

Why are you not simply returning "supported"?

I don't like passing the vif pointer here. At this point, the vif
pointer isn't known to the driver yet (through drv_add_interface) so
it's a dead pointer as far as the sequencing is concerned.

Is there a possibility that drivers need to switch off ethernet format
handling entirely when an incompatible interface is added? For example,
if you add a mesh interface, is there a chance that the AP interface
might no longer be able to handle this?

I'd hope this doesn't happen because I think that would be extremely
complicated to handle safely.

johannes

[RFC 1/3] mac80211: Add provision for 802.11 encap/decap offload

[Vasanthakumar Thiagarajan]

Drivers can have the capability to offload 802.11 encap/decap
to firmware or hardware for data frames. This patch adds a new
hw_flag for driver to advertise the offload support. Drivers
advertising the support have also to implement new ieee80211_ops
callback, get_vif_80211_hdr_offload(), to notify if the 802.11 encap/decap
offload is supported for a particular vif type. Transmit and receive
path offloading 802.11 header (including cipher headers) encap/decap
for data frames will be implemented in separate patches.

Drivers advertising this capability should also implement other
functionalities which deal with 802.11 frame format like below

- Hardware encryption/Decryption
- ADDBA/DELBA offload
- Aggregation and deaggregation of A-MSDU offload
- Fragmentation and defragmentation offload
- Rx reordering of A-MPDU subframe offload
- PN/TSC check offload
- Rx duplication check offload
- Hardware rate control
- Powersave offload

Signed-off-by: Vasanthakumar Thiagarajan 
---
 include/net/mac80211.h| 16 
 net/mac80211/debugfs.c|  1 +
 net/mac80211/driver-ops.h | 21 +
 net/mac80211/main.c   |  4 
 net/mac80211/trace.h  | 33 +
 5 files changed, 75 insertions(+)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index b4faadb..1e3c8b5 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2014,6 +2014,11 @@ struct ieee80211_txq {
  * @IEEE80211_HW_TX_FRAG_LIST: Hardware (or driver) supports sending frag_list
  * skbs, needed for zero-copy software A-MSDU.
  *
+ * @IEEE80211_HW_SUPPORTS_80211_ENCAP_DECAP: Hardware/driver supports 802.11
+ * encap/decap for data frames. Supporting driver have to implement
+ * get_vif_80211_encap_decap_offload() to pass if 802.11 encap/decap
+ * offload is supported for the vif.
+ *
  * @NUM_IEEE80211_HW_FLAGS: number of hardware flags, used for sizing arrays
  */
 enum ieee80211_hw_flags {
@@ -2054,6 +2059,7 @@ enum ieee80211_hw_flags {
IEEE80211_HW_USES_RSS,
IEEE80211_HW_TX_AMSDU,
IEEE80211_HW_TX_FRAG_LIST,
+   IEEE80211_HW_SUPPORTS_80211_ENCAP_DECAP,
 
/* keep last, obviously */
NUM_IEEE80211_HW_FLAGS
@@ -3401,6 +3407,12 @@ enum ieee80211_reconfig_type {
  * synchronization which is needed in case driver has in its RSS queues
  * pending frames that were received prior to the control path action
  * currently taken (e.g. disassociation) but are not processed yet.
+ *
+ * @get_vif_80211_hdr_offload: Called to check if driver or hardware
+ * supports 802.11 encap/decap offload for data frames for the vif.
+ * Drivers implementing this callback should advertise the support
+ * through hw_flags (%IEEE80211_HW_SUPPORTS_80211_ENCAP_DECAP).
+ * This callback can sleep.
  */
 struct ieee80211_ops {
void (*tx)(struct ieee80211_hw *hw,
@@ -3639,6 +3651,10 @@ struct ieee80211_ops {
void (*wake_tx_queue)(struct ieee80211_hw *hw,
  struct ieee80211_txq *txq);
void (*sync_rx_queues)(struct ieee80211_hw *hw);
+
+   int (*get_vif_80211_hdr_offload)(struct ieee80211_hw *hw,
+struct ieee80211_vif *vif,
+bool is_4addr, bool *supported);
 };
 
 /**
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 2906c10..f49fea5 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -302,6 +302,7 @@ static const char *hw_flag_names[] = {
FLAG(USES_RSS),
FLAG(TX_AMSDU),
FLAG(TX_FRAG_LIST),
+   FLAG(SUPPORTS_80211_ENCAP_DECAP),
 #undef FLAG
 };
 
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 184473c..22847d2 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1179,4 +1179,25 @@ static inline void drv_wake_tx_queue(struct 
ieee80211_local *local,
local->ops->wake_tx_queue(&local->hw, &txq->txq);
 }
 
+static inline int
+drv_get_vif_80211_hdr_offload(struct ieee80211_local *local,
+ struct ieee80211_sub_if_data *sdata,
+ bool use_4addr, bool *supported)
+{
+   int ret = -EOPNOTSUPP;
+
+   might_sleep();
+
+   if (local->ops->get_vif_80211_hdr_offload)
+   ret = local->ops->get_vif_80211_hdr_offload(&local->hw,
+   &sdata->vif,
+   use_4addr,
+   supported);
+
+   trace_drv_get_vif_80211_hdr_offload(local, sdata, use_4addr,
+   *supported, ret);
+
+   return ret;
+}
+
 #endif /* __MAC80211_DRIVER_OPS */
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index d00ea9b..2095d7c 100644
--- a/net/mac80211/main.c
+