2018-06-25 09:43:07

by Ganapathi Bhat

[permalink] [raw]
Subject: [PATCH 1/2] mwifiex: replace rx_pkt_lock by rx_reorder_tbl_lock

At present driver spinlock protects iteration of list
rx_reorder_tbl_ptr with rx_reorder_tbl_lock. To protect the
individual items in this list, it uses rx_pkt_lock. But, we can
use a single rx_reorder_tbl_lock for both purposes. This patch
replaces rx_pkt_lock by rx_reorder_tbl_lock.

Signed-off-by: Ganapathi Bhat <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c | 19 ++++++++++---------
drivers/net/wireless/marvell/mwifiex/init.c | 1 -
drivers/net/wireless/marvell/mwifiex/main.h | 3 ---
3 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
index 7ab44cd..5380fba 100644
--- a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
+++ b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
@@ -118,18 +118,18 @@ static int mwifiex_11n_dispatch_pkt(struct mwifiex_private *priv, void *payload)
tbl->win_size;

for (i = 0; i < pkt_to_send; ++i) {
- spin_lock_irqsave(&priv->rx_pkt_lock, flags);
+ spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
rx_tmp_ptr = NULL;
if (tbl->rx_reorder_ptr[i]) {
rx_tmp_ptr = tbl->rx_reorder_ptr[i];
tbl->rx_reorder_ptr[i] = NULL;
}
- spin_unlock_irqrestore(&priv->rx_pkt_lock, flags);
+ spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
if (rx_tmp_ptr)
mwifiex_11n_dispatch_pkt(priv, rx_tmp_ptr);
}

- spin_lock_irqsave(&priv->rx_pkt_lock, flags);
+ spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
/*
* We don't have a circular buffer, hence use rotation to simulate
* circular buffer
@@ -140,7 +140,7 @@ static int mwifiex_11n_dispatch_pkt(struct mwifiex_private *priv, void *payload)
}

tbl->start_win = start_win;
- spin_unlock_irqrestore(&priv->rx_pkt_lock, flags);
+ spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
}

/*
@@ -160,18 +160,19 @@ static int mwifiex_11n_dispatch_pkt(struct mwifiex_private *priv, void *payload)
unsigned long flags;

for (i = 0; i < tbl->win_size; ++i) {
- spin_lock_irqsave(&priv->rx_pkt_lock, flags);
+ spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
if (!tbl->rx_reorder_ptr[i]) {
- spin_unlock_irqrestore(&priv->rx_pkt_lock, flags);
+ spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock,
+ flags);
break;
}
rx_tmp_ptr = tbl->rx_reorder_ptr[i];
tbl->rx_reorder_ptr[i] = NULL;
- spin_unlock_irqrestore(&priv->rx_pkt_lock, flags);
+ spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
mwifiex_11n_dispatch_pkt(priv, rx_tmp_ptr);
}

- spin_lock_irqsave(&priv->rx_pkt_lock, flags);
+ spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
/*
* We don't have a circular buffer, hence use rotation to simulate
* circular buffer
@@ -184,7 +185,7 @@ static int mwifiex_11n_dispatch_pkt(struct mwifiex_private *priv, void *payload)
}
}
tbl->start_win = (tbl->start_win + i) & (MAX_TID_VALUE - 1);
- spin_unlock_irqrestore(&priv->rx_pkt_lock, flags);
+ spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
}

/*
diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c
index d239e92..dab02d7 100644
--- a/drivers/net/wireless/marvell/mwifiex/init.c
+++ b/drivers/net/wireless/marvell/mwifiex/init.c
@@ -439,7 +439,6 @@ int mwifiex_init_lock_list(struct mwifiex_adapter *adapter)
for (i = 0; i < adapter->priv_num; i++) {
if (adapter->priv[i]) {
priv = adapter->priv[i];
- spin_lock_init(&priv->rx_pkt_lock);
spin_lock_init(&priv->wmm.ra_list_spinlock);
spin_lock_init(&priv->curr_bcn_buf_lock);
spin_lock_init(&priv->sta_list_spinlock);
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 69ac0a2..d2b54be 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -616,9 +616,6 @@ struct mwifiex_private {
struct list_head rx_reorder_tbl_ptr;
/* spin lock for rx_reorder_tbl_ptr queue */
spinlock_t rx_reorder_tbl_lock;
- /* spin lock for Rx packets */
- spinlock_t rx_pkt_lock;
-
#define MWIFIEX_ASSOC_RSP_BUF_SIZE 500
u8 assoc_rsp_buf[MWIFIEX_ASSOC_RSP_BUF_SIZE];
u32 assoc_rsp_size;
--
1.9.1


2018-06-25 10:04:51

by Ganapathi Bhat

[permalink] [raw]
Subject: RE: [PATCH 2/2] mwifiex: restructure rx_reorder_tbl_lock usage

For reference, detailed discussion could be found in below thread:
https://patchwork.kernel.org/patch/10033789/

Regards,
Ganapathi
> -----Original Message-----
> From: Ganapathi Bhat [mailto:[email protected]]
> Sent: Monday, June 25, 2018 3:13 PM
> To: [email protected]
> Cc: Brian Norris; Cathy Luo; Xinming Hu; Zhiyuan Yang; James Cao; Mangesh
> Malusare; Douglas Anderson; Ganapathi Bhat
> Subject: [PATCH 2/2] mwifiex: restructure rx_reorder_tbl_lock usage
>
> Driver must ensure that whenever it holds a pointer to the list
> entry mwifiex_rx_reorder_tbl, it must protect the same with
> rx_reorder_tbl_lock. At present there are many places where
> driver does not ensure this. To cover all cases, spinlocks in
> below funcions are moved out and made sure that the caller will
> hold the spinlock:
> mwifiex_11n_dispatch_pkt_until_start_win()
> mwifiex_11n_scan_and_dispatch()
> mwifiex_del_rx_reorder_entry()
> mwifiex_11n_get_rx_reorder_tbl()
> mwifiex_11n_find_last_seq_num()
>
> Signed-off-by: Ganapathi Bhat <[email protected]>
> ---
> drivers/net/wireless/marvell/mwifiex/11n.c | 5 +-
> .../net/wireless/marvell/mwifiex/11n_rxreorder.c | 80 ++++++++++---------
> ---
> drivers/net/wireless/marvell/mwifiex/uap_txrx.c | 3 +
> 3 files changed, 42 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/11n.c
> b/drivers/net/wireless/marvell/mwifiex/11n.c
> index 5d75c97..e2addd8 100644
> --- a/drivers/net/wireless/marvell/mwifiex/11n.c
> +++ b/drivers/net/wireless/marvell/mwifiex/11n.c
> @@ -696,10 +696,11 @@ void mwifiex_11n_delba(struct mwifiex_private
> *priv, int tid)
> "Send delba to tid=%d, %pM\n",
> tid, rx_reor_tbl_ptr->ta);
> mwifiex_send_delba(priv, tid, rx_reor_tbl_ptr->ta,
> 0);
> - goto exit;
> + spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock,
> + flags);
> + return;
> }
> }
> -exit:
> spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
> }
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> index 5380fba..e0d0998 100644
> --- a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> +++ b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> @@ -111,25 +111,21 @@ static int mwifiex_11n_dispatch_pkt(struct
> mwifiex_private *priv, void *payload)
> {
> int pkt_to_send, i;
> void *rx_tmp_ptr;
> - unsigned long flags;
>
> pkt_to_send = (start_win > tbl->start_win) ?
> min((start_win - tbl->start_win), tbl->win_size) :
> tbl->win_size;
>
> for (i = 0; i < pkt_to_send; ++i) {
> - spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
> rx_tmp_ptr = NULL;
> if (tbl->rx_reorder_ptr[i]) {
> rx_tmp_ptr = tbl->rx_reorder_ptr[i];
> tbl->rx_reorder_ptr[i] = NULL;
> }
> - spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
> if (rx_tmp_ptr)
> mwifiex_11n_dispatch_pkt(priv, rx_tmp_ptr);
> }
>
> - spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
> /*
> * We don't have a circular buffer, hence use rotation to simulate
> * circular buffer
> @@ -140,7 +136,6 @@ static int mwifiex_11n_dispatch_pkt(struct
> mwifiex_private *priv, void *payload)
> }
>
> tbl->start_win = start_win;
> - spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
> }
>
> /*
> @@ -160,7 +155,6 @@ static int mwifiex_11n_dispatch_pkt(struct
> mwifiex_private *priv, void *payload)
> unsigned long flags;
>
> for (i = 0; i < tbl->win_size; ++i) {
> - spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
> if (!tbl->rx_reorder_ptr[i]) {
> spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock,
> flags);
> @@ -168,11 +162,9 @@ static int mwifiex_11n_dispatch_pkt(struct
> mwifiex_private *priv, void *payload)
> }
> rx_tmp_ptr = tbl->rx_reorder_ptr[i];
> tbl->rx_reorder_ptr[i] = NULL;
> - spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
> mwifiex_11n_dispatch_pkt(priv, rx_tmp_ptr);
> }
>
> - spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
> /*
> * We don't have a circular buffer, hence use rotation to simulate
> * circular buffer
> @@ -185,7 +177,6 @@ static int mwifiex_11n_dispatch_pkt(struct
> mwifiex_private *priv, void *payload)
> }
> }
> tbl->start_win = (tbl->start_win + i) & (MAX_TID_VALUE - 1);
> - spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
> }
>
> /*
> @@ -218,11 +209,7 @@ static int mwifiex_11n_dispatch_pkt(struct
> mwifiex_private *priv, void *payload)
>
> del_timer_sync(&tbl->timer_context.timer);
> tbl->timer_context.timer_is_set = false;
> -
> - spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
> list_del(&tbl->list);
> - spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
> -
> kfree(tbl->rx_reorder_ptr);
> kfree(tbl);
>
> @@ -240,17 +227,10 @@ struct mwifiex_rx_reorder_tbl *
> mwifiex_11n_get_rx_reorder_tbl(struct mwifiex_private *priv, int tid, u8
> *ta)
> {
> struct mwifiex_rx_reorder_tbl *tbl;
> - unsigned long flags;
>
> - spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
> - list_for_each_entry(tbl, &priv->rx_reorder_tbl_ptr, list) {
> - if (!memcmp(tbl->ta, ta, ETH_ALEN) && tbl->tid == tid) {
> - spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock,
> - flags);
> + list_for_each_entry(tbl, &priv->rx_reorder_tbl_ptr, list)
> + if (!memcmp(tbl->ta, ta, ETH_ALEN) && tbl->tid == tid)
> return tbl;
> - }
> - }
> - spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
>
> return NULL;
> }
> @@ -267,14 +247,9 @@ void mwifiex_11n_del_rx_reorder_tbl_by_ta(struct
> mwifiex_private *priv, u8 *ta)
> return;
>
> spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
> - list_for_each_entry_safe(tbl, tmp, &priv->rx_reorder_tbl_ptr, list) {
> - if (!memcmp(tbl->ta, ta, ETH_ALEN)) {
> - spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock,
> - flags);
> + list_for_each_entry_safe(tbl, tmp, &priv->rx_reorder_tbl_ptr, list)
> + if (!memcmp(tbl->ta, ta, ETH_ALEN))
> mwifiex_del_rx_reorder_entry(priv, tbl);
> - spin_lock_irqsave(&priv->rx_reorder_tbl_lock,
> flags);
> - }
> - }
> spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
>
> return;
> @@ -288,19 +263,11 @@ void
> mwifiex_11n_del_rx_reorder_tbl_by_ta(struct mwifiex_private *priv, u8
> *ta)
> mwifiex_11n_find_last_seq_num(struct reorder_tmr_cnxt *ctx)
> {
> struct mwifiex_rx_reorder_tbl *rx_reorder_tbl_ptr = ctx->ptr;
> - struct mwifiex_private *priv = ctx->priv;
> - unsigned long flags;
> int i;
>
> - spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
> - for (i = rx_reorder_tbl_ptr->win_size - 1; i >= 0; --i) {
> - if (rx_reorder_tbl_ptr->rx_reorder_ptr[i]) {
> - spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock,
> - flags);
> + for (i = rx_reorder_tbl_ptr->win_size - 1; i >= 0; --i)
> + if (rx_reorder_tbl_ptr->rx_reorder_ptr[i])
> return i;
> - }
> - }
> - spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
>
> return -1;
> }
> @@ -318,17 +285,22 @@ void
> mwifiex_11n_del_rx_reorder_tbl_by_ta(struct mwifiex_private *priv, u8
> *ta)
> struct reorder_tmr_cnxt *ctx =
> from_timer(ctx, t, timer);
> int start_win, seq_num;
> + unsigned long flags;
>
> ctx->timer_is_set = false;
> + spin_lock_irqsave(&ctx->priv->rx_reorder_tbl_lock, flags);
> seq_num = mwifiex_11n_find_last_seq_num(ctx);
>
> - if (seq_num < 0)
> + if (seq_num < 0) {
> + spin_unlock_irqrestore(&ctx->priv->rx_reorder_tbl_lock,
> flags);
> return;
> + }
>
> mwifiex_dbg(ctx->priv->adapter, INFO, "info: flush data %d\n",
> seq_num);
> start_win = (ctx->ptr->start_win + seq_num + 1) &
> (MAX_TID_VALUE - 1);
> mwifiex_11n_dispatch_pkt_until_start_win(ctx->priv, ctx->ptr,
> start_win);
> + spin_unlock_irqrestore(&ctx->priv->rx_reorder_tbl_lock, flags);
> }
>
> /*
> @@ -355,11 +327,14 @@ void
> mwifiex_11n_del_rx_reorder_tbl_by_ta(struct mwifiex_private *priv, u8
> *ta)
> * If we get a TID, ta pair which is already present dispatch all the
> * the packets and move the window size until the ssn
> */
> + spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
> tbl = mwifiex_11n_get_rx_reorder_tbl(priv, tid, ta);
> if (tbl) {
> mwifiex_11n_dispatch_pkt_until_start_win(priv, tbl,
> seq_num);
> + spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
> return;
> }
> + spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
> /* if !tbl then create one */
> new_node = kzalloc(sizeof(struct mwifiex_rx_reorder_tbl),
> GFP_KERNEL);
> if (!new_node)
> @@ -570,16 +545,20 @@ int mwifiex_11n_rx_reorder_pkt(struct
> mwifiex_private *priv,
> int prev_start_win, start_win, end_win, win_size;
> u16 pkt_index;
> bool init_window_shift = false;
> + unsigned long flags;
> int ret = 0;
>
> + spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
> tbl = mwifiex_11n_get_rx_reorder_tbl(priv, tid, ta);
> if (!tbl) {
> + spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
> if (pkt_type != PKT_TYPE_BAR)
> mwifiex_11n_dispatch_pkt(priv, payload);
> return ret;
> }
>
> if ((pkt_type == PKT_TYPE_AMSDU) && !tbl->amsdu) {
> + spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
> mwifiex_11n_dispatch_pkt(priv, payload);
> return ret;
> }
> @@ -666,6 +645,8 @@ int mwifiex_11n_rx_reorder_pkt(struct
> mwifiex_private *priv,
> if (!tbl->timer_context.timer_is_set ||
> prev_start_win != tbl->start_win)
> mwifiex_11n_rxreorder_timer_restart(tbl);
> +
> + spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
> return ret;
> }
>
> @@ -694,14 +675,18 @@ int mwifiex_11n_rx_reorder_pkt(struct
> mwifiex_private *priv,
> peer_mac, tid, initiator);
>
> if (cleanup_rx_reorder_tbl) {
> + spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
> tbl = mwifiex_11n_get_rx_reorder_tbl(priv, tid,
> peer_mac);
> if (!tbl) {
> + spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock,
> + flags);
> mwifiex_dbg(priv->adapter, EVENT,
> "event: TID, TA not found in table\n");
> return;
> }
> mwifiex_del_rx_reorder_entry(priv, tbl);
> + spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
> } else {
> ptx_tbl = mwifiex_get_ba_tbl(priv, tid, peer_mac);
> if (!ptx_tbl) {
> @@ -735,6 +720,7 @@ int mwifiex_ret_11n_addba_resp(struct
> mwifiex_private *priv,
> int tid, win_size;
> struct mwifiex_rx_reorder_tbl *tbl;
> uint16_t block_ack_param_set;
> + unsigned long flags;
>
> block_ack_param_set = le16_to_cpu(add_ba_rsp-
> >block_ack_param_set);
>
> @@ -748,17 +734,20 @@ int mwifiex_ret_11n_addba_resp(struct
> mwifiex_private *priv,
> mwifiex_dbg(priv->adapter, ERROR, "ADDBA RSP: failed
> %pM tid=%d)\n",
> add_ba_rsp->peer_mac_addr, tid);
>
> + spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
> tbl = mwifiex_11n_get_rx_reorder_tbl(priv, tid,
> add_ba_rsp-
> >peer_mac_addr);
> if (tbl)
> mwifiex_del_rx_reorder_entry(priv, tbl);
>
> + spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
> return 0;
> }
>
> win_size = (block_ack_param_set &
> IEEE80211_ADDBA_PARAM_BUF_SIZE_MASK)
> >> BLOCKACKPARAM_WINSIZE_POS;
>
> + spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
> tbl = mwifiex_11n_get_rx_reorder_tbl(priv, tid,
> add_ba_rsp->peer_mac_addr);
> if (tbl) {
> @@ -769,6 +758,7 @@ int mwifiex_ret_11n_addba_resp(struct
> mwifiex_private *priv,
> else
> tbl->amsdu = false;
> }
> + spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
>
> mwifiex_dbg(priv->adapter, CMD,
> "cmd: ADDBA RSP: %pM tid=%d ssn=%d win_size=%d\n",
> @@ -808,11 +798,8 @@ void mwifiex_11n_cleanup_reorder_tbl(struct
> mwifiex_private *priv)
>
> spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
> list_for_each_entry_safe(del_tbl_ptr, tmp_node,
> - &priv->rx_reorder_tbl_ptr, list) {
> - spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
> + &priv->rx_reorder_tbl_ptr, list)
> mwifiex_del_rx_reorder_entry(priv, del_tbl_ptr);
> - spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
> - }
> INIT_LIST_HEAD(&priv->rx_reorder_tbl_ptr);
> spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
>
> @@ -936,6 +923,7 @@ void mwifiex_11n_rxba_sync_event(struct
> mwifiex_private *priv,
> int tlv_buf_left = len;
> int ret;
> u8 *tmp;
> + unsigned long flags;
>
> mwifiex_dbg_dump(priv->adapter, EVT_D, "RXBA_SYNC event:",
> event_buf, len);
> @@ -955,14 +943,18 @@ void mwifiex_11n_rxba_sync_event(struct
> mwifiex_private *priv,
> tlv_rxba->mac, tlv_rxba->tid, tlv_seq_num,
> tlv_bitmap_len);
>
> + spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
> rx_reor_tbl_ptr =
> mwifiex_11n_get_rx_reorder_tbl(priv, tlv_rxba->tid,
> tlv_rxba->mac);
> if (!rx_reor_tbl_ptr) {
> + spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock,
> + flags);
> mwifiex_dbg(priv->adapter, ERROR,
> "Can not find rx_reorder_tbl!");
> return;
> }
> + spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
>
> for (i = 0; i < tlv_bitmap_len; i++) {
> for (j = 0 ; j < 8; j++) {
> diff --git a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
> b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
> index 1e6a62c..99cfaee 100644
> --- a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
> +++ b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
> @@ -421,12 +421,15 @@ int mwifiex_process_uap_rx_packet(struct
> mwifiex_private *priv,
> spin_unlock_irqrestore(&priv->sta_list_spinlock, flags);
> }
>
> + spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
> if (!priv->ap_11n_enabled ||
> (!mwifiex_11n_get_rx_reorder_tbl(priv, uap_rx_pd->priority, ta)
> &&
> (le16_to_cpu(uap_rx_pd->rx_pkt_type) != PKT_TYPE_AMSDU))) {
> ret = mwifiex_handle_uap_rx_forward(priv, skb);
> + spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
> return ret;
> }
> + spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
>
> /* Reorder and send to kernel */
> pkt_type = (u8)le16_to_cpu(uap_rx_pd->rx_pkt_type);
> --
> 1.9.1

2018-06-25 09:43:08

by Ganapathi Bhat

[permalink] [raw]
Subject: [PATCH 2/2] mwifiex: restructure rx_reorder_tbl_lock usage

Driver must ensure that whenever it holds a pointer to the list
entry mwifiex_rx_reorder_tbl, it must protect the same with
rx_reorder_tbl_lock. At present there are many places where
driver does not ensure this. To cover all cases, spinlocks in
below funcions are moved out and made sure that the caller will
hold the spinlock:
mwifiex_11n_dispatch_pkt_until_start_win()
mwifiex_11n_scan_and_dispatch()
mwifiex_del_rx_reorder_entry()
mwifiex_11n_get_rx_reorder_tbl()
mwifiex_11n_find_last_seq_num()

Signed-off-by: Ganapathi Bhat <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/11n.c | 5 +-
.../net/wireless/marvell/mwifiex/11n_rxreorder.c | 80 ++++++++++------------
drivers/net/wireless/marvell/mwifiex/uap_txrx.c | 3 +
3 files changed, 42 insertions(+), 46 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/11n.c b/drivers/net/wireless/marvell/mwifiex/11n.c
index 5d75c97..e2addd8 100644
--- a/drivers/net/wireless/marvell/mwifiex/11n.c
+++ b/drivers/net/wireless/marvell/mwifiex/11n.c
@@ -696,10 +696,11 @@ void mwifiex_11n_delba(struct mwifiex_private *priv, int tid)
"Send delba to tid=%d, %pM\n",
tid, rx_reor_tbl_ptr->ta);
mwifiex_send_delba(priv, tid, rx_reor_tbl_ptr->ta, 0);
- goto exit;
+ spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock,
+ flags);
+ return;
}
}
-exit:
spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
}

diff --git a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
index 5380fba..e0d0998 100644
--- a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
+++ b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
@@ -111,25 +111,21 @@ static int mwifiex_11n_dispatch_pkt(struct mwifiex_private *priv, void *payload)
{
int pkt_to_send, i;
void *rx_tmp_ptr;
- unsigned long flags;

pkt_to_send = (start_win > tbl->start_win) ?
min((start_win - tbl->start_win), tbl->win_size) :
tbl->win_size;

for (i = 0; i < pkt_to_send; ++i) {
- spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
rx_tmp_ptr = NULL;
if (tbl->rx_reorder_ptr[i]) {
rx_tmp_ptr = tbl->rx_reorder_ptr[i];
tbl->rx_reorder_ptr[i] = NULL;
}
- spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
if (rx_tmp_ptr)
mwifiex_11n_dispatch_pkt(priv, rx_tmp_ptr);
}

- spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
/*
* We don't have a circular buffer, hence use rotation to simulate
* circular buffer
@@ -140,7 +136,6 @@ static int mwifiex_11n_dispatch_pkt(struct mwifiex_private *priv, void *payload)
}

tbl->start_win = start_win;
- spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
}

/*
@@ -160,7 +155,6 @@ static int mwifiex_11n_dispatch_pkt(struct mwifiex_private *priv, void *payload)
unsigned long flags;

for (i = 0; i < tbl->win_size; ++i) {
- spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
if (!tbl->rx_reorder_ptr[i]) {
spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock,
flags);
@@ -168,11 +162,9 @@ static int mwifiex_11n_dispatch_pkt(struct mwifiex_private *priv, void *payload)
}
rx_tmp_ptr = tbl->rx_reorder_ptr[i];
tbl->rx_reorder_ptr[i] = NULL;
- spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
mwifiex_11n_dispatch_pkt(priv, rx_tmp_ptr);
}

- spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
/*
* We don't have a circular buffer, hence use rotation to simulate
* circular buffer
@@ -185,7 +177,6 @@ static int mwifiex_11n_dispatch_pkt(struct mwifiex_private *priv, void *payload)
}
}
tbl->start_win = (tbl->start_win + i) & (MAX_TID_VALUE - 1);
- spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
}

/*
@@ -218,11 +209,7 @@ static int mwifiex_11n_dispatch_pkt(struct mwifiex_private *priv, void *payload)

del_timer_sync(&tbl->timer_context.timer);
tbl->timer_context.timer_is_set = false;
-
- spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
list_del(&tbl->list);
- spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
-
kfree(tbl->rx_reorder_ptr);
kfree(tbl);

@@ -240,17 +227,10 @@ struct mwifiex_rx_reorder_tbl *
mwifiex_11n_get_rx_reorder_tbl(struct mwifiex_private *priv, int tid, u8 *ta)
{
struct mwifiex_rx_reorder_tbl *tbl;
- unsigned long flags;

- spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
- list_for_each_entry(tbl, &priv->rx_reorder_tbl_ptr, list) {
- if (!memcmp(tbl->ta, ta, ETH_ALEN) && tbl->tid == tid) {
- spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock,
- flags);
+ list_for_each_entry(tbl, &priv->rx_reorder_tbl_ptr, list)
+ if (!memcmp(tbl->ta, ta, ETH_ALEN) && tbl->tid == tid)
return tbl;
- }
- }
- spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);

return NULL;
}
@@ -267,14 +247,9 @@ void mwifiex_11n_del_rx_reorder_tbl_by_ta(struct mwifiex_private *priv, u8 *ta)
return;

spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
- list_for_each_entry_safe(tbl, tmp, &priv->rx_reorder_tbl_ptr, list) {
- if (!memcmp(tbl->ta, ta, ETH_ALEN)) {
- spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock,
- flags);
+ list_for_each_entry_safe(tbl, tmp, &priv->rx_reorder_tbl_ptr, list)
+ if (!memcmp(tbl->ta, ta, ETH_ALEN))
mwifiex_del_rx_reorder_entry(priv, tbl);
- spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
- }
- }
spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);

return;
@@ -288,19 +263,11 @@ void mwifiex_11n_del_rx_reorder_tbl_by_ta(struct mwifiex_private *priv, u8 *ta)
mwifiex_11n_find_last_seq_num(struct reorder_tmr_cnxt *ctx)
{
struct mwifiex_rx_reorder_tbl *rx_reorder_tbl_ptr = ctx->ptr;
- struct mwifiex_private *priv = ctx->priv;
- unsigned long flags;
int i;

- spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
- for (i = rx_reorder_tbl_ptr->win_size - 1; i >= 0; --i) {
- if (rx_reorder_tbl_ptr->rx_reorder_ptr[i]) {
- spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock,
- flags);
+ for (i = rx_reorder_tbl_ptr->win_size - 1; i >= 0; --i)
+ if (rx_reorder_tbl_ptr->rx_reorder_ptr[i])
return i;
- }
- }
- spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);

return -1;
}
@@ -318,17 +285,22 @@ void mwifiex_11n_del_rx_reorder_tbl_by_ta(struct mwifiex_private *priv, u8 *ta)
struct reorder_tmr_cnxt *ctx =
from_timer(ctx, t, timer);
int start_win, seq_num;
+ unsigned long flags;

ctx->timer_is_set = false;
+ spin_lock_irqsave(&ctx->priv->rx_reorder_tbl_lock, flags);
seq_num = mwifiex_11n_find_last_seq_num(ctx);

- if (seq_num < 0)
+ if (seq_num < 0) {
+ spin_unlock_irqrestore(&ctx->priv->rx_reorder_tbl_lock, flags);
return;
+ }

mwifiex_dbg(ctx->priv->adapter, INFO, "info: flush data %d\n", seq_num);
start_win = (ctx->ptr->start_win + seq_num + 1) & (MAX_TID_VALUE - 1);
mwifiex_11n_dispatch_pkt_until_start_win(ctx->priv, ctx->ptr,
start_win);
+ spin_unlock_irqrestore(&ctx->priv->rx_reorder_tbl_lock, flags);
}

/*
@@ -355,11 +327,14 @@ void mwifiex_11n_del_rx_reorder_tbl_by_ta(struct mwifiex_private *priv, u8 *ta)
* If we get a TID, ta pair which is already present dispatch all the
* the packets and move the window size until the ssn
*/
+ spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
tbl = mwifiex_11n_get_rx_reorder_tbl(priv, tid, ta);
if (tbl) {
mwifiex_11n_dispatch_pkt_until_start_win(priv, tbl, seq_num);
+ spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
return;
}
+ spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
/* if !tbl then create one */
new_node = kzalloc(sizeof(struct mwifiex_rx_reorder_tbl), GFP_KERNEL);
if (!new_node)
@@ -570,16 +545,20 @@ int mwifiex_11n_rx_reorder_pkt(struct mwifiex_private *priv,
int prev_start_win, start_win, end_win, win_size;
u16 pkt_index;
bool init_window_shift = false;
+ unsigned long flags;
int ret = 0;

+ spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
tbl = mwifiex_11n_get_rx_reorder_tbl(priv, tid, ta);
if (!tbl) {
+ spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
if (pkt_type != PKT_TYPE_BAR)
mwifiex_11n_dispatch_pkt(priv, payload);
return ret;
}

if ((pkt_type == PKT_TYPE_AMSDU) && !tbl->amsdu) {
+ spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
mwifiex_11n_dispatch_pkt(priv, payload);
return ret;
}
@@ -666,6 +645,8 @@ int mwifiex_11n_rx_reorder_pkt(struct mwifiex_private *priv,
if (!tbl->timer_context.timer_is_set ||
prev_start_win != tbl->start_win)
mwifiex_11n_rxreorder_timer_restart(tbl);
+
+ spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
return ret;
}

@@ -694,14 +675,18 @@ int mwifiex_11n_rx_reorder_pkt(struct mwifiex_private *priv,
peer_mac, tid, initiator);

if (cleanup_rx_reorder_tbl) {
+ spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
tbl = mwifiex_11n_get_rx_reorder_tbl(priv, tid,
peer_mac);
if (!tbl) {
+ spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock,
+ flags);
mwifiex_dbg(priv->adapter, EVENT,
"event: TID, TA not found in table\n");
return;
}
mwifiex_del_rx_reorder_entry(priv, tbl);
+ spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
} else {
ptx_tbl = mwifiex_get_ba_tbl(priv, tid, peer_mac);
if (!ptx_tbl) {
@@ -735,6 +720,7 @@ int mwifiex_ret_11n_addba_resp(struct mwifiex_private *priv,
int tid, win_size;
struct mwifiex_rx_reorder_tbl *tbl;
uint16_t block_ack_param_set;
+ unsigned long flags;

block_ack_param_set = le16_to_cpu(add_ba_rsp->block_ack_param_set);

@@ -748,17 +734,20 @@ int mwifiex_ret_11n_addba_resp(struct mwifiex_private *priv,
mwifiex_dbg(priv->adapter, ERROR, "ADDBA RSP: failed %pM tid=%d)\n",
add_ba_rsp->peer_mac_addr, tid);

+ spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
tbl = mwifiex_11n_get_rx_reorder_tbl(priv, tid,
add_ba_rsp->peer_mac_addr);
if (tbl)
mwifiex_del_rx_reorder_entry(priv, tbl);

+ spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
return 0;
}

win_size = (block_ack_param_set & IEEE80211_ADDBA_PARAM_BUF_SIZE_MASK)
>> BLOCKACKPARAM_WINSIZE_POS;

+ spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
tbl = mwifiex_11n_get_rx_reorder_tbl(priv, tid,
add_ba_rsp->peer_mac_addr);
if (tbl) {
@@ -769,6 +758,7 @@ int mwifiex_ret_11n_addba_resp(struct mwifiex_private *priv,
else
tbl->amsdu = false;
}
+ spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);

mwifiex_dbg(priv->adapter, CMD,
"cmd: ADDBA RSP: %pM tid=%d ssn=%d win_size=%d\n",
@@ -808,11 +798,8 @@ void mwifiex_11n_cleanup_reorder_tbl(struct mwifiex_private *priv)

spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
list_for_each_entry_safe(del_tbl_ptr, tmp_node,
- &priv->rx_reorder_tbl_ptr, list) {
- spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
+ &priv->rx_reorder_tbl_ptr, list)
mwifiex_del_rx_reorder_entry(priv, del_tbl_ptr);
- spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
- }
INIT_LIST_HEAD(&priv->rx_reorder_tbl_ptr);
spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);

@@ -936,6 +923,7 @@ void mwifiex_11n_rxba_sync_event(struct mwifiex_private *priv,
int tlv_buf_left = len;
int ret;
u8 *tmp;
+ unsigned long flags;

mwifiex_dbg_dump(priv->adapter, EVT_D, "RXBA_SYNC event:",
event_buf, len);
@@ -955,14 +943,18 @@ void mwifiex_11n_rxba_sync_event(struct mwifiex_private *priv,
tlv_rxba->mac, tlv_rxba->tid, tlv_seq_num,
tlv_bitmap_len);

+ spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
rx_reor_tbl_ptr =
mwifiex_11n_get_rx_reorder_tbl(priv, tlv_rxba->tid,
tlv_rxba->mac);
if (!rx_reor_tbl_ptr) {
+ spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock,
+ flags);
mwifiex_dbg(priv->adapter, ERROR,
"Can not find rx_reorder_tbl!");
return;
}
+ spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);

for (i = 0; i < tlv_bitmap_len; i++) {
for (j = 0 ; j < 8; j++) {
diff --git a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
index 1e6a62c..99cfaee 100644
--- a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
+++ b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
@@ -421,12 +421,15 @@ int mwifiex_process_uap_rx_packet(struct mwifiex_private *priv,
spin_unlock_irqrestore(&priv->sta_list_spinlock, flags);
}

+ spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
if (!priv->ap_11n_enabled ||
(!mwifiex_11n_get_rx_reorder_tbl(priv, uap_rx_pd->priority, ta) &&
(le16_to_cpu(uap_rx_pd->rx_pkt_type) != PKT_TYPE_AMSDU))) {
ret = mwifiex_handle_uap_rx_forward(priv, skb);
+ spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
return ret;
}
+ spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);

/* Reorder and send to kernel */
pkt_type = (u8)le16_to_cpu(uap_rx_pd->rx_pkt_type);
--
1.9.1

2018-06-27 06:18:40

by Ganapathi Bhat

[permalink] [raw]
Subject: RE: [EXT] Re: [PATCH 2/2] mwifiex: restructure rx_reorder_tbl_lock usage

Hi,

> drivers/net//wireless/marvell/mwifiex/11n_rxreorder.c:155:16: note:
> 'flags' was declared here
> unsigned long flags;
> ^~~~~
We did fix the warning and have upstreamed v2 of the patch.

Thanks,
Ganapathi

2018-06-25 10:04:18

by Ganapathi Bhat

[permalink] [raw]
Subject: RE: [PATCH 1/2] mwifiex: replace rx_pkt_lock by rx_reorder_tbl_lock

For reference, detailed discussion could be found in below thread:
https://patchwork.kernel.org/patch/10033789/

Regards,
Ganapathi
> -----Original Message-----
> From: Ganapathi Bhat [mailto:[email protected]]
> Sent: Monday, June 25, 2018 3:13 PM
> To: [email protected]
> Cc: Brian Norris; Cathy Luo; Xinming Hu; Zhiyuan Yang; James Cao; Mangesh
> Malusare; Douglas Anderson; Ganapathi Bhat
> Subject: [PATCH 1/2] mwifiex: replace rx_pkt_lock by rx_reorder_tbl_lock
>
> At present driver spinlock protects iteration of list
> rx_reorder_tbl_ptr with rx_reorder_tbl_lock. To protect the
> individual items in this list, it uses rx_pkt_lock. But, we can
> use a single rx_reorder_tbl_lock for both purposes. This patch
> replaces rx_pkt_lock by rx_reorder_tbl_lock.
>
> Signed-off-by: Ganapathi Bhat <[email protected]>
> ---
> drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c | 19 ++++++++++----
> -----
> drivers/net/wireless/marvell/mwifiex/init.c | 1 -
> drivers/net/wireless/marvell/mwifiex/main.h | 3 ---
> 3 files changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> index 7ab44cd..5380fba 100644
> --- a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> +++ b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> @@ -118,18 +118,18 @@ static int mwifiex_11n_dispatch_pkt(struct
> mwifiex_private *priv, void *payload)
> tbl->win_size;
>
> for (i = 0; i < pkt_to_send; ++i) {
> - spin_lock_irqsave(&priv->rx_pkt_lock, flags);
> + spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
> rx_tmp_ptr = NULL;
> if (tbl->rx_reorder_ptr[i]) {
> rx_tmp_ptr = tbl->rx_reorder_ptr[i];
> tbl->rx_reorder_ptr[i] = NULL;
> }
> - spin_unlock_irqrestore(&priv->rx_pkt_lock, flags);
> + spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
> if (rx_tmp_ptr)
> mwifiex_11n_dispatch_pkt(priv, rx_tmp_ptr);
> }
>
> - spin_lock_irqsave(&priv->rx_pkt_lock, flags);
> + spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
> /*
> * We don't have a circular buffer, hence use rotation to simulate
> * circular buffer
> @@ -140,7 +140,7 @@ static int mwifiex_11n_dispatch_pkt(struct
> mwifiex_private *priv, void *payload)
> }
>
> tbl->start_win = start_win;
> - spin_unlock_irqrestore(&priv->rx_pkt_lock, flags);
> + spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
> }
>
> /*
> @@ -160,18 +160,19 @@ static int mwifiex_11n_dispatch_pkt(struct
> mwifiex_private *priv, void *payload)
> unsigned long flags;
>
> for (i = 0; i < tbl->win_size; ++i) {
> - spin_lock_irqsave(&priv->rx_pkt_lock, flags);
> + spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
> if (!tbl->rx_reorder_ptr[i]) {
> - spin_unlock_irqrestore(&priv->rx_pkt_lock, flags);
> + spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock,
> + flags);
> break;
> }
> rx_tmp_ptr = tbl->rx_reorder_ptr[i];
> tbl->rx_reorder_ptr[i] = NULL;
> - spin_unlock_irqrestore(&priv->rx_pkt_lock, flags);
> + spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
> mwifiex_11n_dispatch_pkt(priv, rx_tmp_ptr);
> }
>
> - spin_lock_irqsave(&priv->rx_pkt_lock, flags);
> + spin_lock_irqsave(&priv->rx_reorder_tbl_lock, flags);
> /*
> * We don't have a circular buffer, hence use rotation to simulate
> * circular buffer
> @@ -184,7 +185,7 @@ static int mwifiex_11n_dispatch_pkt(struct
> mwifiex_private *priv, void *payload)
> }
> }
> tbl->start_win = (tbl->start_win + i) & (MAX_TID_VALUE - 1);
> - spin_unlock_irqrestore(&priv->rx_pkt_lock, flags);
> + spin_unlock_irqrestore(&priv->rx_reorder_tbl_lock, flags);
> }
>
> /*
> diff --git a/drivers/net/wireless/marvell/mwifiex/init.c
> b/drivers/net/wireless/marvell/mwifiex/init.c
> index d239e92..dab02d7 100644
> --- a/drivers/net/wireless/marvell/mwifiex/init.c
> +++ b/drivers/net/wireless/marvell/mwifiex/init.c
> @@ -439,7 +439,6 @@ int mwifiex_init_lock_list(struct mwifiex_adapter
> *adapter)
> for (i = 0; i < adapter->priv_num; i++) {
> if (adapter->priv[i]) {
> priv = adapter->priv[i];
> - spin_lock_init(&priv->rx_pkt_lock);
> spin_lock_init(&priv->wmm.ra_list_spinlock);
> spin_lock_init(&priv->curr_bcn_buf_lock);
> spin_lock_init(&priv->sta_list_spinlock);
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h
> b/drivers/net/wireless/marvell/mwifiex/main.h
> index 69ac0a2..d2b54be 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.h
> +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> @@ -616,9 +616,6 @@ struct mwifiex_private {
> struct list_head rx_reorder_tbl_ptr;
> /* spin lock for rx_reorder_tbl_ptr queue */
> spinlock_t rx_reorder_tbl_lock;
> - /* spin lock for Rx packets */
> - spinlock_t rx_pkt_lock;
> -
> #define MWIFIEX_ASSOC_RSP_BUF_SIZE 500
> u8 assoc_rsp_buf[MWIFIEX_ASSOC_RSP_BUF_SIZE];
> u32 assoc_rsp_size;
> --
> 1.9.1

2018-06-26 08:29:23

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH 2/2] mwifiex: restructure rx_reorder_tbl_lock usage

Hi Ganapathi,

I love your patch! Perhaps something to improve:

[auto build test WARNING on wireless-drivers-next/master]
[also build test WARNING on v4.18-rc2 next-20180625]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Ganapathi-Bhat/mwifiex-replace-rx_pkt_lock-by-rx_reorder_tbl_lock/20180625-175146
base: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
config: arm-sama5_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=arm

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

In file included from include/linux/irqflags.h:16:0,
from arch/arm/include/asm/bitops.h:28,
from include/linux/bitops.h:38,
from include/linux/kernel.h:11,
from include/linux/list.h:9,
from include/linux/wait.h:7,
from drivers/net//wireless/marvell/mwifiex/decl.h:26,
from drivers/net//wireless/marvell/mwifiex/11n_rxreorder.c:20:
drivers/net//wireless/marvell/mwifiex/11n_rxreorder.c: In function 'mwifiex_11n_rx_reorder_pkt':
>> arch/arm/include/asm/irqflags.h:171:2: warning: 'flags' may be used uninitialized in this function [-Wmaybe-uninitialized]
asm volatile(
^~~
drivers/net//wireless/marvell/mwifiex/11n_rxreorder.c:155:16: note: 'flags' was declared here
unsigned long flags;
^~~~~
--
In file included from include/linux/irqflags.h:16:0,
from arch/arm/include/asm/bitops.h:28,
from include/linux/bitops.h:38,
from include/linux/kernel.h:11,
from include/linux/list.h:9,
from include/linux/wait.h:7,
from drivers/net/wireless/marvell/mwifiex/decl.h:26,
from drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c:20:
drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c: In function 'mwifiex_11n_rx_reorder_pkt':
>> arch/arm/include/asm/irqflags.h:171:2: warning: 'flags' may be used uninitialized in this function [-Wmaybe-uninitialized]
asm volatile(
^~~
drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c:155:16: note: 'flags' was declared here
unsigned long flags;
^~~~~

vim +/flags +171 arch/arm/include/asm/irqflags.h

7ad1bcb2 include/asm-arm/irqflags.h Russell King 2006-08-27 164
7ad1bcb2 include/asm-arm/irqflags.h Russell King 2006-08-27 165 /*
7ad1bcb2 include/asm-arm/irqflags.h Russell King 2006-08-27 166 * restore saved IRQ & FIQ state
7ad1bcb2 include/asm-arm/irqflags.h Russell King 2006-08-27 167 */
6fb18ac9 arch/arm/include/asm/irqflags.h Daniel Thompson 2015-06-10 168 #define arch_local_irq_restore arch_local_irq_restore
df9ee292 arch/arm/include/asm/irqflags.h David Howells 2010-10-07 169 static inline void arch_local_irq_restore(unsigned long flags)
df9ee292 arch/arm/include/asm/irqflags.h David Howells 2010-10-07 170 {
df9ee292 arch/arm/include/asm/irqflags.h David Howells 2010-10-07 @171 asm volatile(
55bdd694 arch/arm/include/asm/irqflags.h Catalin Marinas 2010-05-21 172 " msr " IRQMASK_REG_NAME_W ", %0 @ local_irq_restore"
df9ee292 arch/arm/include/asm/irqflags.h David Howells 2010-10-07 173 :
df9ee292 arch/arm/include/asm/irqflags.h David Howells 2010-10-07 174 : "r" (flags)
df9ee292 arch/arm/include/asm/irqflags.h David Howells 2010-10-07 175 : "memory", "cc");
df9ee292 arch/arm/include/asm/irqflags.h David Howells 2010-10-07 176 }
df9ee292 arch/arm/include/asm/irqflags.h David Howells 2010-10-07 177

:::::: The code at line 171 was first introduced by commit
:::::: df9ee29270c11dba7d0fe0b83ce47a4d8e8d2101 Fix IRQ flag handling naming

:::::: TO: David Howells <[email protected]>
:::::: CC: David Howells <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation


Attachments:
(No filename) (4.44 kB)
.config.gz (24.49 kB)
Download all attachments