* [PATCH v3] ath10k: fix Rx aggregation reordering
@ 2014-07-16 11:19 ` Michal Kazior
0 siblings, 0 replies; 19+ messages in thread
From: Michal Kazior @ 2014-07-16 11:19 UTC (permalink / raw)
To: ath10k; +Cc: linux-wireless, Denton Gentry, Michal Kazior
Firmware doesn't perform Rx reordering so it is
left to the host driver to do that.
Use mac80211 to perform reordering instead of
re-inventing the wheel.
This fixes TCP throughput issues in some
environments.
Reported-by: Denton Gentry <denton.gentry@gmail.com>
Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
This depends on:
* mac80211: fix Rx reordering with RX_FLAG_AMSDU_MORE
* mac80211: add support for Rx reordering offloading
I'm still seeing some issues with TCP traffic.
Apparently firmware/hardware sometimes stalls Rx
for ~100ms and misses some aggregate frames. I
wasn't able to pinpoint what the problem is. The
more parallel TCP streams (iperf) the more often
stalls happen. UDP doesn't seem to trigger the Rx
stalls at all.
Notes:
v3:
* make ath10k_ampdu_action() static
v2:
* split addba/delba handling into separate functions [Kalle]
* extend ampdu_action debug print (+vdev_id, +sta addr, +tid)
drivers/net/wireless/ath/ath10k/htt_rx.c | 94 +++++++++++++++++++++++++++++++-
drivers/net/wireless/ath/ath10k/mac.c | 33 +++++++++++
drivers/net/wireless/ath/ath10k/txrx.c | 3 +-
drivers/net/wireless/ath/ath10k/txrx.h | 1 +
4 files changed, 127 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index eebc860..966bd4a 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -21,6 +21,7 @@
#include "txrx.h"
#include "debug.h"
#include "trace.h"
+#include "mac.h"
#include <linux/log2.h>
@@ -1422,6 +1423,86 @@ static void ath10k_htt_rx_frm_tx_compl(struct ath10k *ar,
}
}
+static void ath10k_htt_rx_addba(struct ath10k *ar, struct htt_resp *resp)
+{
+ struct htt_rx_addba *ev = &resp->rx_addba;
+ struct ath10k_peer *peer;
+ struct ath10k_vif *arvif;
+ u16 info0, tid, peer_id;
+
+ info0 = __le32_to_cpu(ev->info0);
+ tid = MS(info0, HTT_RX_BA_INFO0_TID);
+ peer_id = MS(info0, HTT_RX_BA_INFO0_PEER_ID);
+
+ ath10k_dbg(ATH10K_DBG_HTT,
+ "htt rx addba tid %hu peer_id %hu size %hhu\n",
+ tid, peer_id, ev->window_size);
+
+ spin_lock_bh(&ar->data_lock);
+ peer = ath10k_peer_find_by_id(ar, peer_id);
+ if (!peer) {
+ ath10k_warn("received addba event for invalid peer_id: %hu\n",
+ peer_id);
+ spin_unlock_bh(&ar->data_lock);
+ return;
+ }
+
+ arvif = ath10k_get_arvif(ar, peer->vdev_id);
+ if (!arvif) {
+ ath10k_warn("received addba event for invalid vdev_id: %u\n",
+ peer->vdev_id);
+ spin_unlock_bh(&ar->data_lock);
+ return;
+ }
+
+ ath10k_dbg(ATH10K_DBG_HTT,
+ "htt rx start rx ba session sta %pM tid %hu size %hhu\n",
+ peer->addr, tid, ev->window_size);
+
+ ieee80211_start_rx_ba_session_offl(arvif->vif, peer->addr, tid);
+ spin_unlock_bh(&ar->data_lock);
+}
+
+static void ath10k_htt_rx_delba(struct ath10k *ar, struct htt_resp *resp)
+{
+ struct htt_rx_addba *ev = &resp->rx_addba;
+ struct ath10k_peer *peer;
+ struct ath10k_vif *arvif;
+ u16 info0, tid, peer_id;
+
+ info0 = __le32_to_cpu(ev->info0);
+ tid = MS(info0, HTT_RX_BA_INFO0_TID);
+ peer_id = MS(info0, HTT_RX_BA_INFO0_PEER_ID);
+
+ ath10k_dbg(ATH10K_DBG_HTT,
+ "htt rx delba tid %hu peer_id %hu\n",
+ tid, peer_id);
+
+ spin_lock_bh(&ar->data_lock);
+ peer = ath10k_peer_find_by_id(ar, peer_id);
+ if (!peer) {
+ ath10k_warn("received addba event for invalid peer_id: %hu\n",
+ peer_id);
+ spin_unlock_bh(&ar->data_lock);
+ return;
+ }
+
+ arvif = ath10k_get_arvif(ar, peer->vdev_id);
+ if (!arvif) {
+ ath10k_warn("received addba event for invalid vdev_id: %u\n",
+ peer->vdev_id);
+ spin_unlock_bh(&ar->data_lock);
+ return;
+ }
+
+ ath10k_dbg(ATH10K_DBG_HTT,
+ "htt rx stop rx ba session sta %pM tid %hu\n",
+ peer->addr, tid);
+
+ ieee80211_stop_rx_ba_session_offl(arvif->vif, peer->addr, tid);
+ spin_unlock_bh(&ar->data_lock);
+}
+
void ath10k_htt_t2h_msg_handler(struct ath10k *ar, struct sk_buff *skb)
{
struct ath10k_htt *htt = &ar->htt;
@@ -1515,10 +1596,19 @@ void ath10k_htt_t2h_msg_handler(struct ath10k *ar, struct sk_buff *skb)
case HTT_T2H_MSG_TYPE_STATS_CONF:
trace_ath10k_htt_stats(skb->data, skb->len);
break;
- case HTT_T2H_MSG_TYPE_TX_INSPECT_IND:
case HTT_T2H_MSG_TYPE_RX_ADDBA:
+ ath10k_htt_rx_addba(ar, resp);
+ break;
case HTT_T2H_MSG_TYPE_RX_DELBA:
- case HTT_T2H_MSG_TYPE_RX_FLUSH:
+ ath10k_htt_rx_delba(ar, resp);
+ break;
+ case HTT_T2H_MSG_TYPE_RX_FLUSH: {
+ /* Ignore this event because mac80211 takes care of Rx
+ * aggregation reordering.
+ */
+ break;
+ }
+ case HTT_T2H_MSG_TYPE_TX_INSPECT_IND:
default:
ath10k_dbg(ATH10K_DBG_HTT, "htt event (%d) not handled\n",
resp->hdr.msg_type);
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index b8314a5..67bf8ed 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -4331,6 +4331,38 @@ static u64 ath10k_get_tsf(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
return 0;
}
+static int ath10k_ampdu_action(struct ieee80211_hw *hw,
+ struct ieee80211_vif *vif,
+ enum ieee80211_ampdu_mlme_action action,
+ struct ieee80211_sta *sta, u16 tid, u16 *ssn,
+ u8 buf_size)
+{
+ struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+
+ ath10k_dbg(ATH10K_DBG_MAC, "mac ampdu vdev_id %i sta %pM tid %hu action %d\n",
+ arvif->vdev_id, sta->addr, tid, action);
+
+ switch (action) {
+ case IEEE80211_AMPDU_RX_START:
+ case IEEE80211_AMPDU_RX_STOP:
+ /* HTT AddBa/DelBa events trigger mac80211 Rx BA session
+ * creation/removal. Do we need to verify this?
+ */
+ return 0;
+ case IEEE80211_AMPDU_TX_START:
+ case IEEE80211_AMPDU_TX_STOP_CONT:
+ case IEEE80211_AMPDU_TX_STOP_FLUSH:
+ case IEEE80211_AMPDU_TX_STOP_FLUSH_CONT:
+ case IEEE80211_AMPDU_TX_OPERATIONAL:
+ /* Firmware offloads Tx aggregation entirely so deny mac80211
+ * Tx aggregation requests.
+ */
+ break;
+ }
+
+ return -EOPNOTSUPP;
+}
+
static const struct ieee80211_ops ath10k_ops = {
.tx = ath10k_tx,
.start = ath10k_start,
@@ -4358,6 +4390,7 @@ static const struct ieee80211_ops ath10k_ops = {
.set_bitrate_mask = ath10k_set_bitrate_mask,
.sta_rc_update = ath10k_sta_rc_update,
.get_tsf = ath10k_get_tsf,
+ .ampdu_action = ath10k_ampdu_action,
#ifdef CONFIG_PM
.suspend = ath10k_suspend,
.resume = ath10k_resume,
diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c
index 82669a7..f4fa22d 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -119,8 +119,7 @@ struct ath10k_peer *ath10k_peer_find(struct ath10k *ar, int vdev_id,
return NULL;
}
-static struct ath10k_peer *ath10k_peer_find_by_id(struct ath10k *ar,
- int peer_id)
+struct ath10k_peer *ath10k_peer_find_by_id(struct ath10k *ar, int peer_id)
{
struct ath10k_peer *peer;
diff --git a/drivers/net/wireless/ath/ath10k/txrx.h b/drivers/net/wireless/ath/ath10k/txrx.h
index aee3e20..a90e09f 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.h
+++ b/drivers/net/wireless/ath/ath10k/txrx.h
@@ -24,6 +24,7 @@ void ath10k_txrx_tx_unref(struct ath10k_htt *htt,
struct ath10k_peer *ath10k_peer_find(struct ath10k *ar, int vdev_id,
const u8 *addr);
+struct ath10k_peer *ath10k_peer_find_by_id(struct ath10k *ar, int peer_id);
int ath10k_wait_for_peer_created(struct ath10k *ar, int vdev_id,
const u8 *addr);
int ath10k_wait_for_peer_deleted(struct ath10k *ar, int vdev_id,
--
1.8.5.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3] ath10k: fix Rx aggregation reordering
2014-07-16 11:19 ` Michal Kazior
@ 2014-07-16 11:38 ` Varka Bhadram
-1 siblings, 0 replies; 19+ messages in thread
From: Varka Bhadram @ 2014-07-16 11:38 UTC (permalink / raw)
To: Michal Kazior, ath10k; +Cc: linux-wireless, Denton Gentry
On 07/16/2014 04:49 PM, Michal Kazior wrote:
(...)
> +static void ath10k_htt_rx_addba(struct ath10k *ar, struct htt_resp *resp)
> +{
> + struct htt_rx_addba *ev = &resp->rx_addba;
> + struct ath10k_peer *peer;
> + struct ath10k_vif *arvif;
> + u16 info0, tid, peer_id;
> +
> + info0 = __le32_to_cpu(ev->info0);
> + tid = MS(info0, HTT_RX_BA_INFO0_TID);
> + peer_id = MS(info0, HTT_RX_BA_INFO0_PEER_ID);
> +
> + ath10k_dbg(ATH10K_DBG_HTT,
> + "htt rx addba tid %hu peer_id %hu size %hhu\n",
> + tid, peer_id, ev->window_size);
> +
> + spin_lock_bh(&ar->data_lock);
> + peer = ath10k_peer_find_by_id(ar, peer_id);
> + if (!peer) {
> + ath10k_warn("received addba event for invalid peer_id: %hu\n",
> + peer_id);
> + spin_unlock_bh(&ar->data_lock);
> + return;
> + }
Here my concern in amount of time holding the lock...
spin_lock_bh(&ar->data_lock);
peer = ath10k_peer_find_by_id(ar, peer_id);
if (!peer) {
spin_unlock_bh(&ar->data_lock);
ath10k_warn("received addba event for invalid peer_id: %hu\n",
peer_id);
return;
}
No need to of putting the debug message inside the critical region... :-)
> +
> + arvif = ath10k_get_arvif(ar, peer->vdev_id);
> + if (!arvif) {
> + ath10k_warn("received addba event for invalid vdev_id: %u\n",
> + peer->vdev_id);
> + spin_unlock_bh(&ar->data_lock);
Ditto
> + return;
> + }
> +
> + ath10k_dbg(ATH10K_DBG_HTT,
> + "htt rx start rx ba session sta %pM tid %hu size %hhu\n",
> + peer->addr, tid, ev->window_size);
> +
> + ieee80211_start_rx_ba_session_offl(arvif->vif, peer->addr, tid);
> + spin_unlock_bh(&ar->data_lock);
> +}
> +
> +static void ath10k_htt_rx_delba(struct ath10k *ar, struct htt_resp *resp)
> +{
> + struct htt_rx_addba *ev = &resp->rx_addba;
> + struct ath10k_peer *peer;
> + struct ath10k_vif *arvif;
> + u16 info0, tid, peer_id;
> +
> + info0 = __le32_to_cpu(ev->info0);
> + tid = MS(info0, HTT_RX_BA_INFO0_TID);
> + peer_id = MS(info0, HTT_RX_BA_INFO0_PEER_ID);
> +
> + ath10k_dbg(ATH10K_DBG_HTT,
> + "htt rx delba tid %hu peer_id %hu\n",
> + tid, peer_id);
> +
> + spin_lock_bh(&ar->data_lock);
> + peer = ath10k_peer_find_by_id(ar, peer_id);
> + if (!peer) {
> + ath10k_warn("received addba event for invalid peer_id: %hu\n",
> + peer_id);
> + spin_unlock_bh(&ar->data_lock);
> + return;
Ditto..
> + }
> +
> + arvif = ath10k_get_arvif(ar, peer->vdev_id);
> + if (!arvif) {
> + ath10k_warn("received addba event for invalid vdev_id: %u\n",
> + peer->vdev_id);
> + spin_unlock_bh(&ar->data_lock);
> + return;
> + }
> +
> + ath10k_dbg(ATH10K_DBG_HTT,
> + "htt rx stop rx ba session sta %pM tid %hu\n",
> + peer->addr, tid);
> +
> + ieee80211_stop_rx_ba_session_offl(arvif->vif, peer->addr, tid);
> + spin_unlock_bh(&ar->data_lock);
> +}
> +
> void ath10k_htt_t2h_msg_handler(struct ath10k *ar, struct sk_buff *skb)
> {
> struct ath10k_htt *htt = &ar->htt;
> @@ -1515,10 +1596,19 @@ void ath10k_htt_t2h_msg_handler(struct ath10k *ar, struct sk_buff *skb)
> case HTT_T2H_MSG_TYPE_STATS_CONF:
> trace_ath10k_htt_stats(skb->data, skb->len);
> break;
> - case HTT_T2H_MSG_TYPE_TX_INSPECT_IND:
> case HTT_T2H_MSG_TYPE_RX_ADDBA:
> + ath10k_htt_rx_addba(ar, resp);
> + break;
> case HTT_T2H_MSG_TYPE_RX_DELBA:
> - case HTT_T2H_MSG_TYPE_RX_FLUSH:
> + ath10k_htt_rx_delba(ar, resp);
> + break;
> + case HTT_T2H_MSG_TYPE_RX_FLUSH: {
> + /* Ignore this event because mac80211 takes care of Rx
> + * aggregation reordering.
> + */
> + break;
> + }
> + case HTT_T2H_MSG_TYPE_TX_INSPECT_IND:
> default:
> ath10k_dbg(ATH10K_DBG_HTT, "htt event (%d) not handled\n",
> resp->hdr.msg_type);
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index b8314a5..67bf8ed 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -4331,6 +4331,38 @@ static u64 ath10k_get_tsf(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
> return 0;
> }
>
> +static int ath10k_ampdu_action(struct ieee80211_hw *hw,
> + struct ieee80211_vif *vif,
> + enum ieee80211_ampdu_mlme_action action,
> + struct ieee80211_sta *sta, u16 tid, u16 *ssn,
> + u8 buf_size)
> +{
> + struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
> +
> + ath10k_dbg(ATH10K_DBG_MAC, "mac ampdu vdev_id %i sta %pM tid %hu action %d\n",
> + arvif->vdev_id, sta->addr, tid, action);
> +
> + switch (action) {
> + case IEEE80211_AMPDU_RX_START:
> + case IEEE80211_AMPDU_RX_STOP:
> + /* HTT AddBa/DelBa events trigger mac80211 Rx BA session
> + * creation/removal. Do we need to verify this?
> + */
> + return 0;
> + case IEEE80211_AMPDU_TX_START:
> + case IEEE80211_AMPDU_TX_STOP_CONT:
> + case IEEE80211_AMPDU_TX_STOP_FLUSH:
> + case IEEE80211_AMPDU_TX_STOP_FLUSH_CONT:
> + case IEEE80211_AMPDU_TX_OPERATIONAL:
> + /* Firmware offloads Tx aggregation entirely so deny mac80211
> + * Tx aggregation requests.
> + */
> + break;
Instead of break here we can directly do: return -EOPNOTSUPP;
> + }
> +
> + return -EOPNOTSUPP;
> +}
> +
--
Regards,
Varka Bhadram.
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3] ath10k: fix Rx aggregation reordering
@ 2014-07-16 11:38 ` Varka Bhadram
0 siblings, 0 replies; 19+ messages in thread
From: Varka Bhadram @ 2014-07-16 11:38 UTC (permalink / raw)
To: Michal Kazior, ath10k; +Cc: linux-wireless, Denton Gentry
On 07/16/2014 04:49 PM, Michal Kazior wrote:
(...)
> +static void ath10k_htt_rx_addba(struct ath10k *ar, struct htt_resp *resp)
> +{
> + struct htt_rx_addba *ev = &resp->rx_addba;
> + struct ath10k_peer *peer;
> + struct ath10k_vif *arvif;
> + u16 info0, tid, peer_id;
> +
> + info0 = __le32_to_cpu(ev->info0);
> + tid = MS(info0, HTT_RX_BA_INFO0_TID);
> + peer_id = MS(info0, HTT_RX_BA_INFO0_PEER_ID);
> +
> + ath10k_dbg(ATH10K_DBG_HTT,
> + "htt rx addba tid %hu peer_id %hu size %hhu\n",
> + tid, peer_id, ev->window_size);
> +
> + spin_lock_bh(&ar->data_lock);
> + peer = ath10k_peer_find_by_id(ar, peer_id);
> + if (!peer) {
> + ath10k_warn("received addba event for invalid peer_id: %hu\n",
> + peer_id);
> + spin_unlock_bh(&ar->data_lock);
> + return;
> + }
Here my concern in amount of time holding the lock...
spin_lock_bh(&ar->data_lock);
peer = ath10k_peer_find_by_id(ar, peer_id);
if (!peer) {
spin_unlock_bh(&ar->data_lock);
ath10k_warn("received addba event for invalid peer_id: %hu\n",
peer_id);
return;
}
No need to of putting the debug message inside the critical region... :-)
> +
> + arvif = ath10k_get_arvif(ar, peer->vdev_id);
> + if (!arvif) {
> + ath10k_warn("received addba event for invalid vdev_id: %u\n",
> + peer->vdev_id);
> + spin_unlock_bh(&ar->data_lock);
Ditto
> + return;
> + }
> +
> + ath10k_dbg(ATH10K_DBG_HTT,
> + "htt rx start rx ba session sta %pM tid %hu size %hhu\n",
> + peer->addr, tid, ev->window_size);
> +
> + ieee80211_start_rx_ba_session_offl(arvif->vif, peer->addr, tid);
> + spin_unlock_bh(&ar->data_lock);
> +}
> +
> +static void ath10k_htt_rx_delba(struct ath10k *ar, struct htt_resp *resp)
> +{
> + struct htt_rx_addba *ev = &resp->rx_addba;
> + struct ath10k_peer *peer;
> + struct ath10k_vif *arvif;
> + u16 info0, tid, peer_id;
> +
> + info0 = __le32_to_cpu(ev->info0);
> + tid = MS(info0, HTT_RX_BA_INFO0_TID);
> + peer_id = MS(info0, HTT_RX_BA_INFO0_PEER_ID);
> +
> + ath10k_dbg(ATH10K_DBG_HTT,
> + "htt rx delba tid %hu peer_id %hu\n",
> + tid, peer_id);
> +
> + spin_lock_bh(&ar->data_lock);
> + peer = ath10k_peer_find_by_id(ar, peer_id);
> + if (!peer) {
> + ath10k_warn("received addba event for invalid peer_id: %hu\n",
> + peer_id);
> + spin_unlock_bh(&ar->data_lock);
> + return;
Ditto..
> + }
> +
> + arvif = ath10k_get_arvif(ar, peer->vdev_id);
> + if (!arvif) {
> + ath10k_warn("received addba event for invalid vdev_id: %u\n",
> + peer->vdev_id);
> + spin_unlock_bh(&ar->data_lock);
> + return;
> + }
> +
> + ath10k_dbg(ATH10K_DBG_HTT,
> + "htt rx stop rx ba session sta %pM tid %hu\n",
> + peer->addr, tid);
> +
> + ieee80211_stop_rx_ba_session_offl(arvif->vif, peer->addr, tid);
> + spin_unlock_bh(&ar->data_lock);
> +}
> +
> void ath10k_htt_t2h_msg_handler(struct ath10k *ar, struct sk_buff *skb)
> {
> struct ath10k_htt *htt = &ar->htt;
> @@ -1515,10 +1596,19 @@ void ath10k_htt_t2h_msg_handler(struct ath10k *ar, struct sk_buff *skb)
> case HTT_T2H_MSG_TYPE_STATS_CONF:
> trace_ath10k_htt_stats(skb->data, skb->len);
> break;
> - case HTT_T2H_MSG_TYPE_TX_INSPECT_IND:
> case HTT_T2H_MSG_TYPE_RX_ADDBA:
> + ath10k_htt_rx_addba(ar, resp);
> + break;
> case HTT_T2H_MSG_TYPE_RX_DELBA:
> - case HTT_T2H_MSG_TYPE_RX_FLUSH:
> + ath10k_htt_rx_delba(ar, resp);
> + break;
> + case HTT_T2H_MSG_TYPE_RX_FLUSH: {
> + /* Ignore this event because mac80211 takes care of Rx
> + * aggregation reordering.
> + */
> + break;
> + }
> + case HTT_T2H_MSG_TYPE_TX_INSPECT_IND:
> default:
> ath10k_dbg(ATH10K_DBG_HTT, "htt event (%d) not handled\n",
> resp->hdr.msg_type);
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index b8314a5..67bf8ed 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -4331,6 +4331,38 @@ static u64 ath10k_get_tsf(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
> return 0;
> }
>
> +static int ath10k_ampdu_action(struct ieee80211_hw *hw,
> + struct ieee80211_vif *vif,
> + enum ieee80211_ampdu_mlme_action action,
> + struct ieee80211_sta *sta, u16 tid, u16 *ssn,
> + u8 buf_size)
> +{
> + struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
> +
> + ath10k_dbg(ATH10K_DBG_MAC, "mac ampdu vdev_id %i sta %pM tid %hu action %d\n",
> + arvif->vdev_id, sta->addr, tid, action);
> +
> + switch (action) {
> + case IEEE80211_AMPDU_RX_START:
> + case IEEE80211_AMPDU_RX_STOP:
> + /* HTT AddBa/DelBa events trigger mac80211 Rx BA session
> + * creation/removal. Do we need to verify this?
> + */
> + return 0;
> + case IEEE80211_AMPDU_TX_START:
> + case IEEE80211_AMPDU_TX_STOP_CONT:
> + case IEEE80211_AMPDU_TX_STOP_FLUSH:
> + case IEEE80211_AMPDU_TX_STOP_FLUSH_CONT:
> + case IEEE80211_AMPDU_TX_OPERATIONAL:
> + /* Firmware offloads Tx aggregation entirely so deny mac80211
> + * Tx aggregation requests.
> + */
> + break;
Instead of break here we can directly do: return -EOPNOTSUPP;
> + }
> +
> + return -EOPNOTSUPP;
> +}
> +
--
Regards,
Varka Bhadram.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3] ath10k: fix Rx aggregation reordering
2014-07-16 11:38 ` Varka Bhadram
@ 2014-07-16 12:35 ` Michal Kazior
-1 siblings, 0 replies; 19+ messages in thread
From: Michal Kazior @ 2014-07-16 12:35 UTC (permalink / raw)
To: Varka Bhadram; +Cc: linux-wireless, ath10k@lists.infradead.org, Denton Gentry
On 16 July 2014 13:38, Varka Bhadram <varkabhadram@gmail.com> wrote:
> On 07/16/2014 04:49 PM, Michal Kazior wrote:
>
> (...)
>
>
>> +static void ath10k_htt_rx_addba(struct ath10k *ar, struct htt_resp *resp)
>> +{
>> + struct htt_rx_addba *ev = &resp->rx_addba;
>> + struct ath10k_peer *peer;
>> + struct ath10k_vif *arvif;
>> + u16 info0, tid, peer_id;
>> +
>> + info0 = __le32_to_cpu(ev->info0);
>> + tid = MS(info0, HTT_RX_BA_INFO0_TID);
>> + peer_id = MS(info0, HTT_RX_BA_INFO0_PEER_ID);
>> +
>> + ath10k_dbg(ATH10K_DBG_HTT,
>> + "htt rx addba tid %hu peer_id %hu size %hhu\n",
>> + tid, peer_id, ev->window_size);
>> +
>> + spin_lock_bh(&ar->data_lock);
>> + peer = ath10k_peer_find_by_id(ar, peer_id);
>> + if (!peer) {
>> + ath10k_warn("received addba event for invalid peer_id:
>> %hu\n",
>> + peer_id);
>> + spin_unlock_bh(&ar->data_lock);
>> + return;
>> + }
>
>
> Here my concern in amount of time holding the lock...
>
> spin_lock_bh(&ar->data_lock);
> peer = ath10k_peer_find_by_id(ar, peer_id);
> if (!peer) {
> spin_unlock_bh(&ar->data_lock);
>
> ath10k_warn("received addba event for invalid peer_id: %hu\n",
> peer_id);
> return;
> }
>
> No need to of putting the debug message inside the critical region... :-)
Sounds reasonable in this case as I'm not printing spinlock-protected values.
[...]
>> +static int ath10k_ampdu_action(struct ieee80211_hw *hw,
>> + struct ieee80211_vif *vif,
>> + enum ieee80211_ampdu_mlme_action action,
>> + struct ieee80211_sta *sta, u16 tid, u16
>> *ssn,
>> + u8 buf_size)
>> +{
>> + struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
>> +
>> + ath10k_dbg(ATH10K_DBG_MAC, "mac ampdu vdev_id %i sta %pM tid %hu
>> action %d\n",
>> + arvif->vdev_id, sta->addr, tid, action);
>> +
>> + switch (action) {
>> + case IEEE80211_AMPDU_RX_START:
>> + case IEEE80211_AMPDU_RX_STOP:
>> + /* HTT AddBa/DelBa events trigger mac80211 Rx BA session
>> + * creation/removal. Do we need to verify this?
>> + */
>> + return 0;
>> + case IEEE80211_AMPDU_TX_START:
>> + case IEEE80211_AMPDU_TX_STOP_CONT:
>> + case IEEE80211_AMPDU_TX_STOP_FLUSH:
>> + case IEEE80211_AMPDU_TX_STOP_FLUSH_CONT:
>> + case IEEE80211_AMPDU_TX_OPERATIONAL:
>> + /* Firmware offloads Tx aggregation entirely so deny
>> mac80211
>> + * Tx aggregation requests.
>> + */
>> + break;
>
>
> Instead of break here we can directly do: return -EOPNOTSUPP;
True.
Thanks for review. I'll wait a bit more before I re-post v4 (although
there's no rush since the patch needs mac80211 patches to be applied
first).
Michał
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3] ath10k: fix Rx aggregation reordering
@ 2014-07-16 12:35 ` Michal Kazior
0 siblings, 0 replies; 19+ messages in thread
From: Michal Kazior @ 2014-07-16 12:35 UTC (permalink / raw)
To: Varka Bhadram; +Cc: ath10k@lists.infradead.org, linux-wireless, Denton Gentry
On 16 July 2014 13:38, Varka Bhadram <varkabhadram@gmail.com> wrote:
> On 07/16/2014 04:49 PM, Michal Kazior wrote:
>
> (...)
>
>
>> +static void ath10k_htt_rx_addba(struct ath10k *ar, struct htt_resp *resp)
>> +{
>> + struct htt_rx_addba *ev = &resp->rx_addba;
>> + struct ath10k_peer *peer;
>> + struct ath10k_vif *arvif;
>> + u16 info0, tid, peer_id;
>> +
>> + info0 = __le32_to_cpu(ev->info0);
>> + tid = MS(info0, HTT_RX_BA_INFO0_TID);
>> + peer_id = MS(info0, HTT_RX_BA_INFO0_PEER_ID);
>> +
>> + ath10k_dbg(ATH10K_DBG_HTT,
>> + "htt rx addba tid %hu peer_id %hu size %hhu\n",
>> + tid, peer_id, ev->window_size);
>> +
>> + spin_lock_bh(&ar->data_lock);
>> + peer = ath10k_peer_find_by_id(ar, peer_id);
>> + if (!peer) {
>> + ath10k_warn("received addba event for invalid peer_id:
>> %hu\n",
>> + peer_id);
>> + spin_unlock_bh(&ar->data_lock);
>> + return;
>> + }
>
>
> Here my concern in amount of time holding the lock...
>
> spin_lock_bh(&ar->data_lock);
> peer = ath10k_peer_find_by_id(ar, peer_id);
> if (!peer) {
> spin_unlock_bh(&ar->data_lock);
>
> ath10k_warn("received addba event for invalid peer_id: %hu\n",
> peer_id);
> return;
> }
>
> No need to of putting the debug message inside the critical region... :-)
Sounds reasonable in this case as I'm not printing spinlock-protected values.
[...]
>> +static int ath10k_ampdu_action(struct ieee80211_hw *hw,
>> + struct ieee80211_vif *vif,
>> + enum ieee80211_ampdu_mlme_action action,
>> + struct ieee80211_sta *sta, u16 tid, u16
>> *ssn,
>> + u8 buf_size)
>> +{
>> + struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
>> +
>> + ath10k_dbg(ATH10K_DBG_MAC, "mac ampdu vdev_id %i sta %pM tid %hu
>> action %d\n",
>> + arvif->vdev_id, sta->addr, tid, action);
>> +
>> + switch (action) {
>> + case IEEE80211_AMPDU_RX_START:
>> + case IEEE80211_AMPDU_RX_STOP:
>> + /* HTT AddBa/DelBa events trigger mac80211 Rx BA session
>> + * creation/removal. Do we need to verify this?
>> + */
>> + return 0;
>> + case IEEE80211_AMPDU_TX_START:
>> + case IEEE80211_AMPDU_TX_STOP_CONT:
>> + case IEEE80211_AMPDU_TX_STOP_FLUSH:
>> + case IEEE80211_AMPDU_TX_STOP_FLUSH_CONT:
>> + case IEEE80211_AMPDU_TX_OPERATIONAL:
>> + /* Firmware offloads Tx aggregation entirely so deny
>> mac80211
>> + * Tx aggregation requests.
>> + */
>> + break;
>
>
> Instead of break here we can directly do: return -EOPNOTSUPP;
True.
Thanks for review. I'll wait a bit more before I re-post v4 (although
there's no rush since the patch needs mac80211 patches to be applied
first).
Michał
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3] ath10k: fix Rx aggregation reordering
2014-07-16 12:35 ` Michal Kazior
@ 2014-07-16 12:41 ` Michal Kazior
-1 siblings, 0 replies; 19+ messages in thread
From: Michal Kazior @ 2014-07-16 12:41 UTC (permalink / raw)
To: Varka Bhadram; +Cc: linux-wireless, ath10k@lists.infradead.org, Denton Gentry
On 16 July 2014 14:35, Michal Kazior <michal.kazior@tieto.com> wrote:
> On 16 July 2014 13:38, Varka Bhadram <varkabhadram@gmail.com> wrote:
>> On 07/16/2014 04:49 PM, Michal Kazior wrote:
>>
>> (...)
>>
>>
>>> +static void ath10k_htt_rx_addba(struct ath10k *ar, struct htt_resp *resp)
>>> +{
>>> + struct htt_rx_addba *ev = &resp->rx_addba;
>>> + struct ath10k_peer *peer;
>>> + struct ath10k_vif *arvif;
>>> + u16 info0, tid, peer_id;
>>> +
>>> + info0 = __le32_to_cpu(ev->info0);
>>> + tid = MS(info0, HTT_RX_BA_INFO0_TID);
>>> + peer_id = MS(info0, HTT_RX_BA_INFO0_PEER_ID);
>>> +
>>> + ath10k_dbg(ATH10K_DBG_HTT,
>>> + "htt rx addba tid %hu peer_id %hu size %hhu\n",
>>> + tid, peer_id, ev->window_size);
>>> +
>>> + spin_lock_bh(&ar->data_lock);
>>> + peer = ath10k_peer_find_by_id(ar, peer_id);
>>> + if (!peer) {
>>> + ath10k_warn("received addba event for invalid peer_id:
>>> %hu\n",
>>> + peer_id);
>>> + spin_unlock_bh(&ar->data_lock);
>>> + return;
>>> + }
>>
>>
>> Here my concern in amount of time holding the lock...
>>
>> spin_lock_bh(&ar->data_lock);
>> peer = ath10k_peer_find_by_id(ar, peer_id);
>> if (!peer) {
>> spin_unlock_bh(&ar->data_lock);
>>
>> ath10k_warn("received addba event for invalid peer_id: %hu\n",
>> peer_id);
>> return;
>> }
>>
>> No need to of putting the debug message inside the critical region... :-)
>
> Sounds reasonable in this case as I'm not printing spinlock-protected values.
..and I realized this isn't true upon hitting the send button.
The other print uses peer->vdev_id. The peer was acquired under a lock
and must not be used after the lock is released. It'll just look
confusing if I mix ordering of unlock/print in some cases so I'll
leave it as is.
(sorry for the noise)
Michał
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3] ath10k: fix Rx aggregation reordering
@ 2014-07-16 12:41 ` Michal Kazior
0 siblings, 0 replies; 19+ messages in thread
From: Michal Kazior @ 2014-07-16 12:41 UTC (permalink / raw)
To: Varka Bhadram; +Cc: ath10k@lists.infradead.org, linux-wireless, Denton Gentry
On 16 July 2014 14:35, Michal Kazior <michal.kazior@tieto.com> wrote:
> On 16 July 2014 13:38, Varka Bhadram <varkabhadram@gmail.com> wrote:
>> On 07/16/2014 04:49 PM, Michal Kazior wrote:
>>
>> (...)
>>
>>
>>> +static void ath10k_htt_rx_addba(struct ath10k *ar, struct htt_resp *resp)
>>> +{
>>> + struct htt_rx_addba *ev = &resp->rx_addba;
>>> + struct ath10k_peer *peer;
>>> + struct ath10k_vif *arvif;
>>> + u16 info0, tid, peer_id;
>>> +
>>> + info0 = __le32_to_cpu(ev->info0);
>>> + tid = MS(info0, HTT_RX_BA_INFO0_TID);
>>> + peer_id = MS(info0, HTT_RX_BA_INFO0_PEER_ID);
>>> +
>>> + ath10k_dbg(ATH10K_DBG_HTT,
>>> + "htt rx addba tid %hu peer_id %hu size %hhu\n",
>>> + tid, peer_id, ev->window_size);
>>> +
>>> + spin_lock_bh(&ar->data_lock);
>>> + peer = ath10k_peer_find_by_id(ar, peer_id);
>>> + if (!peer) {
>>> + ath10k_warn("received addba event for invalid peer_id:
>>> %hu\n",
>>> + peer_id);
>>> + spin_unlock_bh(&ar->data_lock);
>>> + return;
>>> + }
>>
>>
>> Here my concern in amount of time holding the lock...
>>
>> spin_lock_bh(&ar->data_lock);
>> peer = ath10k_peer_find_by_id(ar, peer_id);
>> if (!peer) {
>> spin_unlock_bh(&ar->data_lock);
>>
>> ath10k_warn("received addba event for invalid peer_id: %hu\n",
>> peer_id);
>> return;
>> }
>>
>> No need to of putting the debug message inside the critical region... :-)
>
> Sounds reasonable in this case as I'm not printing spinlock-protected values.
..and I realized this isn't true upon hitting the send button.
The other print uses peer->vdev_id. The peer was acquired under a lock
and must not be used after the lock is released. It'll just look
confusing if I mix ordering of unlock/print in some cases so I'll
leave it as is.
(sorry for the noise)
Michał
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3] ath10k: fix Rx aggregation reordering
2014-07-16 12:41 ` Michal Kazior
@ 2014-07-18 12:41 ` Kalle Valo
-1 siblings, 0 replies; 19+ messages in thread
From: Kalle Valo @ 2014-07-18 12:41 UTC (permalink / raw)
To: Michal Kazior
Cc: Varka Bhadram, linux-wireless, ath10k@lists.infradead.org,
Denton Gentry
Michal Kazior <michal.kazior@tieto.com> writes:
>>> Here my concern in amount of time holding the lock...
>>>
>>> spin_lock_bh(&ar->data_lock);
>>> peer = ath10k_peer_find_by_id(ar, peer_id);
>>> if (!peer) {
>>> spin_unlock_bh(&ar->data_lock);
>>>
>>> ath10k_warn("received addba event for invalid peer_id: %hu\n",
>>> peer_id);
>>> return;
>>> }
>>>
>>> No need to of putting the debug message inside the critical region... :-)
>>
>> Sounds reasonable in this case as I'm not printing spinlock-protected values.
>
> ..and I realized this isn't true upon hitting the send button.
>
> The other print uses peer->vdev_id. The peer was acquired under a lock
> and must not be used after the lock is released. It'll just look
> confusing if I mix ordering of unlock/print in some cases so I'll
> leave it as is.
I actually prefer also the original form of "warn(); unlock(); return;",
somehow it feels more natural for me. And this is on error path where
efficiency is not really a priority.
--
Kalle Valo
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3] ath10k: fix Rx aggregation reordering
@ 2014-07-18 12:41 ` Kalle Valo
0 siblings, 0 replies; 19+ messages in thread
From: Kalle Valo @ 2014-07-18 12:41 UTC (permalink / raw)
To: Michal Kazior
Cc: Varka Bhadram, linux-wireless, ath10k@lists.infradead.org,
Denton Gentry
Michal Kazior <michal.kazior@tieto.com> writes:
>>> Here my concern in amount of time holding the lock...
>>>
>>> spin_lock_bh(&ar->data_lock);
>>> peer = ath10k_peer_find_by_id(ar, peer_id);
>>> if (!peer) {
>>> spin_unlock_bh(&ar->data_lock);
>>>
>>> ath10k_warn("received addba event for invalid peer_id: %hu\n",
>>> peer_id);
>>> return;
>>> }
>>>
>>> No need to of putting the debug message inside the critical region... :-)
>>
>> Sounds reasonable in this case as I'm not printing spinlock-protected values.
>
> ..and I realized this isn't true upon hitting the send button.
>
> The other print uses peer->vdev_id. The peer was acquired under a lock
> and must not be used after the lock is released. It'll just look
> confusing if I mix ordering of unlock/print in some cases so I'll
> leave it as is.
I actually prefer also the original form of "warn(); unlock(); return;",
somehow it feels more natural for me. And this is on error path where
efficiency is not really a priority.
--
Kalle Valo
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] ath10k: fix Rx aggregation reordering
2014-07-16 11:19 ` Michal Kazior
(?)
(?)
@ 2014-07-22 18:37 ` Kalle Valo
2014-07-23 9:50 ` Michal Kazior
-1 siblings, 1 reply; 19+ messages in thread
From: Kalle Valo @ 2014-07-22 18:37 UTC (permalink / raw)
To: Michal Kazior; +Cc: ath10k, Denton Gentry
(dropping linux-wireless)
Michal Kazior <michal.kazior@tieto.com> writes:
> Firmware doesn't perform Rx reordering so it is
> left to the host driver to do that.
>
> Use mac80211 to perform reordering instead of
> re-inventing the wheel.
>
> This fixes TCP throughput issues in some
> environments.
>
> Reported-by: Denton Gentry <denton.gentry@gmail.com>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
> ---
> This depends on:
>
> * mac80211: fix Rx reordering with RX_FLAG_AMSDU_MORE
> * mac80211: add support for Rx reordering offloading
>
> I'm still seeing some issues with TCP traffic.
> Apparently firmware/hardware sometimes stalls Rx
> for ~100ms and misses some aggregate frames. I
> wasn't able to pinpoint what the problem is. The
> more parallel TCP streams (iperf) the more often
> stalls happen. UDP doesn't seem to trigger the Rx
> stalls at all.
The mac80211 patches are in mac80211-next, but I can only apply this one
only once they are in wireless-next.
Also the other thing is that 3.17 merge window is very close. I might be
able to squeeze this in but there's the risk that this patch hasn't
received enough testing from being in ath.git. How safe (or risky) is
this patch? If we push this to 3.17, we could try to fix regressions in
-rc releases but that's always more difficult than waiting for 3.18.
--
Kalle Valo
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] ath10k: fix Rx aggregation reordering
2014-07-22 18:37 ` Kalle Valo
@ 2014-07-23 9:50 ` Michal Kazior
2014-07-23 16:20 ` Kalle Valo
0 siblings, 1 reply; 19+ messages in thread
From: Michal Kazior @ 2014-07-23 9:50 UTC (permalink / raw)
To: Kalle Valo; +Cc: ath10k@lists.infradead.org, Denton Gentry
On 22 July 2014 20:37, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> (dropping linux-wireless)
>
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> Firmware doesn't perform Rx reordering so it is
>> left to the host driver to do that.
>>
>> Use mac80211 to perform reordering instead of
>> re-inventing the wheel.
>>
>> This fixes TCP throughput issues in some
>> environments.
>>
>> Reported-by: Denton Gentry <denton.gentry@gmail.com>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>> ---
>> This depends on:
>>
>> * mac80211: fix Rx reordering with RX_FLAG_AMSDU_MORE
>> * mac80211: add support for Rx reordering offloading
>>
>> I'm still seeing some issues with TCP traffic.
>> Apparently firmware/hardware sometimes stalls Rx
>> for ~100ms and misses some aggregate frames. I
>> wasn't able to pinpoint what the problem is. The
>> more parallel TCP streams (iperf) the more often
>> stalls happen. UDP doesn't seem to trigger the Rx
>> stalls at all.
>
> The mac80211 patches are in mac80211-next, but I can only apply this one
> only once they are in wireless-next.
>
> Also the other thing is that 3.17 merge window is very close. I might be
> able to squeeze this in but there's the risk that this patch hasn't
> received enough testing from being in ath.git. How safe (or risky) is
> this patch? If we push this to 3.17, we could try to fix regressions in
> -rc releases but that's always more difficult than waiting for 3.18.
I think it should be fairly safe. The stalls I've observed don't make
it worse than it is without Rx reordering.
But! I need to re-spin this patch since there's an endianess bug with
info0 being treated as __le32 while its __le16.
Michał
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] ath10k: fix Rx aggregation reordering
2014-07-23 9:50 ` Michal Kazior
@ 2014-07-23 16:20 ` Kalle Valo
0 siblings, 0 replies; 19+ messages in thread
From: Kalle Valo @ 2014-07-23 16:20 UTC (permalink / raw)
To: Michal Kazior; +Cc: ath10k@lists.infradead.org, Denton Gentry
Michal Kazior <michal.kazior@tieto.com> writes:
>> The mac80211 patches are in mac80211-next, but I can only apply this one
>> only once they are in wireless-next.
>>
>> Also the other thing is that 3.17 merge window is very close. I might be
>> able to squeeze this in but there's the risk that this patch hasn't
>> received enough testing from being in ath.git. How safe (or risky) is
>> this patch? If we push this to 3.17, we could try to fix regressions in
>> -rc releases but that's always more difficult than waiting for 3.18.
>
> I think it should be fairly safe. The stalls I've observed don't make
> it worse than it is without Rx reordering.
>
> But! I need to re-spin this patch since there's an endianess bug with
> info0 being treated as __le32 while its __le16.
Ok, let's try to get the new version of this patch to 3.17, if at all
possible. But if there are any regressions, please let me know ASAP so
that we can quickly fix it and push the fix to -rc releases.
--
Kalle Valo
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4] ath10k: fix Rx aggregation reordering
2014-07-16 11:19 ` Michal Kazior
@ 2014-07-23 10:20 ` Michal Kazior
-1 siblings, 0 replies; 19+ messages in thread
From: Michal Kazior @ 2014-07-23 10:20 UTC (permalink / raw)
To: ath10k; +Cc: linux-wireless, Michal Kazior, Denton Gentry
Firmware doesn't perform Rx reordering so it is
left to the host driver to do that.
Use mac80211 to perform reordering instead of
re-inventing the wheel.
This fixes TCP throughput issues in some
environments.
Reported-by: Denton Gentry <denton.gentry@gmail.com>
Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
This depends on:
* mac80211: fix Rx reordering with RX_FLAG_AMSDU_MORE
* mac80211: add support for Rx reordering offloading
I'm still seeing some issues with TCP traffic.
Apparently firmware/hardware sometimes stalls Rx
for ~100ms and misses some aggregate frames. I
wasn't able to pinpoint what the problem is. The
more parallel TCP streams (iperf) the more often
stalls happen. UDP doesn't seem to trigger the Rx
stalls at all.
Notes:
v4:
* return immediatelly instead break [Varka]
* fix endianess conversion (__le32 -> __le16)
* use rx_delba in delba callback
v3:
* make ath10k_ampdu_action() static
v2:
* split addba/delba handling into separate functions [Kalle]
* extend ampdu_action debug print (+vdev_id, +sta addr, +tid)
drivers/net/wireless/ath/ath10k/htt_rx.c | 92 +++++++++++++++++++++++++++++++-
drivers/net/wireless/ath/ath10k/mac.c | 33 ++++++++++++
drivers/net/wireless/ath/ath10k/txrx.c | 3 +-
drivers/net/wireless/ath/ath10k/txrx.h | 1 +
4 files changed, 126 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 318efc3..77cdc21 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -21,6 +21,7 @@
#include "txrx.h"
#include "debug.h"
#include "trace.h"
+#include "mac.h"
#include <linux/log2.h>
@@ -1422,6 +1423,86 @@ static void ath10k_htt_rx_frm_tx_compl(struct ath10k *ar,
}
}
+static void ath10k_htt_rx_addba(struct ath10k *ar, struct htt_resp *resp)
+{
+ struct htt_rx_addba *ev = &resp->rx_addba;
+ struct ath10k_peer *peer;
+ struct ath10k_vif *arvif;
+ u16 info0, tid, peer_id;
+
+ info0 = __le16_to_cpu(ev->info0);
+ tid = MS(info0, HTT_RX_BA_INFO0_TID);
+ peer_id = MS(info0, HTT_RX_BA_INFO0_PEER_ID);
+
+ ath10k_dbg(ATH10K_DBG_HTT,
+ "htt rx addba tid %hu peer_id %hu size %hhu\n",
+ tid, peer_id, ev->window_size);
+
+ spin_lock_bh(&ar->data_lock);
+ peer = ath10k_peer_find_by_id(ar, peer_id);
+ if (!peer) {
+ ath10k_warn("received addba event for invalid peer_id: %hu\n",
+ peer_id);
+ spin_unlock_bh(&ar->data_lock);
+ return;
+ }
+
+ arvif = ath10k_get_arvif(ar, peer->vdev_id);
+ if (!arvif) {
+ ath10k_warn("received addba event for invalid vdev_id: %u\n",
+ peer->vdev_id);
+ spin_unlock_bh(&ar->data_lock);
+ return;
+ }
+
+ ath10k_dbg(ATH10K_DBG_HTT,
+ "htt rx start rx ba session sta %pM tid %hu size %hhu\n",
+ peer->addr, tid, ev->window_size);
+
+ ieee80211_start_rx_ba_session_offl(arvif->vif, peer->addr, tid);
+ spin_unlock_bh(&ar->data_lock);
+}
+
+static void ath10k_htt_rx_delba(struct ath10k *ar, struct htt_resp *resp)
+{
+ struct htt_rx_delba *ev = &resp->rx_delba;
+ struct ath10k_peer *peer;
+ struct ath10k_vif *arvif;
+ u16 info0, tid, peer_id;
+
+ info0 = __le16_to_cpu(ev->info0);
+ tid = MS(info0, HTT_RX_BA_INFO0_TID);
+ peer_id = MS(info0, HTT_RX_BA_INFO0_PEER_ID);
+
+ ath10k_dbg(ATH10K_DBG_HTT,
+ "htt rx delba tid %hu peer_id %hu\n",
+ tid, peer_id);
+
+ spin_lock_bh(&ar->data_lock);
+ peer = ath10k_peer_find_by_id(ar, peer_id);
+ if (!peer) {
+ ath10k_warn("received addba event for invalid peer_id: %hu\n",
+ peer_id);
+ spin_unlock_bh(&ar->data_lock);
+ return;
+ }
+
+ arvif = ath10k_get_arvif(ar, peer->vdev_id);
+ if (!arvif) {
+ ath10k_warn("received addba event for invalid vdev_id: %u\n",
+ peer->vdev_id);
+ spin_unlock_bh(&ar->data_lock);
+ return;
+ }
+
+ ath10k_dbg(ATH10K_DBG_HTT,
+ "htt rx stop rx ba session sta %pM tid %hu\n",
+ peer->addr, tid);
+
+ ieee80211_stop_rx_ba_session_offl(arvif->vif, peer->addr, tid);
+ spin_unlock_bh(&ar->data_lock);
+}
+
void ath10k_htt_t2h_msg_handler(struct ath10k *ar, struct sk_buff *skb)
{
struct ath10k_htt *htt = &ar->htt;
@@ -1524,8 +1605,17 @@ void ath10k_htt_t2h_msg_handler(struct ath10k *ar, struct sk_buff *skb)
ath10k_warn("received an unexpected htt tx inspect event\n");
break;
case HTT_T2H_MSG_TYPE_RX_ADDBA:
+ ath10k_htt_rx_addba(ar, resp);
+ break;
case HTT_T2H_MSG_TYPE_RX_DELBA:
- case HTT_T2H_MSG_TYPE_RX_FLUSH:
+ ath10k_htt_rx_delba(ar, resp);
+ break;
+ case HTT_T2H_MSG_TYPE_RX_FLUSH: {
+ /* Ignore this event because mac80211 takes care of Rx
+ * aggregation reordering.
+ */
+ break;
+ }
default:
ath10k_dbg(ATH10K_DBG_HTT, "htt event (%d) not handled\n",
resp->hdr.msg_type);
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 3f9afaa..3baa229 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -4333,6 +4333,38 @@ static u64 ath10k_get_tsf(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
return 0;
}
+static int ath10k_ampdu_action(struct ieee80211_hw *hw,
+ struct ieee80211_vif *vif,
+ enum ieee80211_ampdu_mlme_action action,
+ struct ieee80211_sta *sta, u16 tid, u16 *ssn,
+ u8 buf_size)
+{
+ struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+
+ ath10k_dbg(ATH10K_DBG_MAC, "mac ampdu vdev_id %i sta %pM tid %hu action %d\n",
+ arvif->vdev_id, sta->addr, tid, action);
+
+ switch (action) {
+ case IEEE80211_AMPDU_RX_START:
+ case IEEE80211_AMPDU_RX_STOP:
+ /* HTT AddBa/DelBa events trigger mac80211 Rx BA session
+ * creation/removal. Do we need to verify this?
+ */
+ return 0;
+ case IEEE80211_AMPDU_TX_START:
+ case IEEE80211_AMPDU_TX_STOP_CONT:
+ case IEEE80211_AMPDU_TX_STOP_FLUSH:
+ case IEEE80211_AMPDU_TX_STOP_FLUSH_CONT:
+ case IEEE80211_AMPDU_TX_OPERATIONAL:
+ /* Firmware offloads Tx aggregation entirely so deny mac80211
+ * Tx aggregation requests.
+ */
+ return -EOPNOTSUPP;
+ }
+
+ return -EINVAL;
+}
+
static const struct ieee80211_ops ath10k_ops = {
.tx = ath10k_tx,
.start = ath10k_start,
@@ -4360,6 +4392,7 @@ static const struct ieee80211_ops ath10k_ops = {
.set_bitrate_mask = ath10k_set_bitrate_mask,
.sta_rc_update = ath10k_sta_rc_update,
.get_tsf = ath10k_get_tsf,
+ .ampdu_action = ath10k_ampdu_action,
#ifdef CONFIG_PM
.suspend = ath10k_suspend,
.resume = ath10k_resume,
diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c
index 82669a7..f4fa22d 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -119,8 +119,7 @@ struct ath10k_peer *ath10k_peer_find(struct ath10k *ar, int vdev_id,
return NULL;
}
-static struct ath10k_peer *ath10k_peer_find_by_id(struct ath10k *ar,
- int peer_id)
+struct ath10k_peer *ath10k_peer_find_by_id(struct ath10k *ar, int peer_id)
{
struct ath10k_peer *peer;
diff --git a/drivers/net/wireless/ath/ath10k/txrx.h b/drivers/net/wireless/ath/ath10k/txrx.h
index aee3e20..a90e09f 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.h
+++ b/drivers/net/wireless/ath/ath10k/txrx.h
@@ -24,6 +24,7 @@ void ath10k_txrx_tx_unref(struct ath10k_htt *htt,
struct ath10k_peer *ath10k_peer_find(struct ath10k *ar, int vdev_id,
const u8 *addr);
+struct ath10k_peer *ath10k_peer_find_by_id(struct ath10k *ar, int peer_id);
int ath10k_wait_for_peer_created(struct ath10k *ar, int vdev_id,
const u8 *addr);
int ath10k_wait_for_peer_deleted(struct ath10k *ar, int vdev_id,
--
1.8.5.3
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v4] ath10k: fix Rx aggregation reordering
@ 2014-07-23 10:20 ` Michal Kazior
0 siblings, 0 replies; 19+ messages in thread
From: Michal Kazior @ 2014-07-23 10:20 UTC (permalink / raw)
To: ath10k; +Cc: linux-wireless, Denton Gentry, Michal Kazior
Firmware doesn't perform Rx reordering so it is
left to the host driver to do that.
Use mac80211 to perform reordering instead of
re-inventing the wheel.
This fixes TCP throughput issues in some
environments.
Reported-by: Denton Gentry <denton.gentry@gmail.com>
Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
This depends on:
* mac80211: fix Rx reordering with RX_FLAG_AMSDU_MORE
* mac80211: add support for Rx reordering offloading
I'm still seeing some issues with TCP traffic.
Apparently firmware/hardware sometimes stalls Rx
for ~100ms and misses some aggregate frames. I
wasn't able to pinpoint what the problem is. The
more parallel TCP streams (iperf) the more often
stalls happen. UDP doesn't seem to trigger the Rx
stalls at all.
Notes:
v4:
* return immediatelly instead break [Varka]
* fix endianess conversion (__le32 -> __le16)
* use rx_delba in delba callback
v3:
* make ath10k_ampdu_action() static
v2:
* split addba/delba handling into separate functions [Kalle]
* extend ampdu_action debug print (+vdev_id, +sta addr, +tid)
drivers/net/wireless/ath/ath10k/htt_rx.c | 92 +++++++++++++++++++++++++++++++-
drivers/net/wireless/ath/ath10k/mac.c | 33 ++++++++++++
drivers/net/wireless/ath/ath10k/txrx.c | 3 +-
drivers/net/wireless/ath/ath10k/txrx.h | 1 +
4 files changed, 126 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 318efc3..77cdc21 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -21,6 +21,7 @@
#include "txrx.h"
#include "debug.h"
#include "trace.h"
+#include "mac.h"
#include <linux/log2.h>
@@ -1422,6 +1423,86 @@ static void ath10k_htt_rx_frm_tx_compl(struct ath10k *ar,
}
}
+static void ath10k_htt_rx_addba(struct ath10k *ar, struct htt_resp *resp)
+{
+ struct htt_rx_addba *ev = &resp->rx_addba;
+ struct ath10k_peer *peer;
+ struct ath10k_vif *arvif;
+ u16 info0, tid, peer_id;
+
+ info0 = __le16_to_cpu(ev->info0);
+ tid = MS(info0, HTT_RX_BA_INFO0_TID);
+ peer_id = MS(info0, HTT_RX_BA_INFO0_PEER_ID);
+
+ ath10k_dbg(ATH10K_DBG_HTT,
+ "htt rx addba tid %hu peer_id %hu size %hhu\n",
+ tid, peer_id, ev->window_size);
+
+ spin_lock_bh(&ar->data_lock);
+ peer = ath10k_peer_find_by_id(ar, peer_id);
+ if (!peer) {
+ ath10k_warn("received addba event for invalid peer_id: %hu\n",
+ peer_id);
+ spin_unlock_bh(&ar->data_lock);
+ return;
+ }
+
+ arvif = ath10k_get_arvif(ar, peer->vdev_id);
+ if (!arvif) {
+ ath10k_warn("received addba event for invalid vdev_id: %u\n",
+ peer->vdev_id);
+ spin_unlock_bh(&ar->data_lock);
+ return;
+ }
+
+ ath10k_dbg(ATH10K_DBG_HTT,
+ "htt rx start rx ba session sta %pM tid %hu size %hhu\n",
+ peer->addr, tid, ev->window_size);
+
+ ieee80211_start_rx_ba_session_offl(arvif->vif, peer->addr, tid);
+ spin_unlock_bh(&ar->data_lock);
+}
+
+static void ath10k_htt_rx_delba(struct ath10k *ar, struct htt_resp *resp)
+{
+ struct htt_rx_delba *ev = &resp->rx_delba;
+ struct ath10k_peer *peer;
+ struct ath10k_vif *arvif;
+ u16 info0, tid, peer_id;
+
+ info0 = __le16_to_cpu(ev->info0);
+ tid = MS(info0, HTT_RX_BA_INFO0_TID);
+ peer_id = MS(info0, HTT_RX_BA_INFO0_PEER_ID);
+
+ ath10k_dbg(ATH10K_DBG_HTT,
+ "htt rx delba tid %hu peer_id %hu\n",
+ tid, peer_id);
+
+ spin_lock_bh(&ar->data_lock);
+ peer = ath10k_peer_find_by_id(ar, peer_id);
+ if (!peer) {
+ ath10k_warn("received addba event for invalid peer_id: %hu\n",
+ peer_id);
+ spin_unlock_bh(&ar->data_lock);
+ return;
+ }
+
+ arvif = ath10k_get_arvif(ar, peer->vdev_id);
+ if (!arvif) {
+ ath10k_warn("received addba event for invalid vdev_id: %u\n",
+ peer->vdev_id);
+ spin_unlock_bh(&ar->data_lock);
+ return;
+ }
+
+ ath10k_dbg(ATH10K_DBG_HTT,
+ "htt rx stop rx ba session sta %pM tid %hu\n",
+ peer->addr, tid);
+
+ ieee80211_stop_rx_ba_session_offl(arvif->vif, peer->addr, tid);
+ spin_unlock_bh(&ar->data_lock);
+}
+
void ath10k_htt_t2h_msg_handler(struct ath10k *ar, struct sk_buff *skb)
{
struct ath10k_htt *htt = &ar->htt;
@@ -1524,8 +1605,17 @@ void ath10k_htt_t2h_msg_handler(struct ath10k *ar, struct sk_buff *skb)
ath10k_warn("received an unexpected htt tx inspect event\n");
break;
case HTT_T2H_MSG_TYPE_RX_ADDBA:
+ ath10k_htt_rx_addba(ar, resp);
+ break;
case HTT_T2H_MSG_TYPE_RX_DELBA:
- case HTT_T2H_MSG_TYPE_RX_FLUSH:
+ ath10k_htt_rx_delba(ar, resp);
+ break;
+ case HTT_T2H_MSG_TYPE_RX_FLUSH: {
+ /* Ignore this event because mac80211 takes care of Rx
+ * aggregation reordering.
+ */
+ break;
+ }
default:
ath10k_dbg(ATH10K_DBG_HTT, "htt event (%d) not handled\n",
resp->hdr.msg_type);
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 3f9afaa..3baa229 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -4333,6 +4333,38 @@ static u64 ath10k_get_tsf(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
return 0;
}
+static int ath10k_ampdu_action(struct ieee80211_hw *hw,
+ struct ieee80211_vif *vif,
+ enum ieee80211_ampdu_mlme_action action,
+ struct ieee80211_sta *sta, u16 tid, u16 *ssn,
+ u8 buf_size)
+{
+ struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+
+ ath10k_dbg(ATH10K_DBG_MAC, "mac ampdu vdev_id %i sta %pM tid %hu action %d\n",
+ arvif->vdev_id, sta->addr, tid, action);
+
+ switch (action) {
+ case IEEE80211_AMPDU_RX_START:
+ case IEEE80211_AMPDU_RX_STOP:
+ /* HTT AddBa/DelBa events trigger mac80211 Rx BA session
+ * creation/removal. Do we need to verify this?
+ */
+ return 0;
+ case IEEE80211_AMPDU_TX_START:
+ case IEEE80211_AMPDU_TX_STOP_CONT:
+ case IEEE80211_AMPDU_TX_STOP_FLUSH:
+ case IEEE80211_AMPDU_TX_STOP_FLUSH_CONT:
+ case IEEE80211_AMPDU_TX_OPERATIONAL:
+ /* Firmware offloads Tx aggregation entirely so deny mac80211
+ * Tx aggregation requests.
+ */
+ return -EOPNOTSUPP;
+ }
+
+ return -EINVAL;
+}
+
static const struct ieee80211_ops ath10k_ops = {
.tx = ath10k_tx,
.start = ath10k_start,
@@ -4360,6 +4392,7 @@ static const struct ieee80211_ops ath10k_ops = {
.set_bitrate_mask = ath10k_set_bitrate_mask,
.sta_rc_update = ath10k_sta_rc_update,
.get_tsf = ath10k_get_tsf,
+ .ampdu_action = ath10k_ampdu_action,
#ifdef CONFIG_PM
.suspend = ath10k_suspend,
.resume = ath10k_resume,
diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c
index 82669a7..f4fa22d 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -119,8 +119,7 @@ struct ath10k_peer *ath10k_peer_find(struct ath10k *ar, int vdev_id,
return NULL;
}
-static struct ath10k_peer *ath10k_peer_find_by_id(struct ath10k *ar,
- int peer_id)
+struct ath10k_peer *ath10k_peer_find_by_id(struct ath10k *ar, int peer_id)
{
struct ath10k_peer *peer;
diff --git a/drivers/net/wireless/ath/ath10k/txrx.h b/drivers/net/wireless/ath/ath10k/txrx.h
index aee3e20..a90e09f 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.h
+++ b/drivers/net/wireless/ath/ath10k/txrx.h
@@ -24,6 +24,7 @@ void ath10k_txrx_tx_unref(struct ath10k_htt *htt,
struct ath10k_peer *ath10k_peer_find(struct ath10k *ar, int vdev_id,
const u8 *addr);
+struct ath10k_peer *ath10k_peer_find_by_id(struct ath10k *ar, int peer_id);
int ath10k_wait_for_peer_created(struct ath10k *ar, int vdev_id,
const u8 *addr);
int ath10k_wait_for_peer_deleted(struct ath10k *ar, int vdev_id,
--
1.8.5.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4] ath10k: fix Rx aggregation reordering
2014-07-23 10:20 ` Michal Kazior
@ 2014-07-25 8:18 ` Kalle Valo
-1 siblings, 0 replies; 19+ messages in thread
From: Kalle Valo @ 2014-07-25 8:18 UTC (permalink / raw)
To: Michal Kazior; +Cc: linux-wireless, ath10k, Denton Gentry
Michal Kazior <michal.kazior@tieto.com> writes:
> Firmware doesn't perform Rx reordering so it is
> left to the host driver to do that.
>
> Use mac80211 to perform reordering instead of
> re-inventing the wheel.
>
> This fixes TCP throughput issues in some
> environments.
>
> Reported-by: Denton Gentry <denton.gentry@gmail.com>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
Thanks, applied.
--
Kalle Valo
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v4] ath10k: fix Rx aggregation reordering
@ 2014-07-25 8:18 ` Kalle Valo
0 siblings, 0 replies; 19+ messages in thread
From: Kalle Valo @ 2014-07-25 8:18 UTC (permalink / raw)
To: Michal Kazior; +Cc: ath10k, linux-wireless, Denton Gentry
Michal Kazior <michal.kazior@tieto.com> writes:
> Firmware doesn't perform Rx reordering so it is
> left to the host driver to do that.
>
> Use mac80211 to perform reordering instead of
> re-inventing the wheel.
>
> This fixes TCP throughput issues in some
> environments.
>
> Reported-by: Denton Gentry <denton.gentry@gmail.com>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
Thanks, applied.
--
Kalle Valo
^ permalink raw reply [flat|nested] 19+ messages in thread