| Message ID | 20210901030542.17257-1-benl@squareup.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wcn36xx: handle connection loss indication | expand |
Hi Benjamin, On Wed, 1 Sept 2021 at 05:05, Benjamin Li <benl@squareup.com> wrote: > > Firmware sends delete_sta_context_ind when it detects the AP has gone > away in STA mode. Right now the handler for that indication only handles > AP mode; fix it to also handle STA mode. > > Cc: stable@vger.kernel.org > Fixes: 8def9ec46a5f ("wcn36xx: Enable firmware link monitoring") I think it's good to have but does it really fix the link monitoring issue? Is the connection loss detected in this scenario: - Connect to AP - Force active mode (iw wlan0 set power_save off) - Wait for no data activity - HW shutdown the AP (AP leave) Do you get any indication? In this scenario, DB410C (wcn3620) does not report anything. Regards, Loic
On 01/09/2021 07:40, Loic Poulain wrote:
> iw wlan0 set power_save off
I do this on wcn3680b and get no loss of signal
If I do this though
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c
b/drivers/net/wireless/ath/wcn36xx/smd.c
index 03966072f34c..ba613fbb728d 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2345,6 +2345,8 @@ int wcn36xx_smd_feature_caps_exchange(struct
wcn36xx *wcn)
set_feat_caps(msg_body.feat_caps, DOT11AC);
set_feat_caps(msg_body.feat_caps,
ANTENNA_DIVERSITY_SELECTION);
}
+ set_feat_caps(msg_body.feat_caps, IBSS_HEARTBEAT_OFFLOAD);
+ set_feat_caps(msg_body.feat_caps, WLANACTIVE_OFFLOAD);
PREPARE_HAL_BUF(wcn->hal_buf, msg_body);
@@ -2589,7 +2591,7 @@ static int wcn36xx_smd_missed_beacon_ind(struct
wcn36xx *wcn,
struct wcn36xx_hal_missed_beacon_ind_msg *rsp = buf;
struct ieee80211_vif *vif = NULL;
struct wcn36xx_vif *tmp;
-
+wcn36xx_info("%s/%d\n", __func__, __LINE__);
/* Old FW does not have bss index */
if (wcn36xx_is_fw_version(wcn, 1, 2, 2, 24)) {
list_for_each_entry(tmp, &wcn->vif_list, list) {
@@ -2608,7 +2610,7 @@ static int wcn36xx_smd_missed_beacon_ind(struct
wcn36xx *wcn,
list_for_each_entry(tmp, &wcn->vif_list, list) {
if (tmp->bss_index == rsp->bss_index) {
- wcn36xx_dbg(WCN36XX_DBG_HAL, "beacon missed
bss_index %d\n",
+ wcn36xx_info("beacon missed bss_index %d\n",
rsp->bss_index);
vif = wcn36xx_priv_to_vif(tmp);
ieee80211_connection_loss(vif);
bingo
root@linaro-developer:~# iw wlan0 set power_save off
# pulls plug on AP
root@linaro-developer:~# [ 83.290987] wcn36xx:
wcn36xx_smd_missed_beacon_ind/2594
[ 83.291070] wcn36xx: beacon missed bss_index 0
[ 83.295403] wlan0: Connection to AP e2:63:da:9c:a4:bd lost
I'm not sure if both flags are required but, this is the behavior we want
Thanks for the investigation! As discussed offline, I will send v2 with Fixes: removed, and Bryan will test and submit a separate patch to add the additional feat_caps for the power_save off case. Depending on DB410c testing, these feat_caps may need to be gated for WCN3680 only, in which case Loic's patch to re-enable CONNECTION_MONITOR (but gated for WCN3660/3620) would still be needed. Ben On 9/1/21 4:56 AM, Bryan O'Donoghue wrote: > On 01/09/2021 07:40, Loic Poulain wrote: >> iw wlan0 set power_save off > > I do this on wcn3680b and get no loss of signal > > If I do this though > > diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c > index 03966072f34c..ba613fbb728d 100644 > --- a/drivers/net/wireless/ath/wcn36xx/smd.c > +++ b/drivers/net/wireless/ath/wcn36xx/smd.c > @@ -2345,6 +2345,8 @@ int wcn36xx_smd_feature_caps_exchange(struct wcn36xx *wcn) > set_feat_caps(msg_body.feat_caps, DOT11AC); > set_feat_caps(msg_body.feat_caps, ANTENNA_DIVERSITY_SELECTION); > } > + set_feat_caps(msg_body.feat_caps, IBSS_HEARTBEAT_OFFLOAD); > + set_feat_caps(msg_body.feat_caps, WLANACTIVE_OFFLOAD); > > PREPARE_HAL_BUF(wcn->hal_buf, msg_body); > > @@ -2589,7 +2591,7 @@ static int wcn36xx_smd_missed_beacon_ind(struct wcn36xx *wcn, > struct wcn36xx_hal_missed_beacon_ind_msg *rsp = buf; > struct ieee80211_vif *vif = NULL; > struct wcn36xx_vif *tmp; > - > +wcn36xx_info("%s/%d\n", __func__, __LINE__); > /* Old FW does not have bss index */ > if (wcn36xx_is_fw_version(wcn, 1, 2, 2, 24)) { > list_for_each_entry(tmp, &wcn->vif_list, list) { > @@ -2608,7 +2610,7 @@ static int wcn36xx_smd_missed_beacon_ind(struct wcn36xx *wcn, > > list_for_each_entry(tmp, &wcn->vif_list, list) { > if (tmp->bss_index == rsp->bss_index) { > - wcn36xx_dbg(WCN36XX_DBG_HAL, "beacon missed bss_index %d\n", > + wcn36xx_info("beacon missed bss_index %d\n", > rsp->bss_index); > vif = wcn36xx_priv_to_vif(tmp); > ieee80211_connection_loss(vif); > > > bingo > > root@linaro-developer:~# iw wlan0 set power_save off > > > # pulls plug on AP > > root@linaro-developer:~# [ 83.290987] wcn36xx: wcn36xx_smd_missed_beacon_ind/2594 > [ 83.291070] wcn36xx: beacon missed bss_index 0 > [ 83.295403] wlan0: Connection to AP e2:63:da:9c:a4:bd lost > > I'm not sure if both flags are required but, this is the behavior we want >
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c index 57fa857b290b..f6bea896abe8 100644 --- a/drivers/net/wireless/ath/wcn36xx/smd.c +++ b/drivers/net/wireless/ath/wcn36xx/smd.c @@ -2623,30 +2623,52 @@ static int wcn36xx_smd_delete_sta_context_ind(struct wcn36xx *wcn, size_t len) { struct wcn36xx_hal_delete_sta_context_ind_msg *rsp = buf; - struct wcn36xx_vif *tmp; + struct wcn36xx_vif *vif_priv; + struct ieee80211_vif *vif; + struct ieee80211_bss_conf *bss_conf; struct ieee80211_sta *sta; + bool found = false; if (len != sizeof(*rsp)) { wcn36xx_warn("Corrupted delete sta indication\n"); return -EIO; } - wcn36xx_dbg(WCN36XX_DBG_HAL, "delete station indication %pM index %d\n", - rsp->addr2, rsp->sta_id); + wcn36xx_dbg(WCN36XX_DBG_HAL, + "delete station indication %pM index %d reason %d\n", + rsp->addr2, rsp->sta_id, rsp->reason_code); - list_for_each_entry(tmp, &wcn->vif_list, list) { + list_for_each_entry(vif_priv, &wcn->vif_list, list) { rcu_read_lock(); - sta = ieee80211_find_sta(wcn36xx_priv_to_vif(tmp), rsp->addr2); - if (sta) - ieee80211_report_low_ack(sta, 0); + vif = wcn36xx_priv_to_vif(vif_priv); + + if (vif->type == NL80211_IFTYPE_STATION) { + /* We could call ieee80211_find_sta too, but checking + * bss_conf is clearer. + */ + bss_conf = &vif->bss_conf; + if (vif_priv->sta_assoc && + !memcmp(bss_conf->bssid, rsp->addr2, ETH_ALEN)) { + found = true; + wcn36xx_dbg(WCN36XX_DBG_HAL, + "connection loss bss_index %d\n", + vif_priv->bss_index); + ieee80211_connection_loss(vif); + } + } else { + sta = ieee80211_find_sta(vif, rsp->addr2); + if (sta) { + found = true; + ieee80211_report_low_ack(sta, 0); + } + } + rcu_read_unlock(); - if (sta) + if (found) return 0; } - wcn36xx_warn("STA with addr %pM and index %d not found\n", - rsp->addr2, - rsp->sta_id); + wcn36xx_warn("BSS or STA with addr %pM not found\n", rsp->addr2); return -ENOENT; }
Firmware sends delete_sta_context_ind when it detects the AP has gone away in STA mode. Right now the handler for that indication only handles AP mode; fix it to also handle STA mode. Cc: stable@vger.kernel.org Fixes: 8def9ec46a5f ("wcn36xx: Enable firmware link monitoring") Signed-off-by: Benjamin Li <benl@squareup.com> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/net/wireless/ath/wcn36xx/smd.c | 44 +++++++++++++++++++------- 1 file changed, 33 insertions(+), 11 deletions(-)