[PATCH 1/2] mac80211: Stop TX BA session if buf_size is zero

[PATCH 1/2] mac80211: Stop TX BA session if buf_size is zero

[Helmut Schaa]

If we receive an ADDBA response with status code 0 and a buf_size of 0
we should stop the TX BA session as otherwise we'll end up queuing
frames in ieee80211_tx_prep_agg forever instead of sending them out as
non AMPDUs.

This fixes a problem with AVM Fritz Stick N wireless devices where
frames to this device are not transmitted anymore by mac80211.

Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---
 net/mac80211/agg-tx.c |   18 +++++++-----------
 1 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index c8be8ef..b7075f3 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -777,18 +777,14 @@ void ieee80211_process_addba_resp(struct ieee80211_local *local,
 #ifdef CONFIG_MAC80211_HT_DEBUG
 	printk(KERN_DEBUG "switched off addBA timer for tid %d\n", tid);
 #endif
-
+	/*
+	 * IEEE 802.11-2007 7.3.1.14:
+	 * In an ADDBA Response frame, when the Status Code field
+	 * is set to 0, the Buffer Size subfield is set to a value
+	 * of at least 1.
+	 */
 	if (le16_to_cpu(mgmt->u.action.u.addba_resp.status)
-			== WLAN_STATUS_SUCCESS) {
-		/*
-		 * IEEE 802.11-2007 7.3.1.14:
-		 * In an ADDBA Response frame, when the Status Code field
-		 * is set to 0, the Buffer Size subfield is set to a value
-		 * of at least 1.
-		 */
-		if (!buf_size)
-			goto out;
-
+			== WLAN_STATUS_SUCCESS && buf_size) {
 		if (test_and_set_bit(HT_AGG_STATE_RESPONSE_RECEIVED,
 				     &tid_tx->state)) {
 			/* ignore duplicate response */
-- 
1.7.3.4

[PATCH 2/2] mac80211: Don't use a buf_size=0 in ADDBA requests

[Helmut Schaa]

According to 802.11-2007, 7.3.1.14 it is compliant to use a buf_size of
0 in ADDBA requests. But some devices (AVM Fritz Stick N) arn't able to
handle that correctly and will reply with an ADDBA reponse with a
buf_size of 0 which in turn will disallow BA sessions for these
devices.

To work around this problem, if the hardware doesn't specify an upper
limit for the number of subframes in an AMPDU send the maximum 0x40 by
default in ADDBA requests.

Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---
 net/mac80211/agg-tx.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index b7075f3..13dbd00 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -345,7 +345,9 @@ void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid)
 	/* send AddBA request */
 	ieee80211_send_addba_request(sdata, sta->sta.addr, tid,
 				     tid_tx->dialog_token, start_seq_num,
-				     local->hw.max_tx_aggregation_subframes,
+				     local->hw.max_tx_aggregation_subframes ?
+				     local->hw.max_tx_aggregation_subframes :
+				     0x40,
 				     tid_tx->timeout);
 }
 
-- 
1.7.3.4

Re: [PATCH 2/2] mac80211: Don't use a buf_size=0 in ADDBA requests

[Kalle Valo]

Helmut Schaa <helmut.schaa@googlemail.com> writes:

> According to 802.11-2007, 7.3.1.14 it is compliant to use a buf_size of
> 0 in ADDBA requests. But some devices (AVM Fritz Stick N) arn't able to
> handle that correctly and will reply with an ADDBA reponse with a
> buf_size of 0 which in turn will disallow BA sessions for these
> devices.
>
> To work around this problem, if the hardware doesn't specify an upper
> limit for the number of subframes in an AMPDU send the maximum 0x40 by
> default in ADDBA requests.

[...]

> @@ -345,7 +345,9 @@ void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid)
>  	/* send AddBA request */
>  	ieee80211_send_addba_request(sdata, sta->sta.addr, tid,
>  				     tid_tx->dialog_token, start_seq_num,
> -				     local->hw.max_tx_aggregation_subframes,
> +				     local->hw.max_tx_aggregation_subframes ?
> +				     local->hw.max_tx_aggregation_subframes :
> +				     0x40,
>  				     tid_tx->timeout);

A define would be better than a magic value. This would also need a
comment but if you choose a good name for the define the comment won't
be needed. Also " ? :" inside a function call is not readable IMHO,
maybe instead a separate variable with if() statements?

-- 
Kalle Valo

Re: Re: [PATCH 2/2] mac80211: Don't use a buf_size=0 in ADDBA requests

[Helmut Schaa]

Am Freitag, 5. August 2011, 11:15:12 schrieb Kalle Valo:
> Helmut Schaa <helmut.schaa@googlemail.com> writes:
> 
> > According to 802.11-2007, 7.3.1.14 it is compliant to use a buf_size of
> > 0 in ADDBA requests. But some devices (AVM Fritz Stick N) arn't able to
> > handle that correctly and will reply with an ADDBA reponse with a
> > buf_size of 0 which in turn will disallow BA sessions for these
> > devices.
> >
> > To work around this problem, if the hardware doesn't specify an upper
> > limit for the number of subframes in an AMPDU send the maximum 0x40 by
> > default in ADDBA requests.
> 
> [...]
> 
> > @@ -345,7 +345,9 @@ void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid)
> >  	/* send AddBA request */
> >  	ieee80211_send_addba_request(sdata, sta->sta.addr, tid,
> >  				     tid_tx->dialog_token, start_seq_num,
> > -				     local->hw.max_tx_aggregation_subframes,
> > +				     local->hw.max_tx_aggregation_subframes ?
> > +				     local->hw.max_tx_aggregation_subframes :
> > +				     0x40,
> >  				     tid_tx->timeout);
> 
> A define would be better than a magic value. This would also need a
> comment but if you choose a good name for the define the comment won't
> be needed.

And we even have such a define in ieee80211.h already ;)

	#define IEEE80211_MAX_AMPDU_BUF 0x40

> Also " ? :" inside a function call is not readable IMHO,
> maybe instead a separate variable with if() statements?

Hmm, in this particular case it looks like overkill to me to use a
separate variable.

So, I'll respin this one with s/0x40/IEEE80211_MAX_AMPDU_BUF

Thanks,
Helmut

Re: [PATCH 2/2] mac80211: Don't use a buf_size=0 in ADDBA requests

[Kalle Valo]

Helmut Schaa <helmut.schaa@googlemail.com> writes:

>> Also " ? :" inside a function call is not readable IMHO,
>> maybe instead a separate variable with if() statements?
>
> Hmm, in this particular case it looks like overkill to me to use a
> separate variable.

To me it's overkill to optimise few lines with the cost of
readibility. Example:

ieee80211_send_addba_request(sdata, sta->sta.addr, tid,
                        tid_tx->dialog_token, start_seq_num,
                        local->hw.max_tx_aggregation_subframes ?
                        local->hw.max_tx_aggregation_subframes :
                        IEEE80211_MAX_AMPDU_BUF,
                        tid_tx->timeout);

vs.

ampdu_len = local->hw.max_tx_aggregation_subframes

if (ampdu_len == 0)
        ampdu_len = IEEE80211_MAX_AMPDU_BUF;

ieee80211_send_addba_request(sdata, sta->sta.addr, tid,
                        tid_tx->dialog_token, start_seq_num,
                        ampdu_len,
                        tid_tx->timeout);

So only three lines more (plus the variable declaration) but the code
is simpler to read because the if statement is not embedded to the
function call parameters.

But as always, people have different views about coding styles :)

-- 
Kalle Valo

[PATCHv2 2/2] mac80211: Don't use a buf_size=0 in ADDBA requests

[Helmut Schaa]

According to 802.11-2007, 7.3.1.14 it is compliant to use a buf_size of
0 in ADDBA requests. But some devices (AVM Fritz Stick N) arn't able to
handle that correctly and will reply with an ADDBA reponse with a
buf_size of 0 which in turn will disallow BA sessions for these
devices.

To work around this problem, initialize hw.max_tx_aggregation_subframes
to the maximum AMPDU buffer size 0x40.

Using 0 as default for the bufsize was introduced in commit
5dd36bc933e8be84f8369ac64505a2938f9ce036 (mac80211: allow advertising
correct maximum aggregate size).

Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---

 net/mac80211/main.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 866f269..104fdd9 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -608,6 +608,7 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
 	local->hw.max_rates = 1;
 	local->hw.max_report_rates = 0;
 	local->hw.max_rx_aggregation_subframes = IEEE80211_MAX_AMPDU_BUF;
+	local->hw.max_tx_aggregation_subframes = IEEE80211_MAX_AMPDU_BUF;
 	local->hw.conf.long_frame_max_tx_count = wiphy->retry_long;
 	local->hw.conf.short_frame_max_tx_count = wiphy->retry_short;
 	local->user_power_level = -1;
-- 
1.7.3.4

Re: [PATCHv2 2/2] mac80211: Don't use a buf_size=0 in ADDBA requests

[Johannes Berg]

On Fri, 2011-08-05 at 11:46 +0200, Helmut Schaa wrote:
> According to 802.11-2007, 7.3.1.14 it is compliant to use a buf_size of
> 0 in ADDBA requests. But some devices (AVM Fritz Stick N) arn't able to
> handle that correctly and will reply with an ADDBA reponse with a
> buf_size of 0 which in turn will disallow BA sessions for these
> devices.
> 
> To work around this problem, initialize hw.max_tx_aggregation_subframes
> to the maximum AMPDU buffer size 0x40.
> 
> Using 0 as default for the bufsize was introduced in commit
> 5dd36bc933e8be84f8369ac64505a2938f9ce036 (mac80211: allow advertising
> correct maximum aggregate size).

So, are you saying the problem with getting 0 in replies is solved by
not sending 0 in requests?

Patch looks fine to me, but it's not clear to me that it's the only
thing needed?

Acked-by: Johannes Berg <johannes@sipsolutions.net>

johannes

Re: [PATCHv2 2/2] mac80211: Don't use a buf_size=0 in ADDBA requests

[Helmut Schaa]

On Mon, Aug 8, 2011 at 10:29 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> So, are you saying the problem with getting 0 in replies is solved by
> not sending 0 in requests?

This specific device always replies with a buf_size=0 _if_ we send an
ADDBA request
with a buf_size=0.

The first patch in this series solves a problem were mac80211 would
buffer frames to
this STA forever when the ADDBA response contains a buf_size=0 since the BA
session is stuck in an intermediate state.

However, the AVM Fritz Stick N client behaves correctly if we're using
a buf_size != 0
in the ADDBA request. This is a bug in the Fritz Stick Windows driver
(it should use
the default size of its rx reorder buffer if the ADDBA request
contains 0). However, this
bug can be worked around by using a buf_size != 0 in ADDBA requests. And that's
what the second patch does.

Hope this clarifies my intention :)

Helmut

Re: [PATCHv2 2/2] mac80211: Don't use a buf_size=0 in ADDBA requests

[Johannes Berg]

On Mon, 2011-08-08 at 10:40 +0200, Helmut Schaa wrote:
> On Mon, Aug 8, 2011 at 10:29 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > So, are you saying the problem with getting 0 in replies is solved by
> > not sending 0 in requests?
> 
> This specific device always replies with a buf_size=0 _if_ we send an
> ADDBA request
> with a buf_size=0.
> 
> The first patch in this series solves a problem were mac80211 would
> buffer frames to
> this STA forever when the ADDBA response contains a buf_size=0 since the BA
> session is stuck in an intermediate state.
> 
> However, the AVM Fritz Stick N client behaves correctly if we're using
> a buf_size != 0
> in the ADDBA request. This is a bug in the Fritz Stick Windows driver
> (it should use
> the default size of its rx reorder buffer if the ADDBA request
> contains 0). However, this
> bug can be worked around by using a buf_size != 0 in ADDBA requests. And that's
> what the second patch does.
> 
> Hope this clarifies my intention :)

Yes, thanks.

johannes

Re: [PATCH 1/2] mac80211: Stop TX BA session if buf_size is zero

[Johannes Berg]

On Tue, 2011-07-26 at 12:18 +0200, Helmut Schaa wrote:
> If we receive an ADDBA response with status code 0 and a buf_size of 0
> we should stop the TX BA session as otherwise we'll end up queuing
> frames in ieee80211_tx_prep_agg forever instead of sending them out as
> non AMPDUs.
> 
> This fixes a problem with AVM Fritz Stick N wireless devices where
> frames to this device are not transmitted anymore by mac80211.

I guess this is acceptable since it shouldn't be happening anyway.

Acked-by: Johannes Berg <johannes@sipsolutions.net>

> Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
> ---
>  net/mac80211/agg-tx.c |   18 +++++++-----------
>  1 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
> index c8be8ef..b7075f3 100644
> --- a/net/mac80211/agg-tx.c
> +++ b/net/mac80211/agg-tx.c
> @@ -777,18 +777,14 @@ void ieee80211_process_addba_resp(struct ieee80211_local *local,
>  #ifdef CONFIG_MAC80211_HT_DEBUG
>  	printk(KERN_DEBUG "switched off addBA timer for tid %d\n", tid);
>  #endif
> -
> +	/*
> +	 * IEEE 802.11-2007 7.3.1.14:
> +	 * In an ADDBA Response frame, when the Status Code field
> +	 * is set to 0, the Buffer Size subfield is set to a value
> +	 * of at least 1.
> +	 */
>  	if (le16_to_cpu(mgmt->u.action.u.addba_resp.status)
> -			== WLAN_STATUS_SUCCESS) {
> -		/*
> -		 * IEEE 802.11-2007 7.3.1.14:
> -		 * In an ADDBA Response frame, when the Status Code field
> -		 * is set to 0, the Buffer Size subfield is set to a value
> -		 * of at least 1.
> -		 */
> -		if (!buf_size)
> -			goto out;
> -
> +			== WLAN_STATUS_SUCCESS && buf_size) {
>  		if (test_and_set_bit(HT_AGG_STATE_RESPONSE_RECEIVED,
>  				     &tid_tx->state)) {
>  			/* ignore duplicate response */