[PATCH-v3 1/2] wireless: Support assoc-at-ms timer in sta-info

[PATCH-v3 1/2] wireless: Support assoc-at-ms timer in sta-info

[greearb]

From: Ben Greear <greearb@candelatech.com>

Report time stamp of when sta became associated.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 include/net/cfg80211.h       | 2 ++
 include/uapi/linux/nl80211.h | 2 ++
 net/wireless/nl80211.c       | 1 +
 3 files changed, 5 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index f49eb1464b7a..a3ad78b9d9f4 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1268,6 +1268,7 @@ struct cfg80211_tid_stats {
  *	indicate the relevant values in this struct for them
  * @connected_time: time(in secs) since a station is last connected
  * @inactive_time: time since last station activity (tx/rx) in milliseconds
+ * @assoc_at_ms: time in ms of the last association
  * @rx_bytes: bytes (size of MPDUs) received from this station
  * @tx_bytes: bytes (size of MPDUs) transmitted to this station
  * @llid: mesh local link id
@@ -1324,6 +1325,7 @@ struct station_info {
 	u64 filled;
 	u32 connected_time;
 	u32 inactive_time;
+	u64 assoc_at_ms;
 	u64 rx_bytes;
 	u64 tx_bytes;
 	u16 llid;
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 0f03cfcda965..0fcedde890e7 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -3079,6 +3079,7 @@ enum nl80211_sta_bss_param {
  * @NL80211_STA_INFO_TX_DURATION: aggregate PPDU duration for all frames
  *	sent to the station (u64, usec)
  * @NL80211_STA_INFO_AIRTIME_WEIGHT: current airtime weight for station (u16)
+ * @NL80211_STA_INFO_ASSOC_AT_MS: Timestamp of last association
  * @__NL80211_STA_INFO_AFTER_LAST: internal
  * @NL80211_STA_INFO_MAX: highest possible station info attribute
  */
@@ -3124,6 +3125,7 @@ enum nl80211_sta_info {
 	NL80211_STA_INFO_CONNECTED_TO_GATE,
 	NL80211_STA_INFO_TX_DURATION,
 	NL80211_STA_INFO_AIRTIME_WEIGHT,
+	NL80211_STA_INFO_ASSOC_AT_MS,
 
 	/* keep last */
 	__NL80211_STA_INFO_AFTER_LAST,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index bcb432815c64..cb5acf026900 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -4763,6 +4763,7 @@ static int nl80211_send_station(struct sk_buff *msg, u32 cmd, u32 portid,
 
 	PUT_SINFO(CONNECTED_TIME, connected_time, u32);
 	PUT_SINFO(INACTIVE_TIME, inactive_time, u32);
+	PUT_SINFO_U64(ASSOC_AT_MS, assoc_at_ms);
 
 	if (sinfo->filled & (BIT_ULL(NL80211_STA_INFO_RX_BYTES) |
 			     BIT_ULL(NL80211_STA_INFO_RX_BYTES64)) &&
-- 
2.20.1

[PATCH-v3 2/2] mac80211: add assoc-at-ms support.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Report when sta becomes associated.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 net/mac80211/sta_info.c | 3 +++
 net/mac80211/sta_info.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 044120c45950..918ca9475fec 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -1968,6 +1968,7 @@ int sta_info_move_state(struct sta_info *sta,
 	case IEEE80211_STA_ASSOC:
 		if (sta->sta_state == IEEE80211_STA_AUTH) {
 			set_bit(WLAN_STA_ASSOC, &sta->_flags);
+			sta->assoc_at_ms = ktime_to_ms(ktime_get_real());
 			ieee80211_recalc_min_chandef(sta->sdata);
 			if (!sta->sta.support_p2p_ps)
 				ieee80211_recalc_p2p_go_ps_allowed(sta->sdata);
@@ -2197,6 +2198,7 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo,
 			 BIT_ULL(NL80211_STA_INFO_STA_FLAGS) |
 			 BIT_ULL(NL80211_STA_INFO_BSS_PARAM) |
 			 BIT_ULL(NL80211_STA_INFO_CONNECTED_TIME) |
+			 BIT_ULL(NL80211_STA_INFO_ASSOC_AT_MS) |
 			 BIT_ULL(NL80211_STA_INFO_RX_DROP_MISC);
 
 	if (sdata->vif.type == NL80211_IFTYPE_STATION) {
@@ -2205,6 +2207,7 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo,
 	}
 
 	sinfo->connected_time = ktime_get_seconds() - sta->last_connected;
+	sinfo->assoc_at_ms = sta->assoc_at_ms;
 	sinfo->inactive_time =
 		jiffies_to_msecs(jiffies - ieee80211_sta_last_active(sta));
 
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index c5d557032be5..e94b942cd564 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -455,6 +455,7 @@ struct ieee80211_sta_rx_stats {
  *	the station when it leaves powersave or polls for frames
  * @driver_buffered_tids: bitmap of TIDs the driver has data buffered on
  * @txq_buffered_tids: bitmap of TIDs that mac80211 has txq data buffered on
+ * @assoc_at_ms: time (in ms) of last association
  * @last_connected: time (in seconds) when a station got connected
  * @last_seq_ctrl: last received seq/frag number from this STA (per TID
  *	plus one for non-QoS frames)
@@ -531,6 +532,7 @@ struct sta_info {
 	unsigned long driver_buffered_tids;
 	unsigned long txq_buffered_tids;
 
+	unsigned long assoc_at_ms;
 	long last_connected;
 
 	/* Updated from RX path only, no locking requirements */
-- 
2.20.1

Re: [PATCH-v3 2/2] mac80211: add assoc-at-ms support.

[Ben Greear]

On 4/15/19 10:21 AM, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
> 
> Report when sta becomes associated.

While rebasing, I notice this file is just sitting in patchwork.

Any chance this patch and the previous one in the series can be accepted?

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH-v3 1/2] wireless: Support assoc-at-ms timer in sta-info

[Johannes Berg]

On Mon, 2019-04-15 at 10:21 -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
> 
> Report time stamp of when sta became associated.

I guess that makes sense.

> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
>  include/net/cfg80211.h       | 2 ++
>  include/uapi/linux/nl80211.h | 2 ++
>  net/wireless/nl80211.c       | 1 +
>  3 files changed, 5 insertions(+)
> 
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index f49eb1464b7a..a3ad78b9d9f4 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -1268,6 +1268,7 @@ struct cfg80211_tid_stats {
>   *	indicate the relevant values in this struct for them
>   * @connected_time: time(in secs) since a station is last connected
>   * @inactive_time: time since last station activity (tx/rx) in milliseconds
> + * @assoc_at_ms: time in ms of the last association

I think the "_at_ms" doesn't really make sense. "time in ms" also
doesn't make sense as documentation. I think you need to say what should
be assigned here.

Also, you're actually using ktime_get_real() for this, which again
doesn't make much sense. For exposing an absolute time, I think we're
better off with CLOCK_BOOTTIME like we use elsewhere:

 * @boottime_ns: CLOCK_BOOTTIME timestamp the frame was received at, this is
 *      needed only for beacons and probe responses that update the scan cache.


> + * @NL80211_STA_INFO_ASSOC_AT_MS: Timestamp of last association

_ASSOC_TIMESTAMP or something would make more sense too as the attribute
name, and clearly the documentation needs to spell out the semantics
here too.

johannes

Re: [PATCH-v3 1/2] wireless: Support assoc-at-ms timer in sta-info

[Ben Greear]

On 6/14/19 6:38 AM, Johannes Berg wrote:
> On Mon, 2019-04-15 at 10:21 -0700, greearb@candelatech.com wrote:
>> From: Ben Greear <greearb@candelatech.com>
>>
>> Report time stamp of when sta became associated.
> 
> I guess that makes sense.
> 
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>> ---
>>   include/net/cfg80211.h       | 2 ++
>>   include/uapi/linux/nl80211.h | 2 ++
>>   net/wireless/nl80211.c       | 1 +
>>   3 files changed, 5 insertions(+)
>>
>> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
>> index f49eb1464b7a..a3ad78b9d9f4 100644
>> --- a/include/net/cfg80211.h
>> +++ b/include/net/cfg80211.h
>> @@ -1268,6 +1268,7 @@ struct cfg80211_tid_stats {
>>    *	indicate the relevant values in this struct for them
>>    * @connected_time: time(in secs) since a station is last connected
>>    * @inactive_time: time since last station activity (tx/rx) in milliseconds
>> + * @assoc_at_ms: time in ms of the last association
> 
> I think the "_at_ms" doesn't really make sense. "time in ms" also
> doesn't make sense as documentation. I think you need to say what should
> be assigned here.
> 
> Also, you're actually using ktime_get_real() for this, which again
> doesn't make much sense. For exposing an absolute time, I think we're
> better off with CLOCK_BOOTTIME like we use elsewhere:

The point of my patch was to allow 'iw' to return a more precise time
that the station has been associated, so I am not sure that BOOTIME is
a good thing to use for that?

Here are the pertinent parts of my iw patches:


diff --git a/station.c b/station.c
index 25cbbc3..e7738cc 100644
--- a/station.c
+++ b/station.c
@@ -314,6 +314,12 @@ static int print_sta_handler(struct nl_msg *msg, void *arg)
                 [NL80211_STA_INFO_ACK_SIGNAL_AVG] = { .type = NLA_U8 },
         };
         char *chain;
+       struct timeval now;
+       unsigned long long now_ms;
+
+       gettimeofday(&now, NULL);
+       now_ms = now.tv_sec * 1000;
+       now_ms += (now.tv_usec / 1000);

         nla_parse(tb, NL80211_ATTR_MAX, genlmsg_attrdata(gnlh, 0),
                   genlmsg_attrlen(gnlh, 0), NULL);
@@ -557,8 +563,11 @@ static int print_sta_handler(struct nl_msg *msg, void *arg)
         if (sinfo[NL80211_STA_INFO_CONNECTED_TIME])
                 printf("\n\tconnected time:\t%u seconds",
                         nla_get_u32(sinfo[NL80211_STA_INFO_CONNECTED_TIME]));
+       if (sinfo[NL80211_STA_INFO_ASSOC_AT_MS])
+               printf("\n\tassociated at:\t%llu ms",
+                        (unsigned long long)nla_get_u64(sinfo[NL80211_STA_INFO_ASSOC_AT_MS]));

-       printf("\n");
+       printf("\n\tcurrent time:\t%llu ms\n", now_ms);
         return NL_SKIP;
  }


Thanks,
Ben

> 
>   * @boottime_ns: CLOCK_BOOTTIME timestamp the frame was received at, this is
>   *      needed only for beacons and probe responses that update the scan cache.
> 
> 
>> + * @NL80211_STA_INFO_ASSOC_AT_MS: Timestamp of last association
> 
> _ASSOC_TIMESTAMP or something would make more sense too as the attribute
> name, and clearly the documentation needs to spell out the semantics
> here too.
> 
> johannes
> 


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH-v3 1/2] wireless: Support assoc-at-ms timer in sta-info

[Johannes Berg]

On Fri, 2019-06-14 at 07:46 -0700, Ben Greear wrote:
> 
> The point of my patch was to allow 'iw' to return a more precise time
> that the station has been associated, so I am not sure that BOOTIME is
> a good thing to use for that?

Depends what you want, really.

> +       if (sinfo[NL80211_STA_INFO_ASSOC_AT_MS])
> +               printf("\n\tassociated at:\t%llu ms",
> +                        (unsigned long long)nla_get_u64(sinfo[NL80211_STA_INFO_ASSOC_AT_MS]));
> 
> -       printf("\n");
> +       printf("\n\tcurrent time:\t%llu ms\n", now_ms);

Since you just print the time in milliseconds, and the current time in
milliseconds, you don't even really have any value in the wall clock.
Quite the opposite - this lends itself to subtracting to try to figure
out how long it was associated, which is the completely wrong thing to
do with wall clock - timezone adjustment could've happened inbetween,
for example!

I really see no point in trying to give the wall clock at assoc time. If
no timezone adjustment happened, you can just as well give the boottime
and have the userspace figure out the wall clock. If timezone adjustment
happened, then any calculations are wrong anyway, what would the point
be?

johannes

Re: [PATCH-v3 1/2] wireless: Support assoc-at-ms timer in sta-info

[Ben Greear]

On 6/14/19 7:56 AM, Johannes Berg wrote:
> On Fri, 2019-06-14 at 07:46 -0700, Ben Greear wrote:
>>
>> The point of my patch was to allow 'iw' to return a more precise time
>> that the station has been associated, so I am not sure that BOOTIME is
>> a good thing to use for that?
> 
> Depends what you want, really.
> 
>> +       if (sinfo[NL80211_STA_INFO_ASSOC_AT_MS])
>> +               printf("\n\tassociated at:\t%llu ms",
>> +                        (unsigned long long)nla_get_u64(sinfo[NL80211_STA_INFO_ASSOC_AT_MS]));
>>
>> -       printf("\n");
>> +       printf("\n\tcurrent time:\t%llu ms\n", now_ms);
> 
> Since you just print the time in milliseconds, and the current time in
> milliseconds, you don't even really have any value in the wall clock.
> Quite the opposite - this lends itself to subtracting to try to figure
> out how long it was associated, which is the completely wrong thing to
> do with wall clock - timezone adjustment could've happened inbetween,
> for example!
> 
> I really see no point in trying to give the wall clock at assoc time. If
> no timezone adjustment happened, you can just as well give the boottime
> and have the userspace figure out the wall clock. If timezone adjustment
> happened, then any calculations are wrong anyway, what would the point
> be?

So, maybe I return instead the elapsed time in the netlink API instead of a
timestamp.  I think that will give me the value that I am looking for,
and I can still print out the 'real' time in iw so any tools reading that
output and do some simple math and figure out the 'real' associated-at time.

If that sounds good to you, I'll code that up.

Thanks,
Ben

> 
> johannes
> 


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH-v3 1/2] wireless: Support assoc-at-ms timer in sta-info

[Johannes Berg]

On Fri, 2019-06-14 at 08:14 -0700, Ben Greear wrote:
> 
> 
> So, maybe I return instead the elapsed time in the netlink API instead of a
> timestamp.  I think that will give me the value that I am looking for,
> and I can still print out the 'real' time in iw so any tools reading that
> output and do some simple math and figure out the 'real' associated-at time.

I don't think that's good. There's a delay between filling the message
and then processing it in userspace. We had this in the scan code and
learned that was full of races.

The better thing is to use CLOCK_BOOTTIME nanoseconds, and then
userspace can use clock_gettime() to get the current time etc. and then
do whatever calculations it needs. If it wants to print the realtime
timestamp, it could even calculate that (with some jitter) by doing
clock_gettime() on both realtime and boottime clocks...

johannes