* [RFC 0/2] fix aggregation buffer size limits @ 2011-01-12 12:13 Johannes Berg 2011-01-12 12:13 ` [RFC 1/2] mac80211: track receivers aggregation reorder buffer size Johannes Berg 2011-01-12 12:13 ` [RFC 2/2] iwlwifi: use maximum aggregation size Johannes Berg 0 siblings, 2 replies; 8+ messages in thread From: Johannes Berg @ 2011-01-12 12:13 UTC (permalink / raw) To: linux-wireless Luca and I found that we do not correctly handle peers that want a limit (other than 64) on the number of subframes in an aggregate. Here's a completely untested attempt at fixing this. Comments welcome. johannes ^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC 1/2] mac80211: track receivers aggregation reorder buffer size 2011-01-12 12:13 [RFC 0/2] fix aggregation buffer size limits Johannes Berg @ 2011-01-12 12:13 ` Johannes Berg 2011-01-12 13:01 ` Sujith 2011-01-12 16:51 ` Guy, Wey-Yi 2011-01-12 12:13 ` [RFC 2/2] iwlwifi: use maximum aggregation size Johannes Berg 1 sibling, 2 replies; 8+ messages in thread From: Johannes Berg @ 2011-01-12 12:13 UTC (permalink / raw) To: linux-wireless From: Johannes Berg <johannes.berg@intel.com> The aggregation code currently doesn't implement the buffer size negotiation. It will always request a max buffer size (which is fine, if a little pointless, as the mac80211 code doesn't know and might just use 0 instead), but if the peer requests a smaller size it isn't possible to honour this request. In order to fix this, look at the buffer size in the addBA response frame, keep track of it and pass it to the driver in the ampdu_action callback when called with the IEEE80211_AMPDU_TX_OPERATIONAL action. That way the driver can limit the number of subframes in aggregates appropriately. Note that this doesn't fix any drivers apart from the addition of the new argument -- they all need to be updated separately to use this variable! Signed-off-by: Johannes Berg <johannes.berg@intel.com> --- drivers/net/wireless/ath/ar9170/main.c | 3 ++- drivers/net/wireless/ath/ath9k/htc_drv_main.c | 2 +- drivers/net/wireless/ath/ath9k/main.c | 2 +- drivers/net/wireless/ath/carl9170/main.c | 2 +- drivers/net/wireless/iwlwifi/iwl-agn.c | 3 ++- drivers/net/wireless/iwlwifi/iwl-agn.h | 3 ++- drivers/net/wireless/mac80211_hwsim.c | 3 ++- drivers/net/wireless/mwl8k.c | 3 ++- drivers/net/wireless/rt2x00/rt2800lib.c | 3 ++- drivers/net/wireless/rt2x00/rt2800lib.h | 3 ++- drivers/net/wireless/rtlwifi/core.c | 3 ++- include/net/mac80211.h | 7 ++++++- net/mac80211/agg-rx.c | 4 ++-- net/mac80211/agg-tx.c | 20 +++++++++++++++++--- net/mac80211/driver-ops.h | 6 +++--- net/mac80211/driver-trace.h | 11 +++++++---- net/mac80211/sta_info.h | 2 ++ 17 files changed, 56 insertions(+), 24 deletions(-) --- wireless-testing.orig/include/net/mac80211.h 2011-01-12 13:02:27.000000000 +0100 +++ wireless-testing/include/net/mac80211.h 2011-01-12 13:02:36.000000000 +0100 @@ -1723,6 +1723,10 @@ enum ieee80211_ampdu_mlme_action { * ieee80211_ampdu_mlme_action. Starting sequence number (@ssn) * is the first frame we expect to perform the action on. Notice * that TX/RX_STOP can pass NULL for this parameter. + * The @buf_size parameter is only valid when the action is set to + * %IEEE80211_AMPDU_TX_OPERATIONAL and indicates the peer's reorder + * buffer size (number of subframes) for this session -- aggregates + * containing more subframes than this may not be transmitted to the peer. * Returns a negative error code on failure. * The callback can sleep. * @@ -1825,7 +1829,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 *ssn, + u8 buf_size); int (*get_survey)(struct ieee80211_hw *hw, int idx, struct survey_info *survey); void (*rfkill_poll)(struct ieee80211_hw *hw); --- wireless-testing.orig/net/mac80211/agg-tx.c 2011-01-12 13:02:27.000000000 +0100 +++ wireless-testing/net/mac80211/agg-tx.c 2011-01-12 13:02:36.000000000 +0100 @@ -190,7 +190,7 @@ int ___ieee80211_stop_tx_ba_session(stru ret = drv_ampdu_action(local, sta->sdata, IEEE80211_AMPDU_TX_STOP, - &sta->sta, tid, NULL); + &sta->sta, tid, NULL, 0); /* HW shall not deny going back to legacy */ if (WARN_ON(ret)) { @@ -311,7 +311,7 @@ void ieee80211_tx_ba_session_handle_star 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, &start_seq_num, 0); if (ret) { #ifdef CONFIG_MAC80211_HT_DEBUG printk(KERN_DEBUG "BA request denied - HW unavailable for" @@ -487,7 +487,8 @@ static void ieee80211_agg_tx_operational drv_ampdu_action(local, sta->sdata, IEEE80211_AMPDU_TX_OPERATIONAL, - &sta->sta, tid, NULL); + &sta->sta, tid, NULL, + sta->ampdu_mlme.tid_tx[tid]->buf_size); /* * synchronize with TX path, while splicing the TX path @@ -742,9 +743,11 @@ void ieee80211_process_addba_resp(struct { struct tid_ampdu_tx *tid_tx; u16 capab, tid; + u8 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); @@ -767,12 +770,23 @@ void ieee80211_process_addba_resp(struct 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; + if (test_and_set_bit(HT_AGG_STATE_RESPONSE_RECEIVED, &tid_tx->state)) { /* ignore duplicate response */ goto out; } + tid_tx->buf_size = buf_size; + if (test_bit(HT_AGG_STATE_DRV_READY, &tid_tx->state)) ieee80211_agg_tx_operational(local, sta, tid); --- wireless-testing.orig/net/mac80211/sta_info.h 2011-01-12 13:02:28.000000000 +0100 +++ wireless-testing/net/mac80211/sta_info.h 2011-01-12 13:02:36.000000000 +0100 @@ -82,6 +82,7 @@ enum ieee80211_sta_info_flags { * @state: session state (see above) * @stop_initiator: initiator of a session stop * @tx_stop: TX DelBA frame when stopping + * @buf_size: reorder buffer size at receiver * * This structure's lifetime is managed by RCU, assignments to * the array holding it must hold the aggregation mutex. @@ -101,6 +102,7 @@ struct tid_ampdu_tx { u8 dialog_token; u8 stop_initiator; bool tx_stop; + u8 buf_size; }; /** --- wireless-testing.orig/net/mac80211/agg-rx.c 2011-01-12 13:02:27.000000000 +0100 +++ wireless-testing/net/mac80211/agg-rx.c 2011-01-12 13:02:36.000000000 +0100 @@ -76,7 +76,7 @@ void ___ieee80211_stop_rx_ba_session(str #endif /* CONFIG_MAC80211_HT_DEBUG */ if (drv_ampdu_action(local, sta->sdata, IEEE80211_AMPDU_RX_STOP, - &sta->sta, tid, NULL)) + &sta->sta, tid, NULL, 0)) printk(KERN_DEBUG "HW problem - can not stop rx " "aggregation for tid %d\n", tid); @@ -294,7 +294,7 @@ void ieee80211_process_addba_request(str } ret = drv_ampdu_action(local, sta->sdata, IEEE80211_AMPDU_RX_START, - &sta->sta, tid, &start_seq_num); + &sta->sta, tid, &start_seq_num, 0); #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 */ --- wireless-testing.orig/net/mac80211/driver-ops.h 2011-01-12 13:02:27.000000000 +0100 +++ wireless-testing/net/mac80211/driver-ops.h 2011-01-12 13:02:36.000000000 +0100 @@ -382,17 +382,17 @@ static inline int drv_ampdu_action(struc struct ieee80211_sub_if_data *sdata, enum ieee80211_ampdu_mlme_action action, struct ieee80211_sta *sta, u16 tid, - u16 *ssn) + u16 *ssn, u8 buf_size) { int ret = -EOPNOTSUPP; might_sleep(); - trace_drv_ampdu_action(local, sdata, action, sta, tid, ssn); + trace_drv_ampdu_action(local, sdata, action, sta, tid, ssn, buf_size); if (local->ops->ampdu_action) ret = local->ops->ampdu_action(&local->hw, &sdata->vif, action, - sta, tid, ssn); + sta, tid, ssn, buf_size); trace_drv_return_int(local, ret); --- wireless-testing.orig/net/mac80211/driver-trace.h 2011-01-12 13:02:27.000000000 +0100 +++ wireless-testing/net/mac80211/driver-trace.h 2011-01-12 13:02:36.000000000 +0100 @@ -784,9 +784,9 @@ 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 *ssn, u8 buf_size), - TP_ARGS(local, sdata, action, sta, tid, ssn), + TP_ARGS(local, sdata, action, sta, tid, ssn, buf_size), TP_STRUCT__entry( LOCAL_ENTRY @@ -794,6 +794,7 @@ TRACE_EVENT(drv_ampdu_action, __field(u32, action) __field(u16, tid) __field(u16, ssn) + __field(u8, buf_size) VIF_ENTRY ), @@ -804,11 +805,13 @@ TRACE_EVENT(drv_ampdu_action, __entry->action = action; __entry->tid = tid; __entry->ssn = ssn ? *ssn : 0; + __entry->buf_size = buf_size; ), TP_printk( - LOCAL_PR_FMT VIF_PR_FMT STA_PR_FMT " action:%d tid:%d", - LOCAL_PR_ARG, VIF_PR_ARG, STA_PR_ARG, __entry->action, __entry->tid + LOCAL_PR_FMT VIF_PR_FMT STA_PR_FMT " action:%d tid:%d buf:%d", + LOCAL_PR_ARG, VIF_PR_ARG, STA_PR_ARG, __entry->action, + __entry->tid, __entry->buf_size ) ); --- wireless-testing.orig/drivers/net/wireless/ath/ar9170/main.c 2011-01-12 13:02:27.000000000 +0100 +++ wireless-testing/drivers/net/wireless/ath/ar9170/main.c 2011-01-12 13:02:36.000000000 +0100 @@ -1945,7 +1945,8 @@ static int ar9170_conf_tx(struct ieee802 static int ar9170_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 *ssn, + u8 buf_size) { switch (action) { case IEEE80211_AMPDU_RX_START: --- wireless-testing.orig/drivers/net/wireless/ath/ath9k/htc_drv_main.c 2011-01-12 13:02:27.000000000 +0100 +++ wireless-testing/drivers/net/wireless/ath/ath9k/htc_drv_main.c 2011-01-12 13:02:36.000000000 +0100 @@ -1548,7 +1548,7 @@ static int ath9k_htc_ampdu_action(struct struct ieee80211_vif *vif, enum ieee80211_ampdu_mlme_action action, struct ieee80211_sta *sta, - u16 tid, u16 *ssn) + u16 tid, u16 *ssn, u8 buf_size) { struct ath9k_htc_priv *priv = hw->priv; struct ath9k_htc_sta *ista; --- wireless-testing.orig/drivers/net/wireless/ath/ath9k/main.c 2011-01-12 13:02:27.000000000 +0100 +++ wireless-testing/drivers/net/wireless/ath/ath9k/main.c 2011-01-12 13:02:36.000000000 +0100 @@ -2018,7 +2018,7 @@ static int ath9k_ampdu_action(struct iee struct ieee80211_vif *vif, enum ieee80211_ampdu_mlme_action action, struct ieee80211_sta *sta, - u16 tid, u16 *ssn) + u16 tid, u16 *ssn, u8 buf_size) { struct ath_wiphy *aphy = hw->priv; struct ath_softc *sc = aphy->sc; --- wireless-testing.orig/drivers/net/wireless/ath/carl9170/main.c 2011-01-12 13:02:27.000000000 +0100 +++ wireless-testing/drivers/net/wireless/ath/carl9170/main.c 2011-01-12 13:02:36.000000000 +0100 @@ -1279,7 +1279,7 @@ static int carl9170_op_ampdu_action(stru struct ieee80211_vif *vif, enum ieee80211_ampdu_mlme_action action, struct ieee80211_sta *sta, - u16 tid, u16 *ssn) + u16 tid, u16 *ssn, u8 buf_size) { struct ar9170 *ar = hw->priv; struct carl9170_sta_info *sta_info = (void *) sta->drv_priv; --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-agn.c 2011-01-12 13:02:35.000000000 +0100 +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-agn.c 2011-01-12 13:02:36.000000000 +0100 @@ -3416,7 +3416,8 @@ int iwlagn_mac_set_key(struct ieee80211_ int iwlagn_mac_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 *ssn, + u8 buf_size) { struct iwl_priv *priv = hw->priv; int ret = -EINVAL; --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-agn.h 2011-01-12 13:02:27.000000000 +0100 +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-agn.h 2011-01-12 13:02:36.000000000 +0100 @@ -363,7 +363,8 @@ void iwlagn_mac_update_tkip_key(struct i int iwlagn_mac_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 *ssn, + u8 buf_size); int iwlagn_mac_sta_add(struct ieee80211_hw *hw, struct ieee80211_vif *vif, struct ieee80211_sta *sta); --- wireless-testing.orig/drivers/net/wireless/mac80211_hwsim.c 2011-01-12 13:02:27.000000000 +0100 +++ wireless-testing/drivers/net/wireless/mac80211_hwsim.c 2011-01-12 13:02:36.000000000 +0100 @@ -943,7 +943,8 @@ static int mac80211_hwsim_testmode_cmd(s static int mac80211_hwsim_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 *ssn, + u8 buf_size) { switch (action) { case IEEE80211_AMPDU_TX_START: --- wireless-testing.orig/drivers/net/wireless/mwl8k.c 2011-01-12 13:02:27.000000000 +0100 +++ wireless-testing/drivers/net/wireless/mwl8k.c 2011-01-12 13:02:36.000000000 +0100 @@ -3932,7 +3932,8 @@ static int mwl8k_get_survey(struct ieee8 static int mwl8k_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 *ssn, + u8 buf_size) { switch (action) { case IEEE80211_AMPDU_RX_START: --- wireless-testing.orig/drivers/net/wireless/rt2x00/rt2800lib.c 2011-01-12 13:02:27.000000000 +0100 +++ wireless-testing/drivers/net/wireless/rt2x00/rt2800lib.c 2011-01-12 13:02:36.000000000 +0100 @@ -3530,7 +3530,8 @@ EXPORT_SYMBOL_GPL(rt2800_get_tsf); int rt2800_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 *ssn, + u8 buf_size) { int ret = 0; --- wireless-testing.orig/drivers/net/wireless/rt2x00/rt2800lib.h 2011-01-12 13:02:27.000000000 +0100 +++ wireless-testing/drivers/net/wireless/rt2x00/rt2800lib.h 2011-01-12 13:02:36.000000000 +0100 @@ -198,7 +198,8 @@ int rt2800_conf_tx(struct ieee80211_hw * u64 rt2800_get_tsf(struct ieee80211_hw *hw); int rt2800_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 *ssn, + u8 buf_size); int rt2800_get_survey(struct ieee80211_hw *hw, int idx, struct survey_info *survey); --- wireless-testing.orig/drivers/net/wireless/rtlwifi/core.c 2011-01-12 13:02:27.000000000 +0100 +++ wireless-testing/drivers/net/wireless/rtlwifi/core.c 2011-01-12 13:02:36.000000000 +0100 @@ -748,7 +748,8 @@ static void rtl_op_sta_notify(struct iee static int rtl_op_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 *ssn, + u8 buf_size) { struct rtl_priv *rtlpriv = rtl_priv(hw); ^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC 1/2] mac80211: track receivers aggregation reorder buffer size 2011-01-12 12:13 ` [RFC 1/2] mac80211: track receivers aggregation reorder buffer size Johannes Berg @ 2011-01-12 13:01 ` Sujith 2011-01-12 16:51 ` Guy, Wey-Yi 1 sibling, 0 replies; 8+ messages in thread From: Sujith @ 2011-01-12 13:01 UTC (permalink / raw) To: Johannes Berg; +Cc: linux-wireless Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > The aggregation code currently doesn't implement the > buffer size negotiation. It will always request a max > buffer size (which is fine, if a little pointless, as > the mac80211 code doesn't know and might just use 0 > instead), but if the peer requests a smaller size it > isn't possible to honour this request. > > In order to fix this, look at the buffer size in the > addBA response frame, keep track of it and pass it to > the driver in the ampdu_action callback when called > with the IEEE80211_AMPDU_TX_OPERATIONAL action. That > way the driver can limit the number of subframes in > aggregates appropriately. > > Note that this doesn't fix any drivers apart from the > addition of the new argument -- they all need to be > updated separately to use this variable! At least for ath9k_htc, the fix is pretty simple, something like the patch below. I tested this with a peer advertising a non-default subframe count (using max_tx_aggregation_subframes) and it seems to work. Now all that is needed is to fix the FW to actually _use_ this value. :) diff --git a/drivers/net/wireless/ath/ath9k/htc.h b/drivers/net/wireless/ath/ath9k/htc.h index 1ce506f..94a4b14 100644 --- a/drivers/net/wireless/ath/ath9k/htc.h +++ b/drivers/net/wireless/ath/ath9k/htc.h @@ -210,6 +210,7 @@ struct ath9k_htc_vif { #define ATH9K_HTC_MAX_STA 8 #define ATH9K_HTC_MAX_TID 8 +#define ATH9K_HTC_MAX_SUBFRAME 20 enum tid_aggr_state { AGGR_STOP = 0, @@ -433,7 +434,7 @@ void ath9k_htc_txep(void *priv, struct sk_buff *skb, enum htc_endpoint_id ep_id, void ath9k_htc_beaconep(void *drv_priv, struct sk_buff *skb, enum htc_endpoint_id ep_id, bool txok); -int ath9k_htc_update_cap_target(struct ath9k_htc_priv *priv); +int ath9k_htc_update_cap_target(struct ath9k_htc_priv *priv, u8 buf_size); void ath9k_htc_station_work(struct work_struct *work); void ath9k_htc_aggr_work(struct work_struct *work); void ath9k_ani_work(struct work_struct *work);; diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c b/drivers/net/wireless/ath/ath9k/htc_drv_main.c index f4d576b..3a3f982 100644 --- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c +++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c @@ -304,7 +304,7 @@ static int ath9k_htc_add_monitor_interface(struct ath9k_htc_priv *priv) /* * Set chainmask etc. on the target. */ - ret = ath9k_htc_update_cap_target(priv); + ret = ath9k_htc_update_cap_target(priv, ATH9K_HTC_MAX_SUBFRAME); if (ret) ath_dbg(common, ATH_DBG_CONFIG, "Failed to update capability in target\n"); @@ -430,7 +430,7 @@ static int ath9k_htc_remove_station(struct ath9k_htc_priv *priv, return 0; } -int ath9k_htc_update_cap_target(struct ath9k_htc_priv *priv) +int ath9k_htc_update_cap_target(struct ath9k_htc_priv *priv, u8 buf_size) { struct ath9k_htc_cap_target tcap; int ret; @@ -442,7 +442,7 @@ int ath9k_htc_update_cap_target(struct ath9k_htc_priv *priv) tcap.flags = 0x240c40; tcap.flags_ext = 0x80601000; tcap.ampdu_limit = 0xffff0000; - tcap.ampdu_subframes = 20; + tcap.ampdu_subframes = min((u8)ATH9K_HTC_MAX_SUBFRAME, buf_size); tcap.tx_chainmask_legacy = priv->ah->caps.tx_chainmask; tcap.protmode = 1; tcap.tx_chainmask = priv->ah->caps.tx_chainmask; @@ -1130,7 +1130,7 @@ static int ath9k_htc_add_interface(struct ieee80211_hw *hw, if (ret) goto out; - ret = ath9k_htc_update_cap_target(priv); + ret = ath9k_htc_update_cap_target(priv, ATH9K_HTC_MAX_SUBFRAME); if (ret) ath_dbg(common, ATH_DBG_CONFIG, "Failed to update capability in target\n"); @@ -1548,7 +1548,7 @@ static int ath9k_htc_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif, enum ieee80211_ampdu_mlme_action action, struct ieee80211_sta *sta, - u16 tid, u16 *ssn) + u16 tid, u16 *ssn, u8 buf_size) { struct ath9k_htc_priv *priv = hw->priv; struct ath9k_htc_sta *ista; @@ -1570,6 +1570,12 @@ static int ath9k_htc_ampdu_action(struct ieee80211_hw *hw, break; case IEEE80211_AMPDU_TX_OPERATIONAL: ista = (struct ath9k_htc_sta *) sta->drv_priv; + if (buf_size < ATH9K_HTC_MAX_SUBFRAME) { + ath_dbg(ath9k_hw_common(priv->ah), ATH_DBG_CONFIG, + "Choosing subframe count %d for station %pM\n", + buf_size, sta->addr); + ath9k_htc_update_cap_target(priv, buf_size); + } spin_lock_bh(&priv->tx_lock); ista->tid_state[tid] = AGGR_OPERATIONAL; spin_unlock_bh(&priv->tx_lock); ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC 1/2] mac80211: track receivers aggregation reorder buffer size 2011-01-12 12:13 ` [RFC 1/2] mac80211: track receivers aggregation reorder buffer size Johannes Berg 2011-01-12 13:01 ` Sujith @ 2011-01-12 16:51 ` Guy, Wey-Yi 1 sibling, 0 replies; 8+ messages in thread From: Guy, Wey-Yi @ 2011-01-12 16:51 UTC (permalink / raw) To: Johannes Berg; +Cc: linux-wireless Johannes, ok, got it Wey On Wed, 2011-01-12 at 04:13 -0800, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > The aggregation code currently doesn't implement the > buffer size negotiation. It will always request a max > buffer size (which is fine, if a little pointless, as > the mac80211 code doesn't know and might just use 0 > instead), but if the peer requests a smaller size it > isn't possible to honour this request. > > In order to fix this, look at the buffer size in the > addBA response frame, keep track of it and pass it to > the driver in the ampdu_action callback when called > with the IEEE80211_AMPDU_TX_OPERATIONAL action. That > way the driver can limit the number of subframes in > aggregates appropriately. > > Note that this doesn't fix any drivers apart from the > addition of the new argument -- they all need to be > updated separately to use this variable! > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > --- > drivers/net/wireless/ath/ar9170/main.c | 3 ++- > drivers/net/wireless/ath/ath9k/htc_drv_main.c | 2 +- > drivers/net/wireless/ath/ath9k/main.c | 2 +- > drivers/net/wireless/ath/carl9170/main.c | 2 +- > drivers/net/wireless/iwlwifi/iwl-agn.c | 3 ++- > drivers/net/wireless/iwlwifi/iwl-agn.h | 3 ++- > drivers/net/wireless/mac80211_hwsim.c | 3 ++- > drivers/net/wireless/mwl8k.c | 3 ++- > drivers/net/wireless/rt2x00/rt2800lib.c | 3 ++- > drivers/net/wireless/rt2x00/rt2800lib.h | 3 ++- > drivers/net/wireless/rtlwifi/core.c | 3 ++- > include/net/mac80211.h | 7 ++++++- > net/mac80211/agg-rx.c | 4 ++-- > net/mac80211/agg-tx.c | 20 +++++++++++++++++--- > net/mac80211/driver-ops.h | 6 +++--- > net/mac80211/driver-trace.h | 11 +++++++---- > net/mac80211/sta_info.h | 2 ++ > 17 files changed, 56 insertions(+), 24 deletions(-) > > --- wireless-testing.orig/include/net/mac80211.h 2011-01-12 13:02:27.000000000 +0100 > +++ wireless-testing/include/net/mac80211.h 2011-01-12 13:02:36.000000000 +0100 > @@ -1723,6 +1723,10 @@ enum ieee80211_ampdu_mlme_action { > * ieee80211_ampdu_mlme_action. Starting sequence number (@ssn) > * is the first frame we expect to perform the action on. Notice > * that TX/RX_STOP can pass NULL for this parameter. > + * The @buf_size parameter is only valid when the action is set to > + * %IEEE80211_AMPDU_TX_OPERATIONAL and indicates the peer's reorder > + * buffer size (number of subframes) for this session -- aggregates > + * containing more subframes than this may not be transmitted to the peer. > * Returns a negative error code on failure. > * The callback can sleep. > * > @@ -1825,7 +1829,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 *ssn, > + u8 buf_size); > int (*get_survey)(struct ieee80211_hw *hw, int idx, > struct survey_info *survey); > void (*rfkill_poll)(struct ieee80211_hw *hw); > --- wireless-testing.orig/net/mac80211/agg-tx.c 2011-01-12 13:02:27.000000000 +0100 > +++ wireless-testing/net/mac80211/agg-tx.c 2011-01-12 13:02:36.000000000 +0100 > @@ -190,7 +190,7 @@ int ___ieee80211_stop_tx_ba_session(stru > > ret = drv_ampdu_action(local, sta->sdata, > IEEE80211_AMPDU_TX_STOP, > - &sta->sta, tid, NULL); > + &sta->sta, tid, NULL, 0); > > /* HW shall not deny going back to legacy */ > if (WARN_ON(ret)) { > @@ -311,7 +311,7 @@ void ieee80211_tx_ba_session_handle_star > 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, &start_seq_num, 0); > if (ret) { > #ifdef CONFIG_MAC80211_HT_DEBUG > printk(KERN_DEBUG "BA request denied - HW unavailable for" > @@ -487,7 +487,8 @@ static void ieee80211_agg_tx_operational > > drv_ampdu_action(local, sta->sdata, > IEEE80211_AMPDU_TX_OPERATIONAL, > - &sta->sta, tid, NULL); > + &sta->sta, tid, NULL, > + sta->ampdu_mlme.tid_tx[tid]->buf_size); > > /* > * synchronize with TX path, while splicing the TX path > @@ -742,9 +743,11 @@ void ieee80211_process_addba_resp(struct > { > struct tid_ampdu_tx *tid_tx; > u16 capab, tid; > + u8 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); > > @@ -767,12 +770,23 @@ void ieee80211_process_addba_resp(struct > > 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; > + > if (test_and_set_bit(HT_AGG_STATE_RESPONSE_RECEIVED, > &tid_tx->state)) { > /* ignore duplicate response */ > goto out; > } > > + tid_tx->buf_size = buf_size; > + > if (test_bit(HT_AGG_STATE_DRV_READY, &tid_tx->state)) > ieee80211_agg_tx_operational(local, sta, tid); > > --- wireless-testing.orig/net/mac80211/sta_info.h 2011-01-12 13:02:28.000000000 +0100 > +++ wireless-testing/net/mac80211/sta_info.h 2011-01-12 13:02:36.000000000 +0100 > @@ -82,6 +82,7 @@ enum ieee80211_sta_info_flags { > * @state: session state (see above) > * @stop_initiator: initiator of a session stop > * @tx_stop: TX DelBA frame when stopping > + * @buf_size: reorder buffer size at receiver > * > * This structure's lifetime is managed by RCU, assignments to > * the array holding it must hold the aggregation mutex. > @@ -101,6 +102,7 @@ struct tid_ampdu_tx { > u8 dialog_token; > u8 stop_initiator; > bool tx_stop; > + u8 buf_size; > }; > > /** > --- wireless-testing.orig/net/mac80211/agg-rx.c 2011-01-12 13:02:27.000000000 +0100 > +++ wireless-testing/net/mac80211/agg-rx.c 2011-01-12 13:02:36.000000000 +0100 > @@ -76,7 +76,7 @@ void ___ieee80211_stop_rx_ba_session(str > #endif /* CONFIG_MAC80211_HT_DEBUG */ > > if (drv_ampdu_action(local, sta->sdata, IEEE80211_AMPDU_RX_STOP, > - &sta->sta, tid, NULL)) > + &sta->sta, tid, NULL, 0)) > printk(KERN_DEBUG "HW problem - can not stop rx " > "aggregation for tid %d\n", tid); > > @@ -294,7 +294,7 @@ void ieee80211_process_addba_request(str > } > > ret = drv_ampdu_action(local, sta->sdata, IEEE80211_AMPDU_RX_START, > - &sta->sta, tid, &start_seq_num); > + &sta->sta, tid, &start_seq_num, 0); > #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 */ > --- wireless-testing.orig/net/mac80211/driver-ops.h 2011-01-12 13:02:27.000000000 +0100 > +++ wireless-testing/net/mac80211/driver-ops.h 2011-01-12 13:02:36.000000000 +0100 > @@ -382,17 +382,17 @@ static inline int drv_ampdu_action(struc > struct ieee80211_sub_if_data *sdata, > enum ieee80211_ampdu_mlme_action action, > struct ieee80211_sta *sta, u16 tid, > - u16 *ssn) > + u16 *ssn, u8 buf_size) > { > int ret = -EOPNOTSUPP; > > might_sleep(); > > - trace_drv_ampdu_action(local, sdata, action, sta, tid, ssn); > + trace_drv_ampdu_action(local, sdata, action, sta, tid, ssn, buf_size); > > if (local->ops->ampdu_action) > ret = local->ops->ampdu_action(&local->hw, &sdata->vif, action, > - sta, tid, ssn); > + sta, tid, ssn, buf_size); > > trace_drv_return_int(local, ret); > > --- wireless-testing.orig/net/mac80211/driver-trace.h 2011-01-12 13:02:27.000000000 +0100 > +++ wireless-testing/net/mac80211/driver-trace.h 2011-01-12 13:02:36.000000000 +0100 > @@ -784,9 +784,9 @@ 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 *ssn, u8 buf_size), > > - TP_ARGS(local, sdata, action, sta, tid, ssn), > + TP_ARGS(local, sdata, action, sta, tid, ssn, buf_size), > > TP_STRUCT__entry( > LOCAL_ENTRY > @@ -794,6 +794,7 @@ TRACE_EVENT(drv_ampdu_action, > __field(u32, action) > __field(u16, tid) > __field(u16, ssn) > + __field(u8, buf_size) > VIF_ENTRY > ), > > @@ -804,11 +805,13 @@ TRACE_EVENT(drv_ampdu_action, > __entry->action = action; > __entry->tid = tid; > __entry->ssn = ssn ? *ssn : 0; > + __entry->buf_size = buf_size; > ), > > TP_printk( > - LOCAL_PR_FMT VIF_PR_FMT STA_PR_FMT " action:%d tid:%d", > - LOCAL_PR_ARG, VIF_PR_ARG, STA_PR_ARG, __entry->action, __entry->tid > + LOCAL_PR_FMT VIF_PR_FMT STA_PR_FMT " action:%d tid:%d buf:%d", > + LOCAL_PR_ARG, VIF_PR_ARG, STA_PR_ARG, __entry->action, > + __entry->tid, __entry->buf_size > ) > ); > > --- wireless-testing.orig/drivers/net/wireless/ath/ar9170/main.c 2011-01-12 13:02:27.000000000 +0100 > +++ wireless-testing/drivers/net/wireless/ath/ar9170/main.c 2011-01-12 13:02:36.000000000 +0100 > @@ -1945,7 +1945,8 @@ static int ar9170_conf_tx(struct ieee802 > static int ar9170_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 *ssn, > + u8 buf_size) > { > switch (action) { > case IEEE80211_AMPDU_RX_START: > --- wireless-testing.orig/drivers/net/wireless/ath/ath9k/htc_drv_main.c 2011-01-12 13:02:27.000000000 +0100 > +++ wireless-testing/drivers/net/wireless/ath/ath9k/htc_drv_main.c 2011-01-12 13:02:36.000000000 +0100 > @@ -1548,7 +1548,7 @@ static int ath9k_htc_ampdu_action(struct > struct ieee80211_vif *vif, > enum ieee80211_ampdu_mlme_action action, > struct ieee80211_sta *sta, > - u16 tid, u16 *ssn) > + u16 tid, u16 *ssn, u8 buf_size) > { > struct ath9k_htc_priv *priv = hw->priv; > struct ath9k_htc_sta *ista; > --- wireless-testing.orig/drivers/net/wireless/ath/ath9k/main.c 2011-01-12 13:02:27.000000000 +0100 > +++ wireless-testing/drivers/net/wireless/ath/ath9k/main.c 2011-01-12 13:02:36.000000000 +0100 > @@ -2018,7 +2018,7 @@ static int ath9k_ampdu_action(struct iee > struct ieee80211_vif *vif, > enum ieee80211_ampdu_mlme_action action, > struct ieee80211_sta *sta, > - u16 tid, u16 *ssn) > + u16 tid, u16 *ssn, u8 buf_size) > { > struct ath_wiphy *aphy = hw->priv; > struct ath_softc *sc = aphy->sc; > --- wireless-testing.orig/drivers/net/wireless/ath/carl9170/main.c 2011-01-12 13:02:27.000000000 +0100 > +++ wireless-testing/drivers/net/wireless/ath/carl9170/main.c 2011-01-12 13:02:36.000000000 +0100 > @@ -1279,7 +1279,7 @@ static int carl9170_op_ampdu_action(stru > struct ieee80211_vif *vif, > enum ieee80211_ampdu_mlme_action action, > struct ieee80211_sta *sta, > - u16 tid, u16 *ssn) > + u16 tid, u16 *ssn, u8 buf_size) > { > struct ar9170 *ar = hw->priv; > struct carl9170_sta_info *sta_info = (void *) sta->drv_priv; > --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-agn.c 2011-01-12 13:02:35.000000000 +0100 > +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-agn.c 2011-01-12 13:02:36.000000000 +0100 > @@ -3416,7 +3416,8 @@ int iwlagn_mac_set_key(struct ieee80211_ > int iwlagn_mac_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 *ssn, > + u8 buf_size) > { > struct iwl_priv *priv = hw->priv; > int ret = -EINVAL; > --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-agn.h 2011-01-12 13:02:27.000000000 +0100 > +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-agn.h 2011-01-12 13:02:36.000000000 +0100 > @@ -363,7 +363,8 @@ void iwlagn_mac_update_tkip_key(struct i > int iwlagn_mac_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 *ssn, > + u8 buf_size); > int iwlagn_mac_sta_add(struct ieee80211_hw *hw, > struct ieee80211_vif *vif, > struct ieee80211_sta *sta); > --- wireless-testing.orig/drivers/net/wireless/mac80211_hwsim.c 2011-01-12 13:02:27.000000000 +0100 > +++ wireless-testing/drivers/net/wireless/mac80211_hwsim.c 2011-01-12 13:02:36.000000000 +0100 > @@ -943,7 +943,8 @@ static int mac80211_hwsim_testmode_cmd(s > static int mac80211_hwsim_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 *ssn, > + u8 buf_size) > { > switch (action) { > case IEEE80211_AMPDU_TX_START: > --- wireless-testing.orig/drivers/net/wireless/mwl8k.c 2011-01-12 13:02:27.000000000 +0100 > +++ wireless-testing/drivers/net/wireless/mwl8k.c 2011-01-12 13:02:36.000000000 +0100 > @@ -3932,7 +3932,8 @@ static int mwl8k_get_survey(struct ieee8 > static int > mwl8k_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 *ssn, > + u8 buf_size) > { > switch (action) { > case IEEE80211_AMPDU_RX_START: > --- wireless-testing.orig/drivers/net/wireless/rt2x00/rt2800lib.c 2011-01-12 13:02:27.000000000 +0100 > +++ wireless-testing/drivers/net/wireless/rt2x00/rt2800lib.c 2011-01-12 13:02:36.000000000 +0100 > @@ -3530,7 +3530,8 @@ EXPORT_SYMBOL_GPL(rt2800_get_tsf); > > int rt2800_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 *ssn, > + u8 buf_size) > { > int ret = 0; > > --- wireless-testing.orig/drivers/net/wireless/rt2x00/rt2800lib.h 2011-01-12 13:02:27.000000000 +0100 > +++ wireless-testing/drivers/net/wireless/rt2x00/rt2800lib.h 2011-01-12 13:02:36.000000000 +0100 > @@ -198,7 +198,8 @@ int rt2800_conf_tx(struct ieee80211_hw * > u64 rt2800_get_tsf(struct ieee80211_hw *hw); > int rt2800_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 *ssn, > + u8 buf_size); > int rt2800_get_survey(struct ieee80211_hw *hw, int idx, > struct survey_info *survey); > > --- wireless-testing.orig/drivers/net/wireless/rtlwifi/core.c 2011-01-12 13:02:27.000000000 +0100 > +++ wireless-testing/drivers/net/wireless/rtlwifi/core.c 2011-01-12 13:02:36.000000000 +0100 > @@ -748,7 +748,8 @@ static void rtl_op_sta_notify(struct iee > static int rtl_op_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 *ssn, > + u8 buf_size) > { > struct rtl_priv *rtlpriv = rtl_priv(hw); > > > > -- > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC 2/2] iwlwifi: use maximum aggregation size 2011-01-12 12:13 [RFC 0/2] fix aggregation buffer size limits Johannes Berg 2011-01-12 12:13 ` [RFC 1/2] mac80211: track receivers aggregation reorder buffer size Johannes Berg @ 2011-01-12 12:13 ` Johannes Berg 2011-01-12 16:23 ` Guy, Wey-Yi 1 sibling, 1 reply; 8+ messages in thread From: Johannes Berg @ 2011-01-12 12:13 UTC (permalink / raw) To: linux-wireless From: Johannes Berg <johannes.berg@intel.com> Use the values from the peer to set up the ucode for the right maximum number of subframes in an aggregate. Since the ucode only tracks this per station, use the minimum across all aggregation sessions with this peer. Signed-off-by: Johannes Berg <johannes.berg@intel.com> --- drivers/net/wireless/iwlwifi/iwl-agn-rs.c | 5 +++- drivers/net/wireless/iwlwifi/iwl-agn.c | 32 +++++++++++++++++++++++++----- drivers/net/wireless/iwlwifi/iwl-dev.h | 1 3 files changed, 32 insertions(+), 6 deletions(-) --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-agn-rs.c 2011-01-12 13:08:22.000000000 +0100 +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-agn-rs.c 2011-01-12 13:10:58.000000000 +0100 @@ -2889,6 +2889,8 @@ static void rs_fill_link_cmd(struct iwl_ u8 ant_toggle_cnt = 0; u8 use_ht_possible = 1; u8 valid_tx_ant = 0; + struct iwl_station_priv *sta_priv = + container_of(lq_sta, struct iwl_station_priv, lq_sta); struct iwl_link_quality_cmd *lq_cmd = &lq_sta->lq; /* Override starting rate (index 0) if needed for debug purposes */ @@ -3007,7 +3009,8 @@ static void rs_fill_link_cmd(struct iwl_ repeat_rate--; } - lq_cmd->agg_params.agg_frame_cnt_limit = LINK_QUAL_AGG_FRAME_LIMIT_DEF; + lq_cmd->agg_params.agg_frame_cnt_limit = + sta_priv->max_agg_bufsize ?: LINK_QUAL_AGG_FRAME_LIMIT_DEF; lq_cmd->agg_params.agg_dis_start_th = LINK_QUAL_AGG_DISABLE_START_DEF; lq_cmd->agg_params.agg_time_limit = --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-agn.c 2011-01-12 13:03:34.000000000 +0100 +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-agn.c 2011-01-12 13:11:50.000000000 +0100 @@ -3421,6 +3421,7 @@ int iwlagn_mac_ampdu_action(struct ieee8 { struct iwl_priv *priv = hw->priv; int ret = -EINVAL; + struct iwl_station_priv *sta_priv = (void *) sta->drv_priv; IWL_DEBUG_HT(priv, "A-MPDU action on addr %pM tid %d\n", sta->addr, tid); @@ -3475,11 +3476,28 @@ int iwlagn_mac_ampdu_action(struct ieee8 } break; case IEEE80211_AMPDU_TX_OPERATIONAL: + /* + * If the limit is 0, then it wasn't initialised yet, + * use the default. We can do that since we take the + * minimum below, and we don't want to go above our + * default due to hardware restrictions. + */ + if (sta_priv->max_agg_bufsize == 0) + sta_priv->max_agg_bufsize = + LINK_QUAL_AGG_FRAME_LIMIT_DEF; + + /* + * Even though in theory the peer could have different + * aggregation reorder buffer sizes for different sessions, + * our ucode doesn't allow for that and has a global limit + * for each station. Therefore, use the minimum of all the + * aggregation sessions and our default value. + */ + sta_priv->max_agg_bufsize = + min(sta_priv->max_agg_bufsize, buf_size); + if (priv->cfg->ht_params && priv->cfg->ht_params->use_rts_for_aggregation) { - struct iwl_station_priv *sta_priv = - (void *) sta->drv_priv; - /* * switch to RTS/CTS if it is the prefer protection * method for HT traffic @@ -3487,9 +3505,13 @@ int iwlagn_mac_ampdu_action(struct ieee8 sta_priv->lq_sta.lq.general_params.flags |= LINK_QUAL_FLAGS_SET_STA_TLC_RTS_MSK; - iwl_send_lq_cmd(priv, iwl_rxon_ctx_from_vif(vif), - &sta_priv->lq_sta.lq, CMD_ASYNC, false); } + + sta_priv->lq_sta.lq.agg_params.agg_frame_cnt_limit = + sta_priv->max_agg_bufsize; + + iwl_send_lq_cmd(priv, iwl_rxon_ctx_from_vif(vif), + &sta_priv->lq_sta.lq, CMD_ASYNC, false); ret = 0; break; } --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-dev.h 2011-01-12 13:03:12.000000000 +0100 +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-dev.h 2011-01-12 13:03:31.000000000 +0100 @@ -509,6 +509,7 @@ struct iwl_station_priv { atomic_t pending_frames; bool client; bool asleep; + u8 max_agg_bufsize; }; /** ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC 2/2] iwlwifi: use maximum aggregation size 2011-01-12 12:13 ` [RFC 2/2] iwlwifi: use maximum aggregation size Johannes Berg @ 2011-01-12 16:23 ` Guy, Wey-Yi 2011-01-12 16:38 ` Johannes Berg 0 siblings, 1 reply; 8+ messages in thread From: Guy, Wey-Yi @ 2011-01-12 16:23 UTC (permalink / raw) To: Johannes Berg; +Cc: linux-wireless Hi Johannes, On Wed, 2011-01-12 at 04:13 -0800, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > Use the values from the peer to set up the ucode > for the right maximum number of subframes in an > aggregate. Since the ucode only tracks this per > station, use the minimum across all aggregation > sessions with this peer. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > --- > drivers/net/wireless/iwlwifi/iwl-agn-rs.c | 5 +++- > drivers/net/wireless/iwlwifi/iwl-agn.c | 32 +++++++++++++++++++++++++----- > drivers/net/wireless/iwlwifi/iwl-dev.h | 1 > 3 files changed, 32 insertions(+), 6 deletions(-) > > --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-agn-rs.c 2011-01-12 13:08:22.000000000 +0100 > +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-agn-rs.c 2011-01-12 13:10:58.000000000 +0100 > @@ -2889,6 +2889,8 @@ static void rs_fill_link_cmd(struct iwl_ > u8 ant_toggle_cnt = 0; > u8 use_ht_possible = 1; > u8 valid_tx_ant = 0; > + struct iwl_station_priv *sta_priv = > + container_of(lq_sta, struct iwl_station_priv, lq_sta); > struct iwl_link_quality_cmd *lq_cmd = &lq_sta->lq; > > /* Override starting rate (index 0) if needed for debug purposes */ > @@ -3007,7 +3009,8 @@ static void rs_fill_link_cmd(struct iwl_ > repeat_rate--; > } > > - lq_cmd->agg_params.agg_frame_cnt_limit = LINK_QUAL_AGG_FRAME_LIMIT_DEF; > + lq_cmd->agg_params.agg_frame_cnt_limit = > + sta_priv->max_agg_bufsize ?: LINK_QUAL_AGG_FRAME_LIMIT_DEF; at this point, sta_priv->lq_sta.lq.agg_params.agg_frame_cnt_limit has the right value, why not use it? on the other hand, sta_priv->max_agg_bufsize == sta_priv->lq_sta.lq.agg_params.agg_frame_cnt_limit why need both? > lq_cmd->agg_params.agg_dis_start_th = LINK_QUAL_AGG_DISABLE_START_DEF; > > lq_cmd->agg_params.agg_time_limit = > --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-agn.c 2011-01-12 13:03:34.000000000 +0100 > +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-agn.c 2011-01-12 13:11:50.000000000 +0100 > @@ -3421,6 +3421,7 @@ int iwlagn_mac_ampdu_action(struct ieee8 > { > struct iwl_priv *priv = hw->priv; > int ret = -EINVAL; > + struct iwl_station_priv *sta_priv = (void *) sta->drv_priv; > > IWL_DEBUG_HT(priv, "A-MPDU action on addr %pM tid %d\n", > sta->addr, tid); > @@ -3475,11 +3476,28 @@ int iwlagn_mac_ampdu_action(struct ieee8 > } > break; > case IEEE80211_AMPDU_TX_OPERATIONAL: > + /* > + * If the limit is 0, then it wasn't initialised yet, > + * use the default. We can do that since we take the > + * minimum below, and we don't want to go above our > + * default due to hardware restrictions. > + */ > + if (sta_priv->max_agg_bufsize == 0) > + sta_priv->max_agg_bufsize = > + LINK_QUAL_AGG_FRAME_LIMIT_DEF; > + > + /* > + * Even though in theory the peer could have different > + * aggregation reorder buffer sizes for different sessions, > + * our ucode doesn't allow for that and has a global limit > + * for each station. Therefore, use the minimum of all the > + * aggregation sessions and our default value. > + */ > + sta_priv->max_agg_bufsize = > + min(sta_priv->max_agg_bufsize, buf_size); > + not sure where the "bus_size" come from? > if (priv->cfg->ht_params && > priv->cfg->ht_params->use_rts_for_aggregation) { > - struct iwl_station_priv *sta_priv = > - (void *) sta->drv_priv; > - > /* > * switch to RTS/CTS if it is the prefer protection > * method for HT traffic > @@ -3487,9 +3505,13 @@ int iwlagn_mac_ampdu_action(struct ieee8 > > sta_priv->lq_sta.lq.general_params.flags |= > LINK_QUAL_FLAGS_SET_STA_TLC_RTS_MSK; > - iwl_send_lq_cmd(priv, iwl_rxon_ctx_from_vif(vif), > - &sta_priv->lq_sta.lq, CMD_ASYNC, false); > } > + > + sta_priv->lq_sta.lq.agg_params.agg_frame_cnt_limit = > + sta_priv->max_agg_bufsize; > + > + iwl_send_lq_cmd(priv, iwl_rxon_ctx_from_vif(vif), > + &sta_priv->lq_sta.lq, CMD_ASYNC, false); > ret = 0; > break; > } > --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-dev.h 2011-01-12 13:03:12.000000000 +0100 > +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-dev.h 2011-01-12 13:03:31.000000000 +0100 > @@ -509,6 +509,7 @@ struct iwl_station_priv { > atomic_t pending_frames; > bool client; > bool asleep; > + u8 max_agg_bufsize; > }; > > /** > Wey ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC 2/2] iwlwifi: use maximum aggregation size 2011-01-12 16:23 ` Guy, Wey-Yi @ 2011-01-12 16:38 ` Johannes Berg 2011-01-12 16:46 ` Guy, Wey-Yi 0 siblings, 1 reply; 8+ messages in thread From: Johannes Berg @ 2011-01-12 16:38 UTC (permalink / raw) To: Guy, Wey-Yi; +Cc: linux-wireless On Wed, 2011-01-12 at 08:23 -0800, Guy, Wey-Yi wrote: > > + lq_cmd->agg_params.agg_frame_cnt_limit = > > + sta_priv->max_agg_bufsize ?: LINK_QUAL_AGG_FRAME_LIMIT_DEF; > > at this point, sta_priv->lq_sta.lq.agg_params.agg_frame_cnt_limit has > the right value, why not use it? I wasn't sure it would _always_ have the right value, and if the ucode would accept 0 before needed -- the ?: construct will use it if non-zero, and use the default otherwise. > on the other hand, sta_priv->max_agg_bufsize == > sta_priv->lq_sta.lq.agg_params.agg_frame_cnt_limit > > why need both? Yeah I guess that would work if we put the init somewhere else, and didn't change it above in that function -- we need the minimum calculation to work. This seemed easier to verify to me :-) > > + /* > > + * Even though in theory the peer could have different > > + * aggregation reorder buffer sizes for different sessions, > > + * our ucode doesn't allow for that and has a global limit > > + * for each station. Therefore, use the minimum of all the > > + * aggregation sessions and our default value. > > + */ > > + sta_priv->max_agg_bufsize = > > + min(sta_priv->max_agg_bufsize, buf_size); > > + > not sure where the "bus_size" come from? It's a new argument to the function -- see patch 1/2. johannes ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC 2/2] iwlwifi: use maximum aggregation size 2011-01-12 16:38 ` Johannes Berg @ 2011-01-12 16:46 ` Guy, Wey-Yi 0 siblings, 0 replies; 8+ messages in thread From: Guy, Wey-Yi @ 2011-01-12 16:46 UTC (permalink / raw) To: Johannes Berg; +Cc: linux-wireless Hi Johannes, On Wed, 2011-01-12 at 08:38 -0800, Johannes Berg wrote: > On Wed, 2011-01-12 at 08:23 -0800, Guy, Wey-Yi wrote: > > > > + lq_cmd->agg_params.agg_frame_cnt_limit = > > > + sta_priv->max_agg_bufsize ?: LINK_QUAL_AGG_FRAME_LIMIT_DEF; > > > > at this point, sta_priv->lq_sta.lq.agg_params.agg_frame_cnt_limit has > > the right value, why not use it? > > I wasn't sure it would _always_ have the right value, and if the ucode > would accept 0 before needed -- the ?: construct will use it if > non-zero, and use the default otherwise. > > > on the other hand, sta_priv->max_agg_bufsize == > > sta_priv->lq_sta.lq.agg_params.agg_frame_cnt_limit > > > > why need both? > > Yeah I guess that would work if we put the init somewhere else, and > didn't change it above in that function -- we need the minimum > calculation to work. This seemed easier to verify to me :-) > > > > + /* > > > + * Even though in theory the peer could have different > > > + * aggregation reorder buffer sizes for different sessions, > > > + * our ucode doesn't allow for that and has a global limit > > > + * for each station. Therefore, use the minimum of all the > > > + * aggregation sessions and our default value. > > > + */ > > > + sta_priv->max_agg_bufsize = > > > + min(sta_priv->max_agg_bufsize, buf_size); > > > + > > not sure where the "bus_size" come from? > > It's a new argument to the function -- see patch 1/2. > I don't think I have patch 1/2 Here is what I have [RFC2/2]iwlwifi: use maximum aggregation size [RFC2/2]iwlwifi: advertise max aggregate size Wey ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-01-12 16:52 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-01-12 12:13 [RFC 0/2] fix aggregation buffer size limits Johannes Berg 2011-01-12 12:13 ` [RFC 1/2] mac80211: track receivers aggregation reorder buffer size Johannes Berg 2011-01-12 13:01 ` Sujith 2011-01-12 16:51 ` Guy, Wey-Yi 2011-01-12 12:13 ` [RFC 2/2] iwlwifi: use maximum aggregation size Johannes Berg 2011-01-12 16:23 ` Guy, Wey-Yi 2011-01-12 16:38 ` Johannes Berg 2011-01-12 16:46 ` Guy, Wey-Yi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).