[PATCHv2 0/2] configure dynack through mac80211 stack

[PATCHv2 0/2] configure dynack through mac80211 stack

[Lorenzo Bianconi]

Extend set_coverage_class API in order to enable ACK timeout estimation
algorithm (dynack). Currently dynack is supported just by ath9k. In a different
patchset I will fix signature issue for p54, ath9k_htc and ath5k drivers.

Changes since v1:
- extend set_coverage_class signature insted of define enable_dynack API

Lorenzo Bianconi (2):
  cfg80211: extend coverage_class datatype
  mac80211: extend set_coverage_class signature

 include/net/cfg80211.h       |  5 +++--
 include/net/mac80211.h       |  5 +++--
 include/uapi/linux/nl80211.h |  5 +++++
 net/mac80211/driver-ops.h    |  9 +++------
 net/mac80211/trace.h         |  4 ++--
 net/wireless/nl80211.c       | 14 ++++++++++----
 6 files changed, 26 insertions(+), 16 deletions(-)

-- 
1.9.1

[PATCHv2 1/2] cfg80211: extend coverage_class datatype

[Lorenzo Bianconi]

Use s16 as datatype for coverage_class in order to enable ack timeout
estimation algorithm through nl80211

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
---
 include/net/cfg80211.h       |  5 +++--
 include/uapi/linux/nl80211.h |  5 +++++
 net/wireless/nl80211.c       | 14 ++++++++++----
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 0a080c4..1c8e281 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2876,7 +2876,8 @@ struct wiphy_vendor_command {
  *	include fixed IEs like supported rates
  * @max_sched_scan_ie_len: same as max_scan_ie_len, but for scheduled
  *	scans
- * @coverage_class: current coverage class
+ * @coverage_class: current coverage class, -1 if dynamic ack timeout
+ *	estimation has been enabled
  * @fw_version: firmware version for ethtool reporting
  * @hw_version: hardware version for ethtool reporting
  * @max_num_pmkids: maximum number of PMKIDs supported by device
@@ -2990,7 +2991,7 @@ struct wiphy {
 	u8 retry_long;
 	u32 frag_threshold;
 	u32 rts_threshold;
-	u8 coverage_class;
+	s16 coverage_class;
 
 	char fw_version[ETHTOOL_FWVERS_LEN];
 	u32 hw_version;
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index f1db15b..7cc5ecd 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1594,6 +1594,9 @@ enum nl80211_commands {
  * @NL80211_ATTR_TDLS_INITIATOR: flag attribute indicating the current end is
  *	the TDLS link initiator.
  *
+ * @NL80211_ATTR_WIPHY_DYNACK: whether dynamic ack timeout estimation algorithm
+ *	is enabled
+ *
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
  */
@@ -1936,6 +1939,8 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_TDLS_INITIATOR,
 
+	NL80211_ATTR_WIPHY_DYNACK,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index df7b133..c097827 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -225,6 +225,7 @@ static const struct nla_policy nl80211_policy[NL80211_ATTR_MAX+1] = {
 	[NL80211_ATTR_WIPHY_FRAG_THRESHOLD] = { .type = NLA_U32 },
 	[NL80211_ATTR_WIPHY_RTS_THRESHOLD] = { .type = NLA_U32 },
 	[NL80211_ATTR_WIPHY_COVERAGE_CLASS] = { .type = NLA_U8 },
+	[NL80211_ATTR_WIPHY_DYNACK] = { .type = NLA_FLAG },
 
 	[NL80211_ATTR_IFTYPE] = { .type = NLA_U32 },
 	[NL80211_ATTR_IFINDEX] = { .type = NLA_U32 },
@@ -1269,8 +1270,8 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 				rdev->wiphy.frag_threshold) ||
 		    nla_put_u32(msg, NL80211_ATTR_WIPHY_RTS_THRESHOLD,
 				rdev->wiphy.rts_threshold) ||
-		    nla_put_u8(msg, NL80211_ATTR_WIPHY_COVERAGE_CLASS,
-			       rdev->wiphy.coverage_class) ||
+		    nla_put_s16(msg, NL80211_ATTR_WIPHY_COVERAGE_CLASS,
+				rdev->wiphy.coverage_class) ||
 		    nla_put_u8(msg, NL80211_ATTR_MAX_NUM_SCAN_SSIDS,
 			       rdev->wiphy.max_scan_ssids) ||
 		    nla_put_u8(msg, NL80211_ATTR_MAX_NUM_SCHED_SCAN_SSIDS,
@@ -2046,7 +2047,7 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 	u32 changed;
 	u8 retry_short = 0, retry_long = 0;
 	u32 frag_threshold = 0, rts_threshold = 0;
-	u8 coverage_class = 0;
+	s16 coverage_class = 0;
 
 	ASSERT_RTNL();
 
@@ -2242,10 +2243,15 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 		changed |= WIPHY_PARAM_COVERAGE_CLASS;
 	}
 
+	if (info->attrs[NL80211_ATTR_WIPHY_DYNACK]) {
+		coverage_class = -1;
+		changed |= WIPHY_PARAM_COVERAGE_CLASS;
+	}
+
 	if (changed) {
 		u8 old_retry_short, old_retry_long;
 		u32 old_frag_threshold, old_rts_threshold;
-		u8 old_coverage_class;
+		s16 old_coverage_class;
 
 		if (!rdev->ops->set_wiphy_params)
 			return -EOPNOTSUPP;
-- 
1.9.1

[PATCHv2 2/2] mac80211: extend set_coverage_class signature

[Lorenzo Bianconi]

Extend set_coverage_class API in order to enable ACK timeout estimation
algorithm (dynack).

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
---
 include/net/mac80211.h    | 5 +++--
 net/mac80211/driver-ops.h | 9 +++------
 net/mac80211/trace.h      | 4 ++--
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index dae2e24..30fd354 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2666,7 +2666,8 @@ enum ieee80211_roc_type {
  *
  * @set_coverage_class: Set slot time for given coverage class as specified
  *	in IEEE 802.11-2007 section 17.3.8.6 and modify ACK timeout
- *	accordingly. This callback is not required and may sleep.
+ *	accordingly; coverage_class equals to -1 to enable ACK timeout
+ *	estimation algorithm. This callback is not required and may sleep.
  *
  * @testmode_cmd: Implement a cfg80211 test mode command. The passed @vif may
  *	be %NULL. The callback can sleep.
@@ -2950,7 +2951,7 @@ struct ieee80211_ops {
 	int (*get_survey)(struct ieee80211_hw *hw, int idx,
 		struct survey_info *survey);
 	void (*rfkill_poll)(struct ieee80211_hw *hw);
-	void (*set_coverage_class)(struct ieee80211_hw *hw, u8 coverage_class);
+	int (*set_coverage_class)(struct ieee80211_hw *hw, s16 coverage_class);
 #ifdef CONFIG_NL80211_TESTMODE
 	int (*testmode_cmd)(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 			    void *data, int len);
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 1142395..68a840d 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -450,17 +450,14 @@ static inline int drv_set_rts_threshold(struct ieee80211_local *local,
 }
 
 static inline int drv_set_coverage_class(struct ieee80211_local *local,
-					 u8 value)
+					 s16 value)
 {
-	int ret = 0;
+	int ret = -EOPNOTSUPP;
 	might_sleep();
 
 	trace_drv_set_coverage_class(local, value);
 	if (local->ops->set_coverage_class)
-		local->ops->set_coverage_class(&local->hw, value);
-	else
-		ret = -EOPNOTSUPP;
-
+		ret = local->ops->set_coverage_class(&local->hw, value);
 	trace_drv_return_int(local, ret);
 	return ret;
 }
diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
index 02ac535..38fae7e 100644
--- a/net/mac80211/trace.h
+++ b/net/mac80211/trace.h
@@ -672,13 +672,13 @@ DEFINE_EVENT(local_u32_evt, drv_set_rts_threshold,
 );
 
 TRACE_EVENT(drv_set_coverage_class,
-	TP_PROTO(struct ieee80211_local *local, u8 value),
+	TP_PROTO(struct ieee80211_local *local, s16 value),
 
 	TP_ARGS(local, value),
 
 	TP_STRUCT__entry(
 		LOCAL_ENTRY
-		__field(u8, value)
+		__field(s16, value)
 	),
 
 	TP_fast_assign(
-- 
1.9.1

Re: [PATCHv2 1/2] cfg80211: extend coverage_class datatype

[Johannes Berg]

On Thu, 2014-07-24 at 12:39 +0200, Lorenzo Bianconi wrote:

> @@ -1269,8 +1270,8 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
>  				rdev->wiphy.frag_threshold) ||
>  		    nla_put_u32(msg, NL80211_ATTR_WIPHY_RTS_THRESHOLD,
>  				rdev->wiphy.rts_threshold) ||
> -		    nla_put_u8(msg, NL80211_ATTR_WIPHY_COVERAGE_CLASS,
> -			       rdev->wiphy.coverage_class) ||
> +		    nla_put_s16(msg, NL80211_ATTR_WIPHY_COVERAGE_CLASS,
> +				rdev->wiphy.coverage_class) ||
>  		    nla_put_u8(msg, NL80211_ATTR_MAX_NUM_SCAN_SSIDS,
>  			       rdev->wiphy.max_scan_ssids) ||
>  		    nla_put_u8(msg, NL80211_ATTR_MAX_NUM_SCHED_SCAN_SSIDS,

This will break existing userspace.

Also please supply a better commit message.

johannes

Re: [PATCHv2 1/2] cfg80211: extend coverage_class datatype

[Johannes Berg]

On Thu, 2014-07-24 at 12:39 +0200, Lorenzo Bianconi wrote:

> @@ -2242,10 +2243,15 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
>  		changed |= WIPHY_PARAM_COVERAGE_CLASS;
>  	}
>  
> +	if (info->attrs[NL80211_ATTR_WIPHY_DYNACK]) {
> +		coverage_class = -1;
> +		changed |= WIPHY_PARAM_COVERAGE_CLASS;
> +	}

I think you should also reject having both dyn and fixed attributes.

You should also have a feature flag that drivers set that you test here
and refuse dynamic if it's not supported, I guess?

johannes

Re: [PATCHv2 2/2] mac80211: extend set_coverage_class signature

[Johannes Berg]

On Thu, 2014-07-24 at 12:39 +0200, Lorenzo Bianconi wrote:

> @@ -2950,7 +2951,7 @@ struct ieee80211_ops {
>  	int (*get_survey)(struct ieee80211_hw *hw, int idx,
>  		struct survey_info *survey);
>  	void (*rfkill_poll)(struct ieee80211_hw *hw);
> -	void (*set_coverage_class)(struct ieee80211_hw *hw, u8 coverage_class);
> +	int (*set_coverage_class)(struct ieee80211_hw *hw, s16 coverage_class);

This patch also needs to update all drivers ......

johannes

Re: [PATCHv2 2/2] mac80211: extend set_coverage_class signature

[Lorenzo Bianconi]

> On Thu, 2014-07-24 at 12:39 +0200, Lorenzo Bianconi wrote:
>
>> @@ -2950,7 +2951,7 @@ struct ieee80211_ops {
>>       int (*get_survey)(struct ieee80211_hw *hw, int idx,
>>               struct survey_info *survey);
>>       void (*rfkill_poll)(struct ieee80211_hw *hw);
>> -     void (*set_coverage_class)(struct ieee80211_hw *hw, u8 coverage_class);
>> +     int (*set_coverage_class)(struct ieee80211_hw *hw, s16 coverage_class);
>
> This patch also needs to update all drivers ......

Currently just ath9k, ath5k and p54 implement set_coverage_class
function pointer. I would send a different patch for these changes.
Should I include them in this patchset?

>
> johannes
>

Lorenzo



-- 
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep

Re: [PATCHv2 1/2] cfg80211: extend coverage_class datatype

[Lorenzo Bianconi]

> On Thu, 2014-07-24 at 12:39 +0200, Lorenzo Bianconi wrote:
>
>> @@ -2242,10 +2243,15 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
>>               changed |= WIPHY_PARAM_COVERAGE_CLASS;
>>       }
>>
>> +     if (info->attrs[NL80211_ATTR_WIPHY_DYNACK]) {
>> +             coverage_class = -1;
>> +             changed |= WIPHY_PARAM_COVERAGE_CLASS;
>> +     }
>
> I think you should also reject having both dyn and fixed attributes.
>

Currently iw does not allow to set both attribute at the same time,
but I can use "if-else if" statement in order to exclude that
condition.

> You should also have a feature flag that drivers set that you test here
> and refuse dynamic if it's not supported, I guess?
>

I would rely on lower drivers to fail (returning -EOPNOTSUPP to
set_coverage_class caller) if dynack it is not supported and user is
trying to enable it. Does it sound good for you?

> johannes
>

Lorenzo


-- 
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep

Re: [PATCHv2 1/2] cfg80211: extend coverage_class datatype

[Lorenzo Bianconi]

> On Thu, 2014-07-24 at 12:39 +0200, Lorenzo Bianconi wrote:
>
>> @@ -1269,8 +1270,8 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
>>                               rdev->wiphy.frag_threshold) ||
>>                   nla_put_u32(msg, NL80211_ATTR_WIPHY_RTS_THRESHOLD,
>>                               rdev->wiphy.rts_threshold) ||
>> -                 nla_put_u8(msg, NL80211_ATTR_WIPHY_COVERAGE_CLASS,
>> -                            rdev->wiphy.coverage_class) ||
>> +                 nla_put_s16(msg, NL80211_ATTR_WIPHY_COVERAGE_CLASS,
>> +                             rdev->wiphy.coverage_class) ||
>>                   nla_put_u8(msg, NL80211_ATTR_MAX_NUM_SCAN_SSIDS,
>>                              rdev->wiphy.max_scan_ssids) ||
>>                   nla_put_u8(msg, NL80211_ATTR_MAX_NUM_SCHED_SCAN_SSIDS,
>
> This will break existing userspace.
>
> Also please supply a better commit message.
>
> johannes
>

Ack

-- 
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep

Re: [PATCHv2 1/2] cfg80211: extend coverage_class datatype

[Johannes Berg]

On Thu, 2014-07-24 at 14:21 +0200, Lorenzo Bianconi wrote:

> Currently iw does not allow to set both attribute at the same time,
> but I can use "if-else if" statement in order to exclude that
> condition.

I think you should explicitly reject the configuration if both
attributes are given, not silently use one of them.

> > You should also have a feature flag that drivers set that you test here
> > and refuse dynamic if it's not supported, I guess?
> >
> 
> I would rely on lower drivers to fail (returning -EOPNOTSUPP to
> set_coverage_class caller) if dynack it is not supported and user is
> trying to enable it. Does it sound good for you?

I imagined you'd want to know in 'iw list' whether or not it's supported
too.

johannes

Re: [PATCHv2 2/2] mac80211: extend set_coverage_class signature

[Johannes Berg]

On Thu, 2014-07-24 at 14:12 +0200, Lorenzo Bianconi wrote:
> > On Thu, 2014-07-24 at 12:39 +0200, Lorenzo Bianconi wrote:
> >
> >> @@ -2950,7 +2951,7 @@ struct ieee80211_ops {
> >>       int (*get_survey)(struct ieee80211_hw *hw, int idx,
> >>               struct survey_info *survey);
> >>       void (*rfkill_poll)(struct ieee80211_hw *hw);
> >> -     void (*set_coverage_class)(struct ieee80211_hw *hw, u8 coverage_class);
> >> +     int (*set_coverage_class)(struct ieee80211_hw *hw, s16 coverage_class);
> >
> > This patch also needs to update all drivers ......
> 
> Currently just ath9k, ath5k and p54 implement set_coverage_class
> function pointer. I would send a different patch for these changes.
> Should I include them in this patchset?

Guess what will happen if I merge this patch?

Hint: the kernel will not compile correctly any more.

johannes

Re: [PATCHv2 2/2] mac80211: extend set_coverage_class signature

[Lorenzo Bianconi]

> On Thu, 2014-07-24 at 14:12 +0200, Lorenzo Bianconi wrote:
>> > On Thu, 2014-07-24 at 12:39 +0200, Lorenzo Bianconi wrote:
>> >
>> >> @@ -2950,7 +2951,7 @@ struct ieee80211_ops {
>> >>       int (*get_survey)(struct ieee80211_hw *hw, int idx,
>> >>               struct survey_info *survey);
>> >>       void (*rfkill_poll)(struct ieee80211_hw *hw);
>> >> -     void (*set_coverage_class)(struct ieee80211_hw *hw, u8 coverage_class);
>> >> +     int (*set_coverage_class)(struct ieee80211_hw *hw, s16 coverage_class);
>> >
>> > This patch also needs to update all drivers ......
>>
>> Currently just ath9k, ath5k and p54 implement set_coverage_class
>> function pointer. I would send a different patch for these changes.
>> Should I include them in this patchset?
>
> Guess what will happen if I merge this patch?
>
> Hint: the kernel will not compile correctly any more.
>
> johannes
>

Ok, I will include changes to lower drivers in this patchset.

Lorenzo

-- 
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep

Re: [PATCHv2 1/2] cfg80211: extend coverage_class datatype

[Lorenzo Bianconi]

> On Thu, 2014-07-24 at 14:21 +0200, Lorenzo Bianconi wrote:
>
>> Currently iw does not allow to set both attribute at the same time,
>> but I can use "if-else if" statement in order to exclude that
>> condition.
>
> I think you should explicitly reject the configuration if both
> attributes are given, not silently use one of them.
>

Ack

>> > You should also have a feature flag that drivers set that you test here
>> > and refuse dynamic if it's not supported, I guess?
>> >
>>
>> I would rely on lower drivers to fail (returning -EOPNOTSUPP to
>> set_coverage_class caller) if dynack it is not supported and user is
>> trying to enable it. Does it sound good for you?
>
> I imagined you'd want to know in 'iw list' whether or not it's supported
> too.
>

Ok, I can add  an entry in nl80211_feature_flags, something like
NL80211_FEATURE_ACKTO_ESTIMATION. This flags will be set by lower
driver and will be checked to very if driver supports this feature.
Moreover NL80211_FEATURE_ACKTO_ESTIMATION will be reported to
userspace.

> johannes
>

Lorenzo

-- 
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep