| Message ID | 1593524821-32115-1-git-send-email-loic.poulain@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [1/4] wcn36xx: Fix multiple AMPDU sessions support | expand |
Loic Poulain <loic.poulain@linaro.org> wrote: > Several AMPDU sessions can be started, e.g. for different TIDs. > Currently the driver does not take care of the session ID when > requesting block-ack (statically set to 0), which leads to never > block-acked packet with sessions other than 0. > > Fix this by saving the session id when creating the ba session and > use it in subsequent ba operations. > > This issue can be reproduced with iperf in two steps (tid 0 strem > then tid 6 stream). > > 1.0 iperf -s # wcn36xx side > 1.1 iperf -c ${IP_ADDR} # host side > > Then > > 2.0 iperf -s -u -S 0xC0 # wcn36xx side > 2.1 iperf -c ${IP_ADDR} -u -S 0xC0 -l 2000 # host side > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> What's the difference from the earlier version: https://patchwork.kernel.org/patch/11609871/ A changelog would be nice.
Loic Poulain <loic.poulain@linaro.org> writes: > On Wed, 15 Jul 2020 at 17:33, Kalle Valo <kvalo@codeaurora.org> wrote: > > Loic Poulain <loic.poulain@linaro.org> wrote: > > > Several AMPDU sessions can be started, e.g. for different TIDs. > > Currently the driver does not take care of the session ID when > > requesting block-ack (statically set to 0), which leads to never > > block-acked packet with sessions other than 0. > > > > Fix this by saving the session id when creating the ba session > and > > use it in subsequent ba operations. > > > > This issue can be reproduced with iperf in two steps (tid 0 > strem > > then tid 6 stream). > > > > 1.0 iperf -s # wcn36xx side > > 1.1 iperf -c ${IP_ADDR} # host side > > > > Then > > > > 2.0 iperf -s -u -S 0xC0 # wcn36xx side > > 2.1 iperf -c ${IP_ADDR} -u -S 0xC0 -l 2000 # host side > > > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > > What's the difference from the earlier version: > > https://patchwork.kernel.org/patch/11609871/ > > A changelog would be nice. > > > There is no change, but I've simply included it in this series. > I can resend the series without this one if necessary so that you can > consider only the initial one. No need to resend, I just wanted to understand if there are any changes. In the future try to always include a changelog, that way I don't need to guess if something has changed or not. And don't send HTML mail, linux-wireless list drops it and then your mail won't be visible in the patchwork either.
On 30/06/2020 14:46, Loic Poulain wrote: > Several AMPDU sessions can be started, e.g. for different TIDs. > Currently the driver does not take care of the session ID when > requesting block-ack (statically set to 0), which leads to never > block-acked packet with sessions other than 0. > > Fix this by saving the session id when creating the ba session and > use it in subsequent ba operations. > > This issue can be reproduced with iperf in two steps (tid 0 strem > then tid 6 stream). > > 1.0 iperf -s # wcn36xx side > 1.1 iperf -c ${IP_ADDR} # host side > > Then > > 2.0 iperf -s -u -S 0xC0 # wcn36xx side > 2.1 iperf -c ${IP_ADDR} -u -S 0xC0 -l 2000 # host side > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > --- Getting a few checkpatch errors here CHECK: No space is necessary after a cast #44: FILE: drivers/net/wireless/ath/wcn36xx/smd.c:2112: + rsp = (struct wcn36xx_hal_add_ba_session_rsp_msg *) buf; WARNING: Comparisons should place the constant on the right side of the test #45: FILE: drivers/net/wireless/ath/wcn36xx/smd.c:2113: + if (WCN36XX_FW_MSG_RESULT_SUCCESS != rsp->status) total: 0 errors, 1 warnings, 1 checks, 116 lines checked
diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c index 702b689..af32bd6 100644 --- a/drivers/net/wireless/ath/wcn36xx/main.c +++ b/drivers/net/wireless/ath/wcn36xx/main.c @@ -1083,6 +1083,7 @@ static int wcn36xx_ampdu_action(struct ieee80211_hw *hw, u16 tid = params->tid; u16 *ssn = ¶ms->ssn; int ret = 0; + u8 session; wcn36xx_dbg(WCN36XX_DBG_MAC, "mac ampdu action action %d tid %d\n", action, tid); @@ -1092,10 +1093,11 @@ static int wcn36xx_ampdu_action(struct ieee80211_hw *hw, switch (action) { case IEEE80211_AMPDU_RX_START: sta_priv->tid = tid; - wcn36xx_smd_add_ba_session(wcn, sta, tid, ssn, 0, - get_sta_index(vif, sta_priv)); - wcn36xx_smd_add_ba(wcn); - wcn36xx_smd_trigger_ba(wcn, get_sta_index(vif, sta_priv)); + session = wcn36xx_smd_add_ba_session(wcn, sta, tid, ssn, 0, + get_sta_index(vif, sta_priv)); + wcn36xx_smd_add_ba(wcn, session); + wcn36xx_smd_trigger_ba(wcn, get_sta_index(vif, sta_priv), tid, + session); break; case IEEE80211_AMPDU_RX_STOP: wcn36xx_smd_del_ba(wcn, tid, get_sta_index(vif, sta_priv)); diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c index 77269ac..0ad605f 100644 --- a/drivers/net/wireless/ath/wcn36xx/smd.c +++ b/drivers/net/wireless/ath/wcn36xx/smd.c @@ -2102,6 +2102,22 @@ int wcn36xx_smd_feature_caps_exchange(struct wcn36xx *wcn) return ret; } +static int wcn36xx_smd_add_ba_session_rsp(void *buf, int len, u8 *session) +{ + struct wcn36xx_hal_add_ba_session_rsp_msg *rsp; + + if (len < sizeof(*rsp)) + return -EINVAL; + + rsp = (struct wcn36xx_hal_add_ba_session_rsp_msg *) buf; + if (WCN36XX_FW_MSG_RESULT_SUCCESS != rsp->status) + return rsp->status; + + *session = rsp->ba_session_id; + + return 0; +} + int wcn36xx_smd_add_ba_session(struct wcn36xx *wcn, struct ieee80211_sta *sta, u16 tid, @@ -2110,6 +2126,7 @@ int wcn36xx_smd_add_ba_session(struct wcn36xx *wcn, u8 sta_index) { struct wcn36xx_hal_add_ba_session_req_msg msg_body; + u8 session_id; int ret; mutex_lock(&wcn->hal_mutex); @@ -2135,17 +2152,20 @@ int wcn36xx_smd_add_ba_session(struct wcn36xx *wcn, wcn36xx_err("Sending hal_add_ba_session failed\n"); goto out; } - ret = wcn36xx_smd_rsp_status_check(wcn->hal_buf, wcn->hal_rsp_len); + ret = wcn36xx_smd_add_ba_session_rsp(wcn->hal_buf, wcn->hal_rsp_len, + &session_id); if (ret) { wcn36xx_err("hal_add_ba_session response failed err=%d\n", ret); goto out; } + + ret = session_id; out: mutex_unlock(&wcn->hal_mutex); return ret; } -int wcn36xx_smd_add_ba(struct wcn36xx *wcn) +int wcn36xx_smd_add_ba(struct wcn36xx *wcn, u8 session_id) { struct wcn36xx_hal_add_ba_req_msg msg_body; int ret; @@ -2153,7 +2173,7 @@ int wcn36xx_smd_add_ba(struct wcn36xx *wcn) mutex_lock(&wcn->hal_mutex); INIT_HAL_MSG(msg_body, WCN36XX_HAL_ADD_BA_REQ); - msg_body.session_id = 0; + msg_body.session_id = session_id; msg_body.win_size = WCN36XX_AGGR_BUFFER_SIZE; PREPARE_HAL_BUF(wcn->hal_buf, msg_body); @@ -2212,7 +2232,7 @@ static int wcn36xx_smd_trigger_ba_rsp(void *buf, int len) return rsp->status; } -int wcn36xx_smd_trigger_ba(struct wcn36xx *wcn, u8 sta_index) +int wcn36xx_smd_trigger_ba(struct wcn36xx *wcn, u8 sta_index, u16 tid, u8 session_id) { struct wcn36xx_hal_trigger_ba_req_msg msg_body; struct wcn36xx_hal_trigger_ba_req_candidate *candidate; @@ -2221,7 +2241,7 @@ int wcn36xx_smd_trigger_ba(struct wcn36xx *wcn, u8 sta_index) mutex_lock(&wcn->hal_mutex); INIT_HAL_MSG(msg_body, WCN36XX_HAL_TRIGGER_BA_REQ); - msg_body.session_id = 0; + msg_body.session_id = session_id; msg_body.candidate_cnt = 1; msg_body.header.len += sizeof(*candidate); PREPARE_HAL_BUF(wcn->hal_buf, msg_body); @@ -2229,7 +2249,7 @@ int wcn36xx_smd_trigger_ba(struct wcn36xx *wcn, u8 sta_index) candidate = (struct wcn36xx_hal_trigger_ba_req_candidate *) (wcn->hal_buf + sizeof(msg_body)); candidate->sta_index = sta_index; - candidate->tid_bitmap = 1; + candidate->tid_bitmap = 1 << tid; ret = wcn36xx_smd_send_and_wait(wcn, msg_body.header.len); if (ret) { diff --git a/drivers/net/wireless/ath/wcn36xx/smd.h b/drivers/net/wireless/ath/wcn36xx/smd.h index ff15df8..68c59df 100644 --- a/drivers/net/wireless/ath/wcn36xx/smd.h +++ b/drivers/net/wireless/ath/wcn36xx/smd.h @@ -132,9 +132,9 @@ int wcn36xx_smd_add_ba_session(struct wcn36xx *wcn, u16 *ssn, u8 direction, u8 sta_index); -int wcn36xx_smd_add_ba(struct wcn36xx *wcn); +int wcn36xx_smd_add_ba(struct wcn36xx *wcn, u8 session_id); int wcn36xx_smd_del_ba(struct wcn36xx *wcn, u16 tid, u8 sta_index); -int wcn36xx_smd_trigger_ba(struct wcn36xx *wcn, u8 sta_index); +int wcn36xx_smd_trigger_ba(struct wcn36xx *wcn, u8 sta_index, u16 tid, u8 session_id); int wcn36xx_smd_update_cfg(struct wcn36xx *wcn, u32 cfg_id, u32 value);
Several AMPDU sessions can be started, e.g. for different TIDs. Currently the driver does not take care of the session ID when requesting block-ack (statically set to 0), which leads to never block-acked packet with sessions other than 0. Fix this by saving the session id when creating the ba session and use it in subsequent ba operations. This issue can be reproduced with iperf in two steps (tid 0 strem then tid 6 stream). 1.0 iperf -s # wcn36xx side 1.1 iperf -c ${IP_ADDR} # host side Then 2.0 iperf -s -u -S 0xC0 # wcn36xx side 2.1 iperf -c ${IP_ADDR} -u -S 0xC0 -l 2000 # host side Signed-off-by: Loic Poulain <loic.poulain@linaro.org> --- drivers/net/wireless/ath/wcn36xx/main.c | 10 ++++++---- drivers/net/wireless/ath/wcn36xx/smd.c | 32 ++++++++++++++++++++++++++------ drivers/net/wireless/ath/wcn36xx/smd.h | 4 ++-- 3 files changed, 34 insertions(+), 12 deletions(-)