Skip to contentSkip to conversation

BSS timestamp fix/cleanup

PATCH In review Johannes Berg 4 participants 9 messages Mar 13, 2012 – Dec 24, 2014

Subsystems (2)
Networking drivers (wireless) 802.11 (including cfg80211/NL80211)
lore
Share & tools
lore

Johannes Berg

author maintainer
#0 lore raw

cover BSS timestamp fix/cleanup

I was looking at this code and thought I'd clarify it and remove some abuse in brcmfmac.

johannes

author maintainer
#1 lore raw

PATCH 1/2 brmc80211: dont use jiffies for BSS TSF

From: Johannes Berg <[email protected]>

The cfg80211_inform_bss() timestamp argument is
intended to be the TSF, not any form of host
timestamp.

Signed-off-by Johannes Berg <[email protected]>
---
 drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c |   10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)
drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c +2 -8
@@ -2003,7 +2003,6 @@ static s32 brcmf_inform_single_bss(struc
 	s32 err = 0;
 	u16 channel;
 	u32 freq;
-	u64 notify_timestamp;
 	u16 notify_capability;
 	u16 notify_interval;
 	u8 *notify_ie;

@@ -2026,7 +2025,6 @@ static s32 brcmf_inform_single_bss(struc
 	freq = ieee80211_channel_to_frequency(channel, band->band);
 	notify_channel = ieee80211_get_channel(wiphy, freq);
 
-	notify_timestamp = jiffies_to_msecs(jiffies)*1000; /* uSec */
 	notify_capability = le16_to_cpu(bi->capability);
 	notify_interval = le16_to_cpu(bi->beacon_period);
 	notify_ie = (u8 *)bi + le16_to_cpu(bi->ie_offset);

@@ -2040,10 +2038,9 @@ static s32 brcmf_inform_single_bss(struc
 	WL_CONN("Capability: %X\n", notify_capability);
 	WL_CONN("Beacon interval: %d\n", notify_interval);
 	WL_CONN("Signal: %d\n", notify_signal);
-	WL_CONN("notify_timestamp: %#018llx\n", notify_timestamp);
 
 	bss = cfg80211_inform_bss(wiphy, notify_channel, (const u8 *)bi->BSSID,
-		notify_timestamp, notify_capability, notify_interval, notify_ie,
+		0, notify_capability, notify_interval, notify_ie,
 		notify_ielen, notify_signal, GFP_KERNEL);
 
 	if (!bss)

@@ -2098,7 +2095,6 @@ static s32 wl_inform_ibss(struct brcmf_c
 	s32 err = 0;
 	u16 channel;
 	u32 freq;
-	u64 notify_timestamp;
 	u16 notify_capability;
 	u16 notify_interval;
 	u8 *notify_ie;

@@ -2134,7 +2130,6 @@ static s32 wl_inform_ibss(struct brcmf_c
 	freq = ieee80211_channel_to_frequency(channel, band->band);
 	notify_channel = ieee80211_get_channel(wiphy, freq);
 
-	notify_timestamp = jiffies_to_msecs(jiffies)*1000; /* uSec */
 	notify_capability = le16_to_cpu(bi->capability);
 	notify_interval = le16_to_cpu(bi->beacon_period);
 	notify_ie = (u8 *)bi + le16_to_cpu(bi->ie_offset);

@@ -2145,10 +2140,9 @@ static s32 wl_inform_ibss(struct brcmf_c
 	WL_CONN("capability: %X\n", notify_capability);
 	WL_CONN("beacon interval: %d\n", notify_interval);
 	WL_CONN("signal: %d\n", notify_signal);
-	WL_CONN("notify_timestamp: %#018llx\n", notify_timestamp);
 
 	bss = cfg80211_inform_bss(wiphy, notify_channel, bssid,
-		notify_timestamp, notify_capability, notify_interval,
+		0, notify_capability, notify_interval,
 		notify_ie, notify_ielen, notify_signal, GFP_KERNEL);
 
 	if (!bss) {
author maintainer
#3 lore raw

Re: [PATCH 1/2] brmc80211: dont use jiffies for BSS TSF

In reply to Johannes Berg

By the way - I know now that the proprietary Broadcom driver has the same bug, to the point where this is apparently getting encoded into the Android framework.

I urge you to fix this issue there as well. If an absolute "last updated" timestamp is needed (and "last seen [ms] ago" isn't sufficient) then new API will be needed.

johannes

#4 lore raw

Re: [PATCH 1/2] brmc80211: dont use jiffies for BSS TSF

Quoted 7 lines · Johannes Berg

By the way - I know now that the proprietary Broadcom driver has the same bug, to the point where this is apparently getting encoded into the Android framework.

I urge you to fix this issue there as well. If an absolute "last updated" timestamp is needed (and "last seen [ms] ago" isn't sufficient) then new API will be needed.

Sorry, seem to have missed the original patches somehow. Guess because it says *brmc*80211.

Regards,
Arend

author maintainer
#5 lore raw

Re: [PATCH 1/2] brmc80211: dont use jiffies for BSS TSF

Quoted 11 lines · Arend van Spriel
Quoted 7 lines · Johannes Berg

By the way - I know now that the proprietary Broadcom driver has the same bug, to the point where this is apparently getting encoded into the Android framework.

I urge you to fix this issue there as well. If an absolute "last updated" timestamp is needed (and "last seen [ms] ago" isn't sufficient) then new API will be needed.

Sorry, seem to have missed the original patches somehow. Guess because it says *brmc*80211.

No no - it's a looong time ago and was also applied a long time ago. It just reared it's head again in another place.

johannes

#6 lore raw

Re: [PATCH 1/2] brmc80211: dont use jiffies for BSS TSF

Quoted 10 lines · Johannes Berg
Quoted 9 lines · Arend van Spriel
Quoted 7 lines · Johannes Berg

By the way - I know now that the proprietary Broadcom driver has the same bug, to the point where this is apparently getting encoded into the Android framework.

I urge you to fix this issue there as well. If an absolute "last updated" timestamp is needed (and "last seen [ms] ago" isn't sufficient) then new API will be needed.

Quoted 5 lines
Quoted 2 lines

Sorry, seem to have missed the original patches somehow. Guess because it says *brmc*80211.

No no - it's a looong time ago and was also applied a long time ago. It just reared it's head again in another place.

Found the commit in git log. So you mean the proprietary DHD driver or Android bcmdhd still uses host timestamp. I agree it should be fixed, but I may need some good arguments to convince my co-workers. So what kind of issues are rearing their (ugly) heads? supplicant issues?

Regards,
Arend

#7 lore raw

Re: [PATCH 1/2] brmc80211: dont use jiffies for BSS TSF

Quoted 19 lines · Arend van Spriel
Quoted 18 lines · Johannes Berg
Quoted 14 lines · Arend van Spriel
Quoted 8 lines · Johannes Berg

By the way - I know now that the proprietary Broadcom driver has the same bug, to the point where this is apparently getting encoded into the Android framework.

I urge you to fix this issue there as well. If an absolute "last updated" timestamp is needed (and "last seen [ms] ago" isn't sufficient) then new API will be needed.

Sorry, seem to have missed the original patches somehow. Guess because it says *brmc*80211.

Quoted 8 lines
Quoted 2 lines

No no - it's a looong time ago and was also applied a long time ago. It just reared it's head again in another place.

Found the commit in git log. So you mean the proprietary DHD driver or Android bcmdhd still uses host timestamp. I agree it should be fixed, but I may need some good arguments to convince my co-workers. So what kind of issues are rearing their (ugly) heads? supplicant issues?

Basically the Android framework is using the TSF from the scan results as a timestamp for when the BSS was seen. In Android LL a new CTS test was added to verify these values.
This of course means the CTS test fails for every vendor besides BRCM :)

It seems the framework knows it's a driver bug, since in the java code they explicitly name the variable "tsf", before assigning it to the scan result timestamp..

Arik

author maintainer
#8 lore raw

PATCH 2/2 cfg80211: clarify timestamp in cfg80211_inform_bss

In reply to Johannes Berg

From: Johannes Berg <[email protected]>

This is intended to be the timestamp sent by the
peer in the beacon/probe response, not any form
of host timestamp. Clarify the documentation and
variable names.

Signed-off-by Johannes Berg <[email protected]>
---
 include/net/cfg80211.h |    7 +++----
 net/wireless/scan.c    |    7 +++----
 2 files changed, 6 insertions(+), 8 deletions(-)
include/net/cfg80211.h +3 -4
@@ -2697,7 +2697,7 @@ cfg80211_inform_bss_frame(struct wiphy *
  * @wiphy: the wiphy reporting the BSS
  * @channel: The channel the frame was received on
  * @bssid: the BSSID of the BSS
- * @timestamp: the TSF timestamp sent by the peer
+ * @tsf: the TSF sent by the peer in the beacon/probe response (or 0)
  * @capability: the capability field sent by the peer
  * @beacon_interval: the beacon interval announced by the peer
  * @ie: additional IEs sent by the peer

@@ -2713,9 +2713,8 @@ cfg80211_inform_bss_frame(struct wiphy *
 struct cfg80211_bss * __must_check
 cfg80211_inform_bss(struct wiphy *wiphy,
 		    struct ieee80211_channel *channel,
-		    const u8 *bssid,
-		    u64 timestamp, u16 capability, u16 beacon_interval,
-		    const u8 *ie, size_t ielen,
+		    const u8 *bssid, u64 tsf, u16 capability,
+		    u16 beacon_interval, const u8 *ie, size_t ielen,
 		    s32 signal, gfp_t gfp);
 
 struct cfg80211_bss *cfg80211_get_bss(struct wiphy *wiphy,
net/wireless/scan.c +3 -4
@@ -734,9 +734,8 @@ cfg80211_bss_update(struct cfg80211_regi
 struct cfg80211_bss*
 cfg80211_inform_bss(struct wiphy *wiphy,
 		    struct ieee80211_channel *channel,
-		    const u8 *bssid,
-		    u64 timestamp, u16 capability, u16 beacon_interval,
-		    const u8 *ie, size_t ielen,
+		    const u8 *bssid, u64 tsf, u16 capability,
+		    u16 beacon_interval, const u8 *ie, size_t ielen,
 		    s32 signal, gfp_t gfp)
 {
 	struct cfg80211_internal_bss *res;

@@ -758,7 +757,7 @@ cfg80211_inform_bss(struct wiphy *wiphy,
 	memcpy(res->pub.bssid, bssid, ETH_ALEN);
 	res->pub.channel = channel;
 	res->pub.signal = signal;
-	res->pub.tsf = timestamp;
+	res->pub.tsf = tsf;
 	res->pub.beacon_interval = beacon_interval;
 	res->pub.capability = capability;
 	/*