[PATCH 1/2] wifi: mac80211: introduce ieee80211_refresh_tx_agg_session_timer()

[PATCH 1/2] wifi: mac80211: introduce ieee80211_refresh_tx_agg_session_timer()

[Ryder Lee]

This allows low level drivers to refresh the tx agg session timer, based on
querying stats from the firmware usually. Especially for some mt76 devices
support .net_fill_forward_path would bypass mac80211, which leads to tx BA
session timeout for certain clients.

Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
---
 include/net/mac80211.h | 12 ++++++++++++
 net/mac80211/agg-tx.c  | 17 +++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 76a12bec71d5..920ea31620cd 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -5993,6 +5993,18 @@ void ieee80211_queue_delayed_work(struct ieee80211_hw *hw,
 				  struct delayed_work *dwork,
 				  unsigned long delay);
 
+/**
+ * ieee80211_refresh_tx_agg_session_timer - Refresh a tx agg session timer.
+ * @sta: the station for which to start a BA session
+ * @tid: the TID to BA on.
+ *
+ * This function allows low level driver to refresh tx agg session timer
+ * to maintain BA session, the session level will still be managed by the
+ * mac80211.
+ */
+void ieee80211_refresh_tx_agg_session_timer(struct ieee80211_sta *sta,
+					    u16 tid);
+
 /**
  * ieee80211_start_tx_ba_session - Start a tx Block Ack session.
  * @sta: the station for which to start a BA session
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index f9514bacbd4a..3b651e7f5a73 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -554,6 +554,23 @@ void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid)
 	ieee80211_send_addba_with_timeout(sta, tid_tx);
 }
 
+void ieee80211_refresh_tx_agg_session_timer(struct ieee80211_sta *pubsta,
+					    u16 tid)
+{
+	struct sta_info *sta = container_of(pubsta, struct sta_info, sta);
+	struct tid_ampdu_tx *tid_tx;
+
+	if (WARN_ON_ONCE(tid >= IEEE80211_NUM_TIDS))
+		return;
+
+	tid_tx = rcu_dereference(sta->ampdu_mlme.tid_tx[tid]);
+	if (!tid_tx)
+		return;
+
+	tid_tx->last_tx = jiffies;
+}
+EXPORT_SYMBOL(ieee80211_refresh_tx_agg_session_timer);
+
 /*
  * After accepting the AddBA Response we activated a timer,
  * resetting it after each frame that we send.
-- 
2.18.0

[PATCH 2/2] wifi: mt76: connac: refresh tx session timer for WED device

[Ryder Lee]

There's no keep-alive packet being received by mac80211 stack when WED
is enabled, which leads to tx BA session timeout. This patch calls
ieee80211_refresh_tx_agg_session_timer() to refresh timer according
to tx status reporting.

Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/net/wireless/mediatek/mt76/mt76_connac_mac.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/wireless/mediatek/mt76/mt76_connac_mac.c b/drivers/net/wireless/mediatek/mt76/mt76_connac_mac.c
index aed4ee95fb2e..8d316d3aab8f 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76_connac_mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76_connac_mac.c
@@ -582,6 +582,17 @@ bool mt76_connac2_mac_fill_txs(struct mt76_dev *dev, struct mt76_wcid *wcid,
 			le32_get_bits(txs_data[6], MT_TXS6_MPDU_FAIL_CNT);
 		stats->tx_retries +=
 			le32_get_bits(txs_data[7], MT_TXS7_MPDU_RETRY_CNT);
+
+		if (wcid->sta) {
+			struct ieee80211_sta *sta;
+			u8 tid;
+
+			sta = container_of((void *)wcid, struct ieee80211_sta,
+					   drv_priv);
+			tid = FIELD_GET(MT_TXS0_TID, txs);
+
+			ieee80211_refresh_tx_agg_session_timer(sta, tid);
+		}
 	}
 
 	txrate = FIELD_GET(MT_TXS0_TX_RATE, txs);
-- 
2.18.0

Re: [PATCH 1/2] wifi: mac80211: introduce ieee80211_refresh_tx_agg_session_timer()

[Johannes Berg]

On Sat, 2023-02-18 at 01:50 +0800, Ryder Lee wrote:
> This allows low level drivers to refresh the tx agg session timer, based on
> querying stats from the firmware usually. Especially for some mt76 devices
> support .net_fill_forward_path would bypass mac80211, which leads to tx BA
> session timeout for certain clients.
> 

Does it even matter? We could just request sessions without a timeout in
the first place.

Or do you have a strong reason to need the timeout, such as limited
hardware resources for (TX) aggregation sessions?

But then maybe you should just time them out based on FW statistics
directly, rather than having to periodically refresh the timer in
mac80211?

I don't mind the patch, and I'll happily take it if it's needed, I'm
just wondering if that isn't a very roundabout way of achieving things.

johannes

Re: [PATCH 1/2] wifi: mac80211: introduce ieee80211_refresh_tx_agg_session_timer()

[Ryder Lee]

On Fri, 2023-02-17 at 19:01 +0100, Johannes Berg wrote:
> On Sat, 2023-02-18 at 01:50 +0800, Ryder Lee wrote:
> > This allows low level drivers to refresh the tx agg session timer,
> > based on
> > querying stats from the firmware usually. Especially for some mt76
> > devices
> > support .net_fill_forward_path would bypass mac80211, which leads
> > to tx BA
> > session timeout for certain clients.
> > 
> 
> Does it even matter? We could just request sessions without a timeout
> in
> the first place.
> 

I think we're already. Our main issue is performance periodically drops
every few seconds when .net_fill_forward_path is enabled. Wireless
client have normal 500+ Mb/s iperf3 download speed for several seconds.
Then it drops less than 100 Mb/s for several seconds. Then everything
repeats. Issue occurs only on certain clients. (i.e. Intel cards AX200,
AX1675, Advanced-N 6235 in Win11)

> Or do you have a strong reason to need the timeout, such as limited
> hardware resources for (TX) aggregation sessions?
> 
> But then maybe you should just time them out based on FW statistics
> directly, rather than having to periodically refresh the timer in
> mac80211?
> 
> I don't mind the patch, and I'll happily take it if it's needed, I'm
> just wondering if that isn't a very roundabout way of achieving
> things.
> 
> johannes

Re: [PATCH 1/2] wifi: mac80211: introduce ieee80211_refresh_tx_agg_session_timer()

[Johannes Berg]

On Fri, 2023-02-17 at 18:43 +0000, Ryder Lee wrote:
> On Fri, 2023-02-17 at 19:01 +0100, Johannes Berg wrote:
> > On Sat, 2023-02-18 at 01:50 +0800, Ryder Lee wrote:
> > > This allows low level drivers to refresh the tx agg session timer,
> > > based on
> > > querying stats from the firmware usually. Especially for some mt76
> > > devices
> > > support .net_fill_forward_path would bypass mac80211, which leads
> > > to tx BA
> > > session timeout for certain clients.
> > > 
> > 
> > Does it even matter? We could just request sessions without a timeout
> > in
> > the first place.
> > 
> 
> I think we're already. Our main issue is performance periodically drops
> every few seconds when .net_fill_forward_path is enabled. Wireless
> client have normal 500+ Mb/s iperf3 download speed for several seconds.
> Then it drops less than 100 Mb/s for several seconds. Then everything
> repeats. Issue occurs only on certain clients. (i.e. Intel cards AX200,
> AX1675, Advanced-N 6235 in Win11)
> 

Strange. But how does this patch do anything about it, that should be
completely client agnostic?

johannes

Re: [PATCH 1/2] wifi: mac80211: introduce ieee80211_refresh_tx_agg_session_timer()

[Ryder Lee]

On Fri, 2023-02-17 at 19:53 +0100, Johannes Berg wrote:
> On Fri, 2023-02-17 at 18:43 +0000, Ryder Lee wrote:
> > On Fri, 2023-02-17 at 19:01 +0100, Johannes Berg wrote:
> > > On Sat, 2023-02-18 at 01:50 +0800, Ryder Lee wrote:
> > > > This allows low level drivers to refresh the tx agg session
> > > > timer,
> > > > based on
> > > > querying stats from the firmware usually. Especially for some
> > > > mt76
> > > > devices
> > > > support .net_fill_forward_path would bypass mac80211, which
> > > > leads
> > > > to tx BA
> > > > session timeout for certain clients.
> > > > 
> > > 
> > > Does it even matter? We could just request sessions without a
> > > timeout
> > > in
> > > the first place.
> > > 
> > 
> > I think we're already. Our main issue is performance periodically
> > drops
> > every few seconds when .net_fill_forward_path is enabled. Wireless
> > client have normal 500+ Mb/s iperf3 download speed for several
> > seconds.
> > Then it drops less than 100 Mb/s for several seconds. Then
> > everything
> > repeats. Issue occurs only on certain clients. (i.e. Intel cards
> > AX200,
> > AX1675, Advanced-N 6235 in Win11)
> > 
> 
> Strange. But how does this patch do anything about it, that should be
> completely client agnostic?
> 
> 

Since there's no any keep alive packet being received by host stack,
leads to mac80211 destrory BA sesion.

Ax200 series needs to update timer for each 5s period to maintain ba
session. We originally did this to workaround issue, but obviouly this
hack will not be accepted upstream, since it effectively completely
disables the session expiry timer without removing the code.

--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -568,10 +568,9 @@ static void
sta_tx_agg_session_timer_expired(struct timer_list *t)
 	}
 
 	timeout = tid_tx->last_tx + TU_TO_JIFFIES(tid_tx->timeout);
-	if (time_is_after_jiffies(timeout)) {
-		mod_timer(&tid_tx->session_timer, timeout);
-		return;
-	}
+	/* remove timerout handle for ax210 interoperability issue */
+	mod_timer(&tid_tx->session_timer, timeout);
+	return;

I'm not sure if there's a better way to fix this though.

Ryder

Re: [PATCH 1/2] wifi: mac80211: introduce ieee80211_refresh_tx_agg_session_timer()

[Ryder Lee]

On Fri, 2023-02-17 at 19:02 +0000, Ryder Lee wrote:
> On Fri, 2023-02-17 at 19:53 +0100, Johannes Berg wrote:
> > On Fri, 2023-02-17 at 18:43 +0000, Ryder Lee wrote:
> > > On Fri, 2023-02-17 at 19:01 +0100, Johannes Berg wrote:
> > > > On Sat, 2023-02-18 at 01:50 +0800, Ryder Lee wrote:
> > > > > This allows low level drivers to refresh the tx agg session
> > > > > timer,
> > > > > based on
> > > > > querying stats from the firmware usually. Especially for some
> > > > > mt76
> > > > > devices
> > > > > support .net_fill_forward_path would bypass mac80211, which
> > > > > leads
> > > > > to tx BA
> > > > > session timeout for certain clients.
> > > > > 
> > > > 
> > > > Does it even matter? We could just request sessions without a
> > > > timeout
> > > > in
> > > > the first place.
> > > > 
> > > 
> > > I think we're already. Our main issue is performance periodically
> > > drops
> > > every few seconds when .net_fill_forward_path is enabled.
> > > Wireless
> > > client have normal 500+ Mb/s iperf3 download speed for several
> > > seconds.
> > > Then it drops less than 100 Mb/s for several seconds. Then
> > > everything
> > > repeats. Issue occurs only on certain clients. (i.e. Intel cards
> > > AX200,
> > > AX1675, Advanced-N 6235 in Win11)
> > > 
> > 
> > Strange. But how does this patch do anything about it, that should
> > be
> > completely client agnostic?
> > 
> > 
> 
> Since there's no any keep alive packet being received by host stack,
> leads to mac80211 destrory BA sesion.
> 

More specifically, the BA session relies on client side's Tx data to
maintain ... but the point is mac80211 can't get any data after
.net_fill_forward_path being enabled (HW path). So, we need a way to
notify mac80211 to refresh last_tx... I think my patch is needed for
that case.

Ryder

Re: [PATCH 1/2] wifi: mac80211: introduce ieee80211_refresh_tx_agg_session_timer()

[Ryder Lee]

On Mon, 2023-02-20 at 02:55 +0000, Ryder Lee wrote:
> On Fri, 2023-02-17 at 19:02 +0000, Ryder Lee wrote:
> > On Fri, 2023-02-17 at 19:53 +0100, Johannes Berg wrote:
> > > On Fri, 2023-02-17 at 18:43 +0000, Ryder Lee wrote:
> > > > On Fri, 2023-02-17 at 19:01 +0100, Johannes Berg wrote:
> > > > > On Sat, 2023-02-18 at 01:50 +0800, Ryder Lee wrote:
> > > > > > This allows low level drivers to refresh the tx agg session
> > > > > > timer,
> > > > > > based on
> > > > > > querying stats from the firmware usually. Especially for
> > > > > > some
> > > > > > mt76
> > > > > > devices
> > > > > > support .net_fill_forward_path would bypass mac80211, which
> > > > > > leads
> > > > > > to tx BA
> > > > > > session timeout for certain clients.
> > > > > > 
> > > > > 
> > > > > Does it even matter? We could just request sessions without a
> > > > > timeout
> > > > > in
> > > > > the first place.
> > > > > 
> > > > 
> > > > I think we're already. Our main issue is performance
> > > > periodically
> > > > drops
> > > > every few seconds when .net_fill_forward_path is enabled.
> > > > Wireless
> > > > client have normal 500+ Mb/s iperf3 download speed for several
> > > > seconds.
> > > > Then it drops less than 100 Mb/s for several seconds. Then
> > > > everything
> > > > repeats. Issue occurs only on certain clients. (i.e. Intel
> > > > cards
> > > > AX200,
> > > > AX1675, Advanced-N 6235 in Win11)
> > > > 
> > > 
> > > Strange. But how does this patch do anything about it, that
> > > should
> > > be
> > > completely client agnostic?
> > > 
> > > 
> > 
> > Since there's no any keep alive packet being received by host
> > stack,
> > leads to mac80211 destrory BA sesion.
> > 
> 
> More specifically, the BA session relies on client side's Tx data to

Typo... I mean *our side*. Something like this

ieee80211_8023_xmit()
if (tid_tx->timeout)
	tid_tx->last_tx = jiffies;
Ryder

Re: [PATCH 1/2] wifi: mac80211: introduce ieee80211_refresh_tx_agg_session_timer()

[Johannes Berg]

Hi,

> > > Since there's no any keep alive packet being received by host
> > > stack, leads to mac80211 destrory BA sesion.
> > > 
> > 
> > More specifically, the BA session relies on client side's Tx data to
> 
> Typo... I mean *our side*. Something like this

Sorry. I'm just totally confused - I thought the initiator only set the
timeout, but I see now that it's negotiated and the actual value used is
from the client.

Which explains basically everything.

johannes

Re: [PATCH 1/2] wifi: mac80211: introduce ieee80211_refresh_tx_agg_session_timer()

[Ryder Lee]

On Tue, 2023-02-21 at 10:57 +0100, Johannes Berg wrote:
> Hi,
> 
> > > > Since there's no any keep alive packet being received by host
> > > > stack, leads to mac80211 destrory BA sesion.
> > > > 
> > > 
> > > More specifically, the BA session relies on client side's Tx data
> > > to
> > 
> > Typo... I mean *our side*. Something like this
> 
> Sorry. I'm just totally confused - I thought the initiator only set
> the
> timeout, but I see now that it's negotiated and the actual value used
> is
> from the client.
> 
> Which explains basically everything.
> 
> 

Yup ... after accepting the AddBA Response we activated a timer,
*resetting it after each frame that we send* -
sta_tx_agg_session_timer_expired().

The .net_fill_forward_path() offloads tx path to HW, so it can only
rely on other way to reset as mac80211 isn't aware of that.

Ryder