Subject: [PATCH V2 1/6] ath6kl: Make sure to delete rx aggregation timer in aggr_reset_state()

The timer which is used to flush rx aggregation frames needs to
be disabled when resetting the aggregation state. This is found
in code review.

Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
---
drivers/net/wireless/ath/ath6kl/txrx.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/txrx.c b/drivers/net/wireless/ath/ath6kl/txrx.c
index dd63371..cb7421a 100644
--- a/drivers/net/wireless/ath/ath6kl/txrx.c
+++ b/drivers/net/wireless/ath/ath6kl/txrx.c
@@ -1685,6 +1685,11 @@ void aggr_reset_state(struct aggr_info *aggr_info)
{
u8 tid;

+ if (aggr_info->timer_scheduled) {
+ del_timer(&aggr_info->timer);
+ aggr_info->timer_scheduled = false;
+ }
+
for (tid = 0; tid < NUM_OF_TIDS; tid++)
aggr_delete_tid_state(aggr_info, tid);
}
--
1.7.0.4



Subject: [PATCH V2 6/6] ath6kl: Fix bug in maintaining aggregation state in AP mode

Currently rx aggregation related states are maintained per
vif, but this will not properly work when operating in AP mode.
Aggregation is completely broken when more than one
11n stations are connected to AP mode vif. Fix this issue
by keeping station specific aggregation state in sta_list.

Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
---
V2 - Remove unnecessary rxtid->q (skb_queue) initialization from
ath6kl_alloc_aggr_conn().
- Move aggr_conn allocation from ath6kl_alloc_aggr_conn() to
ath6kl_core_init() and remove ath6kl_alloc_aggr_conn().

drivers/net/wireless/ath/ath6kl/cfg80211.c | 5 ++
drivers/net/wireless/ath/ath6kl/core.c | 7 ++
drivers/net/wireless/ath/ath6kl/core.h | 4 +-
drivers/net/wireless/ath/ath6kl/init.c | 4 +
drivers/net/wireless/ath/ath6kl/main.c | 7 +-
drivers/net/wireless/ath/ath6kl/txrx.c | 106 +++++++++++++++++-----------
6 files changed, 88 insertions(+), 45 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c b/drivers/net/wireless/ath/ath6kl/cfg80211.c
index 8bf447a..fed7b80 100644
--- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
+++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
@@ -2871,6 +2871,11 @@ struct ath6kl *ath6kl_cfg80211_create(void)
/* Note: ar variable must not be accessed after calling this! */
void ath6kl_cfg80211_destroy(struct ath6kl *ar)
{
+ int i;
+
+ for (i = 0; i < AP_MAX_NUM_STA; i++)
+ kfree(ar->sta_list[i].aggr_conn);
+
wiphy_free(ar->wiphy);
}

diff --git a/drivers/net/wireless/ath/ath6kl/core.c b/drivers/net/wireless/ath/ath6kl/core.c
index d764afe..0d92e71 100644
--- a/drivers/net/wireless/ath/ath6kl/core.c
+++ b/drivers/net/wireless/ath/ath6kl/core.c
@@ -253,6 +253,13 @@ struct ath6kl *ath6kl_core_create(struct device *dev)
spin_lock_init(&ar->sta_list[ctr].psq_lock);
skb_queue_head_init(&ar->sta_list[ctr].psq);
skb_queue_head_init(&ar->sta_list[ctr].apsdq);
+ ar->sta_list[ctr].aggr_conn =
+ kzalloc(sizeof(struct aggr_info_conn), GFP_KERNEL);
+ if (!ar->sta_list[ctr].aggr_conn) {
+ ath6kl_err("Failed to allocate memory for sta aggregation information\n");
+ ath6kl_core_destroy(ar);
+ return NULL;
+ }
}

skb_queue_head_init(&ar->mcastpsq);
diff --git a/drivers/net/wireless/ath/ath6kl/core.h b/drivers/net/wireless/ath/ath6kl/core.h
index 1b9054f..ed9fcc1 100644
--- a/drivers/net/wireless/ath/ath6kl/core.h
+++ b/drivers/net/wireless/ath/ath6kl/core.h
@@ -298,6 +298,7 @@ struct ath6kl_sta {
spinlock_t psq_lock;
u8 apsd_info;
struct sk_buff_head apsdq;
+ struct aggr_info_conn *aggr_conn;
};

struct ath6kl_version {
@@ -713,6 +714,7 @@ void ath6kl_free_cookie(struct ath6kl *ar, struct ath6kl_cookie *cookie);
int ath6kl_data_tx(struct sk_buff *skb, struct net_device *dev);

struct aggr_info *aggr_init(struct ath6kl_vif *vif);
+void aggr_conn_init(struct ath6kl_vif *vif, struct aggr_info_conn *aggr_conn);
void ath6kl_rx_refill(struct htc_target *target,
enum htc_endpoint_id endpoint);
void ath6kl_refill_amsdu_rxbufs(struct ath6kl *ar, int count);
@@ -720,7 +722,7 @@ struct htc_packet *ath6kl_alloc_amsdu_rxbuf(struct htc_target *target,
enum htc_endpoint_id endpoint,
int len);
void aggr_module_destroy(struct aggr_info *aggr_info);
-void aggr_reset_state(struct aggr_info *aggr_info);
+void aggr_reset_state(struct aggr_info_conn *aggr_conn);

struct ath6kl_sta *ath6kl_find_sta(struct ath6kl_vif *vif, u8 * node_addr);
struct ath6kl_sta *ath6kl_find_sta_by_aid(struct ath6kl *ar, u8 aid);
diff --git a/drivers/net/wireless/ath/ath6kl/init.c b/drivers/net/wireless/ath/ath6kl/init.c
index 167dc41..4d8eb1c 100644
--- a/drivers/net/wireless/ath/ath6kl/init.c
+++ b/drivers/net/wireless/ath/ath6kl/init.c
@@ -1660,6 +1660,7 @@ void ath6kl_cleanup_vif(struct ath6kl_vif *vif, bool wmi_ready)
void ath6kl_stop_txrx(struct ath6kl *ar)
{
struct ath6kl_vif *vif, *tmp_vif;
+ int i;

set_bit(DESTROY_IN_PROGRESS, &ar->flag);

@@ -1668,6 +1669,9 @@ void ath6kl_stop_txrx(struct ath6kl *ar)
return;
}

+ for (i = 0; i < AP_MAX_NUM_STA; i++)
+ aggr_reset_state(ar->sta_list[i].aggr_conn);
+
spin_lock_bh(&ar->list_lock);
list_for_each_entry_safe(vif, tmp_vif, &ar->vif_list, list) {
list_del(&vif->list);
diff --git a/drivers/net/wireless/ath/ath6kl/main.c b/drivers/net/wireless/ath/ath6kl/main.c
index acb4acb..39da1f9 100644
--- a/drivers/net/wireless/ath/ath6kl/main.c
+++ b/drivers/net/wireless/ath/ath6kl/main.c
@@ -74,6 +74,7 @@ static void ath6kl_add_new_sta(struct ath6kl_vif *vif, u8 *mac, u16 aid,

ar->sta_list_index = ar->sta_list_index | (1 << free_slot);
ar->ap_stats.sta[free_slot].aid = cpu_to_le32(aid);
+ aggr_conn_init(vif, sta->aggr_conn);
}

static void ath6kl_sta_cleanup(struct ath6kl *ar, u8 i)
@@ -94,7 +95,7 @@ static void ath6kl_sta_cleanup(struct ath6kl *ar, u8 i)
sta->sta_flags = 0;

ar->sta_list_index = ar->sta_list_index & ~(1 << i);
-
+ aggr_reset_state(sta->aggr_conn);
}

static u8 ath6kl_remove_sta(struct ath6kl *ar, u8 *mac, u16 reason)
@@ -602,7 +603,7 @@ void ath6kl_connect_event(struct ath6kl_vif *vif, u16 channel, u8 *bssid,
netif_carrier_on(vif->ndev);
spin_unlock_bh(&vif->if_lock);

- aggr_reset_state(vif->aggr_cntxt);
+ aggr_reset_state(vif->aggr_cntxt->aggr_conn);
vif->reconnect_flag = 0;

if ((vif->nw_type == ADHOC_NETWORK) && ar->ibss_ps_enable) {
@@ -924,7 +925,7 @@ void ath6kl_disconnect_event(struct ath6kl_vif *vif, u8 reason, u8 *bssid,
assoc_resp_len, assoc_info,
prot_reason_status);

- aggr_reset_state(vif->aggr_cntxt);
+ aggr_reset_state(vif->aggr_cntxt->aggr_conn);

del_timer(&vif->disconnect_timer);

diff --git a/drivers/net/wireless/ath/ath6kl/txrx.c b/drivers/net/wireless/ath/ath6kl/txrx.c
index 8cf7b2f..62c1210 100644
--- a/drivers/net/wireless/ath/ath6kl/txrx.c
+++ b/drivers/net/wireless/ath/ath6kl/txrx.c
@@ -22,12 +22,18 @@
* aid - tid_mux4..tid_mux7
*/
#define ATH6KL_TID_MASK 0xf
+#define ATH6KL_AID_SHIFT 4

static inline u8 ath6kl_get_tid(u8 tid_mux)
{
return tid_mux & ATH6KL_TID_MASK;
}

+static inline u8 ath6kl_get_aid(u8 tid_mux)
+{
+ return tid_mux >> ATH6KL_AID_SHIFT;
+}
+
static u8 ath6kl_ibss_map_epid(struct sk_buff *skb, struct net_device *dev,
u32 *map_no)
{
@@ -1003,7 +1009,7 @@ static void aggr_slice_amsdu(struct aggr_info *p_aggr,
dev_kfree_skb(skb);
}

-static void aggr_deque_frms(struct aggr_info *p_aggr, u8 tid,
+static void aggr_deque_frms(struct aggr_info_conn *agg_conn, u8 tid,
u16 seq_no, u8 order)
{
struct sk_buff *skb;
@@ -1011,12 +1017,7 @@ static void aggr_deque_frms(struct aggr_info *p_aggr, u8 tid,
struct skb_hold_q *node;
u16 idx, idx_end, seq_end;
struct rxtid_stats *stats;
- struct aggr_info_conn *agg_conn;

- if (!p_aggr || !p_aggr->aggr_conn)
- return;
-
- agg_conn = p_aggr->aggr_conn;
rxtid = &agg_conn->rx_tid[tid];
stats = &agg_conn->stat[tid];

@@ -1047,7 +1048,8 @@ static void aggr_deque_frms(struct aggr_info *p_aggr, u8 tid,

if (node->skb) {
if (node->is_amsdu)
- aggr_slice_amsdu(p_aggr, rxtid, node->skb);
+ aggr_slice_amsdu(agg_conn->aggr_info, rxtid,
+ node->skb);
else
skb_queue_tail(&rxtid->q, node->skb);
node->skb = NULL;
@@ -1066,7 +1068,7 @@ static void aggr_deque_frms(struct aggr_info *p_aggr, u8 tid,
ath6kl_deliver_frames_to_nw_stack(agg_conn->dev, skb);
}

-static bool aggr_process_recv_frm(struct aggr_info *agg_info, u8 tid,
+static bool aggr_process_recv_frm(struct aggr_info_conn *agg_conn, u8 tid,
u16 seq_no,
bool is_amsdu, struct sk_buff *frame)
{
@@ -1077,7 +1079,6 @@ static bool aggr_process_recv_frm(struct aggr_info *agg_info, u8 tid,
u16 idx, st, cur, end;
bool is_queued = false;
u16 extended_end;
- struct aggr_info_conn *agg_conn = agg_info->aggr_conn;

rxtid = &agg_conn->rx_tid[tid];
stats = &agg_conn->stat[tid];
@@ -1086,7 +1087,7 @@ static bool aggr_process_recv_frm(struct aggr_info *agg_info, u8 tid,

if (!rxtid->aggr) {
if (is_amsdu) {
- aggr_slice_amsdu(agg_info, rxtid, frame);
+ aggr_slice_amsdu(agg_conn->aggr_info, rxtid, frame);
is_queued = true;
stats->num_amsdu++;
while ((skb = skb_dequeue(&rxtid->q)))
@@ -1110,7 +1111,7 @@ static bool aggr_process_recv_frm(struct aggr_info *agg_info, u8 tid,
(cur < end || cur > extended_end)) ||
((end > extended_end) && (cur > extended_end) &&
(cur < end))) {
- aggr_deque_frms(agg_info, tid, 0, 0);
+ aggr_deque_frms(agg_conn, tid, 0, 0);
if (cur >= rxtid->hold_q_sz - 1)
rxtid->seq_next = cur - (rxtid->hold_q_sz - 1);
else
@@ -1127,7 +1128,7 @@ static bool aggr_process_recv_frm(struct aggr_info *agg_info, u8 tid,
st = ATH6KL_MAX_SEQ_NO -
(rxtid->hold_q_sz - 2 - cur);

- aggr_deque_frms(agg_info, tid, st, 0);
+ aggr_deque_frms(agg_conn, tid, st, 0);
}

stats->num_oow++;
@@ -1166,7 +1167,7 @@ static bool aggr_process_recv_frm(struct aggr_info *agg_info, u8 tid,

spin_unlock_bh(&rxtid->lock);

- aggr_deque_frms(agg_info, tid, 0, 1);
+ aggr_deque_frms(agg_conn, tid, 0, 1);

if (agg_conn->timer_scheduled)
rxtid->progress = true;
@@ -1278,6 +1279,7 @@ void ath6kl_rx(struct htc_target *target, struct htc_packet *packet)
struct sk_buff *skb1 = NULL;
struct ethhdr *datap = NULL;
struct ath6kl_vif *vif;
+ struct aggr_info_conn *aggr_conn;
u16 seq_no, offset;
u8 tid, if_idx;

@@ -1529,11 +1531,21 @@ void ath6kl_rx(struct htc_target *target, struct htc_packet *packet)

datap = (struct ethhdr *) skb->data;

- if (is_unicast_ether_addr(datap->h_dest) &&
- aggr_process_recv_frm(vif->aggr_cntxt, tid, seq_no,
- is_amsdu, skb))
- /* aggregation code will handle the skb */
- return;
+ if (is_unicast_ether_addr(datap->h_dest)) {
+ if (vif->nw_type == AP_NETWORK) {
+ conn = ath6kl_find_sta(vif, datap->h_source);
+ if (!conn)
+ return;
+ aggr_conn = conn->aggr_conn;
+ } else
+ aggr_conn = vif->aggr_cntxt->aggr_conn;
+
+ if (aggr_process_recv_frm(aggr_conn, tid, seq_no,
+ is_amsdu, skb)) {
+ /* aggregation code will handle the skb */
+ return;
+ }
+ }

ath6kl_deliver_frames_to_nw_stack(vif->ndev, skb);
}
@@ -1558,7 +1570,7 @@ static void aggr_timeout(unsigned long arg)
rxtid->seq_next,
((rxtid->seq_next + rxtid->hold_q_sz-1) &
ATH6KL_MAX_SEQ_NO));
- aggr_deque_frms(aggr_conn->aggr_info, i, 0, 0);
+ aggr_deque_frms(aggr_conn, i, 0, 0);
}

aggr_conn->timer_scheduled = false;
@@ -1598,7 +1610,7 @@ static void aggr_delete_tid_state(struct aggr_info_conn *aggr_conn, u8 tid)
stats = &aggr_conn->stat[tid];

if (rxtid->aggr)
- aggr_deque_frms(aggr_conn->aggr_info, tid, 0, 0);
+ aggr_deque_frms(aggr_conn, tid, 0, 0);

rxtid->aggr = false;
rxtid->progress = false;
@@ -1616,17 +1628,23 @@ static void aggr_delete_tid_state(struct aggr_info_conn *aggr_conn, u8 tid)
void aggr_recv_addba_req_evt(struct ath6kl_vif *vif, u8 tid_mux, u16 seq_no,
u8 win_sz)
{
- struct aggr_info *p_aggr = vif->aggr_cntxt;
- struct aggr_info_conn *aggr_conn;
+ struct ath6kl_sta *sta;
+ struct aggr_info_conn *aggr_conn = NULL;
struct rxtid *rxtid;
struct rxtid_stats *stats;
u16 hold_q_size;
- u8 tid;
+ u8 tid, aid;

- if (!p_aggr || !p_aggr->aggr_conn)
- return;
+ if (vif->nw_type == AP_NETWORK) {
+ aid = ath6kl_get_aid(tid_mux);
+ sta = ath6kl_find_sta_by_aid(vif->ar, aid);
+ if (sta)
+ aggr_conn = sta->aggr_conn;
+ } else
+ aggr_conn = vif->aggr_cntxt->aggr_conn;

- aggr_conn = p_aggr->aggr_conn;
+ if (!aggr_conn)
+ return;

tid = ath6kl_get_tid(tid_mux);
if (tid >= NUM_OF_TIDS)
@@ -1656,8 +1674,7 @@ void aggr_recv_addba_req_evt(struct ath6kl_vif *vif, u8 tid_mux, u16 seq_no,
rxtid->aggr = true;
}

-static void aggr_conn_init(struct ath6kl_vif *vif,
- struct aggr_info_conn *aggr_conn)
+void aggr_conn_init(struct ath6kl_vif *vif, struct aggr_info_conn *aggr_conn)
{
struct rxtid *rxtid;
u8 i;
@@ -1709,39 +1726,46 @@ struct aggr_info *aggr_init(struct ath6kl_vif *vif)

void aggr_recv_delba_req_evt(struct ath6kl_vif *vif, u8 tid_mux)
{
- struct aggr_info *p_aggr = vif->aggr_cntxt;
+ struct ath6kl_sta *sta;
struct rxtid *rxtid;
- struct aggr_info_conn *aggr_conn;
- u8 tid;
+ struct aggr_info_conn *aggr_conn = NULL;
+ u8 tid, aid;
+
+ if (vif->nw_type == AP_NETWORK) {
+ aid = ath6kl_get_aid(tid_mux);
+ sta = ath6kl_find_sta_by_aid(vif->ar, aid);
+ if (sta)
+ aggr_conn = sta->aggr_conn;
+ } else
+ aggr_conn = vif->aggr_cntxt->aggr_conn;

- if (!p_aggr || !p_aggr->aggr_conn)
+ if (!aggr_conn)
return;

tid = ath6kl_get_tid(tid_mux);
if (tid >= NUM_OF_TIDS)
return;

- aggr_conn = p_aggr->aggr_conn;
rxtid = &aggr_conn->rx_tid[tid];

if (rxtid->aggr)
aggr_delete_tid_state(aggr_conn, tid);
}

-void aggr_reset_state(struct aggr_info *aggr_info)
+void aggr_reset_state(struct aggr_info_conn *aggr_conn)
{
u8 tid;

- if (!aggr_info || !aggr_info->aggr_conn)
+ if (!aggr_conn)
return;

- if (aggr_info->aggr_conn->timer_scheduled) {
- del_timer(&aggr_info->aggr_conn->timer);
- aggr_info->aggr_conn->timer_scheduled = false;
+ if (aggr_conn->timer_scheduled) {
+ del_timer(&aggr_conn->timer);
+ aggr_conn->timer_scheduled = false;
}

for (tid = 0; tid < NUM_OF_TIDS; tid++)
- aggr_delete_tid_state(aggr_info->aggr_conn, tid);
+ aggr_delete_tid_state(aggr_conn, tid);
}

/* clean up our amsdu buffer list */
@@ -1768,10 +1792,10 @@ void ath6kl_cleanup_amsdu_rxbufs(struct ath6kl *ar)

void aggr_module_destroy(struct aggr_info *aggr_info)
{
- if (!aggr_info || !aggr_info->aggr_conn)
+ if (!aggr_info)
return;

- aggr_reset_state(aggr_info);
+ aggr_reset_state(aggr_info->aggr_conn);
skb_queue_purge(&aggr_info->rx_amsdu_freeq);
kfree(aggr_info->aggr_conn);
kfree(aggr_info);
--
1.7.0.4


Subject: [PATCH V2 2/6] ath6kl: Fix memory leak when unloading ath6kl_sdio

The patch "ath6kl: create core.c" removes wiphy_free() from
ath6kl_cfg80211_cleanup() and misses to free wiphy in
ath6kl_sdio_remove(). This patch fixes this regression.

Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
---
drivers/net/wireless/ath/ath6kl/sdio.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/sdio.c b/drivers/net/wireless/ath/ath6kl/sdio.c
index 7bb6107..d9f5591 100644
--- a/drivers/net/wireless/ath/ath6kl/sdio.c
+++ b/drivers/net/wireless/ath/ath6kl/sdio.c
@@ -1314,6 +1314,7 @@ static void ath6kl_sdio_remove(struct sdio_func *func)
cancel_work_sync(&ar_sdio->wr_async_work);

ath6kl_core_cleanup(ar_sdio->ar);
+ ath6kl_core_destroy(ar_sdio->ar);

kfree(ar_sdio->dma_buffer);
kfree(ar_sdio);
--
1.7.0.4


Subject: Re: [PATCH V2 1/6] ath6kl: Make sure to delete rx aggregation timer in aggr_reset_state()

On Tue, Jan 24, 2012 at 02:37:04PM +0200, Kalle Valo wrote:
> On 01/21/2012 11:52 AM, Vasanthakumar Thiagarajan wrote:
> > The timer which is used to flush rx aggregation frames needs to
> > be disabled when resetting the aggregation state. This is found
> > in code review.
> >
> > Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
>
> Thanks, all six patches applied.

I'm really sorry that this patch set introduces a crash in accessing
invalid pointer through vif->aggr_cntxt. I'll send the next version
fixing this.

Vasanth

2012-01-25 16:22:54

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH V2 1/6] ath6kl: Make sure to delete rx aggregation timer in aggr_reset_state()

On 01/25/2012 06:03 PM, Vasanthakumar Thiagarajan wrote:
> On Tue, Jan 24, 2012 at 02:37:04PM +0200, Kalle Valo wrote:
>> On 01/21/2012 11:52 AM, Vasanthakumar Thiagarajan wrote:
>>> The timer which is used to flush rx aggregation frames needs to
>>> be disabled when resetting the aggregation state. This is found
>>> in code review.
>>>
>>> Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
>>
>> Thanks, all six patches applied.
>
> I'm really sorry that this patch set introduces a crash in accessing
> invalid pointer through vif->aggr_cntxt. I'll send the next version
> fixing this.

Sorry, it's too late as I have already applied the patches. You need to
create a new patch on top of ath6kl.git which fixes the issue.

Kalle

Subject: [PATCH V2 4/6] ath6kl: Pass vif instead of ar to ath6kl_add_new_sta()

This will be used when initializing station specific aggregation
information.

Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
---
drivers/net/wireless/ath/ath6kl/main.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/main.c b/drivers/net/wireless/ath/ath6kl/main.c
index 3a3b2cc..acb4acb 100644
--- a/drivers/net/wireless/ath/ath6kl/main.c
+++ b/drivers/net/wireless/ath/ath6kl/main.c
@@ -52,10 +52,11 @@ struct ath6kl_sta *ath6kl_find_sta_by_aid(struct ath6kl *ar, u8 aid)
return conn;
}

-static void ath6kl_add_new_sta(struct ath6kl *ar, u8 *mac, u16 aid, u8 *wpaie,
- size_t ielen, u8 keymgmt, u8 ucipher, u8 auth,
- u8 apsd_info)
+static void ath6kl_add_new_sta(struct ath6kl_vif *vif, u8 *mac, u16 aid,
+ u8 *wpaie, size_t ielen, u8 keymgmt,
+ u8 ucipher, u8 auth, u8 apsd_info)
{
+ struct ath6kl *ar = vif->ar;
struct ath6kl_sta *sta;
u8 free_slot;

@@ -430,7 +431,6 @@ void ath6kl_connect_ap_mode_sta(struct ath6kl_vif *vif, u16 aid, u8 *mac_addr,
u8 keymgmt, u8 ucipher, u8 auth,
u8 assoc_req_len, u8 *assoc_info, u8 apsd_info)
{
- struct ath6kl *ar = vif->ar;
u8 *ies = NULL, *wpa_ie = NULL, *pos;
size_t ies_len = 0;
struct station_info sinfo;
@@ -484,7 +484,7 @@ void ath6kl_connect_ap_mode_sta(struct ath6kl_vif *vif, u16 aid, u8 *mac_addr,
pos += 2 + pos[1];
}

- ath6kl_add_new_sta(ar, mac_addr, aid, wpa_ie,
+ ath6kl_add_new_sta(vif, mac_addr, aid, wpa_ie,
wpa_ie ? 2 + wpa_ie[1] : 0,
keymgmt, ucipher, auth, apsd_info);

--
1.7.0.4


Subject: [PATCH V2 5/6] ath6kl: Fix bug in using tid given by addba/delba req events

The tid which is given in addba/delba req event is not
just tid but also muxed with the assoc id (MSB 4 bits)
which can be used to determine the corresponding connected
station in softap mode. The actual tid is LSB 4 bits. Using
the tid as it is with rx_tid[] would result in OOB or invalid
memory access in AP mode.

Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
---
drivers/net/wireless/ath/ath6kl/txrx.c | 25 +++++++++++++++++++++++--
1 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/txrx.c b/drivers/net/wireless/ath/ath6kl/txrx.c
index 8407d01..8cf7b2f 100644
--- a/drivers/net/wireless/ath/ath6kl/txrx.c
+++ b/drivers/net/wireless/ath/ath6kl/txrx.c
@@ -17,6 +17,17 @@
#include "core.h"
#include "debug.h"

+/*
+ * tid - tid_mux0..tid_mux3
+ * aid - tid_mux4..tid_mux7
+ */
+#define ATH6KL_TID_MASK 0xf
+
+static inline u8 ath6kl_get_tid(u8 tid_mux)
+{
+ return tid_mux & ATH6KL_TID_MASK;
+}
+
static u8 ath6kl_ibss_map_epid(struct sk_buff *skb, struct net_device *dev,
u32 *map_no)
{
@@ -1602,7 +1613,7 @@ static void aggr_delete_tid_state(struct aggr_info_conn *aggr_conn, u8 tid)
memset(stats, 0, sizeof(struct rxtid_stats));
}

-void aggr_recv_addba_req_evt(struct ath6kl_vif *vif, u8 tid, u16 seq_no,
+void aggr_recv_addba_req_evt(struct ath6kl_vif *vif, u8 tid_mux, u16 seq_no,
u8 win_sz)
{
struct aggr_info *p_aggr = vif->aggr_cntxt;
@@ -1610,12 +1621,17 @@ void aggr_recv_addba_req_evt(struct ath6kl_vif *vif, u8 tid, u16 seq_no,
struct rxtid *rxtid;
struct rxtid_stats *stats;
u16 hold_q_size;
+ u8 tid;

if (!p_aggr || !p_aggr->aggr_conn)
return;

aggr_conn = p_aggr->aggr_conn;

+ tid = ath6kl_get_tid(tid_mux);
+ if (tid >= NUM_OF_TIDS)
+ return;
+
rxtid = &aggr_conn->rx_tid[tid];
stats = &aggr_conn->stat[tid];

@@ -1691,15 +1707,20 @@ struct aggr_info *aggr_init(struct ath6kl_vif *vif)
return p_aggr;
}

-void aggr_recv_delba_req_evt(struct ath6kl_vif *vif, u8 tid)
+void aggr_recv_delba_req_evt(struct ath6kl_vif *vif, u8 tid_mux)
{
struct aggr_info *p_aggr = vif->aggr_cntxt;
struct rxtid *rxtid;
struct aggr_info_conn *aggr_conn;
+ u8 tid;

if (!p_aggr || !p_aggr->aggr_conn)
return;

+ tid = ath6kl_get_tid(tid_mux);
+ if (tid >= NUM_OF_TIDS)
+ return;
+
aggr_conn = p_aggr->aggr_conn;
rxtid = &aggr_conn->rx_tid[tid];

--
1.7.0.4


2012-01-24 12:40:06

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH V2 1/6] ath6kl: Make sure to delete rx aggregation timer in aggr_reset_state()

On 01/21/2012 11:52 AM, Vasanthakumar Thiagarajan wrote:
> The timer which is used to flush rx aggregation frames needs to
> be disabled when resetting the aggregation state. This is found
> in code review.
>
> Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>

Thanks, all six patches applied.

Kalle

Subject: [PATCH V2 3/6] ath6kl: Define a structure for connection specific aggregation information

This patch just groups connection specific aggregation information
from struct aggr_info into a new structure (struct aggr_info_conn)
so that, in softAP mode, this can be used when each connected station
is made to have it's own aggregation state.

Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
---
V2 - Use aggr_reset_state() to clean up connection specific
aggr state instead of defining a new function, ath6kl_cleanup_aggr_conn().

drivers/net/wireless/ath/ath6kl/cfg80211.c | 2 +-
drivers/net/wireless/ath/ath6kl/core.h | 11 ++-
drivers/net/wireless/ath/ath6kl/txrx.c | 166 +++++++++++++++-------------
3 files changed, 98 insertions(+), 81 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c b/drivers/net/wireless/ath/ath6kl/cfg80211.c
index 594d246..8bf447a 100644
--- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
+++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
@@ -2694,7 +2694,7 @@ void ath6kl_cfg80211_stop_all(struct ath6kl *ar)

static int ath6kl_cfg80211_vif_init(struct ath6kl_vif *vif)
{
- vif->aggr_cntxt = aggr_init(vif->ndev);
+ vif->aggr_cntxt = aggr_init(vif);
if (!vif->aggr_cntxt) {
ath6kl_err("failed to initialize aggr\n");
return -ENOMEM;
diff --git a/drivers/net/wireless/ath/ath6kl/core.h b/drivers/net/wireless/ath/ath6kl/core.h
index 67b22e4..1b9054f 100644
--- a/drivers/net/wireless/ath/ath6kl/core.h
+++ b/drivers/net/wireless/ath/ath6kl/core.h
@@ -242,14 +242,19 @@ struct rxtid_stats {
u32 num_bar;
};

-struct aggr_info {
+struct aggr_info_conn {
u8 aggr_sz;
u8 timer_scheduled;
struct timer_list timer;
struct net_device *dev;
struct rxtid rx_tid[NUM_OF_TIDS];
- struct sk_buff_head free_q;
struct rxtid_stats stat[NUM_OF_TIDS];
+ struct aggr_info *aggr_info;
+};
+
+struct aggr_info {
+ struct aggr_info_conn *aggr_conn;
+ struct sk_buff_head rx_amsdu_freeq;
};

struct ath6kl_wep_key {
@@ -707,7 +712,7 @@ struct ath6kl_cookie *ath6kl_alloc_cookie(struct ath6kl *ar);
void ath6kl_free_cookie(struct ath6kl *ar, struct ath6kl_cookie *cookie);
int ath6kl_data_tx(struct sk_buff *skb, struct net_device *dev);

-struct aggr_info *aggr_init(struct net_device *dev);
+struct aggr_info *aggr_init(struct ath6kl_vif *vif);
void ath6kl_rx_refill(struct htc_target *target,
enum htc_endpoint_id endpoint);
void ath6kl_refill_amsdu_rxbufs(struct ath6kl *ar, int count);
diff --git a/drivers/net/wireless/ath/ath6kl/txrx.c b/drivers/net/wireless/ath/ath6kl/txrx.c
index cb7421a..8407d01 100644
--- a/drivers/net/wireless/ath/ath6kl/txrx.c
+++ b/drivers/net/wireless/ath/ath6kl/txrx.c
@@ -819,10 +819,12 @@ static struct sk_buff *aggr_get_free_skb(struct aggr_info *p_aggr)
{
struct sk_buff *skb = NULL;

- if (skb_queue_len(&p_aggr->free_q) < (AGGR_NUM_OF_FREE_NETBUFS >> 2))
- ath6kl_alloc_netbufs(&p_aggr->free_q, AGGR_NUM_OF_FREE_NETBUFS);
+ if (skb_queue_len(&p_aggr->rx_amsdu_freeq) <
+ (AGGR_NUM_OF_FREE_NETBUFS >> 2))
+ ath6kl_alloc_netbufs(&p_aggr->rx_amsdu_freeq,
+ AGGR_NUM_OF_FREE_NETBUFS);

- skb = skb_dequeue(&p_aggr->free_q);
+ skb = skb_dequeue(&p_aggr->rx_amsdu_freeq);

return skb;
}
@@ -998,12 +1000,14 @@ static void aggr_deque_frms(struct aggr_info *p_aggr, u8 tid,
struct skb_hold_q *node;
u16 idx, idx_end, seq_end;
struct rxtid_stats *stats;
+ struct aggr_info_conn *agg_conn;

- if (!p_aggr)
+ if (!p_aggr || !p_aggr->aggr_conn)
return;

- rxtid = &p_aggr->rx_tid[tid];
- stats = &p_aggr->stat[tid];
+ agg_conn = p_aggr->aggr_conn;
+ rxtid = &agg_conn->rx_tid[tid];
+ stats = &agg_conn->stat[tid];

idx = AGGR_WIN_IDX(rxtid->seq_next, rxtid->hold_q_sz);

@@ -1048,7 +1052,7 @@ static void aggr_deque_frms(struct aggr_info *p_aggr, u8 tid,
stats->num_delivered += skb_queue_len(&rxtid->q);

while ((skb = skb_dequeue(&rxtid->q)))
- ath6kl_deliver_frames_to_nw_stack(p_aggr->dev, skb);
+ ath6kl_deliver_frames_to_nw_stack(agg_conn->dev, skb);
}

static bool aggr_process_recv_frm(struct aggr_info *agg_info, u8 tid,
@@ -1062,9 +1066,10 @@ static bool aggr_process_recv_frm(struct aggr_info *agg_info, u8 tid,
u16 idx, st, cur, end;
bool is_queued = false;
u16 extended_end;
+ struct aggr_info_conn *agg_conn = agg_info->aggr_conn;

- rxtid = &agg_info->rx_tid[tid];
- stats = &agg_info->stat[tid];
+ rxtid = &agg_conn->rx_tid[tid];
+ stats = &agg_conn->stat[tid];

stats->num_into_aggr++;

@@ -1074,7 +1079,7 @@ static bool aggr_process_recv_frm(struct aggr_info *agg_info, u8 tid,
is_queued = true;
stats->num_amsdu++;
while ((skb = skb_dequeue(&rxtid->q)))
- ath6kl_deliver_frames_to_nw_stack(agg_info->dev,
+ ath6kl_deliver_frames_to_nw_stack(agg_conn->dev,
skb);
}
return is_queued;
@@ -1152,7 +1157,7 @@ static bool aggr_process_recv_frm(struct aggr_info *agg_info, u8 tid,

aggr_deque_frms(agg_info, tid, 0, 1);

- if (agg_info->timer_scheduled)
+ if (agg_conn->timer_scheduled)
rxtid->progress = true;
else
for (idx = 0 ; idx < rxtid->hold_q_sz; idx++) {
@@ -1163,8 +1168,8 @@ static bool aggr_process_recv_frm(struct aggr_info *agg_info, u8 tid,
* the frame doesn't remain stuck
* forever.
*/
- agg_info->timer_scheduled = true;
- mod_timer(&agg_info->timer,
+ agg_conn->timer_scheduled = true;
+ mod_timer(&agg_conn->timer,
(jiffies +
HZ * (AGGR_RX_TIMEOUT) / 1000));
rxtid->progress = false;
@@ -1525,13 +1530,13 @@ void ath6kl_rx(struct htc_target *target, struct htc_packet *packet)
static void aggr_timeout(unsigned long arg)
{
u8 i, j;
- struct aggr_info *p_aggr = (struct aggr_info *) arg;
+ struct aggr_info_conn *aggr_conn = (struct aggr_info_conn *) arg;
struct rxtid *rxtid;
struct rxtid_stats *stats;

for (i = 0; i < NUM_OF_TIDS; i++) {
- rxtid = &p_aggr->rx_tid[i];
- stats = &p_aggr->stat[i];
+ rxtid = &aggr_conn->rx_tid[i];
+ stats = &aggr_conn->stat[i];

if (!rxtid->aggr || !rxtid->timer_mon || rxtid->progress)
continue;
@@ -1542,18 +1547,18 @@ static void aggr_timeout(unsigned long arg)
rxtid->seq_next,
((rxtid->seq_next + rxtid->hold_q_sz-1) &
ATH6KL_MAX_SEQ_NO));
- aggr_deque_frms(p_aggr, i, 0, 0);
+ aggr_deque_frms(aggr_conn->aggr_info, i, 0, 0);
}

- p_aggr->timer_scheduled = false;
+ aggr_conn->timer_scheduled = false;

for (i = 0; i < NUM_OF_TIDS; i++) {
- rxtid = &p_aggr->rx_tid[i];
+ rxtid = &aggr_conn->rx_tid[i];

if (rxtid->aggr && rxtid->hold_q) {
for (j = 0; j < rxtid->hold_q_sz; j++) {
if (rxtid->hold_q[j].skb) {
- p_aggr->timer_scheduled = true;
+ aggr_conn->timer_scheduled = true;
rxtid->timer_mon = true;
rxtid->progress = false;
break;
@@ -1565,24 +1570,24 @@ static void aggr_timeout(unsigned long arg)
}
}

- if (p_aggr->timer_scheduled)
- mod_timer(&p_aggr->timer,
+ if (aggr_conn->timer_scheduled)
+ mod_timer(&aggr_conn->timer,
jiffies + msecs_to_jiffies(AGGR_RX_TIMEOUT));
}

-static void aggr_delete_tid_state(struct aggr_info *p_aggr, u8 tid)
+static void aggr_delete_tid_state(struct aggr_info_conn *aggr_conn, u8 tid)
{
struct rxtid *rxtid;
struct rxtid_stats *stats;

- if (!p_aggr || tid >= NUM_OF_TIDS)
+ if (!aggr_conn || tid >= NUM_OF_TIDS)
return;

- rxtid = &p_aggr->rx_tid[tid];
- stats = &p_aggr->stat[tid];
+ rxtid = &aggr_conn->rx_tid[tid];
+ stats = &aggr_conn->stat[tid];

if (rxtid->aggr)
- aggr_deque_frms(p_aggr, tid, 0, 0);
+ aggr_deque_frms(aggr_conn->aggr_info, tid, 0, 0);

rxtid->aggr = false;
rxtid->progress = false;
@@ -1601,22 +1606,25 @@ void aggr_recv_addba_req_evt(struct ath6kl_vif *vif, u8 tid, u16 seq_no,
u8 win_sz)
{
struct aggr_info *p_aggr = vif->aggr_cntxt;
+ struct aggr_info_conn *aggr_conn;
struct rxtid *rxtid;
struct rxtid_stats *stats;
u16 hold_q_size;

- if (!p_aggr)
+ if (!p_aggr || !p_aggr->aggr_conn)
return;

- rxtid = &p_aggr->rx_tid[tid];
- stats = &p_aggr->stat[tid];
+ aggr_conn = p_aggr->aggr_conn;
+
+ rxtid = &aggr_conn->rx_tid[tid];
+ stats = &aggr_conn->stat[tid];

if (win_sz < AGGR_WIN_SZ_MIN || win_sz > AGGR_WIN_SZ_MAX)
ath6kl_dbg(ATH6KL_DBG_WLAN_RX, "%s: win_sz %d, tid %d\n",
__func__, win_sz, tid);

if (rxtid->aggr)
- aggr_delete_tid_state(p_aggr, tid);
+ aggr_delete_tid_state(aggr_conn, tid);

rxtid->seq_next = seq_no;
hold_q_size = TID_WINDOW_SZ(win_sz) * sizeof(struct skb_hold_q);
@@ -1632,31 +1640,23 @@ void aggr_recv_addba_req_evt(struct ath6kl_vif *vif, u8 tid, u16 seq_no,
rxtid->aggr = true;
}

-struct aggr_info *aggr_init(struct net_device *dev)
+static void aggr_conn_init(struct ath6kl_vif *vif,
+ struct aggr_info_conn *aggr_conn)
{
- struct aggr_info *p_aggr = NULL;
struct rxtid *rxtid;
u8 i;

- p_aggr = kzalloc(sizeof(struct aggr_info), GFP_KERNEL);
- if (!p_aggr) {
- ath6kl_err("failed to alloc memory for aggr_node\n");
- return NULL;
- }
-
- p_aggr->aggr_sz = AGGR_SZ_DEFAULT;
- p_aggr->dev = dev;
- init_timer(&p_aggr->timer);
- p_aggr->timer.function = aggr_timeout;
- p_aggr->timer.data = (unsigned long) p_aggr;
-
- p_aggr->timer_scheduled = false;
- skb_queue_head_init(&p_aggr->free_q);
+ aggr_conn->aggr_sz = AGGR_SZ_DEFAULT;
+ aggr_conn->dev = vif->ndev;
+ init_timer(&aggr_conn->timer);
+ aggr_conn->timer.function = aggr_timeout;
+ aggr_conn->timer.data = (unsigned long) aggr_conn;
+ aggr_conn->aggr_info = vif->aggr_cntxt;

- ath6kl_alloc_netbufs(&p_aggr->free_q, AGGR_NUM_OF_FREE_NETBUFS);
+ aggr_conn->timer_scheduled = false;

for (i = 0; i < NUM_OF_TIDS; i++) {
- rxtid = &p_aggr->rx_tid[i];
+ rxtid = &aggr_conn->rx_tid[i];
rxtid->aggr = false;
rxtid->progress = false;
rxtid->timer_mon = false;
@@ -1664,6 +1664,30 @@ struct aggr_info *aggr_init(struct net_device *dev)
spin_lock_init(&rxtid->lock);
}

+}
+
+struct aggr_info *aggr_init(struct ath6kl_vif *vif)
+{
+ struct aggr_info *p_aggr = NULL;
+
+ p_aggr = kzalloc(sizeof(struct aggr_info), GFP_KERNEL);
+ if (!p_aggr) {
+ ath6kl_err("failed to alloc memory for aggr_node\n");
+ return NULL;
+ }
+
+ p_aggr->aggr_conn = kzalloc(sizeof(struct aggr_info_conn), GFP_KERNEL);
+ if (!p_aggr->aggr_conn) {
+ ath6kl_err("failed to alloc memory for connection specific aggr info\n");
+ kfree(p_aggr);
+ return NULL;
+ }
+
+ aggr_conn_init(vif, p_aggr->aggr_conn);
+
+ skb_queue_head_init(&p_aggr->rx_amsdu_freeq);
+ ath6kl_alloc_netbufs(&p_aggr->rx_amsdu_freeq, AGGR_NUM_OF_FREE_NETBUFS);
+
return p_aggr;
}

@@ -1671,27 +1695,32 @@ void aggr_recv_delba_req_evt(struct ath6kl_vif *vif, u8 tid)
{
struct aggr_info *p_aggr = vif->aggr_cntxt;
struct rxtid *rxtid;
+ struct aggr_info_conn *aggr_conn;

- if (!p_aggr)
+ if (!p_aggr || !p_aggr->aggr_conn)
return;

- rxtid = &p_aggr->rx_tid[tid];
+ aggr_conn = p_aggr->aggr_conn;
+ rxtid = &aggr_conn->rx_tid[tid];

if (rxtid->aggr)
- aggr_delete_tid_state(p_aggr, tid);
+ aggr_delete_tid_state(aggr_conn, tid);
}

void aggr_reset_state(struct aggr_info *aggr_info)
{
u8 tid;

- if (aggr_info->timer_scheduled) {
- del_timer(&aggr_info->timer);
- aggr_info->timer_scheduled = false;
+ if (!aggr_info || !aggr_info->aggr_conn)
+ return;
+
+ if (aggr_info->aggr_conn->timer_scheduled) {
+ del_timer(&aggr_info->aggr_conn->timer);
+ aggr_info->aggr_conn->timer_scheduled = false;
}

for (tid = 0; tid < NUM_OF_TIDS; tid++)
- aggr_delete_tid_state(aggr_info, tid);
+ aggr_delete_tid_state(aggr_info->aggr_conn, tid);
}

/* clean up our amsdu buffer list */
@@ -1718,28 +1747,11 @@ void ath6kl_cleanup_amsdu_rxbufs(struct ath6kl *ar)

void aggr_module_destroy(struct aggr_info *aggr_info)
{
- struct rxtid *rxtid;
- u8 i, k;
-
- if (!aggr_info)
+ if (!aggr_info || !aggr_info->aggr_conn)
return;

- if (aggr_info->timer_scheduled) {
- del_timer(&aggr_info->timer);
- aggr_info->timer_scheduled = false;
- }
-
- for (i = 0; i < NUM_OF_TIDS; i++) {
- rxtid = &aggr_info->rx_tid[i];
- if (rxtid->hold_q) {
- for (k = 0; k < rxtid->hold_q_sz; k++)
- dev_kfree_skb(rxtid->hold_q[k].skb);
- kfree(rxtid->hold_q);
- }
-
- skb_queue_purge(&rxtid->q);
- }
-
- skb_queue_purge(&aggr_info->free_q);
+ aggr_reset_state(aggr_info);
+ skb_queue_purge(&aggr_info->rx_amsdu_freeq);
+ kfree(aggr_info->aggr_conn);
kfree(aggr_info);
}
--
1.7.0.4