* [PATCH v2 0/2] Make ampdu tx work correctly @ 2021-06-03 12:06 chris.chiu 2021-06-03 12:06 ` [PATCH v2 1/2] rtl8xxxu: unset the hw capability HAS_RATE_CONTROL chris.chiu 2021-06-03 12:06 ` [PATCH v2 2/2] rtl8xxxu: Fix ampdu_action to get block ack session work chris.chiu 0 siblings, 2 replies; 6+ messages in thread From: chris.chiu @ 2021-06-03 12:06 UTC (permalink / raw) To: Jes.Sorensen, kvalo, davem, kuba Cc: linux-wireless, netdev, linux-kernel, Chris Chiu From: Chris Chiu <chris.chiu@canonical.com> The rtl8xxxu is the driver based on mac80211 framework, but the ampdu tx is never working. Fix the ampdu_action and the hw capability to enable the ampdu tx. Chris Chiu (2): rtl8xxxu: unset the hw capability HAS_RATE_CONTROL rtl8xxxu: Fix ampdu_action to get block ack session work .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h | 1 + .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 28 ++++++++++++------- 2 files changed, 19 insertions(+), 10 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] rtl8xxxu: unset the hw capability HAS_RATE_CONTROL 2021-06-03 12:06 [PATCH v2 0/2] Make ampdu tx work correctly chris.chiu @ 2021-06-03 12:06 ` chris.chiu 2021-06-10 20:18 ` Johannes Berg 2021-06-03 12:06 ` [PATCH v2 2/2] rtl8xxxu: Fix ampdu_action to get block ack session work chris.chiu 1 sibling, 1 reply; 6+ messages in thread From: chris.chiu @ 2021-06-03 12:06 UTC (permalink / raw) To: Jes.Sorensen, kvalo, davem, kuba Cc: linux-wireless, netdev, linux-kernel, Chris Chiu From: Chris Chiu <chris.chiu@canonical.com> Since AMPDU_AGGREGATION is set so packets will be handed to the driver with a flag indicating A-MPDU aggregation and device should be responsible for setting up and starting the TX aggregation with the AMPDU_TX_START action. The TX aggregation is usually started by the rate control algorithm so the HAS_RATE_CONTROL has to be unset for the mac80211 to start BA session by ieee80211_start_tx_ba_session. The realtek chips tx rate will still be handled by the rate adaptive mechanism in the underlying firmware which is controlled by the rate mask H2C command in the driver. Unset HAS_RATE_CONTROL cause no change for the tx rate control and the TX BA session can be started by the mac80211 default rate control mechanism. Signed-off-by: Chris Chiu <chris.chiu@canonical.com> --- Changelog: v2: - Revise the commit message to desribe the purpose of the change in detail. drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c index 9ff09cf7eb62..4cf13d2f86b1 100644 --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c @@ -6678,7 +6678,6 @@ static int rtl8xxxu_probe(struct usb_interface *interface, /* * The firmware handles rate control */ - ieee80211_hw_set(hw, HAS_RATE_CONTROL); ieee80211_hw_set(hw, AMPDU_AGGREGATION); wiphy_ext_feature_set(hw->wiphy, NL80211_EXT_FEATURE_CQM_RSSI_LIST); -- 2.20.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] rtl8xxxu: unset the hw capability HAS_RATE_CONTROL 2021-06-03 12:06 ` [PATCH v2 1/2] rtl8xxxu: unset the hw capability HAS_RATE_CONTROL chris.chiu @ 2021-06-10 20:18 ` Johannes Berg 2021-06-11 14:47 ` Chris Chiu 0 siblings, 1 reply; 6+ messages in thread From: Johannes Berg @ 2021-06-10 20:18 UTC (permalink / raw) To: chris.chiu, Jes.Sorensen, kvalo, davem, kuba Cc: linux-wireless, netdev, linux-kernel Hi Chris, > Since AMPDU_AGGREGATION is set so packets will be handed to the > driver with a flag indicating A-MPDU aggregation and device should > be responsible for setting up and starting the TX aggregation with > the AMPDU_TX_START action. The TX aggregation is usually started by > the rate control algorithm so the HAS_RATE_CONTROL has to be unset > for the mac80211 to start BA session by ieee80211_start_tx_ba_session. > > The realtek chips tx rate will still be handled by the rate adaptive > mechanism in the underlying firmware which is controlled by the > rate mask H2C command in the driver. Unset HAS_RATE_CONTROL cause > no change for the tx rate control and the TX BA session can be started > by the mac80211 default rate control mechanism. This seems ... strange, to say the least? You want to run the full minstrel algorithm just to have it start aggregation sessions at the beginning? I really don't think this makes sense, and it's super confusing. It may also result in things like reporting a TX rate to userspace/other components that *minstrel* thinks is the best rate, rather than your driver's implementation, etc. I suggest you instead just call ieee80211_start_tx_ba_session() at some appropriate time, maybe copying parts of the logic of minstrel_aggr_check(). johannes ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] rtl8xxxu: unset the hw capability HAS_RATE_CONTROL 2021-06-10 20:18 ` Johannes Berg @ 2021-06-11 14:47 ` Chris Chiu 2021-08-13 8:26 ` Johannes Berg 0 siblings, 1 reply; 6+ messages in thread From: Chris Chiu @ 2021-06-11 14:47 UTC (permalink / raw) To: Johannes Berg Cc: Jes.Sorensen, kvalo, davem, kuba, linux-wireless, netdev, Linux Kernel, code On Fri, Jun 11, 2021 at 4:18 AM Johannes Berg <johannes@sipsolutions.net> wrote: > > Hi Chris, > > > Since AMPDU_AGGREGATION is set so packets will be handed to the > > driver with a flag indicating A-MPDU aggregation and device should > > be responsible for setting up and starting the TX aggregation with > > the AMPDU_TX_START action. The TX aggregation is usually started by > > the rate control algorithm so the HAS_RATE_CONTROL has to be unset > > for the mac80211 to start BA session by ieee80211_start_tx_ba_session. > > > > The realtek chips tx rate will still be handled by the rate adaptive > > mechanism in the underlying firmware which is controlled by the > > rate mask H2C command in the driver. Unset HAS_RATE_CONTROL cause > > no change for the tx rate control and the TX BA session can be started > > by the mac80211 default rate control mechanism. > > This seems ... strange, to say the least? You want to run the full > minstrel algorithm just to have it start aggregation sessions at the > beginning? > > I really don't think this makes sense, and it's super confusing. It may > also result in things like reporting a TX rate to userspace/other > components that *minstrel* thinks is the best rate, rather than your > driver's implementation, etc. > > I suggest you instead just call ieee80211_start_tx_ba_session() at some > appropriate time, maybe copying parts of the logic of > minstrel_aggr_check(). > > johannes > > Based on the description in https://github.com/torvalds/linux/blob/master/net/mac80211/agg-tx.c#L32 to L36, if we set HAS_RATE_CONTROL, which means we don't want the software rate control (default minstrel), then we will have to deal with both the rate control and the TX aggregation in the driver, and the .ampdu_action is not really required. Since the rtl8xxxu driver doesn't handle the TX aggregation, and the minstrel is the default rate control (can't even be disabled), that's the reason why I want to unset the HAS_RATE_CONTROL to make use of the existing mac80211 aggregation handling. And the minstrel doesn't really take effect for rate selection in HT mode because most drivers don't provide HT/VHT rates in .bitrates of the ieee80211_supported_band data structure which is required for hw->wiphy->bands. The mac80211 API ieee80211_get_tx_rate() will return 0 when the IEEE80211_TX_RC_MCS is set in rate flags. The tx rate which is filled in the tx descriptor makes no difference because the underlying rate selection will be actually controlled by the controller which we can set rate mask via H2C command. Unless we force the fixed rate in the TX descriptor, we don't really have to fill the tx rate. Reporting TX rate of each packet will not depend on the rate from the minstrel, drivers have to handle it by itself. I'll also try to address that in my next PATCH series. Chris ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] rtl8xxxu: unset the hw capability HAS_RATE_CONTROL 2021-06-11 14:47 ` Chris Chiu @ 2021-08-13 8:26 ` Johannes Berg 0 siblings, 0 replies; 6+ messages in thread From: Johannes Berg @ 2021-08-13 8:26 UTC (permalink / raw) To: Chris Chiu Cc: Jes.Sorensen, kvalo, davem, kuba, linux-wireless, netdev, Linux Kernel, code On Fri, 2021-06-11 at 22:47 +0800, Chris Chiu wrote: > > Based on the description in > https://github.com/torvalds/linux/blob/master/net/mac80211/agg-tx.c#L32 > to L36, if we set HAS_RATE_CONTROL, which means we don't want the > software rate control (default minstrel), then we will have to deal > with both the rate control and the TX aggregation in the driver, and > the .ampdu_action is not really required. > I don't think this is true. You'll probably still want to use the A-MPDU state machine in mac80211, etc. What you *don't* get without rate control in mac80211 is any decision on whether or not to enable A-MPDU, but that's something you can easily do elsewhere and just call ieee80211_start_tx_ba_session() at an appropriate point in time. johannes ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] rtl8xxxu: Fix ampdu_action to get block ack session work 2021-06-03 12:06 [PATCH v2 0/2] Make ampdu tx work correctly chris.chiu 2021-06-03 12:06 ` [PATCH v2 1/2] rtl8xxxu: unset the hw capability HAS_RATE_CONTROL chris.chiu @ 2021-06-03 12:06 ` chris.chiu 1 sibling, 0 replies; 6+ messages in thread From: chris.chiu @ 2021-06-03 12:06 UTC (permalink / raw) To: Jes.Sorensen, kvalo, davem, kuba Cc: linux-wireless, netdev, linux-kernel, Chris Chiu From: Chris Chiu <chris.chiu@canonical.com> The TID is not handled in the ampdu actions. Fix the ampdu_action to handle the ampdu operations according to the TID. The ampdu stop also needs to be handled by ieee80211_stop_tx_ba_cb_irqsafe for the mac80211 to respond accordingly. Signed-off-by: Chris Chiu <chris.chiu@canonical.com> --- .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h | 1 + .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 27 ++++++++++++------- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h index d1a566cc0c9e..ebd69c161899 100644 --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h @@ -1383,6 +1383,7 @@ struct rtl8xxxu_priv { u8 no_pape:1; u8 int_buf[USB_INTR_CONTENT_LENGTH]; u8 rssi_level; + u8 tid_bitmap; /* * 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 4cf13d2f86b1..790be4ecc3d0 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,8 @@ 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 && (priv->tid_bitmap & BIT(tid)) && + (tx_info->flags & IEEE80211_TX_CTL_AMPDU)) tx_desc->txdw1 |= cpu_to_le32(TXDESC32_AGG_ENABLE); else tx_desc->txdw1 |= cpu_to_le32(TXDESC32_AGG_BREAK); @@ -4876,6 +4879,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 +4907,8 @@ 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 && (priv->tid_bitmap & BIT(tid)) && + (tx_info->flags & IEEE80211_TX_CTL_AMPDU)) tx_desc40->txdw2 |= cpu_to_le32(TXDESC40_AGG_ENABLE); else tx_desc40->txdw2 |= cpu_to_le32(TXDESC40_AGG_BREAK); @@ -6089,6 +6095,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) { @@ -6101,17 +6108,19 @@ 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); + priv->tid_bitmap &= ~BIT(tid); + 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__); + priv->tid_bitmap |= BIT(tid); break; case IEEE80211_AMPDU_RX_START: dev_dbg(dev, "%s: IEEE80211_AMPDU_RX_START\n", __func__); -- 2.20.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-08-13 8:26 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-03 12:06 [PATCH v2 0/2] Make ampdu tx work correctly chris.chiu 2021-06-03 12:06 ` [PATCH v2 1/2] rtl8xxxu: unset the hw capability HAS_RATE_CONTROL chris.chiu 2021-06-10 20:18 ` Johannes Berg 2021-06-11 14:47 ` Chris Chiu 2021-08-13 8:26 ` Johannes Berg 2021-06-03 12:06 ` [PATCH v2 2/2] rtl8xxxu: Fix ampdu_action to get block ack session work chris.chiu
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).