[PATCH 0/3] Mesh AID fixes

[PATCH 0/3] Mesh AID fixes

[Bob Copeland]

This patchset corrects the use of peer link id as a stand-in for the
association ID, which is at odds with the standard.  Using an incremental
AID generated on the local sta whenever a peer is added enables smaller
TIM elements, and removes one possible source of ambiguity when plids
collide.

Using a proper aid also will make it easier to implement mesh on ath10k.

Because mac80211 currently only implements the buffering side of mesh
power save, we don't need to change any code that reads the AID from
confirm frames.  Which is good because it was previously always wrong.

Bob Copeland (3):
  mac80211: reorder mesh_plink to remove forward decl
  mac80211: mesh: separate plid and aid concepts
  mac80211: select an AID when creating new mesh STAs

 net/mac80211/cfg.c         |   3 +
 net/mac80211/ieee80211_i.h |   1 +
 net/mac80211/mesh.c        |   7 +++
 net/mac80211/mesh_plink.c  | 147 +++++++++++++++++++++++++--------------------
 net/mac80211/sta_info.c    |   5 +-
 5 files changed, 94 insertions(+), 69 deletions(-)

-- 
2.1.4

[PATCH 1/3] mac80211: reorder mesh_plink to remove forward decl

[Bob Copeland]

Move mesh_plink_frame_tx() above the first caller to remove
the forward declaration.

Signed-off-by: Bob Copeland <me@bobcopeland.com>
---
 net/mac80211/mesh_plink.c | 109 ++++++++++++++++++++++------------------------
 1 file changed, 52 insertions(+), 57 deletions(-)

diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
index 1a7d98398626..7b071e36e55e 100644
--- a/net/mac80211/mesh_plink.c
+++ b/net/mac80211/mesh_plink.c
@@ -53,11 +53,6 @@ static const char * const mplevents[] = {
 	[CLS_IGNR] = "CLS_IGNR"
 };
 
-static int mesh_plink_frame_tx(struct ieee80211_sub_if_data *sdata,
-			       enum ieee80211_self_protected_actioncode action,
-			       u8 *da, u16 llid, u16 plid, u16 reason);
-
-
 /* We only need a valid sta if user configured a minimum rssi_threshold. */
 static bool rssi_threshold_check(struct ieee80211_sub_if_data *sdata,
 				 struct sta_info *sta)
@@ -204,58 +199,6 @@ static u32 mesh_set_ht_prot_mode(struct ieee80211_sub_if_data *sdata)
 	return BSS_CHANGED_HT;
 }
 
-/**
- * __mesh_plink_deactivate - deactivate mesh peer link
- *
- * @sta: mesh peer link to deactivate
- *
- * All mesh paths with this peer as next hop will be flushed
- * Returns beacon changed flag if the beacon content changed.
- *
- * Locking: the caller must hold sta->mesh->plink_lock
- */
-static u32 __mesh_plink_deactivate(struct sta_info *sta)
-{
-	struct ieee80211_sub_if_data *sdata = sta->sdata;
-	u32 changed = 0;
-
-	lockdep_assert_held(&sta->mesh->plink_lock);
-
-	if (sta->mesh->plink_state == NL80211_PLINK_ESTAB)
-		changed = mesh_plink_dec_estab_count(sdata);
-	sta->mesh->plink_state = NL80211_PLINK_BLOCKED;
-	mesh_path_flush_by_nexthop(sta);
-
-	ieee80211_mps_sta_status_update(sta);
-	changed |= ieee80211_mps_set_sta_local_pm(sta,
-			NL80211_MESH_POWER_UNKNOWN);
-
-	return changed;
-}
-
-/**
- * mesh_plink_deactivate - deactivate mesh peer link
- *
- * @sta: mesh peer link to deactivate
- *
- * All mesh paths with this peer as next hop will be flushed
- */
-u32 mesh_plink_deactivate(struct sta_info *sta)
-{
-	struct ieee80211_sub_if_data *sdata = sta->sdata;
-	u32 changed;
-
-	spin_lock_bh(&sta->mesh->plink_lock);
-	changed = __mesh_plink_deactivate(sta);
-	sta->mesh->reason = WLAN_REASON_MESH_PEER_CANCELED;
-	mesh_plink_frame_tx(sdata, WLAN_SP_MESH_PEERING_CLOSE,
-			    sta->sta.addr, sta->mesh->llid, sta->mesh->plid,
-			    sta->mesh->reason);
-	spin_unlock_bh(&sta->mesh->plink_lock);
-
-	return changed;
-}
-
 static int mesh_plink_frame_tx(struct ieee80211_sub_if_data *sdata,
 			       enum ieee80211_self_protected_actioncode action,
 			       u8 *da, u16 llid, u16 plid, u16 reason)
@@ -375,6 +318,58 @@ free:
 	return err;
 }
 
+/**
+ * __mesh_plink_deactivate - deactivate mesh peer link
+ *
+ * @sta: mesh peer link to deactivate
+ *
+ * All mesh paths with this peer as next hop will be flushed
+ * Returns beacon changed flag if the beacon content changed.
+ *
+ * Locking: the caller must hold sta->mesh->plink_lock
+ */
+static u32 __mesh_plink_deactivate(struct sta_info *sta)
+{
+	struct ieee80211_sub_if_data *sdata = sta->sdata;
+	u32 changed = 0;
+
+	lockdep_assert_held(&sta->mesh->plink_lock);
+
+	if (sta->mesh->plink_state == NL80211_PLINK_ESTAB)
+		changed = mesh_plink_dec_estab_count(sdata);
+	sta->mesh->plink_state = NL80211_PLINK_BLOCKED;
+	mesh_path_flush_by_nexthop(sta);
+
+	ieee80211_mps_sta_status_update(sta);
+	changed |= ieee80211_mps_set_sta_local_pm(sta,
+			NL80211_MESH_POWER_UNKNOWN);
+
+	return changed;
+}
+
+/**
+ * mesh_plink_deactivate - deactivate mesh peer link
+ *
+ * @sta: mesh peer link to deactivate
+ *
+ * All mesh paths with this peer as next hop will be flushed
+ */
+u32 mesh_plink_deactivate(struct sta_info *sta)
+{
+	struct ieee80211_sub_if_data *sdata = sta->sdata;
+	u32 changed;
+
+	spin_lock_bh(&sta->mesh->plink_lock);
+	changed = __mesh_plink_deactivate(sta);
+	sta->mesh->reason = WLAN_REASON_MESH_PEER_CANCELED;
+	mesh_plink_frame_tx(sdata, WLAN_SP_MESH_PEERING_CLOSE,
+			    sta->sta.addr, sta->mesh->llid, sta->mesh->plid,
+			    sta->mesh->reason);
+	spin_unlock_bh(&sta->mesh->plink_lock);
+
+	return changed;
+}
+
 static void mesh_sta_info_init(struct ieee80211_sub_if_data *sdata,
 			       struct sta_info *sta,
 			       struct ieee802_11_elems *elems, bool insert)
-- 
2.1.4

[PATCH 2/3] mac80211: mesh: separate plid and aid concepts

[Bob Copeland]

According to 802.11-2012 13.3.1, a mesh STA should assign an AID
upon receipt of a mesh peering open frame rather than using the link
id of the peer.  Using the peer link id has two potential issues:
it may not be unique among the peers, and by its nature it is random,
so the TIM may not compress well.

In preparation for allocating it properly, use sta->sta.aid, but keep
the existing behavior of using the plid.

Additionally, fix a bug where the AID is written into the wrong place
in peering confirm frame skbs.

Signed-off-by: Bob Copeland <me@bobcopeland.com>
---
 net/mac80211/mesh_plink.c | 20 ++++++++++++--------
 net/mac80211/sta_info.c   |  5 +----
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
index 7b071e36e55e..9dca16bd3eb9 100644
--- a/net/mac80211/mesh_plink.c
+++ b/net/mac80211/mesh_plink.c
@@ -200,6 +200,7 @@ static u32 mesh_set_ht_prot_mode(struct ieee80211_sub_if_data *sdata)
 }
 
 static int mesh_plink_frame_tx(struct ieee80211_sub_if_data *sdata,
+			       struct sta_info *sta,
 			       enum ieee80211_self_protected_actioncode action,
 			       u8 *da, u16 llid, u16 plid, u16 reason)
 {
@@ -249,7 +250,7 @@ static int mesh_plink_frame_tx(struct ieee80211_sub_if_data *sdata,
 		if (action == WLAN_SP_MESH_PEERING_CONFIRM) {
 			/* AID */
 			pos = skb_put(skb, 2);
-			put_unaligned_le16(plid, pos + 2);
+			put_unaligned_le16(sta->sta.aid, pos);
 		}
 		if (ieee80211_add_srates_ie(sdata, skb, true, band) ||
 		    ieee80211_add_ext_srates_ie(sdata, skb, true, band) ||
@@ -362,7 +363,7 @@ u32 mesh_plink_deactivate(struct sta_info *sta)
 	spin_lock_bh(&sta->mesh->plink_lock);
 	changed = __mesh_plink_deactivate(sta);
 	sta->mesh->reason = WLAN_REASON_MESH_PEER_CANCELED;
-	mesh_plink_frame_tx(sdata, WLAN_SP_MESH_PEERING_CLOSE,
+	mesh_plink_frame_tx(sdata, sta, WLAN_SP_MESH_PEERING_CLOSE,
 			    sta->sta.addr, sta->mesh->llid, sta->mesh->plid,
 			    sta->mesh->reason);
 	spin_unlock_bh(&sta->mesh->plink_lock);
@@ -619,7 +620,7 @@ static void mesh_plink_timer(unsigned long data)
 	}
 	spin_unlock_bh(&sta->mesh->plink_lock);
 	if (action)
-		mesh_plink_frame_tx(sdata, action, sta->sta.addr,
+		mesh_plink_frame_tx(sdata, sta, action, sta->sta.addr,
 				    sta->mesh->llid, sta->mesh->plid, reason);
 }
 
@@ -689,7 +690,7 @@ u32 mesh_plink_open(struct sta_info *sta)
 	/* set the non-peer mode to active during peering */
 	changed = ieee80211_mps_local_status_update(sdata);
 
-	mesh_plink_frame_tx(sdata, WLAN_SP_MESH_PEERING_OPEN,
+	mesh_plink_frame_tx(sdata, sta, WLAN_SP_MESH_PEERING_OPEN,
 			    sta->sta.addr, sta->mesh->llid, 0, 0);
 	return changed;
 }
@@ -871,13 +872,13 @@ static u32 mesh_plink_fsm(struct ieee80211_sub_if_data *sdata,
 	}
 	spin_unlock_bh(&sta->mesh->plink_lock);
 	if (action) {
-		mesh_plink_frame_tx(sdata, action, sta->sta.addr,
+		mesh_plink_frame_tx(sdata, sta, action, sta->sta.addr,
 				    sta->mesh->llid, sta->mesh->plid,
 				    sta->mesh->reason);
 
 		/* also send confirm in open case */
 		if (action == WLAN_SP_MESH_PEERING_OPEN) {
-			mesh_plink_frame_tx(sdata,
+			mesh_plink_frame_tx(sdata, sta,
 					    WLAN_SP_MESH_PEERING_CONFIRM,
 					    sta->sta.addr, sta->mesh->llid,
 					    sta->mesh->plid, 0);
@@ -1067,8 +1068,9 @@ mesh_process_plink_frame(struct ieee80211_sub_if_data *sdata,
 			goto unlock_rcu;
 		}
 		sta->mesh->plid = plid;
+		sta->sta.aid = plid;
 	} else if (!sta && event == OPN_RJCT) {
-		mesh_plink_frame_tx(sdata, WLAN_SP_MESH_PEERING_CLOSE,
+		mesh_plink_frame_tx(sdata, NULL, WLAN_SP_MESH_PEERING_CLOSE,
 				    mgmt->sa, 0, plid,
 				    WLAN_REASON_MESH_CONFIG);
 		goto unlock_rcu;
@@ -1078,8 +1080,10 @@ mesh_process_plink_frame(struct ieee80211_sub_if_data *sdata,
 	}
 
 	/* 802.11-2012 13.3.7.2 - update plid on CNF if not set */
-	if (!sta->mesh->plid && event == CNF_ACPT)
+	if (!sta->mesh->plid && event == CNF_ACPT) {
 		sta->mesh->plid = plid;
+		sta->sta.aid = sta->mesh->plid;
+	}
 
 	changed |= mesh_plink_fsm(sdata, sta, event);
 
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 9da7d2bc271a..70cd9fa57424 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -635,7 +635,7 @@ static void __sta_info_recalc_tim(struct sta_info *sta, bool ignore_pending)
 	bool indicate_tim = false;
 	u8 ignore_for_tim = sta->sta.uapsd_queues;
 	int ac;
-	u16 id;
+	u16 id = sta->sta.aid;
 
 	if (sta->sdata->vif.type == NL80211_IFTYPE_AP ||
 	    sta->sdata->vif.type == NL80211_IFTYPE_AP_VLAN) {
@@ -643,12 +643,9 @@ static void __sta_info_recalc_tim(struct sta_info *sta, bool ignore_pending)
 			return;
 
 		ps = &sta->sdata->bss->ps;
-		id = sta->sta.aid;
 #ifdef CONFIG_MAC80211_MESH
 	} else if (ieee80211_vif_is_mesh(&sta->sdata->vif)) {
 		ps = &sta->sdata->u.mesh.ps;
-		/* TIM map only for 1 <= PLID <= IEEE80211_MAX_AID */
-		id = sta->mesh->plid % (IEEE80211_MAX_AID + 1);
 #endif
 	} else {
 		return;
-- 
2.1.4

[PATCH 3/3] mac80211: select an AID when creating new mesh STAs

[Bob Copeland]

Instead of using peer link id for AID, generate a new
AID when creating mesh STAs in the kernel peering manager.
This enables smaller TIM elements and more closely follows
the standard, and it also enables mesh to work on drivers
that require a valid AID when the STA is inserted (ath10k
firmware has this requirement, for example).

In the case of userspace-managed stations, we use the AID
from NL80211_CMD_NEW_STATION.

Signed-off-by: Bob Copeland <me@bobcopeland.com>
---
 net/mac80211/cfg.c         |  3 +++
 net/mac80211/ieee80211_i.h |  1 +
 net/mac80211/mesh.c        |  7 +++++++
 net/mac80211/mesh_plink.c  | 30 ++++++++++++++++++++++++------
 4 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 5789d8353505..25f0ff84900e 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1027,6 +1027,9 @@ static void sta_apply_mesh_params(struct ieee80211_local *local,
 	struct ieee80211_sub_if_data *sdata = sta->sdata;
 	u32 changed = 0;
 
+	if (params->aid)
+		set_bit(params->aid, sdata->u.mesh.aid_map);
+
 	if (params->sta_modify_mask & STATION_PARAM_APPLY_PLINK_STATE) {
 		switch (params->plink_state) {
 		case NL80211_PLINK_ESTAB:
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 52930e91c0fd..d691515259a7 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -689,6 +689,7 @@ struct ieee80211_if_mesh {
 	int ps_peers_light_sleep;
 	int ps_peers_deep_sleep;
 	struct ps_data ps;
+	DECLARE_BITMAP(aid_map, IEEE80211_MAX_AID + 1);
 	/* Channel Switching Support */
 	struct mesh_csa_settings __rcu *csa;
 	enum {
diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index e06a5ca7c9a9..f26df5fc29c9 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -150,6 +150,9 @@ void mesh_sta_cleanup(struct sta_info *sta)
 	struct ieee80211_sub_if_data *sdata = sta->sdata;
 	u32 changed;
 
+	if (sta->sta.aid)
+		clear_bit(sta->sta.aid, sdata->u.mesh.aid_map);
+
 	/*
 	 * maybe userspace handles peer allocation and peering, but in either
 	 * case the beacon is still generated by the kernel and we might need
@@ -1323,6 +1326,10 @@ void ieee80211_mesh_init_sdata(struct ieee80211_sub_if_data *sdata)
 	ifmsh->accepting_plinks = true;
 	atomic_set(&ifmsh->mpaths, 0);
 	mesh_rmc_init(sdata);
+
+	/* reserve aid 0 used for mcast indication */
+	set_bit(0, ifmsh->aid_map);
+
 	ifmsh->last_preq = jiffies;
 	ifmsh->next_perr = jiffies;
 	ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_NONE;
diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
index 9dca16bd3eb9..deab8be39627 100644
--- a/net/mac80211/mesh_plink.c
+++ b/net/mac80211/mesh_plink.c
@@ -421,20 +421,43 @@ out:
 	spin_unlock_bh(&sta->mesh->plink_lock);
 }
 
+static int mesh_allocate_aid(struct ieee80211_sub_if_data *sdata)
+{
+	int aid;
+
+	/* mark first empty bit as used in aid_map and return it */
+	do {
+		aid = find_first_zero_bit(sdata->u.mesh.aid_map,
+					  IEEE80211_MAX_AID + 1);
+	} while (aid <= IEEE80211_MAX_AID &&
+		 test_and_set_bit(aid, sdata->u.mesh.aid_map));
+
+	if (aid > IEEE80211_MAX_AID)
+		return -ENOBUFS;
+
+	return aid;
+}
+
 static struct sta_info *
 __mesh_sta_info_alloc(struct ieee80211_sub_if_data *sdata, u8 *hw_addr)
 {
 	struct sta_info *sta;
+	int aid;
 
 	if (sdata->local->num_sta >= MESH_MAX_PLINKS)
 		return NULL;
 
+	aid = mesh_allocate_aid(sdata);
+	if (aid < 0)
+		return NULL;
+
 	sta = sta_info_alloc(sdata, hw_addr, GFP_KERNEL);
 	if (!sta)
 		return NULL;
 
 	sta->mesh->plink_state = NL80211_PLINK_LISTEN;
 	sta->sta.wme = true;
+	sta->sta.aid = aid;
 
 	sta_info_pre_move_state(sta, IEEE80211_STA_AUTH);
 	sta_info_pre_move_state(sta, IEEE80211_STA_ASSOC);
@@ -658,8 +681,6 @@ static u16 mesh_get_new_llid(struct ieee80211_sub_if_data *sdata)
 
 	do {
 		get_random_bytes(&llid, sizeof(llid));
-		/* for mesh PS we still only have the AID range for TIM bits */
-		llid = (llid % IEEE80211_MAX_AID) + 1;
 	} while (llid_in_use(sdata, llid));
 
 	return llid;
@@ -1068,7 +1089,6 @@ mesh_process_plink_frame(struct ieee80211_sub_if_data *sdata,
 			goto unlock_rcu;
 		}
 		sta->mesh->plid = plid;
-		sta->sta.aid = plid;
 	} else if (!sta && event == OPN_RJCT) {
 		mesh_plink_frame_tx(sdata, NULL, WLAN_SP_MESH_PEERING_CLOSE,
 				    mgmt->sa, 0, plid,
@@ -1080,10 +1100,8 @@ mesh_process_plink_frame(struct ieee80211_sub_if_data *sdata,
 	}
 
 	/* 802.11-2012 13.3.7.2 - update plid on CNF if not set */
-	if (!sta->mesh->plid && event == CNF_ACPT) {
+	if (!sta->mesh->plid && event == CNF_ACPT)
 		sta->mesh->plid = plid;
-		sta->sta.aid = sta->mesh->plid;
-	}
 
 	changed |= mesh_plink_fsm(sdata, sta, event);
 
-- 
2.1.4

Re: [PATCH 3/3] mac80211: select an AID when creating new mesh STAs

[Johannes Berg]

On Thu, 2015-07-02 at 09:28 -0400, Bob Copeland wrote:
> 
> +	DECLARE_BITMAP(aid_map, IEEE80211_MAX_AID + 1);
> 
Is there really much point in keeping a long-lived bitmap rather than
iterating the existing stations when adding a new one? It's not such a
frequent operation after all.

johannes

Re: [PATCH 3/3] mac80211: select an AID when creating new mesh STAs

[Bob Copeland]

On Thu, Jul 02, 2015 at 03:31:38PM +0200, Johannes Berg wrote:
> On Thu, 2015-07-02 at 09:28 -0400, Bob Copeland wrote:
> > 
> > +	DECLARE_BITMAP(aid_map, IEEE80211_MAX_AID + 1);
> > 
> Is there really much point in keeping a long-lived bitmap rather than
> iterating the existing stations when adding a new one? It's not such a
> frequent operation after all.

Not really -- I tried it both ways initially, and the bitmap ended up
with less code and u.mesh was still (slightly) smaller than u.mgd, but I
agree it does feel kind of bloaty so I'll prep a version the other way.

-- 
Bob Copeland %% http://bobcopeland.com/

Re: [PATCH 3/3] mac80211: select an AID when creating new mesh STAs

[Johannes Berg]

On Thu, 2015-07-02 at 10:45 -0400, Bob Copeland wrote:
> On Thu, Jul 02, 2015 at 03:31:38PM +0200, Johannes Berg wrote:
> > On Thu, 2015-07-02 at 09:28 -0400, Bob Copeland wrote:
> > > 
> > > +	DECLARE_BITMAP(aid_map, IEEE80211_MAX_AID + 1);
> > > 
> > Is there really much point in keeping a long-lived bitmap rather 
> > than
> > iterating the existing stations when adding a new one? It's not 
> > such a
> > frequent operation after all.
> 
> Not really -- I tried it both ways initially, and the bitmap ended up
> with less code and u.mesh was still (slightly) smaller than u.mgd, 
> but I agree it does feel kind of bloaty so I'll prep a version the 
> other way.

I was less worried about the bloat part than the "we're maintaining
this data twice" and corruptions/missing some remove paths etc. That
doesn't seem very likely though since it goes through the common
alloc/destroy path, so I guess it's not a problem.

Whichever you prefer really, I don't really mind.

johannes