[PATCH 0/3] IOP RX BA session issues

[PATCH 0/3] IOP RX BA session issues

[Maxim Altshul]

Some background: 

There exist some devices (latest mobile phones and some AP's e.g LG G4 H815, 
Samsung Galaxy S5 G900H) that tend to not respect a BA 
sessions maximum size (in Kbps). 

These devices won't respect the AMPDU size that was negotiated 
during associasion (even though they do respect the maximal number of packets).

This violation is characterized by a valid number of 
packets in a single AMPDU who all have the same size.
Even so, the total size will exceed the size negotiated during association.

Eventually, this will cause some undefined behavior, 
which in turn causes the hw to drop packets, causing the throughput to plummet.

The fix is separated to 3 patches:
wlcore patches (patches 2+3):
Add a functionality in which the firmware is able 
to notify the driver that the aggregation window size 
must be changed, and in turn the driver will notify mac80211.

mac80211 patch (patch 1) which will:
a. Make the subframe limitation to be held by each station, 
instead of being held only by hw.
b. Create an api for the driver to call which will remove all violating
BA sessions with a specific peer.

Reopening the BA sessions with a different size will limit the aggregation 
window size, which will in turn limit the maximum size in bytes of the whole aggregation.

I hope that the commit messages are clear enough, and if not then I will change them upon request.

Maxim Altshul (3):
  mac80211: RX BA support for sta max_rx_aggregation_subframes
  wlcore: Pass win_size taken from ieee80211_sta to FW
  wlcore: Add RX_BA_WIN_SIZE_CHANGE_EVENT event

 drivers/net/wireless/ti/wl18xx/event.c | 22 ++++++++++++++++++++++
 drivers/net/wireless/ti/wl18xx/event.h |  1 +
 drivers/net/wireless/ti/wl18xx/main.c  |  3 ++-
 drivers/net/wireless/ti/wlcore/acx.c   |  5 +++--
 drivers/net/wireless/ti/wlcore/acx.h   |  3 ++-
 drivers/net/wireless/ti/wlcore/main.c  |  6 ++++--
 include/net/mac80211.h                 | 22 ++++++++++++++++++++++
 net/mac80211/agg-rx.c                  | 31 +++++++++++++++++++++++++++----
 net/mac80211/ht.c                      | 18 ++++++++++++++++++
 net/mac80211/sta_info.c                |  3 +++
 10 files changed, 104 insertions(+), 10 deletions(-)

-- 
2.9.0

[PATCH 1/3] mac80211: RX BA support for sta max_rx_aggregation_subframes

[Maxim Altshul]

The ability to change the max_rx_aggregation frames is useful
in cases of IOP.

There exist some devices (latest mobile phones and some AP's)
that tend to not respect a BA sessions maximum size (in Kbps).
These devices won't respect the AMPDU size that was negotiated during
associasion (even though they do respect the maximal number of packets).

This violation is characterized by a valid number of packets in
a single AMPDU. Even so, the total size will exceed the size negotiated
during association.

Eventually, this will cause some undefined behavior, which in turn
causes the hw to drop packets, causing the throughput to plummet.

This patch will:
a. Make the subframe limitation to be held by each station,
instead of being held only by hw.
b. Create an api for the driver to call which will remove violating
BA sessions with a specific peer. When the session is reopened, it
will use the new size, limiting the aggregation.

Signed-off-by: Maxim Altshul <maxim.altshul@ti.com>
---
 include/net/mac80211.h  | 22 ++++++++++++++++++++++
 net/mac80211/agg-rx.c   | 31 +++++++++++++++++++++++++++----
 net/mac80211/ht.c       | 18 ++++++++++++++++++
 net/mac80211/sta_info.c |  3 +++
 4 files changed, 70 insertions(+), 4 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index cca510a..eec97d5 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1735,6 +1735,9 @@ struct ieee80211_sta_rates {
  * @supp_rates: Bitmap of supported rates (per band)
  * @ht_cap: HT capabilities of this STA; restricted to our own capabilities
  * @vht_cap: VHT capabilities of this STA; restricted to our own capabilities
+ * @max_rx_aggregation_subframes: restriction on rx buff size for this active
+ *	aggregation. Initially set to local->hw.max_rx_aggregation_subframes but
+ *	can be modified by driver.
  * @wme: indicates whether the STA supports QoS/WME (if local devices does,
  *	otherwise always false)
  * @drv_priv: data area for driver use, will always be aligned to
@@ -1775,6 +1778,7 @@ struct ieee80211_sta {
 	u16 aid;
 	struct ieee80211_sta_ht_cap ht_cap;
 	struct ieee80211_sta_vht_cap vht_cap;
+	u8 max_rx_aggregation_subframes;
 	bool wme;
 	u8 uapsd_queues;
 	u8 max_sp;
@@ -5281,6 +5285,24 @@ void ieee80211_mark_rx_ba_filtered_frames(struct ieee80211_sta *pubsta, u8 tid,
 					  u16 received_mpdus);
 
 /**
+ * ieee80211_change_rx_ba_max_subframes - callback to change
+ * sta.max_rx_aggregation_subframes and stop existing BA sessions
+ *
+ * This capability is useful in cases of IOP, i.e. cases where peer sta
+ * or ap doesn't respect the max size (Kbps) of an AMPDU.
+ * In these cases the driver/chip may recover by decreasing the
+ * max_rx_aggregation_subframes, which will in turn reduce the size of
+ * the whole aggregation.
+ *
+ * @vif: &struct ieee80211_vif pointer from the add_interface callback.
+ * @addr: & to bssid mac address
+ * @max_subframes: new max_rx_aggregation_subframes for this sta
+ */
+void ieee80211_change_rx_ba_max_subframes(struct ieee80211_vif *vif,
+					  const u8 *addr,
+					  u8 max_subframes);
+
+/**
  * ieee80211_send_bar - send a BlockAckReq frame
  *
  * can be used to flush pending frames from the peer's aggregation reorder
diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index a9aff60..8089234 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -147,6 +147,27 @@ void ieee80211_stop_rx_ba_session(struct ieee80211_vif *vif, u16 ba_rx_bitmap,
 }
 EXPORT_SYMBOL(ieee80211_stop_rx_ba_session);
 
+void ieee80211_change_rx_ba_max_subframes(struct ieee80211_vif *vif,
+					  const u8 *addr,
+					  u8 max_subframes)
+{
+	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
+	struct sta_info *sta;
+
+	rcu_read_lock();
+	sta = sta_info_get_bss(sdata, addr);
+
+	if (!sta) {
+		rcu_read_unlock();
+		return;
+	}
+
+	sta->sta.max_rx_aggregation_subframes = max_subframes;
+	ieee80211_queue_work(&sta->local->hw, &sta->ampdu_mlme.work);
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL(ieee80211_change_rx_ba_max_subframes);
+
 /*
  * After accepting the AddBA Request we activated a timer,
  * resetting it after each frame that arrives from the originator.
@@ -297,13 +318,15 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
 	if (buf_size == 0)
 		buf_size = IEEE80211_MAX_AMPDU_BUF;
 
+	/* examine state machine */
+	mutex_lock(&sta->ampdu_mlme.mtx);
+
 	/* make sure the size doesn't exceed the maximum supported by the hw */
-	if (buf_size > local->hw.max_rx_aggregation_subframes)
-		buf_size = local->hw.max_rx_aggregation_subframes;
+	if (buf_size > sta->sta.max_rx_aggregation_subframes)
+		buf_size = sta->sta.max_rx_aggregation_subframes;
 	params.buf_size = buf_size;
 
-	/* examine state machine */
-	mutex_lock(&sta->ampdu_mlme.mtx);
+	ht_dbg(sta->sdata, "AddBA Req buf_size=%d\n", buf_size);
 
 	if (test_bit(tid, sta->ampdu_mlme.agg_session_valid)) {
 		tid_agg_rx = rcu_dereference_protected(
diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index f4a5287..eaff7a8 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -305,6 +305,7 @@ void ieee80211_ba_session_work(struct work_struct *work)
 	struct sta_info *sta =
 		container_of(work, struct sta_info, ampdu_mlme.work);
 	struct tid_ampdu_tx *tid_tx;
+	struct tid_ampdu_rx *tid_rx;
 	int tid;
 
 	/*
@@ -329,6 +330,23 @@ void ieee80211_ba_session_work(struct work_struct *work)
 				sta, tid, WLAN_BACK_RECIPIENT,
 				WLAN_REASON_UNSPECIFIED, true);
 
+		/* Stop RX BA sessions affected by change of
+		 * sta.max_rx_aggregation_subframe
+		 */
+		tid_rx = sta->ampdu_mlme.tid_rx[tid];
+		if (tid_rx &&
+		    tid_rx->buf_size > sta->sta.max_rx_aggregation_subframes) {
+			ht_dbg(sta->sdata,
+			       "buf_size(%d) > max_subframes(%d) stopping tid %d\n",
+			       tid_rx->buf_size,
+			       sta->sta.max_rx_aggregation_subframes,
+			       tid);
+
+			___ieee80211_stop_rx_ba_session(
+				sta, tid, WLAN_BACK_RECIPIENT,
+				WLAN_REASON_UNSPECIFIED, true);
+		}
+
 		spin_lock_bh(&sta->lock);
 
 		tid_tx = sta->ampdu_mlme.tid_start_tx[tid];
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 19f14c9..5e70fa5 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -340,6 +340,9 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
 
 	memcpy(sta->addr, addr, ETH_ALEN);
 	memcpy(sta->sta.addr, addr, ETH_ALEN);
+	sta->sta.max_rx_aggregation_subframes =
+		local->hw.max_rx_aggregation_subframes;
+
 	sta->local = local;
 	sta->sdata = sdata;
 	sta->rx_stats.last_rx = jiffies;
-- 
2.9.0

[PATCH 2/3] wlcore: Pass win_size taken from ieee80211_sta to FW

[Maxim Altshul]

When starting a new BA session, we must pass the win_size to the FW.

To do this we take max_rx_aggregation_subframes (BA RX win size)
which is stored in ieee80211_sta structure (e.g per link and not per HW)

We will use the value stored per link when passing the win_size to
firmware through the ACX_BA_SESSION_RX_SETUP command.

Signed-off-by: Maxim Altshul <maxim.altshul@ti.com>
---
 drivers/net/wireless/ti/wlcore/acx.c  | 5 +++--
 drivers/net/wireless/ti/wlcore/acx.h  | 3 ++-
 drivers/net/wireless/ti/wlcore/main.c | 6 ++++--
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/acx.c b/drivers/net/wireless/ti/wlcore/acx.c
index 26cc23f..a485999 100644
--- a/drivers/net/wireless/ti/wlcore/acx.c
+++ b/drivers/net/wireless/ti/wlcore/acx.c
@@ -1419,7 +1419,8 @@ out:
 
 /* setup BA session receiver setting in the FW. */
 int wl12xx_acx_set_ba_receiver_session(struct wl1271 *wl, u8 tid_index,
-				       u16 ssn, bool enable, u8 peer_hlid)
+				       u16 ssn, bool enable, u8 peer_hlid,
+				       u8 win_size)
 {
 	struct wl1271_acx_ba_receiver_setup *acx;
 	int ret;
@@ -1435,7 +1436,7 @@ int wl12xx_acx_set_ba_receiver_session(struct wl1271 *wl, u8 tid_index,
 	acx->hlid = peer_hlid;
 	acx->tid = tid_index;
 	acx->enable = enable;
-	acx->win_size = wl->conf.ht.rx_ba_win_size;
+	acx->win_size =	win_size;
 	acx->ssn = ssn;
 
 	ret = wlcore_cmd_configure_failsafe(wl, ACX_BA_SESSION_RX_SETUP, acx,
diff --git a/drivers/net/wireless/ti/wlcore/acx.h b/drivers/net/wireless/ti/wlcore/acx.h
index 6321ed4..f46d7fd 100644
--- a/drivers/net/wireless/ti/wlcore/acx.h
+++ b/drivers/net/wireless/ti/wlcore/acx.h
@@ -1113,7 +1113,8 @@ int wl1271_acx_set_ht_information(struct wl1271 *wl,
 int wl12xx_acx_set_ba_initiator_policy(struct wl1271 *wl,
 				       struct wl12xx_vif *wlvif);
 int wl12xx_acx_set_ba_receiver_session(struct wl1271 *wl, u8 tid_index,
-				       u16 ssn, bool enable, u8 peer_hlid);
+				       u16 ssn, bool enable, u8 peer_hlid,
+				       u8 win_size);
 int wl12xx_acx_tsf_info(struct wl1271 *wl, struct wl12xx_vif *wlvif,
 			u64 *mactime);
 int wl1271_acx_ps_rx_streaming(struct wl1271 *wl, struct wl12xx_vif *wlvif,
diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
index 9e1f2d9..9ea477e 100644
--- a/drivers/net/wireless/ti/wlcore/main.c
+++ b/drivers/net/wireless/ti/wlcore/main.c
@@ -5286,7 +5286,9 @@ static int wl1271_op_ampdu_action(struct ieee80211_hw *hw,
 		}
 
 		ret = wl12xx_acx_set_ba_receiver_session(wl, tid, *ssn, true,
-							 hlid);
+				hlid,
+				sta->max_rx_aggregation_subframes);
+
 		if (!ret) {
 			*ba_bitmap |= BIT(tid);
 			wl->ba_rx_session_count++;
@@ -5307,7 +5309,7 @@ static int wl1271_op_ampdu_action(struct ieee80211_hw *hw,
 		}
 
 		ret = wl12xx_acx_set_ba_receiver_session(wl, tid, 0, false,
-							 hlid);
+							 hlid, 0);
 		if (!ret) {
 			*ba_bitmap &= ~BIT(tid);
 			wl->ba_rx_session_count--;
-- 
2.9.0

[PATCH 3/3] wlcore: Add RX_BA_WIN_SIZE_CHANGE_EVENT event

[Maxim Altshul]

This event is used by the Firmware to limit the RX BA win size
for a specific link.

The event handler updates the new size in the mac's sta->sta struct.

BA sessions opened for that link will use the new restricted
win_size. This limitation remains until a new update is received or
until the link is closed.

Signed-off-by: Maxim Altshul <maxim.altshul@ti.com>
---
 drivers/net/wireless/ti/wl18xx/event.c | 22 ++++++++++++++++++++++
 drivers/net/wireless/ti/wl18xx/event.h |  1 +
 drivers/net/wireless/ti/wl18xx/main.c  |  3 ++-
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ti/wl18xx/event.c b/drivers/net/wireless/ti/wl18xx/event.c
index 2c5df43..a69be96 100644
--- a/drivers/net/wireless/ti/wl18xx/event.c
+++ b/drivers/net/wireless/ti/wl18xx/event.c
@@ -217,5 +217,27 @@ int wl18xx_process_mailbox_events(struct wl1271 *wl)
 	if (vector & FW_LOGGER_INDICATION)
 		wlcore_event_fw_logger(wl);
 
+	if (vector & RX_BA_WIN_SIZE_CHANGE_EVENT_ID) {
+		struct wl12xx_vif *wlvif;
+		struct ieee80211_vif *vif;
+		u8 link_id = mbox->rx_ba_link_id;
+		u8 win_size = mbox->rx_ba_win_size;
+		const u8 *addr;
+
+		wlvif = wl->links[link_id].wlvif;
+		vif = wl12xx_wlvif_to_vif(wlvif);
+
+		/* Call MAC routine to update win_size and stop all link active
+		 * BA sessions. This routine returns 0 on failure or previous
+		 * win_size on success
+		 */
+		if (wlvif->bss_type != BSS_TYPE_AP_BSS)
+			addr = vif->bss_conf.bssid;
+		else
+			addr = wl->links[link_id].addr;
+
+		ieee80211_change_rx_ba_max_subframes(vif, addr, win_size);
+	}
+
 	return 0;
 }
diff --git a/drivers/net/wireless/ti/wl18xx/event.h b/drivers/net/wireless/ti/wl18xx/event.h
index ce8ea9c0..4af297f 100644
--- a/drivers/net/wireless/ti/wl18xx/event.h
+++ b/drivers/net/wireless/ti/wl18xx/event.h
@@ -38,6 +38,7 @@ enum {
 	REMAIN_ON_CHANNEL_COMPLETE_EVENT_ID      = BIT(18),
 	DFS_CHANNELS_CONFIG_COMPLETE_EVENT       = BIT(19),
 	PERIODIC_SCAN_REPORT_EVENT_ID            = BIT(20),
+	RX_BA_WIN_SIZE_CHANGE_EVENT_ID           = BIT(21),
 	SMART_CONFIG_SYNC_EVENT_ID               = BIT(22),
 	SMART_CONFIG_DECODE_EVENT_ID             = BIT(23),
 	TIME_SYNC_EVENT_ID                       = BIT(24),
diff --git a/drivers/net/wireless/ti/wl18xx/main.c b/drivers/net/wireless/ti/wl18xx/main.c
index 00a04df..dafb2fd 100644
--- a/drivers/net/wireless/ti/wl18xx/main.c
+++ b/drivers/net/wireless/ti/wl18xx/main.c
@@ -1041,7 +1041,8 @@ static int wl18xx_boot(struct wl1271 *wl)
 		SMART_CONFIG_SYNC_EVENT_ID |
 		SMART_CONFIG_DECODE_EVENT_ID |
 		TIME_SYNC_EVENT_ID |
-		FW_LOGGER_INDICATION;
+		FW_LOGGER_INDICATION |
+		RX_BA_WIN_SIZE_CHANGE_EVENT_ID;
 
 	wl->ap_event_mask = MAX_TX_FAILURE_EVENT_ID;
 
-- 
2.9.0

Re: [PATCH 1/3] mac80211: RX BA support for sta max_rx_aggregation_subframes

[Johannes Berg]

On Thu, 2016-08-18 at 10:36 +0300, Maxim Altshul wrote:
> 
> @@ -1735,6 +1735,9 @@ struct ieee80211_sta_rates {
>   * @supp_rates: Bitmap of supported rates (per band)
>   * @ht_cap: HT capabilities of this STA; restricted to our own
> capabilities
>   * @vht_cap: VHT capabilities of this STA; restricted to our own
> capabilities
> + * @max_rx_aggregation_subframes: restriction on rx buff size for
> this active
> + *	aggregation. Initially set to local-
> >hw.max_rx_aggregation_subframes but
> + *	can be modified by driver.

The documentation for this makes no sense, it's clearly not a per
"active aggregation" parameter in any way.

>  /**
> + * ieee80211_change_rx_ba_max_subframes - callback to change
> + * sta.max_rx_aggregation_subframes and stop existing BA sessions
> + *
> + * This capability is useful in cases of IOP, i.e. cases where peer
> sta
> + * or ap doesn't respect the max size (Kbps) of an AMPDU.
> + * In these cases the driver/chip may recover by decreasing the
> + * max_rx_aggregation_subframes, which will in turn reduce the size
> of
> + * the whole aggregation.
> + *
> + * @vif: &struct ieee80211_vif pointer from the add_interface
> callback.
> + * @addr: & to bssid mac address
> + * @max_subframes: new max_rx_aggregation_subframes for this sta
> + */
> +void ieee80211_change_rx_ba_max_subframes(struct ieee80211_vif *vif,
> +					  const u8 *addr,
> +					  u8 max_subframes);

I see no reason for this to exist, it's practically equivalent to
something like this:

sta = ieee80211_find_sta(vif, addr);
if (sta)
	sta->max_subframes = max_subframes;
ieee80211_stop_rx_ba_session(vif, 0xff, addr);

so there's no real need for this. What you did has an advantage if the
driver is doing something stupid like calling the function with the
same argument multiple times, but that's easily fixed or prevented; I
don't really see any other advantages?

johannes

Re: [PATCH 2/3] wlcore: Pass win_size taken from ieee80211_sta to FW

[Johannes Berg]

On Thu, 2016-08-18 at 10:36 +0300, Maxim Altshul wrote:
> When starting a new BA session, we must pass the win_size to the FW.
> 
> To do this we take max_rx_aggregation_subframes (BA RX win size)
> which is stored in ieee80211_sta structure (e.g per link and not per
> HW)

I believe this is wrong, and you should use the struct
ieee80211_ampdu_params::buf_size value that's passed to ampdu_action
when the action is IEEE80211_AMPDU_RX_START.

johannes