[PATCH v2 0/2] cfg80211/mac80211: last ack signal support in station dump

[PATCH v2 0/2] cfg80211/mac80211: last ack signal support in station dump

[Balaji Pothunoori]

This patch adds "last ack signal" support in station dump if
driver reports ack rssi for last tx packet.


Balaji Pothunoori (2):
  cfg80211: last ack signal support in station dump
  mac80211: last ack signal support in station dump

v2:
 - typo corrected in subject
 
 include/uapi/linux/nl80211.h | 14 +++++++-------
 net/mac80211/sta_info.c      | 20 ++++++++++++++------
 net/wireless/nl80211.c       |  8 ++++----
 3 files changed, 25 insertions(+), 17 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] cfg80211: last ack signal support in station dump

[Balaji Pothunoori]

This patch adds "last ack signal" support in station dump if
driver supports.

Signed-off-by: Balaji Pothunoori <bpothuno@codeaurora.org>
---
v2:
 - typo corrected in subject

 include/uapi/linux/nl80211.h | 14 +++++++-------
 net/wireless/nl80211.c       |  8 ++++----
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 28b3654..3514bef 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -3000,8 +3000,8 @@ enum nl80211_sta_bss_param {
  *	received from the station (u64, usec)
  * @NL80211_STA_INFO_PAD: attribute used for padding for 64-bit alignment
  * @NL80211_STA_INFO_ACK_SIGNAL: signal strength of the last ACK frame(u8, dBm)
- * @NL80211_STA_INFO_DATA_ACK_SIGNAL_AVG: avg signal strength of (data)
- *	ACK frame (s8, dBm)
+ * @NL80211_STA_INFO_ACK_SIGNAL_AVG: avg signal strength of data or management
+ *	ACK frames(s8, dBm)
  * @__NL80211_STA_INFO_AFTER_LAST: internal
  * @NL80211_STA_INFO_MAX: highest possible station info attribute
  */
@@ -3041,7 +3041,7 @@ enum nl80211_sta_info {
 	NL80211_STA_INFO_RX_DURATION,
 	NL80211_STA_INFO_PAD,
 	NL80211_STA_INFO_ACK_SIGNAL,
-	NL80211_STA_INFO_DATA_ACK_SIGNAL_AVG,
+	NL80211_STA_INFO_ACK_SIGNAL_AVG,
 
 	/* keep last */
 	__NL80211_STA_INFO_AFTER_LAST,
@@ -5128,9 +5128,9 @@ enum nl80211_feature_flags {
  *	"radar detected" event.
  * @NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211: Driver supports sending and
  *	receiving control port frames over nl80211 instead of the netdevice.
- * @NL80211_EXT_FEATURE_DATA_ACK_SIGNAL_SUPPORT: This Driver support data ack
- *	rssi if firmware support, this flag is to intimate about ack rssi
- *	support to nl80211.
+ * @NL80211_EXT_FEATURE_ACK_SIGNAL_SUPPORT: This Driver support ack rssi if
+ *	firmware support, this flag is to intimate about ack rssi support
+ *	to nl80211.
  * @NL80211_EXT_FEATURE_TXQS: Driver supports FQ-CoDel-enabled intermediate
  *      TXQs.
  *
@@ -5165,7 +5165,7 @@ enum nl80211_ext_feature_index {
 	NL80211_EXT_FEATURE_HIGH_ACCURACY_SCAN,
 	NL80211_EXT_FEATURE_DFS_OFFLOAD,
 	NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211,
-	NL80211_EXT_FEATURE_DATA_ACK_SIGNAL_SUPPORT,
+	NL80211_EXT_FEATURE_ACK_SIGNAL_SUPPORT,
 	NL80211_EXT_FEATURE_TXQS,
 
 	/* add new features before the definition below */
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 07514ca..29cf5fd 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -4650,11 +4650,11 @@ static int nl80211_send_station(struct sk_buff *msg, u32 cmd, u32 portid,
 	PUT_SINFO_U64(RX_DROP_MISC, rx_dropped_misc);
 	PUT_SINFO_U64(BEACON_RX, rx_beacon);
 	PUT_SINFO(BEACON_SIGNAL_AVG, rx_beacon_signal_avg, u8);
-	PUT_SINFO(ACK_SIGNAL, ack_signal, u8);
 	if (wiphy_ext_feature_isset(&rdev->wiphy,
-				    NL80211_EXT_FEATURE_DATA_ACK_SIGNAL_SUPPORT))
-		PUT_SINFO(DATA_ACK_SIGNAL_AVG, avg_ack_signal, s8);
-
+				    NL80211_EXT_FEATURE_ACK_SIGNAL_SUPPORT)) {
+		PUT_SINFO(ACK_SIGNAL, ack_signal, u8);
+		PUT_SINFO(ACK_SIGNAL_AVG, avg_ack_signal, s8);
+	}
 #undef PUT_SINFO
 #undef PUT_SINFO_U64
 
-- 
2.7.4

[PATCH v2 2/2] mac80211: last ack signal support in station dump

[Balaji Pothunoori]

This patch adds "last ack signal" and "avg ack signal" support
in station dump for valid ack rssi.

Signed-off-by: Balaji Pothunoori <bpothuno@codeaurora.org>
---
v2:
 - typo corrected in subject

 net/mac80211/sta_info.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 6428f1a..12b618e 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -2310,13 +2310,21 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo,
 		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_ACK_SIGNAL);
 	}
 
-	if (ieee80211_hw_check(&sta->local->hw, REPORTS_TX_ACK_STATUS) &&
-	    !(sinfo->filled & BIT_ULL(NL80211_STA_INFO_DATA_ACK_SIGNAL_AVG))) {
-		sinfo->avg_ack_signal =
-			-(s8)ewma_avg_signal_read(
+	if (ieee80211_hw_check(&sta->local->hw, REPORTS_TX_ACK_STATUS)) {
+		if (sta->status_stats.ack_signal_filled && ((!(sinfo->filled &
+		    BIT_ULL(NL80211_STA_INFO_ACK_SIGNAL))) ||
+		    (!(sinfo->filled &
+		    BIT_ULL(NL80211_STA_INFO_ACK_SIGNAL_AVG))))) {
+			sinfo->ack_signal =
+				sta->status_stats.last_ack_signal;
+			sinfo->filled |=
+				BIT_ULL(NL80211_STA_INFO_ACK_SIGNAL);
+			sinfo->avg_ack_signal =
+				-(s8)ewma_avg_signal_read(
 				&sta->status_stats.avg_ack_signal);
-		sinfo->filled |=
-			BIT_ULL(NL80211_STA_INFO_DATA_ACK_SIGNAL_AVG);
+			sinfo->filled |=
+				BIT_ULL(NL80211_STA_INFO_ACK_SIGNAL_AVG);
+		}
 	}
 }
 
-- 
2.7.4

Re: [PATCH v2 1/2] cfg80211: last ack signal support in station dump

[Johannes Berg]

On Mon, 2018-06-04 at 19:41 +0530, Balaji Pothunoori wrote:
> 
> +++ b/include/uapi/linux/nl80211.h
> @@ -3000,8 +3000,8 @@ enum nl80211_sta_bss_param {
>   *	received from the station (u64, usec)
>   * @NL80211_STA_INFO_PAD: attribute used for padding for 64-bit alignment
>   * @NL80211_STA_INFO_ACK_SIGNAL: signal strength of the last ACK frame(u8, dBm)
> - * @NL80211_STA_INFO_DATA_ACK_SIGNAL_AVG: avg signal strength of (data)
> - *	ACK frame (s8, dBm)
> + * @NL80211_STA_INFO_ACK_SIGNAL_AVG: avg signal strength of data or management
> + *	ACK frames(s8, dBm)
>   * @__NL80211_STA_INFO_AFTER_LAST: internal
>   * @NL80211_STA_INFO_MAX: highest possible station info attribute
>   */
> @@ -3041,7 +3041,7 @@ enum nl80211_sta_info {
>  	NL80211_STA_INFO_RX_DURATION,
>  	NL80211_STA_INFO_PAD,
>  	NL80211_STA_INFO_ACK_SIGNAL,
> -	NL80211_STA_INFO_DATA_ACK_SIGNAL_AVG,
> +	NL80211_STA_INFO_ACK_SIGNAL_AVG,

Wait, what happened here? You can't remove old API.

> @@ -5128,9 +5128,9 @@ enum nl80211_feature_flags {
>   *	"radar detected" event.
>   * @NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211: Driver supports sending and
>   *	receiving control port frames over nl80211 instead of the netdevice.
> - * @NL80211_EXT_FEATURE_DATA_ACK_SIGNAL_SUPPORT: This Driver support data ack
> - *	rssi if firmware support, this flag is to intimate about ack rssi
> - *	support to nl80211.
> + * @NL80211_EXT_FEATURE_ACK_SIGNAL_SUPPORT: This Driver support ack rssi if
> + *	firmware support, this flag is to intimate about ack rssi support
> + *	to nl80211.

Same here, why are you removing the data-ack-signal API?

Is that just a rebase error or something?

Or is it intentional, but then please explain what you're trying to do
and I can help find a correct solutions.

johannes

Re: [PATCH v2 2/2] mac80211: last ack signal support in station dump

[Johannes Berg]

On Mon, 2018-06-04 at 19:41 +0530, Balaji Pothunoori wrote:
> 
> +	if (ieee80211_hw_check(&sta->local->hw, REPORTS_TX_ACK_STATUS)) {
> +		if (sta->status_stats.ack_signal_filled && ((!(sinfo->filled &
> +		    BIT_ULL(NL80211_STA_INFO_ACK_SIGNAL))) ||
> +		    (!(sinfo->filled &
> +		    BIT_ULL(NL80211_STA_INFO_ACK_SIGNAL_AVG))))) {

Uh, wow, the indentation here is awful - please break with && and || on
the end of line and indent properly according to the nesting level.

If a line ends up being longer than 80, I think that's better than this
monster expression :)

> +			sinfo->ack_signal =
> +				sta->status_stats.last_ack_signal;
> +			sinfo->filled |=
> +				BIT_ULL(NL80211_STA_INFO_ACK_SIGNAL);
> +			sinfo->avg_ack_signal =
> +				-(s8)ewma_avg_signal_read(
>  				&sta->status_stats.avg_ack_signal);
> -		sinfo->filled |=
> -			BIT_ULL(NL80211_STA_INFO_DATA_ACK_SIGNAL_AVG);
> +			sinfo->filled |=
> +				BIT_ULL(NL80211_STA_INFO_ACK_SIGNAL_AVG);
> +		}

Clearly your previous patch would even break the kernel compile since
the DATA_ACK_SIGNAL_AVG is still used here.



Finally, please also explain why you're adding this feature, at least in
the cover letter ("PATCH 0/2"), I can reuse that as a merge commit
message if necessary.

johannes

Re: [PATCH v2 1/2] cfg80211: last ack signal support in station dump

[Balaji Pothunoori]

On 2018-06-15 17:11, Johannes Berg wrote:
> On Mon, 2018-06-04 at 19:41 +0530, Balaji Pothunoori wrote:
>> 
>> +++ b/include/uapi/linux/nl80211.h
>> @@ -3000,8 +3000,8 @@ enum nl80211_sta_bss_param {
>>   *	received from the station (u64, usec)
>>   * @NL80211_STA_INFO_PAD: attribute used for padding for 64-bit 
>> alignment
>>   * @NL80211_STA_INFO_ACK_SIGNAL: signal strength of the last ACK 
>> frame(u8, dBm)
>> - * @NL80211_STA_INFO_DATA_ACK_SIGNAL_AVG: avg signal strength of 
>> (data)
>> - *	ACK frame (s8, dBm)
>> + * @NL80211_STA_INFO_ACK_SIGNAL_AVG: avg signal strength of data or 
>> management
>> + *	ACK frames(s8, dBm)
>>   * @__NL80211_STA_INFO_AFTER_LAST: internal
>>   * @NL80211_STA_INFO_MAX: highest possible station info attribute
>>   */
>> @@ -3041,7 +3041,7 @@ enum nl80211_sta_info {
>>  	NL80211_STA_INFO_RX_DURATION,
>>  	NL80211_STA_INFO_PAD,
>>  	NL80211_STA_INFO_ACK_SIGNAL,
>> -	NL80211_STA_INFO_DATA_ACK_SIGNAL_AVG,
>> +	NL80211_STA_INFO_ACK_SIGNAL_AVG,
> 
> Wait, what happened here? You can't remove old API.

Here is my intention is to make the unique average ack signal and last 
ack signal
support in station dump irrespective of data or management tx ack 
packet.
Do you want me to add a new API for management tx ack packet?

> 
>> @@ -5128,9 +5128,9 @@ enum nl80211_feature_flags {
>>   *	"radar detected" event.
>>   * @NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211: Driver supports 
>> sending and
>>   *	receiving control port frames over nl80211 instead of the 
>> netdevice.
>> - * @NL80211_EXT_FEATURE_DATA_ACK_SIGNAL_SUPPORT: This Driver support 
>> data ack
>> - *	rssi if firmware support, this flag is to intimate about ack rssi
>> - *	support to nl80211.
>> + * @NL80211_EXT_FEATURE_ACK_SIGNAL_SUPPORT: This Driver support ack 
>> rssi if
>> + *	firmware support, this flag is to intimate about ack rssi support
>> + *	to nl80211.
> 
> Same here, why are you removing the data-ack-signal API?
> 
> Is that just a rebase error or something?
> 
> Or is it intentional, but then please explain what you're trying to do
> and I can help find a correct solutions.
> 
> johannes

Here i renamed above API because in driver i will fill this 
"NL80211_EXT_FEATURE_ACK_SIGNAL_SUPPORT"
if firmware supports either of the one "WMI_SERVICE_TX_DATA_ACK_RSSI" or 
"WMI_SERVICE_HTT_MGMT_TX_COMP_VALID_FLAGS".
This is to make unique avg ack signal/last ack signal support.

Regards,
Balaji.

Re: [PATCH v2 2/2] mac80211: last ack signal support in station dump

[Balaji Pothunoori]

On 2018-06-15 17:13, Johannes Berg wrote:
> On Mon, 2018-06-04 at 19:41 +0530, Balaji Pothunoori wrote:
>> 
>> +	if (ieee80211_hw_check(&sta->local->hw, REPORTS_TX_ACK_STATUS)) {
>> +		if (sta->status_stats.ack_signal_filled && ((!(sinfo->filled &
>> +		    BIT_ULL(NL80211_STA_INFO_ACK_SIGNAL))) ||
>> +		    (!(sinfo->filled &
>> +		    BIT_ULL(NL80211_STA_INFO_ACK_SIGNAL_AVG))))) {
> 
> Uh, wow, the indentation here is awful - please break with && and || on
> the end of line and indent properly according to the nesting level.
> 
> If a line ends up being longer than 80, I think that's better than this
> monster expression :)

Sure , i will modify in v3.

> 
>> +			sinfo->ack_signal =
>> +				sta->status_stats.last_ack_signal;
>> +			sinfo->filled |=
>> +				BIT_ULL(NL80211_STA_INFO_ACK_SIGNAL);
>> +			sinfo->avg_ack_signal =
>> +				-(s8)ewma_avg_signal_read(
>>  				&sta->status_stats.avg_ack_signal);
>> -		sinfo->filled |=
>> -			BIT_ULL(NL80211_STA_INFO_DATA_ACK_SIGNAL_AVG);
>> +			sinfo->filled |=
>> +				BIT_ULL(NL80211_STA_INFO_ACK_SIGNAL_AVG);
>> +		}
> 
> Clearly your previous patch would even break the kernel compile since
> the DATA_ACK_SIGNAL_AVG is still used here.

Here i have used "NL80211_STA_INFO_ACK_SIGNAL_AVG" not as 
"NL80211_STA_INFO_DATA_ACK_SIGNAL_AVG".

> 
> 
> 
> Finally, please also explain why you're adding this feature, at least 
> in
> the cover letter ("PATCH 0/2"), I can reuse that as a merge commit
> message if necessary.

Sure, i will update in v3.

> 
> johannes

Regards,
Balaji.

Re: [PATCH v2 1/2] cfg80211: last ack signal support in station dump

[Johannes Berg]

On Mon, 2018-06-18 at 14:48 +0530, Balaji Pothunoori wrote:
> On 2018-06-15 17:11, Johannes Berg wrote:
> > On Mon, 2018-06-04 at 19:41 +0530, Balaji Pothunoori wrote:
> > > 
> > > +++ b/include/uapi/linux/nl80211.h
> > > @@ -3000,8 +3000,8 @@ enum nl80211_sta_bss_param {
> > >   *	received from the station (u64, usec)
> > >   * @NL80211_STA_INFO_PAD: attribute used for padding for 64-bit 
> > > alignment
> > >   * @NL80211_STA_INFO_ACK_SIGNAL: signal strength of the last ACK 
> > > frame(u8, dBm)
> > > - * @NL80211_STA_INFO_DATA_ACK_SIGNAL_AVG: avg signal strength of 
> > > (data)
> > > - *	ACK frame (s8, dBm)
> > > + * @NL80211_STA_INFO_ACK_SIGNAL_AVG: avg signal strength of data or 
> > > management
> > > + *	ACK frames(s8, dBm)
> > >   * @__NL80211_STA_INFO_AFTER_LAST: internal
> > >   * @NL80211_STA_INFO_MAX: highest possible station info attribute
> > >   */
> > > @@ -3041,7 +3041,7 @@ enum nl80211_sta_info {
> > >  	NL80211_STA_INFO_RX_DURATION,
> > >  	NL80211_STA_INFO_PAD,
> > >  	NL80211_STA_INFO_ACK_SIGNAL,
> > > -	NL80211_STA_INFO_DATA_ACK_SIGNAL_AVG,
> > > +	NL80211_STA_INFO_ACK_SIGNAL_AVG,
> > 
> > Wait, what happened here? You can't remove old API.
> 
> Here is my intention is to make the unique average ack signal and last 
> ack signal
> support in station dump irrespective of data or management tx ack 
> packet.
> Do you want me to add a new API for management tx ack packet?

Well, you can't remove old API.

And the data-ACK was explicitly added because of concerns about signal
varying a lot with MCS, so ACK signal is more reliable.

I suppose the original use here didn't really *need* just *data* ACK, so
perhaps we can redefine it - you'd know better? After all, you defined
the old API :-))

But still, you can't just remove it. I think we're probably OK to
redefine it but then you need to keep API compatibility by adding the
necessary defines.

johannes