LinuxLists.cc - [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[permalink] [raw]

Subject: [PATCHv2 2/2] ath11k: Add ap power save support

AP power save where AP goes to power save mode when no stations associate
to it and come out of power save when any station associate to AP.

This AP power save capability can be used to save power with the drawback
of reduced range or delayed discovery of the AP

This patch also porvides user configuration to enable/disable
this feature using vendor command. This feature is disabled by default.

Tested-on: IPQ8074 WLAN.HK.2.1.0.1-01228-QCAHKSWPL_SILICONZ-1

Signed-off-by: Venkateswara Naralasetty <[email protected]>
---
drivers/net/wireless/ath/ath11k/Makefile | 3 +-
drivers/net/wireless/ath/ath11k/core.h | 8 ++++
drivers/net/wireless/ath/ath11k/mac.c | 44 +++++++++++++++++
drivers/net/wireless/ath/ath11k/mac.h | 1 +
drivers/net/wireless/ath/ath11k/vendor.c | 81 ++++++++++++++++++++++++++++++++
drivers/net/wireless/ath/ath11k/vendor.h | 12 +++++
drivers/net/wireless/ath/ath11k/wmi.c | 32 +++++++++++++
drivers/net/wireless/ath/ath11k/wmi.h | 7 +++
8 files changed, 187 insertions(+), 1 deletion(-)
create mode 100644 drivers/net/wireless/ath/ath11k/vendor.c
create mode 100644 drivers/net/wireless/ath/ath11k/vendor.h

diff --git a/drivers/net/wireless/ath/ath11k/Makefile b/drivers/net/wireless/ath/ath11k/Makefile
index bc4911f..f0b4975 100644
--- a/drivers/net/wireless/ath/ath11k/Makefile
+++ b/drivers/net/wireless/ath/ath11k/Makefile
@@ -16,7 +16,8 @@ ath11k-y += core.o \
ce.o \
peer.o \
dbring.o \
- hw.o
+ hw.o \
+ vendor.o

ath11k-$(CONFIG_ATH11K_DEBUGFS) += debug_htt_stats.o debugfs_sta.o
ath11k-$(CONFIG_NL80211_TESTMODE) += testmode.o
diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h
index d21191c..827020f 100644
--- a/drivers/net/wireless/ath/ath11k/core.h
+++ b/drivers/net/wireless/ath/ath11k/core.h
@@ -23,6 +23,7 @@
#include "thermal.h"
#include "dbring.h"
#include "spectral.h"
+#include "vendor.h"

#define SM(_v, _f) (((_v) << _f##_LSB) & _f##_MASK)

@@ -419,6 +420,11 @@ struct ath11k_vdev_stop_status {
u32 vdev_id;
};

+enum ath11k_ap_ps_state {
+ ATH11K_AP_PS_STATE_OFF,
+ ATH11K_AP_PS_STATE_ON,
+};
+
struct ath11k {
struct ath11k_base *ab;
struct ath11k_pdev *pdev;
@@ -543,6 +549,8 @@ struct ath11k {
#endif
bool dfs_block_radar_events;
struct ath11k_thermal thermal;
+ int ap_ps_enabled;
+ enum ath11k_ap_ps_state ap_ps_state;
};

struct ath11k_band_cap {
diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
index f4a085b..1885834 100644
--- a/drivers/net/wireless/ath/ath11k/mac.c
+++ b/drivers/net/wireless/ath/ath11k/mac.c
@@ -2898,6 +2898,33 @@ static void ath11k_mac_dec_num_stations(struct ath11k_vif *arvif,
ar->num_stations--;
}

+int ath11k_mac_ap_ps_recalc(struct ath11k *ar)
+{
+ struct ath11k_vif *arvif;
+ bool has_sta_iface = false;
+ enum ath11k_ap_ps_state state = ATH11K_AP_PS_STATE_OFF;
+ int ret = 0;
+
+ list_for_each_entry(arvif, &ar->arvifs, list) {
+ if (arvif->vdev_type == WMI_VDEV_TYPE_STA) {
+ has_sta_iface = true;
+ break;
+ }
+ }
+
+ if (!has_sta_iface && !ar->num_stations && ar->ap_ps_enabled)
+ state = ATH11K_AP_PS_STATE_ON;
+
+ if (ar->ap_ps_state == state)
+ return ret;
+
+ ret = ath11k_wmi_pdev_ap_ps_cmd_send(ar, ar->pdev->pdev_id, state);
+ if (!ret)
+ ar->ap_ps_state = state;
+
+ return ret;
+}
+
static int ath11k_mac_station_add(struct ath11k *ar,
struct ieee80211_vif *vif,
struct ieee80211_sta *sta)
@@ -2937,6 +2964,12 @@ static int ath11k_mac_station_add(struct ath11k *ar,
ath11k_dbg(ab, ATH11K_DBG_MAC, "Added peer: %pM for VDEV: %d\n",
sta->addr, arvif->vdev_id);

+ ret = ath11k_mac_ap_ps_recalc(ar);
+ if (ret) {
+ ath11k_warn(ar->ab, "failed to send ap ps ret %d\n", ret);
+ goto exit;
+ }
+
if (ath11k_debug_is_extd_tx_stats_enabled(ar)) {
arsta->tx_stats = kzalloc(sizeof(*arsta->tx_stats), GFP_KERNEL);
if (!arsta->tx_stats) {
@@ -3046,6 +3079,10 @@ static int ath11k_mac_op_sta_state(struct ieee80211_hw *hw,

kfree(arsta->rx_stats);
arsta->rx_stats = NULL;
+
+ ret = ath11k_mac_ap_ps_recalc(ar);
+ if (ret)
+ ath11k_warn(ar->ab, "failed to send ap ps ret %d\n", ret);
} else if (old_state == IEEE80211_STA_AUTH &&
new_state == IEEE80211_STA_ASSOC &&
(vif->type == NL80211_IFTYPE_AP ||
@@ -4210,6 +4247,7 @@ static void ath11k_mac_op_stop(struct ieee80211_hw *hw)

clear_bit(ATH11K_CAC_RUNNING, &ar->dev_flags);
ar->state = ATH11K_STATE_OFF;
+ ar->ap_ps_state = ATH11K_AP_PS_STATE_OFF;
mutex_unlock(&ar->conf_mutex);

cancel_delayed_work_sync(&ar->scan.timeout);
@@ -4533,6 +4571,10 @@ static int ath11k_mac_op_add_interface(struct ieee80211_hw *hw,

ath11k_dp_vdev_tx_attach(ar, arvif);

+ ret = ath11k_mac_ap_ps_recalc(ar);
+ if (ret)
+ ath11k_warn(ar->ab, "failed to set ap ps ret %d\n", ret);
+
mutex_unlock(&ar->conf_mutex);

return 0;
@@ -4619,6 +4661,7 @@ static void ath11k_mac_op_remove_interface(struct ieee80211_hw *hw,

/* Recalc txpower for remaining vdev */
ath11k_mac_txpower_recalc(ar);
+ ath11k_mac_ap_ps_recalc(ar);
clear_bit(ATH11K_FLAG_MONITOR_ENABLED, &ar->monitor_flags);

/* TODO: recal traffic pause state based on the available vdevs */
@@ -6175,6 +6218,7 @@ static int __ath11k_mac_register(struct ath11k *ar)
ARRAY_SIZE(ath11k_iftypes_ext_capa);

ath11k_reg_init(ar);
+ ath11k_vendor_register(ar);

/* advertise HW checksum offload capabilities */
ar->hw->netdev_features = NETIF_F_HW_CSUM;
diff --git a/drivers/net/wireless/ath/ath11k/mac.h b/drivers/net/wireless/ath/ath11k/mac.h
index 0607479..18d2f28 100644
--- a/drivers/net/wireless/ath/ath11k/mac.h
+++ b/drivers/net/wireless/ath/ath11k/mac.h
@@ -146,4 +146,5 @@ int ath11k_mac_tx_mgmt_pending_free(int buf_id, void *skb, void *ctx);
u8 ath11k_mac_bw_to_mac80211_bw(u8 bw);
enum ath11k_supported_bw ath11k_mac_mac80211_bw_to_ath11k_bw(enum rate_info_bw bw);
enum hal_encrypt_type ath11k_dp_tx_get_encrypt_type(u32 cipher);
+int ath11k_mac_ap_ps_recalc(struct ath11k *ar);
#endif
diff --git a/drivers/net/wireless/ath/ath11k/vendor.c b/drivers/net/wireless/ath/ath11k/vendor.c
new file mode 100644
index 0000000..b28cc65
--- /dev/null
+++ b/drivers/net/wireless/ath/ath11k/vendor.c
@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: BSD-3-Clause-Clear
+/*
+ * Copyright (c) 2019-2020 The Linux Foundation. All rights reserved.
+ */
+
+#include <net/netlink.h>
+#include <net/mac80211.h>
+#include <uapi/nl80211-vnd-qca.h>
+#include "core.h"
+#include "debug.h"
+
+static const struct nla_policy
+ath11k_vendor_set_wifi_config_policy[QCA_WLAN_VENDOR_ATTR_CONFIG_MAX + 1] = {
+ [QCA_WLAN_VENDOR_ATTR_CONFIG_GTX] = {.type = NLA_FLAG}
+};
+
+static int ath11k_vendor_set_wifi_config(struct wiphy *wihpy,
+ struct wireless_dev *wdev,
+ const void *data,
+ int data_len)
+{
+ struct ieee80211_vif *vif;
+ struct ath11k_vif *arvif;
+ struct ath11k *ar;
+ struct nlattr *tb[QCA_WLAN_VENDOR_ATTR_CONFIG_MAX + 1];
+ int ret = 0;
+
+ if (!wdev)
+ return -EINVAL;
+
+ vif = wdev_to_ieee80211_vif(wdev);
+ if (!vif)
+ return -EINVAL;
+
+ arvif = (struct ath11k_vif *)vif->drv_priv;
+ if (!arvif)
+ return -EINVAL;
+
+ ar = arvif->ar;
+
+ mutex_lock(&ar->conf_mutex);
+
+ ret = nla_parse(tb, QCA_WLAN_VENDOR_ATTR_CONFIG_MAX, data, data_len,
+ ath11k_vendor_set_wifi_config_policy, NULL);
+ if (ret) {
+ ath11k_warn(ar->ab, "invalid set wifi config policy attribute\n");
+ goto exit;
+ }
+
+ ar->ap_ps_enabled = nla_get_flag(tb[QCA_WLAN_VENDOR_ATTR_CONFIG_GTX]);
+ ret = ath11k_mac_ap_ps_recalc(ar);
+ if (ret) {
+ ath11k_warn(ar->ab, "failed to send ap ps ret %d\n", ret);
+ goto exit;
+ }
+
+exit:
+ mutex_unlock(&ar->conf_mutex);
+ return ret;
+}
+
+static struct wiphy_vendor_command ath11k_vendor_commands[] = {
+ {
+ .info.vendor_id = QCA_NL80211_VENDOR_ID,
+ .info.subcmd = QCA_NL80211_VENDOR_SUBCMD_SET_WIFI_CONFIGURATION,
+ .flags = WIPHY_VENDOR_CMD_NEED_WDEV |
+ WIPHY_VENDOR_CMD_NEED_RUNNING,
+ .doit = ath11k_vendor_set_wifi_config,
+ .policy = ath11k_vendor_set_wifi_config_policy,
+ .maxattr = QCA_WLAN_VENDOR_ATTR_CONFIG_MAX
+ }
+};
+
+int ath11k_vendor_register(struct ath11k *ar)
+{
+ ar->hw->wiphy->vendor_commands = ath11k_vendor_commands;
+ ar->hw->wiphy->n_vendor_commands = ARRAY_SIZE(ath11k_vendor_commands);
+
+ return 0;
+}
+
diff --git a/drivers/net/wireless/ath/ath11k/vendor.h b/drivers/net/wireless/ath/ath11k/vendor.h
new file mode 100644
index 0000000..6eaf07e
--- /dev/null
+++ b/drivers/net/wireless/ath/ath11k/vendor.h
@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: BSD-3-Clause-Clear
+/*
+ * Copyright (c) 2019-2020 The Linux Foundation. All rights reserved.
+ */
+
+#ifndef ATH11K_VENDOR_H
+#define ATH11K_VENDOR_H
+
+int ath11k_vendor_register(struct ath11k *ar);
+
+#endif /* QCA_VENDOR_H */
+
diff --git a/drivers/net/wireless/ath/ath11k/wmi.c b/drivers/net/wireless/ath/ath11k/wmi.c
index a66576f..9aa1718 100644
--- a/drivers/net/wireless/ath/ath11k/wmi.c
+++ b/drivers/net/wireless/ath/ath11k/wmi.c
@@ -1188,6 +1188,38 @@ ath11k_wmi_rx_reord_queue_remove(struct ath11k *ar,
return ret;
}

+int ath11k_wmi_pdev_ap_ps_cmd_send(struct ath11k *ar, u8 pdev_id,
+ u32 param_value)
+{
+ struct ath11k_pdev_wmi *wmi = ar->wmi;
+ struct wmi_pdev_ap_ps_cmd *cmd;
+ struct sk_buff *skb;
+ int ret;
+
+ skb = ath11k_wmi_alloc_skb(wmi->wmi_ab, sizeof(*cmd));
+ if (!skb)
+ return -ENOMEM;
+
+ cmd = (struct wmi_pdev_ap_ps_cmd *)skb->data;
+ cmd->tlv_header = FIELD_PREP(WMI_TLV_TAG,
+ WMI_TAG_PDEV_GREEN_AP_PS_ENABLE_CMD) |
+ FIELD_PREP(WMI_TLV_LEN, sizeof(*cmd) - TLV_HDR_SIZE);
+ cmd->pdev_id = pdev_id;
+ cmd->param_value = param_value;
+
+ ret = ath11k_wmi_cmd_send(wmi, skb, WMI_PDEV_GREEN_AP_PS_ENABLE_CMDID);
+ if (ret) {
+ ath11k_warn(ar->ab, "failed to send ap ps enable/disable cmd\n");
+ dev_kfree_skb(skb);
+ }
+
+ ath11k_dbg(ar->ab, ATH11K_DBG_WMI,
+ "wmi pdev ap ps set pdev id %d value %d\n",
+ pdev_id, param_value);
+
+ return ret;
+}
+
int ath11k_wmi_pdev_set_param(struct ath11k *ar, u32 param_id,
u32 param_value, u8 pdev_id)
{
diff --git a/drivers/net/wireless/ath/ath11k/wmi.h b/drivers/net/wireless/ath/ath11k/wmi.h
index 5a32ba0..e3a5751 100644
--- a/drivers/net/wireless/ath/ath11k/wmi.h
+++ b/drivers/net/wireless/ath/ath11k/wmi.h
@@ -2901,6 +2901,12 @@ struct set_fwtest_params {
u32 value;
};

+struct wmi_pdev_ap_ps_cmd {
+ u32 tlv_header;
+ u32 pdev_id;
+ u32 param_value;
+} __packed;
+
struct wmi_fwtest_set_param_cmd_param {
u32 tlv_header;
u32 param_id;
@@ -5121,4 +5127,5 @@ int ath11k_wmi_vdev_spectral_enable(struct ath11k *ar, u32 vdev_id,
u32 trigger, u32 enable);
int ath11k_wmi_vdev_spectral_conf(struct ath11k *ar,
struct ath11k_wmi_vdev_spectral_conf_param *param);
+int ath11k_wmi_pdev_ap_ps_cmd_send(struct ath11k *ar, u8 pdev_id, u32 value);
#endif
--
2.7.4

2020-09-28 11:25:03

[permalink] [raw]

Subject: Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
> AP power save feature is to save power in AP mode, where AP goes
> to power save mode when no stations associate to it and comes out
> of power save when any station associate to AP.

Why do you think this requires a vendor command? I mean, that seems like
fairly reasonable - even by default - behaviour? And if not done by
default, it could possibly even be configured via the normal powersave
mode/command we already have in nl80211, or by a new normal one?

Why does it even require an nl80211 setting, seems like something you'd
really only want to turn off for debugging (e.g. via debugfs)?

johannes

2020-09-29 07:44:04

[permalink] [raw]

Subject: Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

Johannes Berg <[email protected]> writes:

> On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>> AP power save feature is to save power in AP mode, where AP goes
>> to power save mode when no stations associate to it and comes out
>> of power save when any station associate to AP.
>
> Why do you think this requires a vendor command? I mean, that seems like
> fairly reasonable - even by default - behaviour?

I have not studied the details, but doesn't AP power save break normal
functionality? For example, I would guess probe requests from clients
would be lost. So there's a major drawback when enabling this, right?

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2020-09-29 08:05:59

[permalink] [raw]

Subject: Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

On Tue, 2020-09-29 at 10:40 +0300, Kalle Valo wrote:
> Johannes Berg <[email protected]> writes:
>
> > On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
> > > AP power save feature is to save power in AP mode, where AP goes
> > > to power save mode when no stations associate to it and comes out
> > > of power save when any station associate to AP.
> >
> > Why do you think this requires a vendor command? I mean, that seems like
> > fairly reasonable - even by default - behaviour?
>
> I have not studied the details, but doesn't AP power save break normal
> functionality? For example, I would guess probe requests from clients
> would be lost. So there's a major drawback when enabling this, right?

Not the way it's described above?

johannes

2020-09-29 12:40:54

[permalink] [raw]

Subject: Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

On 2020-09-29 13:10, Kalle Valo wrote:
> Johannes Berg <[email protected]> writes:
>
>> On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>>> AP power save feature is to save power in AP mode, where AP goes
>>> to power save mode when no stations associate to it and comes out
>>> of power save when any station associate to AP.
>>
>> Why do you think this requires a vendor command? I mean, that seems
>> like
>> fairly reasonable - even by default - behaviour?
>
> I have not studied the details, but doesn't AP power save break normal
> functionality? For example, I would guess probe requests from clients
> would be lost. So there's a major drawback when enabling this, right?

This AP power save feature will not break any functionality, Since one
chain is always active and all other chains will be disabled when this
feature is enabled. AP can still be able to beacon and receive probe
request from the clients. The only drawback is reduced network range
when this feature is enabled. Hence, we don't want to enable it by
default.

2020-10-22 06:12:34

[permalink] [raw]

Subject: Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[email protected] writes:

> On 2020-09-29 13:10, Kalle Valo wrote:
>> Johannes Berg <[email protected]> writes:
>>
>>> On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>>>> AP power save feature is to save power in AP mode, where AP goes
>>>> to power save mode when no stations associate to it and comes out
>>>> of power save when any station associate to AP.
>>>
>>> Why do you think this requires a vendor command? I mean, that seems
>>> like
>>> fairly reasonable - even by default - behaviour?
>>
>> I have not studied the details, but doesn't AP power save break normal
>> functionality? For example, I would guess probe requests from clients
>> would be lost. So there's a major drawback when enabling this, right?
>
> This AP power save feature will not break any functionality, Since one
> chain is always active and all other chains will be disabled when this
> feature is enabled. AP can still be able to beacon and receive probe
> request from the clients. The only drawback is reduced network range
> when this feature is enabled. Hence, we don't want to enable it by
> default.

Yeah, we really would not want to enable that by default. But what
should be the path forward, a vendor command or a proper nl80211
command? Any opinions?

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2020-10-22 09:01:38

by Arend Van Spriel

[permalink] [raw]

Subject: Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

On 10/21/2020 7:19 PM, Kalle Valo wrote:
> [email protected] writes:
>
>> On 2020-09-29 13:10, Kalle Valo wrote:
>>> Johannes Berg <[email protected]> writes:
>>>
>>>> On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>>>>> AP power save feature is to save power in AP mode, where AP goes
>>>>> to power save mode when no stations associate to it and comes out
>>>>> of power save when any station associate to AP.
>>>>
>>>> Why do you think this requires a vendor command? I mean, that seems
>>>> like
>>>> fairly reasonable - even by default - behaviour?
>>>
>>> I have not studied the details, but doesn't AP power save break normal
>>> functionality? For example, I would guess probe requests from clients
>>> would be lost. So there's a major drawback when enabling this, right?
>>
>> This AP power save feature will not break any functionality, Since one
>> chain is always active and all other chains will be disabled when this
>> feature is enabled. AP can still be able to beacon and receive probe
>> request from the clients. The only drawback is reduced network range
>> when this feature is enabled. Hence, we don't want to enable it by
>> default.
>
> Yeah, we really would not want to enable that by default. But what
> should be the path forward, a vendor command or a proper nl80211
> command? Any opinions?

I would go for a proper nl80211 command or just add an attribute for use
in NL80211_CMD_START_AP or deal with NL80211_CMD_SET_POWERSAVE when
operating in AP mode.

Regards,
Arend

Attachments:

smime.p7s (4.08 kB)
S/MIME Cryptographic Signature

2020-10-22 09:01:50

by Arend Van Spriel

[permalink] [raw]

Subject: Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

On 10/22/2020 10:00 AM, Arend Van Spriel wrote:
> On 10/21/2020 7:19 PM, Kalle Valo wrote:
>> [email protected] writes:
>>
>>> On 2020-09-29 13:10, Kalle Valo wrote:
>>>> Johannes Berg <[email protected]> writes:
>>>>
>>>>> On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>>>>>> AP power save feature is to save power in AP mode, where AP goes
>>>>>> to power save mode when no stations associate to it and comes out
>>>>>> of power save when any station associate to AP.
>>>>>
>>>>> Why do you think this requires a vendor command? I mean, that seems
>>>>> like
>>>>> fairly reasonable - even by default - behaviour?
>>>>
>>>> I have not studied the details, but doesn't AP power save break normal
>>>> functionality? For example, I would guess probe requests from clients
>>>> would be lost. So there's a major drawback when enabling this, right?
>>>
>>> This AP power save feature will not break any functionality, Since one
>>> chain is always active and all other chains will be disabled when this
>>> feature is enabled. AP can still be able to beacon and receive probe
>>> request from the clients. The only drawback is reduced network range
>>> when this feature is enabled. Hence, we don't want to enable it by
>>> default.
>>
>> Yeah, we really would not want to enable that by default. But what
>> should be the path forward, a vendor command or a proper nl80211
>> command? Any opinions?
>
> I would go for a proper nl80211 command or just add an attribute for use
> in NL80211_CMD_START_AP or deal with NL80211_CMD_SET_POWERSAVE when
> operating in AP mode.

At first I though this functionality was same as SMPS which is in the
802.11 spec, but reading up on it that seems to be a STA feature.

Regards,
Arend

Attachments:

smime.p7s (4.08 kB)
S/MIME Cryptographic Signature

2020-10-23 07:55:21

[permalink] [raw]

Subject: Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

On 2020-10-22 13:30, Arend Van Spriel wrote:
> On 10/21/2020 7:19 PM, Kalle Valo wrote:
>> [email protected] writes:
>>
>>> On 2020-09-29 13:10, Kalle Valo wrote:
>>>> Johannes Berg <[email protected]> writes:
>>>>
>>>>> On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>>>>>> AP power save feature is to save power in AP mode, where AP goes
>>>>>> to power save mode when no stations associate to it and comes out
>>>>>> of power save when any station associate to AP.
>>>>>
>>>>> Why do you think this requires a vendor command? I mean, that seems
>>>>> like
>>>>> fairly reasonable - even by default - behaviour?
>>>>
>>>> I have not studied the details, but doesn't AP power save break
>>>> normal
>>>> functionality? For example, I would guess probe requests from
>>>> clients
>>>> would be lost. So there's a major drawback when enabling this,
>>>> right?
>>>
>>> This AP power save feature will not break any functionality, Since
>>> one
>>> chain is always active and all other chains will be disabled when
>>> this
>>> feature is enabled. AP can still be able to beacon and receive probe
>>> request from the clients. The only drawback is reduced network range
>>> when this feature is enabled. Hence, we don't want to enable it by
>>> default.
>>
>> Yeah, we really would not want to enable that by default. But what
>> should be the path forward, a vendor command or a proper nl80211
>> command? Any opinions?
>
> I would go for a proper nl80211 command or just add an attribute for
> use in NL80211_CMD_START_AP or deal with NL80211_CMD_SET_POWERSAVE
> when operating in AP mode.
>
Sure, I will go with the existing NL80211_CMD_SET_POWERSAVE and I will
send next version.

> Regards,
> Arend

2020-11-02 19:44:57

[permalink] [raw]

Subject: Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[email protected] writes:

> On 2020-10-22 13:30, Arend Van Spriel wrote:
>> On 10/21/2020 7:19 PM, Kalle Valo wrote:
>>> [email protected] writes:
>>>
>>>> On 2020-09-29 13:10, Kalle Valo wrote:
>>>>> Johannes Berg <[email protected]> writes:
>>>>>
>>>>>> On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>>>>>>> AP power save feature is to save power in AP mode, where AP goes
>>>>>>> to power save mode when no stations associate to it and comes out
>>>>>>> of power save when any station associate to AP.
>>>>>>
>>>>>> Why do you think this requires a vendor command? I mean, that seems
>>>>>> like
>>>>>> fairly reasonable - even by default - behaviour?
>>>>>
>>>>> I have not studied the details, but doesn't AP power save break
>>>>> normal
>>>>> functionality? For example, I would guess probe requests from
>>>>> clients
>>>>> would be lost. So there's a major drawback when enabling this,
>>>>> right?
>>>>
>>>> This AP power save feature will not break any functionality, Since
>>>> one
>>>> chain is always active and all other chains will be disabled when
>>>> this
>>>> feature is enabled. AP can still be able to beacon and receive probe
>>>> request from the clients. The only drawback is reduced network range
>>>> when this feature is enabled. Hence, we don't want to enable it by
>>>> default.
>>>
>>> Yeah, we really would not want to enable that by default. But what
>>> should be the path forward, a vendor command or a proper nl80211
>>> command? Any opinions?
>>
>> I would go for a proper nl80211 command or just add an attribute for
>> use in NL80211_CMD_START_AP or deal with NL80211_CMD_SET_POWERSAVE
>> when operating in AP mode.
>>
> Sure, I will go with the existing NL80211_CMD_SET_POWERSAVE and I will
> send next version.

Better to wait first so that we have concensus on this. And need to
check if NL80211_CMD_SET_POWERSAVE is even suitable for AP mode.

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2020-11-06 10:43:22

[permalink] [raw]

Subject: Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

On Mon, 2020-11-02 at 21:44 +0200, Kalle Valo wrote:
> [email protected] writes:
>
> > On 2020-10-22 13:30, Arend Van Spriel wrote:
> > > On 10/21/2020 7:19 PM, Kalle Valo wrote:
> > > > [email protected] writes:
> > > >
> > > > > On 2020-09-29 13:10, Kalle Valo wrote:
> > > > > > Johannes Berg <[email protected]> writes:
> > > > > >
> > > > > > > On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
> > > > > > > > AP power save feature is to save power in AP mode, where AP goes
> > > > > > > > to power save mode when no stations associate to it and comes out
> > > > > > > > of power save when any station associate to AP.
> > > > > > >
> > > > > > > Why do you think this requires a vendor command? I mean, that seems
> > > > > > > like
> > > > > > > fairly reasonable - even by default - behaviour?
> > > > > >
> > > > > > I have not studied the details, but doesn't AP power save break
> > > > > > normal
> > > > > > functionality? For example, I would guess probe requests from
> > > > > > clients
> > > > > > would be lost. So there's a major drawback when enabling this,
> > > > > > right?
> > > > >
> > > > > This AP power save feature will not break any functionality, Since
> > > > > one
> > > > > chain is always active and all other chains will be disabled when
> > > > > this
> > > > > feature is enabled. AP can still be able to beacon and receive probe
> > > > > request from the clients. The only drawback is reduced network range
> > > > > when this feature is enabled. Hence, we don't want to enable it by
> > > > > default.
> > > >
> > > > Yeah, we really would not want to enable that by default. But what
> > > > should be the path forward, a vendor command or a proper nl80211
> > > > command? Any opinions?
> > >
> > > I would go for a proper nl80211 command or just add an attribute for
> > > use in NL80211_CMD_START_AP or deal with NL80211_CMD_SET_POWERSAVE
> > > when operating in AP mode.
> > >
> > Sure, I will go with the existing NL80211_CMD_SET_POWERSAVE and I will
> > send next version.
>
> Better to wait first so that we have concensus on this. And need to
> check if NL80211_CMD_SET_POWERSAVE is even suitable for AP mode.

I suspect that SET_POWERSAVE might be confusing.

Perhaps just with an attribute used in START_AP (and CHANGE_BEACON if
needed) would be sufficient?

johannes

2020-11-17 11:26:28

[permalink] [raw]

Subject: Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

On 2020-11-06 16:11, Johannes Berg wrote:
> On Mon, 2020-11-02 at 21:44 +0200, Kalle Valo wrote:
>> [email protected] writes:
>>
>> > On 2020-10-22 13:30, Arend Van Spriel wrote:
>> > > On 10/21/2020 7:19 PM, Kalle Valo wrote:
>> > > > [email protected] writes:
>> > > >
>> > > > > On 2020-09-29 13:10, Kalle Valo wrote:
>> > > > > > Johannes Berg <[email protected]> writes:
>> > > > > >
>> > > > > > > On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>> > > > > > > > AP power save feature is to save power in AP mode, where AP goes
>> > > > > > > > to power save mode when no stations associate to it and comes out
>> > > > > > > > of power save when any station associate to AP.
>> > > > > > >
>> > > > > > > Why do you think this requires a vendor command? I mean, that seems
>> > > > > > > like
>> > > > > > > fairly reasonable - even by default - behaviour?
>> > > > > >
>> > > > > > I have not studied the details, but doesn't AP power save break
>> > > > > > normal
>> > > > > > functionality? For example, I would guess probe requests from
>> > > > > > clients
>> > > > > > would be lost. So there's a major drawback when enabling this,
>> > > > > > right?
>> > > > >
>> > > > > This AP power save feature will not break any functionality, Since
>> > > > > one
>> > > > > chain is always active and all other chains will be disabled when
>> > > > > this
>> > > > > feature is enabled. AP can still be able to beacon and receive probe
>> > > > > request from the clients. The only drawback is reduced network range
>> > > > > when this feature is enabled. Hence, we don't want to enable it by
>> > > > > default.
>> > > >
>> > > > Yeah, we really would not want to enable that by default. But what
>> > > > should be the path forward, a vendor command or a proper nl80211
>> > > > command? Any opinions?
>> > >
>> > > I would go for a proper nl80211 command or just add an attribute for
>> > > use in NL80211_CMD_START_AP or deal with NL80211_CMD_SET_POWERSAVE
>> > > when operating in AP mode.
>> > >
>> > Sure, I will go with the existing NL80211_CMD_SET_POWERSAVE and I will
>> > send next version.
>>
>> Better to wait first so that we have concensus on this. And need to
>> check if NL80211_CMD_SET_POWERSAVE is even suitable for AP mode.
>
> I suspect that SET_POWERSAVE might be confusing.
>
> Perhaps just with an attribute used in START_AP (and CHANGE_BEACON if
> needed) would be sufficient?

Actually this ap power save configuration is applicable for per pdev.
So, I don't think we can use START AP command here.
I would prefer to go with a new NL80211 command. Please share your
thoughts on this.

>
> johannes

2020-12-23 12:51:24

by Jouni Malinen

[permalink] [raw]

Subject: Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

On Fri, Nov 06, 2020 at 11:41:06AM +0100, Johannes Berg wrote:
> I suspect that SET_POWERSAVE might be confusing.

Why? Isn't the use case here very similar to the existing station mode
use of power save even if the power saving mechanism is more of a vendor
specific extension that applies while there are no associated stations?

> Perhaps just with an attribute used in START_AP (and CHANGE_BEACON if
> needed) would be sufficient?

NL80211_CMD_START_AP with a new attribute (or even re-use of
NL80211_ATTR_PS_STATE) might work for a case where this does not need to
be changed dynamically during the lifetime of the BSS.
NL80211_CMD_SET_BEACON (which maps to the change_beacon() callback)
feels like something that is currently limited to Beacon data updates
with its use of struct cfg80211_beacon_data instead of struct
cfg80211_ap_settings..

That SET_BEACON name is still from the old NEW/SET/DEL_BEACON time.
Should that be renamed to NL80211_CMD_UPDATE_AP if we extend this to
changes that are not really targeting the Beacon frame payload itself?
And should the cfg80211_beacon_data argument be replaced with
cfg80211_ap_settings? It looks like we already have some struct
cfg80211_ap_settings values like inactivity_timeout and beacon_rate (and
maybe some HE parameters?) that one might want to update during the
lifetime of the BSS..

--
Jouni Malinen PGP id EFC895FA

2021-01-15 10:16:17

[permalink] [raw]

Subject: Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

On Wed, 2020-12-23 at 14:46 +0200, Jouni Malinen wrote:
> On Fri, Nov 06, 2020 at 11:41:06AM +0100, Johannes Berg wrote:
> > I suspect that SET_POWERSAVE might be confusing.
>
> Why? Isn't the use case here very similar to the existing station mode
> use of power save even if the power saving mechanism is more of a vendor
> specific extension that applies while there are no associated stations?

Yeah, true, fair point.

However, set-powersave is a bit of a legacy API with state in the
kernel, and sometimes restrictions on how/when you can set it etc. I'm
not sure it's a good idea for those reasons alone?

> > Perhaps just with an attribute used in START_AP (and CHANGE_BEACON if
> > needed) would be sufficient?
>
> NL80211_CMD_START_AP with a new attribute (or even re-use of
> NL80211_ATTR_PS_STATE) might work for a case where this does not need to
> be changed dynamically during the lifetime of the BSS.
> NL80211_CMD_SET_BEACON (which maps to the change_beacon() callback)
> feels like something that is currently limited to Beacon data updates
> with its use of struct cfg80211_beacon_data instead of struct
> cfg80211_ap_settings..
>
> That SET_BEACON name is still from the old NEW/SET/DEL_BEACON time.
> Should that be renamed to NL80211_CMD_UPDATE_AP if we extend this to
> changes that are not really targeting the Beacon frame payload itself?

I'd be surprised if we don't already have non-beacon state there ... but
it looks like only very little non-beacon state, namely the FTM
responder state.

Renaming seems reasonable, we've done it before with START_AP.

> And should the cfg80211_beacon_data argument be replaced with
> cfg80211_ap_settings? It looks like we already have some struct
> cfg80211_ap_settings values like inactivity_timeout and beacon_rate (and
> maybe some HE parameters?) that one might want to update during the
> lifetime of the BSS..

That also seems reasonable.

johannes

2021-01-27 18:05:01

[permalink] [raw]

Subject: Re: [PATCHv2 2/2] ath11k: Add ap power save support

Venkateswara Naralasetty <[email protected]> writes:

> AP power save where AP goes to power save mode when no stations associate
> to it and come out of power save when any station associate to AP.
>
> This AP power save capability can be used to save power with the drawback
> of reduced range or delayed discovery of the AP
>
> This patch also porvides user configuration to enable/disable
> this feature using vendor command. This feature is disabled by default.
>
> Tested-on: IPQ8074 WLAN.HK.2.1.0.1-01228-QCAHKSWPL_SILICONZ-1
>
> Signed-off-by: Venkateswara Naralasetty <[email protected]>

[...]

> +static int ath11k_vendor_set_wifi_config(struct wiphy *wihpy,

s/wihpy/wiphy/

> + struct wireless_dev *wdev,
> + const void *data,
> + int data_len)
> +{
> + struct ieee80211_vif *vif;
> + struct ath11k_vif *arvif;
> + struct ath11k *ar;
> + struct nlattr *tb[QCA_WLAN_VENDOR_ATTR_CONFIG_MAX + 1];
> + int ret = 0;
> +
> + if (!wdev)
> + return -EINVAL;
> +
> + vif = wdev_to_ieee80211_vif(wdev);
> + if (!vif)
> + return -EINVAL;
> +
> + arvif = (struct ath11k_vif *)vif->drv_priv;
> + if (!arvif)
> + return -EINVAL;
> +
> + ar = arvif->ar;
> +
> + mutex_lock(&ar->conf_mutex);
> +
> + ret = nla_parse(tb, QCA_WLAN_VENDOR_ATTR_CONFIG_MAX, data, data_len,
> + ath11k_vendor_set_wifi_config_policy, NULL);
> + if (ret) {
> + ath11k_warn(ar->ab, "invalid set wifi config policy attribute\n");
> + goto exit;
> + }
> +
> + ar->ap_ps_enabled = nla_get_flag(tb[QCA_WLAN_VENDOR_ATTR_CONFIG_GTX]);
> + ret = ath11k_mac_ap_ps_recalc(ar);
> + if (ret) {
> + ath11k_warn(ar->ab, "failed to send ap ps ret %d\n", ret);
> + goto exit;
> + }
> +
> +exit:
> + mutex_unlock(&ar->conf_mutex);
> + return ret;
> +}

Something which I find awkward here is that this is per pdev (=all
vdevs), even though the vendor command is per vif. So if you change the
config on one vif, all other vifs will change as well. And there's no
way to check if the state from user space as there's only a set command
and no equivalent get command.

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2021-01-27 18:17:44