[PATCH 0/3] ath10k: fixes 2013-11-22

[PATCH 0/3] ath10k: fixes 2013-11-22

[Michal Kazior]

Hi,

This has some fixes I've stacked in my local repo.
There are no dependencies between them.

Michal Kazior (3):
  ath10k: use nss provided by mac80211
  ath10k: implement sta_rc_update()
  ath10k: fix Tx status clearing

 drivers/net/wireless/ath/ath10k/mac.c  | 88 +++++++++++++++++++++++++++++++++-
 drivers/net/wireless/ath/ath10k/txrx.c |  2 +-
 drivers/net/wireless/ath/ath10k/wmi.h  |  6 +++
 3 files changed, 93 insertions(+), 3 deletions(-)

-- 
1.8.4.rc3

[PATCH 1/3] ath10k: use nss provided by mac80211

[Michal Kazior]

Calculating STA NSS just from the rateset is not
the greatest idea. The NSS could be possibly
influenced by other things that only mac80211
knows about.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index b70a3b2..15eda44 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -925,7 +925,7 @@ static void ath10k_peer_assoc_h_basic(struct ath10k *ar,
 	else
 		arg->peer_listen_intval = ar->hw->conf.listen_interval;
 
-	arg->peer_num_spatial_streams = 1;
+	arg->peer_num_spatial_streams = max_t(u32, 1, sta->rx_nss);
 
 	/*
 	 * The assoc capabilities are available only in managed mode.
@@ -1075,7 +1075,6 @@ static void ath10k_peer_assoc_h_ht(struct ath10k *ar,
 			arg->peer_ht_rates.rates[n++] = i;
 
 	arg->peer_ht_rates.num_rates = n;
-	arg->peer_num_spatial_streams = max((n+7) / 8, 1);
 
 	ath10k_dbg(ATH10K_DBG_MAC, "mac ht peer %pM mcs cnt %d nss %d\n",
 		   arg->addr,
-- 
1.8.4.rc3

[PATCH 2/3] ath10k: implement sta_rc_update()

[Michal Kazior]

This allows dynamic changes of bandwidth/nss, e.g.
via ht/vht operation mode change notification.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 85 +++++++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/wmi.h |  6 +++
 2 files changed, 91 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 15eda44..65cb63d 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -3269,6 +3269,90 @@ exit:
 	return ret;
 }
 
+static void ath10k_sta_rc_update(struct ieee80211_hw *hw,
+				 struct ieee80211_vif *vif,
+				 struct ieee80211_sta *sta,
+				 u32 changed)
+{
+	struct ath10k *ar = hw->priv;
+	struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+	u32 chwidth, smps;
+	int ret;
+
+	if (changed & IEEE80211_RC_BW_CHANGED) {
+		switch (sta->bandwidth) {
+		default:
+			ath10k_warn("unsupported STA BW: %d\n", sta->bandwidth);
+		case IEEE80211_STA_RX_BW_20:
+			chwidth = WMI_PEER_CHWIDTH_20MHZ;
+			break;
+		case IEEE80211_STA_RX_BW_40:
+			chwidth = WMI_PEER_CHWIDTH_40MHZ;
+			break;
+		case IEEE80211_STA_RX_BW_80:
+			chwidth = WMI_PEER_CHWIDTH_80MHZ;
+			break;
+		}
+
+		ath10k_dbg(ATH10K_DBG_MAC,
+			   "mac update sta %pM bandwidth (peer chwidth %d) to %d\n",
+			   sta->addr, chwidth, sta->bandwidth);
+
+		ret = ath10k_wmi_peer_set_param(ar, arvif->vdev_id, sta->addr,
+						WMI_PEER_CHAN_WIDTH, chwidth);
+		if (ret)
+			ath10k_warn("failed to update STA %pM bandwidth (peer chwidth %d) to %d: %d\n",
+				    sta->addr, chwidth, sta->bandwidth, ret);
+	}
+
+	if (changed & IEEE80211_RC_NSS_CHANGED) {
+		ath10k_dbg(ATH10K_DBG_MAC, "mac update sta %pM nss to %d\n",
+			   sta->addr, sta->rx_nss);
+
+		ret = ath10k_wmi_peer_set_param(ar, arvif->vdev_id, sta->addr,
+						WMI_PEER_NSS, sta->rx_nss);
+		if (ret)
+			ath10k_warn("failed to update STA %pM nss to %d: %d\n",
+				    sta->addr, sta->rx_nss, ret);
+	}
+
+	if (changed & IEEE80211_RC_SMPS_CHANGED) {
+		smps = WMI_PEER_SMPS_PS_NONE;
+
+		switch (sta->smps_mode) {
+		case IEEE80211_SMPS_NUM_MODES:
+			ath10k_warn("invalid smps mode: %d\n", sta->smps_mode);
+		case IEEE80211_SMPS_AUTOMATIC:
+		case IEEE80211_SMPS_OFF:
+			smps = WMI_PEER_SMPS_PS_NONE;
+			break;
+		case IEEE80211_SMPS_STATIC:
+			smps = WMI_PEER_SMPS_STATIC;
+			break;
+		case IEEE80211_SMPS_DYNAMIC:
+			smps = WMI_PEER_SMPS_DYNAMIC;
+			break;
+		}
+
+		ath10k_dbg(ATH10K_DBG_MAC,
+			   "mac update sta %pM smps (peer smps %d) to %d\n",
+			   sta->addr, smps, sta->smps_mode);
+
+		ret = ath10k_wmi_peer_set_param(ar, arvif->vdev_id, sta->addr,
+						WMI_PEER_SMPS_STATE, smps);
+		if (ret)
+			ath10k_warn("failed to update STA %pM smps (peer smps %d) to %d: %d\n",
+				    sta->addr, smps, sta->smps_mode, ret);
+	}
+
+	if (changed & IEEE80211_RC_SUPP_RATES_CHANGED) {
+		/* FIXME: Not implemented. Not really sure if it's possible to
+		 * influence HW rate control so much. */
+		ath10k_dbg(ATH10K_DBG_MAC,
+			   "mac sta rc update - supp rates changed - not implemented\n");
+	}
+}
+
 static const struct ieee80211_ops ath10k_ops = {
 	.tx				= ath10k_tx,
 	.start				= ath10k_start,
@@ -3291,6 +3375,7 @@ static const struct ieee80211_ops ath10k_ops = {
 	.tx_last_beacon			= ath10k_tx_last_beacon,
 	.restart_complete		= ath10k_restart_complete,
 	.get_survey			= ath10k_get_survey,
+	.sta_rc_update			= ath10k_sta_rc_update,
 #ifdef CONFIG_PM
 	.suspend			= ath10k_suspend,
 	.resume				= ath10k_resume,
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 0087d69..e8c4bb7 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -3847,6 +3847,12 @@ enum wmi_peer_smps_state {
 	WMI_PEER_SMPS_DYNAMIC = 0x2
 };
 
+enum wmi_peer_chwidth {
+	WMI_PEER_CHWIDTH_20MHZ = 0,
+	WMI_PEER_CHWIDTH_40MHZ = 1,
+	WMI_PEER_CHWIDTH_80MHZ = 2,
+};
+
 enum wmi_peer_param {
 	WMI_PEER_SMPS_STATE = 0x1, /* see %wmi_peer_smps_state */
 	WMI_PEER_AMPDU      = 0x2,
-- 
1.8.4.rc3

[PATCH 3/3] ath10k: fix Tx status clearing

[Michal Kazior]

Too much of tx info was being cleared. This caused
issues in some setups.

Reported-By: Matti Laakso <malaakso@elisanet.fi>
Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/txrx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c
index d476b2c..2282980 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -75,7 +75,7 @@ void ath10k_txrx_tx_unref(struct ath10k_htt *htt,
 	ath10k_report_offchan_tx(htt->ar, msdu);
 
 	info = IEEE80211_SKB_CB(msdu);
-	memset(info, 0, sizeof(*info));
+	memset(&info->status, 0, sizeof(info->status));
 
 	if (tx_done->discard) {
 		ieee80211_free_txskb(htt->ar->hw, msdu);
-- 
1.8.4.rc3

Re: [PATCH 1/3] ath10k: use nss provided by mac80211

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> Calculating STA NSS just from the rateset is not
> the greatest idea. The NSS could be possibly
> influenced by other things that only mac80211
> knows about.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

Is there a bug which this patch fixes or what motivated you to do this?

-- 
Kalle Valo

Re: [PATCH 3/3] ath10k: fix Tx status clearing

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> Too much of tx info was being cleared. This caused
> issues in some setups.
>
> Reported-By: Matti Laakso <malaakso@elisanet.fi>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

Please describe the issues (or at least some of them) in the commit log.
The idea is that an advanced ath10k user will understand what kind of
bug (from user's point of view) this patch is fixing.

-- 
Kalle Valo

Re: [PATCH 1/3] ath10k: use nss provided by mac80211

[Michal Kazior]

On 24 November 2013 14:50, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> Calculating STA NSS just from the rateset is not
>> the greatest idea. The NSS could be possibly
>> influenced by other things that only mac80211
>> knows about.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> Is there a bug which this patch fixes or what motivated you to do this?

This doesn't fix any bug that I know of. It just seems like probably
should use rx_nss instead of calculating it on our own (in case
something changes in the future).

A station may actually include operation mode change notification in
assoc request declaring it wants to use a different number of streams
than it actually supports as per its mcs map. That's not supported in
mac80211 though yet. Marek has posted some patches a few days ago but
it actually uses a different codepath from what I understand.


Michał

[PATCH v2 0/3] ath10k: fixes 2013-11-22

[Michal Kazior]

Hi,

This has some fixes I've stacked in my local repo.
There are no dependencies between them.

v2:
 * refined commit messages

Michal Kazior (3):
  ath10k: use nss provided by mac80211
  ath10k: implement sta_rc_update()
  ath10k: fix Tx status clearing

 drivers/net/wireless/ath/ath10k/mac.c  | 88 +++++++++++++++++++++++++++++++++-
 drivers/net/wireless/ath/ath10k/txrx.c |  2 +-
 drivers/net/wireless/ath/ath10k/wmi.h  |  6 +++
 3 files changed, 93 insertions(+), 3 deletions(-)

-- 
1.8.4.rc3

[PATCH v2 1/3] ath10k: use nss provided by mac80211

[Michal Kazior]

Calculating STA NSS just from the mcs rateset is
not the greatest idea.

This should prevent connectivity issues if
mac80211 is ever to set rx_nss to something other
rather than base on max mcs map. As an example
operation mode change notification in assoc
request may change rx_nss initial values in the
future.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index b70a3b2..15eda44 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -925,7 +925,7 @@ static void ath10k_peer_assoc_h_basic(struct ath10k *ar,
 	else
 		arg->peer_listen_intval = ar->hw->conf.listen_interval;
 
-	arg->peer_num_spatial_streams = 1;
+	arg->peer_num_spatial_streams = max_t(u32, 1, sta->rx_nss);
 
 	/*
 	 * The assoc capabilities are available only in managed mode.
@@ -1075,7 +1075,6 @@ static void ath10k_peer_assoc_h_ht(struct ath10k *ar,
 			arg->peer_ht_rates.rates[n++] = i;
 
 	arg->peer_ht_rates.num_rates = n;
-	arg->peer_num_spatial_streams = max((n+7) / 8, 1);
 
 	ath10k_dbg(ATH10K_DBG_MAC, "mac ht peer %pM mcs cnt %d nss %d\n",
 		   arg->addr,
-- 
1.8.4.rc3

[PATCH v2 2/3] ath10k: implement sta_rc_update()

[Michal Kazior]

This should fix possible connectivity issues upon
changes of channel width, number of streams or
SMPS on connected stations.

An example trigger would be an action frame with
operation mode change notification.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 85 +++++++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/wmi.h |  6 +++
 2 files changed, 91 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 15eda44..65cb63d 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -3269,6 +3269,90 @@ exit:
 	return ret;
 }
 
+static void ath10k_sta_rc_update(struct ieee80211_hw *hw,
+				 struct ieee80211_vif *vif,
+				 struct ieee80211_sta *sta,
+				 u32 changed)
+{
+	struct ath10k *ar = hw->priv;
+	struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+	u32 chwidth, smps;
+	int ret;
+
+	if (changed & IEEE80211_RC_BW_CHANGED) {
+		switch (sta->bandwidth) {
+		default:
+			ath10k_warn("unsupported STA BW: %d\n", sta->bandwidth);
+		case IEEE80211_STA_RX_BW_20:
+			chwidth = WMI_PEER_CHWIDTH_20MHZ;
+			break;
+		case IEEE80211_STA_RX_BW_40:
+			chwidth = WMI_PEER_CHWIDTH_40MHZ;
+			break;
+		case IEEE80211_STA_RX_BW_80:
+			chwidth = WMI_PEER_CHWIDTH_80MHZ;
+			break;
+		}
+
+		ath10k_dbg(ATH10K_DBG_MAC,
+			   "mac update sta %pM bandwidth (peer chwidth %d) to %d\n",
+			   sta->addr, chwidth, sta->bandwidth);
+
+		ret = ath10k_wmi_peer_set_param(ar, arvif->vdev_id, sta->addr,
+						WMI_PEER_CHAN_WIDTH, chwidth);
+		if (ret)
+			ath10k_warn("failed to update STA %pM bandwidth (peer chwidth %d) to %d: %d\n",
+				    sta->addr, chwidth, sta->bandwidth, ret);
+	}
+
+	if (changed & IEEE80211_RC_NSS_CHANGED) {
+		ath10k_dbg(ATH10K_DBG_MAC, "mac update sta %pM nss to %d\n",
+			   sta->addr, sta->rx_nss);
+
+		ret = ath10k_wmi_peer_set_param(ar, arvif->vdev_id, sta->addr,
+						WMI_PEER_NSS, sta->rx_nss);
+		if (ret)
+			ath10k_warn("failed to update STA %pM nss to %d: %d\n",
+				    sta->addr, sta->rx_nss, ret);
+	}
+
+	if (changed & IEEE80211_RC_SMPS_CHANGED) {
+		smps = WMI_PEER_SMPS_PS_NONE;
+
+		switch (sta->smps_mode) {
+		case IEEE80211_SMPS_NUM_MODES:
+			ath10k_warn("invalid smps mode: %d\n", sta->smps_mode);
+		case IEEE80211_SMPS_AUTOMATIC:
+		case IEEE80211_SMPS_OFF:
+			smps = WMI_PEER_SMPS_PS_NONE;
+			break;
+		case IEEE80211_SMPS_STATIC:
+			smps = WMI_PEER_SMPS_STATIC;
+			break;
+		case IEEE80211_SMPS_DYNAMIC:
+			smps = WMI_PEER_SMPS_DYNAMIC;
+			break;
+		}
+
+		ath10k_dbg(ATH10K_DBG_MAC,
+			   "mac update sta %pM smps (peer smps %d) to %d\n",
+			   sta->addr, smps, sta->smps_mode);
+
+		ret = ath10k_wmi_peer_set_param(ar, arvif->vdev_id, sta->addr,
+						WMI_PEER_SMPS_STATE, smps);
+		if (ret)
+			ath10k_warn("failed to update STA %pM smps (peer smps %d) to %d: %d\n",
+				    sta->addr, smps, sta->smps_mode, ret);
+	}
+
+	if (changed & IEEE80211_RC_SUPP_RATES_CHANGED) {
+		/* FIXME: Not implemented. Not really sure if it's possible to
+		 * influence HW rate control so much. */
+		ath10k_dbg(ATH10K_DBG_MAC,
+			   "mac sta rc update - supp rates changed - not implemented\n");
+	}
+}
+
 static const struct ieee80211_ops ath10k_ops = {
 	.tx				= ath10k_tx,
 	.start				= ath10k_start,
@@ -3291,6 +3375,7 @@ static const struct ieee80211_ops ath10k_ops = {
 	.tx_last_beacon			= ath10k_tx_last_beacon,
 	.restart_complete		= ath10k_restart_complete,
 	.get_survey			= ath10k_get_survey,
+	.sta_rc_update			= ath10k_sta_rc_update,
 #ifdef CONFIG_PM
 	.suspend			= ath10k_suspend,
 	.resume				= ath10k_resume,
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 0087d69..e8c4bb7 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -3847,6 +3847,12 @@ enum wmi_peer_smps_state {
 	WMI_PEER_SMPS_DYNAMIC = 0x2
 };
 
+enum wmi_peer_chwidth {
+	WMI_PEER_CHWIDTH_20MHZ = 0,
+	WMI_PEER_CHWIDTH_40MHZ = 1,
+	WMI_PEER_CHWIDTH_80MHZ = 2,
+};
+
 enum wmi_peer_param {
 	WMI_PEER_SMPS_STATE = 0x1, /* see %wmi_peer_smps_state */
 	WMI_PEER_AMPDU      = 0x2,
-- 
1.8.4.rc3

[PATCH v2 3/3] ath10k: fix Tx status clearing

[Michal Kazior]

Too much of tx info was being cleared. This caused
issues in some setups with tx frame status
reporting.

This should fix some cases of stations not being
able to associate to ath10k AP.

Reported-By: Matti Laakso <malaakso@elisanet.fi>
Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/txrx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c
index d476b2c..2282980 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -75,7 +75,7 @@ void ath10k_txrx_tx_unref(struct ath10k_htt *htt,
 	ath10k_report_offchan_tx(htt->ar, msdu);
 
 	info = IEEE80211_SKB_CB(msdu);
-	memset(info, 0, sizeof(*info));
+	memset(&info->status, 0, sizeof(info->status));
 
 	if (tx_done->discard) {
 		ieee80211_free_txskb(htt->ar->hw, msdu);
-- 
1.8.4.rc3

Re: [PATCH v2 1/3] ath10k: use nss provided by mac80211

[Michal Kazior]

On 26 November 2013 14:57, Michal Kazior <michal.kazior@tieto.com> wrote:
> Calculating STA NSS just from the mcs rateset is
> not the greatest idea.
>
> This should prevent connectivity issues if
> mac80211 is ever to set rx_nss to something other
> rather than base on max mcs map. As an example
> operation mode change notification in assoc
> request may change rx_nss initial values in the
> future.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
> ---

Drop this single patch, please.

I just noticed rx_nss doesn't seem to have correct value when associating..


Michał

Re: [PATCH v2 1/3] ath10k: use nss provided by mac80211

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> On 26 November 2013 14:57, Michal Kazior <michal.kazior@tieto.com> wrote:
>> Calculating STA NSS just from the mcs rateset is
>> not the greatest idea.
>>
>> This should prevent connectivity issues if
>> mac80211 is ever to set rx_nss to something other
>> rather than base on max mcs map. As an example
>> operation mode change notification in assoc
>> request may change rx_nss initial values in the
>> future.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>> ---
>
> Drop this single patch, please.
>
> I just noticed rx_nss doesn't seem to have correct value when associating..

Ok, I'm dropping this.

-- 
Kalle Valo

Re: [PATCH v2 2/3] ath10k: implement sta_rc_update()

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> This should fix possible connectivity issues upon
> changes of channel width, number of streams or
> SMPS on connected stations.
>
> An example trigger would be an action frame with
> operation mode change notification.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

Sorry, somehow missed these on my previous review:

> +	if (changed & IEEE80211_RC_BW_CHANGED) {
> +		switch (sta->bandwidth) {
> +		default:
> +			ath10k_warn("unsupported STA BW: %d\n", sta->bandwidth);
> +		case IEEE80211_STA_RX_BW_20:
> +			chwidth = WMI_PEER_CHWIDTH_20MHZ;
> +			break;
> +		case IEEE80211_STA_RX_BW_40:
> +			chwidth = WMI_PEER_CHWIDTH_40MHZ;
> +			break;
> +		case IEEE80211_STA_RX_BW_80:
> +			chwidth = WMI_PEER_CHWIDTH_80MHZ;
> +			break;
> +		}

I assume that the idea here is to use WMI_PEER_CHWIDTH_20MHZ for the
default case. Actually I would prefer to avoid using default case
altogether, that way the compiler will warn if there's an enum value we
don't check. So for example you could add "case IEEE80211_STA_RX_BW_160"
which prints a warning and uses 20 MHz.

> +	if (changed & IEEE80211_RC_SMPS_CHANGED) {
> +		smps = WMI_PEER_SMPS_PS_NONE;
> +
> +		switch (sta->smps_mode) {
> +		case IEEE80211_SMPS_NUM_MODES:
> +			ath10k_warn("invalid smps mode: %d\n", sta->smps_mode);
> +		case IEEE80211_SMPS_AUTOMATIC:
> +		case IEEE80211_SMPS_OFF:
> +			smps = WMI_PEER_SMPS_PS_NONE;
> +			break;
> +		case IEEE80211_SMPS_STATIC:
> +			smps = WMI_PEER_SMPS_STATIC;
> +			break;
> +		case IEEE80211_SMPS_DYNAMIC:
> +			smps = WMI_PEER_SMPS_DYNAMIC;
> +			break;
> +		}

This is better, but I think it would be clearer to have
IEEE80211_SMPS_NUM_MODES last and not "fall through" the cases.

-- 
Kalle Valo

Re: [PATCH v2 0/3] ath10k: fixes 2013-11-22

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> Hi,
>
> This has some fixes I've stacked in my local repo.
> There are no dependencies between them.
>
> v2:
>  * refined commit messages
>
> Michal Kazior (3):
>   ath10k: use nss provided by mac80211

Dropped by your request.

>   ath10k: implement sta_rc_update()

I had comments for this.

>   ath10k: fix Tx status clearing

This is applied.

-- 
Kalle Valo

Re: [PATCH v2 1/3] ath10k: use nss provided by mac80211

[Johannes Berg]

On Wed, 2013-11-27 at 11:25 +0100, Michal Kazior wrote:
> On 26 November 2013 14:57, Michal Kazior <michal.kazior@tieto.com> wrote:
> > Calculating STA NSS just from the mcs rateset is
> > not the greatest idea.
> >
> > This should prevent connectivity issues if
> > mac80211 is ever to set rx_nss to something other
> > rather than base on max mcs map. As an example
> > operation mode change notification in assoc
> > request may change rx_nss initial values in the
> > future.
> >
> > Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
> > ---
> 
> Drop this single patch, please.
> 
> I just noticed rx_nss doesn't seem to have correct value when associating..

FWIW, there are pending changes to make the opmode data come from
hostapd to mac80211 so that this would be set correctly when the station
is associating.

Or where did you find it wasn't set correctly?

johannes

[RFC] mac80211: fix rx_nss calculation for drivers with hw rc

[Michal Kazior]

Drivers with hardware rate control were given
sta->rx_nss set to 0. This was because rx_nss
calculation procedure was protected by hw/sw rate
control check.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
Hi Johannes,

I've re-checked the issue I've mentioned with the patch 'ath10k: use nss
provided by mac80211'.

Apparently drivers with HW rate control are only affected by this bug
and this patch is what I came up with as a quick fix. Any comments?


 net/mac80211/rate.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/rate.h b/net/mac80211/rate.h
index 5dedc56..32cdbd2 100644
--- a/net/mac80211/rate.h
+++ b/net/mac80211/rate.h
@@ -54,6 +54,8 @@ static inline void rate_control_rate_init(struct sta_info *sta)
 	struct ieee80211_supported_band *sband;
 	struct ieee80211_chanctx_conf *chanctx_conf;
 
+	ieee80211_sta_set_rx_nss(sta);
+
 	if (!ref)
 		return;
 
@@ -67,8 +69,6 @@ static inline void rate_control_rate_init(struct sta_info *sta)
 
 	sband = local->hw.wiphy->bands[chanctx_conf->def.chan->band];
 
-	ieee80211_sta_set_rx_nss(sta);
-
 	ref->ops->rate_init(ref->priv, sband, &chanctx_conf->def, ista,
 			    priv_sta);
 	rcu_read_unlock();
-- 
1.8.4.rc3

Re: [RFC] mac80211: fix rx_nss calculation for drivers with hw rc

[Johannes Berg]

On Mon, 2013-12-02 at 11:54 +0100, Michal Kazior wrote:
> Drivers with hardware rate control were given
> sta->rx_nss set to 0. This was because rx_nss
> calculation procedure was protected by hw/sw rate
> control check.
> 
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
> ---
> Hi Johannes,
> 
> I've re-checked the issue I've mentioned with the patch 'ath10k: use nss
> provided by mac80211'.
> 
> Apparently drivers with HW rate control are only affected by this bug
> and this patch is what I came up with as a quick fix. Any comments?

Yeah this looks reasonable. I guess I'll apply it despite the [RFC]

johannes

Re: [RFC] mac80211: fix rx_nss calculation for drivers with hw rc

[Johannes Berg]

On Mon, 2013-12-02 at 11:54 +0100, Michal Kazior wrote:
> Drivers with hardware rate control were given
> sta->rx_nss set to 0. This was because rx_nss
> calculation procedure was protected by hw/sw rate
> control check.

Applied.

johannes

[PATCH v2] ath10k: implement sta_rc_update()

[Michal Kazior]

This should fix possible connectivity issues upon
changes of channel width, number of streams or
SMPS on connected stations.

An example trigger would be an action frame with
operation mode change notification.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
v2:
 - handle 160MHz case chwidth explicitly instead
   of default case
 - handle SMPS_NUM_MODES without a fall-through

 drivers/net/wireless/ath/ath10k/mac.c | 91 +++++++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/wmi.h |  6 +++
 2 files changed, 97 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index ce9ef349..ddacb1e 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -3310,6 +3310,96 @@ exit:
 	return ret;
 }
 
+static void ath10k_sta_rc_update(struct ieee80211_hw *hw,
+				 struct ieee80211_vif *vif,
+				 struct ieee80211_sta *sta,
+				 u32 changed)
+{
+	struct ath10k *ar = hw->priv;
+	struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+	u32 chwidth, smps;
+	int ret;
+
+	if (changed & IEEE80211_RC_BW_CHANGED) {
+		chwidth = WMI_PEER_CHWIDTH_20MHZ;
+
+		switch (sta->bandwidth) {
+		case IEEE80211_STA_RX_BW_20:
+			chwidth = WMI_PEER_CHWIDTH_20MHZ;
+			break;
+		case IEEE80211_STA_RX_BW_40:
+			chwidth = WMI_PEER_CHWIDTH_40MHZ;
+			break;
+		case IEEE80211_STA_RX_BW_80:
+			chwidth = WMI_PEER_CHWIDTH_80MHZ;
+			break;
+		case IEEE80211_STA_RX_BW_160:
+			chwidth = WMI_PEER_CHWIDTH_20MHZ;
+			ath10k_warn("unsupported STA BW: %d\n", sta->bandwidth);
+			break;
+		}
+
+		ath10k_dbg(ATH10K_DBG_MAC,
+			   "mac update sta %pM bandwidth (peer chwidth %d) to %d\n",
+			   sta->addr, chwidth, sta->bandwidth);
+
+		ret = ath10k_wmi_peer_set_param(ar, arvif->vdev_id, sta->addr,
+						WMI_PEER_CHAN_WIDTH, chwidth);
+		if (ret)
+			ath10k_warn("failed to update STA %pM bandwidth (peer chwidth %d) to %d: %d\n",
+				    sta->addr, chwidth, sta->bandwidth, ret);
+	}
+
+	if (changed & IEEE80211_RC_NSS_CHANGED) {
+		ath10k_dbg(ATH10K_DBG_MAC, "mac update sta %pM nss to %d\n",
+			   sta->addr, sta->rx_nss);
+
+		ret = ath10k_wmi_peer_set_param(ar, arvif->vdev_id, sta->addr,
+						WMI_PEER_NSS, sta->rx_nss);
+		if (ret)
+			ath10k_warn("failed to update STA %pM nss to %d: %d\n",
+				    sta->addr, sta->rx_nss, ret);
+	}
+
+	if (changed & IEEE80211_RC_SMPS_CHANGED) {
+		smps = WMI_PEER_SMPS_PS_NONE;
+
+		switch (sta->smps_mode) {
+		case IEEE80211_SMPS_AUTOMATIC:
+		case IEEE80211_SMPS_OFF:
+			smps = WMI_PEER_SMPS_PS_NONE;
+			break;
+		case IEEE80211_SMPS_STATIC:
+			smps = WMI_PEER_SMPS_STATIC;
+			break;
+		case IEEE80211_SMPS_DYNAMIC:
+			smps = WMI_PEER_SMPS_DYNAMIC;
+			break;
+		case IEEE80211_SMPS_NUM_MODES:
+			smps = WMI_PEER_SMPS_PS_NONE;
+			ath10k_warn("invalid smps mode: %d\n", sta->smps_mode);
+			break;
+		}
+
+		ath10k_dbg(ATH10K_DBG_MAC,
+			   "mac update sta %pM smps (peer smps %d) to %d\n",
+			   sta->addr, smps, sta->smps_mode);
+
+		ret = ath10k_wmi_peer_set_param(ar, arvif->vdev_id, sta->addr,
+						WMI_PEER_SMPS_STATE, smps);
+		if (ret)
+			ath10k_warn("failed to update STA %pM smps (peer smps %d) to %d: %d\n",
+				    sta->addr, smps, sta->smps_mode, ret);
+	}
+
+	if (changed & IEEE80211_RC_SUPP_RATES_CHANGED) {
+		/* FIXME: Not implemented. Not really sure if it's possible to
+		 * influence HW rate control so much. */
+		ath10k_dbg(ATH10K_DBG_MAC,
+			   "mac sta rc update - supp rates changed - not implemented\n");
+	}
+}
+
 static const struct ieee80211_ops ath10k_ops = {
 	.tx				= ath10k_tx,
 	.start				= ath10k_start,
@@ -3332,6 +3422,7 @@ static const struct ieee80211_ops ath10k_ops = {
 	.tx_last_beacon			= ath10k_tx_last_beacon,
 	.restart_complete		= ath10k_restart_complete,
 	.get_survey			= ath10k_get_survey,
+	.sta_rc_update			= ath10k_sta_rc_update,
 #ifdef CONFIG_PM
 	.suspend			= ath10k_suspend,
 	.resume				= ath10k_resume,
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 0087d69..e8c4bb7 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -3847,6 +3847,12 @@ enum wmi_peer_smps_state {
 	WMI_PEER_SMPS_DYNAMIC = 0x2
 };
 
+enum wmi_peer_chwidth {
+	WMI_PEER_CHWIDTH_20MHZ = 0,
+	WMI_PEER_CHWIDTH_40MHZ = 1,
+	WMI_PEER_CHWIDTH_80MHZ = 2,
+};
+
 enum wmi_peer_param {
 	WMI_PEER_SMPS_STATE = 0x1, /* see %wmi_peer_smps_state */
 	WMI_PEER_AMPDU      = 0x2,
-- 
1.8.4.rc3

Re: [PATCH v2] ath10k: implement sta_rc_update()

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> This should fix possible connectivity issues upon
> changes of channel width, number of streams or
> SMPS on connected stations.
>
> An example trigger would be an action frame with
> operation mode change notification.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

[...]

> +static void ath10k_sta_rc_update(struct ieee80211_hw *hw,
> +				 struct ieee80211_vif *vif,
> +				 struct ieee80211_sta *sta,
> +				 u32 changed)
> +{
> +	struct ath10k *ar = hw->priv;
> +	struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
> +	u32 chwidth, smps;
> +	int ret;
> +

[...]

> +		ret = ath10k_wmi_peer_set_param(ar, arvif->vdev_id, sta->addr,
> +						WMI_PEER_CHAN_WIDTH, chwidth);

Johannes pointed out (danke!) that sta_rc_update() must be atomic, but
all these WMI calls can sleep.

-- 
Kalle Valo

Re: [PATCH v2] ath10k: implement sta_rc_update()

[Ben Greear]

On 12/20/2013 02:21 AM, Kalle Valo wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> This should fix possible connectivity issues upon
>> changes of channel width, number of streams or
>> SMPS on connected stations.
>>
>> An example trigger would be an action frame with
>> operation mode change notification.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> [...]
>
>> +static void ath10k_sta_rc_update(struct ieee80211_hw *hw,
>> +				 struct ieee80211_vif *vif,
>> +				 struct ieee80211_sta *sta,
>> +				 u32 changed)
>> +{
>> +	struct ath10k *ar = hw->priv;
>> +	struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
>> +	u32 chwidth, smps;
>> +	int ret;
>> +
>
> [...]
>
>> +		ret = ath10k_wmi_peer_set_param(ar, arvif->vdev_id, sta->addr,
>> +						WMI_PEER_CHAN_WIDTH, chwidth);
>
> Johannes pointed out (danke!) that sta_rc_update() must be atomic, but
> all these WMI calls can sleep.

Another thing I have noticed, but have not had time to think much about,
is that when the firmware gets out of sync for whatever reason, it can
start just ignoring wmi commands and everything times out.  Trying to
remove station vifs that are not currently in the firmware, perhaps due
to firmware crash, is a good way to hit this.

The system has very slow response to any number of things that
might require rtnl (or probably other locks).  I think we are basically
holding rtnl or other large-scale locks through the wmi calls.

I think a good long-term solution would be to make the firmware smart enough
to return error codes instead of just ignoring (or crashing on) requests it
cannot properly handle.  Maybe handling the wmi commands in a more
asynchronous manner might be a good strategy in the driver...

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

[PATCH v2] ath10k: implement sta_rc_update()

[Michal Kazior]

This allows dynamic changes of bandwidth/nss/smps,
e.g. via ht/vht operation mode change
notification.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
v2:
 * fix memset(&arsta, ...) -> memset(arsta, ...)

 drivers/net/wireless/ath/ath10k/core.h |  11 +++
 drivers/net/wireless/ath/ath10k/mac.c  | 149 +++++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/wmi.h  |   6 ++
 3 files changed, 166 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index c0b00e1..147348a 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -228,6 +228,17 @@ struct ath10k_peer {
 	struct ieee80211_key_conf *keys[WMI_MAX_KEY_INDEX + 1];
 };
 
+struct ath10k_sta {
+	struct ath10k_vif *arvif;
+
+	u32 changed; /* IEEE80211_RC_* */
+	u32 bw;
+	u32 nss;
+	u32 smps;
+
+	struct work_struct update_wk;
+};
+
 #define ATH10K_VDEV_SETUP_TIMEOUT_HZ (5*HZ)
 
 struct ath10k_vif {
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 144b4d6..ecc06b08 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -3056,6 +3056,69 @@ exit:
 	return ret;
 }
 
+static void ath10k_sta_rc_update_wk(struct work_struct *wk)
+{
+	struct ath10k *ar;
+	struct ath10k_vif *arvif;
+	struct ath10k_sta *arsta;
+	struct ieee80211_sta *sta;
+	u32 changed, bw, nss, smps;
+	int err;
+
+	arsta = container_of(wk, struct ath10k_sta, update_wk);
+	sta = container_of((void *)arsta, struct ieee80211_sta, drv_priv);
+	arvif = arsta->arvif;
+	ar = arvif->ar;
+
+	spin_lock_bh(&ar->data_lock);
+
+	changed = arsta->changed;
+	arsta->changed = 0;
+
+	bw = arsta->bw;
+	nss = arsta->nss;
+	smps = arsta->smps;
+
+	spin_unlock_bh(&ar->data_lock);
+
+	mutex_lock(&ar->conf_mutex);
+
+	if (changed & IEEE80211_RC_BW_CHANGED) {
+		ath10k_dbg(ATH10K_DBG_MAC, "mac update sta %pM peer bw %d\n",
+			   sta->addr, bw);
+
+		err = ath10k_wmi_peer_set_param(ar, arvif->vdev_id, sta->addr,
+						WMI_PEER_CHAN_WIDTH, bw);
+		if (err)
+			ath10k_warn("failed to update STA %pM peer bw %d: %d\n",
+				    sta->addr, bw, err);
+	}
+
+	if (changed & IEEE80211_RC_NSS_CHANGED) {
+		ath10k_dbg(ATH10K_DBG_MAC, "mac update sta %pM nss %d\n",
+			   sta->addr, nss);
+
+		err = ath10k_wmi_peer_set_param(ar, arvif->vdev_id, sta->addr,
+						WMI_PEER_NSS, nss);
+		if (err)
+			ath10k_warn("failed to update STA %pM nss %d: %d\n",
+				    sta->addr, nss, err);
+	}
+
+	if (changed & IEEE80211_RC_SMPS_CHANGED) {
+		ath10k_dbg(ATH10K_DBG_MAC, "mac update sta %pM smps %d\n",
+			   sta->addr, smps);
+
+		err = ath10k_wmi_peer_set_param(ar, arvif->vdev_id, sta->addr,
+						WMI_PEER_SMPS_STATE, smps);
+		if (err)
+			ath10k_warn("failed to update STA %pM smps %d: %d\n",
+				    sta->addr, smps, err);
+	}
+
+	mutex_unlock(&ar->conf_mutex);
+}
+
 static int ath10k_sta_state(struct ieee80211_hw *hw,
 			    struct ieee80211_vif *vif,
 			    struct ieee80211_sta *sta,
@@ -3064,9 +3127,15 @@ static int ath10k_sta_state(struct ieee80211_hw *hw,
 {
 	struct ath10k *ar = hw->priv;
 	struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+	struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
 	int max_num_peers;
 	int ret = 0;
 
+	/* cancel must be done outside the mutex to avoid deadlock */
+	if ((old_state == IEEE80211_STA_NONE &&
+	     new_state == IEEE80211_STA_NOTEXIST))
+		cancel_work_sync(&arsta->update_wk);
+
 	mutex_lock(&ar->conf_mutex);
 
 	if (old_state == IEEE80211_STA_NOTEXIST &&
@@ -3091,6 +3160,10 @@ static int ath10k_sta_state(struct ieee80211_hw *hw,
 			   "mac vdev %d peer create %pM (new sta) num_peers %d\n",
 			   arvif->vdev_id, sta->addr, ar->num_peers);
 
+		memset(arsta, 0, sizeof(*arsta));
+		arsta->arvif = arvif;
+		INIT_WORK(&arsta->update_wk, ath10k_sta_rc_update_wk);
+
 		ret = ath10k_peer_create(ar, arvif->vdev_id, sta->addr);
 		if (ret)
 			ath10k_warn("Failed to add peer %pM for vdev %d when adding a new sta: %i\n",
@@ -3854,6 +3927,80 @@ static void ath10k_channel_switch_beacon(struct ieee80211_hw *hw,
 	return;
 }
 
+static void ath10k_sta_rc_update(struct ieee80211_hw *hw,
+				 struct ieee80211_vif *vif,
+				 struct ieee80211_sta *sta,
+				 u32 changed)
+{
+	struct ath10k *ar = hw->priv;
+	struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
+	u32 bw, smps;
+
+	spin_lock_bh(&ar->data_lock);
+
+	if (changed & IEEE80211_RC_BW_CHANGED) {
+		bw = WMI_PEER_CHWIDTH_20MHZ;
+
+		switch (sta->bandwidth) {
+		case IEEE80211_STA_RX_BW_20:
+			bw = WMI_PEER_CHWIDTH_20MHZ;
+			break;
+		case IEEE80211_STA_RX_BW_40:
+			bw = WMI_PEER_CHWIDTH_40MHZ;
+			break;
+		case IEEE80211_STA_RX_BW_80:
+			bw = WMI_PEER_CHWIDTH_80MHZ;
+			break;
+		case IEEE80211_STA_RX_BW_160:
+			ath10k_warn("unsupported STA BW: %d\n", sta->bandwidth);
+			bw = WMI_PEER_CHWIDTH_20MHZ;
+			break;
+		}
+
+		arsta->bw = bw;
+	}
+
+	if (changed & IEEE80211_RC_NSS_CHANGED)
+		arsta->nss = sta->rx_nss;
+
+	if (changed & IEEE80211_RC_SMPS_CHANGED) {
+		smps = WMI_PEER_SMPS_PS_NONE;
+
+		switch (sta->smps_mode) {
+		case IEEE80211_SMPS_AUTOMATIC:
+		case IEEE80211_SMPS_OFF:
+			smps = WMI_PEER_SMPS_PS_NONE;
+			break;
+		case IEEE80211_SMPS_STATIC:
+			smps = WMI_PEER_SMPS_STATIC;
+			break;
+		case IEEE80211_SMPS_DYNAMIC:
+			smps = WMI_PEER_SMPS_DYNAMIC;
+			break;
+		case IEEE80211_SMPS_NUM_MODES:
+			ath10k_warn("invalid smps mode: %d\n", sta->smps_mode);
+			smps = WMI_PEER_SMPS_PS_NONE;
+			break;
+		}
+
+		arsta->smps = smps;
+	}
+
+	if (changed & IEEE80211_RC_SUPP_RATES_CHANGED) {
+		/* FIXME: Not implemented. Probably the only way to do it would
+		 * be to re-assoc the peer. */
+		changed &= ~IEEE80211_RC_SUPP_RATES_CHANGED;
+		ath10k_dbg(ATH10K_DBG_MAC,
+			   "mac sta rc update - supp rates changed - not implemented\n");
+	}
+
+	arsta->changed |= changed;
+
+	spin_unlock_bh(&ar->data_lock);
+
+	ieee80211_queue_work(hw, &arsta->update_wk);
+}
+
 static const struct ieee80211_ops ath10k_ops = {
 	.tx				= ath10k_tx,
 	.start				= ath10k_start,
@@ -3878,6 +4025,7 @@ static const struct ieee80211_ops ath10k_ops = {
 	.get_survey			= ath10k_get_survey,
 	.set_bitrate_mask		= ath10k_set_bitrate_mask,
 	.channel_switch_beacon		= ath10k_channel_switch_beacon,
+	.sta_rc_update			= ath10k_sta_rc_update,
 #ifdef CONFIG_PM
 	.suspend			= ath10k_suspend,
 	.resume				= ath10k_resume,
@@ -4253,6 +4401,7 @@ int ath10k_mac_register(struct ath10k *ar)
 	ar->hw->wiphy->max_scan_ie_len = WLAN_SCAN_PARAMS_MAX_IE_LEN;
 
 	ar->hw->vif_data_size = sizeof(struct ath10k_vif);
+	ar->hw->sta_data_size = sizeof(struct ath10k_sta);
 
 	ar->hw->max_listen_interval = ATH10K_MAX_HW_LISTEN_INTERVAL;
 
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 083079f..6df4f95 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -3876,6 +3876,12 @@ enum wmi_peer_smps_state {
 	WMI_PEER_SMPS_DYNAMIC = 0x2
 };
 
+enum wmi_peer_chwidth {
+	WMI_PEER_CHWIDTH_20MHZ = 0,
+	WMI_PEER_CHWIDTH_40MHZ = 1,
+	WMI_PEER_CHWIDTH_80MHZ = 2,
+};
+
 enum wmi_peer_param {
 	WMI_PEER_SMPS_STATE = 0x1, /* see %wmi_peer_smps_state */
 	WMI_PEER_AMPDU      = 0x2,
-- 
1.8.5.3

Re: [PATCH v2] ath10k: implement sta_rc_update()

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> This allows dynamic changes of bandwidth/nss/smps,
> e.g. via ht/vht operation mode change
> notification.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

[...]

> --- a/drivers/net/wireless/ath/ath10k/core.h
> +++ b/drivers/net/wireless/ath/ath10k/core.h
> @@ -228,6 +228,17 @@ struct ath10k_peer {
>  	struct ieee80211_key_conf *keys[WMI_MAX_KEY_INDEX + 1];
>  };
>  
> +struct ath10k_sta {
> +	struct ath10k_vif *arvif;
> +
> +	u32 changed; /* IEEE80211_RC_* */
> +	u32 bw;
> +	u32 nss;
> +	u32 smps;
> +
> +	struct work_struct update_wk;
> +};

Apparently this structure is protected with data_lock, but it would be
good to document that in the code to make it clear.

> +static void ath10k_sta_rc_update(struct ieee80211_hw *hw,
> +				 struct ieee80211_vif *vif,
> +				 struct ieee80211_sta *sta,
> +				 u32 changed)
> +{
> +	struct ath10k *ar = hw->priv;
> +	struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
> +	u32 bw, smps;
> +
> +	spin_lock_bh(&ar->data_lock);
> +
> +	if (changed & IEEE80211_RC_BW_CHANGED) {
> +		bw = WMI_PEER_CHWIDTH_20MHZ;
> +
> +		switch (sta->bandwidth) {
> +		case IEEE80211_STA_RX_BW_20:
> +			bw = WMI_PEER_CHWIDTH_20MHZ;
> +			break;
> +		case IEEE80211_STA_RX_BW_40:
> +			bw = WMI_PEER_CHWIDTH_40MHZ;
> +			break;
> +		case IEEE80211_STA_RX_BW_80:
> +			bw = WMI_PEER_CHWIDTH_80MHZ;
> +			break;
> +		case IEEE80211_STA_RX_BW_160:
> +			ath10k_warn("unsupported STA BW: %d\n", sta->bandwidth);
> +			bw = WMI_PEER_CHWIDTH_20MHZ;
> +			break;

I think it would be also useful to print STA's address in the warning.

> +	if (changed & IEEE80211_RC_SMPS_CHANGED) {
> +		smps = WMI_PEER_SMPS_PS_NONE;
> +
> +		switch (sta->smps_mode) {
> +		case IEEE80211_SMPS_AUTOMATIC:
> +		case IEEE80211_SMPS_OFF:
> +			smps = WMI_PEER_SMPS_PS_NONE;
> +			break;
> +		case IEEE80211_SMPS_STATIC:
> +			smps = WMI_PEER_SMPS_STATIC;
> +			break;
> +		case IEEE80211_SMPS_DYNAMIC:
> +			smps = WMI_PEER_SMPS_DYNAMIC;
> +			break;
> +		case IEEE80211_SMPS_NUM_MODES:
> +			ath10k_warn("invalid smps mode: %d\n", sta->smps_mode);
> +			smps = WMI_PEER_SMPS_PS_NONE;
> +			break;

Maybe here as well?

-- 
Kalle Valo

Re: [PATCH v2] ath10k: implement sta_rc_update()

[Michal Kazior]

On 13 February 2014 15:46, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> This allows dynamic changes of bandwidth/nss/smps,
>> e.g. via ht/vht operation mode change
>> notification.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> [...]
>
>> --- a/drivers/net/wireless/ath/ath10k/core.h
>> +++ b/drivers/net/wireless/ath/ath10k/core.h
>> @@ -228,6 +228,17 @@ struct ath10k_peer {
>>       struct ieee80211_key_conf *keys[WMI_MAX_KEY_INDEX + 1];
>>  };
>>
>> +struct ath10k_sta {
>> +     struct ath10k_vif *arvif;
>> +
>> +     u32 changed; /* IEEE80211_RC_* */
>> +     u32 bw;
>> +     u32 nss;
>> +     u32 smps;
>> +
>> +     struct work_struct update_wk;
>> +};
>
> Apparently this structure is protected with data_lock, but it would be
> good to document that in the code to make it clear.

Good point.



>> +             case IEEE80211_STA_RX_BW_160:
>> +                     ath10k_warn("unsupported STA BW: %d\n", sta->bandwidth);
>> +                     bw = WMI_PEER_CHWIDTH_20MHZ;
>> +                     break;
>
> I think it would be also useful to print STA's address in the warning.
>

[...]

>> +             case IEEE80211_SMPS_NUM_MODES:
>> +                     ath10k_warn("invalid smps mode: %d\n", sta->smps_mode);
>> +                     smps = WMI_PEER_SMPS_PS_NONE;
>> +                     break;
>
> Maybe here as well?

Sounds good. I'll send a v3 with all these things fixed.


Michał