[PATCH] cfg80211: allow to configure dynamic PS timeout

[PATCH] cfg80211: allow to configure dynamic PS timeout

[Stanislaw Gruszka]

Dynamic power save timeout value is suppose to be configurable via
wext, but due to iwconfig bug is not possible to set it using that tool.

Allow to configure PS timeout via nl80211 - add NL80211_ATTR_PS_TIMEOUT
attribute which become timeout stated in ms.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 include/uapi/linux/nl80211.h |  2 ++
 net/wireless/nl80211.c       | 21 +++++++++++++++++----
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 4b28dc0..600d47b 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1990,6 +1990,8 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_SMPS_MODE,
 
+	NL80211_ATTR_PS_TIMEOUT,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index cb9f5a4..39ee3c7 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -7991,7 +7991,7 @@ static int nl80211_set_power_save(struct sk_buff *skb, struct genl_info *info)
 	struct net_device *dev = info->user_ptr[1];
 	u8 ps_state;
 	bool state;
-	int err;
+	int err, ps_timeout;
 
 	if (!info->attrs[NL80211_ATTR_PS_STATE])
 		return -EINVAL;
@@ -8003,17 +8003,27 @@ static int nl80211_set_power_save(struct sk_buff *skb, struct genl_info *info)
 
 	wdev = dev->ieee80211_ptr;
 
+	if (info->attrs[NL80211_ATTR_PS_TIMEOUT]) {
+		ps_timeout = nla_get_s32(info->attrs[NL80211_ATTR_PS_TIMEOUT]);
+		if (ps_timeout <= 0 && ps_timeout != -1)
+			return -EINVAL;
+	} else {
+		ps_timeout = wdev->ps_timeout;
+	}
+
 	if (!rdev->ops->set_power_mgmt)
 		return -EOPNOTSUPP;
 
 	state = (ps_state == NL80211_PS_ENABLED) ? true : false;
 
-	if (state == wdev->ps)
+	if (state == wdev->ps && ps_timeout == wdev->ps_timeout)
 		return 0;
 
-	err = rdev_set_power_mgmt(rdev, dev, state, wdev->ps_timeout);
-	if (!err)
+	err = rdev_set_power_mgmt(rdev, dev, state, ps_timeout);
+	if (!err) {
 		wdev->ps = state;
+		wdev->ps_timeout = ps_timeout;
+	}
 	return err;
 }
 
@@ -8051,6 +8061,9 @@ static int nl80211_get_power_save(struct sk_buff *skb, struct genl_info *info)
 	if (nla_put_u32(msg, NL80211_ATTR_PS_STATE, ps_state))
 		goto nla_put_failure;
 
+	if (nla_put_s32(msg, NL80211_ATTR_PS_TIMEOUT, wdev->ps_timeout))
+		goto nla_put_failure;
+
 	genlmsg_end(msg, hdr);
 	return genlmsg_reply(msg, info);
 
-- 
1.8.3.1

Re: [PATCH] cfg80211: allow to configure dynamic PS timeout

[Arend van Spriel]

On 30-09-14 15:18, Stanislaw Gruszka wrote:
> Dynamic power save timeout value is suppose to be configurable via
> wext, but due to iwconfig bug is not possible to set it using that tool.
> 
> Allow to configure PS timeout via nl80211 - add NL80211_ATTR_PS_TIMEOUT
> attribute which become timeout stated in ms.
> 
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
>  include/uapi/linux/nl80211.h |  2 ++
>  net/wireless/nl80211.c       | 21 +++++++++++++++++----
>  2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
> index 4b28dc0..600d47b 100644
> --- a/include/uapi/linux/nl80211.h
> +++ b/include/uapi/linux/nl80211.h
> @@ -1990,6 +1990,8 @@ enum nl80211_attrs {
>  
>  	NL80211_ATTR_SMPS_MODE,
>  
> +	NL80211_ATTR_PS_TIMEOUT,
> +

Isn't there some doxygen comment where this new attribute should be
described?

Regards,
Arend

Re: [PATCH] cfg80211: allow to configure dynamic PS timeout

[Arend van Spriel]

On 30-09-14 16:04, Arend van Spriel wrote:
> On 30-09-14 15:18, Stanislaw Gruszka wrote:
>> Dynamic power save timeout value is suppose to be configurable via
>> wext, but due to iwconfig bug is not possible to set it using that tool.
>>
>> Allow to configure PS timeout via nl80211 - add NL80211_ATTR_PS_TIMEOUT
>> attribute which become timeout stated in ms.
>>
>> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
>> ---
>>  include/uapi/linux/nl80211.h |  2 ++
>>  net/wireless/nl80211.c       | 21 +++++++++++++++++----
>>  2 files changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
>> index 4b28dc0..600d47b 100644
>> --- a/include/uapi/linux/nl80211.h
>> +++ b/include/uapi/linux/nl80211.h
>> @@ -1990,6 +1990,8 @@ enum nl80211_attrs {
>>  
>>  	NL80211_ATTR_SMPS_MODE,
>>  
>> +	NL80211_ATTR_PS_TIMEOUT,
>> +
> 
> Isn't there some doxygen comment where this new attribute should be
> described?

Uuuhmm, I mean kerneldoc, of course!

> Regards,
> Arend
> 

[PATCH v2] cfg80211: allow to configure dynamic PS timeout

[Stanislaw Gruszka]

Dynamic power save timeout value is suppose to be configurable via
wext, but due to iwconfig bug is not possible to set using that tool.

Allow to configure PS timeout via nl80211 - add NL80211_ATTR_PS_TIMEOUT
attribute which become timeout stated in ms.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
v1 -> v2 : add documentation.

 include/uapi/linux/nl80211.h |  5 +++++
 net/wireless/nl80211.c       | 21 +++++++++++++++++----
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 4b28dc0..66a9ba8 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1638,6 +1638,9 @@ enum nl80211_commands {
  * @NL80211_ATTR_SMPS_MODE: SMPS mode to use (ap mode). see
  *	&enum nl80211_smps_mode.
  *
+ * @NL80211_ATTR_PS_TIMEOUT: Dynamic power save timeout value in miliseconds.
+ *	Special value -1 means driver default.
+ *
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
  */
@@ -1990,6 +1993,8 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_SMPS_MODE,
 
+	NL80211_ATTR_PS_TIMEOUT,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index cb9f5a4..39ee3c7 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -7991,7 +7991,7 @@ static int nl80211_set_power_save(struct sk_buff *skb, struct genl_info *info)
 	struct net_device *dev = info->user_ptr[1];
 	u8 ps_state;
 	bool state;
-	int err;
+	int err, ps_timeout;
 
 	if (!info->attrs[NL80211_ATTR_PS_STATE])
 		return -EINVAL;
@@ -8003,17 +8003,27 @@ static int nl80211_set_power_save(struct sk_buff *skb, struct genl_info *info)
 
 	wdev = dev->ieee80211_ptr;
 
+	if (info->attrs[NL80211_ATTR_PS_TIMEOUT]) {
+		ps_timeout = nla_get_s32(info->attrs[NL80211_ATTR_PS_TIMEOUT]);
+		if (ps_timeout < 0 && ps_timeout != -1)
+			return -EINVAL;
+	} else {
+		ps_timeout = wdev->ps_timeout;
+	}
+
 	if (!rdev->ops->set_power_mgmt)
 		return -EOPNOTSUPP;
 
 	state = (ps_state == NL80211_PS_ENABLED) ? true : false;
 
-	if (state == wdev->ps)
+	if (state == wdev->ps && ps_timeout == wdev->ps_timeout)
 		return 0;
 
-	err = rdev_set_power_mgmt(rdev, dev, state, wdev->ps_timeout);
-	if (!err)
+	err = rdev_set_power_mgmt(rdev, dev, state, ps_timeout);
+	if (!err) {
 		wdev->ps = state;
+		wdev->ps_timeout = ps_timeout;
+	}
 	return err;
 }
 
@@ -8051,6 +8061,9 @@ static int nl80211_get_power_save(struct sk_buff *skb, struct genl_info *info)
 	if (nla_put_u32(msg, NL80211_ATTR_PS_STATE, ps_state))
 		goto nla_put_failure;
 
+	if (nla_put_s32(msg, NL80211_ATTR_PS_TIMEOUT, wdev->ps_timeout))
+		goto nla_put_failure;
+
 	genlmsg_end(msg, hdr);
 	return genlmsg_reply(msg, info);
 
-- 
1.8.3.1

Re: [PATCH v2] cfg80211: allow to configure dynamic PS timeout

[Johannes Berg]

On Wed, 2014-10-01 at 11:27 +0200, Stanislaw Gruszka wrote:
> Dynamic power save timeout value is suppose to be configurable via
> wext, but due to iwconfig bug is not possible to set using that tool.

That's interesting, what's that bug?

> Allow to configure PS timeout via nl80211 - add NL80211_ATTR_PS_TIMEOUT
> attribute which become timeout stated in ms.

Why do you want to be able to set it at all though? I remember having
this discussion years ago, and we said that it wasn't really useful
since the user has no idea when and why this should be changed. I'm not
convinced that changed?

We had to keep the wext for compatibility - maybe that can now be
removed if you say it's broken - but I'm not sure I see much value in
adding it (and you're doing nothing to convince me otherwise, so far)

johannes

Re: [PATCH v2] cfg80211: allow to configure dynamic PS timeout

[Stanislaw Gruszka]

On Mon, Oct 06, 2014 at 05:00:40PM +0200, Johannes Berg wrote:
> On Wed, 2014-10-01 at 11:27 +0200, Stanislaw Gruszka wrote:
> > Dynamic power save timeout value is suppose to be configurable via
> > wext, but due to iwconfig bug is not possible to set using that tool.
> 
> That's interesting, what's that bug?

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=532713

Parsing problem. After reading bug report more detailed I found out it
is fixed on pre beta version v30, but it was not released i.e. on Fedora
we use wireless-tools v29.

> > Allow to configure PS timeout via nl80211 - add NL80211_ATTR_PS_TIMEOUT
> > attribute which become timeout stated in ms.
> 
> Why do you want to be able to set it at all though? I remember having
> this discussion years ago, and we said that it wasn't really useful
> since the user has no idea when and why this should be changed. I'm not
> convinced that changed?
> 
> We had to keep the wext for compatibility - maybe that can now be
> removed if you say it's broken - but I'm not sure I see much value in
> adding it (and you're doing nothing to convince me otherwise, so far)

Zdenek (CCed) reported to me 40% download performance degradation when
PS is used. I think this happen because of delay between packets, but
it is not confirmed yet (I did not provide patches to Zdenek for
testing), hence perhaps problem lies somewere else. I can not reproduce
this issue - I have the same download performance with PS on and off,
I have quite bad performance when set dynamic PS timeout value less
than 20ms, with default 100ms things are fine.

I assume we can provide that setting to user space, if it allow to
fixup performance with PS ?

Zdenek, could you test? You need kernel patch:
http://marc.info/?l=linux-wireless&m=141215578711042&w=2
and two iw patches:
http://marc.info/?l=linux-wireless&m=141208338830615&w=2
http://marc.info/?l=linux-wireless&m=141208338330613&w=2

Thanks
Stanislaw

Re: [PATCH v2] cfg80211: allow to configure dynamic PS timeout

[Johannes Berg]

On Tue, 2014-10-07 at 13:03 +0200, Stanislaw Gruszka wrote:
> On Mon, Oct 06, 2014 at 05:00:40PM +0200, Johannes Berg wrote:
> > On Wed, 2014-10-01 at 11:27 +0200, Stanislaw Gruszka wrote:
> > > Dynamic power save timeout value is suppose to be configurable via
> > > wext, but due to iwconfig bug is not possible to set using that tool.
> > 
> > That's interesting, what's that bug?
> 
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=532713
> 
> Parsing problem. After reading bug report more detailed I found out it
> is fixed on pre beta version v30, but it was not released i.e. on Fedora
> we use wireless-tools v29.

Curious. I have version 30 on Debian, but all of this is *years* old.
Hah.

> > > Allow to configure PS timeout via nl80211 - add NL80211_ATTR_PS_TIMEOUT
> > > attribute which become timeout stated in ms.
> > 
> > Why do you want to be able to set it at all though? I remember having
> > this discussion years ago, and we said that it wasn't really useful
> > since the user has no idea when and why this should be changed. I'm not
> > convinced that changed?
> > 
> > We had to keep the wext for compatibility - maybe that can now be
> > removed if you say it's broken - but I'm not sure I see much value in
> > adding it (and you're doing nothing to convince me otherwise, so far)
> 
> Zdenek (CCed) reported to me 40% download performance degradation when
> PS is used. I think this happen because of delay between packets, but
> it is not confirmed yet (I did not provide patches to Zdenek for
> testing), hence perhaps problem lies somewere else. I can not reproduce
> this issue - I have the same download performance with PS on and off,
> I have quite bad performance when set dynamic PS timeout value less
> than 20ms, with default 100ms things are fine.
> 
> I assume we can provide that setting to user space, if it allow to
> fixup performance with PS ?

I'm not convinced - that'll give you a way to have the bug reporter test
it, or whatever, but that can also be achieved with debugfs or similar,
no?

"Regular" users will never even get there, they'll either give up on
Linux, the machine, the wifi NIC, or live with the low speeds. They'll
never manipulate things here, and this isn't anything that a userspace
tool could automatically manipulate either.

johannes

Re: [PATCH v2] cfg80211: allow to configure dynamic PS timeout

[Krishna Chaitanya]

On Tue, Oct 7, 2014 at 5:35 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Tue, 2014-10-07 at 13:03 +0200, Stanislaw Gruszka wrote:
> > On Mon, Oct 06, 2014 at 05:00:40PM +0200, Johannes Berg wrote:
> > > On Wed, 2014-10-01 at 11:27 +0200, Stanislaw Gruszka wrote:
> > > > Dynamic power save timeout value is suppose to be configurable via
> > > > wext, but due to iwconfig bug is not possible to set using that tool.
> > >
> > > That's interesting, what's that bug?
> >
> > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=532713
> >
> > Parsing problem. After reading bug report more detailed I found out it
> > is fixed on pre beta version v30, but it was not released i.e. on Fedora
> > we use wireless-tools v29.
>
> Curious. I have version 30 on Debian, but all of this is *years* old.
> Hah.
>
> > > > Allow to configure PS timeout via nl80211 - add NL80211_ATTR_PS_TIMEOUT
> > > > attribute which become timeout stated in ms.
> > >
> > > Why do you want to be able to set it at all though? I remember having
> > > this discussion years ago, and we said that it wasn't really useful
> > > since the user has no idea when and why this should be changed. I'm not
> > > convinced that changed?
> > >
> > > We had to keep the wext for compatibility - maybe that can now be
> > > removed if you say it's broken - but I'm not sure I see much value in
> > > adding it (and you're doing nothing to convince me otherwise, so far)
> >
> > Zdenek (CCed) reported to me 40% download performance degradation when
> > PS is used. I think this happen because of delay between packets, but
> > it is not confirmed yet (I did not provide patches to Zdenek for
> > testing), hence perhaps problem lies somewere else. I can not reproduce
> > this issue - I have the same download performance with PS on and off,
> > I have quite bad performance when set dynamic PS timeout value less
> > than 20ms, with default 100ms things are fine.
> >
> > I assume we can provide that setting to user space, if it allow to
> > fixup performance with PS ?
>
> I'm not convinced - that'll give you a way to have the bug reporter test
> it, or whatever, but that can also be achieved with debugfs or similar,
> no?
>
> "Regular" users will never even get there, they'll either give up on
> Linux, the machine, the wifi NIC, or live with the low speeds. They'll
> never manipulate things here, and this isn't anything that a userspace
> tool could automatically manipulate either.

iw/iwconfig tools are not only used by regular users but also used by
developers/
engineers. We use it heavily for all our wifi testing, and when
debugging performance
related issue having a configurable ps timeout is of value. We had to
hack the kernel
to change that value (did not had enough time to work on iw/iwconfig).

So IMHO we need to have this configurable either through iw/mac80211 debugfs.

Re: [PATCH v2] cfg80211: allow to configure dynamic PS timeout

[Johannes Berg]

On Tue, 2014-10-07 at 18:02 +0530, Krishna Chaitanya wrote:

> iw/iwconfig tools are not only used by regular users but also used by
> developers/
> engineers. We use it heavily for all our wifi testing, and when
> debugging performance
> related issue having a configurable ps timeout is of value. We had to
> hack the kernel
> to change that value (did not had enough time to work on iw/iwconfig).
> 
> So IMHO we need to have this configurable either through iw/mac80211 debugfs.

I'm not arguing that it shouldn't be there - I'm arguing it's more a
debug thing than a real operation thing.

johannes