mac80211: indicate BA window size with IEEE80211_AMPDU_TX_OPERATIONAL drv_ampud_action

BA window size for a successful BA setup is not made available to the driver by mac80211. The patch below gets the BA window size from addba response and indicates it to driver through IEEE80211_AMPDU_TX_OPERATIONAL drv_ampdu_action.

---
 include/net/mac80211.h      |    3 ++-
 net/mac80211/agg-rx.c       |    4 ++--
 net/mac80211/agg-tx.c       |   17 ++++++++++-------
 net/mac80211/driver-ops.h   |    6 +++---
 net/mac80211/driver-trace.h |    2 +-
 net/mac80211/sta_info.h     |    1 +
 6 files changed, 19 insertions(+), 14 deletions(-)

index 7f256e2..29ff874 100644
@@ -1734,7 +1734,8 @@ struct ieee80211_ops {
 	int (*ampdu_action)(struct ieee80211_hw *hw,
 			    struct ieee80211_vif *vif,
 			    enum ieee80211_ampdu_mlme_action action,
-			    struct ieee80211_sta *sta, u16 tid, u16 *ssn);
+				struct ieee80211_sta *sta, u16 tid,
+				u16 buf_size, u16 *ssn);
 	int (*get_survey)(struct ieee80211_hw *hw, int idx,
 		struct survey_info *survey);
 	void (*rfkill_poll)(struct ieee80211_hw *hw);

index 965b272..8a2522f 100644
@@ -76,7 +76,7 @@ void ___ieee80211_stop_rx_ba_session(struct sta_info *sta, u16 tid,
 #endif /* CONFIG_MAC80211_HT_DEBUG */
 
 	if (drv_ampdu_action(local, sta->sdata, IEEE80211_AMPDU_RX_STOP,
-			     &sta->sta, tid, NULL))
+			     &sta->sta, tid, 0, NULL))
 		printk(KERN_DEBUG "HW problem - can not stop rx "
 				"aggregation for tid %d\n", tid);
 

@@ -274,7 +274,7 @@ void ieee80211_process_addba_request(struct ieee80211_local *local,
 	}
 
 	ret = drv_ampdu_action(local, sta->sdata, IEEE80211_AMPDU_RX_START,
-			       &sta->sta, tid, &start_seq_num);
+			       &sta->sta, tid, 0, &start_seq_num);
 #ifdef CONFIG_MAC80211_HT_DEBUG
 	printk(KERN_DEBUG "Rx A-MPDU request on tid %d result %d\n", tid, ret);
 #endif /* CONFIG_MAC80211_HT_DEBUG */

index c893f23..4332ca8 100644
@@ -186,7 +186,7 @@ int ___ieee80211_stop_tx_ba_session(struct sta_info *sta, u16 tid,
 
 	ret = drv_ampdu_action(local, sta->sdata,
 			       IEEE80211_AMPDU_TX_STOP,
-			       &sta->sta, tid, NULL);
+			       &sta->sta, tid, 0, NULL);
 
 	/* HW shall not deny going back to legacy */
 	if (WARN_ON(ret)) {

@@ -307,7 +307,7 @@ void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid)
 	start_seq_num = sta->tid_seq[tid] >> 4;
 
 	ret = drv_ampdu_action(local, sdata, IEEE80211_AMPDU_TX_START,
-			       &sta->sta, tid, &start_seq_num);
+			       &sta->sta, tid, 0, &start_seq_num);
 	if (ret) {
 #ifdef CONFIG_MAC80211_HT_DEBUG
 		printk(KERN_DEBUG "BA request denied - HW unavailable for"

@@ -470,7 +470,7 @@ ieee80211_agg_splice_finish(struct ieee80211_local *local, u16 tid)
 }
 
 static void ieee80211_agg_tx_operational(struct ieee80211_local *local,
-					 struct sta_info *sta, u16 tid)
+		struct sta_info *sta, u16 tid, u16 buf_size)
 {
 	lockdep_assert_held(&sta->ampdu_mlme.mtx);
 

@@ -480,7 +480,7 @@ static void ieee80211_agg_tx_operational(struct ieee80211_local *local,
 
 	drv_ampdu_action(local, sta->sdata,
 			 IEEE80211_AMPDU_TX_OPERATIONAL,
-			 &sta->sta, tid, NULL);
+			 &sta->sta, tid, buf_size, NULL);
 
 	/*
 	 * synchronize with TX path, while splicing the TX path

@@ -541,7 +541,7 @@ void ieee80211_start_tx_ba_cb(struct ieee80211_vif *vif, u8 *ra, u16 tid)
 		goto unlock;
 
 	if (test_bit(HT_AGG_STATE_RESPONSE_RECEIVED, &tid_tx->state))
-		ieee80211_agg_tx_operational(local, sta, tid);
+		ieee80211_agg_tx_operational(local, sta, tid, tid_tx->buf_size);
 
  unlock:
 	mutex_unlock(&sta->ampdu_mlme.mtx);

@@ -733,10 +733,11 @@ void ieee80211_process_addba_resp(struct ieee80211_local *local,
 				  size_t len)
 {
 	struct tid_ampdu_tx *tid_tx;
-	u16 capab, tid;
+	u16 capab, tid, buf_size;
 
 	capab = le16_to_cpu(mgmt->u.action.u.addba_resp.capab);
 	tid = (capab & IEEE80211_ADDBA_PARAM_TID_MASK) >> 2;
+    buf_size = (capab & IEEE80211_ADDBA_PARAM_BUF_SIZE_MASK) >> 6;
 
 	mutex_lock(&sta->ampdu_mlme.mtx);
 

@@ -753,6 +754,8 @@ void ieee80211_process_addba_resp(struct ieee80211_local *local,
 
 	del_timer(&tid_tx->addba_resp_timer);
 
+    tid_tx->buf_size = buf_size;
+
 #ifdef CONFIG_MAC80211_HT_DEBUG
 	printk(KERN_DEBUG "switched off addBA timer for tid %d\n", tid);
 #endif

@@ -766,7 +769,7 @@ void ieee80211_process_addba_resp(struct ieee80211_local *local,
 		}
 
 		if (test_bit(HT_AGG_STATE_DRV_READY, &tid_tx->state))
-			ieee80211_agg_tx_operational(local, sta, tid);
+			ieee80211_agg_tx_operational(local, sta, tid, buf_size);
 
 		sta->ampdu_mlme.addba_req_num[tid] = 0;
 	} else {

index 14123dc..383eee7 100644
@@ -354,17 +354,17 @@ static inline int drv_ampdu_action(struct ieee80211_local *local,
 				   struct ieee80211_sub_if_data *sdata,
 				   enum ieee80211_ampdu_mlme_action action,
 				   struct ieee80211_sta *sta, u16 tid,
-				   u16 *ssn)
+				   u16 buf_size, u16 *ssn)
 {
 	int ret = -EOPNOTSUPP;
 
 	might_sleep();
 
-	trace_drv_ampdu_action(local, sdata, action, sta, tid, ssn);
+	trace_drv_ampdu_action(local, sdata, action, sta, tid, buf_size, ssn);
 
 	if (local->ops->ampdu_action)
 		ret = local->ops->ampdu_action(&local->hw, &sdata->vif, action,
-					       sta, tid, ssn);
+					       sta, tid, buf_size, ssn);
 
 	trace_drv_return_int(local, ret);
 

index 5d5d2a9..c98bce0 100644
@@ -733,7 +733,7 @@ TRACE_EVENT(drv_ampdu_action,
 		 struct ieee80211_sub_if_data *sdata,
 		 enum ieee80211_ampdu_mlme_action action,
 		 struct ieee80211_sta *sta, u16 tid,
-		 u16 *ssn),
+		 u16 buf_size, u16 *ssn),
 
 	TP_ARGS(local, sdata, action, sta, tid, ssn),
 

index 54262e7..6685d4d 100644
@@ -95,6 +95,7 @@ struct tid_ampdu_tx {
 	unsigned long state;
 	u8 dialog_token;
 	u8 stop_initiator;
+    u16 buf_size;
 };
 
 /**

1.5.4.1

This patch seems to have a lot of whitespace damage. Please follow the normal practices, etc. Also, the Signed-off-by: line typically includes the full name (e.g. "Signed-off-by: Full Name <[email protected]>").

I didn't look too closely at the code itself, so I'll leave it to others to comment on that...

John

John W. Linville		Someday the world will need a hero, and you
[email protected]			might be all we have.  Be ready.

But why do you want that? Your patch and commit log do not mention why you need this. Also your patch does not address changing all of the other drivers, and breaks compilation so NACK, WTF!

When I compile mac80211 with your patch it fails compilation:

CHECK net/mac80211/main.c
net/mac80211/main.c:617:9: error: invalid bitfield width, -1.
CC [M] net/mac80211/main.o
In file included from net/mac80211/driver-ops.h:7,

                 from net/mac80211/main.c:29:

net/mac80211/driver-trace.h: In function ‘trace_drv_ampdu_action’:
net/mac80211/driver-trace.h:731: warning: passing argument 7 of ‘(void (*)(void *, struct ieee80211_local *, struct ieee80211_sub_if_data *, enum ieee80211_ampdu_mlme_action, struct ieee80211_sta *, u16, u16, u16 *))it_func’ makes integer from pointer without a cast net/mac80211/driver-trace.h:731: note: expected ‘u16’ but argument is of type ‘u16 *’
net/mac80211/driver-trace.h:731: error: too few arguments to function ‘(void (*)(void *, struct ieee80211_local *, struct ieee80211_sub_if_data *, enum ieee80211_ampdu_mlme_action, struct ieee80211_sta *, u16, u16, u16 *))it_func’
make[1]: *** [net/mac80211/main.o] Error 1
make: *** [_module_net/mac80211] Error 2

When I compile ath9k and ath9k_htc with this patch I also get:

CHECK drivers/net/wireless/ath/ath9k/main.c
drivers/net/wireless/ath/ath9k/main.c:2068:31: warning: incorrect type in initializer (incompatible argument 6 (different base types))
drivers/net/wireless/ath/ath9k/main.c:2068:31: expected int (
*ampdu_action )( ... )
drivers/net/wireless/ath/ath9k/main.c:2068:31: got int ( static
[toplevel] *<noident> )( ... )
CC [M] drivers/net/wireless/ath/ath9k/main.o
drivers/net/wireless/ath/ath9k/main.c:2068: warning: initialization from incompatible pointer type

CC [M] drivers/net/wireless/ath/ath9k/htc_drv_txrx.o
CHECK drivers/net/wireless/ath/ath9k/htc_drv_main.c
drivers/net/wireless/ath/ath9k/htc_drv_main.c:1846:31: warning:
incorrect type in initializer (incompatible argument 6 (different base types))
drivers/net/wireless/ath/ath9k/htc_drv_main.c:1846:31: expected int
( *ampdu_action )( ... )
drivers/net/wireless/ath/ath9k/htc_drv_main.c:1846:31: got int (
static [toplevel] *<noident> )( ... )
CC [M] drivers/net/wireless/ath/ath9k/htc_drv_main.o
drivers/net/wireless/ath/ath9k/htc_drv_main.c:1846: warning:
initialization from incompatible pointer type

In fact I'm surprised this even compiles with your patch. If you want to submit this make sure you use both checkpatch.pl to test your patch and also use sparse to compile test. You can get sparse from:

git://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.git

I recommend to use use the v0.4.2 tag:

git checkout -b rel42 v0.4.2
make
make install

Then when compiling to test your patch use C=1 to use sparse.

make C=1 M=drivers/net/wireless/ath/ath9k/

Please read the Developer's Certificate of Origin 1.1, on Documentation/SubmittingPatches.

Also please read:

http://wireless.kernel.org/en/developers/Documentation

Luis

Hi Luis,
Please see inline comments

BA window size from ADDBA response defines how many outstanding MPDUs are allowed for the BA stream by recipient. Since ADDBA response is processed in mac80211 stack, mac80211 should communicate the BA window size to driver (and may be from driver it is further communicated to firmware/hardware) to control the number of outstanding MPDUs while transmitting MPDUs for the stream.

I do not see this compilation error, I think the patch did not apply cleanly. I will send out another patch without extra white spaces that should solve the compilation issue. If the change is fine, we can then send patches to fix the compilation issues for other drivers too.

Thanks
Yogesh

What driver requires this? Your patch does all these changes but do not show any secondary patch of a user, so the patch is pointless right now.

..

Luis

Am Freitag 16 Juli 2010 schrieb Luis R. Rodriguez:

JFI, rt2800 would also need the BA windows size in its tx path (not only once but in every tx descriptor). So maybe it would even make sense to move it to the ieee80211_sta structure? That way rt2800 could also benefit from that change.

Using the approach as implemented in this patch we would have to store the value for each STA on our own in rt2x00 although mac80211 has it already in its own structures.

Just my 2 cents ...
Helmut