[RFC 0/4] wireless: Per-sta NoAck and offload support

[RFC 0/4] wireless: Per-sta NoAck and offload support

[Vasanthakumar Thiagarajan]

Adds infrastructure for driver to offload NoAck functionality, driver
like ath10k could make use of it. Also extends the current ndev wide
NoAck policy to per-station, with sta level NoAck policy configuration
userspace could selectively turn off/on Noack based on various connection
parameters of the station.

Vasanthakumar Thiagarajan (4):
  mac80211: Add NoAck policy functionality offload infrastructure
  nl80211/mac80211: Extend NoAck policy command with peer MAC address
  mac80211: Apply per-peer NoAck tid bitmap configuration
  mac80211: Advertise per-peer NoAck policy support

 include/net/cfg80211.h       |  9 +++++++--
 include/net/mac80211.h       | 18 ++++++++++++++++++
 include/uapi/linux/nl80211.h | 12 +++++++++++-
 net/mac80211/cfg.c           | 45 +++++++++++++++++++++++++++++++++++++++++---
 net/mac80211/debugfs.c       |  1 +
 net/mac80211/driver-ops.h    | 22 ++++++++++++++++++++++
 net/mac80211/iface.c         |  4 ++++
 net/mac80211/main.c          |  8 ++++++++
 net/mac80211/sta_info.h      |  3 +++
 net/mac80211/trace.h         | 28 +++++++++++++++++++++++++++
 net/mac80211/tx.c            |  3 ++-
 net/mac80211/wme.c           | 35 +++++++++++++++++++++++++++++++++-
 net/wireless/nl80211.c       | 12 +++++++++++-
 net/wireless/rdev-ops.h      |  7 ++++---
 net/wireless/trace.h         | 11 +++++++----
 15 files changed, 202 insertions(+), 16 deletions(-)

-- 
1.9.1

[RFC 1/4] mac80211: Add NoAck policy functionality offload infrastructure

[Vasanthakumar Thiagarajan]

Add infrastructure for drivers to implement NoAck policy functionlity.
Driver like ath10k does not use the per-packet TID NoAck policy
configuration.  Instead NoAck map is sent to the firmware/hardware
in control path. Firmware takes care of setting up QOS header and
hw with NoAck policy based on the TID NoAck map.

Drivers having this support should advertise it through a new hw_flag,
IEEE80211_HW_SUPPORTS_NOACK_POLICY, and must implement callback
set_noack_tid_bitmap(). Supporting drivers would receive TID NoAck map
through set_noack_tid_bitmap() instead of receiving as part of every
tx skb.

Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@codeaurora.org>
---
 include/net/mac80211.h    | 15 +++++++++++++++
 net/mac80211/cfg.c        | 10 ++++++++--
 net/mac80211/debugfs.c    |  1 +
 net/mac80211/driver-ops.h | 21 +++++++++++++++++++++
 net/mac80211/iface.c      |  4 ++++
 net/mac80211/main.c       |  4 ++++
 net/mac80211/trace.h      | 25 +++++++++++++++++++++++++
 net/mac80211/tx.c         |  3 ++-
 net/mac80211/wme.c        |  3 ++-
 9 files changed, 82 insertions(+), 4 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index d39fd68..5a49c90 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2081,6 +2081,13 @@ struct ieee80211_txq {
  *	the frame, as it is not synced with the AP/P2P GO yet, and thus the
  *	deauthentication frame might not be transmitted.
  *
+ * @IEEE80211_HW_SUPPORTS_NOACK_POLICY: Hardware (or driver) manages noack
+ *	policy handling. Hardware (or driver) takes care of setting
+ *	noack policy in the qos header and does not wait for the ack based
+ *	on the noack TID map. Driver advertising this support must implement
+ *	callback @set_noack_tid_bitmap to receive the user configured noack TID
+ *	bitmap.
+ *
  * @NUM_IEEE80211_HW_FLAGS: number of hardware flags, used for sizing arrays
  */
 enum ieee80211_hw_flags {
@@ -2125,6 +2132,7 @@ enum ieee80211_hw_flags {
 	IEEE80211_HW_SUPPORTS_TX_FRAG,
 	IEEE80211_HW_SUPPORTS_TDLS_BUFFER_STA,
 	IEEE80211_HW_DEAUTH_NEED_MGD_TX_PREP,
+	IEEE80211_HW_SUPPORTS_NOACK_POLICY,
 
 	/* keep last, obviously */
 	NUM_IEEE80211_HW_FLAGS
@@ -3490,6 +3498,10 @@ enum ieee80211_reconfig_type {
  * @del_nan_func: Remove a NAN function. The driver must call
  *	ieee80211_nan_func_terminated() with
  *	NL80211_NAN_FUNC_TERM_REASON_USER_REQUEST reason code upon removal.
+ *
+ * @set_noack_tid_bitmap: Set NoAck policy TID bitmap for a virtual interface.
+ *	Drivers advertising NoAck policy handing support
+ *	(%IEEE80211_HW_SUPPORTS_NOACK_POLICY) must implement this callback.
  */
 struct ieee80211_ops {
 	void (*tx)(struct ieee80211_hw *hw,
@@ -3771,6 +3783,9 @@ struct ieee80211_ops {
 	void (*del_nan_func)(struct ieee80211_hw *hw,
 			    struct ieee80211_vif *vif,
 			    u8 instance_id);
+
+	int (*set_noack_tid_bitmap)(struct ieee80211_hw *hw,
+				    struct ieee80211_vif *vif, u8 noack_map);
 };
 
 /**
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 5c4b105..a2f0eae 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -347,9 +347,15 @@ static int ieee80211_set_noack_map(struct wiphy *wiphy,
 
 	sdata->noack_map = noack_map;
 
-	ieee80211_check_fast_xmit_iface(sdata);
+	if (!ieee80211_hw_check(&sdata->local->hw, SUPPORTS_NOACK_POLICY)) {
+		ieee80211_check_fast_xmit_iface(sdata);
+		return 0;
+	}
 
-	return 0;
+	if (!ieee80211_sdata_running(sdata))
+		return 0;
+
+	return drv_set_noack_tid_bitmap(sdata->local, sdata, noack_map);
 }
 
 static int ieee80211_add_key(struct wiphy *wiphy, struct net_device *dev,
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index a75653a..b6e6243 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -213,6 +213,7 @@ static ssize_t reset_write(struct file *file, const char __user *user_buf,
 	FLAG(SUPPORTS_TX_FRAG),
 	FLAG(SUPPORTS_TDLS_BUFFER_STA),
 	FLAG(DEAUTH_NEED_MGD_TX_PREP),
+	FLAG(SUPPORTS_NOACK_POLICY),
 #undef FLAG
 };
 
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 4d82fe7..a0a2d3c 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1250,4 +1250,25 @@ static inline void drv_del_nan_func(struct ieee80211_local *local,
 	trace_drv_return_void(local);
 }
 
+static inline int drv_set_noack_tid_bitmap(struct ieee80211_local *local,
+					   struct ieee80211_sub_if_data *sdata,
+					   u16 noack_map)
+{
+	int ret;
+
+	might_sleep();
+	if (!check_sdata_in_driver(sdata))
+		return -EIO;
+
+	if (!local->ops->set_noack_tid_bitmap)
+		return -EOPNOTSUPP;
+
+	trace_drv_set_noack_tid_bitmap(local, sdata, noack_map);
+	ret = local->ops->set_noack_tid_bitmap(&local->hw, &sdata->vif,
+					       noack_map);
+	trace_drv_return_int(local, ret);
+
+	return ret;
+}
+
 #endif /* __MAC80211_DRIVER_OPS */
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index d13ba06..4e120b9 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -631,6 +631,10 @@ int ieee80211_do_open(struct wireless_dev *wdev, bool coming_up)
 				ieee80211_vif_type_p2p(&sdata->vif));
 			if (res)
 				goto err_del_interface;
+
+			if (sdata->noack_map)
+				drv_set_noack_tid_bitmap(local, sdata,
+							 sdata->noack_map);
 		}
 
 		if (sdata->vif.type == NL80211_IFTYPE_AP) {
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 0785d04..78f2574 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -835,6 +835,10 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
 		    (!local->ops->start_nan || !local->ops->stop_nan)))
 		return -EINVAL;
 
+	if (WARN_ON(ieee80211_hw_check(hw, SUPPORTS_NOACK_POLICY) &&
+		    !local->ops->set_noack_tid_bitmap))
+		return -EINVAL;
+
 #ifdef CONFIG_PM
 	if (hw->wiphy->wowlan && (!local->ops->suspend || !local->ops->resume))
 		return -EINVAL;
diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
index 591ad02..3b55569 100644
--- a/net/mac80211/trace.h
+++ b/net/mac80211/trace.h
@@ -1863,6 +1863,31 @@ struct trace_switch_entry {
 	)
 );
 
+TRACE_EVENT(drv_set_noack_tid_bitmap,
+	TP_PROTO(struct ieee80211_local *local,
+		 struct ieee80211_sub_if_data *sdata,
+		 u16 noack_map),
+
+	TP_ARGS(local, sdata, noack_map),
+	TP_STRUCT__entry(
+		LOCAL_ENTRY
+		VIF_ENTRY
+		__field(u16, noack_map)
+	),
+
+	TP_fast_assign(
+		LOCAL_ASSIGN;
+		VIF_ASSIGN;
+		__entry->noack_map = noack_map;
+	),
+
+	TP_printk(
+		LOCAL_PR_FMT  VIF_PR_FMT
+		", noack_map: %u",
+		LOCAL_PR_ARG, VIF_PR_ARG, __entry->noack_map
+	)
+);
+
 /*
  * Tracing for API calls that drivers call.
  */
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 933c67b..27fcef3 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2817,7 +2817,8 @@ void ieee80211_check_fast_xmit(struct sta_info *sta)
 	    test_sta_flag(sta, WLAN_STA_CLEAR_PS_FILT))
 		goto out;
 
-	if (sdata->noack_map)
+	if (sdata->noack_map &&
+	    !ieee80211_hw_check(&local->hw, SUPPORTS_NOACK_POLICY))
 		goto out;
 
 	/* fast-xmit doesn't handle fragmentation at all */
diff --git a/net/mac80211/wme.c b/net/mac80211/wme.c
index 5f7c963..6512a47 100644
--- a/net/mac80211/wme.c
+++ b/net/mac80211/wme.c
@@ -256,7 +256,8 @@ void ieee80211_set_qos_hdr(struct ieee80211_sub_if_data *sdata,
 		       IEEE80211_QOS_CTL_ACK_POLICY_MASK);
 
 	if (is_multicast_ether_addr(hdr->addr1) ||
-	    sdata->noack_map & BIT(tid)) {
+	    (!ieee80211_hw_check(&sdata->local->hw, SUPPORTS_NOACK_POLICY) &&
+	    sdata->noack_map & BIT(tid))) {
 		flags |= IEEE80211_QOS_CTL_ACK_POLICY_NOACK;
 		info->flags |= IEEE80211_TX_CTL_NO_ACK;
 	}
-- 
1.9.1

Re: [RFC 1/4] mac80211: Add NoAck policy functionality offload infrastructure

[Johannes Berg]

On Tue, 2018-03-27 at 14:12 +0530, Vasanthakumar Thiagarajan wrote:
> 
> + * @IEEE80211_HW_SUPPORTS_NOACK_POLICY: Hardware (or driver) manages noack
> + *	policy handling. Hardware (or driver) takes care of setting
> + *	noack policy in the qos header and does not wait for the ack based
> + *	on the noack TID map. Driver advertising this support must implement
> + *	callback @set_noack_tid_bitmap to receive the user configured noack TID
> + *	bitmap.

Do you really need the ops method and the flag?

> +	int (*set_noack_tid_bitmap)(struct ieee80211_hw *hw,
> +				    struct ieee80211_vif *vif, u8 noack_map);

You'd safe some effort by reordering the nl80211 patch first, so you can
immediately introduce it with per-peer capability here.

> +++ b/net/mac80211/cfg.c
> @@ -347,9 +347,15 @@ static int ieee80211_set_noack_map(struct wiphy *wiphy,
>  
>  	sdata->noack_map = noack_map;
>  
> -	ieee80211_check_fast_xmit_iface(sdata);
> +	if (!ieee80211_hw_check(&sdata->local->hw, SUPPORTS_NOACK_POLICY)) {
> +		ieee80211_check_fast_xmit_iface(sdata);
> +		return 0;
> +	}
>  
> -	return 0;
> +	if (!ieee80211_sdata_running(sdata))
> +		return 0;
> +
> +	return drv_set_noack_tid_bitmap(sdata->local, sdata, noack_map);

This doesn't seem right - you should do the fast xmit checks even if
calling the driver, no? And in fact, fast xmit should be permitted when
the noack is offloaded, so you shouldn't set sdata->noack_map in the
offloaded case at all.

> --- a/net/mac80211/iface.c
> +++ b/net/mac80211/iface.c
> @@ -631,6 +631,10 @@ int ieee80211_do_open(struct wireless_dev *wdev, bool coming_up)
>  				ieee80211_vif_type_p2p(&sdata->vif));
>  			if (res)
>  				goto err_del_interface;
> +
> +			if (sdata->noack_map)
> +				drv_set_noack_tid_bitmap(local, sdata,
> +							 sdata->noack_map);

Or maybe you do need to store it for this reason, but need to ignore it
for the purposes of fast-xmit if SUPPORTS_NOACK_POLICY is set.

> +++ b/net/mac80211/tx.c
> @@ -2817,7 +2817,8 @@ void ieee80211_check_fast_xmit(struct sta_info *sta)
>  	    test_sta_flag(sta, WLAN_STA_CLEAR_PS_FILT))
>  		goto out;
>  
> -	if (sdata->noack_map)
> +	if (sdata->noack_map &&
> +	    !ieee80211_hw_check(&local->hw, SUPPORTS_NOACK_POLICY))
>  		goto out;

Ah, and you do :-)

And then maybe you're indeed right and don't need to call the fast xmit
check in that place since it wouldn't have any effect either way.
 
johannes

Re: [RFC 1/4] mac80211: Add NoAck policy functionality offload infrastructure

[vthiagar]

On 2018-03-27 18:23, Johannes Berg wrote:
> On Tue, 2018-03-27 at 14:12 +0530, Vasanthakumar Thiagarajan wrote:
>> 
>> + * @IEEE80211_HW_SUPPORTS_NOACK_POLICY: Hardware (or driver) manages 
>> noack
>> + *	policy handling. Hardware (or driver) takes care of setting
>> + *	noack policy in the qos header and does not wait for the ack based
>> + *	on the noack TID map. Driver advertising this support must 
>> implement
>> + *	callback @set_noack_tid_bitmap to receive the user configured 
>> noack TID
>> + *	bitmap.
> 
> Do you really need the ops method and the flag?

Ath10k would send NoAck policy configuration on control path 
configuration.
It seems a new ops might be appropriate. Perhaps the ops alone is 
sufficient
to know the capability?

> 
>> +	int (*set_noack_tid_bitmap)(struct ieee80211_hw *hw,
>> +				    struct ieee80211_vif *vif, u8 noack_map);
> 
> You'd safe some effort by reordering the nl80211 patch first, so you 
> can
> immediately introduce it with per-peer capability here.

Sure.

> 
>> +++ b/net/mac80211/cfg.c
>> @@ -347,9 +347,15 @@ static int ieee80211_set_noack_map(struct wiphy 
>> *wiphy,
>> 
>>  	sdata->noack_map = noack_map;
>> 
>> -	ieee80211_check_fast_xmit_iface(sdata);
>> +	if (!ieee80211_hw_check(&sdata->local->hw, SUPPORTS_NOACK_POLICY)) {
>> +		ieee80211_check_fast_xmit_iface(sdata);
>> +		return 0;
>> +	}
>> 
>> -	return 0;
>> +	if (!ieee80211_sdata_running(sdata))
>> +		return 0;
>> +
>> +	return drv_set_noack_tid_bitmap(sdata->local, sdata, noack_map);
> 
> This doesn't seem right - you should do the fast xmit checks even if
> calling the driver, no? And in fact, fast xmit should be permitted when
> the noack is offloaded, so you shouldn't set sdata->noack_map in the
> offloaded case at all.
> 
>> --- a/net/mac80211/iface.c
>> +++ b/net/mac80211/iface.c
>> @@ -631,6 +631,10 @@ int ieee80211_do_open(struct wireless_dev *wdev, 
>> bool coming_up)
>>  				ieee80211_vif_type_p2p(&sdata->vif));
>>  			if (res)
>>  				goto err_del_interface;
>> +
>> +			if (sdata->noack_map)
>> +				drv_set_noack_tid_bitmap(local, sdata,
>> +							 sdata->noack_map);
> 
> Or maybe you do need to store it for this reason, but need to ignore it
> for the purposes of fast-xmit if SUPPORTS_NOACK_POLICY is set.
> 
>> +++ b/net/mac80211/tx.c
>> @@ -2817,7 +2817,8 @@ void ieee80211_check_fast_xmit(struct sta_info 
>> *sta)
>>  	    test_sta_flag(sta, WLAN_STA_CLEAR_PS_FILT))
>>  		goto out;
>> 
>> -	if (sdata->noack_map)
>> +	if (sdata->noack_map &&
>> +	    !ieee80211_hw_check(&local->hw, SUPPORTS_NOACK_POLICY))
>>  		goto out;
> 
> Ah, and you do :-)
> 
> And then maybe you're indeed right and don't need to call the fast xmit
> check in that place since it wouldn't have any effect either way.

Right.

Vasanth

[RFC 2/4] nl80211/mac80211: Extend NoAck policy command with peer MAC address

[Vasanthakumar Thiagarajan]

Provides peer level NoAck policy configuration by extending
NL80211_CMD_SET_NOACK_MAP command with peer MAC address.
If user space does not give any peer mac address, the driver
should retain the existing functionality of applying the NoAck
policy for all the staions connected to the netdev. Peer specific
configuration takes precedence over netdev level configuration when
both are set by the user. Drivers supporting per-sta NoAck policy
must advertise the support through the extended flag index
NL80211_EXT_FEATURE_PER_STA_NOACK_MAP.

Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@codeaurora.org>
---
 include/net/cfg80211.h       |  9 +++++++--
 include/uapi/linux/nl80211.h | 12 +++++++++++-
 net/mac80211/cfg.c           |  1 +
 net/wireless/nl80211.c       | 12 +++++++++++-
 net/wireless/rdev-ops.h      |  7 ++++---
 net/wireless/trace.h         | 11 +++++++----
 6 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index fc40843..b974d32 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2872,7 +2872,12 @@ struct cfg80211_external_auth_params {
  * @probe_client: probe an associated client, must return a cookie that it
  *	later passes to cfg80211_probe_status().
  *
- * @set_noack_map: Set the NoAck Map for the TIDs.
+ * @set_noack_map: Set the NoAck Map for the TIDs. When peer is not %NULL NoAck
+ *	map will be applied for that particular peer. When peer is %NULL NoAck
+ *	map will be applied for all the connected stations (except the ones
+ *	which already have per-peer TID map configured) on the netdev.
+ *	Driver should return -ENOSPC when the it does not have room for
+ *	additional entries for per-peer NoAck map.
  *
  * @get_channel: Get the current operating channel for the virtual interface.
  *	For monitor interfaces, it should return %NULL unless there's a single
@@ -3180,7 +3185,7 @@ struct cfg80211_ops {
 
 	int	(*set_noack_map)(struct wiphy *wiphy,
 				  struct net_device *dev,
-				  u16 noack_map);
+				  const u8 *peer, u16 noack_map);
 
 	int	(*get_channel)(struct wiphy *wiphy,
 			       struct wireless_dev *wdev,
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 60fefc5..7425ea6 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -784,7 +784,14 @@
  *	messages. Note that per PHY only one application may register.
  *
  * @NL80211_CMD_SET_NOACK_MAP: sets a bitmap for the individual TIDs whether
- *      No Acknowledgement Policy should be applied.
+ *	No Acknowledgement Policy should be applied. %NL80211_ATTR_MAC is used
+ *	to apply No Acknowledgement policy for a particular connected station.
+ *	Station specific NoAck policy configuration is valid only for STA's
+ *	current connection, i.e. the configuration will not be used when the
+ *	station connects back after disconnection/roaming.
+ *	When user-space does not include %NL80211_ATTR_MAC, the No
+ *	Acknowledgement Policy setting should be treated as per-netdev
+ *	configuration.
  *
  * @NL80211_CMD_CH_SWITCH_NOTIFY: An AP or GO may decide to switch channels
  *	independently of the userspace SME, send this event indicating
@@ -5005,6 +5012,8 @@ enum nl80211_feature_flags {
  *	channel change triggered by radar detection event.
  *	No need to start CAC from user-space, no need to react to
  *	"radar detected" event.
+ * @NL80211_EXT_FEATURE_PER_STA_NOACK_MAP: Driver supports STA specific NoAck
+ *	policy functionality.
  *
  * @NUM_NL80211_EXT_FEATURES: number of extended features.
  * @MAX_NL80211_EXT_FEATURES: highest extended feature index.
@@ -5036,6 +5045,7 @@ enum nl80211_ext_feature_index {
 	NL80211_EXT_FEATURE_LOW_POWER_SCAN,
 	NL80211_EXT_FEATURE_HIGH_ACCURACY_SCAN,
 	NL80211_EXT_FEATURE_DFS_OFFLOAD,
+	NL80211_EXT_FEATURE_PER_STA_NOACK_MAP,
 
 	/* add new features before the definition below */
 	NUM_NL80211_EXT_FEATURES,
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index a2f0eae..621ef38 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -341,6 +341,7 @@ static void ieee80211_del_nan_func(struct wiphy *wiphy,
 
 static int ieee80211_set_noack_map(struct wiphy *wiphy,
 				  struct net_device *dev,
+				  const u8 *peer,
 				  u16 noack_map)
 {
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index fe27ab4..8d7f055a 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -3095,16 +3095,26 @@ static int nl80211_set_noack_map(struct sk_buff *skb, struct genl_info *info)
 	struct cfg80211_registered_device *rdev = info->user_ptr[0];
 	struct net_device *dev = info->user_ptr[1];
 	u16 noack_map;
+	const u8 *peer = NULL;
 
 	if (!info->attrs[NL80211_ATTR_NOACK_MAP])
 		return -EINVAL;
 
+	if (info->attrs[NL80211_ATTR_MAC]) {
+		if (!wiphy_ext_feature_isset(
+				&rdev->wiphy,
+				NL80211_EXT_FEATURE_PER_STA_NOACK_MAP))
+			return -EOPNOTSUPP;
+
+		peer = nla_data(info->attrs[NL80211_ATTR_MAC]);
+	}
+
 	if (!rdev->ops->set_noack_map)
 		return -EOPNOTSUPP;
 
 	noack_map = nla_get_u16(info->attrs[NL80211_ATTR_NOACK_MAP]);
 
-	return rdev_set_noack_map(rdev, dev, noack_map);
+	return rdev_set_noack_map(rdev, dev, peer, noack_map);
 }
 
 struct get_key_cookie {
diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index 84f23ae..5c9089d 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -881,11 +881,12 @@ static inline int rdev_probe_client(struct cfg80211_registered_device *rdev,
 }
 
 static inline int rdev_set_noack_map(struct cfg80211_registered_device *rdev,
-				     struct net_device *dev, u16 noack_map)
+				     struct net_device *dev, const u8 *peer,
+				     u16 noack_map)
 {
 	int ret;
-	trace_rdev_set_noack_map(&rdev->wiphy, dev, noack_map);
-	ret = rdev->ops->set_noack_map(&rdev->wiphy, dev, noack_map);
+	trace_rdev_set_noack_map(&rdev->wiphy, dev, peer, noack_map);
+	ret = rdev->ops->set_noack_map(&rdev->wiphy, dev, peer, noack_map);
 	trace_rdev_return_int(&rdev->wiphy, ret);
 	return ret;
 }
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index 5152938..caecf18 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -1883,21 +1883,24 @@
 );
 
 TRACE_EVENT(rdev_set_noack_map,
-	TP_PROTO(struct wiphy *wiphy, struct net_device *netdev,
+	TP_PROTO(struct wiphy *wiphy, struct net_device *netdev, const u8 *peer,
 		 u16 noack_map),
-	TP_ARGS(wiphy, netdev, noack_map),
+	TP_ARGS(wiphy, netdev, peer, noack_map),
 	TP_STRUCT__entry(
 		WIPHY_ENTRY
 		NETDEV_ENTRY
+		MAC_ENTRY(peer)
 		__field(u16, noack_map)
 	),
 	TP_fast_assign(
 		WIPHY_ASSIGN;
 		NETDEV_ASSIGN;
+		MAC_ASSIGN(peer, peer);
 		__entry->noack_map = noack_map;
 	),
-	TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", noack_map: %u",
-		  WIPHY_PR_ARG, NETDEV_PR_ARG, __entry->noack_map)
+	TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", " MAC_PR_FMT
+		  ", noack_map: %u", WIPHY_PR_ARG, NETDEV_PR_ARG,
+		  MAC_PR_ARG(peer), __entry->noack_map)
 );
 
 DEFINE_EVENT(wiphy_wdev_evt, rdev_get_channel,
-- 
1.9.1

Re: [RFC 2/4] nl80211/mac80211: Extend NoAck policy command with peer MAC address

[Johannes Berg]

On Tue, 2018-03-27 at 14:12 +0530, Vasanthakumar Thiagarajan wrote:
> 
> - * @set_noack_map: Set the NoAck Map for the TIDs.
> + * @set_noack_map: Set the NoAck Map for the TIDs. When peer is not %NULL NoAck
> + *	map will be applied for that particular peer. When peer is %NULL NoAck
> + *	map will be applied for all the connected stations (except the ones
> + *	which already have per-peer TID map configured) on the netdev.
> + *	Driver should return -ENOSPC when the it does not have room for
> + *	additional entries for per-peer NoAck map.

I guess it should also set the default for new stations when the peer is
not given? At least that's how mac80211 would behave now, afaict.

The question is how that interacts with having enough space - are you
sure this is a concern?

>   * @NL80211_CMD_SET_NOACK_MAP: sets a bitmap for the individual TIDs whether
> - *      No Acknowledgement Policy should be applied.
> + *	No Acknowledgement Policy should be applied. %NL80211_ATTR_MAC is used
> + *	to apply No Acknowledgement policy for a particular connected station.
> + *	Station specific NoAck policy configuration is valid only for STA's
> + *	current connection, i.e. the configuration will not be used when the
> + *	station connects back after disconnection/roaming.
> + *	When user-space does not include %NL80211_ATTR_MAC, the No
> + *	Acknowledgement Policy setting should be treated as per-netdev
> + *	configuration.

Here you describe different semantics - i.e. you didn't describe the
"previous per-station settings are kept" part. I'm not sure that part
makes much sense anyhow?

johannes

Re: [RFC 2/4] nl80211/mac80211: Extend NoAck policy command with peer MAC address

[vthiagar]

On 2018-03-27 18:17, Johannes Berg wrote:
> On Tue, 2018-03-27 at 14:12 +0530, Vasanthakumar Thiagarajan wrote:
>> 
>> - * @set_noack_map: Set the NoAck Map for the TIDs.
>> + * @set_noack_map: Set the NoAck Map for the TIDs. When peer is not 
>> %NULL NoAck
>> + *	map will be applied for that particular peer. When peer is %NULL 
>> NoAck
>> + *	map will be applied for all the connected stations (except the 
>> ones
>> + *	which already have per-peer TID map configured) on the netdev.
>> + *	Driver should return -ENOSPC when the it does not have room for
>> + *	additional entries for per-peer NoAck map.
> 
> I guess it should also set the default for new stations when the peer 
> is
> not given? At least that's how mac80211 would behave now, afaict.

Sure. May be setting -1 as the default value when the per-peer NoAck 
policy is not
set would work.

> 
> The question is how that interacts with having enough space - are you
> sure this is a concern?

This will not be an issue at lest for ath10k. This is mainly for a 
(new)driver
which implements the offload but has limitation in supporting more than 
certain
number of peers. Perhaps we can remove it now and add it when such 
driver is
available?

> 
>>   * @NL80211_CMD_SET_NOACK_MAP: sets a bitmap for the individual TIDs 
>> whether
>> - *      No Acknowledgement Policy should be applied.
>> + *	No Acknowledgement Policy should be applied. %NL80211_ATTR_MAC is 
>> used
>> + *	to apply No Acknowledgement policy for a particular connected 
>> station.
>> + *	Station specific NoAck policy configuration is valid only for 
>> STA's
>> + *	current connection, i.e. the configuration will not be used when 
>> the
>> + *	station connects back after disconnection/roaming.
>> + *	When user-space does not include %NL80211_ATTR_MAC, the No
>> + *	Acknowledgement Policy setting should be treated as per-netdev
>> + *	configuration.
> 
> Here you describe different semantics - i.e. you didn't describe the
> "previous per-station settings are kept" part. I'm not sure that part
> makes much sense anyhow?

Not sure I got this comment right. As mentioned in the doc, the previous 
settings
would be reset upon reconnection of the station and any ndev wide 
configuration
will be used. As mentioned above, additionally default value will be set 
to the
station to mark no per-station configuration is given so far.

Vasanth

Re: [RFC 2/4] nl80211/mac80211: Extend NoAck policy command with peer MAC address

[Johannes Berg]

On Wed, 2018-03-28 at 10:24 +0530, vthiagar@codeaurora.org wrote:

> > The question is how that interacts with having enough space - are you
> > sure this is a concern?
> 
> This will not be an issue at lest for ath10k. This is mainly for a 
> (new)driver
> which implements the offload but has limitation in supporting more than 
> certain
> number of peers. Perhaps we can remove it now and add it when such 
> driver is
> available?

Ok, that's good. Yes, I think that sounds better - I have a hard time
imagining a firmware/driver that has space for the station, but doesn't
automatically allocate a u16 bitmap as part of the station :-)

> > >   * @NL80211_CMD_SET_NOACK_MAP: sets a bitmap for the individual TIDs 
> > > whether
> > > - *      No Acknowledgement Policy should be applied.
> > > + *	No Acknowledgement Policy should be applied. %NL80211_ATTR_MAC is 
> > > used
> > > + *	to apply No Acknowledgement policy for a particular connected 
> > > station.
> > > + *	Station specific NoAck policy configuration is valid only for 
> > > STA's
> > > + *	current connection, i.e. the configuration will not be used when 
> > > the
> > > + *	station connects back after disconnection/roaming.
> > > + *	When user-space does not include %NL80211_ATTR_MAC, the No
> > > + *	Acknowledgement Policy setting should be treated as per-netdev
> > > + *	configuration.
> > 
> > Here you describe different semantics - i.e. you didn't describe the
> > "previous per-station settings are kept" part. I'm not sure that part
> > makes much sense anyhow?
> 
> Not sure I got this comment right. As mentioned in the doc, the previous 
> settings
> would be reset upon reconnection of the station and any ndev wide 
> configuration
> will be used. As mentioned above, additionally default value will be set 
> to the
> station to mark no per-station configuration is given so far.

I just thought that there was a difference in how this applies to a
certain station.

Btw, we should probably also have a way to *delete* the per-station
configuration, so it uses the default again?

johannes

Re: [RFC 2/4] nl80211/mac80211: Extend NoAck policy command with peer MAC address

[vthiagar]

On 2018-03-28 13:36, Johannes Berg wrote:
> On Wed, 2018-03-28 at 10:24 +0530, vthiagar@codeaurora.org wrote:
> 
>> > The question is how that interacts with having enough space - are you
>> > sure this is a concern?
>> 
>> This will not be an issue at lest for ath10k. This is mainly for a
>> (new)driver
>> which implements the offload but has limitation in supporting more 
>> than
>> certain
>> number of peers. Perhaps we can remove it now and add it when such
>> driver is
>> available?
> 
> Ok, that's good. Yes, I think that sounds better - I have a hard time
> imagining a firmware/driver that has space for the station, but doesn't
> automatically allocate a u16 bitmap as part of the station :-)
> 
>> > >   * @NL80211_CMD_SET_NOACK_MAP: sets a bitmap for the individual TIDs
>> > > whether
>> > > - *      No Acknowledgement Policy should be applied.
>> > > + *	No Acknowledgement Policy should be applied. %NL80211_ATTR_MAC is
>> > > used
>> > > + *	to apply No Acknowledgement policy for a particular connected
>> > > station.
>> > > + *	Station specific NoAck policy configuration is valid only for
>> > > STA's
>> > > + *	current connection, i.e. the configuration will not be used when
>> > > the
>> > > + *	station connects back after disconnection/roaming.
>> > > + *	When user-space does not include %NL80211_ATTR_MAC, the No
>> > > + *	Acknowledgement Policy setting should be treated as per-netdev
>> > > + *	configuration.
>> >
>> > Here you describe different semantics - i.e. you didn't describe the
>> > "previous per-station settings are kept" part. I'm not sure that part
>> > makes much sense anyhow?
>> 
>> Not sure I got this comment right. As mentioned in the doc, the 
>> previous
>> settings
>> would be reset upon reconnection of the station and any ndev wide
>> configuration
>> will be used. As mentioned above, additionally default value will be 
>> set
>> to the
>> station to mark no per-station configuration is given so far.
> 
> I just thought that there was a difference in how this applies to a
> certain station.

May be the doc needs more update

> 
> Btw, we should probably also have a way to *delete* the per-station
> configuration, so it uses the default again?

Sure. How about setting it to default when the command is received with 
no
NL80211_ATTR_NOACK_MAP attribute for a station?

Vasanth

Re: [RFC 2/4] nl80211/mac80211: Extend NoAck policy command with peer MAC address

[Johannes Berg]

On Wed, 2018-03-28 at 14:20 +0530, vthiagar@codeaurora.org wrote:
> 
> > Btw, we should probably also have a way to *delete* the per-station
> > configuration, so it uses the default again?
> 
> Sure. How about setting it to default when the command is received with 
> no NL80211_ATTR_NOACK_MAP attribute for a station?

Sounds good to me.

johannes

[RFC 3/4] mac80211: Apply per-peer NoAck tid bitmap configuration

[Vasanthakumar Thiagarajan]

Use per-peer noack tid bitmap, if it is configured,
when setting up the qos header. If no per-peer configuration
is set, use the existing nedev wide noack policy configuration.
Also modifies callback set_noack_tid_bitmap() with the provision
to send per-peer NoAck policy configuration to the drivers supporting
the NoAck offload functionality (IEEE80211_HW_SUPPORTS_NOACK_POLICY).

Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@codeaurora.org>
---
 include/net/mac80211.h    |  7 +++++--
 net/mac80211/cfg.c        | 42 +++++++++++++++++++++++++++++++++++++-----
 net/mac80211/driver-ops.h |  5 +++--
 net/mac80211/iface.c      |  2 +-
 net/mac80211/sta_info.h   |  3 +++
 net/mac80211/trace.h      |  9 ++++++---
 net/mac80211/tx.c         |  2 +-
 net/mac80211/wme.c        | 34 +++++++++++++++++++++++++++++++++-
 8 files changed, 89 insertions(+), 15 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 5a49c90..a5ed552 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -3499,7 +3499,9 @@ enum ieee80211_reconfig_type {
  *	ieee80211_nan_func_terminated() with
  *	NL80211_NAN_FUNC_TERM_REASON_USER_REQUEST reason code upon removal.
  *
- * @set_noack_tid_bitmap: Set NoAck policy TID bitmap for a virtual interface.
+ * @set_noack_tid_bitmap: Set NoAck policy TID bitmap. Apply the TID NoAck
+ *	configuration for a particular station when @sta is non-NULL. When @sta
+ *	is NULL, apply TID NoAck configuration at virtual interface level.
  *	Drivers advertising NoAck policy handing support
  *	(%IEEE80211_HW_SUPPORTS_NOACK_POLICY) must implement this callback.
  */
@@ -3785,7 +3787,8 @@ struct ieee80211_ops {
 			    u8 instance_id);
 
 	int (*set_noack_tid_bitmap)(struct ieee80211_hw *hw,
-				    struct ieee80211_vif *vif, u8 noack_map);
+				    struct ieee80211_vif *vif,
+				    struct ieee80211_sta *sta, u8 noack_map);
 };
 
 /**
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 621ef38..1efc9cf 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -345,18 +345,50 @@ static int ieee80211_set_noack_map(struct wiphy *wiphy,
 				  u16 noack_map)
 {
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+	struct sta_info *sta;
+	int ret;
 
-	sdata->noack_map = noack_map;
+	if (!peer) {
+		sdata->noack_map = noack_map;
 
-	if (!ieee80211_hw_check(&sdata->local->hw, SUPPORTS_NOACK_POLICY)) {
-		ieee80211_check_fast_xmit_iface(sdata);
-		return 0;
+		if (!ieee80211_hw_check(&sdata->local->hw, SUPPORTS_NOACK_POLICY)) {
+			ieee80211_check_fast_xmit_iface(sdata);
+			return 0;
+		}
+
+		if (!ieee80211_sdata_running(sdata))
+			return 0;
+
+		return drv_set_noack_tid_bitmap(sdata->local, sdata, NULL,
+						noack_map);
 	}
 
+	/* NoAck policy is for a connected client on the dev */
+
 	if (!ieee80211_sdata_running(sdata))
+		return -ENETDOWN;
+
+	mutex_lock(&sdata->local->sta_mtx);
+
+	sta = sta_info_get_bss(sdata, peer);
+	if (!sta) {
+		mutex_unlock(&sdata->local->sta_mtx);
+		return -ENOENT;
+	}
+
+	sta->noack_map = noack_map;
+
+	if (!ieee80211_hw_check(&sdata->local->hw, SUPPORTS_NOACK_POLICY)) {
+		ieee80211_check_fast_xmit(sta);
+		mutex_unlock(&sdata->local->sta_mtx);
 		return 0;
+	}
+
+	ret = drv_set_noack_tid_bitmap(sdata->local, sdata, sta, noack_map);
 
-	return drv_set_noack_tid_bitmap(sdata->local, sdata, noack_map);
+	mutex_unlock(&sdata->local->sta_mtx);
+
+	return ret;
 }
 
 static int ieee80211_add_key(struct wiphy *wiphy, struct net_device *dev,
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index a0a2d3c..8a2154a 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1252,6 +1252,7 @@ static inline void drv_del_nan_func(struct ieee80211_local *local,
 
 static inline int drv_set_noack_tid_bitmap(struct ieee80211_local *local,
 					   struct ieee80211_sub_if_data *sdata,
+					   struct sta_info *sta,
 					   u16 noack_map)
 {
 	int ret;
@@ -1263,9 +1264,9 @@ static inline int drv_set_noack_tid_bitmap(struct ieee80211_local *local,
 	if (!local->ops->set_noack_tid_bitmap)
 		return -EOPNOTSUPP;
 
-	trace_drv_set_noack_tid_bitmap(local, sdata, noack_map);
+	trace_drv_set_noack_tid_bitmap(local, sdata, &sta->sta, noack_map);
 	ret = local->ops->set_noack_tid_bitmap(&local->hw, &sdata->vif,
-					       noack_map);
+					       &sta->sta, noack_map);
 	trace_drv_return_int(local, ret);
 
 	return ret;
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 4e120b9..1172bed 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -633,7 +633,7 @@ int ieee80211_do_open(struct wireless_dev *wdev, bool coming_up)
 				goto err_del_interface;
 
 			if (sdata->noack_map)
-				drv_set_noack_tid_bitmap(local, sdata,
+				drv_set_noack_tid_bitmap(local, sdata, NULL,
 							 sdata->noack_map);
 		}
 
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index f64eb86..01bee75 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -581,6 +581,9 @@ struct sta_info {
 
 	struct cfg80211_chan_def tdls_chandef;
 
+	/* TID bitmap for station's NoAck policy */
+	u16 noack_map;
+
 	/* keep last! */
 	struct ieee80211_sta sta;
 };
diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
index 3b55569..84089f7 100644
--- a/net/mac80211/trace.h
+++ b/net/mac80211/trace.h
@@ -1866,25 +1866,28 @@ struct trace_switch_entry {
 TRACE_EVENT(drv_set_noack_tid_bitmap,
 	TP_PROTO(struct ieee80211_local *local,
 		 struct ieee80211_sub_if_data *sdata,
+		 struct ieee80211_sta *sta,
 		 u16 noack_map),
 
-	TP_ARGS(local, sdata, noack_map),
+	TP_ARGS(local, sdata, sta, noack_map),
 	TP_STRUCT__entry(
 		LOCAL_ENTRY
 		VIF_ENTRY
+		STA_ENTRY
 		__field(u16, noack_map)
 	),
 
 	TP_fast_assign(
 		LOCAL_ASSIGN;
 		VIF_ASSIGN;
+		STA_ASSIGN;
 		__entry->noack_map = noack_map;
 	),
 
 	TP_printk(
-		LOCAL_PR_FMT  VIF_PR_FMT
+		LOCAL_PR_FMT  VIF_PR_FMT STA_PR_FMT
 		", noack_map: %u",
-		LOCAL_PR_ARG, VIF_PR_ARG, __entry->noack_map
+		LOCAL_PR_ARG, VIF_PR_ARG, STA_PR_ARG, __entry->noack_map
 	)
 );
 
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 27fcef3..5abe73b 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2817,7 +2817,7 @@ void ieee80211_check_fast_xmit(struct sta_info *sta)
 	    test_sta_flag(sta, WLAN_STA_CLEAR_PS_FILT))
 		goto out;
 
-	if (sdata->noack_map &&
+	if ((sdata->noack_map || sta->noack_map) &&
 	    !ieee80211_hw_check(&local->hw, SUPPORTS_NOACK_POLICY))
 		goto out;
 
diff --git a/net/mac80211/wme.c b/net/mac80211/wme.c
index 6512a47..645c50b 100644
--- a/net/mac80211/wme.c
+++ b/net/mac80211/wme.c
@@ -227,6 +227,38 @@ u16 ieee80211_select_queue(struct ieee80211_sub_if_data *sdata,
 }
 
 /**
+ * ieee80211_get_noack_map - Get TID bitmap of NoAck policy. NoAck policy
+ * could be device wide or per-station.
+ *
+ * @sdata: local subif
+ * @mac: MAC address of the receiver
+ */
+u16 ieee80211_get_noack_map(struct ieee80211_sub_if_data *sdata, const u8 *mac)
+{
+	struct sta_info *sta;
+	u16 noack_map = 0;
+
+	/* Retrieve per-station noack_map config for the receiver, if any */
+
+	rcu_read_lock();
+
+	sta = sta_info_get(sdata, mac);
+	if (!sta) {
+		rcu_read_unlock();
+		return noack_map;
+	}
+
+	noack_map = sta->noack_map;
+
+	rcu_read_unlock();
+
+	if (!noack_map)
+		noack_map = sdata->noack_map;
+
+	return noack_map;
+}
+
+/**
  * ieee80211_set_qos_hdr - Fill in the QoS header if there is one.
  *
  * @sdata: local subif
@@ -257,7 +289,7 @@ void ieee80211_set_qos_hdr(struct ieee80211_sub_if_data *sdata,
 
 	if (is_multicast_ether_addr(hdr->addr1) ||
 	    (!ieee80211_hw_check(&sdata->local->hw, SUPPORTS_NOACK_POLICY) &&
-	    sdata->noack_map & BIT(tid))) {
+	    ieee80211_get_noack_map(sdata, hdr->addr1) & BIT(tid))) {
 		flags |= IEEE80211_QOS_CTL_ACK_POLICY_NOACK;
 		info->flags |= IEEE80211_TX_CTL_NO_ACK;
 	}
-- 
1.9.1

Re: [RFC 3/4] mac80211: Apply per-peer NoAck tid bitmap configuration

[Johannes Berg]

On Tue, 2018-03-27 at 14:12 +0530, Vasanthakumar Thiagarajan wrote:
> 
> +u16 ieee80211_get_noack_map(struct ieee80211_sub_if_data *sdata, const u8 *mac)
> +{
> +	struct sta_info *sta;
> +	u16 noack_map = 0;
> +
> +	/* Retrieve per-station noack_map config for the receiver, if any */
> +
> +	rcu_read_lock();
> +
> +	sta = sta_info_get(sdata, mac);
> +	if (!sta) {
> +		rcu_read_unlock();
> +		return noack_map;
> +	}
> +
> +	noack_map = sta->noack_map;
> +
> +	rcu_read_unlock();
> +
> +	if (!noack_map)
> +		noack_map = sdata->noack_map;

So this has an interesting corner case - should it be possible to have a
default noack_map that's non-zero, but override it with 0 for a specific
peer? It seems that it should be, which makes this code wrong.

johannes

Re: [RFC 3/4] mac80211: Apply per-peer NoAck tid bitmap configuration

[vthiagar]

On 2018-03-27 18:24, Johannes Berg wrote:
> On Tue, 2018-03-27 at 14:12 +0530, Vasanthakumar Thiagarajan wrote:
>> 
>> +u16 ieee80211_get_noack_map(struct ieee80211_sub_if_data *sdata, 
>> const u8 *mac)
>> +{
>> +	struct sta_info *sta;
>> +	u16 noack_map = 0;
>> +
>> +	/* Retrieve per-station noack_map config for the receiver, if any */
>> +
>> +	rcu_read_lock();
>> +
>> +	sta = sta_info_get(sdata, mac);
>> +	if (!sta) {
>> +		rcu_read_unlock();
>> +		return noack_map;
>> +	}
>> +
>> +	noack_map = sta->noack_map;
>> +
>> +	rcu_read_unlock();
>> +
>> +	if (!noack_map)
>> +		noack_map = sdata->noack_map;
> 
> So this has an interesting corner case - should it be possible to have 
> a
> default noack_map that's non-zero, but override it with 0 for a 
> specific
> peer? It seems that it should be, which makes this code wrong.

I think 0 as the Noack configuration from user can also be a valid one 
in the case
where user does not want any NoAck policy to be used for a particular 
station even
when a non-zero NoAck configuration is set for ndev level. In this case, 
the logic
may need to be modified so that the default non-zero configuration 
(something like -1)
could be used to determine that the station has been never configured 
with any NoAck
policy and use ndev level configuration. Does this sound reasonable?

Vasanth

Re: [RFC 3/4] mac80211: Apply per-peer NoAck tid bitmap configuration

[Johannes Berg]

> I think 0 as the Noack configuration from user can also be a valid one 
> in the case
> where user does not want any NoAck policy to be used for a particular 
> station even
> when a non-zero NoAck configuration is set for ndev level. In this case, 
> the logic
> may need to be modified so that the default non-zero configuration 
> (something like -1)
> could be used to determine that the station has been never configured 
> with any NoAck
> policy and use ndev level configuration. Does this sound reasonable?

Yes. You'll have to use int instead of u16 I guess, but that's
completely doable.

johannes

Re: [RFC 3/4] mac80211: Apply per-peer NoAck tid bitmap configuration

[vthiagar]

On 2018-03-27 14:12, Vasanthakumar Thiagarajan wrote:
> Use per-peer noack tid bitmap, if it is configured,
> when setting up the qos header. If no per-peer configuration
> is set, use the existing nedev wide noack policy configuration.
> Also modifies callback set_noack_tid_bitmap() with the provision
> to send per-peer NoAck policy configuration to the drivers supporting
> the NoAck offload functionality (IEEE80211_HW_SUPPORTS_NOACK_POLICY).
> 
> Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@codeaurora.org>
> ---
>  include/net/mac80211.h    |  7 +++++--
>  net/mac80211/cfg.c        | 42 
> +++++++++++++++++++++++++++++++++++++-----
>  net/mac80211/driver-ops.h |  5 +++--
>  net/mac80211/iface.c      |  2 +-
>  net/mac80211/sta_info.h   |  3 +++
>  net/mac80211/trace.h      |  9 ++++++---
>  net/mac80211/tx.c         |  2 +-
>  net/mac80211/wme.c        | 34 +++++++++++++++++++++++++++++++++-
>  8 files changed, 89 insertions(+), 15 deletions(-)
> 

<snip>

> diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
> index a0a2d3c..8a2154a 100644
> --- a/net/mac80211/driver-ops.h
> +++ b/net/mac80211/driver-ops.h
> @@ -1252,6 +1252,7 @@ static inline void drv_del_nan_func(struct
> ieee80211_local *local,
> 
>  static inline int drv_set_noack_tid_bitmap(struct ieee80211_local 
> *local,
>  					   struct ieee80211_sub_if_data *sdata,
> +					   struct sta_info *sta,
>  					   u16 noack_map)
>  {
>  	int ret;
> @@ -1263,9 +1264,9 @@ static inline int
> drv_set_noack_tid_bitmap(struct ieee80211_local *local,
>  	if (!local->ops->set_noack_tid_bitmap)
>  		return -EOPNOTSUPP;
> 
> -	trace_drv_set_noack_tid_bitmap(local, sdata, noack_map);
> +	trace_drv_set_noack_tid_bitmap(local, sdata, &sta->sta, noack_map);
>  	ret = local->ops->set_noack_tid_bitmap(&local->hw, &sdata->vif,
> -					       noack_map);
> +					       &sta->sta, noack_map);

Oops, we'll endup in NULL pointer dereference in accessing sta object 
when ndev level
configuration is sent. Ill address this in the next version.

Vasanth

[RFC 4/4] mac80211: Advertise per-peer NoAck policy support

[Vasanthakumar Thiagarajan]

This enables per-peer NoAck handling in mac80211 when
the functionality is not offloaded to the drivers.

Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@codeaurora.org>
---
 net/mac80211/main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 78f2574..2b136fb 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -565,6 +565,10 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len,
 
 	wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_RRM);
 
+	if (!ops->set_noack_tid_bitmap)
+		wiphy_ext_feature_set(wiphy,
+				      NL80211_EXT_FEATURE_PER_STA_NOACK_MAP);
+
 	wiphy->bss_priv_size = sizeof(struct ieee80211_bss);
 
 	local = wiphy_priv(wiphy);
-- 
1.9.1

Re: [RFC 0/4] wireless: Per-sta NoAck and offload support

[Steve deRosier]

Hi Vasanthakumar,

On Tue, Mar 27, 2018 at 1:42 AM, Vasanthakumar Thiagarajan
<vthiagar@codeaurora.org> wrote:
> Adds infrastructure for driver to offload NoAck functionality, driver
> like ath10k could make use of it. Also extends the current ndev wide

I'm not really much of a fan of adding a feature without some use of
the feature. Perhaps if drivers "like" ath10k could use it, maybe you
should add a patch(s) to the series where one of those drivers
actually uses the feature.  An API without an example of use is also
harder to evaluate effectively.

Additionally, if it's relevant, adding use of the feature to hwsim
would both serve the above comment as well as provide testing
capability.


> NoAck policy to per-station, with sta level NoAck policy configuration
> userspace could selectively turn off/on Noack based on various connection
> parameters of the station.
>

This is my own ignorance, perhaps from missing recent netdev
conferences - can you send a link to some documentation of what NoAck
is? Certain things in 802.11 use ack transmissions and
interoperability would be compromised if we didn't conform to spec. I
don't imagine that's what's going on here but I'd like to understand
what the heck NoAck is and I failed to bring up anything useful when I
Googled it.

- Steve

Re: [RFC 0/4] wireless: Per-sta NoAck and offload support

[vthiagar]

On 2018-03-27 22:18, Steve deRosier wrote:
> Hi Vasanthakumar,
> 
> On Tue, Mar 27, 2018 at 1:42 AM, Vasanthakumar Thiagarajan
> <vthiagar@codeaurora.org> wrote:
>> Adds infrastructure for driver to offload NoAck functionality, driver
>> like ath10k could make use of it. Also extends the current ndev wide
> 
> I'm not really much of a fan of adding a feature without some use of
> the feature. Perhaps if drivers "like" ath10k could use it, maybe you
> should add a patch(s) to the series where one of those drivers
> actually uses the feature.  An API without an example of use is also
> harder to evaluate effectively.

I agree driver patch using the new NoAck infrastructure would help with
understanding, ill post it once it is ready. But not sure the driver 
patch
can be part of the same series.

> 
> Additionally, if it's relevant, adding use of the feature to hwsim
> would both serve the above comment as well as provide testing
> capability.

Does not seem like this offload feature is something applicable for 
hwsim
especially mac80211 already offers the same functionality.

> 
> 
>> NoAck policy to per-station, with sta level NoAck policy configuration
>> userspace could selectively turn off/on Noack based on various 
>> connection
>> parameters of the station.
>> 
> 
> This is my own ignorance, perhaps from missing recent netdev
> conferences - can you send a link to some documentation of what NoAck
> is? Certain things in 802.11 use ack transmissions and
> interoperability would be compromised if we didn't conform to spec. I
> don't imagine that's what's going on here but I'd like to understand
> what the heck NoAck is and I failed to bring up anything useful when I
> Googled it.

The NoAck configuration is a bitmap of tid which is used to set NoAck in 
Qos
control field of the data frame for that particular tid. Perhaps you 
could
look at Ack policy subfield section in 802.11 spec.

Vasanth

Re: [RFC 0/4] wireless: Per-sta NoAck and offload support

[Arend van Spriel]

On 3/28/2018 8:09 AM, vthiagar@codeaurora.org wrote:
> On 2018-03-27 22:18, Steve deRosier wrote:
>> Hi Vasanthakumar,
>>
>> On Tue, Mar 27, 2018 at 1:42 AM, Vasanthakumar Thiagarajan
>> <vthiagar@codeaurora.org> wrote:
>>> Adds infrastructure for driver to offload NoAck functionality, driver
>>> like ath10k could make use of it. Also extends the current ndev wide
>>
>> I'm not really much of a fan of adding a feature without some use of
>> the feature. Perhaps if drivers "like" ath10k could use it, maybe you
>> should add a patch(s) to the series where one of those drivers
>> actually uses the feature.  An API without an example of use is also
>> harder to evaluate effectively.
>
> I agree driver patch using the new NoAck infrastructure would help with
> understanding, ill post it once it is ready. But not sure the driver patch
> can be part of the same series.

The API patches would go in mac80211-next tree and indeed the driver 
patch would go through wireless-drivers-next tree. However, an option 
would be to add the driver patch(es) as RFC in the series so Johannes 
can ignore it and we still have an example to look at.

Regards,
Arend

Re: [RFC 0/4] wireless: Per-sta NoAck and offload support

[vthiagar]

On 2018-03-28 13:07, Arend van Spriel wrote:
> On 3/28/2018 8:09 AM, vthiagar@codeaurora.org wrote:
>> On 2018-03-27 22:18, Steve deRosier wrote:
>>> Hi Vasanthakumar,
>>> 
>>> On Tue, Mar 27, 2018 at 1:42 AM, Vasanthakumar Thiagarajan
>>> <vthiagar@codeaurora.org> wrote:
>>>> Adds infrastructure for driver to offload NoAck functionality, 
>>>> driver
>>>> like ath10k could make use of it. Also extends the current ndev wide
>>> 
>>> I'm not really much of a fan of adding a feature without some use of
>>> the feature. Perhaps if drivers "like" ath10k could use it, maybe you
>>> should add a patch(s) to the series where one of those drivers
>>> actually uses the feature.  An API without an example of use is also
>>> harder to evaluate effectively.
>> 
>> I agree driver patch using the new NoAck infrastructure would help 
>> with
>> understanding, ill post it once it is ready. But not sure the driver 
>> patch
>> can be part of the same series.
> 
> The API patches would go in mac80211-next tree and indeed the driver
> patch would go through wireless-drivers-next tree. However, an option
> would be to add the driver patch(es) as RFC in the series so Johannes
> can ignore it and we still have an example to look at.

Sounds good. Ill try to include the driver patch in the next version.

Vasanth

Re: [RFC 0/4] wireless: Per-sta NoAck and offload support

[Johannes Berg]

On Wed, 2018-03-28 at 13:29 +0530, vthiagar@codeaurora.org wrote:
> On 2018-03-28 13:07, Arend van Spriel wrote:
> > On 3/28/2018 8:09 AM, vthiagar@codeaurora.org wrote:
> > > On 2018-03-27 22:18, Steve deRosier wrote:
> > > > Hi Vasanthakumar,
> > > > 
> > > > On Tue, Mar 27, 2018 at 1:42 AM, Vasanthakumar Thiagarajan
> > > > <vthiagar@codeaurora.org> wrote:
> > > > > Adds infrastructure for driver to offload NoAck functionality, 
> > > > > driver
> > > > > like ath10k could make use of it. Also extends the current ndev wide
> > > > 
> > > > I'm not really much of a fan of adding a feature without some use of
> > > > the feature. Perhaps if drivers "like" ath10k could use it, maybe you
> > > > should add a patch(s) to the series where one of those drivers
> > > > actually uses the feature.  An API without an example of use is also
> > > > harder to evaluate effectively.
> > > 
> > > I agree driver patch using the new NoAck infrastructure would help 
> > > with
> > > understanding, ill post it once it is ready. But not sure the driver 
> > > patch
> > > can be part of the same series.
> > 
> > The API patches would go in mac80211-next tree and indeed the driver
> > patch would go through wireless-drivers-next tree. However, an option
> > would be to add the driver patch(es) as RFC in the series so Johannes
> > can ignore it and we still have an example to look at.

FWIW, you can just include it as a regular [PATCH] - Kalle and I have a
way of dealing with that - he just assigns it over to me initially, and
I assign back once the prerequisites have landed.

johannes

Re: [RFC 0/4] wireless: Per-sta NoAck and offload support

[Steve deRosier]

On Tue, Mar 27, 2018 at 11:09 PM,  <vthiagar@codeaurora.org> wrote:
> On 2018-03-27 22:18, Steve deRosier wrote:
>>
>> Hi Vasanthakumar,
>>
>> On Tue, Mar 27, 2018 at 1:42 AM, Vasanthakumar Thiagarajan
>> <vthiagar@codeaurora.org> wrote:
>>>
>>> Adds infrastructure for driver to offload NoAck functionality, driver
>>> like ath10k could make use of it. Also extends the current ndev wide
>>
>>
>> I'm not really much of a fan of adding a feature without some use of
>> the feature. Perhaps if drivers "like" ath10k could use it, maybe you
>> should add a patch(s) to the series where one of those drivers
>> actually uses the feature.  An API without an example of use is also
>> harder to evaluate effectively.
>
>
> I agree driver patch using the new NoAck infrastructure would help with
> understanding, ill post it once it is ready. But not sure the driver patch
> can be part of the same series.
>

It can. I think both Arend and Johannes already covered it.


>>
>> Additionally, if it's relevant, adding use of the feature to hwsim
>> would both serve the above comment as well as provide testing
>> capability.
>
>
> Does not seem like this offload feature is something applicable for hwsim
> especially mac80211 already offers the same functionality.
>

Well, if we desire hwsim to be able to test all the features of
mac80211 (which I don't know if that's true), then it would be
appropriate to place the functionality in hwsim as an optional
turn-on-able feature and have it utilize this API if it's turned on.
It actually would be nice to add it for that purpose. But, admittedly
it's a bit of work as you have to replicate the "hardware" offload
portion in hwsim which obviously you don't if you're working with
actual hardware that implements this feature.

I'd say it's a nice-to-have. If only to keep hwsim in-sync with
mac80211 features for testing. But, I admit I'm asking for work that
is perhaps out-of-scope.

All I really want is a driver that actually uses this as an example of use.

>
> The NoAck configuration is a bitmap of tid which is used to set NoAck in Qos
> control field of the data frame for that particular tid. Perhaps you could
> look at Ack policy subfield section in 802.11 spec.
>

Thank you. Because you gave it a name, I thought we were talking about
something more...involved. I'd appreciate if you pointed out that
context in the commit comment of the first patch in the series.
Something mentioning the Ack policy subfield specifically would give
context for those of us trying to tie it to specific 802.11
specifications.

Thanks,
- Steve