* [PATCH] cfg80211: allow to configure dynamic PS timeout
@ 2014-09-30 13:18 Stanislaw Gruszka
2014-09-30 14:04 ` Arend van Spriel
0 siblings, 1 reply; 9+ messages in thread
From: Stanislaw Gruszka @ 2014-09-30 13:18 UTC (permalink / raw)
To: linux-wireless; +Cc: Johannes Berg
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] cfg80211: allow to configure dynamic PS timeout
2014-09-30 13:18 [PATCH] cfg80211: allow to configure dynamic PS timeout Stanislaw Gruszka
@ 2014-09-30 14:04 ` Arend van Spriel
2014-09-30 14:05 ` Arend van Spriel
0 siblings, 1 reply; 9+ messages in thread
From: Arend van Spriel @ 2014-09-30 14:04 UTC (permalink / raw)
To: Stanislaw Gruszka, linux-wireless; +Cc: Johannes Berg
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] cfg80211: allow to configure dynamic PS timeout
2014-09-30 14:04 ` Arend van Spriel
@ 2014-09-30 14:05 ` Arend van Spriel
2014-10-01 9:27 ` [PATCH v2] " Stanislaw Gruszka
0 siblings, 1 reply; 9+ messages in thread
From: Arend van Spriel @ 2014-09-30 14:05 UTC (permalink / raw)
To: Stanislaw Gruszka, linux-wireless; +Cc: Johannes Berg
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v2] cfg80211: allow to configure dynamic PS timeout
2014-09-30 14:05 ` Arend van Spriel
@ 2014-10-01 9:27 ` Stanislaw Gruszka
2014-10-06 15:00 ` Johannes Berg
0 siblings, 1 reply; 9+ messages in thread
From: Stanislaw Gruszka @ 2014-10-01 9:27 UTC (permalink / raw)
To: Arend van Spriel; +Cc: linux-wireless, Johannes Berg
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2] cfg80211: allow to configure dynamic PS timeout
2014-10-01 9:27 ` [PATCH v2] " Stanislaw Gruszka
@ 2014-10-06 15:00 ` Johannes Berg
2014-10-07 11:03 ` Stanislaw Gruszka
0 siblings, 1 reply; 9+ messages in thread
From: Johannes Berg @ 2014-10-06 15:00 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: Arend van Spriel, linux-wireless
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] cfg80211: allow to configure dynamic PS timeout
2014-10-06 15:00 ` Johannes Berg
@ 2014-10-07 11:03 ` Stanislaw Gruszka
2014-10-07 12:05 ` Johannes Berg
0 siblings, 1 reply; 9+ messages in thread
From: Stanislaw Gruszka @ 2014-10-07 11:03 UTC (permalink / raw)
To: Johannes Berg; +Cc: Arend van Spriel, linux-wireless, Zdenek Kabelac
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] cfg80211: allow to configure dynamic PS timeout
2014-10-07 11:03 ` Stanislaw Gruszka
@ 2014-10-07 12:05 ` Johannes Berg
2014-10-07 12:32 ` Krishna Chaitanya
0 siblings, 1 reply; 9+ messages in thread
From: Johannes Berg @ 2014-10-07 12:05 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: Arend van Spriel, linux-wireless, Zdenek Kabelac
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] cfg80211: allow to configure dynamic PS timeout
2014-10-07 12:05 ` Johannes Berg
@ 2014-10-07 12:32 ` Krishna Chaitanya
2014-10-07 12:47 ` Johannes Berg
0 siblings, 1 reply; 9+ messages in thread
From: Krishna Chaitanya @ 2014-10-07 12:32 UTC (permalink / raw)
To: Johannes Berg
Cc: Stanislaw Gruszka, Arend van Spriel, linux-wireless,
Zdenek Kabelac
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] cfg80211: allow to configure dynamic PS timeout
2014-10-07 12:32 ` Krishna Chaitanya
@ 2014-10-07 12:47 ` Johannes Berg
0 siblings, 0 replies; 9+ messages in thread
From: Johannes Berg @ 2014-10-07 12:47 UTC (permalink / raw)
To: Krishna Chaitanya
Cc: Stanislaw Gruszka, Arend van Spriel, linux-wireless,
Zdenek Kabelac
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-10-07 12:47 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-30 13:18 [PATCH] cfg80211: allow to configure dynamic PS timeout Stanislaw Gruszka
2014-09-30 14:04 ` Arend van Spriel
2014-09-30 14:05 ` Arend van Spriel
2014-10-01 9:27 ` [PATCH v2] " Stanislaw Gruszka
2014-10-06 15:00 ` Johannes Berg
2014-10-07 11:03 ` Stanislaw Gruszka
2014-10-07 12:05 ` Johannes Berg
2014-10-07 12:32 ` Krishna Chaitanya
2014-10-07 12:47 ` Johannes Berg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).