| Message ID | 1598257589-19091-1-git-send-email-vnaralas@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [PATCHv2,1/2] nl80211: vendor-cmd: qca: add command for ap power save | expand |
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
Johannes Berg <johannes@sipsolutions.net> 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?
On Tue, 2020-09-29 at 10:40 +0300, Kalle Valo wrote: > Johannes Berg <johannes@sipsolutions.net> 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
On 2020-09-29 13:10, Kalle Valo wrote: > Johannes Berg <johannes@sipsolutions.net> 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.
vnaralas@codeaurora.org writes: > On 2020-09-29 13:10, Kalle Valo wrote: >> Johannes Berg <johannes@sipsolutions.net> 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?
On 10/21/2020 7:19 PM, Kalle Valo wrote: > vnaralas@codeaurora.org writes: > >> On 2020-09-29 13:10, Kalle Valo wrote: >>> Johannes Berg <johannes@sipsolutions.net> 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
On 10/22/2020 10:00 AM, Arend Van Spriel wrote: > On 10/21/2020 7:19 PM, Kalle Valo wrote: >> vnaralas@codeaurora.org writes: >> >>> On 2020-09-29 13:10, Kalle Valo wrote: >>>> Johannes Berg <johannes@sipsolutions.net> 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
On 2020-10-22 13:30, Arend Van Spriel wrote: > On 10/21/2020 7:19 PM, Kalle Valo wrote: >> vnaralas@codeaurora.org writes: >> >>> On 2020-09-29 13:10, Kalle Valo wrote: >>>> Johannes Berg <johannes@sipsolutions.net> 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
vnaralas@codeaurora.org writes: > On 2020-10-22 13:30, Arend Van Spriel wrote: >> On 10/21/2020 7:19 PM, Kalle Valo wrote: >>> vnaralas@codeaurora.org writes: >>> >>>> On 2020-09-29 13:10, Kalle Valo wrote: >>>>> Johannes Berg <johannes@sipsolutions.net> 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.
On Mon, 2020-11-02 at 21:44 +0200, Kalle Valo wrote: > vnaralas@codeaurora.org writes: > > > On 2020-10-22 13:30, Arend Van Spriel wrote: > > > On 10/21/2020 7:19 PM, Kalle Valo wrote: > > > > vnaralas@codeaurora.org writes: > > > > > > > > > On 2020-09-29 13:10, Kalle Valo wrote: > > > > > > Johannes Berg <johannes@sipsolutions.net> 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
On 2020-11-06 16:11, Johannes Berg wrote: > On Mon, 2020-11-02 at 21:44 +0200, Kalle Valo wrote: >> vnaralas@codeaurora.org writes: >> >> > On 2020-10-22 13:30, Arend Van Spriel wrote: >> > > On 10/21/2020 7:19 PM, Kalle Valo wrote: >> > > > vnaralas@codeaurora.org writes: >> > > > >> > > > > On 2020-09-29 13:10, Kalle Valo wrote: >> > > > > > Johannes Berg <johannes@sipsolutions.net> 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
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..
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
diff --git a/include/uapi/nl80211-vnd-qca.h b/include/uapi/nl80211-vnd-qca.h new file mode 100644 index 0000000..357040a --- /dev/null +++ b/include/uapi/nl80211-vnd-qca.h @@ -0,0 +1,42 @@ +/* SPDX-License-Identifier: ISC */ +/* + * Copyright (c) 2019-2020 The Linux Foundation. All rights reserved. + */ + +#ifndef _UAPI_NL80211_VND_QCA_H +#define _UAPI_NL80211_VND_QCA_H + +/* Vendor id to be used in vendor specific command and events to user space + * NOTE: The authoritative place for definition of QCA_NL80211_VENDOR_ID, + * vendor subcmd definitions prefixed with QCA_NL80211_VENDOR_SUBCMD, and + * qca_wlan_vendor_attr is open source file src/common/qca-vendor.h in + * git://w1.fi/srv/git/hostap.git; the values here are just a copy of that + */ +#define QCA_NL80211_VENDOR_ID 0x001374 + +/** + * enum qca_nl80211_vendor_subcmds - QCA nl80211 vendor command identifiers + * + * @QCA_NL80211_VENDOR_SUBCMD_SET_WIFI_CONFIGURATION: This vendor subcommand is + * used to set wifi configurations by the attributes defined in + * enum qca_wlan_vendor_attr_config. + */ +enum qca_nl80211_vendor_subcmds { + QCA_NL80211_VENDOR_SUBCMD_SET_WIFI_CONFIGURATION = 74, +}; + +/** + * enum qca_wlan_vendor_attr_config: Attributes for data used by + * QCA_NL80211_VENDOR_SUBCMD_SET_WIFI_CONFIGURATION. + * + * @QCA_WLAN_VENDOR_ATTR_CONFIG_GTX: 8-bit unsigned value to trigger + * green Tx power saving 1-Enable, 0-Disable. + */ +enum qca_wlan_vendor_attr_config { + QCA_WLAN_VENDOR_ATTR_CONFIG_GTX = 57, + + QCA_WLAN_VENDOR_ATTR_CONFIG_AFTER_LAST, + QCA_WLAN_VENDOR_ATTR_CONFIG_MAX = + QCA_WLAN_VENDOR_ATTR_CONFIG_AFTER_LAST - 1, +}; +#endif /* _UAPI_NL80211_VND_QCA_H_ */
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. This patch is to add vendor command support to enable/disable ap power save. An example of usage: iw dev wlanx vendor send 0x1374 0x4a ap-ps <val> 0x1374: vendor id 0x4a: vendor subcmd id val: 0 - disable power save 1 - enable power save Signed-off-by: Venkateswara Naralasetty <vnaralas@codeaurora.org> --- v2: * added use case in the commit log. include/uapi/nl80211-vnd-qca.h | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 include/uapi/nl80211-vnd-qca.h