2009-03-23 16:32:35

by Johannes Berg

[permalink] [raw]
Subject: [PATCH 5/8] mac80211: unify and fix TX aggregation start

When TX aggregation becomes operational, we do a number of steps:
1) print a debug message
2) wake the virtual queue
3) notify the driver

Unfortunately, 1) and 3) are only done if the driver is first to
reply to the aggregation request, it is, however, possible that the
remote station replies before the driver! Thus, unify the code for
this and call the new function ieee80211_agg_tx_operational in both
places where TX aggregation can become operational.

Additionally, rename the driver notification from
IEEE80211_AMPDU_TX_RESUME to IEEE80211_AMPDU_TX_OPERATIONAL.

Signed-off-by: Johannes Berg <[email protected]>
---
drivers/net/wireless/ath9k/main.c | 2 -
include/net/mac80211.h | 4 +-
net/mac80211/agg-tx.c | 63 ++++++++++++++++----------------------
3 files changed, 30 insertions(+), 39 deletions(-)

--- wireless-testing.orig/net/mac80211/agg-tx.c 2009-03-23 13:59:19.000000000 +0100
+++ wireless-testing/net/mac80211/agg-tx.c 2009-03-23 14:03:46.000000000 +0100
@@ -404,6 +404,27 @@ int ieee80211_start_tx_ba_session(struct
}
EXPORT_SYMBOL(ieee80211_start_tx_ba_session);

+static void ieee80211_agg_tx_operational(struct ieee80211_local *local,
+ struct sta_info *sta, u16 tid)
+{
+#ifdef CONFIG_MAC80211_HT_DEBUG
+ printk(KERN_DEBUG "Aggregation is on for tid %d \n", tid);
+#endif
+
+ if (local->hw.ampdu_queues) {
+ /*
+ * Wake up the A-MPDU queue, we stopped it earlier,
+ * this will in turn wake the entire AC.
+ */
+ ieee80211_wake_queue_by_reason(&local->hw,
+ local->hw.queues + sta->tid_to_tx_q[tid],
+ IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
+ }
+
+ local->ops->ampdu_action(&local->hw, IEEE80211_AMPDU_TX_OPERATIONAL,
+ &sta->sta, tid, NULL);
+}
+
void ieee80211_start_tx_ba_cb(struct ieee80211_hw *hw, u8 *ra, u16 tid)
{
struct ieee80211_local *local = hw_to_local(hw);
@@ -446,20 +467,8 @@ void ieee80211_start_tx_ba_cb(struct iee

*state |= HT_ADDBA_DRV_READY_MSK;

- if (*state == HT_AGG_STATE_OPERATIONAL) {
-#ifdef CONFIG_MAC80211_HT_DEBUG
- printk(KERN_DEBUG "Aggregation is on for tid %d \n", tid);
-#endif
- if (hw->ampdu_queues) {
- /*
- * Wake up this queue, we stopped it earlier,
- * this will in turn wake the entire AC.
- */
- ieee80211_wake_queue_by_reason(hw,
- hw->queues + sta->tid_to_tx_q[tid],
- IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
- }
- }
+ if (*state == HT_AGG_STATE_OPERATIONAL)
+ ieee80211_agg_tx_operational(local, sta, tid);

out:
spin_unlock_bh(&sta->lock);
@@ -646,9 +655,7 @@ void ieee80211_process_addba_resp(struct
struct ieee80211_mgmt *mgmt,
size_t len)
{
- struct ieee80211_hw *hw = &local->hw;
- u16 capab;
- u16 tid, start_seq_num;
+ u16 capab, tid;
u8 *state;

capab = le16_to_cpu(mgmt->u.action.u.addba_resp.capab);
@@ -682,26 +689,10 @@ void ieee80211_process_addba_resp(struct

*state |= HT_ADDBA_RECEIVED_MSK;

- if (hw->ampdu_queues && *state != curstate &&
- *state == HT_AGG_STATE_OPERATIONAL) {
- /*
- * Wake up this queue, we stopped it earlier,
- * this will in turn wake the entire AC.
- */
- ieee80211_wake_queue_by_reason(hw,
- hw->queues + sta->tid_to_tx_q[tid],
- IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
- }
- sta->ampdu_mlme.addba_req_num[tid] = 0;
+ if (*state != curstate && *state == HT_AGG_STATE_OPERATIONAL)
+ ieee80211_agg_tx_operational(local, sta, tid);

- if (local->ops->ampdu_action) {
- (void)local->ops->ampdu_action(hw,
- IEEE80211_AMPDU_TX_RESUME,
- &sta->sta, tid, &start_seq_num);
- }
-#ifdef CONFIG_MAC80211_HT_DEBUG
- printk(KERN_DEBUG "Resuming TX aggregation for tid %d\n", tid);
-#endif /* CONFIG_MAC80211_HT_DEBUG */
+ sta->ampdu_mlme.addba_req_num[tid] = 0;
} else {
sta->ampdu_mlme.addba_req_num[tid]++;
___ieee80211_stop_tx_ba_session(sta, tid, WLAN_BACK_INITIATOR);
--- wireless-testing.orig/include/net/mac80211.h 2009-03-23 14:00:12.000000000 +0100
+++ wireless-testing/include/net/mac80211.h 2009-03-23 14:03:46.000000000 +0100
@@ -1213,14 +1213,14 @@ enum ieee80211_filter_flags {
* @IEEE80211_AMPDU_RX_STOP: stop Rx aggregation
* @IEEE80211_AMPDU_TX_START: start Tx aggregation
* @IEEE80211_AMPDU_TX_STOP: stop Tx aggregation
- * @IEEE80211_AMPDU_TX_RESUME: resume TX aggregation
+ * @IEEE80211_AMPDU_TX_OPERATIONAL: TX aggregation has become operational
*/
enum ieee80211_ampdu_mlme_action {
IEEE80211_AMPDU_RX_START,
IEEE80211_AMPDU_RX_STOP,
IEEE80211_AMPDU_TX_START,
IEEE80211_AMPDU_TX_STOP,
- IEEE80211_AMPDU_TX_RESUME,
+ IEEE80211_AMPDU_TX_OPERATIONAL,
};

/**
--- wireless-testing.orig/drivers/net/wireless/ath9k/main.c 2009-03-23 13:59:19.000000000 +0100
+++ wireless-testing/drivers/net/wireless/ath9k/main.c 2009-03-23 14:03:46.000000000 +0100
@@ -2730,7 +2730,7 @@ static int ath9k_ampdu_action(struct iee

ieee80211_stop_tx_ba_cb_irqsafe(hw, sta->addr, tid);
break;
- case IEEE80211_AMPDU_TX_RESUME:
+ case IEEE80211_AMPDU_TX_OPERATIONAL:
ath_tx_aggr_resume(sc, sta, tid);
break;
default:

--



2009-03-28 17:29:00

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 5/8] mac80211: unify and fix TX aggregation start

On Fri, 2009-03-27 at 20:01 -0700, Luis R. Rodriguez wrote:

> >> Unfortunately, 1) and 3) are only done if the driver is first to
> >> reply to the aggregation request, it is, however, possible that the
> >> remote station replies before the driver! Thus, unify the code for
> >> this and call the new function ieee80211_agg_tx_operational in both
> >> places where TX aggregation can become operational.
> >>
> >> Additionally, rename the driver notification from
> >> IEEE80211_AMPDU_TX_RESUME to IEEE80211_AMPDU_TX_OPERATIONAL.
> >>
> >> Signed-off-by: Johannes Berg <[email protected]>
> >
> > Nice catch man!
> >
> > Reviewed-by: Luis R. Rodriguez <[email protected]>
>
> Hm, maybe also a backported version could go to stable? Perhaps we can
> trigger a panic on a UP box?

No need -- ath9k always replies _instantly_, and iwlwifi doesn't support
aggregation in stable.

johannes


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2009-03-28 02:27:04

by Luis Chamberlain

[permalink] [raw]
Subject: Re: [PATCH 5/8] mac80211: unify and fix TX aggregation start

On Mon, Mar 23, 2009 at 05:28:39PM +0100, Johannes Berg wrote:
> When TX aggregation becomes operational, we do a number of steps:
> 1) print a debug message
> 2) wake the virtual queue
> 3) notify the driver
>
> Unfortunately, 1) and 3) are only done if the driver is first to
> reply to the aggregation request, it is, however, possible that the
> remote station replies before the driver! Thus, unify the code for
> this and call the new function ieee80211_agg_tx_operational in both
> places where TX aggregation can become operational.
>
> Additionally, rename the driver notification from
> IEEE80211_AMPDU_TX_RESUME to IEEE80211_AMPDU_TX_OPERATIONAL.
>
> Signed-off-by: Johannes Berg <[email protected]>

Nice catch man!

Reviewed-by: Luis R. Rodriguez <[email protected]>

Luis

2009-03-28 03:01:26

by Luis R. Rodriguez

[permalink] [raw]
Subject: Re: [PATCH 5/8] mac80211: unify and fix TX aggregation start

On Fri, Mar 27, 2009 at 7:27 PM, Luis R. Rodriguez
<[email protected]> wrote:
> On Mon, Mar 23, 2009 at 05:28:39PM +0100, Johannes Berg wrote:
>> When TX aggregation becomes operational, we do a number of steps:
>> =C2=A01) print a debug message
>> =C2=A02) wake the virtual queue
>> =C2=A03) notify the driver
>>
>> Unfortunately, 1) and 3) are only done if the driver is first to
>> reply to the aggregation request, it is, however, possible that the
>> remote station replies before the driver! Thus, unify the code for
>> this and call the new function ieee80211_agg_tx_operational in both
>> places where TX aggregation can become operational.
>>
>> Additionally, rename the driver notification from
>> IEEE80211_AMPDU_TX_RESUME to IEEE80211_AMPDU_TX_OPERATIONAL.
>>
>> Signed-off-by: Johannes Berg <[email protected]>
>
> Nice catch man!
>
> Reviewed-by: Luis R. Rodriguez <[email protected]>

Hm, maybe also a backported version could go to stable? Perhaps we can
trigger a panic on a UP box?

Luis