v3: minor cleanups for coding style; shorten lines in commit logs.
v2: split 6 patches into 2 patchsets. This is the set 1.
Andreas Fenkart (4):
mwifiex: correct wrong list in list_empty check
mwifiex: remove unused tid_tbl_lock from mwifiex_tid_tbl
mwifiex: fix infinite loop by removing NO_PKT_PRIO_TID
mwifiex: hold proper locks when accessing ra_list / bss_prio lists
drivers/net/wireless/mwifiex/init.c | 4 +-
drivers/net/wireless/mwifiex/main.h | 3 -
drivers/net/wireless/mwifiex/wmm.c | 71 ++++++++++++++++++-----------------
3 files changed, 37 insertions(+), 41 deletions(-)
From: Andreas Fenkart <[email protected]>
Using NO_PKT_PRIO_TID and tx_pkts_queued to check for an empty
state, can lead to a contradictory state, resulting in an
infinite loop. Currently queueing and dequeuing of packets is
not synchronized, and can happen concurrently. While tx_pkts_queued
is incremented when adding a packet, max prio is set to NO_PKT when
the WMM list is empty. If a packet is added right after the check
for empty, but before setting max prio to NO_PKT, that packet is
trapped and creates an infinite loop.
Because of the new packet, tx_pkts_queued is at least 1, indicating
wmm lists are not empty. Opposing that max prio is NO_PKT, which
means "skip this wmm queue, it has no packets". The infinite loop
results, because the main loop checks the wmm lists for not empty
via tx_pkts_queued, but for dequeing it uses max_prio to see if it
can skip current list. This will never end, unless a new packet is
added which will restore max prio to the level of the trapped packet.
The solution here is to rely on tx_pkts_queued solely for checking
wmm queue to be empty, and drop the NO_PKT define. It does not
address the locking issue.
Signed-off-by: Andreas Fenkart <[email protected]>
Signed-off-by: Avinash Patil <[email protected]>
Signed-off-by: Bing Zhao <[email protected]>
---
drivers/net/wireless/mwifiex/main.h | 1 -
drivers/net/wireless/mwifiex/wmm.c | 13 ++++++-------
2 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/mwifiex/main.h b/drivers/net/wireless/mwifiex/main.h
index 622b17f..b7484ef 100644
--- a/drivers/net/wireless/mwifiex/main.h
+++ b/drivers/net/wireless/mwifiex/main.h
@@ -219,7 +219,6 @@ struct mwifiex_tid_tbl {
#define WMM_HIGHEST_PRIORITY 7
#define HIGH_PRIO_TID 7
#define LOW_PRIO_TID 0
-#define NO_PKT_PRIO_TID (-1)
struct mwifiex_wmm_desc {
struct mwifiex_tid_tbl tid_tbl_ptr[MAX_NUM_TID];
diff --git a/drivers/net/wireless/mwifiex/wmm.c b/drivers/net/wireless/mwifiex/wmm.c
index c1d8488..75c8e80 100644
--- a/drivers/net/wireless/mwifiex/wmm.c
+++ b/drivers/net/wireless/mwifiex/wmm.c
@@ -916,8 +916,12 @@ mwifiex_wmm_get_highest_priolist_ptr(struct mwifiex_adapter *adapter,
do {
priv_tmp = bssprio_node->priv;
- hqp = &priv_tmp->wmm.highest_queued_prio;
+ if (atomic_read(&priv_tmp->wmm.tx_pkts_queued) == 0)
+ goto skip_bss;
+
+ /* iterate over the WMM queues of the BSS */
+ hqp = &priv_tmp->wmm.highest_queued_prio;
for (i = atomic_read(hqp); i >= LOW_PRIO_TID; --i) {
tid_ptr = &(priv_tmp)->wmm.
@@ -976,12 +980,7 @@ mwifiex_wmm_get_highest_priolist_ptr(struct mwifiex_adapter *adapter,
} while (ptr != head);
}
- /* No packet at any TID for this priv. Mark as such
- * to skip checking TIDs for this priv (until pkt is
- * added).
- */
- atomic_set(hqp, NO_PKT_PRIO_TID);
-
+skip_bss:
/* Get next bss priority node */
bssprio_node = list_first_entry(&bssprio_node->list,
struct mwifiex_bss_prio_node,
--
1.7.0.2
From: Andreas Fenkart <[email protected]>
Not locking ra_list when dequeuing packets creates race conditions.
When adding a packet 'tx_pkts_queued' is modified before setting
highest_priority_queue. If in-between the main loop starts, it will
see a packet queued (tx_pkts_queued > 0) but will not find it, since
max prio is not set yet. Depending on the scheduling, the thread
trying to add the packet could complete and restore the situation.
But this is not something to rely on.
Another race condition exists, if a new packet, exceeding current
max prio is added. If concurrently a packet is dequeued, the newly
set max prio will be overwritten with the value of the dequeued
packet. This can occur, because selecting a packet and modifying
the max prio is not atomic. The result in an infinite loop unless,
a new packet is added that has at least the priority of the hidden
packet.
Same applies to bss_prio_tbl. Forward iteration is no proper
lock-free technique and provides no protection from calls to
list_del. Although BSS are currently not added/removed dynamically,
this must not be the case in the future. Hence always hold proper
locks when accessing those lists.
Signed-off-by: Andreas Fenkart <[email protected]>
Signed-off-by: Bing Zhao <[email protected]>
---
drivers/net/wireless/mwifiex/wmm.c | 55 +++++++++++++++++++-----------------
1 files changed, 29 insertions(+), 26 deletions(-)
diff --git a/drivers/net/wireless/mwifiex/wmm.c b/drivers/net/wireless/mwifiex/wmm.c
index 75c8e80..2cc81ba 100644
--- a/drivers/net/wireless/mwifiex/wmm.c
+++ b/drivers/net/wireless/mwifiex/wmm.c
@@ -685,13 +685,13 @@ mwifiex_wmm_add_buf_txqueue(struct mwifiex_private *priv,
ra_list->total_pkts_size += skb->len;
ra_list->pkt_count++;
- atomic_inc(&priv->wmm.tx_pkts_queued);
-
if (atomic_read(&priv->wmm.highest_queued_prio) <
tos_to_tid_inv[tid_down])
atomic_set(&priv->wmm.highest_queued_prio,
tos_to_tid_inv[tid_down]);
+ atomic_inc(&priv->wmm.tx_pkts_queued);
+
spin_unlock_irqrestore(&priv->wmm.ra_list_spinlock, flags);
}
@@ -887,19 +887,15 @@ mwifiex_wmm_get_highest_priolist_ptr(struct mwifiex_adapter *adapter,
struct mwifiex_bss_prio_node *bssprio_node, *bssprio_head;
struct mwifiex_tid_tbl *tid_ptr;
atomic_t *hqp;
- int is_list_empty;
- unsigned long flags;
+ unsigned long flags_bss, flags_ra;
int i, j;
for (j = adapter->priv_num - 1; j >= 0; --j) {
spin_lock_irqsave(&adapter->bss_prio_tbl[j].bss_prio_lock,
- flags);
- is_list_empty = list_empty(&adapter->bss_prio_tbl[j]
- .bss_prio_head);
- spin_unlock_irqrestore(&adapter->bss_prio_tbl[j].bss_prio_lock,
- flags);
- if (is_list_empty)
- continue;
+ flags_bss);
+
+ if (list_empty(&adapter->bss_prio_tbl[j].bss_prio_head))
+ goto skip_prio_tbl;
if (adapter->bss_prio_tbl[j].bss_prio_cur ==
(struct mwifiex_bss_prio_node *)
@@ -924,21 +920,18 @@ mwifiex_wmm_get_highest_priolist_ptr(struct mwifiex_adapter *adapter,
hqp = &priv_tmp->wmm.highest_queued_prio;
for (i = atomic_read(hqp); i >= LOW_PRIO_TID; --i) {
+ spin_lock_irqsave(&priv_tmp->wmm.
+ ra_list_spinlock, flags_ra);
+
tid_ptr = &(priv_tmp)->wmm.
tid_tbl_ptr[tos_to_tid[i]];
/* For non-STA ra_list_curr may be NULL */
if (!tid_ptr->ra_list_curr)
- continue;
+ goto skip_wmm_queue;
- spin_lock_irqsave(&priv_tmp->wmm.
- ra_list_spinlock, flags);
- is_list_empty =
- list_empty(&tid_ptr->ra_list);
- spin_unlock_irqrestore(&priv_tmp->wmm.
- ra_list_spinlock, flags);
- if (is_list_empty)
- continue;
+ if (list_empty(&tid_ptr->ra_list))
+ goto skip_wmm_queue;
/*
* Always choose the next ra we transmitted
@@ -960,10 +953,8 @@ mwifiex_wmm_get_highest_priolist_ptr(struct mwifiex_adapter *adapter,
}
do {
- is_list_empty =
- skb_queue_empty(&ptr->skb_head);
-
- if (!is_list_empty)
+ if (!skb_queue_empty(&ptr->skb_head))
+ /* holds both locks */
goto found;
/* Get next ra */
@@ -978,6 +969,11 @@ mwifiex_wmm_get_highest_priolist_ptr(struct mwifiex_adapter *adapter,
struct mwifiex_ra_list_tbl,
list);
} while (ptr != head);
+
+skip_wmm_queue:
+ spin_unlock_irqrestore(&priv_tmp->wmm.
+ ra_list_spinlock,
+ flags_ra);
}
skip_bss:
@@ -995,14 +991,21 @@ skip_bss:
struct mwifiex_bss_prio_node,
list);
} while (bssprio_node != bssprio_head);
+
+skip_prio_tbl:
+ spin_unlock_irqrestore(&adapter->bss_prio_tbl[j].bss_prio_lock,
+ flags_bss);
}
+
return NULL;
found:
- spin_lock_irqsave(&priv_tmp->wmm.ra_list_spinlock, flags);
+ /* holds bss_prio_lock / ra_list_spinlock */
if (atomic_read(hqp) > i)
atomic_set(hqp, i);
- spin_unlock_irqrestore(&priv_tmp->wmm.ra_list_spinlock, flags);
+ spin_unlock_irqrestore(&priv_tmp->wmm.ra_list_spinlock, flags_ra);
+ spin_unlock_irqrestore(&adapter->bss_prio_tbl[j].bss_prio_lock,
+ flags_bss);
*priv = priv_tmp;
*tid = tos_to_tid[i];
--
1.7.0.2
From: Andreas Fenkart <[email protected]>
adapter->bss_prio_tbl list has already been checked in outer loop.
The inner loop works with priv_tmp->wmm.tid_tbl_ptr list. Also the
lock taken, gives hint that this is likely a copy-paste error.
Signed-off-by: Andreas Fenkart <[email protected]>
Signed-off-by: Avinash Patil <[email protected]>
Signed-off-by: Bing Zhao <[email protected]>
---
drivers/net/wireless/mwifiex/wmm.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/mwifiex/wmm.c b/drivers/net/wireless/mwifiex/wmm.c
index 3ddae52..1b039ba 100644
--- a/drivers/net/wireless/mwifiex/wmm.c
+++ b/drivers/net/wireless/mwifiex/wmm.c
@@ -930,8 +930,7 @@ mwifiex_wmm_get_highest_priolist_ptr(struct mwifiex_adapter *adapter,
spin_lock_irqsave(&tid_ptr->tid_tbl_lock,
flags);
is_list_empty =
- list_empty(&adapter->bss_prio_tbl[j]
- .bss_prio_head);
+ list_empty(&tid_ptr->ra_list);
spin_unlock_irqrestore(&tid_ptr->tid_tbl_lock,
flags);
if (is_list_empty)
--
1.7.0.2
From: Andreas Fenkart <[email protected]>
ra_list_spinlock is used to protect struct mwifiex_wmm_desc and
embedded structures such as ra_list. tid_tbl_lock while more fine
grained, is not used but in one function. That function is not
called reentrantly. To protect ra_list from concurrent modification
ra_list_spinlock must be held.
Signed-off-by: Andreas Fenkart <[email protected]>
Signed-off-by: Avinash Patil <[email protected]>
Signed-off-by: Bing Zhao <[email protected]>
---
drivers/net/wireless/mwifiex/init.c | 4 +---
drivers/net/wireless/mwifiex/main.h | 2 --
drivers/net/wireless/mwifiex/wmm.c | 8 ++++----
3 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/net/wireless/mwifiex/init.c b/drivers/net/wireless/mwifiex/init.c
index 003c014..42d7f0a 100644
--- a/drivers/net/wireless/mwifiex/init.c
+++ b/drivers/net/wireless/mwifiex/init.c
@@ -533,10 +533,8 @@ int mwifiex_init_lock_list(struct mwifiex_adapter *adapter)
if (!adapter->priv[i])
continue;
priv = adapter->priv[i];
- for (j = 0; j < MAX_NUM_TID; ++j) {
+ for (j = 0; j < MAX_NUM_TID; ++j)
INIT_LIST_HEAD(&priv->wmm.tid_tbl_ptr[j].ra_list);
- spin_lock_init(&priv->wmm.tid_tbl_ptr[j].tid_tbl_lock);
- }
INIT_LIST_HEAD(&priv->tx_ba_stream_tbl_ptr);
INIT_LIST_HEAD(&priv->rx_reorder_tbl_ptr);
INIT_LIST_HEAD(&priv->sta_list);
diff --git a/drivers/net/wireless/mwifiex/main.h b/drivers/net/wireless/mwifiex/main.h
index fef89fd..622b17f 100644
--- a/drivers/net/wireless/mwifiex/main.h
+++ b/drivers/net/wireless/mwifiex/main.h
@@ -213,8 +213,6 @@ struct mwifiex_ra_list_tbl {
struct mwifiex_tid_tbl {
struct list_head ra_list;
- /* spin lock for tid table */
- spinlock_t tid_tbl_lock;
struct mwifiex_ra_list_tbl *ra_list_curr;
};
diff --git a/drivers/net/wireless/mwifiex/wmm.c b/drivers/net/wireless/mwifiex/wmm.c
index 1b039ba..c1d8488 100644
--- a/drivers/net/wireless/mwifiex/wmm.c
+++ b/drivers/net/wireless/mwifiex/wmm.c
@@ -927,12 +927,12 @@ mwifiex_wmm_get_highest_priolist_ptr(struct mwifiex_adapter *adapter,
if (!tid_ptr->ra_list_curr)
continue;
- spin_lock_irqsave(&tid_ptr->tid_tbl_lock,
- flags);
+ spin_lock_irqsave(&priv_tmp->wmm.
+ ra_list_spinlock, flags);
is_list_empty =
list_empty(&tid_ptr->ra_list);
- spin_unlock_irqrestore(&tid_ptr->tid_tbl_lock,
- flags);
+ spin_unlock_irqrestore(&priv_tmp->wmm.
+ ra_list_spinlock, flags);
if (is_list_empty)
continue;
--
1.7.0.2