mesh_sync_offset_adjust_tbtt() implements Extensible synchronization
framework ([1] 13.13.2 Extensible synchronization framework). It shall
not operate the flag "TBTT Adjusting subfield" ([1] 8.4.2.100.8 Mesh
Capability), since it is used only for MBCA ([1] 13.13.4 Mesh beacon
collision avoidance, see 13.13.4.4.3 TBTT scanning and adjustment
procedures for detail). So this patch remove the flag operations.
[1] IEEE Std 802.11 2012
Signed-off-by: Masashi Honma <[email protected]>
---
net/mac80211/mesh_sync.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/net/mac80211/mesh_sync.c b/net/mac80211/mesh_sync.c
index faca22c..836d791 100644
--- a/net/mac80211/mesh_sync.c
+++ b/net/mac80211/mesh_sync.c
@@ -172,11 +172,9 @@ static void mesh_sync_offset_adjust_tbtt(struct ieee80211_sub_if_data *sdata,
struct beacon_data *beacon)
{
struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
- u8 cap;
WARN_ON(ifmsh->mesh_sp_id != IEEE80211_SYNC_METHOD_NEIGHBOR_OFFSET);
WARN_ON(!rcu_read_lock_held());
- cap = beacon->meshconf->meshconf_cap;
spin_lock_bh(&ifmsh->sync_offset_lock);
@@ -190,21 +188,13 @@ static void mesh_sync_offset_adjust_tbtt(struct ieee80211_sub_if_data *sdata,
"TBTT : kicking off TBTT adjustment with clockdrift_max=%lld\n",
ifmsh->sync_offset_clockdrift_max);
set_bit(MESH_WORK_DRIFT_ADJUST, &ifmsh->wrkq_flags);
-
- ifmsh->adjusting_tbtt = true;
} else {
msync_dbg(sdata,
"TBTT : max clockdrift=%lld; too small to adjust\n",
(long long)ifmsh->sync_offset_clockdrift_max);
ifmsh->sync_offset_clockdrift_max = 0;
-
- ifmsh->adjusting_tbtt = false;
}
spin_unlock_bh(&ifmsh->sync_offset_lock);
-
- beacon->meshconf->meshconf_cap = ifmsh->adjusting_tbtt ?
- IEEE80211_MESHCONF_CAPAB_TBTT_ADJUSTING | cap :
- ~IEEE80211_MESHCONF_CAPAB_TBTT_ADJUSTING & cap;
}
static const struct sync_method sync_methods[] = {
--
2.7.4
These functions drifts TSF timers, not TBTT.
Signed-off-by: Masashi Honma <[email protected]>
---
net/mac80211/ieee80211_i.h | 4 ++--
net/mac80211/mesh.c | 2 +-
net/mac80211/mesh.h | 2 +-
net/mac80211/mesh_sync.c | 16 ++++++++--------
net/mac80211/tx.c | 2 +-
5 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index d37a577..94b2c9e 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -624,8 +624,8 @@ struct ieee80211_mesh_sync_ops {
struct ieee80211_rx_status *rx_status);
/* should be called with beacon_data under RCU read lock */
- void (*adjust_tbtt)(struct ieee80211_sub_if_data *sdata,
- struct beacon_data *beacon);
+ void (*adjust_tsf)(struct ieee80211_sub_if_data *sdata,
+ struct beacon_data *beacon);
/* add other framework functions here */
};
diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 42120d9..a6b43e4 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -1349,7 +1349,7 @@ void ieee80211_mesh_work(struct ieee80211_sub_if_data *sdata)
ieee80211_mesh_rootpath(sdata);
if (test_and_clear_bit(MESH_WORK_DRIFT_ADJUST, &ifmsh->wrkq_flags))
- mesh_sync_adjust_tbtt(sdata);
+ mesh_sync_adjust_tsf(sdata);
if (test_and_clear_bit(MESH_WORK_MBSS_CHANGED, &ifmsh->wrkq_flags))
mesh_bss_info_changed(sdata);
diff --git a/net/mac80211/mesh.h b/net/mac80211/mesh.h
index 26b9ccb..7e5f271 100644
--- a/net/mac80211/mesh.h
+++ b/net/mac80211/mesh.h
@@ -341,7 +341,7 @@ static inline bool mesh_path_sel_is_hwmp(struct ieee80211_sub_if_data *sdata)
}
void mesh_path_flush_by_iface(struct ieee80211_sub_if_data *sdata);
-void mesh_sync_adjust_tbtt(struct ieee80211_sub_if_data *sdata);
+void mesh_sync_adjust_tsf(struct ieee80211_sub_if_data *sdata);
void ieee80211s_stop(void);
#else
static inline bool mesh_path_sel_is_hwmp(struct ieee80211_sub_if_data *sdata)
diff --git a/net/mac80211/mesh_sync.c b/net/mac80211/mesh_sync.c
index 75608c0..a435f09 100644
--- a/net/mac80211/mesh_sync.c
+++ b/net/mac80211/mesh_sync.c
@@ -12,7 +12,7 @@
#include "mesh.h"
#include "driver-ops.h"
-/* This is not in the standard. It represents a tolerable tbtt drift below
+/* This is not in the standard. It represents a tolerable tsf drift below
* which we do no TSF adjustment.
*/
#define TOFFSET_MINIMUM_ADJUSTMENT 10
@@ -46,7 +46,7 @@ static bool mesh_peer_tbtt_adjusting(struct ieee802_11_elems *ie)
IEEE80211_MESHCONF_CAPAB_TBTT_ADJUSTING) != 0;
}
-void mesh_sync_adjust_tbtt(struct ieee80211_sub_if_data *sdata)
+void mesh_sync_adjust_tsf(struct ieee80211_sub_if_data *sdata)
{
struct ieee80211_local *local = sdata->local;
struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
@@ -57,12 +57,12 @@ void mesh_sync_adjust_tbtt(struct ieee80211_sub_if_data *sdata)
spin_lock_bh(&ifmsh->sync_offset_lock);
if (ifmsh->sync_offset_clockdrift_max < beacon_int_fraction) {
- msync_dbg(sdata, "TBTT : max clockdrift=%lld; adjusting\n",
+ msync_dbg(sdata, "TSF : max clockdrift=%lld; adjusting\n",
(long long) ifmsh->sync_offset_clockdrift_max);
tsfdelta = -ifmsh->sync_offset_clockdrift_max;
ifmsh->sync_offset_clockdrift_max = 0;
} else {
- msync_dbg(sdata, "TBTT : max clockdrift=%lld; adjusting by %llu\n",
+ msync_dbg(sdata, "TSF : max clockdrift=%lld; adjusting by %llu\n",
(long long) ifmsh->sync_offset_clockdrift_max,
(unsigned long long) beacon_int_fraction);
tsfdelta = -beacon_int_fraction;
@@ -167,7 +167,7 @@ static void mesh_sync_offset_rx_bcn_presp(struct ieee80211_sub_if_data *sdata,
rcu_read_unlock();
}
-static void mesh_sync_offset_adjust_tbtt(struct ieee80211_sub_if_data *sdata,
+static void mesh_sync_offset_adjust_tsf(struct ieee80211_sub_if_data *sdata,
struct beacon_data *beacon)
{
struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
@@ -184,12 +184,12 @@ static void mesh_sync_offset_adjust_tbtt(struct ieee80211_sub_if_data *sdata,
* the tsf adjustment to the mesh tasklet
*/
msync_dbg(sdata,
- "TBTT : kicking off TBTT adjustment with clockdrift_max=%lld\n",
+ "TSF : kicking off TSF adjustment with clockdrift_max=%lld\n",
ifmsh->sync_offset_clockdrift_max);
set_bit(MESH_WORK_DRIFT_ADJUST, &ifmsh->wrkq_flags);
} else {
msync_dbg(sdata,
- "TBTT : max clockdrift=%lld; too small to adjust\n",
+ "TSF : max clockdrift=%lld; too small to adjust\n",
(long long)ifmsh->sync_offset_clockdrift_max);
ifmsh->sync_offset_clockdrift_max = 0;
}
@@ -201,7 +201,7 @@ static const struct sync_method sync_methods[] = {
.method = IEEE80211_SYNC_METHOD_NEIGHBOR_OFFSET,
.ops = {
.rx_bcn_presp = &mesh_sync_offset_rx_bcn_presp,
- .adjust_tbtt = &mesh_sync_offset_adjust_tbtt,
+ .adjust_tsf = &mesh_sync_offset_adjust_tsf,
}
},
};
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 2c21b70..874332d 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -4076,7 +4076,7 @@ __ieee80211_beacon_get(struct ieee80211_hw *hw,
}
if (ifmsh->sync_ops)
- ifmsh->sync_ops->adjust_tbtt(sdata, beacon);
+ ifmsh->sync_ops->adjust_tsf(sdata, beacon);
skb = dev_alloc_skb(local->tx_headroom +
beacon->head_len +
--
2.7.4
On Fri, Dec 2, 2016 at 10:59 PM, Masashi Honma <[email protected]> wrote:
> On 2016/12/03 06:13, Bob Copeland wrote:
>>
>> On Fri, Dec 02, 2016 at 12:07:18PM -0800, Thomas Pedersen wrote:
>
>
> # Rejected by linux wireless ML. This is resubmission.
>
> thomas and Bob, Thanks for comments.
>
>> 802.11-2012 13.13.2.2.3:
>> The mesh STA checks if the transmitter of the Beacon frame or Probe
>> Response frame is in the
>> process of the TBTT adjustment (see 13.13.4.4.3).
>
> There are two functionalities.
>
> 1) 13.13.2.2 Neighbor offset synchronization method
> 2) 13.13.4.4 TBTT adjustment
>
> The ifmsh->adjusting_tbtt flag implements "TBTT Adjusting field" in the
> Mesh Configuration field.
>
> The flag is updated by 2).
> 13.13.4.4.3 TBTT scanning and adjustment procedures:
> The mesh STA shall set the TBTT Adjusting field in the Mesh
> Configuration element to 1 in order to announce that the TBTT
> adjustment procedure is ongoing.
>
> And the flag is refered by 1) as you said.
>
>
> The purpose of the flag is to prevent 1) while 2) is ongoing.
>
> In other words, 1) has only read access authority to the flag. However,
> previous code updated the flag in 1). In addition, there is no code for
> 2). So I just remove the invalid accessing codes.
I don't think 1) has read only access to that flag. A TSF adjust will
by definition move the TBTT as well.
--
thomas
On 2016/12/03 06:13, Bob Copeland wrote:
> On Fri, Dec 02, 2016 at 12:07:18PM -0800, Thomas Pedersen wrote:
# Rejected by linux wireless ML. This is resubmission.
thomas and Bob, Thanks for comments.
> 802.11-2012 13.13.2.2.3:
> The mesh STA checks if the transmitter of the Beacon frame or Probe
> Response frame is in the
> process of the TBTT adjustment (see 13.13.4.4.3).
There are two functionalities.
1) 13.13.2.2 Neighbor offset synchronization method
2) 13.13.4.4 TBTT adjustment
The ifmsh->adjusting_tbtt flag implements "TBTT Adjusting field" in the
Mesh Configuration field.
The flag is updated by 2).
13.13.4.4.3 TBTT scanning and adjustment procedures:
The mesh STA shall set the TBTT Adjusting field in the Mesh
Configuration element to 1 in order to announce that the TBTT
adjustment procedure is ongoing.
And the flag is refered by 1) as you said.
The purpose of the flag is to prevent 1) while 2) is ongoing.
In other words, 1) has only read access authority to the flag. However,
previous code updated the flag in 1). In addition, there is no code for
2). So I just remove the invalid accessing codes.
Masashi Honma.
On 2016年12月07日 18:24, Johannes Berg wrote:
>>> And the flag is refered by 1) as you said.
>>>
>>>
>>> The purpose of the flag is to prevent 1) while 2) is ongoing.
>>>
>>> In other words, 1) has only read access authority to the flag.
>>> However,
>>> previous code updated the flag in 1). In addition, there is no code
>>> for
>>> 2). So I just remove the invalid accessing codes.
>>
>> I don't think 1) has read only access to that flag. A TSF adjust will
>> by definition move the TBTT as well.
>
> It seems that the wording in the spec disagrees with that - it says
> (twice) to set the bit only while the TBTT adjustment procedure is
> ongoing, which isn't the case here?
>
> Then again, what exactly *is* this code doing?
>
> It's called mesh_sync_offset_adjust_tbtt() which matches more closely
> "TBTT adjustment" than "neighbor offset synchronization"?
I think so. Because there is not any code creating "TBTT Adjustment
Request frame" even though the frame is required by "TBTT adjustment".
> The code
> looks more like offset synchronization though. Perhaps there's some
> confusing and it's kinda doing both?
In theory, updating the flag with 1) looks not correct because it is not
clearly defined in spec.
In practice, I could consider extending the meaning of the flag over the
spec to use it to avoid referring the updating TSF value by peer as
Thomas said. I have took the statistics how many TSF drift
(ifmsh->sync_offset_clockdrift_max) happens. The attached file shows the
stats. The horizontal axis shows TSF drift time(usec) and vertical axis
shows how many time the drift occurred. The graph shows almost drifts
are under 20usec. In contrast, 2) could causes more than 1000usec drift.
So 1) looks not so large enough to protect with the flag.
Masashi Honma.
On 2016年12月07日 22:33, Bob Copeland wrote:
> This mesh_sync_offset_adjust_tbtt is definitely doing offset
> synchronization, so probably "tbtt" should be renamed "tsf" here.
Right. I will send a patch for this.
> Actually, looking at the code now it doesn't make a lot of sense to set
> this flag for offset sync because TOFFSET_KNOWN flag is completely cleared
> whenever that is set, so we have to be forgetting the current t_offset all
> the time?
Right, TOFFSET_KNOWN flag looks not need to be cleared when
TBTT_ADJUSTING flag is on. Because the t_offset is received when
TBTT_ADJUSTING flag is off. The t_offset is expected to have valid
value. I will remove the code by next patch.
Masashi Honma.
Hi,
I'm not sure what to do with this now :)
> > There are two functionalities.
> >
> > 1) 13.13.2.2 Neighbor offset synchronization method
> > 2) 13.13.4.4 TBTT adjustment
Yes, this much is obvious.
> > The flag is updated by 2).
Yes, the definition of the flag says:
"The TBTT Adjusting subfield is set to 1 while the TBTT adjustment
procedure is ongoing, and is set to 0 otherwise. (See 13.13.4.4.3.)"
(802.11-2012 8.4.2.100.8 Mesh Capability)
> > 13.13.4.4.3 TBTT scanning and adjustment procedures:
> > The mesh STA shall set the TBTT Adjusting field in the Mesh
> > Configuration element to 1 in order to announce that the TBTT
> > adjustment procedure is ongoing.
That's saying the same thing again, I guess :)
> > And the flag is refered by 1) as you said.
> >
> >
> > The purpose of the flag is to prevent 1) while 2) is ongoing.
> >
> > In other words, 1) has only read access authority to the flag.
> > However,
> > previous code updated the flag in 1). In addition, there is no code
> > for
> > 2). So I just remove the invalid accessing codes.
>
> I don't think 1) has read only access to that flag. A TSF adjust will
> by definition move the TBTT as well.
It seems that the wording in the spec disagrees with that - it says
(twice) to set the bit only while the TBTT adjustment procedure is
ongoing, which isn't the case here?
Then again, what exactly *is* this code doing?
It's called mesh_sync_offset_adjust_tbtt() which matches more closely
"TBTT adjustment" than "neighbor offset synchronization"? The code
looks more like offset synchronization though. Perhaps there's some
confusing and it's kinda doing both?
johannes
On Fri, Dec 02, 2016 at 12:07:18PM -0800, Thomas Pedersen wrote:
> On Wed, Nov 30, 2016 at 2:44 PM, Masashi Honma <[email protected]> wrote:
> > mesh_sync_offset_adjust_tbtt() implements Extensible synchronization
> > framework ([1] 13.13.2 Extensible synchronization framework). It shall
> > not operate the flag "TBTT Adjusting subfield" ([1] 8.4.2.100.8 Mesh
> > Capability), since it is used only for MBCA ([1] 13.13.4 Mesh beacon
> > collision avoidance, see 13.13.4.4.3 TBTT scanning and adjustment
> > procedures for detail). So this patch remove the flag operations.
>
> 802.11-2012 13.13.2.2.3:
[snip]
> so, no? I think we need to indicate a TSF adjustment is taking place.
I think so too.
I must ask, what is the prompt for removing it, are you (Masashi)
trying to implement MBCA and this is interfering?
--
Bob Copeland %% http://bobcopeland.com/
mesh_sync_offset_adjust_tbtt() implements Extensible synchronization
framework ([1] 13.13.2 Extensible synchronization framework). It shall
not operate the flag "TBTT Adjusting subfield" ([1] 8.4.2.100.8 Mesh
Capability), since it is used only for MBCA ([1] 13.13.4 Mesh beacon
collision avoidance, see 13.13.4.4.3 TBTT scanning and adjustment
procedures for detail). So this patch remove the flag operations.
[1] IEEE Std 802.11 2012
Signed-off-by: Masashi Honma <[email protected]>
---
net/mac80211/mesh_sync.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/net/mac80211/mesh_sync.c b/net/mac80211/mesh_sync.c
index faca22c..75608c0 100644
--- a/net/mac80211/mesh_sync.c
+++ b/net/mac80211/mesh_sync.c
@@ -123,7 +123,6 @@ static void mesh_sync_offset_rx_bcn_presp(struct ieee80211_sub_if_data *sdata,
*/
if (elems->mesh_config && mesh_peer_tbtt_adjusting(elems)) {
- clear_sta_flag(sta, WLAN_STA_TOFFSET_KNOWN);
msync_dbg(sdata, "STA %pM : is adjusting TBTT\n",
sta->sta.addr);
goto no_sync;
@@ -172,11 +171,9 @@ static void mesh_sync_offset_adjust_tbtt(struct ieee80211_sub_if_data *sdata,
struct beacon_data *beacon)
{
struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
- u8 cap;
WARN_ON(ifmsh->mesh_sp_id != IEEE80211_SYNC_METHOD_NEIGHBOR_OFFSET);
WARN_ON(!rcu_read_lock_held());
- cap = beacon->meshconf->meshconf_cap;
spin_lock_bh(&ifmsh->sync_offset_lock);
@@ -190,21 +187,13 @@ static void mesh_sync_offset_adjust_tbtt(struct ieee80211_sub_if_data *sdata,
"TBTT : kicking off TBTT adjustment with clockdrift_max=%lld\n",
ifmsh->sync_offset_clockdrift_max);
set_bit(MESH_WORK_DRIFT_ADJUST, &ifmsh->wrkq_flags);
-
- ifmsh->adjusting_tbtt = true;
} else {
msync_dbg(sdata,
"TBTT : max clockdrift=%lld; too small to adjust\n",
(long long)ifmsh->sync_offset_clockdrift_max);
ifmsh->sync_offset_clockdrift_max = 0;
-
- ifmsh->adjusting_tbtt = false;
}
spin_unlock_bh(&ifmsh->sync_offset_lock);
-
- beacon->meshconf->meshconf_cap = ifmsh->adjusting_tbtt ?
- IEEE80211_MESHCONF_CAPAB_TBTT_ADJUSTING | cap :
- ~IEEE80211_MESHCONF_CAPAB_TBTT_ADJUSTING & cap;
}
static const struct sync_method sync_methods[] = {
--
2.7.4
On Thu, 2016-12-08 at 10:15 +0900, Masashi Honma wrote:
> mesh_sync_offset_adjust_tbtt() implements Extensible synchronization
> framework ([1] 13.13.2 Extensible synchronization framework). It
> shall
> not operate the flag "TBTT Adjusting subfield" ([1] 8.4.2.100.8 Mesh
> Capability), since it is used only for MBCA ([1] 13.13.4 Mesh beacon
> collision avoidance, see 13.13.4.4.3 TBTT scanning and adjustment
> procedures for detail). So this patch remove the flag operations.
>
Both applied; I changed this patch to remove ifmsh->adjusting_tbtt
completely since it was now unused.
johannes
On Wed, Nov 30, 2016 at 2:44 PM, Masashi Honma <[email protected]> wrote:
> mesh_sync_offset_adjust_tbtt() implements Extensible synchronization
> framework ([1] 13.13.2 Extensible synchronization framework). It shall
> not operate the flag "TBTT Adjusting subfield" ([1] 8.4.2.100.8 Mesh
> Capability), since it is used only for MBCA ([1] 13.13.4 Mesh beacon
> collision avoidance, see 13.13.4.4.3 TBTT scanning and adjustment
> procedures for detail). So this patch remove the flag operations.
802.11-2012 13.13.2.2.3:
The mesh STA checks if the transmitter of the Beacon frame or Probe
Response frame is in the
process of the TBTT adjustment (see 13.13.4.4.3). If the received
frame contains the Mesh
Configuration element and the TBTT Adjusting subfield in the Mesh
Configuration field is 1, the
mesh STA shall invalidate the T offset value for this neighbor STA and
shall not perform the
following steps.
so, no? I think we need to indicate a TSF adjustment is taking place.
--
thomas
On Wed, Dec 07, 2016 at 09:55:41PM +0900, Masashi Honma wrote:
> >It's called mesh_sync_offset_adjust_tbtt() which matches more closely
> >"TBTT adjustment" than "neighbor offset synchronization"?
>
> I think so. Because there is not any code creating "TBTT Adjustment Request
> frame" even though the frame is required by "TBTT adjustment".
This mesh_sync_offset_adjust_tbtt is definitely doing offset
synchronization, so probably "tbtt" should be renamed "tsf" here.
> >The code
> >looks more like offset synchronization though. Perhaps there's some
> >confusing and it's kinda doing both?
>
> In theory, updating the flag with 1) looks not correct because it is not
> clearly defined in spec.
>
> In practice, I could consider extending the meaning of the flag over the
> spec to use it to avoid referring the updating TSF value by peer as Thomas
> said. I have took the statistics how many TSF drift
> (ifmsh->sync_offset_clockdrift_max) happens. The attached file shows the
> stats. The horizontal axis shows TSF drift time(usec) and vertical axis
> shows how many time the drift occurred. The graph shows almost drifts are
> under 20usec. In contrast, 2) could causes more than 1000usec drift. So 1)
> looks not so large enough to protect with the flag.
Yes, offset synchronization is (given decent clocks) supposed to be only
for small tweaks. We will do it up to .8 ms drift though -- above .8 ms, we
just reset drift to zero and adopt the new timing offset. You can see this
kind of large "drift" by restarting a station.
Actually, looking at the code now it doesn't make a lot of sense to set
this flag for offset sync because TOFFSET_KNOWN flag is completely cleared
whenever that is set, so we have to be forgetting the current t_offset all
the time?
--
Bob Copeland %% http://bobcopeland.com/