[PATCH 0/2] BSS timestamp fix/cleanup

[PATCH 0/2] BSS timestamp fix/cleanup

[Johannes Berg]

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

johannes

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

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

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

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c |   10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

--- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c	2012-03-10 09:17:00.000000000 +0100
+++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c	2012-03-13 13:45:43.000000000 +0100
@@ -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) {

[PATCH 2/2] cfg80211: clarify timestamp in cfg80211_inform_bss

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

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 <johannes.berg@intel.com>
---
 include/net/cfg80211.h |    7 +++----
 net/wireless/scan.c    |    7 +++----
 2 files changed, 6 insertions(+), 8 deletions(-)

--- a/include/net/cfg80211.h	2012-03-13 09:11:23.000000000 +0100
+++ b/include/net/cfg80211.h	2012-03-13 13:49:53.000000000 +0100
@@ -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,
--- a/net/wireless/scan.c	2012-03-10 09:17:06.000000000 +0100
+++ b/net/wireless/scan.c	2012-03-13 13:50:24.000000000 +0100
@@ -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;
 	/*

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

[Franky Lin]

On 03/13/2012 05:57 AM, Johannes Berg wrote:
> From: Johannes Berg<johannes.berg@intel.com>
>
> The cfg80211_inform_bss() timestamp argument is
> intended to be the TSF, not any form of host
> timestamp.
>
> Signed-off-by: Johannes Berg<johannes.berg@intel.com>

Thanks Johannes.

Acked-by: Franky Lin <frankyl@broadcom.com>

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

[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

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

[Arend van Spriel]

On 12/22/14 13:58, Johannes Berg wrote:
> 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

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

[Johannes Berg]

On Mon, 2014-12-22 at 14:13 +0100, Arend van Spriel wrote:
> On 12/22/14 13:58, Johannes Berg wrote:
> > 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

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

[Arend van Spriel]

On 12/22/14 14:14, Johannes Berg wrote:
> On Mon, 2014-12-22 at 14:13 +0100, Arend van Spriel wrote:
>> On 12/22/14 13:58, Johannes Berg wrote:
>>> 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.

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

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

[Arik Nemtsov]

On Mon, Dec 22, 2014 at 3:26 PM, Arend van Spriel <arend@broadcom.com> wrote:
> On 12/22/14 14:14, Johannes Berg wrote:
>>
>> On Mon, 2014-12-22 at 14:13 +0100, Arend van Spriel wrote:
>>>
>>> On 12/22/14 13:58, Johannes Berg wrote:
>>>>
>>>> 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.
>
>
> 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