LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] rtl8xxxu: Fix the handling of TX A-MPDU aggregation
@ 2021-08-04 15:13 chris.chiu
  2021-08-06  9:03 ` Kalle Valo
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: chris.chiu @ 2021-08-04 15:13 UTC (permalink / raw)
  To: jes.sorensen, kvalo, davem, kuba
  Cc: code, linux-wireless, netdev, linux-kernel, Chris Chiu

From: Chris Chiu <chris.chiu@canonical.com>

The TX A-MPDU aggregation is not handled in the driver since the
ieee80211_start_tx_ba_session has never been started properly.
Start and stop the TX BA session by tracking the TX aggregation
status of each TID. Fix the ampdu_action and the tx descriptor
accordingly with the given TID.

Signed-off-by: Chris Chiu <chris.chiu@canonical.com>
---

Changelog:
  v2:
   - use data type BITMAP for tx_aggr_started instead of bool array

 .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  |  2 ++
 .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 33 ++++++++++++++-----
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
index 01735776345a..7ddce3c3f0c4 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
@@ -1378,6 +1378,8 @@ struct rtl8xxxu_priv {
 	u8 no_pape:1;
 	u8 int_buf[USB_INTR_CONTENT_LENGTH];
 	u8 rssi_level;
+	DECLARE_BITMAP(tx_aggr_started, IEEE80211_NUM_TIDS);
+	DECLARE_BITMAP(tid_tx_operational, IEEE80211_NUM_TIDS);
 	/*
 	 * Only one virtual interface permitted because only STA mode
 	 * is supported and no iface_combinations are provided.
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index ac1061caacd6..3285a91efb91 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -4805,6 +4805,8 @@ rtl8xxxu_fill_txdesc_v1(struct ieee80211_hw *hw, struct ieee80211_hdr *hdr,
 	struct ieee80211_rate *tx_rate = ieee80211_get_tx_rate(hw, tx_info);
 	struct rtl8xxxu_priv *priv = hw->priv;
 	struct device *dev = &priv->udev->dev;
+	u8 *qc = ieee80211_get_qos_ctl(hdr);
+	u8 tid = qc[0] & IEEE80211_QOS_CTL_TID_MASK;
 	u32 rate;
 	u16 rate_flags = tx_info->control.rates[0].flags;
 	u16 seq_number;
@@ -4828,7 +4830,7 @@ rtl8xxxu_fill_txdesc_v1(struct ieee80211_hw *hw, struct ieee80211_hdr *hdr,
 
 	tx_desc->txdw3 = cpu_to_le32((u32)seq_number << TXDESC32_SEQ_SHIFT);
 
-	if (ampdu_enable)
+	if (ampdu_enable && test_bit(tid, priv->tid_tx_operational))
 		tx_desc->txdw1 |= cpu_to_le32(TXDESC32_AGG_ENABLE);
 	else
 		tx_desc->txdw1 |= cpu_to_le32(TXDESC32_AGG_BREAK);
@@ -4876,6 +4878,8 @@ rtl8xxxu_fill_txdesc_v2(struct ieee80211_hw *hw, struct ieee80211_hdr *hdr,
 	struct rtl8xxxu_priv *priv = hw->priv;
 	struct device *dev = &priv->udev->dev;
 	struct rtl8xxxu_txdesc40 *tx_desc40;
+	u8 *qc = ieee80211_get_qos_ctl(hdr);
+	u8 tid = qc[0] & IEEE80211_QOS_CTL_TID_MASK;
 	u32 rate;
 	u16 rate_flags = tx_info->control.rates[0].flags;
 	u16 seq_number;
@@ -4902,7 +4906,7 @@ rtl8xxxu_fill_txdesc_v2(struct ieee80211_hw *hw, struct ieee80211_hdr *hdr,
 
 	tx_desc40->txdw9 = cpu_to_le32((u32)seq_number << TXDESC40_SEQ_SHIFT);
 
-	if (ampdu_enable)
+	if (ampdu_enable && test_bit(tid, priv->tid_tx_operational))
 		tx_desc40->txdw2 |= cpu_to_le32(TXDESC40_AGG_ENABLE);
 	else
 		tx_desc40->txdw2 |= cpu_to_le32(TXDESC40_AGG_BREAK);
@@ -5015,12 +5019,19 @@ static void rtl8xxxu_tx(struct ieee80211_hw *hw,
 	if (ieee80211_is_data_qos(hdr->frame_control) && sta) {
 		if (sta->ht_cap.ht_supported) {
 			u32 ampdu, val32;
+			u8 *qc = ieee80211_get_qos_ctl(hdr);
+			u8 tid = qc[0] & IEEE80211_QOS_CTL_TID_MASK;
 
 			ampdu = (u32)sta->ht_cap.ampdu_density;
 			val32 = ampdu << TXDESC_AMPDU_DENSITY_SHIFT;
 			tx_desc->txdw2 |= cpu_to_le32(val32);
 
 			ampdu_enable = true;
+
+			if (!test_bit(tid, priv->tx_aggr_started) &&
+			    !(skb->protocol == cpu_to_be16(ETH_P_PAE)))
+				if (!ieee80211_start_tx_ba_session(sta, tid, 0))
+					set_bit(tid, priv->tx_aggr_started);
 		}
 	}
 
@@ -6096,6 +6107,7 @@ rtl8xxxu_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 	struct device *dev = &priv->udev->dev;
 	u8 ampdu_factor, ampdu_density;
 	struct ieee80211_sta *sta = params->sta;
+	u16 tid = params->tid;
 	enum ieee80211_ampdu_mlme_action action = params->action;
 
 	switch (action) {
@@ -6108,17 +6120,20 @@ rtl8xxxu_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 		dev_dbg(dev,
 			"Changed HT: ampdu_factor %02x, ampdu_density %02x\n",
 			ampdu_factor, ampdu_density);
-		break;
+		return IEEE80211_AMPDU_TX_START_IMMEDIATE;
+	case IEEE80211_AMPDU_TX_STOP_CONT:
 	case IEEE80211_AMPDU_TX_STOP_FLUSH:
-		dev_dbg(dev, "%s: IEEE80211_AMPDU_TX_STOP_FLUSH\n", __func__);
-		rtl8xxxu_set_ampdu_factor(priv, 0);
-		rtl8xxxu_set_ampdu_min_space(priv, 0);
-		break;
 	case IEEE80211_AMPDU_TX_STOP_FLUSH_CONT:
-		dev_dbg(dev, "%s: IEEE80211_AMPDU_TX_STOP_FLUSH_CONT\n",
-			 __func__);
+		dev_dbg(dev, "%s: IEEE80211_AMPDU_TX_STOP\n", __func__);
 		rtl8xxxu_set_ampdu_factor(priv, 0);
 		rtl8xxxu_set_ampdu_min_space(priv, 0);
+		clear_bit(tid, priv->tx_aggr_started);
+		clear_bit(tid, priv->tid_tx_operational);
+		ieee80211_stop_tx_ba_cb_irqsafe(vif, sta->addr, tid);
+		break;
+	case IEEE80211_AMPDU_TX_OPERATIONAL:
+		dev_dbg(dev, "%s: IEEE80211_AMPDU_TX_OPERATIONAL\n", __func__);
+		set_bit(tid, priv->tid_tx_operational);
 		break;
 	case IEEE80211_AMPDU_RX_START:
 		dev_dbg(dev, "%s: IEEE80211_AMPDU_RX_START\n", __func__);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] rtl8xxxu: Fix the handling of TX A-MPDU aggregation
  2021-08-04 15:13 [PATCH v2] rtl8xxxu: Fix the handling of TX A-MPDU aggregation chris.chiu
@ 2021-08-06  9:03 ` Kalle Valo
  2021-08-06 10:03 ` Reto Schneider
  2021-08-21 18:17 ` Kalle Valo
  2 siblings, 0 replies; 6+ messages in thread
From: Kalle Valo @ 2021-08-06  9:03 UTC (permalink / raw)
  To: chris.chiu
  Cc: jes.sorensen, davem, kuba, code, linux-wireless, netdev, linux-kernel

chris.chiu@canonical.com writes:

> From: Chris Chiu <chris.chiu@canonical.com>
>
> The TX A-MPDU aggregation is not handled in the driver since the
> ieee80211_start_tx_ba_session has never been started properly.
> Start and stop the TX BA session by tracking the TX aggregation
> status of each TID. Fix the ampdu_action and the tx descriptor
> accordingly with the given TID.
>
> Signed-off-by: Chris Chiu <chris.chiu@canonical.com>

Is this ok to take?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] rtl8xxxu: Fix the handling of TX A-MPDU aggregation
  2021-08-04 15:13 [PATCH v2] rtl8xxxu: Fix the handling of TX A-MPDU aggregation chris.chiu
  2021-08-06  9:03 ` Kalle Valo
@ 2021-08-06 10:03 ` Reto Schneider
  2021-08-06 12:31   ` Kalle Valo
  2021-08-21 18:17 ` Kalle Valo
  2 siblings, 1 reply; 6+ messages in thread
From: Reto Schneider @ 2021-08-06 10:03 UTC (permalink / raw)
  To: chris.chiu
  Cc: code, linux-wireless, netdev, linux-kernel, jes.sorensen, kvalo,
	davem, kuba


Hi Chris,

On 8/4/21 17:13, chris.chiu@canonical.com wrote:
> The TX A-MPDU aggregation is not handled in the driver since the
> ieee80211_start_tx_ba_session has never been started properly.
> Start and stop the TX BA session by tracking the TX aggregation
> status of each TID. Fix the ampdu_action and the tx descriptor
> accordingly with the given TID.

I'd like to test this but I am not sure what to look for (before and 
after applying the patch).

What should I look for when looking at the (sniffed) Wireshark traces?

Kind regards,
Reto

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] rtl8xxxu: Fix the handling of TX A-MPDU aggregation
  2021-08-06 10:03 ` Reto Schneider
@ 2021-08-06 12:31   ` Kalle Valo
  2021-08-09 12:59     ` Chris Chiu
  0 siblings, 1 reply; 6+ messages in thread
From: Kalle Valo @ 2021-08-06 12:31 UTC (permalink / raw)
  To: Reto Schneider
  Cc: chris.chiu, code, linux-wireless, netdev, linux-kernel,
	jes.sorensen, davem, kuba

Reto Schneider <rs@hqv.ch> writes:

> On 8/4/21 17:13, chris.chiu@canonical.com wrote:
>> The TX A-MPDU aggregation is not handled in the driver since the
>> ieee80211_start_tx_ba_session has never been started properly.
>> Start and stop the TX BA session by tracking the TX aggregation
>> status of each TID. Fix the ampdu_action and the tx descriptor
>> accordingly with the given TID.
>
> I'd like to test this but I am not sure what to look for (before and
> after applying the patch).

Thanks, testing feedback is always very much appreciated.

> What should I look for when looking at the (sniffed) Wireshark traces?

From my (maintainer) point of view most important is that there are no
regressions visible to users, for example no data stalls, crashes or
anything like that.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] rtl8xxxu: Fix the handling of TX A-MPDU aggregation
  2021-08-06 12:31   ` Kalle Valo
@ 2021-08-09 12:59     ` Chris Chiu
  0 siblings, 0 replies; 6+ messages in thread
From: Chris Chiu @ 2021-08-09 12:59 UTC (permalink / raw)
  To: Kalle Valo
  Cc: Reto Schneider, code, linux-wireless, netdev, Linux Kernel,
	jes.sorensen, davem, kuba

On Fri, Aug 6, 2021 at 8:32 PM Kalle Valo <kvalo@codeaurora.org> wrote:
>
> Reto Schneider <rs@hqv.ch> writes:
>
> > On 8/4/21 17:13, chris.chiu@canonical.com wrote:
> >> The TX A-MPDU aggregation is not handled in the driver since the
> >> ieee80211_start_tx_ba_session has never been started properly.
> >> Start and stop the TX BA session by tracking the TX aggregation
> >> status of each TID. Fix the ampdu_action and the tx descriptor
> >> accordingly with the given TID.
> >
> > I'd like to test this but I am not sure what to look for (before and
> > after applying the patch).
>
> Thanks, testing feedback is always very much appreciated.
>
> > What should I look for when looking at the (sniffed) Wireshark traces?
>

If you are able to verify the difference by the air capture, please
refer to https://imgur.com/a/jcFQTc8. You should see more than 1
packet aggregated and sent from your wifi adapter's mac address, and
get the block ack response from the Access Point (also shown in the
image). If TX aggregation is not enabled, you will only see 1 tx
packet from your wifi, and get an ack right after from the AP.

Please also help test if there's any possible regression. Thanks so much.

Chris

> From my (maintainer) point of view most important is that there are no
> regressions visible to users, for example no data stalls, crashes or
> anything like that.
>
> --
> https://patchwork.kernel.org/project/linux-wireless/list/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] rtl8xxxu: Fix the handling of TX A-MPDU aggregation
  2021-08-04 15:13 [PATCH v2] rtl8xxxu: Fix the handling of TX A-MPDU aggregation chris.chiu
  2021-08-06  9:03 ` Kalle Valo
  2021-08-06 10:03 ` Reto Schneider
@ 2021-08-21 18:17 ` Kalle Valo
  2 siblings, 0 replies; 6+ messages in thread
From: Kalle Valo @ 2021-08-21 18:17 UTC (permalink / raw)
  To: chris.chiu
  Cc: jes.sorensen, davem, kuba, code, linux-wireless, netdev,
	linux-kernel, Chris Chiu

chris.chiu@canonical.com wrote:

> From: Chris Chiu <chris.chiu@canonical.com>
> 
> The TX A-MPDU aggregation is not handled in the driver since the
> ieee80211_start_tx_ba_session has never been started properly.
> Start and stop the TX BA session by tracking the TX aggregation
> status of each TID. Fix the ampdu_action and the tx descriptor
> accordingly with the given TID.
> 
> Signed-off-by: Chris Chiu <chris.chiu@canonical.com>

Patch applied to wireless-drivers-next.git, thanks.

95a581ab3592 rtl8xxxu: Fix the handling of TX A-MPDU aggregation

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20210804151325.86600-1-chris.chiu@canonical.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-08-21 18:17 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-04 15:13 [PATCH v2] rtl8xxxu: Fix the handling of TX A-MPDU aggregation chris.chiu
2021-08-06  9:03 ` Kalle Valo
2021-08-06 10:03 ` Reto Schneider
2021-08-06 12:31   ` Kalle Valo
2021-08-09 12:59     ` Chris Chiu
2021-08-21 18:17 ` Kalle Valo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).