Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Mon, 8 Feb 2021 07:43:32 -0800
From:   Guenter Roeck <linux@roeck-us.net>
To:     Youghandhar Chintala <youghand@codeaurora.org>
Cc:     johannes@sipsolutions.net, davem@davemloft.net, kuba@kernel.org,
        linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, kuabhs@chromium.org,
        dianders@chromium.org, briannorris@chromium.org,
        pillair@codeaurora.org
Subject: Re: [PATCH 2/3] mac80211: Add support to trigger sta disconnect on
 hardware restart
Message-ID: <20210208154332.GA20186@roeck-us.net>
References: <20201215172352.5311-1-youghand@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201215172352.5311-1-youghand@codeaurora.org>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Tue, Dec 15, 2020 at 10:53:52PM +0530, Youghandhar Chintala wrote:
> Currently in case of target hardware restart, we just reconfig and
> re-enable the security keys and enable the network queues to start
> data traffic back from where it was interrupted.
> 
> Many ath10k wifi chipsets have sequence numbers for the data
> packets assigned by firmware and the mac sequence number will
> restart from zero after target hardware restart leading to mismatch
> in the sequence number expected by the remote peer vs the sequence
> number of the frame sent by the target firmware.
> 
> This mismatch in sequence number will cause out-of-order packets
> on the remote peer and all the frames sent by the device are dropped
> until we reach the sequence number which was sent before we restarted
> the target hardware
> 
> In order to fix this, we trigger a sta disconnect, for the targets
> which expose this corresponding wiphy flag, in case of target hw
> restart. After this there will be a fresh connection and thereby
> avoiding the dropping of frames by remote peer.
> 
> The right fix would be to pull the entire data path into the host
> which is not feasible or would need lots of complex changes and
> will still be inefficient.
> 
> Tested on ath10k using WCN3990, QCA6174
> 
> Signed-off-by: Youghandhar Chintala <youghand@codeaurora.org>
> Reviewed-by: Abhishek Kumar <kuabhs@chromium.org>
> ---
>  net/mac80211/ieee80211_i.h |  3 +++
>  net/mac80211/mlme.c        |  9 +++++++++
>  net/mac80211/util.c        | 22 +++++++++++++++++++---
>  3 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index cde2e3f..8cbeb5f 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -748,6 +748,8 @@ struct ieee80211_if_mesh {
>   *	back to wireless media and to the local net stack.
>   * @IEEE80211_SDATA_DISCONNECT_RESUME: Disconnect after resume.
>   * @IEEE80211_SDATA_IN_DRIVER: indicates interface was added to driver
> + * @IEEE80211_SDATA_DISCONNECT_HW_RESTART: Disconnect after hardware restart
> + *	recovery
>   */
>  enum ieee80211_sub_if_data_flags {
>  	IEEE80211_SDATA_ALLMULTI		= BIT(0),
> @@ -755,6 +757,7 @@ enum ieee80211_sub_if_data_flags {
>  	IEEE80211_SDATA_DONT_BRIDGE_PACKETS	= BIT(3),
>  	IEEE80211_SDATA_DISCONNECT_RESUME	= BIT(4),
>  	IEEE80211_SDATA_IN_DRIVER		= BIT(5),
> +	IEEE80211_SDATA_DISCONNECT_HW_RESTART	= BIT(6),
>  };
>  
>  /**
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index 6adfcb9..e4d0d16 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -4769,6 +4769,15 @@ void ieee80211_sta_restart(struct ieee80211_sub_if_data *sdata)
>  					      true);
>  		sdata_unlock(sdata);
>  		return;
> +	} else if (sdata->flags & IEEE80211_SDATA_DISCONNECT_HW_RESTART) {
> +		sdata->flags &= ~IEEE80211_SDATA_DISCONNECT_HW_RESTART;
> +		mlme_dbg(sdata, "driver requested disconnect after hardware restart\n");
> +		ieee80211_sta_connection_lost(sdata,
> +					      ifmgd->associated->bssid,
> +					      WLAN_REASON_UNSPECIFIED,
> +					      true);
> +		sdata_unlock(sdata);
> +		return;
>  	}
>  	sdata_unlock(sdata);
>  }
> diff --git a/net/mac80211/util.c b/net/mac80211/util.c
> index 8c3c01a..98567a3 100644
> --- a/net/mac80211/util.c
> +++ b/net/mac80211/util.c
> @@ -2567,9 +2567,12 @@ int ieee80211_reconfig(struct ieee80211_local *local)
>  	}
>  	mutex_unlock(&local->sta_mtx);
>  
> -	/* add back keys */
> -	list_for_each_entry(sdata, &local->interfaces, list)
> -		ieee80211_reenable_keys(sdata);
> +
> +	if (!(hw->wiphy->flags & WIPHY_FLAG_STA_DISCONNECT_ON_HW_RESTART)) {
> +		/* add back keys */
> +		list_for_each_entry(sdata, &local->interfaces, list)
> +			ieee80211_reenable_keys(sdata);
> +	}
>  
>  	/* Reconfigure sched scan if it was interrupted by FW restart */
>  	mutex_lock(&local->mtx);
> @@ -2643,6 +2646,19 @@ int ieee80211_reconfig(struct ieee80211_local *local)
>  					IEEE80211_QUEUE_STOP_REASON_SUSPEND,
>  					false);
>  
> +	if ((hw->wiphy->flags & WIPHY_FLAG_STA_DISCONNECT_ON_HW_RESTART) &&
> +	    !reconfig_due_to_wowlan) {
> +		list_for_each_entry(sdata, &local->interfaces, list) {
> +			if (!ieee80211_sdata_running(sdata))
> +				continue;
> +			if (sdata->vif.type == NL80211_IFTYPE_STATION) {
> +				sdata->flags |=
> +					IEEE80211_SDATA_DISCONNECT_HW_RESTART;
> +				ieee80211_sta_restart(sdata);

If CONFIG_PM=n:

ERROR: "ieee80211_sta_restart" [net/mac80211/mac80211.ko] undefined!

Guenter

> +			}
> +		}
> +	}
> +
>  	/*
>  	 * If this is for hw restart things are still running.
>  	 * We may want to change that later, however.