2014-10-10 10:57:30

by Michal Kazior

[permalink] [raw]
Subject: [PATCH 0/3] ath10k: assoc clean ups 2014-10-10

Hi,

This is a bunch of assoc-related clean ups.


Michal Kazior (3):
ath10k: clean up assoc code
ath10k: skip some commands on reassoc
ath10k: clean up sta auth/assoc code

drivers/net/wireless/ath/ath10k/mac.c | 267 +++++++++++++++-------------------
1 file changed, 117 insertions(+), 150 deletions(-)

--
1.8.5.3



2014-10-21 07:34:02

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 0/3] ath10k: assoc clean ups 2014-10-10

Michal Kazior <[email protected]> writes:

> This is a bunch of assoc-related clean ups.
>
>
> Michal Kazior (3):
> ath10k: clean up assoc code
> ath10k: skip some commands on reassoc
> ath10k: clean up sta auth/assoc code

Thanks, all three applied.

--
Kalle Valo

2014-10-10 10:57:31

by Michal Kazior

[permalink] [raw]
Subject: [PATCH 1/3] ath10k: clean up assoc code

There's no need to pass bss_conf explicitly as it
is accessible via vif pointer. This requires
slight changes in function prototypes. While at it
clean up listen interval workaround/command.

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/mac.c | 94 +++++++++++++++++------------------
1 file changed, 46 insertions(+), 48 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 75e0aeb..1021e31 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -1060,51 +1060,45 @@ static int ath10k_mac_vif_setup_ps(struct ath10k_vif *arvif)
/* Station management */
/**********************/

+static u32 ath10k_peer_assoc_h_listen_intval(struct ath10k *ar,
+ struct ieee80211_vif *vif)
+{
+ /* Some firmware revisions have unstable STA powersave when listen
+ * interval is set too high (e.g. 5). The symptoms are firmware doesn't
+ * generate NullFunc frames properly even if buffered frames have been
+ * indicated in Beacon TIM. Firmware would seldom wake up to pull
+ * buffered frames. Often pinging the device from AP would simply fail.
+ *
+ * As a workaround set it to 1.
+ */
+ if (vif->type == NL80211_IFTYPE_STATION)
+ return 1;
+
+ return ar->hw->conf.listen_interval;
+}
+
static void ath10k_peer_assoc_h_basic(struct ath10k *ar,
- struct ath10k_vif *arvif,
+ struct ieee80211_vif *vif,
struct ieee80211_sta *sta,
- struct ieee80211_bss_conf *bss_conf,
struct wmi_peer_assoc_complete_arg *arg)
{
+ struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+
lockdep_assert_held(&ar->conf_mutex);

ether_addr_copy(arg->addr, sta->addr);
arg->vdev_id = arvif->vdev_id;
arg->peer_aid = sta->aid;
arg->peer_flags |= WMI_PEER_AUTH;
-
- if (arvif->vdev_type == WMI_VDEV_TYPE_STA)
- /*
- * Seems FW have problems with Power Save in STA
- * mode when we setup this parameter to high (eg. 5).
- * Often we see that FW don't send NULL (with clean P flags)
- * frame even there is info about buffered frames in beacons.
- * Sometimes we have to wait more than 10 seconds before FW
- * will wakeup. Often sending one ping from AP to our device
- * just fail (more than 50%).
- *
- * Seems setting this FW parameter to 1 couse FW
- * will check every beacon and will wakup immediately
- * after detection buffered data.
- */
- arg->peer_listen_intval = 1;
- else
- arg->peer_listen_intval = ar->hw->conf.listen_interval;
-
+ arg->peer_listen_intval = ath10k_peer_assoc_h_listen_intval(ar, vif);
arg->peer_num_spatial_streams = 1;
-
- /*
- * The assoc capabilities are available only in managed mode.
- */
- if (arvif->vdev_type == WMI_VDEV_TYPE_STA && bss_conf)
- arg->peer_caps = bss_conf->assoc_capability;
+ arg->peer_caps = vif->bss_conf.assoc_capability;
}

static void ath10k_peer_assoc_h_crypto(struct ath10k *ar,
- struct ath10k_vif *arvif,
+ struct ieee80211_vif *vif,
struct wmi_peer_assoc_complete_arg *arg)
{
- struct ieee80211_vif *vif = arvif->vif;
struct ieee80211_bss_conf *info = &vif->bss_conf;
struct cfg80211_bss *bss;
const u8 *rsnie = NULL;
@@ -1361,11 +1355,12 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
}

static void ath10k_peer_assoc_h_qos(struct ath10k *ar,
- struct ath10k_vif *arvif,
+ struct ieee80211_vif *vif,
struct ieee80211_sta *sta,
- struct ieee80211_bss_conf *bss_conf,
struct wmi_peer_assoc_complete_arg *arg)
{
+ struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+
switch (arvif->vdev_type) {
case WMI_VDEV_TYPE_AP:
if (sta->wme)
@@ -1377,7 +1372,7 @@ static void ath10k_peer_assoc_h_qos(struct ath10k *ar,
}
break;
case WMI_VDEV_TYPE_STA:
- if (bss_conf->qos)
+ if (vif->bss_conf.qos)
arg->peer_flags |= WMI_PEER_QOS;
break;
default:
@@ -1386,7 +1381,7 @@ static void ath10k_peer_assoc_h_qos(struct ath10k *ar,
}

static void ath10k_peer_assoc_h_phymode(struct ath10k *ar,
- struct ath10k_vif *arvif,
+ struct ieee80211_vif *vif,
struct ieee80211_sta *sta,
struct wmi_peer_assoc_complete_arg *arg)
{
@@ -1437,22 +1432,21 @@ static void ath10k_peer_assoc_h_phymode(struct ath10k *ar,
}

static int ath10k_peer_assoc_prepare(struct ath10k *ar,
- struct ath10k_vif *arvif,
+ struct ieee80211_vif *vif,
struct ieee80211_sta *sta,
- struct ieee80211_bss_conf *bss_conf,
struct wmi_peer_assoc_complete_arg *arg)
{
lockdep_assert_held(&ar->conf_mutex);

memset(arg, 0, sizeof(*arg));

- ath10k_peer_assoc_h_basic(ar, arvif, sta, bss_conf, arg);
- ath10k_peer_assoc_h_crypto(ar, arvif, arg);
+ ath10k_peer_assoc_h_basic(ar, vif, sta, arg);
+ ath10k_peer_assoc_h_crypto(ar, vif, arg);
ath10k_peer_assoc_h_rates(ar, sta, arg);
ath10k_peer_assoc_h_ht(ar, sta, arg);
ath10k_peer_assoc_h_vht(ar, sta, arg);
- ath10k_peer_assoc_h_qos(ar, arvif, sta, bss_conf, arg);
- ath10k_peer_assoc_h_phymode(ar, arvif, sta, arg);
+ ath10k_peer_assoc_h_qos(ar, vif, sta, arg);
+ ath10k_peer_assoc_h_phymode(ar, vif, sta, arg);

return 0;
}
@@ -1512,8 +1506,7 @@ static void ath10k_bss_assoc(struct ieee80211_hw *hw,
* before calling ath10k_setup_peer_smps() which might sleep. */
ht_cap = ap_sta->ht_cap;

- ret = ath10k_peer_assoc_prepare(ar, arvif, ap_sta,
- bss_conf, &peer_arg);
+ ret = ath10k_peer_assoc_prepare(ar, vif, ap_sta, &peer_arg);
if (ret) {
ath10k_warn(ar, "failed to prepare peer assoc for %pM vdev %i: %d\n",
bss_conf->bssid, arvif->vdev_id, ret);
@@ -1596,15 +1589,18 @@ static void ath10k_bss_disassoc(struct ieee80211_hw *hw,
arvif->is_up = false;
}

-static int ath10k_station_assoc(struct ath10k *ar, struct ath10k_vif *arvif,
- struct ieee80211_sta *sta, bool reassoc)
+static int ath10k_station_assoc(struct ath10k *ar,
+ struct ieee80211_vif *vif,
+ struct ieee80211_sta *sta,
+ bool reassoc)
{
+ struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
struct wmi_peer_assoc_complete_arg peer_arg;
int ret = 0;

lockdep_assert_held(&ar->conf_mutex);

- ret = ath10k_peer_assoc_prepare(ar, arvif, sta, NULL, &peer_arg);
+ ret = ath10k_peer_assoc_prepare(ar, vif, sta, &peer_arg);
if (ret) {
ath10k_warn(ar, "failed to prepare WMI peer assoc for %pM vdev %i: %i\n",
sta->addr, arvif->vdev_id, ret);
@@ -1653,9 +1649,11 @@ static int ath10k_station_assoc(struct ath10k *ar, struct ath10k_vif *arvif,
return ret;
}

-static int ath10k_station_disassoc(struct ath10k *ar, struct ath10k_vif *arvif,
+static int ath10k_station_disassoc(struct ath10k *ar,
+ struct ieee80211_vif *vif,
struct ieee80211_sta *sta)
{
+ struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
int ret = 0;

lockdep_assert_held(&ar->conf_mutex);
@@ -3497,7 +3495,7 @@ static void ath10k_sta_rc_update_wk(struct work_struct *wk)
ath10k_dbg(ar, ATH10K_DBG_MAC, "mac update sta %pM supp rates\n",
sta->addr);

- err = ath10k_station_assoc(ar, arvif, sta, true);
+ err = ath10k_station_assoc(ar, arvif->vif, sta, true);
if (err)
ath10k_warn(ar, "failed to reassociate station: %pM\n",
sta->addr);
@@ -3583,7 +3581,7 @@ static int ath10k_sta_state(struct ieee80211_hw *hw,
ath10k_dbg(ar, ATH10K_DBG_MAC, "mac sta %pM associated\n",
sta->addr);

- ret = ath10k_station_assoc(ar, arvif, sta, false);
+ ret = ath10k_station_assoc(ar, vif, sta, false);
if (ret)
ath10k_warn(ar, "failed to associate station %pM for vdev %i: %i\n",
sta->addr, arvif->vdev_id, ret);
@@ -3597,7 +3595,7 @@ static int ath10k_sta_state(struct ieee80211_hw *hw,
ath10k_dbg(ar, ATH10K_DBG_MAC, "mac sta %pM disassociated\n",
sta->addr);

- ret = ath10k_station_disassoc(ar, arvif, sta);
+ ret = ath10k_station_disassoc(ar, vif, sta);
if (ret)
ath10k_warn(ar, "failed to disassociate station: %pM vdev %i: %i\n",
sta->addr, arvif->vdev_id, ret);
--
1.8.5.3


2014-10-10 10:57:32

by Michal Kazior

[permalink] [raw]
Subject: [PATCH 2/3] ath10k: skip some commands on reassoc

It doesn't make much sense to reconfigure peer
completely upon reassociation. This will make it
easier to have a more uniform association code
across different modes.

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/mac.c | 52 +++++++++++++++++++----------------
1 file changed, 29 insertions(+), 23 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 1021e31..266b740 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -1615,35 +1615,41 @@ static int ath10k_station_assoc(struct ath10k *ar,
return ret;
}

- ret = ath10k_setup_peer_smps(ar, arvif, sta->addr, &sta->ht_cap);
- if (ret) {
- ath10k_warn(ar, "failed to setup peer SMPS for vdev %d: %d\n",
- arvif->vdev_id, ret);
- return ret;
- }
-
- if (!sta->wme && !reassoc) {
- arvif->num_legacy_stations++;
- ret = ath10k_recalc_rtscts_prot(arvif);
+ /* Re-assoc is run only to update supported rates for given station. It
+ * doesn't make much sense to reconfigure the peer completely.
+ */
+ if (!reassoc) {
+ ret = ath10k_setup_peer_smps(ar, arvif, sta->addr,
+ &sta->ht_cap);
if (ret) {
- ath10k_warn(ar, "failed to recalculate rts/cts prot for vdev %d: %d\n",
+ ath10k_warn(ar, "failed to setup peer SMPS for vdev %d: %d\n",
arvif->vdev_id, ret);
return ret;
}
- }

- ret = ath10k_install_peer_wep_keys(arvif, sta->addr);
- if (ret) {
- ath10k_warn(ar, "failed to install peer wep keys for vdev %i: %d\n",
- arvif->vdev_id, ret);
- return ret;
- }
+ ret = ath10k_peer_assoc_qos_ap(ar, arvif, sta);
+ if (ret) {
+ ath10k_warn(ar, "failed to set qos params for STA %pM for vdev %i: %d\n",
+ sta->addr, arvif->vdev_id, ret);
+ return ret;
+ }

- ret = ath10k_peer_assoc_qos_ap(ar, arvif, sta);
- if (ret) {
- ath10k_warn(ar, "failed to set qos params for STA %pM for vdev %i: %d\n",
- sta->addr, arvif->vdev_id, ret);
- return ret;
+ if (!sta->wme) {
+ arvif->num_legacy_stations++;
+ ret = ath10k_recalc_rtscts_prot(arvif);
+ if (ret) {
+ ath10k_warn(ar, "failed to recalculate rts/cts prot for vdev %d: %d\n",
+ arvif->vdev_id, ret);
+ return ret;
+ }
+ }
+
+ ret = ath10k_install_peer_wep_keys(arvif, sta->addr);
+ if (ret) {
+ ath10k_warn(ar, "failed to install peer wep keys for vdev %i: %d\n",
+ arvif->vdev_id, ret);
+ return ret;
+ }
}

return ret;
--
1.8.5.3


2014-10-10 10:57:33

by Michal Kazior

[permalink] [raw]
Subject: [PATCH 3/3] ath10k: clean up sta auth/assoc code

The code can be symmetrical so make it so. This
makes it easier to understand and work with.

Signed-off-by: Michal Kazior <[email protected]>
---
drivers/net/wireless/ath/ath10k/mac.c | 121 ++++++++++++----------------------
1 file changed, 42 insertions(+), 79 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 266b740..07900c4 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -1492,6 +1492,9 @@ static void ath10k_bss_assoc(struct ieee80211_hw *hw,

lockdep_assert_held(&ar->conf_mutex);

+ ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vdev %i assoc bssid %pM aid %d\n",
+ arvif->vdev_id, arvif->bssid, arvif->aid);
+
rcu_read_lock();

ap_sta = ieee80211_find_sta(vif, bss_conf->bssid);
@@ -1534,6 +1537,8 @@ static void ath10k_bss_assoc(struct ieee80211_hw *hw,
"mac vdev %d up (associated) bssid %pM aid %d\n",
arvif->vdev_id, bss_conf->bssid, bss_conf->aid);

+ WARN_ON(arvif->is_up);
+
arvif->aid = bss_conf->aid;
ether_addr_copy(arvif->bssid, bss_conf->bssid);

@@ -1547,9 +1552,6 @@ static void ath10k_bss_assoc(struct ieee80211_hw *hw,
arvif->is_up = true;
}

-/*
- * FIXME: flush TIDs
- */
static void ath10k_bss_disassoc(struct ieee80211_hw *hw,
struct ieee80211_vif *vif)
{
@@ -1559,33 +1561,15 @@ static void ath10k_bss_disassoc(struct ieee80211_hw *hw,

lockdep_assert_held(&ar->conf_mutex);

- /*
- * For some reason, calling VDEV-DOWN before VDEV-STOP
- * makes the FW to send frames via HTT after disassociation.
- * No idea why this happens, even though VDEV-DOWN is supposed
- * to be analogous to link down, so just stop the VDEV.
- */
- ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vdev %d stop (disassociated\n",
- arvif->vdev_id);
-
- /* FIXME: check return value */
- ret = ath10k_vdev_stop(arvif);
-
- /*
- * If we don't call VDEV-DOWN after VDEV-STOP FW will remain active and
- * report beacons from previously associated network through HTT.
- * This in turn would spam mac80211 WARN_ON if we bring down all
- * interfaces as it expects there is no rx when no interface is
- * running.
- */
- ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vdev %d down\n", arvif->vdev_id);
+ ath10k_dbg(ar, ATH10K_DBG_MAC, "mac vdev %i disassoc bssid %pM\n",
+ arvif->vdev_id, arvif->bssid);

- /* FIXME: why don't we print error if wmi call fails? */
ret = ath10k_wmi_vdev_down(ar, arvif->vdev_id);
+ if (ret)
+ ath10k_warn(ar, "faield to down vdev %i: %d\n",
+ arvif->vdev_id, ret);

arvif->def_wep_key_idx = 0;
-
- arvif->is_started = false;
arvif->is_up = false;
}

@@ -3116,54 +3100,8 @@ static void ath10k_bss_info_changed(struct ieee80211_hw *hw,
arvif->u.ap.hidden_ssid = info->hidden_ssid;
}

- /*
- * Firmware manages AP self-peer internally so make sure to not create
- * it in driver. Otherwise AP self-peer deletion may timeout later.
- */
- if (changed & BSS_CHANGED_BSSID &&
- vif->type != NL80211_IFTYPE_AP) {
- if (!is_zero_ether_addr(info->bssid)) {
- if (vif->type == NL80211_IFTYPE_STATION) {
- ath10k_dbg(ar, ATH10K_DBG_MAC,
- "mac vdev %d create peer %pM\n",
- arvif->vdev_id, info->bssid);
-
- ret = ath10k_peer_create(ar, arvif->vdev_id,
- info->bssid);
- if (ret)
- ath10k_warn(ar, "failed to add peer %pM for vdev %d when changing bssid: %i\n",
- info->bssid, arvif->vdev_id,
- ret);
- /*
- * this is never erased as we it for crypto key
- * clearing; this is FW requirement
- */
- ether_addr_copy(arvif->bssid, info->bssid);
-
- ath10k_dbg(ar, ATH10K_DBG_MAC,
- "mac vdev %d start %pM\n",
- arvif->vdev_id, info->bssid);
-
- ret = ath10k_vdev_start(arvif);
- if (ret) {
- ath10k_warn(ar, "failed to start vdev %i: %d\n",
- arvif->vdev_id, ret);
- goto exit;
- }
-
- arvif->is_started = true;
- }
-
- /*
- * Mac80211 does not keep IBSS bssid when leaving IBSS,
- * so driver need to store it. It is needed when leaving
- * IBSS in order to remove BSSID peer.
- */
- if (vif->type == NL80211_IFTYPE_ADHOC)
- memcpy(arvif->bssid, info->bssid,
- ETH_ALEN);
- }
- }
+ if (changed & BSS_CHANGED_BSSID && !is_zero_ether_addr(info->bssid))
+ ether_addr_copy(arvif->bssid, info->bssid);

if (changed & BSS_CHANGED_BEACON_ENABLED)
ath10k_control_beaconing(arvif, info);
@@ -3225,10 +3163,11 @@ static void ath10k_bss_info_changed(struct ieee80211_hw *hw,
ath10k_monitor_stop(ar);
ath10k_bss_assoc(hw, vif, info);
ath10k_monitor_recalc(ar);
+ } else {
+ ath10k_bss_disassoc(hw, vif);
}
}

-exit:
mutex_unlock(&ar->conf_mutex);
}

@@ -3537,8 +3476,7 @@ static int ath10k_sta_state(struct ieee80211_hw *hw,
mutex_lock(&ar->conf_mutex);

if (old_state == IEEE80211_STA_NOTEXIST &&
- new_state == IEEE80211_STA_NONE &&
- vif->type != NL80211_IFTYPE_STATION) {
+ new_state == IEEE80211_STA_NONE) {
/*
* New station addition.
*/
@@ -3562,6 +3500,21 @@ static int ath10k_sta_state(struct ieee80211_hw *hw,
if (ret)
ath10k_warn(ar, "failed to add peer %pM for vdev %d when adding a new sta: %i\n",
sta->addr, arvif->vdev_id, ret);
+
+ if (vif->type == NL80211_IFTYPE_STATION) {
+ WARN_ON(arvif->is_started);
+
+ ret = ath10k_vdev_start(arvif);
+ if (ret) {
+ ath10k_warn(ar, "failed to start vdev %i: %d\n",
+ arvif->vdev_id, ret);
+ WARN_ON(ath10k_peer_delete(ar, arvif->vdev_id,
+ sta->addr));
+ goto exit;
+ }
+
+ arvif->is_started = true;
+ }
} else if ((old_state == IEEE80211_STA_NONE &&
new_state == IEEE80211_STA_NOTEXIST)) {
/*
@@ -3570,13 +3523,23 @@ static int ath10k_sta_state(struct ieee80211_hw *hw,
ath10k_dbg(ar, ATH10K_DBG_MAC,
"mac vdev %d peer delete %pM (sta gone)\n",
arvif->vdev_id, sta->addr);
+
+ if (vif->type == NL80211_IFTYPE_STATION) {
+ WARN_ON(!arvif->is_started);
+
+ ret = ath10k_vdev_stop(arvif);
+ if (ret)
+ ath10k_warn(ar, "failed to stop vdev %i: %d\n",
+ arvif->vdev_id, ret);
+
+ arvif->is_started = false;
+ }
+
ret = ath10k_peer_delete(ar, arvif->vdev_id, sta->addr);
if (ret)
ath10k_warn(ar, "failed to delete peer %pM for vdev %d: %i\n",
sta->addr, arvif->vdev_id, ret);

- if (vif->type == NL80211_IFTYPE_STATION)
- ath10k_bss_disassoc(hw, vif);
} else if (old_state == IEEE80211_STA_AUTH &&
new_state == IEEE80211_STA_ASSOC &&
(vif->type == NL80211_IFTYPE_AP ||
--
1.8.5.3