diff mbox series

wcn36xx: handle connection loss indication

Message ID 20210901030542.17257-1-benl@squareup.com
State Superseded
Headers show
Series wcn36xx: handle connection loss indication | expand

Commit Message

Benjamin Li Sept. 1, 2021, 3:05 a.m. UTC 
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(-)

-- 
2.17.1

Comments

Loic Poulain Sept. 1, 2021, 6:41 a.m. UTC | #1 
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
Bryan O'Donoghue Sept. 1, 2021, 11:56 a.m. UTC | #2 
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
Benjamin Li Sept. 1, 2021, 6:02 p.m. UTC | #3 
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 mbox series

Patch

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;
 }