2010-02-16 20:10:36

by Kalle Valo

[permalink] [raw]
Subject: [RFC PATCH] nl80211: add power save commands

From: Kalle Valo <[email protected]>

The only missing command from nl80211, which Wireless Extensions had,
was support for power save mode. Add a simple command to make it possible
to enable and disable power save.

I was also planning about extending the interface, for example adding the
timeout value, but after thinking more about this I decided not to do it.
Basically there were three reasons.

Firstly, the parameters for power save are very much hardware dependent.
Trying to find a unified interface which would work with all hardware, and
still make sense to users, will be very difficult.

Secondly, IEEE 802.11 power save implementation in Linux is still in state
of flux. We have a long way to still to go and there is no way to predict
what kind of implementation we will have after two years. And because we
need to support nl80211 interface a long time, practically forever, adding
now parameters to nl80211 might create maintenance problems later on.

Third issue is the users. Power save parameters are mostly used for
debugging, so debugfs is better, more flexible, interface for this.
For example, wpa_supplicant currently doesn't configure anything related
to power save mode. It's better to strive that kernel can automatically
optimise the power save parameters, like with help of pm qos network
and other traffic parameters.

Later on, when we have better understanding of power save, we can extend
this command with more features, if there's a need for that.

Signed-off-by: Kalle Valo <[email protected]>
---
include/linux/nl80211.h | 10 +++
include/net/cfg80211.h | 7 +-
net/wireless/core.c | 16 +++--
net/wireless/nl80211.c | 132 ++++++++++++++++++++++++++++++++++++++++++++
net/wireless/wext-compat.c | 10 ++-
5 files changed, 160 insertions(+), 15 deletions(-)

diff --git a/include/linux/nl80211.h b/include/linux/nl80211.h
index 127a730..1567175 100644
--- a/include/linux/nl80211.h
+++ b/include/linux/nl80211.h
@@ -387,6 +387,9 @@ enum nl80211_commands {

NL80211_CMD_SET_TX_BITRATE_MASK,

+ NL80211_CMD_SET_POWER_SAVE,
+ NL80211_CMD_GET_POWER_SAVE,
+
/* add new commands above here */

/* used to define NL80211_CMD_MAX below */
@@ -798,6 +801,8 @@ enum nl80211_attrs {

NL80211_ATTR_TX_RATES,

+ NL80211_ATTR_PS_STATE,
+
/* add attributes here, update the policy in nl80211.c */

__NL80211_ATTR_AFTER_LAST,
@@ -1534,4 +1539,9 @@ enum nl80211_band {
NL80211_BAND_5GHZ,
};

+enum nl80211_ps_state {
+ NL80211_PS_DISABLED,
+ NL80211_PS_ENABLED,
+};
+
#endif /* __LINUX_NL80211_H */
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 5b3569b..816c8a4 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1144,7 +1144,6 @@ struct cfg80211_ops {
struct net_device *dev,
u64 cookie);

- /* some temporary stuff to finish wext */
int (*set_power_mgmt)(struct wiphy *wiphy, struct net_device *dev,
bool enabled, int timeout);
};
@@ -1478,6 +1477,9 @@ struct wireless_dev {
struct cfg80211_internal_bss *auth_bsses[MAX_AUTH_BSSES];
struct cfg80211_internal_bss *current_bss; /* associated / joined */

+ bool ps;
+ int ps_timeout;
+
#ifdef CONFIG_CFG80211_WEXT
/* wext data */
struct {
@@ -1489,8 +1491,7 @@ struct wireless_dev {
u8 bssid[ETH_ALEN], prev_bssid[ETH_ALEN];
u8 ssid[IEEE80211_MAX_SSID_LEN];
s8 default_key, default_mgmt_key;
- bool ps, prev_bssid_valid;
- int ps_timeout;
+ bool prev_bssid_valid;
} wext;
#endif
};
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 71b6b3a..649afc1 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -695,19 +695,21 @@ static int cfg80211_netdev_notifier_call(struct notifier_block * nb,
wdev->wext.default_key = -1;
wdev->wext.default_mgmt_key = -1;
wdev->wext.connect.auth_type = NL80211_AUTHTYPE_AUTOMATIC;
+#endif
+
if (wdev->wiphy->flags & WIPHY_FLAG_PS_ON_BY_DEFAULT)
- wdev->wext.ps = true;
+ wdev->ps = true;
else
- wdev->wext.ps = false;
- wdev->wext.ps_timeout = 100;
+ wdev->ps = false;
+ wdev->ps_timeout = 100;
if (rdev->ops->set_power_mgmt)
if (rdev->ops->set_power_mgmt(wdev->wiphy, dev,
- wdev->wext.ps,
- wdev->wext.ps_timeout)) {
+ wdev->ps,
+ wdev->ps_timeout)) {
/* assume this means it's off */
- wdev->wext.ps = false;
+ wdev->ps = false;
}
-#endif
+
if (!dev->ethtool_ops)
dev->ethtool_ops = &cfg80211_ethtool_ops;

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 5b79ecf..53dad87 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -145,6 +145,7 @@ static struct nla_policy nl80211_policy[NL80211_ATTR_MAX+1] __read_mostly = {
[NL80211_ATTR_DURATION] = { .type = NLA_U32 },
[NL80211_ATTR_COOKIE] = { .type = NLA_U64 },
[NL80211_ATTR_TX_RATES] = { .type = NLA_NESTED },
+ [NL80211_ATTR_PS_STATE] = { .type = NLA_U8 },
};

/* policy for the attributes */
@@ -4549,6 +4550,125 @@ static int nl80211_set_tx_bitrate_mask(struct sk_buff *skb,
return err;
}

+static int nl80211_set_power_save(struct sk_buff *skb, struct genl_info *info)
+{
+ struct cfg80211_registered_device *rdev;
+ struct wireless_dev *wdev;
+ struct net_device *dev;
+ u8 ps_state;
+ bool state;
+ int err;
+
+ if (!info->attrs[NL80211_ATTR_PS_STATE]) {
+ err = -EINVAL;
+ goto out;
+ }
+
+ ps_state = nla_get_u8(info->attrs[NL80211_ATTR_PS_STATE]);
+
+ if (ps_state != NL80211_PS_DISABLED && ps_state != NL80211_PS_ENABLED) {
+ err = -EINVAL;
+ goto out;
+ }
+
+ rtnl_lock();
+
+ err = get_rdev_dev_by_info_ifindex(info, &rdev, &dev);
+ if (err)
+ goto unlock_rdev;
+
+ wdev = dev->ieee80211_ptr;
+
+ if (!rdev->ops->set_power_mgmt) {
+ err = -EOPNOTSUPP;
+ goto unlock_rdev;
+ }
+
+ state = (ps_state == NL80211_PS_ENABLED) ? true : false;
+
+ if (state == wdev->ps)
+ goto unlock_rdev;
+
+ wdev->ps = state;
+
+ if (rdev->ops->set_power_mgmt(wdev->wiphy, dev, wdev->ps,
+ wdev->ps_timeout))
+ /* assume this means it's off */
+ wdev->ps = false;
+
+unlock_rdev:
+ cfg80211_unlock_rdev(rdev);
+ dev_put(dev);
+ rtnl_unlock();
+
+out:
+ return err;
+}
+
+static int nl80211_get_power_save(struct sk_buff *skb, struct genl_info *info)
+{
+ struct cfg80211_registered_device *rdev;
+ enum nl80211_ps_state ps_state;
+ struct wireless_dev *wdev;
+ struct net_device *dev;
+ struct sk_buff *msg;
+ void *hdr;
+ int err;
+
+ rtnl_lock();
+
+ err = get_rdev_dev_by_info_ifindex(info, &rdev, &dev);
+ if (err)
+ goto unlock_rtnl;
+
+ wdev = dev->ieee80211_ptr;
+
+ if (!rdev->ops->set_power_mgmt) {
+ err = -EOPNOTSUPP;
+ goto out;
+ }
+
+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+ if (!msg) {
+ err = -ENOMEM;
+ goto out;
+ }
+
+ hdr = nl80211hdr_put(msg, info->snd_pid, info->snd_seq, 0,
+ NL80211_CMD_GET_POWER_SAVE);
+ if (!hdr) {
+ err = -ENOMEM;
+ goto free_msg;
+ }
+
+ if (wdev->ps)
+ ps_state = NL80211_PS_ENABLED;
+ else
+ ps_state = NL80211_PS_DISABLED;
+
+ NLA_PUT_U8(msg, NL80211_ATTR_PS_STATE, ps_state);
+
+ genlmsg_end(msg, hdr);
+ err = genlmsg_reply(msg, info);
+ goto out;
+
+nla_put_failure:
+ err = -ENOBUFS;
+
+free_msg:
+ nlmsg_free(msg);
+
+out:
+ cfg80211_unlock_rdev(rdev);
+ dev_put(dev);
+
+unlock_rtnl:
+ rtnl_unlock();
+
+ return err;
+}
+
+
static struct genl_ops nl80211_ops[] = {
{
.cmd = NL80211_CMD_GET_WIPHY,
@@ -4829,6 +4949,18 @@ static struct genl_ops nl80211_ops[] = {
.policy = nl80211_policy,
.flags = GENL_ADMIN_PERM,
},
+ {
+ .cmd = NL80211_CMD_SET_POWER_SAVE,
+ .doit = nl80211_set_power_save,
+ .policy = nl80211_policy,
+ .flags = GENL_ADMIN_PERM,
+ },
+ {
+ .cmd = NL80211_CMD_GET_POWER_SAVE,
+ .doit = nl80211_get_power_save,
+ .policy = nl80211_policy,
+ /* can be retrieved by unprivileged users */
+ },
};

static struct genl_multicast_group nl80211_mlme_mcgrp = {
diff --git a/net/wireless/wext-compat.c b/net/wireless/wext-compat.c
index b17eeae..9ab5183 100644
--- a/net/wireless/wext-compat.c
+++ b/net/wireless/wext-compat.c
@@ -1099,8 +1099,8 @@ int cfg80211_wext_siwpower(struct net_device *dev,
{
struct wireless_dev *wdev = dev->ieee80211_ptr;
struct cfg80211_registered_device *rdev = wiphy_to_dev(wdev->wiphy);
- bool ps = wdev->wext.ps;
- int timeout = wdev->wext.ps_timeout;
+ bool ps = wdev->ps;
+ int timeout = wdev->ps_timeout;
int err;

if (wdev->iftype != NL80211_IFTYPE_STATION)
@@ -1133,8 +1133,8 @@ int cfg80211_wext_siwpower(struct net_device *dev,
if (err)
return err;

- wdev->wext.ps = ps;
- wdev->wext.ps_timeout = timeout;
+ wdev->ps = ps;
+ wdev->ps_timeout = timeout;

return 0;

@@ -1147,7 +1147,7 @@ int cfg80211_wext_giwpower(struct net_device *dev,
{
struct wireless_dev *wdev = dev->ieee80211_ptr;

- wrq->disabled = !wdev->wext.ps;
+ wrq->disabled = !wdev->ps;

return 0;
}



2010-02-17 07:38:44

by Holger Schurig

[permalink] [raw]
Subject: Re: [RFC PATCH] nl80211: add power save commands

> The only missing command from nl80211, which Wireless Extensions had,

"iwconfig XXX modulation" ?

2010-02-17 13:36:39

by Johannes Berg

[permalink] [raw]
Subject: Re: [RFC PATCH] nl80211: add power save commands

On Wed, 2010-02-17 at 14:27 +0100, Johannes Berg wrote:
> On Wed, 2010-02-17 at 14:21 +0100, Holger Schurig wrote:
> > > But if you wish you can emulate that with rate mask settings.
> > > Not in cfg80211 though please.
> >
> > No, modulaton != rates.
> >
> > AFAIK you can run 1 MB/s with both CCK (802.11b) and with OFDM
> > (802.11g), but they generate different wave-forms.
>
> No.

Well I should qualify. 1 Mbit isn't CCK, it's DSSS. 5.5 and 11 are CCK,
or optionally PBCC, but I've seen exactly one AP that might support this
(indicated by supporting 22 Mbit, but not 33).

In practice, the only supported rates at least in mac80211 right now are

DSSS: 1 2
CCK: 5.5 11
OFDM: 6 9 ...

The supported preambles would be ERP and DSSS on 2.4 GHz, and OFDM (does
that preamble even have a specific name?) on 5 GHz, I think.

johannes


Attachments:
signature.asc (801.00 B)
This is a digitally signed message part

2010-02-17 14:44:12

by Johannes Berg

[permalink] [raw]
Subject: Re: [RFC PATCH] nl80211: add power save commands

On Wed, 2010-02-17 at 14:56 +0100, Holger Schurig wrote:

> Now that you told me so, I was even able to find a reference for this,
> table 4-5 in
>
> http://books.google.com/books?id=TOuyW0bUoncC&pg=PA189

Cool. I dug up some of it from the spec ;)

But maybe we need to go back to your statement about range ...?

johannes


Attachments:
signature.asc (801.00 B)
This is a digitally signed message part

2010-02-17 14:00:13

by Holger Schurig

[permalink] [raw]
Subject: Re: [RFC PATCH] nl80211: add power save commands

I stand corrected.

Now that you told me so, I was even able to find a reference for this, table 4-5 in

http://books.google.com/books?id=TOuyW0bUoncC&pg=PA189

--
http://www.holgerschurig.de

2010-02-17 09:00:43

by Kalle Valo

[permalink] [raw]
Subject: Re: [RFC PATCH] nl80211: add power save commands

Holger Schurig <[email protected]> writes:

>> The only missing command from nl80211, which Wireless Extensions had,
>
> "iwconfig XXX modulation" ?

Heh, I didn't even know about that command. I'll add "usable command"
to the commit log :)

--
Kalle Valo

2010-02-17 13:27:35

by Johannes Berg

[permalink] [raw]
Subject: Re: [RFC PATCH] nl80211: add power save commands

On Wed, 2010-02-17 at 14:21 +0100, Holger Schurig wrote:
> > But if you wish you can emulate that with rate mask settings.
> > Not in cfg80211 though please.
>
> No, modulaton != rates.
>
> AFAIK you can run 1 MB/s with both CCK (802.11b) and with OFDM
> (802.11g), but they generate different wave-forms.

No.

johannes


Attachments:
signature.asc (801.00 B)
This is a digitally signed message part

2010-02-17 08:33:39

by Johannes Berg

[permalink] [raw]
Subject: Re: [RFC PATCH] nl80211: add power save commands

On Wed, 2010-02-17 at 08:34 +0100, Holger Schurig wrote:
> > The only missing command from nl80211, which Wireless Extensions
> had,
>
> "iwconfig XXX modulation" ?

LOL!

But if you wish you can emulate that with rate mask settings. Not in
cfg80211 though please.

johannes


Attachments:
signature.asc (801.00 B)
This is a digitally signed message part

2010-02-16 20:13:22

by Kalle Valo

[permalink] [raw]
Subject: [RFC PATCH 1/2] iw: update nl80211.h to include power save commands


---
nl80211.h | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/nl80211.h b/nl80211.h
index 127a730..1567175 100644
--- a/nl80211.h
+++ b/nl80211.h
@@ -387,6 +387,9 @@ enum nl80211_commands {

NL80211_CMD_SET_TX_BITRATE_MASK,

+ NL80211_CMD_SET_POWER_SAVE,
+ NL80211_CMD_GET_POWER_SAVE,
+
/* add new commands above here */

/* used to define NL80211_CMD_MAX below */
@@ -798,6 +801,8 @@ enum nl80211_attrs {

NL80211_ATTR_TX_RATES,

+ NL80211_ATTR_PS_STATE,
+
/* add attributes here, update the policy in nl80211.c */

__NL80211_ATTR_AFTER_LAST,
@@ -1534,4 +1539,9 @@ enum nl80211_band {
NL80211_BAND_5GHZ,
};

+enum nl80211_ps_state {
+ NL80211_PS_DISABLED,
+ NL80211_PS_ENABLED,
+};
+
#endif /* __LINUX_NL80211_H */


2010-02-16 20:13:31

by Kalle Valo

[permalink] [raw]
Subject: [RFC PATCH 2/2] iw: add set/get power_save commands


---
Makefile | 2 +
ps.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 84 insertions(+), 1 deletions(-)
create mode 100644 ps.c

diff --git a/Makefile b/Makefile
index bae1bfb..b0a4278 100644
--- a/Makefile
+++ b/Makefile
@@ -17,7 +17,7 @@ CFLAGS += -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing
OBJS = iw.o genl.o event.o info.o phy.o \
interface.o ibss.o station.o survey.o util.o \
mesh.o mpath.o scan.o reg.o version.o \
- reason.o status.o connect.o link.o offch.o
+ reason.o status.o connect.o link.o offch.o ps.o
OBJS += sections.o
ALL = iw

diff --git a/ps.c b/ps.c
new file mode 100644
index 0000000..9b0614c
--- /dev/null
+++ b/ps.c
@@ -0,0 +1,83 @@
+#include <errno.h>
+#include <string.h>
+
+#include <netlink/genl/genl.h>
+#include <netlink/msg.h>
+#include <netlink/attr.h>
+
+#include "nl80211.h"
+#include "iw.h"
+
+static int set_power_save(struct nl80211_state *state,
+ struct nl_cb *cb,
+ struct nl_msg *msg,
+ int argc, char **argv)
+{
+ enum nl80211_ps_state ps_state;
+
+ if (argc != 1) {
+ printf("Invalid parameters!\n");
+ return 2;
+ }
+
+ if (strcmp(argv[0], "on") == 0)
+ ps_state = NL80211_PS_ENABLED;
+ else if (strcmp(argv[0], "off") == 0)
+ ps_state = NL80211_PS_DISABLED;
+ else {
+ printf("Invalid parameter: %s\n", argv[0]);
+ return 2;
+ }
+
+ NLA_PUT_U8(msg, NL80211_ATTR_PS_STATE, ps_state);
+
+ return 0;
+
+ nla_put_failure:
+ return -ENOBUFS;
+}
+
+COMMAND(set, power_save, "<on|off>",
+ NL80211_CMD_SET_POWER_SAVE, 0, CIB_NETDEV, set_power_save,
+ "Set power save state to on or off.");
+
+static int print_power_save_handler(struct nl_msg *msg, void *arg)
+{
+ struct nlattr *attrs[NL80211_ATTR_MAX + 1];
+ struct genlmsghdr *gnlh = nlmsg_data(nlmsg_hdr(msg));
+ const char *s;
+
+ nla_parse(attrs, NL80211_ATTR_MAX, genlmsg_attrdata(gnlh, 0),
+ genlmsg_attrlen(gnlh, 0), NULL);
+
+ if (!attrs[NL80211_ATTR_PS_STATE])
+ return NL_SKIP;
+
+ switch (nla_get_u8(attrs[NL80211_ATTR_PS_STATE])) {
+ case NL80211_PS_ENABLED:
+ s = "on";
+ break;
+ case NL80211_PS_DISABLED:
+ default:
+ s = "off";
+ break;
+ }
+
+ printf("Power save: %s\n", s);
+
+ return NL_SKIP;
+}
+
+static int get_power_save(struct nl80211_state *state,
+ struct nl_cb *cb,
+ struct nl_msg *msg,
+ int argc, char **argv)
+{
+ nl_cb_set(cb, NL_CB_VALID, NL_CB_CUSTOM,
+ print_power_save_handler, NULL);
+ return 0;
+}
+
+COMMAND(get, power_save, "<param>",
+ NL80211_CMD_GET_POWER_SAVE, 0, CIB_NETDEV, get_power_save,
+ "Retrieve power save state.");


2010-02-17 09:07:02

by Kalle Valo

[permalink] [raw]
Subject: Re: [RFC PATCH] nl80211: add power save commands

Johannes Berg <[email protected]> writes:

> On Tue, 2010-02-16 at 22:10 +0200, Kalle Valo wrote:
>
>> [NL80211_ATTR_COOKIE] = { .type = NLA_U64 },
>> [NL80211_ATTR_TX_RATES] = { .type = NLA_NESTED },
>> + [NL80211_ATTR_PS_STATE] = { .type = NLA_U8 },
>> };
>
> Looks good to me, though typically we use NLA_U32 for enums. Should we
> deviate from that because we won't ever extend this anyway, or should we
> use NLA_U32 to be more consistent?

Good point. In my opinion we should definitely use u32 for
consistency, there's no real reason to use u8 here. If it's ok for
you, I'll send v2 later today which uses u32.

Thank you for the review.

--
Kalle Valo

2010-02-17 13:25:34

by Holger Schurig

[permalink] [raw]
Subject: Re: [RFC PATCH] nl80211: add power save commands

> But if you wish you can emulate that with rate mask settings.
> Not in cfg80211 though please.

No, modulaton != rates.

AFAIK you can run 1 MB/s with both CCK (802.11b) and with OFDM
(802.11g), but they generate different wave-forms.


With madwifi cards I had circumstances where the absolute range
of CCK was higher than that of OFDM, even when limiting to 1, 2,
5.5, 11 MB/s. This didn't happen in every storage warehouse, but
it happens. I don't have a clue why, the theory --- as far as I
understand it, which isn't far --- tells me otherwise.

--
http://www.holgerschurig.de

2010-02-16 20:20:19

by Luis R. Rodriguez

[permalink] [raw]
Subject: Re: [RFC PATCH] nl80211: add power save commands

On Tue, Feb 16, 2010 at 12:10 PM, Kalle Valo <[email protected]> wrote:
> From: Kalle Valo <[email protected]>
>
> The only missing command from nl80211, which Wireless Extensions had,
> was support for power save mode.

BTW I actually found another which is missing to be complete to wext:

iwconfig wlan0 txpower 17

I guess I should take care of that since I'm the regulatory dork.

> Add a simple command to make it possible
> to enable and disable power save.

Thanks a lot for this.

Luis

2010-02-17 08:47:42

by Johannes Berg

[permalink] [raw]
Subject: Re: [RFC PATCH] nl80211: add power save commands

On Tue, 2010-02-16 at 22:10 +0200, Kalle Valo wrote:

> [NL80211_ATTR_COOKIE] = { .type = NLA_U64 },
> [NL80211_ATTR_TX_RATES] = { .type = NLA_NESTED },
> + [NL80211_ATTR_PS_STATE] = { .type = NLA_U8 },
> };

Looks good to me, though typically we use NLA_U32 for enums. Should we
deviate from that because we won't ever extend this anyway, or should we
use NLA_U32 to be more consistent?

johannes


Attachments:
signature.asc (801.00 B)
This is a digitally signed message part

2010-02-16 20:33:20

by Kalle Valo

[permalink] [raw]
Subject: Re: [RFC PATCH] nl80211: add power save commands

"Luis R. Rodriguez" <[email protected]> writes:

> On Tue, Feb 16, 2010 at 12:10 PM, Kalle Valo <[email protected]> wrote:
>> From: Kalle Valo <[email protected]>
>>
>> The only missing command from nl80211, which Wireless Extensions had,
>> was support for power save mode.
>
> BTW I actually found another which is missing to be complete to wext:
>
> iwconfig wlan0 txpower 17

Oh, good to know. I'll update the commit log to note this.

> I guess I should take care of that since I'm the regulatory dork.

It would be great if you can do that. The faster we can get rid of WE
the better.

--
Kalle Valo