| Message ID | 1311675508-29005-2-git-send-email-helmut.schaa@googlemail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 :)
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); }
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(-)