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.
Signed-off-by: [email protected]
Signed-off-by: [email protected]
---
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(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 7f256e2..29ff874 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -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);
diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index 965b272..8a2522f 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -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 */
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index c893f23..4332ca8 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -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 {
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 14123dc..383eee7 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -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);
diff --git a/net/mac80211/driver-trace.h b/net/mac80211/driver-trace.h
index 5d5d2a9..c98bce0 100644
--- a/net/mac80211/driver-trace.h
+++ b/net/mac80211/driver-trace.h
@@ -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),
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 54262e7..6685d4d 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -95,6 +95,7 @@ struct tid_ampdu_tx {
unsigned long state;
u8 dialog_token;
u8 stop_initiator;
+ u16 buf_size;
};
/**
--
1.5.4.1
On Fri, Jul 16, 2010 at 2:16 AM, yogeshp <[email protected]> wrote:
> Hi Luis,
> Please see inline comments
>
> Luis R. Rodriguez wrote:
>> On Thu, Jul 15, 2010 at 6:18 AM, yogeshp <[email protected]> wrote:
>>> 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.
>
>> But why do you want that? Your patch and commit log do not mention why
>> you need this.
> 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.
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.
>> When I compile mac80211 with your patch it fails compilation
> 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.
>
..
Luis
Am Freitag 16 Juli 2010 schrieb Luis R. Rodriguez:
> On Fri, Jul 16, 2010 at 2:16 AM, yogeshp <[email protected]> wrote:
> > Hi Luis,
> > Please see inline comments
> >
> > Luis R. Rodriguez wrote:
> >> On Thu, Jul 15, 2010 at 6:18 AM, yogeshp <[email protected]> wrote:
> >>> 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.
> >
> >> But why do you want that? Your patch and commit log do not mention why
> >> you need this.
> > 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.
>
> 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.
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
On Thu, Jul 15, 2010 at 6:18 AM, yogeshp <[email protected]> wrote:
> 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.
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/
> Signed-off-by: [email protected]
> Signed-off-by: [email protected]
Please read the Developer's Certificate of Origin 1.1, on
Documentation/SubmittingPatches.
Also please read:
http://wireless.kernel.org/en/developers/Documentation
Luis
On Thu, Jul 15, 2010 at 06:48:27PM +0530, yogeshp wrote:
> 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.
>
> Signed-off-by: [email protected]
> Signed-off-by: [email protected]
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.
Hi Luis,
Please see inline comments
Luis R. Rodriguez wrote:
> On Thu, Jul 15, 2010 at 6:18 AM, yogeshp <[email protected]> wrote:
>> 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.
> But why do you want that? Your patch and commit log do not mention why
> you need this.
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.
> When I compile mac80211 with your patch it fails compilation
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