| Message ID | 20220516032519.29831-2-ryazanov.s.a@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2587d5198aa5adcbd8896aae4a2404dc13d48637 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ath10k: add encapsulation offloading support | expand |
On Mon, May 16, 2022 at 6:25 AM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote: > We use ieee80211_tx_status() to report each completed tx frame. > Internally, this function calls sta_info_get_by_addrs(), what has a > couple of drawbacks: > 1. additional station lookup causes a performance degradation; > 2. mac80211 can not properly account Ethernet encapsulated frames due > to the inability to properly determine the destination (station) MAC > address since ieee80211_tx_status() assumes the frame has a 802.11 > header. > > The latter is especially destructive if we want to use hardware frames > encapsulation. > > To fix both of these issues, replace ieee80211_tx_status() with > ieee80211_tx_status_ext() call and feed it station pointer from the tx > queue associated with the transmitted frame. > > Tested-on: QCA9888 hw 2.0 10.4-3.9.0.2-00131 > Tested-on: QCA6174 hw 3.2 PCI WLAN.RM.4.4.1-00157-QCARMSWPZ-1 > Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com> > Tested-by: Oldřich Jedlička <oldium.pro@gmail.com> # TP-Link Archer C7 v4 & v5 (QCA9563 + QCA9880) > Tested-by: Edward Matijevic <motolav@gmail.com> # TP-Link Archer C2600 (IPQ8064 + QCA9980 10.4.1.00030-1) > Tested-by: Edward Matijevic <motolav@gmail.com> # QCA9377 PCI in Sta mode > Tested-by: Zhijun You <hujy652@gmail.com> # NETGEAR R7800 (QCA9984 10.4-3.9.0.2-00159) > --- > > Changes since RFC: > * new Tested-on and Tested-by tags > > drivers/net/wireless/ath/ath10k/txrx.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c > index 10123974c3da..72540434c75b 100644 > --- a/drivers/net/wireless/ath/ath10k/txrx.c > +++ b/drivers/net/wireless/ath/ath10k/txrx.c > @@ -43,6 +43,7 @@ static void ath10k_report_offchan_tx(struct ath10k *ar, struct sk_buff *skb) > int ath10k_txrx_tx_unref(struct ath10k_htt *htt, > const struct htt_tx_done *tx_done) > { > + struct ieee80211_tx_status status; > struct ath10k *ar = htt->ar; > struct device *dev = ar->dev; > struct ieee80211_tx_info *info; > @@ -128,7 +129,16 @@ int ath10k_txrx_tx_unref(struct ath10k_htt *htt, > info->status.flags |= IEEE80211_TX_STATUS_ACK_SIGNAL_VALID; > } > > - ieee80211_tx_status(htt->ar->hw, msdu); > + memset(&status, 0, sizeof(status)); > + status.skb = msdu; > + status.info = info; > + > + rcu_read_lock(); > + if (txq && txq->sta) > + status.sta = txq->sta; Just noticed that since we do not dereference the txq->sta pointer here, the above code can be simplified to: if (txq) status.sta = txq->sta; Kalle, should I send V2 or can you change this in your tree? > + ieee80211_tx_status_ext(htt->ar->hw, &status); > + rcu_read_unlock(); > + > /* we do not own the msdu anymore */ > > return 0;
Sergey Ryazanov <ryazanov.s.a@gmail.com> writes: > On Mon, May 16, 2022 at 6:25 AM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote: > >> --- a/drivers/net/wireless/ath/ath10k/txrx.c >> +++ b/drivers/net/wireless/ath/ath10k/txrx.c >> @@ -43,6 +43,7 @@ static void ath10k_report_offchan_tx(struct ath10k *ar, struct sk_buff *skb) >> int ath10k_txrx_tx_unref(struct ath10k_htt *htt, >> const struct htt_tx_done *tx_done) >> { >> + struct ieee80211_tx_status status; >> struct ath10k *ar = htt->ar; >> struct device *dev = ar->dev; >> struct ieee80211_tx_info *info; >> @@ -128,7 +129,16 @@ int ath10k_txrx_tx_unref(struct ath10k_htt *htt, >> info->status.flags |= IEEE80211_TX_STATUS_ACK_SIGNAL_VALID; >> } >> >> - ieee80211_tx_status(htt->ar->hw, msdu); >> + memset(&status, 0, sizeof(status)); >> + status.skb = msdu; >> + status.info = info; >> + >> + rcu_read_lock(); >> + if (txq && txq->sta) >> + status.sta = txq->sta; > > Just noticed that since we do not dereference the txq->sta pointer > here, the above code can be simplified to: > > if (txq) > status.sta = txq->sta; > > Kalle, should I send V2 or can you change this in your tree? I changed this in the pending branch, please check my changes: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=1bd0c16e10229683fab1dd8adf8c4339992688b7
On Wed, May 18, 2022 at 10:30 AM Kalle Valo <kvalo@kernel.org> wrote: > Sergey Ryazanov <ryazanov.s.a@gmail.com> writes: >> On Mon, May 16, 2022 at 6:25 AM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote: >>> --- a/drivers/net/wireless/ath/ath10k/txrx.c >>> +++ b/drivers/net/wireless/ath/ath10k/txrx.c >>> @@ -43,6 +43,7 @@ static void ath10k_report_offchan_tx(struct ath10k *ar, struct sk_buff *skb) >>> int ath10k_txrx_tx_unref(struct ath10k_htt *htt, >>> const struct htt_tx_done *tx_done) >>> { >>> + struct ieee80211_tx_status status; >>> struct ath10k *ar = htt->ar; >>> struct device *dev = ar->dev; >>> struct ieee80211_tx_info *info; >>> @@ -128,7 +129,16 @@ int ath10k_txrx_tx_unref(struct ath10k_htt *htt, >>> info->status.flags |= IEEE80211_TX_STATUS_ACK_SIGNAL_VALID; >>> } >>> >>> - ieee80211_tx_status(htt->ar->hw, msdu); >>> + memset(&status, 0, sizeof(status)); >>> + status.skb = msdu; >>> + status.info = info; >>> + >>> + rcu_read_lock(); >>> + if (txq && txq->sta) >>> + status.sta = txq->sta; >> >> Just noticed that since we do not dereference the txq->sta pointer >> here, the above code can be simplified to: >> >> if (txq) >> status.sta = txq->sta; >> >> Kalle, should I send V2 or can you change this in your tree? > > I changed this in the pending branch, please check my changes: > > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=1bd0c16e10229683fab1dd8adf8c4339992688b7 Exactly what I meant. Thank you! -- Sergey
Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote: > We use ieee80211_tx_status() to report each completed tx frame. > Internally, this function calls sta_info_get_by_addrs(), what has a > couple of drawbacks: > 1. additional station lookup causes a performance degradation; > 2. mac80211 can not properly account Ethernet encapsulated frames due > to the inability to properly determine the destination (station) MAC > address since ieee80211_tx_status() assumes the frame has a 802.11 > header. > > The latter is especially destructive if we want to use hardware frames > encapsulation. > > To fix both of these issues, replace ieee80211_tx_status() with > ieee80211_tx_status_ext() call and feed it station pointer from the tx > queue associated with the transmitted frame. > > Tested-on: QCA9888 hw2.0 PCI 10.4-3.9.0.2-00131 > Tested-on: QCA6174 hw3.2 PCI WLAN.RM.4.4.1-00157-QCARMSWPZ-1 > > Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com> > Tested-by: Oldřich Jedlička <oldium.pro@gmail.com> # TP-Link Archer C7 v4 & v5 (QCA9563 + QCA9880) > Tested-by: Edward Matijevic <motolav@gmail.com> # TP-Link Archer C2600 (IPQ8064 + QCA9980 10.4.1.00030-1) > Tested-by: Edward Matijevic <motolav@gmail.com> # QCA9377 PCI in Sta mode > Tested-by: Zhijun You <hujy652@gmail.com> # NETGEAR R7800 (QCA9984 10.4-3.9.0.2-00159) > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> 4 patches applied to ath-next branch of ath.git, thanks. 2587d5198aa5 ath10k: improve tx status reporting 70f119fb82af ath10k: htt_tx: do not interpret Eth frames as WiFi a09740548275 ath10k: turn rawmode into frame_mode af6d8265c47e ath10k: add encapsulation offloading support
diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c index 10123974c3da..72540434c75b 100644 --- a/drivers/net/wireless/ath/ath10k/txrx.c +++ b/drivers/net/wireless/ath/ath10k/txrx.c @@ -43,6 +43,7 @@ static void ath10k_report_offchan_tx(struct ath10k *ar, struct sk_buff *skb) int ath10k_txrx_tx_unref(struct ath10k_htt *htt, const struct htt_tx_done *tx_done) { + struct ieee80211_tx_status status; struct ath10k *ar = htt->ar; struct device *dev = ar->dev; struct ieee80211_tx_info *info; @@ -128,7 +129,16 @@ int ath10k_txrx_tx_unref(struct ath10k_htt *htt, info->status.flags |= IEEE80211_TX_STATUS_ACK_SIGNAL_VALID; } - ieee80211_tx_status(htt->ar->hw, msdu); + memset(&status, 0, sizeof(status)); + status.skb = msdu; + status.info = info; + + rcu_read_lock(); + if (txq && txq->sta) + status.sta = txq->sta; + ieee80211_tx_status_ext(htt->ar->hw, &status); + rcu_read_unlock(); + /* we do not own the msdu anymore */ return 0;