[PATCH 0/9] ath10k: htt rx fixes and clean ups

[Michal Kazior]

Hi,

I've found a couple of issues while reworking rx
path.


Michal Kazior (9):
ath10k: don't drop control and null func Rx
ath10k: remove unused variable
ath10k: use ieee80211 defines for crypto param lengths
ath10k: fix rx buffer tracing
ath10k: deduplicate htt rx dma unmapping
ath10k: don't drop frames aggressively
ath10k: add extra sanity check when popping amsdu
ath10k: don't forget to replenish after fragmented Rx
ath10k: clear htt->rx_confused on load

drivers/net/wireless/ath/ath10k/htt_rx.c | 91 ++++++++++++++------------------
1 file changed, 39 insertions(+), 52 deletions(-)

[Michal Kazior]

HTT_RX_IND_MPDU_STATUS_MGMT_CTRL was pretty greedy
and because of that ath10k ended up dropping
Control Frames as well as Null Func frames.

Reported-by: Okhwan Lee <ohlee-48zsom2sK/***@public.gmane.org>
Signed-off-by: Michal Kazior <michal.kazior-++***@public.gmane.org>
---
drivers/net/wireless/ath/ath10k/htt_rx.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index fbb3175..63c69d8 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -1200,8 +1200,7 @@ static bool ath10k_htt_rx_amsdu_allowed(struct ath10k_htt *htt,
}

/* Skip mgmt frames while we handle this in WMI */
- if (status == HTT_RX_IND_MPDU_STATUS_MGMT_CTRL ||
- attention & RX_ATTENTION_FLAGS_MGMT_TYPE) {
+ if (attention & RX_ATTENTION_FLAGS_MGMT_TYPE) {
ath10k_dbg(ar, ATH10K_DBG_HTT, "htt rx mgmt ctrl\n");
return false;
}

[Michal Kazior]

Use the globally defined ieee80211 values instead
of re-defining them in the driver again.

Signed-off-by: Michal Kazior <michal.kazior-++***@public.gmane.org>
---
drivers/net/wireless/ath/ath10k/htt_rx.c | 34 +++++++++++++++++++-------------
1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 61ff2ac..ca466ce 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -588,41 +588,47 @@ static int ath10k_htt_rx_crypto_param_len(struct ath10k *ar,
enum htt_rx_mpdu_encrypt_type type)
{
switch (type) {
+ case HTT_RX_MPDU_ENCRYPT_NONE:
+ return 0;
case HTT_RX_MPDU_ENCRYPT_WEP40:
case HTT_RX_MPDU_ENCRYPT_WEP104:
- return 4;
+ return IEEE80211_WEP_IV_LEN;
case HTT_RX_MPDU_ENCRYPT_TKIP_WITHOUT_MIC:
- case HTT_RX_MPDU_ENCRYPT_WEP128: /* not tested */
case HTT_RX_MPDU_ENCRYPT_TKIP_WPA:
- case HTT_RX_MPDU_ENCRYPT_WAPI: /* not tested */
+ return IEEE80211_TKIP_IV_LEN;
case HTT_RX_MPDU_ENCRYPT_AES_CCM_WPA2:
- return 8;
- case HTT_RX_MPDU_ENCRYPT_NONE:
- return 0;
+ return IEEE80211_CCMP_HDR_LEN;
+ case HTT_RX_MPDU_ENCRYPT_WEP128:
+ case HTT_RX_MPDU_ENCRYPT_WAPI:
+ break;
}

- ath10k_warn(ar, "unknown encryption type %d\n", type);
+ ath10k_warn(ar, "unsupported encryption type %d\n", type);
return 0;
}

+#define MICHAEL_MIC_LEN 8
+
static int ath10k_htt_rx_crypto_tail_len(struct ath10k *ar,
enum htt_rx_mpdu_encrypt_type type)
{
switch (type) {
case HTT_RX_MPDU_ENCRYPT_NONE:
+ return 0;
case HTT_RX_MPDU_ENCRYPT_WEP40:
case HTT_RX_MPDU_ENCRYPT_WEP104:
- case HTT_RX_MPDU_ENCRYPT_WEP128:
- case HTT_RX_MPDU_ENCRYPT_WAPI:
- return 0;
+ return IEEE80211_WEP_ICV_LEN;
case HTT_RX_MPDU_ENCRYPT_TKIP_WITHOUT_MIC:
case HTT_RX_MPDU_ENCRYPT_TKIP_WPA:
- return 4;
+ return IEEE80211_TKIP_ICV_LEN;
case HTT_RX_MPDU_ENCRYPT_AES_CCM_WPA2:
- return 8;
+ return IEEE80211_CCMP_MIC_LEN;
+ case HTT_RX_MPDU_ENCRYPT_WEP128:
+ case HTT_RX_MPDU_ENCRYPT_WAPI:
+ break;
}

- ath10k_warn(ar, "unknown encryption type %d\n", type);
+ ath10k_warn(ar, "unsupported encryption type %d\n", type);
return 0;
}

@@ -1427,7 +1433,7 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
/* last fragment of TKIP frags has MIC */
if (!ieee80211_has_morefrags(hdr->frame_control) &&
enctype == HTT_RX_MPDU_ENCRYPT_TKIP_WPA)
- trim += 8;
+ trim += MICHAEL_MIC_LEN;

if (trim > msdu_head->len) {
ath10k_warn(ar, "htt rx fragment: trailer longer than the frame itself? drop\n");

[Michal Kazior]

Tracing function was called before buffers were
unmapped from DMA.

Signed-off-by: Michal Kazior <michal.kazior-++***@public.gmane.org>
---
drivers/net/wireless/ath/ath10k/htt_rx.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index ca466ce..a7d29c8 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -291,9 +291,6 @@ static inline struct sk_buff *ath10k_htt_rx_netbuf_pop(struct ath10k_htt *htt)
htt->rx_ring.sw_rd_idx.msdu_payld = idx;
htt->rx_ring.fill_cnt--;

- trace_ath10k_htt_rx_pop_msdu(ar, msdu->data, msdu->len +
- skb_tailroom(msdu));

[Michal Kazior]

The rx descriptor variable was no longer used in
the rx handler.

Signed-off-by: Michal Kazior <michal.kazior-++***@public.gmane.org>
---
drivers/net/wireless/ath/ath10k/htt_rx.c | 5 -----
1 file changed, 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 63c69d8..61ff2ac 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -1230,7 +1230,6 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
struct ath10k *ar = htt->ar;
struct ieee80211_rx_status *rx_status = &htt->rx_status;
struct htt_rx_indication_mpdu_range *mpdu_ranges;
- struct htt_rx_desc *rxd;
enum htt_rx_mpdu_status status;
struct ieee80211_hdr *hdr;
int num_mpdu_ranges;
@@ -1301,10 +1300,6 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
continue;
}

- rxd = container_of((void *)msdu_head->data,
- struct htt_rx_desc,
- msdu_payload);

[Michal Kazior]

Treat non-chained and chained popping the same
way. Also this makes netbuf pop fully symmetrical
to (re)filling.

Signed-off-by: Michal Kazior <michal.kazior-++***@public.gmane.org>
---
drivers/net/wireless/ath/ath10k/htt_rx.c | 30 +++++++++---------------------
1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index a7d29c8..41a2803 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -291,6 +291,15 @@ static inline struct sk_buff *ath10k_htt_rx_netbuf_pop(struct ath10k_htt *htt)
htt->rx_ring.sw_rd_idx.msdu_payld = idx;
htt->rx_ring.fill_cnt--;

+ dma_unmap_single(htt->ar->dev,
+ ATH10K_SKB_CB(msdu)->paddr,
+ msdu->len + skb_tailroom(msdu),
+ DMA_FROM_DEVICE);
+ ath10k_dbg_dump(ar, ATH10K_DBG_HTT_DUMP, NULL, "htt rx netbuf pop: ",
+ msdu->data, msdu->len + skb_tailroom(msdu));
+ trace_ath10k_htt_rx_pop_msdu(ar, msdu->data, msdu->len +
+ skb_tailroom(msdu));
+
return msdu;
}

@@ -329,16 +338,6 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
while (msdu) {
int last_msdu, msdu_len_invalid, msdu_chained;

- dma_unmap_single(htt->ar->dev,
- ATH10K_SKB_CB(msdu)->paddr,
- msdu->len + skb_tailroom(msdu),
- DMA_FROM_DEVICE);
-
- ath10k_dbg_dump(ar, ATH10K_DBG_HTT_DUMP, NULL, "htt rx pop: ",
- msdu->data, msdu->len + skb_tailroom(msdu));
- trace_ath10k_htt_rx_pop_msdu(ar, msdu->data, msdu->len +
- skb_tailroom(msdu));
-
rx_desc = (struct htt_rx_desc *)msdu->data;

/* FIXME: we must report msdu payload since this is what caller
@@ -429,17 +428,6 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
while (msdu_chained--) {
struct sk_buff *next = ath10k_htt_rx_netbuf_pop(htt);

- dma_unmap_single(htt->ar->dev,
- ATH10K_SKB_CB(next)->paddr,
- next->len + skb_tailroom(next),
- DMA_FROM_DEVICE);
-
- ath10k_dbg_dump(ar, ATH10K_DBG_HTT_DUMP, NULL,
- "htt rx chained: ", next->data,
- next->len + skb_tailroom(next));
- trace_ath10k_htt_rx_pop_msdu(ar, msdu->data, msdu->len +
- skb_tailroom(msdu));

[Michal Kazior]

There's little point in dropping, e.g. frames with
FCS error early in ath10k.

This simplifies amsdu_allowed() and gets rid of
htt_rx_mpdu_status usage finally.

Signed-off-by: Michal Kazior <michal.kazior-++***@public.gmane.org>
---
drivers/net/wireless/ath/ath10k/htt_rx.c | 15 ---------------
1 file changed, 15 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 41a2803..7fa4d87 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -1171,7 +1171,6 @@ static int ath10k_unchain_msdu(struct sk_buff *msdu_head)

static bool ath10k_htt_rx_amsdu_allowed(struct ath10k_htt *htt,
struct sk_buff *head,
- enum htt_rx_mpdu_status status,
bool channel_set,
u32 attention)
{
@@ -1200,16 +1199,6 @@ static bool ath10k_htt_rx_amsdu_allowed(struct ath10k_htt *htt,
return false;
}

- if (status != HTT_RX_IND_MPDU_STATUS_OK &&
- status != HTT_RX_IND_MPDU_STATUS_TKIP_MIC_ERR &&
- status != HTT_RX_IND_MPDU_STATUS_ERR_INV_PEER &&
- !htt->ar->monitor_started) {
- ath10k_dbg(ar, ATH10K_DBG_HTT,
- "htt rx ignoring frame w/ status %d\n",
- status);
- return false;
- }
-
if (test_bit(ATH10K_CAC_RUNNING, &htt->ar->dev_flags)) {
ath10k_dbg(ar, ATH10K_DBG_HTT,
"htt rx CAC running\n");
@@ -1225,7 +1214,6 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
struct ath10k *ar = htt->ar;
struct ieee80211_rx_status *rx_status = &htt->rx_status;
struct htt_rx_indication_mpdu_range *mpdu_ranges;
- enum htt_rx_mpdu_status status;
struct ieee80211_hdr *hdr;
int num_mpdu_ranges;
u32 attention;
@@ -1273,8 +1261,6 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
num_mpdu_ranges));

for (i = 0; i < num_mpdu_ranges; i++) {
- status = mpdu_ranges[i].mpdu_range_status;

[Michal Kazior]

Once driver entered the rx_confused state it would
refuse to rx even after firmware is restarted.
Make sure to clear it so that rx works after, e.g.
hw restart or after all interfaces are stopped.

Signed-off-by: Michal Kazior <michal.kazior-++***@public.gmane.org>
---
drivers/net/wireless/ath/ath10k/htt_rx.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 39e1969..3e52a33 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -497,6 +497,8 @@ int ath10k_htt_rx_alloc(struct ath10k_htt *htt)
size_t size;
struct timer_list *timer = &htt->rx_ring.refill_retry_timer;

+ htt->rx_confused = false;
+
htt->rx_ring.size = ath10k_htt_rx_ring_size(htt);
if (!is_power_of_2(htt->rx_ring.size)) {
ath10k_warn(ar, "htt rx ring size is not power of 2\n");

[Michal Kazior]

In theory it was possible to drain entire HTT Rx
ring via fragmented Rx leading to Rx lockup.

In practice non-data traffic would always trigger
replenishment via the regular Rx handler.

For correctness sake make sure to replenish the
ring on fragmented Rx.

Signed-off-by: Michal Kazior <michal.kazior-++***@public.gmane.org>
---
drivers/net/wireless/ath/ath10k/htt_rx.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index ddb9fe9..39e1969 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -1355,6 +1355,8 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
&attention);
spin_unlock_bh(&htt->rx_ring.lock);

+ tasklet_schedule(&htt->rx_replenish_task);
+
ath10k_dbg(ar, ATH10K_DBG_HTT_DUMP, "htt rx frag ahead\n");

if (ret) {

[Michal Kazior]

The netbuf pop can return NULL. Make sure to check
for that. It shouldn't happen but better safe than
sorry.

Signed-off-by: Michal Kazior <michal.kazior-++***@public.gmane.org>
---
drivers/net/wireless/ath/ath10k/htt_rx.c | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 7fa4d87..ddb9fe9 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -428,6 +428,15 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
while (msdu_chained--) {
struct sk_buff *next = ath10k_htt_rx_netbuf_pop(htt);

+ if (!next) {
+ ath10k_err(ar, "failed to pop chained msdu\n");
+ ath10k_htt_rx_free_msdu_chain(*head_msdu);
+ *head_msdu = NULL;
+ msdu = NULL;
+ htt->rx_confused = true;
+ break;
+ }
+
skb_trim(next, 0);
skb_put(next, min(msdu_len, HTT_RX_BUF_SIZE));
msdu_len -= next->len;

[Kalle Valo]

Post by Michal Kazior
The netbuf pop can return NULL. Make sure to check
for that. It shouldn't happen but better safe than
sorry.
---
drivers/net/wireless/ath/ath10k/htt_rx.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 7fa4d87..ddb9fe9 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -428,6 +428,15 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
while (msdu_chained--) {
struct sk_buff *next = ath10k_htt_rx_netbuf_pop(htt);
+ if (!next) {
+ ath10k_err(ar, "failed to pop chained msdu\n");

ath10k_warn() is better here, right? I have folded this diff to the patch:

--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -429,7 +429,7 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
struct sk_buff *next = ath10k_htt_rx_netbuf_pop(htt);

if (!next) {
- ath10k_err(ar, "failed to pop chained msdu\n");
+ ath10k_warn(ar, "failed to pop chained msdu\n");
ath10k_htt_rx_free_msdu_chain(*head_msdu);
*head_msdu = NULL;
msdu = NULL;

Full commit:

https://github.com/kvalo/ath/commit/03f316720d664a79e874c13df1e86a64c5f2bf5f