[ath9k-devel] [PATCH 0/3] UAPSD support for STA/AP mode

[ath9k-devel] [PATCH 0/3] UAPSD support for STA/AP mode

[Janusz Dziedzic]

First two paches add PS/UAPSD support for AP/P2P_GO mode.
Last one enable UAPSD for STA mode.

In case of FW issues we can easy disable this features by cleaning:
IEEE80211_HW_SUPPORTS_UAPSD/WIPHY_FLAG_AP_UAPSD still have code included.


Janusz Dziedzic (3):
  ath10k: WMI add AP PS
  ath10k: add AP UAPSD support
  ath10k: enable STA UAPSD support

 drivers/net/wireless/ath/ath10k/core.h |    1 +
 drivers/net/wireless/ath/ath10k/mac.c  |  165 +++++++++++++++++++++++++++++---
 drivers/net/wireless/ath/ath10k/wmi.c  |   26 +++++
 drivers/net/wireless/ath/ath10k/wmi.h  |   68 +++++++++++++
 4 files changed, 249 insertions(+), 11 deletions(-)

-- 
1.7.9.5

[ath9k-devel] [PATCH 1/3] ath10k: WMI add AP PS

[Janusz Dziedzic]

Add AP power save (UAPSD) structures, enums.

Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>
---
 drivers/net/wireless/ath/ath10k/wmi.c |   26 +++++++++++++
 drivers/net/wireless/ath/ath10k/wmi.h |   68 +++++++++++++++++++++++++++++++++
 2 files changed, 94 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 15e8123..c5f6fda 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -1896,6 +1896,32 @@ int ath10k_wmi_set_sta_ps_param(struct ath10k *ar, u32 vdev_id,
 	return ath10k_wmi_cmd_send(ar, skb, WMI_STA_POWERSAVE_PARAM_CMDID);
 }
 
+int ath10k_wmi_set_ap_ps_param(struct ath10k *ar, u32 vdev_id, const u8 *mac,
+			       enum wmi_ap_ps_peer_param param_id, u32 value)
+{
+	struct wmi_ap_ps_peer_cmd *cmd;
+	struct sk_buff *skb;
+
+	if (!mac)
+		return -EINVAL;
+
+	skb = ath10k_wmi_alloc_skb(sizeof(*cmd));
+	if (!skb)
+		return -ENOMEM;
+
+	cmd = (struct wmi_ap_ps_peer_cmd *)skb->data;
+	cmd->vdev_id     = __cpu_to_le32(vdev_id);
+	cmd->param_id    = __cpu_to_le32(param_id);
+	cmd->param_value = __cpu_to_le32(value);
+	memcpy(&cmd->peer_macaddr, mac, ETH_ALEN);
+
+	ath10k_dbg(ATH10K_DBG_WMI,
+		   "wmi ap ps param vdev_id 0x%X param %d value %d mac_addr %pM\n",
+		   vdev_id, param_id, value, mac);
+
+	return ath10k_wmi_cmd_send(ar, skb, WMI_AP_PS_PEER_PARAM_CMDID);
+}
+
 int ath10k_wmi_scan_chan_list(struct ath10k *ar,
 			      const struct wmi_scan_chan_list_arg *arg)
 {
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 2ad0431..8d482ab 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -2461,6 +2461,72 @@ struct wmi_sta_mimo_ps_mode_cmd {
 	__le32 mimo_pwrsave_mode;
 } __packed;
 
+/* U-APSD configuration of peer station from (re)assoc request and TSPECs */
+enum wmi_ap_ps_param_uapsd {
+	WMI_AP_PS_UAPSD_AC0_DELIVERY_EN = (1 << 0),
+	WMI_AP_PS_UAPSD_AC0_TRIGGER_EN  = (1 << 1),
+	WMI_AP_PS_UAPSD_AC1_DELIVERY_EN = (1 << 2),
+	WMI_AP_PS_UAPSD_AC1_TRIGGER_EN  = (1 << 3),
+	WMI_AP_PS_UAPSD_AC2_DELIVERY_EN = (1 << 4),
+	WMI_AP_PS_UAPSD_AC2_TRIGGER_EN  = (1 << 5),
+	WMI_AP_PS_UAPSD_AC3_DELIVERY_EN = (1 << 6),
+	WMI_AP_PS_UAPSD_AC3_TRIGGER_EN  = (1 << 7),
+};
+
+/* U-APSD maximum service period of peer station */
+enum wmi_ap_ps_peer_param_max_sp {
+	WMI_AP_PS_PEER_PARAM_MAX_SP_UNLIMITED = 0,
+	WMI_AP_PS_PEER_PARAM_MAX_SP_2 = 1,
+	WMI_AP_PS_PEER_PARAM_MAX_SP_4 = 2,
+	WMI_AP_PS_PEER_PARAM_MAX_SP_6 = 3,
+	MAX_WMI_AP_PS_PEER_PARAM_MAX_SP,
+};
+
+/*
+ * AP power save parameter
+ * Set a power save specific parameter for a peer station
+ */
+enum wmi_ap_ps_peer_param {
+	/* Set uapsd configuration for a given peer.
+	 *
+	 * Include the delivery and trigger enabled state for every AC.
+	 * The host  MLME needs to set this based on AP capability and stations
+	 * request Set in the association request  received from the station.
+	 *
+	 * Lower 8 bits of the value specify the UAPSD configuration.
+	 *
+	 * (see enum wmi_ap_ps_param_uapsd)
+	 * The default value is 0.
+	 */
+	WMI_AP_PS_PEER_PARAM_UAPSD = 0,
+
+	/*
+	 * Set the service period for a UAPSD capable station
+	 *
+	 * The service period from wme ie in the (re)assoc request frame.
+	 *
+	 * (see enum wmi_ap_ps_peer_param_max_sp)
+	 */
+	WMI_AP_PS_PEER_PARAM_MAX_SP = 1,
+
+	/* Time in seconds for aging out buffered frames for STA in PS */
+	WMI_AP_PS_PEER_PARAM_AGEOUT_TIME = 2,
+};
+
+struct wmi_ap_ps_peer_cmd {
+	/* unique id identifying the VDEV, generated by the caller */
+	__le32 vdev_id;
+
+	/* peer MAC address */
+	struct wmi_mac_addr peer_macaddr;
+
+	/* AP powersave param (see enum wmi_ap_ps_peer_param) */
+	__le32 param_id;
+
+	/* AP powersave param value */
+	__le32 param_value;
+} __packed;
+
 /* 128 clients = 4 words */
 #define WMI_TIM_BITMAP_ARRAY_SIZE 4
 
@@ -2876,6 +2942,8 @@ int ath10k_wmi_set_psmode(struct ath10k *ar, u32 vdev_id,
 int ath10k_wmi_set_sta_ps_param(struct ath10k *ar, u32 vdev_id,
 				enum wmi_sta_powersave_param param_id,
 				u32 value);
+int ath10k_wmi_set_ap_ps_param(struct ath10k *ar, u32 vdev_id, const u8 *mac,
+			       enum wmi_ap_ps_peer_param param_id, u32 value);
 int ath10k_wmi_scan_chan_list(struct ath10k *ar,
 			      const struct wmi_scan_chan_list_arg *arg);
 int ath10k_wmi_beacon_send(struct ath10k *ar, const struct wmi_bcn_tx_arg *arg);
-- 
1.7.9.5

[ath9k-devel] [PATCH 1/3] ath10k: WMI add AP PS

[Kalle Valo]

Janusz Dziedzic <janusz.dziedzic@tieto.com> writes:

> Add AP power save (UAPSD) structures, enums.
>
> Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>

[...]

> +	cmd->vdev_id     = __cpu_to_le32(vdev_id);
> +	cmd->param_id    = __cpu_to_le32(param_id);
> +	cmd->param_value = __cpu_to_le32(value);

Remove the extra whitespace before '='.

-- 
Kalle Valo

[ath9k-devel] [PATCH 1/3] ath10k: WMI add AP PS

[Janusz.Dziedzic at tieto.com]

>-----Original Message-----
>From: Kalle Valo [mailto:kvalo at qca.qualcomm.com]
>Sent: 9 maja 2013 09:10
>To: Dziedzic Janusz
>Cc: ath9k-devel at lists.ath9k.org
>Subject: Re: [ath9k-devel] [PATCH 1/3] ath10k: WMI add AP PS
>
>Janusz Dziedzic <janusz.dziedzic@tieto.com> writes:
>
>> Add AP power save (UAPSD) structures, enums.
>>
>> Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>
>
>[...]
>
>> +	cmd->vdev_id     = __cpu_to_le32(vdev_id);
>> +	cmd->param_id    = __cpu_to_le32(param_id);
>> +	cmd->param_value = __cpu_to_le32(value);
>
>Remove the extra whitespace before '='.
>

I did same style we have in wmi.c In other functions,  
Eg. (ath10k_wmi_set_psmode)
        cmd->vdev_id     = __cpu_to_le32(vdev_id);
        cmd->sta_ps_mode = __cpu_to_le32(psmode);

So, should I change this?

BR
Janusz

[ath9k-devel] [PATCH 1/3] ath10k: WMI add AP PS

[Kalle Valo]

<Janusz.Dziedzic@tieto.com> writes:

>>-----Original Message-----
>>From: Kalle Valo [mailto:kvalo at qca.qualcomm.com]
>>Sent: 9 maja 2013 09:10
>>To: Dziedzic Janusz
>>Cc: ath9k-devel at lists.ath9k.org
>>Subject: Re: [ath9k-devel] [PATCH 1/3] ath10k: WMI add AP PS
>>
>>Janusz Dziedzic <janusz.dziedzic@tieto.com> writes:
>>
>>> Add AP power save (UAPSD) structures, enums.
>>>
>>> Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>
>>
>>[...]
>>
>>> +	cmd->vdev_id     = __cpu_to_le32(vdev_id);
>>> +	cmd->param_id    = __cpu_to_le32(param_id);
>>> +	cmd->param_value = __cpu_to_le32(value);
>>
>>Remove the extra whitespace before '='.
>>
>
> I did same style we have in wmi.c In other functions,  

Yeah, I know. But I would prefer not to use that style in ath10k. I have
been trying to remove that style from ath10k, but I haven't converted
all cases yet.

> Eg. (ath10k_wmi_set_psmode)
>         cmd->vdev_id     = __cpu_to_le32(vdev_id);
>         cmd->sta_ps_mode = __cpu_to_le32(psmode);
>
> So, should I change this?

I'm happy to take patches :)

-- 
Kalle Valo

[ath9k-devel] [PATCH 2/3] ath10k: add AP UAPSD support

[Janusz Dziedzic]

Add support for AP (P2P_GO) UAPSD.

Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>
---
 drivers/net/wireless/ath/ath10k/mac.c |   85 ++++++++++++++++++++++++++++++---
 1 file changed, 78 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 930a092..91de248 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -886,21 +886,91 @@ static void ath10k_peer_assoc_h_ht(struct ath10k *ar,
 		   arg->peer_num_spatial_streams);
 }
 
+static void ath10k_peer_assoc_h_qos_ap(struct ath10k *ar,
+				       struct ath10k_vif *arvif,
+				       struct ieee80211_sta *sta,
+				       struct ieee80211_bss_conf *bss_conf,
+				       struct wmi_peer_assoc_complete_arg *arg)
+{
+	u32 uapsd = 0;
+	u32 max_sp = 0;
+
+	if (sta->wme)
+		arg->peer_flags |= WMI_PEER_QOS;
+
+	if (sta->wme && sta->uapsd_queues) {
+		ath10k_dbg(ATH10K_DBG_MAC, "uapsd_queues: 0x%X, max_sp: %d\n",
+			   sta->uapsd_queues, sta->max_sp);
+
+		arg->peer_flags |= WMI_PEER_APSD;
+		arg->peer_flags |= WMI_RC_UAPSD_FLAG;
+
+		if (sta->uapsd_queues & IEEE80211_WMM_IE_STA_QOSINFO_AC_VO)
+			uapsd |= WMI_AP_PS_UAPSD_AC3_DELIVERY_EN |
+				 WMI_AP_PS_UAPSD_AC3_TRIGGER_EN;
+		if (sta->uapsd_queues & IEEE80211_WMM_IE_STA_QOSINFO_AC_VI)
+			uapsd |= WMI_AP_PS_UAPSD_AC2_DELIVERY_EN |
+				 WMI_AP_PS_UAPSD_AC2_TRIGGER_EN;
+		if (sta->uapsd_queues & IEEE80211_WMM_IE_STA_QOSINFO_AC_BK)
+			uapsd |= WMI_AP_PS_UAPSD_AC1_DELIVERY_EN |
+				 WMI_AP_PS_UAPSD_AC1_TRIGGER_EN;
+		if (sta->uapsd_queues & IEEE80211_WMM_IE_STA_QOSINFO_AC_BE)
+			uapsd |= WMI_AP_PS_UAPSD_AC0_DELIVERY_EN |
+				 WMI_AP_PS_UAPSD_AC0_TRIGGER_EN;
+
+
+		if (sta->max_sp < MAX_WMI_AP_PS_PEER_PARAM_MAX_SP)
+			max_sp = sta->max_sp;
+
+		ath10k_wmi_set_ap_ps_param(ar, arvif->vdev_id,
+					   sta->addr,
+					   WMI_AP_PS_PEER_PARAM_UAPSD,
+					   uapsd);
+
+		ath10k_wmi_set_ap_ps_param(ar, arvif->vdev_id,
+					   sta->addr,
+					   WMI_AP_PS_PEER_PARAM_MAX_SP,
+					   max_sp);
+
+		/* TODO setup this based on STA listen interval and
+		   beacon interval. Currently we don't know
+		   sta->listen_interval - mac80211 patch required.
+		   Currently use 10 seconds */
+		ath10k_wmi_set_ap_ps_param(ar, arvif->vdev_id,
+					   sta->addr,
+					   WMI_AP_PS_PEER_PARAM_AGEOUT_TIME,
+					   10);
+	}
+}
+
 /*
- * FIXME: Handle UAPSD later.
+ * FIXME: Handle STA UAPSD later.
  */
+static void ath10k_peer_assoc_h_qos_sta(struct ath10k *ar,
+					struct ath10k_vif *arvif,
+					struct ieee80211_sta *sta,
+					struct ieee80211_bss_conf *bss_conf,
+					struct wmi_peer_assoc_complete_arg *arg)
+{
+	if (bss_conf->qos)
+		arg->peer_flags |= WMI_PEER_QOS;
+}
+
 static void ath10k_peer_assoc_h_qos(struct ath10k *ar,
 				    struct ath10k_vif *arvif,
 				    struct ieee80211_sta *sta,
 				    struct ieee80211_bss_conf *bss_conf,
 				    struct wmi_peer_assoc_complete_arg *arg)
 {
-	if (arvif->vdev_type == WMI_VDEV_TYPE_AP) {
-		if (sta->wme)
-			arg->peer_flags |= WMI_PEER_QOS;
-	} else if (arvif->vdev_type == WMI_VDEV_TYPE_STA) {
-		if (bss_conf->qos)
-			arg->peer_flags |= WMI_PEER_QOS;
+	switch (arvif->vdev_type) {
+	case WMI_VDEV_TYPE_AP:
+		ath10k_peer_assoc_h_qos_ap(ar, arvif, sta, bss_conf, arg);
+		break;
+	case WMI_VDEV_TYPE_STA:
+		ath10k_peer_assoc_h_qos_sta(ar, arvif, sta, bss_conf, arg);
+		break;
+	default:
+		break;
 	}
 }
 
@@ -2798,6 +2868,7 @@ int ath10k_mac_register(struct ath10k *ar)
 	ar->hw->wiphy->flags |= WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL;
 	ar->hw->wiphy->max_remain_on_channel_duration = 5000;
 
+	ar->hw->wiphy->flags |= WIPHY_FLAG_AP_UAPSD;
 	/*
 	 * on LL hardware queues are managed entirely by the FW
 	 * so we only advertise to mac we can do the queues thing
-- 
1.7.9.5

[ath9k-devel] [PATCH 2/3] ath10k: add AP UAPSD support

[Kalle Valo]

Janusz Dziedzic <janusz.dziedzic@tieto.com> writes:

> Add support for AP (P2P_GO) UAPSD.
>
> Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>

You should briefly mention in the commit log how you have tested this.

-- 
Kalle Valo

[ath9k-devel] [PATCH 3/3] ath10k: enable STA UAPSD support

[Janusz Dziedzic]

Enable UAPSD support for STA mode.

Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.h |    1 +
 drivers/net/wireless/ath/ath10k/mac.c  |   80 ++++++++++++++++++++++++++++++--
 2 files changed, 77 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 6c681cf..bef85fb 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -217,6 +217,7 @@ struct ath10k_vif {
 	union {
 		struct {
 			u8 bssid[ETH_ALEN];
+			u32 uapsd;
 		} sta;
 		struct {
 			/* 127 stations; wmi limit */
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 91de248..9b24530 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2296,6 +2296,68 @@ static int ath10k_sta_state(struct ieee80211_hw *hw,
 	return ret;
 }
 
+#define SET_UAPSD(enable, out, flags) do {	\
+	if ((enable))				\
+		(out) |= (flags);		\
+	else					\
+		(out) &= ~(flags);		\
+} while (0)
+
+static int ath10k_conf_tx_uapsd(struct ath10k *ar, struct ieee80211_vif *vif,
+				 u16 ac, bool uapsd)
+{
+	struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
+	u32 value;
+	int ret = 0;
+
+	if (arvif->vdev_type != WMI_VDEV_TYPE_STA)
+		return 0;
+
+	switch (ac) {
+	case IEEE80211_AC_VO:
+		SET_UAPSD(uapsd, arvif->u.sta.uapsd,
+			  WMI_STA_PS_UAPSD_AC3_DELIVERY_EN |
+			  WMI_STA_PS_UAPSD_AC3_TRIGGER_EN);
+		break;
+	case IEEE80211_AC_VI:
+		SET_UAPSD(uapsd, arvif->u.sta.uapsd,
+			  WMI_STA_PS_UAPSD_AC2_DELIVERY_EN |
+			  WMI_STA_PS_UAPSD_AC2_TRIGGER_EN);
+		break;
+	case IEEE80211_AC_BE:
+		SET_UAPSD(uapsd, arvif->u.sta.uapsd,
+			  WMI_STA_PS_UAPSD_AC1_DELIVERY_EN |
+			  WMI_STA_PS_UAPSD_AC1_TRIGGER_EN);
+		break;
+	case IEEE80211_AC_BK:
+		SET_UAPSD(uapsd, arvif->u.sta.uapsd,
+			  WMI_STA_PS_UAPSD_AC0_DELIVERY_EN |
+			  WMI_STA_PS_UAPSD_AC0_TRIGGER_EN);
+		break;
+	}
+
+	ret = ath10k_wmi_set_sta_ps_param(ar, arvif->vdev_id,
+					  WMI_STA_PS_PARAM_UAPSD,
+					  arvif->u.sta.uapsd);
+	if (ret) {
+		ath10k_warn("could not set uapsd params (%d)\n", ret);
+		goto exit;
+	}
+
+	if (arvif->u.sta.uapsd)
+		value = WMI_STA_PS_RX_WAKE_POLICY_POLL_UAPSD;
+	else
+		value = WMI_STA_PS_RX_WAKE_POLICY_WAKE;
+
+	ret = ath10k_wmi_set_sta_ps_param(ar, arvif->vdev_id,
+					  WMI_STA_PS_PARAM_RX_WAKE_POLICY,
+					  value);
+	if (ret)
+		ath10k_warn("could not set rx wake param (%d)\n", ret);
+exit:
+	return ret;
+}
+
 static int ath10k_conf_tx(struct ieee80211_hw *hw,
 			  struct ieee80211_vif *vif, u16 ac,
 			  const struct ieee80211_tx_queue_params *params)
@@ -2321,8 +2383,10 @@ static int ath10k_conf_tx(struct ieee80211_hw *hw,
 		break;
 	}
 
-	if (WARN_ON(!p))
-		return -EINVAL;
+	if (WARN_ON(!p)) {
+		ret = -EINVAL;
+		goto exit;
+	}
 
 	p->cwmin = params->cw_min;
 	p->cwmax = params->cw_max;
@@ -2335,13 +2399,20 @@ static int ath10k_conf_tx(struct ieee80211_hw *hw,
 	 */
 	p->txop = params->txop * 32;
 
-	/* FIXME: can we pass the params->uapsd to the FW? */
+
 	/* FIXME: FW accepts wmm params per hw, not per vif */
 
 	ret = ath10k_wmi_pdev_set_wmm_params(ar, &ar->wmm_params);
-	if (ret)
+	if (ret) {
 		ath10k_warn("could not set wmm params (%d)\n", ret);
+		goto exit;
+	}
 
+	ret = ath10k_conf_tx_uapsd(ar, vif, ac, params->uapsd);
+	if (ret)
+		ath10k_warn("could not set sta uapsd (%d)\n", ret);
+
+exit:
 	mutex_unlock(&ar->conf_mutex);
 	return ret;
 }
@@ -2842,6 +2913,7 @@ int ath10k_mac_register(struct ath10k *ar)
 	ar->hw->flags = IEEE80211_HW_SIGNAL_DBM |
 			IEEE80211_HW_SUPPORTS_PS |
 			IEEE80211_HW_SUPPORTS_DYNAMIC_PS |
+			IEEE80211_HW_SUPPORTS_UAPSD |
 			IEEE80211_HW_MFP_CAPABLE |
 			IEEE80211_HW_REPORTS_TX_ACK_STATUS |
 			IEEE80211_HW_HAS_RATE_CONTROL |
-- 
1.7.9.5

[ath9k-devel] [PATCH 3/3] ath10k: enable STA UAPSD support

[Kalle Valo]

Janusz Dziedzic <janusz.dziedzic@tieto.com> writes:

> Enable UAPSD support for STA mode.
>
> Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>

Same here, please explain briefly what you have tested.

> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -2296,6 +2296,68 @@ static int ath10k_sta_state(struct ieee80211_hw *hw,
>  	return ret;
>  }
>  
> +#define SET_UAPSD(enable, out, flags) do {	\
> +	if ((enable))				\
> +		(out) |= (flags);		\
> +	else					\
> +		(out) &= ~(flags);		\
> +} while (0)

Why the macro? To workaround a long line warning from checkpatch?

If that's a problem we could increate line length limit, for example to
85 or 90. In some cases the 80 char limit is a bit too excessive. Would
that help?

[...]

> +	if (ret) {
> +		ath10k_warn("could not set uapsd params (%d)\n", ret);

"could not set uapsd params: %d\n"

> +		goto exit;
> +	}
> +
> +	if (arvif->u.sta.uapsd)
> +		value = WMI_STA_PS_RX_WAKE_POLICY_POLL_UAPSD;
> +	else
> +		value = WMI_STA_PS_RX_WAKE_POLICY_WAKE;
> +
> +	ret = ath10k_wmi_set_sta_ps_param(ar, arvif->vdev_id,
> +					  WMI_STA_PS_PARAM_RX_WAKE_POLICY,
> +					  value);
> +	if (ret)
> +		ath10k_warn("could not set rx wake param (%d)\n", ret);
> +exit:
> +	return ret;
> +}

Empty line before the exit label.

> @@ -2335,13 +2399,20 @@ static int ath10k_conf_tx(struct ieee80211_hw *hw,
>  	 */
>  	p->txop = params->txop * 32;
>  
> -	/* FIXME: can we pass the params->uapsd to the FW? */
> +

This now has two empty lines, one is enough.

> -	if (ret)
> +	if (ret) {
>  		ath10k_warn("could not set wmm params (%d)\n", ret);
> +		goto exit;
> +	}

The same comment as with the other warning message above.

> +	ret = ath10k_conf_tx_uapsd(ar, vif, ac, params->uapsd);
> +	if (ret)
> +		ath10k_warn("could not set sta uapsd (%d)\n", ret);

Ditto.

-- 
Kalle Valo

[ath9k-devel] [PATCH 3/3] ath10k: enable STA UAPSD support

[Janusz.Dziedzic at tieto.com]

>-----Original Message-----
>From: Kalle Valo [mailto:kvalo at qca.qualcomm.com]
>Sent: 9 maja 2013 09:17
>To: Dziedzic Janusz
>Cc: ath9k-devel at lists.ath9k.org
>Subject: Re: [ath9k-devel] [PATCH 3/3] ath10k: enable STA UAPSD support
>
>Janusz Dziedzic <janusz.dziedzic@tieto.com> writes:
>
>> Enable UAPSD support for STA mode.
>>
>> Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>
>
>Same here, please explain briefly what you have tested.
>
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -2296,6 +2296,68 @@ static int ath10k_sta_state(struct ieee80211_hw
>*hw,
>>  	return ret;
>>  }
>>
>> +#define SET_UAPSD(enable, out, flags) do {	\
>> +	if ((enable))				\
>> +		(out) |= (flags);		\
>> +	else					\
>> +		(out) &= ~(flags);		\
>> +} while (0)
>
>Why the macro? To workaround a long line warning from checkpatch?
>
>If that's a problem we could increate line length limit, for example to
>85 or 90. In some cases the 80 char limit is a bit too excessive. Would that
>help?

Not really. This is because we have to set/clean this flags for every ACs.
So, this is just to prevent code duplication.

        case IEEE80211_AC_VO:
               if (enable)
                        ...
               else
                      ....
               break;
       case IEEE80211_AC_VI:
               if (enable)
                      ....
                else
                     ....
               break;
        .....

I can remove this macro and create inline function instead.

BR
Janusz

[ath9k-devel] [PATCH 3/3] ath10k: enable STA UAPSD support

[Michal Kazior]

On 09/05/13 09:47, Janusz.Dziedzic at tieto.com wrote:
>> -----Original Message-----
>> From: Kalle Valo [mailto:kvalo at qca.qualcomm.com]
>> Sent: 9 maja 2013 09:17
>> To: Dziedzic Janusz
>> Cc: ath9k-devel at lists.ath9k.org
>> Subject: Re: [ath9k-devel] [PATCH 3/3] ath10k: enable STA UAPSD support
>>
>> Janusz Dziedzic <janusz.dziedzic@tieto.com> writes:
>>
>>> Enable UAPSD support for STA mode.
>>>
>>> Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>
>>
>> Same here, please explain briefly what you have tested.
>>
>>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>>> @@ -2296,6 +2296,68 @@ static int ath10k_sta_state(struct ieee80211_hw
>> *hw,
>>>   	return ret;
>>>   }
>>>
>>> +#define SET_UAPSD(enable, out, flags) do {	\
>>> +	if ((enable))				\
>>> +		(out) |= (flags);		\
>>> +	else					\
>>> +		(out) &= ~(flags);		\
>>> +} while (0)
>>
>> Why the macro? To workaround a long line warning from checkpatch?
>>
>> If that's a problem we could increate line length limit, for example to
>> 85 or 90. In some cases the 80 char limit is a bit too excessive. Would that
>> help?
>
> Not really. This is because we have to set/clean this flags for every ACs.
> So, this is just to prevent code duplication.
>
>          case IEEE80211_AC_VO:
>                 if (enable)
>                          ...
>                 else
>                        ....
>                 break;
>         case IEEE80211_AC_VI:
>                 if (enable)
>                        ....
>                  else
>                       ....
>                 break;
>          .....
>
> I can remove this macro and create inline function instead.

Hmm, but since this is per-ac can't we do the following?

switch (ac) {
case IEEE80211_AC_VO:
	val = WMI_STA_PS_UAPSD_AC3_DELIVERY_EN |
               WMI_STA_PS_UAPSD_AC3_TRIGGER_EN;
	break;
// ..
}

if (enable)
	arvif->u.sta.uapsd |= val;
else
	arvif->u.sta.uapsd &= ~val;


No macros and no code duplication.


-- Pozdrawiam / Best regards, Michal Kazior.

[ath9k-devel] [PATCH 3/3] ath10k: enable STA UAPSD support

[Kalle Valo]

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

>> Not really. This is because we have to set/clean this flags for every ACs.
>> So, this is just to prevent code duplication.
>>
>>          case IEEE80211_AC_VO:
>>                 if (enable)
>>                          ...
>>                 else
>>                        ....
>>                 break;
>>         case IEEE80211_AC_VI:
>>                 if (enable)
>>                        ....
>>                  else
>>                       ....
>>                 break;
>>          .....
>>
>> I can remove this macro and create inline function instead.
>
> Hmm, but since this is per-ac can't we do the following?
>
> switch (ac) {
> case IEEE80211_AC_VO:
> 	val = WMI_STA_PS_UAPSD_AC3_DELIVERY_EN |
>               WMI_STA_PS_UAPSD_AC3_TRIGGER_EN;
> 	break;
> // ..
> }
>
> if (enable)
> 	arvif->u.sta.uapsd |= val;
> else
> 	arvif->u.sta.uapsd &= ~val;
>
>
> No macros and no code duplication.

Yes, much better. I hate macros :)

-- 
Kalle Valo

[ath9k-devel] [PATCH 0/3] UAPSD support for STA/AP mode

[Janusz.Dziedzic at tieto.com]

>-----Original Message-----
>From: Dziedzic Janusz
>Sent: 8 maja 2013 09:49
>To: ath9k-devel at lists.ath9k.org
>Cc: Dziedzic Janusz
>Subject: [PATCH 0/3] UAPSD support for STA/AP mode
>
>First two paches add PS/UAPSD support for AP/P2P_GO mode.
>Last one enable UAPSD for STA mode.
>
>In case of FW issues we can easy disable this features by cleaning:
>IEEE80211_HW_SUPPORTS_UAPSD/WIPHY_FLAG_AP_UAPSD still have code
>included.
>
>
>Janusz Dziedzic (3):
>  ath10k: WMI add AP PS
>  ath10k: add AP UAPSD support
>  ath10k: enable STA UAPSD support
>
> drivers/net/wireless/ath/ath10k/core.h |    1 +
> drivers/net/wireless/ath/ath10k/mac.c  |  165
>+++++++++++++++++++++++++++++---
> drivers/net/wireless/ath/ath10k/wmi.c  |   26 +++++
> drivers/net/wireless/ath/ath10k/wmi.h  |   68 +++++++++++++
> 4 files changed, 249 insertions(+), 11 deletions(-)
>

Send V2.

BR
Janusz